ast10x0/i3c: refine state tracking and harden error recovery Address review feedback from PR #278 and improve state safety during DAA and transfer recovery: - Replace boolean `da_assigned` and `ibi_en` with explicit `DaState` and `IbiState` enums. - Fix `do_ccc` timeout handling to return `I3cDrvError::Timeout` immediately. - Enforce sticky fault (`xfer_faulted`) when recovery fails to prevent hardware lockup. - Log and set sticky fault on `init()` failures during SIR timeout recovery. - Add 7-bit address bounds checks and spec-defined default reservations to `AddrBook`. - Update peripheral tests (`i3c_init`, `i3c_irq`) to conform to updated state models. Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
diff --git a/target/ast10x0/peripherals/i3c/ccc.rs b/target/ast10x0/peripherals/i3c/ccc.rs index bd9bfdc..e2cbf99 100644 --- a/target/ast10x0/peripherals/i3c/ccc.rs +++ b/target/ast10x0/peripherals/i3c/ccc.rs
@@ -5,7 +5,7 @@ //! //! Functions and types for executing I3C CCCs. -use super::config::I3cConfig; +use super::config::{DaState, I3cConfig, IbiState}; use super::constants::{ I3C_BCR_IBI_PAYLOAD_HAS_DATA_BYTE, I3C_CCC_GETBCR, I3C_CCC_GETDCR, I3C_CCC_GETMRL, I3C_CCC_GETMWL, I3C_CCC_GETMXDS, I3C_CCC_GETPID, I3C_CCC_GETSTATUS, I3C_CCC_RSTDAA, @@ -13,6 +13,7 @@ }; use super::error::{CccErrorKind, I3cError}; use super::hardware::HardwareInterface; +use super::types::DevKind; // ============================================================================= // CCC Types @@ -153,6 +154,21 @@ // CCC Operations // ============================================================================= +/// Preserve errors with ambiguous bus-side effects. +fn map_ccc_err(e: I3cError) -> I3cError { + match e { + I3cError::Timeout | I3cError::RespError | I3cError::IoError | I3cError::AddressNack => e, + _ => I3cError::CccError(CccErrorKind::Invalid), + } +} + +fn mark_setnewda_unknown(config: &mut I3cConfig, dev_idx: usize, new_da: u8) { + config.mark_da_unknown(dev_idx, Some(new_da)); + if let Some(dev) = config.attached.devices.get_mut(dev_idx) { + dev.ibi_state = IbiState::Unknown; + } +} + /// Enable/disable events for all devices (broadcast) pub fn ccc_events_all_set<H>( hw: &mut H, @@ -163,6 +179,11 @@ where H: HardwareInterface, { + // SIR changes require per-device DAT updates. + if events & super::constants::I3C_CCC_EVT_INTR != 0 { + return Err(I3cError::Access); + } + let id = if enable { ccc_enec(true) } else { @@ -180,7 +201,7 @@ targets: None, }, ) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid)) + .map_err(map_ccc_err) } /// Enable/disable events for a specific device (direct) @@ -223,8 +244,7 @@ targets: Some(&mut tgts[..]), }; - hw.do_ccc(config, &mut payload) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid)) + hw.do_ccc(config, &mut payload).map_err(map_ccc_err) } /// Execute RSTACT (Reset Action) broadcast @@ -247,8 +267,7 @@ targets: None, }; - hw.do_ccc(config, &mut payload) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid)) + hw.do_ccc(config, &mut payload).map_err(map_ccc_err) } /// Get BCR (Bus Characteristics Register) from a device @@ -280,8 +299,7 @@ targets: Some(&mut tgts[..]), }; - hw.do_ccc(config, &mut payload) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid))?; + hw.do_ccc(config, &mut payload).map_err(map_ccc_err)?; Ok(bcr_buf[0]) } @@ -315,18 +333,12 @@ targets: Some(&mut tgts[..]), }; - hw.do_ccc(config, &mut payload) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid))?; + hw.do_ccc(config, &mut payload).map_err(map_ccc_err)?; Ok(dcr_buf[0]) } -/// Bus-only SETNEWDA: send the CCC, touch **no** bookkeeping or DAT state. -/// -/// For the DAA engine (`I3cController::bus_daa`), which addresses a device -/// that answered on a *different* entry's address (mis-assignment / -/// unsolicited cases) and manages the tables itself. Everyone else should use -/// [`ccc_setnewda`]. +/// Send SETNEWDA without changing bookkeeping or DAT state. pub(crate) fn ccc_setnewda_bus_only<H>( hw: &mut H, config: &mut I3cConfig, @@ -336,7 +348,8 @@ where H: HardwareInterface, { - if curr_da == 0 || new_da == 0 { + // Reject addresses that would be truncated on the bus. + if curr_da == 0 || new_da == 0 || new_da >= super::constants::I3C_BROADCAST_ADDR { return Err(I3cError::CccError(CccErrorKind::InvalidParam)); } @@ -358,8 +371,7 @@ targets: Some(&mut tgts[..]), }; - hw.do_ccc(config, &mut payload) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid)) + hw.do_ccc(config, &mut payload).map_err(map_ccc_err) } /// Set new dynamic address for a device @@ -372,26 +384,71 @@ where H: HardwareInterface, { - let Some(pos) = config.attached.pos_of_addr(curr_da) else { + let Some(dev_idx) = config.attached.find_dev_idx_by_addr(curr_da) else { return Err(I3cError::CccError(CccErrorKind::NotFound)); }; + let Some(dev) = config.attached.devices.get(dev_idx) else { + return Err(I3cError::CccError(CccErrorKind::NotFound)); + }; + if dev.kind != DevKind::I3c || dev.da_state != DaState::Verified { + return Err(I3cError::Access); + } + let Some(pos) = config.attached.pos_of(dev_idx) else { + return Err(I3cError::CccError(CccErrorKind::NotFound)); + }; + let ibi_was_enabled = config + .attached + .devices + .iter() + .find(|dev| dev.dyn_addr == curr_da) + .is_some_and(|dev| dev.ibi_state == IbiState::Enabled); - if !config.addrbook.is_free(new_da) { + // Check reservations before sending SETNEWDA. + if new_da != curr_da && !config.addrbook.is_free(new_da) { return Err(I3cError::CccError(CccErrorKind::NoFreeSlot)); } - ccc_setnewda_bus_only(hw, config, curr_da, new_da)?; + if let Err(e) = ccc_setnewda_bus_only(hw, config, curr_da, new_da) { + if matches!(e, I3cError::Timeout | I3cError::RespError) { + // The target may have moved despite the error. + mark_setnewda_unknown(config, dev_idx, new_da); + } + return Err(e); + } - // The device now answers on `new_da`: mirror the move into the address - // book / attached table and reprogram the DAT slot, or every subsequent - // private transfer would still address the device through the stale entry. - // The fresh DAT write restores the SIR/MR-reject defaults — call - // `ibi_enable` again afterwards if the device had IBIs enabled. - config - .reassign_da(curr_da, new_da) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid))?; - hw.attach_i3c_dev(pos.into(), new_da) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid)) + // Reprogram DAT before committing bookkeeping. + if hw.attach_i3c_dev(pos.into(), new_da).is_err() { + // The target moved but DAT did not; block further transfers. + mark_setnewda_unknown(config, dev_idx, new_da); + hw.mark_xfer_faulted(); + return Err(I3cError::CccError(CccErrorKind::Invalid)); + } + + // Commit the move and restore IBI state if needed. + if config.reassign_da(curr_da, new_da).is_err() { + // Bus and DAT moved but bookkeeping did not. + mark_setnewda_unknown(config, dev_idx, new_da); + hw.mark_xfer_faulted(); + return Err(I3cError::CccError(CccErrorKind::Invalid)); + } + + if ibi_was_enabled { + let result = hw.ibi_enable(config, new_da); + if let Some(dev) = config + .attached + .devices + .iter_mut() + .find(|dev| dev.dyn_addr == new_da) + { + dev.ibi_state = match result { + Ok(()) => IbiState::Enabled, + Err(I3cError::Timeout | I3cError::RespError) => IbiState::Unknown, + Err(_) => IbiState::Disabled, + }; + } + result.map_err(map_ccc_err)?; + } + Ok(()) } /// Send a direct write CCC with a small fixed payload. @@ -423,8 +480,7 @@ }), targets: Some(&mut tgts[..]), }; - hw.do_ccc(config, &mut p) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid)) + hw.do_ccc(config, &mut p).map_err(map_ccc_err) } /// Send a direct read CCC into a small fixed buffer. @@ -456,8 +512,7 @@ }), targets: Some(&mut tgts[..]), }; - hw.do_ccc(config, &mut p) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid)) + hw.do_ccc(config, &mut p).map_err(map_ccc_err) } /// Set Maximum Write Length for a device (direct SETMWL); mirrors the value @@ -528,7 +583,14 @@ targets: None, }, ) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid)) + .map_err(map_ccc_err)?; + + for dev in &mut config.attached.devices { + if dev.kind == DevKind::I3c { + dev.mwl = mwl; + } + } + Ok(()) } /// Broadcast SETMRL to all devices. @@ -561,7 +623,17 @@ targets: None, }, ) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid)) + .map_err(map_ccc_err)?; + + for dev in &mut config.attached.devices { + if dev.kind == DevKind::I3c { + dev.mrl = mrl; + if let Some(n) = ibi_len { + dev.max_ibi = n; + } + } + } + Ok(()) } /// Get Maximum Write Length from a device (GETMWL); mirrors the value into @@ -673,8 +745,7 @@ targets: Some(&mut tgts[..]), }; - hw.do_ccc(config, &mut payload) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid))?; + hw.do_ccc(config, &mut payload).map_err(map_ccc_err)?; Ok(bytes_to_pid(&pid_buf)) } @@ -720,8 +791,7 @@ targets: Some(&mut targets_arr[..]), }; - hw.do_ccc(config, &mut payload) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid))?; + hw.do_ccc(config, &mut payload).map_err(map_ccc_err)?; let val = u16::from_be_bytes(data_buf); @@ -739,7 +809,9 @@ { match ccc_getstatus(hw, config, da, GetStatusFormat::Fmt1) { Ok(GetStatusResp::Fmt1 { status }) => Ok(status), - _ => Err(I3cError::CccError(CccErrorKind::Invalid)), + // Fmt1 cannot produce Fmt2. + Ok(GetStatusResp::Fmt2 { .. }) => Err(I3cError::CccError(CccErrorKind::Invalid)), + Err(e) => Err(e), } } @@ -748,16 +820,31 @@ where H: HardwareInterface, { - hw.do_ccc( - config, - &mut CccPayload { - ccc: Some(Ccc { - id: I3C_CCC_RSTDAA, - data: None, - num_xfer: 0, - }), - targets: None, - }, - ) - .map_err(|_| I3cError::CccError(CccErrorKind::Invalid)) + let result = hw + .do_ccc( + config, + &mut CccPayload { + ccc: Some(Ccc { + id: I3C_CCC_RSTDAA, + data: None, + num_xfer: 0, + }), + targets: None, + }, + ) + .map_err(map_ccc_err); + + match result { + Ok(()) => { + config.commit_rstdaa(); + Ok(()) + } + Err(e @ (I3cError::Timeout | I3cError::RespError)) => { + // The command may have reached none, some, or all targets. Keep + // every possible owner reserved until a confirmed RSTDAA. + config.mark_rstdaa_unknown(); + Err(e) + } + Err(e) => Err(e), + } }
diff --git a/target/ast10x0/peripherals/i3c/config.rs b/target/ast10x0/peripherals/i3c/config.rs index 2665dab..ed24c65 100644 --- a/target/ast10x0/peripherals/i3c/config.rs +++ b/target/ast10x0/peripherals/i3c/config.rs
@@ -8,6 +8,7 @@ use core::marker::PhantomData; use heapless::Vec; +use super::constants::NSEC_PER_SEC; use super::error::I3cError; use super::types::DevKind; @@ -19,17 +20,24 @@ pub struct I3cTargetConfig { /// Target flags pub flags: u8, - /// Dynamic address (assigned by controller) + /// Dynamic address assigned by DAA. pub addr: Option<u8>, + /// Static address advertised before DAA. + pub static_addr: Option<u8>, /// Mandatory Data Byte for IBI pub mdb: u8, } impl I3cTargetConfig { - /// Create a new target configuration + /// Create a target configuration with no assigned dynamic address. #[must_use] - pub const fn new(flags: u8, addr: Option<u8>, mdb: u8) -> Self { - Self { flags, addr, mdb } + pub const fn new(flags: u8, static_addr: Option<u8>, mdb: u8) -> Self { + Self { + flags, + addr: None, + static_addr, + mdb, + } } } @@ -83,11 +91,18 @@ } } - /// Check if an address is free (not in use and not reserved) + /// Check whether a 7-bit address is free. #[inline] #[must_use] pub fn is_free(&self, addr: u8) -> bool { - !Self::bit_get(&self.in_use, addr) && !Self::bit_get(&self.reserved, addr) + addr < 128 && !Self::bit_get(&self.in_use, addr) && !Self::bit_get(&self.reserved, addr) + } + + /// Check whether an address is reserved or outside the 7-bit range. + #[inline] + #[must_use] + pub fn is_reserved(&self, addr: u8) -> bool { + addr >= 128 || Self::bit_get(&self.reserved, addr) } /// Reserve default I3C addresses per specification @@ -127,16 +142,42 @@ /// Mark an address as used or free #[inline] pub fn mark_use(&mut self, addr: u8, used: bool) { - if addr != 0 { + // Avoid bitmap aliasing above the 7-bit range. + if addr != 0 && addr < 128 { Self::bit_set(&mut self.in_use, addr, used); } } + + /// Clear all in-use addresses. + pub fn reset_in_use(&mut self) { + self.in_use = [0; 4]; + } } // ============================================================================= // Device Entry // ============================================================================= +/// Cached controller/target IBI state. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum IbiState { + Disabled, + Enabled, + /// An ENEC/DISEC or DAT reprogramming operation had an ambiguous outcome. + Unknown, +} + +/// Confidence in a device entry's dynamic-address assignment. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum DaState { + /// No device has claimed this address. + Unassigned, + /// GETPID confirmed the address owner. + Verified, + /// The address may be claimed, but its owner is unverified. + Unknown, +} + /// Entry for a device attached to the I3C bus #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct DeviceEntry { @@ -164,14 +205,12 @@ pub mwl: u16, /// Maximum IBI payload size pub max_ibi: u8, - /// IBI enabled flag - pub ibi_en: bool, + /// Cached IBI state. + pub ibi_state: IbiState, /// Position in DAT (Device Address Table) pub pos: Option<u8>, - /// Dynamic address verified on the bus (set by DAA once the device's PID - /// was read back at its address). Cleared state means the entry is only a - /// reservation. - pub da_assigned: bool, + /// Dynamic-address confidence. + pub da_state: DaState, } impl DeviceEntry { @@ -191,9 +230,9 @@ mrl: 0, mwl: 0, max_ibi: 0, - ibi_en: false, + ibi_state: IbiState::Disabled, pos: None, - da_assigned: false, + da_state: DaState::Unassigned, } } @@ -213,9 +252,9 @@ mrl: 0, mwl: 0, max_ibi: 0, - ibi_en: false, + ibi_state: IbiState::Disabled, pos: None, - da_assigned: false, + da_state: DaState::Unassigned, } } } @@ -350,7 +389,9 @@ /// Unmap a DAT position #[inline] pub fn unmap_pos(&mut self, pos: u8) { - self.by_pos[pos as usize] = None; + if let Some(slot) = self.by_pos.get_mut(pos as usize) { + *slot = None; + } } } @@ -450,10 +491,14 @@ /// Create a new configuration with default values #[must_use] pub fn new() -> Self { + // Constructors must reserve spec-defined addresses. + let mut addrbook = AddrBook::new(); + addrbook.reserve_defaults(); + Self { common: CommonState::default(), target_config: None, - addrbook: AddrBook::new(), + addrbook, attached: Attached::new(), core_clk_hz: None, core_period: 0, @@ -480,27 +525,29 @@ self.attached = Attached::new(); } - /// Pick an initial dynamic address for a device - /// - /// Tries `desired` first, then `static_addr`, then allocates from pool. + /// Pick and reserve an initial dynamic address for a device. pub fn pick_initial_da(&mut self, static_addr: u8, desired: u8) -> Option<u8> { - if desired != 0 && self.addrbook.is_free(desired) { - self.addrbook.mark_use(desired, true); - return Some(desired); - } - if static_addr != 0 && self.addrbook.is_free(static_addr) { - self.addrbook.mark_use(static_addr, true); - return Some(static_addr); - } - // Mark the fallback allocation too (`alloc_from` is a pure scan), or - // the next caller would be handed the same "initial" address. - let addr = self.addrbook.alloc_from(8)?; + let addr = if desired != 0 && self.addrbook.is_free(desired) { + desired + } else if static_addr != 0 && self.addrbook.is_free(static_addr) { + static_addr + } else { + self.addrbook.alloc_from(8)? + }; self.addrbook.mark_use(addr, true); Some(addr) } /// Reassign a device's dynamic address pub fn reassign_da(&mut self, from: u8, to: u8) -> Result<(), I3cError> { + // Resolve the device before mutating the address book. + let idx = self + .attached + .devices + .iter() + .position(|d| d.dyn_addr == from) + .ok_or(I3cError::DevNotFound)?; + if from == to { return Ok(()); } @@ -510,19 +557,61 @@ self.addrbook.mark_use(from, false); self.addrbook.mark_use(to, true); + if let Some(dev) = self.attached.devices.get_mut(idx) { + dev.dyn_addr = to; + // Keep the next DAA cycle on the new address. + dev.desired_da = to; + } + Ok(()) + } - if let Some((i, mut e)) = self - .attached - .devices - .iter() - .enumerate() - .find_map(|(i, d)| (d.dyn_addr == from).then_some((i, *d))) - { - e.dyn_addr = to; - self.attached.devices[i] = e; - Ok(()) - } else { - Err(I3cError::DevNotFound) + /// Record an address-changing operation whose bus-side outcome is + /// ambiguous. The old and candidate addresses stay reserved so neither + /// can be handed to another target before the owner is verified. + pub(crate) fn mark_da_unknown(&mut self, dev_idx: usize, candidate: Option<u8>) { + let Some(dev) = self.attached.devices.get_mut(dev_idx) else { + return; + }; + if dev.kind != DevKind::I3c { + return; + } + + dev.da_state = DaState::Unknown; + self.addrbook.mark_use(dev.dyn_addr, true); + self.addrbook.mark_use(dev.desired_da, true); + if let Some(addr) = candidate { + self.addrbook.mark_use(addr, true); + } + } + + /// Preserve all possible address owners after an ambiguous broadcast + /// RSTDAA. Existing reservations, including parking addresses, remain + /// intact because the command may not have reached every target. + pub(crate) fn mark_rstdaa_unknown(&mut self) { + for dev in &mut self.attached.devices { + if dev.kind != DevKind::I3c { + continue; + } + dev.da_state = DaState::Unknown; + dev.ibi_state = IbiState::Unknown; + self.addrbook.mark_use(dev.dyn_addr, true); + self.addrbook.mark_use(dev.desired_da, true); + } + } + + /// Commit a confirmed broadcast RSTDAA and rebuild reservations from the + /// addresses that will be offered by the next DAA walk. + pub(crate) fn commit_rstdaa(&mut self) { + self.addrbook.reset_in_use(); + for dev in &mut self.attached.devices { + match dev.kind { + DevKind::I2c => self.addrbook.mark_use(dev.static_addr, true), + DevKind::I3c => { + dev.da_state = DaState::Unassigned; + dev.ibi_state = IbiState::Disabled; + self.addrbook.mark_use(dev.desired_da, true); + } + } } } } @@ -643,6 +732,12 @@ /// Maximum supported core clock (Hz) pub const I3C_MAX_CORE_CLK: u32 = 400_000_000; +/// Maximum I3C SDR SCL frequency. +pub const I3C_MAX_SCL_HZ: u32 = 12_500_000; + +/// Maximum legacy-I2C Fast-mode Plus frequency. +pub const I3C_MAX_I2C_SCL_HZ: u32 = 1_000_000; + impl I3cConfig { /// Validate clock configuration /// @@ -675,24 +770,53 @@ /// config.validate_clock()?; /// ``` pub fn validate_clock(&self) -> Result<(), I3cError> { - if let Some(core_hz) = self.core_clk_hz { - // Check core clock range - if core_hz < I3C_MIN_CORE_CLK_SDR { - return Err(I3cError::InvalidParam); - } - if core_hz > I3C_MAX_CORE_CLK { - return Err(I3cError::InvalidParam); - } + // Enforce protocol frequency limits. + if self.i3c_scl_hz > I3C_MAX_SCL_HZ { + return Err(I3cError::InvalidParam); + } + if self.i2c_scl_hz > I3C_MAX_I2C_SCL_HZ { + return Err(I3cError::InvalidParam); + } + // Primary mode requires I3C timing; legacy-I2C timing is optional. + if !self.is_secondary && self.i3c_scl_hz == 0 { + return Err(I3cError::InvalidParam); + } - // Check I3C SCL achievability (need ~4x core clock for timing - // resolution). `saturating_mul`: an absurd SCL must fail - // validation, not overflow-panic inside the validator. - if self.i3c_scl_hz > 0 && core_hz < self.i3c_scl_hz.saturating_mul(4) { - return Err(I3cError::InvalidParam); - } + if let Some(core_hz) = self.core_clk_hz + && (core_hz < I3C_MIN_CORE_CLK_SDR || core_hz > I3C_MAX_CORE_CLK) + { + return Err(I3cError::InvalidParam); + } - // Check I2C SCL achievability - if self.i2c_scl_hz > 0 && core_hz < self.i2c_scl_hz.saturating_mul(4) { + // Validate against the same fallback used by `init_clock`. + let effective_core_hz = self.core_clk_hz.unwrap_or(I3C_MIN_CORE_CLK_SDR); + if self.i3c_scl_hz > 0 && effective_core_hz < self.i3c_scl_hz.saturating_mul(4) { + return Err(I3cError::InvalidParam); + } + if self.i2c_scl_hz > 0 && effective_core_hz < self.i2c_scl_hz.saturating_mul(4) { + return Err(I3cError::InvalidParam); + } + + // Custom timing requires complete high/low pairs. + if (self.i3c_od_scl_hi_period_ns == 0) != (self.i3c_od_scl_lo_period_ns == 0) + || (self.i3c_pp_scl_hi_period_ns == 0) != (self.i3c_pp_scl_lo_period_ns == 0) + { + return Err(I3cError::InvalidParam); + } + + // Custom periods must fit the 8-bit timing registers. + let period_ns = NSEC_PER_SEC.div_ceil(effective_core_hz.max(1)).max(1); + for ns in [ + self.i3c_od_scl_hi_period_ns, + self.i3c_od_scl_lo_period_ns, + self.i3c_pp_scl_hi_period_ns, + self.i3c_pp_scl_lo_period_ns, + ] { + if ns == 0 { + continue; + } + let cnt = ns.div_ceil(period_ns); + if cnt == 0 || cnt > u32::from(u8::MAX) { return Err(I3cError::InvalidParam); } }
diff --git a/target/ast10x0/peripherals/i3c/controller.rs b/target/ast10x0/peripherals/i3c/controller.rs index 4325ad2..663e7f4 100644 --- a/target/ast10x0/peripherals/i3c/controller.rs +++ b/target/ast10x0/peripherals/i3c/controller.rs
@@ -50,8 +50,8 @@ use core::marker::PhantomData; use super::ccc; -use super::config::{DeviceEntry, I3cConfig, I3cTargetConfig}; -use super::constants::I3C_BROADCAST_ADDR; +use super::config::{DaState, DeviceEntry, I3cConfig, I3cTargetConfig, IbiState}; +use super::constants::{I3C_BROADCAST_ADDR, MAX_PRIV_XFER_CMDS, MAX_XFER_DATA_LEN}; use super::error::I3cError; use super::hardware::HardwareInterface; use super::types::{DevKind, I2cOp, I3cIbi, I3cIbiType, I3cMsg}; @@ -127,6 +127,18 @@ /// another controller, or [`I3cError::Timeout`] if the hardware's initial /// queue-reset poll timed out. pub fn start(mut self) -> Result<I3cController<'c, H, Ready>, I3cError> { + // Reject invalid timing before programming hardware. + self.config.validate_clock()?; + if let Some(static_addr) = self + .config + .target_config + .as_ref() + .and_then(|target| target.static_addr) + && (static_addr >= I3C_BROADCAST_ADDR || self.config.addrbook.is_reserved(static_addr)) + { + return Err(I3cError::InvalidArgs); + } + let bus = self.hw.bus_num() as usize; let ctx = self.hw.isr_ctx(self.config.is_secondary); if !super::hardware::register_i3c_irq_handler(bus, ctx) { @@ -156,6 +168,15 @@ // ============================================================================= impl<'c, H: HardwareInterface> I3cController<'c, H, Ready> { + #[inline] + fn ensure_primary(&self) -> Result<(), I3cError> { + if self.config.is_secondary { + Err(I3cError::Access) + } else { + Ok(()) + } + } + // ========================================================================= // Device Management // ========================================================================= @@ -167,14 +188,19 @@ /// * `desired_da` - Desired dynamic address /// * `slot` - DAT slot to use pub fn attach_i3c_dev(&mut self, pid: u64, desired_da: u8, slot: u8) -> Result<(), I3cError> { + self.ensure_primary()?; let (hw, config) = self.parts(); if desired_da == 0 || desired_da >= I3C_BROADCAST_ADDR { return Err(I3cError::InvalidArgs); } - // Bound the DAT slot: `by_pos` would silently ignore an out-of-range - // slot while the register facade aliases positions > 7 onto the last - // DAT register, corrupting whatever device lives there. - if usize::from(slot) >= super::constants::MAX_DEVICES_PER_BUS { + // GETPID returns a 48-bit value. + if pid >= (1u64 << 48) { + return Err(I3cError::InvalidArgs); + } + // Enforce both software and hardware DAT limits. + if usize::from(slot) >= super::constants::MAX_DEVICES_PER_BUS + || u16::from(slot) >= config.maxdevs + { return Err(I3cError::InvalidArgs); } if config @@ -187,6 +213,28 @@ { return Err(I3cError::DevAlreadyAttached); } + if config.addrbook.is_reserved(desired_da) { + return Err(I3cError::InvalidArgs); + } + if !config.addrbook.is_free(desired_da) { + return Err(I3cError::AddrInUse); + } + if config + .attached + .devices + .iter() + .any(|d| d.dyn_addr == desired_da) + { + return Err(I3cError::AddrInUse); + } + // PID lookups require uniqueness. + if config.attached.devices.iter().any(|d| d.pid == Some(pid)) { + return Err(I3cError::DevAlreadyAttached); + } + + // Program hardware before committing bookkeeping. + hw.attach_i3c_dev(slot.into(), desired_da) + .map_err(|_| I3cError::AddrInUse)?; let dev = DeviceEntry { kind: DevKind::I3c, @@ -201,22 +249,33 @@ mrl: 0, mwl: 0, max_ibi: 0, - ibi_en: false, + ibi_state: IbiState::Disabled, pos: Some(slot), - da_assigned: false, + da_state: DaState::Unassigned, }; - let idx = config - .attached - .attach(dev) - .map_err(|_| I3cError::AddrInUse)?; - config - .attached - .map_pos(slot, u8::try_from(idx).map_err(|_| I3cError::InvalidArgs)?); + let idx = match config.attached.attach(dev) { + Ok(idx) => idx, + Err(_) => { + hw.detach_i3c_dev(slot.into()); + return Err(I3cError::NoFreeSlot); + } + }; + let idx_u8 = match u8::try_from(idx) { + Ok(idx) => idx, + Err(_) => { + config.attached.detach(idx); + hw.detach_i3c_dev(slot.into()); + return Err(I3cError::InvalidArgs); + } + }; + if !config.attached.map_pos(slot, idx_u8) { + config.attached.detach(idx); + hw.detach_i3c_dev(slot.into()); + return Err(I3cError::NoDatPos); + } config.addrbook.mark_use(desired_da, true); - - hw.attach_i3c_dev(slot.into(), desired_da) - .map_err(|_| I3cError::AddrInUse) + Ok(()) } /// Attach a legacy I2C device to the bus. @@ -229,11 +288,15 @@ /// [`detach_i3c_dev`](Self::detach_i3c_dev) (by slot) or /// [`detach_i3c_dev_by_idx`](Self::detach_i3c_dev_by_idx). pub fn attach_i2c_dev(&mut self, static_addr: u8, slot: u8) -> Result<(), I3cError> { + self.ensure_primary()?; let (hw, config) = self.parts(); if static_addr == 0 || static_addr >= I3C_BROADCAST_ADDR { return Err(I3cError::InvalidArgs); } - if usize::from(slot) >= super::constants::MAX_DEVICES_PER_BUS { + // Reject positions outside the implemented DAT. + if usize::from(slot) >= super::constants::MAX_DEVICES_PER_BUS + || u16::from(slot) >= config.maxdevs + { return Err(I3cError::InvalidArgs); } if config @@ -246,24 +309,58 @@ { return Err(I3cError::DevAlreadyAttached); } + if config.i2c_scl_hz == 0 { + return Err(I3cError::InvalidParam); + } + if !config.addrbook.is_free(static_addr) { + return Err(if config.addrbook.is_reserved(static_addr) { + I3cError::InvalidArgs + } else { + I3cError::AddrInUse + }); + } + if config + .attached + .devices + .iter() + .any(|d| d.dyn_addr == static_addr) + { + return Err(I3cError::AddrInUse); + } + + // Program hardware before committing bookkeeping. + hw.attach_i2c_dev(slot.into(), static_addr)?; let mut dev = DeviceEntry::new_i2c(static_addr); dev.pos = Some(slot); - let idx = config - .attached - .attach(dev) - .map_err(|_| I3cError::NoFreeSlot)?; - config - .attached - .map_pos(slot, u8::try_from(idx).map_err(|_| I3cError::InvalidArgs)?); + let idx = match config.attached.attach(dev) { + Ok(idx) => idx, + Err(e) => { + hw.detach_i3c_dev(slot.into()); + return Err(e); + } + }; + let idx_u8 = match u8::try_from(idx) { + Ok(idx) => idx, + Err(_) => { + config.attached.detach(idx); + hw.detach_i3c_dev(slot.into()); + return Err(I3cError::InvalidArgs); + } + }; + if !config.attached.map_pos(slot, idx_u8) { + config.attached.detach(idx); + hw.detach_i3c_dev(slot.into()); + return Err(I3cError::NoDatPos); + } // The static address occupies the same 7-bit space as dynamic ones. config.addrbook.mark_use(static_addr, true); - - hw.attach_i2c_dev(slot.into(), static_addr) + Ok(()) } /// Write to a legacy I2C device (by static address). pub fn i2c_write(&mut self, static_addr: u8, data: &[u8]) -> Result<(), I3cError> { + self.ensure_primary()?; let (hw, config) = self.parts(); let pos = config .attached @@ -276,6 +373,7 @@ /// Read from a legacy I2C device (by static address). `out` is filled /// completely on success. pub fn i2c_read(&mut self, static_addr: u8, out: &mut [u8]) -> Result<(), I3cError> { + self.ensure_primary()?; let (hw, config) = self.parts(); let pos = config .attached @@ -292,6 +390,7 @@ data: &[u8], out: &mut [u8], ) -> Result<(), I3cError> { + self.ensure_primary()?; let (hw, config) = self.parts(); let pos = config .attached @@ -302,7 +401,12 @@ } /// Detach an I3C device by DAT position - pub fn detach_i3c_dev(&mut self, pos: usize) { + pub fn detach_i3c_dev(&mut self, pos: usize) -> Result<(), I3cError> { + self.ensure_primary()?; + // Never touch an aliased or unimplemented DAT position. + if pos >= super::constants::MAX_DEVICES_PER_BUS || pos >= usize::from(self.config.maxdevs) { + return Ok(()); + } let (hw, config) = self.parts(); // Release the dynamic address (parity with `detach_i3c_dev_by_idx`), // or detaching by position would leak it in the address book forever. @@ -313,24 +417,32 @@ .copied() .flatten() .and_then(|idx| config.attached.devices.get(usize::from(idx))) - .map(|dev| dev.dyn_addr); - if let Some(da) = da + .map(|dev| (dev.dyn_addr, dev.da_state)); + if da.is_some_and(|(_, state)| state == DaState::Unknown) { + return Err(I3cError::Busy); + } + if let Some((da, _)) = da && da != 0 { config.addrbook.mark_use(da, false); } config.attached.detach_by_pos(pos); hw.detach_i3c_dev(pos); + Ok(()) } /// Detach an I3C device by device index - pub fn detach_i3c_dev_by_idx(&mut self, dev_idx: usize) { + pub fn detach_i3c_dev_by_idx(&mut self, dev_idx: usize) -> Result<(), I3cError> { + self.ensure_primary()?; let (hw, config) = self.parts(); // `get` (not `[dev_idx]`) keeps this panic-free for the `no_panics` // analysis; an out-of-range index is simply a no-op. let Some(dev) = config.attached.devices.get(dev_idx) else { - return; + return Ok(()); }; + if dev.da_state == DaState::Unknown { + return Err(I3cError::Busy); + } if dev.dyn_addr != 0 { let dyn_addr = dev.dyn_addr; @@ -343,6 +455,7 @@ } config.attached.detach(dev_idx); + Ok(()) } // ========================================================================= @@ -375,12 +488,14 @@ /// // More aggressive recovery /// ctrl.recover_bus(18); /// ``` - pub fn recover_bus(&mut self, scl_toggles: u32) { + pub fn recover_bus(&mut self, scl_toggles: u32) -> Result<(), I3cError> { + self.ensure_primary()?; let (hw, _) = self.parts(); hw.enter_sw_mode(); hw.i3c_toggle_scl_in(scl_toggles); hw.gen_internal_stop(); hw.exit_sw_mode(); + Ok(()) } /// Perform full bus recovery with controller reset @@ -408,9 +523,14 @@ /// [`I3cError::Timeout`] if the controller reset bits did not self-clear — /// the engine is wedged beyond what software recovery can fix. pub fn recover_bus_full(&mut self, reset_mask: u32) -> Result<(), I3cError> { - self.recover_bus(8); + self.recover_bus(8)?; let (hw, _) = self.parts(); - hw.reset_ctrl(reset_mask) + let result = hw.reset_ctrl(reset_mask); + if result.is_err() { + // Block transfers until a successful init. + hw.mark_xfer_faulted(); + } + result } // ========================================================================= @@ -420,6 +540,9 @@ /// Allocate a dynamic address from `start_addr`. #[inline] pub fn alloc_dynamic_address_from(&mut self, start_addr: u8) -> Option<u8> { + if self.ensure_primary().is_err() { + return None; + } let (_, config) = self.parts(); config.addrbook.alloc_from(start_addr) } @@ -432,14 +555,15 @@ #[inline] #[must_use] pub fn target_dynamic_address(&self) -> Option<u8> { - super::hardware::isr_events(self.hw.bus_num() as usize) + let addr = super::hardware::isr_events(self.hw.bus_num() as usize) .dyn_addr() - .or_else(|| self.config.target_config.as_ref().and_then(|t| t.addr)) + .or_else(|| self.config.target_config.as_ref().and_then(|t| t.addr))?; + // Filter stale or reserved values. + (addr != 0 && !self.config.addrbook.is_reserved(addr)).then_some(addr) } /// Max read/write lengths `(mrl, mwl)` the bus master pushed to this - /// target via SETMRL/SETMWL, if any update was observed (latched by the - /// ISR from `SLV_MAX_LEN`). Target mode only. + /// target via SETMRL/SETMWL, if any update was observed. #[inline] #[must_use] pub fn target_max_lengths(&self) -> Option<(u16, u16)> { @@ -448,37 +572,67 @@ /// Set the device's IBI mandatory data byte and enable IBI delivery for `addr`. pub fn enable_ibi(&mut self, addr: u8, mdb: u8) -> Result<(), I3cError> { + self.ensure_primary()?; let (hw, config) = self.parts(); hw.set_ibi_mdb(mdb); - hw.ibi_enable(config, addr) + let result = hw.ibi_enable(config, addr); + if let Some(dev) = config + .attached + .devices + .iter_mut() + .find(|d| d.dyn_addr == addr) + { + dev.ibi_state = match result { + Ok(()) => IbiState::Enabled, + Err(I3cError::Timeout | I3cError::RespError) => IbiState::Unknown, + Err(_) => dev.ibi_state, + }; + } + result } /// Disable IBI delivery for `addr` (DISEC + reject its SIRs). pub fn disable_ibi(&mut self, addr: u8) -> Result<(), I3cError> { + self.ensure_primary()?; let (hw, config) = self.parts(); - hw.ibi_disable(config, addr) + let result = hw.ibi_disable(config, addr); + if let Some(dev) = config + .attached + .devices + .iter_mut() + .find(|d| d.dyn_addr == addr) + { + dev.ibi_state = match result { + Ok(()) => IbiState::Disabled, + Err(I3cError::Timeout | I3cError::RespError | I3cError::AddressNack) => { + IbiState::Unknown + } + Err(_) => dev.ibi_state, + }; + } + result } - /// Re-run the full hardware initialization on a live controller. - /// - /// Recovery hammer for an engine wedged beyond what - /// [`recover_bus_full`](Self::recover_bus_full) can fix (the vendor C - /// driver's `target_rst_worker` equivalent). The ISR registration is left - /// untouched. Side effects: in target mode the dynamic address is dropped - /// (the bus master must re-run DAA) and SIRs are blocked until the next - /// DA assignment; in master mode the DAT slots of attached devices are - /// re-programmed from the bookkeeping (the bus targets keep their - /// addresses — only this controller was reset), but IBIs must be - /// re-enabled via [`enable_ibi`](Self::enable_ibi). + /// Re-run full hardware initialization and rebuild attached DAT entries. pub fn reinit(&mut self) -> Result<(), I3cError> { let (hw, config) = self.parts(); hw.init(config)?; + for dev in &mut config.attached.devices { + dev.ibi_state = IbiState::Disabled; + } for i in 0..config.attached.devices.len() { let Some(dev) = config.attached.devices.get(i) else { continue; }; if let Some(pos) = dev.pos { - let _ = hw.attach_i3c_dev(pos.into(), dev.dyn_addr); + let result = match dev.kind { + DevKind::I3c => hw.attach_i3c_dev(pos.into(), dev.dyn_addr), + DevKind::I2c => hw.attach_i2c_dev(pos.into(), dev.static_addr), + }; + if result.is_err() { + hw.mark_xfer_faulted(); + } + result?; } } cortex_m::asm::dmb(); @@ -487,6 +641,7 @@ /// Issue a private read to `pid`, returning the number of received bytes. pub fn priv_read(&mut self, pid: u64, out: &mut [u8]) -> Result<u32, I3cError> { + self.ensure_primary()?; let (hw, config) = self.parts(); let actual_len = u32::try_from(out.len()).map_err(|_| I3cError::InvalidArgs)?; let mut msgs = [I3cMsg { @@ -503,6 +658,7 @@ /// Issue a private write to `pid`. pub fn priv_write(&mut self, pid: u64, data: &mut [u8]) -> Result<(), I3cError> { + self.ensure_primary()?; let (hw, config) = self.parts(); let actual_len = u32::try_from(data.len()).map_err(|_| I3cError::InvalidArgs)?; let mut msgs = [I3cMsg { @@ -540,22 +696,39 @@ &mut self, static_address: SevenBitAddress, ) -> Result<SevenBitAddress, I3cError> { + self.ensure_primary()?; let (hw, config) = self.parts(); let slot = config .attached .pos_of_addr(static_address) .ok_or(I3cError::AddrInUse)?; - hw.do_entdaa(config, slot.into()) - .map_err(|_| I3cError::AddrInUse)?; - - let pid = ccc::ccc_getpid(hw, config, static_address).map_err(|_| I3cError::Invalid)?; - let dev_idx = config .attached .find_dev_idx_by_addr(static_address) .ok_or(I3cError::Other)?; + // Do not repeat ENTDAA for an address that may already be claimed. + let already_claimed = config + .attached + .devices + .get(dev_idx) + .is_some_and(|d| d.da_state != DaState::Unassigned); + if !already_claimed { + match hw.do_entdaa(config, slot.into()) { + Ok(()) => config.mark_da_unknown(dev_idx, None), + Err(e @ (I3cError::Timeout | I3cError::RespError)) => { + // The target may have latched the DAT address before the + // controller lost completion. + config.mark_da_unknown(dev_idx, None); + return Err(e); + } + Err(e) => return Err(e), + } + } + + let pid = ccc::ccc_getpid(hw, config, static_address)?; + let old_pid = config .attached .devices @@ -566,12 +739,20 @@ if let Some(op) = old_pid && pid != op { + // Preserve the claimed state after a PID mismatch. + if let Some(dev) = config.attached.devices.get_mut(dev_idx) { + dev.da_state = DaState::Unknown; + } return Err(I3cError::Other); } - let bcr = ccc::ccc_getbcr(hw, config, static_address).map_err(|_| I3cError::Invalid)?; - // DCR is informational — a device that NACKs GETDCR still works. - let dcr = ccc::ccc_getdcr(hw, config, static_address).unwrap_or(0); + let bcr = ccc::ccc_getbcr(hw, config, static_address)?; + // DCR is optional; only an address NACK is benign. + let dcr = match ccc::ccc_getdcr(hw, config, static_address) { + Ok(dcr) => dcr, + Err(I3cError::AddressNack) => 0, + Err(e) => return Err(e), + }; { let dev = config @@ -583,7 +764,7 @@ dev.pid = Some(pid); dev.bcr = bcr; dev.dcr = dcr; - dev.da_assigned = true; + dev.da_state = DaState::Verified; } let dyn_addr: SevenBitAddress = config @@ -593,8 +774,15 @@ .ok_or(I3cError::Other)? .dyn_addr; - hw.ibi_enable(config, dyn_addr) - .map_err(|_| I3cError::Other)?; + let ibi_result = hw.ibi_enable(config, dyn_addr); + if let Some(dev) = config.attached.devices.get_mut(dev_idx) { + dev.ibi_state = match ibi_result { + Ok(()) => IbiState::Enabled, + Err(I3cError::Timeout | I3cError::RespError) => IbiState::Unknown, + Err(_) => dev.ibi_state, + }; + } + ibi_result?; Ok(dyn_addr) } @@ -616,11 +804,14 @@ /// is parked on a freshly allocated address so it stops answering /// subsequent ENTDAAs. /// - /// Returns the number of devices verified in this run. Exits when ENTDAA - /// reports no more unassigned devices (NACK/timeout). IBIs are not - /// enabled here — call [`enable_ibi`](Self::enable_ibi) per device + /// Returns the number of devices verified in this run, but only when every + /// attached device that needed an address was verified. If ENTDAA reports + /// no more responders while entries are still pending, returns + /// [`I3cError::DaaNack`] instead of reporting a partial success. IBIs are + /// not enabled here — call [`enable_ibi`](Self::enable_ibi) per device /// afterwards. pub fn bus_daa(&mut self) -> Result<u32, I3cError> { + self.ensure_primary()?; let (hw, config) = self.parts(); let ndevs = config.attached.by_pos.len(); @@ -632,7 +823,7 @@ }; if dev.kind == DevKind::I3c && dev.pid.is_some() - && !dev.da_assigned + && dev.da_state == DaState::Unassigned && let Some(pos) = dev.pos { // pos < 8 enforced by attach_i3c_dev. @@ -642,6 +833,7 @@ let mut verified = 0u32; let mut pos = 0usize; + let mut normal_nack = false; // Hang guard only: every lap either clears a `need` bit, parks an // unsolicited device (finite), or exits via the ENTDAA break below. let mut budget = 8 * (ndevs as u32); @@ -670,15 +862,49 @@ continue; }; - if hw.do_entdaa(config, pos as u32).is_err() { - // NACK/timeout: nothing unassigned left on the bus. - break; + let provisional_idx = config + .attached + .by_pos + .get(pos) + .copied() + .flatten() + .map(usize::from); + + match hw.do_entdaa(config, pos as u32) { + Ok(()) => {} + // Address NACK normally ends DAA. + Err(I3cError::DaaNack) => { + normal_nack = true; + break; + } + Err(e @ (I3cError::Timeout | I3cError::RespError)) => { + // ENTDAA may have assigned this slot before completion was + // lost. Never retry it as unassigned. + if let Some(idx) = provisional_idx { + config.mark_da_unknown(idx, None); + } + return Err(e); + } + Err(e) => return Err(e), } - let Ok(pid) = ccc::ccc_getpid(hw, config, addr) else { - // Winner could not be identified; retry this slot next lap. - pos = (pos + 1) % ndevs; - continue; + // Mark the claimed address unknown until GETPID identifies it. + if let Some(idx) = provisional_idx { + config.mark_da_unknown(idx, None); + } + + let pid = match ccc::ccc_getpid(hw, config, addr) { + Ok(pid) => pid, + // The address is claimed; retrying ENTDAA could collide. + Err(e) => { + // Only RSTDAA can safely clear this unknown owner. + if let Some(dev) = + provisional_idx.and_then(|idx| config.attached.devices.get_mut(idx)) + { + dev.da_state = DaState::Unknown; + } + return Err(e); + } }; let owner = config @@ -695,47 +921,98 @@ .map_or(addr, |d| d.desired_da); if expected == addr { // The intended device answered its own slot. - let bcr = ccc::ccc_getbcr(hw, config, addr).unwrap_or(0); - let dcr = ccc::ccc_getdcr(hw, config, addr).unwrap_or(0); + let bcr = ccc::ccc_getbcr(hw, config, addr)?; + let dcr = match ccc::ccc_getdcr(hw, config, addr) { + Ok(dcr) => dcr, + Err(I3cError::AddressNack) => 0, + Err(e) => return Err(e), + }; if let Some(dev) = config.attached.devices.get_mut(idx) { dev.bcr = bcr; dev.dcr = dcr; - dev.da_assigned = true; + dev.da_state = DaState::Verified; } need &= !(1u32 << pos); verified += 1; - } else if ccc::ccc_setnewda_bus_only(hw, config, addr, expected).is_ok() { - // Wrong device won this slot: it now sits on its own - // expected address (its own DAT slot already holds - // that address), so it is done... - let bcr = ccc::ccc_getbcr(hw, config, expected).unwrap_or(0); - let dcr = ccc::ccc_getdcr(hw, config, expected).unwrap_or(0); - if let Some(dev) = config.attached.devices.get_mut(idx) { - dev.bcr = bcr; - dev.dcr = dcr; - dev.da_assigned = true; + } else { + let setnewda_result = + ccc::ccc_setnewda_bus_only(hw, config, addr, expected); + // Probe ambiguous moves before freeing the old slot. + let move_ambiguous = matches!( + setnewda_result, + Err(I3cError::Timeout | I3cError::RespError) + ); + if setnewda_result.is_ok() || move_ambiguous { + if setnewda_result.is_ok() + && let Some(dev) = + provisional_idx.and_then(|i| config.attached.devices.get_mut(i)) + { + dev.da_state = DaState::Unassigned; + } + if let Some(dev) = config.attached.devices.get_mut(idx) { + dev.da_state = DaState::Unknown; + } + // Verify the winner at its expected address. + let bcr = ccc::ccc_getbcr(hw, config, expected)?; + let dcr = match ccc::ccc_getdcr(hw, config, expected) { + Ok(dcr) => dcr, + Err(I3cError::AddressNack) => 0, + Err(e) => return Err(e), + }; + if move_ambiguous + && let Some(dev) = + provisional_idx.and_then(|i| config.attached.devices.get_mut(i)) + { + // The probe confirmed the move. + dev.da_state = DaState::Unassigned; + } + if let Some(dev) = config.attached.devices.get_mut(idx) { + dev.bcr = bcr; + dev.dcr = dcr; + dev.da_state = DaState::Verified; + } + if let Some(own_pos) = config + .attached + .pos_of(idx) + .or_else(|| config.attached.devices.get(idx).and_then(|d| d.pos)) + { + need &= !(1u32 << u32::from(own_pos)); + } + verified += 1; + // Retry this slot for its intended owner. + } else { + // A definite move failure leaves the slot occupied. + return Err(setnewda_result.err().unwrap_or(I3cError::Other)); } - if let Some(own_pos) = config - .attached - .pos_of(idx) - .or_else(|| config.attached.devices.get(idx).and_then(|d| d.pos)) - { - need &= !(1u32 << u32::from(own_pos)); - } - verified += 1; - // ...and this slot's bit stays set so its intended - // owner gets the next ENTDAA here. } } None => { // Unknown PID: park it on a fresh address so it stops // answering ENTDAA for slots it does not own. let Some(park) = config.addrbook.alloc_from(8) else { - break; + return Err(I3cError::AddrExhausted); }; config.addrbook.mark_use(park, true); - if ccc::ccc_setnewda_bus_only(hw, config, addr, park).is_err() { - config.addrbook.mark_use(park, false); + match ccc::ccc_setnewda_bus_only(hw, config, addr, park) { + Ok(()) => { + if let Some(dev) = + provisional_idx.and_then(|i| config.attached.devices.get_mut(i)) + { + dev.da_state = DaState::Unassigned; + } + } + Err(e @ (I3cError::Timeout | I3cError::RespError)) => { + // Keep an ambiguously adopted parking address reserved. + if let Some(idx) = provisional_idx { + config.mark_da_unknown(idx, Some(park)); + } + return Err(e); + } + Err(e) => { + // Release a definitely unused parking address. + config.addrbook.mark_use(park, false); + return Err(e); + } } // Retry this slot without advancing. continue; @@ -745,11 +1022,21 @@ pos = (pos + 1) % ndevs; } - Ok(verified) + if need == 0 { + Ok(verified) + } else if normal_nack { + // A normal ENTDAA NACK ends discovery, but attached devices are + // still pending. Do not report a partial assignment as success. + Err(I3cError::DaaNack) + } else { + // The retry budget expired with assignments pending. + Err(I3cError::AddrExhausted) + } } /// Acknowledge an IBI from `address` (validates the device is known). pub fn acknowledge_ibi(&mut self, address: SevenBitAddress) -> Result<(), I3cError> { + self.ensure_primary()?; let (_, config) = self.parts(); let dev_idx = config .attached @@ -763,7 +1050,7 @@ .devices .get(dev_idx) .ok_or(I3cError::Other)?; - if dev.pid.is_none() { + if dev.pid.is_none() || dev.da_state != DaState::Verified { return Err(I3cError::Other); } @@ -774,33 +1061,36 @@ /// after receiving a hot-join IBI; nothing else is required here. #[allow(clippy::unused_self)] pub fn handle_hot_join(&mut self) -> Result<(), I3cError> { + self.ensure_primary()?; Ok(()) } /// Bus speed is fixed on the AST1060 controller; this is a no-op. #[allow(clippy::unused_self)] pub fn set_bus_speed(&mut self) -> Result<(), I3cError> { + self.ensure_primary()?; Ok(()) } - /// The AST1060 controller does not support multi-master; this is a no-op. + /// Multi-master mode is unsupported. #[allow(clippy::unused_self)] pub fn request_mastership(&mut self) -> Result<(), I3cError> { - Ok(()) + Err(I3cError::Access) } // --- Target (secondary) mode callbacks --- - /// Initialize target mode with `own_addr` (sets the static/target address). + /// Initialize target-mode software state with `own_addr`. pub fn target_init(&mut self, own_addr: u8) { let (_, config) = self.parts(); - if let Some(t) = config.target_config.as_mut() { - if t.addr.is_none() { - t.addr = Some(own_addr); + if let Some(target) = config.target_config.as_mut() { + if target.addr.is_none() { + target.addr = Some(own_addr); } } else { - config.target_config = - Some(I3cTargetConfig::new(0, Some(own_addr), /* mdb */ 0xae)); + let mut target = I3cTargetConfig::new(0, Some(own_addr), /* mdb */ 0xae); + target.addr = Some(own_addr); + config.target_config = Some(target); } } @@ -821,8 +1111,11 @@ /// caller owns that delay — wait ~1 s after the `TargetDaAssignment` work /// item before calling this if the bus master is slow to settle. pub fn target_on_dynamic_address_assigned(&mut self) { - let da = super::hardware::isr_events(self.hw.bus_num() as usize).dyn_addr(); - if let (Some(da), Some(tc)) = (da, self.config.target_config.as_mut()) { + // Require an ISR-confirmed dynamic address. + let Some(da) = super::hardware::isr_events(self.hw.bus_num() as usize).dyn_addr() else { + return; + }; + if let Some(tc) = self.config.target_config.as_mut() { tc.addr = Some(da); } self.config.sir_allowed_by_sw = true; @@ -863,7 +1156,7 @@ match rc { Ok(()) => Ok(buffer.len() + payload.len()), - _ => Ok(0), + Err(e) => Err(e), } } } @@ -888,6 +1181,7 @@ address: SevenBitAddress, operations: &mut [embedded_hal::i2c::Operation<'_>], ) -> Result<(), I3cError> { + self.ensure_primary()?; let (hw, config) = self.parts(); let pos = config .attached @@ -897,13 +1191,25 @@ if operations.is_empty() { return Ok(()); } + if operations.len() > MAX_PRIV_XFER_CMDS { + return Err(I3cError::TooManyMsgs); + } + // Validate before borrowing caller buffers. + for op in operations.iter() { + let len = match op { + embedded_hal::i2c::Operation::Write(buf) => buf.len(), + embedded_hal::i2c::Operation::Read(buf) => buf.len(), + }; + if len == 0 || len > MAX_XFER_DATA_LEN { + return Err(I3cError::InvalidArgs); + } + } - let mut ops: heapless::Vec<I2cOp<'_>, { super::constants::MAX_PRIV_XFER_CMDS }> = - heapless::Vec::new(); + let mut ops: heapless::Vec<I2cOp<'_>, MAX_PRIV_XFER_CMDS> = heapless::Vec::new(); for op in operations.iter_mut() { let mapped = match op { - embedded_hal::i2c::Operation::Write(b) => I2cOp::Write(b), - embedded_hal::i2c::Operation::Read(b) => I2cOp::Read(core::mem::take(b)), + embedded_hal::i2c::Operation::Write(buf) => I2cOp::Write(&**buf), + embedded_hal::i2c::Operation::Read(buf) => I2cOp::Read(&mut **buf), }; ops.push(mapped).map_err(|_| I3cError::TooManyMsgs)?; }
diff --git a/target/ast10x0/peripherals/i3c/error.rs b/target/ast10x0/peripherals/i3c/error.rs index ae34a3d..ac79e3c 100644 --- a/target/ast10x0/peripherals/i3c/error.rs +++ b/target/ast10x0/peripherals/i3c/error.rs
@@ -33,6 +33,15 @@ IoError, /// Invalid operation or state Invalid, + /// Transfer completed but response status indicated failure; unlike + /// `Timeout`, it may already have taken effect at the target. + RespError, + /// ENTDAA address-phase NACK: no unassigned device answered (expected + /// end of a DAA walk, not a fault like other `RespError` cases). + DaaNack, + /// Target-address phase NACK on a direct transfer, e.g. unsupported + /// optional CCC — kept distinct from other response failures. + AddressNack, /// Address already in use AddrInUse, /// Address space exhausted @@ -65,6 +74,9 @@ Self::Access => write!(f, "access denied"), Self::IoError => write!(f, "I/O error"), Self::Invalid => write!(f, "invalid operation"), + Self::RespError => write!(f, "transfer response indicated failure"), + Self::DaaNack => write!(f, "ENTDAA: no unassigned device answered"), + Self::AddressNack => write!(f, "target address was not acknowledged"), Self::AddrInUse => write!(f, "address in use"), Self::AddrExhausted => write!(f, "address space exhausted"), Self::NoFreeSlot => write!(f, "no free slot"),
diff --git a/target/ast10x0/peripherals/i3c/hardware.rs b/target/ast10x0/peripherals/i3c/hardware.rs index 2d34cd4..6b15260 100644 --- a/target/ast10x0/peripherals/i3c/hardware.rs +++ b/target/ast10x0/peripherals/i3c/hardware.rs
@@ -29,7 +29,7 @@ use critical_section::Mutex; use super::ccc::{ccc_events_set, CccPayload}; -use super::config::{I3cConfig, I3C_MIN_CORE_CLK_SDR}; +use super::config::{DaState, I3cConfig, I3C_MIN_CORE_CLK_SDR}; use super::constants::{ bit, field_get, field_prep, CM_TFR_STS_MASTER_HALT, CM_TFR_STS_TARGET_HALT, COMMAND_ATTR_ADDR_ASSGN_CMD, COMMAND_ATTR_SLAVE_DATA_CMD, COMMAND_ATTR_XFER_ARG, @@ -37,31 +37,35 @@ COMMAND_PORT_CMD, COMMAND_PORT_CP, COMMAND_PORT_DBP, COMMAND_PORT_DEV_COUNT, COMMAND_PORT_DEV_INDEX, COMMAND_PORT_READ_TRANSFER, COMMAND_PORT_ROC, COMMAND_PORT_SPEED, COMMAND_PORT_TID, COMMAND_PORT_TOC, DEV_ADDR_TABLE_IBI_MDB, DEV_ADDR_TABLE_IBI_PEC, - DEV_ADDR_TABLE_LEGACY_I2C_DEV, DEV_ADDR_TABLE_MR_REJECT, DEV_ADDR_TABLE_STATIC_ADDR, - DEV_ADDR_TABLE_SIR_REJECT, I3CG_REG1_SCL_IN_SW_MODE_EN, I3CG_REG1_SCL_IN_SW_MODE_VAL, + DEV_ADDR_TABLE_LEGACY_I2C_DEV, DEV_ADDR_TABLE_MR_REJECT, DEV_ADDR_TABLE_SIR_REJECT, + DEV_ADDR_TABLE_STATIC_ADDR, I3CG_REG1_SCL_IN_SW_MODE_EN, I3CG_REG1_SCL_IN_SW_MODE_VAL, I3CG_REG1_SDA_IN_SW_MODE_EN, I3CG_REG1_SDA_IN_SW_MODE_VAL, I3C_AST10X0_MIPI_MANUF_ID, I3C_BCR_IBI_PAYLOAD_HAS_DATA_BYTE, I3C_BUS_FREE_TIMING_RESET, I3C_BUS_I2C_FMP_TF_MAX_NS, I3C_BUS_I2C_FMP_THIGH_MIN_NS, I3C_BUS_I2C_FMP_TLOW_MIN_NS, I3C_BUS_I2C_FMP_TR_MAX_NS, I3C_BUS_I2C_FM_TF_MAX_NS, I3C_BUS_I2C_FM_THIGH_MIN_NS, I3C_BUS_I2C_FM_TLOW_MIN_NS, I3C_BUS_I2C_FM_TR_MAX_NS, I3C_BUS_I2C_STD_TF_MAX_NS, I3C_BUS_I2C_STD_THIGH_MIN_NS, I3C_BUS_I2C_STD_TLOW_MIN_NS, I3C_BUS_I2C_STD_TR_MAX_NS, I3C_BUS_THIGH_MAX_NS, I3C_CCC_DEVCTRL, - I3C_CCC_ENTDAA, I3C_CCC_EVT_INTR, I3C_CCC_SETHID, I3C_CTRL_POLL_DELAY_NS, - I3C_DEFAULT_STATIC_ADDR, I3C_GLOBAL_RESET_DEASSERT_MASK, I3C_IBI_DATA_THRESHOLD_MAX, - I3C_INIT_POLL_DELAY_NS, I3C_INTR_STATUS_ALL_BITS, I3C_MSG_READ, I3C_OP_TIMEOUT_US, - I3C_POLL_MAX_ITERS, IBIQ_STATUS_IBI_DATA_LEN, IBIQ_STATUS_IBI_DATA_LEN_SHIFT, - SLV_EVENT_CTRL_MRL_UPD, SLV_EVENT_CTRL_MWL_UPD, - IBIQ_STATUS_IBI_ID, IBIQ_STATUS_IBI_ID_SHIFT, INTR_CCC_UPDATED_STAT, INTR_DYN_ADDR_ASSGN_STAT, - INTR_IBI_THLD_STAT, INTR_RESP_READY_STAT, INTR_TRANSFER_ABORT_STAT, INTR_TRANSFER_ERR_STAT, - MAX_CMDS, MAX_PRIV_XFER_CMDS, MAX_XFER_DATA_LEN, NSEC_PER_SEC, RESET_CTRL_ALL, - RESET_CTRL_QUEUES, RESET_CTRL_XFER_QUEUES, RESPONSE_ERROR_IBA_NACK, - RESPONSE_PORT_DATA_LEN_MASK, RESPONSE_PORT_DATA_LEN_SHIFT, RESPONSE_PORT_ERR_STATUS_MASK, - RESPONSE_PORT_ERR_STATUS_SHIFT, RESPONSE_PORT_TID_MASK, RESPONSE_PORT_TID_SHIFT, - SDA_TX_HOLD_MASK, SDA_TX_HOLD_MAX, SDA_TX_HOLD_MIN, SLV_DCR_MASK, SLV_EVENT_CTRL_SIR_EN, + I3C_CCC_ENTDAA, I3C_CCC_EVT_INTR, I3C_CCC_GETBCR, I3C_CCC_GETDCR, I3C_CCC_GETMRL, + I3C_CCC_GETMWL, I3C_CCC_GETMXDS, I3C_CCC_GETPID, I3C_CCC_GETSTATUS, I3C_CCC_SETHID, + I3C_CCC_SETNEWDA, I3C_CTRL_POLL_DELAY_NS, I3C_DEFAULT_STATIC_ADDR, + I3C_GLOBAL_RESET_DEASSERT_MASK, I3C_IBI_DATA_THRESHOLD_MAX, I3C_INIT_POLL_DELAY_NS, + I3C_INTR_STATUS_ALL_BITS, I3C_MSG_READ, I3C_OP_TIMEOUT_US, I3C_POLL_MAX_ITERS, + IBIQ_STATUS_IBI_DATA_LEN, IBIQ_STATUS_IBI_DATA_LEN_SHIFT, IBIQ_STATUS_IBI_ID, + IBIQ_STATUS_IBI_ID_SHIFT, INTR_CCC_UPDATED_STAT, INTR_DYN_ADDR_ASSGN_STAT, INTR_IBI_THLD_STAT, + INTR_RESP_READY_STAT, INTR_TRANSFER_ABORT_STAT, INTR_TRANSFER_ERR_STAT, MAX_CMDS, + MAX_DEVICES_PER_BUS, MAX_PRIV_XFER_CMDS, MAX_XFER_DATA_LEN, NSEC_PER_SEC, RESET_CTRL_ALL, + RESET_CTRL_QUEUES, RESET_CTRL_XFER_QUEUES, RESPONSE_ERROR_ADDRESS_NACK, + RESPONSE_ERROR_IBA_NACK, RESPONSE_PORT_DATA_LEN_MASK, RESPONSE_PORT_DATA_LEN_SHIFT, + RESPONSE_PORT_ERR_STATUS_MASK, RESPONSE_PORT_ERR_STATUS_SHIFT, RESPONSE_PORT_TID_MASK, + RESPONSE_PORT_TID_SHIFT, SDA_TX_HOLD_MASK, SDA_TX_HOLD_MAX, SDA_TX_HOLD_MIN, SLV_DCR_MASK, + SLV_EVENT_CTRL_MRL_UPD, SLV_EVENT_CTRL_MWL_UPD, SLV_EVENT_CTRL_SIR_EN, }; use super::error::I3cError as I3cDrvError; use super::error::I3cError; use super::ibi as ibi_workq; -use super::types::{Completion, I2cOp, I3cCmd, I3cIbi, I3cMsg, I3cXfer, SpeedI2c, SpeedI3c, Tid}; +use super::types::{ + Completion, DevKind, I2cOp, I3cCmd, I3cIbi, I3cMsg, I3cXfer, SpeedI2c, SpeedI3c, Tid, +}; use super::registers::I3cRegisters; use core::sync::atomic::{AtomicBool, AtomicU32, Ordering}; @@ -127,10 +131,8 @@ pending: AtomicU32, /// Dynamic address assigned by the bus master; bit 8 = valid. dyn_addr: AtomicU32, - /// Raw `SLV_MAX_LEN` (MRL in bits 31:16, MWL in bits 15:0) latched by the - /// ISR when the bus master updates it via SETMRL/SETMWL. + /// Raw `SLV_MAX_LEN` (MRL in bits 31:16, MWL in bits 15:0). slv_max_len: AtomicU32, - /// `slv_max_len` holds a master-written value (not reset state). slv_max_len_valid: AtomicBool, /// A deferred fault: the ISR observed a halted/errored engine and left /// recovery (halt/resume sequencing needs the wait policy) to the thread. @@ -159,6 +161,10 @@ self.pending.swap(0, Ordering::AcqRel) } + pub(crate) fn clear_pending(&self) { + self.pending.store(0, Ordering::Release); + } + /// Atomically take (read-and-clear) the deferred-fault flag. pub(crate) fn take_fault(&self) -> bool { self.fault.swap(false, Ordering::AcqRel) @@ -174,14 +180,23 @@ } } - /// Max read/write lengths `(mrl, mwl)` the bus master set via - /// SETMRL/SETMWL, if any update was observed. pub(crate) fn max_len(&self) -> Option<(u16, u16)> { if !self.slv_max_len_valid.load(Ordering::Acquire) { return None; } - let v = self.slv_max_len.load(Ordering::Acquire); - Some(((v >> 16) as u16, (v & 0xffff) as u16)) + let value = self.slv_max_len.load(Ordering::Acquire); + Some(((value >> 16) as u16, (value & 0xffff) as u16)) + } + + /// Clear all ISR-latched state after reinitialization. + pub(crate) fn reset(&self) { + self.pending.store(0, Ordering::Release); + self.dyn_addr.store(0, Ordering::Release); + self.slv_max_len.store(0, Ordering::Release); + self.slv_max_len_valid.store(false, Ordering::Release); + self.fault.store(false, Ordering::Release); + self.target_ibi_done.reset(); + self.target_data_done.reset(); } } @@ -301,7 +316,9 @@ if status & INTR_DYN_ADDR_ASSGN_STAT != 0 { let da = u32::from(regs.dynamic_addr()); events.dyn_addr.store(0x100 | da, Ordering::Release); - let _ = ibi_workq::i3c_ibi_work_enqueue_target_da_assignment(bus); + if !ibi_workq::i3c_ibi_work_enqueue_target_da_assignment(bus) { + events.fault.store(true, Ordering::Release); + } } if (status & INTR_RESP_READY_STAT) != 0 { @@ -312,8 +329,6 @@ // Read-and-clear the event; if the engine halted, defer the // resume sequencing (it needs the wait policy) to the thread. let event = regs.read_slv_event_ctrl(); - // Latch SETMRL/SETMWL updates before the write-back clears the - // update flags (the thread reads them via `max_len`). if event & (SLV_EVENT_CTRL_MRL_UPD | SLV_EVENT_CTRL_MWL_UPD) != 0 { events .slv_max_len @@ -400,10 +415,12 @@ if rx_len > n { regs.rx_fifo_drain(rx_len - n); } - let _ = ibi_workq::i3c_ibi_work_enqueue_target_master_write( + if !ibi_workq::i3c_ibi_work_enqueue_target_master_write( bus, buf.get(..n).unwrap_or(&[]), - ); + ) { + events.fault.store(true, Ordering::Release); + } } if tid == Tid::TargetIbi as usize { @@ -426,6 +443,7 @@ if nibis == 0 { return; } + let events = isr_events(bus); for _ in 0..nibis { let reg = regs.ibi_fifo_pop(); @@ -440,8 +458,8 @@ let rnw = (ibi_id & 1) != 0; if ibi_addr != 2 && rnw { - // SIR - let mut ibi_buf: [u8; 2] = [0u8; 2]; + // Preserve the full supported SIR payload. + let mut ibi_buf = [0u8; ibi_workq::IBI_DATA_MAX as usize]; let take = core::cmp::min(ibi_data_len, ibi_buf.len()); if let Some(dst) = ibi_buf.get_mut(..take) { regs.ibi_fifo_read(dst); @@ -454,14 +472,18 @@ if ibi_data_len > consumed { regs.ibi_fifo_drain(ibi_data_len - consumed); } - let _ = ibi_workq::i3c_ibi_work_enqueue_target_irq( + if !ibi_workq::i3c_ibi_work_enqueue_target_irq( bus, ibi_addr as u8, ibi_buf.get(..take).unwrap_or(&[]), - ); + ) { + events.fault.store(true, Ordering::Release); + } } else if ibi_addr == 2 && !rnw { // hot-join - let _ = ibi_workq::i3c_ibi_work_enqueue_hotjoin(bus); + if !ibi_workq::i3c_ibi_work_enqueue_hotjoin(bus) { + events.fault.store(true, Ordering::Release); + } } else { // normal ibi regs.ibi_fifo_drain(ibi_data_len); @@ -637,6 +659,9 @@ pid: u64, msgs: &mut [I3cMsg], ) -> Result<(), I3cError>; + + /// Block transfers until a successful [`HardwareCore::init`]. + fn mark_xfer_faulted(&mut self); } // ============================================================================= @@ -668,7 +693,7 @@ /// Target (secondary) mode operations pub trait HardwareTarget { /// Write data to target TX buffer - fn target_tx_write(&mut self, buf: &[u8]); + fn target_tx_write(&mut self, config: &I3cConfig, buf: &[u8]) -> Result<(), I3cError>; /// Raise a Hot-Join IBI (target mode) fn target_ibi_raise_hj(&self, config: &mut I3cConfig) -> Result<(), I3cError>; @@ -765,6 +790,8 @@ /// suggested wait window in nanoseconds (advisory). Private so external /// code cannot swap the wait policy out from under an active driver. yield_fn: Y, + /// Sticky fault cleared by successful initialization. + xfer_faulted: AtomicBool, } impl<Y: FnMut(u32)> Ast1060I3c<Y> { @@ -783,7 +810,11 @@ pub unsafe fn new(bus: u8, yield_fn: Y) -> Option<Self> { // SAFETY: forwarded — see this function's contract above. let regs = unsafe { I3cRegisters::new(bus) }?; - Some(Self { regs, yield_fn }) + Some(Self { + regs, + yield_fn, + xfer_faulted: AtomicBool::new(false), + }) } /// Bus index this driver was constructed for. @@ -799,9 +830,12 @@ /// bindings stay "used", but performs no formatting or I/O. The leading /// `$logger` fragment is captured and ignored (never expanded), so the absent /// `logger` field is never referenced. +#[inline] +fn consume_debug_args(_args: core::fmt::Arguments<'_>) {} + macro_rules! i3c_debug { ($logger:expr, $($arg:tt)*) => {{ - let _ = format_args!($($arg)*); + consume_debug_args(format_args!($($arg)*)); }}; } @@ -838,6 +872,31 @@ } impl<Y: FnMut(u32)> Ast1060I3c<Y> { + #[inline] + fn ensure_xfer_ready(&self) -> Result<(), I3cDrvError> { + if self.xfer_faulted.load(Ordering::Acquire) { + return Err(I3cDrvError::IoError); + } + Ok(()) + } + + /// Recover a timed-out transfer and re-arm its IRQs. + fn recover_timeout_xfer(&mut self, config: &mut I3cConfig) { + i3c_debug!(self.logger, "wait_xfer_complete: timeout"); + let halt_ok = self.enter_halt(true, config).is_ok(); + let reset_ok = self.reset_ctrl(RESET_CTRL_XFER_QUEUES).is_ok(); + let exit_ok = self.exit_halt(config).is_ok(); + if !(halt_ok && reset_ok && exit_ok) { + // Recovery failure is sticky until init. + self.xfer_faulted.store(true, Ordering::Release); + } + self.regs.clear_intr_status( + INTR_RESP_READY_STAT | INTR_TRANSFER_ERR_STAT | INTR_TRANSFER_ABORT_STAT, + ); + isr_events(self.bus() as usize).clear_pending(); + self.regs.unmask_master_xfer_irqs(); + } + fn toggle_scl_in(&mut self, count: u32) { for _ in 0..count { self.regs.i3cg_reg1_clear_bits(I3CG_REG1_SCL_IN_SW_MODE_VAL); @@ -873,6 +932,7 @@ /// no `unsafe`. fn process_responses(&mut self, config: &mut I3cConfig, xfer: &mut I3cXfer) { let nresp = self.regs.resp_buf_level(); + let mut ret = 0; for _ in 0..nresp { let resp = self.regs.pop_response(); @@ -900,6 +960,8 @@ if rx_len > 0 { self.regs.rx_fifo_drain(rx_len); } + // Reject an out-of-sync response TID. + ret = -1; continue; } @@ -910,6 +972,10 @@ }; cmd.rx_len = u32::try_from(rx_len).unwrap_or(0); cmd.ret = i32::try_from(err).unwrap_or(-1); + // Accumulate errors by response TID. + if cmd.ret != 0 { + ret = cmd.ret; + } if rx_len == 0 { continue; @@ -923,32 +989,24 @@ self.regs.rx_fifo_read(dst); } else { self.regs.rx_fifo_drain(rx_len); + // Reject responses larger than the caller's buffer. + cmd.ret = -1; + cmd.rx_len = 0; + ret = -1; } } else if rx_len > 0 { self.regs.rx_fifo_drain(rx_len); } } - let mut ret = 0; - for i in 0..nresp { - if let Some(c) = xfer.cmds.get(i) - && c.ret != 0 - { - ret = c.ret; - } - } - if ret != 0 { - // Best-effort recovery; the transfer error is already being - // reported via `xfer.ret`, so a recovery timeout on top of it has - // no separate observable outcome. `RESET_CTRL_XFER_QUEUES` (not - // `RESET_CTRL_QUEUES`) follows the vendor C driver - // (`aspeed_i3c_end_xfer`): this is the master completion path, and - // resetting the IBI queue here would silently drop IBIs that - // arrived during the failed transfer. - let _ = self.enter_halt(false, config); - let _ = self.reset_ctrl(RESET_CTRL_XFER_QUEUES); - let _ = self.exit_halt(config); + // Keep the IBI queue while recovering transfer queues. + let halt_ok = self.enter_halt(false, config).is_ok(); + let reset_ok = self.reset_ctrl(RESET_CTRL_XFER_QUEUES).is_ok(); + let exit_ok = self.exit_halt(config).is_ok(); + if !(halt_ok && reset_ok && exit_ok) { + self.xfer_faulted.store(true, Ordering::Release); + } } xfer.ret = ret; @@ -959,6 +1017,9 @@ fn init(&mut self, config: &mut I3cConfig) -> Result<(), I3cError> { i3c_debug!(self.logger, "i3c init"); + // Block transfers while hardware is partially initialized. + self.xfer_faulted.store(true, Ordering::Release); + self.regs .global_reset_deassert(I3C_GLOBAL_RESET_DEASSERT_MASK); @@ -975,6 +1036,15 @@ self.regs.core_reset_deassert(); self.i3c_disable(config.is_secondary); + // Drop pre-reset ISR and work-queue state while IRQs are disabled. + isr_events(self.bus() as usize).reset(); + ibi_workq::i3c_ibi_workq_clear(self.bus() as usize); + + // Invalidate the software target-address cache too. + if let Some(tc) = config.target_config.as_mut() { + tc.addr = None; + } + i3c_debug!( self.logger, "bus num: {}, is_secondary: {}", @@ -1011,7 +1081,8 @@ self.init_pid(config); - config.maxdevs = self.regs.dat_depth(); + // Clamp hardware depth to the software DAT capacity. + config.maxdevs = self.regs.dat_depth().min(MAX_DEVICES_PER_BUS as u16); config.free_pos = if config.maxdevs == 32 { u32::MAX } else { @@ -1028,7 +1099,13 @@ self.regs.set_hot_join_nack(true); if config.is_secondary { - self.regs.program_secondary_static_addr(9); + // Apply the configured target static address. + let static_addr = config + .target_config + .as_ref() + .and_then(|t| t.static_addr) + .unwrap_or(9); + self.regs.program_secondary_static_addr(static_addr); } else { self.regs.program_primary_dynamic_addr(8); } @@ -1042,6 +1119,9 @@ self.regs.set_hot_join_nack(false); i3c_debug!(self.logger, "i3c init done"); + // Initialization completed; transfers are safe again. + self.xfer_faulted.store(false, Ordering::Release); + // Safety: Ensure memory barrier and init completion before interrupts are enabled by the caller core::sync::atomic::compiler_fence(Ordering::SeqCst); Ok(()) @@ -1123,8 +1203,13 @@ self.regs .set_i2c_fm_timing(ns_to_cnt_u16(fm_hi_ns), ns_to_cnt_u16(fm_lo_ns)); - // I2C FMP - let (i2c_fmp_hi_ns, i2c_fmp_lo_ns) = self.calc_i2c_clk(1_000_000); + // Program FMP at the requested rate above 400 kHz. + let fmp_target_hz = if config.i2c_scl_hz > 400_000 { + config.i2c_scl_hz + } else { + 1_000_000 + }; + let (i2c_fmp_hi_ns, i2c_fmp_lo_ns) = self.calc_i2c_clk(fmp_target_hz); self.regs .set_i2c_fmp_timing(ns_to_cnt_u8(i2c_fmp_hi_ns), ns_to_cnt_u16(i2c_fmp_lo_ns)); @@ -1170,8 +1255,6 @@ } fn calc_i2c_clk(&mut self, fscl_hz: u32) -> (u32, u32) { - use core::cmp::max; - // `.max(1)` on both the SCL frequency and the resulting period keeps the // downstream `div_ceil(period_ns)` divisors provably non-zero (panic-free // for the `no_panics` analysis); a valid `fscl_hz` is unaffected. @@ -1179,24 +1262,25 @@ let (lo_min, hi_min): (u32, u32) = if fscl_hz <= 100_000 { ( - (I3C_BUS_I2C_STD_TLOW_MIN_NS + I3C_BUS_I2C_STD_TF_MAX_NS).div_ceil(period_ns), - (I3C_BUS_I2C_STD_THIGH_MIN_NS + I3C_BUS_I2C_STD_TR_MAX_NS).div_ceil(period_ns), + I3C_BUS_I2C_STD_TLOW_MIN_NS + I3C_BUS_I2C_STD_TF_MAX_NS, + I3C_BUS_I2C_STD_THIGH_MIN_NS + I3C_BUS_I2C_STD_TR_MAX_NS, ) } else if fscl_hz <= 400_000 { ( - (I3C_BUS_I2C_FM_TLOW_MIN_NS + I3C_BUS_I2C_FM_TF_MAX_NS).div_ceil(period_ns), - (I3C_BUS_I2C_FM_THIGH_MIN_NS + I3C_BUS_I2C_FM_TR_MAX_NS).div_ceil(period_ns), + I3C_BUS_I2C_FM_TLOW_MIN_NS + I3C_BUS_I2C_FM_TF_MAX_NS, + I3C_BUS_I2C_FM_THIGH_MIN_NS + I3C_BUS_I2C_FM_TR_MAX_NS, ) } else { ( - (I3C_BUS_I2C_FMP_TLOW_MIN_NS + I3C_BUS_I2C_FMP_TF_MAX_NS).div_ceil(period_ns), - (I3C_BUS_I2C_FMP_THIGH_MIN_NS + I3C_BUS_I2C_FMP_TR_MAX_NS).div_ceil(period_ns), + I3C_BUS_I2C_FMP_TLOW_MIN_NS + I3C_BUS_I2C_FMP_TF_MAX_NS, + I3C_BUS_I2C_FMP_THIGH_MIN_NS + I3C_BUS_I2C_FMP_TR_MAX_NS, ) }; - let leftover = period_ns.saturating_sub(lo_min + hi_min); + // Split the period remaining after timing minima. + let leftover = period_ns.saturating_sub(lo_min.saturating_add(hi_min)); let lo = lo_min + leftover / 2; - let hi = max(period_ns.saturating_sub(lo), hi_min); + let hi = hi_min + leftover.saturating_sub(leftover / 2); (hi, lo) } @@ -1347,6 +1431,11 @@ } fn ibi_enable(&mut self, config: &mut I3cConfig, addr: u8) -> Result<(), I3cDrvError> { + self.ensure_xfer_ready()?; + // Enforce the role at the hardware boundary. + if config.is_secondary { + return Err(I3cDrvError::Access); + } let dev_idx = config .attached .find_dev_idx_by_addr(addr) @@ -1367,8 +1456,15 @@ .devices .get(dev_idx) .ok_or(I3cDrvError::NoSuchDev)?; + if dev.kind != DevKind::I3c || dev.da_state != DaState::Verified { + return Err(I3cDrvError::Access); + } let tgt_bcr: u32 = u32::from(dev.bcr); - let mut reg = self.regs.dat_read(pos.into()); + let dyn_addr = dev.dyn_addr; + + // Accept locally before sending ENEC. + let orig_reg = self.regs.dat_read(pos.into()); + let mut reg = orig_reg; reg &= !DEV_ADDR_TABLE_SIR_REJECT; if tgt_bcr & I3C_BCR_IBI_PAYLOAD_HAS_DATA_BYTE != 0 { reg |= DEV_ADDR_TABLE_IBI_MDB | DEV_ADDR_TABLE_IBI_PEC; @@ -1376,21 +1472,22 @@ self.regs.dat_write_raw(pos.into(), reg); - let mut sir_reject = self.regs.read_sir_reject(); + let orig_sir_reject = self.regs.read_sir_reject(); + let mut sir_reject = orig_sir_reject; sir_reject &= !bit(pos.into()); self.regs.write_sir_reject(sir_reject); self.regs.enable_ibi_thld_irq(); let events = I3C_CCC_EVT_INTR; - // ccc_events_set requires HardwareTransfer trait bound on Self. - // We are inside HardwareTransfer impl for Ast1060I3c. - // Rust might have trouble inferring if Self: HardwareTransfer is not fully established yet? - // But Ast1060I3c implements HardwareTransfer (this block). - // However, ccc_events_set takes `&mut impl HardwareInterface`. - // Ast1060I3c implements HardwareInterface (blanket impl over all sub-traits). - // So this call should be valid. - let _ = ccc_events_set(self, config, dev.dyn_addr, true, events); + if let Err(e) = ccc_events_set(self, config, dyn_addr, true, events) { + // Preserve local acceptance after an ambiguous ENEC result. + if !matches!(e, I3cDrvError::Timeout | I3cDrvError::RespError) { + self.regs.dat_write_raw(pos.into(), orig_reg); + self.regs.write_sir_reject(orig_sir_reject); + } + return Err(e); + } i3c_debug!(self.logger, "i3cd030 (SIR reject) = {:#x}", sir_reject); i3c_debug!( @@ -1414,6 +1511,10 @@ } fn ibi_disable(&mut self, config: &mut I3cConfig, addr: u8) -> Result<(), I3cDrvError> { + self.ensure_xfer_ready()?; + if config.is_secondary { + return Err(I3cDrvError::Access); + } let dev_idx = config .attached .find_dev_idx_by_addr(addr) @@ -1427,14 +1528,19 @@ .attached .devices .get(dev_idx) - .ok_or(I3cDrvError::NoSuchDev)? - .dyn_addr; + .ok_or(I3cDrvError::NoSuchDev) + .and_then(|dev| { + if dev.kind == DevKind::I3c && dev.da_state == DaState::Verified { + Ok(dev.dyn_addr) + } else { + Err(I3cDrvError::Access) + } + })?; - // Tell the device to stop raising SIRs first (DISEC), while the - // controller still ACKs its IBIs; best-effort, mirroring ibi_enable. - let _ = ccc_events_set(self, config, dyn_addr, false, I3C_CCC_EVT_INTR); + // Send DISEC before rejecting locally. + let disec_result = ccc_events_set(self, config, dyn_addr, false, I3C_CCC_EVT_INTR); - // Then reject at the controller: DAT slot + SIR-reject mask. + // Reject locally even if DISEC fails. let mut reg = self.regs.dat_read(pos.into()); reg |= DEV_ADDR_TABLE_SIR_REJECT; reg &= !(DEV_ADDR_TABLE_IBI_MDB | DEV_ADDR_TABLE_IBI_PEC); @@ -1444,25 +1550,30 @@ sir_reject |= bit(pos.into()); self.regs.write_sir_reject(sir_reject); - Ok(()) + disec_result } - fn start_xfer(&mut self, config: &mut I3cConfig, xfer: &mut I3cXfer) { - let _ = config; + fn start_xfer(&mut self, _config: &mut I3cConfig, xfer: &mut I3cXfer) { xfer.ret = -1; - // Clear any stale completion flag and drain any stale responses left - // by an earlier timed-out transfer (the old ISR-side null-pointer - // drain, now done on the thread before the next submission). - let _ = isr_events(self.bus() as usize).take_pending(); + // Clear stale completion and response state. + if isr_events(self.bus() as usize).take_pending() != 0 { + i3c_debug!(self.logger, "cleared stale transfer completion"); + } let nresp = self.regs.resp_buf_level(); for _ in 0..nresp { - let _ = self.regs.pop_response(); + let resp = self.regs.pop_response(); + // Drain stale response data with its descriptor. + let rx_len = field_get( + resp, + RESPONSE_PORT_DATA_LEN_MASK, + RESPONSE_PORT_DATA_LEN_SHIFT, + ) as usize; + if rx_len > 0 { + self.regs.rx_fifo_drain(rx_len); + } } - // Re-arm the completion IRQ sources. If a late response (e.g. from a - // transfer that timed out) arrived with no waiter, the ISR masked the - // sources and nobody unmasked them — without this, the new transfer's - // completion would never be latched and would falsely time out. + // Re-arm IRQs masked by a late response. self.regs.unmask_master_xfer_irqs(); for cmd in xfer.cmds.iter() { @@ -1508,6 +1619,18 @@ != 0 { self.process_responses(config, xfer); + if xfer.ret == 0 + && pending & (INTR_TRANSFER_ERR_STAT | INTR_TRANSFER_ABORT_STAT) != 0 + { + // Recover transfer-level errors lacking a response. + xfer.ret = -1; + let halt_ok = self.enter_halt(false, config).is_ok(); + let reset_ok = self.reset_ctrl(RESET_CTRL_XFER_QUEUES).is_ok(); + let exit_ok = self.exit_halt(config).is_ok(); + if !(halt_ok && reset_ok && exit_ok) { + self.xfer_faulted.store(true, Ordering::Release); + } + } self.regs.unmask_master_xfer_irqs(); return true; } @@ -1518,13 +1641,8 @@ left -= 1; } - // Timeout: recover the engine and re-arm the IRQ sources. Recovery is - // best-effort — the `false` return already reports the timeout. - i3c_debug!(self.logger, "wait_xfer_complete: timeout"); - let _ = self.enter_halt(true, config); - let _ = self.reset_ctrl(RESET_CTRL_XFER_QUEUES); - let _ = self.exit_halt(config); - self.regs.unmask_master_xfer_irqs(); + // Recover and re-arm after timeout. + self.recover_timeout_xfer(config); false } @@ -1561,6 +1679,11 @@ pos: u8, ops: &mut [I2cOp<'a>], ) -> Result<(), I3cDrvError> { + self.ensure_xfer_ready()?; + // Enforce the role at the hardware boundary. + if config.is_secondary { + return Err(I3cDrvError::Access); + } if ops.is_empty() { return Ok(()); } @@ -1589,12 +1712,19 @@ } as u32; let mut cmds: heapless::Vec<I3cCmd<'a>, MAX_CMDS> = heapless::Vec::new(); + // Save requested lengths before responses overwrite `rx_len`. + let mut requested_rx_len: [usize; MAX_PRIV_XFER_CMDS] = [0; MAX_PRIV_XFER_CMDS]; let nops = ops.len(); for (i, op) in ops.iter_mut().enumerate() { let mut cmd = I3cCmd::new(); let len = match op { I2cOp::Write(b) => b.len(), - I2cOp::Read(b) => b.len(), + I2cOp::Read(b) => { + if let Some(slot) = requested_rx_len.get_mut(i) { + *slot = b.len(); + } + b.len() + } }; cmd.cmd_hi = field_prep(COMMAND_PORT_ATTR, COMMAND_ATTR_XFER_ARG) | field_prep( @@ -1638,8 +1768,21 @@ } match xfer.ret { - 0 => Ok(()), - _ => Err(I3cDrvError::IoError), + 0 => { + // Detect per-command short reads. + for (i, requested) in requested_rx_len.iter().enumerate().take(nops) { + if *requested == 0 { + continue; + } + let Some(cmd) = cmds.get(i) else { continue }; + if cmd.ret == 0 && (cmd.rx_len as usize) < *requested { + return Err(I3cDrvError::RespError); + } + } + Ok(()) + } + // Completion with a failed response. + _ => Err(I3cDrvError::RespError), } } @@ -1649,6 +1792,11 @@ config: &mut I3cConfig, payload: &mut CccPayload<'_, '_>, ) -> Result<(), I3cDrvError> { + self.ensure_xfer_ready()?; + // Enforce the role at the hardware boundary. + if config.is_secondary { + return Err(I3cDrvError::Access); + } let mut cmds = [I3cCmd { cmd_lo: 0, cmd_hi: 0, @@ -1662,6 +1810,7 @@ let mut pos = 0; let mut rnw: bool = false; let mut is_broadcast = false; + let mut requested_rx_len: usize = 0; let (id, data_len) = { let Some(ccc) = payload.ccc.as_ref() else { @@ -1669,14 +1818,21 @@ }; (ccc.id, ccc.data.as_deref().map_or(0, <[u8]>::len)) }; + // Reject lengths that exceed the command field. + if data_len > MAX_XFER_DATA_LEN { + return Err(I3cDrvError::Invalid); + } let dbp_is_direct = id > 0x7F; let db: u8 = if dbp_is_direct && data_len > 0 { + // Avoid indexing caller data. payload .ccc .as_ref() .and_then(|c| c.data.as_deref()) - .map_or(0, |d| d[0]) + .and_then(|d| d.first()) + .copied() + .unwrap_or(0) } else { 0 }; @@ -1702,6 +1858,29 @@ else { return Err(I3cDrvError::Invalid); }; + let target_state = config + .attached + .devices + .iter() + .find(|dev| dev.kind == DevKind::I3c && dev.dyn_addr == tgt_addr) + .map(|dev| dev.da_state); + // Unknown targets allow only discovery and DAA operations. + let allowed_for_unknown = matches!( + id, + I3C_CCC_GETPID + | I3C_CCC_GETBCR + | I3C_CCC_GETDCR + | I3C_CCC_GETMWL + | I3C_CCC_GETMRL + | I3C_CCC_GETSTATUS + | I3C_CCC_GETMXDS + | I3C_CCC_SETNEWDA + ); + if !matches!(target_state, Some(DaState::Verified)) + && !(matches!(target_state, Some(DaState::Unknown)) && allowed_for_unknown) + { + return Err(I3cDrvError::Access); + } let pos_ops = config.attached.pos_of_addr(tgt_addr); i3c_debug!( self.logger, @@ -1728,9 +1907,10 @@ if rnw { let len = tp.data.as_deref().map_or(0, <[u8]>::len); - if len == 0 { + if len == 0 || len > MAX_XFER_DATA_LEN { return Err(I3cDrvError::Invalid); } + requested_rx_len = len; cmd.rx_len = u32::try_from(len).map_err(|_| I3cDrvError::Invalid)?; cmd.rx = tp.data.as_deref_mut(); } else { @@ -1738,6 +1918,9 @@ Some(d) => (Some(d), d.len()), None => (None, 0), }; + if len > MAX_XFER_DATA_LEN { + return Err(I3cDrvError::Invalid); + } cmd.tx = d_opt; cmd.tx_len = u32::try_from(len).map_err(|_| I3cDrvError::Invalid)?; tp.num_xfer = len; @@ -1777,13 +1960,17 @@ let mut xfer = I3cXfer::new(&mut cmds[..]); self.start_xfer(config, &mut xfer); - // On timeout `wait_xfer_complete` already recovered the engine; - // `xfer.ret` stays -1 and falls through to the error mapping below - // (same outcome as the reference's timeout path). - let _ = self.wait_xfer_complete(config, &mut xfer, I3C_OP_TIMEOUT_US); + if !self.wait_xfer_complete(config, &mut xfer, I3C_OP_TIMEOUT_US) { + return Err(I3cDrvError::Timeout); + } let ret = xfer.ret; - if ret == i32::try_from(RESPONSE_ERROR_IBA_NACK).map_err(|_| I3cDrvError::Invalid)? { + // Copy before mutably borrowing the payload below. + let actual_rx_len = cmds[0].rx_len as usize; + // IBA_NACK is benign only for broadcast CCCs. + if is_broadcast + && ret == i32::try_from(RESPONSE_ERROR_IBA_NACK).map_err(|_| I3cDrvError::Invalid)? + { return Ok(()); } @@ -1794,13 +1981,34 @@ } } + if !is_broadcast && rnw { + let actual = actual_rx_len; + if let Some(tp) = payload.targets.as_deref_mut().and_then(|ts| ts.first_mut()) { + // Report the actual response length. + tp.num_xfer = actual; + } + if ret == 0 && actual < requested_rx_len { + // Reject short direct-read responses. + return Err(I3cDrvError::RespError); + } + } + match ret { 0 => Ok(()), - _ => Err(I3cDrvError::Invalid), + r if r == i32::try_from(RESPONSE_ERROR_ADDRESS_NACK).unwrap_or(-1) => { + Err(I3cDrvError::AddressNack) + } + // The CCC completed with a response error. + _ => Err(I3cDrvError::RespError), } } fn do_entdaa(&mut self, config: &mut I3cConfig, pos: u32) -> Result<(), I3cDrvError> { + self.ensure_xfer_ready()?; + // Enforce the role at the hardware boundary. + if config.is_secondary { + return Err(I3cDrvError::Access); + } i3c_debug!(self.logger, "do_entdaa: pos={}", pos); let cmd = I3cCmd { cmd_lo: field_prep(COMMAND_PORT_ATTR, COMMAND_ATTR_ADDR_ASSGN_CMD) @@ -1842,7 +2050,11 @@ i3c_debug!(self.logger, "do_entdaa: xfer done"); match xfer.ret { 0 => Ok(()), - _ => Err(I3cDrvError::Invalid), + // Address NACK normally ends DAA. + r if r == i32::try_from(RESPONSE_ERROR_ADDRESS_NACK).unwrap_or(-1) => { + Err(I3cDrvError::DaaNack) + } + _ => Err(I3cDrvError::RespError), } } @@ -1950,7 +2162,21 @@ pid: u64, msgs: &mut [I3cMsg], ) -> Result<(), I3cDrvError> { - let pos_opt = config.attached.pos_of_pid(pid); + self.ensure_xfer_ready()?; + // Enforce the role at the hardware boundary. + if config.is_secondary { + return Err(I3cDrvError::Access); + } + let pos_opt = config + .attached + .devices + .iter() + .position(|dev| { + dev.kind == DevKind::I3c + && dev.pid == Some(pid) + && dev.da_state == DaState::Verified + }) + .and_then(|idx| config.attached.pos_of(idx)); let pos: u8 = pos_opt.ok_or(I3cDrvError::NoDatPos)?; if msgs.len() == 1 { @@ -1974,7 +2200,8 @@ return match xfer.ret { 0 => Ok(()), - _ => Err(I3cDrvError::Timeout), + // Completion with a failed response. + _ => Err(I3cDrvError::RespError), }; } @@ -1994,11 +2221,7 @@ .map_err(|_| I3cDrvError::TooManyMsgs)?; } - let ret = self.priv_xfer_build_cmds(cmds.as_mut_slice(), msgs, pos); - match ret { - Ok(()) => {} - Err(e) => return Err(e), - } + self.priv_xfer_build_cmds(cmds.as_mut_slice(), msgs, pos)?; let mut xfer = I3cXfer::new(cmds.as_mut_slice()); self.start_xfer(config, &mut xfer); @@ -2017,25 +2240,40 @@ match xfer.ret { 0 => Ok(()), - _ => Err(I3cDrvError::Timeout), + // Completion with a failed response. + _ => Err(I3cDrvError::RespError), } } + + fn mark_xfer_faulted(&mut self) { + self.xfer_faulted.store(true, Ordering::Release); + } } impl<Y: FnMut(u32)> HardwareTarget for Ast1060I3c<Y> { - fn target_tx_write(&mut self, buf: &[u8]) { + fn target_tx_write(&mut self, config: &I3cConfig, buf: &[u8]) -> Result<(), I3cDrvError> { + self.ensure_xfer_ready()?; + if !config.is_secondary { + return Err(I3cDrvError::Access); + } + if buf.len() > MAX_XFER_DATA_LEN { + return Err(I3cDrvError::Invalid); + } + let len = buf.len(); self.wr_tx_fifo(buf); let cmd = field_prep(COMMAND_PORT_ATTR, COMMAND_ATTR_SLAVE_DATA_CMD) | field_prep( COMMAND_PORT_ARG_DATA_LEN, - u32::try_from(buf.len()).map_or(0, |v| v), + u32::try_from(len).map_err(|_| I3cDrvError::Invalid)?, ) | field_prep(COMMAND_PORT_TID, Tid::TargetRdData as u32); self.regs.push_cmd(cmd); + Ok(()) } fn target_ibi_raise_hj(&self, config: &mut I3cConfig) -> Result<(), I3cDrvError> { + self.ensure_xfer_ready()?; if !config.is_secondary { return Err(I3cDrvError::Invalid); } @@ -2058,17 +2296,24 @@ buf: &[u8], notifier: &mut I3cIbi, ) -> Result<(), I3cDrvError> { + self.ensure_xfer_ready()?; + if !config.is_secondary { + return Err(I3cDrvError::Access); + } let events = isr_events(self.bus() as usize); // A fault the ISR deferred (errored response / halted engine after a // CCC): recover here on the thread, where the wait policy lives. if events.take_fault() { - // Best-effort: a recovery timeout here must not block the SIR - // attempt below, which has its own timeout/recovery path. i3c_debug!(self.logger, "recovering deferred target fault"); - let _ = self.enter_halt(false, config); - let _ = self.reset_ctrl(RESET_CTRL_QUEUES); - let _ = self.exit_halt(config); + let halt_ok = self.enter_halt(false, config).is_ok(); + let reset_ok = self.reset_ctrl(RESET_CTRL_QUEUES).is_ok(); + let exit_ok = self.exit_halt(config).is_ok(); + if !(halt_ok && reset_ok && exit_ok) { + // Block I/O after failed recovery. + self.xfer_faulted.store(true, Ordering::Release); + return Err(I3cDrvError::IoError); + } } let reg = self.regs.read_slv_event_ctrl(); @@ -2080,6 +2325,15 @@ return Err(I3cDrvError::Invalid); }; + // Validate command lengths before writing the FIFO. + if buf.len() > MAX_XFER_DATA_LEN + || notifier + .payload + .is_some_and(|p| p.len() > MAX_XFER_DATA_LEN) + { + return Err(I3cDrvError::Invalid); + } + self.set_ibi_mdb(mdb); if let Some(p) = notifier.payload && !p.is_empty() @@ -2092,14 +2346,13 @@ let cmd: u32 = field_prep(COMMAND_PORT_ATTR, COMMAND_ATTR_SLAVE_DATA_CMD) | field_prep(COMMAND_PORT_ARG_DATA_LEN, payload_len) | field_prep(COMMAND_PORT_TID, Tid::TargetIbi as u32); - self.regs.push_cmd(cmd); - events.target_ibi_done.reset(); + self.regs.push_cmd(cmd); self.regs.set_resp_buf_threshold(0); - self.target_tx_write(buf); events.target_data_done.reset(); + self.target_tx_write(config, buf)?; self.regs.raise_sir(); @@ -2112,9 +2365,12 @@ // full controller init. Side effects match the C driver: the // dynamic address is dropped (the bus master must re-run DAA) and // `sir_allowed_by_sw` is cleared until the next DA assignment. - // Best-effort; `IoError` below already reports the failure. i3c_debug!(self.logger, "SIR timeout! Reset I3C controller"); - let _ = self.init(config); + if let Err(e) = self.init(config) { + // Keep the controller faulted after failed recovery. + i3c_debug!(self.logger, "SIR timeout recovery init failed: {:?}", e); + self.xfer_faulted.store(true, Ordering::Release); + } return Err(I3cDrvError::IoError); } @@ -2122,11 +2378,15 @@ .target_data_done .wait_for_us(I3C_OP_TIMEOUT_US, &mut self.yield_fn) { - // Best-effort recovery; `Timeout` below already reports the failure. i3c_debug!(self.logger, "wait master read timeout! Reset queues"); self.i3c_disable(config.is_secondary); - let _ = self.reset_ctrl(RESET_CTRL_QUEUES); - self.i3c_enable(config); + if self.reset_ctrl(RESET_CTRL_QUEUES).is_err() { + // Leave the engine disabled and faulted. + i3c_debug!(self.logger, "target-data timeout: queue reset failed"); + self.xfer_faulted.store(true, Ordering::Release); + } else { + self.i3c_enable(config); + } return Err(I3cDrvError::Timeout); }
diff --git a/target/ast10x0/peripherals/i3c/ibi.rs b/target/ast10x0/peripherals/i3c/ibi.rs index c73fddd..0b2434a 100644 --- a/target/ast10x0/peripherals/i3c/ibi.rs +++ b/target/ast10x0/peripherals/i3c/ibi.rs
@@ -22,17 +22,16 @@ //! `Option<IbiWork>` (`IbiWork` is `Copy`, no niche pointers), guarded by the //! same `critical_section`. The process-global queue/handler design (goal.md //! ADR-3) is preserved — an ISR still cannot borrow a stack-owned device, so -//! the IBI plane stays global, arbitrated by `critical_section`. The public -//! API (`i3c_ibi_workq_consumer().dequeue()` + the three enqueue functions) is -//! unchanged. +//! the IBI plane stays global and uses one consumer per bus. use core::cell::UnsafeCell; +use core::sync::atomic::{AtomicBool, Ordering}; use critical_section::Mutex; /// IBI queue depth const IBIQ_DEPTH: usize = 16; /// Maximum IBI payload data size -const IBI_DATA_MAX: u8 = 16; +pub const IBI_DATA_MAX: u8 = 16; /// Maximum private-write payload captured per [`IbiWork::TargetMasterWrite`]. /// /// The vendor C driver delivers the full write (heap-allocated per response); @@ -57,6 +56,8 @@ /// IBI work item representing an interrupt event #[derive(Debug, Clone, Copy)] pub enum IbiWork { + /// One or more older work items were dropped because the queue was full. + Overflow, /// Hot-Join request from a device HotJoin, /// Slave Interrupt Request @@ -92,6 +93,7 @@ buf: [Option<IbiWork>; IBIQ_DEPTH], head: usize, len: usize, + overflowed: bool, } impl IbiRing { @@ -100,27 +102,32 @@ buf: [None; IBIQ_DEPTH], head: 0, len: 0, + overflowed: false, } } fn push(&mut self, work: IbiWork) -> bool { - // `get_mut` + modulo keep this panic-free even if the indices were - // somehow out of range; `head` is normalized first. self.head %= IBIQ_DEPTH; if self.len >= IBIQ_DEPTH { - return false; + // Keep the newest IRQ information when the consumer falls behind. + self.head = (self.head + 1) % IBIQ_DEPTH; + self.len = IBIQ_DEPTH - 1; + self.overflowed = true; } let idx = (self.head + self.len) % IBIQ_DEPTH; - if let Some(slot) = self.buf.get_mut(idx) { - *slot = Some(work); - self.len += 1; - true - } else { - false - } + let Some(slot) = self.buf.get_mut(idx) else { + return false; + }; + *slot = Some(work); + self.len += 1; + true } fn pop(&mut self) -> Option<IbiWork> { + if self.overflowed { + self.overflowed = false; + return Some(IbiWork::Overflow); + } self.head %= IBIQ_DEPTH; if self.len == 0 || self.len > IBIQ_DEPTH { // Empty, or a corrupt length — treat as empty (panic-free). @@ -145,8 +152,15 @@ Mutex::new(UnsafeCell::new(IbiRing::new())), ]; -/// Push `work` onto the ring for `bus`. Returns `false` if `bus` is out of -/// range or the ring is full. +/// Enforce the single-consumer side of the per-bus SPSC queue. +static IBI_CONSUMER_CLAIMED: [AtomicBool; 4] = [ + AtomicBool::new(false), + AtomicBool::new(false), + AtomicBool::new(false), + AtomicBool::new(false), +]; + +/// Push work; a full ring drops the oldest item and records an overflow event. /// /// The `&mut IbiRing` is confined to this leaf function — no caller-provided /// code runs while it is live — so the exclusive borrow cannot be re-entered. @@ -177,6 +191,11 @@ }) } +/// Discard pre-reset work for a bus. +pub fn i3c_ibi_workq_clear(bus: usize) { + while ring_pop(bus).is_some() {} +} + // ============================================================================= // Consumer Handle // ============================================================================= @@ -189,6 +208,14 @@ bus: usize, } +impl Drop for IbiConsumer { + fn drop(&mut self) { + if let Some(claimed) = IBI_CONSUMER_CLAIMED.get(self.bus) { + claimed.store(false, Ordering::Release); + } + } +} + impl IbiConsumer { /// Dequeue the next IBI work item, if any. #[must_use] @@ -202,9 +229,10 @@ /// Returns `None` if the bus index is out of range. #[must_use] pub fn i3c_ibi_workq_consumer(bus: usize) -> Option<IbiConsumer> { - if bus >= IBI_RINGS.len() { - return None; - } + let claimed = IBI_CONSUMER_CLAIMED.get(bus)?; + claimed + .compare_exchange(false, true, Ordering::AcqRel, Ordering::Acquire) + .ok()?; Some(IbiConsumer { bus }) } @@ -224,7 +252,7 @@ ring_push(bus, IbiWork::HotJoin) } -/// Enqueue a target interrupt (SIR) notification +/// Enqueue an SIR. #[must_use] pub fn i3c_ibi_work_enqueue_target_irq(bus: usize, addr: u8, data: &[u8]) -> bool { let mut ibi_buf = [0u8; IBI_DATA_MAX as usize];
diff --git a/target/ast10x0/peripherals/i3c/mod.rs b/target/ast10x0/peripherals/i3c/mod.rs index a915d80..777d979 100644 --- a/target/ast10x0/peripherals/i3c/mod.rs +++ b/target/ast10x0/peripherals/i3c/mod.rs
@@ -56,14 +56,14 @@ // Configuration pub use config::{ - AddrBook, Attached, CommonCfg, CommonState, DeviceEntry, I3cConfig, I3cTargetConfig, ResetSpec, - I3C_MAX_CORE_CLK, I3C_MIN_CORE_CLK_HDR, I3C_MIN_CORE_CLK_SDR, + AddrBook, Attached, CommonCfg, CommonState, DeviceEntry, I3cConfig, I3cTargetConfig, IbiState, + ResetSpec, I3C_MAX_CORE_CLK, I3C_MIN_CORE_CLK_HDR, I3C_MIN_CORE_CLK_SDR, }; // Core types pub use types::{ - Completion, DevKind, I2cOp, I3cCmd, I3cDeviceId, I3cIbi, I3cIbiType, I3cMsg, I3cPid, - I3cStatus, I3cXfer, SpeedI2c, SpeedI3c, Tid, + Completion, DevKind, I2cOp, I3cCmd, I3cDeviceId, I3cIbi, I3cIbiType, I3cMsg, I3cPid, I3cStatus, + I3cXfer, SpeedI2c, SpeedI3c, Tid, }; // Hardware interface
diff --git a/target/ast10x0/peripherals/i3c/registers.rs b/target/ast10x0/peripherals/i3c/registers.rs index 3379977..f891387 100644 --- a/target/ast10x0/peripherals/i3c/registers.rs +++ b/target/ast10x0/peripherals/i3c/registers.rs
@@ -487,7 +487,7 @@ fn fifo_drain<F: FnMut() -> u32>(mut read_word: F, len: usize) { let nwords = (len + 3) >> 2; for _ in 0..nwords { - let _ = read_word(); + read_word(); } }
diff --git a/target/ast10x0/tests/peripherals/i3c/i3c_init/target.rs b/target/ast10x0/tests/peripherals/i3c/i3c_init/target.rs index ab0fd73..67319fc 100644 --- a/target/ast10x0/tests/peripherals/i3c/i3c_init/target.rs +++ b/target/ast10x0/tests/peripherals/i3c/i3c_init/target.rs
@@ -104,7 +104,9 @@ } }; - let _ = console_backend_write_all(sentinel); + if console_backend_write_all(sentinel).is_err() { + pw_log::error!("failed to write test result sentinel"); + } #[expect(clippy::empty_loop)] loop {} }
diff --git a/target/ast10x0/tests/peripherals/i3c/i3c_irq/slave_target.rs b/target/ast10x0/tests/peripherals/i3c/i3c_irq/slave_target.rs index fbd6e90..bb3fd12 100644 --- a/target/ast10x0/tests/peripherals/i3c/i3c_irq/slave_target.rs +++ b/target/ast10x0/tests/peripherals/i3c/i3c_irq/slave_target.rs
@@ -49,6 +49,8 @@ const HOT_JOIN_STARTUP_DELAY_SPINS: u32 = 0x1000_0000; /// Re-raise hot-join while waiting in case the first request hit the NACK window. const HOT_JOIN_RETRY_SPINS: u32 = 0x0400_0000; +/// Maximum hot-join retry windows. +const HOT_JOIN_MAX_RETRIES: u32 = 64; const WAIT_MASTER_WRITE_SPINS: u32 = 0x0400_0000; const XFER_DATA_LEN: usize = 16; @@ -82,6 +84,17 @@ pw_log::info!("[SDUMP{}] dev_addr={:08x}", label as u32, dev_addr as u32); } +/// Log a compact hot-join error code. +fn log_hot_join_err(e: ast10x0_peripherals::i3c::I3cError) { + use ast10x0_peripherals::i3c::I3cError; + let code: u32 = match e { + I3cError::Access => 1, + I3cError::Invalid => 2, + _ => 0xff, + }; + pw_log::error!("target_raise_hot_join failed: code={}", code as u32); +} + fn log_target_hj_state(label: u32) { let regs = unsafe { &*ast1060_pac::I3c2::ptr() }; let dev_addr = regs.i3cd004().read().bits(); @@ -149,7 +162,8 @@ }; match work { - IbiWork::TargetMasterWrite { len, data } => { + IbiWork::Overflow => return Err("IBI queue overflow"), + IbiWork::TargetMasterWrite { len, data, .. } => { log_target_master_write(exchange, len, &data); return Ok(()); } @@ -189,7 +203,7 @@ .i3c_od_scl_lo_period_ns(0) .sda_tx_hold_ns(0) .dcr(0xcc) - .target_config(I3cTargetConfig::new(0, Some(0), 0xae)); + .target_config(I3cTargetConfig::new(0, Some(9), 0xae)); config.init_runtime_fields(); config .validate_clock() @@ -226,37 +240,39 @@ // unmasking cannot deliver an IRQ into partially-initialized state. unsafe { NVIC::unmask(ast1060_pac::Interrupt::i3c2) }; - let dyn_addr = 8u8; - let dev_idx = 0usize; - let _ = ctrl.attach_i3c_dev(0, dyn_addr, dev_idx as u8); - pw_log::info!( - "target dev at slot {}, dyn addr {}", - dev_idx as u32, - dyn_addr as u32 - ); - pw_log::info!("waiting before hot-join..."); spin_wait(HOT_JOIN_STARTUP_DELAY_SPINS); pw_log::info!("raising hot-join; waiting for dynamic address assignment..."); - let hj_ok = ctrl.target_raise_hot_join().is_ok(); - pw_log::info!("[DBG] hot-join raise ok={}", hj_ok as u32); + if let Err(e) = ctrl.target_raise_hot_join() { + log_hot_join_err(e); + } log_target_hj_state(0); // Wait for the controller to assign our dynamic address. let mut spin_count = 0u32; + let mut retries = 0u32; loop { let Some(work) = ibi_cons.dequeue() else { core::hint::spin_loop(); spin_count = spin_count.wrapping_add(1); if spin_count & (HOT_JOIN_RETRY_SPINS - 1) == 0 { - pw_log::info!("[DBG] retry hot-join"); - let hj_ok = ctrl.target_raise_hot_join().is_ok(); - pw_log::info!("[DBG] hot-join retry ok={}", hj_ok as u32); + retries += 1; + if retries > HOT_JOIN_MAX_RETRIES { + // Fail instead of waiting forever. + return Err( + "hot-join: exceeded retry budget waiting for dynamic address assignment", + ); + } + pw_log::info!("[DBG] retry hot-join, attempt {}", retries as u32); + if let Err(e) = ctrl.target_raise_hot_join() { + log_hot_join_err(e); + } log_target_hj_state(1); } continue; }; match work { + IbiWork::Overflow => return Err("IBI queue overflow"), IbiWork::TargetDaAssignment => { let da = ctrl.target_dynamic_address(); if let Some(da) = da { @@ -269,7 +285,7 @@ IbiWork::Sirq { addr, len, .. } => { pw_log::info!("[IBI] SIRQ from 0x{:02x} len {}", addr as u32, len as u32); } - IbiWork::TargetMasterWrite { len, data } => { + IbiWork::TargetMasterWrite { len, data, .. } => { log_target_master_write(0, len, &data); } } @@ -313,7 +329,9 @@ b"TEST_RESULT:FAIL\n" } }; - let _ = console_backend_write_all(sentinel); + if console_backend_write_all(sentinel).is_err() { + pw_log::error!("failed to write target test result sentinel"); + } #[expect(clippy::empty_loop)] loop {} }
diff --git a/target/ast10x0/tests/peripherals/i3c/i3c_irq/target.rs b/target/ast10x0/tests/peripherals/i3c/i3c_irq/target.rs index 19ea3b6..39250b1 100644 --- a/target/ast10x0/tests/peripherals/i3c/i3c_irq/target.rs +++ b/target/ast10x0/tests/peripherals/i3c/i3c_irq/target.rs
@@ -53,6 +53,8 @@ const MAX_EXCHANGES: u32 = 10; const XFER_DATA_LEN: usize = 16; const WAIT_LOG_SPINS: u32 = 0x0400_0000; +/// Maximum consecutive idle spins while waiting for IBI work. +const MAX_WAIT_SPINS: u32 = WAIT_LOG_SPINS * 8; /// Calibrated busy-wait used as the driver's yield/delay hook. Mirrors the /// reference `DummyDelay::delay_ns` (busy-loop of ~`ns / 100` nops). A named @@ -199,9 +201,19 @@ let mut config = build_config()?; // SAFETY: the test owns I3C bus 2 and uses the matching PAC blocks. let hw = unsafe { I3cHw::new(I3C_BUS, yield_delay) }.ok_or("invalid I3C bus index")?; - let mut ctrl = I3cController::new(hw, &mut config) - .start() - .map_err(|_| "controller start failed")?; + let mut ctrl = I3cController::new(hw, &mut config).start().map_err(|e| { + use ast10x0_peripherals::i3c::I3cError; + // 1=InvalidParam (validate_clock), 2=Timeout (reset-poll in init), + // 3=Busy (IRQ slot already claimed), 0xff=anything else. + let code: u32 = match e { + I3cError::InvalidParam => 1, + I3cError::Timeout => 2, + I3cError::Busy => 3, + _ => 0xff, + }; + pw_log::error!("controller start failed: code={}", code as u32); + "controller start failed" + })?; let bus = ctrl.bus_num() as usize; let mut ibi_cons = i3c_ibi_workq_consumer(bus).ok_or("IBI consumer unavailable")?; pw_log::info!("IBI work queue ready on bus {}", bus as u32); @@ -215,13 +227,9 @@ unsafe { NVIC::unmask(ast1060_pac::Interrupt::i3c2) }; pw_log::info!("I3C2 controller ready"); - let dyn_addr = ctrl - .alloc_dynamic_address_from(8) - .ok_or("no dynamic address available")?; + let dyn_addr = 8; ctrl.attach_i3c_dev(KNOWN_PID, dyn_addr, 0) .map_err(|_| "attach_i3c_dev failed")?; - ctrl.enable_ibi(dyn_addr, 0) - .map_err(|_| "ibi_enable failed")?; pw_log::info!("pre-attached dev at slot 0, dyn addr {}", dyn_addr as u32); let mut received = 0u32; @@ -230,6 +238,9 @@ let Some(work) = ibi_cons.dequeue() else { core::hint::spin_loop(); spin_count = spin_count.wrapping_add(1); + if spin_count >= MAX_WAIT_SPINS { + return Err("timed out waiting for IBI work"); + } if spin_count & (WAIT_LOG_SPINS - 1) == 0 { let irq_count = I3C2_IRQ_COUNT.load(core::sync::atomic::Ordering::Relaxed); let status = I3C2_LAST_STATUS.load(core::sync::atomic::Ordering::Relaxed); @@ -268,11 +279,15 @@ } continue; }; + // Measure consecutive idle time only. + spin_count = 0; match work { + IbiWork::Overflow => return Err("IBI queue overflow"), IbiWork::HotJoin => { pw_log::info!("[IBI] hotjoin"); dump_i3c2(0); - let _ = ctrl.handle_hot_join(); + ctrl.handle_hot_join() + .map_err(|_| "handle_hot_join failed")?; match ctrl.assign_dynamic_address(dyn_addr) { Ok(da) => pw_log::info!("DA assigned: 0x{:02x}", da as u32), Err(e) => { @@ -287,15 +302,16 @@ _ => 0xff, }; pw_log::error!("assign_dynamic_address failed: code={}", code as u32); + // DA assignment failure invalidates the test. + return Err("assign_dynamic_address failed"); } } dump_i3c2(1); } IbiWork::Sirq { addr, len, .. } => { pw_log::info!("[IBI] SIRQ from 0x{:02x} len {}", addr as u32, len as u32); - if ctrl.acknowledge_ibi(addr).is_err() { - pw_log::error!("acknowledge_ibi failed"); - } + ctrl.acknowledge_ibi(addr) + .map_err(|_| "acknowledge_ibi failed")?; let exchange = received; let (read_len, read_data) = master_read_from_target(&mut ctrl)?; @@ -360,7 +376,9 @@ b"TEST_RESULT:FAIL\n" } }; - let _ = console_backend_write_all(sentinel); + if console_backend_write_all(sentinel).is_err() { + pw_log::error!("failed to write controller test result sentinel"); + } #[expect(clippy::empty_loop)] loop {} }