On 08/17/2013 01:54 AM, Linus Walleij 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.
>
> 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.
>
> Cc: devicetree@xxxxxxxxxxxxxxx
> Cc: Javier Martinez Canillas <javier.martinez@xxxxxxxxxxxxxxx>
> Cc: Enric Balletbo i Serra <eballetbo@xxxxxxxxx>
> Cc: Grant Likely <grant.likely@xxxxxxxxxx>
> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@xxxxxxxxxxxx>
> Cc: Santosh Shilimkar <santosh.shilimkar@xxxxxx>
> Cc: Kevin Hilman <khilman@xxxxxxxxxx>
> Cc: Balaji T K <balajitk@xxxxxx>
> Cc: Tony Lindgren <tony@xxxxxxxxxxx>
> Cc: Jon Hunter <jgchunter@xxxxxxxxx>
> Signed-off-by: Lars Poeschel <poeschel@xxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> drivers/gpio/gpiolib-of.c | 161 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 160 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 665f953..216e939 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,161 @@ int of_gpio_simple_xlate(struct gpio_chip *gc,
> EXPORT_SYMBOL(of_gpio_simple_xlate);
>
> /**
> + * 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
> + * @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,
> + const struct gpio_chip * const gc,
> + bool 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_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 %s references GPIO %d (%d)\n",
> + child->name, gc->base + offset, 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, child->name);
> + if (ret)
> + pr_err("gpiolib OF: could not request IRQ GPIO %d (%d) for node %s (%d)\n",
> + gc->base + offset, offset, child->name, ret);
> + ret = gpio_direction_input(gc->base + offset);
> + if (ret)
> + pr_err("gpiolib OF: could not set IRQ GPIO %d (%d) as input for node %s (%d)\n",
> + gc->base + offset, offset, child->name, ret);
> + } else {
> + gpio_free(gc->base + offset);
> + }
> + }
> + }
> +}
> +
> +/**
> + * 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;
> +
> + /*
> + * 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_irq_lines(root, np, gc, request);
> + of_node_put(root);
> +}
> +
> +#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
> * @mm_gc: pointer to the of_mm_gpio_chip allocated structure
> @@ -170,6 +325,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 +388,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)
>
Hi Linus,
Sorry for being out of the loop, I was pretty busy last weeks. I just tested
your patch on my OMAP3 board and it solves the issue we had in OMAP.
Thanks a lot for solving this long standing issue!
Tested-by: Javier Martinez Canillas <javier.martinez@xxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
