From 4ddad4e74c4beed937772165c66ac0d740c5d890 Mon Sep 17 00:00:00 2001 From: Wilfried Chauveau Date: Fri, 16 Sep 2022 17:19:54 +0100 Subject: [PATCH] fix concurrent accesses to sm_execctrl and sm_instr when sideset isn't optional (#448) * fix concurrent accesses to sm_execctrl and sm_instr when sideset isn't optional * review & document unsafe blocks --- Cargo.toml | 1 + boards/rp-pico/examples/pico_pio_pwm.rs | 24 +- rp2040-hal/src/pio.rs | 645 +++++++++++++----------- 3 files changed, 368 insertions(+), 302 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index eba2f7c..62ae9cc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,3 +28,4 @@ rp2040-hal = { path = "./rp2040-hal" } [patch.crates-io] rp2040-hal = { path = "./rp2040-hal" } +i2c-pio = { git = "https://github.com/ithinuel/i2c-pio-rs", branch = "fix-pio-exec-instruction" } diff --git a/boards/rp-pico/examples/pico_pio_pwm.rs b/boards/rp-pico/examples/pico_pio_pwm.rs index daf2d29..770b63c 100644 --- a/boards/rp-pico/examples/pico_pio_pwm.rs +++ b/boards/rp-pico/examples/pico_pio_pwm.rs @@ -34,7 +34,7 @@ use rp_pico::hal; // Import pio crates use hal::pio::{PIOBuilder, Running, StateMachine, Tx, ValidStateMachine, SM0}; -use pio::{InstructionOperands, OutDestination}; +use pio::{Instruction, InstructionOperands, OutDestination}; use pio_proc::pio_file; /// Set pio pwm period @@ -53,20 +53,22 @@ fn pio_pwm_set_period( let mut sm = sm.stop(); tx.write(period); - sm.exec_instruction( - InstructionOperands::PULL { + sm.exec_instruction(Instruction { + operands: InstructionOperands::PULL { if_empty: false, block: false, - } - .encode(), - ); - sm.exec_instruction( - InstructionOperands::OUT { + }, + delay: 0, + side_set: None, + }); + sm.exec_instruction(Instruction { + operands: InstructionOperands::OUT { destination: OutDestination::ISR, bit_count: 32, - } - .encode(), - ); + }, + delay: 0, + side_set: None, + }); sm.start() } diff --git a/rp2040-hal/src/pio.rs b/rp2040-hal/src/pio.rs index cf7d1fd..698d4be 100644 --- a/rp2040-hal/src/pio.rs +++ b/rp2040-hal/src/pio.rs @@ -4,7 +4,7 @@ use crate::{ atomic_register_access::{write_bitmask_clear, write_bitmask_set}, resets::SubsystemReset, }; -use pio::{Program, SideSet, Wrap}; +use pio::{Instruction, InstructionOperands, Program, SideSet, Wrap}; use rp2040_pac::{PIO0, PIO1}; const PIO_INSTRUCTION_COUNT: usize = 32; @@ -191,31 +191,37 @@ impl PIO

