On Wed, Nov 12, 2025 at 8:06 PM Matthias Brugger <[email protected]> wrote: > > > > On 11/11/2025 22:17, Cibil Pankiras wrote: > > Hi Matthias, > > > > On Mon, Nov 10, 2025 at 4:42 PM Matthias Brugger <[email protected]> wrote: > >> > >> > >> > >> On 10/11/2025 12:24, Cibil Pankiras wrote: > >>> This patch adds support for configuring GPIO pull-up and pull-down > >>> resistors in the BCM283x pinctrl driver. It implements the brcm,pull > >>> device tree property to control pin bias settings. > >>> > >>> The implementation follows the hardware-specific pull control > >>> mechanisms: > >>> - BCM2835: two-step GPPUD register sequence > >>> - BCM2711: direct per-pin control registers > >>> > >>> This enables device tree configurations to specify pull-up, pull-down, > >>> or no bias for individual GPIO pins. > >>> > >>> Tested on Raspberry Pi boards with both BCM2835 and BCM2711 SoCs. > >>> > >>> Signed-off-by: Cibil Pankiras <[email protected]> > >>> --- > >>> arch/arm/mach-bcm283x/include/mach/gpio.h | 10 ++ > >>> drivers/pinctrl/broadcom/pinctrl-bcm283x.c | 102 ++++++++++++++++++++- > >>> 2 files changed, 109 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/arch/arm/mach-bcm283x/include/mach/gpio.h > >>> b/arch/arm/mach-bcm283x/include/mach/gpio.h > >>> index 4aeb48eeb20..c54414a012c 100644 > >>> --- a/arch/arm/mach-bcm283x/include/mach/gpio.h > >>> +++ b/arch/arm/mach-bcm283x/include/mach/gpio.h > >>> @@ -26,6 +26,16 @@ > >>> #define BCM2835_GPIO_FSEL_BANK(gpio) (gpio / 10) > >>> #define BCM2835_GPIO_FSEL_SHIFT(gpio) ((gpio % 10) * 3) > >>> > >>> +/* BCM2835 GPIO Pull-up/down register offsets */ > >>> +#define BCM2835_GPPUD 37 > >>> +#define BCM2835_GPPUDCLK0 38 > >>> + > >>> +/* BCM2711 GPIO Pull-up/down control */ > >>> +#define BCM2711_GPPUD_CNTRL_REG0 57 > >>> +#define BCM2711_PUD_REG_OFFSET(gpio) ((gpio) / 16) > >>> +#define BCM2711_PUD_REG_SHIFT(gpio) (((gpio) % 16) * 2) > >>> +#define BCM2711_PUD_2711_MASK 0x3 > >>> + > >>> struct bcm2835_gpio_regs { > >>> u32 gpfsel[6]; > >>> u32 reserved1; > >>> diff --git a/drivers/pinctrl/broadcom/pinctrl-bcm283x.c > >>> b/drivers/pinctrl/broadcom/pinctrl-bcm283x.c > >>> index cf9350c151e..c588015905d 100644 > >>> --- a/drivers/pinctrl/broadcom/pinctrl-bcm283x.c > >>> +++ b/drivers/pinctrl/broadcom/pinctrl-bcm283x.c > >>> @@ -21,6 +21,8 @@ > >>> #include <asm/system.h> > >>> #include <asm/io.h> > >>> #include <asm/gpio.h> > >>> +#include <dt-bindings/pinctrl/bcm2835.h> > >>> +#include <linux/delay.h> > >>> > >>> struct bcm283x_pinctrl_priv { > >>> u32 *base_reg; > >>> @@ -54,7 +56,66 @@ static int bcm2835_gpio_get_func_id(struct udevice > >>> *dev, unsigned int gpio) > >>> } > >>> > >>> /* > >>> - * bcm283x_pinctrl_set_state: configure pin functions. > >>> + * bcm2835_gpio_set_pull: Set GPIO pull-up/down resistor for BCM2835 > >>> + * @dev: the pinctrl device > >>> + * @gpio: the GPIO pin number > >>> + * @pull: pull setting (BCM2835_PUD_OFF, BCM2835_PUD_DOWN, > >>> BCM2835_PUD_UP) > >>> + */ > >>> +static void bcm2835_gpio_set_pull(struct udevice *dev, unsigned int > >>> gpio, int pull) > >>> +{ > >>> + struct bcm283x_pinctrl_priv *priv = dev_get_priv(dev); > >>> + u32 bank = BCM2835_GPPUDCLK0 + BCM2835_GPIO_COMMON_BANK(gpio); > >>> + u32 bit = BCM2835_GPIO_COMMON_SHIFT(gpio); > >>> + > >>> + /* Set required control signal */ > >>> + writel(pull & 0x3, &priv->base_reg[BCM2835_GPPUD]); > >>> + udelay(1); > >>> + > >>> + /* Clock the control signal into the GPIO pads */ > >>> + writel(1 << bit, &priv->base_reg[bank]); > >>> + udelay(1); > >>> + > >>> + /* Remove the control signal and clock */ > >>> + writel(0, &priv->base_reg[BCM2835_GPPUD]); > >>> + writel(0, &priv->base_reg[bank]); > >>> +} > >>> + > >>> +/* > >>> + * bcm2711_gpio_set_pull: Set GPIO pull-up/down resistor for BCM2711 > >>> + * @dev: the pinctrl device > >>> + * @gpio: the GPIO pin number > >>> + * @pull: pull setting (BCM2835_PUD_OFF, BCM2835_PUD_DOWN, > >>> BCM2835_PUD_UP) > >>> + */ > >>> +static void bcm2711_gpio_set_pull(struct udevice *dev, unsigned int > >>> gpio, int pull) > >>> +{ > >>> + struct bcm283x_pinctrl_priv *priv = dev_get_priv(dev); > >>> + u32 reg_offset; > >>> + u32 bit_shift; > >>> + u32 pull_bits; > >>> + > >>> + /* Findout which GPIO_PUP_PDN_CNTRL register to use */ > >>> + reg_offset = BCM2711_GPPUD_CNTRL_REG0 + > >>> BCM2711_PUD_REG_OFFSET(gpio); > >>> + > >>> + /* Findout the bit position */ > >>> + bit_shift = BCM2711_PUD_REG_SHIFT(gpio); > >>> + > >>> + /* Update the 2-bit field for this GPIO */ > >>> + pull_bits = pull & BCM2711_PUD_2711_MASK; > >>> + clrsetbits_le32(&priv->base_reg[reg_offset], > >>> + BCM2711_PUD_2711_MASK << bit_shift, > >>> + pull_bits << bit_shift); > >>> +} > >>> + > >>> +static void bcm283x_gpio_set_pull(struct udevice *dev, unsigned int > >>> gpio, int pull) > >>> +{ > >>> + if (device_is_compatible(dev, "brcm,bcm2835-gpio")) > >>> + bcm2835_gpio_set_pull(dev, gpio, pull); > >>> + else > >>> + bcm2711_gpio_set_pull(dev, gpio, pull); > >>> +} > >>> + > >>> +/* > >>> + * bcm283x_pinctrl_set_state: configure pin functions and pull states. > >>> * @dev: the pinctrl device to be configured. > >>> * @config: the state to be configured. > >>> * @return: 0 in success > >>> @@ -62,8 +123,9 @@ static int bcm2835_gpio_get_func_id(struct udevice > >>> *dev, unsigned int gpio) > >>> int bcm283x_pinctrl_set_state(struct udevice *dev, struct udevice > >>> *config) > >>> { > >>> u32 pin_arr[MAX_PINS_PER_BANK]; > >>> + u32 pull_arr[MAX_PINS_PER_BANK]; > >>> int function; > >>> - int i, len, pin_count = 0; > >>> + int i, len, pin_count = 0, pull_len = 0, pull_count = 0; > >>> > >>> if (!dev_read_prop(config, "brcm,pins", &len) || !len || > >>> len & 0x3 || dev_read_u32_array(config, "brcm,pins", pin_arr, > >>> @@ -82,8 +144,42 @@ int bcm283x_pinctrl_set_state(struct udevice *dev, > >>> struct udevice *config) > >>> return -EINVAL; > >>> } > >>> > >>> - for (i = 0; i < pin_count; i++) > >>> + /* Check if brcm,pull property exists */ > >>> + if (dev_read_prop(config, "brcm,pull", &pull_len) && pull_len > 0) { > >>> + if (pull_len & 0x3) { > >> > >> We should check for arbitrary values, so check should be, pull_len > 0x2 > > > > In the current version of the patch, I validate each element in > > pull_arr[] as follows: > > for (i = 0; i < pull_count; i++) { > > if (pull_arr[i] > 2) { > > debug("Invalid pull value %d for pin %d in pinconfig %s\n", > > pull_arr[i], pin_arr[i], config->name); > > return -EINVAL; > > > > Could you please clarify? Did you mean to replace the alignment check > > `if (pull_len & 0x3)` with `pull_len > 0x2` ? > > > > I think I miss some basic understanding here. Please help me with that. From > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.yaml?h=v6.18-rc5#n88 > <snip> > brcm,pull: > description: > > Legacy pull setting for the pins (0=none, 1=pull-down, > 2=pull-up). > $ref: /schemas/types.yaml#/definitions/uint32-array > maximum: 2 > </snip> > > So if I understand correctly you are reading the number of bytes in brcm,pull, > which should be either one or two u32 values. The brcm,pull property is a u32 array that defines the pin bias for each pin listed in brcm,pins.
> But you error out only if the pull_len is 0x3. How is that? My gut feeling > tells me it should be either 0x2 or > 0x4 everything else is an invalid DTB. But as I said I think I miss some > fundamental understanding here. Can you please elaborate. I read the number of bytes in pull_len and perform a 4 byte alignment check (pull_len & 0x3). This ensures the array length is always aligned to 4 bytes. This pattern is similar to how brcm,pins array is validated in the same function. I just realized I missed one detail, "brcm,pull may contain either a single value which will be applied to all pins in brcm,pins, or 1 value for each entry in brcm,pins." Reference, https://www.kernel.org/doc/Documentation/devicetree/bindings/pinctrl/brcm,bcm2835-gpio.txt https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/pinctrl-rp1.c?h=v6.18-rc5#n1225 I will send a v2 patch implementing this behavior. > > By the way, I suppose we implement the deprecated binding because the RPi FW > still uses this, correct? Yes, the dtb blobs passed from RPi firmware still uses it the deprecated binding Regards, Cibil -- EGYM SE, Einsteinstraße 172, 81677 München Geschäftsführende Direktoren: Patrick Meininger, Philipp Roesch-Schlanderer, Florian Sauter Gerichtsstand München | Amtsgericht München HRB 303509 | USt.-Id. DE275313632
