[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Grant Likely]

On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 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.

Ugh, that's pretty awful, and it doesn't actually solve the root
problem of the GPIO and IRQ subsystems not cooperating. It's also a
very DT-centric solution even though we're going to see the exact same
issue on ACPI machines.

We have to solve the problem in a better way than that. Rearranging
your patch description, here are some of the points you brought up so
I can comment on them...

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

If a gpio line is being both requested as a gpio and used as an
interrupt line, then either a) it's a bug, or b) the gpio line needs
to be used as input only so it is compatible with irq usage. b) should
be supportable.

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

Surely we can fix this. I still don't see a problem of having the
controller request the gpio when it is claimed as an irq if we can get
around the problem of another user performing a /valid/ request on the
same GPIO line. The solution may be to have a special form of request
or flag that allows it to be shared.

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

Should also be solvable if the gpio request problem is solved.

g.

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> 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.
>
> Ugh, that's pretty awful, and it doesn't actually solve the root
> problem of the GPIO and IRQ subsystems not cooperating. It's also a
> very DT-centric solution even though we're going to see the exact same
> issue on ACPI machines.

The problem is that the patches for OMAP that I applied
and now have had to revert solves it in an even uglier way,
leading to breaking boards, as was noticed.

The approach in this patch has the potential to actually
work without regressing a bunch of boards...

Whether this is a problem in ACPI or not remains to be seen,
but I'm not sure about that. Device trees allows for a GPIO line
to be used as an interrupt source and GPIO line orthogonally,
and that is the root of this problem. Does ACPI have the same
problem, or does it impose natural restrictions on such use
cases?

> We have to solve the problem in a better way than that. Rearranging
> your patch description, here are some of the points you brought up so
> I can comment on them...
>
>> 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.
>
> If a gpio line is being both requested as a gpio and used as an
> interrupt line, then either a) it's a bug, or b) the gpio line needs
> to be used as input only so it is compatible with irq usage. b) should
> be supportable.

Yes this is what I'm saying too I think...

The bug in (a) manifested itself in the OMAP patch with
no real solution in sight.

>> - The GPIO debugfs file claims this GPIO line is "free".
>
> Surely we can fix this. I still don't see a problem of having the
> controller request the gpio when it is claimed as an irq if we can get
> around the problem of another user performing a /valid/ request on the
> same GPIO line. The solution may be to have a special form of request
> or flag that allows it to be shared.

I don't see how sharing works here, or how another user,
i.e. another one than the user wanting to recieve the IRQ,
can validly request such a line? What would the usecase for
that valid request be?

Basically I believe these two things need to be exclusive in
the DT world:

A: request_irq(a resource passed from "interrupts");
     -> core implicitly performs gpio_request()
         gpio_direction_input()

B: gpio_request(a resource passed from "gpios");
     gpio_direction_input()
     request_irq(gpio_to_irq())

Never both. And IIUC that was what happened in the
OMAP case.

>> - 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.
>
> Should also be solvable if the gpio request problem is solved.

Agreed...

Yours,
Linus Walleij

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 07/31/2013 01:44 AM, Linus Walleij wrote:
> On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote:
>> On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> 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.
>>
>> Ugh, that's pretty awful, and it doesn't actually solve the root
>> problem of the GPIO and IRQ subsystems not cooperating. It's also a
>> very DT-centric solution even though we're going to see the exact same
>> issue on ACPI machines.
> 
> The problem is that the patches for OMAP that I applied
> and now have had to revert solves it in an even uglier way,
> leading to breaking boards, as was noticed.
> 
> The approach in this patch has the potential to actually
> work without regressing a bunch of boards...
>
> Whether this is a problem in ACPI or not remains to be seen,
> but I'm not sure about that. Device trees allows for a GPIO line
> to be used as an interrupt source and GPIO line orthogonally,
> and that is the root of this problem. Does ACPI have the same
> problem, or does it impose natural restrictions on such use
> cases?
>

I agree with Linus here. The problem is that GPIO controllers that can work as
IRQ sources are treated in the kernel as if there where two separate controlers
that are rather orthogonal: an irq_chip and a gpio_chip.
But DT allows to use a GPIO line as an IRQ just by using an omap-gpio phandle as
"interrupt-parent".

So, there should be a place where both irq_chip and gpio_chip has to be related
somehow to properly configure a GPIO (request it and setting it as input) when
used as an IRQ by DT.

My patch for OMAP used an irq_domain_ops .map function handler to configure the
GPIO when a IRQ was mapped since that seemed to me as the best place to do it.
This worked well in OMAP2+ platforms but unfortunately broke OMAP1 platforms
since they are still using legacy domain mapping thus not call .map.

Linus' approach is much better IMHO since it is both generic and will only try
to setup an GPIO when a GPIO bank is added by the DT core. So checking there if
a GPIO line is being used as an IRQ and configuring the GPIO seems like a good
solution to me.

Old platforms like OMAP1 won't break since they don't (and probably never will)
have DT support.

>> We have to solve the problem in a better way than that. Rearranging
>> your patch description, here are some of the points you brought up so
>> I can comment on them...
>>
>>> 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.
>>
>> If a gpio line is being both requested as a gpio and used as an
>> interrupt line, then either a) it's a bug, or b) the gpio line needs
>> to be used as input only so it is compatible with irq usage. b) should
>> be supportable.
> 
> Yes this is what I'm saying too I think...
> 
> The bug in (a) manifested itself in the OMAP patch with
> no real solution in sight.
> 
>>> - The GPIO debugfs file claims this GPIO line is "free".
>>
>> Surely we can fix this. I still don't see a problem of having the
>> controller request the gpio when it is claimed as an irq if we can get
>> around the problem of another user performing a /valid/ request on the
>> same GPIO line. The solution may be to have a special form of request
>> or flag that allows it to be shared.
> 
> I don't see how sharing works here, or how another user,
> i.e. another one than the user wanting to recieve the IRQ,
> can validly request such a line? What would the usecase for
> that valid request be?
> 
> Basically I believe these two things need to be exclusive in
> the DT world:
> 
> A: request_irq(a resource passed from "interrupts");
>      -> core implicitly performs gpio_request()
>          gpio_direction_input()
> 
> B: gpio_request(a resource passed from "gpios");
>      gpio_direction_input()
>      request_irq(gpio_to_irq())
> 
> Never both. And IIUC that was what happened in the
> OMAP case.
>

Actually the OMAP case was not related to resource conflict but I'll explain the
issues we had with the OMAP patch in a moment. First I just want to say that I
don't think that makes sense that there are two users of a GPIO. There *could*
be two users for the same IRQ that is mapped to a GPIO but in that case there
will be only one user of the GPIO and that will be the IRQ controller even when
in practice is the same chip and the same driver. If the same GPIO line is used
for two devices in the DT, then gpio_request_one(gpio, GPIOF_IN, NULL) should be
called just once and that state has to be saved somewhere so whoever setups the
GPIO won't do it twice for the same IRQ.

But if a GPIO line is mapped as an IRQ and another user calls gpio_request()
then is correct that he gets an -EBUSY.

There are two issues that have to be solved at least for the OMAP case but they
seems to be general:

1) When a GPIO-IRQ mapping has to be created. e.g: call to irq_create_mapping()
2) When a GPIO-IRQ is setup.e.g: calling irq_set_chip_{data,and_handler,etc}()
3) When a GPIO line has to be requested and configured as input. e.g: call
gpio_request_one(gpio, GPIOF_IN, NULL)

For 1) in the DT case we don't need an explicit call to irq_create_mapping()
since it is called from irq_create_of_mapping() when an IRQ is mapped while
these explicit calls are needed for platforms that still use legacy boot.

The problem is that board files and drivers that has not not been completed
migrated to DT assumes (at least for OMAP) that *every* GPIO line is mapped as
an IRQ and they just do:

gpio_request(gpio,...);
gpio_direction_input()
request[_threaded]_irq(gpio_to_irq(gpio), ...);

My patch-set changed the gpio-omap driver to not map all GPIO lines but only the
ones that were really used as an IRQ and let the DT core to do the mapping from
irq_create_of_mapping(). The first problem reported with the OMAP patch was that
a driver was using the above sequence and that the GPIO had not been mapped.
This user was booting with DT and so this showed a bug in the driver and a DT
that did not conform with the standard schema used in mainline but this shows a
potential issue.

For 2) I moved all the GPIO-IRQ setup logic to the .map function handler since I
(wrongly) thought that .map will be called either when an explicit
irq_create_mapping() call was made and when the OF core handled it via
irq_create_of_mapping(). This was certainly true for OMAP2+ platforms and such
DT and legacy boot worked on my tests.

But it broke OMAP1 platforms since as I said still use legacy booting and thus
don't call the .map function handler. This was the reason why the patches got
reverted and not a resource conflict.

For 3) my patch called the gpio_request_one(gpio, GPIOF_IN, NULL) only if
chip->of_node was defined in the irqchip .map function handler.

But as we saw, this approach was a completely mess and we had to revert three
patches and bother Linus W on his holidays :)

So, I don't really know how to solve this issue and it seems I did more harm
than good when trying to do it. I think that Linus' approach is the least bad
solution but it seems to me that it just solves 3) but no 1) and 2).

>>> - 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.
>>
>> Should also be solvable if the gpio request problem is solved.
> 
> Agreed...
>

+1

> Yours,
> Linus Walleij
> 

Thanks a lot and best regards,
Javier

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 31.07.2013 10:35, schrieb Javier Martinez Canillas:

>
> The problem is that board files and drivers that has not not been completed
> migrated to DT assumes (at least for OMAP) that *every* GPIO line is mapped as
> an IRQ and they just do:
>
> gpio_request(gpio,...);
> gpio_direction_input()
> request[_threaded]_irq(gpio_to_irq(gpio), ...);
>
> My patch-set changed the gpio-omap driver to not map all GPIO lines but only the
> ones that were really used as an IRQ and let the DT core to do the mapping from
> irq_create_of_mapping(). The first problem reported with the OMAP patch was that
> a driver was using the above sequence and that the GPIO had not been mapped.
> This user was booting with DT and so this showed a bug in the driver and a DT
> that did not conform with the standard schema used in mainline but this shows a
> potential issue.

There must have been a bug in the patch too. I've also added that 
iinterrupt-parent stuff (with the same flags as used by the driver) and 
just have let the driver call

request_threaded_irq(gpio_to_irq(gpio), flags);

without the gpio_request()/input() before. And request_threaded_irq() 
returned -EBUSY. I'm pretty sure nothing else did use that gpio, but I 
haven't looked at why request_threaded_irq() returned -EBUSY. I assume 
the new mapping stuff did reserve the irq in such a way, that the driver 
couldn't request the IRQ.

Otherwise I wouldn't have had a problem by just adding the necessary 
entries to the DT.

But I have to say I didn't like the syntax too, and it wasn't obvious 
how the syntax is and how to conclude from a gpio number to an 
irq-number and the patch didn't really include some documentation or 
useful example.

Regards,

Alexander Holler

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 02.08.2013 11:57, schrieb Alexander Holler:

> There must have been a bug in the patch too. I've also added that
> iinterrupt-parent stuff (with the same flags as used by the driver) and
> just have let the driver call
> 
> request_threaded_irq(gpio_to_irq(gpio), flags);
> 
> without the gpio_request()/input() before. And request_threaded_irq()

Not to forget that it would/will be a big problem, if you will forbid
drivers to use gpio_request() when they use the gpio as irq (as it
was/is necessary now).

With my kernel 3.10.4 with some patches on top, I see
# git grep gpio_to_irq | wc -l
678

So there are a lot of places where

gpio_request();
gpio_direction_input()
request_irq();

would have been changed to

#ifdef CONFIG_OF_GPIO
gpio_request();
gpio_direction_input()
#endif
request_irq();

to not fail during runtime (hopefully with some descriptive error
message if it fails).

Regards,

