[PATCH v3 0/2] Implement wake event support on Tegra186 and later

[PATCH v3 0/2] Implement wake event support on Tegra186 and later

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi,

The following is a set of patches that allow certain interrupts to be
used as wakeup sources on Tegra186 and later. To implement this, each
of the GPIO controllers' IRQ domain needs to become hierarchical, and
parented to the PMC domain. The PMC domain in turn implements a new
IRQ domain that is a child to the GIC IRQ domain.

The above ensures that the interrupt chip implementation of the PMC is
called at the correct time. The ->irq_set_type() and ->irq_set_wake()
implementations program the PMC wake registers in a way to enable the
given interrupts as wakeup sources.

This is based on a suggestion from Thomas Gleixner that resulted from
the following thread:

        https://lkml.org/lkml/2018/9/13/1042

Changes in v3:
- use irq_create_fwspec_mapping() instead of irq_domain_alloc_irqs()
- drop preparatory patch exporting irq_domain_alloc_irqs()
- properly set GPIO instance on Tegra186

Changes in v2:
- dropped the Tegra PMC specific patches to simplify the series
- drop wakeup-parent usage, lookup up PMC by compatible
- convert Tegra186 GPIO driver to use valid mask
- move hierarchy support code into gpiolib core

Linus, this is a new revision based on our previous discussion. Sorry it
took so long to get back to this. I also verified that with this series
I can make things work with gpio-keys whether I use the "gpios" property
or the "interrupts" property, which was your primary concern.

I'm also adding Lina to the thread since she's been basing her QCOM
series on top of this patch. Lina, it'd be great if you could confirm
that the changes I made in this version continue to work for you.

Thierry

Thierry Reding (2):
  gpio: Add support for hierarchical IRQ domains
  gpio: tegra186: Implement wake event support

 drivers/gpio/Kconfig         |   1 +
 drivers/gpio/gpio-tegra186.c | 120 +++++++++++++++++++++++++++++++----
 drivers/gpio/gpiolib.c       |  33 ++++++++--
 include/linux/gpio/driver.h  |   8 +++
 4 files changed, 144 insertions(+), 18 deletions(-)

-- 
2.21.0

[PATCH v3 1/2] gpio: Add support for hierarchical IRQ domains

[Thierry Reding]

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

 drivers/gpio/Kconfig        |  1 +
 drivers/gpio/gpiolib.c      | 33 +++++++++++++++++++++++++++++----
 include/linux/gpio/driver.h |  8 ++++++++
 3 files changed, 38 insertions(+), 4 deletions(-)

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:
 	 *
-- 
2.21.0

[PATCH v3 2/2] gpio: tegra186: Implement wake event support

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The GPIO controller doesn't have any controls to enable the system to
wake up from low power states based on activity on GPIO pins. An extra
hardware block that is part of the power management controller (PMC)
contains these controls. In order for the GPIO controller to be able
to cooperate with the PMC, obtain a reference to the PMC's IRQ domain
and make it a parent to the GPIO controller's IRQ domain. This way the
PMC gets an opportunity to program the additional registers required
to enable wakeup sources on suspend.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- use irq_create_fwspec_mapping() instead of irq_domain_alloc_irqs()
- initialize tegra_gpio_soc.instance field on Tegra186

Changes in v2:
- don't use wakeup-parent property but look up PMC by compatible

 drivers/gpio/gpio-tegra186.c | 120 +++++++++++++++++++++++++++++++----
 1 file changed, 106 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index 7d42e3d7572c..59fc84be9c13 100644
