From 6d02f4485b2d81a7895a1349cecdf180ccb4eda5 Mon Sep 17 00:00:00 2001 From: Andelf Date: Mon, 30 Sep 2024 16:29:35 +0800 Subject: [PATCH 1/5] Revert "fix(spi): add busy loop waiting for tx finish" This reverts commit 32fb3bdc74b727a7d34cc21908e1bac23101a80b. --- src/spi/mod.rs | 70 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 57 insertions(+), 13 deletions(-) diff --git a/src/spi/mod.rs b/src/spi/mod.rs index 80e5f3a..7a8f2cf 100644 --- a/src/spi/mod.rs +++ b/src/spi/mod.rs @@ -5,11 +5,13 @@ //! - SPI_CS_SELECT: v53, v68, //! - SPI_SUPPORT_DIRECTIO: v53, v68 +use core::future::poll_fn; use core::marker::PhantomData; use core::ptr; +use core::sync::atomic::{compiler_fence, Ordering}; +use core::task::Poll; use embassy_futures::join::join; -use embassy_futures::yield_now; use embassy_hal_internal::{into_ref, Peripheral, PeripheralRef}; use embassy_sync::waitqueue::AtomicWaker; // re-export @@ -18,9 +20,11 @@ pub use embedded_hal::spi::{Mode, MODE_0, MODE_1, MODE_2, MODE_3}; use self::consts::*; use crate::dma::{self, word, ChannelAndRequest}; use crate::gpio::AnyPin; +use crate::interrupt::typelevel::Interrupt as _; use crate::mode::{Async, Blocking, Mode as PeriMode}; pub use crate::pac::spi::vals::{AddrLen, AddrPhaseFormat, DataPhaseFormat, TransMode}; use crate::time::Hertz; +use crate::{interrupt, pac}; #[cfg(any(hpm53, hpm68, hpm6e))] mod consts { @@ -33,7 +37,34 @@ mod consts { pub const FIFO_SIZE: usize = 4; } -// NOTE: SPI end interrupt is not working under DMA mode +// - MARK: interrupt handler + +/// Interrupt handler. +pub struct InterruptHandler { + _phantom: PhantomData, +} + +impl interrupt::typelevel::Handler for InterruptHandler { + unsafe fn on_interrupt() { + on_interrupt(T::info().regs, T::state()); + + // PLIC ack is handled by typelevel Handler + } +} + +unsafe fn on_interrupt(r: pac::spi::Spi, s: &'static State) { + let status = r.intr_st().read(); + + defmt::info!("in irq"); + + if status.endint() { + s.waker.wake(); + + r.intr_en().modify(|w| w.set_endinten(false)); + } + + r.intr_st().write_value(status); // W1C +} // - MARK: Helper enums @@ -378,6 +409,7 @@ impl<'d> Spi<'d, Async> { sclk: impl Peripheral

> + 'd, mosi: impl Peripheral

> + 'd, miso: impl Peripheral

> + 'd, + _irq: impl interrupt::typelevel::Binding> + 'd, tx_dma: impl Peripheral

> + 'd, rx_dma: impl Peripheral

> + 'd, config: Config, @@ -410,6 +442,7 @@ impl<'d> Spi<'d, Async> { peri: impl Peripheral

+ 'd, sclk: impl Peripheral

> + 'd, miso: impl Peripheral

> + 'd, + _irq: impl interrupt::typelevel::Binding> + 'd, rx_dma: impl Peripheral

> + 'd, config: Config, ) -> Self { @@ -441,6 +474,7 @@ impl<'d> Spi<'d, Async> { peri: impl Peripheral

+ 'd, sclk: impl Peripheral

> + 'd, mosi: impl Peripheral

> + 'd, + _irq: impl interrupt::typelevel::Binding> + 'd, tx_dma: impl Peripheral