Alexander Holler

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 02.08.2013 17:35, schrieb Alexander Holler:
> Am 02.08.2013 11:57, schrieb Alexander Holler:
> 
>> There must have been a bug in the patch too. I've also added that
>> iinterrupt-parent stuff (with the same flags as used by the driver) and
>> just have let the driver call
>>
>> request_threaded_irq(gpio_to_irq(gpio), flags);
>>
>> without the gpio_request()/input() before. And request_threaded_irq()

Thinking again about what I've tried, I might have make the failure to
add the interrupt-parent line to the mmc-section (where the gpio was
specified) and not to the gpio(-controller) section where it might have
belong too (again, there was no example and no documentation).

But why I again bother you all with another mail, is that I don't like
the fact that the irq-flags might have to be specified in the DT and the
source (and they have to match).

A solution might be to use the flags specified in the DT for the
particular irq and ignore the flags specified as parameter to
request_irq(). At least as long as DT isn't mandatory. And in the
future, the flags could just be removed from the call to request_irq()
depending (then visible too) only on the flags as specified in the DT.

Regards,

Alexander Holler

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Joel Fernandes]

On 07/31/2013 03:35 AM, Javier Martinez Canillas wrote:
> On 07/31/2013 01:44 AM, Linus Walleij wrote:
>> On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote:
>>> On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> 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.
>>>
>>> Ugh, that's pretty awful, and it doesn't actually solve the root
>>> problem of the GPIO and IRQ subsystems not cooperating. It's also a
>>> very DT-centric solution even though we're going to see the exact same
>>> issue on ACPI machines.
>>
>> The problem is that the patches for OMAP that I applied
>> and now have had to revert solves it in an even uglier way,
>> leading to breaking boards, as was noticed.
>>
>> The approach in this patch has the potential to actually
>> work without regressing a bunch of boards...
>>
>> Whether this is a problem in ACPI or not remains to be seen,
>> but I'm not sure about that. Device trees allows for a GPIO line
>> to be used as an interrupt source and GPIO line orthogonally,
>> and that is the root of this problem. Does ACPI have the same
>> problem, or does it impose natural restrictions on such use
>> cases?
>>
> 
> I agree with Linus here. The problem is that GPIO controllers that can work as
> IRQ sources are treated in the kernel as if there where two separate controlers
> that are rather orthogonal: an irq_chip and a gpio_chip.
> But DT allows to use a GPIO line as an IRQ just by using an omap-gpio phandle as
> "interrupt-parent".
> 
> So, there should be a place where both irq_chip and gpio_chip has to be related
> somehow to properly configure a GPIO (request it and setting it as input) when
> used as an IRQ by DT.
> 
> My patch for OMAP used an irq_domain_ops .map function handler to configure the
> GPIO when a IRQ was mapped since that seemed to me as the best place to do it.
> This worked well in OMAP2+ platforms but unfortunately broke OMAP1 platforms
> since they are still using legacy domain mapping thus not call .map.

Just wondering- why .map not called for omap1? irq_create_mapping does seem to
call  -> irq_domain_associate which calls map function. For omap case, GPIO
driver does call irq_create_mapping, just like omap2+ no?

Further, if for any reason the .map is not called. Can you not call gpio_request
yourself direct in omap_gpio_chip_init function?

Does it really matter if you call gpio_request from .map or from the chip_init
function?

Also on a different note.. this would call gpio_request for *every* gpio line,
but isn't that what your original patch that got reverted was doing in
omap_gpio_chip_init:

+       if (!bank->chip.of_node)
+               for (j = 0; j < bank->width; j++)
+                       irq_create_mapping(bank->domain, j);

Just trying to understand your initial patch better.

Regards,

-Joel

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/10/2013 09:00 AM, Joel Fernandes wrote:
> On 07/31/2013 03:35 AM, Javier Martinez Canillas wrote:
>> On 07/31/2013 01:44 AM, Linus Walleij wrote:
>>> On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote:
>>>> On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>> 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.
>>>>
>>>> Ugh, that's pretty awful, and it doesn't actually solve the root
>>>> problem of the GPIO and IRQ subsystems not cooperating. It's also a
>>>> very DT-centric solution even though we're going to see the exact same
>>>> issue on ACPI machines.
>>>
>>> The problem is that the patches for OMAP that I applied
>>> and now have had to revert solves it in an even uglier way,
>>> leading to breaking boards, as was noticed.
>>>
>>> The approach in this patch has the potential to actually
>>> work without regressing a bunch of boards...
>>>
>>> Whether this is a problem in ACPI or not remains to be seen,
>>> but I'm not sure about that. Device trees allows for a GPIO line
>>> to be used as an interrupt source and GPIO line orthogonally,
>>> and that is the root of this problem. Does ACPI have the same
>>> problem, or does it impose natural restrictions on such use
>>> cases?
>>>
>> 
>> I agree with Linus here. The problem is that GPIO controllers that can work as
>> IRQ sources are treated in the kernel as if there where two separate controlers
>> that are rather orthogonal: an irq_chip and a gpio_chip.
>> But DT allows to use a GPIO line as an IRQ just by using an omap-gpio phandle as
>> "interrupt-parent".
>> 
>> So, there should be a place where both irq_chip and gpio_chip has to be related
>> somehow to properly configure a GPIO (request it and setting it as input) when
>> used as an IRQ by DT.
>> 
>> My patch for OMAP used an irq_domain_ops .map function handler to configure the
>> GPIO when a IRQ was mapped since that seemed to me as the best place to do it.
>> This worked well in OMAP2+ platforms but unfortunately broke OMAP1 platforms
>> since they are still using legacy domain mapping thus not call .map.
> 
> Just wondering- why .map not called for omap1? irq_create_mapping does seem to
> call  -> irq_domain_associate which calls map function. For omap case, GPIO
> driver does call irq_create_mapping, just like omap2+ no?
>

That is what I understood too when writing the patch but I remember someone
mentioning legacy domain mapping not calling the .map function handler as a
possible cause for the OMAP1 regression and since Linus decided to revert the
patches in favor of a more general solution I didn't care to check if that was
true or not. Now looking at irq_create_mapping() I see that my assumption was
correct so I don't know what was the bug that caused the OMAP1 regression.

> Further, if for any reason the .map is not called. Can you not call gpio_request
> yourself direct in omap_gpio_chip_init function?
> 

No, since you can't request a GPIO for all GPIO pins in the bank. Users have to
do it explicitly (or implicitly in the case of GPIO mapped as IRQ in DT).

> Does it really matter if you call gpio_request from .map or from the chip_init
> function?
> 

Yes it does, because in DT the core calls irq_create_of_mapping() ->
irq_create_mapping() -> .map(). That way only are requested the GPIO pins that
are mapped as IRQ and not all of them.

> Also on a different note.. this would call gpio_request for *every* gpio line,
> but isn't that what your original patch that got reverted was doing in
> omap_gpio_chip_init:
> 
> +       if (!bank->chip.of_node)
> +               for (j = 0; j < bank->width; j++)
> +                       irq_create_mapping(bank->domain, j);
> 

No it won't. This is only needed for the legacy (non-DT) boot since no one calls
irq_create_mapping() so it has to be called explicitly.

And in that case .map will be called but gpio_request() won't since the call is
made only when bank->chip.of_node is not NULL.

> Just trying to understand your initial patch better.
> 
> Regards,
> 
> -Joel
> 

Best regards,
Javier

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Joel Fernandes]

On 09/10/2013 08:17 AM, Javier Martinez Canillas wrote:
> On 09/10/2013 09:00 AM, Joel Fernandes wrote:
>> On 07/31/2013 03:35 AM, Javier Martinez Canillas wrote:
>>> On 07/31/2013 01:44 AM, Linus Walleij wrote:
>>>> On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote:
>>>>> On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>> 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.
>>>>>
>>>>> Ugh, that's pretty awful, and it doesn't actually solve the root
>>>>> problem of the GPIO and IRQ subsystems not cooperating. It's also a
>>>>> very DT-centric solution even though we're going to see the exact same
>>>>> issue on ACPI machines.
>>>>
>>>> The problem is that the patches for OMAP that I applied
>>>> and now have had to revert solves it in an even uglier way,
>>>> leading to breaking boards, as was noticed.
>>>>
>>>> The approach in this patch has the potential to actually
>>>> work without regressing a bunch of boards...
>>>>
>>>> Whether this is a problem in ACPI or not remains to be seen,
>>>> but I'm not sure about that. Device trees allows for a GPIO line
>>>> to be used as an interrupt source and GPIO line orthogonally,
>>>> and that is the root of this problem. Does ACPI have the same
>>>> problem, or does it impose natural restrictions on such use
>>>> cases?
>>>>
>>>
>>> I agree with Linus here. The problem is that GPIO controllers that can work as
>>> IRQ sources are treated in the kernel as if there where two separate controlers
>>> that are rather orthogonal: an irq_chip and a gpio_chip.
>>> But DT allows to use a GPIO line as an IRQ just by using an omap-gpio phandle as
>>> "interrupt-parent".
>>>
>>> So, there should be a place where both irq_chip and gpio_chip has to be related
>>> somehow to properly configure a GPIO (request it and setting it as input) when
>>> used as an IRQ by DT.
>>>
>>> My patch for OMAP used an irq_domain_ops .map function handler to configure the
>>> GPIO when a IRQ was mapped since that seemed to me as the best place to do it.
>>> This worked well in OMAP2+ platforms but unfortunately broke OMAP1 platforms
>>> since they are still using legacy domain mapping thus not call .map.
>>
>> Just wondering- why .map not called for omap1? irq_create_mapping does seem to
>> call  -> irq_domain_associate which calls map function. For omap case, GPIO
>> driver does call irq_create_mapping, just like omap2+ no?
>>
> 
> That is what I understood too when writing the patch but I remember someone
> mentioning legacy domain mapping not calling the .map function handler as a
> possible cause for the OMAP1 regression and since Linus decided to revert the
> patches in favor of a more general solution I didn't care to check if that was
> true or not. Now looking at irq_create_mapping() I see that my assumption was
> correct so I don't know what was the bug that caused the OMAP1 regression.

Only stuff you deleted from the chip_init function was:

-       for (j = 0; j < bank->width; j++) {
-               int irq = irq_create_mapping(bank->domain, j);
-               irq_set_lockdep_class(irq, &gpio_lock_class);
-               irq_set_chip_data(irq, bank);
-               if (bank->is_mpuio) {
-                       omap_mpuio_alloc_gc(bank, irq, bank->width);
-               } else {
-                       irq_set_chip_and_handler(irq, &gpio_irq_chip,
-                                                handle_simple_irq);
-                       set_irq_flags(irq, IRQF_VALID);
-               }

and you moved all of it to the .map function in your patch. Not sure what could
be breaking OMAP1 cases.
You could potentially add that back with some #ifdef for OMAP1?

Either way, map should be called looks like. If its not called, then the above
block can be explicity called for OMAP1 case in omap_chip_gpio_init.

What was strange is one person reported that mappings were not created for
OMAP1. But I am wondering what you changed could result in not creating that
mapping. Really nothing..

I think your initial patch is much better than fixing up DT but then I may be
missing other problems with your patch that Linus's patch addresses.

>> Further, if for any reason the .map is not called. Can you not call gpio_request
>> yourself direct in omap_gpio_chip_init function?
>>
> 
> No, since you can't request a GPIO for all GPIO pins in the bank. Users have to
> do it explicitly (or implicitly in the case of GPIO mapped as IRQ in DT).

Ah since you split the patch up into 2, I missed the gpio_request stuff. Ok,
that makes sense.

>> Does it really matter if you call gpio_request from .map or from the chip_init
>> function?
>>
> 
> Yes it does, because in DT the core calls irq_create_of_mapping() ->
> irq_create_mapping() -> .map(). That way only are requested the GPIO pins that
> are mapped as IRQ and not all of them.

>> Also on a different note.. this would call gpio_request for *every* gpio line,
>> but isn't that what your original patch that got reverted was doing in
>> omap_gpio_chip_init:
>>
>> +       if (!bank->chip.of_node)
>> +               for (j = 0; j < bank->width; j++)
>> +                       irq_create_mapping(bank->domain, j);
>>
> 
> No it won't. This is only needed for the legacy (non-DT) boot since no one calls
> irq_create_mapping() so it has to be called explicitly.
> 
> And in that case .map will be called but gpio_request() won't since the call is
> made only when bank->chip.of_node is not NULL.

Ok, thanks for the explanation. That makes sense to me.

Regards,

-Joel

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/10/2013 05:00 PM, Joel Fernandes wrote:
> On 09/10/2013 08:17 AM, Javier Martinez Canillas wrote:
>> On 09/10/2013 09:00 AM, Joel Fernandes wrote:
>>> On 07/31/2013 03:35 AM, Javier Martinez Canillas wrote:
>>>> On 07/31/2013 01:44 AM, Linus Walleij wrote:
>>>>> On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote:
>>>>>> On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>>> 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.
>>>>>>
>>>>>> Ugh, that's pretty awful, and it doesn't actually solve the root
>>>>>> problem of the GPIO and IRQ subsystems not cooperating. It's also a
>>>>>> very DT-centric solution even though we're going to see the exact same
>>>>>> issue on ACPI machines.
>>>>>
>>>>> The problem is that the patches for OMAP that I applied
>>>>> and now have had to revert solves it in an even uglier way,
>>>>> leading to breaking boards, as was noticed.
>>>>>
>>>>> The approach in this patch has the potential to actually
>>>>> work without regressing a bunch of boards...
>>>>>
>>>>> Whether this is a problem in ACPI or not remains to be seen,
>>>>> but I'm not sure about that. Device trees allows for a GPIO line
>>>>> to be used as an interrupt source and GPIO line orthogonally,
>>>>> and that is the root of this problem. Does ACPI have the same
>>>>> problem, or does it impose natural restrictions on such use
>>>>> cases?
>>>>>
>>>>
>>>> I agree with Linus here. The problem is that GPIO controllers that can work as
>>>> IRQ sources are treated in the kernel as if there where two separate controlers
>>>> that are rather orthogonal: an irq_chip and a gpio_chip.
>>>> But DT allows to use a GPIO line as an IRQ just by using an omap-gpio phandle as
>>>> "interrupt-parent".
>>>>
>>>> So, there should be a place where both irq_chip and gpio_chip has to be related
>>>> somehow to properly configure a GPIO (request it and setting it as input) when
>>>> used as an IRQ by DT.
>>>>
>>>> My patch for OMAP used an irq_domain_ops .map function handler to configure the
>>>> GPIO when a IRQ was mapped since that seemed to me as the best place to do it.
>>>> This worked well in OMAP2+ platforms but unfortunately broke OMAP1 platforms
>>>> since they are still using legacy domain mapping thus not call .map.
>>>
>>> Just wondering- why .map not called for omap1? irq_create_mapping does seem to
>>> call  -> irq_domain_associate which calls map function. For omap case, GPIO
>>> driver does call irq_create_mapping, just like omap2+ no?
>>>
>> 
>> That is what I understood too when writing the patch but I remember someone
>> mentioning legacy domain mapping not calling the .map function handler as a
>> possible cause for the OMAP1 regression and since Linus decided to revert the
>> patches in favor of a more general solution I didn't care to check if that was
>> true or not. Now looking at irq_create_mapping() I see that my assumption was
>> correct so I don't know what was the bug that caused the OMAP1 regression.
> 
> Only stuff you deleted from the chip_init function was:
> 
> -       for (j = 0; j < bank->width; j++) {
> -               int irq = irq_create_mapping(bank->domain, j);
> -               irq_set_lockdep_class(irq, &gpio_lock_class);
> -               irq_set_chip_data(irq, bank);
> -               if (bank->is_mpuio) {
> -                       omap_mpuio_alloc_gc(bank, irq, bank->width);
> -               } else {
> -                       irq_set_chip_and_handler(irq, &gpio_irq_chip,
> -                                                handle_simple_irq);
> -                       set_irq_flags(irq, IRQF_VALID);
> -               }
> 
> and you moved all of it to the .map function in your patch. Not sure what could
> be breaking OMAP1 cases.
> You could potentially add that back with some #ifdef for OMAP1?
> 
> Either way, map should be called looks like. If its not called, then the above
> block can be explicity called for OMAP1 case in omap_chip_gpio_init.
> 
> What was strange is one person reported that mappings were not created for
> OMAP1. But I am wondering what you changed could result in not creating that
> mapping. Really nothing..
> 
> I think your initial patch is much better than fixing up DT but then I may be
> missing other problems with your patch that Linus's patch addresses.
> 
>>> Further, if for any reason the .map is not called. Can you not call gpio_request
>>> yourself direct in omap_gpio_chip_init function?
>>>
>> 
>> No, since you can't request a GPIO for all GPIO pins in the bank. Users have to
>> do it explicitly (or implicitly in the case of GPIO mapped as IRQ in DT).
> 
> Ah since you split the patch up into 2, I missed the gpio_request stuff. Ok,
> that makes sense.
> 
>>> Does it really matter if you call gpio_request from .map or from the chip_init
>>> function?
>>>
>> 
>> Yes it does, because in DT the core calls irq_create_of_mapping() ->
>> irq_create_mapping() -> .map(). That way only are requested the GPIO pins that
>> are mapped as IRQ and not all of them.
> 
>>> Also on a different note.. this would call gpio_request for *every* gpio line,
>>> but isn't that what your original patch that got reverted was doing in
>>> omap_gpio_chip_init:
>>>
>>> +       if (!bank->chip.of_node)
>>> +               for (j = 0; j < bank->width; j++)
>>> +                       irq_create_mapping(bank->domain, j);
>>>
>> 
>> No it won't. This is only needed for the legacy (non-DT) boot since no one calls
>> irq_create_mapping() so it has to be called explicitly.
>> 
>> And in that case .map will be called but gpio_request() won't since the call is
>> made only when bank->chip.of_node is not NULL.
> 
> Ok, thanks for the explanation. That makes sense to me.
>

I'm glad that it helped to you to better understand the approach but you
shouldn't spend time on this since Linus W had made very clear that he doesn't
want a local solution that would be replicated on each platform since this is
not an OMAP only issue.

If you are interested in this problem you should joining the thread "Re: [PATCH
v3] gpio: interrupt consistency check for OF GPIO IRQs" [1] were is currently
being discussed this approach.

It turns out that many developers don't agree that this is the right solution
neither since the patch only solves a part of the problem. That we should try to
fix both the DT and legacy non-DT cases (i.e: doing explicit calls to gpilib
functions to setup the GPIO). And also take into account drivers that request
both the GPIO pin and the mapped IRQ.

> Regards,
> 
> -Joel
> 

Best regards,
javier

[1]: http://www.spinics.net/lists/kernel/msg1599899.html

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Joel Fernandes]

On 09/10/2013 10:48 AM, Javier Martinez Canillas wrote:
> On 09/10/2013 05:00 PM, Joel Fernandes wrote:
>> On 09/10/2013 08:17 AM, Javier Martinez Canillas wrote:
>>> On 09/10/2013 09:00 AM, Joel Fernandes wrote:
>>>> On 07/31/2013 03:35 AM, Javier Martinez Canillas wrote:
>>>>> On 07/31/2013 01:44 AM, Linus Walleij wrote:
>>>>>> On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote:
>>>>>>> On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>>>> 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.
>>>>>>>
>>>>>>> Ugh, that's pretty awful, and it doesn't actually solve the root
>>>>>>> problem of the GPIO and IRQ subsystems not cooperating. It's also a
>>>>>>> very DT-centric solution even though we're going to see the exact same
>>>>>>> issue on ACPI machines.
>>>>>>
>>>>>> The problem is that the patches for OMAP that I applied
>>>>>> and now have had to revert solves it in an even uglier way,
>>>>>> leading to breaking boards, as was noticed.
>>>>>>
>>>>>> The approach in this patch has the potential to actually
>>>>>> work without regressing a bunch of boards...
>>>>>>
>>>>>> Whether this is a problem in ACPI or not remains to be seen,
>>>>>> but I'm not sure about that. Device trees allows for a GPIO line
>>>>>> to be used as an interrupt source and GPIO line orthogonally,
>>>>>> and that is the root of this problem. Does ACPI have the same
>>>>>> problem, or does it impose natural restrictions on such use
>>>>>> cases?
>>>>>>
>>>>>
>>>>> I agree with Linus here. The problem is that GPIO controllers that can work as
>>>>> IRQ sources are treated in the kernel as if there where two separate controlers
>>>>> that are rather orthogonal: an irq_chip and a gpio_chip.
>>>>> But DT allows to use a GPIO line as an IRQ just by using an omap-gpio phandle as
>>>>> "interrupt-parent".
>>>>>
>>>>> So, there should be a place where both irq_chip and gpio_chip has to be related
>>>>> somehow to properly configure a GPIO (request it and setting it as input) when
>>>>> used as an IRQ by DT.
>>>>>
>>>>> My patch for OMAP used an irq_domain_ops .map function handler to configure the
>>>>> GPIO when a IRQ was mapped since that seemed to me as the best place to do it.
>>>>> This worked well in OMAP2+ platforms but unfortunately broke OMAP1 platforms
>>>>> since they are still using legacy domain mapping thus not call .map.
>>>>
>>>> Just wondering- why .map not called for omap1? irq_create_mapping does seem to
>>>> call  -> irq_domain_associate which calls map function. For omap case, GPIO
>>>> driver does call irq_create_mapping, just like omap2+ no?
>>>>
>>>
>>> That is what I understood too when writing the patch but I remember someone
>>> mentioning legacy domain mapping not calling the .map function handler as a
>>> possible cause for the OMAP1 regression and since Linus decided to revert the
>>> patches in favor of a more general solution I didn't care to check if that was
>>> true or not. Now looking at irq_create_mapping() I see that my assumption was
>>> correct so I don't know what was the bug that caused the OMAP1 regression.
>>
>> Only stuff you deleted from the chip_init function was:
>>
>> -       for (j = 0; j < bank->width; j++) {
>> -               int irq = irq_create_mapping(bank->domain, j);
>> -               irq_set_lockdep_class(irq, &gpio_lock_class);
>> -               irq_set_chip_data(irq, bank);
>> -               if (bank->is_mpuio) {
>> -                       omap_mpuio_alloc_gc(bank, irq, bank->width);
>> -               } else {
>> -                       irq_set_chip_and_handler(irq, &gpio_irq_chip,
>> -                                                handle_simple_irq);
>> -                       set_irq_flags(irq, IRQF_VALID);
>> -               }
>>
>> and you moved all of it to the .map function in your patch. Not sure what could
>> be breaking OMAP1 cases.
>> You could potentially add that back with some #ifdef for OMAP1?
>>
>> Either way, map should be called looks like. If its not called, then the above
>> block can be explicity called for OMAP1 case in omap_chip_gpio_init.
>>
>> What was strange is one person reported that mappings were not created for
>> OMAP1. But I am wondering what you changed could result in not creating that
>> mapping. Really nothing..
>>
>> I think your initial patch is much better than fixing up DT but then I may be
>> missing other problems with your patch that Linus's patch addresses.
>>
>>>> Further, if for any reason the .map is not called. Can you not call gpio_request
>>>> yourself direct in omap_gpio_chip_init function?
>>>>
>>>
>>> No, since you can't request a GPIO for all GPIO pins in the bank. Users have to
>>> do it explicitly (or implicitly in the case of GPIO mapped as IRQ in DT).
>>
>> Ah since you split the patch up into 2, I missed the gpio_request stuff. Ok,
>> that makes sense.
>>
>>>> Does it really matter if you call gpio_request from .map or from the chip_init
>>>> function?
>>>>
>>>
>>> Yes it does, because in DT the core calls irq_create_of_mapping() ->
>>> irq_create_mapping() -> .map(). That way only are requested the GPIO pins that
>>> are mapped as IRQ and not all of them.
>>
>>>> Also on a different note.. this would call gpio_request for *every* gpio line,
>>>> but isn't that what your original patch that got reverted was doing in
>>>> omap_gpio_chip_init:
>>>>
>>>> +       if (!bank->chip.of_node)
>>>> +               for (j = 0; j < bank->width; j++)
>>>> +                       irq_create_mapping(bank->domain, j);
>>>>
>>>
>>> No it won't. This is only needed for the legacy (non-DT) boot since no one calls
>>> irq_create_mapping() so it has to be called explicitly.
>>>
>>> And in that case .map will be called but gpio_request() won't since the call is
>>> made only when bank->chip.of_node is not NULL.
>>
>> Ok, thanks for the explanation. That makes sense to me.
>>
> 
> I'm glad that it helped to you to better understand the approach but you
> shouldn't spend time on this since Linus W had made very clear that he doesn't
> want a local solution that would be replicated on each platform since this is
> not an OMAP only issue.

Ok.

> If you are interested in this problem you should joining the thread "Re: [PATCH
> v3] gpio: interrupt consistency check for OF GPIO IRQs" [1] were is currently
> being discussed this approach.

