[spi] [spi_host] [spi_device] DIFs missing hardware state guards before control register writes
Nobody has claimed this yet.
- Dominant language
- SystemVerilog
- Stars
- 3.6k
- Forks
- 1.1k
- Avg merge
- 2d 22h
- Merged PRs (30d)
- 141
Description
### Description
While reviewing the SPI Host and SPI Device DIFs against their RTL
specifications (OpenTitan @ ffab8da7381c66b3d2e2b78a382a9f6937e5482e),
we noticed three functions that modify control registers without
verifying the hardware is in a ready state, despite explicit
documentation warnings and the availability of query APIs.
All three follow the same pattern: the DIF provides a status query
function, but the control function does not automatically invoke it.
---
## 1. SPI Host: `dif_spi_host_write_command` lacks READY check
**Document constraint** (`spi_host.hjson`):
> "Writing to COMMAND when READY is low is an error, and will trigger an interrupt."
**Available query API**: `dif_spi_host_get_status()` returns `STATUS.READY`.
**Code** (`dif_spi_host.c`):
Directly writes `COMMAND` without checking `STATUS.READY`.
---
## 2. SPI Device: `dif_spi_device_configure` lacks CSB check before mode switch
**Document constraint** (`spi_device.hjson`):
> "Changing modes randomly can result in unknown values being passed between spi_dev/host due to passthrough"
**Available query API**: `dif_spi_device_get_csb_status()` returns `CSB`.
**Code** (`dif_spi_device.c`):
Modifies `CONTROL.MODE` without verifying `CSB` is high (upstream host inactive).
---
## 3. SPI Device: `dif_spi_device_clear_flash_status_request` lacks CSB check
**Document constraint** (`spi_device.hjson`):
> "The reset should only be used when the upstream SPI host is known to be inactive."
**Available query API**: `dif_spi_device_get_csb_status()`.
**Code** (`dif_spi_device.c`):
Sets `FLASH_STATUS_CLR` without verifying `CSB` is high.
---
## Common pattern
| Function | Control Register | Required State | Query API Available |
|----------|-----------------|---------------|-------------------|
| `dif_spi_host_write_command` | `COMMAND` | `STATUS.READY = 1` | `dif_spi_host_get_status` |
| `dif_spi_device_configure` | `CONTROL.MODE` | `CSB = 1` (inactive) | `dif_spi_device_get_csb_status` |
| `dif_spi_device_clear_flash_status_request` | `FLASH_STATUS_CLR` | `CSB = 1` (inactive) | `dif_spi_device_get_csb_status` |
## Suggested fix
Add the corresponding state check at the beginning of each function,
returning `kDifUnavailable` when the hardware is not in the required
state. This adds defensive programming without affecting the common
path (the condition is usually met).
## Version
- OpenTitan commit: ffab8da7381c66b3d2e2b78a382a9f6937e5482e
- SPI Host: V2, S2
- SPI Device: V2, S2
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with dif_spi_host.c and dif_spi_device.c, reading dif_spi_host_write_command, dif_spi_device_configure, dif_spi_device_clear_flash_status_request, and their status-query APIs against spi_host.hjson and spi_device.hjson. Done means each control-register write verifies the required hardware state and returns kDifUnavailable when that state is not met.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- embedded-iot
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100