[RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Peter Ujfalusi]

Hi,

The shared GPIO line for external components tends to be a common issue and
there is no 'clean' way of handling it.

I'm aware of the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag, which must be provided when
a driver tries to request a GPIO which is already in use.
However the driver must know that the component is going to be used in such a
way, which can be said to any external components with GPIO line, so in theory
all drivers must set this flag when requesting the GPIO...

But with the GPIOD_FLAGS_BIT_NONEXCLUSIVE all clients have full control of the
GPIO line. For example any device using the same GPIO as reset/enable line can
reset/enable other devices, which is not something the other device might like
or can handle.
For example a device needs to be configured after it is enabled, but some other
driver would reset it while handling the same GPIO -> the device is not
operational anymmore as it lost it's configuration.

With the gpio-shared gpiochip we can overcome this by giving the gpio-shared
the role of making sure that the GPIO line only changes state when it will not
disturb any of the clients sharing the same GPIO line.

The 'sticky' state of the line depends on the board design, which can be
communicated with the hold-active-state property:

GPIO_ACTIVE_HIGH: the line must be high as long as any of the clients want it to
be high
GPIO_ACTIVE_LOW: the line must be low as long as any of the clients want it to
be low

In board DTS files it is just adding the node to descibe the shared GPIO line
and point the users of this line to the shared-gpio node instead of the real
GPIO.

Something like this:

codec_reset: gpio-shared0 {
	compatible = "gpio-shared";
	gpio-controller;
	#gpio-cells = <2>;

	root-gpios = <&audio_exp 0 GPIO_ACTIVE_HIGH>;

	branch-count = <2>;
	hold-active-state = <GPIO_ACTIVE_HIGH>;
};

&main_i2c3 {
	audio_exp: gpio@21 {
		compatible = "ti,tca6416";
		reg = <0x21>;
		gpio-controller;
		#gpio-cells = <2>;
	};

	pcm3168a_a: audio-codec@47 {
		compatible = "ti,pcm3168a";
		reg = <0x47>;

		#sound-dai-cells = <1>;

		rst-gpios = <&codec_reset 0 GPIO_ACTIVE_HIGH>;
		...
	};

	pcm3168a_b: audio-codec@46 {
		compatible = "ti,pcm3168a";
		reg = <0x46>;

		#sound-dai-cells = <1>;

		rst-gpios = <&codec_reset 1 GPIO_ACTIVE_HIGH>;
		...
	};
};

If any of the codec requests the GPIO to be high, the line will go up and will
only going to be low when both of them set's their shared line to low.

Note: other option would be to have something similar to gpio-hog (gpio-shared)
support in the core itself, but then all of the logic and state handling for the
users of the shared line needs to be moved there.
Simply counting the low and high requests would not work as the GPIO framework
by design does not refcounts the state, iow gpio_set(0) three times and
gpio_set(1) would set the line high.

I have also looked at the reset framework, but again it can not be applied in a
generic way for GPIOs shared for other purposes and all existing drivers must
be converted to use the reset framework (and adding a linux only warpper on top
of reset GPIOs).

Regards,
Peter
---
Peter Ujfalusi (2):
  dt-bindings: gpio: Add binding document for shared GPIO
  gpio: Add new driver for handling 'shared' gpio lines on boards

 .../devicetree/bindings/gpio/gpio-shared.yaml | 100 ++++++++
 drivers/gpio/Kconfig                          |   6 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-shared.c                    | 229 ++++++++++++++++++
 4 files changed, 336 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-shared.yaml
 create mode 100644 drivers/gpio/gpio-shared.c

-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[RFC v2 1/2] dt-bindings: gpio: Add binding document for shared GPIO

[Peter Ujfalusi]

Some board design opts to use the same GPIO line routed to different
onboard components.

The GPIO in question might be a reset line, enable line or mode selection
line, etc.
The drivers for the components do not know if in some board they have
dedicated GPIO on other boards they might share a GPIO line with other
entities, not necessary from the same class:

Two codec sharing the same enable line
One codec and one amplifier sharing the same line
Regulators sharing the same line
Display panels, backlights and touchscreen controllers

And any variation of these.

There is one thing usually the board designers make sure that the level
needed for the GPIO is matching for the components.

The shared GPIO bindings can be used to describe the board level split of a
single GPIO line.

We have two cases to take care:
1. GPIO line should be LOW to enable any of the components
if any of the shared line is requested to be LOW, set the GPIO line low

2. GPIO line should be HIGH to enable any of the components
if any of the shared line is requested to be HIGH, set the GPIO line high

At the end it is:
1. logical AND for the shared lines
2. logical OR for the shared lines

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 .../devicetree/bindings/gpio/gpio-shared.yaml | 100 ++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-shared.yaml

diff --git a/Documentation/devicetree/bindings/gpio/gpio-shared.yaml b/Documentation/devicetree/bindings/gpio/gpio-shared.yaml
new file mode 100644
index 000000000000..30dbd8f6d2a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-shared.yaml
@@ -0,0 +1,100 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-shared.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bindings for shared GPIO lines in board level
+
+maintainers:
+  - Linus Walleij <linus.walleij@linaro.org>
+  - Bartosz Golaszewski <bgolaszewski@baylibre.com>
+  - Peter Ujfalusi <peter.ujfalusi@ti.com>
+
+description: |
+  Some board design opts to use the same GPIO line routed to different
+  onboard components.
+  
+  The GPIO in question might be a reset line, enable line or mode selection
+  line, etc.
+  The drivers for the components do not know if in some board they have
+  dedicated GPIO on other boards they might share a GPIO line with other
+  entities, not necessary from the same class:
+  
+  Two codec sharing the same enable line
+  One codec and one amplifier sharing the same line
+  Regulators sharing the same line
+  Display panels, backlights and touchscreen controllers
+  
+  And any variation of these.
+  
+  There is one thing usually the board designers make sure that the level
+  needed for the GPIO is matching for the components.
+  
+  The shared GPIO bindings can be used to describe the board level split of a
+  single GPIO line.
+  
+  We have two cases to take care:
+  1. GPIO line should be LOW to enable any of the components
+  if any of the shared line is requested to be LOW, set the GPIO line low
+  
+  2. GPIO line should be HIGH to enable any of the components
+  if any of the shared line is requested to be HIGH, set the GPIO line high
+  
+  At the end it is:
+  1. logical AND for the shared lines
+  2. logical OR for the shared lines
+
+properties:
+  compatible:
+    items:
+      - const: gpio-shared
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+  root-gpios:
+    description: |
+      The shared GPIO line
+    maxItems: 1
+
+  branch-count:
+    description: |
+      Number of users of the shared GPIO line
+    maxItems: 1
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+
+  hold-active-state:
+    description: |
+      The active level of the GPIO line for all of the clients:
+      GPIO_ACTIVE_HIGH: if the GPIO must be high for the components,
+      GPIO_ACTIVE_LOW: if the GPIO must be low for the components
+      to enable them.
+    maxItems: 1
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+  
+required:
+  - compatible
+  - "#gpio-cells"
+  - gpio-controller
+  - root-gpios
+  - branch-count
+  - hold-active-state
+
+examples:
+  - |+
+    #include <dt-bindings/gpio/gpio.h>
+    codec_reset: gpio-shared0 {
+        compatible = "gpio-shared";
+        gpio-controller;
+        #gpio-cells = <2>;
+        
+        root-gpios = <&audio_exp 0 GPIO_ACTIVE_HIGH>;
+        
+        branch-count = <2>;
+        hold-active-state = <GPIO_ACTIVE_HIGH>;
+    };
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[RFC v2 2/2] gpio: Add new driver for handling 'shared' gpio lines on boards

[Peter Ujfalusi]

Some board design opts to use the same GPIO line routed to different
onboard components.

The GPIO in question might be a reset line, enable line or mode selection
line, etc.
The drivers for the components do not know if in some board they have
dedicated GPIO on other boards they might share a GPIO line with other
entities, not necessary from the same class:

Two codec sharing the same enable line
One codec and one amplifier sharing the same line
Regulators sharing the same line
Display panels, backlights and touchscreen controllers

And any variation of these.

There is one thing usually the board designers make sure that the level
needed for the GPIO is matching for the components.

This driver adds a gpiochip to handle the board level split of a single
GPIO line and based on the active users of the line it will handle the
real GPIO to a level it should be:

We have two cases to take care:
1. GPIO line should be LOW to enable any of the components
if any of the shared line is requested to be LOW, set the GPIO line low

2. GPIO line should be HIGH to enable any of the components
if any of the shared line is requested to be HIGH, set the GPIO line high

At the end it is:
1. logical AND for the shared lines
2. logical OR for the shared lines

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/gpio/Kconfig       |   6 +
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-shared.c | 229 +++++++++++++++++++++++++++++++++++++
 3 files changed, 236 insertions(+)
 create mode 100644 drivers/gpio/gpio-shared.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 088a8a0f8add..29585a13670e 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -69,6 +69,12 @@ config GPIO_SYSFS
 	  ioctl() operations instead. The character device is always
 	  available.
 
+config GPIO_SHARED
+	tristate "Driver for handling shared GPIO lines"
+	depends on OF_GPIO
+	help
+	  When a single GPIO line is connected to different peripherals.
+
 config GPIO_GENERIC
 	depends on HAS_IOMEM # Only for IOMEM drivers
 	tristate
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index e4599f90f702..f368268cbd3a 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -123,6 +123,7 @@ obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
 obj-$(CONFIG_GPIO_SCH311X)		+= gpio-sch311x.o
 obj-$(CONFIG_GPIO_SCH)			+= gpio-sch.o
 obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
+obj-$(CONFIG_GPIO_SHARED)		+= gpio-shared.o
 obj-$(CONFIG_GPIO_SODAVILLE)		+= gpio-sodaville.o
 obj-$(CONFIG_GPIO_SPEAR_SPICS)		+= gpio-spear-spics.o
 obj-$(CONFIG_GPIO_SPRD)			+= gpio-sprd.o
