Set EXTEXCLALL when enabling multi-core operation (#898)

* Add on-target test to verify atomics

* Set EXTEXCLALL when enabling multi-core operation

Fixes #897

* Reset core1 at top of on-target tests

* cargo fmt

on-target-tests/Cargo.toml

@@ -45,6 +45,10 @@ name = "i2c_loopback_async"
 harness = false
 name = "gpio"
 
+[[test]]
+harness = false
+name = "multicore"
+
 [dependencies]
 cortex-m = "0.7"
 cortex-m-rt = "0.7"
@@ -65,6 +69,7 @@ panic-probe = {version = "0.3", features = ["print-defmt"]}
 rp2040-boot2 = { version = "0.3.0", optional = true }
 rp2040-hal = {path = "../rp2040-hal", features = ["critical-section-impl", "defmt", "rt", "i2c-write-iter"], optional = true}
 rp235x-hal = {path = "../rp235x-hal", features = ["critical-section-impl", "defmt", "rt", "i2c-write-iter"], optional = true}
+portable-atomic = {version = "1.7.0", features = ["critical-section"]}
 
 [features]
 

on-target-tests/tests/dma_dyn.rs

@@ -53,6 +53,8 @@ mod testdata {
     ];
 }
 
+mod init;
+
 #[defmt_test::tests]
 mod tests {
     use crate::testdata;
@@ -73,7 +75,7 @@ mod tests {
     #[init]
     fn setup() -> State {
         unsafe {
-            hal::sio::spinlock_reset();
+            crate::init::reset_cleanup();
         }
         let mut pac = pac::Peripherals::take().unwrap();
         #[cfg(feature = "rp2040")]

on-target-tests/tests/dma_m2m_u16.rs

@@ -49,6 +49,8 @@ mod testdata {
     ];
 }
 
+mod init;
+
 #[defmt_test::tests]
 mod tests {
     use crate::testdata;
@@ -69,7 +71,7 @@ mod tests {
     #[init]
     fn setup() -> State {
         unsafe {
-            hal::sio::spinlock_reset();
+            crate::init::reset_cleanup();
         }
         let mut pac = pac::Peripherals::take().unwrap();
         #[cfg(feature = "rp2040")]

on-target-tests/tests/dma_m2m_u32.rs

@@ -49,6 +49,8 @@ mod testdata {
     ];
 }
 
+mod init;
+
 #[defmt_test::tests]
 mod tests {
     use crate::testdata;
@@ -69,7 +71,7 @@ mod tests {
     #[init]
     fn setup() -> State {
         unsafe {
-            hal::sio::spinlock_reset();
+            crate::init::reset_cleanup();
         }
         let mut pac = pac::Peripherals::take().unwrap();
         #[cfg(feature = "rp2040")]

on-target-tests/tests/dma_m2m_u8.rs

@@ -49,6 +49,8 @@ mod testdata {
     ];
 }
 
+mod init;
+
 #[defmt_test::tests]
 mod tests {
     use crate::testdata;
@@ -69,7 +71,7 @@ mod tests {
     #[init]
     fn setup() -> State {
         unsafe {
-            hal::sio::spinlock_reset();
+            crate::init::reset_cleanup();
         }
         let mut pac = pac::Peripherals::take().unwrap();
         #[cfg(feature = "rp2040")]

on-target-tests/tests/dma_spi_loopback_u16.rs

@@ -58,6 +58,8 @@ mod testdata {
     ];
 }
 
+mod init;
+
 #[defmt_test::tests]
 mod tests {
     use crate::testdata;
@@ -78,7 +80,7 @@ mod tests {
     #[init]
     fn setup() -> State {
         unsafe {
-            hal::sio::spinlock_reset();
+            crate::init::reset_cleanup();
         }
         let mut pac = pac::Peripherals::take().unwrap();
         #[cfg(feature = "rp2040")]

on-target-tests/tests/dma_spi_loopback_u8.rs

@@ -43,6 +43,8 @@ struct State {
     spi: Option<spi::Spi<spi::Enabled, SPI0, (MOSI, MISO, SCLK), 8>>,
 }
 
+mod init;
+
 mod testdata {
     #[allow(dead_code)]
     pub const ARRAY_U8: [u8; 10] = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
@@ -79,7 +81,7 @@ mod tests {
     #[init]
     fn setup() -> State {
         unsafe {
-            hal::sio::spinlock_reset();
+            crate::init::reset_cleanup();
         }
         let mut pac = pac::Peripherals::take().unwrap();
         #[cfg(feature = "rp2040")]

on-target-tests/tests/gpio.rs

@@ -29,6 +29,8 @@ pub static IMAGE_DEF: hal::block::ImageDef = hal::block::ImageDef::secure_exe();
 /// if your board has a different frequency
 const XTAL_FREQ_HZ: u32 = 12_000_000u32;
 
+mod init;
+
 #[defmt_test::tests]
 mod tests {
     use crate::hal;
@@ -41,7 +43,7 @@ mod tests {
     #[init]
     fn setup() -> () {
         unsafe {
-            hal::sio::spinlock_reset();
+            crate::init::reset_cleanup();
         }
         let mut pac = pac::Peripherals::take().unwrap();
         #[cfg(feature = "rp2040")]

on-target-tests/tests/i2c_loopback.rs

@@ -39,6 +39,7 @@ pub static IMAGE_DEF: hal::block::ImageDef = hal::block::ImageDef::secure_exe();
 const XTAL_FREQ_HZ: u32 = 12_000_000u32;
 
 pub mod i2c_tests;
+mod init;
 
 #[interrupt]
 unsafe fn I2C1_IRQ() {
@@ -51,6 +52,7 @@ mod tests {
 
     #[init]
     fn setup() -> State {
+        unsafe { crate::init::reset_cleanup() };
         i2c_tests::blocking::setup(super::XTAL_FREQ_HZ, ADDR_7BIT)
     }
 

on-target-tests/tests/i2c_loopback_async.rs

@@ -39,6 +39,7 @@ pub static IMAGE_DEF: hal::block::ImageDef = hal::block::ImageDef::secure_exe();
 const XTAL_FREQ_HZ: u32 = 12_000_000u32;
 
 pub mod i2c_tests;
+mod init;
 
 #[interrupt]
 unsafe fn I2C0_IRQ() {
@@ -59,6 +60,7 @@ mod tests {
 
     #[init]
     fn setup() -> State {
+        unsafe { crate::init::reset_cleanup() };
         non_blocking::setup(super::XTAL_FREQ_HZ, ADDR_7BIT)
     }
 

on-target-tests/tests/i2c_tests/blocking.rs

@@ -58,9 +58,6 @@ macro_rules! assert_restart_count {
 }
 
 pub fn setup<T: ValidAddress>(xtal_freq_hz: u32, addr: T) -> State {
-    unsafe {
-        hal::sio::spinlock_reset();
-    }
     let mut pac = pac::Peripherals::take().unwrap();
     let mut watchdog = Watchdog::new(pac.WATCHDOG);
 

on-target-tests/tests/i2c_tests/non_blocking.rs

@@ -52,9 +52,6 @@ async fn wait_with(payload: &RefCell<TargetState>, mut f: impl FnMut(&TargetStat
 }
 
 pub fn setup<T: ValidAddress>(xtal_freq_hz: u32, addr: T) -> State {
-    unsafe {
-        hal::sio::spinlock_reset();
-    }
     let mut pac = pac::Peripherals::take().unwrap();
     let mut watchdog = Watchdog::new(pac.WATCHDOG);
 

on-target-tests/tests/init/mod.rs

@@ -0,0 +1,27 @@
+#[cfg(feature = "rp2040")]
+use rp2040_hal as hal;
+#[cfg(feature = "rp235x")]
+use rp235x_hal as hal;
+
+use hal::pac;
+
+/// When test cases are run from a debugger, there may not be a complete
+/// system reset between test cases. To get clean initial conditions, reset
+/// core1 and spinlocks.
+///
+/// This must only be called immediatly after booting core0, ie. at the start
+/// of the `#[init]` function.
+pub unsafe fn reset_cleanup() {
+    unsafe {
+        (*pac::PSM::PTR)
+            .frce_off()
+            .modify(|_, w| w.proc1().set_bit());
+        while !(*pac::PSM::PTR).frce_off().read().proc1().bit_is_set() {
+            cortex_m::asm::nop();
+        }
+        (*pac::PSM::PTR)
+            .frce_off()
+            .modify(|_, w| w.proc1().clear_bit());
+        hal::sio::spinlock_reset();
+    }
+}

on-target-tests/tests/multicore.rs

@@ -0,0 +1,117 @@
+#![no_std]
+#![no_main]
+#![cfg(test)]
+
+use defmt_rtt as _; // defmt transport
+use defmt_test as _;
+use panic_probe as _;
+#[cfg(feature = "rp2040")]
+use rp2040_hal as hal; // memory layout // panic handler
+#[cfg(feature = "rp235x")]
+use rp235x_hal as hal;
+
+use core::sync::atomic::Ordering;
+use portable_atomic::{AtomicU32, AtomicU8};
+
+/// The linker will place this boot block at the start of our program image. We
+/// need this to help the ROM bootloader get our code up and running.
+/// Note: This boot block is not necessary when using a rp-hal based BSP
+/// as the BSPs already perform this step.
+#[cfg(feature = "rp2040")]
+#[link_section = ".boot2"]
+#[used]
+pub static BOOT2: [u8; 256] = rp2040_boot2::BOOT_LOADER_GENERIC_03H;
+
+/// Tell the Boot ROM about our application
+#[cfg(feature = "rp235x")]
+#[link_section = ".start_block"]
+#[used]
+pub static IMAGE_DEF: hal::block::ImageDef = hal::block::ImageDef::secure_exe();
+
+/// External high-speed crystal on the Raspberry Pi Pico board is 12 MHz. Adjust
+/// if your board has a different frequency
+const XTAL_FREQ_HZ: u32 = 12_000_000u32;
+
+static STATE: AtomicU8 = AtomicU8::new(0);
+static COUNTER: AtomicU32 = AtomicU32::new(0);
+const STEPS: u32 = 100000;
+
+mod init;
+
+#[defmt_test::tests]
+mod tests {
+    use crate::hal;
+    use crate::hal::clocks::init_clocks_and_plls;
+    use crate::hal::pac;
+    use crate::XTAL_FREQ_HZ;
+    use hal::watchdog::Watchdog;
+
+    use core::sync::atomic::Ordering;
+    use hal::multicore::{Multicore, Stack};
+
+    static CORE1_STACK: Stack<4096> = Stack::new();
+
+    #[init]
+    fn setup() -> () {
+        unsafe {
+            crate::init::reset_cleanup();
+        }
+        let mut pac = pac::Peripherals::take().unwrap();
+        #[cfg(feature = "rp2040")]
+        let _core = pac::CorePeripherals::take().unwrap();
+        let mut watchdog = Watchdog::new(pac.WATCHDOG);
+
+        let _clocks = init_clocks_and_plls(
+            XTAL_FREQ_HZ,
+            pac.XOSC,
+            pac.CLOCKS,
+            pac.PLL_SYS,
+            pac.PLL_USB,
+            &mut pac.RESETS,
+            &mut watchdog,
+        )
+        .ok()
+        .unwrap();
+
+        let mut sio = hal::Sio::new(pac.SIO);
+
+        let _pins = hal::gpio::Pins::new(
+            pac.IO_BANK0,
+            pac.PADS_BANK0,
+            sio.gpio_bank0,
+            &mut pac.RESETS,
+        );
+
+        let mut mc = Multicore::new(&mut pac.PSM, &mut pac.PPB, &mut sio.fifo);
+
+        let cores = mc.cores();
+        let core1 = &mut cores[1];
+        let _test = core1.spawn(CORE1_STACK.take().unwrap(), move || super::core1_task());
+    }
+
+    #[test]
+    fn check_atomics() {
+        super::STATE.store(1, Ordering::Release);
+        for _ in 0..super::STEPS {
+            super::COUNTER.fetch_add(1, Ordering::Relaxed);
+        }
+        while super::STATE.load(Ordering::Acquire) != 2 {}
+
+        let counter = super::COUNTER.load(Ordering::Acquire);
+        assert_eq!(2 * super::STEPS, counter);
+    }
+}
+
+fn core1_task() {
+    loop {
+        match STATE.load(Ordering::Acquire) {
+            1 => {
+                for _ in 0..STEPS {
+                    COUNTER.fetch_add(1, Ordering::Relaxed);
+                }
+                STATE.store(2, Ordering::Release);
+            }
+            _ => (),
+        }
+    }
+}

rp235x-hal/src/multicore.rs

@@ -71,6 +71,26 @@ fn core1_setup(stack_limit: *mut usize) {
     // TODO: irq priorities
 }
 
+/// Set the EXTEXCLALL bit in ACTLR.
+///
+/// The default MPU memory map marks all memory as non-shareable, so atomics don't
+/// synchronize memory accesses between cores at all. This bit forces all memory to be
+/// considered shareable regardless of what the MPU says.
+///
+/// TODO: does this interfere somehow if the user wants to use a custom MPU configuration?
+/// maybe we need to add a way to disable this?
+///
+/// This must be done FOR EACH CORE.
+///
+/// (Copied from https://github.com/embassy-rs/embassy/blob/9da04cc38ea5cc17740bd9921f9f5cbb1c689a31/embassy-rp/src/lib.rs)
+fn enable_actlr_extexclall() {
+    unsafe {
+        (*cortex_m::peripheral::ICB::PTR)
+            .actlr
+            .modify(|w| w | (1 << 29));
+    }
+}
+
 /// Multicore execution management.
 pub struct Multicore<'p> {
     cores: [Core<'p>; 2],
@@ -245,6 +265,8 @@ impl Core<'_> {
     where
         F: FnOnce() + Send + 'static,
     {
+        // Needs to be enabled on both cores to make atomics work between cores.
+        enable_actlr_extexclall();
         if let Some((psm, ppb, fifo)) = self.inner.as_mut() {
             // The first two ignored `u64` parameters are there to take up all of the registers,
             // which means that the rest of the arguments are taken from the stack,
@@ -255,6 +277,8 @@ impl Core<'_> {
                 entry: *mut ManuallyDrop<F>,
                 stack_limit: *mut usize,
             ) -> ! {
+                // Needs to be enabled on both cores to make atomics work between cores.
+                enable_actlr_extexclall();
                 core1_setup(stack_limit);
 
                 let entry = unsafe { ManuallyDrop::take(&mut *entry) };