[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+***@public.gmane.org>
Reviewed-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+***@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 */
+***@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+***@public.gmane.org>");
+MODULE_DESCRIPTION("gpio reset controller");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:gpio-reset");
+MODULE_DEVICE_TABLE(of, gpio_reset_dt_ids);

[Shawn Guo]

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

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

[Mark Rutland]

Post by 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.
---
- 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.
+
+- 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
+
+- 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.
+
+
+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 */
+ /* ... */
+ 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 */
***@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.

[Stephen Warren]

Post by Mark Rutland

Post by 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.
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.
+
+- 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
+
+- 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.
+
+
+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 */
+ /* ... */
+ 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.

Post by Mark Rutland
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
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.
/* Device with nRESET pin connected to GPIO5_0 */
/* 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.

[Mark Rutland]

Post by Stephen Warren

Post by Mark Rutland

Post by 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.
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.
+
+- 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
+
+- 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.
+
+
+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 */
+ /* ... */
+ 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.

Post by Stephen Warren

Post by Mark Rutland
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
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.
/* Device with nRESET pin connected to GPIO5_0 */
/* 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.

[Stephen Warren]

Post by Mark Rutland

Post by Stephen Warren

Post by Mark Rutland

Post by 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.
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.
+
+- 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
+
+- 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.
+
+
+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 */
+ /* ... */
+ 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.

Post by Mark Rutland
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.

Post by Mark Rutland
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.

[Philipp Zabel]

Hi Mark,

Post by Mark Rutland

Post by 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.
---
- 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.
+
+- 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
+
+- 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.
+
+
+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 */
+ /* ... */
+ 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.

Post by Mark Rutland
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
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.
/* Device with nRESET pin connected to GPIO5_0 */
/* 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:

***@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);

Post by Mark Rutland
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

[Mark Rutland]

Post by Philipp Zabel
Hi Mark,

Hi Philipp,

Post by Philipp Zabel

Post by Mark Rutland

Post by 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.
---
- 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.
+
+- 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
+
+- 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.
+
+
+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 */
+ /* ... */
+ 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.

Post by Mark Rutland
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
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.
/* Device with nRESET pin connected to GPIO5_0 */
/* 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

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

Post by Philipp Zabel
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
device_reset_usleep(dev, 1000);
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.

[Roger Quadros]

Hi Mark,

Post by Mark Rutland

Post by Philipp Zabel
Hi Mark,

Hi Philipp,
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

[Stephen Warren]

Post by Philipp Zabel

Post by Philipp Zabel

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

...

Post by Philipp Zabel

Post by Philipp Zabel
/* Device with nRESET pin connected to GPIO5_0 */
/* 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.

[Philipp Zabel]

Post by Stephen Warren

Post by Philipp Zabel

Post by Philipp Zabel

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

...

Post by Philipp Zabel

Post by Philipp Zabel
/* Device with nRESET pin connected to GPIO5_0 */
/* 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

[Stephen Warren]

Post by Philipp Zabel

Post by Stephen Warren

Post by Philipp Zabel

Post by Philipp Zabel

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

...

Post by Philipp Zabel

Post by Philipp Zabel
/* Device with nRESET pin connected to GPIO5_0 */
/* 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
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.

Post by Philipp Zabel
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.

[Philipp Zabel]

Hi Stephen

Post by Stephen Warren

Post by Philipp Zabel

Post by Stephen Warren

Post by Philipp Zabel

Post by Philipp Zabel

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

...

Post by Philipp Zabel

Post by Philipp Zabel
/* Device with nRESET pin connected to GPIO5_0 */
/* 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
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.

Post by Stephen Warren

Post by Philipp Zabel
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.

Post by Stephen Warren

Post by Philipp Zabel
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>;

Post by Stephen Warren
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

[Stephen Warren]

Post by Philipp Zabel
Hi Stephen

Post by Stephen Warren

Post by Philipp Zabel

Post by Stephen Warren

Post by Philipp Zabel

Post by Philipp Zabel

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

...

Post by Philipp Zabel

Post by Philipp Zabel
/* Device with nRESET pin connected to GPIO5_0 */
/* 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
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";

Post by Philipp Zabel

Post by Stephen Warren

Post by Philipp Zabel
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.

Post by Stephen Warren

Post by Philipp Zabel
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.

[Philipp Zabel]

Post by Stephen Warren

Post by Philipp Zabel
Hi Stephen

Post by Stephen Warren

Post by Philipp Zabel

Post by Stephen Warren

Post by Philipp Zabel

Post by Philipp Zabel

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

...

Post by Philipp Zabel

Post by Philipp Zabel
/* Device with nRESET pin connected to GPIO5_0 */
/* 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
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.

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.

Post by Stephen Warren
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.

Post by 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
nvidia,phy-reset-gpio = <&gpio 1 0>;
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");

Post by Stephen Warren

Post by Philipp Zabel
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

[Stephen Warren]

...

Post by Philipp Zabel

Post by 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
nvidia,phy-reset-gpio = <&gpio 1 0>;
reset-gpios = <&gpio 1 0>;
reset-names = "phy";

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);
}
rstc = reset_control_get_legacy(dev, "phy", "nvidia,phy-reset-gpio");

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