It's probably my fault...

emulator · Post by **emulator** » Sun Apr 05, 2026 10:33 am

So the new RP2350 and updated RP2040 ports are taking shape and you've started to use it for your project. Suddenly you find an inconsistency or something that simply doesn't work. Well, it's probably my fault. The work has been a pretty significant set of changes and there are likely some latent bugs in the code. Defect reports are appreciated, small repro examples desired and patches are beloved.

If you find an issue, please check TRUNK as the issue may already have been fixed.
If you have a repro case either post a code block in the forum or link to a GitHub gist (or equivalent).
For patches please post a diff
Code: Select all
```
svn diff
```
Code: Select all
```
git diff
```

xyzzy · Post by **xyzzy** » Mon Apr 06, 2026 8:00 pm

I think this is a better way to handle the RP register port layout. Avoids the extra indirection through a table of pointers.

I left the masking in, but I think it's not necessary. I can't see any reason it's needed, except to deal with code that is trying to randomly write to GPIOs and manages to hit the SPI or USB bits. But such code is buggy and should be fixed. And will still crash if it tries a random GPIO > 63 and goes off the end of the port table. And writing to random valid non-SPI/USB GPIOs can cause all sorts of problems too. So masking the bits doesn't really fix anything.

Besides making GPIO operations more costly, there are other downsides. It makes it impossible to use the USB pins as GPIOs, which isn't common but is a perfectly reasonable thing to do if you know what you are doing. One might say, "it's possible to write to the SIO registers directly and do it that way." But then there's another problem. Any code that does a read-modify-write to change only certain GPIO lines in the high register will also mask off the SPI/USB bits and set them to zero! So even if you set those bits directly via SIO->..., perfectly well written code elsewhere that is trying to NOT clobber bits it doesn't want to modify will still clear them out.

Code: Select all

From 6190bea228f19d09932c52e856da2f8bce19a681 Mon Sep 17 00:00:00 2001
From: Trent Piepho <[email protected]>
Date: Sun, 5 Apr 2026 14:43:58 -0700
Subject: [PATCH] More efficient RP GPIO port code

Instead of going through an extra indirection table to get the correct
register address for the low and hi port, take advantage of the offset
between the low and hi registers being the same for every register.

On the RP2350, every hi register immediately follows the low register.
While on the RP2040, they are offset by eight registers.
---
 os/hal/ports/RP/LLD/GPIOv1/hal_pal_lld.c | 18 ++----------------
 os/hal/ports/RP/LLD/GPIOv1/hal_pal_lld.h | 12 ++++++------
 os/hal/ports/RP/RP2040/rp_pal_lld.h      | 10 ++++++++++
 os/hal/ports/RP/RP2350/rp_pal_lld.h      | 10 ++++++++++
 4 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/os/hal/ports/RP/LLD/GPIOv1/hal_pal_lld.c b/os/hal/ports/RP/LLD/GPIOv1/hal_pal_lld.c
index 7741a0e8f3..8d0a14f2dd 100644
--- a/os/hal/ports/RP/LLD/GPIOv1/hal_pal_lld.c
+++ b/os/hal/ports/RP/LLD/GPIOv1/hal_pal_lld.c
@@ -52,24 +52,10 @@
 
 const rp_pal_port_t _pal_ports[RP_PAL_IOPORTS_COUNT] = {
   {
-    .in        = &SIO->GPIO_IN,
-    .out       = &SIO->GPIO_OUT,
-    .out_set   = &SIO->GPIO_OUT_SET,
-    .out_clr   = &SIO->GPIO_OUT_CLR,
-    .out_xor   = &SIO->GPIO_OUT_XOR,
-    .oe_set    = &SIO->GPIO_OE_SET,
-    .oe_clr    = &SIO->GPIO_OE_CLR,
     .valid_mask = RP_PAL_PORT0_VALID_MASK,
   },
 #if RP_GPIO_NUM_LINES > 32U
   {
-    .in        = &SIO->GPIO_HI_IN,
-    .out       = &SIO->GPIO_HI_OUT,
-    .out_set   = &SIO->GPIO_HI_OUT_SET,
-    .out_clr   = &SIO->GPIO_HI_OUT_CLR,
-    .out_xor   = &SIO->GPIO_HI_OUT_XOR,
-    .oe_set    = &SIO->GPIO_HI_OE_SET,
-    .oe_clr    = &SIO->GPIO_HI_OE_CLR,
     .valid_mask = RP_PAL_PORT1_VALID_MASK,
   },
 #endif
@@ -119,13 +105,13 @@ static void rp_pal_pad_set_mode(ioportid_t port,
   }
 
   /* Release the output driver while reprogramming mux and pad control. */
-  *rpp->oe_clr = bit;
+  _SIO_REG_PORT(GPIO_OE_CLR, port) = bit;
 
   IO_BANK0->GPIO[abspad].CTRL = ctrlbits;
   PADS_BANK0->GPIO[abspad] = padbits;
 
   if (oebits != 0U) {
-    *rpp->oe_set = bit;
+    _SIO_REG_PORT(GPIO_OE_SET, port) = bit;
   }
 }
 
diff --git a/os/hal/ports/RP/LLD/GPIOv1/hal_pal_lld.h b/os/hal/ports/RP/LLD/GPIOv1/hal_pal_lld.h
index 47350faea3..70ece86a10 100644
--- a/os/hal/ports/RP/LLD/GPIOv1/hal_pal_lld.h
+++ b/os/hal/ports/RP/LLD/GPIOv1/hal_pal_lld.h
@@ -329,7 +329,7 @@ extern const rp_pal_port_t _pal_ports[RP_PAL_IOPORTS_COUNT];
  * @notapi
  */
 #define pal_lld_readport(port)                                              \
-  ((ioportmask_t)(*_pal_ports[(uint32_t)(port)].in) &                      \
+  (_SIO_REG_PORT(GPIO_IN, (port)) &                                         \
    (ioportmask_t)(_pal_ports[(uint32_t)(port)].valid_mask))
 
 /**
@@ -343,7 +343,7 @@ extern const rp_pal_port_t _pal_ports[RP_PAL_IOPORTS_COUNT];
  * @notapi
  */
 #define pal_lld_readlatch(port)                                             \
-  ((ioportmask_t)(*_pal_ports[(uint32_t)(port)].out) &                     \
+  (_SIO_REG_PORT(GPIO_OUT, (port)) &                                        \
    (ioportmask_t)(_pal_ports[(uint32_t)(port)].valid_mask))
 
 /**
@@ -363,7 +363,7 @@ extern const rp_pal_port_t _pal_ports[RP_PAL_IOPORTS_COUNT];
  */
 #define pal_lld_writeport(port, bits)                                       \
   do {                                                                      \
-    *_pal_ports[(uint32_t)(port)].out =                                     \
+    _SIO_REG_PORT(GPIO_OUT, (port)) =                                       \
       (uint32_t)(bits) & _pal_ports[(uint32_t)(port)].valid_mask;           \
   } while (false)
 
@@ -380,7 +380,7 @@ extern const rp_pal_port_t _pal_ports[RP_PAL_IOPORTS_COUNT];
  */
 #define pal_lld_setport(port, bits)                                         \
   do {                                                                      \
-    *_pal_ports[(uint32_t)(port)].out_set =                                 \
+    _SIO_REG_PORT(GPIO_OUT_SET, (port)) =                                   \
       (uint32_t)(bits) & _pal_ports[(uint32_t)(port)].valid_mask;           \
   } while (false)
 
@@ -397,7 +397,7 @@ extern const rp_pal_port_t _pal_ports[RP_PAL_IOPORTS_COUNT];
  */
 #define pal_lld_clearport(port, bits)                                       \
   do {                                                                      \
-    *_pal_ports[(uint32_t)(port)].out_clr =                                 \
+    _SIO_REG_PORT(GPIO_OUT_CLR, (port)) =                                   \
       (uint32_t)(bits) & _pal_ports[(uint32_t)(port)].valid_mask;           \
   } while (false)
 
@@ -414,7 +414,7 @@ extern const rp_pal_port_t _pal_ports[RP_PAL_IOPORTS_COUNT];
  */
 #define pal_lld_toggleport(port, bits)                                      \
   do {                                                                      \
-    *_pal_ports[(uint32_t)(port)].out_xor =                                 \
+    _SIO_REG_PORT(GPIO_OUT_XOR, (port)) =                                   \
       (uint32_t)(bits) & _pal_ports[(uint32_t)(port)].valid_mask;           \
   } while (false)
 
diff --git a/os/hal/ports/RP/RP2040/rp_pal_lld.h b/os/hal/ports/RP/RP2040/rp_pal_lld.h
index 3001f402ee..3d605e9e7f 100644
--- a/os/hal/ports/RP/RP2040/rp_pal_lld.h
+++ b/os/hal/ports/RP/RP2040/rp_pal_lld.h
@@ -37,4 +37,14 @@
                                              PAL_RP_PAD_SCHMITT)
 /** @} */
 
+/**
+ * @brief Access a SIO GPIO register by port.
+ * @details Given a register name (field of SIO) and port (0, 1), produce a
+ *          reference to the correct GPIO register for the port.  On the RP2040
+ *          every "hi" register is eight registers after the "low" register.
+ *
+ * @notapi
+ */
+#define _SIO_REG_PORT(reg, port) *(&SIO->reg + ((port) ? 8 : 0))
+
 #endif /* RP2040_RP_PAL_LLD_H */
diff --git a/os/hal/ports/RP/RP2350/rp_pal_lld.h b/os/hal/ports/RP/RP2350/rp_pal_lld.h
index 9c6d42b3bf..8ce27bc5af 100644
--- a/os/hal/ports/RP/RP2350/rp_pal_lld.h
+++ b/os/hal/ports/RP/RP2350/rp_pal_lld.h
@@ -43,4 +43,14 @@
                                              PAL_RP_PAD_SCHMITT)
 /** @} */
 
+/**
+ * @brief Access a SIO GPIO register by port.
+ * @details Given a register name (field of SIO) and port (0, 1), produce a
+ *          reference to the correct GPIO register for the port.  On the RP2350
+ *          every "hi" register is immediately after the "low" register.
+ *
+ * @notapi
+ */
+#define _SIO_REG_PORT(reg, port) *(&SIO->reg + (port))
+
 #endif /* RP2350_RP_PAL_LLD_H */
-- 
2.49.0

emulator · Post by **emulator** » Thu Apr 09, 2026 1:26 pm

I will take a look tomorrow, goal for this weekend is to make a refactor of both PAL and PIO LLDs.

- E

emulator · Post by **emulator** » Sat Apr 11, 2026 5:27 pm

@xyzzy I applied a modified version of your change today. The mask is used only for an osalDebug so for non-debug builds no overhead. I might just remove it completely though as I tend to agree with your point that we should just always trust the engineer knows what they are doing. PTAL and let me know if you have any feedback, thank you!

xyzzy · Post by **xyzzy** » Mon Apr 20, 2026 7:50 am

Noticed this while trying to find out why the qmk delay function is off by a factor of 150. Which happens also be the ratio between the supposed tick frequency of 1 MHz and the CPU clock at 150 MHz.

Code: Select all

From b890236f821fde1643cbcd3102267adaa7b5906f Mon Sep 17 00:00:00 2001
From: Trent Piepho <[email protected]>
Date: Sun, 19 Apr 2026 23:43:33 -0700
Subject: [PATCH 1/1] Stop RP tick generator timer before changing cycle count
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

From the RM §8.5.1, "Before changing the cycle count, always stop the tick
generator with the TIMER0_CTRL.ENABLE bit.  You can re-enable once the tick
generator is configured."
---
 os/hal/ports/RP/RP2350/rp_clocks.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/os/hal/ports/RP/RP2350/rp_clocks.c b/os/hal/ports/RP/RP2350/rp_clocks.c
index 524c45340e..679f1b0664 100644
--- a/os/hal/ports/RP/RP2350/rp_clocks.c
+++ b/os/hal/ports/RP/RP2350/rp_clocks.c
@@ -144,6 +144,8 @@ void rp_clock_init(void) {
   /* Calculate cycles for 1us tick based on clk_ref frequency. */
   uint32_t cycles = RP_XOSCCLK / 1000000U;
 
+  /* Disable running timer before changing cycle count */
+  TICKS->TICK[TICKS_TIMER0].CTRL = 0;
   /* Start tick generators */
   for (uint32_t i = 0U; i < 6U; i++) {
     TICKS->TICK[i].CYCLES = cycles;
-- 
2.49.0

ChibiOS Free Embedded RTOS

It's probably my fault...

It's probably my fault...

Re: It's probably my fault...

Re: It's probably my fault...

Re: It's probably my fault...

Re: It's probably my fault...