Properly map IO and PAD register offsets for QSPI

[lifeisafractal]

In the UART bootloader example the QSPI pins are used for normal IO and a qspi_gpio_set_function() function sets the pins' IO and PAD registers for use. This function didn't take into account that the IO and PAD register banks for the QSPI IOs are not in the same order. This PR corrects this by adding a lookup table to map between the two and takes an enum as the gpio parameter to signal to the caller the expected order for the function call.

It is worth noting that this example did happen to work for UART TX only due to happenstance. It configured offsets 3 and 4 (which were correct offsets for the PAD bank for SD2 and SD3), but these offsets map to SD1 and SD2 in the IO bank. This results in SD2 getting fully configured which provides UART0 TX, UART0 RX is left mis-configured and non-functional (only the PAD is configured), and SD1 gets its IO register erroneously configured. This example never made use of RX which is likely why this got missed since TX worked.

Even though this example happens to work, it's wroth correcting this function for posterity. Since the pico-sdk doesn't have functions for configuring the QSPI IOs, I found this example and assumed I could safely borrow this function, but the fact that it is incorrect lead to me burning several hours before I figured out what was going on. It's also likely worth getting the pin config for QSPI into the pico-sdk itself as a first class citizen, but for now it's worth fixing this example.

Also, if anyone has info on why the QSPI PAD and IO register offsets don't match, I'd love to know just out of curiosity. It's also curious that the QSPI IO bank has entries for the USB pins (DP and DM) at the start, but they don't have corresponding entries in PAD. You'd think those would have been put at the end of the bank. This is a non-issue code wise as the DP/DM are hard coded in the QSPI IO register map struct, and the gpio[] array for the actual QSPI pins does start at the base of the QSPI pins. It's all just odd...

Unrelated, this PR also adds static to local functions used in the UART example as a best practice.

[github-actions]

Please do not submit against master, use develop instead

[lurch]

Are there any useful defines from https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2350/hardware_regs/include/hardware/regs/io_qspi.h or https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2350/hardware_regs/include/hardware/regs/pads_qspi.h that can be used here?

Also, there might be useful info in raspberrypi/pico-sdk#1799 and/or some of the other issues that it links to.

[lifeisafractal]

I looked at io_qspi.h and pads_qspi.h and they do have IO_QSPI_GPIO_QSPI_*_CTRL_OFFSET and PADS_QSPI_GPIO_QSPI_*_OFFSET, but these are relative to the base of the peripheral where as the pads_qspi_hw and io_qspi_hw register map structs have the gpio array within the that are each relative to the base of the GPIOs within that bank (also, the IO bank has 2x32-bit registers per GPIO). My opinion is it'd end up being more confusing to try utilize any defines from these files but I'm open to a suggestion. FWIW, I'm referencing gpio_set_function(...) which uses the similar IO/PAD reg map structs for the normal GPIO bank to setup the pins.

I had come across raspberrypi/pico-sdk#1799 before as I have a product in the works that use a QSPI-less RP2350 as a co-processor using the the UART bootloader. I went through the linked issues and PRs and I don't think there is too much new in any of those beyond some reinforcing discussion that this would be a nice-to-have in the SDK. I think the short of the long is that someone (maybe me?) just needs to go take a stab at adding first-class support for the QSPI io bank to the pico-sdk.

Independent of doing that (which is a bigger lift), I think it's worth getting this fix merged here still. This example seems to be where people trying to do QSPI IO on the RP2350 end up after some googling, and as it stands now this code is broken for general use cases which doesn't feel great.

[lurch]

Independent of doing that (which is a bigger lift), I think it's worth getting this fix merged here still.