diff --git a/drivers/gpio/gpio-shared.c b/drivers/gpio/gpio-shared.c
new file mode 100644
index 000000000000..37affc40cdf8
--- /dev/null
+++ b/drivers/gpio/gpio-shared.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *  Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com
+ *  Author: Peter Ujfalusi <peter.ujfalusi@ti.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <dt-bindings/gpio/gpio.h>
+
+enum gpio_shared_mode {
+	GPIO_SHARED_AND = 0,
+	GPIO_SHARED_OR,
+};
+
+struct gpio_client {
+	unsigned requested:1;
+	int value;
+};
+
+struct gpio_shared_priv {
+	struct device *dev;
+	struct gpio_desc *root_gpio;
+
+	struct gpio_chip gpio_chip;
+	enum gpio_shared_mode share_mode;
+	int root_value;
+
+	struct mutex mutex; /* protecting the counters */
+	int high_count;
+	int low_count;
+
+	/* root gpio calbacks */
+	int (*root_get)(const struct gpio_desc *desc);
+	void (*root_set)(struct gpio_desc *desc, int value);
+
+	struct gpio_client *clients;
+};
+
+static int gpio_shared_aggregate_root_value(struct gpio_shared_priv *priv)
+{
+	int value = 0;
+	int i;
+
+	for (i = 0; i < priv->gpio_chip.ngpio; i++) {
+		if (priv->clients[i].requested) {
+			if (priv->share_mode == GPIO_SHARED_AND)
+				value &= priv->clients[i].value;
+			else
+				value |= priv->clients[i].value;
+		}
+	}
+
+	return value;
+}
+
+static int gpio_shared_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpio_shared_priv *priv = gpiochip_get_data(chip);
+	int ret = 0;
+
+	if (priv->clients[offset].requested) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	mutex_lock(&priv->mutex);
+	priv->clients[offset].requested = 1;
+	priv->clients[offset].value = priv->root_value;
+
+out:
+	mutex_unlock(&priv->mutex);
+	return ret;
+}
+
+static void gpio_shared_gpio_free(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpio_shared_priv *priv = gpiochip_get_data(chip);
+
+	priv->clients[offset].requested = 0;
+}
+
+static void gpio_shared_gpio_set(struct gpio_chip *chip, unsigned int offset,
+				 int value)
+{
+	struct gpio_shared_priv *priv = gpiochip_get_data(chip);
+	int root_value;
+
+	mutex_lock(&priv->mutex);
+	priv->clients[offset].value = value;
+
+	root_value = gpio_shared_aggregate_root_value(priv);
+	if (priv->root_value != root_value) {
+		priv->root_set(priv->root_gpio, root_value);
+
+		/* Update the root's and client's value for the change */
+		priv->root_value = root_value;
+	}
+	mutex_unlock(&priv->mutex);
+}
+
+static int gpio_shared_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpio_shared_priv *priv = gpiochip_get_data(chip);
+	int value;
+
+	mutex_lock(&priv->mutex);
+	value = priv->clients[offset].value;
+	mutex_unlock(&priv->mutex);
+
+	return value;
+}
+
+static int gpio_shared_gpio_direction_out(struct gpio_chip *chip,
+					  unsigned int offset, int value)
+{
+	gpio_shared_gpio_set(chip, offset, value);
+
+	return 0;
+}
+
+static const struct gpio_chip gpio_shared_template_chip = {
+	.owner			= THIS_MODULE,
+	.request		= gpio_shared_gpio_request,
+	.free			= gpio_shared_gpio_free,
+	.set			= gpio_shared_gpio_set,
+	.get			= gpio_shared_gpio_get,
+	.direction_output	= gpio_shared_gpio_direction_out,
+	.base			= -1,
+};
+
+static const struct of_device_id gpio_shared_of_match[] = {
+	{ .compatible = "gpio-shared", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, gpio_shared_of_match);
+
+static int gpio_shared_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gpio_shared_priv *priv;
+	u32 val;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	priv->gpio_chip = gpio_shared_template_chip;
+	priv->gpio_chip.label = dev_name(dev);
+	priv->gpio_chip.parent = dev;
+	priv->gpio_chip.of_node = dev->of_node;
+
+	ret = of_property_read_u32(dev->of_node, "branch-count", &val);
+	if (ret) {
+		dev_err(dev, "branch-count is not provided\n");
+		return ret;
+	}
+
+	priv->gpio_chip.ngpio = val;
+
+	priv->clients = devm_kcalloc(dev, priv->gpio_chip.ngpio,
+				     sizeof(*priv->clients), GFP_KERNEL);
+	if (!priv->clients)
+		return -ENOMEM;
+
+	priv->root_gpio = devm_gpiod_get(dev, "root", GPIOD_ASIS);
+	if (IS_ERR(priv->root_gpio)) {
+		ret = PTR_ERR(priv->root_gpio);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Failed to get root GPIO\n");
+
+		return ret;
+	}
+
+	/* If the root GPIO is input, change it to output */
+	if (gpiod_get_direction(priv->root_gpio))
+		gpiod_direction_output(priv->root_gpio, 0);
+
+	priv->gpio_chip.can_sleep = gpiod_cansleep(priv->root_gpio);
+	if (priv->gpio_chip.can_sleep) {
+		priv->root_get = gpiod_get_value_cansleep;
+		priv->root_set = gpiod_set_value_cansleep;
+	} else {
+		priv->root_get = gpiod_get_value;
+		priv->root_set = gpiod_set_value;
+	}
+
+	priv->root_value = priv->root_get(priv->root_gpio);
+
+	ret = of_property_read_u32(dev->of_node, "hold-active-state", &val);
+	if (ret)
+		val = GPIO_ACTIVE_LOW;
+
+	if (val == GPIO_ACTIVE_HIGH)
+		priv->share_mode = GPIO_SHARED_OR;
+
+	dev_set_drvdata(dev, priv);
+	mutex_init(&priv->mutex);
+
+	return devm_gpiochip_add_data(dev, &priv->gpio_chip, priv);
+}
+
+static int gpio_shared_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static struct platform_driver gpio_shared_driver = {
+	.driver = {
+		.name	= "gpio-shared",
+		.of_match_table = gpio_shared_of_match,
+	},
+	.probe		= gpio_shared_probe,
+	.remove		= gpio_shared_remove,
+};
+
+module_platform_driver(gpio_shared_driver);
+
+MODULE_ALIAS("platform:gpio-shared");
+MODULE_DESCRIPTION("Generic shared GPIO driver");
+MODULE_AUTHOR("Peter Ujfalusi <peter.ujfalusi@ti.com>");
+MODULE_LICENSE("GPL v2");
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Rob Herring]

On Wed, Oct 30, 2019 at 7:03 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>
> Hi,
>
> The shared GPIO line for external components tends to be a common issue and
> there is no 'clean' way of handling it.
>
> I'm aware of the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag, which must be provided when
> a driver tries to request a GPIO which is already in use.
> However the driver must know that the component is going to be used in such a
> way, which can be said to any external components with GPIO line, so in theory
> all drivers must set this flag when requesting the GPIO...
>
> But with the GPIOD_FLAGS_BIT_NONEXCLUSIVE all clients have full control of the
> GPIO line. For example any device using the same GPIO as reset/enable line can
> reset/enable other devices, which is not something the other device might like
> or can handle.
> For example a device needs to be configured after it is enabled, but some other
> driver would reset it while handling the same GPIO -> the device is not
> operational anymmore as it lost it's configuration.
>
> With the gpio-shared gpiochip we can overcome this by giving the gpio-shared
> the role of making sure that the GPIO line only changes state when it will not
> disturb any of the clients sharing the same GPIO line.

Why can't we just add a shared flag like we have for interrupts?
Effectively, we have that for resets too, it's just hardcoded in the
the drivers.

Rob

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Peter Ujfalusi]

On 30/10/2019 15.12, Rob Herring wrote:
> On Wed, Oct 30, 2019 at 7:03 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>>
>> Hi,
>>
>> The shared GPIO line for external components tends to be a common issue and
>> there is no 'clean' way of handling it.
>>
>> I'm aware of the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag, which must be provided when
>> a driver tries to request a GPIO which is already in use.
>> However the driver must know that the component is going to be used in such a
>> way, which can be said to any external components with GPIO line, so in theory
>> all drivers must set this flag when requesting the GPIO...
>>
>> But with the GPIOD_FLAGS_BIT_NONEXCLUSIVE all clients have full control of the
>> GPIO line. For example any device using the same GPIO as reset/enable line can
>> reset/enable other devices, which is not something the other device might like
>> or can handle.
>> For example a device needs to be configured after it is enabled, but some other
>> driver would reset it while handling the same GPIO -> the device is not
>> operational anymmore as it lost it's configuration.
>>
>> With the gpio-shared gpiochip we can overcome this by giving the gpio-shared
>> the role of making sure that the GPIO line only changes state when it will not
>> disturb any of the clients sharing the same GPIO line.
> 
> Why can't we just add a shared flag like we have for interrupts?
> Effectively, we have that for resets too, it's just hardcoded in the
> the drivers.

This would be kind of the same thing what the
GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for
fixed-regulators afaik.

But let's say that a board design will pick two components (C1 and C2)
and use the same GPIO line to enable them. We already have the drivers
for them and they are used in boards already.

Both needs the GPIO line to be high for normal operation.
One or both of them needs register writes after they are enabled.

During boot both requests the GPIO (OUTPUT_LOW) and sets it high, then
run the register setup.

C1 request GPIO (LOW)
C1 gpio_set(1)
C1 register writes
C2 requests GPIO (LOW)
 C1 placed to reset and looses the configuration
C2 gpio_set(1)
 C1 also enabled
C2 register writes

At this point C2 is operational, C1 is not.

In shared GPIO case the GPIO should be handled like a regulator with a
twist that the 'sticky' state of the GPIO might be low or high depending
on the needs of the components it is connected to.

The shared GPIO line is a board design quirk and basically any device
which have reset/enable GPIO must be able to work in a situation when
they are sharing that line with other components and the driver should
not know much about this small detail.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Philipp Zabel]

