[PATCH 1/2] dt-bindings: reset: Add generic GPIO reset binding

[PATCH 1/2] dt-bindings: reset: Add generic GPIO reset binding

[Sean Anderson]

This adds a binding for a generic GPIO reset driver. This driver is
designed to easily add a GPIO-based reset to a driver which expected a
reset controller. It offers greater flexibility than a reset-gpios
property, and allows for one code path to be shared for GPIO resets and
MMIO-based resets.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 .../devicetree/bindings/reset/gpio-reset.yaml | 93 +++++++++++++++++++
 1 file changed, 93 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/gpio-reset.yaml

diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.yaml b/Documentation/devicetree/bindings/reset/gpio-reset.yaml
new file mode 100644
index 000000000000..de2ab074cea3
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/gpio-reset.yaml
@@ -0,0 +1,93 @@
+# SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/gpio-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic GPIO reset driver
+
+maintainers:
+  - Sean Anderson <seanga2@gmail.com>
+
+description: |
+  This is a generic GPIO reset driver which can provide a reset-controller
+  interface for GPIO-based reset lines. This driver always operates with
+  logical GPIO values; to invert the polarity, specify GPIO_ACTIVE_LOW in the
+  GPIO's flags.
+
+properties:
+  compatible:
+    const: gpio-reset
+
+  '#reset-cells':
+    const: 1
+
+  reset-gpios:
+    description: |
+      GPIOs to assert when asserting a reset. There is a one-to-one mapping
+      between the reset specifier and the index of the GPIO in this list to
+      assert.
+
+  done-gpios:
+    description: |
+      GPIOs which indicate that the device controlled by the GPIO has exited
+      reset. There must be one done GPIO for each reset GPIO, or no done GPIOs
+      at all. The driver will wait for up to done-timeout-us for the
+      corresponding done GPIO to assert before returning.
+
+  pre-assert-us:
+    default: 0
+    description: |
+      Microseconds to delay between when the reset was requested to be
+      asserted, and asserting the reset GPIO
+
+  post-assert-us:
+    default: 0
+    description: |
+      Microseconds to delay after asserting the reset GPIO and before returning
+      to the caller.
+
+  pre-deassert-us:
+    default: 0
+    description: |
+      Microseconds to delay between when the reset was requested to be
+      deasserted, and asserting the reset GPIO
+
+  post-deassert-us:
+    default: 0
+    description: |
+      Microseconds to delay after deasserting the reset GPIO and before
+      returning to the caller. This delay is always present, even if the done
+      GPIO goes high earlier.
+
+  done-timeout-us:
+    default: 1000
+    description:
+      Microseconds to wait for the done GPIO to assert after deasserting the
+      reset GPIO. If post-deassert-us is present, this property defaults to 10
+      times that delay. The timeout starts after waiting for the post deassert
+      delay.
+
+required:
+  - '#reset-cells'
+  - compatible
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    pcs_reset: reset-pcs {
+        #reset-cells = <1>;
+        compatible = "gpio-reset";
+        reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>,
+                      <&gpio 1 GPIO_ACTIVE_LOW>,
+                      <&gpio 2 GPIO_ACTIVE_LOW>,
+                      <&gpio 3 GPIO_ACTIVE_LOW>;
+        done-gpios = <&gpio 4 GPIO_ACTIVE_HIGH>,
+                     <&gpio 5 GPIO_ACTIVE_HIGH>,
+                     <&gpio 6 GPIO_ACTIVE_HIGH>,
+                     <&gpio 7 GPIO_ACTIVE_HIGH>;
+        post-deassert-us = <100>;
+    };
-- 
2.25.1

[PATCH 2/2] reset: Add GPIO-based reset controller

[Sean Anderson]

This adds a driver to control GPIO-based resets using the reset
controller API. This allows using a common interface for devices which
may have both GPIO resets and reset controllers, depending on the
configuration. It also allows for easier sharing of reset GPIOs in the
case where one GPIO is a reset for multiple devices.

There are several properties for specifying pre/post-(de)assert delays.
This device can also use a "reset done" GPIO, for cases when such a GPIO
is provided by the device being reset. This can be useful when the
datasheet does not otherwise specify reset timings, or specifies a much
longer maximum reset delay than the typical delay.

There is one queue for waiters on done GPIOs. I don't anticipate there
being a penalty from this, since there will likely only be concurrent
waiters during startup. I believe that wait_event_idle_timeout is the
correct function to use here, but there are a lot of variants of this
function, so I am not completely sure it is the best.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

 MAINTAINERS                |   5 +
 drivers/reset/Kconfig      |  11 ++
 drivers/reset/Makefile     |   1 +
 drivers/reset/reset-gpio.c | 223 +++++++++++++++++++++++++++++++++++++
 4 files changed, 240 insertions(+)
 create mode 100644 drivers/reset/reset-gpio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8d118d7957d2..0a54c4dd83d9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7813,6 +7813,11 @@ F:	Documentation/i2c/muxes/i2c-mux-gpio.rst
 F:	drivers/i2c/muxes/i2c-mux-gpio.c
 F:	include/linux/platform_data/i2c-mux-gpio.h
 
+GENERIC GPIO RESET DRIVER
+M:	Sean Anderson <seanga2@gmail.com>
+S:	Supported
+F:	drivers/reset/reset-gpio.c
+
 GENERIC HDLC (WAN) DRIVERS
 M:	Krzysztof Halasa <khc@pm.waw.pl>
 S:	Maintained
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index be799a5abf8a..24888005baf8 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -66,6 +66,17 @@ config RESET_BRCMSTB_RESCAL
 	  This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on
 	  BCM7216.
 
+config RESET_GPIO
+	tristate "GPIO reset controller"
+	depends on OF
+	help
+	  This enables a generic controller for resets attached via GPIOs. It
+	  may be used to add GPIO resets to drivers which expect a reset
+	  controller. It supports adding delays and waiting for a "done" GPIO
+	  to be asserted.
+
+	  If compiled as module, it will be called reset-gpio.
+
 config RESET_HSDK
 	bool "Synopsys HSDK Reset Driver"
 	depends on HAS_IOMEM
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 21d46d8869ff..f577ec16fd93 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
 obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
 obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
 obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
+obj-$(CONFIG_RESET_GPIO) += reset-gpio.o
 obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
 obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
 obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
diff --git a/drivers/reset/reset-gpio.c b/drivers/reset/reset-gpio.c
new file mode 100644
index 000000000000..93d3dbb150e0
--- /dev/null
+++ b/drivers/reset/reset-gpio.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2021 Sean Anderson <sean.anderson@seco.com>
+ *
+ * This driver controls GPIOs used to reset device(s). It may be used for when
+ * there is a need for more complex behavior than a simple reset-gpios
+ * property. It may also be used to unify code paths between device-based and
+ * gpio-based resets.
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/sched.h>
+#include <linux/wait.h>
+
+/**
+ * struct reset_gpio_priv - Private data for GPIO reset driver
+ * @rc: Reset controller for this driver
+ * @done_queue: Queue to wait for changes on done GPIOs. Events occur whenever
+ *              the value of any done GPIO changes. Valid only when @done is
+ *              non-%NULL.
+ * @reset: Array of gpios to use when (de)asserting resets
+ * @done: Array of gpios to determine whether a reset has finished; may be
+ *        %NULL
+ * @done_timeout_jiffies: Timeout when waiting for a done GPIO to be asserted, in jiffies
+ * @post_assert_delay: Time to wait after asserting a reset, in us
+ * @post_deassert_delay: Time to wait after deasserting a reset, in us
+ */
+struct reset_gpio_priv {
+	struct reset_controller_dev rc;
+	struct wait_queue_head done_queue;
+	struct gpio_descs *reset;
+	struct gpio_descs *done;
+	unsigned long done_timeout_jiffies;
+	u32 pre_assert_delay;
+	u32 post_assert_delay;
+	u32 pre_deassert_delay;
+	u32 post_deassert_delay;
+};
+
+static inline struct reset_gpio_priv
+*rc_to_reset_gpio(struct reset_controller_dev *rc)
+{
+	return container_of(rc, struct reset_gpio_priv, rc);
+}
+
+static int reset_gpio_assert(struct reset_controller_dev *rc, unsigned long id)
+{
+	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
+
+	if (priv->pre_assert_delay)
+		fsleep(priv->pre_assert_delay);
+	gpiod_set_value_cansleep(priv->reset->desc[id], 1);
+	if (priv->post_assert_delay)
+		fsleep(priv->post_assert_delay);
+	return 0;
+}
+
+static int reset_gpio_deassert(struct reset_controller_dev *rc,
+			       unsigned long id)
+{
+	int ret = 0;
+	unsigned int remaining;
+	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
+
+	if (priv->pre_deassert_delay)
+		fsleep(priv->pre_deassert_delay);
+	gpiod_set_value_cansleep(priv->reset->desc[id], 0);
+	if (priv->post_deassert_delay)
+		fsleep(priv->post_deassert_delay);
+
+	if (!priv->done)
+		return 0;
+
+	remaining = wait_event_idle_timeout(
+		priv->done_queue,
+		(ret = gpiod_get_value_cansleep(priv->done->desc[id])),
+		priv->done_timeout_jiffies);
+	dev_dbg(rc->dev, "%s: remaining=%u\n", __func__, remaining);
+	if (ret < 0)
+		return ret;
+	if (ret)
+		return 0;
+	return -ETIMEDOUT;
+}
+
+static int reset_gpio_reset(struct reset_controller_dev *rc, unsigned long id)
+{
+	int ret = reset_gpio_assert(rc, id);
+
+	if (!ret)
+		return ret;
+
+	return reset_gpio_deassert(rc, id);
+}
+
+static int reset_gpio_status(struct reset_controller_dev *rc, unsigned long id)
+{
+	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
+
+	return gpiod_get_value_cansleep(priv->reset->desc[id]);
+}
+
+static const struct reset_control_ops reset_gpio_ops = {
+	.reset = reset_gpio_reset,
+	.assert = reset_gpio_assert,
+	.deassert = reset_gpio_deassert,
+	.status = reset_gpio_status,
+};
+
+static irqreturn_t reset_gpio_irq(int irq, void *data)
+{
+	struct reset_gpio_priv *priv = data;
+
+	wake_up(&priv->done_queue);
+	return IRQ_HANDLED;
+}
+
+static int reset_gpio_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct reset_gpio_priv *priv;
+	u32 done_timeout_us;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, priv);
+
+	/* A short macro to reduce repetitive error handling */
+#define read_delay(propname, val) do { \
+	ret = of_property_read_u32(dev->of_node, (propname), &(val)); \
+	if (ret == -EINVAL) \
+		(val) = 0; \
+	else if (ret) \
+		return dev_err_probe(dev, ret, \
+				     "Could not read %s\n", propname); \
+} while (0)
+
+	read_delay("pre-assert-us", priv->pre_assert_delay);
+	read_delay("post-assert-us", priv->post_assert_delay);
+	read_delay("pre-deassert-us", priv->pre_deassert_delay);
+	read_delay("post-deassert-us", priv->post_deassert_delay);
+
+	ret = of_property_read_u32(np, "done-timeout-us", &done_timeout_us);
+	if (ret == -EINVAL) {
+		if (priv->post_deassert_delay)
+			done_timeout_us = 10 * priv->post_deassert_delay;
+		else
+			done_timeout_us = 1000;
+	} else if (ret)
+		return dev_err_probe(dev, ret,
+				     "Could not read done timeout\n");
+	priv->done_timeout_jiffies = usecs_to_jiffies(done_timeout_us);
+
+	priv->reset = devm_gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(priv->reset))
+		return dev_err_probe(dev, PTR_ERR(priv->reset),
+				     "Could not get reset gpios\n");
+
+	priv->done = devm_gpiod_get_array_optional(dev, "done",
+						   GPIOD_IN);
+	if (IS_ERR(priv->done))
+		return dev_err_probe(dev, PTR_ERR(priv->done),
+				     "Could not get done gpios\n");
+	if (priv->done) {
+		int i;
+
+		if (priv->reset->ndescs != priv->done->ndescs)
+			return dev_err_probe(dev, -EINVAL,
+					     "Number of reset and done gpios does not match\n");
+		init_waitqueue_head(&priv->done_queue);
+		for (i = 0; i < priv->done->ndescs; i++) {
+			ret = gpiod_to_irq(priv->done->desc[i]);
+			if (ret < 0)
+				return dev_err_probe(dev, ret,
+						     "Could not convert GPIO to IRQ\n");
+
+			ret = devm_request_irq(dev, ret, reset_gpio_irq,
+					       IRQF_SHARED, dev_name(dev),
+					       priv);
+			if (ret)
+				return dev_err_probe(dev, ret,
+						     "Could not request IRQ\n");
+		}
+	}
+
+	priv->rc.ops = &reset_gpio_ops;
+	priv->rc.owner = THIS_MODULE;
+	priv->rc.dev = dev;
+	priv->rc.of_node = np;
+	priv->rc.nr_resets = priv->reset->ndescs;
+	ret = devm_reset_controller_register(dev, &priv->rc);
+	if (!ret)
+		dev_info(dev, "probed with %u resets\n", priv->reset->ndescs);
+	return ret;
+}
+
+static const struct of_device_id reset_gpio_of_match[] = {
+	{ .compatible = "gpio-reset", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, reset_gpio_of_match);
+
+static struct platform_driver reset_gpio_driver = {
+	.probe = reset_gpio_probe,
+	.driver = {
+		.name = "gpio-reset",
+		.of_match_table = of_match_ptr(reset_gpio_of_match),
+	},
+};
+module_platform_driver(reset_gpio_driver);
+
+MODULE_ALIAS("platform:gpio-reset");
+MODULE_DESCRIPTION("Generic GPIO reset driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

Re: [PATCH 2/2] reset: Add GPIO-based reset controller

[Sean Anderson]

On 10/18/21 7:49 PM, Sean Anderson wrote:
> This adds a driver to control GPIO-based resets using the reset
> controller API. This allows using a common interface for devices which
> may have both GPIO resets and reset controllers, depending on the
> configuration. It also allows for easier sharing of reset GPIOs in the
> case where one GPIO is a reset for multiple devices.
> 
> There are several properties for specifying pre/post-(de)assert delays.
> This device can also use a "reset done" GPIO, for cases when such a GPIO
> is provided by the device being reset. This can be useful when the
> datasheet does not otherwise specify reset timings, or specifies a much
> longer maximum reset delay than the typical delay.
> 
> There is one queue for waiters on done GPIOs. I don't anticipate there
> being a penalty from this, since there will likely only be concurrent
> waiters during startup. I believe that wait_event_idle_timeout is the
> correct function to use here, but there are a lot of variants of this
> function, so I am not completely sure it is the best.
> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
>   MAINTAINERS                |   5 +
>   drivers/reset/Kconfig      |  11 ++
>   drivers/reset/Makefile     |   1 +
>   drivers/reset/reset-gpio.c | 223 +++++++++++++++++++++++++++++++++++++
>   4 files changed, 240 insertions(+)
>   create mode 100644 drivers/reset/reset-gpio.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8d118d7957d2..0a54c4dd83d9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7813,6 +7813,11 @@ F:	Documentation/i2c/muxes/i2c-mux-gpio.rst
>   F:	drivers/i2c/muxes/i2c-mux-gpio.c
>   F:	include/linux/platform_data/i2c-mux-gpio.h
>   
> +GENERIC GPIO RESET DRIVER
> +M:	Sean Anderson <seanga2@gmail.com>
> +S:	Supported
> +F:	drivers/reset/reset-gpio.c
> +
>   GENERIC HDLC (WAN) DRIVERS
>   M:	Krzysztof Halasa <khc@pm.waw.pl>
>   S:	Maintained
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index be799a5abf8a..24888005baf8 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -66,6 +66,17 @@ config RESET_BRCMSTB_RESCAL
>   	  This enables the RESCAL reset controller for SATA, PCIe0, or PCIe1 on
>   	  BCM7216.
>   
> +config RESET_GPIO
> +	tristate "GPIO reset controller"
> +	depends on OF
> +	help
> +	  This enables a generic controller for resets attached via GPIOs. It
> +	  may be used to add GPIO resets to drivers which expect a reset
> +	  controller. It supports adding delays and waiting for a "done" GPIO
> +	  to be asserted.
> +
> +	  If compiled as module, it will be called reset-gpio.
> +
>   config RESET_HSDK
>   	bool "Synopsys HSDK Reset Driver"
>   	depends on HAS_IOMEM
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 21d46d8869ff..f577ec16fd93 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
>   obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
>   obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
>   obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> +obj-$(CONFIG_RESET_GPIO) += reset-gpio.o
>   obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
>   obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
>   obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> diff --git a/drivers/reset/reset-gpio.c b/drivers/reset/reset-gpio.c
> new file mode 100644
> index 000000000000..93d3dbb150e0
> --- /dev/null
> +++ b/drivers/reset/reset-gpio.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2021 Sean Anderson <sean.anderson@seco.com>
> + *
> + * This driver controls GPIOs used to reset device(s). It may be used for when
> + * there is a need for more complex behavior than a simple reset-gpios
> + * property. It may also be used to unify code paths between device-based and
> + * gpio-based resets.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/sched.h>
> +#include <linux/wait.h>
> +
> +/**
> + * struct reset_gpio_priv - Private data for GPIO reset driver
> + * @rc: Reset controller for this driver
> + * @done_queue: Queue to wait for changes on done GPIOs. Events occur whenever
> + *              the value of any done GPIO changes. Valid only when @done is
> + *              non-%NULL.
> + * @reset: Array of gpios to use when (de)asserting resets
> + * @done: Array of gpios to determine whether a reset has finished; may be
> + *        %NULL
> + * @done_timeout_jiffies: Timeout when waiting for a done GPIO to be asserted, in jiffies
> + * @post_assert_delay: Time to wait after asserting a reset, in us
> + * @post_deassert_delay: Time to wait after deasserting a reset, in us
> + */
> +struct reset_gpio_priv {
> +	struct reset_controller_dev rc;
> +	struct wait_queue_head done_queue;
> +	struct gpio_descs *reset;
> +	struct gpio_descs *done;
> +	unsigned long done_timeout_jiffies;
> +	u32 pre_assert_delay;
> +	u32 post_assert_delay;
> +	u32 pre_deassert_delay;
> +	u32 post_deassert_delay;
> +};
> +
> +static inline struct reset_gpio_priv
> +*rc_to_reset_gpio(struct reset_controller_dev *rc)
> +{
> +	return container_of(rc, struct reset_gpio_priv, rc);
> +}
> +
> +static int reset_gpio_assert(struct reset_controller_dev *rc, unsigned long id)
> +{
> +	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
> +
> +	if (priv->pre_assert_delay)
> +		fsleep(priv->pre_assert_delay);
> +	gpiod_set_value_cansleep(priv->reset->desc[id], 1);
> +	if (priv->post_assert_delay)
> +		fsleep(priv->post_assert_delay);
> +	return 0;
> +}
> +
> +static int reset_gpio_deassert(struct reset_controller_dev *rc,
> +			       unsigned long id)
> +{
> +	int ret = 0;
> +	unsigned int remaining;
> +	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
> +
> +	if (priv->pre_deassert_delay)
> +		fsleep(priv->pre_deassert_delay);
> +	gpiod_set_value_cansleep(priv->reset->desc[id], 0);
> +	if (priv->post_deassert_delay)
> +		fsleep(priv->post_deassert_delay);
> +
> +	if (!priv->done)
> +		return 0;
> +
> +	remaining = wait_event_idle_timeout(
> +		priv->done_queue,
> +		(ret = gpiod_get_value_cansleep(priv->done->desc[id])),
> +		priv->done_timeout_jiffies);
> +	dev_dbg(rc->dev, "%s: remaining=%u\n", __func__, remaining);
> +	if (ret < 0)
> +		return ret;
> +	if (ret)
> +		return 0;
> +	return -ETIMEDOUT;
> +}
> +
> +static int reset_gpio_reset(struct reset_controller_dev *rc, unsigned long id)
> +{
> +	int ret = reset_gpio_assert(rc, id);
> +
> +	if (!ret)
> +		return ret;
> +
> +	return reset_gpio_deassert(rc, id);
> +}
> +
> +static int reset_gpio_status(struct reset_controller_dev *rc, unsigned long id)
> +{
> +	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
> +
> +	return gpiod_get_value_cansleep(priv->reset->desc[id]);
> +}
> +
> +static const struct reset_control_ops reset_gpio_ops = {
> +	.reset = reset_gpio_reset,
> +	.assert = reset_gpio_assert,
> +	.deassert = reset_gpio_deassert,
> +	.status = reset_gpio_status,
> +};
> +
> +static irqreturn_t reset_gpio_irq(int irq, void *data)
> +{
> +	struct reset_gpio_priv *priv = data;
> +
> +	wake_up(&priv->done_queue);
> +	return IRQ_HANDLED;
> +}
> +
> +static int reset_gpio_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct reset_gpio_priv *priv;
> +	u32 done_timeout_us;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, priv);
> +
> +	/* A short macro to reduce repetitive error handling */
> +#define read_delay(propname, val) do { \
> +	ret = of_property_read_u32(dev->of_node, (propname), &(val)); \
> +	if (ret == -EINVAL) \
> +		(val) = 0; \
> +	else if (ret) \
> +		return dev_err_probe(dev, ret, \
> +				     "Could not read %s\n", propname); \
> +} while (0)
> +
> +	read_delay("pre-assert-us", priv->pre_assert_delay);
> +	read_delay("post-assert-us", priv->post_assert_delay);
> +	read_delay("pre-deassert-us", priv->pre_deassert_delay);
> +	read_delay("post-deassert-us", priv->post_deassert_delay);
> +
> +	ret = of_property_read_u32(np, "done-timeout-us", &done_timeout_us);
> +	if (ret == -EINVAL) {
> +		if (priv->post_deassert_delay)
> +			done_timeout_us = 10 * priv->post_deassert_delay;
> +		else
> +			done_timeout_us = 1000;
> +	} else if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Could not read done timeout\n");
> +	priv->done_timeout_jiffies = usecs_to_jiffies(done_timeout_us);
> +
> +	priv->reset = devm_gpiod_get_array(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(priv->reset))
> +		return dev_err_probe(dev, PTR_ERR(priv->reset),
> +				     "Could not get reset gpios\n");
> +
> +	priv->done = devm_gpiod_get_array_optional(dev, "done",
> +						   GPIOD_IN);
> +	if (IS_ERR(priv->done))
> +		return dev_err_probe(dev, PTR_ERR(priv->done),
> +				     "Could not get done gpios\n");
> +	if (priv->done) {
> +		int i;
> +
> +		if (priv->reset->ndescs != priv->done->ndescs)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Number of reset and done gpios does not match\n");
> +		init_waitqueue_head(&priv->done_queue);
> +		for (i = 0; i < priv->done->ndescs; i++) {
> +			ret = gpiod_to_irq(priv->done->desc[i]);
> +			if (ret < 0)
> +				return dev_err_probe(dev, ret,
> +						     "Could not convert GPIO to IRQ\n");
> +
> +			ret = devm_request_irq(dev, ret, reset_gpio_irq,
> +					       IRQF_SHARED, dev_name(dev),
> +					       priv);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "Could not request IRQ\n");
> +		}
> +	}
> +
> +	priv->rc.ops = &reset_gpio_ops;
> +	priv->rc.owner = THIS_MODULE;
> +	priv->rc.dev = dev;
> +	priv->rc.of_node = np;
> +	priv->rc.nr_resets = priv->reset->ndescs;
> +	ret = devm_reset_controller_register(dev, &priv->rc);
> +	if (!ret)
> +		dev_info(dev, "probed with %u resets\n", priv->reset->ndescs);
> +	return ret;
> +}
> +
> +static const struct of_device_id reset_gpio_of_match[] = {
> +	{ .compatible = "gpio-reset", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, reset_gpio_of_match);
> +
> +static struct platform_driver reset_gpio_driver = {
> +	.probe = reset_gpio_probe,
> +	.driver = {
> +		.name = "gpio-reset",
> +		.of_match_table = of_match_ptr(reset_gpio_of_match),
> +	},
> +};
> +module_platform_driver(reset_gpio_driver);
> +
> +MODULE_ALIAS("platform:gpio-reset");
> +MODULE_DESCRIPTION("Generic GPIO reset driver");
> +MODULE_LICENSE("GPL v2");
> 

ping?

Philipp, should I be CCing anyone else? MAINTAINERS only lists you and vger...

--Sean

Re: [PATCH 2/2] reset: Add GPIO-based reset controller

[Philipp Zabel]

Hi Sean,

On Mon, 2021-10-25 at 14:17 -0400, Sean Anderson wrote:
[...]
> ping?
> 
> Philipp, should I be CCing anyone else? MAINTAINERS only lists you and vger...

That is still correct. Maybe it is time to request a dedicated reset
driver mailing list for better visibility.

I was on vacation for a bit, but thankfully Rob already pointed you in
the right direction. I would like this to live in the core and continue
to use the established reset-gpios bindings.

I'm not sure whether adding a full reset_controller_dev for GPIO resets
is necessary. Maybe there is a good reasond for it, but I found adding a
gpio_desc pointer to struct reset_control and implementing non-shared
GPIO resets in the core would be trivial - I prototyped this at some
point (untested, see below).

To support shared resets, the already requested gpio_desc would have to
be found and used to retrieve the corresponding requested reset_control
from a list. This could probably be achieved by exporting gpiod_find()
from gpiolib.

----------8<----------
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 61e688882643..b82acb38deb5 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -7,6 +7,8 @@
 #include <linux/atomic.h>
 #include <linux/device.h>
 #include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/kref.h>
@@ -26,6 +28,8 @@ static LIST_HEAD(reset_lookup_list);
  * struct reset_control - a reset control
  * @rcdev: a pointer to the reset controller device
  *         this reset control belongs to
+ * @gpio: a pointer to the gpio_desc controlling the
+ *        reset line, alternative to rcdev and id.
  * @list: list entry for the rcdev's reset controller list
  * @id: ID of the reset controller in the reset
  *      controller device
@@ -41,6 +45,7 @@ static LIST_HEAD(reset_lookup_list);
 struct reset_control {
 	struct reset_controller_dev *rcdev;
 	struct list_head list;
+	struct gpio_desc *gpio;
 	unsigned int id;
 	struct kref refcnt;
 	bool acquired;
@@ -330,7 +335,8 @@ int reset_control_reset(struct reset_control *rstc)
 	if (!rstc)
 		return 0;
 
-	if (WARN_ON(IS_ERR(rstc)))
+	if (WARN_ON(IS_ERR(rstc)) ||
+	    WARN_ON(rstc->gpio))
 		return -EINVAL;
 
 	if (reset_control_is_array(rstc))
@@ -458,7 +464,14 @@ int reset_control_assert(struct reset_control *rstc)
 
 		if (atomic_dec_return(&rstc->deassert_count) != 0)
 			return 0;
+	}
+
+	if (rstc->gpio) {
+		gpiod_set_value(rstc->gpio, 1);
+		return 0;
+	}
 
+	if (rstc->shared) {
 		/*
 		 * Shared reset controls allow the reset line to be in any state
 		 * after this call, so doing nothing is a valid option.
@@ -551,6 +564,11 @@ int reset_control_deassert(struct reset_control *rstc)
 		}
 	}
 
+	if (rstc->gpio) {
+		gpiod_set_value(rstc->gpio, 0);
+		return 0;
+	}
+
 	/*
 	 * If the reset controller does not implement .deassert(), we assume
 	 * that it handles self-deasserting reset lines via .reset(). In that
@@ -559,7 +577,7 @@ int reset_control_deassert(struct reset_control *rstc)
 	 * return -ENOTSUPP.
 	 */
 	if (!rstc->rcdev->ops->deassert)
-		return 0;
+		return -ENOTSUPP;
 
 	return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->id);
 }
@@ -609,8 +627,10 @@ int reset_control_status(struct reset_control *rstc)
 	if (WARN_ON(IS_ERR(rstc)) || reset_control_is_array(rstc))
 		return -EINVAL;
 
-	if (rstc->rcdev->ops->status)
+	if (rstc->rcdev && rstc->rcdev->ops->status)
 		return rstc->rcdev->ops->status(rstc->rcdev, rstc->id);
+	if (rstc->gpio)
+		return gpiod_get_value(rstc->gpio);
 
 	return -ENOTSUPP;
 }
@@ -796,9 +816,11 @@ static void __reset_control_release(struct kref *kref)
 
 	lockdep_assert_held(&reset_list_mutex);
 
-	module_put(rstc->rcdev->owner);
+	if (rstc->rcdev) {
+		module_put(rstc->rcdev->owner);
 
-	list_del(&rstc->list);
+		list_del(&rstc->list);
+	}
 	kfree(rstc);
 }
 
@@ -935,19 +957,66 @@ __reset_control_get_from_lookup(struct device *dev, const char *con_id,
 	return rstc;
 }
 
+static struct reset_control *__reset_control_get_gpio(struct device *dev,
+						      const char *id,
+						      bool optional,
+						      bool acquired)
+{
+	struct reset_control *rstc;
+	struct gpio_desc *gpio;
+	char prop[32];
+
+	if (id)
+		snprintf(prop, sizeof(prop), "%s-reset", id);
+	gpio = gpiod_get_optional(dev, id ? prop : "reset", GPIOD_ASIS);
+	if (IS_ERR(gpio))
+		return ERR_CAST(gpio);
+	if (!gpio)
+		return optional ? NULL : ERR_PTR(-ENOENT);
+
+	rstc = kzalloc(sizeof(*rstc), GFP_KERNEL);
+	if (!rstc) {
+		gpiod_put(gpio);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	/*
+	 * Set the reset GPIO to output, deasserted, unless the GPIO is already
+	 * set to output, asserted.
+	 */
+	if (!(gpiod_get_direction(gpio) == GPIOF_DIR_OUT &&
+	      gpiod_get_value(gpio) == 1))
+		gpiod_direction_output(gpio, 0);
+
+	rstc->gpio = gpio;
+	rstc->refcnt = 1;
+	rstc->acquired = acquired;
+	rstc->shared = false;
+
+	return rstc;
+}
+
 struct reset_control *__reset_control_get(struct device *dev, const char *id,
 					  int index, bool shared, bool optional,
 					  bool acquired)
 {
+	struct reset_control *rstc = ERR_PTR(-ENOENT);
+
 	if (WARN_ON(shared && acquired))
 		return ERR_PTR(-EINVAL);
 
 	if (dev->of_node)
-		return __of_reset_control_get(dev->of_node, id, index, shared,
+		rstc = __of_reset_control_get(dev->of_node, id, index, shared,
 					      optional, acquired);
+	if ((rstc == NULL || PTR_ERR(rstc) == -ENOENT) && index == 0 &&
+	    shared == 0)
+		rstc = __reset_control_get_gpio(dev, id, optional, acquired);
+
+	if (rstc == NULL || PTR_ERR(rstc) == -ENOENT)
+		rstc = __reset_control_get_from_lookup(dev, id, shared,
+						       optional, acquired);
 
-	return __reset_control_get_from_lookup(dev, id, shared, optional,
-					       acquired);
+	return rstc;
 }
 EXPORT_SYMBOL_GPL(__reset_control_get);
 
---------->8----------

regards
Philipp

Re: [PATCH 1/2] dt-bindings: reset: Add generic GPIO reset binding

[Rob Herring]

On Mon, Oct 18, 2021 at 07:49:21PM -0400, Sean Anderson wrote:
> This adds a binding for a generic GPIO reset driver. This driver is
> designed to easily add a GPIO-based reset to a driver which expected a
> reset controller. It offers greater flexibility than a reset-gpios
> property, and allows for one code path to be shared for GPIO resets and
> MMIO-based resets.

I would like to do this last part, but not requiring a binding change. 
IOW, be able to register any 'reset-gpios' property as a reset provider 
directly without this added level of indirection.

> 
> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> ---
> 
>  .../devicetree/bindings/reset/gpio-reset.yaml | 93 +++++++++++++++++++
>  1 file changed, 93 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/gpio-reset.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.yaml b/Documentation/devicetree/bindings/reset/gpio-reset.yaml
> new file mode 100644
> index 000000000000..de2ab074cea3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/gpio-reset.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)

GPL-2.0-only not GPL-2.0+

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/gpio-reset.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic GPIO reset driver
> +
> +maintainers:
> +  - Sean Anderson <seanga2@gmail.com>
> +
> +description: |
> +  This is a generic GPIO reset driver which can provide a reset-controller
> +  interface for GPIO-based reset lines. This driver always operates with
> +  logical GPIO values; to invert the polarity, specify GPIO_ACTIVE_LOW in the
> +  GPIO's flags.
> +
> +properties:
> +  compatible:
> +    const: gpio-reset
> +
> +  '#reset-cells':
> +    const: 1
> +
> +  reset-gpios:
> +    description: |
> +      GPIOs to assert when asserting a reset. There is a one-to-one mapping
> +      between the reset specifier and the index of the GPIO in this list to
> +      assert.
> +
> +  done-gpios:
> +    description: |
> +      GPIOs which indicate that the device controlled by the GPIO has exited
> +      reset. There must be one done GPIO for each reset GPIO, or no done GPIOs
> +      at all. The driver will wait for up to done-timeout-us for the
> +      corresponding done GPIO to assert before returning.

This is odd. Do you have some examples of h/w needing this done signal? 
It certainly doesn't seem like something we have a generic need for.

> +
> +  pre-assert-us:
> +    default: 0
> +    description: |
> +      Microseconds to delay between when the reset was requested to be
> +      asserted, and asserting the reset GPIO
> +
> +  post-assert-us:
> +    default: 0
> +    description: |
> +      Microseconds to delay after asserting the reset GPIO and before returning
> +      to the caller.
> +
> +  pre-deassert-us:
> +    default: 0
> +    description: |
> +      Microseconds to delay between when the reset was requested to be
> +      deasserted, and asserting the reset GPIO
> +
> +  post-deassert-us:
> +    default: 0
> +    description: |
> +      Microseconds to delay after deasserting the reset GPIO and before
> +      returning to the caller. This delay is always present, even if the done
> +      GPIO goes high earlier.
> +
> +  done-timeout-us:
> +    default: 1000
> +    description:
> +      Microseconds to wait for the done GPIO to assert after deasserting the
> +      reset GPIO. If post-deassert-us is present, this property defaults to 10
> +      times that delay. The timeout starts after waiting for the post deassert
> +      delay.

There's a reason we don't have all these timing values in DT. The timing 
requirements are defined by each device (being reset) and implied by 
their compatible strings. If we wanted a macro language for power 
sequence timings of regulators, clocks, resets, enables, etc., then we 
would have designed such a thing already.

> +
> +required:
> +  - '#reset-cells'
> +  - compatible
> +  - reset-gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    pcs_reset: reset-pcs {
> +        #reset-cells = <1>;
> +        compatible = "gpio-reset";
> +        reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>,
> +                      <&gpio 1 GPIO_ACTIVE_LOW>,
> +                      <&gpio 2 GPIO_ACTIVE_LOW>,
> +                      <&gpio 3 GPIO_ACTIVE_LOW>;
> +        done-gpios = <&gpio 4 GPIO_ACTIVE_HIGH>,
> +                     <&gpio 5 GPIO_ACTIVE_HIGH>,
> +                     <&gpio 6 GPIO_ACTIVE_HIGH>,
> +                     <&gpio 7 GPIO_ACTIVE_HIGH>;
> +        post-deassert-us = <100>;
> +    };
> -- 
> 2.25.1
> 
> 

Re: [PATCH 1/2] dt-bindings: reset: Add generic GPIO reset binding

[Sean Anderson]

Hi Rob,

On 10/26/21 10:27 PM, Rob Herring wrote:
> On Mon, Oct 18, 2021 at 07:49:21PM -0400, Sean Anderson wrote:
>> This adds a binding for a generic GPIO reset driver. This driver is
>> designed to easily add a GPIO-based reset to a driver which expected a
>> reset controller. It offers greater flexibility than a reset-gpios
>> property, and allows for one code path to be shared for GPIO resets and
>> MMIO-based resets.
>
> I would like to do this last part, but not requiring a binding change.
> IOW, be able to register any 'reset-gpios' property as a reset provider
> directly without this added level of indirection.

That would be nice, but it seems like someone would have to go through
every driver with a reset-gpios property and convert them. Since the
reset GPIOs are

>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>>
>>  .../devicetree/bindings/reset/gpio-reset.yaml | 93 +++++++++++++++++++
>>  1 file changed, 93 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/reset/gpio-reset.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.yaml b/Documentation/devicetree/bindings/reset/gpio-reset.yaml
>> new file mode 100644
>> index 000000000000..de2ab074cea3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/gpio-reset.yaml
>> @@ -0,0 +1,93 @@
>> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)
>
> GPL-2.0-only not GPL-2.0+

GPL-2.0+ is a strict superset. And bindings are required to be BSD
anyway. I don't see the issue.

>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/reset/gpio-reset.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Generic GPIO reset driver
>> +
>> +maintainers:
>> +  - Sean Anderson <seanga2@gmail.com>
>> +
>> +description: |
>> +  This is a generic GPIO reset driver which can provide a reset-controller
>> +  interface for GPIO-based reset lines. This driver always operates with
>> +  logical GPIO values; to invert the polarity, specify GPIO_ACTIVE_LOW in the
>> +  GPIO's flags.
>> +
>> +properties:
>> +  compatible:
>> +    const: gpio-reset
>> +
>> +  '#reset-cells':
>> +    const: 1
>> +
>> +  reset-gpios:
>> +    description: |
>> +      GPIOs to assert when asserting a reset. There is a one-to-one mapping
>> +      between the reset specifier and the index of the GPIO in this list to
>> +      assert.
>> +
>> +  done-gpios:
>> +    description: |
>> +      GPIOs which indicate that the device controlled by the GPIO has exited
>> +      reset. There must be one done GPIO for each reset GPIO, or no done GPIOs
>> +      at all. The driver will wait for up to done-timeout-us for the
>> +      corresponding done GPIO to assert before returning.
>
> This is odd. Do you have some examples of h/w needing this done signal?
> It certainly doesn't seem like something we have a generic need for.

Yes [1]. This device has a "reset done" signal, but no reset timings
specified in the datasheet. I don't know if this is truly needed,
because we can read the ID register, but it is nice when bringing up the
device. I left it in because I was using it.

[1] https://lore.kernel.org/netdev/20211004191527.1610759-16-sean.anderson@seco.com/

>> +
>> +  pre-assert-us:
>> +    default: 0
>> +    description: |
>> +      Microseconds to delay between when the reset was requested to be
>> +      asserted, and asserting the reset GPIO
>> +
>> +  post-assert-us:
>> +    default: 0
>> +    description: |
>> +      Microseconds to delay after asserting the reset GPIO and before returning
>> +      to the caller.
>> +
>> +  pre-deassert-us:
>> +    default: 0
>> +    description: |
>> +      Microseconds to delay between when the reset was requested to be
>> +      deasserted, and asserting the reset GPIO
>> +
>> +  post-deassert-us:
>> +    default: 0
>> +    description: |
>> +      Microseconds to delay after deasserting the reset GPIO and before
>> +      returning to the caller. This delay is always present, even if the done
>> +      GPIO goes high earlier.
>> +
>> +  done-timeout-us:
>> +    default: 1000
>> +    description:
>> +      Microseconds to wait for the done GPIO to assert after deasserting the
>> +      reset GPIO. If post-deassert-us is present, this property defaults to 10
>> +      times that delay. The timeout starts after waiting for the post deassert
>> +      delay.
>
> There's a reason we don't have all these timing values in DT. The timing
> requirements are defined by each device (being reset) and implied by
> their compatible strings. If we wanted a macro language for power
> sequence timings of regulators, clocks, resets, enables, etc., then we
> would have designed such a thing already.

Well, there are already things like reset-assert-us and
reset-deassert-us in [2, 3, 4] (with different names(!)). Part of what I
want to address with this device is that there are several existing
properties which specify various aspects of the above timings. I think
it would be good to standardize on these. Maybe this should be a
property which applies to the reset consumer? Analogously, we also
have assigned-clocks so that not every driver has to know what the
correct frequency/parent is (especially when they can vary among
different hardware variations).

--Sean

[2] Documentation/devicetree/bindings/net/ethernet-phy.yaml
[3] Documentation/devicetree/bindings/net/mdio.yaml
[4] Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml

>> +
>> +required:
>> +  - '#reset-cells'
>> +  - compatible
>> +  - reset-gpios
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    pcs_reset: reset-pcs {
>> +        #reset-cells = <1>;
>> +        compatible = "gpio-reset";
>> +        reset-gpios = <&gpio 0 GPIO_ACTIVE_LOW>,
>> +                      <&gpio 1 GPIO_ACTIVE_LOW>,
>> +                      <&gpio 2 GPIO_ACTIVE_LOW>,
>> +                      <&gpio 3 GPIO_ACTIVE_LOW>;
>> +        done-gpios = <&gpio 4 GPIO_ACTIVE_HIGH>,
>> +                     <&gpio 5 GPIO_ACTIVE_HIGH>,
>> +                     <&gpio 6 GPIO_ACTIVE_HIGH>,
>> +                     <&gpio 7 GPIO_ACTIVE_HIGH>;
>> +        post-deassert-us = <100>;
>> +    };
>> --
>> 2.25.1
>>
>>
>

Re: [PATCH 1/2] dt-bindings: reset: Add generic GPIO reset binding

[Sean Anderson]

(I forgot to finish my thought)

On 10/28/21 11:19 AM, Sean Anderson wrote:
> Hi Rob,
>
> On 10/26/21 10:27 PM, Rob Herring wrote:
>> On Mon, Oct 18, 2021 at 07:49:21PM -0400, Sean Anderson wrote:
>>> This adds a binding for a generic GPIO reset driver. This driver is
>>> designed to easily add a GPIO-based reset to a driver which expected a
>>> reset controller. It offers greater flexibility than a reset-gpios
>>> property, and allows for one code path to be shared for GPIO resets and
>>> MMIO-based resets.
>>
>> I would like to do this last part, but not requiring a binding change.
>> IOW, be able to register any 'reset-gpios' property as a reset provider
>> directly without this added level of indirection.
>
> That would be nice, but it seems like someone would have to go through
> every driver with a reset-gpios property and convert them. Since the
> reset GPIOs are
...effectively ad-hoc, I think it would be difficult to do a conversion
in a generic manner (especially since some devices will fail to probe if
they don't get control of the GPIOs). Maybe this could be done with a
single reset driver which has reset GPIOs added to it dynamically? E.g.
when a driver requests a reset named "some-name" and there is a
reset-gpios property, request the GPIOs and assign them to the reset
driver, then hand that reset back to the calling driver.

On the other hand, having a separate driver like this makes it easy to
incrementally convert existing drivers. Although it does require a
change in bindings.

--Sean

Re: [PATCH 1/2] dt-bindings: reset: Add generic GPIO reset binding

[Rob Herring]

On Thu, Oct 28, 2021 at 10:19 AM Sean Anderson <sean.anderson@seco.com> wrote:
>
> Hi Rob,
>
> On 10/26/21 10:27 PM, Rob Herring wrote:
> > On Mon, Oct 18, 2021 at 07:49:21PM -0400, Sean Anderson wrote:
> >> This adds a binding for a generic GPIO reset driver. This driver is
> >> designed to easily add a GPIO-based reset to a driver which expected a
> >> reset controller. It offers greater flexibility than a reset-gpios
> >> property, and allows for one code path to be shared for GPIO resets and
> >> MMIO-based resets.
> >
> > I would like to do this last part, but not requiring a binding change.
> > IOW, be able to register any 'reset-gpios' property as a reset provider
> > directly without this added level of indirection.
>
> That would be nice, but it seems like someone would have to go through
> every driver with a reset-gpios property and convert them. Since the
> reset GPIOs are

All that has to happen is when a driver requests a reset, the reset
subsystem can check for a 'reset-gpios' when there is not a 'resets'
property. If it finds one, then it can either instantiate a reset
provider or add that GPIO to an existing provider. Then you can
convert drivers one by one, or not.

> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
> >> ---
> >>
> >>  .../devicetree/bindings/reset/gpio-reset.yaml | 93 +++++++++++++++++++
> >>  1 file changed, 93 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/reset/gpio-reset.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.yaml b/Documentation/devicetree/bindings/reset/gpio-reset.yaml
> >> new file mode 100644
> >> index 000000000000..de2ab074cea3
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/reset/gpio-reset.yaml
> >> @@ -0,0 +1,93 @@
> >> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)
> >
> > GPL-2.0-only not GPL-2.0+
>
> GPL-2.0+ is a strict superset. And bindings are required to be BSD
> anyway. I don't see the issue.

Not everyone agrees with GPLv3. What about GPLv4, v5, etc.? You're
okay with them no matter what they say?

The issue is many people pay no attention. Just copy whatever they
started from, or put whatever they want. The dts files are a mess. The
binding docs all defaulted to GPL2. So I'm fixing the mess with
bindings and that means dictating the license.

> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/reset/gpio-reset.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Generic GPIO reset driver
> >> +
> >> +maintainers:
> >> +  - Sean Anderson <seanga2@gmail.com>
> >> +
> >> +description: |
> >> +  This is a generic GPIO reset driver which can provide a reset-controller
> >> +  interface for GPIO-based reset lines. This driver always operates with
> >> +  logical GPIO values; to invert the polarity, specify GPIO_ACTIVE_LOW in the
> >> +  GPIO's flags.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: gpio-reset
> >> +
> >> +  '#reset-cells':
> >> +    const: 1
> >> +
> >> +  reset-gpios:
> >> +    description: |
> >> +      GPIOs to assert when asserting a reset. There is a one-to-one mapping
> >> +      between the reset specifier and the index of the GPIO in this list to
> >> +      assert.
> >> +
> >> +  done-gpios:
> >> +    description: |
> >> +      GPIOs which indicate that the device controlled by the GPIO has exited
> >> +      reset. There must be one done GPIO for each reset GPIO, or no done GPIOs
> >> +      at all. The driver will wait for up to done-timeout-us for the
> >> +      corresponding done GPIO to assert before returning.
> >
> > This is odd. Do you have some examples of h/w needing this done signal?
> > It certainly doesn't seem like something we have a generic need for.
>
> Yes [1]. This device has a "reset done" signal, but no reset timings
> specified in the datasheet. I don't know if this is truly needed,
> because we can read the ID register, but it is nice when bringing up the
> device. I left it in because I was using it.

Okay, but done-gpios belongs in the device node that has a done
signal. Your binding pretty assumes you always have one because you
need equal numbers of reset and done gpios.

Anyways, I don't think this binding is going anywhere.

>
> [1] https://lore.kernel.org/netdev/20211004191527.1610759-16-sean.anderson@seco.com/
>
> >> +
> >> +  pre-assert-us:
> >> +    default: 0
> >> +    description: |
> >> +      Microseconds to delay between when the reset was requested to be
> >> +      asserted, and asserting the reset GPIO
> >> +
> >> +  post-assert-us:
> >> +    default: 0
> >> +    description: |
> >> +      Microseconds to delay after asserting the reset GPIO and before returning
> >> +      to the caller.
> >> +
> >> +  pre-deassert-us:
> >> +    default: 0
> >> +    description: |
> >> +      Microseconds to delay between when the reset was requested to be
> >> +      deasserted, and asserting the reset GPIO
> >> +
> >> +  post-deassert-us:
> >> +    default: 0
> >> +    description: |
> >> +      Microseconds to delay after deasserting the reset GPIO and before
> >> +      returning to the caller. This delay is always present, even if the done
> >> +      GPIO goes high earlier.
> >> +
> >> +  done-timeout-us:
> >> +    default: 1000
> >> +    description:
> >> +      Microseconds to wait for the done GPIO to assert after deasserting the
> >> +      reset GPIO. If post-deassert-us is present, this property defaults to 10
> >> +      times that delay. The timeout starts after waiting for the post deassert
> >> +      delay.
> >
> > There's a reason we don't have all these timing values in DT. The timing
> > requirements are defined by each device (being reset) and implied by
> > their compatible strings. If we wanted a macro language for power
> > sequence timings of regulators, clocks, resets, enables, etc., then we
> > would have designed such a thing already.
>
> Well, there are already things like reset-assert-us and
> reset-deassert-us in [2, 3, 4] (with different names(!)).

Yes, things evolve poorly. What's just one more property added at a time.

> Part of what I
> want to address with this device is that there are several existing
> properties which specify various aspects of the above timings. I think
> it would be good to standardize on these. Maybe this should be a
> property which applies to the reset consumer? Analogously, we also
> have assigned-clocks so that not every driver has to know what the
> correct frequency/parent is (especially when they can vary among
> different hardware variations).

Yes, there are some examples, but you won't find many new examples.
The rule is power sequencing requirements/timing is implied by the
device's compatible string.

You are looking at just reset. What about timing WRT regulators and
clocks for starters?

Rob

Re: [PATCH 1/2] dt-bindings: reset: Add generic GPIO reset binding

