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

[Javier Martinez Canillas <javier.martinez@collabora.co.uk>]

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