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

[PATCH v10] 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 v9:
 - Use gpio_set_value_cansleep unconditionally. reset_controller_assert/deassert
   should not be called from atomic context.
---
 .../devicetree/bindings/reset/gpio-reset.txt       |  35 ++++
 drivers/reset/Kconfig                              |  11 ++
 drivers/reset/Makefile                             |   1 +
 drivers/reset/gpio-reset.c                         | 184 +++++++++++++++++++++
 4 files changed, 231 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..3049b22
--- /dev/null
+++ b/drivers/reset/gpio-reset.c
@@ -0,0 +1,184 @@
+/*
+ * 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_cansleep(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),
+	},
+};
+
+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");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:gpio-reset");
+MODULE_DEVICE_TABLE(of, gpio_reset_dt_ids);
-- 
1.8.3.2

[PATCH v10] 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@pengutronix.de>
Reviewed-by: Stephen Warren <swarren@nvidia.com>
---
Changes since v9:
 - Use gpio_set_value_cansleep unconditionally. reset_controller_assert/deassert
   should not be called from atomic context.
---
 .../devicetree/bindings/reset/gpio-reset.txt       |  35 ++++
 drivers/reset/Kconfig                              |  11 ++
 drivers/reset/Makefile                             |   1 +
 drivers/reset/gpio-reset.c                         | 184 +++++++++++++++++++++
 4 files changed, 231 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 at 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..3049b22
--- /dev/null
+++ b/drivers/reset/gpio-reset.c
@@ -0,0 +1,184 @@
+/*
+ * 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_cansleep(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),
+	},
+};
+
+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@pengutronix.de>");
+MODULE_DESCRIPTION("gpio reset controller");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:gpio-reset");
+MODULE_DEVICE_TABLE(of, gpio_reset_dt_ids);
-- 
1.8.3.2

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

[Shawn Guo]

On Thu, Jul 18, 2013 at 11:26:26AM +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>

Tested-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

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

[Shawn Guo]

On Thu, Jul 18, 2013 at 11:26:26AM +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@pengutronix.de>
> Reviewed-by: Stephen Warren <swarren@nvidia.com>

Tested-by: Shawn Guo <shawn.guo@linaro.org>

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

[Mark Rutland]

On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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@pengutronix.de>
> Reviewed-by: Stephen Warren <swarren@nvidia.com>
> ---
> Changes since v9:
>  - Use gpio_set_value_cansleep unconditionally. reset_controller_assert/deassert
>    should not be called from atomic context.
> ---
>  .../devicetree/bindings/reset/gpio-reset.txt       |  35 ++++
>  drivers/reset/Kconfig                              |  11 ++
>  drivers/reset/Makefile                             |   1 +
>  drivers/reset/gpio-reset.c                         | 184 +++++++++++++++++++++
>  4 files changed, 231 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 */
> +};

I don't like the approach here. The reset GPIO line is not a device in
itself, and surely the way in which it must be toggled to reset a device
is a property of that device, not the GPIO. We're leaking a Linux
internal abstraction rather than describing teh hardware.

I think thie linkage of the gpio would be better described on the node
for the device with the reset input, in a binding-specific property for
the device (e.g. names for the specific input line on the device):

I'd imagine the delay required would be fixed for a device (or possibly
influenced by clock inputs), and can either be knwon by the driver
without dt information, or discovered elsewhere (e.g. dervied from the
rates of clock inputs). We shuold be able to derive that from the
compatible string in most cases.

I think this should look more like the below:

/* Device with nRESET pin connected to GPIO5_0 */
sii902x@39 {
	/* named for the actual input line */
	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
	/* 
	 * If there's some configurable property relating to the reset
	 * line, we can describe it
	 */
	vendor,some-optional-reset-gpio-property;
	...
};

If we want a separate device in the Linux driver model to abstract this,
that's fine, but it should not be described in the dt. The device driver
can call some generic helpers to handle creating said reset device in
the driver model.

Thanks,
Mark.

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

[Mark Rutland]

On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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@pengutronix.de>
> Reviewed-by: Stephen Warren <swarren@nvidia.com>
> ---
> Changes since v9:
>  - Use gpio_set_value_cansleep unconditionally. reset_controller_assert/deassert
>    should not be called from atomic context.
> ---
>  .../devicetree/bindings/reset/gpio-reset.txt       |  35 ++++
>  drivers/reset/Kconfig                              |  11 ++
>  drivers/reset/Makefile                             |   1 +
>  drivers/reset/gpio-reset.c                         | 184 +++++++++++++++++++++
>  4 files changed, 231 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 at 39 {
> +	/* ... */
> +	resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */
> +};

I don't like the approach here. The reset GPIO line is not a device in
itself, and surely the way in which it must be toggled to reset a device
is a property of that device, not the GPIO. We're leaking a Linux
internal abstraction rather than describing teh hardware.

I think thie linkage of the gpio would be better described on the node
for the device with the reset input, in a binding-specific property for
the device (e.g. names for the specific input line on the device):

I'd imagine the delay required would be fixed for a device (or possibly
influenced by clock inputs), and can either be knwon by the driver
without dt information, or discovered elsewhere (e.g. dervied from the
rates of clock inputs). We shuold be able to derive that from the
compatible string in most cases.

I think this should look more like the below:

/* Device with nRESET pin connected to GPIO5_0 */
sii902x at 39 {
	/* named for the actual input line */
	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
	/* 
	 * If there's some configurable property relating to the reset
	 * line, we can describe it
	 */
	vendor,some-optional-reset-gpio-property;
	...
};

If we want a separate device in the Linux driver model to abstract this,
that's fine, but it should not be described in the dt. The device driver
can call some generic helpers to handle creating said reset device in
the driver model.

Thanks,
Mark.

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

[Stephen Warren]

On 08/02/2013 03:28 AM, Mark Rutland wrote:
> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.

>> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt

>> +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 */
>> +};
> 
> I don't like the approach here. The reset GPIO line is not a device in
> itself, and surely the way in which it must be toggled to reset a device
> is a property of that device, not the GPIO. We're leaking a Linux
> internal abstraction rather than describing the hardware.

At first, I was going to say I disagree completely, but I do see your point.

I don't think that having a separate reset controller concept in DT, and
having a GPIO implementation of that, is purely exposing Linux concepts
in the DT. It's just one way of looking at the HW. As you say, another
is that any external chip that needs reset has a GPIO input.

The history here is that we have to concept of IP modules inside a chip
that can be reset, and that reset is driven by some external entity to
the IP block, and hence there's a concept of a "reset controller" that
drives that reset. That enables arbitrary different implementations to
cover the case of the IP block being dropped into different SoCs with
different ways of resetting it.

Extend that same concept to external chips that happen to have GPIOs
driving the chip's reset inputs, and you get this GPIO reset controller
binding.

On the surface, any external chip is going to be reset by a GPIO, and
hence that should be represented by a property in that external chip's
DT node, just as you say.

But what if the reset pin is /not/ hooked up to a GPIO? What if there's
a dedicated "reset" HW device that drives the reset input, and all it
can do is pulse the reset, not maintain it at some static level, and
hence that signal can't be represented as a GPIO? Or what if the core of
the external chip gets integrated into an SoC, which has a reset
controller rather than a GPIO input? Of course, I have no idea how
likely this would be.

To cover those cases, continuing to abstract the reset input
semantically as a reset input, rather than as a plain GPIO, might make
sense.

> I think thie linkage of the gpio would be better described on the node
> for the device with the reset input, in a binding-specific property for
> the device (e.g. names for the specific input line on the device):
> 
> I'd imagine the delay required would be fixed for a device (or possibly
> influenced by clock inputs), and can either be knwon by the driver
> without dt information, or discovered elsewhere (e.g. dervied from the
> rates of clock inputs). We shuold be able to derive that from the
> compatible string in most cases.
> 
> I think this should look more like the below:
> 
> /* Device with nRESET pin connected to GPIO5_0 */
> sii902x@39 {
> 	/* named for the actual input line */
> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> 	/* 
> 	 * If there's some configurable property relating to the reset
> 	 * line, we can describe it
> 	 */
> 	vendor,some-optional-reset-gpio-property;
> 	...
> };

If we decide to ignore the case where the reset signal isn't driven by a
GPIO, yes, the above binding does look better.

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

[Stephen Warren]

On 08/02/2013 03:28 AM, Mark Rutland wrote:
> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.

>> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt

>> +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 at 39 {
>> +	/* ... */
>> +	resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */
>> +};
> 
> I don't like the approach here. The reset GPIO line is not a device in
> itself, and surely the way in which it must be toggled to reset a device
> is a property of that device, not the GPIO. We're leaking a Linux
> internal abstraction rather than describing the hardware.

At first, I was going to say I disagree completely, but I do see your point.

I don't think that having a separate reset controller concept in DT, and
having a GPIO implementation of that, is purely exposing Linux concepts
in the DT. It's just one way of looking at the HW. As you say, another
is that any external chip that needs reset has a GPIO input.

The history here is that we have to concept of IP modules inside a chip
that can be reset, and that reset is driven by some external entity to
the IP block, and hence there's a concept of a "reset controller" that
drives that reset. That enables arbitrary different implementations to
cover the case of the IP block being dropped into different SoCs with
different ways of resetting it.

Extend that same concept to external chips that happen to have GPIOs
driving the chip's reset inputs, and you get this GPIO reset controller
binding.

On the surface, any external chip is going to be reset by a GPIO, and
hence that should be represented by a property in that external chip's
DT node, just as you say.

But what if the reset pin is /not/ hooked up to a GPIO? What if there's
a dedicated "reset" HW device that drives the reset input, and all it
can do is pulse the reset, not maintain it at some static level, and
hence that signal can't be represented as a GPIO? Or what if the core of
the external chip gets integrated into an SoC, which has a reset
controller rather than a GPIO input? Of course, I have no idea how
likely this would be.

To cover those cases, continuing to abstract the reset input
semantically as a reset input, rather than as a plain GPIO, might make
sense.

> I think thie linkage of the gpio would be better described on the node
> for the device with the reset input, in a binding-specific property for
> the device (e.g. names for the specific input line on the device):
> 
> I'd imagine the delay required would be fixed for a device (or possibly
> influenced by clock inputs), and can either be knwon by the driver
> without dt information, or discovered elsewhere (e.g. dervied from the
> rates of clock inputs). We shuold be able to derive that from the
> compatible string in most cases.
> 
> I think this should look more like the below:
> 
> /* Device with nRESET pin connected to GPIO5_0 */
> sii902x at 39 {
> 	/* named for the actual input line */
> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> 	/* 
> 	 * If there's some configurable property relating to the reset
> 	 * line, we can describe it
> 	 */
> 	vendor,some-optional-reset-gpio-property;
> 	...
> };

If we decide to ignore the case where the reset signal isn't driven by a
GPIO, yes, the above binding does look better.

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

[Philipp Zabel]

Hi Mark,

Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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@pengutronix.de>
> > Reviewed-by: Stephen Warren <swarren@nvidia.com>
> > ---
> > Changes since v9:
> >  - Use gpio_set_value_cansleep unconditionally. reset_controller_assert/deassert
> >    should not be called from atomic context.
> > ---
> >  .../devicetree/bindings/reset/gpio-reset.txt       |  35 ++++
> >  drivers/reset/Kconfig                              |  11 ++
> >  drivers/reset/Makefile                             |   1 +
> >  drivers/reset/gpio-reset.c                         | 184 +++++++++++++++++++++
> >  4 files changed, 231 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 */
> > +};
> 
> I don't like the approach here. The reset GPIO line is not a device in
> itself, and surely the way in which it must be toggled to reset a device
> is a property of that device, not the GPIO. We're leaking a Linux
> internal abstraction rather than describing teh hardware.

you are right, this results from the reset bindings being designed for
actual hardware reset controllers that appeared in several SoCs.
Initially I've sent the GPIO reset driver only as an example.

> I think thie linkage of the gpio would be better described on the node
> for the device with the reset input, in a binding-specific property for
> the device (e.g. names for the specific input line on the device):
> 
> I'd imagine the delay required would be fixed for a device (or possibly
> influenced by clock inputs), and can either be knwon by the driver
> without dt information, or discovered elsewhere (e.g. dervied from the
> rates of clock inputs). We shuold be able to derive that from the
> compatible string in most cases.
> 
> I think this should look more like the below:
> 
> /* Device with nRESET pin connected to GPIO5_0 */
> sii902x@39 {
> 	/* named for the actual input line */
> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> 	/* 
> 	 * If there's some configurable property relating to the reset
> 	 * line, we can describe it
> 	 */
> 	vendor,some-optional-reset-gpio-property;
> 	...
> };

I don't like the arbitrary name, as that makes it difficult to handle
this in an automated way. In this case I'd prefer to use 'reset-gpios'
and optionally 'reset-gpio-names' analogous to how clocks and interrupts
(and resets) are handled. The framework itself could then also check the
reset-gpios property and create the gpio reset controllers on the fly:

sii902x@39 {
	reset-gpios <&gpio5 0 GPIO_ACTIVE_LOW>;
	reset-gpio-names = "nreset";
	reset-delay-us = <10000>;
	initially-in-reset;
};

But this reintroduces the (currently theoretical) problem of how to
express delays and initially-in-reset information for multiple gpio
resets on a single device if that information cannot be determined by
the driver automatically.

If there is a fixed delay (or a delay calculated from a known clock), we
can just extend the API:
	device_reset_usleep(dev, 1000);
instead of currently:
	device_reset(dev);

> If we want a separate device in the Linux driver model to abstract this,
> that's fine, but it should not be described in the dt. The device driver
> can call some generic helpers to handle creating said reset device in
> the driver model.

regards
Philipp

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

[Philipp Zabel]

Hi Mark,

Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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@pengutronix.de>
> > Reviewed-by: Stephen Warren <swarren@nvidia.com>
> > ---
> > Changes since v9:
> >  - Use gpio_set_value_cansleep unconditionally. reset_controller_assert/deassert
> >    should not be called from atomic context.
> > ---
> >  .../devicetree/bindings/reset/gpio-reset.txt       |  35 ++++
> >  drivers/reset/Kconfig                              |  11 ++
> >  drivers/reset/Makefile                             |   1 +
> >  drivers/reset/gpio-reset.c                         | 184 +++++++++++++++++++++
> >  4 files changed, 231 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 at 39 {
> > +	/* ... */
> > +	resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */
> > +};
> 
> I don't like the approach here. The reset GPIO line is not a device in
> itself, and surely the way in which it must be toggled to reset a device
> is a property of that device, not the GPIO. We're leaking a Linux
> internal abstraction rather than describing teh hardware.

you are right, this results from the reset bindings being designed for
actual hardware reset controllers that appeared in several SoCs.
Initially I've sent the GPIO reset driver only as an example.

> I think thie linkage of the gpio would be better described on the node
> for the device with the reset input, in a binding-specific property for
> the device (e.g. names for the specific input line on the device):
> 
> I'd imagine the delay required would be fixed for a device (or possibly
> influenced by clock inputs), and can either be knwon by the driver
> without dt information, or discovered elsewhere (e.g. dervied from the
> rates of clock inputs). We shuold be able to derive that from the
> compatible string in most cases.
> 
> I think this should look more like the below:
> 
> /* Device with nRESET pin connected to GPIO5_0 */
> sii902x at 39 {
> 	/* named for the actual input line */
> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> 	/* 
> 	 * If there's some configurable property relating to the reset
> 	 * line, we can describe it
> 	 */
> 	vendor,some-optional-reset-gpio-property;
> 	...
> };

I don't like the arbitrary name, as that makes it difficult to handle
this in an automated way. In this case I'd prefer to use 'reset-gpios'
and optionally 'reset-gpio-names' analogous to how clocks and interrupts
(and resets) are handled. The framework itself could then also check the
reset-gpios property and create the gpio reset controllers on the fly:

sii902x at 39 {
	reset-gpios <&gpio5 0 GPIO_ACTIVE_LOW>;
	reset-gpio-names = "nreset";
	reset-delay-us = <10000>;
	initially-in-reset;
};

But this reintroduces the (currently theoretical) problem of how to
express delays and initially-in-reset information for multiple gpio
resets on a single device if that information cannot be determined by
the driver automatically.

If there is a fixed delay (or a delay calculated from a known clock), we
can just extend the API:
	device_reset_usleep(dev, 1000);
instead of currently:
	device_reset(dev);

> If we want a separate device in the Linux driver model to abstract this,
> that's fine, but it should not be described in the dt. The device driver
> can call some generic helpers to handle creating said reset device in
> the driver model.

regards
Philipp

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

[Mark Rutland]

On Fri, Aug 02, 2013 at 09:09:35PM +0100, Stephen Warren wrote:
> On 08/02/2013 03:28 AM, Mark Rutland wrote:
> > On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
> 
> >> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt
> 
> >> +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 */
> >> +};
> > 
> > I don't like the approach here. The reset GPIO line is not a device in
> > itself, and surely the way in which it must be toggled to reset a device
> > is a property of that device, not the GPIO. We're leaking a Linux
> > internal abstraction rather than describing the hardware.
> 
> At first, I was going to say I disagree completely, but I do see your point.
> 
> I don't think that having a separate reset controller concept in DT, and
> having a GPIO implementation of that, is purely exposing Linux concepts
> in the DT. It's just one way of looking at the HW. As you say, another
> is that any external chip that needs reset has a GPIO input.
> 
> The history here is that we have to concept of IP modules inside a chip
> that can be reset, and that reset is driven by some external entity to
> the IP block, and hence there's a concept of a "reset controller" that
> drives that reset. That enables arbitrary different implementations to
> cover the case of the IP block being dropped into different SoCs with
> different ways of resetting it.
> 
> Extend that same concept to external chips that happen to have GPIOs
> driving the chip's reset inputs, and you get this GPIO reset controller
> binding.
> 
> On the surface, any external chip is going to be reset by a GPIO, and
> hence that should be represented by a property in that external chip's
> DT node, just as you say.
> 
> But what if the reset pin is /not/ hooked up to a GPIO? What if there's
> a dedicated "reset" HW device that drives the reset input, and all it
> can do is pulse the reset, not maintain it at some static level, and
> hence that signal can't be represented as a GPIO? Or what if the core of
> the external chip gets integrated into an SoC, which has a reset
> controller rather than a GPIO input? Of course, I have no idea how
> likely this would be.
> 
> To cover those cases, continuing to abstract the reset input
> semantically as a reset input, rather than as a plain GPIO, might make
> sense.

That does sound reasonable. Do we have any of the above cases described
in any existing DTs?

If I've not misunderstood, there seem to be a lot of *reset-gpio*
properties Documented (e.g. nvidia,phy-reset-gpio) which look like what
I was suggesting (though perhaps that's a different meaning of reset,
I'm not that familiar with PHYs). Given it's a common pattern already, I
think it would make sense to allow for that style of binding (in
addition to supporting more complex reset devices as required).

The reset bindings (Documentation/devicetree/bindings/reset/reset.txt)
seem to suggest that both are reasonable.

> 
> > I think thie linkage of the gpio would be better described on the node
> > for the device with the reset input, in a binding-specific property for
> > the device (e.g. names for the specific input line on the device):
> > 
> > I'd imagine the delay required would be fixed for a device (or possibly
> > influenced by clock inputs), and can either be knwon by the driver
> > without dt information, or discovered elsewhere (e.g. dervied from the
> > rates of clock inputs). We shuold be able to derive that from the
> > compatible string in most cases.
> > 
> > I think this should look more like the below:
> > 
> > /* Device with nRESET pin connected to GPIO5_0 */
> > sii902x@39 {
> > 	/* named for the actual input line */
> > 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> > 	/* 
> > 	 * If there's some configurable property relating to the reset
> > 	 * line, we can describe it
> > 	 */
> > 	vendor,some-optional-reset-gpio-property;
> > 	...
> > };
> 
> If we decide to ignore the case where the reset signal isn't driven by a
> GPIO, yes, the above binding does look better.
> 

For the GPIO case, I'd prefer to describe the GPIO input directly. I'm
not opposed to also supporting the description of more complex reset
devices. I believe it's feasible to support both?

Thanks,
Mark.

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

[Mark Rutland]

On Fri, Aug 02, 2013 at 09:09:35PM +0100, Stephen Warren wrote:
> On 08/02/2013 03:28 AM, Mark Rutland wrote:
> > On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
> 
> >> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt
> 
> >> +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 at 39 {
> >> +	/* ... */
> >> +	resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */
> >> +};
> > 
> > I don't like the approach here. The reset GPIO line is not a device in
> > itself, and surely the way in which it must be toggled to reset a device
> > is a property of that device, not the GPIO. We're leaking a Linux
> > internal abstraction rather than describing the hardware.
> 
> At first, I was going to say I disagree completely, but I do see your point.
> 
> I don't think that having a separate reset controller concept in DT, and
> having a GPIO implementation of that, is purely exposing Linux concepts
> in the DT. It's just one way of looking at the HW. As you say, another
> is that any external chip that needs reset has a GPIO input.
> 
> The history here is that we have to concept of IP modules inside a chip
> that can be reset, and that reset is driven by some external entity to
> the IP block, and hence there's a concept of a "reset controller" that
> drives that reset. That enables arbitrary different implementations to
> cover the case of the IP block being dropped into different SoCs with
> different ways of resetting it.
> 
> Extend that same concept to external chips that happen to have GPIOs
> driving the chip's reset inputs, and you get this GPIO reset controller
> binding.
> 
> On the surface, any external chip is going to be reset by a GPIO, and
> hence that should be represented by a property in that external chip's
> DT node, just as you say.
> 
> But what if the reset pin is /not/ hooked up to a GPIO? What if there's
> a dedicated "reset" HW device that drives the reset input, and all it
> can do is pulse the reset, not maintain it at some static level, and
> hence that signal can't be represented as a GPIO? Or what if the core of
> the external chip gets integrated into an SoC, which has a reset
> controller rather than a GPIO input? Of course, I have no idea how
> likely this would be.
> 
> To cover those cases, continuing to abstract the reset input
> semantically as a reset input, rather than as a plain GPIO, might make
> sense.

That does sound reasonable. Do we have any of the above cases described
in any existing DTs?

If I've not misunderstood, there seem to be a lot of *reset-gpio*
properties Documented (e.g. nvidia,phy-reset-gpio) which look like what
I was suggesting (though perhaps that's a different meaning of reset,
I'm not that familiar with PHYs). Given it's a common pattern already, I
think it would make sense to allow for that style of binding (in
addition to supporting more complex reset devices as required).

The reset bindings (Documentation/devicetree/bindings/reset/reset.txt)
seem to suggest that both are reasonable.

> 
> > I think thie linkage of the gpio would be better described on the node
> > for the device with the reset input, in a binding-specific property for
> > the device (e.g. names for the specific input line on the device):
> > 
> > I'd imagine the delay required would be fixed for a device (or possibly
> > influenced by clock inputs), and can either be knwon by the driver
> > without dt information, or discovered elsewhere (e.g. dervied from the
> > rates of clock inputs). We shuold be able to derive that from the
> > compatible string in most cases.
> > 
> > I think this should look more like the below:
> > 
> > /* Device with nRESET pin connected to GPIO5_0 */
> > sii902x at 39 {
> > 	/* named for the actual input line */
> > 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> > 	/* 
> > 	 * If there's some configurable property relating to the reset
> > 	 * line, we can describe it
> > 	 */
> > 	vendor,some-optional-reset-gpio-property;
> > 	...
> > };
> 
> If we decide to ignore the case where the reset signal isn't driven by a
> GPIO, yes, the above binding does look better.
> 

For the GPIO case, I'd prefer to describe the GPIO input directly. I'm
not opposed to also supporting the description of more complex reset
devices. I believe it's feasible to support both?

Thanks,
Mark.

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

[Mark Rutland]

