opentitan icon indicating copy to clipboard operation
opentitan copied to clipboard

[spi_device] Late CSb de-assertion for op-complete commands

Open eunchan opened this issue 3 years ago • 7 comments

The current SPI_DEVICE HWIP has a few limitation of detecting the host system's misbehaviors:

  • If host de-asserts CSb prior to the full 8 bit opcode, HW IP silently drops
  • If host de-assrets CSb later than 8th beat, but the received opcode expects op-complete command (such as chip erase), it is expected to discard the command. The current version of SPI_DEVICE, if host terminates the command at 9th beat ~15th beat, the HW uploads the opcode but does not report any errors.

CC: @tjaychen @a-will @weicaiyang @jeoongp

eunchan avatar Oct 14 '22 22:10 eunchan

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.

andreaskurth avatar Feb 23 '23 16:02 andreaskurth

  • If host de-asserts CSb prior to the full 8 bit opcode, HW IP silently drops

From the V2 review https://github.com/lowRISC/opentitan/pull/15539, it sounds like https://github.com/lowRISC/opentitan/pull/15637 was merged to check the block does not lock up if CSB de-asserts in bits 0-7.

  • If host de-assrets CSb later than 8th beat, but the received opcode expects op-complete command (such as chip erase), it is expected to discard the command. The current version of SPI_DEVICE, if host terminates the command at 9th beat ~15th beat, the HW uploads the opcode but does not report any errors.

This condition appears to be currently untested, follow up to check priority of a fix + test of the expected discard behaviour.

estimate 8 remaining 2023-03-22 8

hcallahan-lowrisc avatar Mar 22 '23 18:03 hcallahan-lowrisc

From the V2 review https://github.com/lowRISC/opentitan/issues/15539, it sounds like https://github.com/lowRISC/opentitan/pull/15637 was merged to check the block does not lock up if CSB de-asserts in bits 0-7.

Caution: Since PR #15770, this test has been weakened to https://github.com/lowRISC/opentitan/blob/99d8423fdf0babb4c8032e48cdf210b8056c1abc/hw/dv/sv/spi_agent/spi_host_driver.sv?plain=1#L95-L99

due to the known limitation in #15721

andreaskurth avatar Mar 22 '23 20:03 andreaskurth

Note that there is a number of open issues about CSB behaviour and robustness to errors, so some estimated effort (if deemed necessary) may overlap between those issues. (https://github.com/lowRISC/opentitan/issues/10058 https://github.com/lowRISC/opentitan/issues/12747 https://github.com/lowRISC/opentitan/issues/15517 https://github.com/lowRISC/opentitan/issues/15721)

hcallahan-lowrisc avatar Mar 23 '23 15:03 hcallahan-lowrisc

Postponed for production integration.

moidx avatar Apr 05 '23 14:04 moidx

I don't think the host aborting commands needs to be reported to SW, but it might be good to upload only commands that are a valid length.

a-will avatar Jan 31 '24 19:01 a-will

Agreed on that first point. The second point would be nice to have, but ok to implement in a future revision because it is technically not required from a functionality behavior (this functionality would be detecting out of spec behavior).

msfschaffner avatar Feb 22 '24 18:02 msfschaffner