SPI pipelined writes are disabled

[jdeblese]

Got a weird one - using atsamd-hal 0.23.1 on a Feather M0 and compiling with release optimizations, I see extra transactions on the line between M0 and an SD card, as if the MISO bytes are being delayed by one word. I've verified the issue is in the write_word_by_word call by modifying it to use transfer_word_in_place instead of disabling the receiver, writing data, and then re-enabling the receiver.

Found this issue in the process of tracking down why embedded-sdmmc card intialization worked when compiled in debug mode but failed in release mode. I captured the SPI comms (had to write a new ExclusiveDevice that let me bring the CS line out to an extra pin, since the Feather SD CS line is not accessible) and tried to verify that embedded-sdmmc was deterministic by replaying the conversation, but was finding extra single-word read transactions in my capture. This led me to try modifying write_word_by_word.

Replaced this linked code with what's below, and the SD card finally initialized properly with a release build.
https://github.com/atsamd-rs/atsamd/blob/atsamd-hal-0.23.1/hal/src/sercom/spi/impl_ehal/mod.rs#L155-L171

    fn write_word_by_word(&mut self, words: &[Word<C>]) -> Result<(), Error> {
        for word in words {
            self.transfer_word_in_place(*word)?;
        }
        Ok(())
    }

Anyone else seen anything like this before? I checked the D21 errata but found no mention of any issue with SPI disable.

Edit: another theory, as I hit submit - is it possible that the original write_word_by_word was re-enabling the receiver before the write actually went out, and so was actually receiving the byte it was trying to ignore? That'd suggest that another fix would be to wait for the write to complete before re-enabling.

[jdeblese]

Also having problems with DMA-enabled SPI, and the fix was to add a flush_tx() call to write_dma() before re-enabling the receiver. Likely the same issue, so waiting for the transmission to complete is likely the best fix in both locations.

[pellico]

I had the same problem when upgrading feather_m0 create.
This behavior is due this commit

Original code was:

 #[inline]
    fn write_word_by_word(&mut self, words: &[Word<C>]) -> Result<(), Error> {
        // Ignore RX buffer overflows by disabling the receiver
        self.config.as_mut().regs.rx_disable();

        for word in words {
            self.block_on_flags(Flags::DRE)?;
            unsafe {
                self.write_data(word.as_());
            }
        }
        self.flush_tx(); ///<-------- This has been removed

        // Reenable receiver only if necessary
        if D::RX_ENABLE {
            self.config.as_mut().regs.rx_enable();
        }
        Ok(())
    }
}

self.flush_tx(); has been removed in order to allow pipeling of operation.

By skipping the wait on transmit completion the re-enabling is happening while transmission and consequently the reading (full duplex is assumed) is still in progress and later it is not read. Therefore when you perform other operation e.g. transfer_word_in_place this start always with a write followed by wait for read completion but this is already set by previous transaction and then you read the previous value.

This explains the shift by one.

This is not happening with transfer_word_in_place because this function has to wait to read back the answer of SPI device and the read of last write is not skipped.

From my analysis this is NOT a bug because the modification was done in order to support pipelined operation.

See the explanation of flushing in atsamd documentation

The idea behind is that developer could perform multiple pipelined writes where you don't care the read back and then you can call flush to re synchronize the data stream.

This is described in embedded_hal spec

SpiDevice trait requires flushing at the end of every transaction.

Typically SpiDevice trait implementation use SpiBus that provide a flush function

Why we see still the issue when I use different transaction ?

in the atsamd SpiBus::flush is implemented like that:

// Implementations for SPIs in either Master or Slave mode.
impl<P, M, C, D, R, T> Spi<Config<P, M, C>, D, R, T>
.....
   /// Wait on a TXC while ignoring buffer overflow errors.
    #[inline]
    fn flush_tx(&mut self) {
        // Ignore buffer overflow errors
        let _ = self.block_on_flags(Flags::TXC);
    }

.......

impl<P, M, C> SpiBus<Word<C>> for Spi<Config<P, M, C>, Duplex>
.....
#[inline]
    fn flush(&mut self) -> Result<(), Error> {
        self.flush_tx();
        Ok(())
    }

So it is clear that flush function wait the write is completed but is not clearing the receive FIFO.

Is this complaint with embedded_hal spec about flushing?

My answer is YES because the description requires the transmit buffer is empty and there is no activity on bus.

However the spec doesn't forbid to empty the receive FIFO therefore i propose the following change:

#[inline]
    fn flush(&mut self) -> Result<(), Error> {
        self.flush_tx();
   
        if D::RX_ENABLE {
             self.flush_rx();
        }
              Ok(())

TL;DR

The present behaviur is expected and fulfilling the embedded_hal spec.
The issue is in the definition of flush in the spec.
It it possible to fix the issue by changing flush function without violating the spec

[jdeblese]

Thank you for sharing those links, I had of course overlooked that section on flushing.

However, I still have a concern with how the support for pipelined transactions is implemented - as written it precludes using SpiDevice::transaction() with a Write operation followed by a Read operation because there's no flush in between. embedded-hal-bus's ExclusiveDevice::transaction() will call SpiBus::write(), which for the atsamd calls write_word_by_word() and doesn't subsequently call flush.

An alternative to flushing inside atsamd-hal would be to call flush in SpiDevice::transaction between a Write and a Read. This does seem to be a gap in the embedded-hal spec, because their section on flushing does not mention what the behavior should be with mixed write+read transactions.

Downstream users could define transactions as [Write, DelayNs(0), Read] as a workaround, but that'll panic when using ExclusiveDevice with NoDelay.

Edit: And, embedded-hal could reasonably argue that flushing is just about waiting for completion, and that a write should in principle never result in anything ending up in the receive buffer regardless of if it's pipelined or not.

[pellico]

@jdeblese
I have opened a in issue in embedde-hal project and it seems that there is a spec misunderstanding.

Please refer to detailed explanation at this link: rust-embedded/embedded-hal#739 (comment)

So I think the solution is to restore the flush_tx before the re-enabling of receiver

[jbeaurivage]

#1010 fixes the immediate issue. However, this disables pipelined writes. A better fix is proposed in the PR; it would nice to implement it.