On Wed, 2019-10-30 at 15:32 +0200, Peter Ujfalusi wrote:
> 
> On 30/10/2019 15.12, Rob Herring wrote:
> > On Wed, Oct 30, 2019 at 7:03 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> > > Hi,
> > > 
> > > The shared GPIO line for external components tends to be a common issue and
> > > there is no 'clean' way of handling it.
> > > 
> > > I'm aware of the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag, which must be provided when
> > > a driver tries to request a GPIO which is already in use.
> > > However the driver must know that the component is going to be used in such a
> > > way, which can be said to any external components with GPIO line, so in theory
> > > all drivers must set this flag when requesting the GPIO...
> > > 
> > > But with the GPIOD_FLAGS_BIT_NONEXCLUSIVE all clients have full control of the
> > > GPIO line. For example any device using the same GPIO as reset/enable line can
> > > reset/enable other devices, which is not something the other device might like
> > > or can handle.
> > > For example a device needs to be configured after it is enabled, but some other
> > > driver would reset it while handling the same GPIO -> the device is not
> > > operational anymmore as it lost it's configuration.
> > > 
> > > With the gpio-shared gpiochip we can overcome this by giving the gpio-shared
> > > the role of making sure that the GPIO line only changes state when it will not
> > > disturb any of the clients sharing the same GPIO line.
> > 
> > Why can't we just add a shared flag like we have for interrupts?
> > Effectively, we have that for resets too, it's just hardcoded in the
> > the drivers.
> 
> This would be kind of the same thing what the
> GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for
> fixed-regulators afaik.
> 
> But let's say that a board design will pick two components (C1 and C2)
> and use the same GPIO line to enable them. We already have the drivers
> for them and they are used in boards already.
> 
> Both needs the GPIO line to be high for normal operation.
> One or both of them needs register writes after they are enabled.
> 
> During boot both requests the GPIO (OUTPUT_LOW) and sets it high, then
> run the register setup.
> 
> C1 request GPIO (LOW)
> C1 gpio_set(1)
> C1 register writes
> C2 requests GPIO (LOW)
>  C1 placed to reset and looses the configuration
> C2 gpio_set(1)
>  C1 also enabled
> C2 register writes
> 
> At this point C2 is operational, C1 is not.
> 
> In shared GPIO case the GPIO should be handled like a regulator with a
> twist that the 'sticky' state of the GPIO might be low or high depending
> on the needs of the components it is connected to.
> 
> The shared GPIO line is a board design quirk and basically any device
> which have reset/enable GPIO must be able to work in a situation when
> they are sharing that line with other components and the driver should
> not know much about this small detail.

What about components that require a register write right after being
enabled, for example to put the device into a low power state, to
silence it on a bus, or to mask some initially enabled interrupts?

regards
Philipp

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Peter Ujfalusi]

On 30/10/2019 15.51, Philipp Zabel wrote:
> On Wed, 2019-10-30 at 15:32 +0200, Peter Ujfalusi wrote:
>>
>> On 30/10/2019 15.12, Rob Herring wrote:
>>> On Wed, Oct 30, 2019 at 7:03 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>>>> Hi,
>>>>
>>>> The shared GPIO line for external components tends to be a common issue and
>>>> there is no 'clean' way of handling it.
>>>>
>>>> I'm aware of the GPIOD_FLAGS_BIT_NONEXCLUSIVE flag, which must be provided when
>>>> a driver tries to request a GPIO which is already in use.
>>>> However the driver must know that the component is going to be used in such a
>>>> way, which can be said to any external components with GPIO line, so in theory
>>>> all drivers must set this flag when requesting the GPIO...
>>>>
>>>> But with the GPIOD_FLAGS_BIT_NONEXCLUSIVE all clients have full control of the
>>>> GPIO line. For example any device using the same GPIO as reset/enable line can
>>>> reset/enable other devices, which is not something the other device might like
>>>> or can handle.
>>>> For example a device needs to be configured after it is enabled, but some other
>>>> driver would reset it while handling the same GPIO -> the device is not
>>>> operational anymmore as it lost it's configuration.
>>>>
>>>> With the gpio-shared gpiochip we can overcome this by giving the gpio-shared
>>>> the role of making sure that the GPIO line only changes state when it will not
>>>> disturb any of the clients sharing the same GPIO line.
>>>
>>> Why can't we just add a shared flag like we have for interrupts?
>>> Effectively, we have that for resets too, it's just hardcoded in the
>>> the drivers.
>>
>> This would be kind of the same thing what the
>> GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for
>> fixed-regulators afaik.
>>
>> But let's say that a board design will pick two components (C1 and C2)
>> and use the same GPIO line to enable them. We already have the drivers
>> for them and they are used in boards already.
>>
>> Both needs the GPIO line to be high for normal operation.
>> One or both of them needs register writes after they are enabled.
>>
>> During boot both requests the GPIO (OUTPUT_LOW) and sets it high, then
>> run the register setup.
>>
>> C1 request GPIO (LOW)
>> C1 gpio_set(1)
>> C1 register writes
>> C2 requests GPIO (LOW)
>>  C1 placed to reset and looses the configuration
>> C2 gpio_set(1)
>>  C1 also enabled
>> C2 register writes
>>
>> At this point C2 is operational, C1 is not.
>>
>> In shared GPIO case the GPIO should be handled like a regulator with a
>> twist that the 'sticky' state of the GPIO might be low or high depending
>> on the needs of the components it is connected to.
>>
>> The shared GPIO line is a board design quirk and basically any device
>> which have reset/enable GPIO must be able to work in a situation when
>> they are sharing that line with other components and the driver should
>> not know much about this small detail.
> 
> What about components that require a register write right after being
> enabled, for example to put the device into a low power state, to
> silence it on a bus, or to mask some initially enabled interrupts?

You are right, if a device needs driver to silence it when enabled (we
might not have the driver compiled) then this can be a problem.

But the same thing applies to components without enable/reset GPIO and
only needing power, no?

I would trust (I know...) on the board designers to not bundle
components of such kinds.

> 
> regards
> Philipp
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

On Wed, Oct 30, 2019 at 03:32:09PM +0200, Peter Ujfalusi wrote:
> On 30/10/2019 15.12, Rob Herring wrote:

> > Why can't we just add a shared flag like we have for interrupts?
> > Effectively, we have that for resets too, it's just hardcoded in the
> > the drivers.

> This would be kind of the same thing what the
> GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for
> fixed-regulators afaik.

The theory with that was that any usage of this would need the
higher level code using the GPIO to cooperate so they didn't step
on each other's toes so the GPIO code should just punt to it.

> But let's say that a board design will pick two components (C1 and C2)
> and use the same GPIO line to enable them. We already have the drivers
> for them and they are used in boards already.

This is basically an attempt to make a generic implementation of
that cooperation for simple cases.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Peter Ujfalusi]

On 30/10/2019 16.17, Mark Brown wrote:
> On Wed, Oct 30, 2019 at 03:32:09PM +0200, Peter Ujfalusi wrote:
>> On 30/10/2019 15.12, Rob Herring wrote:
> 
>>> Why can't we just add a shared flag like we have for interrupts?
>>> Effectively, we have that for resets too, it's just hardcoded in the
>>> the drivers.
> 
>> This would be kind of the same thing what the
>> GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for
>> fixed-regulators afaik.
> 
> The theory with that was that any usage of this would need the
> higher level code using the GPIO to cooperate so they didn't step
> on each other's toes so the GPIO code should just punt to it.

But from the client driver point of view a GPIO is still GPIO and if the
components are unrelated then it is hard to patch things together from
the top.

>> But let's say that a board design will pick two components (C1 and C2)
>> and use the same GPIO line to enable them. We already have the drivers
>> for them and they are used in boards already.
> 
> This is basically an attempt to make a generic implementation of
> that cooperation for simple cases.

Without some generic implementation we would need to patch up drivers
(sometimes unrelated) for components which uses the same GPIO line on
some random board.

We have such a thing for tlv310aic3x (for Nokia n900). I would need one
for pcm3168a and there were an attempt for max98927 also.
We also have boards where one GPIO is used for backlight and panel, etc.

If we would have generic way then it can be tuned for more exotic
setups, but again, I believe that board designers are not doing things
just to be evil. What they connect most of the times are 'sane'

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Rob Herring]

On Wed, Oct 30, 2019 at 9:30 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>
>
>
> On 30/10/2019 16.17, Mark Brown wrote:
> > On Wed, Oct 30, 2019 at 03:32:09PM +0200, Peter Ujfalusi wrote:
> >> On 30/10/2019 15.12, Rob Herring wrote:
> >
> >>> Why can't we just add a shared flag like we have for interrupts?
> >>> Effectively, we have that for resets too, it's just hardcoded in the
> >>> the drivers.
> >
> >> This would be kind of the same thing what the
> >> GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for
> >> fixed-regulators afaik.
> >
> > The theory with that was that any usage of this would need the
> > higher level code using the GPIO to cooperate so they didn't step
> > on each other's toes so the GPIO code should just punt to it.
>
> But from the client driver point of view a GPIO is still GPIO and if the
> components are unrelated then it is hard to patch things together from
> the top.

You can't escape a driver being aware. If a driver depends on that
GPIO to actually be set to states the driver says, then it can't be
guaranteed to work. For example, maybe the driver assumes the device
is in reset state after toggling reset and doesn't work if not in
reset state. The driver has to be aware no matter what you do in DT.

Rob

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Peter Ujfalusi]

On 30/10/2019 20.49, Rob Herring wrote:
> On Wed, Oct 30, 2019 at 9:30 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>>
>>
>>
>> On 30/10/2019 16.17, Mark Brown wrote:
>>> On Wed, Oct 30, 2019 at 03:32:09PM +0200, Peter Ujfalusi wrote:
>>>> On 30/10/2019 15.12, Rob Herring wrote:
>>>
>>>>> Why can't we just add a shared flag like we have for interrupts?
>>>>> Effectively, we have that for resets too, it's just hardcoded in the
>>>>> the drivers.
>>>
>>>> This would be kind of the same thing what the
>>>> GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for
>>>> fixed-regulators afaik.
>>>
>>> The theory with that was that any usage of this would need the
>>> higher level code using the GPIO to cooperate so they didn't step
>>> on each other's toes so the GPIO code should just punt to it.
>>
>> But from the client driver point of view a GPIO is still GPIO and if the
>> components are unrelated then it is hard to patch things together from
>> the top.
> 
> You can't escape a driver being aware. If a driver depends on that
> GPIO to actually be set to states the driver says, then it can't be
> guaranteed to work. For example, maybe the driver assumes the device
> is in reset state after toggling reset and doesn't work if not in
> reset state. The driver has to be aware no matter what you do in DT.

That's true for some device, but it is also true that some can not
tolerate being reset without them knowing it.

If all users of the shared GPIO have full control over it then they can
just toggle it whatever way they want. How would a regulator, codec,
amplifier would negotiate on what to do with the shared GPIO?

Another not uncommon setup is when the two components needs different level:
C1: ENABLE is high active
C2: RESET is high active

To enable C1, the GPIO should be high. To enable C2 the GPIO must be low.
In the board one of the branch of the shared GPIO needs (and have) a
logic inverter.

If they both control the same GPIO then they must have requested it with
different GPIO_ACTIVE_ since the drivers are written according to chip
spec, so C1 sets the GPIO to 1, C2 sets it to 0, the inversion for one
of them must happen in gpio core, right?

