reset: Add driver for gpio-controlled reset pins

This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree.

Signed-off-by Philipp Zabel <[email protected]>

Reviewed-by Stephen Warren <[email protected]>

This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree.

Signed-off-by Philipp Zabel <[email protected]>

Reviewed-by Stephen Warren <[email protected]>

From 2f8ce9e5d6525b98f3828b707458e83fabb39d50 Mon Sep 17 00:00:00 2001

Hi Philipp,

Quoted 6 lines · Philipp Zabel

This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree.

Signed-off-by Philipp Zabel <[email protected]>

Reviewed-by Stephen Warren <[email protected]>

I see this patch is very useful, as GPIOs are widely used to reset components/devices on board. But I do not find the patch in v3.11-rc1. What's your plan about it?

Also, I'm wondering if we should register the driver a little bit early. Please see the following patch. If it makes sense to you, I can send the patch to you, or you can just quash it into yours.

Shawn

---8<--------

From 2f8ce9e5d6525b98f3828b707458e83fabb39d50 Mon Sep 17 00:00:00 2001
From: Shawn Guo <[email protected]>
Date: Sun, 14 Jul 2013 20:41:00 +0800
Subject: [PATCH] ENGR00269945: reset: register gpio-reset driver in
arch_initcall

It's a little bit late to register gpio-reset driver at module_init time, because gpio-reset provides reset control via gpio for other devices which are mostly probed at module_init time too. And it becomes even worse, when the gpio comes from IO expander on I2C bus, e.g. pca953x. In that case, gpio-reset needs to be ready before I2C bus driver which is generally ready at subsys_initcall time. Let's register gpio-reset driver in arch_initcall() to have it ready early enough.

The defer probe mechanism is not used here, because a reset controller driver should be reasonably registered early than other devices. More importantly, defer probe doe not help in some nasty cases, e.g. the gpio-pca953x device itself needs a reset from gpio-reset driver to start working.

Quoted 8 lines

It's a little bit late to register gpio-reset driver at module_init time, because gpio-reset provides reset control via gpio for other devices which are mostly probed at module_init time too. And it becomes even worse, when the gpio comes from IO expander on I2C bus, e.g. pca953x. In that case, gpio-reset needs to be ready before I2C bus driver which is generally ready at subsys_initcall time. Let's register gpio-reset driver in arch_initcall() to have it ready early enough.

There's no need for the reset driver to be registered before its users; the users of the reset GPIO will simply have their probe deferred until the reset controller is available, and then everything will work out just fine.

Quoted 5 lines

The defer probe mechanism is not used here, because a reset controller driver should be reasonably registered early than other devices. More importantly, defer probe doe not help in some nasty cases, e.g. the gpio-pca953x device itself needs a reset from gpio-reset driver to start working.

That should work fine with deferred probe.

Quoted 14 lines

Quoted 8 lines

It's a little bit late to register gpio-reset driver at module_init time, because gpio-reset provides reset control via gpio for other devices which are mostly probed at module_init time too. And it becomes even worse, when the gpio comes from IO expander on I2C bus, e.g. pca953x. In that case, gpio-reset needs to be ready before I2C bus driver which is generally ready at subsys_initcall time. Let's register gpio-reset driver in arch_initcall() to have it ready early enough.

There's no need for the reset driver to be registered before its users; the users of the reset GPIO will simply have their probe deferred until the reset controller is available, and then everything will work out just fine.

Quoted 7 lines

Quoted 5 lines

The defer probe mechanism is not used here, because a reset controller driver should be reasonably registered early than other devices. More importantly, defer probe doe not help in some nasty cases, e.g. the gpio-pca953x device itself needs a reset from gpio-reset driver to start working.

That should work fine with deferred probe.

I should probably rework the commit log. But I do not see a problem to register gpio-reset driver a little bit earlier. On the other hand, if we reply on deferred probe, many drivers probe could be deferred. For example, on my system, the gpio-pca953x on I2C bus works as a GPIO controller and provides resets to many board level components. Deferring probe of gpio-pca953x on I2C bus means every single probe of all these components gets deferred. IMO, this situation should be reasonably avoided.

so you want to have gpio-reset probed at arch_initcall time and you have gpio-pca953x probed at subsys_initcall time. Won't then all gpio-reset devices that use gpios on pca953x to reset other peripherals need to be deferred?

Quoted 20 lines · Stephen Warren

Quoted 8 lines

