Skip to content

embassy-rp: fix set_input_sync_bypass pin offset#5116

Draft
diogo464 wants to merge 1 commit into
embassy-rs:mainfrom
diogo464:fix-input-sync-bypass
Draft

embassy-rp: fix set_input_sync_bypass pin offset#5116
diogo464 wants to merge 1 commit into
embassy-rs:mainfrom
diogo464:fix-input-sync-bypass

Conversation

@diogo464

Copy link
Copy Markdown
Contributor

currently this function causes a crash in debug mode when used with pins greater than 32 (available on the rp235xb variant) because it overflows when doing the shift. this commit applies the offset to pin value before doing the shift. this assumes that when using a pin greater than 32 the GPIOBASE has already been set to 16.

@Dirbaio

Dirbaio commented Dec 19, 2025

Copy link
Copy Markdown
Member

this assumes that when using a pin greater than 32 the GPIOBASE has already been set to 16.

this assumption isn't quite right. gpiobase is calculated when initializing the PIO according to the entire set of used pins, see here:

PIO::PIO.gpiobase().write(|w| w.set_gpiobase(shift == 16));

so pins 16..31 can either need offset 0 or offset 16 depending on what other pins are in use.

You can probably read the GPIOBASE reg to know the offset to use.

currently this function causes a crash in debug mode when used with pins greater than
32 (available on the rp235xb variant) because it overflows when doing
the shift. this commit applies the GPIOBASE offset to pin value before doing the
shift.
@diogo464
diogo464 force-pushed the fix-input-sync-bypass branch from 46f0516 to eccf60a Compare December 20, 2025 09:09
@diogo464
diogo464 marked this pull request as draft December 20, 2025 09:09
@diogo464

Copy link
Copy Markdown
Contributor Author

ok, I have force pushed to change the commit message and turned this into a draft because right now it is not working.

You can probably read the GPIOBASE reg to know the offset to use.

I updated the code to read that register but the problem is that at the time the set_input_sync_bypass function is called the register has not yet been set to 16.
To give a more specific example, I am using the cyw43-pio crate with a dev board that uses those higher gpio pins to communicate with the cyw43.
The pio driver calls the function on line 189

pin_io.set_input_sync_bypass(true);

but the GPIOBASE register is only set later on line 210
sm.set_config(&cfg);

when setting the configuration for the state machine, that happens on that line you linked in your comment above.

I am fairly new to this embedded stuff so I am not sure about this but I was going through the RP2350 data sheet and the impression I get is that there are 3 PIO blocks each with its own independent GPIOBASE register and 4 state machines each. right now it seems that this GPIOBASE register is written to in the StateMachine::set_config function but each state machine in the same PIO block uses the same GPIOBASE register so if we used more than one state machine and they needed different offsets this would cause a problem. I am not sure but maybe the offset could be passed in to Pio::new and then instead of doing this check

let mut low_ok = true;
let mut high_ok = true;
let in_pins = config.pins.in_base..config.pins.in_base + config.in_count;
let side_pins = config.pins.sideset_base..config.pins.sideset_base + config.pins.sideset_count;
let set_pins = config.pins.set_base..config.pins.set_base + config.pins.set_count;
let out_pins = config.pins.out_base..config.pins.out_base + config.pins.out_count;
for pin_range in [in_pins, side_pins, set_pins, out_pins] {
for pin in pin_range {
low_ok &= pin < 32;
high_ok &= pin >= 16;
}
}
if !low_ok && !high_ok {
panic!(
"All pins must either be <32 or >=16, in:{:?}-{:?}, side:{:?}-{:?}, set:{:?}-{:?}, out:{:?}-{:?}",
config.pins.in_base,
config.pins.in_base + config.in_count - 1,
config.pins.sideset_base,
config.pins.sideset_base + config.pins.sideset_count - 1,
config.pins.set_base,
config.pins.set_base + config.pins.set_count - 1,
config.pins.out_base,
config.pins.out_base + config.pins.out_count - 1,
)
}
let shift = if low_ok { 0 } else { 16 };

in the StateMachine::set_config, this function could just panic if the offset is enabled but a pin lower than 16 is used or if there is no offset and a pin greater than 31 is used. Any idea on how to fix this @Dirbaio ?

@cedrivard

Copy link
Copy Markdown

Confirming this bug affects the SparkFun IoT RedBoard RP2350 (which uses GPIOs 36/37/38 for the RM2 WiFi module's CS/CLK/DATA lines).

Symptom:
cyw43::new() hangs forever in the FEEDBEAD SPI bus test loop. The first read returns 0x83830383 (garbled data) instead of 0xFEEDBEAD, then subsequent reads return 0xFFFFFFFF. The CYW43 chip is functional (verified with MicroPython on the same board).

Root cause:
set_input_sync_bypass(true) on GPIO 38 computes 1u32 << 38, which wraps to 1 << 6 in release mode — setting the wrong bit. Without the sync bypass, the PIO input synchronizer adds 2-3 clock cycles of latency, causing the PIO to sample stale/garbage data from the CYW43.

Workaround:
Manually write the correct bits to the INPUT_SYNC_BYPASS register after PioSpi::new() returns (at which point gpiobase has been set by sm.set_config()):

  // Fix sync bypass for GPIOs 37 (CLK) and 38 (DATA) with gpiobase=16                                                                                                                                            
  unsafe {                                                                                                                                                                                                        
      let pio0_base = 0x5020_0000u32;                                                                                                                                                                             
      let sync_bypass_offset = 0x0c8u32;                                                                                                                                                                          
      let reg = (pio0_base + sync_bypass_offset) as *mut u32;                                                                                                                                                     
      let current = core::ptr::read_volatile(reg);                                                                                                                                                                
      core::ptr::write_volatile(reg, current | (1 << 22) | (1 << 21));                                                                                                                                            
  } 

@cedrivard

Copy link
Copy Markdown

I've opened #6391 with a fix for this: it offsets the input_sync_bypass mask by GPIOBASE in embassy-rp, and reorders the cyw43-pio call to after set_config so GPIOBASE is established first. Verified on hardware (RP2350B, CYW43 on GPIO 36/37/38).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants