lowRISC / lowRISC/opentitan

[spi_host,doc] Update SPI Host documentation

Open
#26,487 0 comments 0 reactions 0 assignees View on GitHub
Component:Doc IP:spi_host
Dominant language
SystemVerilog
Stars
3.6k
Forks
1.1k
Avg merge
2d 22h
Merged PRs (30d)
141

Description

### Description

While reading the SPI Host documentation and comparing it with Verilator-generated waves, it appeared that the documentation does not reflect the behavior of the current implementation and/or is not precise enough to fully understand how the HW handles corner cases. I'm not familiar enough with HW implementation so my understanding may be incomplete or wrong:

1. In https://opentitan.org/book/hw/ip/spi_host/doc/theory_of_operation.html#special-command-fields
> "_The command is complete when [STATUS.ACTIVE](https://opentitan.org/book/hw/ip/spi_host/doc/registers.html#status) goes low._"

As commands are pushed by the FW into a command FIFO, and commands popped out and executed by the HW, it seems the `STATUS.active` bit goes high only once ALL pushed commands have been executed (or discarded on error?). It does not seem the `STATUS.active` bit goes low after _each_ command is executed, only once all of them are. The doc statement is a bit confusing for a FW developer to properly track command(s) execution.

2. In https://opentitan.org/book/hw/ip/spi_host/doc/registers.html#configopts--clkdiv
> "_The period of sck, T(sck) then becomes 2*(CLK_DIV+1)*T(core)_"

_by a factor of (CLKDIV+1) relative to the core clock frequency_ & _T(core)_ are a bit misleading, especially for top Darjeeling. I believe it is more the clock of the SPI host input clock that the one of the core. I would also mention `(CLKDIV+1)` only once, as it repeatead as 2*(CLK_DIV+1). It could be modified with a reference to https://opentitan.org/book/hw/ip/spi_host/doc/theory_of_operation.html#clock-rate-selection which already documents this ratio.

3. https://opentitan.org/book/hw/ip/spi_host/doc/registers.html#control--sw_rst
> "_In the current implementation, the CDC FIFOs are drained not reset._"

CDC may not be known by FW developer, so a reference to what CDC actually is would be useful (_e.g._ https://opentitan.org/book/doc/contributing/hw/methodology.html#cdc-methodology ?). It could be useful to develop this requirements by listing which FIFOs should be checked for emptyness once the `CONTROL.swrst` command is issued.

4. https://opentitan.org/book/hw/ip/spi_host/doc/registers.html#command
> "_Unlike the [CONFIGOPTS](https://opentitan.org/book/hw/ip/spi_host/doc/registers.html#configopts) multi-register, there is only one command register for controlling all attached SPI devices_"

This is no longer true since `CONFIGOPTS` is now a single register.

The update is already required here: https://opentitan.org/book/hw/ip/spi_host/doc/theory_of_operation.html#csid-register
> "_(taken from the CSID’th element of the CONFIGOPTS multi-register)._"

and here: https://opentitan.org/book/hw/ip/spi_host/doc/theory_of_operation.html#chip-select-masks
> "_the CONFIGOPTS multi-register corresponding to [CSID](https://opentitan.org/book/hw/ip/spi_host/doc/registers.html#csid)_..."

maybe elsewhere.

6. Error management It is really hard to guess what actually happens for each error type: is the command executed or not? Is it pushed into the command FIFO? Where the error handling actually occurs? There are 6 error types, reported through a single register (`ERROR_STATUS`).
For example, when the command FIFO is full, writing a new command to the `COMMAND` register discards the command and raises the `ERROR_STATUS.cmdbusy` flag, but this condition does not seem to prevent all previously pushed commands to be executed with no further action, _i.e._ the `ERROR_STATUS.cmdbusy` does not need to be cleared so that pushed commands are executed. It only prevents the FW to push new commands till it clears this bit. This is not however the same for `ERROR_STATUS.cmdinval` and `ERROR_STATUS.csidinval` bits. When one of these conditions are met, execution of all commands (including the ones already pushed into the Command FIFO) is halted.
In other words, some errors stop command execution immediately, some do not. I really think this should be documented to help the FW developer to handle errors.

6. Command FIFO
It may be hard to figure out that whereas the Command FIFO is 4-command depth, 5 commands (or segments) can actually been pushed, as the first command is immediately popped out of the FIFO for being executed.

Contributor guide

Open the contributing guide

Research direction

Start with the SPI Host theory_of_operation and registers documentation pages linked in the issue, then compare their statements with the current implementation and Verilator-generated waves. Update the explanations for STATUS.ACTIVE, clock selection, CDC FIFOs, CONFIGOPTS wording, error handling, and command FIFO depth so firmware developers can determine execution and completion behavior.

Written by the indexing model from the issue text.

Assessment

Domain
documentation, embedded-iot
Issue type
Documentation
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.