[RFC] gpio: about the need to manage irq mapping dynamically.

[RFC] gpio: about the need to manage irq mapping dynamically.

[Jerome Brunet]

Hi Linus,

I would to (re)start the discussion around the management of the
irq_mapping in the gpio framework:

Many gpio controllers are able to provide a working irq to all the gpios at the
same time. On those controller we can create all the irq mapping during the
probe of the controller. Things are easy.

Then, there is another type of controller, with constraints that make it
impossible to provide a working irq to all pins at the same it. On Amlogic SoC
for example, we have only 8 parent irqs with and 8 to many (the number of pins)
router. Ressource has be to managed in a more "dynamic" fashion.

To handle this we tried:
- [0]: To create the mapping in the gpio_to_irq. Linus, you pointed out that
this is not allowed as gpio_to_irq is callable in irq context, therefore it
should not sleep. Actually 3 drivers [2] are calling gpio_to_irq in irq
handlers.
I would also add that "gpio_to_irq" has no "free" counter part, so this approach
leaks mappings. The fact that 26 gpio drivers [3] create mapping in the
gpio_to_irq callback shows that the problem of managing the irq mapping is not
specific to Amlogic and that an evolution might be useful.

- [1]: To create an empty domain to get a virq for each pins but delay the setup
of the interrupt hierarchy to the "irq_request_resources" callback. Testing this
approach led me to some nasty race conditions in the interrupt handlers. I
discussed this solution with Marc and he told me that doing the setup of the
interrupt hierarchy in the irqchip callbacks is "too late" . If I understood
correctly (Marc feel to correct me if necessary), you should only get a virq
when the full interrupt hierarchy is setup, from the triggering device to the
CPU. 

With this last comment, I don't think there is a (clean) way ATM for a gpio
driver to create the mapping "on demand". By "on-demand", I mean the consumer
drivers doing this kind of thing:

ret = request_irq(gpio_to_irq(GPIOX_Y), ... )

I would to like propose a way to fix that. It is not going to be a oneline fix,
but I'm convinced we can do it w/o breaking anything:

1) Introduce new gpio driver callbacks (.irq_prepare, .irq_unprepare): Drivers
will be free to implement those callbacks or not. Driver which can create all
their mappings at probe time would obviously skip those.

2) Introduce gpio_irq_prepare and gpio_irq_unprepare to the gpio API: This
functions would do refcounting to handle shared irq and avoid wrongly disposing
of used mappings. Then call the new drivers callbacks, if defined. A devm
version of gpio_irq_prepare might be usefull

3) Add calls to gpio_irq_prepare to consumer drivers probe/init which use the
gpio_to_irq callback. I don't think this is going to be that hard to do, but
it's going be to long and boring ...

As long as a platform does not implement these new callbacks, all the above is
basically a noop and should not change anything.

I think this course of action would allow us to slowly fix those offending
drivers [3], providing them with a solution.

As this is not going to be a small task, I would like to get your view on it
before going further.

Best Regards
Jerome

[0]: https://patchwork.ozlabs.org/patch/684208/
[1]: http://lkml.kernel.org/r/5b352c8d-a426-fa73-58b7-0c935979492b@gmail.com

[2]: Drivers using gpio_to_irq in irq_handlers
* drivers/gpio/gpio-ep93xx.c 
* drivers/gpio/gpio-pxa.c
* drivers/gpio/gpio-tegra.c

[3]: Drivers creating mapping in the gpio_to_irq callback (either through
irq_create_mapping, irq_create_fwspec_mapping or regmap_irq_get_virq)
* arch/sh/boards/mach-x3proto/gpio.c
* drivers/gpio/gpio-bcm-kona.c
* drivers/gpio/gpio-da9052.c
* drivers/gpio/gpio-da9055.c
* drivers/gpio/gpio-wm831x.c
* drivers/gpio/gpio-wm8994.c
* drivers/pinctrl/nomadik/pinctrl-abx500.c
* drivers/pinctrl/pinctrl-as3722.c
* drivers/pinctrl/pinctrl-rockchip.c
* drivers/pinctrl/samsung/pinctrl-samsung.c
* drivers/pinctrl/stm32/pinctrl-stm32.c
* arch/arm/plat-orion/gpio.c
* drivers/gpio/gpio-davinci.c
* drivers/gpio/gpio-em.c
* drivers/gpio/gpio-grgpio.c
* drivers/gpio/gpio-max77620.c
* drivers/gpio/gpio-mpc8xxx.c
* drivers/gpio/gpio-mvebu.c
* drivers/gpio/gpio-palmas.c
* drivers/gpio/gpio-tb10x.c
* drivers/gpio/gpio-tps6586x.c
* drivers/gpio/gpio-tz1090.c
* drivers/gpio/gpio-xgene-sb.c
* drivers/pinctrl/pinctrl-adi2.c
* drivers/pinctrl/samsung/pinctrl-exynos5440.c

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Linus Walleij]

On Thu, Jun 15, 2017 at 6:20 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:

> To handle this we tried:
> - [0]: To create the mapping in the gpio_to_irq. Linus, you pointed out that
> this is not allowed as gpio_to_irq is callable in irq context, therefore it
> should not sleep. Actually 3 drivers [2] are calling gpio_to_irq in irq
> handlers.

This is not the problem. The problem is that gpio_to_irq() is entirely
optional: it is not at all requires to have called gpio_to_irq() before using
an IRQ from a GPIO line, as the interrupt chips and gpio chips are
orthogonal.

So if gpio_to_irq() is called, then a mapping needs to be retrieved, using
an irqdomain or similar, and if one does not already exist, it should be mapped
at this point.

But actually doing the mapping/unmapping of IRQs and related resource
management should be handled by the irqchip/irqdomain, not ever by
gpio_to_irq().

And someone may just be requesting an IRQ from the irqchip without
calling gpio_to_irq(). Anytime. And with devicetree this happens all of the
time.

> I would also add that "gpio_to_irq" has no "free" counter part, so this approach
> leaks mappings.

Exactly.

> The fact that 26 gpio drivers [3] create mapping in the
> gpio_to_irq callback shows that the problem of managing the irq mapping is not
> specific to Amlogic and that an evolution might be useful.

Calling irq_create_mapping() in gpio_to_irq() is not a problem if
the mapping is free:ed elsewhere. But yeah, there are lots of old
drivers with old and erroneous solutions to this problem.

> - [1]: To create an empty domain to get a virq for each pins but delay the setup
> of the interrupt hierarchy to the "irq_request_resources" callback. Testing this
> approach led me to some nasty race conditions in the interrupt handlers. I
> discussed this solution with Marc and he told me that doing the setup of the
> interrupt hierarchy in the irqchip callbacks is "too late" . If I understood
> correctly (Marc feel to correct me if necessary), you should only get a virq
> when the full interrupt hierarchy is setup, from the triggering device to the
> CPU.

OK I am as always confused by "virq" which I guess means "virtual IRQ"
which is confusing since all Linux IRQ numbers are in some sense
virtual, and since this has nothing to do with virtualization, which is another
area where IRQchips are very delicate.

But I get it.

> With this last comment, I don't think there is a (clean) way ATM for a gpio
> driver to create the mapping "on demand". By "on-demand", I mean the consumer
> drivers doing this kind of thing:
>
> ret = request_irq(gpio_to_irq(GPIOX_Y), ... )
>
> I would to like propose a way to fix that.

OK solutions are good....

> It is not going to be a oneline fix,
> but I'm convinced we can do it w/o breaking anything:
>
> 1) Introduce new gpio driver callbacks (.irq_prepare, .irq_unprepare): Drivers
> will be free to implement those callbacks or not. Driver which can create all
> their mappings at probe time would obviously skip those.
>
> 2) Introduce gpio_irq_prepare and gpio_irq_unprepare to the gpio API: This
> functions would do refcounting to handle shared irq and avoid wrongly disposing
> of used mappings. Then call the new drivers callbacks, if defined. A devm
> version of gpio_irq_prepare might be usefull
>
> 3) Add calls to gpio_irq_prepare to consumer drivers probe/init which use the
> gpio_to_irq callback. I don't think this is going to be that hard to do, but
> it's going be to long and boring ...