Ok, if possible if you could CC me on this thread as well, would be grateful.
Thanks.


Regards,

-Joel

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 10.09.2013 17:00, schrieb Joel Fernandes:

> I think your initial patch is much better than fixing up DT but then I may be
> missing other problems with your patch that Linus's patch addresses.

The initial patch had the problem that it not only did introduce 
irq-mappings for only those gpios which are marked as IRQs, but it 
requested those gpios too and preconfigured them

And that breaks every driver which uses gpios for IRQs.

To summarize what happens if a driver uses a gpio as irq:

gpio_request() // This works only if the gpio was not requested before
gpio_direction_input()
gpio_to_irq() // This needs an irq-mapping
request_threaded_irq()

So I would suggest multiple steps to change that:

1. Create a mapping for every gpio found in DT (or all gpios if no DT is 
used). I think that is what Linus patch does (sorry, I haven't really 
followed this thread and didn't look in deep at the patch).

2. Implement gpio_request_for_irq()
This would just be a small macro for gpio_request(); gpio_direction_input()

3. Change all drivers which do use gpio_to_irq() to use 
gpio_request_for_irq() instead of gpio_request() and 
gpio_direction_input(). This will end up with a big series of more or 
less trivial patches (I count 677 occurences of gpio_to_irq) and might 
be splitted.

4. request gpios and set them to input when a gpio is marked as an IRQ 
in the DT and DT is used. Change gpio_rquest_for_irq() to a NOP if DT is 
used or leave it as it is for the none-dt case.


Regards,

Alexander Holler

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 11.09.2013 09:05, schrieb Alexander Holler:
> Am 10.09.2013 17:00, schrieb Joel Fernandes:
>
>> I think your initial patch is much better than fixing up DT but then I
>> may be
>> missing other problems with your patch that Linus's patch addresses.
>
> The initial patch had the problem that it not only did introduce
> irq-mappings for only those gpios which are marked as IRQs, but it
> requested those gpios too and preconfigured them
>
> And that breaks every driver which uses gpios for IRQs.
>
> To summarize what happens if a driver uses a gpio as irq:
>
> gpio_request() // This works only if the gpio was not requested before
> gpio_direction_input()
> gpio_to_irq() // This needs an irq-mapping
> request_threaded_irq()
>
> So I would suggest multiple steps to change that:
>
> 1. Create a mapping for every gpio found in DT (or all gpios if no DT is
> used). I think that is what Linus patch does (sorry, I haven't really
> followed this thread and didn't look in deep at the patch).
>
> 2. Implement gpio_request_for_irq()
> This would just be a small macro for gpio_request(); gpio_direction_input()
>
> 3. Change all drivers which do use gpio_to_irq() to use
> gpio_request_for_irq() instead of gpio_request() and
> gpio_direction_input(). This will end up with a big series of more or
> less trivial patches (I count 677 occurences of gpio_to_irq) and might
> be splitted.
>
> 4. request gpios and set them to input when a gpio is marked as an IRQ
> in the DT and DT is used. Change gpio_rquest_for_irq() to a NOP if DT is
> used or leave it as it is for the none-dt case.

But I still see a possible problem with requesting a gpio (centralized) 
if it is marked as IRQ in DT: it does this always.

I'm not sure, but there might be cases where this isn't wanted. Just 
that a GPIO is marked as IRQ in the DT for one driver, doesn't mean that 
this driver will be really used (and something else might use the GPIO 
instead).

Regards,

Alexander Holler

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

And another small update. ;)

Am 11.09.2013 09:16, schrieb Alexander Holler:

>> To summarize what happens if a driver uses a gpio as irq:
>>
>> gpio_request() // This works only if the gpio was not requested before
>> gpio_direction_input()
>> gpio_to_irq() // This needs an irq-mapping
>> request_threaded_irq()
>>
>> So I would suggest multiple steps to change that:
>>
>> 1. Create a mapping for every gpio found in DT (or all gpios if no DT is
>> used). I think that is what Linus patch does (sorry, I haven't really
>> followed this thread and didn't look in deep at the patch).
>>
>> 2. Implement gpio_request_for_irq()
>> This would just be a small macro for gpio_request();
>> gpio_direction_input()
>>

I would add gpio_to_irq() to that macro, so that it returns the irq 
number or zero if something failed.

Regards,

Alexander Holler

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 11.09.2013 09:30, schrieb Alexander Holler:
> And another small update. ;)
>
> Am 11.09.2013 09:16, schrieb Alexander Holler:
>
>>> To summarize what happens if a driver uses a gpio as irq:
>>>
>>> gpio_request() // This works only if the gpio was not requested before
>>> gpio_direction_input()
>>> gpio_to_irq() // This needs an irq-mapping
>>> request_threaded_irq()
>>>
>>> So I would suggest multiple steps to change that:
>>>
>>> 1. Create a mapping for every gpio found in DT (or all gpios if no DT is
>>> used). I think that is what Linus patch does (sorry, I haven't really
>>> followed this thread and didn't look in deep at the patch).
>>>
>>> 2. Implement gpio_request_for_irq()
>>> This would just be a small macro for gpio_request();
>>> gpio_direction_input()
>>>
>
> I would add gpio_to_irq() to that macro, so that it returns the irq
> number or zero if something failed.

Doing that, it would be easy to mark gpio_to_irq() as deprecated and 
driver authors would change automatically to use gpio_request_for_irq()
(or gpio_request_as_irq() which might more correct english).

So I would suggest just the two steps above, the first with the mapping 
and the second which introduces gpio_request_as_irq() and marks 
gpio_to_irq() as deprecated.

Everthing else I would leave out for the future (so I wouldn't request 
gpios centrally).

Regards,

Alexander Holler

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

Am Mittwoch, 31. Juli 2013, 01:44:53 schrieb Linus Walleij:
> On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> 
wrote:
> > On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij 
<linus.walleij@linaro.org> wrote:
> >> 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.
> > 
> > Ugh, that's pretty awful, and it doesn't actually solve the root
> > problem of the GPIO and IRQ subsystems not cooperating. It's also a
> > very DT-centric solution even though we're going to see the exact same
> > issue on ACPI machines.
> 
> The problem is that the patches for OMAP that I applied
> and now have had to revert solves it in an even uglier way,
> leading to breaking boards, as was noticed.
> 
> The approach in this patch has the potential to actually
> work without regressing a bunch of boards...

I fully agree. I spent many time understanding the problem and reading 
mails to see what happend so far. This is by far the best way to solve the 
problem that was proposed until now. I support that approach.
 
> Whether this is a problem in ACPI or not remains to be seen,
> but I'm not sure about that. Device trees allows for a GPIO line
> to be used as an interrupt source and GPIO line orthogonally,
> and that is the root of this problem. Does ACPI have the same
> problem, or does it impose natural restrictions on such use
> cases?

Is requesting GPIOs directly common in the ACPI world? Does ACPI allow to 
use GPIOs for interrupts?
And even if ACPI had the same problems than device tree, finding a common 
solution seems very difficult to me.
I also prefer Linus' way here. This can be fixes in gpiolib for the device 
tree path and this is ok.

> > We have to solve the problem in a better way than that.

Linus asked for real solutions more than once. Can anybody propose a better 
way ?

> >> 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.
> > 
> > If a gpio line is being both requested as a gpio and used as an
> > interrupt line, then either a) it's a bug, or b) the gpio line needs
> > to be used as input only so it is compatible with irq usage. b) should
> > be supportable.
> 
> Yes this is what I'm saying too I think...
> 
> The bug in (a) manifested itself in the OMAP patch with
> no real solution in sight.
> 
> >> - The GPIO debugfs file claims this GPIO line is "free".
> > 
> > Surely we can fix this. I still don't see a problem of having the
> > controller request the gpio when it is claimed as an irq if we can get
> > around the problem of another user performing a /valid/ request on the
> > same GPIO line. The solution may be to have a special form of request
> > or flag that allows it to be shared.
> 
> I don't see how sharing works here, or how another user,
> i.e. another one than the user wanting to recieve the IRQ,
> can validly request such a line? What would the usecase for
> that valid request be?

I also don't see the usecase for that. Sure, that can be done, but it is of 
no use. If it shows, that someone really needs this, it can be implemented 
later.

> Basically I believe these two things need to be exclusive in
> the DT world:
> 
> A: request_irq(a resource passed from "interrupts");
>      -> core implicitly performs gpio_request()
>          gpio_direction_input()
> 
> B: gpio_request(a resource passed from "gpios");
>      gpio_direction_input()
>      request_irq(gpio_to_irq())
> 
> Never both. And IIUC that was what happened in the
> OMAP case.

ACK.
 
> >> - 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.
> > 
> > Should also be solvable if the gpio request problem is solved.
> 
> Agreed...

To push the discussion a bit further, I took Linus patch and extended it so 
that it can be tested now on real world hardware. I tested it on my device 
tree enabled board and it works fine here. Refer to [1], [2] for version 2 
of the patch.
It completely scans the device tree for interrupt-controller users of the 
gpio_chip in question. It requests the needed gpios and sets them as input.
If the gpio_chip is removed, it also frees the gpio lines.
Folks please help testing!

Thanks,
Lars

[1] http://marc.info/?l=linux-kernel&m=137638745909335&w=2
[2] https://patchwork.kernel.org/patch/2843525/

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Laurent Pinchart]

Hi Linus,

On Wednesday 31 July 2013 01:44:53 Linus Walleij wrote:
> On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely wrote:
> > On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij wrote:
> >> 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.
> > 
> > Ugh, that's pretty awful, and it doesn't actually solve the root
> > problem of the GPIO and IRQ subsystems not cooperating. It's also a
> > very DT-centric solution even though we're going to see the exact same
> > issue on ACPI machines.
> 
> The problem is that the patches for OMAP that I applied and now have had to
> revert solves it in an even uglier way, leading to breaking boards, as was
> noticed.
> 
> The approach in this patch has the potential to actually work without
> regressing a bunch of boards...
> 
> Whether this is a problem in ACPI or not remains to be seen, but I'm not
> sure about that. Device trees allows for a GPIO line to be used as an
> interrupt source and GPIO line orthogonally, and that is the root of this
> problem. Does ACPI have the same problem, or does it impose natural
> restrictions on such use cases?
> 
> > We have to solve the problem in a better way than that. Rearranging
> > your patch description, here are some of the points you brought up so
> > I can comment on them...
> > 
> >> 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.
> > 
> > If a gpio line is being both requested as a gpio and used as an
> > interrupt line, then either a) it's a bug, or b) the gpio line needs
> > to be used as input only so it is compatible with irq usage. b) should
> > be supportable.
> 
> Yes this is what I'm saying too I think...
> 
> The bug in (a) manifested itself in the OMAP patch with no real solution in
> sight.
> 
> >> - The GPIO debugfs file claims this GPIO line is "free".
> > 
> > Surely we can fix this. I still don't see a problem of having the
> > controller request the gpio when it is claimed as an irq if we can get
> > around the problem of another user performing a /valid/ request on the
> > same GPIO line. The solution may be to have a special form of request
> > or flag that allows it to be shared.
> 
> I don't see how sharing works here, or how another user, i.e. another one
> than the user wanting to recieve the IRQ, can validly request such a line?
> What would the usecase for that valid request be?

