`ot_spi_device`,`ot_spi_host`: Bugfix + Refactoring + Passthrough mode implementation #255

ziuziakowska · 2025-10-23T14:10:21Z

This PR implements the Passthrough mode for OT SPI Device and any required changes in OT SPI Host. Passthrough mode allows for external SPI transfers to be passed through to a downstream device, and optionally intercepted or modified by OT SPI Device.

In HW, OT SPI Device and OT SPI Host are linked by their SPI bus and a wire that represents whether the Device is in Passthrough mode. If Passthrough mode is enabled, the SPI Host is entirely bypassed and SPI Device has full control over the bus, which it uses to talk to the same downstream flash as SPI Host. See Documentation reference, RTL.
This passthrough mechanism is only implemented by OT SPI Host if it has one CS line: RTL

The structure of the PR is as follows:

The initial commits consist of refactoring work in ot_spi_device for the Passthrough implementation.
The subsequent commits implement the requirements for Passthrough mode in ot_spi_host - two GPIOs are used to communicate Passthrough enabled and the Chip Select as driven by OT SPI Device, OT SPI Device and OT SPI Host are then linked together.
The final commits contain the implementation of Passthrough mode in ot_spi_device.

I have tried to handle most corner-cases I can think of, but some conflicting configuration options might not behave as expected.

Unimplemented features:

2-cycle read pipeline is not implemented, as QEMU SPI transactions are modelled with byte granularity.
Dummy cycle counts between 1-7 are not supported, for the same reason as above. Any non-zero dummy cycle count will fallback to 8 cycles.

Testing:

This implementation has been tested against spi_passthru_test. See lowRISC/opentitan#28649 for the changes required in OpenTitan. To test manually the steps are:

Create a 32MiB backing storage for the flash: dd if=/dev/zero of=<flash> bs=1M count=32
Add QEMU to the execution environment of spi_passthru_test, with these additional flags: -global ot-earlgrey-board.spiflash0=w25q256 -drive if=mtd,file=<flash>,format=raw,bus=0

docs/opentitan/earlgrey.md

hw/opentitan/ot_spi_device.c

hw/riscv/ot_earlgrey.c

hw/opentitan/ot_spi_device.c

hw/riscv/ot_earlgrey.c

hw/opentitan/ot_spi_device.c

hw/opentitan/trace-events

hw/opentitan/ot_spi_device.c

ziuziakowska · 2025-10-31T16:14:32Z

I have discovered that OT SPI Device is not the only device controlling this SPI bus, the bus is actually shared by OT SPI Host 0 in Earlgrey, and OT SPI Host is bypassed entirely when Passthrough mode is enabled. This seems to only be documented here. As such this PR has been reworked a bit.

rivos-eblot

quick review.

hw/opentitan/ot_spi_host.c

hw/opentitan/ot_spi_device.c

ziuziakowska · 2025-11-05T12:44:53Z

Just missing some checks in OT SPI Host to inhibit its activity when Passthrough on SPI Device is enabled, which should be a very pathological situation. Otherwise ready for review and nits up to now.

AlexJones0

LGTM (with 1 additional comment), thanks for all the hard work on this!

hw/opentitan/ot_spi_device.c

AlexJones0 · 2025-11-07T14:33:16Z

hw/opentitan/ot_spi_device.c

-                      "%s: W/O register 0x%02" HWADDR_PRIx " (%s)\n", __func__,
-                      addr, SPI_REG_NAME(reg));
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: %s: W/O register 0x%02x (%s)\n",
+                      __func__, s->ot_id, (uint32_t)addr, SPI_REG_NAME(reg));


I think this can just be fixed in a separate PR (not an issue introduced by this PR, don't want to add even more work), but this should probably be TPM_REG_NAME (@engdoreis)

rivos-eblot

LGTM, however the 80-column rule is broken in a couple of locations:

Comments can be rewrapped (which implies shortening or splitting a partial URL)
Macro definitions seem to be mis-managed that with clang-format-20, alignment takes precedence over line with (despite the settings), so till it is fixed, the best option is to add another /* clang-format off/on */ block. I spent a dozen of minutes experiencing different settings but they all fail.

Note:
This PR breaks several of our SPI device unit tests, but I would not bet on them and I have no time at the moment to track down the issue(s) and I do not want to delay this PR any longer.
I'll try to run them on Verilator when possible. This means adding support for the Verilator SPI device protocol. I'm not sure how difficult this could be.

AlexJones0 · 2025-11-07T15:28:27Z

however the 80-column rule is broken in a couple of locations

I'm slightly confused by this - the code looks okay to me and is ot-format.sh compliant?

Macro definitions seem to be mis-managed that with clang-format-20, alignment takes precedence over line with (despite the settings), so till it is fixed, the best option is to add another /* clang-format off/on */ block. I spent a dozen of minutes experiencing different settings but they all fail.

Yes, I've seen this before - were there some specific macros that you think @ziuziakowska needs to add clang-format blocks around in this PR?

This PR breaks several of our SPI device unit tests, but I would not bet on them and I have no time at the moment to track down the issue(s) and I do not want to delay this PR any longer.
I'll try to run them on Verilator when possible. This means adding support for the Verilator SPI device protocol. I'm not sure how difficult this could be.

Hmm, that's mildly concerning. For the open source OpenTitan testing, our tests using SPI bootstrapping (e.g. ownership transfer) and the TPM test are all still passing, so it looks good, but it's possible that there's an edge case exercised by your tests that has been missed somewhere (or, like you suggest, the tests might themselves have problems).

rivos-eblot · 2025-11-07T15:43:50Z

I'm slightly confused by this - the code looks okay to me and is ot-format.sh compliant?

I think the new changes are ok, but some of the existing lines were not:

    FIELD(FLASH_STATUS, TB, 5u, 1u) /* beware actual bits depend on emulated dev. */
    SHARED_FIELD(CMD_INFO_DUMMY_SIZE, 12u, 3u) /* limited to bits, ignore in QEMU */
    SHARED_FIELD(CMD_INFO_DUMMY_EN, 15u, 1u) /* only use this bit for dummy cfg */
    SHARED_FIELD(CMD_INFO_PAYLOAD_DIR, 20u, 1u) /* not used in Flash mode (guess) */
    SHARED_FIELD(CMD_INFO_PAYLOAD_SWAP_EN, 21u, 1u) /* not used in Flash mode */
#define SPI_SRAM_CMD_OFFSET        (SPI_SRAM_PAYLOAD_OFFSET + SPI_SRAM_PAYLOAD_SIZE)
 * opentitan.org/book/hw/ip/spi_device/doc/programmers_guide.html#dual-port-sram-layout
#define SPI_SRAM_ADDR_END          (SPI_SRAM_TPM_WRITE_OFFSET + SPI_SRAM_TPM_WRITE_SIZE)

ot-format.sh calls clang-format-20 which itself fails to fix those lines (and destroy any attempt to fix them :-))

Yes, I've seen this before - were there some specific macros that you think @ziuziakowska needs to add clang-format blocks around in this PR?

The block that contains the two #define lines above. For the remaining problematic lines, the comments need to be moved/split somehow.

Hmm, that's mildly concerning. For the open source OpenTitan testing, our tests using SPI bootstrapping (e.g. ownership transfer) and the TPM test are all still passing, so it looks good, but it's possible that there's an edge case exercised by your tests that has been missed somewhere (or, like you suggest, the tests might themselves have problems).

I think that we at least overlooked the default 0x00 value (vs. the default flash value, 0xff). That could be one source of issues. Depending on which commit from this PR I've tested, some other failures triggered, but I do not have time to investigate them right now. I really need to find a way to run those tests against Verilator. It it anyway more likely that the test is wrong rather than the emulation is.

rivos-eblot · 2025-11-07T15:48:27Z

Beware that at least one commit in the PR does not build ([ot] hw/opentitan: ot_spi_device: DEFAULT_TX_VALUE -> DEFAULT_TX_RX_VALUE) which might be an issue for bisecting.

Signed-off-by: Alice Ziuziakowska <[email protected]>

The bit-fields previously used here are just advisory/convention, the whole 22-bit field is RW and software maintained. This is also expected behaviour for spi_passthru_test. Signed-off-by: Alice Ziuziakowska <[email protected]>

The bytes are received over the wire and stored in the buffer most-significant byte first (big-endian), so the dummy byte is the last byte not the first. Signed-off-by: Alice Ziuziakowska <[email protected]>

Expected by spi_passthru_test, see: https://github.com/lowRISC/opentitan/blob/master/sw/host/tests/chip/spi_device/src/spi_passthru.rs#L70-L80 Signed-off-by: Alice Ziuziakowska <[email protected]>

…ALUE Name is more verbose but matches the usage better, as in `ot_spi_host` Signed-off-by: Alice Ziuziakowska <[email protected]>

This commit simplifies command slot definitions and the associated state and control flow. The HW CFG and the HW STA commands have been combined into just HW commands, as in flash mode they are handled in hardware by some way and should therefore share the same data path. For passthrough mode, a subset of these HW commands can be intercepted (all except for WREN and WRDI). As the matched command slot number determines whether the command is a SW or HW command, the `OtSpiFlashCommand` enum has been removed in favour of using `is_sw_command` to return the boolean. Command slot matching has been brought out into its own function, `match_command_slot`, which returns whether an opcode was matched in the command info registers. The decoded slot number is only valid if this returns true. Also a bug is fixed in `ot_spi_device_exec_command` where hardcoded opcodes for read commands were used instead of the opcodes in the associated command slot. Signed-off-by: Alice Ziuziakowska <[email protected]>

Signed-off-by: Alice Ziuziakowska <[email protected]>

... and removes `OtSpiDeviceAddrMode`. `ot_spi_device_get_command_address_size` now returns the size of the address field in the current command, using the value in `cmd_info` and the current 4B enable state. Signed-off-by: Alice Ziuziakowska <[email protected]>

Log the `ot_id`, and remove usages of `HWADDR_PRIx` Signed-off-by: Alice Ziuziakowska <[email protected]>

Signed-off-by: Alice Ziuziakowska <[email protected]>

This implements a passthrough enable and chip select IRQ and a method to interact with the downstream SPI bus, for use by upstream SPI Device. Signed-off-by: Alice Ziuziakowska <[email protected]>

This adds the corresponding out IRQs to OT SPI Device, as well as a property that contains the downstream OT SPI Host. Signed-off-by: Alice Ziuziakowska <[email protected]>

…IRQs Signed-off-by: Alice Ziuziakowska <[email protected]>

This commit implements the Passthrough mode on SPI Device. Passthrough mode allows the device to act as a proxy for a downstream flash device, optionally intercepting commands to send back its own values, filtering commands sent downstream, or transparently translating address and payload bytes. This also "implements" the Disabled mode, which discards all bytes sent to the device. Signed-off-by: Alice Ziuziakowska <[email protected]>

Signed-off-by: Alice Ziuziakowska <[email protected]>

ziuziakowska · 2025-11-07T16:18:06Z

Fixed any failing-to-build commits.

rivos-eblot · 2025-11-07T16:24:12Z

Fixed any failing-to-build commits.

Thanks for the changes and the work on the SPI device.

AlexJones0 · 2025-11-07T17:03:18Z

Failing regression tests on this PR (hmac_functest and sha{256,384,512}_functest) are known failing in upstream OpenTitan earlgrey_1.0.0 and should also be fixed soon (the fix is found and PRed, the PR is not yet merged).

rivos-eblot requested changes Oct 23, 2025

View reviewed changes

ziuziakowska mentioned this pull request Oct 28, 2025

[qemu] Earlgrey 1.0.0 spi_device passthrough suppport lowRISC/opentitan#27929

Open

ziuziakowska force-pushed the spi_device_passthrough branch 2 times, most recently from b7c3b04 to 2e2300b Compare October 28, 2025 13:30

rivos-eblot requested changes Oct 28, 2025

View reviewed changes

ziuziakowska force-pushed the spi_device_passthrough branch 2 times, most recently from 2264c77 to f1a68a2 Compare October 29, 2025 14:57

ziuziakowska requested a review from rivos-eblot October 29, 2025 16:03

rivos-eblot requested changes Oct 30, 2025

View reviewed changes

ziuziakowska force-pushed the spi_device_passthrough branch from f1a68a2 to 6e8f034 Compare October 30, 2025 11:53

ziuziakowska marked this pull request as ready for review October 30, 2025 11:54

ziuziakowska requested review from AlexJones0 and jwnrt October 30, 2025 11:55

ziuziakowska force-pushed the spi_device_passthrough branch 2 times, most recently from e8ef6d7 to ad5c39c Compare October 30, 2025 14:20

ziuziakowska requested a review from rivos-eblot October 30, 2025 15:17

ziuziakowska marked this pull request as draft October 30, 2025 16:51

ziuziakowska changed the title ~~[WIP] ot_spi_device: Refactoring + Passthrough mode implementation~~ [WIP] ot_spi_device,ot_spi_host: Refactoring + Passthrough mode implementation Oct 31, 2025

ziuziakowska force-pushed the spi_device_passthrough branch from ca61a08 to 28ffc3c Compare October 31, 2025 16:09

rivos-eblot requested changes Oct 31, 2025

View reviewed changes

ziuziakowska force-pushed the spi_device_passthrough branch from 28ffc3c to a49e9bf Compare November 3, 2025 14:12

rivos-eblot reviewed Nov 3, 2025

View reviewed changes

hw/opentitan/ot_spi_device.c Outdated Show resolved Hide resolved

hw/opentitan/ot_spi_device.c Outdated Show resolved Hide resolved

ziuziakowska force-pushed the spi_device_passthrough branch from a49e9bf to 3a643ee Compare November 5, 2025 12:01

ziuziakowska changed the title ~~[WIP] ot_spi_device,ot_spi_host: Refactoring + Passthrough mode implementation~~ ot_spi_device,ot_spi_host: Bugfix + Refactoring + Passthrough mode implementation Nov 5, 2025

ziuziakowska force-pushed the spi_device_passthrough branch from 3a643ee to 05aa2a7 Compare November 5, 2025 12:14

ziuziakowska marked this pull request as ready for review November 5, 2025 12:15

ziuziakowska force-pushed the spi_device_passthrough branch from 05aa2a7 to 6fc1a21 Compare November 5, 2025 12:16

ziuziakowska requested a review from rivos-eblot November 5, 2025 12:25

AlexJones0 approved these changes Nov 7, 2025

View reviewed changes

rivos-eblot approved these changes Nov 7, 2025

View reviewed changes

ziuziakowska force-pushed the spi_device_passthrough branch 2 times, most recently from 1d723e3 to d111644 Compare November 7, 2025 15:21

ziuziakowska added 6 commits November 7, 2025 15:57

[ot] hw/opentitan: ot_spi_device: format macros manually

d4eefba

Signed-off-by: Alice Ziuziakowska <[email protected]>

[ot] hw/opentitan: ot_spi_device: remove unused struct

31d3905

Signed-off-by: Alice Ziuziakowska <[email protected]>

[ot] hw/opentitan: ot_spi_device: fix address parsing in read SFDP

30da5a7

The bytes are received over the wire and stored in the buffer most-significant byte first (big-endian), so the dummy byte is the last byte not the first. Signed-off-by: Alice Ziuziakowska <[email protected]>

[ot] hw/opentitan: ot_spi_device: change end of JEDEC ID buffer to 0s

4ca708e

Expected by spi_passthru_test, see: https://github.com/lowRISC/opentitan/blob/master/sw/host/tests/chip/spi_device/src/spi_passthru.rs#L70-L80 Signed-off-by: Alice Ziuziakowska <[email protected]>

[ot] hw/opentitan: ot_spi_device: DEFAULT_TX_VALUE -> DEFAULT_TX_RX_V…

552ce75

…ALUE Name is more verbose but matches the usage better, as in `ot_spi_host` Signed-off-by: Alice Ziuziakowska <[email protected]>

ziuziakowska force-pushed the spi_device_passthrough branch from d111644 to 74767fc Compare November 7, 2025 15:59

ziuziakowska added 10 commits November 7, 2025 16:15

[ot] hw/opentitan: ot_spi_device: add trace event for flash transfer

e732646

Signed-off-by: Alice Ziuziakowska <[email protected]>

[ot] hw/opentitan: ot_spi_device: change logging

9071602

Log the `ot_id`, and remove usages of `HWADDR_PRIx` Signed-off-by: Alice Ziuziakowska <[email protected]>

[ot] hw/opentitan: ot_spi_host: change CS lines into IbexIRQs

5bf30dd

Signed-off-by: Alice Ziuziakowska <[email protected]>

[ot] hw/opentitan: ot_spi_host: SPI Host support for passthrough mode

b02a915

This implements a passthrough enable and chip select IRQ and a method to interact with the downstream SPI bus, for use by upstream SPI Device. Signed-off-by: Alice Ziuziakowska <[email protected]>

[ot] hw/opentitan: ot_spi_device: Add Passthrough IRQs and SPI Host prop

7df6e09

This adds the corresponding out IRQs to OT SPI Device, as well as a property that contains the downstream OT SPI Host. Signed-off-by: Alice Ziuziakowska <[email protected]>

[ot] hw/riscv: ot_earlgrey,ot_darjeeling: connect SPI Device to Host …

293f153

…IRQs Signed-off-by: Alice Ziuziakowska <[email protected]>

[ot] docs/opentitan: ot_spi_device: Update SPI Device documentation

5059dc5

Signed-off-by: Alice Ziuziakowska <[email protected]>

ziuziakowska force-pushed the spi_device_passthrough branch from 74767fc to 5059dc5 Compare November 7, 2025 16:16

AlexJones0 approved these changes Nov 7, 2025

View reviewed changes

AlexJones0 merged commit 3f9e85b into lowRISC:ot-9.2.0 Nov 7, 2025
9 of 10 checks passed

ot_spi_device,ot_spi_host: Bugfix + Refactoring + Passthrough mode implementation #255

ot_spi_device,ot_spi_host: Bugfix + Refactoring + Passthrough mode implementation #255

Uh oh!

Conversation

ziuziakowska commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ziuziakowska commented Oct 31, 2025

Uh oh!

rivos-eblot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ziuziakowska commented Nov 5, 2025

Uh oh!

AlexJones0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexJones0 Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

rivos-eblot left a comment

Choose a reason for hiding this comment

Uh oh!

AlexJones0 commented Nov 7, 2025

Uh oh!

rivos-eblot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rivos-eblot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ziuziakowska commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rivos-eblot commented Nov 7, 2025

Uh oh!

AlexJones0 commented Nov 7, 2025

Uh oh!

Uh oh!

Reviewers

`ot_spi_device`,`ot_spi_host`: Bugfix + Refactoring + Passthrough mode implementation #255

`ot_spi_device`,`ot_spi_host`: Bugfix + Refactoring + Passthrough mode implementation #255

ziuziakowska commented Oct 23, 2025 •

edited

Loading

rivos-eblot commented Nov 7, 2025 •

edited

Loading

rivos-eblot commented Nov 7, 2025 •

edited

Loading

ziuziakowska commented Nov 7, 2025 •

edited

Loading