2013-08-13 09:50:52

by Lars Poeschel

[permalink] [raw]
Subject: [PATCH v2] RFC: interrupt consistency check for OF GPIO IRQs

From: Linus Walleij <[email protected]>

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_reques() and gpio_direction_input() on these,
making them unreachable from the GPIO side.

Cc: [email protected]
Cc: Javier Martinez Canillas <[email protected]>
Cc: Enric Balletbo i Serra <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc: Santosh Shilimkar <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Balaji T K <[email protected]>
Cc: Tony Lindgren <[email protected]>
Cc: Jon Hunter <[email protected]>
Signed-off-by: Linus Walleij <[email protected]>
Signed-off-by: Lars Poeschel <[email protected]>

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 665f953..5f6ac79 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>

@@ -127,6 +127,160 @@ int of_gpio_simple_xlate(struct gpio_chip *gc,
EXPORT_SYMBOL(of_gpio_simple_xlate);

/**
+ * of_gpio_scan_nodes_and_x_irq_lines - internal function to recursively scan
+ * the device tree and request or free the GPIOs that are to be use
+ * @node: node to start the scanning at
+ * @gcn: 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 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_x_irq_lines function.
+ */
+void of_gpio_scan_nodes_and_x_irq_lines(
+ const struct device_node *const node,
+ struct device_node * const gcn,
+ const struct gpio_chip * const gc, int const request)
+{
+ struct device_node *child;
+ struct device_node *irq_parent;
+ const __be32 *intspec;
+ u32 intlen;
+ u32 offset;
+ int ret;
+ int num_irq;
+ int i;
+
+ if (node == NULL)
+ return;
+
+ for_each_child_of_node(node, child) {
+ of_gpio_scan_nodes_and_x_irq_lines(child, gcn, 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);
+
+ num_irq = of_irq_count(gcn);
+ for (i = 0; i < num_irq; i++) {
+ /*
+ * The first cell is always the local IRQ number, and
+ * this corresponds to the GPIO line offset for a GPIO
+ * chip.
+ *
+ * FIXME: take interrupt-cells into account here.
+ */
+ offset = of_read_number(intspec + i, 1);
+ pr_debug("gpiochip: OF node references offset=%d\n",
+ offset);
+
+ 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 + offset, "OF IRQ");
+ if (ret)
+ pr_err("gpiolib OF: could not request IRQ GPIO %d (%d)\n",
+ gc->base + offset, offset);
+ ret = gpio_direction_input(gc->base + offset);
+ if (ret)
+ pr_err("gpiolib OF: could not set IRQ GPIO %d (%d) as input\n",
+ gc->base + offset, offset);
+ } else {
+ gpio_free(gc->base + offset);
+ }
+ }
+ }
+}
+
+#define of_gpiochip_request_irq_lines(np, gc) \
+ of_gpiochip_x_irq_lines(np, gc, 1)
+
+#define of_gpiochip_free_irq_lines(np, gc) \
+ of_gpiochip_x_irq_lines(np, gc, 0)
+
+/**
+ * of_gpiochip_x_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_x_irq_lines(struct device_node *np,
+ struct gpio_chip *gc, int request)
+{
+ struct device_node *root;
+
+ /*
+ * 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;
+
+ of_gpio_scan_nodes_and_x_irq_lines(root, np, gc, request);
+
+ of_node_put(root);
+}
+
+/**
* of_mm_gpiochip_add - Add memory mapped GPIO chip (bank)
* @np: device node of the GPIO chip
* @mm_gc: pointer to the of_mm_gpio_chip allocated structure
@@ -170,6 +324,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 +387,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


2013-08-13 15:23:05

by Kevin Hilman

[permalink] [raw]
Subject: Re: [PATCH v2] RFC: interrupt consistency check for OF GPIO IRQs

Lars Poeschel <[email protected]> writes:

> From: Linus Walleij <[email protected]>
>
> 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.

And another reason, which happens on OMAP (not that the others aren't
already enough to make the case):

- Platform-specific power management code may gate clocks or power to
unused GPIO banks resulting in faults when accessing a GPIO that
has not been properly requested via gpio_request().

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

Kevin

2013-08-15 09:53:29

by Tomasz Figa

[permalink] [raw]
Subject: Re: [PATCH v2] RFC: interrupt consistency check for OF GPIO IRQs

Hi Lars, Linus,

On Tuesday 13 of August 2013 11:46:35 Lars Poeschel wrote:
> From: Linus Walleij <[email protected]>
>
> 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_reques() and gpio_direction_input() on these,
> making them unreachable from the GPIO side.

The idea is rather interesting, but there are some problems I commented on
inline. (Feel free to ignore those that are nits, since this is at RFC
stage.)

> Cc: [email protected]
> Cc: Javier Martinez Canillas <[email protected]>
> Cc: Enric Balletbo i Serra <[email protected]>
> Cc: Grant Likely <[email protected]>
> Cc: Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
> Cc: Santosh Shilimkar <[email protected]>
> Cc: Kevin Hilman <[email protected]>
> Cc: Balaji T K <[email protected]>
> Cc: Tony Lindgren <[email protected]>
> Cc: Jon Hunter <[email protected]>
> Signed-off-by: Linus Walleij <[email protected]>
> Signed-off-by: Lars Poeschel <[email protected]>
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 665f953..5f6ac79 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>
>
> @@ -127,6 +127,160 @@ int of_gpio_simple_xlate(struct gpio_chip *gc,
> EXPORT_SYMBOL(of_gpio_simple_xlate);
>
> /**
> + * of_gpio_scan_nodes_and_x_irq_lines - internal function to

Hmm, what is an "x irq line"?

> recursively scan + * the device tree and request or free the GPIOs that
> are to be use + * @node: node to start the scanning at
> + * @gcn: 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 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_x_irq_lines
> function. + */
> +void of_gpio_scan_nodes_and_x_irq_lines(
> + const struct device_node *const node,
> + struct device_node * const gcn,
> + const struct gpio_chip * const gc, int const request)
> +{
> + struct device_node *child;
> + struct device_node *irq_parent;
> + const __be32 *intspec;
> + u32 intlen;
> + u32 offset;
> + int ret;
> + int num_irq;
> + int i;
> +
> + if (node == NULL)
> + return;
> +
> + for_each_child_of_node(node, child) {
> + of_gpio_scan_nodes_and_x_irq_lines(child, gcn, gc,
request);

nit: A blank line would be nice here.

> + /* Check if we have an IRQ parent, else continue */
> + irq_parent = of_irq_find_parent(child);
> + if (!irq_parent)
> + continue;

You can probably put the irq_parent node here to not duplicate calls in
both code paths below.

> + /* 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);
> +
> + num_irq = of_irq_count(gcn);
> + for (i = 0; i < num_irq; i++) {
> + /*
> + * The first cell is always the local IRQ number,
and
> + * this corresponds to the GPIO line offset for a
GPIO
> + * chip.
> + *
> + * FIXME: take interrupt-cells into account here.

This is the biggest problem of this patch. It assumes that there is only a
single and shared GPIO/interrupt specification scheme, which might not be
true.

First of all, almost all interrupt bindings have an extra, semi-generic
flags field as last cell in the specifier. Now there can be more than one
cell used to index GPIOs and interrupts, for example:

interrupts = <1 3 8>

which could mean: bank 1, pin 3, low level triggered.

I think you may need to reuse a lot of the code that normally parses the
interrupts property, i.e. the irq_of_parse_and_map() path, which will then
give you the hwirq index inside your irq chip, which may (or may not) be
the same as the GPIO offset inside your GPIO chip.

If you're unlucky enough to spot a controller that uses completely
different numbering schemes for GPIOs and interrupts, then you're probably
screwed, because only the driver for this controller can know the mapping
between them.

I don't have any good ideas for doing this properly at the moment, but I
will think about it and post another reply if something nice comes to my
mind.

> + */
> + offset = of_read_number(intspec + i, 1);

nit: of_read_number is a little overkill for simply getting a single cell.
You could use be32_to_cpup(intspec + i) to achieve the same.

> + pr_debug("gpiochip: OF node references
offset=%d\n",
> + offset);
> +
> + 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 + offset, "OF
IRQ");
> + if (ret)
> + pr_err("gpiolib OF: could not
request IRQ GPIO %d (%d)\n",
> + gc->base + offset, offset);

Would some kind of error handling be a bad idea here? Like, for example,
marking this IRQ as invalid or something among these lines.

> + ret = gpio_direction_input(gc->base +
offset);
> + if (ret)
> + pr_err("gpiolib OF: could not set
IRQ GPIO %d (%d) as input\n",
> + gc->base + offset, offset);

I'm not sure if this is the correct action if someone already requested
the GPIO before and probably already set it up with their own set of
parameters (not necessarily the same as enforced by calling
gpio_direction_input()).

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

What if the request failed? This would mean calling gpio_free() on a GPIO
previously requested by someone else.

> + }
> + }
> + }
> +}
> +
> +#define of_gpiochip_request_irq_lines(np, gc) \
> + of_gpiochip_x_irq_lines(np, gc, 1)
> +
> +#define of_gpiochip_free_irq_lines(np, gc) \
> + of_gpiochip_x_irq_lines(np, gc, 0)

Aha, so the x is a wildcard. Fine, although it makes the code slightly
less reader-friendly IMHO.

Best regards,
Tomasz


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part.

2013-08-15 12:13:15

by Lars Poeschel

[permalink] [raw]
Subject: Re: [PATCH v2] RFC: interrupt consistency check for OF GPIO IRQs

On Thursday 15 August 2013 at 11:53:15, Tomasz Figa wrote:
> Hi Lars, Linus,
>
> On Tuesday 13 of August 2013 11:46:35 Lars Poeschel wrote:
> > From: Linus Walleij <[email protected]>
> >
> > 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_reques() and gpio_direction_input() on these,
> > making them unreachable from the GPIO side.
>
> The idea is rather interesting, but there are some problems I commented
> on inline. (Feel free to ignore those that are nits, since this is at
> RFC stage.)

I don't want to ignore. I want to discuss and test the idea here. Thanks
for your contribution!

> > /**
> >
> > + * of_gpio_scan_nodes_and_x_irq_lines - internal function to
>
> Hmm, what is an "x irq line"?

You already found out. Comment on this is below.

> > + for_each_child_of_node(node, child) {
> > + of_gpio_scan_nodes_and_x_irq_lines(child, gcn, gc,
>
> request);
>
> nit: A blank line would be nice here.

Ok.

> > + /* Check if we have an IRQ parent, else continue */
> > + irq_parent = of_irq_find_parent(child);
> > + if (!irq_parent)
> > + continue;
>
> You can probably put the irq_parent node here to not duplicate calls in
> both code paths below.

I thought about that before. Is this really allowed? Would the inequality-
check (irq_parent != gcn) after of_node_put be still valid?

> > + /* 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);
> > +
> > + num_irq = of_irq_count(gcn);
> > + for (i = 0; i < num_irq; i++) {
> > + /*
> > + * The first cell is always the local IRQ number,
>
> and
>
> > + * this corresponds to the GPIO line offset for a
>
> GPIO
>
> > + * chip.
> > + *
> > + * FIXME: take interrupt-cells into account here.
>
> This is the biggest problem of this patch. It assumes that there is only
> a single and shared GPIO/interrupt specification scheme, which might
> not be true.
>
> First of all, almost all interrupt bindings have an extra, semi-generic
> flags field as last cell in the specifier. Now there can be more than
> one cell used to index GPIOs and interrupts, for example:
>
> interrupts = <1 3 8>
>
> which could mean: bank 1, pin 3, low level triggered.
>
> I think you may need to reuse a lot of the code that normally parses the
> interrupts property, i.e. the irq_of_parse_and_map() path, which will
> then give you the hwirq index inside your irq chip, which may (or may
> not) be the same as the GPIO offset inside your GPIO chip.

Ok, valid objection. This is what the FIXME is about. But this should be
solvable. Am I right that there are 3 cases to handle:
interrupt-cells = 1: It is the index for the gpio.
interrupt-cells = 2: First is index for the gpio, second flags (like low
level triggered)
interrupt-cells = 3: First bank number, middle gpio inside that bank, last
flags.

You are right, that we should try to reuse existing code for that. I will
see, if I find the time to put up a third patch, which honors this.

> If you're unlucky enough to spot a controller that uses completely
> different numbering schemes for GPIOs and interrupts, then you're
> probably screwed, because only the driver for this controller can know
> the mapping between them.

Do such cases exist right now? Shouldn't be the number I request for
interrupt in the device tree the gpio number I use with this chip ?
Everything else would be weird.

> I don't have any good ideas for doing this properly at the moment, but I
> will think about it and post another reply if something nice comes to my
> mind.

If we really have to take this into account, that would require an
additional function pointer inside gpio_chip, something like irq_to_gpio.
The driver has to implement this functions then.

> > + */
> > + offset = of_read_number(intspec + i, 1);
>
> nit: of_read_number is a little overkill for simply getting a single
> cell. You could use be32_to_cpup(intspec + i) to achieve the same.

Ok.

> > + pr_debug("gpiochip: OF node references
>
> offset=%d\n",
>
> > + offset);
> > +
> > + 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 + offset, "OF
>
> IRQ");
>
> > + if (ret)
> > + pr_err("gpiolib OF: could not
>
> request IRQ GPIO %d (%d)\n",
>
> > + gc->base + offset, offset);
>
> Would some kind of error handling be a bad idea here? Like, for example,
> marking this IRQ as invalid or something among these lines.

If that fails, things are like before and someone would request an irq for
a gpio he does not own. Again what misses here is a connection between gpio
and irq subsystem. I could only record that this gpio pin failed somewhere
in the gpio subsystem, but what has to fail later is the request for the
irq inside the irq subsystem.
Hmm....

> > + ret = gpio_direction_input(gc->base +
>
> offset);
>
> > + if (ret)
> > + pr_err("gpiolib OF: could not set
>
> IRQ GPIO %d (%d) as input\n",
>
> > + gc->base + offset, offset);
>
> I'm not sure if this is the correct action if someone already requested
> the GPIO before and probably already set it up with their own set of
> parameters (not necessarily the same as enforced by calling
> gpio_direction_input()).

That should definitely not happen! This is what this patch is for. As we
are requesting this gpio somewhere inside the gpiochip_add process, no one
should be able to request that gpio earlier.

> > + } else {
> > + gpio_free(gc->base + offset);
>
> What if the request failed? This would mean calling gpio_free() on a
> GPIO previously requested by someone else.

See above.

> > + }
> > + }
> > + }
> > +}
> > +
> > +#define of_gpiochip_request_irq_lines(np, gc) \
> > + of_gpiochip_x_irq_lines(np, gc, 1)
> > +
> > +#define of_gpiochip_free_irq_lines(np, gc) \
> > + of_gpiochip_x_irq_lines(np, gc, 0)
>
> Aha, so the x is a wildcard. Fine, although it makes the code slightly
> less reader-friendly IMHO.

I am not happy with this naming as well, but I did not want to duplicate
the code. I could have seperate request_irq_lines and free_irq_lines
functions having most of the code in common. Has someone a better solution
in mind ?

Thanks,
Lars

2013-08-15 12:31:21

by Tomasz Figa

[permalink] [raw]
Subject: Re: [PATCH v2] RFC: interrupt consistency check for OF GPIO IRQs

On Thursday 15 of August 2013 14:12:43 Lars Poeschel wrote:
> On Thursday 15 August 2013 at 11:53:15, Tomasz Figa wrote:
> > Hi Lars, Linus,
> >
> > On Tuesday 13 of August 2013 11:46:35 Lars Poeschel wrote:
> > > From: Linus Walleij <[email protected]>
> > >
> > > 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_reques() and gpio_direction_input() on these,
> > > making them unreachable from the GPIO side.
> >
> > The idea is rather interesting, but there are some problems I
> > commented
> > on inline. (Feel free to ignore those that are nits, since this is at
> > RFC stage.)
>
> I don't want to ignore. I want to discuss and test the idea here. Thanks
> for your contribution!

You're welcome.

> > > /**
> > >
> > > + * of_gpio_scan_nodes_and_x_irq_lines - internal function to
> >
> > Hmm, what is an "x irq line"?
>
> You already found out. Comment on this is below.
>
> > > + for_each_child_of_node(node, child) {
> > > + of_gpio_scan_nodes_and_x_irq_lines(child, gcn, gc,
> >
> > request);
> >
> > nit: A blank line would be nice here.
>
> Ok.
>
> > > + /* Check if we have an IRQ parent, else continue */
> > > + irq_parent = of_irq_find_parent(child);
> > > + if (!irq_parent)
> > > + continue;
> >
> > You can probably put the irq_parent node here to not duplicate calls
> > in
> > both code paths below.
>
> I thought about that before. Is this really allowed? Would the
> inequality- check (irq_parent != gcn) after of_node_put be still valid?

Well, this is just a check over pointers. Assuming that you know that your
GPIO chip won't go away while this code is running, you can safely compare
the pointers as the same device tree node can't have two different
pointers.

> > > + /* 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);
> > > +
> > > + num_irq = of_irq_count(gcn);
> > > + for (i = 0; i < num_irq; i++) {
> > > + /*
> > > + * The first cell is always the local IRQ number,
> >
> > and
> >
> > > + * this corresponds to the GPIO line offset for a
> >
> > GPIO
> >
> > > + * chip.
> > > + *
> > > + * FIXME: take interrupt-cells into account here.
> >
> > This is the biggest problem of this patch. It assumes that there is
> > only a single and shared GPIO/interrupt specification scheme, which
> > might not be true.
> >
> > First of all, almost all interrupt bindings have an extra,
> > semi-generic
> > flags field as last cell in the specifier. Now there can be more than
> >
> > one cell used to index GPIOs and interrupts, for example:
> > interrupts = <1 3 8>
> >
> > which could mean: bank 1, pin 3, low level triggered.
> >
> > I think you may need to reuse a lot of the code that normally parses
> > the interrupts property, i.e. the irq_of_parse_and_map() path, which
> > will then give you the hwirq index inside your irq chip, which may
> > (or may not) be the same as the GPIO offset inside your GPIO chip.
>
> Ok, valid objection. This is what the FIXME is about. But this should be
> solvable. Am I right that there are 3 cases to handle:
> interrupt-cells = 1: It is the index for the gpio.
> interrupt-cells = 2: First is index for the gpio, second flags (like low
> level triggered)
> interrupt-cells = 3: First bank number, middle gpio inside that bank,
> last flags.

Well, there are basically no restrictions over the format of GPIO and
interrupt specifiers. Any driver is free to define its own and provide
private .xlate() callback to parse it. No assumptions should be made about
the format, other than each GPIO/interrupt is specified by numer of cells
specified in #interrupt- or #gpio-cells property of the controller.

> You are right, that we should try to reuse existing code for that. I
> will see, if I find the time to put up a third patch, which honors
> this.
> > If you're unlucky enough to spot a controller that uses completely
> > different numbering schemes for GPIOs and interrupts, then you're
> > probably screwed, because only the driver for this controller can know
> > the mapping between them.
>
> Do such cases exist right now? Shouldn't be the number I request for
> interrupt in the device tree the gpio number I use with this chip ?
> Everything else would be weird.

We can't simply cover all the existing cases and say it's done. We should
design things that are more or less future-proof.

I can imagine a single device that is both a GPIO expander and an
interrupt controller, but has separate sets of pins, e.g. 8 dedicated
interrupt pins and 16 separate general-purpose I/O pins. It would have
both interrupt-controller and gpio-controller properties, but wouldn't not
have any mapping between both namespaces at all, because both are about
completely different entities.

> > I don't have any good ideas for doing this properly at the moment, but
> > I will think about it and post another reply if something nice comes
> > to my mind.
>
> If we really have to take this into account, that would require an
> additional function pointer inside gpio_chip, something like
> irq_to_gpio. The driver has to implement this functions then.

Something like this could probably handle this, but I'm not sure if this
is really necessary. Let me think about it a bit more as I think this
could be done in a completely different way, but need to figure out all
the details.

> > > + */
> > > + offset = of_read_number(intspec + i, 1);
> >
> > nit: of_read_number is a little overkill for simply getting a single
> > cell. You could use be32_to_cpup(intspec + i) to achieve the same.
>
> Ok.
>
> > > + pr_debug("gpiochip: OF node references
> >
> > offset=%d\n",
> >
> > > + offset);
> > > +
> > > + 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 + offset, "OF
> >
> > IRQ");
> >
> > > + if (ret)
> > > + pr_err("gpiolib OF: could not
> >
> > request IRQ GPIO %d (%d)\n",
> >
> > > + gc->base + offset, offset);
> >
> > Would some kind of error handling be a bad idea here? Like, for
> > example, marking this IRQ as invalid or something among these lines.
>
> If that fails, things are like before and someone would request an irq
> for a gpio he does not own. Again what misses here is a connection
> between gpio and irq subsystem. I could only record that this gpio pin
> failed somewhere in the gpio subsystem, but what has to fail later is
> the request for the irq inside the irq subsystem.
> Hmm....

See below.

> > > + ret = gpio_direction_input(gc->base +
> >
> > offset);
> >
> > > + if (ret)
> > > + pr_err("gpiolib OF: could not set
> >
> > IRQ GPIO %d (%d) as input\n",
> >
> > > + gc->base + offset, offset);
> >
> > I'm not sure if this is the correct action if someone already
> > requested
> > the GPIO before and probably already set it up with their own set of
> > parameters (not necessarily the same as enforced by calling
> > gpio_direction_input()).
>
> That should definitely not happen! This is what this patch is for. As we
> are requesting this gpio somewhere inside the gpiochip_add process, no
> one should be able to request that gpio earlier.

Ahh, right. The pins are being requested before the GPIO chip actually
becomes available to the consumers. So this is probably fine. Sorry for
the noise.

> > > + } else {
> > > + gpio_free(gc->base + offset);
> >
> > What if the request failed? This would mean calling gpio_free() on a
> > GPIO previously requested by someone else.
>
> See above.

Fair enough.

> > > + }
> > > + }
> > > + }
> > > +}
> > > +
> > > +#define of_gpiochip_request_irq_lines(np, gc) \
> > > + of_gpiochip_x_irq_lines(np, gc, 1)
> > > +
> > > +#define of_gpiochip_free_irq_lines(np, gc) \
> > > + of_gpiochip_x_irq_lines(np, gc, 0)
> >
> > Aha, so the x is a wildcard. Fine, although it makes the code slightly
> > less reader-friendly IMHO.
>
> I am not happy with this naming as well, but I did not want to duplicate
> the code. I could have seperate request_irq_lines and free_irq_lines
> functions having most of the code in common. Has someone a better
> solution in mind ?

Well, simply a better name would be enough, but this is just a nitpick of
mine.

Best regards,
Tomasz


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part.

2013-08-17 00:02:30

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v2] RFC: interrupt consistency check for OF GPIO IRQs

Hi Lars,

On Tue, Aug 13, 2013 at 11:46 AM, Lars Poeschel <[email protected]> wrote:

> From: Linus Walleij <[email protected]>
>
> 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 took the liberty to take your tweaked V2 version of my patch,
tweak it a bit more (addressing some of Tomasz comments)
and re-submit, along with two patches making it do the trick
on my Nomadik test-system.

Yours,
Linus Walleij

2013-08-17 00:17:12

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v2] RFC: interrupt consistency check for OF GPIO IRQs

On Thu, Aug 15, 2013 at 11:53 AM, Tomasz Figa <[email protected]> wrote:

>> + /* Check if we have an IRQ parent, else continue */
>> + irq_parent = of_irq_find_parent(child);
>> + if (!irq_parent)
>> + continue;
>
> You can probably put the irq_parent node here to not duplicate calls in
> both code paths below.

But...

>> + /* Is it so that this very GPIO chip is the parent? */
>> + if (irq_parent != gcn) {

I am using it here in this comparison.

>> + of_node_put(irq_parent);
>> + continue;
>> + }
>> + of_node_put(irq_parent);

Then here I put it.

>> + num_irq = of_irq_count(gcn);
>> + for (i = 0; i < num_irq; i++) {
>> + /*
>> + * The first cell is always the local IRQ number,
> and
>> + * this corresponds to the GPIO line offset for a
> GPIO
>> + * chip.
>> + *
>> + * FIXME: take interrupt-cells into account here.
>
> This is the biggest problem of this patch. It assumes that there is only a
> single and shared GPIO/interrupt specification scheme, which might not be
> true.
>
> First of all, almost all interrupt bindings have an extra, semi-generic
> flags field as last cell in the specifier. Now there can be more than one
> cell used to index GPIOs and interrupts, for example:
>
> interrupts = <1 3 8>
>
> which could mean: bank 1, pin 3, low level triggered.

You are right, but:
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
Specifies how to handle the one-celled and two-celled versions.

And nothing else is specified. So it's not overly complex.

But we have to read out the #interrupt-cells specifier from the
controller I guess, but since we the gcn pointer we can do this
easily. (I'll fix...)

> I think you may need to reuse a lot of the code that normally parses the
> interrupts property, i.e. the irq_of_parse_and_map() path, which will then
> give you the hwirq index inside your irq chip, which may (or may not) be
> the same as the GPIO offset inside your GPIO chip.

We don't need to map it, and that's the hard part of that code.
We just need to add some code to check the number of
cells.

> If you're unlucky enough to spot a controller that uses completely
> different numbering schemes for GPIOs and interrupts, then you're probably
> screwed, because only the driver for this controller can know the mapping
> between them.

But the binding documents state how this shall be done, as illustrated.
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
Documentation/devicetree/bindings/gpio/gpio.txt

>> + if (ret)
>> + pr_err("gpiolib OF: could not
> request IRQ GPIO %d (%d)\n",
>> + gc->base + offset, offset);
>
> Would some kind of error handling be a bad idea here? Like, for example,
> marking this IRQ as invalid or something among these lines.

Sorry I'm not following. The GPIO core has no clue of how the
driver implements its irqchip and no handle on it so it cannot
go in and mark any interrupts.

>> + ret = gpio_direction_input(gc->base +
> offset);
>> + if (ret)
>> + pr_err("gpiolib OF: could not set
> IRQ GPIO %d (%d) as input\n",
>> + gc->base + offset, offset);
>
> I'm not sure if this is the correct action if someone already requested
> the GPIO before and probably already set it up with their own set of
> parameters (not necessarily the same as enforced by calling
> gpio_direction_input()).

What do you mean with " someone already requested the GPIO
before" here?

This code is run right after the gpiochip is added, noone has a
chance of requesting anything before this happens. This is the
general idea: steal all GPIOs marked to be used as interrupts
so no other consumer can get at them.

>> + } else {
>> + gpio_free(gc->base + offset);
>
> What if the request failed? This would mean calling gpio_free() on a GPIO
> previously requested by someone else.

Noone can request before us, as stated.


>> +#define of_gpiochip_request_irq_lines(np, gc) \
>> + of_gpiochip_x_irq_lines(np, gc, 1)
>> +
>> +#define of_gpiochip_free_irq_lines(np, gc) \
>> + of_gpiochip_x_irq_lines(np, gc, 0)
>
> Aha, so the x is a wildcard. Fine, although it makes the code slightly
> less reader-friendly IMHO.

I renamed it a bit.

Yours,
Linus Walleij

2013-08-17 00:26:33

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v2] RFC: interrupt consistency check for OF GPIO IRQs

On Thu, Aug 15, 2013 at 2:31 PM, Tomasz Figa <[email protected]> wrote:
> On Thursday 15 of August 2013 14:12:43 Lars Poeschel wrote:

> Well, there are basically no restrictions over the format of GPIO and
> interrupt specifiers. Any driver is free to define its own and provide
> private .xlate() callback to parse it. No assumptions should be made about
> the format, other than each GPIO/interrupt is specified by numer of cells
> specified in #interrupt- or #gpio-cells property of the controller.

Surely we can assume that the binding documents can be trusted?
And for this we only need this:
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

Yours,
Linus Walleij

2013-08-17 09:59:27

by Tomasz Figa

[permalink] [raw]
Subject: Re: [PATCH v2] RFC: interrupt consistency check for OF GPIO IRQs

[Ccing DT maintainers, as they may have some ideas as well]

On Saturday 17 of August 2013 02:16:11 Linus Walleij wrote:
> On Thu, Aug 15, 2013 at 11:53 AM, Tomasz Figa <[email protected]>
wrote:
> >> + /* Check if we have an IRQ parent, else continue */
> >> + irq_parent = of_irq_find_parent(child);
> >> + if (!irq_parent)
> >> + continue;
> >
> > You can probably put the irq_parent node here to not duplicate calls
> > in
> > both code paths below.
>
> But...
>
> >> + /* Is it so that this very GPIO chip is the parent? */
> >> + if (irq_parent != gcn) {
>
> I am using it here in this comparison.

You don't seem to be dereferencing it in this comparison.

> >> + of_node_put(irq_parent);
> >> + continue;
> >> + }
> >> + of_node_put(irq_parent);
>
> Then here I put it.
>
> >> + num_irq = of_irq_count(gcn);
> >> + for (i = 0; i < num_irq; i++) {
> >> + /*
> >> + * The first cell is always the local IRQ
> >> number,
> >
> > and
> >
> >> + * this corresponds to the GPIO line offset for
> >> a
> >
> > GPIO
> >
> >> + * chip.
> >> + *
> >> + * FIXME: take interrupt-cells into account
> >> here.
> >
> > This is the biggest problem of this patch. It assumes that there is
> > only a single and shared GPIO/interrupt specification scheme, which
> > might not be true.
> >
> > First of all, almost all interrupt bindings have an extra,
> > semi-generic
> > flags field as last cell in the specifier. Now there can be more than
> > one>
> > cell used to index GPIOs and interrupts, for example:
> > interrupts = <1 3 8>
> >
> > which could mean: bank 1, pin 3, low level triggered.
>
> You are right, but:
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> Specifies how to handle the one-celled and two-celled versions.
>
> And nothing else is specified. So it's not overly complex.

The documentation states that:

It is the responsibility of the interrupt controller's binding to
define the length and format of the interrupt specifier.

Then two _example_ formats follow, preceded by following statement:

The following two variants are commonly used:

I already know a variant which uses three (Exynos combiner) and four
(S3C24xx interrupt controller) cells. They are not pin controllers, but
you can't stop anyone from adopting similar or even more complex
specifiers formats for their hardware, especially when it matches more
closely the interrupt/pin layout used in their hardware.

> But we have to read out the #interrupt-cells specifier from the
> controller I guess, but since we the gcn pointer we can do this
> easily. (I'll fix...)
>
> > I think you may need to reuse a lot of the code that normally parses
> > the interrupts property, i.e. the irq_of_parse_and_map() path, which
> > will then give you the hwirq index inside your irq chip, which may
> > (or may not) be the same as the GPIO offset inside your GPIO chip.
>
> We don't need to map it, and that's the hard part of that code.
> We just need to add some code to check the number of
> cells.

Not really. Only the interrupt controller's driver (or rather its .xlate()
callback) knows how to map all the cells into hwirq index. So we must at
least get the irq domain associated with the node and call its .xlate()
callback with requested count of cells.

> > If you're unlucky enough to spot a controller that uses completely
> > different numbering schemes for GPIOs and interrupts, then you're
> > probably screwed, because only the driver for this controller can
> > know the mapping between them.
>
> But the binding documents state how this shall be done, as illustrated.
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt

No. Both specifier formats shown in the documentation are just examples.
As I already quoted, specifier format is up to the binding of particular
interrupt controller, without any restrictions.

> Documentation/devicetree/bindings/gpio/gpio.txt
>
> >> + if (ret)
> >> + pr_err("gpiolib OF: could not
> >
> > request IRQ GPIO %d (%d)\n",
> >
> >> + gc->base + offset,
> >> offset);>
> > Would some kind of error handling be a bad idea here? Like, for
> > example, marking this IRQ as invalid or something among these lines.
>
> Sorry I'm not following. The GPIO core has no clue of how the
> driver implements its irqchip and no handle on it so it cannot
> go in and mark any interrupts.

Well, this code already tries to map GPIO pins into interrupts. As I
explained above, you will need to get access to irq domain anyway, so it
shouldn't be a problem to do some operations on interrupts here.

But...

> >> + ret = gpio_direction_input(gc->base +
> >
> > offset);
> >
> >> + if (ret)
> >> + pr_err("gpiolib OF: could not
> >> set
> >
> > IRQ GPIO %d (%d) as input\n",
> >
> >> + gc->base + offset,
> >> offset);>
> > I'm not sure if this is the correct action if someone already
> > requested
> > the GPIO before and probably already set it up with their own set of
> > parameters (not necessarily the same as enforced by calling
> > gpio_direction_input()).
>
> What do you mean with " someone already requested the GPIO
> before" here?
>
> This code is run right after the gpiochip is added, noone has a
> chance of requesting anything before this happens. This is the
> general idea: steal all GPIOs marked to be used as interrupts
> so no other consumer can get at them.

...yes, I haven't noticed that this is called from gpiochip add context
and so gotten it wrong a bit. Please ignore the original comment
(including previous one as well, since it is unlikely that request_gpio()
will fail for us).

> >> + } else {
> >> + gpio_free(gc->base + offset);
> >
> > What if the request failed? This would mean calling gpio_free() on a
> > GPIO previously requested by someone else.
>
> Noone can request before us, as stated.

Right.

> >> +#define of_gpiochip_request_irq_lines(np, gc) \
> >> + of_gpiochip_x_irq_lines(np, gc, 1)
> >> +
> >> +#define of_gpiochip_free_irq_lines(np, gc) \
> >> + of_gpiochip_x_irq_lines(np, gc, 0)
> >
> > Aha, so the x is a wildcard. Fine, although it makes the code slightly
> > less reader-friendly IMHO.
>
> I renamed it a bit.

OK.

Best regards,
Tomasz

2013-08-19 19:35:30

by Stephen Warren

[permalink] [raw]
Subject: Re: [PATCH v2] RFC: interrupt consistency check for OF GPIO IRQs

On 08/17/2013 03:59 AM, Tomasz Figa wrote:
> [Ccing DT maintainers, as they may have some ideas as well]
>
> On Saturday 17 of August 2013 02:16:11 Linus Walleij wrote:
>> On Thu, Aug 15, 2013 at 11:53 AM, Tomasz Figa <[email protected]> wrote:
...

>>> This is the biggest problem of this patch. It assumes that there is
>>> only a single and shared GPIO/interrupt specification scheme, which
>>> might not be true.
>>>
>>> First of all, almost all interrupt bindings have an extra,
>>> semi-generic
>>> flags field as last cell in the specifier. Now there can be more than
>>> one>
>>> cell used to index GPIOs and interrupts, for example:
>>> interrupts = <1 3 8>
>>>
>>> which could mean: bank 1, pin 3, low level triggered.
>>
>> You are right, but:
>> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> Specifies how to handle the one-celled and two-celled versions.
>>
>> And nothing else is specified. So it's not overly complex.
>
> The documentation states that:
>
> It is the responsibility of the interrupt controller's binding to
> define the length and format of the interrupt specifier.
>
> Then two _example_ formats follow, preceded by following statement:
>
> The following two variants are commonly used:
>
> I already know a variant which uses three (Exynos combiner) and four
> (S3C24xx interrupt controller) cells. They are not pin controllers, but
> you can't stop anyone from adopting similar or even more complex
> specifiers formats for their hardware, especially when it matches more
> closely the interrupt/pin layout used in their hardware.

Yes, the binding doc interrupts.txt mentioned above does not specify
*the* one-/two-cell format, but *a* common/possible one- and two-cell
format. There's no strict reason that all interrupt controllers have to
use those exact formats. The only way to parse interrupt specifiers is
to ask the driver for the the interrupt controller code to parse the
property.

The same goes for GPIOs.

>> But we have to read out the #interrupt-cells specifier from the
>> controller I guess, but since we the gcn pointer we can do this
>> easily. (I'll fix...)
>>
>>> I think you may need to reuse a lot of the code that normally parses
>>> the interrupts property, i.e. the irq_of_parse_and_map() path, which
>>> will then give you the hwirq index inside your irq chip, which may
>>> (or may not) be the same as the GPIO offset inside your GPIO chip.
>>
>> We don't need to map it, and that's the hard part of that code.
>> We just need to add some code to check the number of
>> cells.
>
> Not really. Only the interrupt controller's driver (or rather its .xlate()
> callback) knows how to map all the cells into hwirq index. So we must at
> least get the irq domain associated with the node and call its .xlate()
> callback with requested count of cells.

Yes.

2013-08-21 13:21:34

by Lars Poeschel

[permalink] [raw]
Subject: Re: [PATCH v2] RFC: interrupt consistency check for OF GPIO IRQs

On Monday 19 August 2013 at 21:35:22, Stephen Warren wrote:
> On 08/17/2013 03:59 AM, Tomasz Figa wrote:
> > [Ccing DT maintainers, as they may have some ideas as well]
> >
> > On Saturday 17 of August 2013 02:16:11 Linus Walleij wrote:
> >> On Thu, Aug 15, 2013 at 11:53 AM, Tomasz Figa <[email protected]>
wrote:
> ...
>
> >>> This is the biggest problem of this patch. It assumes that there is
> >>> only a single and shared GPIO/interrupt specification scheme, which
> >>> might not be true.
> >>>
> >>> First of all, almost all interrupt bindings have an extra,
> >>> semi-generic
> >>> flags field as last cell in the specifier. Now there can be more
> >>> than one>
> >>>
> >>> cell used to index GPIOs and interrupts, for example:
> >>> interrupts = <1 3 8>
> >>>
> >>> which could mean: bank 1, pin 3, low level triggered.
> >>
> >> You are right, but:
> >> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> >> Specifies how to handle the one-celled and two-celled versions.
> >>
> >> And nothing else is specified. So it's not overly complex.
> >
> > The documentation states that:
> > It is the responsibility of the interrupt controller's binding to
> > define the length and format of the interrupt specifier.
> >
> > Then two _example_ formats follow, preceded by following statement:
> > The following two variants are commonly used:
> > I already know a variant which uses three (Exynos combiner) and four
> > (S3C24xx interrupt controller) cells. They are not pin controllers,
> > but you can't stop anyone from adopting similar or even more complex
> > specifiers formats for their hardware, especially when it matches
> > more closely the interrupt/pin layout used in their hardware.
>
> Yes, the binding doc interrupts.txt mentioned above does not specify
> *the* one-/two-cell format, but *a* common/possible one- and two-cell
> format. There's no strict reason that all interrupt controllers have to
> use those exact formats. The only way to parse interrupt specifiers is
> to ask the driver for the the interrupt controller code to parse the
> property.

I agree with you. I also understand the interrupts.txt binding doc as it
lists only a common/possible cell format, but it's purpose is not to
restrict it to this.

I send an updated patch, that uses the drivers xlate function to parse the
interrupt property in a few minutes.

Thanks for clarifying this,
Lars

2013-08-21 23:00:17

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH v2] RFC: interrupt consistency check for OF GPIO IRQs

On Wed, Aug 21, 2013 at 3:21 PM, Lars Poeschel <[email protected]> wrote:
> On Monday 19 August 2013 at 21:35:22, Stephen Warren wrote:
>> On 08/17/2013 03:59 AM, Tomasz Figa wrote:

>> > Then two _example_ formats follow, preceded by following statement:
>> > The following two variants are commonly used:
>> > I already know a variant which uses three (Exynos combiner) and four
>> > (S3C24xx interrupt controller) cells. They are not pin controllers,
>> > but you can't stop anyone from adopting similar or even more complex
>> > specifiers formats for their hardware, especially when it matches
>> > more closely the interrupt/pin layout used in their hardware.
>>
>> Yes, the binding doc interrupts.txt mentioned above does not specify
>> *the* one-/two-cell format, but *a* common/possible one- and two-cell
>> format. There's no strict reason that all interrupt controllers have to
>> use those exact formats. The only way to parse interrupt specifiers is
>> to ask the driver for the the interrupt controller code to parse the
>> property.
>
> I agree with you. I also understand the interrupts.txt binding doc as it
> lists only a common/possible cell format, but it's purpose is not to
> restrict it to this.
>
> I send an updated patch, that uses the drivers xlate function to parse the
> interrupt property in a few minutes.

Thanks that you take this on a spin, I've been busy...

(I also agree with the other comments here, let's see if we can
figure this out!)

Yours,
Linus Walleij