lowRISC / opentitan

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

[spi_host] Request software/firmware controller chip select #23260

Open jettr opened 1 month ago

jettr commented 1 month ago

Description

The chip select pin for the current spi host implementation does not support an easy way to independently assert or deassert the chip select pin in firmware. Right now, the spi host controls the chips select pin in hardware relative to transaction it is sending out.

It is common for the hardware interface layer for a spi host to expose controlling chip select behavior independently of transactions being sent. Here is an example from pigweed and another example from linux. Our own hil also assumes that chip select can be controlled independently. We may have a few workarounds, but we haven't fully confirmed their viability yet.

It would be beneficial if firmware could control the chip select output via register access, e.g. one bit for (hardware vs software control) and another bit for (software override high vs software override low).

@moidx @kupiakos @cfrantz fyi

a-will commented 1 month ago

It would be beneficial if firmware could control the chip select output via register access, e.g. one bit for (hardware vs software control) and another bit for (software override high vs software override low).

The question here is how is it beneficial? Why does an override need to exist in the IP?

Just because there are APIs doesn't mean that there is a need to support a software override in the IP. I believe those APIs exist primarily for SPI controllers that don't have hardware CSB support (or that need to use a GPIO because their chip-select is not on a hardware-supported CSB pad).

Meanwhile, the IP already gives control of the timing of CSB relative to the transaction. For use cases, what is missing?

a-will commented 1 month ago

Perhaps we would be missing support for JESD252-style resets? Is that a critical use case? And are there others?

jettr commented 1 month ago

Our use case is a USB-SPIHost bridge over the USB debug connection to interact with the SPI bus that is connected to the OT chip. The USB host controls the data flow, and OT doesn't always know how many bytes will be in each logical SPI transaction (which consists of multiple USB packets). There are also clean up paths that need to de-assert chip select without sending a dummy packet either.

kupiakos commented 1 month ago

Just because there are APIs doesn't mean that there is a need to support a software override in the IP. I believe those APIs exist primarily for SPI controllers that don't have hardware CSB support

These APIs exist for SPI controllers that allow software-controlled CSB, whether through a GPIO or through a hardware override. For firmware ported to OpenTitan using APIs like this, it means either refactoring code using those APIs into something explicitly transaction-based to mirror the hardware (and refactoring the drivers all of the other platforms it's implemented for), or leaving the IP-controlled pin unused and reserving a GPIO pin. I'm doing the former, but it does have a code size impact on other platforms that allow software-controlled CSB.

OT doesn't always know how many bytes will be in each logical SPI transaction (which consists of multiple USB packets)

For most, but not all, cases, this bridge can know whether the current receive segment of an overall transaction is the last one.

There are also clean up paths that need to de-assert chip select without sending a dummy packet either.

One issue is that, even with the override, we'd need to ensure that any ongoing transactions in the state machine are cancelled before we can assert chip select again, so IIUC we'd want to do a SW_RST or config change anyways.

a-will commented 1 month ago

Just because there are APIs doesn't mean that there is a need to support a software override in the IP. I believe those APIs exist primarily for SPI controllers that don't have hardware CSB support

These APIs exist for SPI controllers that allow software-controlled CSB, whether through a GPIO or through a hardware override. For firmware ported to OpenTitan using APIs like this, it means either refactoring code using those APIs into something explicitly transaction-based to mirror the hardware (and refactoring the drivers all of the other platforms it's implemented for), or leaving the IP-controlled pin unused and reserving a GPIO pin. I'm doing the former, but it does have a code size impact on other platforms that allow software-controlled CSB.

I'm just saying that the purpose of the SPI abstraction layer is to perform SPI transactions. It's a reversal of the dependency graph to require hardware to support features that can be abused for something that is not SPI transactions ...is what I'd say if JEDEC didn't make a bizarre reset for SPI NOR flash that definitely isn't a SPI transaction, hehe.

a-will commented 1 month ago

The next question is... Are these use cases critical for A1? Or just nice-to-have?

jettr commented 1 month ago

How difficult of a hardware change is this?

To my knowledge, we still haven't found a perfect work around to support our USB/SPI bridge protocol, but we may if we continue experimenting. I wouldn't rate this functionality at critical level, but I would rate it higher than nice-to-have for A1.

@kupiakos any thoughts about how needed this functionality is in A1 silicon, and how viable our current workarounds are in supporting out USB/SPI bridge?

kupiakos commented 1 month ago

This functionality is nice-to-have, and not necessary for A1. The refactors for our USB/SPI bridge are sufficient; they do have a code size impact for non-OpenTitan platforms.

It's a reversal of the dependency graph to require hardware to support features that can be abused for something that is not SPI transactions

I pretty fundamentally disagree here: hardware is designed in order to have firmware run on it for a specific task, and having no way to override chip select strictly reduces the set of generically-designed firmware that can be ported to run on it.

vogelpi commented 1 month ago

We discussed this in the triage meeting and agreed that we cannot support this due to schedule constraints for M4. We'll create a feature request for future releases though.