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 <linus.wall...@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_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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