On Mon, Aug 05, 2013 at 08:32:16AM +0100, Philipp Zabel wrote:
> Hi Mark,

Hi Philipp,

> 
> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
> > On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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@pengutronix.de>
> > > Reviewed-by: Stephen Warren <swarren@nvidia.com>
> > > ---
> > > Changes since v9:
> > >  - Use gpio_set_value_cansleep unconditionally. reset_controller_assert/deassert
> > >    should not be called from atomic context.
> > > ---
> > >  .../devicetree/bindings/reset/gpio-reset.txt       |  35 ++++
> > >  drivers/reset/Kconfig                              |  11 ++
> > >  drivers/reset/Makefile                             |   1 +
> > >  drivers/reset/gpio-reset.c                         | 184 +++++++++++++++++++++
> > >  4 files changed, 231 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 */
> > > +};
> > 
> > I don't like the approach here. The reset GPIO line is not a device in
> > itself, and surely the way in which it must be toggled to reset a device
> > is a property of that device, not the GPIO. We're leaking a Linux
> > internal abstraction rather than describing teh hardware.
> 
> you are right, this results from the reset bindings being designed for
> actual hardware reset controllers that appeared in several SoCs.
> Initially I've sent the GPIO reset driver only as an example.
> 
> > I think thie linkage of the gpio would be better described on the node
> > for the device with the reset input, in a binding-specific property for
> > the device (e.g. names for the specific input line on the device):
> > 
> > I'd imagine the delay required would be fixed for a device (or possibly
> > influenced by clock inputs), and can either be knwon by the driver
> > without dt information, or discovered elsewhere (e.g. dervied from the
> > rates of clock inputs). We shuold be able to derive that from the
> > compatible string in most cases.
> > 
> > I think this should look more like the below:
> > 
> > /* Device with nRESET pin connected to GPIO5_0 */
> > sii902x@39 {
> > 	/* named for the actual input line */
> > 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> > 	/* 
> > 	 * If there's some configurable property relating to the reset
> > 	 * line, we can describe it
> > 	 */
> > 	vendor,some-optional-reset-gpio-property;
> > 	...
> > };
> 
> I don't like the arbitrary name, as that makes it difficult to handle
> this in an automated way. In this case I'd prefer to use 'reset-gpios'
> and optionally 'reset-gpio-names' analogous to how clocks and interrupts
> (and resets) are handled. The framework itself could then also check the
> reset-gpios property and create the gpio reset controllers on the fly:

I'm not sure about the structure you suggest. The main reason we'd want
to tell apart multiple reset GPIOs (via reset-gpio-names) would be that
the device needs to have said GPIOs poked in a device-specific manner
(i.e. with some ordering and timing constraints, possibly only a subset
of the GPIOs needing to be poked to reset components of the device).

Given that, I'm not sure what separating the GPIOs out into a
reset-gpios property buys us, as the driver still needs to handle them
(at least to provide some additional information to device reset
framework code).

> 
> sii902x@39 {
> 	reset-gpios <&gpio5 0 GPIO_ACTIVE_LOW>;
> 	reset-gpio-names = "nreset";
> 	reset-delay-us = <10000>;
> 	initially-in-reset;
> };
> 
> But this reintroduces the (currently theoretical) problem of how to
> express delays and initially-in-reset information for multiple gpio
> resets on a single device if that information cannot be determined by
> the driver automatically.
> 
> If there is a fixed delay (or a delay calculated from a known clock), we
> can just extend the API:
> 	device_reset_usleep(dev, 1000);
> instead of currently:
> 	device_reset(dev);

I was working on the assumption that any required delay would be a
function of the devices input clocks (or fixed, if the device has some
fixed rate internal clock). If that's true, the API extension you
suggest makes sense to me.

I'm not sure about initially-in-reset. Is it not possible for the driver
to poke the GPIO controller to ensure the GPIOs are in the state
required?

Thanks,
Mark.

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

[Mark Rutland]

On Mon, Aug 05, 2013 at 08:32:16AM +0100, Philipp Zabel wrote:
> Hi Mark,

Hi Philipp,

> 
> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
> > On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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@pengutronix.de>
> > > Reviewed-by: Stephen Warren <swarren@nvidia.com>
> > > ---
> > > Changes since v9:
> > >  - Use gpio_set_value_cansleep unconditionally. reset_controller_assert/deassert
> > >    should not be called from atomic context.
> > > ---
> > >  .../devicetree/bindings/reset/gpio-reset.txt       |  35 ++++
> > >  drivers/reset/Kconfig                              |  11 ++
> > >  drivers/reset/Makefile                             |   1 +
> > >  drivers/reset/gpio-reset.c                         | 184 +++++++++++++++++++++
> > >  4 files changed, 231 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 at 39 {
> > > +	/* ... */
> > > +	resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */
> > > +};
> > 
> > I don't like the approach here. The reset GPIO line is not a device in
> > itself, and surely the way in which it must be toggled to reset a device
> > is a property of that device, not the GPIO. We're leaking a Linux
> > internal abstraction rather than describing teh hardware.
> 
> you are right, this results from the reset bindings being designed for
> actual hardware reset controllers that appeared in several SoCs.
> Initially I've sent the GPIO reset driver only as an example.
> 
> > I think thie linkage of the gpio would be better described on the node
> > for the device with the reset input, in a binding-specific property for
> > the device (e.g. names for the specific input line on the device):
> > 
> > I'd imagine the delay required would be fixed for a device (or possibly
> > influenced by clock inputs), and can either be knwon by the driver
> > without dt information, or discovered elsewhere (e.g. dervied from the
> > rates of clock inputs). We shuold be able to derive that from the
> > compatible string in most cases.
> > 
> > I think this should look more like the below:
> > 
> > /* Device with nRESET pin connected to GPIO5_0 */
> > sii902x at 39 {
> > 	/* named for the actual input line */
> > 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> > 	/* 
> > 	 * If there's some configurable property relating to the reset
> > 	 * line, we can describe it
> > 	 */
> > 	vendor,some-optional-reset-gpio-property;
> > 	...
> > };
> 
> I don't like the arbitrary name, as that makes it difficult to handle
> this in an automated way. In this case I'd prefer to use 'reset-gpios'
> and optionally 'reset-gpio-names' analogous to how clocks and interrupts
> (and resets) are handled. The framework itself could then also check the
> reset-gpios property and create the gpio reset controllers on the fly:

I'm not sure about the structure you suggest. The main reason we'd want
to tell apart multiple reset GPIOs (via reset-gpio-names) would be that
the device needs to have said GPIOs poked in a device-specific manner
(i.e. with some ordering and timing constraints, possibly only a subset
of the GPIOs needing to be poked to reset components of the device).

Given that, I'm not sure what separating the GPIOs out into a
reset-gpios property buys us, as the driver still needs to handle them
(at least to provide some additional information to device reset
framework code).

> 
> sii902x at 39 {
> 	reset-gpios <&gpio5 0 GPIO_ACTIVE_LOW>;
> 	reset-gpio-names = "nreset";
> 	reset-delay-us = <10000>;
> 	initially-in-reset;
> };
> 
> But this reintroduces the (currently theoretical) problem of how to
> express delays and initially-in-reset information for multiple gpio
> resets on a single device if that information cannot be determined by
> the driver automatically.
> 
> If there is a fixed delay (or a delay calculated from a known clock), we
> can just extend the API:
> 	device_reset_usleep(dev, 1000);
> instead of currently:
> 	device_reset(dev);

I was working on the assumption that any required delay would be a
function of the devices input clocks (or fixed, if the device has some
fixed rate internal clock). If that's true, the API extension you
suggest makes sense to me.

I'm not sure about initially-in-reset. Is it not possible for the driver
to poke the GPIO controller to ensure the GPIOs are in the state
required?

Thanks,
Mark.

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

[Stephen Warren]

On 08/05/2013 09:13 AM, Mark Rutland wrote:
> On Fri, Aug 02, 2013 at 09:09:35PM +0100, Stephen Warren wrote:
>> On 08/02/2013 03:28 AM, Mark Rutland wrote:
>>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
>>
>>>> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt
>>
>>>> +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 */
>>>> +};
>>>
>>> I don't like the approach here. The reset GPIO line is not a device in
>>> itself, and surely the way in which it must be toggled to reset a device
>>> is a property of that device, not the GPIO. We're leaking a Linux
>>> internal abstraction rather than describing the hardware.
>>
>> At first, I was going to say I disagree completely, but I do see your point.
>>
>> I don't think that having a separate reset controller concept in DT, and
>> having a GPIO implementation of that, is purely exposing Linux concepts
>> in the DT. It's just one way of looking at the HW. As you say, another
>> is that any external chip that needs reset has a GPIO input.
>>
>> The history here is that we have to concept of IP modules inside a chip
>> that can be reset, and that reset is driven by some external entity to
>> the IP block, and hence there's a concept of a "reset controller" that
>> drives that reset. That enables arbitrary different implementations to
>> cover the case of the IP block being dropped into different SoCs with
>> different ways of resetting it.
>>
>> Extend that same concept to external chips that happen to have GPIOs
>> driving the chip's reset inputs, and you get this GPIO reset controller
>> binding.
>>
>> On the surface, any external chip is going to be reset by a GPIO, and
>> hence that should be represented by a property in that external chip's
>> DT node, just as you say.
>>
>> But what if the reset pin is /not/ hooked up to a GPIO? What if there's
>> a dedicated "reset" HW device that drives the reset input, and all it
>> can do is pulse the reset, not maintain it at some static level, and
>> hence that signal can't be represented as a GPIO? Or what if the core of
>> the external chip gets integrated into an SoC, which has a reset
>> controller rather than a GPIO input? Of course, I have no idea how
>> likely this would be.
>>
>> To cover those cases, continuing to abstract the reset input
>> semantically as a reset input, rather than as a plain GPIO, might make
>> sense.
> 
> That does sound reasonable. Do we have any of the above cases described
> in any existing DTs?

I haven't explicitly checked, so I don't know. My argument was
admittedly entirely theoretical.

> If I've not misunderstood, there seem to be a lot of *reset-gpio*
> properties Documented (e.g. nvidia,phy-reset-gpio) which look like what
> I was suggesting (though perhaps that's a different meaning of reset,
> I'm not that familiar with PHYs).

I think that nvidia,phy-reset-gpio is exactly what we're talking about here.

> Given it's a common pattern already, I
> think it would make sense to allow for that style of binding (in
> addition to supporting more complex reset devices as required).

Yes, certainly. If for nothing else than "legacy" bindings:-) But I
think explicit properties are a perfectly reasonable approach even going
forward.

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

[Stephen Warren]

On 08/05/2013 09:13 AM, Mark Rutland wrote:
> On Fri, Aug 02, 2013 at 09:09:35PM +0100, Stephen Warren wrote:
>> On 08/02/2013 03:28 AM, Mark Rutland wrote:
>>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
>>
>>>> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.txt b/Documentation/devicetree/bindings/reset/gpio-reset.txt
>>
>>>> +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 at 39 {
>>>> +	/* ... */
>>>> +	resets = <&sii902x_reset>; /* active-low GPIO5_0, 10 ms delay */
>>>> +};
>>>
>>> I don't like the approach here. The reset GPIO line is not a device in
>>> itself, and surely the way in which it must be toggled to reset a device
>>> is a property of that device, not the GPIO. We're leaking a Linux
>>> internal abstraction rather than describing the hardware.
>>
>> At first, I was going to say I disagree completely, but I do see your point.
>>
>> I don't think that having a separate reset controller concept in DT, and
>> having a GPIO implementation of that, is purely exposing Linux concepts
>> in the DT. It's just one way of looking at the HW. As you say, another
>> is that any external chip that needs reset has a GPIO input.
>>
>> The history here is that we have to concept of IP modules inside a chip
>> that can be reset, and that reset is driven by some external entity to
>> the IP block, and hence there's a concept of a "reset controller" that
>> drives that reset. That enables arbitrary different implementations to
>> cover the case of the IP block being dropped into different SoCs with
>> different ways of resetting it.
>>
>> Extend that same concept to external chips that happen to have GPIOs
>> driving the chip's reset inputs, and you get this GPIO reset controller
>> binding.
>>
>> On the surface, any external chip is going to be reset by a GPIO, and
>> hence that should be represented by a property in that external chip's
>> DT node, just as you say.
>>
>> But what if the reset pin is /not/ hooked up to a GPIO? What if there's
>> a dedicated "reset" HW device that drives the reset input, and all it
>> can do is pulse the reset, not maintain it at some static level, and
>> hence that signal can't be represented as a GPIO? Or what if the core of
>> the external chip gets integrated into an SoC, which has a reset
>> controller rather than a GPIO input? Of course, I have no idea how
>> likely this would be.
>>
>> To cover those cases, continuing to abstract the reset input
>> semantically as a reset input, rather than as a plain GPIO, might make
>> sense.
> 
> That does sound reasonable. Do we have any of the above cases described
> in any existing DTs?

I haven't explicitly checked, so I don't know. My argument was
admittedly entirely theoretical.

> If I've not misunderstood, there seem to be a lot of *reset-gpio*
> properties Documented (e.g. nvidia,phy-reset-gpio) which look like what
> I was suggesting (though perhaps that's a different meaning of reset,
> I'm not that familiar with PHYs).

I think that nvidia,phy-reset-gpio is exactly what we're talking about here.

> Given it's a common pattern already, I
> think it would make sense to allow for that style of binding (in
> addition to supporting more complex reset devices as required).

Yes, certainly. If for nothing else than "legacy" bindings:-) But I
think explicit properties are a perfectly reasonable approach even going
forward.

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

[Stephen Warren]

On 08/05/2013 01:32 AM, Philipp Zabel wrote:
> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
...
>> I think this should look more like the below:
>>
>> /* Device with nRESET pin connected to GPIO5_0 */
>> sii902x@39 {
>> 	/* named for the actual input line */
>> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
>> 	/* 
>> 	 * If there's some configurable property relating to the reset
>> 	 * line, we can describe it
>> 	 */
>> 	vendor,some-optional-reset-gpio-property;
>> 	...
>> };
> 
> I don't like the arbitrary name, as that makes it difficult to handle
> this in an automated way. In this case I'd prefer to use 'reset-gpios'
> and optionally 'reset-gpio-names' analogous to how clocks and interrupts
> (and resets) are handled.

Hmm. Just be aware that you can't force existing bindings to be
retro-actively modified, or you'll break the DT ABI. So, at the very
least we'd have to allow the existing custom-property-based approach for
bindings where it's already been used.

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

[Stephen Warren]

On 08/05/2013 01:32 AM, Philipp Zabel wrote:
> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
...
>> I think this should look more like the below:
>>
>> /* Device with nRESET pin connected to GPIO5_0 */
>> sii902x at 39 {
>> 	/* named for the actual input line */
>> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
>> 	/* 
>> 	 * If there's some configurable property relating to the reset
>> 	 * line, we can describe it
>> 	 */
>> 	vendor,some-optional-reset-gpio-property;
>> 	...
>> };
> 
> I don't like the arbitrary name, as that makes it difficult to handle
> this in an automated way. In this case I'd prefer to use 'reset-gpios'
> and optionally 'reset-gpio-names' analogous to how clocks and interrupts
> (and resets) are handled.

Hmm. Just be aware that you can't force existing bindings to be
retro-actively modified, or you'll break the DT ABI. So, at the very
least we'd have to allow the existing custom-property-based approach for
bindings where it's already been used.

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

