spi: Add SPI master implementation #53

astapleton · 2025-05-30T01:13:48Z

This adds a master mode implementation for the SPI peripheral.

The driver is split such that the core logic is implemented in spi.rs, and the rest is contained within a submodule:

User configuration is split off into the config module
The transaction module encapsulates the different transaction types (read, write, transfer, transfer inplace). Buffer management is abstracted into the transaction type to enable non-blocking usage the peripheral.
The public facing APIs are mostly implemented in the hal module (embedded-hal trait implementations), and the nonblocking module, which exposes a NonBlocking trait that a user needs to explicitly use in order to use the non-blocking APIs (expected to be the less used case, this avoids polluting the public API too much).

usbalbin · 2025-06-01T09:45:01Z

src/spi.rs

@@ -0,0 +1,1008 @@
+//! Serial Peripheral Interface (SPI)
+//!
+//! This module provides functionality for SPI as both a Master. It


Remove both here?

usbalbin · 2025-06-01T11:12:42Z

src/spi.rs

+        assert_eq!(
+            config.hardware_cs.enabled(),
+            PINS::HCS_PRESENT,
+            "If the hardware cs is enabled in the config, an HCS pin must be present in the given pins"
+        );


What if the chip select defaulted to none and instead configured using a builder-like method with both the pin and the corresponding config? Then this sort of check would not be needed?

Correct me if I am wrong here, as a bonus that would then ensure PINS is always clk, miso, mosi so then we do not need the tuple to emulate variable number of arguments

So are you suggesting we pass the CS pin to a method on the Config struct? or add a builder like method to Spi to provide the CS pin (e.g. SPI1.spi(...).with_hardware_cs<P>(pin: P))?

I did not really think that far to be honest. :)

In my mind I think it reads quite nice as SPI1.spi(pins...).with_hardware_cs<P>(pin: P, config: CsCfg)).finalize(rcc_thing). Where .finalize() or something actually sets up the entire peripheral, unless the cs setup can be done after init?

I do think it might be a bit of a plus to keep cs Pin and cs config together for usability and discoverability. But I do not have any too strong opinions.

Anyways that was just a quick thought without really thinking through the consequences. :)

I'll give it some thought. I do take the point that letting the compiler do the work here would be nicer, but will have to think about what structure makes sense.

I think I would prefer to leave this as is for now. I think the pin allocation pattern in this HAL could use some work in general (it would be nice to replicate what stm32f4xx-hal did), so I would defer this sort of change to during or after that effort.

usbalbin · 2025-06-05T12:51:58Z

src/spi.rs

+            !read.is_empty() && !write.is_empty(),
+            "Transfer buffers should not be non-zero length"
+        );
+        if self.inner.communication_mode() != CommunicationMode::FullDuplex {


Since this is only set once in init, how about we make this part of the type? Then we can just not implement the features that should be unavailable? However I suppose the array of operations thing might be harder to do in that case..

Here is a very quick and messy attempt to get that working. A lot of cleanup needed, there is probably a lot of the weird where clauses and traits that can be simplified and a lot of other things that i have missed :)

I agree it would be nice. I made an effort to do it too, but it requires so much refactoring, that I'm not sure it's worth it. In your example you don't actually remove the runtime checks either, they're just pushed into the trait implementations as unimplemented calls. You'd need to break the non-blocking trait into separate traits, and it just becomes a lot for minimal gain, in my opinion.

Fair enough :)

usbalbin · 2025-06-05T12:53:16Z

src/spi.rs

