earlgrey: Implement SPI driver and SPI Flash Server by lchiawei · Pull Request #334 · OpenPRoT/openprot

lchiawei · 2026-07-06T23:57:05Z

This is for early review.

These commits are modified from the internal implementation. The main change I made is mentioned in the message for each commit.

linux-foundation-easycla · 2026-07-06T23:57:10Z

The committers listed above are authorized under a signed CLA.

✅ login: lchiawei / name: Chia-Wei Liu (09366d1, 50fcfa6, 6214b62, a263a01)

…or safety Port the Serial Flash Discoverable Parameters (SFDP) parser library from the legacy implementation and apply strict safety refactorings to comply with project safety and coding guidelines. Key modifications and fixes made during the port: - Time representation: Replaced the custom `Nanoseconds` type with standard `core::time::Duration` across all timing tables (Erase, Page Program, Byte Program, Chip Erase). - Legacy Bug Fixes: Corrected incorrect bitmasks in `ChipEraseTimeUnit::from_bits` and `DurationEnumAUnit::from_bits` from `& 0b1` to `& 0b11`, enabling the parser to correctly resolve all 4 states instead of triggering panics. - Panic-Free Safety: Eliminated all `unreachable!()` wildcard arms in enum deserializers, replacing them with catch-all wildcards returning default or safe fallback states. - Underflow & Overflow Protection: - Added capacity bounds checks in `MemoryDensity::byte_len` to prevent subtraction underflow when bit length is less than 3. - Added shift boundary checks in `PowerOf2::value` to prevent undefined behavior/panics when exponent exceeds platform `usize` bit width. - Safe Slicing: Replaced direct indexing `[..bytes_len]` in `SfdpReader::read_parameter_table` with safe `.get_mut()` slicing propagating `FLASH_GENERIC_SFDP_PARAMETERS_TOO_LONG` on buffer overflow. - Code Cleanups: Refactored `U24` to use `to_le_bytes`/`from_le_bytes` instead of raw bitwise shifts, and streamlined `MemoryDensity::from_byte_len` with pattern matching. Includes target-agnostic unit tests running against mock SFDP tables. Signed-off-by: Chia-Wei Liu <lchiawei@google.com>

cfrantz · 2026-07-08T03:43:44Z

+
+use bitfield_struct::bitfield;
+use core::mem::offset_of;
+use core::time::Duration;


We should investigate pw_time::Duration and see if its a better fit.

Can we just stick to use core::time::Duration in this PR because we don't really utilize the timeout in our driver? Also, one of the reason that I use core library here is that I would like to make the library and unit tests in this file to be less platform-dependent and easily tested on host. But I agreed that we should care about whether it costs too much to perform (microseconds * clock) on the hardware.

pw_time is a single 64-bit integer, has a host implementation and it allows you to choose an arbitrary tick rate by implementing Clock on a type.

core::time::Duration has a more complex representation:

