From 6bf95692936fd710456e4c78c0cefd72c14f3fbc Mon Sep 17 00:00:00 2001 From: Justin Beaurivage Date: Thu, 10 Nov 2022 11:15:44 -0500 Subject: [PATCH] Fix SPI-DMA transfers --- hal/src/dmac/async_api.rs | 26 +++---- hal/src/sercom/async_dma.rs | 34 ++++++++- hal/src/sercom/spi/async_api.rs | 120 ++++++++++++++++++++++++-------- 3 files changed, 138 insertions(+), 42 deletions(-) diff --git a/hal/src/dmac/async_api.rs b/hal/src/dmac/async_api.rs index 1e737443409..a35879fd316 100644 --- a/hal/src/dmac/async_api.rs +++ b/hal/src/dmac/async_api.rs @@ -1,4 +1,7 @@ -use crate::{dmac::waker::WAKERS, util::BitIter}; +use crate::{ + dmac::{waker::WAKERS, TriggerSource}, + util::BitIter, +}; use cortex_m::interrupt::InterruptNumber; use cortex_m_interrupt::NvicInterruptRegistration; @@ -57,6 +60,8 @@ mod thumbv6m { if wake { dmac.chctrla.modify(|_, w| w.enable().clear_bit()); + dmac.chctrlb + .modify(|_, w| w.trigsrc().variant(TriggerSource::DISABLE)); WAKERS[pend_channel as usize].wake(); } } @@ -156,31 +161,28 @@ mod thumbv7em { }; if wake { - dmac.channel[channel] - .chctrla - .modify(|_, w| w.enable().clear_bit()); + dmac.channel[channel].chctrla.modify(|_, w| { + w.enable().clear_bit(); + w.trigsrc().variant(TriggerSource::DISABLE) + }); WAKERS[channel].wake(); } } fn on_interrupt_0() { - const CHANNEL: usize = 0; - on_interrupt(CHANNEL); + on_interrupt(0); } fn on_interrupt_1() { - const CHANNEL: usize = 1; - on_interrupt(CHANNEL); + on_interrupt(1); } fn on_interrupt_2() { - const CHANNEL: usize = 2; - on_interrupt(CHANNEL); + on_interrupt(2); } fn on_interrupt_3() { - const CHANNEL: usize = 3; - on_interrupt(CHANNEL); + on_interrupt(3); } fn on_interrupt_other() { diff --git a/hal/src/sercom/async_dma.rs b/hal/src/sercom/async_dma.rs index 8b23fe0aa92..f0901f6bd39 100644 --- a/hal/src/sercom/async_dma.rs +++ b/hal/src/sercom/async_dma.rs @@ -83,11 +83,26 @@ unsafe impl Buffer for SercomPtr { } } +/// Perform a SERCOM DMA read with a provided `&mut [T]` pub(super) async fn read_dma( channel: &mut impl AnyChannel, sercom_ptr: SercomPtr, words: &mut [T], ) -> Result<(), Error> { + read_dma_buffer::<_, _, S>(channel, sercom_ptr, words).await +} + +/// Perform a SERCOM DMA read with a provided [`Buffer`] +pub(super) async fn read_dma_buffer( + channel: &mut impl AnyChannel, + sercom_ptr: SercomPtr, + buf: B, +) -> Result<(), Error> +where + T: Beat, + B: Buffer, + S: Sercom, +{ #[cfg(feature = "min-samd51g")] let trigger_action = TriggerAction::BURST; @@ -96,10 +111,11 @@ pub(super) async fn read_dma( channel .as_mut() - .transfer_future(sercom_ptr, words, S::DMA_RX_TRIGGER, trigger_action) + .transfer_future(sercom_ptr, buf, S::DMA_RX_TRIGGER, trigger_action) .await } +/// Perform a SERCOM DMA write with a provided `&[T]` pub(super) async fn write_dma( channel: &mut impl AnyChannel, sercom_ptr: SercomPtr, @@ -109,6 +125,20 @@ pub(super) async fn write_dma( // to words as long as the transfer hasn't completed. let words = ImmutableSlice::from_slice(words); + write_dma_buffer::<_, _, S>(channel, sercom_ptr, words).await +} + +/// Perform a SERCOM DMA write with a provided [`Buffer`] +pub(super) async fn write_dma_buffer( + channel: &mut impl AnyChannel, + sercom_ptr: SercomPtr, + buf: B, +) -> Result<(), Error> +where + T: Beat, + B: Buffer, + S: Sercom, +{ #[cfg(feature = "min-samd51g")] let trigger_action = TriggerAction::BURST; @@ -117,6 +147,6 @@ pub(super) async fn write_dma( channel .as_mut() - .transfer_future(words, sercom_ptr, S::DMA_TX_TRIGGER, trigger_action) + .transfer_future(buf, sercom_ptr, S::DMA_TX_TRIGGER, trigger_action) .await } diff --git a/hal/src/sercom/spi/async_api.rs b/hal/src/sercom/spi/async_api.rs index a121314adea..e03d2a4d72d 100644 --- a/hal/src/sercom/spi/async_api.rs +++ b/hal/src/sercom/spi/async_api.rs @@ -88,6 +88,18 @@ where _tx_channel: T, } +#[cfg(feature = "defmt")] +impl defmt::Format for SpiFuture +where + C: ValidConfig, + A: Capability, + N: InterruptNumber, +{ + fn format(&self, f: defmt::Formatter) { + defmt::write!(f, "SpiFuture defmt shim\n"); + } +} + /// Convenience type for a [`SpiFuture`] with RX and TX capabilities pub type SpiFutureDuplex = SpiFuture; @@ -461,13 +473,49 @@ mod impl_ehal { mod dma { use super::*; use crate::{ - dmac::{AnyChannel, Beat, ReadyFuture}, + dmac::{AnyChannel, Beat, Buffer, ReadyFuture}, sercom::{ - async_dma::{read_dma, write_dma, SercomPtr}, + async_dma::{read_dma, read_dma_buffer, write_dma, write_dma_buffer, SercomPtr}, spi::Size, }, }; + struct DummyBuffer { + word: T, + length: usize, + } + + /// Sink/source buffer to use for unidirectional SPI-DMA transfers. + /// + /// When reading/writing from a [`Duplex`] [`SpiFuture`] with DMA enabled, + /// we must always read and write the same number of words, regardless of + /// whether we care about the result (ie, for [`write`], we discard the read + /// words, whereas for [`read`], we must send a no-op word). + /// + /// This [`Buffer`] implementation provides a source/sink for a single word, + /// but with a variable length. + impl DummyBuffer { + fn new(word: T, length: usize) -> Self { + Self { word, length } + } + } + + unsafe impl Buffer for DummyBuffer { + type Beat = T; + + fn incrementing(&self) -> bool { + false + } + + fn buffer_len(&self) -> usize { + self.length + } + + fn dma_ptr(&mut self) -> *mut Self::Beat { + &mut self.word as *mut _ + } + } + impl SpiFuture where C: ValidConfig, @@ -580,39 +628,55 @@ mod dma { ) -> Result<(), Error> { assert!(read.is_some() || write.is_some()); - let dma_capable = if let (Some(r), Some(w)) = (read.as_ref(), write.as_ref()) { - r.len() == w.len() - } else { - true - }; + let spi_ptr = self.sercom_ptr(); + + match (read, write) { + (Some(r), Some(w)) => { + if r.len() == w.len() { + let tx_fut = write_dma::<_, S>(&mut self._rx_channel, spi_ptr.clone(), w); + let rx_fut = read_dma::<_, S>(&mut self._tx_channel, spi_ptr, r); + + let (read_res, write_res) = futures::join!(rx_fut, tx_fut); + write_res.and(read_res).map_err(Error::Dma)?; + } else { + // Short circuit if we got a length mismatch, as we have to send word by + // word + self.transfer_word_by_word(r, w).await?; + return Ok(()); + } + } - if dma_capable { - let maybe_source = [self.nop_word]; - // Use a random value as the sink buffer since we're just going to discard if we - // don't need it - let mut maybe_sink = [0xFF.as_()]; + (Some(r), None) => { + let source = DummyBuffer::new(self.nop_word, r.len()); + let rx_fut = read_dma::<_, S>(&mut self._rx_channel, spi_ptr.clone(), r); + let tx_fut = + write_dma_buffer::<_, _, S>(&mut self._tx_channel, spi_ptr, source); - let source = write.unwrap_or(&maybe_source); - let sink = read.unwrap_or(&mut maybe_sink); - let spi_ptr = self.sercom_ptr(); + let (read_res, write_res) = futures::join!(rx_fut, tx_fut); + write_res.and(read_res).map_err(Error::Dma)?; + } - let tx_fut = write_dma::<_, S>(&mut self._rx_channel, spi_ptr.clone(), source); - let rx_fut = read_dma::<_, S>(&mut self._tx_channel, spi_ptr, sink); + (None, Some(w)) => { + // Use a random value as the sink buffer since we're just going to discard the + // read words + let sink = DummyBuffer::new(0xFF.as_(), w.len()); + let rx_fut = + read_dma_buffer::<_, _, S>(&mut self._rx_channel, spi_ptr.clone(), sink); + let tx_fut = write_dma::<_, S>(&mut self._tx_channel, spi_ptr, w); - let (write_res, read_res) = futures::join!(rx_fut, tx_fut); - write_res.and(read_res).map_err(Error::Dma)?; - self.spi.read_flags_errors()?; + let (read_res, write_res) = futures::join!(rx_fut, tx_fut); + write_res.and(read_res).map_err(Error::Dma)?; + } - // Wait for transmission to complete. If we don't do that, we might return too - // early and disable the CS line, resulting in a corrupted transfer. - self.wait_flags(Flags::TXC).await; - } - // If there is a length mismatch, we need to do the transfer word by word. - else { - self.transfer_word_by_word(read.unwrap(), write.unwrap()) - .await?; + _ => panic!("Must provide at lease one buffer"), } + self.spi.read_flags_errors()?; + + // Wait for transmission to complete. If we don't do that, we might return too + // early and disable the CS line, resulting in a corrupted transfer. + self.wait_flags(Flags::TXC).await; + Ok(()) }