reset: Add driver for gpio-controlled reset pins

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

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

Reviewed-by Stephen Warren <[email protected]>

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

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

Reviewed-by Stephen Warren <[email protected]>

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

Documentation/devicetree/bindings/reset/gpio-reset.txt new file +25 -0

new file mode 100644
index 0000000..bca5348
@@ -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>;

Quoted 38 lines · Philipp Zabel

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

Documentation/devicetree/bindings/reset/gpio-reset.txt +32 -0

 
+GPIO reset controller
+=====================
+
+A GPIO reset controller controls a single GPIO that is connected to the reset
+pin of a peripheral IC. Please also refer to reset.txt in this directory for
+common reset controller binding usage.
+
+Required properties:
+- compatible: Should be "gpio-reset"
+- reset-gpios: A gpio used as reset line. The gpio specifier for this property
+               depends on the gpio controller that provides the gpio.
+- #reset-cells: 0, see below
+
+Optional properties:
+- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for
+                  this duration to reset.
+- initially-in-reset: boolean. If not set, the initial state should be a
+                      deasserted reset line. If this property exists, the
+                      reset line should be kept in reset.
+
+example:
+
+sii902x_reset: gpio-reset {
+	compatible = "gpio-reset";
+	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
+	reset-delay-us = <10000>;
+	initially-in-reset;
+	#reset-cells = <0>;
+};
+
+/* Device with nRESET pin connected to GPIO5_0 */
+sii902x at 39 {

Documentation/devicetree/bindings/reset/gpio-reset.txt line 35

Quoted 3 lines

Documentation/devicetree/bindings/reset/gpio-reset.txt +3 -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.

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

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

I think this should look more like the below:

/* Device with nRESET pin connected to GPIO5_0 */

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

Quoted 39 lines · Mark Rutland

Quoted 38 lines · Philipp Zabel

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

Documentation/devicetree/bindings/reset/gpio-reset.txt +32 -0

 
+GPIO reset controller
+=====================
+
+A GPIO reset controller controls a single GPIO that is connected to the reset
+pin of a peripheral IC. Please also refer to reset.txt in this directory for
+common reset controller binding usage.
+
+Required properties:
+- compatible: Should be "gpio-reset"
+- reset-gpios: A gpio used as reset line. The gpio specifier for this property
+               depends on the gpio controller that provides the gpio.
+- #reset-cells: 0, see below
+
+Optional properties:
+- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for
+                  this duration to reset.
+- initially-in-reset: boolean. If not set, the initial state should be a
+                      deasserted reset line. If this property exists, the
+                      reset line should be kept in reset.
+
+example:
+
+sii902x_reset: gpio-reset {
+	compatible = "gpio-reset";
+	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
+	reset-delay-us = <10000>;
+	initially-in-reset;
+	#reset-cells = <0>;
+};
+
+/* Device with nRESET pin connected to GPIO5_0 */
+sii902x at 39 {

Quoted 8 lines

Documentation/devicetree/bindings/reset/gpio-reset.txt line 35

Quoted 3 lines

Documentation/devicetree/bindings/reset/gpio-reset.txt +3 -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.

Quoted 14 lines

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 */

Quoted 10 lines

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

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

Quoted 79 lines · Stephen Warren

Quoted 39 lines · Mark Rutland

Quoted 38 lines · Philipp Zabel

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

Documentation/devicetree/bindings/reset/gpio-reset.txt +32 -0

 
+GPIO reset controller
+=====================
+
+A GPIO reset controller controls a single GPIO that is connected to the reset
+pin of a peripheral IC. Please also refer to reset.txt in this directory for
+common reset controller binding usage.
+
+Required properties:
+- compatible: Should be "gpio-reset"
+- reset-gpios: A gpio used as reset line. The gpio specifier for this property
+               depends on the gpio controller that provides the gpio.
+- #reset-cells: 0, see below
+
+Optional properties:
+- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for
+                  this duration to reset.
+- initially-in-reset: boolean. If not set, the initial state should be a
+                      deasserted reset line. If this property exists, the
+                      reset line should be kept in reset.
+
+example:
+
+sii902x_reset: gpio-reset {
+	compatible = "gpio-reset";
+	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
+	reset-delay-us = <10000>;
+	initially-in-reset;
+	#reset-cells = <0>;
+};
+
+/* Device with nRESET pin connected to GPIO5_0 */
+sii902x at 39 {

Quoted 8 lines

Documentation/devicetree/bindings/reset/gpio-reset.txt line 35

Quoted 3 lines

Documentation/devicetree/bindings/reset/gpio-reset.txt +3 -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.

Quoted 3 lines

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

Quoted 57 lines

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

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

Reviewed-by Stephen Warren <[email protected]>

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

Documentation/devicetree/bindings/reset/gpio-reset.txt new file +32 -0

new file mode 100644
index 0000000..bca5348
@@ -0,0 +1,35 @@
+GPIO reset controller
+=====================
+
+A GPIO reset controller controls a single GPIO that is connected to the reset
+pin of a peripheral IC. Please also refer to reset.txt in this directory for
+common reset controller binding usage.
+
+Required properties:
+- compatible: Should be "gpio-reset"
+- reset-gpios: A gpio used as reset line. The gpio specifier for this property
+               depends on the gpio controller that provides the gpio.
+- #reset-cells: 0, see below
+
+Optional properties:
+- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for
+                  this duration to reset.
+- initially-in-reset: boolean. If not set, the initial state should be a
+                      deasserted reset line. If this property exists, the
+                      reset line should be kept in reset.
+
+example:
+
+sii902x_reset: gpio-reset {
+	compatible = "gpio-reset";
+	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
+	reset-delay-us = <10000>;
+	initially-in-reset;
+	#reset-cells = <0>;
+};
+
+/* Device with nRESET pin connected to GPIO5_0 */
+sii902x at 39 {

Documentation/devicetree/bindings/reset/gpio-reset.txt line 35

Quoted 3 lines

Documentation/devicetree/bindings/reset/gpio-reset.txt new file +3 -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 */

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

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

Hi Mark,

Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:

Quoted 58 lines · Philipp Zabel

Quoted 57 lines

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

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

Reviewed-by Stephen Warren <[email protected]>

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

Documentation/devicetree/bindings/reset/gpio-reset.txt new file +32 -0

new file mode 100644
index 0000000..bca5348
@@ -0,0 +1,35 @@
+GPIO reset controller
+=====================
+
+A GPIO reset controller controls a single GPIO that is connected to the reset
+pin of a peripheral IC. Please also refer to reset.txt in this directory for
+common reset controller binding usage.
+
+Required properties:
+- compatible: Should be "gpio-reset"
+- reset-gpios: A gpio used as reset line. The gpio specifier for this property
+               depends on the gpio controller that provides the gpio.
+- #reset-cells: 0, see below
+
+Optional properties:
+- reset-delay-us: delay in microseconds. The gpio reset line will be asserted for
+                  this duration to reset.
+- initially-in-reset: boolean. If not set, the initial state should be a
+                      deasserted reset line. If this property exists, the
+                      reset line should be kept in reset.
+
+example:
+
+sii902x_reset: gpio-reset {
+	compatible = "gpio-reset";
+	reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
+	reset-delay-us = <10000>;
+	initially-in-reset;
+	#reset-cells = <0>;
+};
+
+/* Device with nRESET pin connected to GPIO5_0 */
+sii902x at 39 {

Quoted 8 lines

Documentation/devicetree/bindings/reset/gpio-reset.txt line 35

Quoted 3 lines

Documentation/devicetree/bindings/reset/gpio-reset.txt new file +3 -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.

Quoted 13 lines

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 */

Quoted 10 lines

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

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

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

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

If there is a fixed delay (or a delay calculated from a known clock), we can just extend the API:

	device_reset_usleep(dev, 1000);
instead of currently:
	device_reset(dev);

Quoted 1 line · Philipp Zabel

Hi Mark,

Hi Philipp,

Quoted 1 line

Quoted 1 line

Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:

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

Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:

Quoted 4 lines · Philipp Zabel

Quoted 3 lines

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.

Quoted 13 lines

I think this should look more like the below:

/* Device with nRESET pin connected to GPIO5_0 */

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

I don't like the arbitrary name, as that makes it difficult to handle this in an automated way. In this case I'd prefer to use 'reset-gpios' and optionally 'reset-gpio-names' analogous to how clocks and interrupts (and resets) are handled.

Quoted 5 lines

Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:

Quoted 4 lines · Philipp Zabel

Quoted 3 lines

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.

...

Quoted 14 lines

Quoted 13 lines

I think this should look more like the below:

/* Device with nRESET pin connected to GPIO5_0 */

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

Quoted 4 lines

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.

Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren:

Quoted 26 lines · Philipp Zabel

Quoted 5 lines

Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:

Quoted 4 lines · Philipp Zabel

Quoted 3 lines

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.

...

Quoted 14 lines

Quoted 13 lines

I think this should look more like the below:

/* Device with nRESET pin connected to GPIO5_0 */

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

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 32 lines

Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren:

Quoted 26 lines · Philipp Zabel

Quoted 5 lines

Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:

Quoted 4 lines · Philipp Zabel

Quoted 3 lines

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.

...

Quoted 14 lines

Quoted 13 lines

I think this should look more like the below:

/* Device with nRESET pin connected to GPIO5_0 */

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

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 8 lines

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

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

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

Quoted 7 lines

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

Quoted 9 lines

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.

Hi Stephen

Am Dienstag, den 06.08.2013, 10:59 -0600 schrieb Stephen Warren:

Quoted 42 lines · Philipp Zabel

Quoted 40 lines

Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren:

Quoted 26 lines · Philipp Zabel

Quoted 5 lines

Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:

Quoted 4 lines · Philipp Zabel

Quoted 3 lines

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.

...

Quoted 14 lines

Quoted 13 lines

I think this should look more like the below:

/* Device with nRESET pin connected to GPIO5_0 */

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

Quoted 4 lines

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.

Quoted 4 lines

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

Quoted 4 lines

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.

Quoted 7 lines

Quoted 7 lines

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.

Quoted 10 lines

Quoted 9 lines

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.

Quoted 1 line

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.

Quoted 45 lines

Hi Stephen

Am Dienstag, den 06.08.2013, 10:59 -0600 schrieb Stephen Warren:

Quoted 42 lines · Philipp Zabel

Quoted 40 lines

Am Montag, den 05.08.2013, 11:24 -0600 schrieb Stephen Warren:

Quoted 26 lines · Philipp Zabel

Quoted 5 lines

Am Freitag, den 02.08.2013, 10:28 +0100 schrieb Mark Rutland:

Quoted 4 lines · Philipp Zabel

Quoted 3 lines

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.

...

Quoted 14 lines

Quoted 13 lines

I think this should look more like the below:

/* Device with nRESET pin connected to GPIO5_0 */

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

Quoted 4 lines

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.

Quoted 4 lines

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

Quoted 8 lines

Quoted 4 lines

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";

Quoted 4 lines

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.

Am Donnerstag, den 08.08.2013, 12:43 -0600 schrieb Stephen Warren:

Quoted 8 lines

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:

Quoted 8 lines

old:

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

new:

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

Correct. In this case:

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

Maybe encapsulate this in a convenience wrapper:

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