[PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

From: Linus Walleij <linus.walleij@linaro.org>

Currently the kernel is ambigously treating GPIOs and interrupts
from a GPIO controller: GPIOs and interrupts are treated as
orthogonal. This unfortunately makes it unclear how to actually
retrieve and request a GPIO line or interrupt from a GPIO
controller in the device tree probe path.

In the non-DT probe path it is clear that the GPIO number has to
be passed to the consuming device, and if it is to be used as
an interrupt, the consumer shall performa a gpio_to_irq() mapping
and request the resulting IRQ number.

In the DT boot path consumers may have been given one or more
interrupts from the interrupt-controller side of the GPIO chip
in an abstract way, such that the driver is not aware of the
fact that a GPIO chip is backing this interrupt, and the GPIO
side of the same line is never requested with gpio_request().
A typical case for this is ethernet chips which just take some
interrupt line which may be a "hard" interrupt line (such as an
external interrupt line from an SoC) or a cascaded interrupt
connected to a GPIO line.

This has the following undesired effects:

- The GPIOlib subsystem is not aware that the line is in use
  and willingly lets some other consumer perform gpio_request()
  on it, leading to a complex resource conflict if it occurs.

- The GPIO debugfs file claims this GPIO line is "free".

- The line direction of the interrupt GPIO line is not
  explicitly set as input, even though it is obvious that such
  a line need to be set up in this way, often making the system
  depend on boot-on defaults for this kind of settings.

To solve this dilemma, perform an interrupt consistency check
when adding a GPIO chip: if the chip is both gpio-controller and
interrupt-controller, walk all children of the device tree,
check if these in turn reference the interrupt-controller, and
if they do, loop over the interrupts used by that child and
perform gpio_request() and gpio_direction_input() on these,
making them unreachable from the GPIO side.

The patch has been devised by Linus Walleij and Lars Poeschel.

Changelog V3:
- define of_gpiochip_reserve_irq_lines as empty if
  CONFIG_OF_IRQ is not defined. Walking the devicetree simply
  makes no sense in this case and caused a compile time error
  on SPARC.
- exit of_gpiochip_reserve_irq_lines if no irq_domain can be
  found.
- convert the irqspec to cpu byte order before invoking the
  irq_domains xlate function.

Changelog V2:
- To be able to parse custom interrupts properties from the
  device tree, get a reference to the drivers irq_domain
  and use the xlate function to parse the proptery and
  get the irq number. This is tested with
  #interrupt-cells = 1, 2, and 3 and multiple interrupts
  per property.

Cc: devicetree@vger.kernel.org
Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Cc: Enric Balletbo i Serra <eballetbo@gmail.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Balaji T K <balajitk@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Jon Hunter <jgchunter@gmail.com>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-of.c |  201 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 200 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 665f953..d328c5d 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -10,7 +10,6 @@
  * the Free Software Foundation; either version 2 of the License, or
  * (at your option) any later version.
  */
-
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/module.h>
@@ -19,6 +18,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_gpio.h>
+#include <linux/of_irq.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/slab.h>
 
@@ -126,6 +126,201 @@ int of_gpio_simple_xlate(struct gpio_chip *gc,
 }
 EXPORT_SYMBOL(of_gpio_simple_xlate);
 
+#if defined(CONFIG_OF_IRQ)
+/**
+ * of_gpio_scan_irq_lines() - internal function to recursively scan the device
+ * tree and request or free the GPIOs that are to be used as IRQ lines
+ * @node:	node to start the scanning at
+ * @gcn:	device node of the GPIO chip
+ * @irq_domain:	the irq_domain for the GPIO chip
+ * @intsize:	size of one single interrupt in the device tree for the GPIO
+ *		chip. It is the same as #interrupt-cells.
+ * @gc:		GPIO chip instantiated from same node
+ * @request:	wheter the function should request(true) or free(false) the
+ *		irq lines
+ *
+ * This is a internal function that calls itself to recursively scan the device
+ * tree. It scans for uses of the device_node gcn as an interrupt-controller.
+ * If it finds some, it requests the corresponding gpio lines that are to be
+ * used as irq lines and sets them as input.
+ *
+ * If the request parameter is 0 it frees the gpio lines.
+ * For more information see documentation of of_gpiochip_reserve_irq_lines
+ * function.
+ */
+static void of_gpio_scan_irq_lines(const struct device_node *const node,
+				   struct device_node *const gcn,
+				   struct irq_domain *const irq_domain,
+				   const u32 intsize,
+				   const struct gpio_chip * const gc,
+				   bool request)
+{
+	struct device_node *child;
+	struct device_node *irq_parent;
+	const __be32 *intspec;
+	u32 intlen;
+	u32 specifier[OF_MAX_IRQ_SPEC];
+	int ret;
+	int i, j;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+
+	if (node == NULL)
+		return;
+
+	for_each_child_of_node(node, child) {
+		of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
+				       request);
+		/* Check if we have an IRQ parent, else continue */
+		irq_parent = of_irq_find_parent(child);
+		if (!irq_parent)
+			continue;
+
+		/* Is it so that this very GPIO chip is the parent? */
+		if (irq_parent != gcn) {
+			of_node_put(irq_parent);
+			continue;
+		}
+		of_node_put(irq_parent);
+
+		pr_debug("gpiochip OF: node %s references GPIO interrupt lines\n",
+				child->name);
+
+		/* Get the interrupts property */
+		intspec = of_get_property(child, "interrupts", &intlen);
+		if (intspec == NULL)
+			continue;
+		intlen /= sizeof(*intspec);
+
+		for (i = 0; i < intlen; i += intsize) {
+			/*
+			 * Find out the local IRQ number. This corresponds to
+			 * the GPIO line offset for a GPIO chip.
+			 */
+			if (irq_domain->ops->xlate) {
+				for (j = 0; j < intsize; j++)
+					specifier[j] = of_read_number(intspec +
+								i + j, 1);
+				irq_domain->ops->xlate(irq_domain, gcn,
+						       specifier, intsize,
+						       &hwirq, &type);
+			} else
+				hwirq = be32_to_cpu(intspec[0]);
+
+			pr_debug("gpiochip OF: node %s references GPIO %lu (%lu)\n",
+				child->name, gc->base + hwirq, hwirq);
+
+			if (request) {
+				/*
+				 * This child is making a reference to this
+				 * chip through the interrupts property, so
+				 * reserve these GPIO lines and set them as
+				 * input.
+				 */
+				ret = gpio_request(gc->base + hwirq,
+						   child->name);
+				if (ret)
+					pr_err("gpiolib OF: could not request IRQ GPIO %lu (%lu) for node %s (%d)\n",
+						gc->base + hwirq, hwirq,
+						child->name, ret);
+				ret = gpio_direction_input(gc->base + hwirq);
+				if (ret)
+					pr_err("gpiolib OF: could not set IRQ GPIO %lu (%lu) as input for node %s (%d)\n",
+						gc->base + hwirq, hwirq,
+						child->name, ret);
+			} else {
+				gpio_free(gc->base + hwirq);
+			}
+		}
+	}
+}
+
+/**
+ * of_gpiochip_reserve_irq_lines() - request or free GPIO IRQ lines
+ * @np:		device node of the GPIO chip
+ * @gc:		GPIO chip instantiated from same node
+ * @request:	wheter the function should request(1) or free(0) the irq lines
+ *
+ * This function should not be used directly, use the macros
+ * of_gpiochip_request_irq_lines or of_gpiochip_free_gpio_lines instead.
+ *
+ * For the case of requesting the irq lines (request == 1) this function is
+ * called after instantiating a GPIO chip from a device tree node to assert
+ * that all interrupts derived from the chip are consistently requested as
+ * GPIO lines, if the GPIO chip is BOTH a gpio-controller AND an
+ * interrupt-controller.
+ *
+ * If another node in the device tree is referencing the interrupt-controller
+ * portions of the GPIO chip, such that it is using a GPIO line as some
+ * arbitrary interrupt source, the following holds:
+ *
+ * - That line must NOT be used anywhere else in the device tree as a
+ *   <&gpio N> reference, or GPIO and interrupt usage may conflict.
+ *
+ * Conversely, if a node is using a line as a direct reference to a GPIO line,
+ * no node in the tree may use that line as an interrupt.
+ *
+ * If another node is referencing a GPIO line, and also want to use that line
+ * as an interrupt source, it is necessary for this driver to use the
+ * gpio_to_irq() kernel interface.
+ *
+ * For the case of freeing the irq lines (request == 0) this function simply
+ * uses the same device tree information used to request the irq lines to call
+ * gpiochip_free on that GPIOs.
+ */
+static void of_gpiochip_reserve_irq_lines(struct device_node *np,
+					  struct gpio_chip *gc, bool request)
+{
+	struct device_node *root;
+	const __be32 *tmp;
+	struct irq_domain *irq_domain;
+	u32 intsize;
+
+	/*
+	 * If this chip is not tagged as interrupt-controller, there is
+	 * no problem so we just exit.
+	 */
+	if (!of_property_read_bool(np, "interrupt-controller"))
+		return;
+
+	/*
+	 * Proceed to check the consistency of all references to this
+	 * GPIO chip.
+	 */
+	root = of_find_node_by_path("/");
+	if (!root)
+		return;
+
+	tmp = of_get_property(np, "#interrupt-cells", NULL);
+	if (tmp == NULL)
+		intsize = 1;
+	else
+		intsize = be32_to_cpu(*tmp);
+
+	irq_domain = irq_find_host(np);
+	if (irq_domain == NULL) {
+		dev_err(gc->dev, "Unable to find a irq_domain, aborting to request gpios for interrupts used from combined gpio-/interrupt-controllers\n");
+		goto err;
+	}
+
+	of_gpio_scan_irq_lines(root, np, irq_domain, intsize, gc, request);
+err:
+	of_node_put(root);
+}
+#else
+static void of_gpiochip_reserve_irq_lines(struct device_node *np,
+					  struct gpio_chip *gc, bool request)
+{
+	return;
+}
+#endif /* defined(CONFIG_OF_IRQ) */
+
+#define of_gpiochip_request_irq_lines(np, gc) \
+	of_gpiochip_reserve_irq_lines(np, gc, true)
+
+#define of_gpiochip_free_irq_lines(np, gc) \
+	of_gpiochip_reserve_irq_lines(np, gc, false)
+
 /**
  * of_mm_gpiochip_add - Add memory mapped GPIO chip (bank)
  * @np:		device node of the GPIO chip
@@ -170,6 +365,8 @@ int of_mm_gpiochip_add(struct device_node *np,
 	if (ret)
 		goto err2;
 
+	of_gpiochip_request_irq_lines(np, gc);
+
 	return 0;
 err2:
 	iounmap(mm_gc->regs);
@@ -231,12 +428,14 @@ void of_gpiochip_add(struct gpio_chip *chip)
 		chip->of_xlate = of_gpio_simple_xlate;
 	}
 
+	of_gpiochip_request_irq_lines(chip->of_node, chip);
 	of_gpiochip_add_pin_range(chip);
 	of_node_get(chip->of_node);
 }
 
 void of_gpiochip_remove(struct gpio_chip *chip)
 {
+	of_gpiochip_free_irq_lines(chip->of_node, chip);
 	gpiochip_remove_pin_ranges(chip);
 
 	if (chip->of_node)
-- 
1.7.10.4

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/26/2013 08:07 AM, Lars Poeschel wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> Currently the kernel is ambigously treating GPIOs and interrupts
> from a GPIO controller: GPIOs and interrupts are treated as
> orthogonal. This unfortunately makes it unclear how to actually
> retrieve and request a GPIO line or interrupt from a GPIO
> controller in the device tree probe path.

I still think that this patch is the wrong approach. Instead, the logic
should be hooked into gpio_request() and request_irq(). This patch only
addresses DT, and ignores anything else, hence doesn't seem like the
right level of abstraction to plug in, since the issue is not related to DT.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Santosh Shilimkar]

On Tuesday 27 August 2013 04:17 PM, Stephen Warren wrote:
> On 08/26/2013 08:07 AM, Lars Poeschel wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> Currently the kernel is ambigously treating GPIOs and interrupts
>> from a GPIO controller: GPIOs and interrupts are treated as
>> orthogonal. This unfortunately makes it unclear how to actually
>> retrieve and request a GPIO line or interrupt from a GPIO
>> controller in the device tree probe path.
> 
> I still think that this patch is the wrong approach. Instead, the logic
> should be hooked into gpio_request() and request_irq(). This patch only
> addresses DT, and ignores anything else, hence doesn't seem like the
> right level of abstraction to plug in, since the issue is not related to DT.
> 
On the issue, Rajendra cooked up a patch [1] to handle the issue at
irqdomain level which was shot down after further discussion. It might
be useful for the discussion here. Ofcourse it was also targeted to
solve only DT issue.

Regards,
Santosh

[1]
"[PATCH] irqdomain: Do a gpio_request in case of a gpio irq_chip"
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg83451.html

RE: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Strashko, Grygorii]

Hi Lars, Linus,

I've few comments regarding this patch and problem in general.

On 08/26/2013 05:07 PM, Lars Poeschel wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
>
> Currently the kernel is ambigously treating GPIOs and interrupts
> from a GPIO controller: GPIOs and interrupts are treated as
> orthogonal. This unfortunately makes it unclear how to actually
> retrieve and request a GPIO line or interrupt from a GPIO
> controller in the device tree probe path.
>
> In the non-DT probe path it is clear that the GPIO number has to
> be passed to the consuming device, and if it is to be used as
> an interrupt, the consumer shall performa a gpio_to_irq() mapping
> and request the resulting IRQ number.
>
> In the DT boot path consumers may have been given one or more
> interrupts from the interrupt-controller side of the GPIO chip
> in an abstract way, such that the driver is not aware of the
> fact that a GPIO chip is backing this interrupt, and the GPIO
> side of the same line is never requested with gpio_request().
> A typical case for this is ethernet chips which just take some
> interrupt line which may be a "hard" interrupt line (such as an
> external interrupt line from an SoC) or a cascaded interrupt
> connected to a GPIO line.
>
> This has the following undesired effects:
>
> - The GPIOlib subsystem is not aware that the line is in use
>    and willingly lets some other consumer perform gpio_request()
>    on it, leading to a complex resource conflict if it occurs.
>
> - The GPIO debugfs file claims this GPIO line is "free".
>
> - The line direction of the interrupt GPIO line is not
>    explicitly set as input, even though it is obvious that such
>    a line need to be set up in this way, often making the system
>    depend on boot-on defaults for this kind of settings.
>
> To solve this dilemma, perform an interrupt consistency check
> when adding a GPIO chip: if the chip is both gpio-controller and
> interrupt-controller, walk all children of the device tree,
> check if these in turn reference the interrupt-controller, and
> if they do, loop over the interrupts used by that child and
> perform gpio_request() and gpio_direction_input() on these,
> making them unreachable from the GPIO side.
>
> The patch has been devised by Linus Walleij and Lars Poeschel.
>
> Changelog V3:
> - define of_gpiochip_reserve_irq_lines as empty if
>    CONFIG_OF_IRQ is not defined. Walking the devicetree simply
>    makes no sense in this case and caused a compile time error
>    on SPARC.
> - exit of_gpiochip_reserve_irq_lines if no irq_domain can be
>    found.
> - convert the irqspec to cpu byte order before invoking the
>    irq_domains xlate function.
>
> Changelog V2:
> - To be able to parse custom interrupts properties from the
>    device tree, get a reference to the drivers irq_domain
>    and use the xlate function to parse the proptery and
>    get the irq number. This is tested with
>    #interrupt-cells = 1, 2, and 3 and multiple interrupts
>    per property.
>
> Cc: devicetree@vger.kernel.org
> Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Cc: Enric Balletbo i Serra <eballetbo@gmail.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Balaji T K <balajitk@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Jon Hunter <jgchunter@gmail.com>
> Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>   drivers/gpio/gpiolib-of.c |  201 ++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 200 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 665f953..d328c5d 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -10,7 +10,6 @@
>    * the Free Software Foundation; either version 2 of the License, or
>    * (at your option) any later version.
>    */
> -
>   #include <linux/device.h>
>   #include <linux/errno.h>
>   #include <linux/module.h>
> @@ -19,6 +18,7 @@
>   #include <linux/of.h>
>   #include <linux/of_address.h>
>   #include <linux/of_gpio.h>
> +#include <linux/of_irq.h>
>   #include <linux/pinctrl/pinctrl.h>
>   #include <linux/slab.h>
>
> @@ -126,6 +126,201 @@ int of_gpio_simple_xlate(struct gpio_chip *gc,
>   }
>   EXPORT_SYMBOL(of_gpio_simple_xlate);
>
> +#if defined(CONFIG_OF_IRQ)
> +/**
> + * of_gpio_scan_irq_lines() - internal function to recursively scan the device
> + * tree and request or free the GPIOs that are to be used as IRQ lines
> + * @node:	node to start the scanning at
> + * @gcn:	device node of the GPIO chip
> + * @irq_domain:	the irq_domain for the GPIO chip
> + * @intsize:	size of one single interrupt in the device tree for the GPIO
> + *		chip. It is the same as #interrupt-cells.
> + * @gc:		GPIO chip instantiated from same node
> + * @request:	wheter the function should request(true) or free(false) the
> + *		irq lines
> + *
> + * This is a internal function that calls itself to recursively scan the device
> + * tree. It scans for uses of the device_node gcn as an interrupt-controller.
> + * If it finds some, it requests the corresponding gpio lines that are to be
> + * used as irq lines and sets them as input.
> + *
> + * If the request parameter is 0 it frees the gpio lines.
> + * For more information see documentation of of_gpiochip_reserve_irq_lines
> + * function.
> + */
> +static void of_gpio_scan_irq_lines(const struct device_node *const node,
> +				   struct device_node *const gcn,
> +				   struct irq_domain *const irq_domain,
> +				   const u32 intsize,
> +				   const struct gpio_chip * const gc,
> +				   bool request)
> +{
> +	struct device_node *child;
> +	struct device_node *irq_parent;
> +	const __be32 *intspec;
> +	u32 intlen;
> +	u32 specifier[OF_MAX_IRQ_SPEC];
> +	int ret;
> +	int i, j;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +
> +	if (node == NULL)
> +		return;
> +
> +	for_each_child_of_node(node, child) {


p1) for_each_child_of_node will traverse all the child nodes
independently of whether the node is available or not. As result, GPIO
line may be requested for disabled nodes (status="disabled")

p2) If any child of complex devices, such MFD, will use GPIO line as IRQ, then corresponding GPIO line will always be requested.
BUT - the MFD driver may not be loaded at all, so there will be "GPIO leak" which will affect on power consumption (GPIO bank will not be switched to idle state).

p3) Would it be allowed to use shared GPIO IRQs after this patch?

p4) Who/When will free GPIO line? This question is not about GPIO chip removing/unloading -
It's about unloading GPIO chip consumers: loadable
drivers (especially MFD devices) and such feature as DT-overlays.
( Looks like "Release resources assigned by DT-core/DT-helpers" is
common problem.
For example, similar problem with IRQ descriptors:
irq_dispose_mapping() is not called for IRQs, which were allocated/mapped by
 DT-core from irq_of_parse_and_map().
 So, there is no simple way to free IRQ-descriptors in case if MFD is unloaded
 and, at the same time, It is IRQ controller and uses linear irq_domain.)

> +		of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
> +				       request);
> +		/* Check if we have an IRQ parent, else continue */
> +		irq_parent = of_irq_find_parent(child);
> +		if (!irq_parent)
> +			continue;
> +
> +		/* Is it so that this very GPIO chip is the parent? */
> +		if (irq_parent != gcn) {
> +			of_node_put(irq_parent);
> +			continue;
> +		}
> +		of_node_put(irq_parent);
> +
> +		pr_debug("gpiochip OF: node %s references GPIO interrupt lines\n",
> +				child->name);
> +
> +		/* Get the interrupts property */
> +		intspec = of_get_property(child, "interrupts", &intlen);
> +		if (intspec == NULL)
> +			continue;
> +		intlen /= sizeof(*intspec);
> +
> +		for (i = 0; i < intlen; i += intsize) {
> +			/*
> +			 * Find out the local IRQ number. This corresponds to
> +			 * the GPIO line offset for a GPIO chip.
> +			 */
> +			if (irq_domain->ops->xlate) {
> +				for (j = 0; j < intsize; j++)
> +					specifier[j] = of_read_number(intspec +
> +								i + j, 1);
> +				irq_domain->ops->xlate(irq_domain, gcn,
> +						       specifier, intsize,
> +						       &hwirq, &type);
> +			} else
> +				hwirq = be32_to_cpu(intspec[0]);
> +
> +			pr_debug("gpiochip OF: node %s references GPIO %lu (%lu)\n",
> +				child->name, gc->base + hwirq, hwirq);
> +
> +			if (request) {
> +				/*
> +				 * This child is making a reference to this
> +				 * chip through the interrupts property, so
> +				 * reserve these GPIO lines and set them as
> +				 * input.
> +				 */
> +				ret = gpio_request(gc->base + hwirq,
> +						   child->name);
> +				if (ret)
> +					pr_err("gpiolib OF: could not request IRQ GPIO %lu (%lu) for node %s (%d)\n",
> +						gc->base + hwirq, hwirq,
> +						child->name, ret);
> +				ret = gpio_direction_input(gc->base + hwirq);
> +				if (ret)
> +					pr_err("gpiolib OF: could not set IRQ GPIO %lu (%lu) as input for node %s (%d)\n",
> +						gc->base + hwirq, hwirq,
> +						child->name, ret);

gpio_request_one

> +			} else {
> +				gpio_free(gc->base + hwirq);
> +			}
> +		}
> +	}

[...]

> +}
>   /**
>    * of_mm_gpiochip_add - Add memory mapped GPIO chip (bank)
>    * @np:		device node of the GPIO chip
> @@ -170,6 +365,8 @@ int of_mm_gpiochip_add(struct device_node *np,
>   	if (ret)
>   		goto err2;
>

p5) Most of GPIO chip implementations use simple call to gpiochip_add
and not of_mm_gpiochip_add - at least gpio-omap.c (which is important
for me:)

> +	of_gpiochip_request_irq_lines(np, gc);
> +
>   	return 0;
>   err2:
>   	iounmap(mm_gc->regs);
> @@ -231,12 +428,14 @@ void of_gpiochip_add(struct gpio_chip *chip)
>   		chip->of_xlate = of_gpio_simple_xlate;
>   	}
>
> +	of_gpiochip_request_irq_lines(chip->of_node, chip);
>   	of_gpiochip_add_pin_range(chip);
>   	of_node_get(chip->of_node);
>   }
>
>   void of_gpiochip_remove(struct gpio_chip *chip)
>   {
> +	of_gpiochip_free_irq_lines(chip->of_node, chip);
>   	gpiochip_remove_pin_ranges(chip);
>
>   	if (chip->of_node)
>

I think there are two major problems (at least about IRQs, but seems
the same is valid for DMA too):
1) Data duplication
- from one side DT has all needed information about HW resources
- from other side, the same data maintained using "struct resource" per
device
  Such duplication should definitely be removed and all data should be
taken from DT (if DT is the future of course:))).

2) DT-core/helpers - performs actions on HW resources which they
shouldn't do, i think (like IRQ mapping&descriptors allocation, GPIO
request and etc.). Instead all these stuff should be done/initiated by
drivers when (and only when) they are going to request&use HW resource.

[v1]
According, to explained above, I thought, that it would be reasonable
to move IRQ parsing&mapping code inside platform_get_xxx() helpers and
and corresponding code for GPIO requesting inside irq_of_parse_and_map() -
so, any manipulation with IRQ resources will by performed only by request
form drivers (this solves [p1] & [p2] & [p5]).
[to prove this concept I've created patch -
 dirty version attached below, boot tested on OMAP4 SDP, based on v3.11-rc6]

In addition, I tried to use "devres" API for GPIO requesting (this was my goal to solve [p4]) -
but I stuck with the problem of getting pointer on device which requests IRQ
and got problem with frameworks like SPI & I2C which use
irq_of_parse_and_map() directly before calling the device_add().

[v2]
A result, I think, that the best way to handle IRQ resources
configuration will be from request_threaded_irq():
- create "of" equivalents for request_irq()/request_threaded_irq()/
/devm_request_threaded_irq()/devm_request_irq()
- switch drivers to use them and finally get rid of IORESOURCE_IRQ type at all.

To solve [p3], IRQF_SHARED need to be populated through DT per IRQ
somehow, and GPIO should be requested only once.
Or "gpio-reset"/"fixed-regulator" could be used in DT for such GPIOs and
GPIO shouldn't never be requested by "of_irq" code if IRQF_SHARED
is specified for some IRQ.

Of course, I could miss smth, because my investigation is based on ARM OMAP2 code only.

Regards,
-grygorii

---
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 3c3197a..68dcb15 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -22,7 +22,8 @@
 #include <linux/pm_runtime.h>
 #include <linux/idr.h>
 #include <linux/acpi.h>
-
+#include <linux/of_irq.h>
+#include <linux/gpio.h>
 #include "base.h"
 #include "power/power.h"

@@ -64,6 +65,19 @@ struct resource *platform_get_resource(struct platform_device *dev,
 {
 	int i;

+#ifdef CONFIG_OF
+	if (IORESOURCE_IRQ == (type & IORESOURCE_TYPE_BITS)) {
+		struct resource	 *r_irq;
+
+		r_irq = devm_kzalloc(&dev->dev, sizeof(*r_irq), GFP_KERNEL);
+		if (r_irq) {
+			if (!of_irq_to_resource(dev->dev.of_node, num, r_irq))
+				return NULL;
+		}
+		return r_irq;
+	}
+#endif
+
 	for (i = 0; i < dev->num_resources; i++) {
 		struct resource *r = &dev->resource[i];

@@ -87,9 +101,13 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 		return -ENXIO;
 	return dev->archdata.irqs[num];
 #else
+#ifndef CONFIG_OF
 	struct resource *r = platform_get_resource(dev, IORESOURCE_IRQ, num);

 	return r ? r->start : -ENXIO;
+#else
+	return irq_of_parse_and_map(dev->dev.of_node, num);
+#endif
 #endif
 }
 EXPORT_SYMBOL_GPL(platform_get_irq);
@@ -105,7 +123,18 @@ struct resource *platform_get_resource_byname(struct platform_device *dev,
 					      const char *name)
 {
 	int i;
-
+#ifdef CONFIG_OF
+	const char *name_irq = NULL;
+	int index = 0;
+
+	if (IORESOURCE_IRQ == (type & IORESOURCE_TYPE_BITS))
+		while (!of_property_read_string_index(dev->dev.of_node,
+			"interrupt-names", index, &name_irq)) {
+			if (!strcmp(name, name_irq)) {
+				return platform_get_resource(dev, type, index);
+			}
+		}
+#endif
 	for (i = 0; i < dev->num_resources; i++) {
 		struct resource *r = &dev->resource[i];

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 1264923..91f3330 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -25,7 +25,19 @@
 #include <linux/of_irq.h>
 #include <linux/string.h>
 #include <linux/slab.h>
+#include <linux/gpio.h>
+#include <linux/of_platform.h>

+
+static int irq_of_gpiochip_find(struct gpio_chip *gc, void *data)
+{
+	struct device_node *np = data;
+
+	if (gc->of_node != np)
+		return false;
+
+	return true;
+}
 /**
  * irq_of_parse_and_map - Parse and map an interrupt into linux virq space
  * @device: Device node of the device whose interrupt is to be mapped
@@ -37,12 +49,57 @@
 unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
 {
 	struct of_irq oirq;
+	unsigned int virq;
+	irq_hw_number_t hwirq;
+	unsigned int type = IRQ_TYPE_NONE;

 	if (of_irq_map_one(dev, index, &oirq))
 		return 0;

-	return irq_create_of_mapping(oirq.controller, oirq.specifier,
-				     oirq.size);
+	virq = irq_create_of_mapping(oirq.controller, oirq.specifier,
+				     oirq.size, &hwirq, &type);
+	if (!virq)
+		return virq;
+
+	if (of_property_read_bool(oirq.controller, "gpio-controller")) {
+		struct gpio_chip *gc;
+		struct irq_domain *domain;
+		int ret;
+		struct platform_device *pdev;
+		pr_err("========= GPIO-IRQ %s %ul->%u\n", oirq.controller->name, (unsigned int)hwirq, virq);
+
+		domain = irq_find_host(oirq.controller);
+		if (!domain) {
+			pr_err("======= could not find IRQ domain for GPIO chip\n");
+			return 0;
+		}
+
+		gc = gpiochip_find(oirq.controller, irq_of_gpiochip_find);
+		if (!gc) {
+			pr_err("======= could not find GPIO chip\n");
+			return 0;
+		}
+
+/*		pdev = of_find_device_by_node(dev);
+		if (!pdev) {
+			pr_err("======= could not find pdev %s\n", dev->name);
+			return 0;
+		} else {
+			ret = devm_gpio_request_one(&pdev->dev,
+				    gc->base + hwirq, GPIOF_DIR_IN, dev->name);
+		}*/
+		ret = gpio_request_one(gc->base + hwirq, GPIOF_DIR_IN, dev->name);
+		if (ret)
+			pr_err("======= could not request IRQ GPIO %lu (%u) for node %s (%d)\n",
+			       gc->base + hwirq, (unsigned int)hwirq, dev->name, ret);
+	}
+
+	/* Set type if specified and different than the current one */
+	if (type != IRQ_TYPE_NONE &&
+	    type != irq_get_trigger_type(virq))
+		irq_set_irq_type(virq, type);
+
+	return virq;
 }
 EXPORT_SYMBOL_GPL(irq_of_parse_and_map);

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index e0a6514..c685fce 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -168,7 +168,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
 			rc = of_address_to_resource(np, i, res);
 			WARN_ON(rc);
 		}
-		WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
+		//WARN_ON(of_irq_to_resource_table(np, res, num_irq) != num_irq);
 	}

 	dev->dev.of_node = of_node_get(np);
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 535cecf..8e50653 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -65,7 +65,9 @@ extern int of_irq_map_one(struct device_node *device, int index,
 			  struct of_irq *out_irq);
 extern unsigned int irq_create_of_mapping(struct device_node *controller,
 					  const u32 *intspec,
-					  unsigned int intsize);
+					  unsigned int intsize,
+					  irq_hw_number_t *hwirq,
+					  unsigned int *type);
 extern int of_irq_to_resource(struct device_node *dev, int index,
 			      struct resource *r);
 extern int of_irq_count(struct device_node *dev);
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 706724e..ff0d26d 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -466,11 +466,10 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base,
 EXPORT_SYMBOL_GPL(irq_create_strict_mappings);

 unsigned int irq_create_of_mapping(struct device_node *controller,
-				   const u32 *intspec, unsigned int intsize)
+				   const u32 *intspec, unsigned int intsize,
+				   irq_hw_number_t *hwirq, unsigned int *type)
 {
 	struct irq_domain *domain;
-	irq_hw_number_t hwirq;
-	unsigned int type = IRQ_TYPE_NONE;
 	unsigned int virq;

 	domain = controller ? irq_find_host(controller) : irq_default_domain;
@@ -482,22 +481,16 @@ unsigned int irq_create_of_mapping(struct device_node *controller,

 	/* If domain has no translation, then we assume interrupt line */
 	if (domain->ops->xlate == NULL)
-		hwirq = intspec[0];
+		*hwirq = intspec[0];
 	else {
 		if (domain->ops->xlate(domain, controller, intspec, intsize,
-				     &hwirq, &type))
+				       hwirq, type))
 			return 0;
 	}

 	/* Create mapping */
-	virq = irq_create_mapping(domain, hwirq);
-	if (!virq)
-		return virq;
+	virq = irq_create_mapping(domain, *hwirq);

-	/* Set type if specified and different than the current one */
-	if (type != IRQ_TYPE_NONE &&
-	    type != irq_get_trigger_type(virq))
-		irq_set_irq_type(virq, type);
 	return virq;
 }
 EXPORT_SYMBOL_GPL(irq_create_of_mapping);

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Tue, Aug 27, 2013 at 10:17 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/26/2013 08:07 AM, Lars Poeschel wrote:
>>
>> Currently the kernel is ambigously treating GPIOs and interrupts
>> from a GPIO controller: GPIOs and interrupts are treated as
>> orthogonal. This unfortunately makes it unclear how to actually
>> retrieve and request a GPIO line or interrupt from a GPIO
>> controller in the device tree probe path.
>
> I still think that this patch is the wrong approach. Instead, the logic
> should be hooked into gpio_request() and request_irq(). This patch only
> addresses DT, and ignores anything else, hence doesn't seem like the
> right level of abstraction to plug in, since the issue is not related to DT.

We tried to do it that way, and it exploded. See commit
b4419e1a15905191661ffe75ba2f9e649f5d565e
"gpio/omap: auto request GPIO as input if used as IRQ via DT"

Here request_irq() augmented through its irqdomain to
issue gpio_request_one().

Why was this patch reverted? It seems this is what has not
managed to reach the audience here.

It turns out some drivers were already doing this:

request_gpio(gpio);
gpio_direction_input(gpio);
request_irq(gpio_to_irq(gpio));

Looks perfectly valid right?

Not so: after the above change, we tried to request the
GPIO a *second time* in the GPIO driver's irq map function,
and of course it failed, as it was already taken.

So saying that it should be done in the request_irq()
function is imposing this other semantic on the kernel
instead: you must *NOT* request the GPIO with
request_gpio() if you're going to use it as an IRQ.

(Also, it force us to implement the same code in each
and every driver, but that is a lesser problem.)

I don't quite understand what is so hard around this.
We cannot get away from restricting the semantics in
some way, if gpio-controllers shall also be interrupt-controllers,
the current patch is the least intrusive I've seen so far.

And I don't even think this is a Linux problem, handing
out the same resource with two hands is ambigous and is
going to cause problems with any operating system.
It is better to restrict this and say in the binding doc that
the gpio line cannot be <&gpio n> assigned when there
is an interrupt on the same line.

We can start out by printing warnings when we fail to
request the corresponding GPIO for an interrupt line
though, it will be annoying enough and a kind of
compromise.

Or it has to be the GPIO input hogs.

Yours,
Linus Walleij

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 08/29/2013 09:26 PM, Linus Walleij wrote:
> On Tue, Aug 27, 2013 at 10:17 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/26/2013 08:07 AM, Lars Poeschel wrote:
>>>
>>> Currently the kernel is ambigously treating GPIOs and interrupts
>>> from a GPIO controller: GPIOs and interrupts are treated as
>>> orthogonal. This unfortunately makes it unclear how to actually
>>> retrieve and request a GPIO line or interrupt from a GPIO
>>> controller in the device tree probe path.
>>
>> I still think that this patch is the wrong approach. Instead, the logic
>> should be hooked into gpio_request() and request_irq(). This patch only
>> addresses DT, and ignores anything else, hence doesn't seem like the
>> right level of abstraction to plug in, since the issue is not related to DT.
> 
> We tried to do it that way, and it exploded. See commit
> b4419e1a15905191661ffe75ba2f9e649f5d565e
> "gpio/omap: auto request GPIO as input if used as IRQ via DT"
> 
> Here request_irq() augmented through its irqdomain to
> issue gpio_request_one().
> 
> Why was this patch reverted? It seems this is what has not
> managed to reach the audience here.
>

The mentioned patch also broke some ancient platforms that have not been (and
probably never will) migrated to DT like OMAP1.

So, after trying to add this logic by using a custom irq domain .map function
handler for the OMAP GPIO driver I agree with Linus that is better to provide a
solution at the Device Tree level to not affect platforms that are still using
legacy boot.

Legacy boot don't need a fix since it does not have this issue. Platform code
just request the GPIO and do the setup (gpio_direction_input) before the drivers
call to request_irq().

> It turns out some drivers were already doing this:
> 
> request_gpio(gpio);
> gpio_direction_input(gpio);
> request_irq(gpio_to_irq(gpio));
>
> Looks perfectly valid right?
>
> Not so: after the above change, we tried to request the
> GPIO a *second time* in the GPIO driver's irq map function,
> and of course it failed, as it was already taken.
>
> So saying that it should be done in the request_irq()
> function is imposing this other semantic on the kernel
> instead: you must *NOT* request the GPIO with
> request_gpio() if you're going to use it as an IRQ.
> 
> (Also, it force us to implement the same code in each
> and every driver, but that is a lesser problem.)
> 
> I don't quite understand what is so hard around this.
> We cannot get away from restricting the semantics in
> some way, if gpio-controllers shall also be interrupt-controllers,
> the current patch is the least intrusive I've seen so far.
> 

We have been trying to solve this issue for a few months by now and Linus'
approach seems to be the most sensible solution to me.

Drivers that request an IRQ and assume that platform code will request and setup
the GPIO have been broken since the boards using these drivers were migrated to
DT (e.g: smsc911x on OMAP2+ boards). I know is really hard to agree on the
better approach to solve this but it would be great if we can find a solution
and fix these broken platforms.

> And I don't even think this is a Linux problem, handing
> out the same resource with two hands is ambigous and is
> going to cause problems with any operating system.
> It is better to restrict this and say in the binding doc that
> the gpio line cannot be <&gpio n> assigned when there
> is an interrupt on the same line.
>

Indeed, if a driver wants to manually request a GPIO to be used as an IRQ, then
the DT binding for that driver should not use a gpio as "interrupt-parent" and
should leave the driver to handle this manually.

On the other hand, if a driver just calls request_irq() and it does not know if
the IRQ is a real interrupt line from an interrupt controller or a GPIO line
acting as an IRQ, then the DT binding should just have a required
"interrupt-parent" property to define the phandle for the interrupt controller
used which could or could not be a GPIO controller.

> We can start out by printing warnings when we fail to
> request the corresponding GPIO for an interrupt line
> though, it will be annoying enough and a kind of
> compromise.
>

If the GPIO pin is first requested because it is described to be an IRQ in a DT
and then a driver tries to request the same GPIO pin, then there is a bug in
either the DT or the driver IMHO. The same assumption is made with platform code
right now when using legacy boot, I don't understand why is different for the DT
case.

> Or it has to be the GPIO input hogs.
> 
> Yours,
> Linus Walleij
> 

Thanks a lot and best regards,
Javier

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/29/2013 01:26 PM, Linus Walleij wrote:
> On Tue, Aug 27, 2013 at 10:17 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/26/2013 08:07 AM, Lars Poeschel wrote:
>>>
>>> Currently the kernel is ambigously treating GPIOs and interrupts
>>> from a GPIO controller: GPIOs and interrupts are treated as
>>> orthogonal. This unfortunately makes it unclear how to actually
>>> retrieve and request a GPIO line or interrupt from a GPIO
>>> controller in the device tree probe path.
>>
>> I still think that this patch is the wrong approach. Instead, the logic
>> should be hooked into gpio_request() and request_irq(). This patch only
>> addresses DT, and ignores anything else, hence doesn't seem like the
>> right level of abstraction to plug in, since the issue is not related to DT.
> 
> We tried to do it that way, and it exploded. See commit
> b4419e1a15905191661ffe75ba2f9e649f5d565e
> "gpio/omap: auto request GPIO as input if used as IRQ via DT"
> 
> Here request_irq() augmented through its irqdomain to
> issue gpio_request_one().
> 
> Why was this patch reverted? It seems this is what has not
> managed to reach the audience here.
> 
> It turns out some drivers were already doing this:
> 
> request_gpio(gpio);
> gpio_direction_input(gpio);
> request_irq(gpio_to_irq(gpio));
> 
> Looks perfectly valid right?
> 
> Not so: after the above change, we tried to request the
> GPIO a *second time* in the GPIO driver's irq map function,
> and of course it failed, as it was already taken.
> 
> So saying that it should be done in the request_irq()
> function is imposing this other semantic on the kernel
> instead: you must *NOT* request the GPIO with
> request_gpio() if you're going to use it as an IRQ.

Surely both request_gpio() and request_irq() must both request the GPIO
(amongst other things), with the caveat that if the same driver does
both, this specific "sharing" is allowed.

If that won't work, then the very core concept behind what this patch is
attempting to do won't work.

> (Also, it force us to implement the same code in each
> and every driver, but that is a lesser problem.)

Drivers don't have to do the request; the IRQ/GPIO core can do this,
with drivers simply providing an irq_to_gpio() (which only returns valid
data iff there's a 1:1 mapping between IRQs and GPIOs in that particular
HW).

> I don't quite understand what is so hard around this.
> We cannot get away from restricting the semantics in
> some way, if gpio-controllers shall also be interrupt-controllers,
> the current patch is the least intrusive I've seen so far.

Yet the current patch only addresses a limited set of cases, since it
doesn't hook the APIs but rather parses the DT. It doesn't cover the
non-DT case. It should if the feature is useful.

> And I don't even think this is a Linux problem, handing
> out the same resource with two hands is ambigous and is
> going to cause problems with any operating system.
> It is better to restrict this and say in the binding doc that
> the gpio line cannot be <&gpio n> assigned when there
> is an interrupt on the same line.

That won't work; there is already HW that allows this and drivers that
assume this.

I still haven't seen an answer to why we really care about this; how
many times has code actually allocated the same GPIO/IRQ when it
shouldn't, in a way that it wasn't detectable by some other mechanism,
i.e. the feature just not working? Why are we even trying to solve this
issue? I'm not totally convinced it even makes sense to try and solve it.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
...
> We have been trying to solve this issue for a few months by now and Linus'
> approach seems to be the most sensible solution to me.
> 
> Drivers that request an IRQ and assume that platform code will request and setup
> the GPIO have been broken since the boards using these drivers were migrated to
> DT (e.g: smsc911x on OMAP2+ boards).

That's only true if the driver for the GPIO controller is buggy.
Whatever request_irq() maps down to in the GPIO/IRQ controller driver
simply needs to set up the pin as an interrupt input, then it doesn't
matter which order the driver does things.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

Am Freitag, 30. August 2013, 13:55:26 schrieb Stephen Warren:
> On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
> ...
> 
> > We have been trying to solve this issue for a few months by now and Linus'
> > approach seems to be the most sensible solution to me.
> > 
> > Drivers that request an IRQ and assume that platform code will request and
> > setup the GPIO have been broken since the boards using these drivers were
> > migrated to DT (e.g: smsc911x on OMAP2+ boards).
> 
> That's only true if the driver for the GPIO controller is buggy.
> Whatever request_irq() maps down to in the GPIO/IRQ controller driver
> simply needs to set up the pin as an interrupt input, then it doesn't
> matter which order the driver does things.

Is it really that easy?
request_irq() should request the gpio and set it to input that it needs to 
fulfill the irq request. That would then be the way to go for new drivers and 
in the DT case.
Some leagcy drivers currently do this:

request_gpio(gpio);
gpio_direction_input(gpio);
request_irq(gpio_to_irq(gpio));

In that case request_irq should not fail because the driver is already the 
correct owner of this gpio. But if some other entity owns this gpio it should 
fail.

Also if I understand you correct the other way round should also possible:

request_irq(gpio_to_irq(gpio));
request_gpio(gpio);
gpio_direction_input(gpio);

request_irq() also requests the gpio then but the following request_gpio() 
should also not fail.

To have it work that way we have to track the owners of all requested gpios 
somewhere. Or am I wrong here?
Where and how to record these owners? Can gpio system achieve this? gpios are 
requested without an owning device.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

Am Freitag, 30. August 2013, 13:53:45 schrieb Stephen Warren:
> On 08/29/2013 01:26 PM, Linus Walleij wrote:
> > On Tue, Aug 27, 2013 at 10:17 PM, Stephen Warren <swarren@wwwdotorg.org> 
wrote:
> >> On 08/26/2013 08:07 AM, Lars Poeschel wrote:
> >>> Currently the kernel is ambigously treating GPIOs and interrupts
> >>> from a GPIO controller: GPIOs and interrupts are treated as
> >>> orthogonal. This unfortunately makes it unclear how to actually
> >>> retrieve and request a GPIO line or interrupt from a GPIO
> >>> controller in the device tree probe path.
> >> 
> >> I still think that this patch is the wrong approach. Instead, the logic
> >> should be hooked into gpio_request() and request_irq(). This patch only
> >> addresses DT, and ignores anything else, hence doesn't seem like the
> >> right level of abstraction to plug in, since the issue is not related to
> >> DT.> 
> > We tried to do it that way, and it exploded. See commit
> > b4419e1a15905191661ffe75ba2f9e649f5d565e
> > "gpio/omap: auto request GPIO as input if used as IRQ via DT"
> > 
> > Here request_irq() augmented through its irqdomain to
> > issue gpio_request_one().
> > 
> > Why was this patch reverted? It seems this is what has not
> > managed to reach the audience here.
> > 
> > It turns out some drivers were already doing this:
> > 
> > request_gpio(gpio);
> > gpio_direction_input(gpio);
> > request_irq(gpio_to_irq(gpio));
> > 
> > Looks perfectly valid right?
> > 
> > Not so: after the above change, we tried to request the
> > GPIO a *second time* in the GPIO driver's irq map function,
> > and of course it failed, as it was already taken.
> > 
> > So saying that it should be done in the request_irq()
> > function is imposing this other semantic on the kernel
> > instead: you must *NOT* request the GPIO with
> > request_gpio() if you're going to use it as an IRQ.
> 
> Surely both request_gpio() and request_irq() must both request the GPIO
> (amongst other things), with the caveat that if the same driver does
> both, this specific "sharing" is allowed.

As explained in my previous mail, I do not see, how this should work.

> If that won't work, then the very core concept behind what this patch is
> attempting to do won't work.

The concept only does not work in the non-DT case, which we do neither try to 
address with this patch.

> > (Also, it force us to implement the same code in each
> > and every driver, but that is a lesser problem.)
> 
> Drivers don't have to do the request; the IRQ/GPIO core can do this,
> with drivers simply providing an irq_to_gpio() (which only returns valid
> data iff there's a 1:1 mapping between IRQs and GPIOs in that particular
> HW).
> 
> > I don't quite understand what is so hard around this.
> > We cannot get away from restricting the semantics in
> > some way, if gpio-controllers shall also be interrupt-controllers,
> > the current patch is the least intrusive I've seen so far.
> 
> Yet the current patch only addresses a limited set of cases, since it
> doesn't hook the APIs but rather parses the DT. It doesn't cover the
> non-DT case. It should if the feature is useful.

As pointed out before, only DT has this problem, that, whatever you do, 
otherwise it has no chance to request a gpio for a gpio-backed irq. Drivers 
can do this and board files also can do this.
I agree with you that it is somewhat odd, that there are two different ways, 
doing the same thing, but I don't see another way yet.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Fri, Aug 30, 2013 at 9:53 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> I still haven't seen an answer to why we really care about this; how
> many times has code actually allocated the same GPIO/IRQ when it
> shouldn't, in a way that it wasn't detectable by some other mechanism,
> i.e. the feature just not working? Why are we even trying to solve this
> issue? I'm not totally convinced it even makes sense to try and solve it.

We care about this because a number of OMAP boards are not
working properly when booted from device tree, and they have a hard
time figuring out a solution to the problem. Last try exploded. Now
they are looking to create a patch that will fix the actual problem.

Yours,
Linus Walleij

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Fri, Aug 30, 2013 at 9:55 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
> ...
>> We have been trying to solve this issue for a few months by now and Linus'
>> approach seems to be the most sensible solution to me.
>>
>> Drivers that request an IRQ and assume that platform code will request and setup
>> the GPIO have been broken since the boards using these drivers were migrated to
>> DT (e.g: smsc911x on OMAP2+ boards).
>
> That's only true if the driver for the GPIO controller is buggy.
> Whatever request_irq() maps down to in the GPIO/IRQ controller driver
> simply needs to set up the pin as an interrupt input, then it doesn't
> matter which order the driver does things.

As mentioned it can't do that, because doing that creates a
restriction on which order the driver does things...

But you mentioned that you wanted an API that would account
for the case where the *same driver* requested the same resource
(a GPIO line) to be used for both IRQ and GPIO, through two
different calls.

I would be happy to see how we could do that, preferably in a
generic way.

Since the gpio_request() does not contain the signature of the
calling driver I don't see how we could do this without refactoring
the whole world.

In that case it would probably be easiest to
*first* proceed to complete Alexandre's suggested refactorings for
GPIO descriptors, which tie down GPIOs to be requested like
clocks and regulators and thus tied to a device, so we can from
there proceed to implement such a conditional request,
as we will then have the required information in the GPIO
subsystem.

Yours,
Linus Walleij

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/31/2013 09:32 AM, anish singh wrote:
> On Sat, Aug 31, 2013 at 1:23 AM, Stephen Warren <swarren@wwwdotorg.org
> <mailto:swarren@wwwdotorg.org>> wrote:

>     I still haven't seen an answer to why we really care about this; how
>     many times has code actually allocated the same GPIO/IRQ when it
>     shouldn't, in a way that it wasn't detectable by some other mechanism,
>     i.e. the feature just not working? Why are we even trying to solve this
>     issue? I'm not totally convinced it even makes sense to try and
>     solve it.
> 
>  
> Probably this issue(same gpio/irq being used by multiple drivers) is
> very rare
> but debugging it is bit difficult.

Really? It's easy to just look in /proc/interrupts and
/sys/kernel/debug/gpio.

> This generally happens when we are working
> on latest revised boards where latest gpio number of some driver
> conflicts with the some other driver.

At least in the DT case, which is all that this patch solves, the DT is
describing the HW GPIO/IRQ numbers, so the Linux GPIO numbers are
irrelevant; everything is expressed as raw HW numbers, which are quite
easy to check.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/02/2013 03:25 AM, Lars Poeschel wrote:
> Am Freitag, 30. August 2013, 13:55:26 schrieb Stephen Warren:
>> On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
>> ...
>>
>>> We have been trying to solve this issue for a few months by now and Linus'
>>> approach seems to be the most sensible solution to me.
>>>
>>> Drivers that request an IRQ and assume that platform code will request and
>>> setup the GPIO have been broken since the boards using these drivers were
>>> migrated to DT (e.g: smsc911x on OMAP2+ boards).
>>
>> That's only true if the driver for the GPIO controller is buggy.
>> Whatever request_irq() maps down to in the GPIO/IRQ controller driver
>> simply needs to set up the pin as an interrupt input, then it doesn't
>> matter which order the driver does things.
> 
> Is it really that easy?

Yes.

> request_irq() should request the gpio and set it to input that it needs to 
> fulfill the irq request. That would then be the way to go for new drivers and 
> in the DT case.

Either explicitly request the GPIO, or simply directly program the HW in
whatever way is required for the pin to operate as an IRQ.

> Some leagcy drivers currently do this:
> 
> request_gpio(gpio);
> gpio_direction_input(gpio);
> request_irq(gpio_to_irq(gpio));
> 
> In that case request_irq should not fail because the driver is already the 
> correct owner of this gpio. But if some other entity owns this gpio it should 
> fail.

Yes.

> Also if I understand you correct the other way round should also possible:
> 
> request_irq(gpio_to_irq(gpio));
> request_gpio(gpio);
> gpio_direction_input(gpio);
> 
> request_irq() also requests the gpio then but the following request_gpio() 
> should also not fail.

I don't believe that code sequence is currently banned. If we want to
ban it, we should document that. Until this is documented as being
banned, I think we must fully support that code sequence.

> To have it work that way we have to track the owners of all requested gpios 
> somewhere. Or am I wrong here?
> Where and how to record these owners? Can gpio system achieve this? gpios are 
> requested without an owning device.

Yes. But, I believe we need to fully track every GPIO/IRQ owner already
right now; if a driver (not the GPIO/IRQ chip driver, but a driver that
uses the GPIOs/IRQs) calls gpio_request()/request_irq(), and this patch
has already requested them, then we already need to fully track GPIO/IRQ
ownership to make sure that the driver's own requests aren't failed
because the DT/GPIO core has already requested them on its behalf.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/02/2013 03:38 AM, Lars Poeschel wrote:
> Am Freitag, 30. August 2013, 13:53:45 schrieb Stephen Warren:
...
>> Yet the current patch only addresses a limited set of cases, since it
>> doesn't hook the APIs but rather parses the DT. It doesn't cover the
>> non-DT case. It should if the feature is useful.
> 
> As pointed out before, only DT has this problem, that,

That's completely false. Both DT and non-DT can represent the exact same
HW, and use the exact same drivers. It's equally possible to write a bug
in a board file or a DT file (i.e. a typo or incorrect reading of the
schematic) that causes the wrong GPIO or IRQ ID to be used, and hence
for there to be conflicts. Any solution to this issue needs to address
both cases.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/03/2013 06:35 AM, Linus Walleij wrote:
> On Fri, Aug 30, 2013 at 9:53 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> I still haven't seen an answer to why we really care about this; how
>> many times has code actually allocated the same GPIO/IRQ when it
>> shouldn't, in a way that it wasn't detectable by some other mechanism,
>> i.e. the feature just not working? Why are we even trying to solve this
>> issue? I'm not totally convinced it even makes sense to try and solve it.
> 
> We care about this because a number of OMAP boards are not
> working properly when booted from device tree, and they have a hard
> time figuring out a solution to the problem. Last try exploded. Now
> they are looking to create a patch that will fix the actual problem.

Is something missing from /proc/interrupts or /sys/kernel/debug/gpios?
If so, let's just add it.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/03/2013 06:43 AM, Linus Walleij wrote:
> On Fri, Aug 30, 2013 at 9:55 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/29/2013 06:24 PM, Javier Martinez Canillas wrote:
>> ...
>>> We have been trying to solve this issue for a few months by now and Linus'
>>> approach seems to be the most sensible solution to me.
>>>
>>> Drivers that request an IRQ and assume that platform code will request and setup
>>> the GPIO have been broken since the boards using these drivers were migrated to
>>> DT (e.g: smsc911x on OMAP2+ boards).
>>
>> That's only true if the driver for the GPIO controller is buggy.
>> Whatever request_irq() maps down to in the GPIO/IRQ controller driver
>> simply needs to set up the pin as an interrupt input, then it doesn't
>> matter which order the driver does things.
> 
> As mentioned it can't do that, because doing that creates a
> restriction on which order the driver does things...

I am not convinced here. Which driver (GPIO/IRQ controller driver, or
the driver which uses GPIOs/IRQs?) Which operations?

> But you mentioned that you wanted an API that would account
> for the case where the *same driver* requested the same resource
> (a GPIO line) to be used for both IRQ and GPIO, through two
> different calls.
> 
> I would be happy to see how we could do that, preferably in a
> generic way.
> 
> Since the gpio_request() does not contain the signature of the
> calling driver I don't see how we could do this without refactoring
> the whole world.
> 
> In that case it would probably be easiest to
> *first* proceed to complete Alexandre's suggested refactorings for
> GPIO descriptors, which tie down GPIOs to be requested like
> clocks and regulators and thus tied to a device, so we can from
> there proceed to implement such a conditional request,
> as we will then have the required information in the GPIO
> subsystem.

Indeed, that does seem necessary if you want the GPIO/IRQ core to be
able to implement this feature.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Wednesday 04 September 2013 11:29:47, Stephen Warren wrote:
> On 09/03/2013 06:35 AM, Linus Walleij wrote:
> > On Fri, Aug 30, 2013 at 9:53 PM, Stephen Warren <swarren@wwwdotorg.org> 
wrote:
> >> I still haven't seen an answer to why we really care about this; how
> >> many times has code actually allocated the same GPIO/IRQ when it
> >> shouldn't, in a way that it wasn't detectable by some other mechanism,
> >> i.e. the feature just not working? Why are we even trying to solve this
> >> issue? I'm not totally convinced it even makes sense to try and solve it.
> > 
> > We care about this because a number of OMAP boards are not
> > working properly when booted from device tree, and they have a hard
> > time figuring out a solution to the problem. Last try exploded. Now
> > they are looking to create a patch that will fix the actual problem.
> 
> Is something missing from /proc/interrupts or /sys/kernel/debug/gpios?
> If so, let's just add it.

That would require procfs to be mounted to /proc and even CONFIG_PROC_FS to be 
compiled in. Drivers have to work without that requirement.
That would require CONFIG_DEBUG_FS to be set, right? Drivers have to work 
without that.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Wednesday 04 September 2013 11:27:12, Stephen Warren wrote:
> On 09/02/2013 03:25 AM, Lars Poeschel wrote:
> > Some leagcy drivers currently do this:
> > 
> > request_gpio(gpio);
> > gpio_direction_input(gpio);
> > request_irq(gpio_to_irq(gpio));
> > 
> > In that case request_irq should not fail because the driver is already the
> > correct owner of this gpio. But if some other entity owns this gpio it
> > should fail.
> 
> Yes.
> 
> > Also if I understand you correct the other way round should also possible:
> > 
> > request_irq(gpio_to_irq(gpio));
> > request_gpio(gpio);
> > gpio_direction_input(gpio);
> > 
> > request_irq() also requests the gpio then but the following request_gpio()
> > should also not fail.
> 
> I don't believe that code sequence is currently banned. If we want to
> ban it, we should document that. Until this is documented as being
> banned, I think we must fully support that code sequence.
> 
> > To have it work that way we have to track the owners of all requested
> > gpios
> > somewhere. Or am I wrong here?
> > Where and how to record these owners? Can gpio system achieve this? gpios
> > are requested without an owning device.
> 
> Yes. But, I believe we need to fully track every GPIO/IRQ owner already
> right now; if a driver (not the GPIO/IRQ chip driver, but a driver that
> uses the GPIOs/IRQs) calls gpio_request()/request_irq(), and this patch
> has already requested them, then we already need to fully track GPIO/IRQ
> ownership to make sure that the driver's own requests aren't failed
> because the DT/GPIO core has already requested them on its behalf.

The driver that tries to use the GPIO requested by this patch before HAS to 
fail. This is exactly the intention of this patch. We don't want the GPIO to 
be requested any more, if it is used as an interrupt pin.
To be able to implement the way you proposed some mails before, I have to be 
able to do some checks that to my opinion (and if I understand Linus 
correctly) aren't possible at the moment. So I asked, where and how to record 
these owners of the GPIOs? And can the GPIO system achieve this and how? Can 
you please shed some light on this! Thanks!

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Wednesday 04 September 2013 11:29:00, Stephen Warren wrote:
> On 09/02/2013 03:38 AM, Lars Poeschel wrote:
> > Am Freitag, 30. August 2013, 13:53:45 schrieb Stephen Warren:
> ...
> 
> >> Yet the current patch only addresses a limited set of cases, since it
> >> doesn't hook the APIs but rather parses the DT. It doesn't cover the
> >> non-DT case. It should if the feature is useful.
> > 
> > As pointed out before, only DT has this problem, that,
> 
> That's completely false. Both DT and non-DT can represent the exact same
> HW, and use the exact same drivers. It's equally possible to write a bug
> in a board file or a DT file (i.e. a typo or incorrect reading of the
> schematic) that causes the wrong GPIO or IRQ ID to be used, and hence
> for there to be conflicts. Any solution to this issue needs to address
> both cases.

This is again not the point. This is not what this patch is trying to solve. 
The patch is trying to solve problem A. But you are talking about problem B.
Sure I can write a bug in board files and I can write a bug in DT files. The 
patch is not trying to prevent that. This is a completly different thing.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/04/2013 02:35 AM, Lars Poeschel wrote:
> On Wednesday 04 September 2013 11:29:47, Stephen Warren wrote:
>> On 09/03/2013 06:35 AM, Linus Walleij wrote:
>>> On Fri, Aug 30, 2013 at 9:53 PM, Stephen Warren <swarren@wwwdotorg.org> 
> wrote:
>>>> I still haven't seen an answer to why we really care about this; how
>>>> many times has code actually allocated the same GPIO/IRQ when it
>>>> shouldn't, in a way that it wasn't detectable by some other mechanism,
>>>> i.e. the feature just not working? Why are we even trying to solve this
>>>> issue? I'm not totally convinced it even makes sense to try and solve it.
>>>
>>> We care about this because a number of OMAP boards are not
>>> working properly when booted from device tree, and they have a hard
>>> time figuring out a solution to the problem. Last try exploded. Now
>>> they are looking to create a patch that will fix the actual problem.
>>
>> Is something missing from /proc/interrupts or /sys/kernel/debug/gpios?
>> If so, let's just add it.
> 
> That would require procfs to be mounted to /proc and even CONFIG_PROC_FS to be 
> compiled in. Drivers have to work without that requirement.
> That would require CONFIG_DEBUG_FS to be set, right? Drivers have to work 
> without that.

Drivers will/could work fine without those options enabled and
filesystems mounted. Nothing in this patch series affects that, nor
would not applying the patch series.

However, you only need this patch series if there's some kind of
problematic resource conflict and you want to debug it. If your system
has a problem and you want to debug it, it seems entirely reasonable to
make use of (require making use of) the debugging tools that are already
part of the kernel.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/04/2013 03:05 AM, Lars Poeschel wrote:
> On Wednesday 04 September 2013 11:27:12, Stephen Warren wrote:
>> On 09/02/2013 03:25 AM, Lars Poeschel wrote:
>>> Some leagcy drivers currently do this:
>>>
>>> request_gpio(gpio);
>>> gpio_direction_input(gpio);
>>> request_irq(gpio_to_irq(gpio));
>>>
>>> In that case request_irq should not fail because the driver is already the
>>> correct owner of this gpio. But if some other entity owns this gpio it
>>> should fail.
>>
>> Yes.
>>
>>> Also if I understand you correct the other way round should also possible:
>>>
>>> request_irq(gpio_to_irq(gpio));
>>> request_gpio(gpio);
>>> gpio_direction_input(gpio);
>>>
>>> request_irq() also requests the gpio then but the following request_gpio()
>>> should also not fail.
>>
>> I don't believe that code sequence is currently banned. If we want to
>> ban it, we should document that. Until this is documented as being
>> banned, I think we must fully support that code sequence.
>>
>>> To have it work that way we have to track the owners of all requested
>>> gpios
>>> somewhere. Or am I wrong here?
>>> Where and how to record these owners? Can gpio system achieve this? gpios
>>> are requested without an owning device.
>>
>> Yes. But, I believe we need to fully track every GPIO/IRQ owner already
>> right now; if a driver (not the GPIO/IRQ chip driver, but a driver that
>> uses the GPIOs/IRQs) calls gpio_request()/request_irq(), and this patch
>> has already requested them, then we already need to fully track GPIO/IRQ
>> ownership to make sure that the driver's own requests aren't failed
>> because the DT/GPIO core has already requested them on its behalf.
> 
> The driver that tries to use the GPIO requested by this patch before HAS to 
> fail. This is exactly the intention of this patch. We don't want the GPIO to 
> be requested any more, if it is used as an interrupt pin.

That will break existing drivers. There are drivers that request the
same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
that way.

> To be able to implement the way you proposed some mails before, I have to be 
> able to do some checks that to my opinion (and if I understand Linus 
> correctly) aren't possible at the moment.

True.

> So I asked, where and how to record 
> these owners of the GPIOs? And can the GPIO system achieve this and how? Can 
> you please shed some light on this! Thanks!

Alex Courbot's (proposed) new gpiod API has a "dev" parameter to
gpiod_request() which could be used for this purpose. Something similar
could be done for interrupts.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/04/2013 03:21 AM, Lars Poeschel wrote:
> On Wednesday 04 September 2013 11:29:00, Stephen Warren wrote:
>> On 09/02/2013 03:38 AM, Lars Poeschel wrote:
>>> Am Freitag, 30. August 2013, 13:53:45 schrieb Stephen Warren:
>> ...
>>
>>>> Yet the current patch only addresses a limited set of cases, since it
>>>> doesn't hook the APIs but rather parses the DT. It doesn't cover the
>>>> non-DT case. It should if the feature is useful.
>>>
>>> As pointed out before, only DT has this problem, that,
>>
>> That's completely false. Both DT and non-DT can represent the exact same
>> HW, and use the exact same drivers. It's equally possible to write a bug
>> in a board file or a DT file (i.e. a typo or incorrect reading of the
>> schematic) that causes the wrong GPIO or IRQ ID to be used, and hence
>> for there to be conflicts. Any solution to this issue needs to address
>> both cases.
> 
> This is again not the point. This is not what this patch is trying to solve. 
> The patch is trying to solve problem A. But you are talking about problem B.
> Sure I can write a bug in board files and I can write a bug in DT files. The 
> patch is not trying to prevent that. This is a completly different thing.

I'm not trying to assert what this current patch was written to solve. I
am asserting what the patch should be (have been) written to solve.

My point is that we shouldn't only solve the problem in case B, but
rather solve the problem in all cases (A, B, and anything else). Doing
anything else isn't useful in my opinion; it's too special-cased.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Mark Brown]

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

On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
> On 09/04/2013 03:05 AM, Lars Poeschel wrote:

> > The driver that tries to use the GPIO requested by this patch before HAS to 
> > fail. This is exactly the intention of this patch. We don't want the GPIO to 
> > be requested any more, if it is used as an interrupt pin.

> That will break existing drivers. There are drivers that request the
> same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
> that way.

Yes, plus input devices and audio jack detection among others.  This
pattern is very common if the GPIO is actually being used as a GPIO, an
edge triggered interrupt is used to flag when something happens and the
state is determined by reading the GPIO state (often with some
debounce).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Tuesday 10 September 2013 17:19:24, Mark Brown wrote:
> On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
> > On 09/04/2013 03:05 AM, Lars Poeschel wrote:
> > > The driver that tries to use the GPIO requested by this patch before HAS
> > > to
> > > fail. This is exactly the intention of this patch. We don't want the
> > > GPIO to be requested any more, if it is used as an interrupt pin.
> > 
> > That will break existing drivers. There are drivers that request the
> > same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
> > that way.
> 
> Yes, plus input devices and audio jack detection among others.  This
> pattern is very common if the GPIO is actually being used as a GPIO, an
> edge triggered interrupt is used to flag when something happens and the
> state is determined by reading the GPIO state (often with some
> debounce).

And I say it again for those coming into the discussion late, like it has been 
said many many times before: This patch does not break any of this drivers. 
They simply request their GPIO from DT and turn it into an irq using 
gpio_to_irq, request that irq on their behalf and use it as GPIO and IRQ in 
parallel. At least this is not a problem.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/10/2013 10:47 AM, Lars Poeschel wrote:
> On Tuesday 10 September 2013 17:19:24, Mark Brown wrote:
>> On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
>> > On 09/04/2013 03:05 AM, Lars Poeschel wrote:
>> > > The driver that tries to use the GPIO requested by this patch before HAS
>> > > to
>> > > fail. This is exactly the intention of this patch. We don't want the
>> > > GPIO to be requested any more, if it is used as an interrupt pin.
>> > 
>> > That will break existing drivers. There are drivers that request the
>> > same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
>> > that way.
>> 
>> Yes, plus input devices and audio jack detection among others.  This
>> pattern is very common if the GPIO is actually being used as a GPIO, an
>> edge triggered interrupt is used to flag when something happens and the
>> state is determined by reading the GPIO state (often with some
>> debounce).
> 
> And I say it again for those coming into the discussion late, like it has been 
> said many many times before: This patch does not break any of this drivers. 
> They simply request their GPIO from DT and turn it into an irq using 
> gpio_to_irq, request that irq on their behalf and use it as GPIO and IRQ in 
> parallel. At least this is not a problem.
> 

I agree with Lars and think that we should stop arguing about the limitations of
this patch and how this doesn't solve:


a) The fact that platform code has to call:

gpio_request()
gpio_direction_input()
request_irq()

b) That there are complex use cases where the same driver can request both a
GPIO and the mapped IRQ.

The only thing that this patch tries to solve is when a driver expect to request
a IRQ and it doesn't care if is a real IRQ line from an interrupt controller or
a GPIO pin mapped as an IRQ.

With board files we used to explicitly do the GPIO setup
(gpio_{request,direction_input}) but with DT these board files are gone and we
need a way to setup a GPIO implicitly when is mapped as an IRQ.

That's the only use case that this patch covers.

In legacy non-DT booting boards files will continue doing whatever are doing now
to ensure that drivers calling request_irq() will succeed and complex drivers
can just not define the GPIO-IRQ mapping in the DT and do whatever they need to
do manually.

Now, if just solving this use case is not enough and we want a more general
solution then we should start discussing how that solution should look like so
it can be implemented.

The most concrete idea so far is the one proposed by Stephen to pass a struct
device to gpiolib functions so GPIO controller drivers know when the same device
or a different one is requesting a GPIO twice to allow sharing a GPIO or not.

Also, it would be great if we can find a temporary solution so we can finally
have ethernet working with DT on most OMAP2+ boards. Since I expect that doing
the mentioned change on gpiolib would take at least a couple of kernel releases.

Thanks a lot and best regards,
Javier

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/10/2013 07:56 AM, Javier Martinez Canillas wrote:
...
> The only thing that this patch tries to solve is when a driver expect to request
> a IRQ and it doesn't care if is a real IRQ line from an interrupt controller or
> a GPIO pin mapped as an IRQ.

That can be solved in the interrupt chip driver. The fact the previous
attempt didn't work doesn't mean that it's impossible.

> With board files we used to explicitly do the GPIO setup
> (gpio_{request,direction_input}) but with DT these board files are gone and we
> need a way to setup a GPIO implicitly when is mapped as an IRQ.

Well, that's just an example of hacking around something in a board file
that should have been fixed in the GPIO/IRQ controller.

> That's the only use case that this patch covers.
...
> Also, it would be great if we can find a temporary solution so we can finally
> have ethernet working with DT on most OMAP2+ boards. Since I expect that doing
> the mentioned change on gpiolib would take at least a couple of kernel releases.

Really? I thought this patch was error-checking to make sure that
different drivers didn't request a GPIO and an IRQ that are actually the
same signal. This patch shouldn't affect any functionality except in
cases where there's a bug in the DT (incorrect GPIO/IRQ passed to some
driver).

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/10/2013 02:47 AM, Lars Poeschel wrote:
> On Tuesday 10 September 2013 17:19:24, Mark Brown wrote:
>> On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
>>> On 09/04/2013 03:05 AM, Lars Poeschel wrote:
>>>> The driver that tries to use the GPIO requested by this patch before HAS
>>>> to
>>>> fail. This is exactly the intention of this patch. We don't want the
>>>> GPIO to be requested any more, if it is used as an interrupt pin.
>>>
>>> That will break existing drivers. There are drivers that request the
>>> same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
>>> that way.
>>
>> Yes, plus input devices and audio jack detection among others.  This
>> pattern is very common if the GPIO is actually being used as a GPIO, an
>> edge triggered interrupt is used to flag when something happens and the
>> state is determined by reading the GPIO state (often with some
>> debounce).
> 
> And I say it again for those coming into the discussion late, like it has been 
> said many many times before: This patch does not break any of this drivers. 
> They simply request their GPIO from DT and turn it into an irq using 
> gpio_to_irq, request that irq on their behalf and use it as GPIO and IRQ in 
> parallel. At least this is not a problem.

Doesn't this patch call gpio_request() on the GPIO first, and hence
prevent the driver's own gpio_request() from succeeding, since the GPIO
is already requested? If this is not a problem, it sounds like a bug in
gpio_request() not ensuring mutual exclusion for the GPIO.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/10/2013 09:52 PM, Stephen Warren wrote:
> On 09/10/2013 07:56 AM, Javier Martinez Canillas wrote:
> ...
>> The only thing that this patch tries to solve is when a driver expect to request
>> a IRQ and it doesn't care if is a real IRQ line from an interrupt controller or
>> a GPIO pin mapped as an IRQ.
> 
> That can be solved in the interrupt chip driver. The fact the previous
> attempt didn't work doesn't mean that it's impossible.
>

Indeed. Unfortunately that patch-set got merged as a fix quite late on the -rc
cycle so it was safer to just revert the patches instead of analyzing the
regression and providing a fix.

If Linus is fond about taking a local fix for gpio-omap then we can try again
as a RFC with a better test coverage than before (although the patches had
several tested and acked-by but no one tested on OMAP1 until the patches were
merged) getting some TI folks in the loop who have access to those ancient OMAP1
devices. That way we can repost as a patch just once we are definitely sure that
it won't cause a regression on any OMAP platform.

>> With board files we used to explicitly do the GPIO setup
>> (gpio_{request,direction_input}) but with DT these board files are gone and we
>> need a way to setup a GPIO implicitly when is mapped as an IRQ.
> 
> Well, that's just an example of hacking around something in a board file
> that should have been fixed in the GPIO/IRQ controller.
> 
>> That's the only use case that this patch covers.
> ...
>> Also, it would be great if we can find a temporary solution so we can finally
>> have ethernet working with DT on most OMAP2+ boards. Since I expect that doing
>> the mentioned change on gpiolib would take at least a couple of kernel releases.
> 
> Really? I thought this patch was error-checking to make sure that
> different drivers didn't request a GPIO and an IRQ that are actually the
> same signal. This patch shouldn't affect any functionality except in
> cases where there's a bug in the DT (incorrect GPIO/IRQ passed to some
> driver).
> 

Yes, it doesn't do any error-checking neither prevent a driver to request a GPIO
and an IRQ that are the same signal (as long as this is supported by the
GPIO/IRQ controller and its driver). The only thing that Linus patch does is to
auto request a GPIO for pins that are mapped as IRQ in DT (i.e: interrupt-parent
= <&gpio6>).

The name of the function introduced by Linus is
of_gpiochip_interrupt_consistency_check() but probably a better name is
of_gpiochip_autorequest_irq() or something like that.

A similar behavior could be obtained by let's say calling gpio_request() in
irq_create_of_mapping() if you wanted to add the logic in the IRQ chip DT core
instead of doing it in the GPIO chip DT core as Linus does with his patch.

That's why I don't understand why Linus patch could be an issue for drivers that
needs to request both the GPIO and the mapped IRQ.

If this is already supported then nothing will break. If the driver tries to
request the GPIO twice because this is already made by the DT core then I think
is a bug in the driver and has to be fixed to support DT since there won't be
any need to do this manually anymore.

Best regards,
Javier

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Mark Brown]

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

On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:

> Doesn't this patch call gpio_request() on the GPIO first, and hence
> prevent the driver's own gpio_request() from succeeding, since the GPIO
> is already requested? If this is not a problem, it sounds like a bug in
> gpio_request() not ensuring mutual exclusion for the GPIO.

Or at the very least something that's likely to break in the future.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/10/2013 03:37 PM, Mark Brown wrote:
> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
> 
>> Doesn't this patch call gpio_request() on the GPIO first, and
>> hence prevent the driver's own gpio_request() from succeeding,
>> since the GPIO is already requested? If this is not a problem, it
>> sounds like a bug in gpio_request() not ensuring mutual exclusion
>> for the GPIO.
> 
> Or at the very least something that's likely to break in the
> future.

Looking at the GPIO code, it already prevents double-requests:

>         if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
>                 desc_set_label(desc, label ? : "?");
>                 status = 0;
>         } else {
>                 status = -EBUSY;
>                 module_put(chip->owner);
>                 goto done;
>         }

And I tested it in practice, and it really does fail.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/11/2013 12:34 AM, Stephen Warren wrote:
> On 09/10/2013 03:37 PM, Mark Brown wrote:
>> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
>> 
>>> Doesn't this patch call gpio_request() on the GPIO first, and
>>> hence prevent the driver's own gpio_request() from succeeding,
>>> since the GPIO is already requested? If this is not a problem, it
>>> sounds like a bug in gpio_request() not ensuring mutual exclusion
>>> for the GPIO.
>> 
>> Or at the very least something that's likely to break in the
>> future.
> 
> Looking at the GPIO code, it already prevents double-requests:
> 
>>         if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
>>                 desc_set_label(desc, label ? : "?");
>>                 status = 0;
>>         } else {
>>                 status = -EBUSY;
>>                 module_put(chip->owner);
>>                 goto done;
>>         }
> 
> And I tested it in practice, and it really does fail.
> 

I'm a bit confused now. Doesn't the fact that gpio_request() prevents
double-requests mean that the use-case that you say that have not been covered
by this patch can't actually happen?

I mean, if when using board files an explicit call to gpio_request() is made by
platform code then a driver can't call gpio_request() for the same gpio. So this
patch shouldn't cause any regression since is just auto-requesting a GPIO when
is mapped as an IRQ in a DT which basically will be the same that was made by
board files before.

To give you an example of an use-case that this patch is trying to solve:

OMAP SoCs have a General-Purpose Memory Controller (GPMC) that can be used to
interface with Pseudo-SRAM devices such as ethernet controllers. So with board
files we currently have this (arch/arm/mach-omap2/gpmc-smsc911x.c):

void __init gpmc_smsc911x_init(struct omap_smsc911x_platform_data *gpmc_cfg)
{
       ....
       if (gpio_request_one(gpmc_cfg->gpio_irq, GPIOF_IN, "smsc911x irq")) {
                pr_err("Failed to request IRQ GPIO%d\n", gpmc_cfg->gpio_irq);
                goto free1;
       }
       ....
       gpmc_smsc911x_resources[1].start = gpio_to_irq(gpmc_cfg->gpio_irq);
       ...
       pdev = platform_device_register_resndata(NULL, "smsc911x", gpmc_cfg->id,
                 gpmc_smsc911x_resources, ARRAY_SIZE(gpmc_smsc911x_resources),
                 &gpmc_smsc911x_config, sizeof(gpmc_smsc911x_config));
       ...
}

and later in the smsc911x ethernet driver probe function:

static int smsc911x_drv_probe(struct platform_device *pdev)
{
       retval = request_irq(dev->irq, smsc911x_irqhandler,
                             irq_flags | IRQF_SHARED, dev->name, dev);
       ...
       irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
       ...
       dev->irq = irq_res->start;
       ...
       retval = request_irq(dev->irq, smsc911x_irqhandler,
                             irq_flags | IRQF_SHARED, dev->name, dev);
       ...
}

The driver just knows that it has to get the IRQ from a struct resource and it
doesn't care if that is a real IRQ line from an interrupt controller or a GPIO
pin mapped as an IRQ. With linus patch I just can define on a DT (GPMC
properties omitted for simplicity):

        ethernet@5,0 {
                pinctrl-names = "default";
                pinctrl-0 = <&smsc911x_pins>;
                compatible = "smsc,lan9221", "smsc,lan9115";
                reg = <5 0 0xff>;
                bank-width = <2>;
                interrupt-parent = <&gpio6>;
                interrupts = <16 8>;
                vmmc-supply = <&vddvario>;
                vmmc_aux-supply = <&vdd33a>;
                reg-io-width = <4>;

                smsc,save-mac-address;
        };

and it will just work. Without Linus patch the call to request_irq() will fail
because a call to gpio_request() has not been made (and thus the GPIO bank was
not enabled).

Thanks a lot and best regards,
Javier

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Joel Fernandes]

On 09/10/2013 04:23 PM, Javier Martinez Canillas wrote:
> On 09/10/2013 09:52 PM, Stephen Warren wrote:
>> On 09/10/2013 07:56 AM, Javier Martinez Canillas wrote:
>> ...
>>> The only thing that this patch tries to solve is when a driver expect to request
>>> a IRQ and it doesn't care if is a real IRQ line from an interrupt controller or
>>> a GPIO pin mapped as an IRQ.
>>
>> That can be solved in the interrupt chip driver. The fact the previous
>> attempt didn't work doesn't mean that it's impossible.
>>
> 
> Indeed. Unfortunately that patch-set got merged as a fix quite late on the -rc
> cycle so it was safer to just revert the patches instead of analyzing the
> regression and providing a fix.
> 
> If Linus is fond about taking a local fix for gpio-omap then we can try again
> as a RFC with a better test coverage than before (although the patches had
> several tested and acked-by but no one tested on OMAP1 until the patches were
> merged) getting some TI folks in the loop who have access to those ancient OMAP1
> devices. That way we can repost as a patch just once we are definitely sure that
> it won't cause a regression on any OMAP platform.
> 
>>> With board files we used to explicitly do the GPIO setup
>>> (gpio_{request,direction_input}) but with DT these board files are gone and we
>>> need a way to setup a GPIO implicitly when is mapped as an IRQ.
>>
>> Well, that's just an example of hacking around something in a board file
>> that should have been fixed in the GPIO/IRQ controller.
>>
>>> That's the only use case that this patch covers.
>> ...
>>> Also, it would be great if we can find a temporary solution so we can finally
>>> have ethernet working with DT on most OMAP2+ boards. Since I expect that doing
>>> the mentioned change on gpiolib would take at least a couple of kernel releases.
>>
>> Really? I thought this patch was error-checking to make sure that
>> different drivers didn't request a GPIO and an IRQ that are actually the
>> same signal. This patch shouldn't affect any functionality except in
>> cases where there's a bug in the DT (incorrect GPIO/IRQ passed to some
>> driver).
>>
> 
> Yes, it doesn't do any error-checking neither prevent a driver to request a GPIO
> and an IRQ that are the same signal (as long as this is supported by the
> GPIO/IRQ controller and its driver). The only thing that Linus patch does is to
> auto request a GPIO for pins that are mapped as IRQ in DT (i.e: interrupt-parent
> = <&gpio6>).
> 
> The name of the function introduced by Linus is
> of_gpiochip_interrupt_consistency_check() but probably a better name is
> of_gpiochip_autorequest_irq() or something like that.
> 
> A similar behavior could be obtained by let's say calling gpio_request() in
> irq_create_of_mapping() if you wanted to add the logic in the IRQ chip DT core
> instead of doing it in the GPIO chip DT core as Linus does with his patch.
> 
> That's why I don't understand why Linus patch could be an issue for drivers that
> needs to request both the GPIO and the mapped IRQ.
> 
> If this is already supported then nothing will break. If the driver tries to
> request the GPIO twice because this is already made by the DT core then I think
> is a bug in the driver and has to be fixed to support DT since there won't be
> any need to do this manually anymore.

Quick question- Wouldn't this mean that there have to be 2 code paths in the
driver then.. One for DT-case where gpio_request double request is prevented,
and one for non-DT case where you would do the gpio_request followed by
request_irq. I'm not sure if there are drivers today that use DT and need to
converted to prevent the double request? If there are, and such drivers are also
used on non-DT platform, then we would have to fork 2 different code paths while
requesting an IRQ for DT/non-DT in the driver..

Also this path kind of enforces that the driver author must be aware whether
driver is being used on DT or non-DT platform.. Linus mentioned enforcing of
semantics, this is also enforcing semantics no?

Looks like the correct fix for this as discussed in this thread should be to
associate the struct device with the GPIO request, remember it and use the info
during request_irq. This is provided that the old pattern of gpio_request and
request_irq is continued to be used and working. I know this is some time away
so I'm not too opinionated about it.

Regards,

-Joel

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:
> On 09/11/2013 12:34 AM, Stephen Warren wrote:
>> On 09/10/2013 03:37 PM, Mark Brown wrote:
>>> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
>>>
>>>> Doesn't this patch call gpio_request() on the GPIO first, and
>>>> hence prevent the driver's own gpio_request() from succeeding,
>>>> since the GPIO is already requested? If this is not a problem, it
>>>> sounds like a bug in gpio_request() not ensuring mutual exclusion
>>>> for the GPIO.
>>>
>>> Or at the very least something that's likely to break in the
>>> future.
>>
>> Looking at the GPIO code, it already prevents double-requests:
>>
>>>         if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
>>>                 desc_set_label(desc, label ? : "?");
>>>                 status = 0;
>>>         } else {
>>>                 status = -EBUSY;
>>>                 module_put(chip->owner);
>>>                 goto done;
>>>         }
>>
>> And I tested it in practice, and it really does fail.
>>
> 
> I'm a bit confused now. Doesn't the fact that gpio_request() prevents
> double-requests mean that the use-case that you say that have not been covered
> by this patch can't actually happen?
> 
> I mean, if when using board files an explicit call to gpio_request() is made by
> platform code then a driver can't call gpio_request() for the same gpio. So this
> patch shouldn't cause any regression since is just auto-requesting a GPIO when
> is mapped as an IRQ in a DT which basically will be the same that was made by
> board files before.

I'm not familiar with the board file path; Linus describe this.

It sounds like that path is for the case where a driver /only/ cares
about using a pin as an IRQ, and hence the driver only calls
request_irq(). The board file is (earlier) calling gpio_request() in
order to set up that input pin to work correctly as an IRQ. Hence, there
is no double-call to gpio_request().

The case I said wouldn't work is:

* This patch calls gpio_request() in order to make the pin work as an IRQ.

* Driver uses the pin as both a GPIO and an IRQ, and hence calls
gpio_request() and request_irq().

So, there's a double-call to gpio_request(), which fails, and the driver
fails to probe.

I believe this situation is exactly what cause the original patch to the
OMAP driver to be reverted; that patch should have touched the HW
directly to solve the problem when the IRQ was requested, rather than
calling into the GPIO subsystem (which also has the side-effect of
touching the HW in the same way as desired).

> To give you an example of an use-case that this patch is trying to solve:
> 
> OMAP SoCs have a General-Purpose Memory Controller (GPMC) that can be used to
> interface with Pseudo-SRAM devices such as ethernet controllers. So with board
> files we currently have this (arch/arm/mach-omap2/gpmc-smsc911x.c):
> ...

As we discussed on IRC (so mainly for the record in the mailing list
archive), I believe that if a driver wants to use a pin as an interrupt
and only an interrupt, even if the pin has the capability in HW to be a
GPIO (or absolutely anything else at all), then the only call in the
entire kernel (board code, DT core code, IRQ core, driver, ...) should
be a single request_irq(), and the IRQ chip driver needs to program the
HW appropriately to make that work.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Monday 16 September 2013 13:43:50, Stephen Warren wrote:
> On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:
> > On 09/11/2013 12:34 AM, Stephen Warren wrote:
> >> On 09/10/2013 03:37 PM, Mark Brown wrote:
> >>> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
> >>>> Doesn't this patch call gpio_request() on the GPIO first, and
> >>>> hence prevent the driver's own gpio_request() from succeeding,
> >>>> since the GPIO is already requested? If this is not a problem, it
> >>>> sounds like a bug in gpio_request() not ensuring mutual exclusion
> >>>> for the GPIO.
> >>> 
> >>> Or at the very least something that's likely to break in the
> >>> future.
> >> 
> >> Looking at the GPIO code, it already prevents double-requests:
> >>>         if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> >>>         
> >>>                 desc_set_label(desc, label ? : "?");
> >>>                 status = 0;
> >>>         
> >>>         } else {
> >>>         
> >>>                 status = -EBUSY;
> >>>                 module_put(chip->owner);
> >>>                 goto done;
> >>>         
> >>>         }
> >> 
> >> And I tested it in practice, and it really does fail.
> > 
> > I'm a bit confused now. Doesn't the fact that gpio_request() prevents
> > double-requests mean that the use-case that you say that have not been
> > covered by this patch can't actually happen?
> > 
> > I mean, if when using board files an explicit call to gpio_request() is
> > made by platform code then a driver can't call gpio_request() for the
> > same gpio. So this patch shouldn't cause any regression since is just
> > auto-requesting a GPIO when is mapped as an IRQ in a DT which basically
> > will be the same that was made by board files before.
> 
> I'm not familiar with the board file path; Linus describe this.

It seems Linus is busy, I'll try to help out.

> It sounds like that path is for the case where a driver /only/ cares
> about using a pin as an IRQ, and hence the driver only calls
> request_irq(). The board file is (earlier) calling gpio_request() in
> order to set up that input pin to work correctly as an IRQ. Hence, there
> is no double-call to gpio_request().

No, a board file is not a path or something. A board file describes the wirings 
and specifics of an (embedded) computer in C code. The complete knowledge of 
how things are connected on a board and which drivers to use is in this piece 
of code. Devicetree replaces legacy board files. These two do pretty much the 
same, but board files have more power, because they are executed and can 
contain whatever code is needed to setup a board.
But you are right, the driver only calls request_irq(), the board file set up 
the pin before and told the driver which irq to use.
 
> The case I said wouldn't work is:
> 
> * This patch calls gpio_request() in order to make the pin work as an IRQ.
> 
> * Driver uses the pin as both a GPIO and an IRQ, and hence calls
> gpio_request() and request_irq().
> 
> So, there's a double-call to gpio_request(), which fails, and the driver
> fails to probe.

Again, no. In that case you don't define your pin as irq in the device tree, 
but only as gpio. The driver knows how to handle gpios and turn them into irqs 
so you have to present it a gpio not an irq. In that case the patch will not 
call gpio_request() and there is no double-call to gpio_request().
 
> I believe this situation is exactly what cause the original patch to the
> OMAP driver to be reverted; that patch should have touched the HW
> directly to solve the problem when the IRQ was requested, rather than
> calling into the GPIO subsystem (which also has the side-effect of
> touching the HW in the same way as desired).

The situation is different. You can try that out. Test your board/driver with 
this patch and test it with the original patch.

> > To give you an example of an use-case that this patch is trying to solve:
> > 
> > OMAP SoCs have a General-Purpose Memory Controller (GPMC) that can be used
> > to interface with Pseudo-SRAM devices such as ethernet controllers. So
> > with board files we currently have this
> > (arch/arm/mach-omap2/gpmc-smsc911x.c): ...
> 
> As we discussed on IRC (so mainly for the record in the mailing list
> archive), I believe that if a driver wants to use a pin as an interrupt
> and only an interrupt, even if the pin has the capability in HW to be a
> GPIO (or absolutely anything else at all), then the only call in the
> entire kernel (board code, DT core code, IRQ core, driver, ...) should
> be a single request_irq(), and the IRQ chip driver needs to program the
> HW appropriately to make that work.

I agree with you that it would be the best if the only call would be 
request_irq and the chip driver programs the HW appropriately. It would be a 
dream, but unfortunately this is not possible at the moment. This is something 
that Linus pointed out very very early in this whole discussion. The gpio and 
irq frameworks don't share any information. The irq framework has no chance to 
program the HW, because it will never find the related gpio.
For this to work the frameworks have to change (and possibly all drivers/board 
files/whatever using request_irq() and/or request_gpio()) have to change.
That is something that I do not dare to do alone.

Regards,
Lars

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/16/2013 10:03 AM, Lars Poeschel wrote:
> On Monday 16 September 2013 13:43:50, Stephen Warren wrote:
>> On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:
>>> On 09/11/2013 12:34 AM, Stephen Warren wrote:
>>>> On 09/10/2013 03:37 PM, Mark Brown wrote:
>>>>> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
>>>>>> Doesn't this patch call gpio_request() on the GPIO first, and
>>>>>> hence prevent the driver's own gpio_request() from succeeding,
>>>>>> since the GPIO is already requested? If this is not a problem, it
>>>>>> sounds like a bug in gpio_request() not ensuring mutual exclusion
>>>>>> for the GPIO.
>>>>>
>>>>> Or at the very least something that's likely to break in the
>>>>> future.
>>>>
>>>> Looking at the GPIO code, it already prevents double-requests:
>>>>>         if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
>>>>>         
>>>>>                 desc_set_label(desc, label ? : "?");
>>>>>                 status = 0;
>>>>>         
>>>>>         } else {
>>>>>         
>>>>>                 status = -EBUSY;
>>>>>                 module_put(chip->owner);
>>>>>                 goto done;
>>>>>         
>>>>>         }
>>>>
>>>> And I tested it in practice, and it really does fail.
>>>
>>> I'm a bit confused now. Doesn't the fact that gpio_request() prevents
>>> double-requests mean that the use-case that you say that have not been
>>> covered by this patch can't actually happen?
>>>
>>> I mean, if when using board files an explicit call to gpio_request() is
>>> made by platform code then a driver can't call gpio_request() for the
>>> same gpio. So this patch shouldn't cause any regression since is just
>>> auto-requesting a GPIO when is mapped as an IRQ in a DT which basically
>>> will be the same that was made by board files before.
>>
>> I'm not familiar with the board file path; Linus describe this.
> 
> It seems Linus is busy, I'll try to help out.
> 
>> It sounds like that path is for the case where a driver /only/ cares
>> about using a pin as an IRQ, and hence the driver only calls
>> request_irq(). The board file is (earlier) calling gpio_request() in
>> order to set up that input pin to work correctly as an IRQ. Hence, there
>> is no double-call to gpio_request().
> 
> No, a board file is not a path or something. A board file describes the wirings 
> and specifics of an (embedded) computer in C code. The complete knowledge of 
> how things are connected on a board and which drivers to use is in this piece 
> of code. Devicetree replaces legacy board files. These two do pretty much the 
> same, but board files have more power, because they are executed and can 
> contain whatever code is needed to setup a board.
> But you are right, the driver only calls request_irq(), the board file set up 
> the pin before and told the driver which irq to use.

path == code path, or execution path. I'm well aware of what board files
are in general.

I'm just not familiar with board files that employ this particular hack.

>> The case I said wouldn't work is:
>>
>> * This patch calls gpio_request() in order to make the pin work as an IRQ.
>>
>> * Driver uses the pin as both a GPIO and an IRQ, and hence calls
>> gpio_request() and request_irq().
>>
>> So, there's a double-call to gpio_request(), which fails, and the driver
>> fails to probe.
> 
> Again, no. In that case you don't define your pin as irq in the device tree, 
> but only as gpio. The driver knows how to handle gpios and turn them into irqs 
> so you have to present it a gpio not an irq. In that case the patch will not 
> call gpio_request() and there is no double-call to gpio_request().

That is a way to make this patch work, yes. However, there's no
guarantee that every driver or DT binding works this way. Forcing
bindings to work that way is forcing Linux-internal details upon
bindings, which should not be done. Put another way, I don't believe
there's any rule when writing DT bindings that states that bindings must
not describe the same pin as both a GPIO and an IRQ, although admittedly
that may be unusual.

...
> I agree with you that it would be the best if the only call would be 
> request_irq and the chip driver programs the HW appropriately. It would be a 
> dream, but unfortunately this is not possible at the moment. This is something 
> that Linus pointed out very very early in this whole discussion. The gpio and 
> irq frameworks don't share any information. The irq framework has no chance to 
> program the HW, because it will never find the related gpio.
> For this to work the frameworks have to change (and possibly all drivers/board 
> files/whatever using request_irq() and/or request_gpio()) have to change.
> That is something that I do not dare to do alone.

This is a controller-specific issue, and has nothing to do with the GPIO
or IRQ frameworks. The driver for the combined irq/gpio_chip simply
needs to program the HW when the IRQ is requested or set up. The Tegra
driver already works this way, so there's actual proof that it is
possible to do this in practice.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/16/2013 07:09 PM, Stephen Warren wrote:
> On 09/16/2013 10:03 AM, Lars Poeschel wrote:
>> On Monday 16 September 2013 13:43:50, Stephen Warren wrote:
>>> On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:
>>>> On 09/11/2013 12:34 AM, Stephen Warren wrote:
>>>>> On 09/10/2013 03:37 PM, Mark Brown wrote:
>>>>>> On Tue, Sep 10, 2013 at 01:53:47PM -0600, Stephen Warren wrote:
>>>>>>> Doesn't this patch call gpio_request() on the GPIO first, and
>>>>>>> hence prevent the driver's own gpio_request() from succeeding,
>>>>>>> since the GPIO is already requested? If this is not a problem, it
>>>>>>> sounds like a bug in gpio_request() not ensuring mutual exclusion
>>>>>>> for the GPIO.
>>>>>>
>>>>>> Or at the very least something that's likely to break in the
>>>>>> future.
>>>>>
>>>>> Looking at the GPIO code, it already prevents double-requests:
>>>>>>         if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
>>>>>>         
>>>>>>                 desc_set_label(desc, label ? : "?");
>>>>>>                 status = 0;
>>>>>>         
>>>>>>         } else {
>>>>>>         
>>>>>>                 status = -EBUSY;
>>>>>>                 module_put(chip->owner);
>>>>>>                 goto done;
>>>>>>         
>>>>>>         }
>>>>>
>>>>> And I tested it in practice, and it really does fail.
>>>>
>>>> I'm a bit confused now. Doesn't the fact that gpio_request() prevents
>>>> double-requests mean that the use-case that you say that have not been
>>>> covered by this patch can't actually happen?
>>>>
>>>> I mean, if when using board files an explicit call to gpio_request() is
>>>> made by platform code then a driver can't call gpio_request() for the
>>>> same gpio. So this patch shouldn't cause any regression since is just
>>>> auto-requesting a GPIO when is mapped as an IRQ in a DT which basically
>>>> will be the same that was made by board files before.
>>>
>>> I'm not familiar with the board file path; Linus describe this.
>> 
>> It seems Linus is busy, I'll try to help out.
>> 
>>> It sounds like that path is for the case where a driver /only/ cares
>>> about using a pin as an IRQ, and hence the driver only calls
>>> request_irq(). The board file is (earlier) calling gpio_request() in
>>> order to set up that input pin to work correctly as an IRQ. Hence, there
>>> is no double-call to gpio_request().
>> 
>> No, a board file is not a path or something. A board file describes the wirings 
>> and specifics of an (embedded) computer in C code. The complete knowledge of 
>> how things are connected on a board and which drivers to use is in this piece 
>> of code. Devicetree replaces legacy board files. These two do pretty much the 
>> same, but board files have more power, because they are executed and can 
>> contain whatever code is needed to setup a board.
>> But you are right, the driver only calls request_irq(), the board file set up 
>> the pin before and told the driver which irq to use.
> 
> path == code path, or execution path. I'm well aware of what board files
> are in general.
> 
> I'm just not familiar with board files that employ this particular hack.
> 
>>> The case I said wouldn't work is:
>>>
>>> * This patch calls gpio_request() in order to make the pin work as an IRQ.
>>>
>>> * Driver uses the pin as both a GPIO and an IRQ, and hence calls
>>> gpio_request() and request_irq().
>>>
>>> So, there's a double-call to gpio_request(), which fails, and the driver
>>> fails to probe.
>> 
>> Again, no. In that case you don't define your pin as irq in the device tree, 
>> but only as gpio. The driver knows how to handle gpios and turn them into irqs 
>> so you have to present it a gpio not an irq. In that case the patch will not 
>> call gpio_request() and there is no double-call to gpio_request().
> 
> That is a way to make this patch work, yes. However, there's no
> guarantee that every driver or DT binding works this way. Forcing
> bindings to work that way is forcing Linux-internal details upon
> bindings, which should not be done. Put another way, I don't believe
> there's any rule when writing DT bindings that states that bindings must
> not describe the same pin as both a GPIO and an IRQ, although admittedly
> that may be unusual.
> 
> ...
>> I agree with you that it would be the best if the only call would be 
>> request_irq and the chip driver programs the HW appropriately. It would be a 
>> dream, but unfortunately this is not possible at the moment. This is something 
>> that Linus pointed out very very early in this whole discussion. The gpio and 
>> irq frameworks don't share any information. The irq framework has no chance to 
>> program the HW, because it will never find the related gpio.
>> For this to work the frameworks have to change (and possibly all drivers/board 
>> files/whatever using request_irq() and/or request_gpio()) have to change.
>> That is something that I do not dare to do alone.
> 
> This is a controller-specific issue, and has nothing to do with the GPIO
> or IRQ frameworks. The driver for the combined irq/gpio_chip simply
> needs to program the HW when the IRQ is requested or set up. The Tegra
> driver already works this way, so there's actual proof that it is
> possible to do this in practice.
> 

Hi Stephen,

I finally had some time to look at this and tried what you suggested, that is
programming the hardware directly to do the setup when a IRQ is requested.

I tested booting my OMAP3 board with DT and legacy booting and it both cases it
works as expected.

I sent a RFC patch "[RFC] gpio/omap: auto-setup a GPIO when used as an IRQ" [1].

It would be great if I can get some feedback from you to see if that is what you
meant.

Thanks a lot and best regards,
Javier

[1]: https://lkml.org/lkml/2013/9/22/78

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Wed, Sep 11, 2013 at 9:43 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/10/2013 06:52 PM, Javier Martinez Canillas wrote:

>> I'm a bit confused now. Doesn't the fact that gpio_request() prevents
>> double-requests mean that the use-case that you say that have not been covered
>> by this patch can't actually happen?
>>
>> I mean, if when using board files an explicit call to gpio_request() is made by
>> platform code then a driver can't call gpio_request() for the same gpio. So this
>> patch shouldn't cause any regression since is just auto-requesting a GPIO when
>> is mapped as an IRQ in a DT which basically will be the same that was made by
>> board files before.
>
> I'm not familiar with the board file path; Linus describe this.

Oh um? Not following, that stuff is right above, what is unclear
about this that I need to describe?

Yours,
Linus Walleij

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Wed, Sep 11, 2013 at 9:43 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> I believe this situation is exactly what cause the original patch to the
> OMAP driver to be reverted; that patch should have touched the HW
> directly to solve the problem when the IRQ was requested, rather than
> calling into the GPIO subsystem (which also has the side-effect of
> touching the HW in the same way as desired).

And that has the side-effect that this line, which is now set up
in the HW as an input line, can be gpio_request()ed by some other
code path and set up as output, screwing around with the very
same registers.

I think the kernel should prevent such things.

But I think you're making it sound like the kernel should not prevent
such things, but instead give the drivers enough rope to shoot itself
in the foot if it so desires.

But a third solution is possible: the driver keep track of whether
a line is already requested as IRQ and if that happens, will not
allow gpio_set_direction_output() and other incompatible uses,
but still allow gpio_set_direction_input() and gpio_get_value(),
that would be OK.

In the GPIO descriptor we can flag this, so it would be preferable
to have that tracking in the gpiolib core rather than the driver
though.

> As we discussed on IRC (so mainly for the record in the mailing list
> archive), I believe that if a driver wants to use a pin as an interrupt
> and only an interrupt, even if the pin has the capability in HW to be a
> GPIO (or absolutely anything else at all), then the only call in the
> entire kernel (board code, DT core code, IRQ core, driver, ...) should
> be a single request_irq(), and the IRQ chip driver needs to program the
> HW appropriately to make that work.

I agree with this, but also think that the kernel should prevent the
same resource for being used in an incompatible way.

Yours,
Linus Walleij

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Mon, Sep 16, 2013 at 7:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> Put another way, I don't believe
> there's any rule when writing DT bindings that states that bindings must
> not describe the same pin as both a GPIO and an IRQ, although admittedly
> that may be unusual.

Actually I think you've won me over here.

But I do not think that this shall be done at the expense of letting
DT-based drivers do nasty things like setting up the same GPIO
line as an IRQ (hammering the hardware to do that) and then
request the same line to be an output line and drive it, for
example.

So the state of the GPIO line has to be tracked: if it is set as
input and tied to an IRQ it should *not* be possible for the
other code path to request it and set it as output. But it should
be possible to set it as input again and read the value.

Laurent deascribed exactly the latter usecase, which is OK.

>> I agree with you that it would be the best if the only call would be
>> request_irq and the chip driver programs the HW appropriately. It would be a
>> dream, but unfortunately this is not possible at the moment. This is something
>> that Linus pointed out very very early in this whole discussion. The gpio and
>> irq frameworks don't share any information. The irq framework has no chance to
>> program the HW, because it will never find the related gpio.
>> For this to work the frameworks have to change (and possibly all drivers/board
>> files/whatever using request_irq() and/or request_gpio()) have to change.
>> That is something that I do not dare to do alone.
>
> This is a controller-specific issue, and has nothing to do with the GPIO
> or IRQ frameworks. The driver for the combined irq/gpio_chip simply
> needs to program the HW when the IRQ is requested or set up. The Tegra
> driver already works this way, so there's actual proof that it is
> possible to do this in practice.

And how to you block the same line from being gpio_request()ed
and set as output?

Yours,
Linus Walleij

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/23/2013 01:53 PM, Linus Walleij wrote:
> On Wed, Sep 11, 2013 at 9:43 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> I believe this situation is exactly what cause the original patch to the
>> OMAP driver to be reverted; that patch should have touched the HW
>> directly to solve the problem when the IRQ was requested, rather than
>> calling into the GPIO subsystem (which also has the side-effect of
>> touching the HW in the same way as desired).
> 
> And that has the side-effect that this line, which is now set up
> in the HW as an input line, can be gpio_request()ed by some other
> code path and set up as output, screwing around with the very
> same registers.
> 
> I think the kernel should prevent such things.

It might be nice if it could do that.

However, that is 100% unrelated to the problem at hand.

A driver which only cares about an IRQ should be able to call just IRQ
APIs and have the HW work. Since not all IRQs are GPIOs, the thing that
causes the HW to work should not involve the GPIO subsystem in any way
at all.

So, the first issue - making a correctly configured system functionally
work without resorting to hacks such as requesting a GPIO to make an IRQ
work, is one thing. This issue should be solved purely inside the
specific HW driver.

Having the kernel detect when two different drivers both request the
same resource is entirely another thing. The solution to the first issue
must not rely on any solution to this second issue.

I'm also not convinced it's possible to solve this second issue given
the current kernel APIs, since there's not enough semantic information;
requests of GPIOs and IRQs aren't actually tied to a particular driver
at present (there's no "struct device *dev" parameter to request_irq or
gpio_request) and so the subsystems can't actually tell who is
requesting the GPIO/IRQ, and hence can't detect when the same driver, or
a different driver, is requesting the same core resource for different
purposes.

Equally, I am actually not 100% sure we want the core to prevent this.
Why shouldn't two different drivers request the same IRQ? Why shouldn't
at least one driver, perhaps more, request the pin as a GPIO (assuming
it will only read the GPIO value, not flip the pin to output). This
exact situation might happen on some Tegra boards where there's a GPIO
for VBUS_EN that affects 2 USB ports. It's supposed to be driven
open-collector. If an external entity forces it low, it means
over-current. Drivers for both ports might want to receive interrupts on
both edges, and even request the GPIO for input so they can both tell
which level the signal is at, in order to report OC/not-OC back up to
higher levels, as soon as any change occurs.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/23/2013 02:01 PM, Linus Walleij wrote:
> On Mon, Sep 16, 2013 at 7:09 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> Put another way, I don't believe
>> there's any rule when writing DT bindings that states that bindings must
>> not describe the same pin as both a GPIO and an IRQ, although admittedly
>> that may be unusual.
> 
> Actually I think you've won me over here.
> 
> But I do not think that this shall be done at the expense of letting
> DT-based drivers do nasty things like setting up the same GPIO
> line as an IRQ (hammering the hardware to do that) and then
> request the same line to be an output line and drive it, for
> example.
> 
> So the state of the GPIO line has to be tracked: if it is set as
> input and tied to an IRQ it should *not* be possible for the
> other code path to request it and set it as output. But it should
> be possible to set it as input again and read the value.
> 
> Laurent deascribed exactly the latter usecase, which is OK.
> 
>>> I agree with you that it would be the best if the only call would be
>>> request_irq and the chip driver programs the HW appropriately. It would be a
>>> dream, but unfortunately this is not possible at the moment. This is something
>>> that Linus pointed out very very early in this whole discussion. The gpio and
>>> irq frameworks don't share any information. The irq framework has no chance to
>>> program the HW, because it will never find the related gpio.
>>> For this to work the frameworks have to change (and possibly all drivers/board
>>> files/whatever using request_irq() and/or request_gpio()) have to change.
>>> That is something that I do not dare to do alone.
>>
>> This is a controller-specific issue, and has nothing to do with the GPIO
>> or IRQ frameworks. The driver for the combined irq/gpio_chip simply
>> needs to program the HW when the IRQ is requested or set up. The Tegra
>> driver already works this way, so there's actual proof that it is
>> possible to do this in practice.
> 
> And how to you block the same line from being gpio_request()ed
> and set as output?

To be honest, I really don't think this problem is terribly likely to
occur, so I'm really not convinced that it's worth putting a lot of
effort into solving it.

If the problem does occur, it's trivial to see that this has happened by
looking at /proc/interrupts and /sys/kernel/debug/gpio, assuming the
system doesn't hang when this happens due to an interrupt storm, and if
it does, it should be pretty easy to track down the problematic register
write.

That said, I'm not outright against the kernel checking for this
situation. I think it'd work as follows:

The only code that knows the correlation between the IRQ and GPIO in
question is the combined gpio_chip/irq_chip driver. Everything /has/ to
be driven through that driver. In some cases, an IRQ may not be related
to any GPIO at all; there are certainly IRQ controllers which take
inputs directly from outside the chip, but have no GPIO functionality at
all on those signals.

That driver needs to maintain some state that indicates which of its
IRQs have been requested. Any time a GPIO request (I mean e.g.
set_output() not request_gpio)) comes in, the request needs to be
validated against that IRQ usage state. If there's a conflict, deny the
GPIO request.

Now, it's quite possible that the code to maintain this state and
perform the checks will be similar/common across multiple drivers. If
so, by all means implement the code somewhere common, and have the
various irq_chip/gpio_chip drivers call into it.

The main thing is that all of this has to be driven/controlled/enabled
by the gpio_chip/irq_chip driver itself, not as some global/blanket
feature of the GPIO or IRQ subsystems.

Perhaps rather than having the gpio_chip/irq_chip drivers physically
implement a function which calls this common code, they could set some
flags/data/... in the struct gpio_chip/irq_chip indicating that they
desire the core code that implements the error-checking to be enabled.
That might save some levels of stack. But the key is still that the
whole scenario is controlled by the end driver, not always assumed to
apply by the core code.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Mon, Sep 23, 2013 at 10:12 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/23/2013 01:53 PM, Linus Walleij wrote:

>> I think the kernel should prevent such things.
>
> It might be nice if it could do that.
>
> However, that is 100% unrelated to the problem at hand.

I don't think it is unrelated when the old OMAP boardfile-based
code definately prevents such uses by its strict usage
of gpio_request() for all IRQ-bound GPIOs.

I think not preventing it for the DT boot path is setting lower
standards for DT code than for boardfile code which is not
what we should be doing.

> A driver which only cares about an IRQ should be able to call just IRQ
> APIs and have the HW work. Since not all IRQs are GPIOs, the thing that
> causes the HW to work should not involve the GPIO subsystem in any way
> at all.

Yes I have bought into that concept now.

> Having the kernel detect when two different drivers both request the
> same resource is entirely another thing. The solution to the first issue
> must not rely on any solution to this second issue.

I understand this stance from a DT point of view - which is about
resource passing and its syntax and semantics.

>From a GPIO subsystem point of view, in keeping resources under
kernel control, I naturally do not agree.

> I'm also not convinced it's possible to solve this second issue given
> the current kernel APIs, since there's not enough semantic information;
> requests of GPIOs and IRQs aren't actually tied to a particular driver
> at present (there's no "struct device *dev" parameter to request_irq or
> gpio_request) and so the subsystems can't actually tell who is
> requesting the GPIO/IRQ, and hence can't detect when the same driver, or
> a different driver, is requesting the same core resource for different
> purposes.

Solving the issue that e.g. two different drivers competing about the
same resource (as in one driver requesting an IRQ and another one
requesting a GPIO) is not what I'm after here.

I'm more after the GPIO subsystem having knowledge of a certain
GPIO line being requested for IRQ, and denying that line to be set
as input.

Maybe this can actually be achieved quite easily with
an additional API? Like gpio_lock_as_irq(gpio) which flags this
in .flags of struct gpio_desc and prevent such things?

Alexandre what do you think about this idea?

> Equally, I am actually not 100% sure we want the core to prevent this.
> Why shouldn't two different drivers request the same IRQ? Why shouldn't
> at least one driver, perhaps more, request the pin as a GPIO (assuming
> it will only read the GPIO value, not flip the pin to output).

But I have already stated that this is OK?

Are we talking past each other now?

> This
> exact situation might happen on some Tegra boards where there's a GPIO
> for VBUS_EN that affects 2 USB ports. It's supposed to be driven
> open-collector. If an external entity forces it low, it means
> over-current.

You are describing a very good reason for the core to be
doing exactly what I described I think?

Yours,
Linus Walleij

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Mon, Sep 23, 2013 at 10:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/23/2013 02:01 PM, Linus Walleij wrote:

>> And how to you block the same line from being gpio_request()ed
>> and set as output?
>
> To be honest, I really don't think this problem is terribly likely to
> occur, so I'm really not convinced that it's worth putting a lot of
> effort into solving it.

I have a different opionion, I think this is important. As GPIO
subsystem maintainer I need to be convinced of the integrity
of the system.

> If the problem does occur, it's trivial to see that this has happened by
> looking at /proc/interrupts and /sys/kernel/debug/gpio,

I basically want /sys/kernel/debug/gpio to say if a certain line
is tied for IRQ.

> That driver needs to maintain some state that indicates which of its
> IRQs have been requested. Any time a GPIO request (I mean e.g.
> set_output() not request_gpio)) comes in, the request needs to be
> validated against that IRQ usage state. If there's a conflict, deny the
> GPIO request.

I think this should be done by gpiolib, and I think it is trivial
given the right APIs. Putting it in the drivers will just create
an array of similar-look Rube Goldberg-machines and code
duplication. There is no point.

> Now, it's quite possible that the code to maintain this state and
> perform the checks will be similar/common across multiple drivers. If
> so, by all means implement the code somewhere common, and have the
> various irq_chip/gpio_chip drivers call into it.

And this is what we should do in gpiolib.

> The main thing is that all of this has to be driven/controlled/enabled
> by the gpio_chip/irq_chip driver itself, not as some global/blanket
> feature of the GPIO or IRQ subsystems.

Sure.

> Perhaps rather than having the gpio_chip/irq_chip drivers physically
> implement a function which calls this common code, they could set some
> flags/data/... in the struct gpio_chip/irq_chip indicating that they
> desire the core code that implements the error-checking to be enabled.

I think it should more be like a function they can call to flag
a GPIO as used for IRQ.

Yours,
Linus Walleij

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/24/2013 02:26 AM, Linus Walleij wrote:
> On Mon, Sep 23, 2013 at 10:12 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 09/23/2013 01:53 PM, Linus Walleij wrote:
> 
>>> I think the kernel should prevent such things.
>>
>> It might be nice if it could do that.
>>
>> However, that is 100% unrelated to the problem at hand.
> 
> I don't think it is unrelated when the old OMAP boardfile-based
> code definately prevents such uses by its strict usage
> of gpio_request() for all IRQ-bound GPIOs.
> 
> I think not preventing it for the DT boot path is setting lower
> standards for DT code than for boardfile code which is not
> what we should be doing.

Semantics matter.

In the old board file code, the gpio_request()s were present to work
around the bug in the OMAP driver where request_irq() wouldn't configure
the IRQ signal correctly. That's the primary reason those calls were there.

Now, this had the side-effect of also preventing anything else from
calling gpio_request() on those GPIOs, but that wasn't the primary
motivation; just a convenient effect.

...
> Solving the issue that e.g. two different drivers competing about the
> same resource (as in one driver requesting an IRQ and another one
> requesting a GPIO) is not what I'm after here.
> 
> I'm more after the GPIO subsystem having knowledge of a certain
> GPIO line being requested for IRQ, and denying that line to be set
> as input.

s/input/output/ I assume.

...
> Maybe this can actually be achieved quite easily with
> an additional API? Like gpio_lock_as_irq(gpio) which flags this
> in .flags of struct gpio_desc and prevent such things?
> 
> Alexandre what do you think about this idea?
> 
>> Equally, I am actually not 100% sure we want the core to prevent this.
>> Why shouldn't two different drivers request the same IRQ? Why shouldn't
>> at least one driver, perhaps more, request the pin as a GPIO (assuming
>> it will only read the GPIO value, not flip the pin to output).
> 
> But I have already stated that this is OK?
> 
> Are we talking past each other now?

If all you want to do is prevent gpio_direction_input() on a GPIO that's
in use as a GPIO, then that's probably OK.

However, the interrupt consistency patch that was posted implemented
that restriction by calling gpio_request(), and the wording of most of
what you've written implies to me that implementing the restriction by
calling gpio_request() is what you're after. That approach imposes far
more restrictions than just preventing gpio_direction_input(). Imposing
those additional restrictions is what I'm objecting to.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/24/2013 02:31 AM, Linus Walleij wrote:
> On Mon, Sep 23, 2013 at 10:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
...
>> Perhaps rather than having the gpio_chip/irq_chip drivers physically
>> implement a function which calls this common code, they could set some
>> flags/data/... in the struct gpio_chip/irq_chip indicating that they
>> desire the core code that implements the error-checking to be enabled.
> 
> I think it should more be like a function they can call to flag
> a GPIO as used for IRQ.

For the record, that's pretty much exactly what I meant by implementing
it in the drivers. The irq_chip driver knows when the IRQ has been
requested, and calls some gpiolib function to mark the GPIO as
in-use-as-an-IRQ.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Tue, Sep 24, 2013 at 6:59 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/24/2013 02:31 AM, Linus Walleij wrote:
>> On Mon, Sep 23, 2013 at 10:21 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> ...
>>> Perhaps rather than having the gpio_chip/irq_chip drivers physically
>>> implement a function which calls this common code, they could set some
>>> flags/data/... in the struct gpio_chip/irq_chip indicating that they
>>> desire the core code that implements the error-checking to be enabled.
>>
>> I think it should more be like a function they can call to flag
>> a GPIO as used for IRQ.
>
> For the record, that's pretty much exactly what I meant by implementing
> it in the drivers. The irq_chip driver knows when the IRQ has been
> requested, and calls some gpiolib function to mark the GPIO as
> in-use-as-an-IRQ.

Ah OK we're on the same page, this is progressing :-)

Yours,
Linus Walleij

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Gerlando Falauto]

Hi everyone,

[jumping in on an old discussion]

On 09/09/2013 06:19 PM, Mark Brown wrote:
> On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
>> On 09/04/2013 03:05 AM, Lars Poeschel wrote:
>
>>> The driver that tries to use the GPIO requested by this patch before HAS to
>>> fail. This is exactly the intention of this patch. We don't want the GPIO to
>>> be requested any more, if it is used as an interrupt pin.
>
>> That will break existing drivers. There are drivers that request the
>> same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
>> that way.
>
> Yes, plus input devices and audio jack detection among others.  This
> pattern is very common if the GPIO is actually being used as a GPIO, an
> edge triggered interrupt is used to flag when something happens and the
> state is determined by reading the GPIO state (often with some
> debounce).

I actually came across this thread while looking for an answer to the 
following (apparently trivial) question:

If you were to write a new driver & binding, what would be, in general, 
the recommended DT binding for a cascade interrupt controller (or any 
other peripheral, for that matter), which is connected through a GPIO 
(to be used as IRQ)?

a) Through gpios = <&gpio0 N>
b) through interrupt-parent = <&gpio0> & interrupts <N 
IRQ_TYPE_LEVEL_LOW>, or
c) both?

Thanks!
Gerlando

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 11/11/2013 11:28 AM, Gerlando Falauto wrote:
> Hi everyone,
> 
> [jumping in on an old discussion]
> 
> On 09/09/2013 06:19 PM, Mark Brown wrote:
>> On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
>>> On 09/04/2013 03:05 AM, Lars Poeschel wrote:
>>
>>>> The driver that tries to use the GPIO requested by this patch before
>>>> HAS to
>>>> fail. This is exactly the intention of this patch. We don't want the
>>>> GPIO to
>>>> be requested any more, if it is used as an interrupt pin.
>>
>>> That will break existing drivers. There are drivers that request the
>>> same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
>>> that way.
>>
>> Yes, plus input devices and audio jack detection among others.  This
>> pattern is very common if the GPIO is actually being used as a GPIO, an
>> edge triggered interrupt is used to flag when something happens and the
>> state is determined by reading the GPIO state (often with some
>> debounce).
> 
> I actually came across this thread while looking for an answer to the
> following (apparently trivial) question:
> 
> If you were to write a new driver & binding, what would be, in general,
> the recommended DT binding for a cascade interrupt controller (or any
> other peripheral, for that matter), which is connected through a GPIO
> (to be used as IRQ)?
> 
> a) Through gpios = <&gpio0 N>
> b) through interrupt-parent = <&gpio0> & interrupts <N
> IRQ_TYPE_LEVEL_LOW>, or
> c) both?

(b) alone.

>From the perspective of the child interrupt controller, its output is
purely an interrupt. The fact that the parent interrupt controller could
use that pin as a GPIO in some other context (e.g. on a different board)
isn't something that the child interrupt controller should know/care about.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Gerlando Falauto]

Hi Stephan,

On 11/11/2013 07:53 PM, Stephen Warren wrote:
> On 11/11/2013 11:28 AM, Gerlando Falauto wrote:
>> Hi everyone,
>>
>> [jumping in on an old discussion]
>>
>> On 09/09/2013 06:19 PM, Mark Brown wrote:
>>> On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
>>>> On 09/04/2013 03:05 AM, Lars Poeschel wrote:
>>>
>>>>> The driver that tries to use the GPIO requested by this patch before
>>>>> HAS to
>>>>> fail. This is exactly the intention of this patch. We don't want the
>>>>> GPIO to
>>>>> be requested any more, if it is used as an interrupt pin.
>>>
>>>> That will break existing drivers. There are drivers that request the
>>>> same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
>>>> that way.
>>>
>>> Yes, plus input devices and audio jack detection among others.  This
>>> pattern is very common if the GPIO is actually being used as a GPIO, an
>>> edge triggered interrupt is used to flag when something happens and the
>>> state is determined by reading the GPIO state (often with some
>>> debounce).
>>
>> I actually came across this thread while looking for an answer to the
>> following (apparently trivial) question:
>>
>> If you were to write a new driver & binding, what would be, in general,
>> the recommended DT binding for a cascade interrupt controller (or any
>> other peripheral, for that matter), which is connected through a GPIO
>> (to be used as IRQ)?
>>
>> a) Through gpios = <&gpio0 N>
>> b) through interrupt-parent = <&gpio0> & interrupts <N
>> IRQ_TYPE_LEVEL_LOW>, or
>> c) both?
>
> (b) alone.
>
>  From the perspective of the child interrupt controller, its output is
> purely an interrupt. The fact that the parent interrupt controller could
> use that pin as a GPIO in some other context (e.g. on a different board)
> isn't something that the child interrupt controller should know/care about.
>

Thanks for your answer. So it should be the parent driver to proactively 
configure the GPIO as input, active high etc..., when an IRQ for its 
GPIOs is requested, right?

Moving to a different target (actually Mark's original point), i.e. a 
generic device which is connected through a GPIO, using the IRQ to 
trigger an event, what would be the recommended way?

Thanks again!
Gerlando

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 11/11/2013 12:17 PM, Gerlando Falauto wrote:
> Hi Stephan,
> 
> On 11/11/2013 07:53 PM, Stephen Warren wrote:
>> On 11/11/2013 11:28 AM, Gerlando Falauto wrote:
>>> Hi everyone,
>>>
>>> [jumping in on an old discussion]
>>>
>>> On 09/09/2013 06:19 PM, Mark Brown wrote:
>>>> On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
>>>>> On 09/04/2013 03:05 AM, Lars Poeschel wrote:
>>>>
>>>>>> The driver that tries to use the GPIO requested by this patch before
>>>>>> HAS to
>>>>>> fail. This is exactly the intention of this patch. We don't want the
>>>>>> GPIO to
>>>>>> be requested any more, if it is used as an interrupt pin.
>>>>
>>>>> That will break existing drivers. There are drivers that request the
>>>>> same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
>>>>> that way.
>>>>
>>>> Yes, plus input devices and audio jack detection among others.  This
>>>> pattern is very common if the GPIO is actually being used as a GPIO, an
>>>> edge triggered interrupt is used to flag when something happens and the
>>>> state is determined by reading the GPIO state (often with some
>>>> debounce).
>>>
>>> I actually came across this thread while looking for an answer to the
>>> following (apparently trivial) question:
>>>
>>> If you were to write a new driver & binding, what would be, in general,
>>> the recommended DT binding for a cascade interrupt controller (or any
>>> other peripheral, for that matter), which is connected through a GPIO
>>> (to be used as IRQ)?
>>>
>>> a) Through gpios = <&gpio0 N>
>>> b) through interrupt-parent = <&gpio0> & interrupts <N
>>> IRQ_TYPE_LEVEL_LOW>, or
>>> c) both?
>>
>> (b) alone.
>>
>>  From the perspective of the child interrupt controller, its output is
>> purely an interrupt. The fact that the parent interrupt controller could
>> use that pin as a GPIO in some other context (e.g. on a different board)
>> isn't something that the child interrupt controller should know/care
>> about.
>>
> 
> Thanks for your answer. So it should be the parent driver to proactively
> configure the GPIO as input, active high etc..., when an IRQ for its
> GPIOs is requested, right?

That was the conclusion of this thread, or a similar thread, yes.

> Moving to a different target (actually Mark's original point), i.e. a
> generic device which is connected through a GPIO, using the IRQ to
> trigger an event, what would be the recommended way?

Again, the device that's the interrupt source is simply emitting an
interrupt signal, so that's all it should know about.

Now, if you're talking about a device that's emitting just some random
data signal, where the driver wants to be notified of changes in the
signal, then that should be modelled as a GPIO, which the driver can
then convert internally to an IRQ and request. That's because from the
HW perspective, the signal isn't an IRQ output.

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Tomasz Figa]

On Monday 11 of November 2013 12:33:20 Stephen Warren wrote:
> On 11/11/2013 12:17 PM, Gerlando Falauto wrote:
> > Hi Stephan,
> > 
> > On 11/11/2013 07:53 PM, Stephen Warren wrote:
> >> On 11/11/2013 11:28 AM, Gerlando Falauto wrote:
> >>> Hi everyone,
> >>>
> >>> [jumping in on an old discussion]
> >>>
> >>> On 09/09/2013 06:19 PM, Mark Brown wrote:
> >>>> On Wed, Sep 04, 2013 at 02:16:36PM -0600, Stephen Warren wrote:
> >>>>> On 09/04/2013 03:05 AM, Lars Poeschel wrote:
> >>>>
> >>>>>> The driver that tries to use the GPIO requested by this patch before
> >>>>>> HAS to
> >>>>>> fail. This is exactly the intention of this patch. We don't want the
> >>>>>> GPIO to
> >>>>>> be requested any more, if it is used as an interrupt pin.
> >>>>
> >>>>> That will break existing drivers. There are drivers that request the
> >>>>> same GPIO and IRQ. IIRC, the SDHCI CD (Card Detect) GPIO is requested
> >>>>> that way.
> >>>>
> >>>> Yes, plus input devices and audio jack detection among others.  This
> >>>> pattern is very common if the GPIO is actually being used as a GPIO, an
> >>>> edge triggered interrupt is used to flag when something happens and the
> >>>> state is determined by reading the GPIO state (often with some
> >>>> debounce).
> >>>
> >>> I actually came across this thread while looking for an answer to the
> >>> following (apparently trivial) question:
> >>>
> >>> If you were to write a new driver & binding, what would be, in general,
> >>> the recommended DT binding for a cascade interrupt controller (or any
> >>> other peripheral, for that matter), which is connected through a GPIO
> >>> (to be used as IRQ)?
> >>>
> >>> a) Through gpios = <&gpio0 N>
> >>> b) through interrupt-parent = <&gpio0> & interrupts <N
> >>> IRQ_TYPE_LEVEL_LOW>, or
> >>> c) both?
> >>
> >> (b) alone.
> >>
> >>  From the perspective of the child interrupt controller, its output is
> >> purely an interrupt. The fact that the parent interrupt controller could
> >> use that pin as a GPIO in some other context (e.g. on a different board)
> >> isn't something that the child interrupt controller should know/care
> >> about.
> >>
> > 
> > Thanks for your answer. So it should be the parent driver to proactively
> > configure the GPIO as input, active high etc..., when an IRQ for its
> > GPIOs is requested, right?
> 
> That was the conclusion of this thread, or a similar thread, yes.

I would only add that settings, such as pull up/down control that depends
on the chip that drives the interrupt pin (i.e. the child interrupt
controller), are usually configured using pin control bindings.

Best regards,
Tomasz

Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Mon, Nov 11, 2013 at 8:33 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 11/11/2013 12:17 PM, Gerlando Falauto wrote:

>> Thanks for your answer. So it should be the parent driver to proactively
>> configure the GPIO as input, active high etc..., when an IRQ for its
>> GPIOs is requested, right?
>
> That was the conclusion of this thread, or a similar thread, yes.

Yepps second that.

And we added an API to that you can flag a GPIO as being
in use by an IRQ using gpio_lock_as_irq() from your
irq_chip callbacks.

Yours,
Linus Walleij