Skip to content

Commit

Permalink
Address a few review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@bugadani
bugadani committed Nov 27, 2024
1 parent 44cb4a9 commit b4830de
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 104 deletions.
51 changes: 19 additions & 32 deletions esp-hal/src/dma/buffers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ cfg_if::cfg_if! {
/// Burst size used when transferring to and from external memory.
#[derive(Clone, Copy, PartialEq, Eq, Debug)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub enum ExternalBurstSize {
pub enum ExternalBurstConfig {
/// 16 bytes
Size16 = 16,

Expand All @@ -24,12 +24,12 @@ cfg_if::cfg_if! {
Size64 = 64,
}

impl ExternalBurstSize {
impl ExternalBurstConfig {
/// The default external memory burst length.
pub const DEFAULT: Self = Self::Size16;
}

impl Default for ExternalBurstSize {
impl Default for ExternalBurstConfig {
fn default() -> Self {
Self::DEFAULT
}
Expand All @@ -38,20 +38,20 @@ cfg_if::cfg_if! {
/// Internal memory access burst mode.
#[derive(Clone, Copy, PartialEq, Eq, Debug)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub enum InternalBurstTransfer {
pub enum InternalBurstConfig {
/// Burst mode is disabled.
Disabled,

/// Burst mode is enabled.
Enabled,
}

impl InternalBurstTransfer {
impl InternalBurstConfig {
/// The default internal burst mode configuration.
pub const DEFAULT: Self = Self::Disabled;
}

impl Default for InternalBurstTransfer {
impl Default for InternalBurstConfig {
fn default() -> Self {
Self::DEFAULT
}
Expand All @@ -64,19 +64,19 @@ cfg_if::cfg_if! {
/// Configures the burst size for PSRAM transfers.
///
/// Burst mode is always enabled for PSRAM transfers.
pub external: ExternalBurstSize,
pub external: ExternalBurstConfig,

/// Enables or disables the burst mode for internal memory transfers.
///
/// The burst size is not configurable.
pub internal: InternalBurstTransfer,
pub internal: InternalBurstConfig,
}

impl BurstConfig {
/// The default burst mode configuration.
pub const DEFAULT: Self = Self {
external: ExternalBurstSize::DEFAULT,
internal: InternalBurstTransfer::DEFAULT,
external: ExternalBurstConfig::DEFAULT,
internal: InternalBurstConfig::DEFAULT,
};
}

Expand Down Expand Up @@ -108,37 +108,37 @@ cfg_if::cfg_if! {
}
}

type InternalBurstTransfer = BurstConfig;
type InternalBurstConfig = BurstConfig;
}
}

#[cfg(psram_dma)]
impl ExternalBurstSize {
impl ExternalBurstConfig {
const fn min_psram_alignment(self, direction: TransferDirection) -> usize {
// S2: Specifically, size and buffer address pointer in receive descriptors
// S2 TRM: Specifically, size and buffer address pointer in receive descriptors
// should be 16-byte, 32-byte or 64-byte aligned. For data frame whose
// length is not a multiple of 16 bytes, 32 bytes, or 64 bytes, EDMA adds
// padding bytes to the end.

// S3: Size and Address for IN transfers must be block aligned. For receive
// S3 TRM: Size and Address for IN transfers must be block aligned. For receive
// descriptors, if the data length received are not aligned with block size,
// GDMA will pad the data received with 0 until they are aligned to
// initiate burst transfer. You can read the length field in receive descriptors
// to obtain the length of valid data received
if matches!(direction, TransferDirection::In) {
self as usize
} else {
// S2: Size, length and buffer address pointer in transmit descriptors are not
// necessarily aligned with block size.
// S2 TRM: Size, length and buffer address pointer in transmit descriptors are
// not necessarily aligned with block size.

// S3: Size, length, and buffer address pointer in transmit descriptors do not
// need to be aligned.
// S3 TRM: Size, length, and buffer address pointer in transmit descriptors do
// not need to be aligned.
1
}
}
}

impl InternalBurstTransfer {
impl InternalBurstConfig {
pub(super) fn is_burst_enabled(self) -> bool {
!matches!(self, Self::Disabled)
}
Expand Down Expand Up @@ -468,10 +468,6 @@ impl DmaTxBuf {
}

/// Configures the DMA to use burst transfers to access this buffer.
///
/// Note that the hardware is allowed to ignore this setting. If you attempt
/// to use burst transfers with improperly aligned buffers, starting the
/// transfer will result in [`DmaError::InvalidAlignment`].
pub fn set_burst_config(&mut self, burst: BurstConfig) -> Result<(), DmaBufError> {
let len = self.len();
self.configure(burst, len)
Expand Down Expand Up @@ -547,7 +543,6 @@ unsafe impl DmaTxBuffer for DmaTxBuf {

cfg_if::cfg_if! {
if #[cfg(psram_dma)] {
// Optimization: avoid locking for PSRAM range.
let is_data_in_psram = !is_valid_ram_address(self.buffer.as_ptr() as usize);
if is_data_in_psram {
unsafe {
Expand Down Expand Up @@ -631,10 +626,6 @@ impl DmaRxBuf {
}

/// Configures the DMA to use burst transfers to access this buffer.
///
/// Note that the hardware is allowed to ignore this setting. If you attempt
/// to use burst transfers with improperly aligned buffers, starting the
/// transfer will result in [`DmaError::InvalidAlignment`].
pub fn set_burst_config(&mut self, burst: BurstConfig) -> Result<(), DmaBufError> {
let len = self.len();
self.configure(burst, len)
Expand Down Expand Up @@ -833,10 +824,6 @@ impl DmaRxTxBuf {
}

/// Configures the DMA to use burst transfers to access this buffer.
///
/// Note that the hardware is allowed to ignore this setting. If you attempt
/// to use burst transfers with improperly aligned buffers, starting the
/// transfer will result in [`DmaError::InvalidAlignment`].
pub fn set_burst_config(&mut self, burst: BurstConfig) -> Result<(), DmaBufError> {
let len = self.len();
self.configure(burst, len)
Expand Down
15 changes: 0 additions & 15 deletions esp-hal/src/dma/m2m.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,21 +152,6 @@ where
.prepare_transfer_without_start(self.peripheral, &self.rx_chain)?;
self.channel.rx.set_mem2mem_mode(true);
}
#[cfg(esp32s3)]
{
let align = match unsafe { crate::soc::cache_get_dcache_line_size() } {
16 => DmaExtMemBKSize::Size16,
32 => DmaExtMemBKSize::Size32,
64 => DmaExtMemBKSize::Size64,
_ => panic!("unsupported cache line size"),
};
if crate::soc::is_valid_psram_address(tx_ptr as usize) {
self.channel.tx.set_ext_mem_block_size(align);
}
if crate::soc::is_valid_psram_address(rx_ptr as usize) {
self.channel.rx.set_ext_mem_block_size(align);
}
}
self.channel.tx.start_transfer()?;
self.channel.rx.start_transfer()?;
Ok(DmaTransferRx::new(self))
Expand Down
75 changes: 27 additions & 48 deletions esp-hal/src/dma/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -366,26 +366,6 @@ impl DmaDescriptor {
true => Owner::Dma,
}
}

fn iter(&self) -> impl Iterator<Item = &DmaDescriptor> {
core::iter::successors(Some(self), |d| {
if d.next.is_null() {
None
} else {
Some(unsafe { &*d.next })
}
})
}

fn iter_mut(&mut self) -> impl Iterator<Item = &mut DmaDescriptor> {
core::iter::successors(Some(self), |d| {
if d.next.is_null() {
None
} else {
Some(unsafe { &mut *d.next })
}
})
}
}

// The pointers in the descriptor can be Sent.
Expand Down Expand Up @@ -1134,15 +1114,28 @@ impl<'a> DescriptorSet<'a> {

/// Returns an iterator over the linked descriptors.
fn linked_iter(&self) -> impl Iterator<Item = &DmaDescriptor> {
self.descriptors.first().into_iter().flat_map(|d| d.iter())
let mut was_last = false;
self.descriptors.iter().take_while(move |d| {
if was_last {
false
} else {
was_last = d.next.is_null();
true
}
})
}

/// Returns an iterator over the linked descriptors.
fn linked_iter_mut(&mut self) -> impl Iterator<Item = &mut DmaDescriptor> {
self.descriptors
.first_mut()
.into_iter()
.flat_map(|d| d.iter_mut())
let mut was_last = false;
self.descriptors.iter_mut().take_while(move |d| {
if was_last {
false
} else {
was_last = d.next.is_null();
true
}
})
}

/// Associate each descriptor with a chunk of the buffer.
Expand Down Expand Up @@ -1286,12 +1279,12 @@ pub enum DmaExtMemBKSize {
}

#[cfg(psram_dma)]
impl From<ExternalBurstSize> for DmaExtMemBKSize {
fn from(size: ExternalBurstSize) -> Self {
impl From<ExternalBurstConfig> for DmaExtMemBKSize {
fn from(size: ExternalBurstConfig) -> Self {
match size {
ExternalBurstSize::Size16 => DmaExtMemBKSize::Size16,
ExternalBurstSize::Size32 => DmaExtMemBKSize::Size32,
ExternalBurstSize::Size64 => DmaExtMemBKSize::Size64,
ExternalBurstConfig::Size16 => DmaExtMemBKSize::Size16,
ExternalBurstConfig::Size32 => DmaExtMemBKSize::Size32,
ExternalBurstConfig::Size64 => DmaExtMemBKSize::Size64,
}
}
}
Expand Down Expand Up @@ -1761,9 +1754,6 @@ pub trait Rx: crate::private::Sealed {

fn stop_transfer(&mut self);

#[cfg(psram_dma)]
fn set_ext_mem_block_size(&self, size: DmaExtMemBKSize);

#[cfg(gdma)]
fn set_mem2mem_mode(&mut self, value: bool);

Expand Down Expand Up @@ -1925,7 +1915,8 @@ where
}

#[cfg(psram_dma)]
self.set_ext_mem_block_size(preparation.burst_transfer.external.into());
self.rx_impl
.set_ext_mem_block_size(preparation.burst_transfer.external.into());
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);
Expand Down Expand Up @@ -2023,11 +2014,6 @@ where
self.rx_impl.stop()
}

#[cfg(psram_dma)]
fn set_ext_mem_block_size(&self, size: DmaExtMemBKSize) {
self.rx_impl.set_ext_mem_block_size(size);
}

#[cfg(gdma)]
fn set_mem2mem_mode(&mut self, value: bool) {
self.rx_impl.set_mem2mem_mode(value);
Expand Down Expand Up @@ -2096,9 +2082,6 @@ pub trait Tx: crate::private::Sealed {

fn stop_transfer(&mut self);

#[cfg(psram_dma)]
fn set_ext_mem_block_size(&self, size: DmaExtMemBKSize);

fn is_done(&self) -> bool {
self.pending_out_interrupts()
.contains(DmaTxInterrupt::TotalEof)
Expand Down Expand Up @@ -2220,7 +2203,8 @@ where
}

#[cfg(psram_dma)]
self.set_ext_mem_block_size(preparation.burst_transfer.external.into());
self.tx_impl
.set_ext_mem_block_size(preparation.burst_transfer.external.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);
Expand Down Expand Up @@ -2325,11 +2309,6 @@ where
self.tx_impl.stop()
}

#[cfg(psram_dma)]
fn set_ext_mem_block_size(&self, size: DmaExtMemBKSize) {
self.tx_impl.set_ext_mem_block_size(size);
}

fn listen_out(&self, interrupts: impl Into<EnumSet<DmaTxInterrupt>>) {
self.tx_impl.listen(interrupts);
}
Expand Down
4 changes: 2 additions & 2 deletions examples/src/bin/spi_loopback_dma_psram.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
use esp_backtrace as _;
use esp_hal::{
delay::Delay,
dma::{BurstConfig, DmaRxBuf, DmaTxBuf, ExternalBurstSize},
dma::{BurstConfig, DmaRxBuf, DmaTxBuf, ExternalBurstConfig},
peripheral::Peripheral,
prelude::*,
spi::{
Expand All @@ -51,7 +51,7 @@ macro_rules! dma_alloc_buffer {
}

const DMA_BUFFER_SIZE: usize = 8192;
const DMA_ALIGNMENT: ExternalBurstSize = ExternalBurstSize::Size64;
const DMA_ALIGNMENT: ExternalBurstConfig = ExternalBurstConfig::Size64;
const DMA_CHUNK_SIZE: usize = 4096 - DMA_ALIGNMENT as usize;

#[entry]
Expand Down
14 changes: 7 additions & 7 deletions hil-test/tests/spi_half_duplex_write_psram.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
use defmt::error;
use esp_alloc as _;
use esp_hal::{
dma::{BurstConfig, DmaRxBuf, DmaTxBuf, ExternalBurstSize, InternalBurstTransfer},
dma::{BurstConfig, DmaRxBuf, DmaTxBuf, ExternalBurstConfig, InternalBurstConfig},
dma_buffers,
dma_descriptors_chunk_size,
gpio::interconnect::InputSignal,
Expand Down Expand Up @@ -85,7 +85,7 @@ mod tests {
#[timeout(3)]
fn test_spi_writes_are_correctly_by_pcnt(ctx: Context) {
const DMA_BUFFER_SIZE: usize = 4;
const DMA_ALIGNMENT: ExternalBurstSize = ExternalBurstSize::Size32;
const DMA_ALIGNMENT: ExternalBurstConfig = ExternalBurstConfig::Size32;
const DMA_CHUNK_SIZE: usize = 4096 - DMA_ALIGNMENT as usize;

let (_, descriptors) = dma_descriptors_chunk_size!(0, DMA_BUFFER_SIZE, DMA_CHUNK_SIZE);
Expand All @@ -94,8 +94,8 @@ mod tests {
descriptors,
buffer,
BurstConfig {
internal: InternalBurstTransfer::default(),
external: ExternalBurstSize::Size32,
internal: InternalBurstConfig::default(),
external: ExternalBurstConfig::Size32,
},
)
.unwrap();
Expand Down Expand Up @@ -142,7 +142,7 @@ mod tests {
#[timeout(3)]
fn test_spidmabus_writes_are_correctly_by_pcnt(ctx: Context) {
const DMA_BUFFER_SIZE: usize = 4;
const DMA_ALIGNMENT: ExternalBurstSize = ExternalBurstSize::Size32; // matches dcache line size
const DMA_ALIGNMENT: ExternalBurstConfig = ExternalBurstConfig::Size32; // matches dcache line size
const DMA_CHUNK_SIZE: usize = 4096 - DMA_ALIGNMENT as usize; // 64 byte aligned

let (_, descriptors) = dma_descriptors_chunk_size!(0, DMA_BUFFER_SIZE, DMA_CHUNK_SIZE);
Expand All @@ -151,8 +151,8 @@ mod tests {
descriptors,
buffer,
BurstConfig {
internal: InternalBurstTransfer::default(),
external: ExternalBurstSize::Size32,
internal: InternalBurstConfig::default(),
external: ExternalBurstConfig::Size32,
},
)
.unwrap();
Expand Down

0 comments on commit b4830de

Please sign in to comment.