Add missing function pins for RP2350B (QFN-80)

[cantudo]

Hello, I am working on a project with a RP2350B microcontroller and I noticed that the extra pins that come with this version (the one with the QFN-80 package) do not have the function traits implemented. I think the gpio functionality is working, but they need to be marked as valid for functions such as uart, spi, etc.

In this PR I have not added all pins and function traits, I just made it as a test for GPIO32/GPIO33 uart. Is this the correct way of implementing this? Should I add the rest of the pins? Thank you.

[thejpster]

Yes, that looks right on brief inspection.

I wonder what happens if you try and use a B-only GPIO on an RP2350A.

[cantudo]

My guess is that it does not happen anything other than some register changing value, if you look at the datasheet (page 601) it seems that the registers are the same for both. Maybe on the RP2350A it is not connected internally or something. But i might be completely wrong I do not know much about this.

Should I complete the rest of the pins for this PR?

[thejpster]

Should I complete the rest of the pins for this PR?

If you can, that would be great.

[cantudo]

Hey, I implemented the rest of the function pins missing, let me know what you think 😊 !

[thejpster]

Can you rebase on main to clear out all the unrelated commits?

[cantudo]

Yeah, sorry about that, i just rebased it.

[ithinuel]

Thank you for this PR !
The changes in the various peripherals look alright to me (didn't get to test them).

One thing I'd like that we make sure though is that the GPIO impl works as expected.
Eg i believe that because those pins are above 31, they wouldn't behave as intended when added to a pin group.

[cantudo]

Hm yeah, you are right sio::Sio::read_bank0() returns a u32, so no way you can actually read all of the pins. I am a bit lost in how to handle this. It seems that read_bank0() calls a bits() method whose return type is REG::Ux.

macro_rules! raw_reg {
    ($ U : ty , $ size : literal , $ mask : ident) => {
        impl RawReg for $U {
            #[inline(always)]
            fn mask<const WI: u8>() -> Self {
                $mask::<WI>()
            }
            #[inline(always)]
            fn one() -> Self {
                1
            }
        }
        const fn $mask<const WI: u8>() -> $U {
            <$U>::MAX >> ($size - WI)
        }
        impl FieldSpec for $U {
            type Ux = $U;
        }
    };
}
raw_reg!(u8, 8, mask_u8);
raw_reg!(u16, 16, mask_u16);
raw_reg!(u32, 32, mask_u32);
raw_reg!(u64, 64, mask_u64);
#[doc = " Raw register type"]
pub trait RegisterSpec {
    #[doc = " Raw register type (`u8`, `u16`, `u32`, ...)."]
    type Ux: RawReg;
}

Given that code, maybe we should change the return type of read_bank0()? If it returned a u64, it would use the 64 bit RawReg impl. I do not know how this translates to hardware though >.< Thank you for your feedback.

[thelenlucas]

It appears that the register pair (GPIO_HIGH_IN and GPIO_HIGH_OUT) control the functionality of the extra QFN-80 package pins. It looks like these were also used in the RP2040, but had most of the pins reserved out, so my instinct is that they're left unused, but should be connected to the USB pins, because it seems like the same register is used for those pins.

Probably best to stick with u32's if at all possible.

[cantudo]

Oh, I see, thank you. So maybe we could do something like:

/// Reads the whole bank0 at once.
pub fn read_bank0() -> u64 {
    unsafe {
        let lo: u32 = (*pac::SIO::ptr()).gpio_in().read().bits();
        let hi: u32 = (*pac::SIO::ptr()).gpio_hi_in().read().bits();
        (lo as u64) | ((hi as u64) << 32)
    }
}

For read_bank0() in PinGroup??

[thejpster]

I think you'd need a comment so say it is done as two reads, and you might want to disable interrupts to avoid the two reads being spread apart in time.

[cantudo]

Thank you for the feedback, I am trying to implement this, but I hit a problem and I am stuck trying to figure this out.

So I changed the whole read_bank0() function to this:

    /// Reads the whole bank0 at once.
    /// Two registers are read, `gpio_in` and `gpio_hi_in`, which are combined into a single 64-bit value.
    /// `gpio_hi_in` is only relevant for the QFN-80 package, so the upper 32 bits should be ignored for
    /// the QFN-48 package.
    pub fn read_bank0() -> u64 {
        unsafe {
            // Disable interrupts to avoid time disparity between the two reads
            let interrupts_active = crate::arch::interrupts_enabled();
            crate::arch::interrupt_disable();
            
            let lo: u32 = (*pac::SIO::ptr()).gpio_in().read().bits();
            let hi: u32 = (*pac::SIO::ptr()).gpio_hi_in().read().bits();

            if interrupts_active {
                crate::arch::interrupt_enable();
            }
            
            ((hi as u64) << 32) | (lo as u64)
        }
    }

But we also need to change the set functions for changing the output pins value. Okey, so the current implementation is as follows:

/// Write this set of pins all at the same time.
    ///
    /// This only affects output pins. Input pins in the
    /// set are ignored.
    pub fn set(&mut self, state: PinState) {
        use super::pin::pin_sealed::PinIdOps;
        let mask = self.0.write_mask();
        let head_id = self.0.head.borrow().id();
        if state == PinState::Low {
            head_id.sio_out_clr().write(|w: &mut W<GPIO_OUT_CLR_SPEC>| unsafe { w.bits(mask) });
        } else {
            head_id.sio_out_set().write(|w: &mut W<GPIO_OUT_SET_SPEC>| unsafe { w.bits(mask) });
        }
    }

I want to change it by creating two u32 variables for the low and high mask. We could then write the mask to each of the registers. The problem is that the sio_out_clr() (and all of the related, sio_out(), sio_out_set(), etc.) all return &pac::sio::GPIO_OUT_CLR or equivalent, so that they can only affect pins in the lower part, pins 0-31. The trait PinIdOps, where sio_out_clr() and such are defined, has a is_hi() method. This is a bit odd since I imagine that if the pin matches the is_hi() then acting on the sio_out_clr() method would clear the other pins values (the ones from 0-31) regardless. I think this should be changed, but I am not sure how.

What is your opinion on this? I thought this was going to be a quick and easy PR ngl >.<