pub struct Duration { secs: u64, nanos: Nanoseconds, // Always 0 <= nanos < NANOS_PER_SEC }

Math ops on this type will always involve the 64-bit computation and whatever Nanoseconds is (probably a u32) and will involve managing carry/borrow between the two components.

I think for now, we should do what the original code did: create a Nanoseconds type alias over a nanosecond resolution clock:

pub struct NanoClock { ... } impl pw_time::Clock for NanoClock { const TICKS_PER_SEC: u64 = 1_000_000_000; } type Nanoseconds = pw_time::Duration<NanoClock>;

This strategy will keep the complexity of the core type out of this code but still allow precise extraction of the time value if needed.

cfrantz · 2026-07-08T03:55:24Z

+    type Error = SpiError;
+}
+
+impl<Mmio: SpiHostMmio> embedded_hal::spi::SpiDevice for SpiHost<Mmio> {


Does embedded_hal's SPI traits take into account different spi transaction widths? (Single, Dual, Quad)?

We can stick with embedded_hal for now. In the future, we'll consider extending the hal or writing own extension traits to handle Dual and Quad modes.

Yeah, I believe that embedded_hal only has single mode. Do we have requirements for DUAL and QUAD modes in foreseeable milestones?

cfrantz · 2026-07-08T03:57:36Z

+        if let Some(jesd216a) = table.table_jesd216a() {
+            let page_size = jesd216a.word11.page_size().value();
+            if page_size != 256 {
+                pw_log::error!(


We shouldn't log in the spi_flash driver.

Removed all the occurrences using pw_log in this file.

cfrantz · 2026-07-08T04:15:13Z

+    fn format_cmd_prefix(opcode: u8, addr: u32, addr_bytes: u8) -> ([u8; 5], usize) {
+        let [b0, b1, b2, b3] = addr.to_be_bytes();
+        if addr_bytes == 3 {
+            ([opcode, b1, b2, b3, 0], 4)
+        } else {
+            ([opcode, b0, b1, b2, b3], 5)
+        }
+    }


Returning the fixed sized array and size like this requires a checked slicing operation.
We can avoid the checked slice by returning a slice from the command formatting function:

fn format_cmd_prefix(buf: &mut [u8; MAX_PREFIX], opcode: u8, addr: u32, addr_size: u8) -> &[u8] { let [b0, b1, b2, b3] = addr.to_be_bytes(); buf[0] = opcode; if addr_bytes == 3 { buf[1] = b1; buf[2] = b2; buf[3] = b3; &buf[..4] } else { buf[1] = b0; buf[2] = b1; buf[3] = b2; buf[4] = b3; &buf[..5] } }

Done. Addressed it together with the comment below.

cfrantz · 2026-07-08T04:19:04Z

+        let (mut prefix, mut len) = Self::format_cmd_prefix(
+            self.read_cmd.opcode,
+            start_addr.offset(),
+            self.read_cmd.addr_bytes,
+        );
+
+        for _ in 0..self.read_cmd.dummy_bytes {
+            if let Some(cell) = prefix.get_mut(len) {
+                *cell = 0;
+                len = len
+                    .checked_add(1)
+                    .ok_or(error::FLASH_GENERIC_BAD_ALIGNMENT)?;
+            }
+        }


The command formatting could be made into a function on SfCmd and we could avoid repeating this sequence of code for the various operations.

Done. Also prefilled the buffer with 0s so that we do not have to fill the dummy bytes anymore.

cfrantz · 2026-07-08T20:14:47Z

+pub trait SpiHostMmio {
+    /// Get the register block for reading.
+    fn regs(&self) -> spi_host::RegisterBlock<ureg::RealMmio<'_>>;
+    /// Get the register block for writing.
+    fn regs_mut(&mut self) -> spi_host::RegisterBlock<ureg::RealMmioMut<'_>>;
+}
+
+impl SpiHostMmio for spi_host::SpiHost0 {
+    fn regs(&self) -> spi_host::RegisterBlock<ureg::RealMmio<'_>> {
+        self.regs()
+    }
+    fn regs_mut(&mut self) -> spi_host::RegisterBlock<ureg::RealMmioMut<'_>> {
+        self.regs_mut()
+    }
+}
+
+impl SpiHostMmio for spi_host::SpiHost1 {
+    fn regs(&self) -> spi_host::RegisterBlock<ureg::RealMmio<'_>> {
+        self.regs()
+    }
+    fn regs_mut(&mut self) -> spi_host::RegisterBlock<ureg::RealMmioMut<'_>> {
+        self.regs_mut()
+    }
+}
+
+/// Earlgrey SPI Host driver.
+pub struct SpiHost<Mmio: SpiHostMmio> {
+    mmio: Mmio,
+}


We should try to note use these extra traits. Instead, we should declare SpiHost something like this:

type SpiHostMmio = spi_host::RegisterBlock<ureg::RealMmioMut>; pub struct SpiHost<M: SpiHostMmio> { mmio: M, }

We need to do a bit of work to understand if we need any lifetime annotations. I think 'static is appropriate as it is going to exclusively own the peripheral.

I found that SPI Host 0 and 1 have literally identical type spi_host::RegisterBlock<ureg::RealMmioMut> instead of just having a same trait, so we can even avoid the generics.

cfrantz · 2026-07-08T20:15:53Z