It should be possible to add pass-through mode for gpio-shared so that
all requests would propagate to the root GPIO if that's what needed for
some setups.

That way the gpio-shared would nicely handle the GPIO inversions, would
be able to handle cases to avoid unwanted reset/enable of components or
allow components to be ninja-reset.

I think it would be possible to add gpiod_is_shared(struct gpio_desc
*desc) so users can check if the GPIO is shared - it would only return
true if the gpio-shared is not in pass-through mode so they can know
that the state they see on their gpio desc is not necessary matching
with reality.
Probably another gpiod_shared_get_root_value() to fetch the root's state?

I intentionally not returning that in the driver as clients might skip a
gpio_set_value() seeing that the GPIO line is already in a state they
would want it, but that would not register their needs for the level.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Rob Herring]

On Thu, Oct 31, 2019 at 3:00 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>
>
>
> On 30/10/2019 20.49, Rob Herring wrote:
> > On Wed, Oct 30, 2019 at 9:30 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> >>
> >>
> >>
> >> On 30/10/2019 16.17, Mark Brown wrote:
> >>> On Wed, Oct 30, 2019 at 03:32:09PM +0200, Peter Ujfalusi wrote:
> >>>> On 30/10/2019 15.12, Rob Herring wrote:
> >>>
> >>>>> Why can't we just add a shared flag like we have for interrupts?
> >>>>> Effectively, we have that for resets too, it's just hardcoded in the
> >>>>> the drivers.
> >>>
> >>>> This would be kind of the same thing what the
> >>>> GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for
> >>>> fixed-regulators afaik.
> >>>
> >>> The theory with that was that any usage of this would need the
> >>> higher level code using the GPIO to cooperate so they didn't step
> >>> on each other's toes so the GPIO code should just punt to it.
> >>
> >> But from the client driver point of view a GPIO is still GPIO and if the
> >> components are unrelated then it is hard to patch things together from
> >> the top.
> >
> > You can't escape a driver being aware. If a driver depends on that
> > GPIO to actually be set to states the driver says, then it can't be
> > guaranteed to work. For example, maybe the driver assumes the device
> > is in reset state after toggling reset and doesn't work if not in
> > reset state. The driver has to be aware no matter what you do in DT.
>
> That's true for some device, but it is also true that some can not
> tolerate being reset without them knowing it.

You mean a reset when the driver is not loaded would not work? How
could that ever work? I don't think you can have any reset control in
the drivers in that case.

> If all users of the shared GPIO have full control over it then they can
> just toggle it whatever way they want. How would a regulator, codec,
> amplifier would negotiate on what to do with the shared GPIO?
>
> Another not uncommon setup is when the two components needs different level:
> C1: ENABLE is high active
> C2: RESET is high active
>
> To enable C1, the GPIO should be high. To enable C2 the GPIO must be low.
> In the board one of the branch of the shared GPIO needs (and have) a
> logic inverter.
>
> If they both control the same GPIO then they must have requested it with
> different GPIO_ACTIVE_ since the drivers are written according to chip
> spec, so C1 sets the GPIO to 1, C2 sets it to 0, the inversion for one
> of them must happen in gpio core, right?

No, drivers are written to set the state to active/inactive. The DT
GPIO_ACTIVE_ flags can depend on an inverter being present (BTW, there
was a recent attempt to do an inverter binding).


> It should be possible to add pass-through mode for gpio-shared so that
> all requests would propagate to the root GPIO if that's what needed for
> some setups.
>
> That way the gpio-shared would nicely handle the GPIO inversions, would
> be able to handle cases to avoid unwanted reset/enable of components or
> allow components to be ninja-reset.

What does ninja-reset mean?

> I think it would be possible to add gpiod_is_shared(struct gpio_desc
> *desc) so users can check if the GPIO is shared - it would only return
> true if the gpio-shared is not in pass-through mode so they can know
> that the state they see on their gpio desc is not necessary matching
> with reality.
> Probably another gpiod_shared_get_root_value() to fetch the root's state?
>
> I intentionally not returning that in the driver as clients might skip a
> gpio_set_value() seeing that the GPIO line is already in a state they
> would want it, but that would not register their needs for the level.
>
> - Péter
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Peter Ujfalusi]

On 01/11/2019 15.46, Rob Herring wrote:
> On Thu, Oct 31, 2019 at 3:00 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>>
>>
>>
>> On 30/10/2019 20.49, Rob Herring wrote:
>>> On Wed, Oct 30, 2019 at 9:30 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>>>>
>>>>
>>>>
>>>> On 30/10/2019 16.17, Mark Brown wrote:
>>>>> On Wed, Oct 30, 2019 at 03:32:09PM +0200, Peter Ujfalusi wrote:
>>>>>> On 30/10/2019 15.12, Rob Herring wrote:
>>>>>
>>>>>>> Why can't we just add a shared flag like we have for interrupts?
>>>>>>> Effectively, we have that for resets too, it's just hardcoded in the
>>>>>>> the drivers.
>>>>>
>>>>>> This would be kind of the same thing what the
>>>>>> GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for
>>>>>> fixed-regulators afaik.
>>>>>
>>>>> The theory with that was that any usage of this would need the
>>>>> higher level code using the GPIO to cooperate so they didn't step
>>>>> on each other's toes so the GPIO code should just punt to it.
>>>>
>>>> But from the client driver point of view a GPIO is still GPIO and if the
>>>> components are unrelated then it is hard to patch things together from
>>>> the top.
>>>
>>> You can't escape a driver being aware. If a driver depends on that
>>> GPIO to actually be set to states the driver says, then it can't be
>>> guaranteed to work. For example, maybe the driver assumes the device
>>> is in reset state after toggling reset and doesn't work if not in
>>> reset state. The driver has to be aware no matter what you do in DT.
>>
>> That's true for some device, but it is also true that some can not
>> tolerate being reset without them knowing it.
> 
> You mean a reset when the driver is not loaded would not work? How
> could that ever work?

No, what I mean is that one device is reset because the driver for the
other device toggles the GPIO line.

If one driver toggles the GPIO line directly then the GPIO line is going
to be toggled for all the devices the GPIO line is connected to.

> I don't think you can have any reset control in
> the drivers in that case.

The device needs the RST line to be high, otherwise it is not
accessible. If it does not have reset control how can we make sure that
the GPIO line is in correct state?

gpio-hog does not work all the time because we can not trust probe order
and w/o gpio binding on the user deferred probing is not possible.
If for some reason the gpio controller is probed after the drivers
depending on the reset/enable GPIO then there's not much we can do.

>> If all users of the shared GPIO have full control over it then they can
>> just toggle it whatever way they want. How would a regulator, codec,
>> amplifier would negotiate on what to do with the shared GPIO?
>>
>> Another not uncommon setup is when the two components needs different level:
>> C1: ENABLE is high active
>> C2: RESET is high active
>>
>> To enable C1, the GPIO should be high. To enable C2 the GPIO must be low.
>> In the board one of the branch of the shared GPIO needs (and have) a
>> logic inverter.
>>
>> If they both control the same GPIO then they must have requested it with
>> different GPIO_ACTIVE_ since the drivers are written according to chip
>> spec, so C1 sets the GPIO to 1, C2 sets it to 0, the inversion for one
>> of them must happen in gpio core, right?
> 
> No, drivers are written to set the state to active/inactive.

I think the drivers are written in a way to follow what their datasheets
are tells. If it say that the GPIO line must be high to enable the
device then they gpiod_set_value(1), if the line must be low to enable
them then they will gpiod_set_value(0).

> The DT GPIO_ACTIVE_ flags can depend on an inverter being present (BTW, there
> was a recent attempt to do an inverter binding).

Yes.
If the line is inverted on the board, than the DT GPIO_ACTIVE_LOW will
invert it to the correct level.

We have two off the shelf components, C1 and C2. They have a driver
written based on the datasheets.
C1 needs HIGH (LOW reset/disable)
 uses gpiod_set_value(1) to enable the device

C2 needs LOW (HIGH reset/disable)
 uses gpiod_set_value(0) to enable the device

When they are connected to a dedicated GPIO the DT binding has
GPIO_ACTIVE_HIGH since when the GPIO is set to 1 it goes HIGH, right?

If two device is connected to one GPIO one of them needs an inverter on
the GPIO line after it is split into two, let say C2 got inverted line:
C1 tells in DT that the line is not inverted: GPIO_ACTIVE_HOGH
C2 tells in DT that the line is inverted: GPIO_ACTIVE_LOW

GPIO HIGH -> D1 is enabled
	  -> !HIGH -> LOW -> D2 is enabled

If both would request the same physical GPIO then how would this work? A
single GPIO can not be handled in inverted and non inverted way at the
same time.

But this is just a side effect that this would be easy to handle with
this DT binding and driver.
After all, it will describe the GPIO line split.

>> It should be possible to add pass-through mode for gpio-shared so that
>> all requests would propagate to the root GPIO if that's what needed for
>> some setups.
>>
>> That way the gpio-shared would nicely handle the GPIO inversions, would
>> be able to handle cases to avoid unwanted reset/enable of components or
>> allow components to be ninja-reset.
> 
> What does ninja-reset mean?

Ninjas attack from ambush ;)
The device is reset w/o it's driver being aware that it ever happened as
other driver toggled the shared GPIO line.

>> I think it would be possible to add gpiod_is_shared(struct gpio_desc
>> *desc) so users can check if the GPIO is shared - it would only return
>> true if the gpio-shared is not in pass-through mode so they can know
>> that the state they see on their gpio desc is not necessary matching
>> with reality.
>> Probably another gpiod_shared_get_root_value() to fetch the root's state?
>>
>> I intentionally not returning that in the driver as clients might skip a
>> gpio_set_value() seeing that the GPIO line is already in a state they
>> would want it, but that would not register their needs for the level.
>>
>> - Péter
>>
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Rob Herring]