When the GPIO is wired to a status signal (such as an MMC card detect signal) 
the driver might want to read the state of the signal independently of the 
interrupt handler.

> Basically I believe these two things need to be exclusive in the DT world:
> 
> A: request_irq(a resource passed from "interrupts");
>      -> core implicitly performs gpio_request()
>          gpio_direction_input()
> 
> B: gpio_request(a resource passed from "gpios");
>      gpio_direction_input()
>      request_irq(gpio_to_irq())
> 
> Never both. And IIUC that was what happened in the OMAP case.

Isn't the core issue that we can translate a GPIO number to an IRQ number, but 
not the other way around ? If that could be done, we could request the GPIO 
and configure it as an input when the IRQ is requested.

> >> - 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.
> > 
> > Should also be solvable if the gpio request problem is solved.
> 
> Agreed...

-- 
Regards,

Laurent Pinchart

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Tue, Aug 20, 2013 at 12:04 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wednesday 31 July 2013 01:44:53 Linus Walleij wrote:

>> I don't see how sharing works here, or how another user, i.e. another one
>> than the user wanting to recieve the IRQ, can validly request such a line?
>> What would the usecase for that valid request be?
>
> When the GPIO is wired to a status signal (such as an MMC card detect signal)
> the driver might want to read the state of the signal independently of the
> interrupt handler.

That is true. But for such a complex usecase I think it's reasonable that
we only specify the GPIO in the device tree, and the driver utilizing the
IRQ need to take that and perform gpio_to_irq() on it, and then it still
works to use it both ways.

>> Basically I believe these two things need to be exclusive in the DT world:
>>
>> A: request_irq(a resource passed from "interrupts");
>>      -> core implicitly performs gpio_request()
>>          gpio_direction_input()
>>
>> B: gpio_request(a resource passed from "gpios");
>>      gpio_direction_input()
>>      request_irq(gpio_to_irq())
>>
>> Never both. And IIUC that was what happened in the OMAP case.
>
> Isn't the core issue that we can translate a GPIO number to an IRQ number, but
> not the other way around ? If that could be done, we could request the GPIO
> and configure it as an input when the IRQ is requested.

That is true. It would be easier if all GPIO drivers has an irqchip and
and irqdomain, then we could implement irq_to_gpio() properly in gpiolib
and this would not be a problem. Alas, not all do.

But I also think that the DT contains (as demonstrated by the patch)
all information about what interrupts and GPIOs may conflict, so I
also see this as something of a consistency check, but it could go
in either way.

Yours,
Linus Walleij

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Laurent Pinchart]

Hi Linus,

Sorry for the late reply.

On Thursday 22 August 2013 00:02:39 Linus Walleij wrote:
> On Tue, Aug 20, 2013 at 12:04 AM, Laurent Pinchart wrote:
> > On Wednesday 31 July 2013 01:44:53 Linus Walleij wrote:
> >> I don't see how sharing works here, or how another user, i.e. another one
> >> than the user wanting to recieve the IRQ, can validly request such a
> >> line? What would the usecase for that valid request be?
> > 
> > When the GPIO is wired to a status signal (such as an MMC card detect
> > signal) the driver might want to read the state of the signal
> > independently of the interrupt handler.
> 
> That is true. But for such a complex usecase I think it's reasonable that
> we only specify the GPIO in the device tree, and the driver utilizing the
> IRQ need to take that and perform gpio_to_irq() on it, and then it still
> works to use it both ways.

I'm pretty sure I would have had an objection a couple of weeks ago when I was 
looking into this, but I can't think of another use case for now, so I agree 
with you.

> >> Basically I believe these two things need to be exclusive in the DT
> >> world:
> >> 
> >> A: request_irq(a resource passed from "interrupts");
> >>      -> core implicitly performs gpio_request()
> >>          gpio_direction_input()
> >> 
> >> B: gpio_request(a resource passed from "gpios");
> >>      gpio_direction_input()
> >>      request_irq(gpio_to_irq())
> >> 
> >> Never both. And IIUC that was what happened in the OMAP case.
> > 
> > Isn't the core issue that we can translate a GPIO number to an IRQ number,
> > but not the other way around ? If that could be done, we could request
> > the GPIO and configure it as an input when the IRQ is requested.
> 
> That is true. It would be easier if all GPIO drivers has an irqchip and
> and irqdomain, then we could implement irq_to_gpio() properly in gpiolib
> and this would not be a problem. Alas, not all do.
> 
> But I also think that the DT contains (as demonstrated by the patch)
> all information about what interrupts and GPIOs may conflict, so I
> also see this as something of a consistency check, but it could go
> in either way.

-- 
Regards,

Laurent Pinchart

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 22.08.2013 00:02, schrieb Linus Walleij:
> On Tue, Aug 20, 2013 at 12:04 AM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> On Wednesday 31 July 2013 01:44:53 Linus Walleij wrote:
> 
>>> I don't see how sharing works here, or how another user, i.e. another one
>>> than the user wanting to recieve the IRQ, can validly request such a line?
>>> What would the usecase for that valid request be?
>>
>> When the GPIO is wired to a status signal (such as an MMC card detect signal)
>> the driver might want to read the state of the signal independently of the
>> interrupt handler.
> 
> That is true. But for such a complex usecase I think it's reasonable that
> we only specify the GPIO in the device tree, and the driver utilizing the
> IRQ need to take that and perform gpio_to_irq() on it, and then it still
> works to use it both ways.

Hmm, the problem is that DT is seen as fixed. So if someone marks a GPIO
as an IRQ, it can never be used otherwise. So if you really go this way,
you should make this pretty clear in the documentation.

Looking from the other side, why do you want to mark GPIOs as IRQs in
the DT at all? And how will this be done? I found the way it was done in
the reverted patch very confusing because it needed an IRQ number. That
IRQ number depends on the mapping and isn't hw specific (and currently
just human doable because of the simple mapping).

Regards,

Alexander Holler

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/11/2013 05:30 PM, Alexander Holler wrote:
> Am 22.08.2013 00:02, schrieb Linus Walleij:
>> On Tue, Aug 20, 2013 at 12:04 AM, Laurent Pinchart
>> <laurent.pinchart@ideasonboard.com> wrote:
>>> On Wednesday 31 July 2013 01:44:53 Linus Walleij wrote:
>> 
>>>> I don't see how sharing works here, or how another user, i.e. another one
>>>> than the user wanting to recieve the IRQ, can validly request such a line?
>>>> What would the usecase for that valid request be?
>>>
>>> When the GPIO is wired to a status signal (such as an MMC card detect signal)
>>> the driver might want to read the state of the signal independently of the
>>> interrupt handler.
>> 
>> That is true. But for such a complex usecase I think it's reasonable that
>> we only specify the GPIO in the device tree, and the driver utilizing the
>> IRQ need to take that and perform gpio_to_irq() on it, and then it still
>> works to use it both ways.
> 
> Hmm, the problem is that DT is seen as fixed. So if someone marks a GPIO
> as an IRQ, it can never be used otherwise. So if you really go this way,
> you should make this pretty clear in the documentation.
> 

DT is fixed because that describes the hardware which is fixed of course. So if
a chip IRQ line is connected to a GPIO pin in a controller that should be
described in the DT and that pin can't be used for anything else.

> Looking from the other side, why do you want to mark GPIOs as IRQs in
> the DT at all? 

Because from the component point-of-view that is wired to the SoC, that GPIO is
an IRQ line and so it has to be described.

> And how will this be done? I found the way it was done in
> the reverted patch very confusing because it needed an IRQ number. That
> IRQ number depends on the mapping and isn't hw specific (and currently
> just human doable because of the simple mapping).
> 

That's is not true. You don't define an IRQ number what you define is a GPIO
number that is mapped as IRQ. The GPIO number does not depend on the mapping and
it only depends on the GPIO controller.

This has absolutely nothing to do with the reverted patches and is described in
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt.

The only difference is that the reverted patches did actually take an action
when a GPIO pin was mapped as an IRQ (requesting the GPIO and as input).

So for example in an OMAP board DT you can define something like this:

ethernet at 5,0 {
        compatible = "smsc,lan9221", "smsc,lan9115";
        interrupt-parent = <&gpio6>;
        interrupts = <16 8>;
};

Since each OMAP GPIO bank has 32 GPIO pins, then what you are defining is that
the GPIO 176 (5 * 32 + 16) will be mapped as the IRQ line for the ethernet
controller.

I explained the exact use case I'm trying to solve in the thread "Re: [PATCH v3]
gpio: interrupt consistency check for OF GPIO IRQs" [1] if you need more
context. I'm sure others cc'ed in this thread have different (but similar)
requirements.

> Regards,
> 
> Alexander Holler
>

Thanks a lot and best regards,
Javier


[1]: http://www.kernelhub.org/?p=2&msg=326503

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 11.09.2013 18:14, schrieb Javier Martinez Canillas:
> On 09/11/2013 05:30 PM, Alexander Holler wrote:
>> Am 22.08.2013 00:02, schrieb Linus Walleij:
>>> On Tue, Aug 20, 2013 at 12:04 AM, Laurent Pinchart
>>> <laurent.pinchart@ideasonboard.com> wrote:
>>>> On Wednesday 31 July 2013 01:44:53 Linus Walleij wrote:
>>>
>>>>> I don't see how sharing works here, or how another user, i.e. another one
>>>>> than the user wanting to recieve the IRQ, can validly request such a line?
>>>>> What would the usecase for that valid request be?
>>>>
>>>> When the GPIO is wired to a status signal (such as an MMC card detect signal)
>>>> the driver might want to read the state of the signal independently of the
>>>> interrupt handler.
>>>
>>> That is true. But for such a complex usecase I think it's reasonable that
>>> we only specify the GPIO in the device tree, and the driver utilizing the
>>> IRQ need to take that and perform gpio_to_irq() on it, and then it still
>>> works to use it both ways.
>>
>> Hmm, the problem is that DT is seen as fixed. So if someone marks a GPIO
>> as an IRQ, it can never be used otherwise. So if you really go this way,
>> you should make this pretty clear in the documentation.
>>
>
> DT is fixed because that describes the hardware which is fixed of course. So if
> a chip IRQ line is connected to a GPIO pin in a controller that should be
> described in the DT and that pin can't be used for anything else.
>

If you request an gpio for every gpio which defined as IRQ centrally, 
drivers can't just use that gpio as gpio afterwards. And there might be 
one driver which uses the irq but another one which wants the gpio. But 
if you define a gpio as irq in dt, you take away that choice.