--- a/drivers/gpio/gpio-tegra186.c
+++ b/drivers/gpio/gpio-tegra186.c
@@ -56,6 +56,7 @@ struct tegra_gpio_soc {
 	const struct tegra_gpio_port *ports;
 	unsigned int num_ports;
 	const char *name;
+	unsigned int instance;
 };
 
 struct tegra_gpio {
@@ -237,6 +238,38 @@ static int tegra186_gpio_of_xlate(struct gpio_chip *chip,
 	return offset + pin;
 }
 
+static int tegra186_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct tegra_gpio *gpio = gpiochip_get_data(chip);
+	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;
+		unsigned int i;
+
+		for (i = 0; i < gpio->soc->num_ports; i++) {
+			if (offset < gpio->soc->ports[i].pins)
+				break;
+
+			offset -= gpio->soc->ports[i].pins;
+		}
+
+		offset += i * 8;
+
+		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(domain, offset);
+}
+
 static void tegra186_irq_ack(struct irq_data *data)
 {
 	struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
@@ -330,7 +363,7 @@ static int tegra186_irq_set_type(struct irq_data *data, unsigned int type)
 	else
 		irq_set_handler_locked(data, handle_edge_irq);
 
-	return 0;
+	return irq_chip_set_type_parent(data, type);
 }
 
 static void tegra186_gpio_irq(struct irq_desc *desc)
@@ -370,39 +403,82 @@ static void tegra186_gpio_irq(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-static int tegra186_gpio_irq_domain_xlate(struct irq_domain *domain,
-					  struct device_node *np,
-					  const u32 *spec, unsigned int size,
-					  unsigned long *hwirq,
-					  unsigned int *type)
+static int tegra186_gpio_irq_domain_translate(struct irq_domain *domain,
+					      struct irq_fwspec *fwspec,
+					      unsigned long *hwirq,
+					      unsigned int *type)
 {
 	struct tegra_gpio *gpio = gpiochip_get_data(domain->host_data);
 	unsigned int port, pin, i, offset = 0;
 
-	if (size < 2)
+	if (WARN_ON(gpio->gpio.of_gpio_n_cells < 2))
+		return -EINVAL;
+
+	if (WARN_ON(fwspec->param_count < gpio->gpio.of_gpio_n_cells))
 		return -EINVAL;
 
-	port = spec[0] / 8;
-	pin = spec[0] % 8;
+	port = fwspec->param[0] / 8;
+	pin = fwspec->param[0] % 8;
 
-	if (port >= gpio->soc->num_ports) {
-		dev_err(gpio->gpio.parent, "invalid port number: %u\n", port);
+	if (port >= gpio->soc->num_ports)
 		return -EINVAL;
-	}
 
 	for (i = 0; i < port; i++)
 		offset += gpio->soc->ports[i].pins;
 
-	*type = spec[1] & IRQ_TYPE_SENSE_MASK;
+	*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
 	*hwirq = offset + pin;
 
 	return 0;
 }
 
+static int tegra186_gpio_irq_domain_alloc(struct irq_domain *domain,
+					  unsigned int virq,
+					  unsigned int num_irqs,
+					  void *data)
+{
+	struct tegra_gpio *gpio = gpiochip_get_data(domain->host_data);
+	struct irq_fwspec *fwspec = data;
+	struct irq_fwspec spec;
+	unsigned long hwirq;
+	unsigned int type;
+	int err;
+
+	if (WARN_ON(num_irqs > 1))
+		return -EINVAL;
+
+	if (WARN_ON(fwspec->param_count < 2))
+		return -EINVAL;
+
+	err = tegra186_gpio_irq_domain_translate(domain, fwspec, &hwirq, &type);
+	if (err < 0)
+		return err;
+
+	err = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &gpio->intc,
+					    gpio);
+	if (err < 0)
+		return err;
+
+	spec.fwnode = domain->parent->fwnode;
+	spec.param_count = 3;
+	spec.param[0] = gpio->soc->instance;
+	spec.param[1] = fwspec->param[0];
+	spec.param[2] = fwspec->param[1];
+
+	return irq_domain_alloc_irqs_parent(domain, virq, num_irqs, &spec);
+}
+
 static const struct irq_domain_ops tegra186_gpio_irq_domain_ops = {
+	.translate = tegra186_gpio_irq_domain_translate,
+	.alloc = tegra186_gpio_irq_domain_alloc,
 	.map = gpiochip_irq_map,
 	.unmap = gpiochip_irq_unmap,
-	.xlate = tegra186_gpio_irq_domain_xlate,
+};
+
+static const struct of_device_id tegra186_pmc_of_match[] = {
+	{ .compatible = "nvidia,tegra186-pmc" },
+	{ .compatible = "nvidia,tegra194-pmc" },
+	{ /* sentinel */ }
 };
 
 static int tegra186_gpio_probe(struct platform_device *pdev)