So how are you intending to deal with mixed semantics when, as in the
devicetree case, the GPIO driver is also flagged as an interrupt-controller
and consumers can request IRQs from it with just platform_get_irq()
and expect it to work?

It seems this usecase drives a truck through that approach.

The keyword is separation of concerns, the irqchip abstraction should
deal with interrupts, we do not want the gpiochip to do half of the work.

Here is an example from arch/arm/boot/dts/ste-snowball.dts:

ethernet@0 {
        compatible = "smsc,lan9115";
        reg = <0 0x10000>;
        interrupts = <12 IRQ_TYPE_EDGE_RISING>;
        interrupt-parent = <&gpio4>;
(...)
};

Notice: no GPIOs are taken by this driver, this ethernet driver does not know
that its interrupt is supplied by a GPIO, in fact on many platforms
this ethernet
chip has a dedicated IRQ line so that should not be required, i.e. the ethernet
chip does not need to know that a GPIO controller is supplying this interrupt,
why should it? It is just some interrupt.

The driver looks like so drivers/net/ethernet/smsc/smsc911x.c:

irq = platform_get_irq(pdev, 0);

And that is all it does.

The above usecase is not uncommon.

If this interrupt+GPIO controller was using your scheme, where would these
new callbacks be called?

Yours,
Linus Walleij

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Jerome Brunet]

On Tue, 2017-06-20 at 10:39 +0200, Linus Walleij wrote:
> On Thu, Jun 15, 2017 at 6:20 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> 
> > To handle this we tried:
> > - [0]: To create the mapping in the gpio_to_irq. Linus, you pointed out that
> > this is not allowed as gpio_to_irq is callable in irq context, therefore it
> > should not sleep. Actually 3 drivers [2] are calling gpio_to_irq in irq
> > handlers.
> 
> This is not the problem. The problem is that gpio_to_irq() is entirely
> optional: it is not at all requires to have called gpio_to_irq() before using
> an IRQ from a GPIO line, as the interrupt chips and gpio chips are
> orthogonal.

Agreed. no problem with this
If you don't use gpio_to_irq, they are *completely* orthogonal.
If you do use gpio_to_irq, you are creating this tiny relationship which makes
all the difference, IMO.

> 
> So if gpio_to_irq() is called, then a mapping needs to be retrieved, using
> an irqdomain or similar, and if one does not already exist, it should be
> mapped
> at this point.
> 
> But actually doing the mapping/unmapping of IRQs and related resource
> management should be handled by the irqchip/irqdomain, not ever by
> gpio_to_irq().
> 
> And someone may just be requesting an IRQ from the irqchip without
> calling gpio_to_irq(). Anytime. And with devicetree this happens all of the
> time.

Yes, for sure. But in this case there no issue as the mapping creation is
handled in device-tree code. PSB

> 
> > I would also add that "gpio_to_irq" has no "free" counter part, so this
> > approach
> > leaks mappings.
> 
> Exactly.
> 
> > The fact that 26 gpio drivers [3] create mapping in the
> > gpio_to_irq callback shows that the problem of managing the irq mapping is
> > not
> > specific to Amlogic and that an evolution might be useful.
> 
> Calling irq_create_mapping() in gpio_to_irq() is not a problem if
> the mapping is free:ed elsewhere. But yeah, there are lots of old
> drivers with old and erroneous solutions to this problem.

A lot of these drivers are old indeed, and dealing with all this history must be
quite a challenge ;)
However, some are fairly recent.

> 
> > - [1]: To create an empty domain to get a virq for each pins but delay the
> > setup
> > of the interrupt hierarchy to the "irq_request_resources" callback. Testing
> > this
> > approach led me to some nasty race conditions in the interrupt handlers. I
> > discussed this solution with Marc and he told me that doing the setup of the
> > interrupt hierarchy in the irqchip callbacks is "too late" . If I understood
> > correctly (Marc feel to correct me if necessary), you should only get a virq
> > when the full interrupt hierarchy is setup, from the triggering device to
> > the
> > CPU.
> 
> OK I am as always confused by "virq" which I guess means "virtual IRQ"
> which is confusing since all Linux IRQ numbers are in some sense
> virtual, and since this has nothing to do with virtualization, which is
> another
> area where IRQchips are very delicate.
> 

Just using the usual irq wording (hwirq for the numbers specific to the
controller, virq for the linux interrupt numbers)

> But I get it.
> 
> > With this last comment, I don't think there is a (clean) way ATM for a gpio
> > driver to create the mapping "on demand". By "on-demand", I mean the
> > consumer
> > drivers doing this kind of thing:
> > 
> > ret = request_irq(gpio_to_irq(GPIOX_Y), ... )
> > 
> > I would to like propose a way to fix that.
> 
> OK solutions are good....
> 
> > It is not going to be a oneline fix,
> > but I'm convinced we can do it w/o breaking anything:
> > 
> > 1) Introduce new gpio driver callbacks (.irq_prepare, .irq_unprepare):
> > Drivers
> > will be free to implement those callbacks or not. Driver which can create
> > all
> > their mappings at probe time would obviously skip those.
> > 
> > 2) Introduce gpio_irq_prepare and gpio_irq_unprepare to the gpio API: This
> > functions would do refcounting to handle shared irq and avoid wrongly
> > disposing
> > of used mappings. Then call the new drivers callbacks, if defined. A devm
> > version of gpio_irq_prepare might be usefull
> > 
> > 3) Add calls to gpio_irq_prepare to consumer drivers probe/init which use
> > the
> > gpio_to_irq callback. I don't think this is going to be that hard to do, but
> > it's going be to long and boring ...
> 
> So how are you intending to deal with mixed semantics when, as in the
> devicetree case, the GPIO driver is also flagged as an interrupt-controller
> and consumers can request IRQs from it with just platform_get_irq()
> and expect it to work?

I don't think there any issue with this. PSB

> 
> It seems this usecase drives a truck through that approach.
> 
> The keyword is separation of concerns, the irqchip abstraction should
> deal with interrupts, we do not want the gpiochip to do half of the work.
> 
> Here is an example from arch/arm/boot/dts/ste-snowball.dts:
> 
> ethernet@0 {
>         compatible = "smsc,lan9115";
>         reg = <0 0x10000>;
>         interrupts = <12 IRQ_TYPE_EDGE_RISING>;
>         interrupt-parent = <&gpio4>;
> (...)
> };
> 
> Notice: no GPIOs are taken by this driver, this ethernet driver does not know
> that its interrupt is supplied by a GPIO, in fact on many platforms
> this ethernet
> chip has a dedicated IRQ line so that should not be required, i.e. the
> ethernet
> chip does not need to know that a GPIO controller is supplying this interrupt,
> why should it? It is just some interrupt.

Agreed. 

> 
> The driver looks like so drivers/net/ethernet/smsc/smsc911x.c:
> 
> irq = platform_get_irq(pdev, 0);
> 
> And that is all it does.
> 
> The above usecase is not uncommon.
> 

100% agreed, this is a common use case. In fact, I think driver should prefer
doing so instead of using gpio_to_irq when possible ... but this another issue
and we have to deal with what we have, right ?

So there is 2 use cases:
* What you described above: This is indeed completely orthogonal to gpio
framework. It will be using the irqchip callbacks only. In this particular case,
it is the device tree platform code which will handle the mapping creation for
us.
See: of_device_alloc in drivers/of/platform.c:144, this will eventually call
irq_parse_and_map, which calls irq_create_mapping. 

* When a driver use the following approach:
-- ret = request_irq(gpio_to_irq(GPIOX_Y), ... )
of course the device tree code won't create the mapping for us.
It needs to be created in some other way.
I believe, only the gpio framework can do it at this point, and this where there
is a tiny relationship between the irq and gpio part, as mentioned above.

If we are lucky, all the mappings can be created for all pins at probe time. No
real management to be done in this case, just find your mapping a be done with
it.

If, for whatever reason, the mappings can't all be created at probe time, then
you have problem with the drivers using request_irq(gpio_to_irq(GPIOX_Y), ... )
style ... where should we create this mapping ? There is no place holder for it.

IOW, DT provides a way to get irq and it manages the mapping creation. By
providing the gpio_to_irq call, the gpio framework also provides a path to get
interrupts for consumer driver. Mapping also needs to be managed. I think there
a hole there.

I think the solution described in this RFC provide a solution for this, but does
not change anything for the other approaches (DT-based interrupt specifier or
probe time mapping creation)

> If this interrupt+GPIO controller was using your scheme, where would these
> new callbacks be called?

In the consumer drivers which use the "request_irq(gpio_to_irq(GPIOX_Y), ... )"
approach, most likely in the probe/init function.

Driver have a choice, either
* use the gpio approach. I believe these drivers are actually interested in the
gpio state but would like to avoid polling monitoring. 
* use the DT approach, like your example with the ste-snowball. You don't care
about the gpio here, all care about is the event.

Regards
Jerome
 
> 
> Yours,
> Linus Walleij

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Linus Walleij]

On Tue, Jun 20, 2017 at 12:26 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:

So I finally understood that what you want is to handle those cases
where gpio_to_irq() is currently in use, and not expand the use of that
function. And that is good.

> On Tue, 2017-06-20 at 10:39 +0200, Linus Walleij wrote:

>> > The fact that 26 gpio drivers [3] create mapping in the
>> > gpio_to_irq callback shows that the problem of managing the irq mapping is
>> > not
>> > specific to Amlogic and that an evolution might be useful.
>>
>> Calling irq_create_mapping() in gpio_to_irq() is not a problem if
>> the mapping is free:ed elsewhere. But yeah, there are lots of old
>> drivers with old and erroneous solutions to this problem.
>
> A lot of these drivers are old indeed, and dealing with all this history must be
> quite a challenge ;)
> However, some are fairly recent.

Unfortunately the API has merits. Like for keys: OK there is a key
for an GPIO line, and then it asks "can I have an IRQ for that"?
There are cases where the IRQ is really optional, not a required
feature.

> So there is 2 use cases:
> * What you described above: This is indeed completely orthogonal to gpio
> framework. It will be using the irqchip callbacks only. In this particular case,
> it is the device tree platform code which will handle the mapping creation for
> us.

OK

So the problem is really to use drivers that cannot do all their
mappings at probe
time with drivers that use gpio_to_irq(), right?

So, if the idea is to patch *ALL* drivers using gpio_to_irq(), I think
for each case
it should be investigated whether that should really be using gpio_to_irq()
but I suspect they are all pretty solid.

Well you're probably right, the API has to change for the case where the number
of IRQs are limited and cannot just be handled out to the left and right, but we
should really replace *ALL* occurences of gpio_to_irq() with a pair of
gpio_irq_prepare() that also returns the valid IRQ if found, and
gpio_irq_unprepare()
that removes it.

For the transitional period, gpio_to_irq() should *FAIL* if gpio_irq_prepare()
was called first for the same GPIO line.

Eventually gpio_to_irq() should be DELETED and replaced in full with
the prepare/unprepare calls.

And I would like strong confidence that the change will be carried all the way
through, not half-done and left to me to complete. I already have too many
problems of that type.

Yours,
Linus Walleij

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Jerome Brunet]

On Tue, 2017-06-20 at 18:37 +0200, Linus Walleij wrote:
> On Tue, Jun 20, 2017 at 12:26 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> 
> So I finally understood that what you want is to handle those cases
> where gpio_to_irq() is currently in use, and not expand the use of that
> function. And that is good.

Indeed

> 
> > On Tue, 2017-06-20 at 10:39 +0200, Linus Walleij wrote:
> > > > The fact that 26 gpio drivers [3] create mapping in the
> > > > gpio_to_irq callback shows that the problem of managing the irq mapping
> > > > is
> > > > not
> > > > specific to Amlogic and that an evolution might be useful.
> > > 
> > > Calling irq_create_mapping() in gpio_to_irq() is not a problem if
> > > the mapping is free:ed elsewhere. But yeah, there are lots of old
> > > drivers with old and erroneous solutions to this problem.
> > 
> > A lot of these drivers are old indeed, and dealing with all this history
> > must be
> > quite a challenge ;)
> > However, some are fairly recent.
> 
> Unfortunately the API has merits. Like for keys: OK there is a key
> for an GPIO line, and then it asks "can I have an IRQ for that"?
> There are cases where the IRQ is really optional, not a required
> feature.
> 

I think you misunderstood me. I have nothing against the API. I absolutely want
to keep it. I'd like to extend it.

> > So there is 2 use cases:
> > * What you described above: This is indeed completely orthogonal to gpio
> > framework. It will be using the irqchip callbacks only. In this particular
> > case,
> > it is the device tree platform code which will handle the mapping creation
> > for
> > us.
> 
> OK
> 
> So the problem is really to use drivers that cannot do all their
> mappings at probe
> time with drivers that use gpio_to_irq(), right?

Yep

> 
> So, if the idea is to patch *ALL* drivers using gpio_to_irq(), I think
> for each case
> it should be investigated whether that should really be using gpio_to_irq()
> but I suspect they are all pretty solid.
> 
> Well you're probably right, the API has to change for the case where the
> number
> of IRQs are limited and cannot just be handled out to the left and right, but
> we
> should really replace *ALL* occurences of gpio_to_irq() with a pair of
> gpio_irq_prepare() that also returns the valid IRQ if found, and
> gpio_irq_unprepare()
> that removes it.
> 
> For the transitional period, gpio_to_irq() should *FAIL* if gpio_irq_prepare()
> was called first for the same GPIO line.
> 
> Eventually gpio_to_irq() should be DELETED and replaced in full with
> the prepare/unprepare calls.

Woahh, that's not what I meant. gpio_to_irq should stay. Getting rid of it would
be a mess and it is a useful call.

The gpio_irq_prepare is meant so that the consumer can tell the gpio driver it
will want to get irq from a particular gpio at some point.

IOW, it's the consumer saying to the gpio driver "please do whatever you need to
do, if anything, so this gpio can generate an interrupt"

This is a much simpler change. Using devm, all we need is to put a
devm_gpio_irq_prepare(<gpio_num>) in the probe of the drivers using gpio_to_irq.

Mandating call to gpio_irq_prepare before any call to gpio_to_irq will be fairly
easy.

> 
> And I would like strong confidence that the change will be carried all the way
> through, not half-done and left to me to complete. I already have too many
> problems of that type.


Fair enough.

> 
> Yours,
> Linus Walleij

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Linus Walleij]

On Tue, Jun 20, 2017 at 7:23 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Tue, 2017-06-20 at 18:37 +0200, Linus Walleij wrote:

>> Eventually gpio_to_irq() should be DELETED and replaced in full with
>> the prepare/unprepare calls.
>
> Woahh, that's not what I meant. gpio_to_irq should stay. Getting rid of it would
> be a mess and it is a useful call.
>
> The gpio_irq_prepare is meant so that the consumer can tell the gpio driver it
> will want to get irq from a particular gpio at some point.
>
> IOW, it's the consumer saying to the gpio driver "please do whatever you need to
> do, if anything, so this gpio can generate an interrupt"
>
> This is a much simpler change. Using devm, all we need is to put a
> devm_gpio_irq_prepare(<gpio_num>) in the probe of the drivers using gpio_to_irq.
>
> Mandating call to gpio_irq_prepare before any call to gpio_to_irq will be fairly
> easy.

So why can't we just return the IRQ from prepare() and be done with it
instead of having two calls? (Plus a third eventual unprepare()).

Clocks, regulators and godknowswhat is managed by two symmetrical
calls, so why shouldn't GPIO IRQs be?

It would be counterintuitive to have a third call in the middle.

Yours,
Linus Walleij

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Jerome Brunet]

On Wed, 2017-06-21 at 22:50 +0200, Linus Walleij wrote:
> On Tue, Jun 20, 2017 at 7:23 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> > On Tue, 2017-06-20 at 18:37 +0200, Linus Walleij wrote:
> > > Eventually gpio_to_irq() should be DELETED and replaced in full with
> > > the prepare/unprepare calls.
> > 
> > Woahh, that's not what I meant. gpio_to_irq should stay. Getting rid of it
> > would
> > be a mess and it is a useful call.
> > 
> > The gpio_irq_prepare is meant so that the consumer can tell the gpio driver
> > it
> > will want to get irq from a particular gpio at some point.
> > 
> > IOW, it's the consumer saying to the gpio driver "please do whatever you
> > need to
> > do, if anything, so this gpio can generate an interrupt"
> > 
> > This is a much simpler change. Using devm, all we need is to put a
> > devm_gpio_irq_prepare(<gpio_num>) in the probe of the drivers using
> > gpio_to_irq.
> > 
> > Mandating call to gpio_irq_prepare before any call to gpio_to_irq will be
> > fairly
> > easy.
> 
> So why can't we just return the IRQ from prepare() and be done with it

We can return it here as well, it's actually a good idea. New drivers could just
use that one if they are keeping track of the irq number.

> instead of having two calls? (Plus a third eventual unprepare()).
> 
> Clocks, regulators and godknowswhat is managed by two symmetrical
> calls, so why shouldn't GPIO IRQs be?

The approach is exactly the same as what we trying to follow in the irq
framework:

framework     | irq                 | gpio
-----------------------------------------------------
index         | hwirq               | gpio_num
creation      | irq_create_mapping  | gpio_irq_prepare   ?
removal       | irq_dispose_mapping | gpio_irq_unprepare ?
(fast) lookup | irq_find_mapping    | gpio_to_irq

We are going to have at lookup function somehow, why not expose it ?

Another reason for keeping gpio_to_irq is that many existing drivers using the
callback don't keep track of their irq number, just the gpio. For every irq
operation, they call gpio_to_irq. Things like this:

* irq_enable(gpio_to_irq(<gpio_num>))
* irq_release(gpio_to_irq(<gpio_num>))
* etc ...

It's a bit lazy maybe, but I don't think there is anything utterly wrong with
it.

Getting rid of gpio_to_irq would mean reworking all those drivers so they keep
track of their irq number. I think this would be a mess for a very little gain.

Also, except for the 26 gpio drivers I have listed, the rest should already be
implementing what qualify as a "lookup" function in gpio_to_irq. I don't think
we should by modifying every single gpio driver when there is a solution to
'surgically' address the matter.

The series would already affect a significant amount of drivers, I'm trying to
keep it as simple an contained as possible.

If that is OK with you, I could send an RFC implementing the idea but fixing
only 1 gpio driver and 2 or 3 consumer. We would have actual code to discuss on,
it might be easier. 

> 
> It would be counterintuitive to have a third call in the middle.
> 
> Yours,
> Linus Walleij

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Grygorii Strashko]

On 06/22/2017 09:25 AM, Jerome Brunet wrote:
> On Wed, 2017-06-21 at 22:50 +0200, Linus Walleij wrote:
>> On Tue, Jun 20, 2017 at 7:23 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>> On Tue, 2017-06-20 at 18:37 +0200, Linus Walleij wrote:
>>>> Eventually gpio_to_irq() should be DELETED and replaced in full with
>>>> the prepare/unprepare calls.
>>>
>>> Woahh, that's not what I meant. gpio_to_irq should stay. Getting rid of it
>>> would
>>> be a mess and it is a useful call.
>>>
>>> The gpio_irq_prepare is meant so that the consumer can tell the gpio driver
>>> it
>>> will want to get irq from a particular gpio at some point.
>>>
>>> IOW, it's the consumer saying to the gpio driver "please do whatever you
>>> need to
>>> do, if anything, so this gpio can generate an interrupt"
>>>
>>> This is a much simpler change. Using devm, all we need is to put a
>>> devm_gpio_irq_prepare(<gpio_num>) in the probe of the drivers using
>>> gpio_to_irq.
>>>
>>> Mandating call to gpio_irq_prepare before any call to gpio_to_irq will be
>>> fairly
>>> easy.
>>
>> So why can't we just return the IRQ from prepare() and be done with it
> 
> We can return it here as well, it's actually a good idea. New drivers could just
> use that one if they are keeping track of the irq number.
> 
>> instead of having two calls? (Plus a third eventual unprepare()).
>>
>> Clocks, regulators and godknowswhat is managed by two symmetrical
>> calls, so why shouldn't GPIO IRQs be?
> 
> The approach is exactly the same as what we trying to follow in the irq
> framework:
> 
> framework     | irq                 | gpio
> -----------------------------------------------------
> index         | hwirq               | gpio_num
> creation      | irq_create_mapping  | gpio_irq_prepare   ?
> removal       | irq_dispose_mapping | gpio_irq_unprepare ?
> (fast) lookup | irq_find_mapping    | gpio_to_irq
> 
> We are going to have at lookup function somehow, why not expose it ?
> 
> Another reason for keeping gpio_to_irq is that many existing drivers using the
> callback don't keep track of their irq number, just the gpio. For every irq
> operation, they call gpio_to_irq. Things like this:
> 
> * irq_enable(gpio_to_irq(<gpio_num>))
> * irq_release(gpio_to_irq(<gpio_num>))
> * etc ...
> 
> It's a bit lazy maybe, but I don't think there is anything utterly wrong with
> it.
> 
> Getting rid of gpio_to_irq would mean reworking all those drivers so they keep
> track of their irq number. I think this would be a mess for a very little gain.
> 
> Also, except for the 26 gpio drivers I have listed, the rest should already be
> implementing what qualify as a "lookup" function in gpio_to_irq. I don't think
> we should by modifying every single gpio driver when there is a solution to
> 'surgically' address the matter.
> 
> The series would already affect a significant amount of drivers, I'm trying to
> keep it as simple an contained as possible.
> 
> If that is OK with you, I could send an RFC implementing the idea but fixing
> only 1 gpio driver and 2 or 3 consumer. We would have actual code to discuss on,
> it might be easier.
> 

I'd like to add my 5 cents here :)
1) "To create the mapping in the gpio_to_irq. Linus, you pointed out that this is not
 allowed as gpio_to_irq is callable in irq context, therefore it should not sleep.
 Actually 3 drivers [2] are calling gpio_to_irq in irq handlers."

It's not allowed to call gpio_to_irq() from IRQ handlers, as mappings should already exist at
that time and It might require to do sleepable calls to crate IRQ mappings and configure HW. 

Drivers, pointed out in first e-mail, should use other APIs in their IRQ handlers:
drivers/gpio/gpio-ep93xx.c 
^ direct call to ep93xx_gpio_to_irq()
* drivers/gpio/gpio-pxa.c
 ^ use irq_find_mapping()
* drivers/gpio/gpio-tegra.c
 ^ use irq_find_mapping()

Also note, IRQ mappings can be created as dynamically (each time  gpio_to_irq() is called) as
statically (in probe). The last approach is widely used in gpio drivers due to compatibility and
legacy reasons. 

2) As per above I do not really understand why gpio_irq_prepare() is required.

3) As per problem description irq_dispose_mapping() equivalent for GPIO IRQs might be required,
but what is the real use-case? Modules reloading or unloading one module and loading another instead?
Usually GPIO and IRQ configuration is static and defined in DT, so IRQ mapping created by first call to
gpio_to_irq()/platform_get_irq()/of_irq_get() and any subsequent calls just re-use already created mapping.

-- 
regards,
-grygorii

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Jerome Brunet]

On Tue, 2017-06-27 at 12:49 -0500, Grygorii Strashko wrote:
> 
> On 06/22/2017 09:25 AM, Jerome Brunet wrote:
> > On Wed, 2017-06-21 at 22:50 +0200, Linus Walleij wrote:
> > > On Tue, Jun 20, 2017 at 7:23 PM, Jerome Brunet <jbrunet@baylibre.com>
> > > wrote:
> > > > On Tue, 2017-06-20 at 18:37 +0200, Linus Walleij wrote:
> > > > > Eventually gpio_to_irq() should be DELETED and replaced in full with
> > > > > the prepare/unprepare calls.
> > > > 
> > > > Woahh, that's not what I meant. gpio_to_irq should stay. Getting rid of
> > > > it
> > > > would
> > > > be a mess and it is a useful call.
> > > > 
> > > > The gpio_irq_prepare is meant so that the consumer can tell the gpio
> > > > driver
> > > > it
> > > > will want to get irq from a particular gpio at some point.
> > > > 
> > > > IOW, it's the consumer saying to the gpio driver "please do whatever you
> > > > need to
> > > > do, if anything, so this gpio can generate an interrupt"
> > > > 
> > > > This is a much simpler change. Using devm, all we need is to put a
> > > > devm_gpio_irq_prepare(<gpio_num>) in the probe of the drivers using
> > > > gpio_to_irq.
> > > > 
> > > > Mandating call to gpio_irq_prepare before any call to gpio_to_irq will
> > > > be
> > > > fairly
> > > > easy.
> > > 
> > > So why can't we just return the IRQ from prepare() and be done with it
> > 
> > We can return it here as well, it's actually a good idea. New drivers could
> > just
> > use that one if they are keeping track of the irq number.
> > 
> > > instead of having two calls? (Plus a third eventual unprepare()).
> > > 
> > > Clocks, regulators and godknowswhat is managed by two symmetrical
> > > calls, so why shouldn't GPIO IRQs be?
> > 
> > The approach is exactly the same as what we trying to follow in the irq
> > framework:
> > 
> > framework     | irq                 | gpio
> > -----------------------------------------------------
> > index         | hwirq               | gpio_num
> > creation      | irq_create_mapping  | gpio_irq_prepare   ?
> > removal       | irq_dispose_mapping | gpio_irq_unprepare ?
> > (fast) lookup | irq_find_mapping    | gpio_to_irq
> > 
> > We are going to have at lookup function somehow, why not expose it ?
> > 
> > Another reason for keeping gpio_to_irq is that many existing drivers using
> > the
> > callback don't keep track of their irq number, just the gpio. For every irq
> > operation, they call gpio_to_irq. Things like this:
> > 
> > * irq_enable(gpio_to_irq(<gpio_num>))
> > * irq_release(gpio_to_irq(<gpio_num>))
> > * etc ...
> > 
> > It's a bit lazy maybe, but I don't think there is anything utterly wrong
> > with
> > it.
> > 
> > Getting rid of gpio_to_irq would mean reworking all those drivers so they
> > keep
> > track of their irq number. I think this would be a mess for a very little
> > gain.
> > 
> > Also, except for the 26 gpio drivers I have listed, the rest should already
> > be
> > implementing what qualify as a "lookup" function in gpio_to_irq. I don't
> > think
> > we should by modifying every single gpio driver when there is a solution to
> > 'surgically' address the matter.
> > 
> > The series would already affect a significant amount of drivers, I'm trying
> > to
> > keep it as simple an contained as possible.
> > 
> > If that is OK with you, I could send an RFC implementing the idea but fixing
> > only 1 gpio driver and 2 or 3 consumer. We would have actual code to discuss
> > on,
> > it might be easier.
> > 
> 
> I'd like to add my 5 cents here :)
> 1) "To create the mapping in the gpio_to_irq. Linus, you pointed out that this
> is not
>  allowed as gpio_to_irq is callable in irq context, therefore it should not
> sleep.
>  Actually 3 drivers [2] are calling gpio_to_irq in irq handlers."
> 
> It's not allowed to call gpio_to_irq() from IRQ handlers, as mappings should
> already exist at
> that time and It might require to do sleepable calls to crate IRQ mappings and
> configure HW. 
> 
> Drivers, pointed out in first e-mail, should use other APIs in their IRQ
> handlers:
> drivers/gpio/gpio-ep93xx.c 
> ^ direct call to ep93xx_gpio_to_irq()
> * drivers/gpio/gpio-pxa.c
>  ^ use irq_find_mapping()
> * drivers/gpio/gpio-tegra.c
>  ^ use irq_find_mapping()
> 
> Also note, IRQ mappings can be created as dynamically (each
> time  gpio_to_irq() is called)

Not according to a previous reply from Linus. Right or wrong, it would have made
my life a lot easier if it was OK :)

>  as
> statically (in probe). The last approach is widely used in gpio drivers due to
> compatibility and
> legacy reasons.

Agreed this is the current situation.

> 
> 2) As per above I do not really understand why gpio_irq_prepare() is required.

I'd like to point you the thread which initially triggered this rfc:
https://patchwork.ozlabs.org/patch/684208/

At the time Linus strongly rejected the idea of calling  irq_create_mapping (or
any sleeping functions) in gpio_to_irq: please see the reply from Oct 26, 2016
(sorry for quoting such an old discussion but this is really the starting point)

* Me: There is really a *lot* of gpio drivers which use irq_create_mapping in
the to_irq callback, are these all wrong ?
* Linus: Yes they are all wrong. They should all be using irq_find_mapping().

* Me: If this should not be used, what should we all do instead ?
* Linus: Call irq_create_mapping() in some other place.

gpio_prepare_irq is a proposition for this 'other place'.

> 
> 3) As per problem description irq_dispose_mapping() equivalent for GPIO IRQs
> might be required,
> but what is the real use-case? Modules reloading or unloading one module and
> loading another instead?
> Usually GPIO and IRQ configuration is static and defined in DT, so IRQ mapping
> created by first call to
> gpio_to_irq()/platform_get_irq()/of_irq_get() and any subsequent calls just
> re-use already created mapping.

Providing that you always create mapping for the same pins, yes
What happens when gpio irq (the pin) is different each time ? and you exhaust
the ressource ?

It is a corner case, but possible isn't it ? With the gpio char driver
interface  maybe. You would have to reboot to flush the old mappings and
continue, right ? 

You could also fail to set the trigger type (which you won't be able to set in
gpio_to_irq). If so, shouldn't you release the mapping ? (that's real use-case
we are having by the way).

Cheers
Jerome

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Grygorii Strashko]

On 06/27/2017 01:25 PM, Jerome Brunet wrote:
> On Tue, 2017-06-27 at 12:49 -0500, Grygorii Strashko wrote:
>>
>> On 06/22/2017 09:25 AM, Jerome Brunet wrote:
>>> On Wed, 2017-06-21 at 22:50 +0200, Linus Walleij wrote:
>>>> On Tue, Jun 20, 2017 at 7:23 PM, Jerome Brunet <jbrunet@baylibre.com>
>>>> wrote:
>>>>> On Tue, 2017-06-20 at 18:37 +0200, Linus Walleij wrote:
>>>>>> Eventually gpio_to_irq() should be DELETED and replaced in full with
>>>>>> the prepare/unprepare calls.
>>>>>
>>>>> Woahh, that's not what I meant. gpio_to_irq should stay. Getting rid of
>>>>> it
>>>>> would
>>>>> be a mess and it is a useful call.
>>>>>
>>>>> The gpio_irq_prepare is meant so that the consumer can tell the gpio
>>>>> driver
>>>>> it
>>>>> will want to get irq from a particular gpio at some point.
>>>>>
>>>>> IOW, it's the consumer saying to the gpio driver "please do whatever you
>>>>> need to
>>>>> do, if anything, so this gpio can generate an interrupt"
>>>>>
>>>>> This is a much simpler change. Using devm, all we need is to put a
>>>>> devm_gpio_irq_prepare(<gpio_num>) in the probe of the drivers using
>>>>> gpio_to_irq.
>>>>>
>>>>> Mandating call to gpio_irq_prepare before any call to gpio_to_irq will
>>>>> be
>>>>> fairly
>>>>> easy.
>>>>
>>>> So why can't we just return the IRQ from prepare() and be done with it
>>>
>>> We can return it here as well, it's actually a good idea. New drivers could
>>> just
>>> use that one if they are keeping track of the irq number.
>>>
>>>> instead of having two calls? (Plus a third eventual unprepare()).
>>>>
>>>> Clocks, regulators and godknowswhat is managed by two symmetrical
>>>> calls, so why shouldn't GPIO IRQs be?
>>>
>>> The approach is exactly the same as what we trying to follow in the irq
>>> framework:
>>>
>>> framework     | irq                 | gpio
>>> -----------------------------------------------------
>>> index         | hwirq               | gpio_num
>>> creation      | irq_create_mapping  | gpio_irq_prepare   ?
>>> removal       | irq_dispose_mapping | gpio_irq_unprepare ?
>>> (fast) lookup | irq_find_mapping    | gpio_to_irq
>>>
>>> We are going to have at lookup function somehow, why not expose it ?
>>>
>>> Another reason for keeping gpio_to_irq is that many existing drivers using
>>> the
>>> callback don't keep track of their irq number, just the gpio. For every irq
>>> operation, they call gpio_to_irq. Things like this:
>>>
>>> * irq_enable(gpio_to_irq(<gpio_num>))
>>> * irq_release(gpio_to_irq(<gpio_num>))
>>> * etc ...
>>>
>>> It's a bit lazy maybe, but I don't think there is anything utterly wrong
>>> with
>>> it.
>>>
>>> Getting rid of gpio_to_irq would mean reworking all those drivers so they
>>> keep
>>> track of their irq number. I think this would be a mess for a very little
>>> gain.
>>>
>>> Also, except for the 26 gpio drivers I have listed, the rest should already
>>> be
>>> implementing what qualify as a "lookup" function in gpio_to_irq. I don't
>>> think
>>> we should by modifying every single gpio driver when there is a solution to
>>> 'surgically' address the matter.
>>>
>>> The series would already affect a significant amount of drivers, I'm trying
>>> to
>>> keep it as simple an contained as possible.
>>>
>>> If that is OK with you, I could send an RFC implementing the idea but fixing
>>> only 1 gpio driver and 2 or 3 consumer. We would have actual code to discuss
>>> on,
>>> it might be easier.
>>>
>>
>> I'd like to add my 5 cents here :)
>> 1) "To create the mapping in the gpio_to_irq. Linus, you pointed out that this
>> is not
>>   allowed as gpio_to_irq is callable in irq context, therefore it should not
>> sleep.
>>   Actually 3 drivers [2] are calling gpio_to_irq in irq handlers."
>>
>> It's not allowed to call gpio_to_irq() from IRQ handlers, as mappings should
>> already exist at
>> that time and It might require to do sleepable calls to crate IRQ mappings and
>> configure HW.
>>
>> Drivers, pointed out in first e-mail, should use other APIs in their IRQ
>> handlers:
>> drivers/gpio/gpio-ep93xx.c
>> ^ direct call to ep93xx_gpio_to_irq()
>> * drivers/gpio/gpio-pxa.c
>>   ^ use irq_find_mapping()
>> * drivers/gpio/gpio-tegra.c
>>   ^ use irq_find_mapping()
>>
>> Also note, IRQ mappings can be created as dynamically (each
>> time  gpio_to_irq() is called)
> 
> Not according to a previous reply from Linus. Right or wrong, it would have made
> my life a lot easier if it was OK :)
> 
>>   as
>> statically (in probe). The last approach is widely used in gpio drivers due to
>> compatibility and
>> legacy reasons.
> 
> Agreed this is the current situation.
> 
>>
>> 2) As per above I do not really understand why gpio_irq_prepare() is required.
> 
> I'd like to point you the thread which initially triggered this rfc:
> https://patchwork.ozlabs.org/patch/684208/
> 
> At the time Linus strongly rejected the idea of calling  irq_create_mapping (or
> any sleeping functions) in gpio_to_irq: please see the reply from Oct 26, 2016
> (sorry for quoting such an old discussion but this is really the starting point)
> 
> * Me: There is really a *lot* of gpio drivers which use irq_create_mapping in
> the to_irq callback, are these all wrong ?
> * Linus: Yes they are all wrong. They should all be using irq_find_mapping().
> 
> * Me: If this should not be used, what should we all do instead ?
> * Linus: Call irq_create_mapping() in some other place.
> 
> gpio_prepare_irq is a proposition for this 'other place'.

And my opinion is still the same here - It should be perfectly valid to create
mappings from gpio_to_irq() to handle properly orthogonality of gpiochip and
gpio-irqchip functionality and satisfy SPARSE_IRQ goal (allocate Linux virq and
irq descriptors on demand).

The valid use case for gpio_to_irq() is below (and only one valid use-case):
- consumer driver probe() (or other init function):
 gpio = [devm_]gpiod_get()
 irq =  gpio_to_irq(gpio)
	|- in general case -> irq_create_mapping()
 request_irq(irq,...)

>From another side, driver may request GPIO IRQ without even knowing about gpiochip
- consumer driver probe() (or other init function):
 irq = platform_get_irq()/of_irq_get()
 	|- in general case -> irq_create_mapping()
 request_irq(irq,...)

In both cases, IRQ mapping is expected to be created by corresponding IRQ domain.
More over, drivers can share GPIO IRQ and in this case first call to
irq_create_mapping() will actually create mapping while second will be transformed to
irq_find_mapping().

> 
>>
>> 3) As per problem description irq_dispose_mapping() equivalent for GPIO IRQs
>> might be required,
>> but what is the real use-case? Modules reloading or unloading one module and
>> loading another instead?
>> Usually GPIO and IRQ configuration is static and defined in DT, so IRQ mapping
>> created by first call to
>> gpio_to_irq()/platform_get_irq()/of_irq_get() and any subsequent calls just
>> re-use already created mapping.
> 
> Providing that you always create mapping for the same pins, yes
> What happens when gpio irq (the pin) is different each time ? and you exhaust
> the ressource ?

So, do you expect use-case like:
- request GPIO IRQs for pins A1..A8
- free GPIO IRQs A5..A8
- request GPIO IRQs for pins B1..A4
...
(you have 8 gpio irqs as per your description)

This is not smth really common and that why I'm asking ;) and that's why nobody really care
about GPIO IRQs un-mapping. It's kinda expected to plan GPIO IRQ usage very carefully hence
you have only 8 of them.

> 
> It is a corner case, but possible isn't it ? With the gpio char driver
> interface  maybe. You would have to reboot to flush the old mappings and
> continue, right ?

true, but no complains till now ;) And, as I said, It might be reasonable to
have gpio_irq_unprepare() - if you have real use-case description ;)
But note, it's unclear how to integrate it? Will it be required to modify all drivers
which are using  gpio_to_irq() (not good) or it can be integrated in gpiolib and triggered automatically (good)?

> 
> You could also fail to set the trigger type (which you won't be able to set in
> gpio_to_irq). If so, shouldn't you release the mapping ? (that's real use-case
> we are having by the way).

Sry, I didn't get this. IRQ type is set in request_irq() or by irqd_set_trigger_type().
Its actually expected to be reset (configured to safe state) in gpio-irqchip->irq_shutdown()
callback.

Also, It might be helpful to take a look on drivers/irqchip/irq-crossbar.c.



-- 
regards,
-grygorii

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Linus Walleij]

On Tue, Jun 27, 2017 at 8:25 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:

> At the time Linus strongly rejected the idea of calling  irq_create_mapping (or
> any sleeping functions) in gpio_to_irq: please see the reply from Oct 26, 2016
> (sorry for quoting such an old discussion but this is really the starting point)
>
> * Me: There is really a *lot* of gpio drivers which use irq_create_mapping in
> the to_irq callback, are these all wrong ?
> * Linus: Yes they are all wrong. They should all be using irq_find_mapping().
>
> * Me: If this should not be used, what should we all do instead ?
> * Linus: Call irq_create_mapping() in some other place.
>
> gpio_prepare_irq is a proposition for this 'other place'.

There is a misunderstanding here.

I wrote (at the time):

> Yes, but you want to call irq_create_mapping() in slowpath (irq setup)
> and irq_find_mapping() in fastpath (irq handler). Else the first IRQ
> may result in unwelcomed surprises.

This does not mean that irq_create_mapping() cannot be called
in irq context because I think it can.

What it means is that I think that is suboptimal from a performance
point of view if called from gpio_to_irq(), because nominally, at this
point, the mapping should already exist, since the GPIO and IRQ
frameworks are orthogonal.

But that may not apply to the case with many-to-few interrupt
mappings... so I think I was in some 1-to-1-mapping context
when I wrote this. Sorry :(

So I changed my mind, it is fine for this type of driver to call
irq_create_mapping() in gpio_to_irq(). Preferably with some comment
around the call.

It remains to see what happens if gpio_to_irq() would fail, as can
happen in this case. Like if gpio_to_irq() would return 0 (NO_IRQ)
or something negative. I think many drivers are not equipped for
handling this.

So I guess if you could change the semantics of all drivers
calling gpio_to_irq() to also handle say 0 as an error and bail
out, we can call irq_create_mapping() in gpio_to_irq().

Sorry if I'm confused... or confusing.

Yours,
Linus Walleij

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Linus Walleij]

On Tue, Jun 27, 2017 at 10:43 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> And my opinion is still the same here - It should be perfectly valid to create
> mappings from gpio_to_irq() to handle properly orthogonality of gpiochip and
> gpio-irqchip functionality and satisfy SPARSE_IRQ goal (allocate Linux virq and
> irq descriptors on demand).

You are right.

I would rather say: GPIO drivers that have a 1-to-1 mapping between GPIO
lines and IRQs should not do it, they should map up them all at probe().

Drivers that actually have fewer IRQs than GPIO lines should be able to
create the mappings in gpio_to_irq().

Yours,
Linus Walleij

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Jerome Brunet]

On Thu, 2017-06-29 at 16:14 +0200, Linus Walleij wrote:
> On Tue, Jun 27, 2017 at 8:25 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> 
> > At the time Linus strongly rejected the idea of calling  irq_create_mapping
> > (or
> > any sleeping functions) in gpio_to_irq: please see the reply from Oct 26,
> > 2016
> > (sorry for quoting such an old discussion but this is really the starting
> > point)
> > 
> > * Me: There is really a *lot* of gpio drivers which use irq_create_mapping
> > in
> > the to_irq callback, are these all wrong ?
> > * Linus: Yes they are all wrong. They should all be using
> > irq_find_mapping().
> > 
> > * Me: If this should not be used, what should we all do instead ?
> > * Linus: Call irq_create_mapping() in some other place.
> > 
> > gpio_prepare_irq is a proposition for this 'other place'.
> 
> There is a misunderstanding here.
> 
> I wrote (at the time):
> 
> > Yes, but you want to call irq_create_mapping() in slowpath (irq setup)
> > and irq_find_mapping() in fastpath (irq handler). Else the first IRQ
> > may result in unwelcomed surprises.
> 
> This does not mean that irq_create_mapping() cannot be called
> in irq context because I think it can.
> 
> What it means is that I think that is suboptimal from a performance
> point of view if called from gpio_to_irq(), because nominally, at this
> point, the mapping should already exist, since the GPIO and IRQ
> frameworks are orthogonal.

Agreed. It should. In such case, irq_create_mapping is just a call to
irq_find_mapping which is fine. If, for whatever reason this is not the case,
this is going to call sleeping function in irq context. It should not happen but
it is not entirely impossible ...

> 
> But that may not apply to the case with many-to-few interrupt
> mappings... so I think I was in some 1-to-1-mapping context
> when I wrote this. Sorry :(
> 
> So I changed my mind, it is fine for this type of driver to call
> irq_create_mapping() in gpio_to_irq(). Preferably with some comment
> around the call.

What about disposing of the mapping ? there still is no counter part function to
gpio_to_irq. It seems weird to leave them lying around, don't you think ?

Here is a real use we will be having a meson:
* MMC driver load and call gpio_to_irq on its cd pin
  This is going to create a mapping 
* MMC driver request an irq with IRQ_EDGE_BOTH trigger (which we don't/can't
  support at the moment). request_irq fails.
* MMC defaults to polling the GPIO

Remember that we have only 8 possibles mappings. Now there is one (useless)
mapping lying around which can't get rid of.

If there is more drivers doing this sort of tricks, we are going to exhaust the
ressource pretty quickly.

That's just one exemple. The fact is, as soon as a driver calls gpio_to_irq, the
mapping is going to stay forever. Don't you think we should try address this ?

I will happily (re)submit a patch with irq_create_mapping in gpio_to_irq to
bring the 'initial' gpio irq support in meson, until something better comes
up...

> 
> It remains to see what happens if gpio_to_irq() would fail, as can
> happen in this case. Like if gpio_to_irq() would return 0 (NO_IRQ)
> or something negative. I think many drivers are not equipped for
> handling this.

I think this is already fine, gpio_to_irq is supposed to return 0 if there is no
mapping, isn't it ? 

> 
> So I guess if you could change the semantics of all drivers
> calling gpio_to_irq() to also handle say 0 as an error and bail
> out, we can call irq_create_mapping() in gpio_to_irq().
> 
> Sorry if I'm confused... or confusing.

Aren't we all ... we'll get there, someday :)

> 
> Yours,
> Linus Walleij

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Marc Zyngier]

On 29/06/17 15:57, Jerome Brunet wrote:
> On Thu, 2017-06-29 at 16:14 +0200, Linus Walleij wrote:
>> On Tue, Jun 27, 2017 at 8:25 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>>
>>> At the time Linus strongly rejected the idea of calling  irq_create_mapping
>>> (or
>>> any sleeping functions) in gpio_to_irq: please see the reply from Oct 26,
>>> 2016
>>> (sorry for quoting such an old discussion but this is really the starting
>>> point)
>>>
>>> * Me: There is really a *lot* of gpio drivers which use irq_create_mapping
>>> in
>>> the to_irq callback, are these all wrong ?
>>> * Linus: Yes they are all wrong. They should all be using
>>> irq_find_mapping().
>>>
>>> * Me: If this should not be used, what should we all do instead ?
>>> * Linus: Call irq_create_mapping() in some other place.
>>>
>>> gpio_prepare_irq is a proposition for this 'other place'.
>>
>> There is a misunderstanding here.
>>
>> I wrote (at the time):
>>
>>> Yes, but you want to call irq_create_mapping() in slowpath (irq setup)
>>> and irq_find_mapping() in fastpath (irq handler). Else the first IRQ
>>> may result in unwelcomed surprises.
>>
>> This does not mean that irq_create_mapping() cannot be called
>> in irq context because I think it can.
>>
>> What it means is that I think that is suboptimal from a performance
>> point of view if called from gpio_to_irq(), because nominally, at this
>> point, the mapping should already exist, since the GPIO and IRQ
>> frameworks are orthogonal.
> 
> Agreed. It should. In such case, irq_create_mapping is just a call to
> irq_find_mapping which is fine. If, for whatever reason this is not the case,
> this is going to call sleeping function in irq context. It should not happen but
> it is not entirely impossible ...
> 
>>
>> But that may not apply to the case with many-to-few interrupt
>> mappings... so I think I was in some 1-to-1-mapping context
>> when I wrote this. Sorry :(
>>
>> So I changed my mind, it is fine for this type of driver to call
>> irq_create_mapping() in gpio_to_irq(). Preferably with some comment
>> around the call.
> 
> What about disposing of the mapping ? there still is no counter part function to
> gpio_to_irq. It seems weird to leave them lying around, don't you think ?
> 
> Here is a real use we will be having a meson:
> * MMC driver load and call gpio_to_irq on its cd pin
>   This is going to create a mapping 
> * MMC driver request an irq with IRQ_EDGE_BOTH trigger (which we don't/can't
>   support at the moment). request_irq fails.
> * MMC defaults to polling the GPIO
> 
> Remember that we have only 8 possibles mappings. Now there is one (useless)
> mapping lying around which can't get rid of.

Which should be dropped by a dispose_mapping() call in the MMC driver
(or ideally some gpio-specific wrapper around this function).

> If there is more drivers doing this sort of tricks, we are going to exhaust the
> ressource pretty quickly.

We can fix those drivers as we go. It's not like there's a huge variety
of potential HW on this particular platform of yours.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Jerome Brunet]

On Thu, 2017-06-29 at 17:53 +0100, Marc Zyngier wrote:
> On 29/06/17 15:57, Jerome Brunet wrote:
> > On Thu, 2017-06-29 at 16:14 +0200, Linus Walleij wrote:
> > > On Tue, Jun 27, 2017 at 8:25 PM, Jerome Brunet <jbrunet@baylibre.com>
> > > wrote:
> > > 
> > > > At the time Linus strongly rejected the idea of
> > > > calling  irq_create_mapping
> > > > (or
> > > > any sleeping functions) in gpio_to_irq: please see the reply from Oct
> > > > 26,
> > > > 2016
> > > > (sorry for quoting such an old discussion but this is really the
> > > > starting
> > > > point)
> > > > 
> > > > * Me: There is really a *lot* of gpio drivers which use
> > > > irq_create_mapping
> > > > in
> > > > the to_irq callback, are these all wrong ?
> > > > * Linus: Yes they are all wrong. They should all be using
> > > > irq_find_mapping().
> > > > 
> > > > * Me: If this should not be used, what should we all do instead ?
> > > > * Linus: Call irq_create_mapping() in some other place.
> > > > 
> > > > gpio_prepare_irq is a proposition for this 'other place'.
> > > 
> > > There is a misunderstanding here.
> > > 
> > > I wrote (at the time):
> > > 
> > > > Yes, but you want to call irq_create_mapping() in slowpath (irq setup)
> > > > and irq_find_mapping() in fastpath (irq handler). Else the first IRQ
> > > > may result in unwelcomed surprises.
> > > 
> > > This does not mean that irq_create_mapping() cannot be called
> > > in irq context because I think it can.
> > > 
> > > What it means is that I think that is suboptimal from a performance
> > > point of view if called from gpio_to_irq(), because nominally, at this
> > > point, the mapping should already exist, since the GPIO and IRQ
> > > frameworks are orthogonal.
> > 
> > Agreed. It should. In such case, irq_create_mapping is just a call to
> > irq_find_mapping which is fine. If, for whatever reason this is not the
> > case,
> > this is going to call sleeping function in irq context. It should not happen
> > but
> > it is not entirely impossible ...
> > 
> > > 
> > > But that may not apply to the case with many-to-few interrupt
> > > mappings... so I think I was in some 1-to-1-mapping context
> > > when I wrote this. Sorry :(
> > > 
> > > So I changed my mind, it is fine for this type of driver to call
> > > irq_create_mapping() in gpio_to_irq(). Preferably with some comment
> > > around the call.
> > 
> > What about disposing of the mapping ? there still is no counter part
> > function to
> > gpio_to_irq. It seems weird to leave them lying around, don't you think ?
> > 
> > Here is a real use we will be having a meson:
> > * MMC driver load and call gpio_to_irq on its cd pin
> >   This is going to create a mapping 
> > * MMC driver request an irq with IRQ_EDGE_BOTH trigger (which we don't/can't
> >   support at the moment). request_irq fails.
> > * MMC defaults to polling the GPIO
> > 
> > Remember that we have only 8 possibles mappings. Now there is one (useless)
> > mapping lying around which can't get rid of.
> 
> Which should be dropped by a dispose_mapping() call in the MMC driver
> (or ideally some gpio-specific wrapper around this function).

Actually you must go through a gpio-specific function.

Not all gpio drivers use irq domains and mapping unfortunately (legacy...)  
Only the gpio driver knows how this "mapping" (between the gpio number and the
virq) was done.

The proposition at the start of this is my idea for this gpio-specific function.
I'm open to other ideas, of course :)

> 
> > If there is more drivers doing this sort of tricks, we are going to exhaust
> > the
> > ressource pretty quickly.
> 
> We can fix those drivers as we go. It's not like there's a huge variety
> of potential HW on this particular platform of yours.
> 

Those drivers are consumer of gpios. They should not have platform specific
fixes/quirks , right? Like the mmc framework, or the gpio-button driver...

In addition, this problem of un-disposed mappings may also affect any of the 26
driver which create mapping in gpio_to_irq, so it is not specific to a single
platform.

> Thanks,
> 
> 	M.

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Linus Walleij]

On Thu, Jun 29, 2017 at 4:57 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Thu, 2017-06-29 at 16:14 +0200, Linus Walleij wrote:

>> So I changed my mind, it is fine for this type of driver to call
>> irq_create_mapping() in gpio_to_irq(). Preferably with some comment
>> around the call.
>
> What about disposing of the mapping ? there still is no counter part function to
> gpio_to_irq. It seems weird to leave them lying around, don't you think ?

Yeah that sucks. We need a way to fix that.

Is gpio_irq_prepare/unprepare the right approach?

Yours,
Linus Walleij

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Grygorii Strashko]

On 06/29/2017 09:16 AM, Linus Walleij wrote:
> On Tue, Jun 27, 2017 at 10:43 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
> 
>> And my opinion is still the same here - It should be perfectly valid to create
>> mappings from gpio_to_irq() to handle properly orthogonality of gpiochip and
>> gpio-irqchip functionality and satisfy SPARSE_IRQ goal (allocate Linux virq and
>> irq descriptors on demand).
> 
> You are right.
> 
> I would rather say: GPIO drivers that have a 1-to-1 mapping between GPIO
> lines and IRQs should not do it, they should map up them all at probe().
> 

Sry, can't completely agree here :( There could be 300 (or even thousands)
of gpios and only dozen of them will be used as GPIO IRQ, so statical mapping will
just waste system resources. So, better not define such kind of restrictions -
it seems platform/system specific.

> Drivers that actually have fewer IRQs than GPIO lines should be able to
> create the mappings in gpio_to_irq().

-- 
regards,
-grygorii

Re: [RFC] gpio: about the need to manage irq mapping dynamically.

[Linus Walleij]

On Fri, Jun 30, 2017 at 9:54 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 06/29/2017 09:16 AM, Linus Walleij wrote:
>> On Tue, Jun 27, 2017 at 10:43 PM, Grygorii Strashko
>> <grygorii.strashko@ti.com> wrote:
>>
>>> And my opinion is still the same here - It should be perfectly valid to create
>>> mappings from gpio_to_irq() to handle properly orthogonality of gpiochip and
>>> gpio-irqchip functionality and satisfy SPARSE_IRQ goal (allocate Linux virq and
>>> irq descriptors on demand).
>>
>> You are right.
>>
>> I would rather say: GPIO drivers that have a 1-to-1 mapping between GPIO
>> lines and IRQs should not do it, they should map up them all at probe().
>>
>
> Sry, can't completely agree here :( There could be 300 (or even thousands)
> of gpios and only dozen of them will be used as GPIO IRQ, so statical mapping will
> just waste system resources. So, better not define such kind of restrictions -
> it seems platform/system specific.

If there could, yeah.

But the majority of the worlds systems use a hardcoded value of 512 GPIOs.

See include/asm-generic/gpio.h:

#ifndef ARCH_NR_GPIOS
#if defined(CONFIG_ARCH_NR_GPIO) && CONFIG_ARCH_NR_GPIO > 0
#define ARCH_NR_GPIOS CONFIG_ARCH_NR_GPIO
#else
#define ARCH_NR_GPIOS           512
#endif
#endif

The only arch that overrides this is ARM, which has CONFIG_ARCH_NR_GPIO:

# The GPIO number here must be sorted by descending number. In case of
# a multiplatform kernel, we just want the highest value required by the
# selected platforms.
config ARCH_NR_GPIO
        int
        default 1024 if ARCH_BRCMSTB || ARCH_SHMOBILE || ARCH_TEGRA || \
                ARCH_ZYNQ
        default 512 if ARCH_EXYNOS || ARCH_KEYSTONE || SOC_OMAP5 || \
                SOC_DRA7XX || ARCH_S3C24XX || ARCH_S3C64XX || ARCH_S5PV210
        default 416 if ARCH_SUNXI
        default 392 if ARCH_U8500
        default 352 if ARCH_VT8500
        default 288 if ARCH_ROCKCHIP
        default 264 if MACH_H4700
        default 0
        help
          Maximum number of GPIOs in the system.

          If unsure, leave the default value.

So actually, noone use more than 1024 GPIOs.

For each GPIO a descriptor of 16 bytes is allocated. So in worst case 16KiB.

Also this is an upper cap: it just means the nax we have on any platform is 1024
statically allocated GPIO descriptors, not that they all get mapped to IRQs.
That happens on a per-gpiochip basis.

But every irq descriptor is pretty big (somewhere around 64+ bytes), so you have
a point.

If every GPIO allocates an IRQ descriptor, it may add up to as much as 64KiB
on a machine with 1024 GPIOs.

Yours,
Linus Walleij