From 5317fe5bda90c42c29d0d4a48db83f599eff06d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Thu, 14 Nov 2024 13:14:28 +0100 Subject: [PATCH] Configure burst transfer with a non-bool for future chip support --- esp-hal/src/dma/buffers.rs | 115 ++++++++++++++++++++++++++----------- esp-hal/src/dma/gdma.rs | 8 +-- esp-hal/src/dma/mod.rs | 32 +++++------ esp-hal/src/dma/pdma.rs | 12 ++-- 4 files changed, 108 insertions(+), 59 deletions(-) diff --git a/esp-hal/src/dma/buffers.rs b/esp-hal/src/dma/buffers.rs index 53f5a19a2b..a1089d6ff7 100644 --- a/esp-hal/src/dma/buffers.rs +++ b/esp-hal/src/dma/buffers.rs @@ -5,16 +5,49 @@ use crate::soc::is_slice_in_dram; #[cfg(esp32s3)] use crate::soc::is_slice_in_psram; +/// Burst transfer configuration. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +pub enum BurstTransfer { + /// Burst mode is disabled. + Disabled, + + /// Burst mode is enabled. + Enabled, +} + +impl BurstTransfer { + pub(super) fn is_burst_enabled(self) -> bool { + !matches!(self, Self::Disabled) + } +} + +/// Maximum length of a burst transfer. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] +pub enum TransferDirection { + /// DMA transfer from peripheral or external memory to memory. + In, + /// DMA transfer from memory to peripheral or external memory. + Out, +} + /// Holds all the information needed to configure a DMA channel for a transfer. pub struct Preparation { /// The descriptor the DMA will start from. pub start: *mut DmaDescriptor, - /// Block size for PSRAM transfers - #[cfg_attr(not(esp32s3), allow(dead_code))] - pub block_size: Option, + /// The direction of the DMA transfer. + pub direction: TransferDirection, - /// Specifies whether the data should be transferred in burst mode. + /// Block size for PSRAM transfers. + /// + /// The implementation of the buffer must provide a block size if the data + /// is in PSRAM. + #[cfg(esp32s3)] + pub external_memory_block_size: Option, + + /// Configures the DMA to transfer data in bursts. /// /// The implementation of the buffer must ensure that burst mode is only /// enabled when alignment requirements are met. @@ -22,8 +55,7 @@ pub struct Preparation { /// There are no additional alignment requirements for TX burst transfers, /// but RX transfers require all descriptors to have buffer pointers and /// sizes that are a multiple of 4 (word aligned). - // TODO: currently unused, buffers should not hardcode their preference. - pub burst: bool, + pub burst_transfer: BurstTransfer, /// Configures the "check owner" feature of the DMA channel. /// @@ -328,9 +360,11 @@ unsafe impl DmaTxBuffer for DmaTxBuf { Preparation { start: self.descriptors.head(), - block_size: self.block_size, - // This is TX, the DMA channel is free to do a burst transfer. - burst: true, + direction: TransferDirection::Out, + #[cfg(esp32s3)] + external_memory_block_size: self.block_size, + // TODO: support burst transfers. + burst_transfer: BurstTransfer::Disabled, check_owner: None, } } @@ -477,11 +511,16 @@ unsafe impl DmaRxBuffer for DmaRxBuf { Preparation { start: self.descriptors.head(), - block_size: None, - // DmaRxBuf doesn't currently enforce the alignment requirements required for bursting. - // In the future, it could either enforce the alignment or calculate if the alignment - // requirements happen to be met. - burst: false, + direction: TransferDirection::In, + + // TODO: support external memory access. + #[cfg(esp32s3)] + external_memory_block_size: None, + + // TODO: DmaRxBuf doesn't currently enforce the alignment requirements required for + // bursting. In the future, it could either enforce the alignment or + // calculate if the alignment requirements happen to be met. + burst_transfer: BurstTransfer::Disabled, check_owner: None, } } @@ -606,10 +645,14 @@ unsafe impl DmaTxBuffer for DmaRxTxBuf { Preparation { start: self.tx_descriptors.head(), - block_size: None, // TODO: support block size! + direction: TransferDirection::Out, + + // TODO: support external memory access. + #[cfg(esp32s3)] + external_memory_block_size: None, - // This is TX, the DMA channel is free to do a burst transfer. - burst: true, + // TODO: This is TX, the DMA channel is free to do a burst transfer. + burst_transfer: BurstTransfer::Disabled, check_owner: None, } } @@ -637,11 +680,15 @@ unsafe impl DmaRxBuffer for DmaRxTxBuf { Preparation { start: self.rx_descriptors.head(), - block_size: None, // TODO: support block size! + direction: TransferDirection::In, - // DmaRxTxBuf doesn't currently enforce the alignment requirements required for + // TODO: support external memory access. + #[cfg(esp32s3)] + external_memory_block_size: None, + + // TODO: DmaRxTxBuf doesn't currently enforce the alignment requirements required for // bursting. - burst: false, + burst_transfer: BurstTransfer::Disabled, check_owner: None, } } @@ -778,11 +825,15 @@ unsafe impl DmaRxBuffer for DmaRxStreamBuf { } Preparation { start: self.descriptors.as_mut_ptr(), - block_size: None, + direction: TransferDirection::In, - // DmaRxStreamBuf doesn't currently enforce the alignment requirements required for - // bursting. - burst: false, + // TODO: support external memory access. + #[cfg(esp32s3)] + external_memory_block_size: None, + + // TODO: DmaRxStreamBuf doesn't currently enforce the alignment requirements required + // for bursting. + burst_transfer: BurstTransfer::Disabled, // Whilst we give ownership of the descriptors the DMA, the correctness of this buffer // implementation doesn't rely on the DMA checking for descriptor ownership. @@ -992,10 +1043,10 @@ unsafe impl DmaTxBuffer for EmptyBuf { #[allow(unused_unsafe)] // stable requires unsafe, nightly complains about it Preparation { start: unsafe { core::ptr::addr_of_mut!(EMPTY).cast() }, - block_size: None, - - // This is TX, the DMA channel is free to do a burst transfer. - burst: true, + direction: TransferDirection::Out, + #[cfg(esp32s3)] + external_memory_block_size: None, + burst_transfer: BurstTransfer::Disabled, // As we don't give ownership of the descriptor to the DMA, it's important that the DMA // channel does *NOT* check for ownership, otherwise the channel will return an error. @@ -1023,10 +1074,10 @@ unsafe impl DmaRxBuffer for EmptyBuf { #[allow(unused_unsafe)] // stable requires unsafe, nightly complains about it Preparation { start: unsafe { core::ptr::addr_of_mut!(EMPTY).cast() }, - block_size: None, - - // As much as bursting is meaningless here, the descriptor does meet the requirements. - burst: true, + direction: TransferDirection::In, + #[cfg(esp32s3)] + external_memory_block_size: None, + burst_transfer: BurstTransfer::Disabled, // As we don't give ownership of the descriptor to the DMA, it's important that the DMA // channel does *NOT* check for ownership, otherwise the channel will return an error. diff --git a/esp-hal/src/dma/gdma.rs b/esp-hal/src/dma/gdma.rs index f5b4efd909..8e9eca19d6 100644 --- a/esp-hal/src/dma/gdma.rs +++ b/esp-hal/src/dma/gdma.rs @@ -169,10 +169,10 @@ impl RegisterAccess for ChannelTxImpl { conf0.modify(|_, w| w.out_rst().clear_bit()); } - fn set_burst_mode(&self, burst_mode: bool) { + fn set_burst_mode(&self, burst_mode: BurstTransfer) { self.ch() .out_conf0() - .modify(|_, w| w.out_data_burst_en().bit(burst_mode)); + .modify(|_, w| w.out_data_burst_en().bit(burst_mode.is_burst_enabled())); } fn set_descr_burst_mode(&self, burst_mode: bool) { @@ -393,10 +393,10 @@ impl RegisterAccess for ChannelRxImpl { conf0.modify(|_, w| w.in_rst().clear_bit()); } - fn set_burst_mode(&self, burst_mode: bool) { + fn set_burst_mode(&self, burst_mode: BurstTransfer) { self.ch() .in_conf0() - .modify(|_, w| w.in_data_burst_en().bit(burst_mode)); + .modify(|_, w| w.in_data_burst_en().bit(burst_mode.is_burst_enabled())); } fn set_descr_burst_mode(&self, burst_mode: bool) { diff --git a/esp-hal/src/dma/mod.rs b/esp-hal/src/dma/mod.rs index f74362da91..d3389c881a 100644 --- a/esp-hal/src/dma/mod.rs +++ b/esp-hal/src/dma/mod.rs @@ -1819,7 +1819,9 @@ where ) -> Result<(), DmaError> { let preparation = buffer.prepare(); - self.rx_impl.set_burst_mode(false); + debug_assert_eq!(preparation.direction, TransferDirection::In); + + self.rx_impl.set_burst_mode(preparation.burst_transfer); self.rx_impl.set_descr_burst_mode(true); self.rx_impl.set_check_owner(preparation.check_owner); @@ -2058,8 +2060,9 @@ where peri: DmaPeripheral, chain: &DescriptorChain, ) -> Result<(), DmaError> { - // TODO: based on the ESP32-S3 TRM the alignment check is not needed for TX! - // for esp32s3 we check each descriptor buffer that points to psram for + // Based on the ESP32-S3 TRM the alignment check is not needed for TX + + // For esp32s3 we check each descriptor buffer that points to PSRAM for // alignment and writeback the cache for that buffer #[cfg(esp32s3)] for des in chain.descriptors.iter() { @@ -2095,20 +2098,15 @@ where buffer: &mut BUF, ) -> Result<(), DmaError> { let preparation = buffer.prepare(); - cfg_if::cfg_if!( - if #[cfg(esp32s3)] { - if let Some(block_size) = preparation.block_size { - self.set_ext_mem_block_size(block_size.into()); - } - } else { - // we ensure that block_size is some only for PSRAM addresses - if preparation.block_size.is_some() { - return Err(DmaError::UnsupportedMemoryRegion); - } - } - ); - self.tx_impl.set_burst_mode(false); + debug_assert_eq!(preparation.direction, TransferDirection::Out); + + #[cfg(esp32s3)] + if let Some(block_size) = preparation.external_memory_block_size { + self.set_ext_mem_block_size(block_size.into()); + } + + self.tx_impl.set_burst_mode(preparation.burst_transfer); self.tx_impl.set_descr_burst_mode(true); self.tx_impl.set_check_owner(preparation.check_owner); @@ -2184,7 +2182,7 @@ pub trait RegisterAccess: crate::private::Sealed { /// Enable/Disable INCR burst transfer for channel reading /// accessing data in internal RAM. - fn set_burst_mode(&self, burst_mode: bool); + fn set_burst_mode(&self, burst_mode: BurstTransfer); /// Enable/Disable burst transfer for channel reading /// descriptors in internal RAM. diff --git a/esp-hal/src/dma/pdma.rs b/esp-hal/src/dma/pdma.rs index c7856fc410..4f01404dd3 100644 --- a/esp-hal/src/dma/pdma.rs +++ b/esp-hal/src/dma/pdma.rs @@ -57,10 +57,10 @@ impl> RegisterAccess for SpiDma spi.dma_conf().modify(|_, w| w.out_rst().clear_bit()); } - fn set_burst_mode(&self, burst_mode: bool) { + fn set_burst_mode(&self, burst_mode: BurstTransfer) { let spi = self.0.register_block(); spi.dma_conf() - .modify(|_, w| w.out_data_burst_en().bit(burst_mode)); + .modify(|_, w| w.out_data_burst_en().bit(burst_mode.is_burst_enabled())); } fn set_descr_burst_mode(&self, burst_mode: bool) { @@ -222,7 +222,7 @@ impl> RegisterAccess for SpiDma spi.dma_conf().modify(|_, w| w.in_rst().clear_bit()); } - fn set_burst_mode(&self, _burst_mode: bool) {} + fn set_burst_mode(&self, _burst_mode: BurstTransfer) {} fn set_descr_burst_mode(&self, burst_mode: bool) { let spi = self.0.register_block(); @@ -480,11 +480,11 @@ impl> RegisterAccess for I2sDma reg_block.lc_conf().modify(|_, w| w.out_rst().clear_bit()); } - fn set_burst_mode(&self, burst_mode: bool) { + fn set_burst_mode(&self, burst_mode: BurstTransfer) { let reg_block = self.0.register_block(); reg_block .lc_conf() - .modify(|_, w| w.out_data_burst_en().bit(burst_mode)); + .modify(|_, w| w.out_data_burst_en().bit(burst_mode.is_burst_enabled())); } fn set_descr_burst_mode(&self, burst_mode: bool) { @@ -659,7 +659,7 @@ impl> RegisterAccess for I2sDma reg_block.lc_conf().modify(|_, w| w.in_rst().clear_bit()); } - fn set_burst_mode(&self, _burst_mode: bool) {} + fn set_burst_mode(&self, _burst_mode: BurstTransfer) {} fn set_descr_burst_mode(&self, burst_mode: bool) { let reg_block = self.0.register_block();