[Philipp Zabel]

Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren:
> On 08/05/2013 01:32 AM, Philipp Zabel wrote:
> > Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
> >> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
> ...
> >> I think this should look more like the below:
> >>
> >> /* Device with nRESET pin connected to GPIO5_0 */
> >> sii902x@39 {
> >> 	/* named for the actual input line */
> >> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> >> 	/* 
> >> 	 * If there's some configurable property relating to the reset
> >> 	 * line, we can describe it
> >> 	 */
> >> 	vendor,some-optional-reset-gpio-property;
> >> 	...
> >> };
> > 
> > I don't like the arbitrary name, as that makes it difficult to handle
> > this in an automated way. In this case I'd prefer to use 'reset-gpios'
> > and optionally 'reset-gpio-names' analogous to how clocks and interrupts
> > (and resets) are handled.
> 
> Hmm. Just be aware that you can't force existing bindings to be
> retro-actively modified, or you'll break the DT ABI. So, at the very
> least we'd have to allow the existing custom-property-based approach for
> bindings where it's already been used.

Of course we have to continue supporting existing bindings. We could
continue using the GPIO API directly for those cases, or we could add a
function similar to of_get_named_gpio to wrap the GPIO:

	rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0);
	reset_control_assert(rstc);
	usleep(1000);
	reset_control_deassert(rstc);

My point is that for new bindings I'd prefer a well known property name
such as reset-gpios and a -names string list (as described
Documentation/devicetree/bindings/resource-names.txt) over ad-hoc
property names such as nreset-gpios, <vendor>-<submodule>-(n)reset,
nrst-gpios etc., both for consistency with existing resource properties
and because it is easier to grep for a single property name than for a
combination of all possible datasheet spellings of "reset".

I'd like
	rstc = reset_control_get(dev, "nreset");
to go look for
	resets = <&src 3>;
	reset-names = "nreset";
or for
	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
	reset-gpio-names = "nreset";
by default.

regards
Philipp

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

[Philipp Zabel]

Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren:
> On 08/05/2013 01:32 AM, Philipp Zabel wrote:
> > Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
> >> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
> ...
> >> I think this should look more like the below:
> >>
> >> /* Device with nRESET pin connected to GPIO5_0 */
> >> sii902x at 39 {
> >> 	/* named for the actual input line */
> >> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> >> 	/* 
> >> 	 * If there's some configurable property relating to the reset
> >> 	 * line, we can describe it
> >> 	 */
> >> 	vendor,some-optional-reset-gpio-property;
> >> 	...
> >> };
> > 
> > I don't like the arbitrary name, as that makes it difficult to handle
> > this in an automated way. In this case I'd prefer to use 'reset-gpios'
> > and optionally 'reset-gpio-names' analogous to how clocks and interrupts
> > (and resets) are handled.
> 
> Hmm. Just be aware that you can't force existing bindings to be
> retro-actively modified, or you'll break the DT ABI. So, at the very
> least we'd have to allow the existing custom-property-based approach for
> bindings where it's already been used.

Of course we have to continue supporting existing bindings. We could
continue using the GPIO API directly for those cases, or we could add a
function similar to of_get_named_gpio to wrap the GPIO:

	rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0);
	reset_control_assert(rstc);
	usleep(1000);
	reset_control_deassert(rstc);

My point is that for new bindings I'd prefer a well known property name
such as reset-gpios and a -names string list (as described
Documentation/devicetree/bindings/resource-names.txt) over ad-hoc
property names such as nreset-gpios, <vendor>-<submodule>-(n)reset,
nrst-gpios etc., both for consistency with existing resource properties
and because it is easier to grep for a single property name than for a
combination of all possible datasheet spellings of "reset".

I'd like
	rstc = reset_control_get(dev, "nreset");
to go look for
	resets = <&src 3>;
	reset-names = "nreset";
or for
	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
	reset-gpio-names = "nreset";
by default.

regards
Philipp

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

[Roger Quadros]

Hi Mark,

On 08/05/2013 06:35 PM, Mark Rutland wrote:
> On Mon, Aug 05, 2013 at 08:32:16AM +0100, Philipp Zabel wrote:
>> Hi Mark,
> 
> Hi Philipp,
> 
>>
>> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:

> 
> I'm not sure about initially-in-reset. Is it not possible for the driver
> to poke the GPIO controller to ensure the GPIOs are in the state
> required?

We need initially-in-reset, so that the device can be kept in reset even
when the device driver is not available or loaded late in the boot cycle.

cheers,
-roger

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

[Roger Quadros]

Hi Mark,

On 08/05/2013 06:35 PM, Mark Rutland wrote:
> On Mon, Aug 05, 2013 at 08:32:16AM +0100, Philipp Zabel wrote:
>> Hi Mark,
> 
> Hi Philipp,
> 
>>
>> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:

> 
> I'm not sure about initially-in-reset. Is it not possible for the driver
> to poke the GPIO controller to ensure the GPIOs are in the state
> required?

We need initially-in-reset, so that the device can be kept in reset even
when the device driver is not available or loaded late in the boot cycle.

cheers,
-roger

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

[Stephen Warren]

On 08/06/2013 01:32 AM, Philipp Zabel wrote:
> Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren:
>> On 08/05/2013 01:32 AM, Philipp Zabel wrote:
>>> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
>>>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
>> ...
>>>> I think this should look more like the below:
>>>>
>>>> /* Device with nRESET pin connected to GPIO5_0 */
>>>> sii902x@39 {
>>>> 	/* named for the actual input line */
>>>> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
>>>> 	/* 
>>>> 	 * If there's some configurable property relating to the reset
>>>> 	 * line, we can describe it
>>>> 	 */
>>>> 	vendor,some-optional-reset-gpio-property;
>>>> 	...
>>>> };
>>>
>>> I don't like the arbitrary name, as that makes it difficult to handle
>>> this in an automated way. In this case I'd prefer to use 'reset-gpios'
>>> and optionally 'reset-gpio-names' analogous to how clocks and interrupts
>>> (and resets) are handled.
>>
>> Hmm. Just be aware that you can't force existing bindings to be
>> retro-actively modified, or you'll break the DT ABI. So, at the very
>> least we'd have to allow the existing custom-property-based approach for
>> bindings where it's already been used.
> 
> Of course we have to continue supporting existing bindings. We could
> continue using the GPIO API directly for those cases, or we could add a
> function similar to of_get_named_gpio to wrap the GPIO:
> 
> 	rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0);
> 	reset_control_assert(rstc);
> 	usleep(1000);
> 	reset_control_deassert(rstc);

Well, you'd need to pass two names into that function; one would be the
name of the legacy property and the other the entry in reset-names or
reset-gpio-names. It's quite unlikely that the same string would be used
in both places.

> My point is that for new bindings I'd prefer a well known property name
> such as reset-gpios and a -names string list (as described
> Documentation/devicetree/bindings/resource-names.txt) over ad-hoc
> property names such as nreset-gpios, <vendor>-<submodule>-(n)reset,
> nrst-gpios etc., both for consistency with existing resource properties
> and because it is easier to grep for a single property name than for a
> combination of all possible datasheet spellings of "reset".
> 
> I'd like
> 	rstc = reset_control_get(dev, "nreset");
> to go look for
> 	resets = <&src 3>;
> 	reset-names = "nreset";
> or for
> 	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> 	reset-gpio-names = "nreset";
> by default.

That's rather complex for little benefit though. Take a look at existing
plain GPIO bindings, regulators, etc. They all simply encode the name
you're looking for directly into the property name, which is a lot less
overhead; simpler for humans to write and read without having to match n
properties together, and simpler to parse in code.

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

[Stephen Warren]

On 08/06/2013 01:32 AM, Philipp Zabel wrote:
> Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren:
>> On 08/05/2013 01:32 AM, Philipp Zabel wrote:
>>> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
>>>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
>> ...
>>>> I think this should look more like the below:
>>>>
>>>> /* Device with nRESET pin connected to GPIO5_0 */
>>>> sii902x at 39 {
>>>> 	/* named for the actual input line */
>>>> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
>>>> 	/* 
>>>> 	 * If there's some configurable property relating to the reset
>>>> 	 * line, we can describe it
>>>> 	 */
>>>> 	vendor,some-optional-reset-gpio-property;
>>>> 	...
>>>> };
>>>
>>> I don't like the arbitrary name, as that makes it difficult to handle
>>> this in an automated way. In this case I'd prefer to use 'reset-gpios'
>>> and optionally 'reset-gpio-names' analogous to how clocks and interrupts
>>> (and resets) are handled.
>>
>> Hmm. Just be aware that you can't force existing bindings to be
>> retro-actively modified, or you'll break the DT ABI. So, at the very
>> least we'd have to allow the existing custom-property-based approach for
>> bindings where it's already been used.
> 
> Of course we have to continue supporting existing bindings. We could
> continue using the GPIO API directly for those cases, or we could add a
> function similar to of_get_named_gpio to wrap the GPIO:
> 
> 	rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0);
> 	reset_control_assert(rstc);
> 	usleep(1000);
> 	reset_control_deassert(rstc);