@@ -410,6 +486,7 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
 	unsigned int i, j, offset;
 	struct gpio_irq_chip *irq;
 	struct tegra_gpio *gpio;
+	struct device_node *np;
 	struct resource *res;
 	char **names;
 	int err;
@@ -484,12 +561,14 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
 	gpio->gpio.of_node = pdev->dev.of_node;
 	gpio->gpio.of_gpio_n_cells = 2;
 	gpio->gpio.of_xlate = tegra186_gpio_of_xlate;
+	gpio->gpio.to_irq = tegra186_gpio_to_irq;
 
 	gpio->intc.name = pdev->dev.of_node->name;
 	gpio->intc.irq_ack = tegra186_irq_ack;
 	gpio->intc.irq_mask = tegra186_irq_mask;
 	gpio->intc.irq_unmask = tegra186_irq_unmask;
 	gpio->intc.irq_set_type = tegra186_irq_set_type;
+	gpio->intc.irq_set_wake = irq_chip_set_wake_parent;
 
 	irq = &gpio->gpio.irq;
 	irq->chip = &gpio->intc;
@@ -501,6 +580,15 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
 	irq->num_parents = gpio->num_irq;
 	irq->parents = gpio->irq;
 
+	np = of_find_matching_node(NULL, tegra186_pmc_of_match);
+	if (np) {
+		irq->parent_domain = irq_find_host(np);
+		of_node_put(np);
+
+		if (!irq->parent_domain)
+			return -EPROBE_DEFER;
+	}
+
 	irq->map = devm_kcalloc(&pdev->dev, gpio->gpio.ngpio,
 				sizeof(*irq->map), GFP_KERNEL);
 	if (!irq->map)
@@ -567,6 +655,7 @@ static const struct tegra_gpio_soc tegra186_main_soc = {
 	.num_ports = ARRAY_SIZE(tegra186_main_ports),
 	.ports = tegra186_main_ports,
 	.name = "tegra186-gpio",
+	.instance = 0,
 };
 
 #define TEGRA186_AON_GPIO_PORT(port, base, count, controller)	\
@@ -592,6 +681,7 @@ static const struct tegra_gpio_soc tegra186_aon_soc = {
 	.num_ports = ARRAY_SIZE(tegra186_aon_ports),
 	.ports = tegra186_aon_ports,
 	.name = "tegra186-gpio-aon",
+	.instance = 1,
 };
 
 #define TEGRA194_MAIN_GPIO_PORT(port, base, count, controller)	\
@@ -637,6 +727,7 @@ static const struct tegra_gpio_soc tegra194_main_soc = {
 	.num_ports = ARRAY_SIZE(tegra194_main_ports),
 	.ports = tegra194_main_ports,
 	.name = "tegra194-gpio",
+	.instance = 0,
 };
 
 #define TEGRA194_AON_GPIO_PORT(port, base, count, controller)	\
@@ -659,6 +750,7 @@ static const struct tegra_gpio_soc tegra194_aon_soc = {
 	.num_ports = ARRAY_SIZE(tegra194_aon_ports),
 	.ports = tegra194_aon_ports,
 	.name = "tegra194-gpio-aon",
+	.instance = 1,
 };
 
 static const struct of_device_id tegra186_gpio_of_match[] = {
-- 
2.21.0

Re: [PATCH v3 1/2] gpio: Add support for hierarchical IRQ domains

[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
>
> 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

Re: [PATCH v3 1/2] gpio: Add support for hierarchical IRQ domains

[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

Re: [PATCH v3 1/2] gpio: Add support for hierarchical IRQ domains

[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

Re: [PATCH v3 1/2] gpio: Add support for hierarchical IRQ domains

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3377 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/2] gpio: Add support for hierarchical IRQ domains

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/2] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

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

Re: [PATCH v3 1/2] gpio: Add support for hierarchical IRQ domains

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/2] gpio: Add support for hierarchical IRQ domains

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2167 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/2] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

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