On Fri, Nov 1, 2019 at 10:20 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>
>
>
> On 01/11/2019 15.46, Rob Herring wrote:
> > On Thu, Oct 31, 2019 at 3:00 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> >>
> >>
> >>
> >> On 30/10/2019 20.49, Rob Herring wrote:
> >>> On Wed, Oct 30, 2019 at 9:30 AM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 30/10/2019 16.17, Mark Brown wrote:
> >>>>> On Wed, Oct 30, 2019 at 03:32:09PM +0200, Peter Ujfalusi wrote:
> >>>>>> On 30/10/2019 15.12, Rob Herring wrote:
> >>>>>
> >>>>>>> Why can't we just add a shared flag like we have for interrupts?
> >>>>>>> Effectively, we have that for resets too, it's just hardcoded in the
> >>>>>>> the drivers.
> >>>>>
> >>>>>> This would be kind of the same thing what the
> >>>>>> GPIOD_FLAGS_BIT_NONEXCLUSIVE does, which was a quick workaround for
> >>>>>> fixed-regulators afaik.
> >>>>>
> >>>>> The theory with that was that any usage of this would need the
> >>>>> higher level code using the GPIO to cooperate so they didn't step
> >>>>> on each other's toes so the GPIO code should just punt to it.
> >>>>
> >>>> But from the client driver point of view a GPIO is still GPIO and if the
> >>>> components are unrelated then it is hard to patch things together from
> >>>> the top.
> >>>
> >>> You can't escape a driver being aware. If a driver depends on that
> >>> GPIO to actually be set to states the driver says, then it can't be
> >>> guaranteed to work. For example, maybe the driver assumes the device
> >>> is in reset state after toggling reset and doesn't work if not in
> >>> reset state. The driver has to be aware no matter what you do in DT.
> >>
> >> That's true for some device, but it is also true that some can not
> >> tolerate being reset without them knowing it.
> >
> > You mean a reset when the driver is not loaded would not work? How
> > could that ever work?
>
> No, what I mean is that one device is reset because the driver for the
> other device toggles the GPIO line.
>
> If one driver toggles the GPIO line directly then the GPIO line is going
> to be toggled for all the devices the GPIO line is connected to.

Of course. That would be the typical case. I'd assume we would want to
handle that the same way as shared resets. Reset can only be asserted
when all clients want reset asserted. I guess when the first client
probes, it asserts and deasserts the reset.

>
> > I don't think you can have any reset control in
> > the drivers in that case.
>
> The device needs the RST line to be high, otherwise it is not
> accessible. If it does not have reset control how can we make sure that
> the GPIO line is in correct state?

Just like the reset code, drivers register their use of the reset and
the core tracks users and prevents resetting when not safe. Maybe the
reset subsystem needs to learn about GPIO resets. It could even
default to knowing 'reset-gpios' property as we've somewhat
standardized that. Then you just register your GPIO reset line with
the reset subsystem. When it gets the same line registered more than
once, then it knows to handle sharing the line. If you need to know
the line is shared before then, then you need something in DT. A flag
is enough for that.

> gpio-hog does not work all the time because we can not trust probe order
> and w/o gpio binding on the user deferred probing is not possible.
> If for some reason the gpio controller is probed after the drivers
> depending on the reset/enable GPIO then there's not much we can do.
>
> >> If all users of the shared GPIO have full control over it then they can
> >> just toggle it whatever way they want. How would a regulator, codec,
> >> amplifier would negotiate on what to do with the shared GPIO?
> >>
> >> Another not uncommon setup is when the two components needs different level:
> >> C1: ENABLE is high active
> >> C2: RESET is high active
> >>
> >> To enable C1, the GPIO should be high. To enable C2 the GPIO must be low.
> >> In the board one of the branch of the shared GPIO needs (and have) a
> >> logic inverter.
> >>
> >> If they both control the same GPIO then they must have requested it with
> >> different GPIO_ACTIVE_ since the drivers are written according to chip
> >> spec, so C1 sets the GPIO to 1, C2 sets it to 0, the inversion for one
> >> of them must happen in gpio core, right?
> >
> > No, drivers are written to set the state to active/inactive.
>
> I think the drivers are written in a way to follow what their datasheets
> are tells. If it say that the GPIO line must be high to enable the
> device then they gpiod_set_value(1), if the line must be low to enable
> them then they will gpiod_set_value(0).

gpiod_set_value(1) sets the line to the active state defined in DT
GPIO flags, not the electrical level of the signal. This issue is a
good example of precisely why the gpiod API was defined this way. I do
think it is a bit confusing though. Perhaps reusing _{get,set}_value
API was not the best naming.


> > The DT GPIO_ACTIVE_ flags can depend on an inverter being present (BTW, there
> > was a recent attempt to do an inverter binding).
>
> Yes.
> If the line is inverted on the board, than the DT GPIO_ACTIVE_LOW will
> invert it to the correct level.

Yes, if the signal is normally GPIO_ACTIVE_HIGH.

> We have two off the shelf components, C1 and C2. They have a driver
> written based on the datasheets.
> C1 needs HIGH (LOW reset/disable)
>  uses gpiod_set_value(1) to enable the device

No. The active state for a 'reset-gpios' is the state in which reset
is active/asserted. So gpiod_set_value(1) should always mean 'assert
reset'.

If we're talking about an 'enable-gpios', then the active state is
when the device is active/enabled. So it's the inverse of
'reset-gpios'.

> C2 needs LOW (HIGH reset/disable)
>  uses gpiod_set_value(0) to enable the device

Yes. The GPIO flag would be GPIO_ACTIVE_HIGH and gpiod_set_value(0) is
reset de-asserted.

> When they are connected to a dedicated GPIO the DT binding has
> GPIO_ACTIVE_HIGH since when the GPIO is set to 1 it goes HIGH, right?

No, as explained above. C2 would be GPIO_ACTIVE_HIGH, C1 would be
GPIO_ACTIVE_LOW normally.

> If two device is connected to one GPIO one of them needs an inverter on
> the GPIO line after it is split into two, let say C2 got inverted line:
> C1 tells in DT that the line is not inverted: GPIO_ACTIVE_HOGH
> C2 tells in DT that the line is inverted: GPIO_ACTIVE_LOW

C1 needs GPIO_ACTIVE_LOW here.

> GPIO HIGH -> D1 is enabled
>           -> !HIGH -> LOW -> D2 is enabled
>
> If both would request the same physical GPIO then how would this work? A
> single GPIO can not be handled in inverted and non inverted way at the
> same time.
>
> But this is just a side effect that this would be easy to handle with
> this DT binding and driver.
> After all, it will describe the GPIO line split.
>
> >> It should be possible to add pass-through mode for gpio-shared so that
> >> all requests would propagate to the root GPIO if that's what needed for
> >> some setups.
> >>
> >> That way the gpio-shared would nicely handle the GPIO inversions, would
> >> be able to handle cases to avoid unwanted reset/enable of components or
> >> allow components to be ninja-reset.
> >
> > What does ninja-reset mean?
>
> Ninjas attack from ambush ;)
> The device is reset w/o it's driver being aware that it ever happened as
> other driver toggled the shared GPIO line.
>
> >> I think it would be possible to add gpiod_is_shared(struct gpio_desc
> >> *desc) so users can check if the GPIO is shared - it would only return
> >> true if the gpio-shared is not in pass-through mode so they can know
> >> that the state they see on their gpio desc is not necessary matching
> >> with reality.
> >> Probably another gpiod_shared_get_root_value() to fetch the root's state?
> >>
> >> I intentionally not returning that in the driver as clients might skip a
> >> gpio_set_value() seeing that the GPIO line is already in a state they
> >> would want it, but that would not register their needs for the level.
> >>
> >> - Péter
> >>
> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>
> - Péter
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Linus Walleij]

On Mon, Nov 4, 2019 at 8:11 PM Rob Herring <robh+dt@kernel.org> wrote:
> [Peter]
> > The device needs the RST line to be high, otherwise it is not
> > accessible. If it does not have reset control how can we make sure that
> > the GPIO line is in correct state?
>
> Just like the reset code, drivers register their use of the reset and
> the core tracks users and prevents resetting when not safe. Maybe the
> reset subsystem needs to learn about GPIO resets. (...)

I agree. Certainly the reset subsystem can do what the regulator
subsystem is already doing: request the GPIO line nonexclusive
and handle any reference counting and/or quirks that are needed
in a hypothetical drivers/reset/reset-gpio.c driver.

There is no such driver today, just a "reset" driver in
drivers/power/reset that resets the whole system.

But I see no problem in creating a proper reset driver in drivers/reset
to handle a few peripherals with a shared GPIO reset line.

Yours,
Linus Walleij

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Peter Ujfalusi]

On 05/11/2019 11.58, Linus Walleij wrote:
> On Mon, Nov 4, 2019 at 8:11 PM Rob Herring <robh+dt@kernel.org> wrote:
>> [Peter]
>>> The device needs the RST line to be high, otherwise it is not
>>> accessible. If it does not have reset control how can we make sure that
>>> the GPIO line is in correct state?
>>
>> Just like the reset code, drivers register their use of the reset and
>> the core tracks users and prevents resetting when not safe. Maybe the
>> reset subsystem needs to learn about GPIO resets. (...)
> 
> I agree. Certainly the reset subsystem can do what the regulator
> subsystem is already doing: request the GPIO line nonexclusive
> and handle any reference counting and/or quirks that are needed
> in a hypothetical drivers/reset/reset-gpio.c driver.

I did wrote the reset-gpio driver first ;)
then it failed the thought test on several levels.

to get a reset control one either use the shared or exclusive API.
Depending on which one you use, the behavior changes. With exclusive it
works like a GPIO (no refcounting of asserts), with shared it refcounts.

It fails flat if I boot with old dtb blob which did not had the "resets"
and "#reset-cells" (from the user's point of view). Even if the old dtb
had rst/enable/reset-gpios defined.

It is kind of hard to use it for 'Output Enable' type of gpios. They are
not reset or enable signals for the peripheral, but to open a gate to
outside, for example allow an amplifier to drive the analog line on (one
of) it's output for example.

> There is no such driver today, just a "reset" driver in
> drivers/power/reset that resets the whole system.

Yep, I have checked that as well before I wrote my own gpio-reset

> But I see no problem in creating a proper reset driver in drivers/reset
> to handle a few peripherals with a shared GPIO reset line.

Even if we have a reset-gpio driver we will have the same issue that the
regulator might reset things underneath the tiddly refcounted reset line
for non regulator users, plus one extra which is using the line as
output enable.

With the gpio-shared all of these can be handled in a nice way and we
can add the pass-through mode to it which is assumed by some setups or
use refcounting as it is in the initial patch.

And we need to modify the drivers to ask for shared/nonexclusive reset/gpio.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Grygorii Strashko]

On 05/11/2019 11:58, Linus Walleij wrote:
> On Mon, Nov 4, 2019 at 8:11 PM Rob Herring <robh+dt@kernel.org> wrote:
>> [Peter]
>>> The device needs the RST line to be high, otherwise it is not
>>> accessible. If it does not have reset control how can we make sure that
>>> the GPIO line is in correct state?
>>
>> Just like the reset code, drivers register their use of the reset and
>> the core tracks users and prevents resetting when not safe. Maybe the
>> reset subsystem needs to learn about GPIO resets. (...)
> 
> I agree. Certainly the reset subsystem can do what the regulator
> subsystem is already doing: request the GPIO line nonexclusive
> and handle any reference counting and/or quirks that are needed
> in a hypothetical drivers/reset/reset-gpio.c driver.
> 
> There is no such driver today, just a "reset" driver in
> drivers/power/reset that resets the whole system.
> 
> But I see no problem in creating a proper reset driver in drivers/reset
> to handle a few peripherals with a shared GPIO reset line.

Personally, I agree with Mark's comment here:

> [Mark]
> The theory with that was that any usage of this would need the
> higher level code using the GPIO to cooperate so they didn't step
> on each other's toes so the GPIO code should just punt to it.
> 
>> But let's say that a board design will pick two components (C1 and C2)
>> and use the same GPIO line to enable them. We already have the drivers
>> for them and they are used in boards already.
> 
> This is basically an attempt to make a generic implementation of
> that cooperation for simple cases.
> 

This looks like unsolvable problem in generic way.
Lets assume there are some generic shared reset controller invented, but then
- What if some driver is loaded/unloaded and corresponding device uses shared
   reset which is de-asserted already?
   In this case, driver should never ever expect that target device has all
   registers in default state.
- What if reset is required as part of error recovery procedure? The error recovery
   will not be supported by such design.
- PM: Device reset could be part of suspend/resume sequence. if one of the devices
   is wake-up source, but other are not, those devices might be in very unexpected state during resume.
- There could be dependencies on reset timings, shared reset might work for
   similar devices (like set of net phys) and does not work if connected devices are different.

It seems, the only one case when it might help is system boot when:
  - similar devices are connected to the reset line
  - drivers are not expected to be re-loaded
  - device reset is not part of any recovery procedure
  - safe reset timings can be defined for all connected devices
(but hey - if this is boot only then gpio-hogs should work. Are they?)

Actually, MDIO bus is one such example where reset line can be toggled by
as by MDIO bus controller as by PHY drivers.

So, even thing will move forward with this - it'll be good to have noted
above restrictions in documentation.

-- 
Best regards,
grygorii

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Peter Ujfalusi]

On 04/11/2019 21.11, Rob Herring wrote:
>> If one driver toggles the GPIO line directly then the GPIO line is going
>> to be toggled for all the devices the GPIO line is connected to.
> 
> Of course. That would be the typical case. I'd assume we would want to
> handle that the same way as shared resets. Reset can only be asserted
> when all clients want reset asserted. I guess when the first client
> probes, it asserts and deasserts the reset.

The exclusive flavor of reset API acts like a GPIO, while the shared
ones will result in refcounted behavior.

With describing the hw via gpio-shared we could support different modes:
- pass-through: like a GPIO with nonexclusive, any request will
propagate to the root-gpio
- refcounted low: the line is kept low as long as at least one client is
requiring to to be low
- refcounted high: the line is kept high as long as at least one client
is requiring to to be high

>>> I don't think you can have any reset control in
>>> the drivers in that case.
>>
>> The device needs the RST line to be high, otherwise it is not
>> accessible. If it does not have reset control how can we make sure that
>> the GPIO line is in correct state?
> 
> Just like the reset code, drivers register their use of the reset and
> the core tracks users and prevents resetting when not safe. Maybe the
> reset subsystem needs to learn about GPIO resets. It could even
> default to knowing 'reset-gpios' property as we've somewhat
> standardized that. Then you just register your GPIO reset line with
> the reset subsystem. When it gets the same line registered more than
> once, then it knows to handle sharing the line. If you need to know
> the line is shared before then, then you need something in DT. A flag
> is enough for that.

What about things where the gpio is not a reset or enable for the chip,
but an enable for one of it's output?
Or if a shared GPIO is connected to address select pins?

>>> No, drivers are written to set the state to active/inactive.
>>
>> I think the drivers are written in a way to follow what their datasheets
>> are tells. If it say that the GPIO line must be high to enable the
>> device then they gpiod_set_value(1), if the line must be low to enable
>> them then they will gpiod_set_value(0).
> 
> gpiod_set_value(1) sets the line to the active state defined in DT
> GPIO flags, not the electrical level of the signal. This issue is a
> good example of precisely why the gpiod API was defined this way. I do
> think it is a bit confusing though. Perhaps reusing _{get,set}_value
> API was not the best naming.

Yes, it is confusing and I think most drivers are using it following
their corresponding datasheets, iow if the datasheet say the line must
be low then set_value(0) is used.

It does look weird to use set_value(reset_gpio, 1) in the code when the
documentation say that the reset pin must be _low_ to place the part
into reset, even if you look up the DT documentation and dts files for
GPIO_ACTIVE_HIGH/LOW usage among boards.

Especially if you have two peripherals where both is enabled when their
pin is LOW, but one names it RST and is high active, the other names it
EN and it is low active. especially that lots of devices do not even
states any active mode for the pin, just if high, it is in reset, if low
then it is enabled.

I get the notion of how it is, but it does feel a bit unnatural in times.

>>> The DT GPIO_ACTIVE_ flags can depend on an inverter being present (BTW, there
>>> was a recent attempt to do an inverter binding).
>>
>> Yes.
>> If the line is inverted on the board, than the DT GPIO_ACTIVE_LOW will
>> invert it to the correct level.
> 
> Yes, if the signal is normally GPIO_ACTIVE_HIGH.
> 
>> We have two off the shelf components, C1 and C2. They have a driver
>> written based on the datasheets.
>> C1 needs HIGH (LOW reset/disable)
>>  uses gpiod_set_value(1) to enable the device
> 
> No. The active state for a 'reset-gpios' is the state in which reset
> is active/asserted. So gpiod_set_value(1) should always mean 'assert
> reset'.
> 
> If we're talking about an 'enable-gpios', then the active state is
> when the device is active/enabled. So it's the inverse of
> 'reset-gpios'.
> 
>> C2 needs LOW (HIGH reset/disable)
>>  uses gpiod_set_value(0) to enable the device
> 
> Yes. The GPIO flag would be GPIO_ACTIVE_HIGH and gpiod_set_value(0) is
> reset de-asserted.
> 
>> When they are connected to a dedicated GPIO the DT binding has
>> GPIO_ACTIVE_HIGH since when the GPIO is set to 1 it goes HIGH, right?
> 
> No, as explained above. C2 would be GPIO_ACTIVE_HIGH, C1 would be
> GPIO_ACTIVE_LOW normally.
> 
>> If two device is connected to one GPIO one of them needs an inverter on
>> the GPIO line after it is split into two, let say C2 got inverted line:
>> C1 tells in DT that the line is not inverted: GPIO_ACTIVE_HOGH
>> C2 tells in DT that the line is inverted: GPIO_ACTIVE_LOW
> 
> C1 needs GPIO_ACTIVE_LOW here.

Hrm, so the GPIO_ACTIVE_ can not be used as a means to tell that if the
gpio line is active (set to 1) at the source then the signal at the
component's pin is going to be high (GPIO_ACTIVE_HIGH) or low
(GPIO_ACTIVE_LOW)?

> 
>> GPIO HIGH -> D1 is enabled
>>           -> !HIGH -> LOW -> D2 is enabled
>>
>> If both would request the same physical GPIO then how would this work? A
>> single GPIO can not be handled in inverted and non inverted way at the
>> same time.
>>
>> But this is just a side effect that this would be easy to handle with
>> this DT binding and driver.
>> After all, it will describe the GPIO line split.
>>
>>>> It should be possible to add pass-through mode for gpio-shared so that
>>>> all requests would propagate to the root GPIO if that's what needed for
>>>> some setups.
>>>>
>>>> That way the gpio-shared would nicely handle the GPIO inversions, would
>>>> be able to handle cases to avoid unwanted reset/enable of components or
>>>> allow components to be ninja-reset.
>>>
>>> What does ninja-reset mean?
>>
>> Ninjas attack from ambush ;)
>> The device is reset w/o it's driver being aware that it ever happened as
>> other driver toggled the shared GPIO line.
>>
>>>> I think it would be possible to add gpiod_is_shared(struct gpio_desc
>>>> *desc) so users can check if the GPIO is shared - it would only return
>>>> true if the gpio-shared is not in pass-through mode so they can know
>>>> that the state they see on their gpio desc is not necessary matching
>>>> with reality.
>>>> Probably another gpiod_shared_get_root_value() to fetch the root's state?
>>>>
>>>> I intentionally not returning that in the driver as clients might skip a
>>>> gpio_set_value() seeing that the GPIO line is already in a state they
>>>> would want it, but that would not register their needs for the level.
>>>>
>>>> - Péter
>>>>
>>>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>>>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>
>> - Péter
>>
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Peter Ujfalusi]

On 05/11/2019 14.15, Grygorii Strashko wrote:
> 
> 
> On 05/11/2019 11:58, Linus Walleij wrote:
>> On Mon, Nov 4, 2019 at 8:11 PM Rob Herring <robh+dt@kernel.org> wrote:
>>> [Peter]
>>>> The device needs the RST line to be high, otherwise it is not
>>>> accessible. If it does not have reset control how can we make sure that
>>>> the GPIO line is in correct state?
>>>
>>> Just like the reset code, drivers register their use of the reset and
>>> the core tracks users and prevents resetting when not safe. Maybe the
>>> reset subsystem needs to learn about GPIO resets. (...)
>>
>> I agree. Certainly the reset subsystem can do what the regulator
>> subsystem is already doing: request the GPIO line nonexclusive
>> and handle any reference counting and/or quirks that are needed
>> in a hypothetical drivers/reset/reset-gpio.c driver.
>>
>> There is no such driver today, just a "reset" driver in
>> drivers/power/reset that resets the whole system.
>>
>> But I see no problem in creating a proper reset driver in drivers/reset
>> to handle a few peripherals with a shared GPIO reset line.
> 
> Personally, I agree with Mark's comment here:
> 
>> [Mark]
>> The theory with that was that any usage of this would need the
>> higher level code using the GPIO to cooperate so they didn't step
>> on each other's toes so the GPIO code should just punt to it.
>>
>>> But let's say that a board design will pick two components (C1 and C2)
>>> and use the same GPIO line to enable them. We already have the drivers
>>> for them and they are used in boards already.
>>
>> This is basically an attempt to make a generic implementation of
>> that cooperation for simple cases.
>>
> 
> This looks like unsolvable problem in generic way.
> Lets assume there are some generic shared reset controller invented, but
> then
> - What if some driver is loaded/unloaded and corresponding device uses
> shared
>   reset which is de-asserted already?
>   In this case, driver should never ever expect that target device has all
>   registers in default state.
> - What if reset is required as part of error recovery procedure? The
> error recovery
>   will not be supported by such design.
> - PM: Device reset could be part of suspend/resume sequence. if one of
> the devices
>   is wake-up source, but other are not, those devices might be in very
> unexpected state during resume.
> - There could be dependencies on reset timings, shared reset might work for
>   similar devices (like set of net phys) and does not work if connected
> devices are different.

