[PATCH v8] reset: Add driver for gpio-controlled reset pins

[PATCH v8] reset: Add driver for gpio-controlled reset pins

[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 <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Reviewed-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes since v7:
 - renamed __gpio_reset_set to gpio_reset_set
 - made delay_us signed, negative if reset-delay-us is not set
 - added error messages for reset-gpios and reset-delay-us misconfiguration
 - set drvdata before calling reset_controller_register
---
 .../devicetree/bindings/reset/gpio-reset.txt       |  35 +++++
 drivers/reset/Kconfig                              |  11 ++
 drivers/reset/Makefile                             |   1 +
 drivers/reset/gpio-reset.c                         | 174 +++++++++++++++++++++
 4 files changed, 221 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/gpio-reset.txt
 create mode 100644 drivers/reset/gpio-reset.c

diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt
new file mode 100644
index 0000000..bca5348
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/gpio-reset.txt
@@ -0,0 +1,35 @@
+GPIO reset controller
+=====================
+
+A GPIO reset controller controls a single GPIO that is connected to the reset
+pin of a peripheral IC. Please also refer to reset.txt in this directory for
+common reset controller binding usage.
+
+Required properties:
+- compatible: Should be "gpio-reset"
+- reset-gpios: A gpio used as reset line. The gpio specifier for this property
+               depends on the gpio controller that provides the gpio.
+- #reset-cells: 0, see below
+
+Optional properties:
+- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for
+                  this duration to reset.
+- initially-in-reset: boolean. If not set, the initial state should be a
+                      deasserted reset line. If this property exists, the
+                      reset line should be kept in reset.
+
+example:
+
+sii902x_reset: gpio-reset {
+	compatible = "gpio-reset";
+	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
+	reset-delay-us = <10000>;
+	initially-in-reset;
+	#reset-cells = <0>;
+};
+
+/* Device with nRESET pin connected to GPIO5_0 */
+sii902x@39 {
+	/* ... */
+	resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */
+};
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index c9d04f7..1a862df 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -11,3 +11,14 @@ menuconfig RESET_CONTROLLER
 	  via GPIOs or SoC-internal reset controller modules.
 
 	  If unsure, say no.
+
+if RESET_CONTROLLER
+
+config RESET_GPIO
+	tristate "GPIO reset controller support"
+	depends on GPIOLIB && OF
+	help
+	  This driver provides support for reset lines that are controlled
+	  directly by GPIOs.
+
+endif
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 1e2d83f..b854f20 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_RESET_CONTROLLER) += core.o
+obj-$(CONFIG_RESET_GPIO) += gpio-reset.o
diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c
new file mode 100644
index 0000000..5d2515a
--- /dev/null
+++ b/drivers/reset/gpio-reset.c
@@ -0,0 +1,174 @@
+/*
+ * GPIO Reset Controller driver
+ *
+ * Copyright 2013 Philipp Zabel, Pengutronix
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+
+struct gpio_reset_data {
+	struct reset_controller_dev rcdev;
+	unsigned int gpio;
+	bool active_low;
+	s32 delay_us;
+};
+
+static void gpio_reset_set(struct reset_controller_dev *rcdev, int asserted)
+{
+	struct gpio_reset_data *drvdata = container_of(rcdev,
+			struct gpio_reset_data, rcdev);
+	int value = asserted;
+
+	if (drvdata->active_low)
+		value = !value;
+
+	gpio_set_value(drvdata->gpio, value);
+}
+
+static int gpio_reset(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct gpio_reset_data *drvdata = container_of(rcdev,
+			struct gpio_reset_data, rcdev);
+
+	if (drvdata->delay_us < 0)
+		return -ENOSYS;
+
+	gpio_reset_set(rcdev, 1);
+	udelay(drvdata->delay_us);
+	gpio_reset_set(rcdev, 0);
+
+	return 0;
+}
+
+static int gpio_reset_assert(struct reset_controller_dev *rcdev,
+		unsigned long id)
+{
+	gpio_reset_set(rcdev, 1);
+
+	return 0;
+}
+
+static int gpio_reset_deassert(struct reset_controller_dev *rcdev,
+		unsigned long id)
+{
+	gpio_reset_set(rcdev, 0);
+
+	return 0;
+}
+
+static struct reset_control_ops gpio_reset_ops = {
+	.reset = gpio_reset,
+	.assert = gpio_reset_assert,
+	.deassert = gpio_reset_deassert,
+};
+
+static int of_gpio_reset_xlate(struct reset_controller_dev *rcdev,
+			       const struct of_phandle_args *reset_spec)
+{
+	if (WARN_ON(reset_spec->args_count != 0))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int gpio_reset_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct gpio_reset_data *drvdata;
+	enum of_gpio_flags flags;
+	unsigned long gpio_flags;
+	bool initially_in_reset;
+	int ret;
+
+	drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL);
+	if (drvdata == NULL)
+		return -ENOMEM;
+
+	if (of_gpio_named_count(np, "reset-gpios") != 1) {
+		dev_err(&pdev->dev,
+			"reset-gpios property missing, or not a single gpio\n");
+		return -EINVAL;
+	}
+
+	drvdata->gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &flags);
+	if (drvdata->gpio == -EPROBE_DEFER) {
+		return drvdata->gpio;
+	} else if (!gpio_is_valid(drvdata->gpio)) {
+		dev_err(&pdev->dev, "invalid reset gpio: %d\n", drvdata->gpio);
+		return drvdata->gpio;
+	}
+
+	drvdata->active_low = flags & OF_GPIO_ACTIVE_LOW;
+
+	ret = of_property_read_u32(np, "reset-delay-us", &drvdata->delay_us);
+	if (ret < 0)
+		drvdata->delay_us = -1;
+	else if (drvdata->delay_us < 0)
+		dev_warn(&pdev->dev, "reset delay too high\n");
+
+	initially_in_reset = of_property_read_bool(np, "initially-in-reset");
+	if (drvdata->active_low ^ initially_in_reset)
+		gpio_flags = GPIOF_OUT_INIT_HIGH;
+	else
+		gpio_flags = GPIOF_OUT_INIT_LOW;
+
+	ret = devm_gpio_request_one(&pdev->dev, drvdata->gpio, gpio_flags, NULL);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to request gpio %d: %d\n",
+			drvdata->gpio, ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, drvdata);
+
+	drvdata->rcdev.of_node = np;
+	drvdata->rcdev.owner = THIS_MODULE;
+	drvdata->rcdev.nr_resets = 1;
+	drvdata->rcdev.ops = &gpio_reset_ops;
+	drvdata->rcdev.of_xlate = of_gpio_reset_xlate;
+	reset_controller_register(&drvdata->rcdev);
+
+	return 0;
+}
+
+static int gpio_reset_remove(struct platform_device *pdev)
+{
+	struct gpio_reset_data *drvdata = platform_get_drvdata(pdev);
+
+	reset_controller_unregister(&drvdata->rcdev);
+
+	return 0;
+}
+
+static struct of_device_id gpio_reset_dt_ids[] = {
+	{ .compatible = "gpio-reset" },
+	{ }
+};
+
+static struct platform_driver gpio_reset_driver = {
+	.probe = gpio_reset_probe,
+	.remove = gpio_reset_remove,
+	.driver = {
+		.name = "gpio-reset",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(gpio_reset_dt_ids),
+	},
+};
+
+module_platform_driver(gpio_reset_driver);
+
+MODULE_AUTHOR("Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>");
+MODULE_DESCRIPTION("gpio reset controller");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:gpio-reset");
+MODULE_DEVICE_TABLE(of, gpio_reset_dt_ids);
-- 
1.8.2.rc2

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Pavel Machek]

Hi!

> 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 <p.zabel@pengutronix.de>
> Reviewed-by: Stephen Warren <swarren@nvidia.com>

Reviewed-by: Pavel Machek <pavel@ucw.cz>

Seems ok to me. Having value = asserted is interesting, but you are
basically using it as a documentation, so it looks ok.

Thanks,

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Shawn Guo]

Hi Philipp,

On Thu, May 30, 2013 at 11:09:00AM +0200, Philipp Zabel wrote:
> 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 <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Reviewed-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

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 <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
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.

Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/reset/gpio-reset.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c
index 4f372f9..a7f214f 100644
--- a/drivers/reset/gpio-reset.c
+++ b/drivers/reset/gpio-reset.c
@@ -168,7 +168,17 @@ static struct platform_driver gpio_reset_driver = {
 	},
 };
 
-module_platform_driver(gpio_reset_driver);
+static int __init gpio_reset_init(void)
+{
+	return platform_driver_register(&gpio_reset_driver);
+}
+arch_initcall(gpio_reset_init);
+
+static void __exit gpio_reset_exit(void)
+{
+	platform_driver_unregister(&gpio_reset_driver);
+}
+module_exit(gpio_reset_exit);
 
 MODULE_AUTHOR("Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>");
 MODULE_DESCRIPTION("gpio reset controller");
-- 
1.7.9.5

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Stephen Warren]

On 07/15/2013 07:50 PM, Shawn Guo wrote:
> Hi Philipp,
> 
> On Thu, May 30, 2013 at 11:09:00AM +0200, Philipp Zabel wrote:
>> 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 <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Reviewed-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> 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 <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 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.

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.

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

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Shawn Guo]

On Mon, Jul 15, 2013 at 09:35:52PM -0600, Stephen Warren wrote:
> > 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.
> 
> > 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.

Shawn

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Philipp Zabel]

Hi Shawn,

Am Dienstag, den 16.07.2013, 12:10 +0800 schrieb Shawn Guo:
> On Mon, Jul 15, 2013 at 09:35:52PM -0600, Stephen Warren wrote:
> > > 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.
> > 
> > > 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?

regards
Philipp

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Shawn Guo]

On Tue, Jul 16, 2013 at 11:49:26AM +0200, Philipp Zabel wrote:
> 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?

Yes, they will be deferred, but they will be probed and ready at
subsys_initcall time when gpio-pca953x probes.  This is still good since
most of component device drivers probes at module_init time.

Shawn

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Stephen Warren]

On 07/15/2013 10:10 PM, Shawn Guo wrote:
> On Mon, Jul 15, 2013 at 09:35:52PM -0600, Stephen Warren wrote:
>>> 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.
>>
>>> 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. 

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.

Consider a SoC that has:

* An external PMIC, which the CPU has to communicate with to enable
power to all SoC components outside the CPU and a single I2C instance
dedicated to communicating with the PMIC.
* An on-SoC reset controller (for on-SoC) modules that resets other
on-SoC I2C controllers
* An on-SoC I2C controller which communicates with a GPIO expander
* One of the GPIOs on that expander is the reset GPIO for some other
device connected by I2C

What initcall levels are you going to use for all the drivers
(PM-related I2C, PMIC, on-SoC reset controller, secondary I2C
controller, GPIO expander IC, GPIO-reset "device", the final device
affected by the GPIO reset controller).

Now wonder whether that same order is going to be suitable for every
single other board. That's quite unlikely...

 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.

Yes, deferred probe will happen. If we want to solve that, I believe we
need a more generic solution specifically targeted at that, rather that
playing games with initcalls.

(It'd be nice if each DT node declared all its resources in a way the DT
core could parse them and determine the dependency graph itself, without
having to write code in all drivers to extract that information from DT.
This would need a standardized resource representation in DT, and as
such would mean throwing out all the bindings and starting again...
Perhaps some parallel set of properties could be invented to hint at the
order, but still fall back to deferred probe where that information was
missing or inaccurate.)

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Shawn Guo]

On Tue, Jul 16, 2013 at 09:45:43AM -0600, Stephen Warren wrote:
> 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.

Shawn

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Stephen Warren]

On 07/16/2013 09:02 PM, Shawn Guo wrote:
> On Tue, Jul 16, 2013 at 09:45:43AM -0600, Stephen Warren wrote:
>> 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.

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.

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Pavel Machek]

On Wed 2013-07-17 10:57:08, Stephen Warren wrote:
> On 07/16/2013 09:02 PM, Shawn Guo wrote:
> > On Tue, Jul 16, 2013 at 09:45:43AM -0600, Stephen Warren wrote:
> >> 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.
> 
> 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.

AFAIR, defered probing is known to be a "hack" by its authors. We
should still try to get initcall levels right...
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Stephen Warren]

On 07/17/2013 03:38 PM, Pavel Machek wrote:
> On Wed 2013-07-17 10:57:08, Stephen Warren wrote:
>> On 07/16/2013 09:02 PM, Shawn Guo wrote:
>>> On Tue, Jul 16, 2013 at 09:45:43AM -0600, Stephen Warren wrote:
>>>> 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.
>>
>> 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.
> 
> 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.

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Pavel Machek]

Hi!

> >>> 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.
> > 
> > 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 <grant.likely@secretlab.ca>
To:		      linux-kernel@vger.kernel.org
Subject:		[PATCH] drivercore: Add driver probe deferral
mechanism
Date:		Mon, 5 Mar 2012 08:47:41 -0700
Message-ID:
<1330962461-9061-1-git-send-email-grant.likely@secretlab.ca>
...

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.

							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Olof Johansson]

Hi,

On Thu, Jul 18, 2013 at 4:25 AM, Pavel Machek <pavel-ynQEQJNshbs@public.gmane.org> wrote:

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


-Olof

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Shawn Guo]

On Thu, Jul 18, 2013 at 11:45:29AM -0700, Olof Johansson wrote:
> Hi,
> 
> On Thu, Jul 18, 2013 at 4:25 AM, Pavel Machek <pavel-ynQEQJNshbs@public.gmane.org> wrote:
> 
> > 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 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.

Shawn

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Stephen Warren]

On 07/18/2013 09:23 PM, Shawn Guo wrote:
> On Thu, Jul 18, 2013 at 11:45:29AM -0700, Olof Johansson wrote:
...
>> 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.

If we fiddle with initcall levels to do this, (a) it won't always work;
what may be optimal for one board/SoC may not be optimal for another,
and with multi-platform kernels, we have to take a broader view, (b) the
only beneficiary is Linux; if we add information to DT for this, every
OS could benefit in the same way.

My view is that when one HW object depends on another (e.g. it's reset
by some other module, sends an interrupt to another module, is affected
by a pin controller module, etc.), that really is a HW issue, and hence
is appropriate to represent in device tree.

In fact, we already do represent the dependencies in device tree; e.g. a
foo-gpios/foo-reset/... property already indicates this. The issue is
that there's no standard naming/structure across all types of DT binding
(GPIO, reset, ...), and hence the only way to parse these dependencies
is to do exactly what deferred probe is doing; try probing each driver,
which then encompasses all device-specific and subsystem-specific naming
and DT structure in its actions, and if it fails, then defer probe.

If we had some explicit information re: dependencies in the DT, in an
entirely standard format, perhaps the device core or similar could parse
this and not even attempt probe until the relevant devices had completed
probe. If the information turned out to be bogus (e.g. loops, missing
drivers), we could always fall back to pure deferred probe later in the
boot sequence.

Or perhaps we should revisit whether DT node order should be defined as
having a guaranteed influence on probe order. We could enforce that the
source order has no influence, but then run a post-processing tool that
finds all the dependencies from the phandles in the DT, and then
re-sorts the DTB to get the probing order right? That would probably
require every single driver to be module_initcall, or to ignore driver
registration order and only probe devices in the order they appear in DT.

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Shawn Guo]

On Fri, Jul 19, 2013 at 09:45:20AM -0600, Stephen Warren wrote:
> 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.

I do not have a kernel supporting all those devices yet.

Shawn

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Philipp Zabel]

Am Donnerstag, den 18.07.2013, 11:45 -0700 schrieb Olof Johansson:
> Hi,
> 
> On Thu, Jul 18, 2013 at 4:25 AM, Pavel Machek <pavel@denx.de> wrote:
> 
> > 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.

Alright, I've backed out the arch_initcall change and resent the patch.
If you are fine with that, I'll send you a pull request for this and the
"reset: allow drivers to request probe deferral" patch.

regards
Philipp

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Grant Likely]

On Thu, Jul 18, 2013 at 4:25 AM, Pavel Machek <pavel-ynQEQJNshbs@public.gmane.org> wrote:
> Hi!
>
>> >>> 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.
>> >
>> > 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 <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> To:                   linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject:                [PATCH] drivercore: Add driver probe deferral
> mechanism
> Date:           Mon, 5 Mar 2012 08:47:41 -0700
> Message-ID:
> <1330962461-9061-1-git-send-email-grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> ...
>
> 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.

That wasn't the point I was trying to make in the above thread. My
point was that -EPROBE_DEFER is the best option that we currently have
to get rid of the initcall ordering madness. I'm all ears if someone
comes up with a simple and inexpensive alternative that can take into
account dependencies between devices.

g.

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Grant Likely]

On Wed, Jul 17, 2013 at 9:57 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 07/16/2013 09:02 PM, Shawn Guo wrote:
>> On Tue, Jul 16, 2013 at 09:45:43AM -0600, Stephen Warren wrote:
>>> 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.
>
> My understanding is that deferred probe was implemented specifically to
> avoid having to, or allowing, the use of initcall levels to determine
> probe order.

Correct. Futzing around with initcalls may optimize particular use
cases, but it isn't complete. I've been pushing for all drivers to use
module_initcall() unless there is a very strong reason to do
otherwise. Premature optimization (without measuring time consumed) is
not a strong justification.

g.

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Philipp Zabel]

Hi Stephen,

Am Montag, den 15.07.2013, 21:35 -0600 schrieb Stephen Warren:
> On 07/15/2013 07:50 PM, Shawn Guo wrote:
> > Hi Philipp,
> > 
> > On Thu, May 30, 2013 at 11:09:00AM +0200, Philipp Zabel wrote:
> >> 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 <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> Reviewed-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > 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 <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > 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.
> 
> 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.
> 
> > 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.

regards
Philipp

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Stephen Warren]

On 07/16/2013 03:59 AM, Philipp Zabel wrote:
...
> Deferred probing is fine, but it'd be nice to keep the probe deferral
> loops to a minimum where possible and/or reasonable.

I agree, but manually selecting initcalls for drivers is not the
solution. See my other email.

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Shawn Guo]

On Tue, Jul 16, 2013 at 09:50:42AM +0800, Shawn Guo wrote:
> Hi Philipp,
> 
> On Thu, May 30, 2013 at 11:09:00AM +0200, Philipp Zabel wrote:
> > 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 <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > Reviewed-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> 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.

Shawn

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

>From 822a0c4fb7c99b371844ba7e957affcaa8cb1cfe Mon Sep 17 00:00:00 2001
From: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
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 <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/reset/gpio-reset.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c
index 5d2515a..4f372f9 100644
--- a/drivers/reset/gpio-reset.c
+++ b/drivers/reset/gpio-reset.c
@@ -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)
-- 
1.7.9.5

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Philipp Zabel]

Am Dienstag, den 16.07.2013, 14:51 +0800 schrieb Shawn Guo:
> On Tue, Jul 16, 2013 at 09:50:42AM +0800, Shawn Guo wrote:
> > Hi Philipp,
> > 
> > On Thu, May 30, 2013 at 11:09:00AM +0200, Philipp Zabel wrote:
> > > 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 <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > Reviewed-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > 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.

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

regards
Philipp

> Shawn
> 
> ----8<---------------
> 
> From 822a0c4fb7c99b371844ba7e957affcaa8cb1cfe Mon Sep 17 00:00:00 2001
> From: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 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 <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/reset/gpio-reset.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c
> index 5d2515a..4f372f9 100644
> --- a/drivers/reset/gpio-reset.c
> +++ b/drivers/reset/gpio-reset.c
> @@ -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)

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Shawn Guo]

On Tue, Jul 16, 2013 at 11:51:19AM +0200, Philipp Zabel wrote:
> Looks good to me. I can squash it into the original patch and resend if
> you like.

Yes, please.  Thanks.

Shawn

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Stephen Warren]

On 07/16/2013 12:51 AM, Shawn Guo wrote:
> On Tue, Jul 16, 2013 at 09:50:42AM +0800, Shawn Guo wrote:
>> Hi Philipp,
>>
>> On Thu, May 30, 2013 at 11:09:00AM +0200, Philipp Zabel wrote:
>>> 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 <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>> Reviewed-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> 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.

> diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c

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

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Shawn Guo]

On Tue, Jul 16, 2013 at 09:47:02AM -0600, Stephen Warren wrote:
> > diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c
> 
> > -	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.

Ah, yes.  I was confused by the API gpio_cansleep().  Which API we
should call depends on the calling context.  And the context should
come from the gpio-reset client device driver.  IOW, we have no idea
what the context is in gpio-reset driver.  To start with something
simple, we may want to define the gpio-reset as a sleepable interface
and always call gpio_set_value_cansleep() in there.

Shawn

Re: [PATCH v8] reset: Add driver for gpio-controlled reset pins

[Philipp Zabel]

Am Dienstag, den 16.07.2013, 09:47 -0600 schrieb Stephen Warren:
> On 07/16/2013 12:51 AM, Shawn Guo wrote:
> > On Tue, Jul 16, 2013 at 09:50:42AM +0800, Shawn Guo wrote:
> >> Hi Philipp,
> >>
> >> On Thu, May 30, 2013 at 11:09:00AM +0200, Philipp Zabel wrote:
> >>> 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 <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >>> Reviewed-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >>
> >> 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.
> 
> > diff --git a/drivers/reset/gpio-reset.c b/drivers/reset/gpio-reset.c
> 
> > -	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.

In which case I'd say let's just call the _cansleep variant here
unconditionally and declare that reset_control_assert/deassert() may
sleep, just as reset_control_reset() has to anyway.

regards
Philipp