Skip to contentSkip to conversation

pinctrl: at91: fix deferred probing support

PATCH New Rob Herring 6 participants 10 messages Jul 12, 2018 – Jan 27, 2023

Subsystems (4)
GPIO Arm/Microchip (AT91) SoC support Pin control Pin controller - Microchip AT91
lore
Share & tools
lore

Rob Herring

author
#0 lore raw

PATCH 1/1 pinctrl: at91: fix deferred probing support

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 <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Nicolas Ferre <[email protected]>
Cc Alexandre Belloni <[email protected]>
Cc linux-arm-kernel at lists.infradead.org
Cc linux-gpio at vger.kernel.org
Signed-off-by Rob Herring <[email protected]>

---
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(-)
drivers/pinctrl/pinctrl-at91.c +13 -23
index 50f0ec42c637..167838901b75 100644
@@ -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 =
Signature · 1 line
2.17.1
maintainer
#1 lore raw

PATCH 1/1 pinctrl: at91: fix deferred probing support

Quoted 9 lines · 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.

Quoted 7 lines
Cc Jean-Christophe Plagniol-Villard <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Nicolas Ferre <[email protected]>
Cc Alexandre Belloni <[email protected]>
Cc linux-arm-kernel at lists.infradead.org
Cc linux-gpio at vger.kernel.org
Signed-off-by Rob Herring <[email protected]>
Acked-by Ludovic Desroches <[email protected]>
Quoted 6 lines

---
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

Quoted 81 lines

Rob

 drivers/pinctrl/pinctrl-at91.c | 36 ++++++++++++----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)
drivers/pinctrl/pinctrl-at91.c +13 -23
index 50f0ec42c637..167838901b75 100644
@@ -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 =
Signature · 6 lines
2.17.1

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#2 lore raw

PATCH 1/1 pinctrl: at91: fix deferred probing support

In reply to Rob Herring

Hi Rob,

Quoted 16 lines · 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 <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Nicolas Ferre <[email protected]>
Cc Alexandre Belloni <[email protected]>
Cc linux-arm-kernel at lists.infradead.org
Cc linux-gpio at vger.kernel.org
Signed-off-by Rob Herring <[email protected]>
Quoted 5 lines

---
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.

Signature · 3 lines
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
author
#3 lore raw

PATCH 1/1 pinctrl: at91: fix deferred probing support

Quoted 30 lines · Alexandre Belloni

Hi Rob,

Quoted 16 lines · 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 <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Nicolas Ferre <[email protected]>
Cc Alexandre Belloni <[email protected]>
Cc linux-arm-kernel at lists.infradead.org
Cc linux-gpio at vger.kernel.org
Signed-off-by Rob Herring <[email protected]>
Quoted 5 lines

---
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)

Quoted 7 lines

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

#4 lore raw

PATCH 1/1 pinctrl: at91: fix deferred probing support

Quoted 38 lines · Rob Herring
Quoted 36 lines · Alexandre Belloni

Hi Rob,

Quoted 16 lines · 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 <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Nicolas Ferre <[email protected]>
Cc Alexandre Belloni <[email protected]>
Cc linux-arm-kernel at lists.infradead.org
Cc linux-gpio at vger.kernel.org
Signed-off-by Rob Herring <[email protected]>
Quoted 5 lines

---
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.

Quoted 7 lines
Quoted 1 line

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

author
#5 lore raw

PATCH 1/1 pinctrl: at91: fix deferred probing support

Quoted 77 lines · Linus Walleij
Quoted 38 lines · Rob Herring
Quoted 36 lines · Alexandre Belloni

Hi Rob,

Quoted 16 lines · 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 <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Nicolas Ferre <[email protected]>
Cc Alexandre Belloni <[email protected]>
Cc linux-arm-kernel at lists.infradead.org
Cc linux-gpio at vger.kernel.org
Signed-off-by Rob Herring <[email protected]>
Quoted 5 lines

---
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.

Quoted 7 lines
Quoted 1 line

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.

Quoted 6 lines

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

#6 lore raw
In reply to Alexandre Belloni
Quoted 29 lines · Alexandre Belloni

Hi Rob,

Quoted 16 lines · 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 <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Nicolas Ferre <[email protected]>
Cc Alexandre Belloni <[email protected]>
Cc linux-arm-kernel at lists.infradead.org
Cc linux-gpio at vger.kernel.org
Signed-off-by Rob Herring <[email protected]>
Quoted 5 lines

---
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)

Quoted 8 lines

or

gpiod_get_value: invalid GPIO (errorpointer)

This happens both before and after the pinctrl driver probed.

I didn't investigate much yet.

I applied this patch as well as of_platform_default_populate removal patch on v6.1.0-rc6 with the at91-sam9x60-ek I did not experience the issues that are mentioned on this thread or
ARM: at91: remove unnecessary of_platform_default_populate calls, thread.

Has this been fixed since 2018?

Best,
Ryan
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

#7 lore raw
Quoted 6 lines · On Mon, Nov 21, 2022 at 10:56 PM <[email protected]> wrote:

I applied this patch as well as of_platform_default_populate removal patch on v6.1.0-rc6 with the at91-sam9x60-ek I did not experience the issues that are mentioned on this thread or
ARM: at91: remove unnecessary of_platform_default_populate calls, thread.

Has this been fixed since 2018?

I didn't apply this patch back then because I could not see clear consensus that it fixes the problem.

But I can apply it if it makes things better. Should I? Rob? Ludovic?

The situation is better than in 2018 because of drastic device core changes trying to reorder probe() calls in resource dependency order.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

maintainer Tested-by Acked-by
#8 lore raw

Hi Linus

Quoted 7 lines · Linus Walleij
Quoted 5 lines · On Mon, Nov 21, 2022 at 10:56 PM <[email protected]> wrote:

I applied this patch as well as of_platform_default_populate removal patch on v6.1.0-rc6 with the at91-sam9x60-ek I did not experience the issues that are mentioned on this thread or
ARM: at91: remove unnecessary of_platform_default_populate calls, thread.

Quoted 6 lines
Quoted 1 line

Has this been fixed since 2018?

I didn't apply this patch back then because I could not see clear consensus that it fixes the problem.

But I can apply it if it makes things better. Should I? Rob? Ludovic?

Can you please apply this patch as I verified as well that it doesn't cause issues on a different SoC than Ryan:

Tested-by Nicolas Ferre <[email protected]> # on sama5d3 xplained
Acked-by Nicolas Ferre <[email protected]>

Best regards,
Nicolas

Quoted 5 lines

The situation is better than in 2018 because of drastic device core changes trying to reorder probe() calls in resource dependency order.

Yours,
Linus Walleij

Signature · 7 lines
Nicolas Ferre


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
#9 lore raw
Quoted 7 lines · Nicolas Ferre
Quoted 1 line

But I can apply it if it makes things better. Should I? Rob? Ludovic?

Can you please apply this patch as I verified as well that it doesn't cause issues on a different SoC than Ryan:

Tested-by Nicolas Ferre <[email protected]> # on sama5d3 xplained
Acked-by Nicolas Ferre <[email protected]>

OK patch applied!

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel