Spi dma irq

[yvettewu1]

No description provided.

[rusty1968]

Can you create a PR?

[yvettewu1]

Can you create a PR?

Done

[rusty1968]

Location: src/spi/norflash.rs:91-101

pub struct SpiNorData<'a> {
    pub mode: Jesd216Mode,
    pub opcode: u32,
    pub dummy_cycle: u32,
    pub addr_len: u32,
    pub addr: u32,
    pub data_len: u32,           // Comment says "not in used" in several places
    pub tx_buf: &'a [u8],
    pub rx_buf: &'a mut [u8],
    pub data_direct: u32,        // Uses magic constants SPI_NOR_DATA_DIRECT_READ/WRITE
}

Problems:

1. data_direct uses magic u32 values instead of an enum
2. data_len appears unused but is still required
3. The struct is used across all layers, coupling them

Recommended fix:

pub enum DataDirection {
    Read,
    Write,
    None,
}

pub struct SpiNorCommand<'a> {
    pub mode: Jesd216Mode,
    pub opcode: u8,              // Commands are 8-bit
    pub address: Option<FlashAddress>,
    pub dummy_cycles: u8,
    pub data: TransferData<'a>,
}

pub enum TransferData<'a> {
    Read(&'a mut [u8]),
    Write(&'a [u8]),
    None,
}

pub struct FlashAddress {
    pub value: u32,
    pub width: AddressWidth,
}

pub enum AddressWidth {
    ThreeByte,
    FourByte,
}

[rusty1968]

Location: src/spi/spicontroller.rs (and fmccontroller.rs)

SpiController handles:

1. Register block access (bare metal)
2. Chip select management
3. DMA configuration and execution
4. Timing calibration
5. Decode range management
6. Implements SpiBus trait
7. Implements SpiBusWithCs trait

Problem: Single Responsibility Principle violation. Makes testing, modification, and reasoning about the code difficult.
https://en.wikipedia.org/wiki/Single-responsibility_principle

Recommended decomposition:

// Low-level register access
struct SpiRegisters {
    regs: &'static ast1060_pac::spi::RegisterBlock,
}

// DMA operations
struct SpiDma<'a> {
    regs: &'a SpiRegisters,
}

// Timing calibration
struct SpiCalibration<'a> {
    regs: &'a SpiRegisters,
    dma: &'a mut SpiDma<'a>,
}

// High-level controller combining components
pub struct SpiController<'a> {
    regs: SpiRegisters,
    dma: SpiDma<'a>,
    config: SpiConfig,
    state: SpiData,
}

[rusty1968]

Location: src/spi/mod.rs:233

pub fn spi_cal_dummy_cycle(bus_width: u32, dummy_cycle: u32) -> u32 {
    let dummy_byte = dummy_cycle / (8 / bus_width);  // Panics if bus_width == 0 or bus_width > 8
    //...
}

Risk scenarios:

• bus_width == 0 → 8 / 0 panics
• bus_width > 8 (e.g., 16) → 8 / 16 == 0 → dummy_cycle / 0 panics

Fix:

pub fn spi_cal_dummy_cycle(bus_width: u32, dummy_cycle: u32) -> Option<u32> {
    if bus_width == 0 || bus_width > 8 {
        return None;
    }
    let bits_per_cycle = 8 / bus_width;
    if bits_per_cycle == 0 {
        return None;
    }
    let dummy_byte = dummy_cycle / bits_per_cycle;
    Some(((dummy_byte & 0x3) << 6) | (((dummy_byte & 0x4) >> 2) << 14))
}

[rusty1968]

CS index used without bounds checking:

Arrays are sized [_; ASPEED_MAX_CS] where ASPEED_MAX_CS = 5, but CS validation uses > instead of >=:

File	Line	Code
spicontroller.rs	913	if cs > self.spi_config.max_cs (allows cs == max_cs)
fmccontroller.rs	868	if cs > self.spi_config.max_cs (allows cs == max_cs)

Then the code accesses:

self.spi_data.cmd_mode[cs]      // Panics if cs >= 5
self.spi_data.decode_addr[cs]   // Panics if cs >= 5

Fix: Change validation to >=:

fn select_cs(&mut self, cs: usize) -> Result<(), SpiError> {
    if cs >= self.spi_config.max_cs || cs >= ASPEED_MAX_CS {
        return Err(SpiError::CsSelectFailed(cs));
    }
    self.current_cs = cs;
    Ok(())
}

[rusty1968]

mod.rs Has Too Many Responsibilities

src/spi/mod.rs mixes:

• Error types (lines 21-50)
• Trait definitions (lines 52-60)
• Constants (lines 63-114)
• Data structures (lines 116-182)
• Macros (lines 184-192)
• Utility functions (lines 194-519)
• GPIO/SCU manipulation (lines 389-519)

Recommendation: Split into separate files:

• error.rs - Error types
• traits.rs - SpiBusWithCs trait
• config.rs - SpiConfig, SpiData, CommandMode, etc.
• util.rs - Frequency calculation, calibration helpers
• spim.rs - SPIM GPIO/SCU configuration

[rusty1968]

Location: src/spi/mod.rs:389

pub static mut GPIO_ORI_VAL: [u32; 4] = [0; 4];

This is a data race hazard. Access is through get_gpio_ori_val() (line 390-392) but writes occur in spim_proprietary_pre_config() without any synchronization.

Recommendation: Use core::sync::atomic::AtomicU32 or a Mutex<RefCell<>> pattern.

[rusty1968]

These will panic unconditionally if executed:

File	Line	Function
device.rs	53	Operation::DelayNs(_) => todo!()
fmccontroller.rs	858	transfer_in_place()
fmccontroller.rs	862	flush()
spicontroller.rs	903	transfer_in_place()
spicontroller.rs	907	flush()
spidmairqtest.rs	81, 201, 230	Device ID match arms

Fix: Return Err(SpiError::Other("not implemented")) instead:

fn transfer_in_place(&mut self, _buffer: &mut [u8]) -> Result<(), SpiError> {
    Err(SpiError::Other("transfer_in_place not supported"))
}

[yvettewu1]

Location: src/spi/norflash.rs:91-101

pub struct SpiNorData<'a> {
    pub mode: Jesd216Mode,
    pub opcode: u32,
    pub dummy_cycle: u32,
    pub addr_len: u32,
    pub addr: u32,
    pub data_len: u32,           // Comment says "not in used" in several places
    pub tx_buf: &'a [u8],
    pub rx_buf: &'a mut [u8],
    pub data_direct: u32,        // Uses magic constants SPI_NOR_DATA_DIRECT_READ/WRITE
}

Problems:

1. data_direct uses magic u32 values instead of an enum
2. data_len appears unused but is still required
3. The struct is used across all layers, coupling them

Recommended fix:

pub enum DataDirection {
    Read,
    Write,
    None,
}

pub struct SpiNorCommand<'a> {
    pub mode: Jesd216Mode,
    pub opcode: u8,              // Commands are 8-bit
    pub address: Option<FlashAddress>,
    pub dummy_cycles: u8,
    pub data: TransferData<'a>,
}

pub enum TransferData<'a> {
    Read(&'a mut [u8]),
    Write(&'a [u8]),
    None,
}

pub struct FlashAddress {
    pub value: u32,
    pub width: AddressWidth,
}

pub enum AddressWidth {
    ThreeByte,
    FourByte,
}

1. addr_len is required, some of the spi command's addr_len is '0x0', i am deleting the "Not in Used" comment
2. addr is not optional since the SpiController and FmcController is checking and use it. but i will implement the new structrue and enum.
3. Will Keep the transfer data the same.

[yvettewu1]

Location: src/spi/spicontroller.rs (and fmccontroller.rs)

SpiController handles:

1. Register block access (bare metal)
2. Chip select management
3. DMA configuration and execution
4. Timing calibration
5. Decode range management
6. Implements SpiBus trait
7. Implements SpiBusWithCs trait

Problem: Single Responsibility Principle violation. Makes testing, modification, and reasoning about the code difficult. https://en.wikipedia.org/wiki/Single-responsibility_principle

Recommended decomposition:

// Low-level register access
struct SpiRegisters {
    regs: &'static ast1060_pac::spi::RegisterBlock,
}

// DMA operations
struct SpiDma<'a> {
    regs: &'a SpiRegisters,
}

// Timing calibration
struct SpiCalibration<'a> {
    regs: &'a SpiRegisters,
    dma: &'a mut SpiDma<'a>,
}

// High-level controller combining components
pub struct SpiController<'a> {
    regs: SpiRegisters,
    dma: SpiDma<'a>,
    config: SpiConfig,
    state: SpiData,
}

the current SpiController:
It handles spi pure data regardless of dma a not. dma is just a form of transaction, which is determined at build.
Will not change .

pub struct SpiController<'a> {
regs: &'static ast1060_pac::spi::RegisterBlock,
current_cs: usize,
spi_config: SpiConfig,
spi_data: SpiData,
pub dbg_uart: Option<&'a mut UartController<'a>>,
}