and some driver shamelessly implements runtime power/reset control while
other driver does not (they were never used on board where they had
shared GPIO, probably power at most)

> 
> It seems, the only one case when it might help is system boot when:
>  - similar devices are connected to the reset line
>  - drivers are not expected to be re-loaded
>  - device reset is not part of any recovery procedure
>  - safe reset timings can be defined for all connected devices
> (but hey - if this is boot only then gpio-hogs should work. Are they?)

That is another thing which almost works ;)
w/o gpio binding deferred probing is not possible if the GPIO controller
is probed later.
In some cases it might be even impossible to make sure that the GPIO
controller would probe first (GPIO extender on different i2c bus than
the user(s) of the gpio line)
In some cases moving around nodes in DT might artificially make things
work, but then someone compiles the expander as module, or some 'small'
change in kernel and the probe order on the bus changes.
I don't think it is a valid thing to have commits on the DT files
saying: move the expander front/after the hog affected user since since
Monday the probe order has changed. Then move it back two weeks later ;)

> 
> Actually, MDIO bus is one such example where reset line can be toggled by
> as by MDIO bus controller as by PHY drivers.
> 
> So, even thing will move forward with this - it'll be good to have noted
> above restrictions in documentation.
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Grygorii Strashko]

On 05/11/2019 14:32, Peter Ujfalusi wrote:
> 
> 
> On 05/11/2019 14.15, Grygorii Strashko wrote:
>>
>>
>> On 05/11/2019 11:58, Linus Walleij wrote:
>>> On Mon, Nov 4, 2019 at 8:11 PM Rob Herring <robh+dt@kernel.org> wrote:
>>>> [Peter]
>>>>> The device needs the RST line to be high, otherwise it is not
>>>>> accessible. If it does not have reset control how can we make sure that
>>>>> the GPIO line is in correct state?
>>>>
>>>> Just like the reset code, drivers register their use of the reset and
>>>> the core tracks users and prevents resetting when not safe. Maybe the
>>>> reset subsystem needs to learn about GPIO resets. (...)
>>>
>>> I agree. Certainly the reset subsystem can do what the regulator
>>> subsystem is already doing: request the GPIO line nonexclusive
>>> and handle any reference counting and/or quirks that are needed
>>> in a hypothetical drivers/reset/reset-gpio.c driver.
>>>
>>> There is no such driver today, just a "reset" driver in
>>> drivers/power/reset that resets the whole system.
>>>
>>> But I see no problem in creating a proper reset driver in drivers/reset
>>> to handle a few peripherals with a shared GPIO reset line.
>>
>> Personally, I agree with Mark's comment here:
>>
>>> [Mark]
>>> The theory with that was that any usage of this would need the
>>> higher level code using the GPIO to cooperate so they didn't step
>>> on each other's toes so the GPIO code should just punt to it.
>>>
>>>> But let's say that a board design will pick two components (C1 and C2)
>>>> and use the same GPIO line to enable them. We already have the drivers
>>>> for them and they are used in boards already.
>>>
>>> This is basically an attempt to make a generic implementation of
>>> that cooperation for simple cases.
>>>
>>
>> This looks like unsolvable problem in generic way.
>> Lets assume there are some generic shared reset controller invented, but
>> then
>> - What if some driver is loaded/unloaded and corresponding device uses
>> shared
>>    reset which is de-asserted already?
>>    In this case, driver should never ever expect that target device has all
>>    registers in default state.
>> - What if reset is required as part of error recovery procedure? The
>> error recovery
>>    will not be supported by such design.
>> - PM: Device reset could be part of suspend/resume sequence. if one of
>> the devices
>>    is wake-up source, but other are not, those devices might be in very
>> unexpected state during resume.
>> - There could be dependencies on reset timings, shared reset might work for
>>    similar devices (like set of net phys) and does not work if connected
>> devices are different.
> 
> and some driver shamelessly implements runtime power/reset control while
> other driver does not (they were never used on board where they had
> shared GPIO, probably power at most)
> 
>>
>> It seems, the only one case when it might help is system boot when:
>>   - similar devices are connected to the reset line
>>   - drivers are not expected to be re-loaded
>>   - device reset is not part of any recovery procedure
>>   - safe reset timings can be defined for all connected devices
>> (but hey - if this is boot only then gpio-hogs should work. Are they?)
> 
> That is another thing which almost works ;)
> w/o gpio binding deferred probing is not possible if the GPIO controller
> is probed later.
> In some cases it might be even impossible to make sure that the GPIO
> controller would probe first (GPIO extender on different i2c bus than
> the user(s) of the gpio line)
> In some cases moving around nodes in DT might artificially make things
> work, but then someone compiles the expander as module, or some 'small'
> change in kernel and the probe order on the bus changes.
> I don't think it is a valid thing to have commits on the DT files
> saying: move the expander front/after the hog affected user since since
> Monday the probe order has changed. Then move it back two weeks later ;)
>

Ok. Above sounds like real problem. The implicit dependence is exist, but can't
be resolved if any driver depends on gpio-hog of some gpio-controller.
Probe deferring of gpio-controller will not lead to probe differing of dependent driver.

Question: will gpio-hog mechanism resolve your case if it works (and probe differing issues)?

-- 
Best regards,
grygorii

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Peter Ujfalusi]

On 05/11/2019 20.07, Grygorii Strashko wrote:
>>> (but hey - if this is boot only then gpio-hogs should work. Are they?)
>>
>> That is another thing which almost works ;)
>> w/o gpio binding deferred probing is not possible if the GPIO controller
>> is probed later.
>> In some cases it might be even impossible to make sure that the GPIO
>> controller would probe first (GPIO extender on different i2c bus than
>> the user(s) of the gpio line)
>> In some cases moving around nodes in DT might artificially make things
>> work, but then someone compiles the expander as module, or some 'small'
>> change in kernel and the probe order on the bus changes.
>> I don't think it is a valid thing to have commits on the DT files
>> saying: move the expander front/after the hog affected user since since
>> Monday the probe order has changed. Then move it back two weeks later ;)
>>
> 
> Ok. Above sounds like real problem. The implicit dependence is exist,
> but can't
> be resolved if any driver depends on gpio-hog of some gpio-controller.
> Probe deferring of gpio-controller will not lead to probe differing of
> dependent driver.
> 
> Question: will gpio-hog mechanism resolve your case if it works (and
> probe differing issues)?

I see gpio-hog to fulfill different role, use cases. It is more like
controlling muxes on boards to select between different exclusive
features. Things like route the I2S lines to analog codec or HDMI, route
RGB video to LCD panel or to HDMI, etc.

But, if it would work it could be used for components which can be
enabled all the time. On the other hand, if a device has reset/enable
line then the driver should have a way to control it.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Philipp Zabel]

On Wed, 2019-11-06 at 11:23 +0200, Peter Ujfalusi wrote:
> 
> On 05/11/2019 20.07, Grygorii Strashko wrote:
> > > > (but hey - if this is boot only then gpio-hogs should work. Are they?)
> > > 
> > > That is another thing which almost works ;)
> > > w/o gpio binding deferred probing is not possible if the GPIO controller
> > > is probed later.
> > > In some cases it might be even impossible to make sure that the GPIO
> > > controller would probe first (GPIO extender on different i2c bus than
> > > the user(s) of the gpio line)
> > > In some cases moving around nodes in DT might artificially make things
> > > work, but then someone compiles the expander as module, or some 'small'
> > > change in kernel and the probe order on the bus changes.
> > > I don't think it is a valid thing to have commits on the DT files
> > > saying: move the expander front/after the hog affected user since since
> > > Monday the probe order has changed. Then move it back two weeks later ;)
> > > 
> > 
> > Ok. Above sounds like real problem. The implicit dependence is exist,
> > but can't
> > be resolved if any driver depends on gpio-hog of some gpio-controller.
> > Probe deferring of gpio-controller will not lead to probe differing of
> > dependent driver.
> > 
> > Question: will gpio-hog mechanism resolve your case if it works (and
> > probe differing issues)?
> 
> I see gpio-hog to fulfill different role, use cases. It is more like
> controlling muxes on boards to select between different exclusive
> features. Things like route the I2S lines to analog codec or HDMI, route
> RGB video to LCD panel or to HDMI, etc.
> 
> But, if it would work it could be used for components which can be
> enabled all the time. On the other hand, if a device has reset/enable
> line then the driver should have a way to control it.

I wonder if it would be useful to differentiate between required and
suggested state in the consumer facing GPIO API for nonexclusive GPIOs.

A driver that is ok with the enable line going into active state at any
time while the device is suspended could use

	gpiod_set_value(en_gpio, 1);

to resume, but

	gpiod_politely_suggest_value(en_gpio, 0);

or similar to suspend, and the core could allow other drivers to
override this state. Similarly to how the regulator framework allows
consumers to set a voltage range, and the core decides on the actual
voltage that fits the constraints.

regards
Philipp

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Enrico Weigelt, metux IT consult]

On 30.10.19 13:04, Peter Ujfalusi wrote:

Hi,

> For example any device using the same GPIO as reset/enable line can
> reset/enable other devices, which is not something the other device might like
> or can handle.

IMHO, for such cases, invidual drivers shouldn't fiddle w/ raw gpio's
directly, but be connected to (gpio-based) reset controllers or
regulators instead. I believe, GPIO isn't the correct abstraction layer
for such cases: it's not even IO, just O.

Let's sit back and rethink what the driver really wants to tell in those
cases. For the enable lines we have:

a) make sure the device is enabled/powered
b) device does not need to be enabled/powered anymore
c) device must be powercycled

You see, it's actually tristate, which gets relevant if multiple devices
on one line.

Now add reset lines:

a) force device into reset state
b) force device out of reset state
c) allow device going into reset state (but no need to force)
d) allow device coming out of reset state (but no need to force)

It even gets more weird if a device can be reset or powercycled
externally.


hmm, not entirely trivial ...

