lowRISC / lowRISC/opentitan

[spi] [spi_host] [spi_device] DIFs missing hardware state guards before control register writes

Open
#30,530 1 comment 0 reactions 0 assignees View on GitHub

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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.