Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SPI Engine: add control for SDO idle state #1320

Merged
merged 12 commits into from
Jul 17, 2024
Merged

SPI Engine: add control for SDO idle state #1320

merged 12 commits into from
Jul 17, 2024

Conversation

LBFFilho
Copy link
Contributor

@LBFFilho LBFFilho commented May 3, 2024

Makes bit [3] of the SPI Configuration Register select the value of SDO when CS is inactive or during read-only transfers.

This is useful for parts such as the AD4000 series, where we want to keep the ADC's SDI line high during conversion, but still want to be able to write to the ADC SPI registers when needed.

PR Type

  • Bug fix (change that fixes an issue)
  • New feature (change that adds new functionality)
  • Breaking change (has dependencies in other repos or will cause CI to fail)

PR Checklist

  • I have followed the code style guidelines
  • I have performed a self-review of changes
  • I have compiled all hdl projects and libraries affected by this PR
  • I have tested in hardware affected projects, at least on relevant boards
  • I have commented my code, at least hard-to-understand parts
  • I have signed off all commits from this PR
  • I have updated the documentation (wiki pages, ReadMe files, Copyright etc)
  • I have not introduced new Warnings/Critical Warnings on compilation
  • I have added new hdl testbenches or updated existing ones

@machschmitt
Copy link

Hi,

I tested this and the current HDL version works for AD4000 series.
Built pulsar_adc HDL project for CoraZ7 and tested with ADAQ4003.
Despite working nicely for AD4000 series, the SDO idle state for when CS is inactive is currently dependent on the tx buffer of the previous transfer. If the last SDO state for the previous transfer is low, SDO is left low. Again, this is working for AD4000 so I'm happy with it. Though, maybe it's better to adjust it now (if not hard to do so) or document the actual behavior.

@machschmitt
Copy link

machschmitt commented May 31, 2024

Vivado hw monitor captures.
If SPI-Engine SDO idle high configured, SDO should be high when CS is inactive to follow AD4000 specifications. The following worked but could be different.
sdo_state_when_cs_off1
The following is ok, but sharing here in case it helps find out how to fix the previous print.
sdo_state_when_cs_off2
This is also okay and yet another example of how sometimes things seem to work by accident with SPI-Engine.
sdo_state_when_cs_off3

@LBFFilho
Copy link
Contributor Author

LBFFilho commented Jun 3, 2024

Hi @machschmitt,

What's happened is that I had introduced a SPI Engine parameter in the last commit to control whether this feature is available on the hdl at compile time, but didn't add it to the pulsar_adc project, so it didn't happen correctly. Can you try with this new commit?

To activate the new feature, you have to build the hdl with:

make USE_SDO_IDLE_STATE=1

@dlech
Copy link
Collaborator

dlech commented Jun 6, 2024

Does this actually need to be a compile option? It seems like it would be OK to always enable this and have the SDO line(s) go low when there is no TX data unless the new bit is set.

I think usually it is expected for this line to be low when there is no TX data (i.e. a read-only transfer) on other SPI controllers.

@LBFFilho
Copy link
Contributor Author

LBFFilho commented Jun 11, 2024

I've changed it so that by default the feature is active. If testing on other projects shows they are not affected, the parameter will be completely removed before merging.

@LBFFilho LBFFilho marked this pull request as ready for review June 18, 2024 18:27
@machschmitt
Copy link

Hi, so for the record, the provided HDL with MOSI idle configuration on SPI-Engine proved to work on the tests I made yesterday.
The MOSI line is now brought high if write transfers end with 0/low.
With this feature, it was possible to both configure and sample ADAQ4003.

Screenshot from 2024-06-18 15-03-38

Screenshot from 2024-06-18 15-04-22

Screenshot from 2024-06-18 15-05-08

Screenshot from 2024-06-18 15-05-19

Screenshot from 2024-06-18 14-41-44

Copy link

@machschmitt machschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test results showed this version to be good by exhibiting expected SPI line signals on vivado HW monitor and by consistently carrying correct data up to software layers.

Makes bit [3] of the SPI Configuration Register select the value
of SDO when CS is inactive or during read-only transfers.

Signed-off-by: Laez Barbosa <[email protected]>
Remove deleted parameter, fix spi engine version

Signed-off-by: Laez Barbosa <[email protected]>
@sarpadi
Copy link
Contributor

sarpadi commented Jul 4, 2024

Tested in hardware AD4630 and CN0561

Signed-off-by: Laez Barbosa <[email protected]>
@LBFFilho LBFFilho requested a review from sarpadi July 10, 2024 11:48
sarpadi
sarpadi previously approved these changes Jul 10, 2024
Copy link
Contributor

@gastmaier gastmaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please bump core version in the tb repo.
Played around with different values on spi_engine_instr_pkg.sv.
Is this delay between the rising CS edge and the SDO idle necessary?
image

Signed-off-by: Laez Barbosa <[email protected]>
Signed-off-by: Laez Barbosa <[email protected]>
@gastmaier
Copy link
Contributor

@machschmitt can you test the final version on hw?

@machschmitt
Copy link

Project continues to work as expected with the BOOT.BIN @LBFFilho sent me yesterday.
Register access (read/write) work nicely and ADC readings are still good so the current version LGTM.

@LBFFilho LBFFilho merged commit 5e5e3de into main Jul 17, 2024
1 of 3 checks passed
@LBFFilho LBFFilho deleted the spi_idle_state branch July 17, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants