ot_spi_device: Fixes and buffer flip pacing delay adjustment
#291
Conversation
AlexJones0
requested review from
engdoreis,
jwnrt,
rivos-eblot and
ziuziakowska
| * a buffer flip event) then this will not happen, and the timer must exhaust | ||
| * on its own, thus we only set this delay to a small arbitrary value of 10 ms. | ||
| * | ||
| * @todo: A better approach might be to yield to the vCPU every few bytes/words |
There was a problem hiding this comment.
Yes that could be a better alternative; however would need to be carefully measured with several kind of hosts, as it might come with a large overhead, depending on the host.
There was a problem hiding this comment.
I agree - I do not know how common this pattern of guest SW relying on concurrent operation with the SPI Host buffer reads is but it would not be ideal to slow down general SPI Host usage because of it. It would need to be more carefully thought out (hence the todo 😅)
| if (ot_spi_device_is_cs_active(s)) { | ||
| if (!ot_spi_device_is_cs_active(s)) { |
There was a problem hiding this comment.
Now that TPM mode has been implemented there is also the STATUS.tpm_csb bit which is also active low: Hardware Interfaces, so maybe this function should be inlined here and extended to also do that.
There was a problem hiding this comment.
Even though TPM mode is now implemented, the TPM CS itself is not (we only have 1 CS in the protocol currently) - see relevant discussion here.
I think that it's a good idea to make sure this isn't lost though, so I've added a TODO capturing this missing field.
This register is read-only, it should not be writable. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
The SPI Device's `STATUS.csb` field is the "Direct input of CSb signal". The current logic reports a 1 if the CS is active (i.e. we are not idle or in some error state, the SPI bus is active), but /CS is active low and as such it should report a 0 when active, and a 1 when not. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
See the relevant comment - this arbitrary timeout is much longer than expected and can cause unexpected timeouts when using the SPI device for large reads with software that does not expect to handle this event and fill these buffers. When using an icount shift of 6, we can often end up waiting just over a second for the read to resume, greatly slowing down SPI operation. Signed-off-by: Alex Jones <alex.jones@lowrisc.org>
AlexJones0
force-pushed
the
spi_dev_fixes
branch
from
68b6e52 to
0c38169
Compare
4 participants
See the commit messages for more details - fix bugs in
LAST_READ_ADDRregister writability and the chip select status value being reported inverted. Also reduce the arbitrary flipbuf pacing delay so that large SPI transfers do not end up delayed by 1+ second each time (see comment for more details).