+    type Error = SpiError;
+}
+
+impl<Mmio: SpiHostMmio> embedded_hal::spi::SpiDevice for SpiHost<Mmio> {


We can stick with embedded_hal for now. In the future, we'll consider extending the hal or writing own extension traits to handle Dual and Quad modes.

lchiawei · 2026-07-08T21:39:11Z

Also rolled back the unrelated services/flash/server.rs to make this PR more focused.

Implement the low-level SPI Host driver for the Earlgrey platform and its smoke test. The implementation: - Implements the embedded-hal SpiDevice trait for synchronous transaction support. - Implements control/data path separation by introducing SpiConfig and configure() interface. - Utilizes checked timeout busy-loops and safe slice accessors to prevent panics and infinite hangs. - Includes a userspace smoke test validating SPI read transactions by checking the SFDP signature. Signed-off-by: Chia-Wei Liu <lchiawei@google.com>

Implement a generic SPI NOR Flash driver based on JEDEC SFDP parameters. The implementation: - Implements the Flash trait for SpiFlash to perform erase, read, and program operations. - Interacts with hardware via the embedded-hal SpiDevice abstraction. - Automatically configures memory geometry and parameters parsed dynamically from SFDP. - Includes a userspace smoke test verifying basic page erase, program, and readback. Signed-off-by: Chia-Wei Liu <lchiawei@google.com>

Introduce a wait_group based combined flash server and its integration test. The implementation: - Implements combined_flash_server that multiplexes EFlash and SPI Flash IPC requests on a single thread using wait_group. - Decouples driver execution from thread blocking strategies via the Blocking trait. - Includes spi_flash_test client to verify parallel sequential EFlash and SPI Flash operations on FPGA. Signed-off-by: Chia-Wei Liu <lchiawei@google.com>

lchiawei · 2026-07-08T23:20:55Z

I found myself missing the boundary check for program() (as what I did in erase()/read()) in spi_flash, so I just added it.
Besides, I also added a QEMU version for SPI host smoke test to make it covered by our presubmit CI.

cfrantz · 2026-07-08T22:24:15Z

+        let target = target_slice
+            .get_mut(..bytes_len)
+            .ok_or(error::FLASH_GENERIC_SFDP_PARAMETERS_TOO_LONG)?;


Was this flagged by the panic detector? The min expression above should have already proved to the optimizer that bytes_len can never be larger than the size of the target_slice.

cfrantz · 2026-07-08T22:40:14Z

+
+use bitfield_struct::bitfield;
+use core::mem::offset_of;
+use core::time::Duration;


pw_time is a single 64-bit integer, has a host implementation and it allows you to choose an arbitrary tick rate by implementing Clock on a type.

core::time::Duration has a more complex representation:

pub struct Duration { secs: u64, nanos: Nanoseconds, // Always 0 <= nanos < NANOS_PER_SEC }

Math ops on this type will always involve the 64-bit computation and whatever Nanoseconds is (probably a u32) and will involve managing carry/borrow between the two components.

I think for now, we should do what the original code did: create a Nanoseconds type alias over a nanosecond resolution clock:

pub struct NanoClock { ... } impl pw_time::Clock for NanoClock { const TICKS_PER_SEC: u64 = 1_000_000_000; } type Nanoseconds = pw_time::Duration<NanoClock>;

This strategy will keep the complexity of the core type out of this code but still allow precise extraction of the time value if needed.

cfrantz · 2026-07-08T22:42:33Z

+/// Trait representing a SPI Host MMIO interface.
+pub trait SpiHostMmio {
+    /// Get the register block for reading.
+    fn regs(&self) -> spi_host::RegisterBlock<ureg::RealMmio<'_>>;
+    /// Get the register block for writing.
+    fn regs_mut(&mut self) -> spi_host::RegisterBlock<ureg::RealMmioMut<'_>>;
+}
+
+impl SpiHostMmio for spi_host::SpiHost0 {
+    fn regs(&self) -> spi_host::RegisterBlock<ureg::RealMmio<'_>> {
+        self.regs()
+    }
+    fn regs_mut(&mut self) -> spi_host::RegisterBlock<ureg::RealMmioMut<'_>> {
+        self.regs_mut()
+    }
+}
+
+impl SpiHostMmio for spi_host::SpiHost1 {
+    fn regs(&self) -> spi_host::RegisterBlock<ureg::RealMmio<'_>> {
+        self.regs()
+    }
+    fn regs_mut(&mut self) -> spi_host::RegisterBlock<ureg::RealMmioMut<'_>> {
+        self.regs_mut()
+    }
+}
+
+/// Earlgrey SPI Host driver.
+pub struct SpiHost<Mmio: SpiHostMmio> {
+    mmio: Mmio,
+}


