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)

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.

Done. However, since neither pw_time::Duration nor core::ops::Mul are owned by ourselves, we cannot overload the * operator and need to implement our own mul method.

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.

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.

Done. We can use the simple slicing here.

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.

I come up with a question: If we accept spi_host::RegisterBlock<ureg::RealMmioMut<'_>> or similar things directly instead of a token for constructor, how can the compiler make sure the user doesn't initialize it twice?

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.

Done. To make user's life simpler, I also add two const SpiConfig DEFAULT_SPI0 & DEFAULT_SPI1. Original implementation hard codes different clkdiv values when initializing SpiHost0/SpiHost1, but I think we prefer to extract them and make them configurable here?

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.

Done. We can simply use * here.

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

Hi @cfrantz, I just ported the entire SPI flash from the original project to here, and we should be able to review it by simply diffing these two files. (Unit tests are still WIP.)

Also, some of the comments above may become obsolete due to this re-implementation.

Port the Serial Flash Discoverable Parameters (SFDP) parser library from the legacy implementation. Key modifications and fixes made during the port: - Time representation: Replaced the custom `Nanoseconds` type with new custom type based on `core::time::Duration` across all timing tables. - 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. - Default Value in Enums: 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. - 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. Also includes original unit tests running against mock SFDP tables. Signed-off-by: Chia-Wei Liu <lchiawei@google.com>

Key modifications and fixes made during the port: - Implements the `embedded-hal` `SpiDevice` trait, which includes: * Defines a related `SpiError` type. * Defines the `transaction` method in the struct, which accepts an array of operations, and `csaat` will be asserted during the entire operation series. * Marks the operations like `Transfer`, `TransferInPlace`, and `DelayNs` as unsupported explicitly, which is also originally unsupported. * Removes unsupported hardware optimization methods like `prefetch`, `drain_fifo`, and `aligned_drain_fifo` since they are not defined in `embedded_hal`. If we need the original optimization that only works on aligned words, we can branch on aligned/unaligned input and apply similar optimization when the input is aligned. However, we should stick with the input type defined in `embedded_hal`. - Defines a dedicated `SpiConfig` struct and a dedicated `configure` method to make SPI Host configurable. Also includes a smoke test validating SPI read transactions by checking the SFDP signature. Signed-off-by: Chia-Wei Liu <lchiawei@google.com>

Key modifications and fixes made during the port: - Makes the driver explicitly enter "Global 4-byte" mode when flash size >= 16 MB, which includes: * Update the command config in `from_sfdp()` & `from_sfdp_conservative()`. * Explicitly enters the global 4-byte mode at the end of initialization. - Skips the support of DUAL / QUAD in `SpiTxnWidth`. Also, inits the driver with non-conservative version for now. - Moves `transfer_req_resp` from legacy SPI Host driver to SPI Flash driver, as a helper method. - Moves `check_valid_size` from legacy Flash trait to SPI Flash driver, as a helper method. - Adds `is_busy` helper method as what we did for `BlockingFlash` trait, although we didn't implement this trait directly now. - Wraps legacy `page_size` and `size` into a `geometry` method required by `Flash` trait. Currently we hard-coded the erasable bitmap as 4KB and 64KB, and we may parse them in the future. - For `erase()` method, the `Flash` trait only accepts the size declared in the erasable bitmap, but here we uses the legacy implementation which will loop over the size to erase. However, we still only accepts power of 2 as the erase size due to `Flash` trait's contract. Also removes the original `erase_page()` method. - Drops `FlashStreamingRead` support (as we mentioned in SPI Host driver commit). The migration and refactoring for the unit tests of SPI driver is still WIP. Signed-off-by: Chia-Wei Liu <lchiawei@google.com>

lchiawei · 2026-07-14T22:28:44Z

Close this PR for now as I split and moved it to #345, #346 and #347.

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 2 times, most recently 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 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

lchiawei force-pushed the spi-driver-sync-refactor branch 3 times, most recently from a7eb428 to 7c64e49 Compare July 12, 2026 10:40

lchiawei force-pushed the spi-driver-sync-refactor branch from 7c64e49 to f56eeca Compare July 13, 2026 17:17

lchiawei force-pushed the spi-driver-sync-refactor branch 3 times, most recently from 5aeea4f to fbaf876 Compare July 14, 2026 00:10

lchiawei added 2 commits July 14, 2026 08:24

earlgrey: tests: Add SPI Flash IPC integration test

804cfb5

lchiawei force-pushed the spi-driver-sync-refactor branch from fbaf876 to 804cfb5 Compare July 14, 2026 00:25

lchiawei closed this Jul 14, 2026

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!

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!

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!

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

edited

Loading