[Sean Anderson]

On 10/28/21 9:35 PM, Rob Herring wrote:
> On Thu, Oct 28, 2021 at 10:19 AM Sean Anderson <sean.anderson@seco.com> wrote:
>>
>> Hi Rob,
>>
>> On 10/26/21 10:27 PM, Rob Herring wrote:
>> > On Mon, Oct 18, 2021 at 07:49:21PM -0400, Sean Anderson wrote:
>> >> This adds a binding for a generic GPIO reset driver. This driver is
>> >> designed to easily add a GPIO-based reset to a driver which expected a
>> >> reset controller. It offers greater flexibility than a reset-gpios
>> >> property, and allows for one code path to be shared for GPIO resets and
>> >> MMIO-based resets.
>> >
>> > I would like to do this last part, but not requiring a binding change.
>> > IOW, be able to register any 'reset-gpios' property as a reset provider
>> > directly without this added level of indirection.
>>
>> That would be nice, but it seems like someone would have to go through
>> every driver with a reset-gpios property and convert them. Since the
>> reset GPIOs are
> 
> All that has to happen is when a driver requests a reset, the reset
> subsystem can check for a 'reset-gpios' when there is not a 'resets'
> property. If it finds one, then it can either instantiate a reset
> provider or add that GPIO to an existing provider. Then you can
> convert drivers one by one, or not.

I will have a stab at this.

>> >>
>> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> >> ---
>> >>
>> >>  .../devicetree/bindings/reset/gpio-reset.yaml | 93 +++++++++++++++++++
>> >>  1 file changed, 93 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/reset/gpio-reset.yaml
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/reset/gpio-reset.yaml b/Documentation/devicetree/bindings/reset/gpio-reset.yaml
>> >> new file mode 100644
>> >> index 000000000000..de2ab074cea3
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/reset/gpio-reset.yaml
>> >> @@ -0,0 +1,93 @@
>> >> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)
>> >
>> > GPL-2.0-only not GPL-2.0+
>>
>> GPL-2.0+ is a strict superset. And bindings are required to be BSD
>> anyway. I don't see the issue.
> 
> Not everyone agrees with GPLv3. What about GPLv4, v5, etc.? You're
> okay with them no matter what they say?

So you would rather have GPL-2.0-only OR GPL-3.0-only OR BSD-2-Clause :)

> The issue is many people pay no attention. Just copy whatever they
> started from, or put whatever they want. The dts files are a mess. The
> binding docs all defaulted to GPL2. So I'm fixing the mess with
> bindings and that means dictating the license.

Ok

>> >> +%YAML 1.2
>> >> +---
>> >> +$id: http://devicetree.org/schemas/reset/gpio-reset.yaml#
>> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> >> +
>> >> +title: Generic GPIO reset driver
>> >> +
>> >> +maintainers:
>> >> +  - Sean Anderson <seanga2@gmail.com>
>> >> +
>> >> +description: |
>> >> +  This is a generic GPIO reset driver which can provide a reset-controller
>> >> +  interface for GPIO-based reset lines. This driver always operates with
>> >> +  logical GPIO values; to invert the polarity, specify GPIO_ACTIVE_LOW in the
>> >> +  GPIO's flags.
>> >> +
>> >> +properties:
>> >> +  compatible:
>> >> +    const: gpio-reset
>> >> +
>> >> +  '#reset-cells':
>> >> +    const: 1
>> >> +
>> >> +  reset-gpios:
>> >> +    description: |
>> >> +      GPIOs to assert when asserting a reset. There is a one-to-one mapping
>> >> +      between the reset specifier and the index of the GPIO in this list to
>> >> +      assert.
>> >> +
>> >> +  done-gpios:
>> >> +    description: |
>> >> +      GPIOs which indicate that the device controlled by the GPIO has exited
>> >> +      reset. There must be one done GPIO for each reset GPIO, or no done GPIOs
>> >> +      at all. The driver will wait for up to done-timeout-us for the
>> >> +      corresponding done GPIO to assert before returning.
>> >
>> > This is odd. Do you have some examples of h/w needing this done signal?
>> > It certainly doesn't seem like something we have a generic need for.
>>
>> Yes [1]. This device has a "reset done" signal, but no reset timings
>> specified in the datasheet. I don't know if this is truly needed,
>> because we can read the ID register, but it is nice when bringing up the
>> device. I left it in because I was using it.
> 
> Okay, but done-gpios belongs in the device node that has a done
> signal. Your binding pretty assumes you always have one because you
> need equal numbers of reset and done gpios.

Have two devices, one with done GPIOs, and one without.

> Anyways, I don't think this binding is going anywhere.
> 
>>
>> [1] https://lore.kernel.org/netdev/20211004191527.1610759-16-sean.anderson@seco.com/
>>
>> >> +
>> >> +  pre-assert-us:
>> >> +    default: 0
>> >> +    description: |
>> >> +      Microseconds to delay between when the reset was requested to be
>> >> +      asserted, and asserting the reset GPIO
>> >> +
>> >> +  post-assert-us:
>> >> +    default: 0
>> >> +    description: |
>> >> +      Microseconds to delay after asserting the reset GPIO and before returning
>> >> +      to the caller.
>> >> +
>> >> +  pre-deassert-us:
>> >> +    default: 0
>> >> +    description: |
>> >> +      Microseconds to delay between when the reset was requested to be
>> >> +      deasserted, and asserting the reset GPIO
>> >> +
>> >> +  post-deassert-us:
>> >> +    default: 0
>> >> +    description: |
>> >> +      Microseconds to delay after deasserting the reset GPIO and before
>> >> +      returning to the caller. This delay is always present, even if the done
>> >> +      GPIO goes high earlier.
>> >> +
>> >> +  done-timeout-us:
>> >> +    default: 1000
>> >> +    description:
>> >> +      Microseconds to wait for the done GPIO to assert after deasserting the
>> >> +      reset GPIO. If post-deassert-us is present, this property defaults to 10
>> >> +      times that delay. The timeout starts after waiting for the post deassert
>> >> +      delay.
>> >
>> > There's a reason we don't have all these timing values in DT. The timing
>> > requirements are defined by each device (being reset) and implied by
>> > their compatible strings. If we wanted a macro language for power
>> > sequence timings of regulators, clocks, resets, enables, etc., then we
>> > would have designed such a thing already.
>>
>> Well, there are already things like reset-assert-us and
>> reset-deassert-us in [2, 3, 4] (with different names(!)).
> 
> Yes, things evolve poorly. What's just one more property added at a time.
> 
>> Part of what I
>> want to address with this device is that there are several existing
>> properties which specify various aspects of the above timings. I think
>> it would be good to standardize on these. Maybe this should be a
>> property which applies to the reset consumer? Analogously, we also
>> have assigned-clocks so that not every driver has to know what the
>> correct frequency/parent is (especially when they can vary among
>> different hardware variations).
> 
> Yes, there are some examples, but you won't find many new examples.
> The rule is power sequencing requirements/timing is implied by the
> device's compatible string.
> 
> You are looking at just reset. What about timing WRT regulators and
> clocks for starters?

*shrug*

This just seems like a very common area which needs to customized.

--Sean