> For example a device needs to be configured after it is enabled, but some other
> driver would reset it while handling the same GPIO -> the device is not
> operational anymmore as it lost it's configuration.

Yeah, at least we need some signalling to the driver, so it can do the
necessary steps. From the driver's PoV, it's an "foreign reset".

> With the gpio-shared gpiochip we can overcome this by giving the gpio-shared
> the role of making sure that the GPIO line only changes state when it will not
> disturb any of the clients sharing the same GPIO line.

How exactly do we know when such disturbance can / cannot happen ?
That would be depending on individual chips *and* how they're wired on
the board. We'd end up with some logical multiplexer, that's board
specific.

<snip>

> If any of the codec requests the GPIO to be high, the line will go up and will
> only going to be low when both of them set's their shared line to low.

So, if one driver request reset, all attached devices will be reset ?
Or if all drivers request reset, all attached devices will be reset ?

Doesn't look so quite non-disturbing to me :o

> I have also looked at the reset framework, but again it can not be applied in a
> generic way for GPIOs shared for other purposes 

What are the exact scenarios you have in mind ?

> and all existing drivers must
> be converted to use the reset framework (and adding a linux only warpper on top
> of reset GPIOs).

Maybe a bit time consuming, but IMHO not difficult. We could add generic
helpers for creating a reset driver on a gpio. So the drivers wouldn't
even care about gpio itself anymore, but let the reset subsystem so it
all (eg. look for DT node and request corresponding gpio, etc).

IMHO, that's something we should do nevertheless, even if it's just for
cleaner code.

After that we could put any kind of funny logic behind the scenes (eg.
one could connect the reset pin to a spare uart instead of gpio, etc),
w/o ever touching the individual drivers.


--mtx

-- 
Dringender Hinweis: aufgrund existenzieller Bedrohung durch "Emotet"
sollten Sie *niemals* MS-Office-Dokumente via E-Mail annehmen/öffenen,
selbst wenn diese von vermeintlich vertrauenswürdigen Absendern zu
stammen scheinen. Andernfalls droht Totalschaden.
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Philipp Zabel]

Hi,

On Mon, 2019-11-18 at 13:15 +0100, Enrico Weigelt, metux IT consult wrote:
> On 30.10.19 13:04, Peter Ujfalusi wrote:
[...]
> Let's sit back and rethink what the driver really wants to tell in those
> cases. For the enable lines we have:
> 
> a) make sure the device is enabled/powered
> b) device does not need to be enabled/powered anymore
> c) device must be powercycled
> 
> You see, it's actually tristate, which gets relevant if multiple devices
> on one line.

Is this just a GPIO-controlled power domain?

> Now add reset lines:
> 
> a) force device into reset state
> b) force device out of reset state
> c) allow device going into reset state (but no need to force)
> d) allow device coming out of reset state (but no need to force)
> 
> It even gets more weird if a device can be reset or powercycled
> externally.

And some drivers just require "b), but device must have been in reset
state at any point in the past".

> hmm, not entirely trivial ...
> 
> > For example a device needs to be configured after it is enabled, but some other
> > driver would reset it while handling the same GPIO -> the device is not
> > operational anymmore as it lost it's configuration.
> 
> Yeah, at least we need some signalling to the driver, so it can do the
> necessary steps. From the driver's PoV, it's an "foreign reset".

This could become complicated fast. It's trivial to add a notification
mechanism and to let notified drivers veto the foreign reset. But what
if driver (a) wants to reset its hardware and driver (b) could save its
state and handle being reset, but only after some currently active
transfer is finished. Now whether the reset succeeds would depend on how
long driver (b) expects its transfer to last and on how long driver (a)
would be willing to wait for the reset?

[...]
> > and all existing drivers must
> > be converted to use the reset framework (and adding a linux only warpper on top
> > of reset GPIOs).
> 
> Maybe a bit time consuming, but IMHO not difficult. We could add generic
> helpers for creating a reset driver on a gpio. So the drivers wouldn't
> even care about gpio itself anymore, but let the reset subsystem so it
> all (eg. look for DT node and request corresponding gpio, etc).
> 
> IMHO, that's something we should do nevertheless, even if it's just for
> cleaner code.

We can't change the current DT bindings though. One thing we could do is
teach the reset controller framework to handle reset-gpios properties
itself. Still, that wouldn't help with the enable and powerdown GPIOs.

> After that we could put any kind of funny logic behind the scenes (eg.
> one could connect the reset pin to a spare uart instead of gpio, etc),
> w/o ever touching the individual drivers.

I'm not convinced at all that this is a good thing to do behind the
scenes. For those cases I'd prefer adding a "resets" property to the
device bindings and explicitly describing a "uart-reset-controller" in
the device tree, see for example the "pwm-clock".

regards
Philipp

Re: [RFC v2 0/2] gpio: Support for shared GPIO lines on boards

[Peter Ujfalusi]

Hi,

On 18/11/2019 14.15, Enrico Weigelt, metux IT consult wrote:
> On 30.10.19 13:04, Peter Ujfalusi wrote:
> 
> Hi,
> 
>> For example any device using the same GPIO as reset/enable line can
>> reset/enable other devices, which is not something the other device might like
>> or can handle.
> 
> IMHO, for such cases, invidual drivers shouldn't fiddle w/ raw gpio's
> directly, but be connected to (gpio-based) reset controllers or
> regulators instead.

Which is a (linux) software abstraction of an electric wire coming out
from the gpio (or gpo) controller then split into two (or more branch)
and connect to external components...

> I believe, GPIO isn't the correct abstraction layer
> for such cases: it's not even IO, just O.

A GPIO pin configured as output is O ;)

> Let's sit back and rethink what the driver really wants to tell in those
> cases. For the enable lines we have:
> 
> a) make sure the device is enabled/powered
> b) device does not need to be enabled/powered anymore
> c) device must be powercycled
> 
> You see, it's actually tristate, which gets relevant if multiple devices
> on one line.

Yes. Things gets a bit blurry when a GPIO line is used to enable/gate
signals from/to a chip on top of enable/disable, like muting an
amplifier's analog output.

> Now add reset lines:
> 
> a) force device into reset state
> b) force device out of reset state
> c) allow device going into reset state (but no need to force)
> d) allow device coming out of reset state (but no need to force)

I would say that coming out of reset is always forced as there is a
reason why you want to take it out - it is going to be used.

> It even gets more weird if a device can be reset or powercycled
> externally.
> 
> hmm, not entirely trivial ...

When we have only one user of the GPIO reset/enable line we will have a)
and b) happening. If the GPIO is shared most likely the intention of the
hw design dictates c) and d)

>> For example a device needs to be configured after it is enabled, but some other
>> driver would reset it while handling the same GPIO -> the device is not
>> operational anymmore as it lost it's configuration.
> 
> Yeah, at least we need some signalling to the driver, so it can do the
> necessary steps. From the driver's PoV, it's an "foreign reset".

Notification callback for state change?

>> With the gpio-shared gpiochip we can overcome this by giving the gpio-shared
>> the role of making sure that the GPIO line only changes state when it will not
>> disturb any of the clients sharing the same GPIO line.
> 
> How exactly do we know when such disturbance can / cannot happen ?
> That would be depending on individual chips *and* how they're wired on
> the board. We'd end up with some logical multiplexer, that's board
> specific.
> 
> <snip>
> 
>> If any of the codec requests the GPIO to be high, the line will go up and will
>> only going to be low when both of them set's their shared line to low.
> 
> So, if one driver request reset, all attached devices will be reset ?
> Or if all drivers request reset, all attached devices will be reset ?

The later.

> Doesn't look so quite non-disturbing to me :o

This is what regulators and the reset framework is doing, no?

>> I have also looked at the reset framework, but again it can not be applied in a
>> generic way for GPIOs shared for other purposes 
> 
> What are the exact scenarios you have in mind ?

grep -R enable-gpios Documentation/devicetree/bindings/*

pick two random device from the output, place it on a board with shared
enable GPIO line.
I know I over simplify (or complicate) the real world use.

>> and all existing drivers must
>> be converted to use the reset framework (and adding a linux only warpper on top
>> of reset GPIOs).
> 
> Maybe a bit time consuming, but IMHO not difficult. We could add generic
> helpers for creating a reset driver on a gpio. So the drivers wouldn't
> even care about gpio itself anymore, but let the reset subsystem so it
> all (eg. look for DT node and request corresponding gpio, etc).

You mean that users would use reset_control_get_optional_shared() only
and if there is no valid reset binding that the reset core would look
for a gpio binding and instantiate a gpio-reset controller?
But before instantiating it, it would look around in some list to see if
the gpio-reset controller for the same gpio line is already exist?

> IMHO, that's something we should do nevertheless, even if it's just for
> cleaner code.

I'm not sure about that.
D1 have ENABLE pin (enable-gpios as per dt documentation),
 if the line is high, the device is enabled, if low it is disabled.

D2 have RESET pin (reset-gpios as per dt documentation),
 if the line is high, the device is enabled, if low it is disabled.

D1's driver would:
enable-gpios = <&gpio1 0 GPIO_ACTIVE_HIGH>;

priv->reset = reset_control_get_optional_shared(dev, "enable");

/* Place it to reset: ENABLE pin should be pulled low */
reset_control_assert(priv->reset);
/* Remove from reset: ENABLE pin should be high */
reset_control_deassert(priv->reset);

D2's driver would:
reset-gpios = <&gpio1 0 GPIO_ACTIVE_LOW>;

priv->reset = reset_control_get_optional_shared(dev, "reset");

/* Place it to reset: RESET pin should be pulled low */
reset_control_assert(priv->reset);
/* Remove from reset: RESET pin should be high */
reset_control_deassert(priv->reset);

The reset framework must know somehow that the reset control for D1 is
an enable type of gpio, so it must treat it as inverted polarity while
the reset type of binding should be follow the selected active level.

Then it must protect (most likely) the deasserted state: it does not
mater if there is any assert request for the reset_control if we have
one deassert active as at least one device must be enabled.

For new dts files the virtual reset-gpio controller node can be present
and the level of assert and deassert is told to it via the gpio binding.

Something like this?

> After that we could put any kind of funny logic behind the scenes (eg.
> one could connect the reset pin to a spare uart instead of gpio, etc),
> w/o ever touching the individual drivers.

Not sure if I follow you here.

On the other hand the gpio line itself can be seen as a regultator
itself (3.3V most of the time) so in theory all GPIOs can be regulators
as well, but regulator framework protects the >0 volt state while there
are devices which can be enabled when the ENABLE/RST pin is pulled low.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki