Add support for {Read,Write}NoAddrIncr SPI operations, leverage them in transceivers
#1768
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cbiffle
approved these changes
May 2, 2024
| same address. These operations are useful for exposing FIFO-like interfaces | ||
| via a single memory-mapped address location. | ||
| */ | ||
|
|
There was a problem hiding this comment.
I feel like the removed comment a couple lines down, explaining why the enum only contains a subset of the operations, was kinda useful. Adding a single line comment to each op enum saying something like
/// This is just reads; for writes, see WriteOp
would help avoid confusing readers, I think.
Also implement their use for transcievers I2C buffers for the QSFP FPGA design.
hawkw
reviewed
May 3, 2024
Aaron-Hartwig
force-pushed
the
aaron/fpga-spi-more-ops
branch
from
e6d3ba6
to
c23f2e2
Compare
|
My latest push rebases this, fixes an FPGA bug, and addresses the review comments. Thank you both very much! I'll be queuing this up for auto-merge. |
This was referenced May 20, 2024
Closed
Aaron-Hartwig
added a commit
that referenced
this pull request
May 20, 2024
#1768 did not properly account for the FIFO behavior of the FPGA's data buffers. The "check the status byte" portion of the loop happened outside the part where we read the buffer, and since the buffer was just memory-mapped registers it could be repeatedly without consequence. Since the data was now in a FIFO, I was inadvertently draining the FIFO before the transaction was done. This PR consolidates the "is I2C done yet" logic into the `get_i2c_status_and_read_buffer` so calling code can just deal with the status register and the data buffer. Fixes #1786
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part 1) Adjustments to
fpga-api/src/lib.rs,fpga-server/src/main.rs, andfpga.idolfor the new SPI operations.As of oxidecomputer/quartz#139 the FPGA's SPI peripheral supports these operations. Prior to this PR the FPGA drivers did not take an operation argument for read operations because there was only a single type of read. Now that there are two (
ReadandReadAddrNoIncr), the intended operation must be specified. Given that up until this pointReadwas the only option (and will probably remain the primary use 99.9% of the time) that has been made the default for thereadoperation which the majority of callers use. To utilize the new read operation, one can just call the underlyingread_bytesfunction which takes aReadOpparameter.Part 2) Making use of the new SPI operations in the
transceiverscodeThis also implements their use for transceivers I2C buffers for the QSFP FPGA design. All the
*_regs.*files are generated from the FPGA build process and are brought in alongside the.bitfile. The transceivers code has been reworked to leverage the SPI refactoring from oxidecomputer/quartz#146. This code utilizes the non-address incrementing SPI operations to read/write to the I2C buffers in the FPGA.