My previous comment may have gotten lost. These traits (which exist in the original) aren't really needed. We need only to keep a spi_host::RegisterBlock<ureg::RealMmioMut<'_>> in struct SpiHost. We can get or construct that entity from the spi_host registers crate.

cfrantz · 2026-07-08T22:46:16Z

+            let chunk = data.get(..chunk_len).ok_or(SpiError::InvalidTransaction)?;
+            data = data.get(chunk_len..).ok_or(SpiError::InvalidTransaction)?;


It seems like we're performing the same slicing operation twice here. It also seems that min should allow us to do the slicing operation without needing the get.

Is this something flagged by the no_panics_test?

I misread the two get operations last night. The min operation should prove to the optimizer that chunk_len is in bounds, and we can use split to get the two slices we want:

let (chunk, rest) = data.split_at(chunk_len); data = rest;

cfrantz · 2026-07-08T22:50:00Z

+}
+
+impl SfCmd {
+    fn format_prefix<'a>(&self, buf: &'a mut [u8], addr: u32) -> Result<&'a [u8], ErrorCode> {


If you always pass an exact sized buf, you are passing an array instead of a slice. Since arrays have known sizes, there are certain panic checks that can proven at compile time. I recommend establishing a max prefix size (e.g. 6 bytes), making that a constant and then using buffers of that size everywhere they are needed.

cfrantz · 2026-07-08T22:55:42Z

+            .ok_or(error::FLASH_GENERIC_BAD_ALIGNMENT)?;
+
+        // Fill prefix with 0 to initialize dummy bytes segment automatically.
+        prefix.fill(0);


If the prefix buffer is always declared in the caller like let mut buffer = [0u8; PREFIX_LEN], you don't need to fill it with zeros. You can make it part of the contract for this function that the caller needs to pre-initialize the buffer to zero.

cfrantz · 2026-07-08T22:56:36Z

+                let addr_slice: &mut [u8; 3] = rest
+                    .get_mut(..3)
+                    .ok_or(error::FLASH_GENERIC_BAD_ALIGNMENT)?
+                    .try_into()
+                    .map_err(|_| error::FLASH_GENERIC_BAD_ALIGNMENT)?;
+                *addr_slice = [b1, b2, b3];


This also seems like a lot of error checking for what is poking 3 or 4 bytes into a buffer that should be of a known and correct size.

cfrantz · 2026-07-08T22:59:47Z

