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

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

[Linus Walleij]

Hi Lisheng,

this patch got a bit mangled but I get where you're going.

I think Meson needs to be augmented to use hierarchical gpiolib irqchip
because this seems to be what the system is doing.

So start with drivers/pinctrl/meson/Kconfig and add:

select GPIOLIB_IRQCHIP
select IRQ_DOMAIN_HIEARARCHY

Then use the generic hierarchical gpiolib irqchip as described
in Documentation/driver-api/gpio/driver.rst
Type
git grep child_to_parent_hwirq
for several examples of how to do this.

Yours,
Linus Walleij

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

[Jerome Brunet]

On Fri 04 Dec 2020 at 10:13, Linus Walleij <linus.walleij@linaro.org> wrote:

> Hi Lisheng,
>
> this patch got a bit mangled but I get where you're going.
>
> I think Meson needs to be augmented to use hierarchical gpiolib irqchip
> because this seems to be what the system is doing.
>
> So start with drivers/pinctrl/meson/Kconfig and add:
>
> select GPIOLIB_IRQCHIP
> select IRQ_DOMAIN_HIEARARCHY
>
> Then use the generic hierarchical gpiolib irqchip as described
> in Documentation/driver-api/gpio/driver.rst
> Type
> git grep child_to_parent_hwirq
> for several examples of how to do this.

One reason the irqchip has not been linked to the gpio controller so far
is IRQ_EDGE_BOTH which the irqchip does not support (expect for the
latest sm1 family)

This is a problem we discussed a couple of years ago.

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.

When drivers relying IRQ_EDGE_BOTH first try the `gpio_to_irq()`,
allocating the irq works but setting the type does not. We are then left
with unused allocated irqs (and we don't have much)

Frameworks using gpio_to_irq() are often capable() of parsing interrupt
properties directly too. So far, it was enough to work around the problem.

I admit, I have not been following gpiolib closely since then, maybe
some progress have been made

>
> Yours,
> Linus Walleij

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

[Andy Shevchenko]

On Fri, Dec 4, 2020 at 4:25 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Fri 04 Dec 2020 at 10:13, Linus Walleij <linus.walleij@linaro.org> 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.

I'm not sure why we are talking about legacy API which should not be used.
Besides that I didn't get what you meant under counterpart API (IRQ
descriptor has a mapping to the IRQ chip which keeps the mapping to
whatever hardware wants).

--
With Best Regards,
Andy Shevchenko

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

[Jerome Brunet]

On Mon 07 Dec 2020 at 11:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Dec 4, 2020 at 4:25 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>> On Fri 04 Dec 2020 at 10:13, Linus Walleij <linus.walleij@linaro.org> 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.
>
> I'm not sure why we are talking about legacy API which should not be
> used.

I would have been happy to forget about it, but it seems to be the topic
of the thread :)

> Besides that I didn't get what you meant under counterpart API (IRQ
> descriptor has a mapping to the IRQ chip which keeps the mapping to
> whatever hardware wants).

 * This HW has to create the mapping between GPIO and irq number
   dynamically. The number of irqs available is very limited.
 * We only get to know a mapping is required when gpio_to_irq() is called
 * There is no way to know when it is safe to dispose of the created
   mapping
 * 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

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.

If there was a way to know it is safe to dispose of the mapping, then
letting users of gpio_to_irq() try and fail would be OK, but we don't
have that AFAIK.

This is why gpio_to_irq() or gpiolib irqchip had not been added so far
on this HW. I don't think it is worth fixing, especially if the API is
considered to be legacy.

On this HW, getting an interupt from a pin is done by going directly
after the interrupt controller, like here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/amlogic/meson-gxbb-nanopi-k2.dts#n173

AFAICT, making pps-gpio parse an "interrupt" property should be doable
too.

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

[Linus Walleij]

On Mon, Dec 7, 2020 at 12:07 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Mon 07 Dec 2020 at 11:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Dec 4, 2020 at 4:25 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
> >> On Fri 04 Dec 2020 at 10:13, Linus Walleij <linus.walleij@linaro.org> 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.

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.

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?

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

>  * We only get to know a mapping is required when gpio_to_irq() is called

No that callback should not be used for that.

>  * 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? Then do not assign
interrupts to these drivers in the device tree. These requesting devices
and their requests are under your control. 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?

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

Yours,
Linus Walleij

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

[Jerome Brunet]

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

> On Mon, Dec 7, 2020 at 12:07 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>> On Mon 07 Dec 2020 at 11:18, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>> > On Fri, Dec 4, 2020 at 4:25 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>> >> On Fri 04 Dec 2020 at 10:13, Linus Walleij <linus.walleij@linaro.org> 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

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

[Linus Walleij]

On Mon, Dec 7, 2020 at 2:25 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
> [Me]

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

Aha I see now.

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

The gpiolib irqchip kind of suppose there is a 1-to-1 mapping between a
GPIO line and an IRQ, so I see the reasoning. That said, the callbacks
are code so a deviant remapping irq line "pool" could possibly be used.

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

I don't see any problem with gpio_to_irq() always returning -EINVAL
in situations like this.

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

True. But you can say "no" to anything trying to do that, that way you
will only hand out the irqs on a first-come-first serve basis to the clients
that use the irqs directly and thus you get it under control.

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

I think you can enable it and use gpiolibs hierarchical irqchip but let
gpio_to_irq() say no to everything.

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

OK I get it.

It's just that from my point of view using the hierarchical gpiolib irqchip
has a value in itself even if gpio_to_irq() isn't used at all because it brings
the criss-cross under control.

If the author wants to get some driver to work, such as MMC card detect
or ethernet phy or similar, what s/he should do is to go and fix the driver
to ask for an irq directly from the platform device or similar if it
can't get an
IRQ from the GPIO line by using gpiod_to_irq().

Yours,
Linus Walleij