[v2,1/5] gpio: Add support for hierarchical IRQ domains

From: Thierry Reding <treding@nvidia.com>

Hierarchical IRQ domains can be used to stack different IRQ controllers
on top of each other. One specific use-case where this can be useful is
if a power management controller has top-level controls for wakeup
interrupts. In such cases, the power management controller can be a
parent to other interrupt controllers and program additional registers
when an IRQ has its wake capability enabled or disabled.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- select IRQ_DOMAIN_HIERARCHY to avoid build failure
- move more code into the gpiolib core

 drivers/gpio/Kconfig        |  2 +-
 drivers/gpio/gpiolib.c      | 33 +++++++++++++++++++++++++++++----
 include/linux/gpio/driver.h |  6 ++++++
 3 files changed, 36 insertions(+), 5 deletions(-)

Hi Thierry!

thanks a lot for the patch! This is really in the right direction
of how I want things to go with hierarchical IRQs.

Some comments:

On Thu, Nov 29, 2018 at 6:03 PM Thierry Reding <thierry.reding@gmail.com> wrote:

>  config GPIOLIB_IRQCHIP
> -       select IRQ_DOMAIN
> +       select IRQ_DOMAIN_HIERARCHY

I understand that IRQ_DOMAIN_HIERARCHY implies
IRQ_DOMAIN but I kind of like if we anyway select both
(unless Kconfig dislikes it).

Otherwise it looks like we're just using hierarchies.

>  static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
>  {
> +       struct irq_domain *domain = chip->irq.domain;
> +
>         if (!gpiochip_irqchip_irq_valid(chip, offset))
>                 return -ENXIO;
>
> +       if (irq_domain_is_hierarchy(domain)) {
> +               struct irq_fwspec spec;
> +
> +               spec.fwnode = domain->fwnode;
> +               spec.param_count = 2;
> +               spec.param[0] = offset;
> +               spec.param[1] = 0;
> +
> +               return irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &spec);
> +       }
> +
>         return irq_create_mapping(chip->irq.domain, offset);
>  }

This is really nice.

> -       gpiochip->to_irq = gpiochip_to_irq;
> +       /*
> +        * Allow GPIO chips to override the ->to_irq() if they really need to.
> +        * This should only be very rarely needed, the majority should be fine
> +        * with gpiochip_to_irq().
> +        */
> +       if (!gpiochip->to_irq)
> +               gpiochip->to_irq = gpiochip_to_irq;

And I see this is what your driver does, which leaves the default
domain hierarchy callback path unused.

It's better if you indirect the pointer like we do with some other
irqchip stuff (see Hans Verkuils patches), i.e. if ->to_irq
is defined, we save that function pointer and call that at the
end of the gpiochip_to_irq() pointer, then we override it
with our own.

Since the Tegra186 clearly .to_irq() clearly is mostly the
same plus some extra, this should work fine and give
lesser lines of code in that driver.

Apart from that I am a big fan of this patch!

Yours,
Linus Walleij

On Fri, Dec 14, 2018 at 02:41:01PM +0100, Linus Walleij wrote:
> Hi Thierry!
> 
> thanks a lot for the patch! This is really in the right direction
> of how I want things to go with hierarchical IRQs.
> 
> Some comments:
> 
> On Thu, Nov 29, 2018 at 6:03 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> >  config GPIOLIB_IRQCHIP
> > -       select IRQ_DOMAIN
> > +       select IRQ_DOMAIN_HIERARCHY
> 
> I understand that IRQ_DOMAIN_HIERARCHY implies
> IRQ_DOMAIN but I kind of like if we anyway select both
> (unless Kconfig dislikes it).
> 
> Otherwise it looks like we're just using hierarchies.

Okay, I can add that.

> 
> >  static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
> >  {
> > +       struct irq_domain *domain = chip->irq.domain;
> > +
> >         if (!gpiochip_irqchip_irq_valid(chip, offset))
> >                 return -ENXIO;
> >
> > +       if (irq_domain_is_hierarchy(domain)) {
> > +               struct irq_fwspec spec;
> > +
> > +               spec.fwnode = domain->fwnode;
> > +               spec.param_count = 2;
> > +               spec.param[0] = offset;
> > +               spec.param[1] = 0;
> > +
> > +               return irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &spec);
> > +       }
> > +
> >         return irq_create_mapping(chip->irq.domain, offset);
> >  }
> 
> This is really nice.
> 
> > -       gpiochip->to_irq = gpiochip_to_irq;
> > +       /*
> > +        * Allow GPIO chips to override the ->to_irq() if they really need to.
> > +        * This should only be very rarely needed, the majority should be fine
> > +        * with gpiochip_to_irq().
> > +        */
> > +       if (!gpiochip->to_irq)
> > +               gpiochip->to_irq = gpiochip_to_irq;
> 
> And I see this is what your driver does, which leaves the default
> domain hierarchy callback path unused.

Actually this is only temporary until the patch that uses a sparse
number space (with the valid masks). After the last patch in the series
the need to override this goes away, so I could follow-up with a patch
to revert this. Or alternatively we could squash the two Tegra patches.
I think keeping them separate is slightly nicer.

Of course, I would even prefer to not move to the sparse number space,
but you seemed to feel very strongly about it last time we discussed
this.

> It's better if you indirect the pointer like we do with some other
> irqchip stuff (see Hans Verkuils patches), i.e. if ->to_irq
> is defined, we save that function pointer and call that at the
> end of the gpiochip_to_irq() pointer, then we override it
> with our own.
> 
> Since the Tegra186 clearly .to_irq() clearly is mostly the
> same plus some extra, this should work fine and give
> lesser lines of code in that driver.

That sounds an awful lot like a midlayer. I'm not a big fan of that at
all. There are various reasons for this, but others have described it in
much better detail than I could. See here for example:

	https://lwn.net/Articles/336262/

In this particular case one potential problem is that gpiochip_to_irq()
might not always do the right things for everyone, and that it turn may
incentivize people to work around it creatively. For example, one driver
may want to perform some operation before gpiochip_to_irq() is called,
while another driver may have to perform an operation after the call. If
you move towards a midlayer there's no way to satisfy both, unless you
go and add pre- and post-operations of some sort.

I'd like to propose that we instead provide gpiochip_to_irq() as a
helper that drivers can call into. In order to do that, we just need to
make sure that drivers have access to it. Then they can override the
->to_irq() like this:

	static int foo_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
	{
		/* do something before */
		...

		err = gpiochip_to_irq(chip, offset);
		if (err < 0)
			return err;

		/* do something after */
		...
	}

That way we get all the benefits of sharing the code and at the same
time we don't impose any artificial constraints on drivers. Typically in
the library pattern drivers that don't need anything extra would simply
set gpiochip_to_irq() as their implementation, so the default assignment
in the gpiolib core wouldn't be necessary, but that's just a minor
implementation detail.

What do you think?

Thierry

Cc:ing Neil Brown who might hit me on the fingers for
some statements here, let's see :)

On Tue, Dec 18, 2018 at 11:06 PM Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Fri, Dec 14, 2018 at 02:41:01PM +0100, Linus Walleij wrote:

> > > -       gpiochip->to_irq = gpiochip_to_irq;
> > > +       /*
> > > +        * Allow GPIO chips to override the ->to_irq() if they really need to.
> > > +        * This should only be very rarely needed, the majority should be fine
> > > +        * with gpiochip_to_irq().
> > > +        */
> > > +       if (!gpiochip->to_irq)
> > > +               gpiochip->to_irq = gpiochip_to_irq;
> >
> > And I see this is what your driver does, which leaves the default
> > domain hierarchy callback path unused.
>
> Actually this is only temporary until the patch that uses a sparse
> number space (with the valid masks). After the last patch in the series
> the need to override this goes away, so I could follow-up with a patch
> to revert this. Or alternatively we could squash the two Tegra patches.
> I think keeping them separate is slightly nicer.

OK then!

> Of course, I would even prefer to not move to the sparse number space,
> but you seemed to feel very strongly about it last time we discussed
> this.

Admittedly it is one of those things where I am working on intuition
and as such it may be wrong.

The main reason in this case is to keep interfaces simple.
Of course "simple" is a bit subjective. But keeping to a
certain predictable pattern makes things easier to understand
as long as the pattern is somewhat reasonable.

This pattern I think it is related to the irqdomains: in the
irqdomains a linear irqdomain with dynamically allocated
IRQ descriptors from a sparse number space is clearly
preferred.

By similarity and the fact that we are mapping GPIOs to
IRQs here, I think it is more intuitive and less confusing
to use a linear GPIO offset range and dynamically
allocated GPIO descriptors.

Today we do not allocate GPIO descriptors
dynamically, they are allocated as part of the gpiochip
struct. But I do see it as an end goal to allocate them
dynamically or at least not allocate GPIO descriptors for
the "holes" in the .valid_mask.

> > It's better if you indirect the pointer like we do with some other
> > irqchip stuff (see Hans Verkuils patches), i.e. if ->to_irq
> > is defined, we save that function pointer and call that at the
> > end of the gpiochip_to_irq() pointer, then we override it
> > with our own.
> >
> > Since the Tegra186 clearly .to_irq() clearly is mostly the
> > same plus some extra, this should work fine and give
> > lesser lines of code in that driver.
>
> That sounds an awful lot like a midlayer. I'm not a big fan of that at
> all. There are various reasons for this, but others have described it in
> much better detail than I could. See here for example:
>
>         https://lwn.net/Articles/336262/

Reading that article I do not think gpiolib qualifies as midlayer,
since it is after all optional and not every driver uses it, so
it is available on a case-by-case basis. The article even says:

"That common functionality which it is so tempting to put in a
midlayer should instead be provided as library routines which
can used, augmented, or ignored by each bottom level
driver independently."

Contrast that by the SCSI emulation in libata: is it even
possible to bypass? Not really, no. gpiolib is not like that.

> In this particular case one potential problem is that gpiochip_to_irq()
> might not always do the right things for everyone, and that it turn may
> incentivize people to work around it creatively. For example, one driver
> may want to perform some operation before gpiochip_to_irq() is called,
> while another driver may have to perform an operation after the call. If
> you move towards a midlayer there's no way to satisfy both, unless you
> go and add pre- and post-operations of some sort.

I don't really buy into that.

gpiochip_to_irq() is *ONLY* a translation mechanism. The main
abstraction is and should be irqchip.

In usecases like device tree the same device exposes a
gpiochip API and an irqchip API and those two are supposed
to be orthogonal. The Documentation/driver-api/gpio/driver.rst
says:

--------8<----------8<--------8<--------8<--------
It is legal for any IRQ consumer to request an IRQ from any irqchip no matter
if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
irq_chip are orthogonal, and offering their services independent of each
other.

gpiod_to_irq() is just a convenience function to figure out the IRQ for a
certain GPIO line and should not be relied upon to have been called before
the IRQ is used.

So always prepare the hardware and make it ready for action in respective
callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having
been called first.

This orthogonality leads to ambiguities that we need to solve: if there is
competition inside the subsystem which side is using the resource (a certain
GPIO line and register for example) it needs to deny certain operations and
keep track of usage inside of the gpiolib subsystem. This is why the API
below exists.
--------8<----------8<--------8<--------8<--------

So now indeed it seems that your implementation actually
has a bug here. I didn't see it before: if we make use of
the irq off the irqchip without gpiochip_to_irq() having
been called before, will the (hierarchical) IRQ still work?

Much of the idea and reason behind GPIOLIB_IRQCHIP is
to make sure this can hold true.

So this hunk of your original patch:

+       if (irq_domain_is_hierarchy(domain)) {
+               struct irq_fwspec spec;
+
+               spec.fwnode = domain->fwnode;
+               spec.param_count = 2;
+               spec.param[0] = offset;
+               spec.param[1] = 0;
+
+               return irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &spec);
+       }

Is probably misplaced: gpiochip_to_irq() should really
only contain irq_create_mapping() which is what will
also be done by the device tree (or ACPI, I hope) core
when providing an irq as a resource.

The snippet above needs to go somewhere else, if it
needs to happen before any of the irqchip callbacks
then just loop over all the IRQs in
gpiochip_add_irqchip() and allocate IRQs for all valid
IRQs from the start as a last resort I suppose.

A last alternative would be to assume that all other ways
an IRQ is mapped (besides using gpiochip_to_irq()) will
also do the same thing. E.g, you would have to go into
drivers/of/irq.c and patch irq_of_parse_and_map()
to do the same thing. This is the site that creates the
mapping when .to_irq() is not called in the DT case.

Maybe the DT or irqchip maintainers can give a better
answer to how this should happen. Relying on .to_irq()
to always be called is certainly not the answer. We already
did that mistake in the past :(

> I'd like to propose that we instead provide gpiochip_to_irq() as a
> helper that drivers can call into.

>        static int foo_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
>        {
>                /* do something before */
>                ...
>
>               err = gpiochip_to_irq(chip, offset);
>               if (err < 0)
>                       return err;
>
>               /* do something after */
>               ...
>       }

Under your assumptions that is fine, and the same reason as to
why we control pins and regulators and clock in callbacks from
say .suspend() instead of enforcing a certain order. It could be
necessary if the translation gets complicated enough.

But right now I am mostly worried that this implementation does
not cover for the fact that we have to make sure that hierarchical
irqchips in gpiolib work even if .to_irq() was not called first.

Yours,
Linus Walleij