Well, you'd need to pass two names into that function; one would be the
name of the legacy property and the other the entry in reset-names or
reset-gpio-names. It's quite unlikely that the same string would be used
in both places.

> My point is that for new bindings I'd prefer a well known property name
> such as reset-gpios and a -names string list (as described
> Documentation/devicetree/bindings/resource-names.txt) over ad-hoc
> property names such as nreset-gpios, <vendor>-<submodule>-(n)reset,
> nrst-gpios etc., both for consistency with existing resource properties
> and because it is easier to grep for a single property name than for a
> combination of all possible datasheet spellings of "reset".
> 
> I'd like
> 	rstc = reset_control_get(dev, "nreset");
> to go look for
> 	resets = <&src 3>;
> 	reset-names = "nreset";
> or for
> 	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> 	reset-gpio-names = "nreset";
> by default.

That's rather complex for little benefit though. Take a look at existing
plain GPIO bindings, regulators, etc. They all simply encode the name
you're looking for directly into the property name, which is a lot less
overhead; simpler for humans to write and read without having to match n
properties together, and simpler to parse in code.

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

[Philipp Zabel]

Hi Stephen

Am Dienstag, den 06.08.2013, 10:59 -0600 schrieb Stephen Warren:
> On 08/06/2013 01:32 AM, Philipp Zabel wrote:
> > Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren:
> >> On 08/05/2013 01:32 AM, Philipp Zabel wrote:
> >>> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
> >>>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
> >> ...
> >>>> I think this should look more like the below:
> >>>>
> >>>> /* Device with nRESET pin connected to GPIO5_0 */
> >>>> sii902x at 39 {
> >>>> 	/* named for the actual input line */
> >>>> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> >>>> 	/* 
> >>>> 	 * If there's some configurable property relating to the reset
> >>>> 	 * line, we can describe it
> >>>> 	 */
> >>>> 	vendor,some-optional-reset-gpio-property;
> >>>> 	...
> >>>> };
> >>>
> >>> I don't like the arbitrary name, as that makes it difficult to handle
> >>> this in an automated way. In this case I'd prefer to use 'reset-gpios'
> >>> and optionally 'reset-gpio-names' analogous to how clocks and interrupts
> >>> (and resets) are handled.
> >>
> >> Hmm. Just be aware that you can't force existing bindings to be
> >> retro-actively modified, or you'll break the DT ABI. So, at the very
> >> least we'd have to allow the existing custom-property-based approach for
> >> bindings where it's already been used.
> > 
> > Of course we have to continue supporting existing bindings. We could
> > continue using the GPIO API directly for those cases, or we could add a
> > function similar to of_get_named_gpio to wrap the GPIO:
> > 
> > 	rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0);
> > 	reset_control_assert(rstc);
> > 	usleep(1000);
> > 	reset_control_deassert(rstc);
> 
> Well, you'd need to pass two names into that function; one would be the
> name of the legacy property and the other the entry in reset-names or
> reset-gpio-names. It's quite unlikely that the same string would be used
> in both places.

there is no reset-names here. The legacy properties are only one GPIO
per property or addressed by index, currently. I don't want to change
that.

> > My point is that for new bindings I'd prefer a well known property name
> > such as reset-gpios and a -names string list (as described
> > Documentation/devicetree/bindings/resource-names.txt) over ad-hoc
> > property names such as nreset-gpios, <vendor>-<submodule>-(n)reset,
> > nrst-gpios etc., both for consistency with existing resource properties
> > and because it is easier to grep for a single property name than for a
> > combination of all possible datasheet spellings of "reset".

^ This is my main concern.

> > I'd like
> > 	rstc = reset_control_get(dev, "nreset");
> > to go look for
> > 	resets = <&src 3>;
> > 	reset-names = "nreset";
> > or for
> > 	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> > 	reset-gpio-names = "nreset";
> > by default.
> 
> That's rather complex for little benefit though.

In the majority of cases there will only be one reset per device. In
this case, the supplemental names property is not needed. The
resets/reset-names binding is using this scheme already, so using the
same for reset-gpios improves consistency.
If it weren't for the customary *-gpios property names, I'd suggest to
allow using GPIOs in the resets property directly:
	resets = <&gpio5 0 GPIO_ACTIVE_LOW>;

> Take a look at existing
> plain GPIO bindings, regulators, etc. They all simply encode the name
> you're looking for directly into the property name, which is a lot less
> overhead; simpler for humans to write and read without having to match n
> properties together, and simpler to parse in code.

It is not simpler to parse for humans if there is not a clear naming
scheme. For regulators, I can at least grep for the common "-supply"
suffix to recognize regulator supplies. But the "nrst-gpios" won't be
easily found. And if the datasheet calls the pin "RESET_N", or even
"CE", how will I call the reset property? I'd strongly prefer to
standardize on a recognizable name.

regards
Philipp

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

[Stephen Warren]

On 08/08/2013 03:42 AM, Philipp Zabel wrote:
> Hi Stephen
> 
> Am Dienstag, den 06.08.2013, 10:59 -0600 schrieb Stephen Warren:
>> On 08/06/2013 01:32 AM, Philipp Zabel wrote:
>>> Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren:
>>>> On 08/05/2013 01:32 AM, Philipp Zabel wrote:
>>>>> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
>>>>>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
>>>> ...
>>>>>> I think this should look more like the below:
>>>>>>
>>>>>> /* Device with nRESET pin connected to GPIO5_0 */
>>>>>> sii902x at 39 {
>>>>>> 	/* named for the actual input line */
>>>>>> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
>>>>>> 	/* 
>>>>>> 	 * If there's some configurable property relating to the reset
>>>>>> 	 * line, we can describe it
>>>>>> 	 */
>>>>>> 	vendor,some-optional-reset-gpio-property;
>>>>>> 	...
>>>>>> };
>>>>>
>>>>> I don't like the arbitrary name, as that makes it difficult to handle
>>>>> this in an automated way. In this case I'd prefer to use 'reset-gpios'
>>>>> and optionally 'reset-gpio-names' analogous to how clocks and interrupts
>>>>> (and resets) are handled.
>>>>
>>>> Hmm. Just be aware that you can't force existing bindings to be
>>>> retro-actively modified, or you'll break the DT ABI. So, at the very
>>>> least we'd have to allow the existing custom-property-based approach for
>>>> bindings where it's already been used.
>>>
>>> Of course we have to continue supporting existing bindings. We could
>>> continue using the GPIO API directly for those cases, or we could add a
>>> function similar to of_get_named_gpio to wrap the GPIO:
>>>
>>> 	rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0);
>>> 	reset_control_assert(rstc);
>>> 	usleep(1000);
>>> 	reset_control_deassert(rstc);
>>
>> Well, you'd need to pass two names into that function; one would be the
>> name of the legacy property and the other the entry in reset-names or
>> reset-gpio-names. It's quite unlikely that the same string would be used
>> in both places.
> 
> there is no reset-names here. The legacy properties are only one GPIO
> per property or addressed by index, currently. I don't want to change
> that.

So do you foresee:

1) Making zero changes to the existing binding and just keeping the
various custom stuff that some have

or

2) Migrating existing bindings to the new scheme, and simply
deprecating, but still allowing, the old custom properties.

If (1), then yes, of_get_named_reset_control() would only need one
parameter, either the name of the existing custom property or the
reset-names value, depending on whether the binding was old or new.

This might be problematic, since if you pass in "foo" expecting that to
be a reset-names entry, it would also accidentally allow a property
named "foo" to provide the information, even though that wasn't defined
in the binding.

