[PATCH 0/2] pinctrl: bcm2835: Fix gpio hogs and pin reinitialisation

[PATCH 0/2] pinctrl: bcm2835: Fix gpio hogs and pin reinitialisation

[Phil Elwell]

Tackle two problems with the pinctrl-bcm2835 driver and its Device Tree
configuration:

1. The pinctrl-bcm2835 driver is a combined pinctrl/gpio driver.
Currently the gpio side is registered first, but this breaks gpio hogs
(which are configured during gpiochip_add_data).

2. Since [1], a "gpio-ranges" property is required in order for pins
to be returned to inputs when freed. Note that without patch 1, the
device never gets out of EPROBE_DEFER.

Note that the Fixes: tags are little more than hooks to hang the back-ports
on - no blame is intended.

[1] commit 2ab73c6d8323 ("gpio: Support GPIO controllers without
        pin-ranges")

Phil Elwell (2):
  pinctrl: bcm2835: Change init order for gpio hogs
  ARM: dts: gpio-ranges property is now required

 arch/arm/boot/dts/bcm2711.dtsi        |  2 ++
 arch/arm/boot/dts/bcm283x.dtsi        |  2 ++
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 29 +++++++++++++++------------
 3 files changed, 20 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH 1/2] pinctrl: bcm2835: Change init order for gpio hogs

[Phil Elwell]

...and gpio-ranges

pinctrl-bcm2835 is a combined pinctrl/gpio driver. Currently the gpio
side is registered first, but this breaks gpio hogs (which are
configured during gpiochip_add_data). Part of the hog initialisation
is a call to pinctrl_gpio_request, and since the pinctrl driver hasn't
yet been registered this results in an -EPROBE_DEFER from which it can
never recover.

Change the initialisation sequence to register the pinctrl driver
first.

This also solves a similar problem with the gpio-ranges property, which
is required in order for released pins to be returned to inputs.

Fixes: 73345a18d464b ("pinctrl: bcm2835: Pass irqchip when adding
                       gpiochip")
Signed-off-by: Phil Elwell <phil@raspberrypi.com>
---
 drivers/pinctrl/bcm/pinctrl-bcm2835.c | 29 +++++++++++++++------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
index 2abcc6ce4eba..b607d10e4cbd 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
@@ -1244,6 +1244,18 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev)
 		raw_spin_lock_init(&pc->irq_lock[i]);
 	}
 
+	pc->pctl_desc = *pdata->pctl_desc;
+	pc->pctl_dev = devm_pinctrl_register(dev, &pc->pctl_desc, pc);
+	if (IS_ERR(pc->pctl_dev)) {
+		gpiochip_remove(&pc->gpio_chip);
+		return PTR_ERR(pc->pctl_dev);
+	}
+
+	pc->gpio_range = *pdata->gpio_range;
+	pc->gpio_range.base = pc->gpio_chip.base;
+	pc->gpio_range.gc = &pc->gpio_chip;
+	pinctrl_add_gpio_range(pc->pctl_dev, &pc->gpio_range);
+
 	girq = &pc->gpio_chip.irq;
 	girq->chip = &bcm2835_gpio_irq_chip;
 	girq->parent_handler = bcm2835_gpio_irq_handler;
@@ -1251,8 +1263,10 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev)
 	girq->parents = devm_kcalloc(dev, BCM2835_NUM_IRQS,
 				     sizeof(*girq->parents),
 				     GFP_KERNEL);
-	if (!girq->parents)
+	if (!girq->parents) {
+		pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range);
 		return -ENOMEM;
+	}
 
 	if (is_7211) {
 		pc->wake_irq = devm_kcalloc(dev, BCM2835_NUM_IRQS,
@@ -1307,21 +1321,10 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev)
 	err = gpiochip_add_data(&pc->gpio_chip, pc);
 	if (err) {
 		dev_err(dev, "could not add GPIO chip\n");
+		pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range);
 		return err;
 	}
 
-	pc->pctl_desc = *pdata->pctl_desc;
-	pc->pctl_dev = devm_pinctrl_register(dev, &pc->pctl_desc, pc);
-	if (IS_ERR(pc->pctl_dev)) {
-		gpiochip_remove(&pc->gpio_chip);
-		return PTR_ERR(pc->pctl_dev);
-	}
-
-	pc->gpio_range = *pdata->gpio_range;
-	pc->gpio_range.base = pc->gpio_chip.base;
-	pc->gpio_range.gc = &pc->gpio_chip;
-	pinctrl_add_gpio_range(pc->pctl_dev, &pc->gpio_range);
-
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH 1/2] pinctrl: bcm2835: Change init order for gpio hogs

[Phil Elwell]

Hi Andy,

