| Message ID | 20190529145322.20630-2-thierry.reding@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | Implement wake event support on Tegra186 and later | expand |
On Wed, May 29, 2019 at 4:53 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> > --- > Changes in v3: > - use irq_create_fwspec_mapping() instead of irq_domain_alloc_irqs() > - add missing kerneldoc for new parent_domain field > - keep IRQ_DOMAIN dependency for clarity > > Changes in v2: > - select IRQ_DOMAIN_HIERARCHY to avoid build failure > - move more code into the gpiolib core This is looking really good! > config GPIOLIB_IRQCHIP > select IRQ_DOMAIN > + select IRQ_DOMAIN_HIERARCHY > bool Hm OK I guess. It would be ugly to ifdef all hierarchy code in gpiolib. > 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] = IRQ_TYPE_NONE; > + > + return irq_create_fwspec_mapping(&spec); > + } > + > return irq_create_mapping(chip->irq.domain, offset); This is looking really good! > + /* > + * 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; Please drop this. The default .to_irq() should be good for everyone. Also patch 2/2 now contains a identical copy of the gpiolib .to_irq() which I suspect you indended to drop, actually. Other than that I'm ready to merge the v3 of this! Yours, Linus Walleij
On Wed, May 29, 2019 at 4:53 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> > --- > Changes in v3: > - use irq_create_fwspec_mapping() instead of irq_domain_alloc_irqs() > - add missing kerneldoc for new parent_domain field > - keep IRQ_DOMAIN dependency for clarity Actually I applied the patch, and dropped the two lines making it possible to override .to_irq() for now, so I can illustrate my idea on top. If I manage. Yours, Linus Walleij
On Sun, Jun 2, 2019 at 3:51 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, May 29, 2019 at 4:53 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> > > --- > > Changes in v3: > > - use irq_create_fwspec_mapping() instead of irq_domain_alloc_irqs() > > - add missing kerneldoc for new parent_domain field > > - keep IRQ_DOMAIN dependency for clarity > > Actually I applied the patch, and dropped the two lines making > it possible to override .to_irq() for now, so I can illustrate my > idea on top. If I manage. Bah I rewrote the whole think as I want it, maybe my ideas are stupid but take a look at it, also very interested in input from the irqchip maintainers. Sending it out as RFC in a moment. Yours, Linus Walleij
On Sun, Jun 02, 2019 at 03:46:00PM +0200, Linus Walleij wrote: > On Wed, May 29, 2019 at 4:53 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> > > --- > > Changes in v3: > > - use irq_create_fwspec_mapping() instead of irq_domain_alloc_irqs() > > - add missing kerneldoc for new parent_domain field > > - keep IRQ_DOMAIN dependency for clarity > > > > Changes in v2: > > - select IRQ_DOMAIN_HIERARCHY to avoid build failure > > - move more code into the gpiolib core > > This is looking really good! > > > config GPIOLIB_IRQCHIP > > select IRQ_DOMAIN > > + select IRQ_DOMAIN_HIERARCHY > > bool > > Hm OK I guess. It would be ugly to ifdef all hierarchy > code in gpiolib. > > > 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] = IRQ_TYPE_NONE; > > + > > + return irq_create_fwspec_mapping(&spec); > > + } > > + > > return irq_create_mapping(chip->irq.domain, offset); > > This is looking really good! > > > + /* > > + * 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; > > Please drop this. The default .to_irq() should be good for everyone. > Also patch 2/2 now contains a identical copy of the gpiolib > .to_irq() which I suspect you indended to drop, actually. It's not actually identical to the gpiolib implementation. There's still the conversion to the non-linear DT representation for GPIO specifiers from the linear GPIO number space, which is not taken care of by the gpiolib variant. That's precisely the point why this patch makes it possible to let the driver override things. More generally, if we drop this we restrict access to the built-in hierarchical support to devices that use 2 cells as the specifier. Most drivers support that, but there are a few exceptions: - gpio-lpc32xx - gpio-mt7621 - gpio-pxa gpio-pxa seems like it's really just two-cell, but the other two are definitely different. As discussed before gpio-tegra186 is also different but could be tweaked into doing two-cell by generating the GPIO/IRQ map upfront. Thierry
On Sun, Jun 02, 2019 at 10:35:22PM +0200, Linus Walleij wrote: > On Sun, Jun 2, 2019 at 3:51 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, May 29, 2019 at 4:53 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> > > > --- > > > Changes in v3: > > > - use irq_create_fwspec_mapping() instead of irq_domain_alloc_irqs() > > > - add missing kerneldoc for new parent_domain field > > > - keep IRQ_DOMAIN dependency for clarity > > > > Actually I applied the patch, and dropped the two lines making > > it possible to override .to_irq() for now, so I can illustrate my > > idea on top. If I manage. > > Bah I rewrote the whole think as I want it, maybe my ideas are stupid > but take a look at it, also very interested in input from the irqchip > maintainers. > > Sending it out as RFC in a moment. Okay, taking a look. Thierry
On Mon, Jun 3, 2019 at 9:53 AM Thierry Reding <thierry.reding@gmail.com> wrote: > Me > > Please drop this. The default .to_irq() should be good for everyone. > > Also patch 2/2 now contains a identical copy of the gpiolib > > .to_irq() which I suspect you indended to drop, actually. > > It's not actually identical to the gpiolib implementation. There's still > the conversion to the non-linear DT representation for GPIO specifiers > from the linear GPIO number space, which is not taken care of by the > gpiolib variant. That's precisely the point why this patch makes it > possible to let the driver override things. OK something is off here, because the purpose of the irqdomain is exactly to translate between different number spaces, so it should not happen in the .to_irq() function at all. Irqdomain uses .map() in the old variant and .translate() in the hierarchical variant to do this, so something is skewed. All .to_irq() should ever do is just call the irqdomain to do the translation, no other logic (unless I am mistaken) so we should be able to keep the simple .to_irq() logic inside gpiolib. Yours, Linus Walleij
On Mon, Jun 03, 2019 at 12:58:02PM +0200, Linus Walleij wrote: > On Mon, Jun 3, 2019 at 9:53 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > Me > > > > Please drop this. The default .to_irq() should be good for everyone. > > > Also patch 2/2 now contains a identical copy of the gpiolib > > > .to_irq() which I suspect you indended to drop, actually. > > > > It's not actually identical to the gpiolib implementation. There's still > > the conversion to the non-linear DT representation for GPIO specifiers > > from the linear GPIO number space, which is not taken care of by the > > gpiolib variant. That's precisely the point why this patch makes it > > possible to let the driver override things. > > OK something is off here, because the purpose of the irqdomain > is exactly to translate between different number spaces, so it should > not happen in the .to_irq() function at all. > > Irqdomain uses .map() in the old variant and .translate() in the > hierarchical variant to do this, so something is skewed. > > All .to_irq() should ever do is just call the irqdomain to do the > translation, no other logic (unless I am mistaken) so we should > be able to keep the simple .to_irq() logic inside gpiolib. Well, that's exactly the problem that I'm trying to solve. The problem is that .translate() translates from the DT number space to the GPIO or IRQ number space. However, since gpiochip_to_irq() now wants to call the irq_create_fwspec_mapping() interface, it must convert from the offset (in GPIO space) into the DT number space, which is what that function expects. Thierry
On Mon, Jun 03, 2019 at 02:12:27PM +0200, Thierry Reding wrote: > On Mon, Jun 03, 2019 at 12:58:02PM +0200, Linus Walleij wrote: > > On Mon, Jun 3, 2019 at 9:53 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > Me > > > > > > Please drop this. The default .to_irq() should be good for everyone. > > > > Also patch 2/2 now contains a identical copy of the gpiolib > > > > .to_irq() which I suspect you indended to drop, actually. > > > > > > It's not actually identical to the gpiolib implementation. There's still > > > the conversion to the non-linear DT representation for GPIO specifiers > > > from the linear GPIO number space, which is not taken care of by the > > > gpiolib variant. That's precisely the point why this patch makes it > > > possible to let the driver override things. > > > > OK something is off here, because the purpose of the irqdomain > > is exactly to translate between different number spaces, so it should > > not happen in the .to_irq() function at all. > > > > Irqdomain uses .map() in the old variant and .translate() in the > > hierarchical variant to do this, so something is skewed. > > > > All .to_irq() should ever do is just call the irqdomain to do the > > translation, no other logic (unless I am mistaken) so we should > > be able to keep the simple .to_irq() logic inside gpiolib. > > Well, that's exactly the problem that I'm trying to solve. The problem > is that .translate() translates from the DT number space to the GPIO or > IRQ number space. However, since gpiochip_to_irq() now wants to call the > irq_create_fwspec_mapping() interface, it must convert from the offset > (in GPIO space) into the DT number space, which is what that function > expects. Hm... I wonder if we even need this irq_create_fwspec_mapping() there. Couldn't we just do an irq_create_mapping() since we already know which one of the GPIO IRQ controller's interrupts we want to create a mapping for? If we already convert to the GPIO number space in the .translate() then the offset already corresponds to the one that we need to map, no? I'll make a note to try that tomorrow. Thierry
On Mon, Jun 3, 2019 at 2:12 PM Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Jun 03, 2019 at 12:58:02PM +0200, Linus Walleij wrote: > > On Mon, Jun 3, 2019 at 9:53 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > Me > > > > > > Please drop this. The default .to_irq() should be good for everyone. > > > > Also patch 2/2 now contains a identical copy of the gpiolib > > > > .to_irq() which I suspect you indended to drop, actually. > > > > > > It's not actually identical to the gpiolib implementation. There's still > > > the conversion to the non-linear DT representation for GPIO specifiers > > > from the linear GPIO number space, which is not taken care of by the > > > gpiolib variant. That's precisely the point why this patch makes it > > > possible to let the driver override things. > > > > OK something is off here, because the purpose of the irqdomain > > is exactly to translate between different number spaces, so it should > > not happen in the .to_irq() function at all. > > > > Irqdomain uses .map() in the old variant and .translate() in the > > hierarchical variant to do this, so something is skewed. > > > > All .to_irq() should ever do is just call the irqdomain to do the > > translation, no other logic (unless I am mistaken) so we should > > be able to keep the simple .to_irq() logic inside gpiolib. > > Well, that's exactly the problem that I'm trying to solve. The problem > is that .translate() translates from the DT number space to the GPIO or > IRQ number space. However, since gpiochip_to_irq() now wants to call the > irq_create_fwspec_mapping() interface, it must convert from the offset > (in GPIO space) into the DT number space, which is what that function > expects. It sounds a lot like you want to use the irqdomain APIs for something they were not made for. The irqdomain in general remaps from hwirqs to Linux irqs. The hierarchical irqdomain 1-to-1 remaps hwirqs on interrupt controller A to hwirqs on interrupt controller B, then to Linux irqs. The DT number space is not normally any concern of the irqdomain, and is not normally used to shape up that. But how to do it is another question. Maybe making it possible to override .translate() is what we need to do, as you say. That way .translate() can account for weirdness in the DT, get the real hwirq out and proceed to translate as usual. And then in the end the irqdomain .translate() is indeed used to fix up weird DTs.... well. Hm this one is strange. :) Yours, Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 62f3fe06cd2f..b87cc36005d8 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -45,6 +45,7 @@ config GPIO_ACPI config GPIOLIB_IRQCHIP select IRQ_DOMAIN + select IRQ_DOMAIN_HIERARCHY bool config DEBUG_GPIO diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index e013d417a936..6b64bfa90089 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1825,9 +1825,22 @@ EXPORT_SYMBOL_GPL(gpiochip_irq_domain_deactivate); 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] = IRQ_TYPE_NONE; + + return irq_create_fwspec_mapping(&spec); + } + return irq_create_mapping(chip->irq.domain, offset); } @@ -1936,7 +1949,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, type = IRQ_TYPE_NONE; } - 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; + gpiochip->irq.default_type = type; gpiochip->irq.lock_key = lock_key; gpiochip->irq.request_key = request_key; @@ -1946,9 +1966,14 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, else ops = &gpiochip_domain_ops; - 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); if (!gpiochip->irq.domain) return -EINVAL; diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index a1d273c96016..472f2c127bf6 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -48,6 +48,14 @@ struct gpio_irq_chip { */ const struct irq_domain_ops *domain_ops; + /** + * @parent_domain: + * + * If non-NULL, will be set as the parent of this GPIO interrupt + * controller's IRQ domain to establish a hierarchy. + */ + struct irq_domain *parent_domain; + /** * @handler: *