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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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

Re: [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@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

Re: [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@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@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

Re: [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@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@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

Re: [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@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@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

Re: [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@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

Re: [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@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

Re: [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@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

Re: [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

Re: [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.

Re: [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@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

Re: [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@1000 {
	interrupt-controller;
	#interrupt-cells = <1>;
};

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

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

Re: [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@1000 {
> 	interrupt-controller;
> 	#interrupt-cells = <1>;
> };
> 
> intc2: intc@2000 {
> 	interrupt-controller;
> 	#interrupt-cells = <2>;
> };
> 
> device@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

Re: [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@1000 {
> > 	interrupt-controller;
> > 	#interrupt-cells = <1>;
> > };
> > 
> > intc2: intc@2000 {
> > 	interrupt-controller;
> > 	#interrupt-cells = <2>;
> > };
> > 
> > device@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

Re: [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@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.

Re: [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@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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html