On 01/12/2021 12:06, Andy Shevchenko wrote:
> On Monday, November 29, 2021, Phil Elwell <phil@raspberrypi.com 
> <mailto:phil@raspberrypi.com>> wrote:
> 
>     ...and gpio-ranges
> 
>     pinctrl-bcm2835 is a combined pinctrl/gpio driver. Currently the gpio
>     side is registered first, but this breaks gpio hogs (which are
>     configured during gpiochip_add_data). Part of the hog initialisation
>     is a call to pinctrl_gpio_request, and since the pinctrl driver hasn't
>     yet been registered this results in an -EPROBE_DEFER from which it can
>     never recover.
> 
>     Change the initialisation sequence to register the pinctrl driver
>     first.
> 
>     This also solves a similar problem with the gpio-ranges property, which
>     is required in order for released pins to be returned to inputs.
> 
> 
> We have a callback in GPIO chip to register pin ranges, why driver does it 
> separately?

A few experiments (this is not my driver) appear to show that the call to 
pinctrl_add_gpio_range can be removed, but only once the gpio-ranges DT property 
has been added if we want to remain functionality throughout a bisect. That tidy 
up might be better done with a followup commit once the DT patch has also
been accepted, unless it's possible to guarantee the sequencing between
the pinctrl/gpio tree and the DT tree.

>     Fixes: 73345a18d464b ("pinctrl: bcm2835: Pass irqchip when adding
>                             gpiochip")
>     Signed-off-by: Phil Elwell <phil@raspberrypi.com <mailto:phil@raspberrypi.com>>
> 
> 
> 
> Is it originally so strange indentation or is it only on my side?

The "g" is below the "p" in the patch.

Thanks,

Phil

>     ---
>       drivers/pinctrl/bcm/pinctrl-bcm2835.c | 29 +++++++++++++++------------
>       1 file changed, 16 insertions(+), 13 deletions(-)
> 
>     diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>     b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>     index 2abcc6ce4eba..b607d10e4cbd 100644
>     --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>     +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
>     @@ -1244,6 +1244,18 @@ static int bcm2835_pinctrl_probe(struct
>     platform_device *pdev)
>                      raw_spin_lock_init(&pc->irq_lock[i]);
>              }
> 
>     +       pc->pctl_desc = *pdata->pctl_desc;
>     +       pc->pctl_dev = devm_pinctrl_register(dev, &pc->pctl_desc, pc);
>     +       if (IS_ERR(pc->pctl_dev)) {
>     +               gpiochip_remove(&pc->gpio_chip);
>     +               return PTR_ERR(pc->pctl_dev);
>     +       }
>     +
>     +       pc->gpio_range = *pdata->gpio_range;
>     +       pc->gpio_range.base = pc->gpio_chip.base;
>     +       pc->gpio_range.gc = &pc->gpio_chip;
>     +       pinctrl_add_gpio_range(pc->pctl_dev, &pc->gpio_range);
>     +
>              girq = &pc->gpio_chip.irq;
>              girq->chip = &bcm2835_gpio_irq_chip;
>              girq->parent_handler = bcm2835_gpio_irq_handler;
>     @@ -1251,8 +1263,10 @@ static int bcm2835_pinctrl_probe(struct
>     platform_device *pdev)
>              girq->parents = devm_kcalloc(dev, BCM2835_NUM_IRQS,
>                                           sizeof(*girq->parents),
>                                           GFP_KERNEL);
>     -       if (!girq->parents)
>     +       if (!girq->parents) {
>     +               pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range);
>                      return -ENOMEM;
>     +       }
> 
>              if (is_7211) {
>                      pc->wake_irq = devm_kcalloc(dev, BCM2835_NUM_IRQS,
>     @@ -1307,21 +1321,10 @@ static int bcm2835_pinctrl_probe(struct
>     platform_device *pdev)
>              err = gpiochip_add_data(&pc->gpio_chip, pc);
>              if (err) {
>                      dev_err(dev, "could not add GPIO chip\n");
>     +               pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range);
>                      return err;
>              }
> 
>     -       pc->pctl_desc = *pdata->pctl_desc;
>     -       pc->pctl_dev = devm_pinctrl_register(dev, &pc->pctl_desc, pc);
>     -       if (IS_ERR(pc->pctl_dev)) {
>     -               gpiochip_remove(&pc->gpio_chip);
>     -               return PTR_ERR(pc->pctl_dev);
>     -       }
>     -
>     -       pc->gpio_range = *pdata->gpio_range;
>     -       pc->gpio_range.base = pc->gpio_chip.base;
>     -       pc->gpio_range.gc = &pc->gpio_chip;
>     -       pinctrl_add_gpio_range(pc->pctl_dev, &pc->gpio_range);
>     -
>              return 0;
>       }
> 
>     -- 
>     2.25.1
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH 1/2] pinctrl: bcm2835: Change init order for gpio hogs

