On 08/21/2013 03:49 PM, Tomasz Figa wrote:
> Hi Lars, Linus,
>
> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
>> From: Linus Walleij <linus.walleij@xxxxxxxxxx>
>>
>> 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.
That last point should simply be taken care of by the IRQ driver in the
relevant callbacks.
>> 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,
It seems a little odd to solve this only for DT. What about the non-DT case?
>> 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.
What about bindings that require a GPIO to be specified, yet don't allow
an IRQ to be specified, and the driver internally does perform
gpio_to_irq() on it? I don't think one can detect that case.
Isn't it better to have the IRQ chip's .request() operation convert the
IRQ to a GPIO if relevant (which it can do since it has specific
knowledge of the HW) and take ownership of the GPIO at that level?
That would cover both the exceptions I pointed out above.
I vaguely recall seeing patches along those lines before, but there must
have been some other problem pointed out...
>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>> +static void of_gpio_scan_irq_lines(const struct device_node *const
>> + for (i = 0; i < intlen; i += intsize) {
>> + /*
>> + * Find out the local IRQ number. This corresponds to
>> + * the GPIO line offset for a GPIO chip.
>
> I'm still not convinced that this assumption is correct. This code will
> behave erraticaly in cases where it is not true, requesting innocent GPIO
> pins.
>
>> + */
>> + if (irq_domain && irq_domain->ops->xlate)
>> + irq_domain->ops->xlate(irq_domain, gcn,
>> + intspec + i, intsize,
>> + &hwirq, &type);
>> + else
>> + hwirq = intspec[0];
>
> Is it a correct fallback when irq_domain is NULL?
Indeed this fallback is dangerous. The /only/ way to parse an IRQ
specifier is with binding-specific knowledge, which is obtained by
calling irq_domain->ops->xlate(). If the IRQ domain can't be found, this
operation simply has to be deferred; we can't just guess and hope.
>
>> +
>> + hwirq = be32_to_cpu(hwirq);
>
> Is this conversion correct? I don't think hwirq could be big endian here
> (unless running on a big endian CPU).
I think that should be inside the else branch above.
--
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
