[PATCH] pinctrl: at91: fix deferred probing support

[PATCH] pinctrl: at91: fix deferred probing support

[Rob Herring]

AT91 pinctrl deferred probing support is broken if the GPIO devices
haven't probed first and set gpio_banks to non-zero. The later condition
that only some GPIO devices haven't probed can't actually happen as
either all the GPIO devices have probed first or none of them have. Plus
the pinctrl driver has already returned -EINVAL, so it's not on the
deferred list to retry probing.

Fix this by consolidating the checking that all GPIO devices are probed.

Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-gpio@vger.kernel.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
This is a result of trying to remove of_platform_default_populate from 
at91 code and relying on the DT core handling populating devices. That 
changed the probe order and broke booting.

Compile tested only.

Rob

 drivers/pinctrl/pinctrl-at91.c | 36 ++++++++++++----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
index 50f0ec42c637..167838901b75 100644
--- a/drivers/pinctrl/pinctrl-at91.c
+++ b/drivers/pinctrl/pinctrl-at91.c
@@ -1162,7 +1162,7 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 				 struct at91_pinctrl *info)
 {
 	int ret = 0;
-	int i, j;
+	int i, j, ngpio_chips_enabled = 0;
 	uint32_t *tmp;
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *child;
@@ -1175,10 +1175,17 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
 		of_match_device(at91_pinctrl_of_match, &pdev->dev)->data;
 	at91_pinctrl_child_count(info, np);
 
-	if (gpio_banks < 1) {
-		dev_err(&pdev->dev, "you need to specify at least one gpio-controller\n");
-		return -EINVAL;
-	}
+	/*
+	 * We need all the GPIO drivers to probe FIRST, or we will not be able
+	 * to obtain references to the struct gpio_chip * for them, and we
+	 * need this to proceed.
+	 */
+	for (i = 0; i < MAX_GPIO_BANKS; i++)
+		if (gpio_chips[i])
+			ngpio_chips_enabled++;
+
+	if (ngpio_chips_enabled < info->nactive_banks)
+		return -EPROBE_DEFER;
 
 	ret = at91_pinctrl_mux_mask(info, np);
 	if (ret)
@@ -1234,7 +1241,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 {
 	struct at91_pinctrl *info;
 	struct pinctrl_pin_desc *pdesc;
-	int ret, i, j, k, ngpio_chips_enabled = 0;
+	int ret, i, j, k;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -1244,23 +1251,6 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	/*
-	 * We need all the GPIO drivers to probe FIRST, or we will not be able
-	 * to obtain references to the struct gpio_chip * for them, and we
-	 * need this to proceed.
-	 */
-	for (i = 0; i < gpio_banks; i++)
-		if (gpio_chips[i])
-			ngpio_chips_enabled++;
-
-	if (ngpio_chips_enabled < info->nactive_banks) {
-		dev_warn(&pdev->dev,
-			 "All GPIO chips are not registered yet (%d/%d)\n",
-			 ngpio_chips_enabled, info->nactive_banks);
-		devm_kfree(&pdev->dev, info);
-		return -EPROBE_DEFER;
-	}
-
 	at91_pinctrl_desc.name = dev_name(&pdev->dev);
 	at91_pinctrl_desc.npins = gpio_banks * MAX_NB_GPIO_PER_BANK;
 	at91_pinctrl_desc.pins = pdesc =
-- 
2.17.1

Re: [PATCH] pinctrl: at91: fix deferred probing support

[Ludovic Desroches]

On Thu, Jul 12, 2018 at 01:22:22PM -0600, Rob Herring wrote:
> AT91 pinctrl deferred probing support is broken if the GPIO devices
> haven't probed first and set gpio_banks to non-zero. The later condition
> that only some GPIO devices haven't probed can't actually happen as
> either all the GPIO devices have probed first or none of them have. Plus
> the pinctrl driver has already returned -EINVAL, so it's not on the
> deferred list to retry probing.
> 
> Fix this by consolidating the checking that all GPIO devices are probed.
> 
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-gpio@vger.kernel.org
> Signed-off-by: Rob Herring <robh@kernel.org>
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com> 

> ---
> This is a result of trying to remove of_platform_default_populate from 
> at91 code and relying on the DT core handling populating devices. That 
> changed the probe order and broke booting.
> 
> Compile tested only.

Tested on an at91-sama5d4_xplained board, it still boots.

Regards

Ludovic

> 
> Rob
> 
>  drivers/pinctrl/pinctrl-at91.c | 36 ++++++++++++----------------------
>  1 file changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-at91.c b/drivers/pinctrl/pinctrl-at91.c
> index 50f0ec42c637..167838901b75 100644
> --- a/drivers/pinctrl/pinctrl-at91.c
> +++ b/drivers/pinctrl/pinctrl-at91.c
> @@ -1162,7 +1162,7 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
>  				 struct at91_pinctrl *info)
>  {
>  	int ret = 0;
> -	int i, j;
> +	int i, j, ngpio_chips_enabled = 0;
>  	uint32_t *tmp;
>  	struct device_node *np = pdev->dev.of_node;
>  	struct device_node *child;
> @@ -1175,10 +1175,17 @@ static int at91_pinctrl_probe_dt(struct platform_device *pdev,
>  		of_match_device(at91_pinctrl_of_match, &pdev->dev)->data;
>  	at91_pinctrl_child_count(info, np);
>  
> -	if (gpio_banks < 1) {
> -		dev_err(&pdev->dev, "you need to specify at least one gpio-controller\n");
> -		return -EINVAL;
> -	}
> +	/*
> +	 * We need all the GPIO drivers to probe FIRST, or we will not be able
> +	 * to obtain references to the struct gpio_chip * for them, and we
> +	 * need this to proceed.
> +	 */
> +	for (i = 0; i < MAX_GPIO_BANKS; i++)
> +		if (gpio_chips[i])
> +			ngpio_chips_enabled++;
> +
> +	if (ngpio_chips_enabled < info->nactive_banks)
> +		return -EPROBE_DEFER;
>  
>  	ret = at91_pinctrl_mux_mask(info, np);
>  	if (ret)
> @@ -1234,7 +1241,7 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  {
>  	struct at91_pinctrl *info;
>  	struct pinctrl_pin_desc *pdesc;
> -	int ret, i, j, k, ngpio_chips_enabled = 0;
> +	int ret, i, j, k;
>  
>  	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
>  	if (!info)
> @@ -1244,23 +1251,6 @@ static int at91_pinctrl_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	/*
> -	 * We need all the GPIO drivers to probe FIRST, or we will not be able
> -	 * to obtain references to the struct gpio_chip * for them, and we
> -	 * need this to proceed.
> -	 */
> -	for (i = 0; i < gpio_banks; i++)
> -		if (gpio_chips[i])
> -			ngpio_chips_enabled++;
> -
> -	if (ngpio_chips_enabled < info->nactive_banks) {
> -		dev_warn(&pdev->dev,
> -			 "All GPIO chips are not registered yet (%d/%d)\n",
> -			 ngpio_chips_enabled, info->nactive_banks);
> -		devm_kfree(&pdev->dev, info);
> -		return -EPROBE_DEFER;
> -	}
> -
>  	at91_pinctrl_desc.name = dev_name(&pdev->dev);
>  	at91_pinctrl_desc.npins = gpio_banks * MAX_NB_GPIO_PER_BANK;
>  	at91_pinctrl_desc.pins = pdesc =
> -- 
> 2.17.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: at91: fix deferred probing support

[Alexandre Belloni]

Hi Rob,

On 12/07/2018 13:22:22-0600, Rob Herring wrote:
> AT91 pinctrl deferred probing support is broken if the GPIO devices
> haven't probed first and set gpio_banks to non-zero. The later condition
> that only some GPIO devices haven't probed can't actually happen as
> either all the GPIO devices have probed first or none of them have. Plus
> the pinctrl driver has already returned -EINVAL, so it's not on the
> deferred list to retry probing.
> 
> Fix this by consolidating the checking that all GPIO devices are probed.
> 
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-gpio@vger.kernel.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> This is a result of trying to remove of_platform_default_populate from 
> at91 code and relying on the DT core handling populating devices. That 
> changed the probe order and broke booting.
> 

This solves part of the issue but when tested with the
of_platform_default_populate removal, many drivers will fail with
gpiod_set_value: invalid GPIO (errorpointer)

or 

gpiod_get_value: invalid GPIO (errorpointer)

This happens both before and after the pinctrl driver probed.

I didn't investigate much yet.

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] pinctrl: at91: fix deferred probing support

[Rob Herring]

On Fri, Jul 13, 2018 at 3:47 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> Hi Rob,
>
> On 12/07/2018 13:22:22-0600, Rob Herring wrote:
> > AT91 pinctrl deferred probing support is broken if the GPIO devices
> > haven't probed first and set gpio_banks to non-zero. The later condition
> > that only some GPIO devices haven't probed can't actually happen as
> > either all the GPIO devices have probed first or none of them have. Plus
> > the pinctrl driver has already returned -EINVAL, so it's not on the
> > deferred list to retry probing.
> >
> > Fix this by consolidating the checking that all GPIO devices are probed.
> >
> > Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-gpio@vger.kernel.org
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> > This is a result of trying to remove of_platform_default_populate from
> > at91 code and relying on the DT core handling populating devices. That
> > changed the probe order and broke booting.
> >
>
> This solves part of the issue but when tested with the
> of_platform_default_populate removal, many drivers will fail with
> gpiod_set_value: invalid GPIO (errorpointer)
>
> or
>
> gpiod_get_value: invalid GPIO (errorpointer)
>
> This happens both before and after the pinctrl driver probed.
>
> I didn't investigate much yet.

Looks to me like it may be a circular dependency. The GPIO request
functions depend on the pinctrl driver which depends on the gpio
driver. Maybe returning EPROBE_DEFER in at91_gpio_request_enable and
removing the requirement that the GPIO driver probe first would fix
it...

Rob

Re: [PATCH] pinctrl: at91: fix deferred probing support

[Linus Walleij]

On Fri, Jul 13, 2018 at 5:27 PM Rob Herring <robh@kernel.org> wrote:
> On Fri, Jul 13, 2018 at 3:47 AM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> >
> > Hi Rob,
> >
> > On 12/07/2018 13:22:22-0600, Rob Herring wrote:
> > > AT91 pinctrl deferred probing support is broken if the GPIO devices
> > > haven't probed first and set gpio_banks to non-zero. The later condition
> > > that only some GPIO devices haven't probed can't actually happen as
> > > either all the GPIO devices have probed first or none of them have. Plus
> > > the pinctrl driver has already returned -EINVAL, so it's not on the
> > > deferred list to retry probing.
> > >
> > > Fix this by consolidating the checking that all GPIO devices are probed.
> > >
> > > Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> > > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > > Cc: linux-arm-kernel@lists.infradead.org
> > > Cc: linux-gpio@vger.kernel.org
> > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > ---
> > > This is a result of trying to remove of_platform_default_populate from
> > > at91 code and relying on the DT core handling populating devices. That
> > > changed the probe order and broke booting.
> > >
> >
> > This solves part of the issue but when tested with the
> > of_platform_default_populate removal, many drivers will fail with
> > gpiod_set_value: invalid GPIO (errorpointer)
> >
> > or
> >
> > gpiod_get_value: invalid GPIO (errorpointer)
> >
> > This happens both before and after the pinctrl driver probed.
> >
> > I didn't investigate much yet.
>
> Looks to me like it may be a circular dependency. The GPIO request
> functions depend on the pinctrl driver which depends on the gpio
> driver. Maybe returning EPROBE_DEFER in at91_gpio_request_enable and
> removing the requirement that the GPIO driver probe first would fix
> it...

I think you're spot in.

This kind of problem has cropped up in these subsystems since day
one and we probably just have different lame attempts to paper
over it all over.

I suspect that what we need to do is more akin to the DRM model
which I recently learned about (drivers/gpu/drm/vc4/vc4_drv.c is a very
clean and good example) using the core kernel component
infrastructure:

1. Drivers probe() independently, doing intiialization of state
  container struct and retreiveing resources like I/O memory,
  regulators, IRQ, DMA... but no registering interfaces to
  the subsystems yet.

2. The probe of the main component (master) matches the
  subcomponents on the platform bus. In this case, whatever
  needs to come first is the master. I guess pin control.
  return component_master_add_with_match(dev, &vc4_drm_ops, match);

3. Subcomponents:
   return component_add(&pdev->dev, &vc4_dsi_ops);

4. Later on we take control over the binding order.
   the .bind callback of the main component issues
   component_bind_all(dev, drm) and there the subdevices
   are bound.

The process is perfectly reversible. The idea here is to break the
neck of the problem by doing enough set-up in probe() so that
bind() can commence with all its resources. My Nomadik driver
already makes a half-assed attempt at this (not using the component
framework) but I could refactor it to provide an example if
there is interest.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: at91: fix deferred probing support

[Rob Herring]

On Mon, Jul 16, 2018 at 7:58 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Jul 13, 2018 at 5:27 PM Rob Herring <robh@kernel.org> wrote:
> > On Fri, Jul 13, 2018 at 3:47 AM Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> > >
> > > Hi Rob,
> > >
> > > On 12/07/2018 13:22:22-0600, Rob Herring wrote:
> > > > AT91 pinctrl deferred probing support is broken if the GPIO devices
> > > > haven't probed first and set gpio_banks to non-zero. The later condition
> > > > that only some GPIO devices haven't probed can't actually happen as
> > > > either all the GPIO devices have probed first or none of them have. Plus
> > > > the pinctrl driver has already returned -EINVAL, so it's not on the
> > > > deferred list to retry probing.
> > > >
> > > > Fix this by consolidating the checking that all GPIO devices are probed.
> > > >
> > > > Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> > > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > > Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
> > > > Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > Cc: linux-gpio@vger.kernel.org
> > > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > > ---
> > > > This is a result of trying to remove of_platform_default_populate from
> > > > at91 code and relying on the DT core handling populating devices. That
> > > > changed the probe order and broke booting.
> > > >
> > >
> > > This solves part of the issue but when tested with the
> > > of_platform_default_populate removal, many drivers will fail with
> > > gpiod_set_value: invalid GPIO (errorpointer)
> > >
> > > or
> > >
> > > gpiod_get_value: invalid GPIO (errorpointer)
> > >
> > > This happens both before and after the pinctrl driver probed.
> > >
> > > I didn't investigate much yet.
> >
> > Looks to me like it may be a circular dependency. The GPIO request
> > functions depend on the pinctrl driver which depends on the gpio
> > driver. Maybe returning EPROBE_DEFER in at91_gpio_request_enable and
> > removing the requirement that the GPIO driver probe first would fix
> > it...
>
> I think you're spot in.
>
> This kind of problem has cropped up in these subsystems since day
> one and we probably just have different lame attempts to paper
> over it all over.
>
> I suspect that what we need to do is more akin to the DRM model
> which I recently learned about (drivers/gpu/drm/vc4/vc4_drv.c is a very
> clean and good example) using the core kernel component
> infrastructure:
>
> 1. Drivers probe() independently, doing intiialization of state
>   container struct and retreiveing resources like I/O memory,
>   regulators, IRQ, DMA... but no registering interfaces to
>   the subsystems yet.
>
> 2. The probe of the main component (master) matches the
>   subcomponents on the platform bus. In this case, whatever
>   needs to come first is the master. I guess pin control.
>   return component_master_add_with_match(dev, &vc4_drm_ops, match);
>
> 3. Subcomponents:
>    return component_add(&pdev->dev, &vc4_dsi_ops);
>
> 4. Later on we take control over the binding order.
>    the .bind callback of the main component issues
>    component_bind_all(dev, drm) and there the subdevices
>    are bound.
>
> The process is perfectly reversible. The idea here is to break the
> neck of the problem by doing enough set-up in probe() so that
> bind() can commence with all its resources. My Nomadik driver
> already makes a half-assed attempt at this (not using the component
> framework) but I could refactor it to provide an example if
> there is interest.

I'm not really a fan of the component f/w. It feels bolted on and
should be more integrated into the driver model IMHO (though I've not
given any thought to what that would look like).

The problem in this case could be easily avoided by removing
"simple-bus" from the pinctrl node (having a driver for a node should
be a clue that "simple-bus" is not appropriate). Then the pinctrl
driver can properly sequence things by calling of_platform_populate to
instantiate the GPIO devices first and then registering with pinctrl
subsys. However, we can't just remove "simple-bus" to fix this without
having a compatibility problem.

Rob