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@xxxxxxxxx> wrote: > > > From: Thierry Reding <treding@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > 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
Attachment:
signature.asc
Description: PGP signature