It's a little bit late to register gpio-reset driver at module_init time, because gpio-reset provides reset control via gpio for other devices which are mostly probed at module_init time too. And it becomes even worse, when the gpio comes from IO expander on I2C bus, e.g. pca953x. In that case, gpio-reset needs to be ready before I2C bus driver which is generally ready at subsys_initcall time. Let's register gpio-reset driver in arch_initcall() to have it ready early enough.

There's no need for the reset driver to be registered before its users; the users of the reset GPIO will simply have their probe deferred until the reset controller is available, and then everything will work out just fine.

Quoted 5 lines

The defer probe mechanism is not used here, because a reset controller driver should be reasonably registered early than other devices. More importantly, defer probe doe not help in some nasty cases, e.g. the gpio-pca953x device itself needs a reset from gpio-reset driver to start working.

Quoted 1 line

That should work fine with deferred probe.

I should probably rework the commit log. But I do not see a problem to register gpio-reset driver a little bit earlier.

if we reply on deferred probe, many drivers probe could be deferred. For example, on my system, the gpio-pca953x on I2C bus works as a GPIO controller and provides resets to many board level components. Deferring probe of gpio-pca953x on I2C bus means every single probe of all these components gets deferred. IMO, this situation should be reasonably avoided.

Registering the driver earlier won't cause any bugs. However, it's not the correct approach.

Deferred probe /is/ the approach for assuring correct dependencies between drivers. It works and should be used. There are not enough initcall levels to play games using initcalls and solve all the issues, and the ordering requirements vary board-to-board. Deferred probe at runtime handles this without having to manually place all the drivers into specific initcall levels, and dynamically adjusts to board differences, since it all happens automatically at run-time.

Quoted 10 lines · Stephen Warren

Registering the driver earlier won't cause any bugs. However, it's not the correct approach.

Deferred probe /is/ the approach for assuring correct dependencies between drivers. It works and should be used. There are not enough initcall levels to play games using initcalls and solve all the issues, and the ordering requirements vary board-to-board. Deferred probe at runtime handles this without having to manually place all the drivers into specific initcall levels, and dynamically adjusts to board differences, since it all happens automatically at run-time.

I do not quite follow the argument here. I agree with you that deferred probe is the approach to solve dependencies. But it does not necessarily mean that initcall can not be used to help it save some nasty or nested deferring. Deferred probe and initcalls are not two mutually exclusive mechanisms but two which can help each other.

Quoted 12 lines · Shawn Guo

Quoted 10 lines · Stephen Warren

Registering the driver earlier won't cause any bugs. However, it's not the correct approach.

Deferred probe /is/ the approach for assuring correct dependencies between drivers. It works and should be used. There are not enough initcall levels to play games using initcalls and solve all the issues, and the ordering requirements vary board-to-board. Deferred probe at runtime handles this without having to manually place all the drivers into specific initcall levels, and dynamically adjusts to board differences, since it all happens automatically at run-time.

Quoted 5 lines

I do not quite follow the argument here. I agree with you that deferred probe is the approach to solve dependencies. But it does not necessarily mean that initcall can not be used to help it save some nasty or nested deferring. Deferred probe and initcalls are not two mutually exclusive mechanisms but two which can help each other.

My understanding is that deferred probe was implemented specifically to avoid having to, or allowing, the use of initcall levels to determine probe order.

However, if someone closely associated with the implementation of deferred probe (e.g. Grant, or a device core maintainer) is willing to step up and say I'm wrong, I'll drop my objection.

Quoted 23 lines · Stephen Warren

Quoted 12 lines · Shawn Guo

Quoted 10 lines · Stephen Warren

Registering the driver earlier won't cause any bugs. However, it's not the correct approach.

Deferred probe /is/ the approach for assuring correct dependencies between drivers. It works and should be used. There are not enough initcall levels to play games using initcalls and solve all the issues, and the ordering requirements vary board-to-board. Deferred probe at runtime handles this without having to manually place all the drivers into specific initcall levels, and dynamically adjusts to board differences, since it all happens automatically at run-time.

Quoted 5 lines

I do not quite follow the argument here. I agree with you that deferred probe is the approach to solve dependencies. But it does not necessarily mean that initcall can not be used to help it save some nasty or nested deferring. Deferred probe and initcalls are not two mutually exclusive mechanisms but two which can help each other.

My understanding is that deferred probe was implemented specifically to avoid having to, or allowing, the use of initcall levels to determine probe order.

Quoted 3 lines

However, if someone closely associated with the implementation of deferred probe (e.g. Grant, or a device core maintainer) is willing to step up and say I'm wrong, I'll drop my objection.

AFAIR, defered probing is known to be a "hack" by its authors. We should still try to get initcall levels right...

Quoted 14 lines

Quoted 13 lines

Quoted 5 lines

I do not quite follow the argument here. I agree with you that deferred probe is the approach to solve dependencies. But it does not necessarily mean that initcall can not be used to help it save some nasty or nested deferring. Deferred probe and initcalls are not two mutually exclusive mechanisms but two which can help each other.

My understanding is that deferred probe was implemented specifically to avoid having to, or allowing, the use of initcall levels to determine probe order.

However, if someone closely associated with the implementation of deferred probe (e.g. Grant, or a device core maintainer) is willing to step up and say I'm wrong, I'll drop my objection.

Quoted 2 lines

AFAIR, defered probing is known to be a "hack" by its authors. We should still try to get initcall levels right...

I don't /think/ it was the concept of deferred probe that was considered hacky, but perhaps just the first proof-of-concept implementation, and any hackiness was presumably solved before the feature was merged.

It sound to me like keeping ammount of -EPROBE_DEFER to minimum is still preferred.

Hi,

Quoted 2 lines · Pavel Machek

It sound to me like keeping ammount of -EPROBE_DEFER to minimum is still preferred.

Hand-crafting initcall level ordering of various drivers and subsystem is probably an even greater evil though. We've done it in the past, but now that we have deferred probe we have the option of not having to do it every time, such as this.

You're spending an awful lot of energy arguing over this, but nobody has actually shown data of how much deferral is happening -- and that it's a real problem. Until someone does so there's no reason to change it from the default module init level at all, I'd say.

On Thu, Jul 18, 2013 at 11:45:29AM -0700, Olof Johansson wrote:

Quoted 4 lines

You're spending an awful lot of energy arguing over this, but nobody has actually shown data of how much deferral is happening -- and that it's a real problem. Until someone does so there's no reason to change it from the default module init level at all, I'd say.

On imx6qdl-sabreauto board, there are 3 MAX7310 units as IO expanders to output 3 x 8 = 24 GPIOs. 18 of them are used for resets, enables and pin steering for various peripherals on the board.

BACKLITE_ON
SAT_SHUTDN_B
CPU_PER_RST_B
MAIN_PER_RST_B
IPOD_RST_B
MLB_RST_B
SSI_STEERING
GPS_RST_B

GPS_PWREN
VIDEO_ADC_PWRDN_B
ENET_CAN1_STEER
EIMD30_BTUART3_STEER
CAN_STBY
CAN_EN
USB_H1_PWR

USB_OTG_PWR_ON
SAT_RST_B
NAND_BT_WIFI_STEER

Most of these GPIOs need to set up properly at client device driver's probe stage - module_init() time. So if we have all these service providers resets/enables/steering API registered at module_init() too, the probes of all these client device drivers stand a good chance to be deferred.

That sounds like it /could/ well be an issue. Do you have all that actually hooked up through device tree so you can demonstrate the amount of deferred probing that /actually/ happens though?

In order to push the probe order in the right direction, I would personally prefer to see some kind of explicit hinting or outright representation of probe order in the DT.

Hi,

Quoted 2 lines · Pavel Machek

It sound to me like keeping ammount of -EPROBE_DEFER to minimum is still preferred.

Hand-crafting initcall level ordering of various drivers and subsystem is probably an even greater evil though. We've done it in the past, but now that we have deferred probe we have the option of not having to do it every time, such as this.

You're spending an awful lot of energy arguing over this, but nobody has actually shown data of how much deferral is happening -- and that it's a real problem. Until someone does so there's no reason to change it from the default module init level at all, I'd say.

Hi!

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

Quoted 5 lines

I do not quite follow the argument here. I agree with you that deferred probe is the approach to solve dependencies. But it does not necessarily mean that initcall can not be used to help it save some nasty or nested deferring. Deferred probe and initcalls are not two mutually exclusive mechanisms but two which can help each other.

My understanding is that deferred probe was implemented specifically to avoid having to, or allowing, the use of initcall levels to determine probe order.

However, if someone closely associated with the implementation of deferred probe (e.g. Grant, or a device core maintainer) is willing to step up and say I'm wrong, I'll drop my objection.

Quoted 6 lines

Quoted 2 lines

AFAIR, defered probing is known to be a "hack" by its authors. We should still try to get initcall levels right...

