GPIO driver implementation does not use conform to BCM2711 (RPi4)

[abdes]

According to https://github.com/RPi-Distro/raspi-gpio/blob/master/raspi-gpio.c#L194 and to BC2711 ARM Peripherals, there is no more a GPPUD0 register. Instead GPPUPPDN0 is the register to use. Additionally, the procedure for pull-up/pull-down activation does not use the GPPUDCLK0 for BCM2711.

    if (is_2711)
    {
        int pullreg = GPPUPPDN0 + (gpio>>4);
        int pullshift = (gpio & 0xf) << 1;
        unsigned int pullbits;
        unsigned int pull;

        switch (type)
        {
        case PULL_NONE:
            pull = 0;
            break;
        case PULL_UP:
            pull = 1;
            break;
        case PULL_DOWN:
            pull = 2;
            break;
        default:
            return 1; /* An illegal value */
        }

        pullbits = *(gpio_base + pullreg);
        pullbits &= ~(3 << pullshift);
        pullbits |= (pull << pullshift);
        *(gpio_base + pullreg) = pullbits;
    }
    else
    {
        int clkreg = GPPUDCLK0 + (gpio>>5);
        int clkbit = 1 << (gpio & 0x1f);

        *(gpio_base + GPPUD) = type;
        delay_us(10);
        *(gpio_base + clkreg) = clkbit;
        delay_us(10);
        *(gpio_base + GPPUD) = 0;
        delay_us(10);
        *(gpio_base + clkreg) = 0;
        delay_us(10);
    }

Could this be behind some of the glitches we randomly see when using the chainloader?

I have made changes as per the below diffs and tested several times on Pi 4 with no issues when plugging/unplugging the USB serial cable. I am not sure though if I should enable pup/pdn for the TX or RX when using a serial to USB cable. Maybe you could enlighten me there.

diff --git a/07_uart_chainloader/src/_arch/aarch64/cpu.rs b/07_uart_chainloader/src/_arch/aarch64/cpu.rs
index 34cf2ab..b1518d5 100644
--- a/07_uart_chainloader/src/_arch/aarch64/cpu.rs
+++ b/07_uart_chainloader/src/_arch/aarch64/cpu.rs
@@ -39,14 +39,6 @@ pub unsafe fn _start() -> ! {
 
 pub use asm::nop;
 
-/// Spin for `n` cycles.
-#[inline(always)]
-pub fn spin_for_cycles(n: usize) {
-    for _ in 0..n {
-        asm::nop();
-    }
-}
-
 /// Pause execution on the core.
 #[inline(always)]
 pub fn wait_forever() -> ! {
diff --git a/07_uart_chainloader/src/bsp/device_driver/bcm/bcm2xxx_gpio.rs b/07_uart_chainloader/src/bsp/device_driver/bcm/bcm2xxx_gpio.rs
index 0b01db4..85ee230 100644
--- a/07_uart_chainloader/src/bsp/device_driver/bcm/bcm2xxx_gpio.rs
+++ b/07_uart_chainloader/src/bsp/device_driver/bcm/bcm2xxx_gpio.rs
@@ -5,7 +5,7 @@
 //! GPIO Driver.
 
 use crate::{
-    bsp::device_driver::common::MMIODerefWrapper, cpu, driver, synchronization,
+    bsp::device_driver::common::MMIODerefWrapper, driver, synchronization,
     synchronization::NullLock,
 };
 use register::{mmio::*, register_bitfields, register_structs};
@@ -39,18 +39,22 @@ register_bitfields! {
         ]
     ],
 
-    /// GPIO Pull-up/down Clock Register 0
-    GPPUDCLK0 [
-        /// Pin 15
-        PUDCLK15 OFFSET(15) NUMBITS(1) [
-            NoEffect = 0,
-            AssertClock = 1
+    /// GPIO Pull-up/down Register 0
+    GPPUPPDN0 [
+        /// GPIO 15
+        PUPPDN15 OFFSET(30) NUMBITS(2) [
+            NoResistor = 0,
+            PullUp = 1,
+            PullDown = 2,
+            Reserved = 3
         ],
 
-        /// Pin 14
-        PUDCLK14 OFFSET(14) NUMBITS(1) [
-            NoEffect = 0,
-            AssertClock = 1
+        /// GPIO 14
+        PUPPDN14 OFFSET(28) NUMBITS(2) [
+            NoResistor = 0,
+            PullUp = 1,
+            PullDown = 2,
+            Reserved = 3
         ]
     ]
 }
@@ -65,9 +69,7 @@ register_structs! {
         (0x10 => GPFSEL4: ReadWrite<u32>),
         (0x14 => GPFSEL5: ReadWrite<u32>),
         (0x18 => _reserved1),
-        (0x94 => GPPUD: ReadWrite<u32>),
-        (0x98 => GPPUDCLK0: ReadWrite<u32, GPPUDCLK0::Register>),
-        (0x9C => GPPUDCLK1: ReadWrite<u32>),
+        (0xe4 => GPPUPPDN0: ReadWrite<u32, GPPUPPDN0::Register>),
         (0xA0 => @END),
     }
 }
@@ -117,16 +119,10 @@ impl GPIOInner {
             .GPFSEL1
             .modify(GPFSEL1::FSEL14::AltFunc0 + GPFSEL1::FSEL15::AltFunc0);
 
-        // Enable pins 14 and 15.
-        self.registers.GPPUD.set(0);
-        cpu::spin_for_cycles(150);
-
+        // PUPDN to 0 for GPIO14 and GPIO15
         self.registers
-            .GPPUDCLK0
-            .write(GPPUDCLK0::PUDCLK14::AssertClock + GPPUDCLK0::PUDCLK15::AssertClock);
-        cpu::spin_for_cycles(150);
-
-        self.registers.GPPUDCLK0.set(0);
+            .GPPUPPDN0
+            .modify(GPPUPPDN0::PUPPDN14::NoResistor + GPPUPPDN0::PUPPDN15::NoResistor);
     }
 }

[andre-richter]

OMG, a brand new and shiny BCM2711 ARM Peripherals 🤩

I wasn't aware this was released recently. Nor was I aware that there are significant differences between the Pi3/Pi4 GPIO to be honest, because on my Pi4 it just worked out-of-the box, so I never questioned this.

While I cannot dive deep into your findings right now, all of this sounds plausible. I guess it makes sense split bcm2xxx_gpio.rs into Pi3 and Pi4 versions!

[andre-richter]

I'll test it on my side and then add it to the tutorials asap!

[andre-richter]

@abdes

I am not sure though if I should enable pup/pdn for the TX or RX when using a serial to USB cable. Maybe you could enlighten me there.

I would argue that since the USB-serial is already connected when the Pi executes the GPIO init code, NoResistor should be okay, since both the TX and RX pins are actively driven already in both directions (by the Pi and the USB serial).

It might still make sense to configure pull-up resistors for the pins. They would ensure the pin always falls back to the default value 1/high_level of the UART protocol.
I am considering introducing that change. If you want, you can test on your side if it works when you configure pull-up for both.

Best,
Andre

[abdes]

Thank you