search

lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.58k stars 774 forks source link

[spi_device] SW fails to update addr_4b_en in the middle of sim #15543

Closed weicaiyang closed 9 months ago

weicaiyang commented 2 years ago

Here is the sequence

  1. host sends EN4B to set addr_4b_en = 1
  2. SW programs the CSR to clear addr_4b_en
  3. after host sends another command (not EN4B), addr_4b_en is set to 1 again by HW.
  4. SW reads addr_4b_en and sees an unexpected value.

I know we expect SW to only set the addr_4b_en during initialization. Not sure if we want to allow SW to update it on the fly when host side is idle?

I save a waveform session in this directory. Use this command to open it

cd /mnt/disks/filestores/opentitan-shared/users/weicai/scratch/lvl_intr/spi_device-sim-vcs/2.spi_device_flash_all/latest verdi -ssr Verdi.ses &

cc: @eunchan @tjaychen

jeoongp commented 2 years ago

Here, this error is caused when SW clears addr_4b_en at the right next CSB de-assertion. HW write-protection (#15326) needs at least one more CSB assertion to make sure addr_4b is synchronized from SPI_CLK domain to SYS_CLK domain. After the next CSB assertion, SW can clear it at CSB de-assertion, too.

I am also not sure if this HW requirement is OK. @eunchan @tjaychen to comment.

tjaychen commented 2 years ago

Sorry I'm a bit confused. I thought the software setting for 4b_en was just directly synchronized into the spi clock domain, are we saying that synchronization misses the CSB de-assertion window?

eunchan commented 2 years ago

@jeoongp is right. The logic prevents the SW from updating the addr_4b until the HW update is synced into SW side (and is visible to SPI_CLK domain again). So, the current impl. needs two SPI transactions to fully unlock the SW access.

The issue is that SW does not know the current HW status unless it polls CFG.addr_4b.

Question is:

Should we allow SW to update the addr_4b on-the-fly? It wasn't the assumption at the beginning.

tjaychen commented 2 years ago

i think as long as we can support the initial case Alex talked about.. maybe it's okay to table this to future revisions and move on. It's probably worth documenting this into spec though so the user doesn't think they can change it whenever they like.

eunchan commented 2 years ago

Agreed.

On Oct 18, 2022, at 1:43 PM, tjaychen @.***> wrote:

i think as long as we can support the initial case Alex talked about.. maybe it's okay to table this to future revisions and move on. It's probably worth documenting this into spec though so the user doesn't think they can change it whenever they like.

— Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/15543#issuecomment-1282981617, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJDG3X4HSADMM5JJBEM3RLWD4DVRANCNFSM6AAAAAARHQYC3Q. You are receiving this because you were mentioned.

weicaiyang commented 2 years ago

sounds good, tagged this future release and M3

a-will commented 2 years ago

Just to be clear, these two cases need to work.

A 4-byte read address at the end:

  1. Upstream host issues EN4B.
  2. SW resets upstream host and clears addr4b_en.
  3. Upstream host issues EN4B, then a read (must be 4-byte address).

A 3-byte read address at the end:

  1. Upstream host issues EN4B.
  2. SW resets upstream host and clears addr4b_en.
  3. Upstream host issues a read (must be 3-byte address).
weicaiyang commented 2 years ago

@a-will With this issue, we need to reset the spi_device in the step2 of both cases (especially case 2), so that configuring addr4b happens before receiving any transaction.

  1. SW resets upstream host and clears addr4b_en via resetting the spi_device.
a-will commented 2 years ago

@a-will With this issue, we need to reset the spi_device in the step2 of both cases (especially case 2), so that configuring addr4b happens before receiving any transaction.

  1. SW resets upstream host and clears addr4b_en via resetting the spi_device.

Okay, but what about the flip side?

A 3-byte read address at the end:

  1. Upstream host issues EX4B.
  2. SW resets upstream host and sets addr4b_en.
  3. Upstream host issues EX4B, then a read (must be 3-byte address).

A 4-byte read address at the end:

  1. Upstream host issues EX4B.
  2. SW resets upstream host and sets addr4b_en.
  3. Upstream host issues a read (must be 4-byte address).
weicaiyang commented 2 years ago

@a-will With this issue, we need to reset the spi_device in the step2 of both cases (especially case 2), so that configuring addr4b happens before receiving any transaction.

  1. SW resets upstream host and clears addr4b_en via resetting the spi_device.

Okay, but what about the flip side?

A 3-byte read address at the end:

  1. Upstream host issues EX4B.
  2. SW resets upstream host and sets addr4b_en.
  3. Upstream host issues EX4B, then a read (must be 3-byte address).

A 4-byte read address at the end:

  1. Upstream host issues EX4B.
  2. SW resets upstream host and sets addr4b_en.
  3. Upstream host issues a read (must be 4-byte address).

It's the same. As long as we reset spi_device at step 2, it works. SW can only change addr4b_en before any upstream transaction arrives. Then, upstream can issue any EX/EN4B to configure the address mode.

a-will commented 2 years ago

It's the same. As long as we reset spi_device at step 2, it works. SW can only change addr4b_en before any upstream transaction arrives. Then, upstream can issue any EX/EN4B to configure the address mode.

Okay, thanks for explaining the limits! Now I still think this implementation ought to be improved: SW should not need to reconfigure spi_device for these use cases. ;)

jeoongp commented 1 year ago

@weicaiyang this fix is for future release. Is it a M3 item?

weicaiyang commented 1 year ago

@weicaiyang this fix is for future release. Is it a M3 item?

I'm fine with either. @a-will WDYT?

a-will commented 1 year ago

Let's say that this should be done for M3, but in my mind, it is not a blocker.

The requirement to reset and redo init of spi_device to propagate these address mode changes is not at all ergonomic. However, software that is written to comply with this requirement will still work, even if the requirement goes away in a future release.

jeoongp commented 1 year ago

I am not clear of what fix is needed in this case. As I reviewed this ticket again, two fixes seem to be suggested.

  1. Supporting SW to update the addr_4b on-the-fly which does not work in the current design from @tjaychen
  2. Supporting address mode changes without SW reconfiguration from @a-will

Which is the future release?

weicaiyang commented 1 year ago

Supporting SW to update the addr_4b on-the-fly which does not work in the current design from @tjaychen Supporting address mode changes without SW reconfiguration from @a-will

I think they are the same. The issue we have is that, after receiving some SPI transactions, if SW reconfigures addr_4b without reseting spi_device, it may fail. Hence, the addr_4b SW configuration window is after reset and before the 1st SPI transaction.

jeoongp commented 1 year ago

Ah.. I see. In this failed case, does step 2 clear addr_4b_en after 4 sys_clk cycles? This is a turn-around time from HW update to SW reg update. If SW tries to update different value during this synchronization, it is just dropped by higher priority on HW write.

andreaskurth commented 1 year ago

Triaged for spi_device. Assigning to M2.5 with https://github.com/lowRISC/opentitan/labels/Priority%3AP2 because this seems to be a limitation for which we have a SW workaround, but we should still confirm with the product team that this is okay. If so, we can indeed defer to https://github.com/lowRISC/opentitan/labels/Type%3AFutureRelease.

andreaskurth commented 1 year ago

As discussed in a meeting with the product team last week, the limitation that an address mode change requires SW to reset and reconfigure spi_device should not be a problem because it's part of the init sequence coming back from deep sleep. It's not ergonomic but SW that is written to comply with this limitation will continue to work even if this limitation goes away in a future release, as @a-will pointed out.

We're thus deferring this to https://github.com/lowRISC/opentitan/labels/Type%3AFutureRelease.