Equally, if every binding is either-or, you may as well force drivers
passing the binding to just call different APIs based on their binding
definition.

If (2), then you'd need to pass both the legacy property name and
reset-names value separately, since I imagine those values would be
different, consider:

old:

	nvidia,phy-reset-gpio = <&gpio 1 0>;

new:

	reset-gpios = <&gpio 1 0>;
	reset-names = "phy";

>>> My point is that for new bindings I'd prefer a well known property name
>>> such as reset-gpios and a -names string list (as described
>>> Documentation/devicetree/bindings/resource-names.txt) over ad-hoc
>>> property names such as nreset-gpios, <vendor>-<submodule>-(n)reset,
>>> nrst-gpios etc., both for consistency with existing resource properties
>>> and because it is easier to grep for a single property name than for a
>>> combination of all possible datasheet spellings of "reset".
> 
> ^ This is my main concern.
> 
>>> I'd like
>>> 	rstc = reset_control_get(dev, "nreset");
>>> to go look for
>>> 	resets = <&src 3>;
>>> 	reset-names = "nreset";
>>> or for
>>> 	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
>>> 	reset-gpio-names = "nreset";
>>> by default.
>>
>> That's rather complex for little benefit though.
> 
> In the majority of cases there will only be one reset per device. In
> this case, the supplemental names property is not needed. The
> resets/reset-names binding is using this scheme already, so using the
> same for reset-gpios improves consistency.

I think that if there is just a "foo" property, it should always be
accessed by index into the list, whereas if there are both a foo and
foo-names property, it should always be accessed by name lookup. This
makes the lookup order much clearer. There are some unfortunate
exceptions to this today (regs and interrupts can have names, but those
aren't used in all cases!), but I'd like to avoid propagating that mistake.

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

[Philipp Zabel]

Am Donnerstag, den 08.08.2013, 12:43 -0600 schrieb Stephen Warren:
> On 08/08/2013 03:42 AM, Philipp Zabel wrote:
> > Hi Stephen
> > 
> > Am Dienstag, den 06.08.2013, 10:59 -0600 schrieb Stephen Warren:
> >> On 08/06/2013 01:32 AM, Philipp Zabel wrote:
> >>> Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren:
> >>>> On 08/05/2013 01:32 AM, Philipp Zabel wrote:
> >>>>> Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:
> >>>>>> On Thu, Jul 18, 2013 at 10:26:26AM +0100, 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.
> >>>> ...
> >>>>>> I think this should look more like the below:
> >>>>>>
> >>>>>> /* Device with nRESET pin connected to GPIO5_0 */
> >>>>>> sii902x at 39 {
> >>>>>> 	/* named for the actual input line */
> >>>>>> 	nreset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> >>>>>> 	/* 
> >>>>>> 	 * If there's some configurable property relating to the reset
> >>>>>> 	 * line, we can describe it
> >>>>>> 	 */
> >>>>>> 	vendor,some-optional-reset-gpio-property;
> >>>>>> 	...
> >>>>>> };
> >>>>>
> >>>>> I don't like the arbitrary name, as that makes it difficult to handle
> >>>>> this in an automated way. In this case I'd prefer to use 'reset-gpios'
> >>>>> and optionally 'reset-gpio-names' analogous to how clocks and interrupts
> >>>>> (and resets) are handled.
> >>>>
> >>>> Hmm. Just be aware that you can't force existing bindings to be
> >>>> retro-actively modified, or you'll break the DT ABI. So, at the very
> >>>> least we'd have to allow the existing custom-property-based approach for
> >>>> bindings where it's already been used.
> >>>
> >>> Of course we have to continue supporting existing bindings. We could
> >>> continue using the GPIO API directly for those cases, or we could add a
> >>> function similar to of_get_named_gpio to wrap the GPIO:
> >>>
> >>> 	rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0);
> >>> 	reset_control_assert(rstc);
> >>> 	usleep(1000);
> >>> 	reset_control_deassert(rstc);
> >>
> >> Well, you'd need to pass two names into that function; one would be the
> >> name of the legacy property and the other the entry in reset-names or
> >> reset-gpio-names. It's quite unlikely that the same string would be used
> >> in both places.
> > 
> > there is no reset-names here. The legacy properties are only one GPIO
> > per property or addressed by index, currently. I don't want to change
> > that.
> 
> So do you foresee:
> 
> 1) Making zero changes to the existing binding and just keeping the
> various custom stuff that some have
> 
> or
> 
> 2) Migrating existing bindings to the new scheme, and simply
> deprecating, but still allowing, the old custom properties.

Yes, I'd like to deprecate the (<vendor>,)<custom>-gpio binding scheme
for reset gpios, on a case by case basis.

> If (1), then yes, of_get_named_reset_control() would only need one
> parameter, either the name of the existing custom property or the
> reset-names value, depending on whether the binding was old or new.
>
> This might be problematic, since if you pass in "foo" expecting that to
> be a reset-names entry, it would also accidentally allow a property
> named "foo" to provide the information, even though that wasn't defined
> in the binding.

Yes, of_get_named_reset_control() was just an example, to be used for
the legacy case only.

> Equally, if every binding is either-or, you may as well force drivers
> passing the binding to just call different APIs based on their binding
> definition.
> 
> If (2), then you'd need to pass both the legacy property name and
> reset-names value separately, since I imagine those values would be
> different, consider:
> 
> old:
> 
> 	nvidia,phy-reset-gpio = <&gpio 1 0>;
> 
> new:
> 
> 	reset-gpios = <&gpio 1 0>;
> 	reset-names = "phy";

Correct. In this case:

	rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0);
	if (IS_ERR(rstc)) {
		ret = PTR_ERR(rstc);
		if (ret == -EPROBE_DEFER)
			return ret;
		rstc = reset_control_get(dev, "phy");
		if (IS_ERR(rstc))
			return PTR_ERR(rstc);
	}

Maybe encapsulate this in a convenience wrapper:

	rstc = reset_control_get_legacy(dev, "phy", "nvidia,phy-reset-gpio");

> > In the majority of cases there will only be one reset per device. In
> > this case, the supplemental names property is not needed. The
> > resets/reset-names binding is using this scheme already, so using the
> > same for reset-gpios improves consistency.
> 
> I think that if there is just a "foo" property, it should always be
> accessed by index into the list, whereas if there are both a foo and
> foo-names property, it should always be accessed by name lookup. This
> makes the lookup order much clearer. There are some unfortunate
> exceptions to this today (regs and interrupts can have names, but those
> aren't used in all cases!), but I'd like to avoid propagating that mistake.

Agreed. Looking up regs and interrupts by index is error-prone if there
are more than one, but surely leaving the -names property out in case
there is only one entry should be fine?

regards
Philipp

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

[Stephen Warren]

On 08/12/2013 05:04 AM, Philipp Zabel wrote:
> Am Donnerstag, den 08.08.2013, 12:43 -0600 schrieb Stephen Warren:
...
>> Equally, if every binding is either-or, you may as well force drivers
>> passing the binding to just call different APIs based on their binding
>> definition.
>>
>> If (2), then you'd need to pass both the legacy property name and
>> reset-names value separately, since I imagine those values would be
>> different, consider:
>>
>> old:
>>
>> 	nvidia,phy-reset-gpio = <&gpio 1 0>;
>>
>> new:
>>
>> 	reset-gpios = <&gpio 1 0>;
>> 	reset-names = "phy";
> 
> Correct. In this case:
> 
> 	rstc = of_get_named_reset_control(np, "nvidia,phy-reset-gpio", 0);
> 	if (IS_ERR(rstc)) {
> 		ret = PTR_ERR(rstc);
> 		if (ret == -EPROBE_DEFER)
> 			return ret;
> 		rstc = reset_control_get(dev, "phy");
> 		if (IS_ERR(rstc))
> 			return PTR_ERR(rstc);
> 	}
> 
> Maybe encapsulate this in a convenience wrapper:
> 
> 	rstc = reset_control_get_legacy(dev, "phy", "nvidia,phy-reset-gpio");

Yes, that's exactly what I envisaged. Thanks.