On Tue, Sep 25, 2018 at 10:11:06AM +0200, Linus Walleij wrote:
> Hi Thierry!
> 
> Thanks for the patch!
> 
> I am a bit ignorant about irqdomains so I will probably need an ACK
> from some irq maintainer before I can apply this.
> 
> On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
> <thierry.reding@gmail.com> wrote:
> 
> > 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>
> 
> While I think it is really important that we start supporting hierarchical
> irqdomains in the gpiolib core, I want a more complete approach,
> so that drivers that need hierarchical handling of irqdomains
> can get the same support from gpiolib as they get for simple
> domains.
> 
> > @@ -1918,7 +1918,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
> >                 type = IRQ_TYPE_NONE;
> >         }
> >
> > -       gpiochip->to_irq = gpiochip_to_irq;
> > +       if (!gpiochip->to_irq)
> > +               gpiochip->to_irq = gpiochip_to_irq;
> 
> So here you let the drivers override the .to_irq() function and that
> I think gets confusing as we are asking gpiolib to handle our
> irqchip.
> 
> 
> > -       gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> > -                                                    gpiochip->irq.first,
> > -                                                    ops, gpiochip);
> > +       if (gpiochip->irq.parent_domain)
> > +               gpiochip->irq.domain = irq_domain_add_hierarchy(gpiochip->irq.parent_domain,
> > +                                                               0, gpiochip->ngpio,
> > +                                                               np, ops, gpiochip);
> > +       else
> > +               gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> > +                                                            gpiochip->irq.first,
> > +                                                            ops, gpiochip);
> 
> So this part is great: if we pass in a parent domain the core helps us
> create the hierarchy.
> 
> But the stuff in .to_irq() should also be handled in the gpiolib core:
> the irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &spec) for
> example. That way you should not need any external .to_irq() function.
> 
> I can't see if you need to pull more stuff into the core to accomplish
> that, but I think in essence the core gpiolib needs to be more helpful
> with hierarchies.

This is not as trivial as it sounds. I think we could probably provide a
simple helper in the core that may work for the majority of GPIO
controllers, and would be similar to irq_domain_xlate_twocell(). The
problem is that ->gpio_to_irq() effectively needs to convert the offset
of the pin in the GPIO controller to an IRQ specifier. If the IRQ domain
can use irq_domain_xlate_twocell(), that should be easy, but if it does
not, then we likely need a custom implementation as well.

For example, as you may remember, the Tegra186 GPIO controller is
somewhat quirky in that it has a number of banks, each of which can have
any number of pins up to 8. However, in order to prevent users from
attempting to use one of the non-existent GPIOs, we resorted to
compacting the GPIO number space so that the GPIO specifier uses
basically a (bank, pin) pair that is converted into a GPIO offset. The
same is done for interrupt specifiers.

Since there is no 1:1 relationship between the value in the specifier
and the GPIO offset, we can't use irq_domain_xlate_twocell(). Similarly
we won't be able to use the standard gpiochip_to_irq() counterpart for
two cell specifiers to construct the IRQ specifier, but instead we'll
have to convert it based on the offset and the number of pins per bank
(that is, the inverse of what we do in tegra186_gpio_of_xlate()).

I think we can probably just implement the simple two-cell version in
gpiochip_to_irq() directly and leave it up to drivers that require
something more to override ->to_irq().

Another alternative that I had pondered about was to add another level
of indirection and have a generic implementation in gpiochip_to_irq()
that calls back into a new ->to_irq_fwspec() function that drivers can
implement to fill in the struct irq_fwspec as required by the
irq_domain_alloc_irqs() function. We could still provide a default
implementation for the common two-cell case, so most drivers wouldn't
have to know about it.

I don't think any of the above has massive advantages over the other.
The latter will be slightly more compact, I would assume, but the former
gives us more flexibility if we need it.

Thierry