Re: 0001-add-amlogic-gpio-to-irq

[Jerome Brunet <jbrunet@xxxxxxxxxxxx>]

On Mon 07 Dec 2020 at 13:34, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:

> On Mon, Dec 7, 2020 at 12:07 PM Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote:
>> On Mon 07 Dec 2020 at 11:18, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>> > On Fri, Dec 4, 2020 at 4:25 PM Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote:
>> >> On Fri 04 Dec 2020 at 10:13, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>> >
>> >> This HW only has 8 irqs that can each be mapped to a pin. No direct
>> >> translation can be made, we have to allocate an irq to monitor the line.
>> >> So when gpio_to_irq() was called, we had to do that allocation dynamically
>> >> to return a valid irq number. Since there was no counter part to
>> >> gpio_to_irq(), those allocation cannot be freed during the lifetime of
>> >> the device.
>
> gpio_to_irq() is just a helper really and should not really be used to allocate
> anything.

Agreed

>
> In device tree systems, the GPIO provider should nominally present itsel
> as a dual-mode gpio-controller and interrupt-controller for example:
>
>                 gpio1: gpio@4e000000 {
>                         compatible = "cortina,gemini-gpio", "faraday,ftgpio010";
>                         reg = <0x4e000000 0x100>;
>                         interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;
>                         resets = <&syscon GEMINI_RESET_GPIO1>;
>                         clocks = <&syscon GEMINI_CLK_APB>;
>                         gpio-controller;
>                         #gpio-cells = <2>;
>                         interrupt-controller;
>                         #interrupt-cells = <2>;
>                 };
>
> The GPIOs are normally *not* translated to IRQs in this set-up. Rather the
> interrupts are requested by consumers using request_[threaded_]irq()
> which means you should be using the irqchip callbacks such as
> .irq_request_resources() and .irq_release_resources() to allocate one
> of the free irq lines to use. These will be called at the right points if a
> properly written driver requests an IRQ and when the driver is removed.
>
> In some rare cases gpio_to_irq() is used because all the driver knows is
> a GPIO number and it want to try to obtain an IRQ for it, and if a 1-to-1
> mapping exists it returns this number. This is not the norm, but the
> exception.

Sure

>
> So maybe the problem is that you need to go back and think about
> updating the DT bindings for this thing to include interrupt-controller
> as well?

We do
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-meson-gpio.c

That's actually the only thing we provide, on purpose.

>
>>  * This HW has to create the mapping between GPIO and irq number
>>    dynamically. The number of irqs available is very limited.
>
> This should be done using irq_chip callbacks.
>

Yes

>>  * We only get to know a mapping is required when gpio_to_irq() is called
>
> No that callback should not be used for that.

Agreed ... I was trying explain why we did *not* push a patch similar to what
was proposed here, or use gpiolib irqchip.

>
>>  * There is no way to know when it is safe to dispose of the created
>>    mapping
>
> The way that is done is when .irq_release_resources() is called.
>
>>  * Some drivers require a trigger type we don't support. These will create
>>    mappings and not use it because of the failure when .set_type() is
>>    called
>
> I don't quite understand this. Do you mean you are bombarded by pointless
> requests for interrupts that will not work anyways?

When we tried the approach suggested in this patch (again I agree it is
bad, which is why I'm against it), some drivers out there (I don't
remember which one TBH - that was 3 years ago) parsed the "gpio"
property and tried gpio_to_irq() and if it did not work then go
something else (like polling).

However the allocation stayed behind. It does not take much
"bombardment" when you only have 8.

> Then do not assign
> interrupts to these drivers in the device tree.

We don't.

> These requesting devices and their requests are under your control.

We control the ressources of the devices through DT, not the necessarily
drivers (which may be generic)

Some device needs the gpio, even if we don't want the irq.
We can't always prevent the driver to try gpio_to_irq().

This why I don't want gpio_to_irq() to be enabled on this HW, because it
would not be under our control anymore.


> The drivers should be able to
> back out and work without interrupt if request_irq() fails because it
> can't provide the type on interrupt you want:
>
> int irq = request_irq(irq, my_isr, IRQF_TRIGGER_RISING |
> IRQF_TRIGGER_FALLING, "My ISR", cookie);
> // This results in .irq_request_resources() and .irq_set_type()
> if (irq < 0) {
>    // Oopps out of IRQs or couldn't support double edges, bail out or
> use polling
> }
>
> Just do it like this (you might have to augment your drivers) and you'll
> be fine?
>

Again agreed. I'm really sorry if I have been that unclear about my
motive here. We already had that discussion 3 years ago, I totally
understand your point and agree. I was trying (and failing) to tell the
author of the patch that this approach had already been discussed in
past and that, unless gpiolib dramatically changed since then,
gpio_to_irq() should be used in this way and he should use irqchip we
already provide.

>> To answer your question, there an API which lets us know a mapping is
>> needed, but none to inform that it is not required anymore. The GPIO API
>> was not meant to used like this. Not saying it is good or bad, this is
>> just how it is.
>
> So don't use it?

Exactly

>
> Yours,
> Linus Walleij