I don't /think/ it was the concept of deferred probe that was considered hacky, but perhaps just the first proof-of-concept implementation, and any hackiness was presumably solved before the feature was merged.

Well...

http://lwn.net/Articles/485194/

From:           Grant Likely <[email protected]>
To:                   linux-kernel at vger.kernel.org
Subject:                [PATCH] drivercore: Add driver probe deferral

mechanism
Date: Mon, 5 Mar 2012 08:47:41 -0700
Message-ID:
<[email protected]>
...

I know that not everybody is happy with this approach, but I've yet to see a better alternative. However, it is *really easy* to find all the
users of deferred probe since any user must return -EPROBE_DEFER explicitly.
If/when a better approach is found, all the users will be easy to find and modify.

...

It sound to me like keeping ammount of -EPROBE_DEFER to minimum is still preferred.

Quoted 12 lines · Shawn Guo

Quoted 10 lines · Stephen Warren

Registering the driver earlier won't cause any bugs. However, it's not the correct approach.

Deferred probe /is/ the approach for assuring correct dependencies between drivers. It works and should be used. There are not enough initcall levels to play games using initcalls and solve all the issues, and the ordering requirements vary board-to-board. Deferred probe at runtime handles this without having to manually place all the drivers into specific initcall levels, and dynamically adjusts to board differences, since it all happens automatically at run-time.

Quoted 5 lines

I do not quite follow the argument here. I agree with you that deferred probe is the approach to solve dependencies. But it does not necessarily mean that initcall can not be used to help it save some nasty or nested deferring. Deferred probe and initcalls are not two mutually exclusive mechanisms but two which can help each other.

My understanding is that deferred probe was implemented specifically to avoid having to, or allowing, the use of initcall levels to determine probe order.

Quoted 28 lines

Hi Philipp,

Quoted 6 lines · Philipp Zabel

This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree.

Signed-off-by Philipp Zabel <[email protected]>

Reviewed-by Stephen Warren <[email protected]>

I see this patch is very useful, as GPIOs are widely used to reset components/devices on board. But I do not find the patch in v3.11-rc1. What's your plan about it?

Also, I'm wondering if we should register the driver a little bit early. Please see the following patch. If it makes sense to you, I can send the patch to you, or you can just quash it into yours.

Shawn

---8<--------

From 2f8ce9e5d6525b98f3828b707458e83fabb39d50 Mon Sep 17 00:00:00 2001
From: Shawn Guo <[email protected]>
Date: Sun, 14 Jul 2013 20:41:00 +0800
Subject: [PATCH] ENGR00269945: reset: register gpio-reset driver in
arch_initcall

Quoted 8 lines

It's a little bit late to register gpio-reset driver at module_init time, because gpio-reset provides reset control via gpio for other devices which are mostly probed at module_init time too. And it becomes even worse, when the gpio comes from IO expander on I2C bus, e.g. pca953x. In that case, gpio-reset needs to be ready before I2C bus driver which is generally ready at subsys_initcall time. Let's register gpio-reset driver in arch_initcall() to have it ready early enough.

There's no need for the reset driver to be registered before its users; the users of the reset GPIO will simply have their probe deferred until the reset controller is available, and then everything will work out just fine.

Quoted 5 lines

The defer probe mechanism is not used here, because a reset controller driver should be reasonably registered early than other devices. More importantly, defer probe doe not help in some nasty cases, e.g. the gpio-pca953x device itself needs a reset from gpio-reset driver to start working.

That should work fine with deferred probe.

Deferred probing is fine, but it'd be nice to keep the probe deferral loops to a minimum where possible and/or reasonable.

Hi Philipp,

Quoted 6 lines · Philipp Zabel

This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree.

Signed-off-by Philipp Zabel <[email protected]>

Reviewed-by Stephen Warren <[email protected]>

I see this patch is very useful, as GPIOs are widely used to reset components/devices on board. But I do not find the patch in v3.11-rc1. What's your plan about it?

Also, I'm wondering if we should register the driver a little bit early. Please see the following patch. If it makes sense to you, I can send the patch to you, or you can just quash it into yours.

From 822a0c4fb7c99b371844ba7e957affcaa8cb1cfe Mon Sep 17 00:00:00 2001

Quoted 14 lines

Hi Philipp,

Quoted 6 lines · Philipp Zabel

This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree.

Signed-off-by Philipp Zabel <[email protected]>

Reviewed-by Stephen Warren <[email protected]>

I see this patch is very useful, as GPIOs are widely used to reset components/devices on board. But I do not find the patch in v3.11-rc1. What's your plan about it?

