From 6cec6fa09b3bcc01ada4d5d5decd02cc2a3a8e4f Mon Sep 17 00:00:00 2001
From: pennae <github@quasiparticle.net>
Date: Tue, 25 Apr 2023 23:17:57 +0200
Subject: [PATCH] rp/pio: don't use modify on shared registers

pio control registers are notionally shared between state machines as
well. state machine operations that change these registers must use
atomic accesses (or critical sections, which would be overkill).

notably PioPin::set_input_sync_bypass was even wrong, enabling the
bypass on a pin requires the corresponding bit to be set (not cleared).
the PioCommon function got it right.
---
 embassy-rp/src/pio.rs | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/embassy-rp/src/pio.rs b/embassy-rp/src/pio.rs
index b20ebbc7..e9a67fd4 100644
--- a/embassy-rp/src/pio.rs
+++ b/embassy-rp/src/pio.rs
@@ -298,9 +298,11 @@ impl<PIO: PioInstance> PioPin<PIO> {
     pub fn set_input_sync_bypass<'a>(&mut self, bypass: bool) {
         let mask = 1 << self.pin();
         unsafe {
-            PIO::PIO
-                .input_sync_bypass()
-                .modify(|w| *w = if bypass { *w & !mask } else { *w | mask });
+            if bypass {
+                PIO::PIO.input_sync_bypass().write_set(|w| *w = mask);
+            } else {
+                PIO::PIO.input_sync_bypass().write_clear(|w| *w = mask);
+            }
         }
     }
 
@@ -336,18 +338,19 @@ pub trait PioStateMachine: sealed::PioStateMachine + Sized + Unpin {
     }
 
     fn restart(&mut self) {
+        let mask = 1u8 << Self::Sm::SM_NO;
         unsafe {
-            Self::Pio::PIO
-                .ctrl()
-                .modify(|w| w.set_sm_restart(1u8 << Self::Sm::SM_NO));
+            Self::Pio::PIO.ctrl().write_set(|w| w.set_sm_restart(mask));
         }
     }
     fn set_enable(&mut self, enable: bool) {
         let mask = 1u8 << Self::Sm::SM_NO;
         unsafe {
-            Self::Pio::PIO
-                .ctrl()
-                .modify(|w| w.set_sm_enable((w.sm_enable() & !mask) | (if enable { mask } else { 0 })));
+            if enable {
+                Self::Pio::PIO.ctrl().write_set(|w| w.set_sm_enable(mask));
+            } else {
+                Self::Pio::PIO.ctrl().write_clear(|w| w.set_sm_enable(mask));
+            }
         }
     }
 
@@ -419,10 +422,9 @@ pub trait PioStateMachine: sealed::PioStateMachine + Sized + Unpin {
     }
 
     fn clkdiv_restart(&mut self) {
+        let mask = 1u8 << Self::Sm::SM_NO;
         unsafe {
-            Self::Pio::PIO
-                .ctrl()
-                .modify(|w| w.set_clkdiv_restart(1u8 << Self::Sm::SM_NO));
+            Self::Pio::PIO.ctrl().write_set(|w| w.set_clkdiv_restart(mask));
         }
     }
 
@@ -869,9 +871,11 @@ pub trait PioCommon: sealed::PioCommon + Sized {
 
     fn set_input_sync_bypass<'a>(&'a mut self, bypass: u32, mask: u32) {
         unsafe {
-            Self::Pio::PIO
-                .input_sync_bypass()
-                .modify(|w| *w = (*w & !mask) | (bypass & mask));
+            // this can interfere with per-pin bypass functions. splitting the
+            // modification is going to be fine since nothing that relies on
+            // it can reasonably run before we finish.
+            Self::Pio::PIO.input_sync_bypass().write_set(|w| *w = mask & bypass);
+            Self::Pio::PIO.input_sync_bypass().write_clear(|w| *w = mask & !bypass);
         }
     }