>> Looking from the other side, why do you want to mark GPIOs as IRQs in
>> the DT at all?
>
> Because from the component point-of-view that is wired to the SoC, that GPIO is
> an IRQ line and so it has to be described.
>
>> And how will this be done? I found the way it was done in
>> the reverted patch very confusing because it needed an IRQ number. That
>> IRQ number depends on the mapping and isn't hw specific (and currently
>> just human doable because of the simple mapping).
>>
>
> That's is not true. You don't define an IRQ number what you define is a GPIO
> number that is mapped as IRQ. The GPIO number does not depend on the mapping and
> it only depends on the GPIO controller.
>
> This has absolutely nothing to do with the reverted patches and is described in
> Documentation/devicetree/bindings/interrupt-controller/interrupts.txt.
>
> The only difference is that the reverted patches did actually take an action
> when a GPIO pin was mapped as an IRQ (requesting the GPIO and as input).
>
> So for example in an OMAP board DT you can define something like this:
>
> ethernet at 5,0 {
>          compatible = "smsc,lan9221", "smsc,lan9115";
>          interrupt-parent = <&gpio6>;
>          interrupts = <16 8>;
> };
>
> Since each OMAP GPIO bank has 32 GPIO pins, then what you are defining is that
> the GPIO 176 (5 * 32 + 16) will be mapped as the IRQ line for the ethernet
> controller.
>
> I explained the exact use case I'm trying to solve in the thread "Re: [PATCH v3]
> gpio: interrupt consistency check for OF GPIO IRQs" [1] if you need more
> context. I'm sure others cc'ed in this thread have different (but similar)
> requirements.

So I would extend my previous proposal
(http://www.mail-archive.com/linux-omap at vger.kernel.org/msg95202.html)
for a gpio_request_as_irq() such, that I would change that to 
request_irq_new(number, irq_controller) where the irq_controller would 
be the gpio-controller in case of omap (while marking gpio_to_irq() as 
deprecated).

So request_irq_new() would than do the following on omap if the 
irq-controller is a gpio-controller:

gpio_request() // This works only if the gpio was not requested before
gpio_direction_input()
(build irq-mapping)
gpio_to_irq() // This needs an irq-mapping
return request_threaded_irq()

and all drivers could replace the above sequence just with 
request_irq_new(number_from_dt, irq-controller_from_dt).

How's that?

Regards,

Alexander Holler

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 11.09.2013 19:42, schrieb Alexander Holler:
> Am 11.09.2013 18:14, schrieb Javier Martinez Canillas:

>> So for example in an OMAP board DT you can define something like this:
>>
>> ethernet at 5,0 {
>>          compatible = "smsc,lan9221", "smsc,lan9115";
>>          interrupt-parent = <&gpio6>;
>>          interrupts = <16 8>;
>> };
>>
>> Since each OMAP GPIO bank has 32 GPIO pins, then what you are defining
>> is that
>> the GPIO 176 (5 * 32 + 16) will be mapped as the IRQ line for the
>> ethernet
>> controller.

By the way, how do you define two GPIOs/IRQs from different
gpio-banks/irq-controllers wuth that scheme?

Would that be like below?

 ethernet at 5,0 {
          compatible = "smsc,lan9221", "smsc,lan9115";
          interrupt-parent = <&gpio6>;
          interrupts = <16 8>;
          interrupt-parent = <&gpio7>;
          interrupts = <1 IRQF_TRIGGER_FALLING>; /* GPIO7_1 */
 };

So multiple definitions of interrupt-parent are allowed and the order
does matter? And such does work? Sorry for asking, but I'm relatively
new to DT. ;)

Regards,

Alexander Holler

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Javier Martinez Canillas]

On 09/12/2013 10:55 AM, Alexander Holler wrote:
> Am 11.09.2013 19:42, schrieb Alexander Holler:
>> Am 11.09.2013 18:14, schrieb Javier Martinez Canillas:
> 
>>> So for example in an OMAP board DT you can define something like this:
>>>
>>> ethernet at 5,0 {
>>>          compatible = "smsc,lan9221", "smsc,lan9115";
>>>          interrupt-parent = <&gpio6>;
>>>          interrupts = <16 8>;
>>> };
>>>
>>> Since each OMAP GPIO bank has 32 GPIO pins, then what you are defining
>>> is that
>>> the GPIO 176 (5 * 32 + 16) will be mapped as the IRQ line for the
>>> ethernet
>>> controller.
> 
> By the way, how do you define two GPIOs/IRQs from different
> gpio-banks/irq-controllers wuth that scheme?
> 

That is indeed a very good question and I don't have a definite answer.

> Would that be like below?
> 
>  ethernet at 5,0 {
>           compatible = "smsc,lan9221", "smsc,lan9115";
>           interrupt-parent = <&gpio6>;
>           interrupts = <16 8>;
>           interrupt-parent = <&gpio7>;
>           interrupts = <1 IRQF_TRIGGER_FALLING>; /* GPIO7_1 */
>  };
>

I just looked at
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt and it
doesn't mention that use-case (same device using two different interrupts from
two different interrupt-controller).

So I went and look at the source in drivers/of/irq.c and noticed that the
"interrupts" property and its "interrupt-parent" is parsed by the
of_irq_map_one() function.

/**
 * of_irq_map_one - Resolve an interrupt for a device
 * @device: the device whose interrupt is to be resolved
 * @index: index of the interrupt to resolve
 * @out_irq: structure of_irq filled by this function
 *
 * This function resolves an interrupt, walking the tree, for a given
 * device-tree node. It's the high level pendant to of_irq_map_raw().
 */
int of_irq_map_one(struct device_node *device, int index, struct of_irq *out_irq)
{
        struct device_node *p;
        ...
        /* Get the interrupts property */
        intspec = of_get_property(device, "interrupts", &intlen);
        ...
        /* Look for the interrupt parent. */
        p = of_irq_find_parent(device);
        ...
}

/**
 * of_irq_find_parent - Given a device node, find its interrupt parent node
 * @child: pointer to device node
 *
 * Returns a pointer to the interrupt parent node, or NULL if the interrupt
 * parent could not be determined.
 */
struct device_node *of_irq_find_parent(struct device_node *child)
{
	struct device_node *p;
	const __be32 *parp;

	if (!of_node_get(child))
		return NULL;

	do {
		parp = of_get_property(child, "interrupt-parent", NULL);
		if (parp == NULL)
			p = of_get_parent(child);
		else {
			if (of_irq_workarounds & OF_IMAP_NO_PHANDLE)
				p = of_node_get(of_irq_dflt_pic);
			else
				p = of_find_node_by_phandle(be32_to_cpup(parp));
		}
		of_node_put(child);
		child = p;
	} while (p && of_get_property(p, "#interrupt-cells", NULL) == NULL);

	return p;
}

So, if I understood the code correctly the DT IRQ core doesn't expect a device
node to have more than one "interrupt-parent" property.

It *should* work though if you have multiple "interrupts" properties defined and
all of them have the same "interrupt-parent":

       interrupt-parent = <&gpio6>;
       interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */
       interrupts = <2 IRQF_TRIGGER_LOW>; /* GPIO6_2 */

since of_irq_map_one() will be called for each "interrupts" and the correct
"interrupt-parent" will get obtained by of_irq_find_parent().

> So multiple definitions of interrupt-parent are allowed and the order
> does matter? And such does work? Sorry for asking, but I'm relatively
> new to DT. ;)
>

No worries, I'm very new to DT too so let's wait for Grant, Stephen or Linus to
give us a definite answer :)

> Regards,
> 
> Alexander Holler
> 

Best regards,
Javier

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 12.09.2013 12:11, schrieb Javier Martinez Canillas:
> On 09/12/2013 10:55 AM, Alexander Holler wrote:

...
>>
>> By the way, how do you define two GPIOs/IRQs from different
>> gpio-banks/irq-controllers wuth that scheme?
>>
>
> That is indeed a very good question and I don't have a definite answer.
>
>> Would that be like below?
>>
>>   ethernet at 5,0 {
>>            compatible = "smsc,lan9221", "smsc,lan9115";
>>            interrupt-parent = <&gpio6>;
>>            interrupts = <16 8>;
>>            interrupt-parent = <&gpio7>;
>>            interrupts = <1 IRQF_TRIGGER_FALLING>; /* GPIO7_1 */
>>   };
>>

...

> So, if I understood the code correctly the DT IRQ core doesn't expect a device
> node to have more than one "interrupt-parent" property.
>
> It *should* work though if you have multiple "interrupts" properties defined and
> all of them have the same "interrupt-parent":
>
>         interrupt-parent = <&gpio6>;
>         interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */
>         interrupts = <2 IRQF_TRIGGER_LOW>; /* GPIO6_2 */
>
> since of_irq_map_one() will be called for each "interrupts" and the correct
> "interrupt-parent" will get obtained by of_irq_find_parent().

I assumed that answer. So to make such a scenario possible, something 
like this might be neccessary:

          interrupts = <&gpio6 1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */
          interrupts = <&gpio7 2 IRQF_TRIGGER_LOW>; /* GPIO7_2 */

or, to be compatible

          interrupts = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */
          interrupts = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */

Another problem is the naming. In all the above cases, the driver would 
not know which IRQ he should use for what. Maybe the order defines it, 
but that wouldn't be very verbose. And I think just changing the name 
would make travelling the tree impossible, as only the driver itself 
would know the name and it's meaning.

Regards,

Alexander Holler

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 12.09.2013 12:28, schrieb Alexander Holler:
> Am 12.09.2013 12:11, schrieb Javier Martinez Canillas:
>> On 09/12/2013 10:55 AM, Alexander Holler wrote:
>
> ...
>>>
>>> By the way, how do you define two GPIOs/IRQs from different
>>> gpio-banks/irq-controllers wuth that scheme?
>>>
>>
>> That is indeed a very good question and I don't have a definite answer.
>>
>>> Would that be like below?
>>>
>>>   ethernet at 5,0 {
>>>            compatible = "smsc,lan9221", "smsc,lan9115";
>>>            interrupt-parent = <&gpio6>;
>>>            interrupts = <16 8>;
>>>            interrupt-parent = <&gpio7>;
>>>            interrupts = <1 IRQF_TRIGGER_FALLING>; /* GPIO7_1 */
>>>   };
>>>
>
> ...
>
>> So, if I understood the code correctly the DT IRQ core doesn't expect
>> a device
>> node to have more than one "interrupt-parent" property.
>>
>> It *should* work though if you have multiple "interrupts" properties
>> defined and
>> all of them have the same "interrupt-parent":
>>
>>         interrupt-parent = <&gpio6>;
>>         interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */
>>         interrupts = <2 IRQF_TRIGGER_LOW>; /* GPIO6_2 */
>>
>> since of_irq_map_one() will be called for each "interrupts" and the
>> correct
>> "interrupt-parent" will get obtained by of_irq_find_parent().
>
> I assumed that answer. So to make such a scenario possible, something
> like this might be neccessary:
>
>           interrupts = <&gpio6 1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */
>           interrupts = <&gpio7 2 IRQF_TRIGGER_LOW>; /* GPIO7_2 */
>
> or, to be compatible
>
>           interrupts = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */
>           interrupts = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */
>
> Another problem is the naming. In all the above cases, the driver would
> not know which IRQ he should use for what. Maybe the order defines it,
> but that wouldn't be very verbose. And I think just changing the name
> would make travelling the tree impossible, as only the driver itself
> would know the name and it's meaning.

On a second look, travelling the tree is still possible if the solution 
would be like above (without that interrupt-parent). So if a driver 
requires two interrupts he could use

       interrupt-foo = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */
       interrupt-bar = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */

And travelling the tree will still be possible because walking from the 
interrupt-controllers (those gpio) downwards would end up at the 
interrupt definitions, so the name of them isn't needed to find them in 
the tree.

Regards,