Quoted 3 lines

Also, I'm wondering if we should register the driver a little bit early. Please see the following patch. If it makes sense to you, I can send the patch to you, or you can just quash it into yours.

And here is another change request.

Shawn

----8<---------------

From 822a0c4fb7c99b371844ba7e957affcaa8cb1cfe Mon Sep 17 00:00:00 2001
From: Shawn Guo <[email protected]>
Date: Sun, 14 Jul 2013 20:28:05 +0800
Subject: [PATCH] ENGR00269945: reset: handle cansleep case in gpio-reset

Some gpio reset may be backed by a gpio that can sleep, e.g. pca953x gpio output. For such gpio, gpio_set_value_cansleep() should be called. Otherwise, the WARN_ON(chip->can_sleep) in gpiod_set_value() will be hit. Add a gpio_cansleep() check to handle cansleep gpio.

Signed-off-by Shawn Guo <[email protected]>

---
 drivers/reset/gpio-reset.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/reset/gpio-reset.c +4 -1

index 5d2515a..4f372f9 100644
@@ -32,7 +32,10 @@ static void gpio_reset_set(struct reset_controller_dev *rcdev, int asserted)
 	if (drvdata->active_low)
 		value = !value;
 
-	gpio_set_value(drvdata->gpio, value);
+	if (gpio_cansleep(drvdata->gpio))
+		gpio_set_value_cansleep(drvdata->gpio, value);
+	else
+		gpio_set_value(drvdata->gpio, value);
 }
 
 static int gpio_reset(struct reset_controller_dev *rcdev, unsigned long id)

Looks good to me. I can squash it into the original patch and resend if you like.

Quoted 10 lines · Shawn Guo

Hi Philipp,

Quoted 6 lines · Philipp Zabel

This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree.

Signed-off-by Philipp Zabel <[email protected]>

Reviewed-by Stephen Warren <[email protected]>

Quoted 7 lines

I see this patch is very useful, as GPIOs are widely used to reset components/devices on board. But I do not find the patch in v3.11-rc1. What's your plan about it?

Also, I'm wondering if we should register the driver a little bit early. Please see the following patch. If it makes sense to you, I can send the patch to you, or you can just quash it into yours.

And here is another change request.

drivers/reset/gpio-reset.c static void gpio_reset_set(struct reset_controller_dev *rcdev, int asserted) · line 38

Quoted 7 lines

drivers/reset/gpio-reset.c +4 -1

 
-	gpio_set_value(drvdata->gpio, value);
+	if (gpio_cansleep(drvdata->gpio))
+		gpio_set_value_cansleep(drvdata->gpio, value);
+	else
+		gpio_set_value(drvdata->gpio, value);

That's not right. Calling gpio_set_value() v.s. gpio_set_value_cansleep() should be based on the properties of the calling context, not the GPIO being controlled. In other words, if it's permissible to call gpio_set_value_cansleep() at this point in the code, simply always call that, and remove the conditional logic.

Quoted 22 lines · Shawn Guo

Quoted 10 lines

Hi Philipp,

Quoted 6 lines · Philipp Zabel

This driver implements a reset controller device that toggle a gpio connected to a reset pin of a peripheral IC. The delay between assertion and de-assertion of the reset signal can be configured via device tree.

Signed-off-by Philipp Zabel <[email protected]>

Reviewed-by Stephen Warren <[email protected]>

Quoted 7 lines

I see this patch is very useful, as GPIOs are widely used to reset components/devices on board. But I do not find the patch in v3.11-rc1. What's your plan about it?

Also, I'm wondering if we should register the driver a little bit early. Please see the following patch. If it makes sense to you, I can send the patch to you, or you can just quash it into yours.

And here is another change request.

drivers/reset/gpio-reset.c +0 -0

drivers/reset/gpio-reset.c static void gpio_reset_set(struct reset_controller_dev *rcdev, int asserted) · line 38

Quoted 5 lines

drivers/reset/gpio-reset.c +4 -1

-	gpio_set_value(drvdata->gpio, value);
+	if (gpio_cansleep(drvdata->gpio))
+		gpio_set_value_cansleep(drvdata->gpio, value);
+	else
+		gpio_set_value(drvdata->gpio, value);

That's not right. Calling gpio_set_value() v.s. gpio_set_value_cansleep() should be based on the properties of the calling context, not the GPIO being controlled. In other words, if it's permissible to call gpio_set_value_cansleep() at this point in the code, simply always call that, and remove the conditional logic.