[Andy Shevchenko]

On Wed, Dec 1, 2021 at 5:18 PM Phil Elwell <phil@raspberrypi.com> wrote:
> On 01/12/2021 12:06, Andy Shevchenko wrote:
> > On Monday, November 29, 2021, Phil Elwell <phil@raspberrypi.com
> > <mailto:phil@raspberrypi.com>> wrote:
> >
> >     ...and gpio-ranges
> >
> >     pinctrl-bcm2835 is a combined pinctrl/gpio driver. Currently the gpio
> >     side is registered first, but this breaks gpio hogs (which are
> >     configured during gpiochip_add_data). Part of the hog initialisation
> >     is a call to pinctrl_gpio_request, and since the pinctrl driver hasn't
> >     yet been registered this results in an -EPROBE_DEFER from which it can
> >     never recover.
> >
> >     Change the initialisation sequence to register the pinctrl driver
> >     first.
> >
> >     This also solves a similar problem with the gpio-ranges property, which
> >     is required in order for released pins to be returned to inputs.
> >
> >
> > We have a callback in GPIO chip to register pin ranges, why driver does it
> > separately?
>
> A few experiments (this is not my driver) appear to show that the call to
> pinctrl_add_gpio_range can be removed, but only once the gpio-ranges DT property
> has been added if we want to remain functionality throughout a bisect. That tidy
> up might be better done with a followup commit once the DT patch has also
> been accepted, unless it's possible to guarantee the sequencing between
> the pinctrl/gpio tree and the DT tree.

What I meant is why these calls are done in the probe and not in
->add_pin_ranges() callback?
Shouldn't it fix the issue you have observed?

...

> >     Fixes: 73345a18d464b ("pinctrl: bcm2835: Pass irqchip when adding
> >                             gpiochip")
> >     Signed-off-by: Phil Elwell <phil@raspberrypi.com <mailto:phil@raspberrypi.com>>
> >
> > Is it originally so strange indentation or is it only on my side?
>
> The "g" is below the "p" in the patch.

Which is wrong. Tags mustn't be multilines (i.e. split over a single line).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pinctrl: bcm2835: Change init order for gpio hogs

[Phil Elwell]

On 01/12/2021 15:38, Andy Shevchenko wrote:
> On Wed, Dec 1, 2021 at 5:18 PM Phil Elwell <phil@raspberrypi.com> wrote:
>> On 01/12/2021 12:06, Andy Shevchenko wrote:
>>> On Monday, November 29, 2021, Phil Elwell <phil@raspberrypi.com
>>> <mailto:phil@raspberrypi.com>> wrote:
>>>
>>>      ...and gpio-ranges
>>>
>>>      pinctrl-bcm2835 is a combined pinctrl/gpio driver. Currently the gpio
>>>      side is registered first, but this breaks gpio hogs (which are
>>>      configured during gpiochip_add_data). Part of the hog initialisation
>>>      is a call to pinctrl_gpio_request, and since the pinctrl driver hasn't
>>>      yet been registered this results in an -EPROBE_DEFER from which it can
>>>      never recover.
>>>
>>>      Change the initialisation sequence to register the pinctrl driver
>>>      first.
>>>
>>>      This also solves a similar problem with the gpio-ranges property, which
>>>      is required in order for released pins to be returned to inputs.
>>>
>>>
>>> We have a callback in GPIO chip to register pin ranges, why driver does it
>>> separately?
>>
>> A few experiments (this is not my driver) appear to show that the call to
>> pinctrl_add_gpio_range can be removed, but only once the gpio-ranges DT property
>> has been added if we want to remain functionality throughout a bisect. That tidy
>> up might be better done with a followup commit once the DT patch has also
>> been accepted, unless it's possible to guarantee the sequencing between
>> the pinctrl/gpio tree and the DT tree.
> 
> What I meant is why these calls are done in the probe and not in
> ->add_pin_ranges() callback?
> Shouldn't it fix the issue you have observed?

I'm no expert in the field, isn't it preferable to set the gpio-ranges 
pinctrl<->GPIO correspondence with a single line of DT rather than several lines 
of code? A quick grep shows over 700 instances of gpio-ranges in DT, at least 
some of which are reflexive, and only 7 drivers with add_pin_ranges methods.

>>>      Fixes: 73345a18d464b ("pinctrl: bcm2835: Pass irqchip when adding
>>>                              gpiochip")
>>>      Signed-off-by: Phil Elwell <phil@raspberrypi.com <mailto:phil@raspberrypi.com>>
>>>
>>> Is it originally so strange indentation or is it only on my side?
>>
>> The "g" is below the "p" in the patch.
> 
> Which is wrong. Tags mustn't be multilines (i.e. split over a single line).

checkpatch disagrees:

scripts/checkpatch.pl 0001-ARM-dts-gpio-ranges-property-is-now-required.patch
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#10:
[1] commit 2ab73c6d8323 ("gpio: Support GPIO controllers without pin-ranges")

Phil

Re: [PATCH 1/2] pinctrl: bcm2835: Change init order for gpio hogs

[Andy Shevchenko]

On Wed, Dec 1, 2021 at 6:08 PM Phil Elwell <phil@raspberrypi.com> wrote:
> On 01/12/2021 15:38, Andy Shevchenko wrote:

...

> >>>      Fixes: 73345a18d464b ("pinctrl: bcm2835: Pass irqchip when adding
> >>>                              gpiochip")

> >>> Is it originally so strange indentation or is it only on my side?
> >>
> >> The "g" is below the "p" in the patch.
> >
> > Which is wrong. Tags mustn't be multilines (i.e. split over a single line).
>
> checkpatch disagrees:

I don't care less about checkpatch false positives. You have a chance
to fix it, btw.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pinctrl: bcm2835: Change init order for gpio hogs

[Phil Elwell]

On 01/12/2021 16:25, Andy Shevchenko wrote:
> On Wed, Dec 1, 2021 at 6:08 PM Phil Elwell <phil@raspberrypi.com> wrote:
>> On 01/12/2021 15:38, Andy Shevchenko wrote:
> 
> ...
> 
>>>>>       Fixes: 73345a18d464b ("pinctrl: bcm2835: Pass irqchip when adding
>>>>>                               gpiochip")
> 
>>>>> Is it originally so strange indentation or is it only on my side?
>>>>
>>>> The "g" is below the "p" in the patch.
>>>
>>> Which is wrong. Tags mustn't be multilines (i.e. split over a single line).
>>
>> checkpatch disagrees:
> 
> I don't care less about checkpatch false positives. You have a chance
> to fix it, btw.

I only split it because of checkpatch. What a great tool.

I'll change for V2, which I can submit now if you'll ack it with just that change.

Phil

[PATCH 2/2] ARM: dts: gpio-ranges property is now required

[Phil Elwell]

Since [1], added in 5.7, the absence of a gpio-ranges property has
prevented GPIOs from being restored to inputs when released.
Add those properties for BCM283x and BCM2711 devices.

[1] commit 2ab73c6d8323 ("gpio: Support GPIO controllers without
    pin-ranges")

Fixes: 2ab73c6d8323 ("gpio: Support GPIO controllers without
                      pin-ranges")
Signed-off-by: Phil Elwell <phil@raspberrypi.com>
---
 arch/arm/boot/dts/bcm2711.dtsi | 2 ++
 arch/arm/boot/dts/bcm283x.dtsi | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
index 9e01dbca4a01..dff18fc9a906 100644
--- a/arch/arm/boot/dts/bcm2711.dtsi
+++ b/arch/arm/boot/dts/bcm2711.dtsi
@@ -582,6 +582,8 @@ &gpio {
 		     <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>,
 		     <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>;
 
+	gpio-ranges = <&gpio 0 0 58>;
+
 	gpclk0_gpio49: gpclk0_gpio49 {
 		pin-gpclk {
 			pins = "gpio49";
diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
index a3e06b680947..c113661a6668 100644
--- a/arch/arm/boot/dts/bcm283x.dtsi
+++ b/arch/arm/boot/dts/bcm283x.dtsi
@@ -126,6 +126,8 @@ gpio: gpio@7e200000 {
 			interrupt-controller;
 			#interrupt-cells = <2>;
 
+			gpio-ranges = <&gpio 0 0 54>;
+
 			/* Defines common pin muxing groups
 			 *
 			 * While each pin can have its mux selected
-- 
2.25.1

Re: [PATCH 2/2] ARM: dts: gpio-ranges property is now required

[Linus Walleij]

On Mon, Nov 29, 2021 at 11:56 AM Phil Elwell <phil@raspberrypi.com> wrote:

> Since [1], added in 5.7, the absence of a gpio-ranges property has
> prevented GPIOs from being restored to inputs when released.
> Add those properties for BCM283x and BCM2711 devices.
>
> [1] commit 2ab73c6d8323 ("gpio: Support GPIO controllers without
>     pin-ranges")
>
> Fixes: 2ab73c6d8323 ("gpio: Support GPIO controllers without
>                       pin-ranges")
> Signed-off-by: Phil Elwell <phil@raspberrypi.com>

With the funny linebreak fixed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Please merge this patch through the Broadcom/SoC tree.

Yours,
Linus Walleij