Alexander Holler

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 12.09.2013 13:09, schrieb Alexander Holler:
> Am 12.09.2013 12:28, schrieb Alexander Holler:
>> Am 12.09.2013 12:11, schrieb Javier Martinez Canillas:
>>> On 09/12/2013 10:55 AM, Alexander Holler wrote:
>>
>> ...
>>>>
>>>> By the way, how do you define two GPIOs/IRQs from different
>>>> gpio-banks/irq-controllers wuth that scheme?
>>>>
>>>
>>> That is indeed a very good question and I don't have a definite answer.
>>>
>>>> Would that be like below?
>>>>
>>>>   ethernet at 5,0 {
>>>>            compatible = "smsc,lan9221", "smsc,lan9115";
>>>>            interrupt-parent = <&gpio6>;
>>>>            interrupts = <16 8>;
>>>>            interrupt-parent = <&gpio7>;
>>>>            interrupts = <1 IRQF_TRIGGER_FALLING>; /* GPIO7_1 */
>>>>   };
>>>>
>>
>> ...
>>
>>> So, if I understood the code correctly the DT IRQ core doesn't expect
>>> a device
>>> node to have more than one "interrupt-parent" property.
>>>
>>> It *should* work though if you have multiple "interrupts" properties
>>> defined and
>>> all of them have the same "interrupt-parent":
>>>
>>>         interrupt-parent = <&gpio6>;
>>>         interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */
>>>         interrupts = <2 IRQF_TRIGGER_LOW>; /* GPIO6_2 */
>>>
>>> since of_irq_map_one() will be called for each "interrupts" and the
>>> correct
>>> "interrupt-parent" will get obtained by of_irq_find_parent().
>>
>> I assumed that answer. So to make such a scenario possible, something
>> like this might be neccessary:
>>
>>           interrupts = <&gpio6 1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */
>>           interrupts = <&gpio7 2 IRQF_TRIGGER_LOW>; /* GPIO7_2 */
>>
>> or, to be compatible
>>
>>           interrupts = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */
>>           interrupts = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */
>>
>> Another problem is the naming. In all the above cases, the driver would
>> not know which IRQ he should use for what. Maybe the order defines it,
>> but that wouldn't be very verbose. And I think just changing the name
>> would make travelling the tree impossible, as only the driver itself
>> would know the name and it's meaning.
>
> On a second look, travelling the tree is still possible if the solution
> would be like above (without that interrupt-parent). So if a driver
> requires two interrupts he could use
>
>        interrupt-foo = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */
>        interrupt-bar = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */
>
> And travelling the tree will still be possible because walking from the
> interrupt-controllers (those gpio) downwards would end up at the
> interrupt definitions, so the name of them isn't needed to find them in
> the tree.

I've just seen how they solved it for dma:

			dmas = <&edma0 16
				&edma0 17>;
			dma-names = "rx", "tx";

so it would be like

       interrupts = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */
       interrupts = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */
       interrupt-names = "foo", "bar";

Or this would be possible:

       interrupt-parent = <&gpio6 &gpio7>;
       interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */
       interrupts = <1 IRQF_TRIGGER_LOW>; /* GPIO7_1 */
       interrupt-names = "foo", "bar";

Regards,

Alexander

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 12.09.2013 13:26, schrieb Alexander Holler:
> Am 12.09.2013 13:09, schrieb Alexander Holler:
>> Am 12.09.2013 12:28, schrieb Alexander Holler:
>>> Am 12.09.2013 12:11, schrieb Javier Martinez Canillas:
>>>> On 09/12/2013 10:55 AM, Alexander Holler wrote:
>>>
...
>>>
>>>> So, if I understood the code correctly the DT IRQ core doesn't expect
>>>> a device
>>>> node to have more than one "interrupt-parent" property.
>>>>
>>>> It *should* work though if you have multiple "interrupts" properties
>>>> defined and
>>>> all of them have the same "interrupt-parent":
>>>>
>>>>         interrupt-parent = <&gpio6>;
>>>>         interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */
>>>>         interrupts = <2 IRQF_TRIGGER_LOW>; /* GPIO6_2 */
>>>>
>>>> since of_irq_map_one() will be called for each "interrupts" and the
>>>> correct
>>>> "interrupt-parent" will get obtained by of_irq_find_parent().
>>>
>>> I assumed that answer. So to make such a scenario possible, something
>>> like this might be neccessary:
>>>
>>>           interrupts = <&gpio6 1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */
>>>           interrupts = <&gpio7 2 IRQF_TRIGGER_LOW>; /* GPIO7_2 */
>>>
>>> or, to be compatible
>>>
>>>           interrupts = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */
>>>           interrupts = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */
>>>
>>> Another problem is the naming. In all the above cases, the driver would
>>> not know which IRQ he should use for what. Maybe the order defines it,
>>> but that wouldn't be very verbose. And I think just changing the name
>>> would make travelling the tree impossible, as only the driver itself
>>> would know the name and it's meaning.
>>
>> On a second look, travelling the tree is still possible if the solution
>> would be like above (without that interrupt-parent). So if a driver
>> requires two interrupts he could use
>>
>>        interrupt-foo = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */
>>        interrupt-bar = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */
>>
>> And travelling the tree will still be possible because walking from the
>> interrupt-controllers (those gpio) downwards would end up at the
>> interrupt definitions, so the name of them isn't needed to find them in
>> the tree.
>
> I've just seen how they solved it for dma:
>
>              dmas = <&edma0 16
>                  &edma0 17>;
>              dma-names = "rx", "tx";
>
> so it would be like
>
>        interrupts = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */
>        interrupts = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */
>        interrupt-names = "foo", "bar";
>
> Or this would be possible:
>
>        interrupt-parent = <&gpio6 &gpio7>;
>        interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */
>        interrupts = <1 IRQF_TRIGGER_LOW>; /* GPIO7_1 */
>        interrupt-names = "foo", "bar";
>

And looking at how gpios are defined, I think it should be like that:


           interrupts = <&gpio6 1 IRQF_TRIGGER_HIGH
                         &gpio7 2 IRQF_TRIGGER_LOW
           >;
           interrupt-names = "foo", "bar";

So without that interrupt-parent.

Regards,

Alexander

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 09/12/2013 05:37 AM, Alexander Holler wrote:
> Am 12.09.2013 13:26, schrieb Alexander Holler:
>> Am 12.09.2013 13:09, schrieb Alexander Holler:
>>> Am 12.09.2013 12:28, schrieb Alexander Holler:
>>>> Am 12.09.2013 12:11, schrieb Javier Martinez Canillas:
>>>>> On 09/12/2013 10:55 AM, Alexander Holler wrote:
...
>>>>> So, if I understood the code correctly the DT IRQ core doesn't expect
>>>>> a device node to have more than one "interrupt-parent" property.

The root-cause is the binding definition, not the code.

The interrupts property does not contain the phandle of the IRQ
controller, but rather the interrupt-parent property does. Thus, there
is a single interrupt parent for each node, unless you employ some
tricks (see below).

>>>>> It *should* work though if you have multiple "interrupts" properties
>>>>> defined and
>>>>> all of them have the same "interrupt-parent":
>>>>>
>>>>>         interrupt-parent = <&gpio6>;
>>>>>         interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */
>>>>>         interrupts = <2 IRQF_TRIGGER_LOW>; /* GPIO6_2 */

DT is a key/value data structure, not a list of property (name, value)
pairs. In other words, you can't have multiple properties of the same
name in a node. At least in the compiled DTB; you /might/ be able to
compile the DT above with dtc, but if so the second definition of the
property will just over-write the first.

...
>> I've just seen how they solved it for dma:
>>
>>              dmas = <&edma0 16
>>                  &edma0 17>;
>>              dma-names = "rx", "tx";
...
> And looking at how gpios are defined, I think it should be like that:
> 
>           interrupts = <&gpio6 1 IRQF_TRIGGER_HIGH
>                         &gpio7 2 IRQF_TRIGGER_LOW
>           >;
>           interrupt-names = "foo", "bar";
> 
> So without that interrupt-parent.

IRQs, DMA channels, and GPIOs are all different things. Their bindings
are defined independently. While it's good to define new types of
bindings consistently with other bindings, this hasn't always happened,
so you can make zero assumptions about the IRQ bindings by reading the
documentation for any other kind of binding.

Multiple interrupts are defined as follows:

	// Optional; otherwise inherited from parent/grand-parent/...
	interrupt-parent = <&gpio6>;
	// Must be in a fixed order, unless binding defines that the
	// optional interrupt-names property is to be used.
	interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>;
	// Optional; binding for device defines whether it must
	// be present
	interrupt-names = "foo", "bar";

If you need multiple interrupts, each with a different parent, you need
to use an interrupt-map property (Google it for a more complete
explanation I guess). Unlike "interrupts", "interrupt-map" has a phandle
in each entry, and hence each entry can refer to a different IRQ
controller. You end up defining a dummy interrupt controller node (which
may be the leaf node with multiple IRQ outputs, which then points at
itself as the interrupt parent), pointing the leaf node's
interrupt-parent at that node, and then having interrupt-map "demux" the
N interrupt outputs to the various interrupt controllers.

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Alexander Holler]

Am 12.09.2013 17:19, schrieb Stephen Warren:
>
> IRQs, DMA channels, and GPIOs are all different things. Their bindings
> are defined independently. While it's good to define new types of
> bindings consistently with other bindings, this hasn't always happened,
> so you can make zero assumptions about the IRQ bindings by reading the
> documentation for any other kind of binding.
>
> Multiple interrupts are defined as follows:
>
> 	// Optional; otherwise inherited from parent/grand-parent/...
> 	interrupt-parent = <&gpio6>;
> 	// Must be in a fixed order, unless binding defines that the
> 	// optional interrupt-names property is to be used.
> 	interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>;
> 	// Optional; binding for device defines whether it must
> 	// be present
> 	interrupt-names = "foo", "bar";
>
> If you need multiple interrupts, each with a different parent, you need
> to use an interrupt-map property (Google it for a more complete
> explanation I guess). Unlike "interrupts", "interrupt-map" has a phandle
> in each entry, and hence each entry can refer to a different IRQ
> controller. You end up defining a dummy interrupt controller node (which
> may be the leaf node with multiple IRQ outputs, which then points at
> itself as the interrupt parent), pointing the leaf node's
> interrupt-parent at that node, and then having interrupt-map "demux" the
> N interrupt outputs to the various interrupt controllers.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

What a mess. I assume that is the price that bindings don't have to change.

Thanks for clarifying that,

Alexander Holler

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Grant Likely]

On Thu, 12 Sep 2013 17:57:00 +0200, Alexander Holler <holler@ahsoftware.de> wrote:
> Am 12.09.2013 17:19, schrieb Stephen Warren:
> >
> > IRQs, DMA channels, and GPIOs are all different things. Their bindings
> > are defined independently. While it's good to define new types of
> > bindings consistently with other bindings, this hasn't always happened,
> > so you can make zero assumptions about the IRQ bindings by reading the
> > documentation for any other kind of binding.
> >
> > Multiple interrupts are defined as follows:
> >
> > 	// Optional; otherwise inherited from parent/grand-parent/...
> > 	interrupt-parent = <&gpio6>;
> > 	// Must be in a fixed order, unless binding defines that the
> > 	// optional interrupt-names property is to be used.
> > 	interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>;
> > 	// Optional; binding for device defines whether it must
> > 	// be present
> > 	interrupt-names = "foo", "bar";
> >
> > If you need multiple interrupts, each with a different parent, you need
> > to use an interrupt-map property (Google it for a more complete
> > explanation I guess). Unlike "interrupts", "interrupt-map" has a phandle
> > in each entry, and hence each entry can refer to a different IRQ
> > controller. You end up defining a dummy interrupt controller node (which
> > may be the leaf node with multiple IRQ outputs, which then points at
> > itself as the interrupt parent), pointing the leaf node's
> > interrupt-parent at that node, and then having interrupt-map "demux" the
> > N interrupt outputs to the various interrupt controllers.
> 
> What a mess. I assume that is the price that bindings don't have to change.
> 
> Thanks for clarifying that,
> 
> Alexander Holler

Actually, I think it is solveable but doing so requires a new binding
for interrupts. I took a shot at implementing it earlier this week and
I've got working patches that I'll be posting soon. I created a new
"interrupts-extended" property that uses a phandle+args type of
binding like this:

intc1: intc at 1000 {
	interrupt-controller;
	#interrupt-cells = <1>;
};

intc2: intc at 2000 {
	interrupt-controller;
	#interrupt-cells = <2>;
};

device at 3000 {
	interrupts-extended = <&intc1 5> <&intc2 3 4> <&intc1 6>;
};

'interrupts-extended' will be proposed as a directly replacement of the
'interrupts' property and it will eliminate the need for an
interrupt-map property. A node will be allowed to have one or the other,
but not both.

I'll write up a proper binding document and post for review.

g.

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Laurent Pinchart]

Hi Grant,

On Tuesday 17 September 2013 17:36:32 Grant Likely wrote:
> On Thu, 12 Sep 2013 17:57:00 +0200, Alexander Holler wrote:
> > Am 12.09.2013 17:19, schrieb Stephen Warren:
> > > IRQs, DMA channels, and GPIOs are all different things. Their bindings
> > > are defined independently. While it's good to define new types of
> > > bindings consistently with other bindings, this hasn't always happened,
> > > so you can make zero assumptions about the IRQ bindings by reading the
> > > documentation for any other kind of binding.
> > > 
> > > Multiple interrupts are defined as follows:
> > > 	// Optional; otherwise inherited from parent/grand-parent/...
> > > 	interrupt-parent = <&gpio6>;
> > > 	// Must be in a fixed order, unless binding defines that the
> > > 	// optional interrupt-names property is to be used.
> > > 	interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>;
> > > 	// Optional; binding for device defines whether it must
> > > 	// be present
> > > 	interrupt-names = "foo", "bar";
> > > 
> > > If you need multiple interrupts, each with a different parent, you need
> > > to use an interrupt-map property (Google it for a more complete
> > > explanation I guess). Unlike "interrupts", "interrupt-map" has a phandle
> > > in each entry, and hence each entry can refer to a different IRQ
> > > controller. You end up defining a dummy interrupt controller node (which
> > > may be the leaf node with multiple IRQ outputs, which then points at
> > > itself as the interrupt parent), pointing the leaf node's
> > > interrupt-parent at that node, and then having interrupt-map "demux" the
> > > N interrupt outputs to the various interrupt controllers.
> > 
> > What a mess. I assume that is the price that bindings don't have to
> > change.
> > 
> > Thanks for clarifying that,
> > 
> > Alexander Holler
> 
> Actually, I think it is solveable but doing so requires a new binding
> for interrupts. I took a shot at implementing it earlier this week and
> I've got working patches that I'll be posting soon. I created a new
> "interrupts-extended" property that uses a phandle+args type of
> binding like this:
> 
> intc1: intc at 1000 {
> 	interrupt-controller;
> 	#interrupt-cells = <1>;
> };
> 
> intc2: intc at 2000 {
> 	interrupt-controller;
> 	#interrupt-cells = <2>;
> };
> 
> device at 3000 {
> 	interrupts-extended = <&intc1 5> <&intc2 3 4> <&intc1 6>;
> };
> 
> 'interrupts-extended' will be proposed as a directly replacement of the
> 'interrupts' property and it will eliminate the need for an
> interrupt-map property. A node will be allowed to have one or the other,
> but not both.
> 
> I'll write up a proper binding document and post for review.

Any progress on this ? I'll need to use multiple interrupts with different 
parents in the near future, I can take this over if needed.

I've also been thinking that we could possibly reuse the "interrupts" property 
without defining a new "interrupts-extended". When parsing the property the 
code would use the current DT bindings if an interrupt-parent is present, and 
the new DT bindings if it isn't.

-- 
Regards,

Laurent Pinchart

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [131020 05:41]:
> Hi Grant,
> 
> On Tuesday 17 September 2013 17:36:32 Grant Likely wrote:
> > On Thu, 12 Sep 2013 17:57:00 +0200, Alexander Holler wrote:
> > > Am 12.09.2013 17:19, schrieb Stephen Warren:
> > > > IRQs, DMA channels, and GPIOs are all different things. Their bindings
> > > > are defined independently. While it's good to define new types of
> > > > bindings consistently with other bindings, this hasn't always happened,
> > > > so you can make zero assumptions about the IRQ bindings by reading the
> > > > documentation for any other kind of binding.
> > > > 
> > > > Multiple interrupts are defined as follows:
> > > > 	// Optional; otherwise inherited from parent/grand-parent/...
> > > > 	interrupt-parent = <&gpio6>;
> > > > 	// Must be in a fixed order, unless binding defines that the
> > > > 	// optional interrupt-names property is to be used.
> > > > 	interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>;
> > > > 	// Optional; binding for device defines whether it must
> > > > 	// be present
> > > > 	interrupt-names = "foo", "bar";
> > > > 
> > > > If you need multiple interrupts, each with a different parent, you need
> > > > to use an interrupt-map property (Google it for a more complete
> > > > explanation I guess). Unlike "interrupts", "interrupt-map" has a phandle
> > > > in each entry, and hence each entry can refer to a different IRQ
> > > > controller. You end up defining a dummy interrupt controller node (which
> > > > may be the leaf node with multiple IRQ outputs, which then points at
> > > > itself as the interrupt parent), pointing the leaf node's
> > > > interrupt-parent at that node, and then having interrupt-map "demux" the
> > > > N interrupt outputs to the various interrupt controllers.
> > > 
> > > What a mess. I assume that is the price that bindings don't have to
> > > change.
> > > 
> > > Thanks for clarifying that,
> > > 
> > > Alexander Holler
> > 
> > Actually, I think it is solveable but doing so requires a new binding
> > for interrupts. I took a shot at implementing it earlier this week and
> > I've got working patches that I'll be posting soon. I created a new
> > "interrupts-extended" property that uses a phandle+args type of
> > binding like this:
> > 
> > intc1: intc at 1000 {
> > 	interrupt-controller;
> > 	#interrupt-cells = <1>;
> > };
> > 
> > intc2: intc at 2000 {
> > 	interrupt-controller;
> > 	#interrupt-cells = <2>;
> > };
> > 
> > device at 3000 {
> > 	interrupts-extended = <&intc1 5> <&intc2 3 4> <&intc1 6>;
> > };
> > 
> > 'interrupts-extended' will be proposed as a directly replacement of the
> > 'interrupts' property and it will eliminate the need for an
> > interrupt-map property. A node will be allowed to have one or the other,
> > but not both.
> > 
> > I'll write up a proper binding document and post for review.
> 
> Any progress on this ? I'll need to use multiple interrupts with different 
> parents in the near future, I can take this over if needed.

Grant posted the interrupts-extended binding few days ago:

http://lkml.org/lkml/2013/10/15/760
 
> I've also been thinking that we could possibly reuse the "interrupts" property 
> without defining a new "interrupts-extended". When parsing the property the 
> code would use the current DT bindings if an interrupt-parent is present, and 
> the new DT bindings if it isn't.

That could lead to mysterious failures easily as the binding
behaves in two different ways :) Probably best to have a separate
binding for it.

Regards,

Tony

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 10/20/2013 01:41 PM, Laurent Pinchart wrote:
> Hi Grant,
> 
> On Tuesday 17 September 2013 17:36:32 Grant Likely wrote:
>> On Thu, 12 Sep 2013 17:57:00 +0200, Alexander Holler wrote:
>>> Am 12.09.2013 17:19, schrieb Stephen Warren:
>>>> IRQs, DMA channels, and GPIOs are all different things. Their bindings
>>>> are defined independently. While it's good to define new types of
>>>> bindings consistently with other bindings, this hasn't always happened,
>>>> so you can make zero assumptions about the IRQ bindings by reading the
>>>> documentation for any other kind of binding.
>>>>
>>>> Multiple interrupts are defined as follows:
>>>> 	// Optional; otherwise inherited from parent/grand-parent/...
>>>> 	interrupt-parent = <&gpio6>;
>>>> 	// Must be in a fixed order, unless binding defines that the
>>>> 	// optional interrupt-names property is to be used.
>>>> 	interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>;
>>>> 	// Optional; binding for device defines whether it must
>>>> 	// be present
>>>> 	interrupt-names = "foo", "bar";
>>>>
>>>> If you need multiple interrupts, each with a different parent, you need
>>>> to use an interrupt-map property...
...
>> Actually, I think it is solveable but doing so requires a new binding
>> for interrupts. I took a shot at implementing it earlier this week and
>> I've got working patches that I'll be posting soon. I created a new
>> "interrupts-extended" property that uses a phandle+args type of
>> binding like this:
...
>> device at 3000 {
>> 	interrupts-extended = <&intc1 5> <&intc2 3 4> <&intc1 6>;
>> };
...
> Any progress on this ? I'll need to use multiple interrupts with different 
> parents in the near future, I can take this over if needed.
> 
> I've also been thinking that we could possibly reuse the "interrupts" property 
> without defining a new "interrupts-extended". When parsing the property the 
> code would use the current DT bindings if an interrupt-parent is present, and 
> the new DT bindings if it isn't.

interrupt-parents doesn't have to be present in individual nodes; it can
be inherited from the parent. That means you'd have to convert whole
sub-trees at once. It seems much more flexible to use a new property and
hence make it explicit what format the data is in.

[PATCH] RFC: interrupt consistency check for OF GPIO IRQs

[Laurent Pinchart]

Hi Stephen,

On Sunday 20 October 2013 22:35:04 Stephen Warren wrote:
> On 10/20/2013 01:41 PM, Laurent Pinchart wrote:
> > On Tuesday 17 September 2013 17:36:32 Grant Likely wrote:
> >> On Thu, 12 Sep 2013 17:57:00 +0200, Alexander Holler wrote:
> >>> Am 12.09.2013 17:19, schrieb Stephen Warren:
> >>>> IRQs, DMA channels, and GPIOs are all different things. Their bindings
> >>>> are defined independently. While it's good to define new types of
> >>>> bindings consistently with other bindings, this hasn't always happened,
> >>>> so you can make zero assumptions about the IRQ bindings by reading the
> >>>> documentation for any other kind of binding.
> >>>> 
> >>>> Multiple interrupts are defined as follows:
> >>>> 	// Optional; otherwise inherited from parent/grand-parent/...
> >>>> 	interrupt-parent = <&gpio6>;
> >>>> 	// Must be in a fixed order, unless binding defines that the
> >>>> 	// optional interrupt-names property is to be used.
> >>>> 	interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>;
> >>>> 	// Optional; binding for device defines whether it must
> >>>> 	// be present
> >>>> 	interrupt-names = "foo", "bar";
> >>>> 
> >>>> If you need multiple interrupts, each with a different parent, you need
> >>>> to use an interrupt-map property...
> 
> ...
> 
> >> Actually, I think it is solveable but doing so requires a new binding
> >> for interrupts. I took a shot at implementing it earlier this week and
> >> I've got working patches that I'll be posting soon. I created a new
> >> "interrupts-extended" property that uses a phandle+args type of
> 
> >> binding like this:
> ...
> 
> >> device at 3000 {
> >> 	interrupts-extended = <&intc1 5> <&intc2 3 4> <&intc1 6>;
> >> };
> 
> ...
> 
> > Any progress on this ? I'll need to use multiple interrupts with different
> > parents in the near future, I can take this over if needed.
> > 
> > I've also been thinking that we could possibly reuse the "interrupts"
> > property without defining a new "interrupts-extended". When parsing the
> > property the code would use the current DT bindings if an
> > interrupt-parent is present, and the new DT bindings if it isn't.
> 
> interrupt-parents doesn't have to be present in individual nodes; it can
> be inherited from the parent. That means you'd have to convert whole
> sub-trees at once.

Very good point. I agree with you, a new property is then better.

> It seems much more flexible to use a new property and hence make it explicit
> what format the data is in.

-- 
Regards,

Laurent Pinchart