hardware_pio: fix pio_encode_mov(pio_pindirs, ...) on RP2350#2946
Closed
anoshyn wants to merge 1 commit into
Closed
hardware_pio: fix pio_encode_mov(pio_pindirs, ...) on RP2350#2946anoshyn wants to merge 1 commit into
anoshyn wants to merge 1 commit into
Conversation
On RP2350, MOV destination PINDIRS exists and encodes as field 3 in the instruction word. SET/IN/OUT continue to encode PINDIRS as 4. The pio_src_dest enum collapses these into a single value (pio_pindirs = 4) because SET/IN/OUT see it that way, so the MOV encoder was emitting field 4 - that's MOV EXEC, not MOV PINDIRS. pio_encode_mov(pio_pindirs, pio_x) was returning 0xA081 instead of the correct 0xA061 (raspberrypi#2754). The bug was hard to fix simply because pio_pindirs and pio_exec_mov are numerically identical in release builds: both reduce to 4 once the NDEBUG-gated _PIO_INVALID_* flags are zero. A naive remap of "dest == 4 to 3 in MOV" would also break pio_encode_mov(pio_exec_mov, ...), where field 4 is correct. Add a non-NDEBUG-gated marker bit (_PIO_MARKER_PINDIRS, 0x100) carried on pio_pindirs only. _pio_mov_dest_bits() checks the marker and remaps to 3 on PICO_PIO_VERSION > 0. The existing & 7u masks in every encoder discard the high bits before they reach the emitted instruction word, so no other encoder is affected. Also drop _PIO_INVALID_MOV_DEST from pio_pindirs on RP2350 so the debug-mode assertion no longer fires for the now-valid case. Verified by host-side test harness, dumping the constant-folded encoding emitted by each pio_encode_* on both PIO versions: test RP2350 (v1) RP2040 (v0) mov pindirs,x 0xa061 (fixed) 0xa081 (unchanged) mov exec_mov,x 0xa081 0xa081 (no regression) mov pc,x 0xa0a1 0xa0a1 mov y,y (NOP) 0xa042 0xa042 set pindirs,1 0xe081 0xe081 (SET uses 4) out pindirs,1 0x6081 0x6081 (OUT uses 4) in pins,1 0x4001 0x4001 mov_not pindirs,x 0xa069 0xa089 mov_reverse pindirs,x 0xa071 0xa091 Full SDK build clean for both PICO_PLATFORM=rp2040 and PICO_PLATFORM=rp2350. Fixes raspberrypi#2754.
anoshyn
force-pushed
the
fix-pio-mov-pindirs
branch
from
a552d06 to
6a990bb
Compare
Author
|
Force-pushed to drop the |
Contributor
|
closed in favor of #2958 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2754. On RP2350,
pio_encode_mov(pio_pindirs, pio_x)returned0xA081(which isMOV EXEC, X) instead of the correct0xA061(MOV PINDIRS, X).The PIO MOV destination encoding for PINDIRS differs from the SET/IN/OUT encoding:
The
pio_src_destenum collapses bothpio_pindirsandpio_exec_movto low-3-bits = 4 because the SET/IN/OUT encoder is the original consumer. The MOV encoder needs to remap PINDIRS to 3 — but only for PINDIRS, not for EXEC. In release builds (NDEBUG),pio_pindirsandpio_exec_movare numerically identical (the_PIO_INVALID_*flag bits are all zero), so a naive "dest == 4 → 3" remap would also breakpio_encode_mov(pio_exec_mov, ...).This PR adds a non-
NDEBUG-gated marker bit (_PIO_MARKER_PINDIRS = 0x100) carried only bypio_pindirs. The new_pio_mov_dest_bits()helper checks the marker onPICO_PIO_VERSION > 0and remaps to 3. All existing encoders already mask with& 7ubefore emitting the destination field, so the marker bit is discarded everywhere it isn't being checked — no other encoder is affected. The debug-mode_PIO_INVALID_MOV_DESTassertion is also dropped frompio_pindirson RP2350.Test plan
Host-side verification of the constant-folded encoding emitted by each
pio_encode_*:mov pindirs, xmov exec_mov, xmov pc, xmov y, y(NOP)set pindirs, 1out pindirs, 1in pins, 1mov_not pindirs, xmov_reverse pindirs, xPICO_PLATFORM=rp2040 PICO_BOARD=pico.PICO_PLATFORM=rp2350 PICO_BOARD=pico2.