I totally agree! Sorry if I gave the opposite impression. (I was just wondering if there was an "easy" way to avoid having two "lookup tables" hard-coded inside this example, but you've obviously already looked at this in more detail than me 🙂 )

ping @will-v-pi (who wrote this uart-bootloader example).

EDIT: Also ping @matiasilva in case there's any "fiddling" that needs to be done in the auto-generation of the io_qspi_hw and pads_qspi_hw structs that would make this any "easier" to solve in the long-term. EDIT2: At the very least, perhaps hardware/structs/io_qspi.h#L196 and hardware/structs/pads_qspi.h#L42 should have a comment listing what order the GPIOs are in within each io[6] array?

[will-v-pi]

It's also curious that the QSPI IO bank has entries for the USB pins (DP and DM) at the start, but they don't have corresponding entries in PAD

This is because the USB PHY is directly connected to the pins, there is no PADs block in the hardware

[lifeisafractal]

It's also curious that the QSPI IO bank has entries for the USB pins (DP and DM) at the start, but they don't have corresponding entries in PAD

This is because the USB PHY is directly connected to the pins, there is no PADs block in the hardware

That part makes sense to me, although I still don't get why they didn't tack them on the end rather than the beginning, but I guess that's not a huge deal. The fact that the actual QSPI pins are in different orders for the PAD and IO banks is the part that really confuses me 🤔 .

[lifeisafractal]

Independent of doing that (which is a bigger lift), I think it's worth getting this fix merged here still.

I totally agree! Sorry if I gave the opposite impression. (I was just wondering if there was an "easy" way to avoid having two "lookup tables" hard-coded inside this example, but you've obviously already looked at this in more detail than me 🙂 )

ping @will-v-pi (who wrote this uart-bootloader example).

EDIT: Also ping @matiasilva in case there's any "fiddling" that needs to be done in the auto-generation of the io_qspi_hw and pads_qspi_hw structs that would make this any "easier" to solve in the long-term. EDIT2: At the very least, perhaps hardware/structs/io_qspi.h#L196 and hardware/structs/pads_qspi.h#L42 should have a comment listing what order the GPIOs are in within each io[6] array?

No prob, and all good! I agree I'm not 100% sure the best way to tackle this on the SDK side, but I think the current solution here is solid enough and won't lead people astray if they steal it for QSPI IO usage which isn't in the SDK currently.

I'm going to drop a comment on raspberrypi/pico-sdk#1799 referencing this PR and throw in some thoughts on what that API should look like given the confusing orders of the QSPI IO and PAD register banks.

[lurch]

So to summarise, the issue here is that the QSPI IO registers are in this order:

$ grep "IO_QSPI_GPIO_.*_CTRL_OFFSET" src/rp2350/hardware_regs/include/hardware/regs/io_qspi.h
#define IO_QSPI_GPIO_QSPI_SCLK_CTRL_OFFSET _u(0x00000014)
#define IO_QSPI_GPIO_QSPI_SS_CTRL_OFFSET _u(0x0000001c)
#define IO_QSPI_GPIO_QSPI_SD0_CTRL_OFFSET _u(0x00000024)
#define IO_QSPI_GPIO_QSPI_SD1_CTRL_OFFSET _u(0x0000002c)
#define IO_QSPI_GPIO_QSPI_SD2_CTRL_OFFSET _u(0x00000034)
#define IO_QSPI_GPIO_QSPI_SD3_CTRL_OFFSET _u(0x0000003c)

(i.e. SCLK, SS, SD0, SD1, SD2, SD3), whereas the QSPI PADS registers are in this order:

$ grep "PADS_QSPI_GPIO_.*_OFFSET" src/rp2350/hardware_regs/include/hardware/regs/pads_qspi.h
#define PADS_QSPI_GPIO_QSPI_SCLK_OFFSET _u(0x00000004)
#define PADS_QSPI_GPIO_QSPI_SD0_OFFSET _u(0x00000008)
#define PADS_QSPI_GPIO_QSPI_SD1_OFFSET _u(0x0000000c)
#define PADS_QSPI_GPIO_QSPI_SD2_OFFSET _u(0x00000010)
#define PADS_QSPI_GPIO_QSPI_SD3_OFFSET _u(0x00000014)
#define PADS_QSPI_GPIO_QSPI_SS_OFFSET _u(0x00000018)

(i.e. SCLK, SD0, SD1, SD2, SD3, SS).

I asked our hardware engineers about this, and they pointed me at our internal ticket discussing it (i.e. this is a known issue). I'm not sure how much of that ticket I can discuss publicly, but something that it mentions is that the QSPI ACCESSCTRL bits:

$ grep "ACCESSCTRL_GPIO_NSMASK1_QSPI_.*SB" src/rp2350/hardware_regs/include/hardware/regs/accessctrl.h 
#define ACCESSCTRL_GPIO_NSMASK1_QSPI_SD_MSB    _u(31)
#define ACCESSCTRL_GPIO_NSMASK1_QSPI_SD_LSB    _u(28)
#define ACCESSCTRL_GPIO_NSMASK1_QSPI_CSN_MSB    _u(27)
#define ACCESSCTRL_GPIO_NSMASK1_QSPI_CSN_LSB    _u(27)
#define ACCESSCTRL_GPIO_NSMASK1_QSPI_SCK_MSB    _u(26)
#define ACCESSCTRL_GPIO_NSMASK1_QSPI_SCK_LSB    _u(26)

(i.e. SCK, CSN, SD0, SD1, SD2, SD3), and the QSPI SIO bits:

$ grep "SIO_GPIO_HI_IN_QSPI_.*SB" src/rp2350/hardware_regs/include/hardware/regs/sio.h 
#define SIO_GPIO_HI_IN_QSPI_SD_MSB    _u(31)
#define SIO_GPIO_HI_IN_QSPI_SD_LSB    _u(28)
#define SIO_GPIO_HI_IN_QSPI_CSN_MSB    _u(27)
#define SIO_GPIO_HI_IN_QSPI_CSN_LSB    _u(27)
#define SIO_GPIO_HI_IN_QSPI_SCK_MSB    _u(26)
#define SIO_GPIO_HI_IN_QSPI_SCK_LSB    _u(26)

(i.e. SCK, CSN, SD0, SD1, SD2, SD3) are effectively in the same order as the QSPI IO registers.

[lurch]

To expand on my previous comment even further: I also noticed that the example-code in raspberrypi/pico-sdk#2417 (comment) defines:

// The SDK sources do not currently enumerate these very high order GPIOs
#define GPIO58_QSPI_SCLK                58
#define GPIO59_QSPI_SS                  59
#define GPIO60_QSPI_SD0                 60
#define GPIO61_QSPI_SD1                 61
#define GPIO62_QSPI_SD2                 62
#define GPIO63_QSPI_SD3                 63

So out of curiosity I asked @andygpz11 where these "GPIO numbers" came from, and after a bit of rummaging he found them:

$ grep "IO_QSPI_.*_CTRL_FUNCSEL_VALUE_SIOB_PROC" src/rp2350/hardware_regs/include/hardware/regs/io_qspi.h
#define IO_QSPI_USBPHY_DP_CTRL_FUNCSEL_VALUE_SIOB_PROC_56 _u(0x05)
#define IO_QSPI_USBPHY_DM_CTRL_FUNCSEL_VALUE_SIOB_PROC_57 _u(0x05)
#define IO_QSPI_GPIO_QSPI_SCLK_CTRL_FUNCSEL_VALUE_SIOB_PROC_58 _u(0x05)
#define IO_QSPI_GPIO_QSPI_SS_CTRL_FUNCSEL_VALUE_SIOB_PROC_59 _u(0x05)
#define IO_QSPI_GPIO_QSPI_SD0_CTRL_FUNCSEL_VALUE_SIOB_PROC_60 _u(0x05)
#define IO_QSPI_GPIO_QSPI_SD1_CTRL_FUNCSEL_VALUE_SIOB_PROC_61 _u(0x05)
#define IO_QSPI_GPIO_QSPI_SD2_CTRL_FUNCSEL_VALUE_SIOB_PROC_62 _u(0x05)
#define IO_QSPI_GPIO_QSPI_SD3_CTRL_FUNCSEL_VALUE_SIOB_PROC_63 _u(0x05)