Repository navigation
Conversation
8b872c3 to
3848965
Compare
|
Would it be reasonable to include a new I feel a readily available, generic B variant would make it much easier to test for issues/regressions in this bringup. While I say this with two B-variant boards on my desk that could be added here, a vendor custom board might introduce unknowns. There is not, and (afaik) will never be, a B-variant RPI_PICO2 so it might be the wrongest place to hang this, but it's very tidy 😆 |
|
Code size report: |
|
I've also completely overlooked RP2350's Alt Function 0 here, which is used for HSTX. Right now |
RP2350 builds were using the incomplete rp2_af.csv alt function table
which broke pins > 31 in RP2350B (48-pin QFN-80) builds:
>>> machine.Pin(31)
Pin(GPIO31, mode=IN, pull=PULL_DOWN)
>>> machine.Pin(32)
Pin(GPIO32, mode=ALT, pull=PULL_DOWN, alt=31)
boards/*._af.csv: Add separate alt-functions tables for RP2040,
RP2350 and RP2350B.
CMakeLists.txt: Pick correct _af.csv to pass into make-pins.py.
boards/make-pins.py: handle CORESIGHT and XIP_CS1 alt functions.
machine_pin.c: Add PIO2, CORESIGHT and XIP_CS1 alt functions.
Signed-off-by: Phil Howard <[email protected]>
3848965 to
25c09fa
Compare
|
Actually running the test build on an RP2350B doesn't seem to fix the problem I was seeing, but rather makes it worse: Looks like all pins now default to alt=31 which is Edit: Okay I have fired up vanilla MicroPython on an o.g. Pico and, indeed, |
dpgeorge
left a comment
There was a problem hiding this comment.
@Gadgetoid thanks for this PR. Apart from the minor comments it looks all correct to me, it seems quite an "obvious" fix.
I checked RPI_PICO firmware and it's unchanged with this PR (binary equivalent except for the embedded git hash). So that's good, nothing to worry about with the RP2040.
For RP2350 based boards, it looks correct (as I say above). Any strangeness you see with default alt=31 doesn't look related to this PR, and we should tackle that separately (if it needs tackling at all).
| else() | ||
| set(GEN_PINS_AF_CSV "${MICROPY_PORT_DIR}/boards/rp2350_af.csv") | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
Maybe the default PICO_NUM_GPIOS bits of code below should come before this section, just to be clear what the default is? Ie:
if(NOT PICO_NUM_GPIOS)
set(PICO_NUM_GPIOS 30)
endif()
if(NOT PICO_NUM_EXT_GPIOS)
set(PICO_NUM_EXT_GPIOS 10)
endif()
if(PICO_RP2040)
set(GEN_PINS_AF_CSV "${MICROPY_PORT_DIR}/boards/rp2040_af.csv")
elseif(PICO_RP2350)
if(PICO_NUM_GPIOS EQUAL 48)
set(GEN_PINS_AF_CSV "${MICROPY_PORT_DIR}/boards/rp2350b_af.csv")
else()
set(GEN_PINS_AF_CSV "${MICROPY_PORT_DIR}/boards/rp2350_af.csv")
endif()
endif()
| af_unit = int(m.group(3)) if m.group(3) is not None else 0 | ||
| if af_idx == 10: | ||
| # AF11 uses UART_AUX in lieu of UART | ||
| af_fn = "UART_AUX" |
There was a problem hiding this comment.
Instead of this special case, could just name the pins in the af.csv file like UART_AUX0_TX etc. That won't match the datasheet exactly but could be a little clearer? Not sure.
There was a problem hiding this comment.
I left this as-is, it was too tricky to change the af.csv file due to the way the regex works here.
Could possibly be cleaned up in the future.
| af_fn = "UART_AUX" | ||
| if af_fn.startswith("QMI"): | ||
| # The full func name is GPIO_FUNC_XIP_CS1 | ||
| af_fn = "XIP_CS1" |
There was a problem hiding this comment.
As above, could rename the pins in the af.csv files so this bit isn't needed.
| af_fn = "XIP_CS1" | ||
| if af_fn.startswith("TRACE"): | ||
| # The various TRACE functions all belong under CORESIGHT_TRACE | ||
| af_fn = "CORESIGHT_TRACE" |
| { MP_ROM_QSTR(MP_QSTR_ALT_USB), MP_ROM_INT(GPIO_FUNC_USB) }, | ||
| #if PICO_RP2350 | ||
| { MP_ROM_QSTR(MP_QSTR_ALT_XIP_CS1), MP_ROM_INT(GPIO_FUNC_XIP_CS1) }, | ||
| { MP_ROM_QSTR(MP_QSTR_ALT_CORESIGHT), MP_ROM_INT(GPIO_FUNC_CORESIGHT_TRACE) }, |
There was a problem hiding this comment.
Should this be MP_QSTR_ALT_CORESIGHT_TRACE to match both the C-level enum GPIO_FUNC_CORESIGHT_TRACE and the af_fn name?
Looks like there are 3 options for naming this alt func: TRACE, CORESIGHT and CORESIGHT_TRACE. All 3 seem reasonable to me (the C enum is GPIO_FUNC_CORESIGHT_TRACE and the pin alt func names all start with TRACE). But which ever one is chosen, the QSTR here should match it.
There was a problem hiding this comment.
I renamed the QSTR to ALT_CORESIGHT_TRACE.
FYI I'm taking a crack at an implementation for #17111, so will a way to set alt function 0 for HSTX. |
|
Actually, just tinkered around with this on my end. It's fairly trivial to fix the alt functions starting at 1: micropython/ports/rp2/boards/make-pins.py Line 68 in 6773051 Change to Then in the .csv files, add an extra column for Also need to add the micropython/ports/rp2/machine_pin.c Lines 510 to 511 in 6773051 Add |
That would be needed for RP2040 as well. It looks quite straightforward, but I think best for a follow-up PR, so we can get this one through. |
|
@Gadgetoid I'm happy to make the changes I suggested above, so we can get this fix in ASAP for the upcoming 1.27.0 release. |
Please go ahead if you can, I'm buried at the moment! Sorry! I can't remember if I addressed it here, but the alt functions of RP2040 and RP2350 don't quite match up such that a pin configured as CS1 or for coresight trace (alt9 on RP2350, non existent on RP2040) will read as USB_something (alt10 on RP2350, alt9 on RP2040). I noticed this when using the OTP fuses for setting CS1_GPIO. |
|
Merged in 40df953 with minor changes as mentioned above.
Not sure if that's still an issue, but can be addressed later if needed. |
Follow up from micropython#17692. Signed-off-by: Dryw Wade <[email protected]>
|
Thanks @sfe-SparkFro for testing. Those results above look correct to me. |
A follow up to 40df953 / PR #17692, this commit adds the HSTX alternate pin function for GPIO12-19 on the RP2350. Signed-off-by: Dryw Wade <[email protected]>
A follow up to cb2d7a4 / PR micropython#17692, this commit adds the HSTX alternate pin function for GPIO12-19 on the RP2350. Signed-off-by: Dryw Wade <[email protected]>

RP2350 builds were using the incomplete rp2_af.csv alt function table which broke pins > 31 in RP2350B (48-pin QFN-80) builds:
UART_AUX(alt function 11) andPIO2(alt function 8, directly conflicting the RP2040 functionGPIO_FUNC_GPCK) were also missing.Changes
boards/*._af.csv: Add separate alt-functions tables for RP2040,RP2350 and RP2350B.
CMakeLists.txt: Pick correct _af.csv to pass into make-pins.py.boards/make-pins.py: handle CORESIGHT and XIP_CS1 alt functions.machine_pin.c: Add PIO2, CORESIGHT and XIP_CS1 alt functions.Testing
UART_AUX
I tested UART_AUX on an RP2350B board with the following code and a Raspberry Pi Debug Probe:
The pins are correctly configured as
UART_AUX(presumably this always worked?) thanks to the use ofUART_FUNCSEL_NUM, but configuring as UART AUX is now enabled inmachine.Pintoo.To be totally sure this worked, I set up UART 0 on pins 0 and 1, then changed them with
machine.Pin():B-variant Build
I set up a generic B-variant for testing, built against the vanilla
RPI_PICO2board with:And the following config:
ports/rp2/boards/RPI_PICO2/mpconfigvariant_B.cmakeports/rp2/boards/RPI_PICO2/pico2b.h:ports/rp2/boards/RPI_PICO2/pins_b.csv:Trade-offs and Alternatives
I'm not sure we need
CORESIGHTandXIP_CS1alt functions but they are included for completeness. They needed fixups inmake-pins.pybecause it naively chops the first portion of the signal name off and tries to use that as the GPIO function.UART_AUXis more interesting, it gives some more UART options on alt-function 11, but also needs special casing to ensureGPIO_FUNC_UART_AUXis used in place ofGPIO_FUNC_UART. eg:The addition of a
PIO2alt function constant might be a separate and important fix for RP2350 variants in general (and may conflict, I need to check that but I've run out of day). Edit: I checked. I don't think anyone has caught this, though there are other outstanding issues with PIO2.