lowRISC
[spi_device] Tagging for Upload Command/Address FIFOs and Payload Buffer
Currently, address fifo and the payload do not have any information that SW is able to figure out which command these address and/or payload belong to.
If the host system follows the protocol correctly, it is easy for SW to get. For example, SW can assume that the block erase will always have address field but not payload buffer.
However, if the host system does not follow the protocol, or the SPI is prone to error, it may have chances to receive incomplete commands or redundant data at the end of a SPI transactions. In this case, the matches between the address and command FIFOs will be broken.
The suggestion is to have tags to figure out the connections among the FIFOs and buffer. Easy (from the HW design point of view) revision is to have tag field in command/ address/ and payload that SW can read out.
Cleaner revision is, to update the address to the index same as the command fifo is written. Payload to have tag CSR pointing to the correct cmd FIFO entry. This requires the overhaul of the FIFOs and additional CSR for payload buffer.
BREAKING CHANGE: This will revise the way how to get the address and payload for the upload commands. SW and DV needs to be revised too.
CC: @alphan
Based on discussions with @alphan looks like SW expects only one command is being pushed to the upload FIFOs until the command is processed. So, matching the address and payload is easy with the current design.
This may lead to reducing the size of Command/Address FIFOs (to 2~4).
Based on discussions with @alphan looks like SW expects only one command is being pushed to the upload FIFOs until the command is processed. So, matching the address and payload is easy with the current design.
Just to be clear, this was for bootstrap in ROM, not sure about other use cases.
@a-will i think in terms of normal, valid commands, this is probably true most of the time. Especially since commands we want to upload / capture are "usually" going to cause BUSY assertion, which should prevent the host from sending more.
I have in the past heard of desires to capture "bad" commands also so the device can figure out what's going on, but I think Alex will have a better sense whether that's an actually useful scenario.
Now that we have hardware handling WREN / WRDI, we cover the common use cases with the capability to handle just one uploaded command. Any operation on the arrays causes BUSY to assert, and flash parts do not decode most commands while BUSY.
The program/erase suspend commands don't fit into that set, but I suspect we don't intend to support them. The two-command soft reset sequence also doesn't set BUSY for the first command.
Capturing "bad" commands seems nice, but it's also not essential for the part's operation, I think. But how could it be used in practice? Diagnostics? Force reset the host because it seems suspicious? I'm not sure, myself.
Triaged for spi_device. Assigning to M2.5 with https://github.com/lowRISC/opentitan/labels/Priority%3AP2 because even though this is about robustness in an environment with errors, we should check with the product team if the behavior of spi_device is acceptable. If yes, we can defer this to https://github.com/lowRISC/opentitan/labels/Type%3AFutureRelease.
Reviewing this issue to estimate effort, I'm unsure if the original issue is still valid after the conclusion of #11871 (e.g #12614 was merged which seems to to be relevant), or if the question of the FIFO's becoming out-of-sync due to a misbehaving host is still open and relevant. @eunchan @a-will Could you help me understand the state of this issue? Thanks
This likely requires a substantial change of the spi_device architecture, hence I would recommend not to do this for earlgrey Prod.
Agreed here.
If anything, I'd recommend eliminating the FIFO altogether and only accepting a single command (perhaps with a tag of the WEL bit status at the time the command was uploaded).
For behaving like a SPI flash, uploaded commands are intended to work in conjunction with the WIP bit, so it is impossible to queue up multiple commands at a time. Any command that does not immediately get serviced uses the WIP bit to indicate when it completes. By contrast, any command that must be serviced immediately cannot be processed by the high-latency firmware, so the spi_device IP would need to handle it directly, without uploading.
Given the target application, this buffering architecture seems unnecessarily complex.