{ p: &Program<{ pio::RP2040_MAX_PROGRAM_SIZE }>, ) -> Result, InstallError> { if let Some(offset) = self.find_offset_for_instructions(&p.code, p.origin) { - for (i, instr) in p - .code + p.code .iter() + .cloned() .map(|instr| { - let mut instr = pio::Instruction::decode(*instr, p.side_set).unwrap(); + if let Some(Instruction { + operands: InstructionOperands::JMP { condition, address }, + delay, + side_set, + }) = Instruction::decode(instr, p.side_set) + { + // JMP instruction. We need to apply offset here + let address = address + offset as u8; + assert!( + address < pio::RP2040_MAX_PROGRAM_SIZE as u8, + "Invalid JMP out of the program after offset addition" + ); - instr.operands = match instr.operands { - pio::InstructionOperands::JMP { condition, address } => { - // JMP instruction. We need to apply offset here - let address = address + offset as u8; - assert!( - address < pio::RP2040_MAX_PROGRAM_SIZE as u8, - "Invalid JMP out of the program after offset addition" - ); - pio::InstructionOperands::JMP { condition, address } + Instruction { + operands: InstructionOperands::JMP { condition, address }, + delay, + side_set, } - _ => instr.operands, - }; - - instr.encode(p.side_set) + .encode(p.side_set) + } else { + instr + } }) .enumerate() - { - self.pio.instr_mem[i + offset].write(|w| unsafe { w.bits(instr as u32) }) - } + .for_each(|(i, instr)| { + self.pio.instr_mem[i + offset].write(|w| unsafe { w.instr_mem0().bits(instr) }) + }); self.used_instruction_space |= Self::instruction_mask(p.code.len()) << offset; Ok(InstalledProgram { offset: offset as u8, @@ -470,28 +476,20 @@ impl UninitStateMachine { // Safety: The Send trait assumes this is the only write to sm_clkdiv fn set_clock_divisor(&self, int: u16, frac: u8) { - self.sm().sm_clkdiv.write(|w| { - unsafe { - w.int().bits(int); - w.frac().bits(frac); - } - - w - }); + // Safety: This is the only write to this register + unsafe { + self.sm() + .sm_clkdiv + .write(|w| w.int().bits(int).frac().bits(frac)); + } } - /// Execute the instruction immediately. - // Safety: The Send trait assumes this is the only write to sm_instr while uninitialized. The - // initialized `StateMachine` may also use this register. The `UnintStateMachine` is consumed - // by `PIOBuilder.build` to create `StateMachine` - fn exec_instruction(&mut self, instruction: u16) { - self.sm() - .sm_instr - .write(|w| unsafe { w.sm0_instr().bits(instruction) }) + unsafe fn sm(&self) -> &rp2040_pac::pio0::SM { + &*self.sm } - fn sm(&self) -> &rp2040_pac::pio0::SM { - unsafe { &*self.sm } + unsafe fn pio(&self) -> &rp2040_pac::pio0::RegisterBlock { + &*self.block } } @@ -541,28 +539,92 @@ impl StateMachine { /// The address of the instruction currently being executed. pub fn instruction_address(&self) -> u32 { - self.sm.sm().sm_addr.read().bits() + // Safety: Read only access without side effect + unsafe { self.sm.sm().sm_addr.read().bits() } } #[deprecated(note = "Renamed to exec_instruction")] - ///Execute the instruction immediately. + /// Execute the instruction immediately. pub fn set_instruction(&mut self, instruction: u16) { + let instruction = + Instruction::decode(instruction, self.program.side_set).expect("Invalid instruction"); self.exec_instruction(instruction); } /// Execute the instruction immediately. /// - /// While this is allowed even when the state machine is running, the datasheet says: - /// > If EXEC instructions are used, instructions written to INSTR must not stall. - /// It's unclear what happens if this is violated. - pub fn exec_instruction(&mut self, instruction: u16) { - // TODO: clarify what happens if the instruction stalls. - self.sm.exec_instruction(instruction); + /// If an instruction written to INSTR stalls, it is stored in the same instruction latch used + /// by OUT EXEC and MOV EXEC, and will overwrite an in-progress instruction there. If EXEC + /// instructions are used, instructions written to INSTR must not stall. + pub fn exec_instruction(&mut self, instruction: Instruction) { + let instruction = instruction.encode(self.program.side_set); + + // Safety: all accesses to this register are controlled by this instance + unsafe { + self.sm + .sm() + .sm_instr + .write(|w| w.sm0_instr().bits(instruction)) + } } /// Check if the current instruction is stalled. pub fn stalled(&self) -> bool { - self.sm.sm().sm_execctrl.read().exec_stalled().bits() + // Safety: read only access without side effect + unsafe { self.sm.sm().sm_execctrl.read().exec_stalled().bits() } + } + + /// Drain Tx fifo. + pub fn drain_tx_fifo(&mut self) { + // According to the datasheet 3.5.4.2 Page 358: + // + // When autopull is enabled, the behaviour of 'PULL' is altered: it becomes a no-op + // if the OSR is full. This is to avoid a race condition against the system + // DMA. It behaves as a fence: either an autopull has already taken place, in which case + // the 'PULL' has no effect, or the program will stall on the 'PULL' until data becomes + // available in the FIFO. + + // TODO: encode at compile time once pio 0.3.0 is out + const OUT: InstructionOperands = InstructionOperands::OUT { + destination: pio::OutDestination::NULL, + bit_count: 32, + }; + const PULL: InstructionOperands = InstructionOperands::PULL { + if_empty: false, + block: false, + }; + + // Safety: all accesses to these registers are controlled by this instance + unsafe { + let sm = &self.sm.sm(); + let sm_pinctrl = &sm.sm_pinctrl; + let sm_instr = &sm.sm_instr; + let fstat = &self.sm.pio().fstat; + + let operands = if sm.sm_shiftctrl.read().autopull().bit_is_set() { + OUT + } else { + PULL + } + .encode(); + + // Safety: sm0_instr may be accessed from SM::exec_instruction. + let mut saved_sideset_count = 0; + sm_pinctrl.modify(|r, w| { + saved_sideset_count = r.sideset_count().bits(); + w.sideset_count().bits(0) + }); + + let mask = 1 << SM::id(); + // white tx fifo is not empty + while (fstat.read().txempty().bits() & mask) == 0 { + sm_instr.write(|w| w.sm0_instr().bits(operands)) + } + + if saved_sideset_count != 0 { + sm_pinctrl.modify(|_, w| w.sideset_count().bits(saved_sideset_count)); + } + } } } @@ -590,14 +652,12 @@ impl StateMachine { let int = divisor as u16; let frac = ((divisor - int as f32) * 256.0) as u8; - self.sm.sm().sm_clkdiv.write(|w| { - unsafe { - w.int().bits(int); - w.frac().bits(frac); - } + self.sm.set_clock_divisor(int, frac); + } - w - }); + /// Change the clock divider of a stopped state machine using a 16.8 fixed point value. + pub fn clock_divisor_fixed_point(&mut self, int: u16, frac: u8) { + self.sm.set_clock_divisor(int, frac); } /// Sets the pin state for the specified pins. @@ -606,29 +666,50 @@ impl StateMachine { /// other state machines of the same PIO block. /// /// The iterator's item are pairs of `(pin_number, pin_state)`. - // Safety: this exclusively manages the SM resource and is created from the - // `UninitStateMachine` byt adding a program. pub fn set_pins(&mut self, pins: impl IntoIterator) { - let saved_ctrl = self.sm.sm().sm_pinctrl.read(); - let saved_execctrl = self.sm.sm().sm_execctrl.read(); - for (pin_num, pin_state) in pins { - self.sm - .sm() - .sm_pinctrl - .write(|w| unsafe { w.set_base().bits(pin_num).set_count().bits(1) }); - self.exec_instruction( - pio::InstructionOperands::SET { - destination: pio::SetDestination::PINS, - data: if PinState::High == pin_state { 1 } else { 0 }, - } - .encode(), - ); + // TODO: turn those three into const once pio 0.3.0 is released + let set_high_instr = InstructionOperands::SET { + destination: pio::SetDestination::PINS, + data: 1, + } + .encode(); + let set_low_instr = InstructionOperands::SET { + destination: pio::SetDestination::PINS, + data: 0, + } + .encode(); + + // Safety: all accesses to these registers are controlled by this instance + unsafe { + let sm = self.sm.sm(); + let sm_pinctrl = &sm.sm_pinctrl; + let sm_execctrl = &sm.sm_execctrl; + let sm_instr = &sm.sm_instr; + + // sideset_count is implicitly set to 0 when the set_base/set_count are written (rather + // than modified) + let saved_pin_ctrl = sm_pinctrl.read().bits(); + let mut saved_execctrl = 0; + + sm_execctrl.modify(|r, w| { + saved_execctrl = r.bits(); + w.out_sticky().clear_bit() + }); + + for (pin_num, pin_state) in pins { + sm_pinctrl.write(|w| w.set_base().bits(pin_num).set_count().bits(1)); + let instruction = if pin_state == PinState::High { + set_high_instr + } else { + set_low_instr + }; + + sm_instr.write(|w| w.sm0_instr().bits(instruction)) + } + + sm_pinctrl.write(|w| w.bits(saved_pin_ctrl)); + sm_execctrl.write(|w| w.bits(saved_execctrl)); } - let sm = self.sm.sm(); - sm.sm_pinctrl - .write(|w| unsafe { w.bits(saved_ctrl.bits()) }); - sm.sm_execctrl - .write(|w| unsafe { w.bits(saved_execctrl.bits()) }) } /// Set pin directions. @@ -637,33 +718,50 @@ impl StateMachine { /// other state machines of the same PIO block. /// /// The iterator's item are pairs of `(pin_number, pin_dir)`. - // Safety: this exclusively manages the SM resource and is created from the - // `UninitStateMachine` byt adding a program. pub fn set_pindirs(&mut self, pindirs: impl IntoIterator) { - let saved_ctrl = self.sm.sm().sm_pinctrl.read(); - let saved_execctrl = self.sm.sm().sm_execctrl.read(); - self.sm - .sm() - .sm_execctrl - .modify(|_, w| w.out_sticky().clear_bit()); - for (pinnum, pin_dir) in pindirs { - self.sm - .sm() - .sm_pinctrl - .write(|w| unsafe { w.set_base().bits(pinnum).set_count().bits(1) }); - self.exec_instruction( - pio::InstructionOperands::SET { - destination: pio::SetDestination::PINDIRS, - data: if PinDir::Output == pin_dir { 1 } else { 0 }, - } - .encode(), - ); + // TODO: turn those three into const once pio 0.3.0 is released + let set_output_instr = InstructionOperands::SET { + destination: pio::SetDestination::PINDIRS, + data: 1, + } + .encode(); + let set_input_instr = InstructionOperands::SET { + destination: pio::SetDestination::PINDIRS, + data: 0, + } + .encode(); + + // Safety: all accesses to these registers are controlled by this instance + unsafe { + let sm = self.sm.sm(); + let sm_pinctrl = &sm.sm_pinctrl; + let sm_execctrl = &sm.sm_execctrl; + let sm_instr = &sm.sm_instr; + + // sideset_count is implicitly set to 0 when the set_base/set_count are written (rather + // than modified) + let saved_pin_ctrl = sm_pinctrl.read().bits(); + let mut saved_execctrl = 0; + + sm_execctrl.modify(|r, w| { + saved_execctrl = r.bits(); + w.out_sticky().clear_bit() + }); + + for (pin_num, pin_dir) in pindirs { + sm_pinctrl.write(|w| w.set_base().bits(pin_num).set_count().bits(1)); + let instruction = if pin_dir == PinDir::Output { + set_output_instr + } else { + set_input_instr + }; + + sm_instr.write(|w| w.sm0_instr().bits(instruction)) + } + + sm_pinctrl.write(|w| w.bits(saved_pin_ctrl)); + sm_execctrl.write(|w| w.bits(saved_execctrl)); } - let sm = self.sm.sm(); - sm.sm_pinctrl - .write(|w| unsafe { w.bits(saved_ctrl.bits()) }); - sm.sm_execctrl - .write(|w| unsafe { w.bits(saved_execctrl.bits()) }); } } @@ -1060,10 +1158,7 @@ impl<'sm, SM: ValidStateMachine> Drop for Synchronize<'sm, SM> { // Restart the clocks of all state machines specified by the mask. // Bits 11:8 of CTRL contain CLKDIV_RESTART. let sm_mask = self.sm_mask << 8; - // Safety: We only use the atomic alias of the register. - unsafe { - write_bitmask_set((*self.sm.sm.block).ctrl.as_ptr(), sm_mask as u32); - } + self.sm.sm.set_ctrl_bits(sm_mask); } } @@ -1084,16 +1179,37 @@ impl StateMachine { pub fn restart(&mut self) { // pause the state machine self.sm.set_enabled(false); - // revert it to its wrap target - self.sm.exec_instruction( - pio::InstructionOperands::JMP { + + // Safety: all accesses to these registers are controlled by this instance + unsafe { + let sm = self.sm.sm(); + let sm_pinctrl = &sm.sm_pinctrl; + let sm_instr = &sm.sm_instr; + + // save exec_ctrl & make side_set optional + let mut saved_sideset_count = 0; + sm_pinctrl.modify(|r, w| { + saved_sideset_count = r.sideset_count().bits(); + w.sideset_count().bits(0) + }); + + // revert it to its wrap target + let instruction = InstructionOperands::JMP { condition: pio::JmpCondition::Always, address: self.program.wrap_target(), } - .encode(), - ); - // clear osr/isr - self.sm.restart(); + .encode(); + sm_instr.write(|w| w.sm0_instr().bits(instruction)); + + // restore exec_ctrl + if saved_sideset_count != 0 { + sm_pinctrl.modify(|_, w| w.sideset_count().bits(saved_sideset_count)); + } + + // clear osr/isr + self.sm.restart(); + } + // unpause the state machine self.sm.set_enabled(true); } @@ -1111,9 +1227,8 @@ unsafe impl Send for Rx {} // Safety: `Rx` is marked Send so ensure all accesses remain atomic and no new concurrent accesses // are added. impl Rx { - fn register_block(&self) -> &pac::pio0::RegisterBlock { - // Safety: The register is unique to this Rx instance. - unsafe { &*self.block } + unsafe fn block(&self) -> &pac::pio0::RegisterBlock { + &*self.block } /// Gets the FIFO's address. @@ -1123,7 +1238,9 @@ impl Rx { /// NB: You are responsible for using the pointer correctly and not /// underflowing the buffer. pub fn fifo_address(&self) -> *const u32 { - self.register_block().rxf[SM::id()].as_ptr() + // Safety: returning the address is safe as such. The user is responsible for any + // dereference ops at that address. + unsafe { self.block().rxf[SM::id()].as_ptr() } } /// Gets the FIFO's `DREQ` value. @@ -1147,36 +1264,41 @@ impl Rx { } // Safety: The register is unique to this Rx instance. - Some(self.register_block().rxf[SM::id() as usize].read().bits()) + Some(unsafe { core::ptr::read_volatile(self.fifo_address()) }) } /// Enable/Disable the autopush feature of the state machine. // Safety: This register is read by Rx, this is the only write. pub fn enable_autopush(&mut self, enable: bool) { - self.register_block().sm[SM::id()] - .sm_shiftctrl - .modify(|_, w| w.autopush().bit(enable)) + // Safety: only instance reading/writing to autopush bit and no other write to this + // register + unsafe { + self.block().sm[SM::id()] + .sm_shiftctrl + .modify(|_, w| w.autopush().bit(enable)) + } } /// Indicate if the rx FIFO is empty pub fn is_empty(&self) -> bool { - self.register_block().fstat.read().rxempty().bits() & (1 << SM::id()) != 0 + // Safety: Read only access without side effect + unsafe { self.block().fstat.read().rxempty().bits() & (1 << SM::id()) != 0 } } /// Indicate if the rx FIFO is full pub fn is_full(&self) -> bool { - self.register_block().fstat.read().rxfull().bits() & (1 << SM::id()) != 0 + // Safety: Read only access without side effect + unsafe { self.block().fstat.read().rxfull().bits() & (1 << SM::id()) != 0 } } /// Enable RX FIFO not empty interrupt. /// /// This interrupt is raised when the RX FIFO is not empty, i.e. one could read more data from it. pub fn enable_rx_not_empty_interrupt(&self, id: PioIRQ) { + // Safety: Atomic write to a single bit owned by this instance unsafe { write_bitmask_set( - self.register_block().sm_irq[id.to_index()] - .irq_inte - .as_ptr(), + self.block().sm_irq[id.to_index()].irq_inte.as_ptr(), 1 << SM::id(), ); } @@ -1184,11 +1306,10 @@ impl Rx { /// Disable RX FIFO not empty interrupt. pub fn disable_rx_not_empty_interrupt(&self, id: PioIRQ) { + // Safety: Atomic write to a single bit owned by this instance unsafe { write_bitmask_clear( - self.register_block().sm_irq[id.to_index()] - .irq_inte - .as_ptr(), + self.block().sm_irq[id.to_index()].irq_inte.as_ptr(), 1 << SM::id(), ); } @@ -1201,11 +1322,10 @@ impl Rx { } else { write_bitmask_clear }; + // Safety: Atomic write to a single bit owned by this instance unsafe { action( - self.register_block().sm_irq[id.to_index()] - .irq_intf - .as_ptr(), + self.block().sm_irq[id.to_index()].irq_intf.as_ptr(), 1 << SM::id(), ); } @@ -1224,9 +1344,21 @@ unsafe impl Send for Tx {} // Safety: `Tx` is marked Send so ensure all accesses remain atomic and no new concurrent accesses // are added. impl Tx { - fn register_block(&self) -> &pac::pio0::RegisterBlock { - // Safety: The register is unique to this Tx instance. - unsafe { &*self.block } + unsafe fn block(&self) -> &pac::pio0::RegisterBlock { + &*self.block + } + + fn write_generic(&mut self, value: T) -> bool { + if !self.is_full() { + // Safety: Only accessed by this instance (unless DMA is used). + unsafe { + let reg_ptr = self.fifo_address() as *mut T; + reg_ptr.write_volatile(value); + } + true + } else { + false + } } /// Gets the FIFO's address. @@ -1236,7 +1368,8 @@ impl Tx { /// NB: You are responsible for using the pointer correctly and not /// overflowing the buffer. pub fn fifo_address(&self) -> *const u32 { - self.register_block().txf[SM::id()].as_ptr() + // Safety: The only access to this register + unsafe { self.block().txf[SM::id()].as_ptr() } } /// Gets the FIFO's `DREQ` value. @@ -1255,19 +1388,7 @@ impl Tx { /// /// Returns `true` if the value was written to FIFO, `false` otherwise. pub fn write(&mut self, value: u32) -> bool { - // Safety: The register is never written by software. - let is_full = self.is_full(); - - if is_full { - return false; - } - - unsafe { - let reg_ptr = self.fifo_address() as *mut u32; - reg_ptr.write_volatile(value); - } - - true + self.write_generic(value) } /// Write a replicated u8 value to TX FIFO. @@ -1289,19 +1410,7 @@ impl Tx { /// /// [section_2_1_4]: pub fn write_u8_replicated(&mut self, value: u8) -> bool { - // Safety: The register is never written by software. - let is_full = self.is_full(); - - if is_full { - return false; - } - - unsafe { - let reg_ptr = self.fifo_address() as *mut u8; - reg_ptr.write_volatile(value); - } - - true + self.write_generic(value) } /// Write a replicated 16bit value to TX FIFO. @@ -1323,19 +1432,7 @@ impl Tx { /// /// [section_2_1_4]: pub fn write_u16_replicated(&mut self, value: u16) -> bool { - // Safety: The register is never written by software. - let is_full = self.is_full(); - - if is_full { - return false; - } - - unsafe { - let reg_ptr = self.fifo_address() as *mut u16; - reg_ptr.write_volatile(value); - } - - true + self.write_generic(value) } /// Checks if the state machine has stalled on empty TX FIFO during a blocking PULL, or an OUT @@ -1344,7 +1441,8 @@ impl Tx { /// **Note this is a sticky flag and may not reflect the current state of the machine.** pub fn has_stalled(&self) -> bool { let mask = 1 << SM::id(); - self.register_block().fdebug.read().txstall().bits() & mask == mask + // Safety: read-only access without side-effect + unsafe { self.block().fdebug.read().txstall().bits() & mask == mask } } /// Clears the `tx_stalled` flag. @@ -1352,66 +1450,31 @@ impl Tx { let mask = 1 << SM::id(); // Safety: These bits are WC, only the one corresponding to this SM is set. - self.register_block() - .fdebug - .write(|w| unsafe { w.txstall().bits(mask) }); + unsafe { + self.block().fdebug.write(|w| w.txstall().bits(mask)); + } } /// Indicate if the tx FIFO is empty pub fn is_empty(&self) -> bool { - self.register_block().fstat.read().txempty().bits() & (1 << SM::id()) != 0 + // Safety: read-only access without side-effect + unsafe { self.block().fstat.read().txempty().bits() & (1 << SM::id()) != 0 } } /// Indicate if the tx FIFO is full pub fn is_full(&self) -> bool { - self.register_block().fstat.read().txfull().bits() & (1 << SM::id()) != 0 - } - - /// Drain Tx fifo. - pub fn drain_fifo(&mut self) { - // According to the datasheet 3.5.4.2 Page 358: - // - // When autopull is enabled, the behaviour of 'PULL' is altered: it becomes a no-op - // if the OSR is full. This is to avoid a race condition against the system - // DMA. It behaves as a fence: either an autopull has already taken place, in which case - // the 'PULL' has no effect, or the program will stall on the 'PULL' until data becomes - // available in the FIFO. - let instr = if self.register_block().sm[SM::id()] - .sm_shiftctrl - .read() - .autopull() - .bit_is_set() - { - pio::InstructionOperands::OUT { - destination: pio::OutDestination::NULL, - bit_count: 32, - } - } else { - pio::InstructionOperands::PULL { - if_empty: false, - block: false, - } - } - .encode(); - // Safety: The only other place this register is written is - // `UninitStatemachine.exec_instruction`, `Tx` is only created after init. - let mask = 1 << SM::id(); - while self.register_block().fstat.read().txempty().bits() & mask != mask { - self.register_block().sm[SM::id()] - .sm_instr - .write(|w| unsafe { w.sm0_instr().bits(instr) }) - } + // Safety: read-only access without side-effect + unsafe { self.block().fstat.read().txfull().bits() & (1 << SM::id()) != 0 } } /// Enable TX FIFO not full interrupt. /// /// This interrupt is raised when the TX FIFO is not full, i.e. one could push more data to it. pub fn enable_tx_not_full_interrupt(&self, id: PioIRQ) { + // Safety: Atomic access to the register. Bit only modified by this Tx unsafe { write_bitmask_set( - self.register_block().sm_irq[id.to_index()] - .irq_inte - .as_ptr(), + self.block().sm_irq[id.to_index()].irq_inte.as_ptr(), 1 << (SM::id() + 4), ); } @@ -1419,11 +1482,10 @@ impl Tx { /// Disable TX FIFO not full interrupt. pub fn disable_tx_not_full_interrupt(&self, id: PioIRQ) { + // Safety: Atomic access to the register. Bit only modified by this Tx unsafe { write_bitmask_clear( - self.register_block().sm_irq[id.to_index()] - .irq_inte - .as_ptr(), + self.block().sm_irq[id.to_index()].irq_inte.as_ptr(), 1 << (SM::id() + 4), ); } @@ -1431,11 +1493,10 @@ impl Tx { /// Force TX FIFO not full interrupt. pub fn force_tx_not_full_interrupt(&self, id: PioIRQ) { + // Safety: Atomic access to the register. Bit only modified by this Tx unsafe { write_bitmask_set( - self.register_block().sm_irq[id.to_index()] - .irq_intf - .as_ptr(), + self.block().sm_irq[id.to_index()].irq_intf.as_ptr(), 1 << (SM::id() + 4), ); } @@ -1463,6 +1524,7 @@ impl<'a, P: PIOExt, const IRQ: usize> Interrupt<'a, P, IRQ> { /// does not correspond with the state machine index; any state machine can raise any one of the four interrupts. pub fn enable_sm_interrupt(&self, id: u8) { assert!(id < 4, "invalid state machine interrupt number"); + // Safety: Atomic write to a single bit owned by this instance unsafe { write_bitmask_set(self.irq().irq_inte.as_ptr(), 1 << (id + 8)); } @@ -1473,6 +1535,7 @@ impl<'a, P: PIOExt, const IRQ: usize> Interrupt<'a, P, IRQ> { /// See [`Self::enable_sm_interrupt`] for info about the index. pub fn disable_sm_interrupt(&self, id: u8) { assert!(id < 4, "invalid state machine interrupt number"); + // Safety: Atomic write to a single bit owned by this instance unsafe { write_bitmask_clear(self.irq().irq_inte.as_ptr(), 1 << (id + 8)); } @@ -1487,6 +1550,7 @@ impl<'a, P: PIOExt, const IRQ: usize> Interrupt<'a, P, IRQ> { /// See [`Self::enable_sm_interrupt`] for info about the index. pub fn force_sm_interrupt(&self, id: u8, set: bool) { assert!(id < 4, "invalid state machine interrupt number"); + // Safety: Atomic write to a single bit owned by this instance unsafe { if set { write_bitmask_set(self.irq().irq_intf.as_ptr(), 1 << (id + 8)); @@ -1506,6 +1570,7 @@ impl<'a, P: PIOExt, const IRQ: usize> Interrupt<'a, P, IRQ> { )] pub fn enable_tx_not_full_interrupt(&self, id: u8) { assert!(id < 4, "invalid state machine interrupt number"); + // Safety: Atomic write to a single bit owned by this instance unsafe { write_bitmask_set(self.irq().irq_inte.as_ptr(), 1 << (id + 4)); } @@ -1520,6 +1585,7 @@ impl<'a, P: PIOExt, const IRQ: usize> Interrupt<'a, P, IRQ> { )] pub fn disable_tx_not_full_interrupt(&self, id: u8) { assert!(id < 4, "invalid state machine interrupt number"); + // Safety: Atomic write to a single bit owned by this instance unsafe { write_bitmask_clear(self.irq().irq_inte.as_ptr(), 1 << (id + 4)); } @@ -1534,6 +1600,7 @@ impl<'a, P: PIOExt, const IRQ: usize> Interrupt<'a, P, IRQ> { )] pub fn force_tx_not_full_interrupt(&self, id: u8) { assert!(id < 4, "invalid state machine interrupt number"); + // Safety: Atomic write to a single bit owned by this instance unsafe { write_bitmask_set(self.irq().irq_intf.as_ptr(), 1 << (id + 4)); } @@ -1549,6 +1616,7 @@ impl<'a, P: PIOExt, const IRQ: usize> Interrupt<'a, P, IRQ> { )] pub fn enable_rx_not_empty_interrupt(&self, id: u8) { assert!(id < 4, "invalid state machine interrupt number"); + // Safety: Atomic write to a single bit owned by this instance unsafe { write_bitmask_set(self.irq().irq_inte.as_ptr(), 1 << id); } @@ -1563,6 +1631,7 @@ impl<'a, P: PIOExt, const IRQ: usize> Interrupt<'a, P, IRQ> { )] pub fn disable_rx_not_empty_interrupt(&self, id: u8) { assert!(id < 4, "invalid state machine interrupt number"); + // Safety: Atomic write to a single bit owned by this instance unsafe { write_bitmask_clear(self.irq().irq_inte.as_ptr(), 1 << id); } @@ -1577,6 +1646,7 @@ impl<'a, P: PIOExt, const IRQ: usize> Interrupt<'a, P, IRQ> { )] pub fn force_rx_not_empty_interrupt(&self, id: u8) { assert!(id < 4, "invalid state machine interrupt number"); + // Safety: Atomic write to a single bit owned by this instance unsafe { write_bitmask_set(self.irq().irq_intf.as_ptr(), 1 << id); } @@ -1586,22 +1656,28 @@ impl<'a, P: PIOExt, const IRQ: usize> Interrupt<'a, P, IRQ> { /// /// This is the state of the interrupts without interrupt masking and forcing. pub fn raw(&self) -> InterruptState { - InterruptState(self.register_block().intr.read().bits()) + InterruptState( + // Safety: Read only access without side effect + unsafe { self.block().intr.read().bits() }, + ) } /// Get the interrupt state. /// /// This is the state of the interrupts after interrupt masking and forcing. pub fn state(&self) -> InterruptState { - InterruptState(self.irq().irq_ints.read().bits()) + InterruptState( + // Safety: Read only access without side effect + unsafe { self.irq().irq_ints.read().bits() }, + ) } - fn register_block(&self) -> &rp2040_pac::pio0::RegisterBlock { - unsafe { &*self.block } + unsafe fn block(&self) -> &rp2040_pac::pio0::RegisterBlock { + &*self.block } - fn irq(&self) -> &rp2040_pac::pio0::SM_IRQ { - &self.register_block().sm_irq[IRQ] + unsafe fn irq(&self) -> &rp2040_pac::pio0::SM_IRQ { + &self.block().sm_irq[IRQ] } } @@ -1917,74 +1993,61 @@ impl PIOBuilder

{ // Write all configuration bits sm.set_clock_divisor(self.clock_divisor.0, self.clock_divisor.1); - sm.sm().sm_execctrl.write(|w| { - w.side_en().bit(self.program.side_set.optional()); - w.side_pindir().bit(self.program.side_set.pindirs()); + // Safety: Only instance owning the SM + unsafe { + sm.sm().sm_execctrl.write(|w| { + w.side_en().bit(self.program.side_set.optional()); + w.side_pindir().bit(self.program.side_set.pindirs()); - unsafe { w.jmp_pin().bits(self.jmp_pin); - } - if let Some(inline_out) = self.inline_out { - w.inline_out_en().bit(true); - unsafe { + if let Some(inline_out) = self.inline_out { + w.inline_out_en().bit(true); w.out_en_sel().bits(inline_out); + } else { + w.inline_out_en().bit(false); } - } else { - w.inline_out_en().bit(false); - } - w.out_sticky().bit(self.out_sticky); + w.out_sticky().bit(self.out_sticky); - unsafe { w.wrap_top().bits(offset as u8 + self.program.wrap.source); w.wrap_bottom() .bits(offset as u8 + self.program.wrap.target); - } - let n = match self.mov_status { - MovStatusConfig::Tx(n) => { - w.status_sel().bit(false); - n - } - MovStatusConfig::Rx(n) => { - w.status_sel().bit(true); - n - } - }; - unsafe { - w.status_n().bits(n); - } + let n = match self.mov_status { + MovStatusConfig::Tx(n) => { + w.status_sel().bit(false); + n + } + MovStatusConfig::Rx(n) => { + w.status_sel().bit(true); + n + } + }; + w.status_n().bits(n) + }); - w - }); + sm.sm().sm_shiftctrl.write(|w| { + let (fjoin_rx, fjoin_tx) = match self.fifo_join { + Buffers::RxTx => (false, false), + Buffers::OnlyTx => (false, true), + Buffers::OnlyRx => (true, false), + }; + w.fjoin_rx().bit(fjoin_rx); + w.fjoin_tx().bit(fjoin_tx); - sm.sm().sm_shiftctrl.write(|w| { - let (fjoin_rx, fjoin_tx) = match self.fifo_join { - Buffers::RxTx => (false, false), - Buffers::OnlyTx => (false, true), - Buffers::OnlyRx => (true, false), - }; - w.fjoin_rx().bit(fjoin_rx); - w.fjoin_tx().bit(fjoin_tx); - - unsafe { // TODO: Encode 32 as zero, and error on 0 w.pull_thresh().bits(self.pull_threshold); w.push_thresh().bits(self.push_threshold); - } - w.out_shiftdir().bit(self.out_shiftdir.bit()); - w.in_shiftdir().bit(self.in_shiftdir.bit()); + w.out_shiftdir().bit(self.out_shiftdir.bit()); + w.in_shiftdir().bit(self.in_shiftdir.bit()); - w.autopull().bit(self.autopull); - w.autopush().bit(self.autopush); + w.autopull().bit(self.autopull); + w.autopush().bit(self.autopush) + }); - w - }); - - sm.sm().sm_pinctrl.write(|w| { - unsafe { + sm.sm().sm_pinctrl.write(|w| { w.sideset_count().bits(self.program.side_set.bits()); w.set_count().bits(self.set_count); w.out_count().bits(self.out_count); @@ -1992,11 +2055,9 @@ impl PIOBuilder

{ w.in_base().bits(self.in_base); w.sideset_base().bits(self.side_set_base); w.set_base().bits(self.set_base); - w.out_base().bits(self.out_base); - } - - w - }); + w.out_base().bits(self.out_base) + }) + } // Restart SM and its clock sm.restart(); @@ -2004,13 +2065,15 @@ impl PIOBuilder

{ // Set starting location by forcing the state machine to execute a jmp // to the beginning of the program we loaded in. - sm.exec_instruction( - pio::InstructionOperands::JMP { - condition: pio::JmpCondition::Always, - address: offset as u8, - } - .encode(), - ); + let instr = InstructionOperands::JMP { + condition: pio::JmpCondition::Always, + address: offset as u8, + } + .encode(); + // Safety: Only instance owning the SM + unsafe { + sm.sm().sm_instr.write(|w| w.sm0_instr().bits(instr)); + } let rx = Rx { block: sm.block,