+    ) -> Result<Transaction<TransferInplace<'a, W>, W>, Error> {
+        assert!(
+            !words.is_empty(),
+            "Transfer buffer should not be non-zero length"


Suggested change

"Transfer buffer should not be non-zero length"

"Transfer buffer should not be of zero length"

usbalbin · 2025-06-05T12:54:11Z

src/spi.rs

+    ) -> Result<Transaction<Transfer<'a, W>, W>, Error> {
+        assert!(
+            !read.is_empty() && !write.is_empty(),
+            "Transfer buffers should not be non-zero length"


Suggested change

"Transfer buffers should not be non-zero length"

"Transfer buffers should not be of zero length"

tytouf · 2025-06-06T11:00:44Z

I tested this branch and I found that the following pin is not correctly configured (it exists also on h533) :

+++ b/src/spi/spi_def.rs
@@ -341,7 +341,6 @@ mod rm0481_common {
                 NoMosi,
                 #[cfg(feature = "h523_h533")]
                 gpio::PA8<Alternate<6>>,
-                #[cfg(feature = "stm32h523")]
                 gpio::PC1<Alternate<6>>,

tytouf · 2025-06-06T16:53:18Z

I'm seeing instabilities in the SPI SCLK signal : on first transaction I get
a long first "tick" while on subsequent transactions it works fine. It's repeating for each first transaction. See attached picture :

astapleton · 2025-06-06T21:09:36Z

I'm seeing instabilities in the SPI SCLK signal : on first transaction I get a long first "tick" while on subsequent transactions it works fine. It's repeating for each first transaction.

I haven't observed that. What mode is the SPI being used in (phase/polarity)? I can take a look to see if I can reproduce.

tytouf · 2025-06-07T09:10:09Z

I'm seeing instabilities in the SPI SCLK signal : on first transaction I get a long first "tick" while on subsequent transactions it works fine. It's repeating for each first transaction.

I haven't observed that. What mode is the SPI being used in (phase/polarity)? I can take a look to see if I can reproduce.

Sorry, here's the configuration :

    let ccdr = rcc
        .sys_ck(160.MHz())
        .pll1_q_ck(20.MHz())
        .freeze(pwrcfg, &dp.SBS);

  let sck = gpioa.pa0.into_alternate();
    let miso = gpioc.pc0.into_alternate();
    let mosi = gpioc.pc1.into_alternate();

    // Initialise the SPI peripheral.
    let imu_spi = dp.SPI4.spi(
        (sck, miso, mosi),
        spi::Config::new(spi::MODE_3).endianness(spi::Endianness::MsbFirst),
        5.MHz(),   //10.MHz(),
        ccdr.peripheral.SPI4,
        &ccdr.clocks,
    );

Wratox · 2025-06-12T11:55:46Z

When using(taken from the w5500):

fn read_frame(&mut self, block: u8, address: u16, data: &mut [u8]) -> Result<(), SPI::Error> {
    let address_phase = address.to_be_bytes();
    let control_phase = block << 3;

    self.spi.transaction(&mut [
        Operation::Write(&address_phase), // 2 bytes
        Operation::Write(&[control_phase]), // 1 byte
        Operation::TransferInPlace(data), // 1 byte
    ])?;

    Ok(())
}

I get this in my logic analyser:

but the data variable from the code snippet above (which should be 4) is 1
My knowledge of spi is limited but it seems like the write operation does not ignore the read data during the transaction.

Here's my config if it is of any value to you:

// Initialise the SPI peripheral.
let spi = dp.SPI1.spi(
    // Give ownership of the pins
    (sck, miso, mosi, hcs),
    // Create a config with the hardware chip select given
    spi::Config::new(spi::MODE_0)
        // Put 1 us idle time between every word sent
        .inter_word_delay(0.000001)
        // Specify that we use the hardware cs
        .hardware_cs(spi::HardwareCS {
            // See the docs of the HardwareCSMode to see what the different modes do
            mode: spi::HardwareCSMode::FrameTransaction,
            // Put 1 us between the CS being asserted and the first clock
            assertion_delay: 0.000001,
            // Our CS should be high when not active and low when asserted
            polarity: spi::Polarity::IdleHigh,
        })
        .communication_mode(CommunicationMode::FullDuplex),
    1.MHz(),
    ccdr.peripheral.SPI1,
    &ccdr.clocks,
);

usbalbin · 2025-06-12T16:29:46Z

Signals in the post above:

Yellow CS
Green CLK
Blue MISO
Purple/Pink MOSI

usbalbin · 2025-06-12T16:34:19Z

In other words, it seems that even when Operation::Write the RX fifo is added to. So once we come to the Operation::TransferInPlace the first thing in the fifo is the byte received during the first Operation::Write for case above. That first byte is then written to the slice and unexpected things happen :)

Atleast that seems to be what is going on from what we can tell.

astapleton · 2025-06-12T17:23:22Z

Thanks for testing! The data in the Rx FIFO should be discarded during the write operation. I'll do some testing to see what might be going on there.

usbalbin · 2025-06-12T18:27:55Z

src/spi.rs

+        // Keep emptying Rx FIFO if data to write is longer than data to read
+        let discard_remainder = transaction.rx_remainder_to_discard();
+        if transaction.is_read_complete() && discard_remainder > 0 {
+            let count = self.discard_rx_fifo(discard_remainder)?;


Hm, shouldn't this ensure that we discard the received data during a Write?

it should, yes

I'll have to double check that we are not using some old version of your branch, just in case. However I will not have access to @Wratox's files until tomorrow

I reproduced it. It's a bug in checking whether a transaction is complete that does not consider whether there are is any data to discard.

This should be fixed in d099991

That did indeed solve the problem we were having. Thanks!

…e receiver/transmitter

astapleton · 2025-06-12T22:10:57Z

I'm seeing instabilities in the SPI SCLK signal : on first transaction I get a long first "tick" while on subsequent transactions it works fine. It's repeating for each first transaction. See attached picture : ...

Ok, I do see this in MODE 2/3 (CPOL=1). So what is happening is that the SPI is relinquishing control of the pins when the SPI is disabled at the end of a transaction, which brings the pins low. When the peripheral is enabled at the beginning of a transaction it then pulls the line high (just before the transaction starts), which is that first edge you (and your logic analyzer) are seeing. The delay is the delay between enabling the peripheral and writing the first byte. When I count the clock pulses subsequent to that edge, I still get the correct number for a byte.

Unfortunately, because you're manually controlling the CS line, when the clock is brought high when the SPI peripheral is enabled, it is indistinguishable from an extra clock pulse. When I give the SPI peripheral control of the CS line (see spi_send_frames example) it only asserts the CS line after bringing the clock line high.

The AFCNTR pin controls whether the SPI relinquishes control of the pin when the SPI is disabled. Setting this bit holds the clock line high all the time and the extra clock pulse is not an issue. I've set this bit now during initial configuration (see a794a62) . Given that the pins are passed to the peripheral during initialization, there is no currently supported use case to allow sharing of the pins or reason for the SPI peripheral to relinquish control of the pins.

tytouf · 2025-06-18T10:16:57Z

ue. I've set this bit now during initial configuration (see a794a62) . Given that the pins are passed to the peripheral during initializ

Excellent, thanks @astapleton . I found the same thing in parallel but was late seeing your message.

usbalbin

This looks good to me now! Very nice :)

astapleton requested review from ryan-summers and usbalbin May 30, 2025 01:13

astapleton mentioned this pull request May 30, 2025

Feature request - SPI #36

Closed

astapleton force-pushed the as/spi-master branch 2 times, most recently from fac51d0 to 15a6a7a Compare May 30, 2025 01:21

This was referenced May 28, 2025

Peripheral overview #49

Open

Add support for STM32H523/STM32H533 #39

Closed

usbalbin reviewed Jun 1, 2025

View reviewed changes

spi: Add SPI master implementation

bd6ece1

astapleton force-pushed the as/spi-master branch from 15a6a7a to bd6ece1 Compare June 2, 2025 18:08

Fix documentation

dd86364

usbalbin reviewed Jun 5, 2025

View reviewed changes

usbalbin reviewed Jun 12, 2025

View reviewed changes

astapleton removed the request for review from ryan-summers June 12, 2025 19:44

astapleton added 3 commits June 12, 2025 13:56

Fix end of transaction check and account for no flushing when a simpl…

d099991

…e receiver/transmitter

Fix pin assignment for 533

142d4b1

SPI retains control of pins while disabled; documentation fixes

a794a62

usbalbin approved these changes Jul 7, 2025

View reviewed changes

astapleton merged commit d317c61 into master Jul 7, 2025
33 checks passed

astapleton deleted the as/spi-master branch July 7, 2025 22:32

	"Transfer buffer should not be non-zero length"
	"Transfer buffer should not be of zero length"

	"Transfer buffers should not be non-zero length"
	"Transfer buffers should not be of zero length"

spi: Add SPI master implementation #53

spi: Add SPI master implementation #53

Uh oh!

Conversation

astapleton commented May 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astapleton Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tytouf commented Jun 6, 2025

Uh oh!

tytouf commented Jun 6, 2025

Uh oh!

astapleton commented Jun 6, 2025

Uh oh!

tytouf commented Jun 7, 2025

Uh oh!

Wratox commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

usbalbin commented Jun 12, 2025

Uh oh!

usbalbin commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astapleton commented Jun 12, 2025

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!

astapleton commented Jun 12, 2025

Uh oh!

tytouf commented Jun 18, 2025

Uh oh!

usbalbin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

astapleton Jun 2, 2025 •

edited

Loading

Wratox commented Jun 12, 2025 •

edited

Loading

usbalbin commented Jun 12, 2025 •

edited

Loading