> + 'd, config: Config, ) -> Self { @@ -499,12 +533,19 @@ impl<'d> Spi<'d, Async> { return Ok(()); } + defmt::info!("write spi"); + let r = self.info.regs; + let s = self.state; self.set_word_size(W::CONFIG); self.configure_transfer(data.len(), 0, &TransferConfig::default())?; + r.intr_en().modify(|w| { + w.set_endinten(true); + }); + r.ctrl().modify(|w| w.set_txdmaen(true)); let tx_dst = r.data().as_ptr() as *mut W; @@ -514,13 +555,17 @@ impl<'d> Spi<'d, Async> { tx_f.await; - r.ctrl().modify(|w| w.set_txdmaen(false)); + poll_fn(move |cx| { + if r.intr_en().read().endinten() { + return Poll::Pending; + } else { + s.waker.register(cx.waker()); + return Poll::Ready(()); + } + }) + .await; - // NOTE: SPI end(finish) interrupt is not working under DMA mode, a busy-loop wait is necessary for TX mode. - // The same goes for `transfer` fn. - while r.status().read().spiactive() { - yield_now().await; - } + r.ctrl().modify(|w| w.set_txdmaen(false)); Ok(()) } @@ -584,11 +629,6 @@ impl<'d> Spi<'d, Async> { w.set_txdmaen(false); }); - // See `write` - while r.status().read().spiactive() { - yield_now().await; - } - Ok(()) } @@ -639,6 +679,10 @@ impl<'d, M: PeriMode> Spi<'d, M> { this.enable_and_configure(&config).unwrap(); + unsafe { + T::Interrupt::enable(); + } + this } From 6861f4d64d03e68ef64152eb13e9a1943740a100 Mon Sep 17 00:00:00 2001 From: Andelf Date: Mon, 30 Sep 2024 17:40:23 +0800 Subject: [PATCH 2/5] wip(spi): try to make spi end irq work --- src/dma/util.rs | 10 ++++++++++ src/dma/v2.rs | 24 +++++++++++++++++++++++ src/spi/mod.rs | 51 ++++++++++++++++++++++++++++++++----------------- 3 files changed, 68 insertions(+), 17 deletions(-) diff --git a/src/dma/util.rs b/src/dma/util.rs index 5aaca57..28bec39 100644 --- a/src/dma/util.rs +++ b/src/dma/util.rs @@ -48,6 +48,16 @@ impl<'d> ChannelAndRequest<'d> { Transfer::new_write_raw(&mut self.channel, self.request, buf, peri_addr, options) } + pub unsafe fn write_raw_with_spi_fix<'a, W: Word>( + &'a mut self, + buf: *const W, + buf_len: usize, + peri_addr: *mut W, + options: TransferOptions, + ) -> Transfer<'a> { + Transfer::new_write_raw_with_spi_fix(&mut self.channel, self.request, buf, buf_len, peri_addr, options) + } + #[allow(dead_code)] pub unsafe fn write_repeated<'a, W: Word>( &'a mut self, diff --git a/src/dma/v2.rs b/src/dma/v2.rs index ab7f6d3..c473960 100644 --- a/src/dma/v2.rs +++ b/src/dma/v2.rs @@ -441,6 +441,30 @@ impl<'a> Transfer<'a> { ) } + /// Create a new write DMA transfer (memory to peripheral), using raw pointers. + pub unsafe fn new_write_raw_with_spi_fix( + channel: impl Peripheral

+ 'a, + request: Request, + buf: *const W, + fixed_buf_len: usize, + peri_addr: *mut W, + options: TransferOptions, + ) -> Self { + into_ref!(channel); + + Self::new_inner( + channel.map_into(), + request, + Dir::MemoryToPeripheral, + peri_addr as *const u32, + buf as *mut u32, + fixed_buf_len, + true, + W::size(), + options, + ) + } + /// Create a new write DMA transfer (memory to peripheral), writing the same value repeatedly. pub unsafe fn new_write_repeated( channel: impl Peripheral

+ 'a, diff --git a/src/spi/mod.rs b/src/spi/mod.rs index 7a8f2cf..8017e7c 100644 --- a/src/spi/mod.rs +++ b/src/spi/mod.rs @@ -8,7 +8,6 @@ use core::future::poll_fn; use core::marker::PhantomData; use core::ptr; -use core::sync::atomic::{compiler_fence, Ordering}; use core::task::Poll; use embassy_futures::join::join; @@ -16,6 +15,7 @@ use embassy_hal_internal::{into_ref, Peripheral, PeripheralRef}; use embassy_sync::waitqueue::AtomicWaker; // re-export pub use embedded_hal::spi::{Mode, MODE_0, MODE_1, MODE_2, MODE_3}; +use futures_util::future::{select, Either}; use self::consts::*; use crate::dma::{self, word, ChannelAndRequest}; @@ -533,8 +533,6 @@ impl<'d> Spi<'d, Async> { return Ok(()); } - defmt::info!("write spi"); - let r = self.info.regs; let s = self.state; @@ -549,21 +547,40 @@ impl<'d> Spi<'d, Async> { r.ctrl().modify(|w| w.set_txdmaen(true)); let tx_dst = r.data().as_ptr() as *mut W; - let mut opts = dma::TransferOptions::default(); - opts.burst = dma::Burst::from_size(FIFO_SIZE / 2); - let tx_f = unsafe { self.tx_dma.as_mut().unwrap().write(data, tx_dst, opts) }; - - tx_f.await; + let opts = dma::TransferOptions::default(); // BUG: Busrt size must be 1(default) for SPI. or else END interrupt will not be triggered. + + //let tx_f = unsafe { self.tx_dma.as_mut().unwrap().write(data, tx_dst, opts) }; + // BUG: No idea why "+1" is needed here, but without it, END interrupt will not be triggered. + // BUG: "+n"(n>1) also triggers END interrupt, but the data seems not correct. + let tx_f = unsafe { + self.tx_dma + .as_mut() + .unwrap() + .write_raw_with_spi_fix(data.as_ptr(), data.len() + 1, tx_dst, opts) + }; - poll_fn(move |cx| { + let end_f = poll_fn(move |cx| { + s.waker.register(cx.waker()); if r.intr_en().read().endinten() { return Poll::Pending; } else { - s.waker.register(cx.waker()); return Poll::Ready(()); } - }) - .await; + }); + + // NOTE: if "+n"(n>1) is used, tx_f never finishes, use select instead of join + // join only works for "+1" + join(end_f, tx_f).await; + + /* + match select(end_f, tx_f).await { + Either::Left((_v, _)) => (), + Either::Right((_v, end_f)) => { + end_f.await; + defmt::println!("end f"); + } + } + */ r.ctrl().modify(|w| w.set_txdmaen(false)); @@ -645,8 +662,10 @@ impl<'d> Spi<'d, Async> { /// In-place bidirectional transfer, using DMA. /// /// This writes the contents of `data` on MOSI, and puts the received data on MISO in `data`, at the same time. - pub async fn transfer_in_place(&mut self, data: &mut [W], config: &TransferConfig) -> Result<(), Error> { - self.transfer_inner(data, data, config).await + pub async fn transfer_in_place(&mut self, data: &mut [W]) -> Result<(), Error> { + let mut config = TransferConfig::default(); + config.transfer_mode = TransMode::WRITE_READ_TOGETHER; + self.transfer_inner(data, data, &config).await } } @@ -1196,8 +1215,6 @@ impl<'d, W: Word> embedded_hal_async::spi::SpiBus for Spi<'d, Async> { } async fn transfer_in_place(&mut self, words: &mut [W]) -> Result<(), Self::Error> { - let mut options = TransferConfig::default(); - options.transfer_mode = TransMode::WRITE_READ_TOGETHER; - self.transfer_in_place(words, &options).await + self.transfer_in_place(words).await } } From 388334976f0ead47ad3d5c80ca0ecf6b035c576e Mon Sep 17 00:00:00 2001 From: Andelf Date: Mon, 30 Sep 2024 17:48:50 +0800 Subject: [PATCH 3/5] fixup! wip(spi): try to make spi end irq work --- src/spi/mod.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/spi/mod.rs b/src/spi/mod.rs index 8017e7c..9f59552 100644 --- a/src/spi/mod.rs +++ b/src/spi/mod.rs @@ -55,8 +55,6 @@ impl interrupt::typelevel::Handler for InterruptHandl unsafe fn on_interrupt(r: pac::spi::Spi, s: &'static State) { let status = r.intr_st().read(); - defmt::info!("in irq"); - if status.endint() { s.waker.wake(); From e5c495c8f9208b6fff92757e7fbe375fb533df19 Mon Sep 17 00:00:00 2001 From: Andelf Date: Tue, 1 Oct 2024 08:54:18 +0800 Subject: [PATCH 4/5] wip(spi): refine async write impl --- src/dma/util.rs | 10 ---------- src/dma/v2.rs | 35 +++++++---------------------------- src/spi/mod.rs | 42 +++++++++++++++++++++--------------------- 3 files changed, 28 insertions(+), 59 deletions(-) diff --git a/src/dma/util.rs b/src/dma/util.rs index 28bec39..5aaca57 100644 --- a/src/dma/util.rs +++ b/src/dma/util.rs @@ -48,16 +48,6 @@ impl<'d> ChannelAndRequest<'d> { Transfer::new_write_raw(&mut self.channel, self.request, buf, peri_addr, options) } - pub unsafe fn write_raw_with_spi_fix<'a, W: Word>( - &'a mut self, - buf: *const W, - buf_len: usize, - peri_addr: *mut W, - options: TransferOptions, - ) -> Transfer<'a> { - Transfer::new_write_raw_with_spi_fix(&mut self.channel, self.request, buf, buf_len, peri_addr, options) - } - #[allow(dead_code)] pub unsafe fn write_repeated<'a, W: Word>( &'a mut self, diff --git a/src/dma/v2.rs b/src/dma/v2.rs index c473960..ad2029c 100644 --- a/src/dma/v2.rs +++ b/src/dma/v2.rs @@ -40,7 +40,7 @@ pub struct TransferOptions { impl Default for TransferOptions { fn default() -> Self { Self { - burst: Burst::Liner(0), + burst: Burst::Exponential(0), // 1 transfer priority: false, circular: false, half_transfer_irq: false, @@ -53,8 +53,6 @@ impl Default for TransferOptions { #[derive(Debug, Copy, Clone, PartialEq, Eq)] #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub enum Burst { - // 0:1transfer; 0xf: 16 transfer - Liner(u8), /* 0x0: 1 transfer 0x1: 2 transfers @@ -69,6 +67,9 @@ pub enum Burst { 0xa: 1024 transfers */ Exponential(u8), + // For BURSTOPT = 1 + // 0:1transfer; 0xf: 16 transfer + Liner(u8), } impl Burst { @@ -273,10 +274,12 @@ impl AnyChannel { w.set_infiniteloop(options.circular); // false: Use burst mode // true: Send all data at once - w.set_handshakeopt(false); + w.set_handshakeopt(false); // always false in sdk w.set_burstopt(options.burst.burstopt()); w.set_priority(options.priority); + + // In DMA handshake case, source burst size must be 1 transfer, that is 0 w.set_srcburstsize(options.burst.burstsize()); w.set_srcwidth(src_width.width()); w.set_dstwidth(dst_width.width()); @@ -441,30 +444,6 @@ impl<'a> Transfer<'a> { ) } - /// Create a new write DMA transfer (memory to peripheral), using raw pointers. - pub unsafe fn new_write_raw_with_spi_fix( - channel: impl Peripheral

+ 'a, - request: Request, - buf: *const W, - fixed_buf_len: usize, - peri_addr: *mut W, - options: TransferOptions, - ) -> Self { - into_ref!(channel); - - Self::new_inner( - channel.map_into(), - request, - Dir::MemoryToPeripheral, - peri_addr as *const u32, - buf as *mut u32, - fixed_buf_len, - true, - W::size(), - options, - ) - } - /// Create a new write DMA transfer (memory to peripheral), writing the same value repeatedly. pub unsafe fn new_write_repeated( channel: impl Peripheral

+ 'a, diff --git a/src/spi/mod.rs b/src/spi/mod.rs index 9f59552..3d6bdbc 100644 --- a/src/spi/mod.rs +++ b/src/spi/mod.rs @@ -8,6 +8,7 @@ use core::future::poll_fn; use core::marker::PhantomData; use core::ptr; +use core::sync::atomic::compiler_fence; use core::task::Poll; use embassy_futures::join::join; @@ -55,6 +56,8 @@ impl interrupt::typelevel::Handler for InterruptHandl unsafe fn on_interrupt(r: pac::spi::Spi, s: &'static State) { let status = r.intr_st().read(); + defmt::println!("in irq"); + if status.endint() { s.waker.wake(); @@ -545,16 +548,11 @@ impl<'d> Spi<'d, Async> { r.ctrl().modify(|w| w.set_txdmaen(true)); let tx_dst = r.data().as_ptr() as *mut W; - let opts = dma::TransferOptions::default(); // BUG: Busrt size must be 1(default) for SPI. or else END interrupt will not be triggered. - - //let tx_f = unsafe { self.tx_dma.as_mut().unwrap().write(data, tx_dst, opts) }; - // BUG: No idea why "+1" is needed here, but without it, END interrupt will not be triggered. - // BUG: "+n"(n>1) also triggers END interrupt, but the data seems not correct. let tx_f = unsafe { self.tx_dma .as_mut() .unwrap() - .write_raw_with_spi_fix(data.as_ptr(), data.len() + 1, tx_dst, opts) + .write(data, tx_dst, dma::TransferOptions::default()) }; let end_f = poll_fn(move |cx| { @@ -566,21 +564,12 @@ impl<'d> Spi<'d, Async> { } }); - // NOTE: if "+n"(n>1) is used, tx_f never finishes, use select instead of join - // join only works for "+1" - join(end_f, tx_f).await; + end_f.await; - /* - match select(end_f, tx_f).await { - Either::Left((_v, _)) => (), - Either::Right((_v, end_f)) => { - end_f.await; - defmt::println!("end f"); - } - } - */ + compiler_fence(core::sync::atomic::Ordering::SeqCst); r.ctrl().modify(|w| w.set_txdmaen(false)); + drop(tx_f); Ok(()) } @@ -696,6 +685,7 @@ impl<'d, M: PeriMode> Spi<'d, M> { this.enable_and_configure(&config).unwrap(); + T::Interrupt::set_priority(interrupt::Priority::P1); unsafe { T::Interrupt::enable(); } @@ -788,6 +778,10 @@ impl<'d, M: PeriMode> Spi<'d, M> { return Err(Error::InvalidArgument); } + if config.transfer_mode == TransMode::WRITE_READ_TOGETHER && write_len != read_len { + return Err(Error::InvalidArgument); + } + let r = self.info.regs; // SPI format init @@ -806,18 +800,24 @@ impl<'d, M: PeriMode> Spi<'d, M> { w.set_addrfmt(config.addr_phase); w.set_dualquad(config.data_phase); w.set_tokenen(false); - #[cfg(not(ip_feature_spi_new_trans_count))] + // #[cfg(not(ip_feature_spi_new_trans_count))] match config.transfer_mode { TransMode::WRITE_READ_TOGETHER | TransMode::READ_DUMMY_WRITE | TransMode::WRITE_DUMMY_READ | TransMode::READ_WRITE | TransMode::WRITE_READ => { + w.set_wrtrancnt((write_len as u16 - 1) & 0x1ff); + w.set_rdtrancnt((read_len as u16 - 1) & 0x1ff); + } + TransMode::WRITE_ONLY | TransMode::DUMMY_WRITE => { w.set_wrtrancnt(write_len as u16 - 1); + w.set_rdtrancnt(0x1ff); + } + TransMode::READ_ONLY | TransMode::DUMMY_READ => { w.set_rdtrancnt(read_len as u16 - 1); + w.set_wrtrancnt(0x1ff); } - TransMode::WRITE_ONLY | TransMode::DUMMY_WRITE => w.set_wrtrancnt(write_len as u16 - 1), - TransMode::READ_ONLY | TransMode::DUMMY_READ => w.set_rdtrancnt(read_len as u16 - 1), TransMode::NO_DATA => (), _ => (), } From 42f69049b38ed09e937d731339761755f16b83db Mon Sep 17 00:00:00 2001 From: Andelf Date: Tue, 1 Oct 2024 09:04:48 +0800 Subject: [PATCH 5/5] wip(spi): add bug demo --- examples/hpm5300evk/src/bin/async_spi_bug.rs | 50 ++++++++++++++++++++ src/spi/mod.rs | 2 - 2 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 examples/hpm5300evk/src/bin/async_spi_bug.rs diff --git a/examples/hpm5300evk/src/bin/async_spi_bug.rs b/examples/hpm5300evk/src/bin/async_spi_bug.rs new file mode 100644 index 0000000..60ba7ff --- /dev/null +++ b/examples/hpm5300evk/src/bin/async_spi_bug.rs @@ -0,0 +1,50 @@ +#![no_main] +#![no_std] +#![feature(type_alias_impl_trait)] +#![feature(impl_trait_in_assoc_type)] +#![feature(abi_riscv_interrupt)] + +use embassy_time::{Duration, Timer}; +use hal::gpio::{Level, Output}; +use hal::peripherals; +use hpm_hal::bind_interrupts; +use hpm_hal::time::Hertz; +use {defmt_rtt as _, hpm_hal as hal}; + +bind_interrupts!(struct Irqs { + SPI1 => hal::spi::InterruptHandler; +}); + +#[embassy_executor::main(entry = "hpm_hal::entry")] +async fn main(_spawner: embassy_executor::Spawner) -> ! { + let config = hal::Config::default(); + let p = hal::init(config); + + let mut config = hal::spi::Config::default(); + config.frequency = Hertz::mhz(2); + let mut spi = hal::spi::Spi::new_txonly(p.SPI1, p.PA27, p.PA29, Irqs, p.HDMA_CH0, config); + + defmt::info!("go !"); + + spi.write(&[0xaa_u8; 1]).await.unwrap(); // lower values fail. larger(10 or so) values work + + // The following lines are never reached + + defmt::println!("bytes sent"); + + let mut led = Output::new(p.PA23, Level::Low, Default::default()); + loop { + led.set_high(); + Timer::after(Duration::from_millis(500)).await; + led.set_low(); + Timer::after(Duration::from_millis(500)).await; + defmt::println!("tick"); + } +} + +#[panic_handler] +unsafe fn panic(info: &core::panic::PanicInfo) -> ! { + defmt::println!("panic!\n {}", defmt::Debug2Format(info)); + + loop {} +} diff --git a/src/spi/mod.rs b/src/spi/mod.rs index 3d6bdbc..e9afba0 100644 --- a/src/spi/mod.rs +++ b/src/spi/mod.rs @@ -56,8 +56,6 @@ impl interrupt::typelevel::Handler for InterruptHandl unsafe fn on_interrupt(r: pac::spi::Spi, s: &'static State) { let status = r.intr_st().read(); - defmt::println!("in irq"); - if status.endint() { s.waker.wake();