+
+    pub fn read_status(&mut self) -> Result<u8, ErrorCode> {
+        let tx = [OP_READ_STATUS];
+        let mut rx = [0u8];


A neat trick you can do instead of dealing with 1-byte arrays:

let mut rx = 0u8; let mut ops = [ embedded_hal::spi::Operation::Read(core::slice::from_mut(&mut rx)), ];

This works because the "slice" type can be thought of as a built-in type in rust and it can construct a slice-of-1 from a ref-to-item.

cfrantz · 2026-07-09T00:47:42Z

+            let chunk = data
+                .get(..chunk_len)
+                .ok_or(error::FLASH_GENERIC_BAD_ALIGNMENT)?;


This should also be provable because of the min operation.

cfrantz · 2026-07-09T15:57:00Z

+    pub fn init(&mut self) -> Result<(), SpiError> {
+        // Default safe configuration (24MHz if core is 96MHz, Mode 0, CS0)
+        let default_config = SpiConfig {
+            clkdiv: 1,


spi_host_0 and spi_host_1 are in different clock domains:

spi_host_0 is in the high-speed peripheral domain

spi_host_1 is in the normal peripheral domain

We need to pass different dividers to initialize them to the same speed.

cfrantz · 2026-07-09T15:58:49Z

+        self.mmio.control().write(|w| w.sw_rst(true));
+
+        // Wait for both FIFOs to empty.
+        let mut timeout = TIMEOUT_LIMIT;


I don't think we need these timeout loops: a timeout in these cases mean the hardware is broken and isn't clocking data through the peripheral.

cfrantz · 2026-07-09T16:01:47Z

+            let mut timeout = TIMEOUT_LIMIT;
+            loop {
+                let status = self.mmio.status().read();
+                if status.ready() && !status.active() {
+                    break;
+                }
+                timeout = timeout.checked_sub(1).ok_or(SpiError::Timeout)?;
+            }


Similarly, a timeout here means bad hardware. This is the place a block-for-interrupt belongs. We can busy-loop for now and add interrupt support in the future (leave a todo):

while !self.mmio.regs_mut().status().read().ready() { // TODO: we should block here for and interrupt that would signal the hardware is ready. }

cfrantz · 2026-07-09T16:02:45Z

+            let mut timeout = TIMEOUT_LIMIT;
+            loop {
+                let status = self.mmio.status().read();
+                if status.ready() && !status.active() {
+                    break;
+                }
+                timeout = timeout.checked_sub(1).ok_or(SpiError::Timeout)?;
+            }


Busy-loop for now.

cfrantz · 2026-07-09T16:04:00Z

+            let (mut chunk_dest, rest) = dest.split_at_mut(chunk_len);
+            dest = rest;
+
+            let mut rx_timeout = TIMEOUT_LIMIT;


Busy-loop for now.

cfrantz · 2026-07-09T16:07:54Z

+            while !chunk_dest.is_empty() {
+                let status = self.mmio.status().read();
+                let rxqd = status.rxqd() as usize; // rxqd is in 32-bit words
+                let bytes_in_fifo = rxqd.checked_mul(4).ok_or(SpiError::HardwareError)?;


Is the checked_mul necessary? We know from the hardware spec that the fifo depth can have a certain maximum size (8 bits in the status register). We can never get a value from the hardware that would overflow. We also min this value against the chunk_dest.len(), so we're bounding the length appropriately.

cfrantz · 2026-07-09T16:09:27Z

nit: we should put this test in //target/earlgrey/tests/drivers/spi_host/

cfrantz · 2026-07-09T16:11:10Z

+impl Blocking for SpiFlashSleep {
+    fn wait_for_notification(&self) {
+        // Sleep for 1 millisecond between WIP status checks to reduce CPU load
+        let _ = sleep_until(SystemClock::now() + Duration::from_millis(1));
+    }
+}


Sleeping while waiting for the SPI flash is more properly the responsibility of the spi_host driver and the sleep should be interrupt driven.

cfrantz · 2026-07-09T16:11:55Z

Lets do a comparison of this driver vs. the original mutask driver.

Let me reorganize the spi_flash.rs here so that we can perform an easier line-to-line comparison with the original mutask implementation

lchiawei force-pushed the spi-driver-sync-refactor branch 2 times, most recently from 90b5641 to 50fcfa6 Compare July 7, 2026 18:31

lchiawei closed this Jul 7, 2026

lchiawei reopened this Jul 7, 2026

lchiawei force-pushed the spi-driver-sync-refactor branch from 50fcfa6 to 8e945dc Compare July 7, 2026 19:26

lchiawei requested a review from cfrantz July 7, 2026 20:42

lchiawei force-pushed the spi-driver-sync-refactor branch from 8e945dc to 7b332e5 Compare July 7, 2026 21:09

lchiawei force-pushed the spi-driver-sync-refactor branch from 7b332e5 to ccefb91 Compare July 7, 2026 22:48

cfrantz reviewed Jul 8, 2026

View reviewed changes

lchiawei force-pushed the spi-driver-sync-refactor branch from ccefb91 to 1e40e96 Compare July 8, 2026 21:37

lchiawei added 3 commits July 9, 2026 06:54

lchiawei force-pushed the spi-driver-sync-refactor branch from 1e40e96 to 60217d9 Compare July 8, 2026 23:17

cfrantz reviewed Jul 9, 2026

View reviewed changes

		let chunk = data.get(..chunk_len).ok_or(SpiError::InvalidTransaction)?;
		data = data.get(chunk_len..).ok_or(SpiError::InvalidTransaction)?;

Uh oh!

Conversation

lchiawei commented Jul 6, 2026

Uh oh!

linux-foundation-easycla Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lchiawei commented Jul 8, 2026

Uh oh!

lchiawei commented Jul 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

linux-foundation-easycla Bot commented Jul 6, 2026 •

edited

Loading