linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] Implement wake event support on Tegra186 and later
@ 2018-11-29 17:03 Thierry Reding
  2018-11-29 17:03 ` [PATCH v2 1/5] gpio: Add support for hierarchical IRQ domains Thierry Reding
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Thierry Reding @ 2018-11-29 17:03 UTC (permalink / raw)
  To: Linus Walleij, Thomas Gleixner; +Cc: linux-gpio, linux-tegra, linux-kernel

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 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 contains the conversion patch to use a valid mask instead of
the sparse number space that you requested. I've kept it in a separate
commit because I'm still hoping that you may decide that it's okay to
have the sparse number space. The delta is just 53 lines of code, which
I think is small enough to warrant the extra code. But if you insist on
having the core take care of everything and that the Tegra GPIO driver
should be over-allocating GPIO and IRQ descriptors in order to make it
look more like other drivers, then so be it. In either case, the series
now has gpiolib core support for hierarchies and Tegra works with it in
both the sparse number space and valid mask cases.

Thomas, patch 2 is required in order to avoid build failures when the
Tegra186 GPIO driver is built as a module. Since there's a build
dependency on that from patch 4, I think it'd be easiest if this could
go in via Linus' tree with your Acked-by. Would you mind providing one?

Thierry

Thierry Reding (5):
  gpio: Add support for hierarchical IRQ domains
  genirq: Export irq_chip_set_wake_parent()
  gpio: tegra186: Rename flow variable to type
  gpio: tegra186: Implement wake event support
  gpio: tegra186: Use valid mask instead of sparse number space

 drivers/gpio/Kconfig         |   2 +-
 drivers/gpio/gpio-tegra186.c | 220 ++++++++++++++++++++---------------
 drivers/gpio/gpiolib.c       |  33 +++++-
 include/linux/gpio/driver.h  |   6 +
 kernel/irq/chip.c            |   1 +
 5 files changed, 161 insertions(+), 101 deletions(-)

-- 
2.19.1


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/5] gpio: Add support for hierarchical IRQ domains
  2018-11-29 17:03 [PATCH v2 0/5] Implement wake event support on Tegra186 and later Thierry Reding
@ 2018-11-29 17:03 ` Thierry Reding
  2018-12-14 13:41   ` Linus Walleij
  2018-11-29 17:03 ` [PATCH v2 2/5] genirq: Export irq_chip_set_wake_parent() Thierry Reding
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Thierry Reding @ 2018-11-29 17:03 UTC (permalink / raw)
  To: Linus Walleij, Thomas Gleixner; +Cc: linux-gpio, linux-tegra, linux-kernel

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(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 587e5f005b61..1d8abaab09f7 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -43,7 +43,7 @@ config GPIO_ACPI
 	depends on 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 cd84315ad586..247ca4c1241f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1777,9 +1777,22 @@ static const struct irq_domain_ops gpiochip_domain_ops = {
 
 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);
 }
 
@@ -1888,7 +1901,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;
@@ -1898,9 +1918,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 9c8d5d491680..eb8b35f1d8b6 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -47,6 +47,12 @@ struct gpio_irq_chip {
 	 */
 	const struct irq_domain_ops *domain_ops;
 
+	/**
+	 * @parent_domain:
+	 *
+	 */
+	struct irq_domain *parent_domain;
+
 	/**
 	 * @handler:
 	 *
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 2/5] genirq: Export irq_chip_set_wake_parent()
  2018-11-29 17:03 [PATCH v2 0/5] Implement wake event support on Tegra186 and later Thierry Reding
  2018-11-29 17:03 ` [PATCH v2 1/5] gpio: Add support for hierarchical IRQ domains Thierry Reding
@ 2018-11-29 17:03 ` Thierry Reding
  2018-12-05 21:31   ` Linus Walleij
  2018-11-29 17:03 ` [PATCH v2 3/5] gpio: tegra186: Rename flow variable to type Thierry Reding
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Thierry Reding @ 2018-11-29 17:03 UTC (permalink / raw)
  To: Linus Walleij, Thomas Gleixner; +Cc: linux-gpio, linux-tegra, linux-kernel

From: Thierry Reding <treding@nvidia.com>

Interrupt controllers in a hierarchy want to use this function to
propogate ->irq_set_wake() operations to their parent domains.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 kernel/irq/chip.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index a2b3d9de999c..b3aea5282243 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1381,6 +1381,7 @@ int irq_chip_set_wake_parent(struct irq_data *data, unsigned int on)
 
 	return -ENOSYS;
 }
+EXPORT_SYMBOL_GPL(irq_chip_set_wake_parent);
 #endif
 
 /**
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 3/5] gpio: tegra186: Rename flow variable to type
  2018-11-29 17:03 [PATCH v2 0/5] Implement wake event support on Tegra186 and later Thierry Reding
  2018-11-29 17:03 ` [PATCH v2 1/5] gpio: Add support for hierarchical IRQ domains Thierry Reding
  2018-11-29 17:03 ` [PATCH v2 2/5] genirq: Export irq_chip_set_wake_parent() Thierry Reding
@ 2018-11-29 17:03 ` Thierry Reding
  2018-12-14 13:34   ` Linus Walleij
  2018-11-29 17:03 ` [PATCH v2 4/5] gpio: tegra186: Implement wake event support Thierry Reding
  2018-11-29 17:03 ` [PATCH v2 5/5] gpio: tegra186: Use valid mask instead of sparse number space Thierry Reding
  4 siblings, 1 reply; 15+ messages in thread
From: Thierry Reding @ 2018-11-29 17:03 UTC (permalink / raw)
  To: Linus Walleij, Thomas Gleixner; +Cc: linux-gpio, linux-tegra, linux-kernel

From: Thierry Reding <treding@nvidia.com>

The IRQ core code refers to the interrupt type by that name, whereas the
term flow is almost never used. Some GPIO controllers use the term
flow_type, but it is most consistent to just go with the IRQ core
terminology.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpio/gpio-tegra186.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index 9d0292c8a199..66ec38bb7954 100644
--- a/drivers/gpio/gpio-tegra186.c
+++ b/drivers/gpio/gpio-tegra186.c
@@ -279,7 +279,7 @@ static void tegra186_irq_unmask(struct irq_data *data)
 	writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
 }
 
-static int tegra186_irq_set_type(struct irq_data *data, unsigned int flow)
+static int tegra186_irq_set_type(struct irq_data *data, unsigned int type)
 {
 	struct tegra_gpio *gpio = irq_data_get_irq_chip_data(data);
 	void __iomem *base;
@@ -293,7 +293,7 @@ static int tegra186_irq_set_type(struct irq_data *data, unsigned int flow)
 	value &= ~TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_TYPE_MASK;
 	value &= ~TEGRA186_GPIO_ENABLE_CONFIG_TRIGGER_LEVEL;
 
-	switch (flow & IRQ_TYPE_SENSE_MASK) {
+	switch (type & IRQ_TYPE_SENSE_MASK) {
 	case IRQ_TYPE_NONE:
 		break;
 
@@ -325,7 +325,7 @@ static int tegra186_irq_set_type(struct irq_data *data, unsigned int flow)
 
 	writel(value, base + TEGRA186_GPIO_ENABLE_CONFIG);
 
-	if ((flow & IRQ_TYPE_EDGE_BOTH) == 0)
+	if ((type & IRQ_TYPE_EDGE_BOTH) == 0)
 		irq_set_handler_locked(data, handle_level_irq);
 	else
 		irq_set_handler_locked(data, handle_edge_irq);
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 4/5] gpio: tegra186: Implement wake event support
  2018-11-29 17:03 [PATCH v2 0/5] Implement wake event support on Tegra186 and later Thierry Reding
                   ` (2 preceding siblings ...)
  2018-11-29 17:03 ` [PATCH v2 3/5] gpio: tegra186: Rename flow variable to type Thierry Reding
@ 2018-11-29 17:03 ` Thierry Reding
  2018-11-29 17:03 ` [PATCH v2 5/5] gpio: tegra186: Use valid mask instead of sparse number space Thierry Reding
  4 siblings, 0 replies; 15+ messages in thread
From: Thierry Reding @ 2018-11-29 17:03 UTC (permalink / raw)
  To: Linus Walleij, Thomas Gleixner; +Cc: linux-gpio, linux-tegra, linux-kernel

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 v2:
- don't use wakeup-parent property but look up PMC by compatible

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

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index 66ec38bb7954..1215e08ffe02 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] = 0;
+
+		return irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &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,73 @@ 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 = 0;
+
+	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 +477,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 +552,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 +571,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)
@@ -637,6 +716,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 +739,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.19.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 5/5] gpio: tegra186: Use valid mask instead of sparse number space
  2018-11-29 17:03 [PATCH v2 0/5] Implement wake event support on Tegra186 and later Thierry Reding
                   ` (3 preceding siblings ...)
  2018-11-29 17:03 ` [PATCH v2 4/5] gpio: tegra186: Implement wake event support Thierry Reding
@ 2018-11-29 17:03 ` Thierry Reding
  4 siblings, 0 replies; 15+ messages in thread
From: Thierry Reding @ 2018-11-29 17:03 UTC (permalink / raw)
  To: Linus Walleij, Thomas Gleixner; +Cc: linux-gpio, linux-tegra, linux-kernel

From: Thierry Reding <treding@nvidia.com>

While the sparse number space has the advantage of only providing (and
therefore allocating) the GPIOs (and interrupts) that really exist for
the given SoC, it also requires special cases in various callbacks.

Using the valid masks for GPIOs and interrupts enables reusing more of
the code provided by gpiolib at the expense of allocating more GPIOs
and interrupts than necessary.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpio/gpio-tegra186.c | 171 ++++++++++++-----------------------
 1 file changed, 59 insertions(+), 112 deletions(-)

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index 1215e08ffe02..69bcc057db26 100644
--- a/drivers/gpio/gpio-tegra186.c
+++ b/drivers/gpio/gpio-tegra186.c
@@ -70,35 +70,12 @@ struct tegra_gpio {
 	void __iomem *base;
 };
 
-static const struct tegra_gpio_port *
-tegra186_gpio_get_port(struct tegra_gpio *gpio, unsigned int *pin)
-{
-	unsigned int start = 0, i;
-
-	for (i = 0; i < gpio->soc->num_ports; i++) {
-		const struct tegra_gpio_port *port = &gpio->soc->ports[i];
-
-		if (*pin >= start && *pin < start + port->pins) {
-			*pin -= start;
-			return port;
-		}
-
-		start += port->pins;
-	}
-
-	return NULL;
-}
-
 static void __iomem *tegra186_gpio_get_base(struct tegra_gpio *gpio,
 					    unsigned int pin)
 {
-	const struct tegra_gpio_port *port;
-
-	port = tegra186_gpio_get_port(gpio, &pin);
-	if (!port)
-		return NULL;
+	const struct tegra_gpio_port *port = &gpio->soc->ports[pin / 8];
 
-	return gpio->base + port->offset + pin * 0x20;
+	return gpio->base + port->offset + (pin % 8) * 0x20;
 }
 
 static int tegra186_gpio_get_direction(struct gpio_chip *chip,
@@ -208,68 +185,6 @@ static void tegra186_gpio_set(struct gpio_chip *chip, unsigned int offset,
 	writel(value, base + TEGRA186_GPIO_OUTPUT_VALUE);
 }
 
-static int tegra186_gpio_of_xlate(struct gpio_chip *chip,
-				  const struct of_phandle_args *spec,
-				  u32 *flags)
-{
-	struct tegra_gpio *gpio = gpiochip_get_data(chip);
-	unsigned int port, pin, i, offset = 0;
-
-	if (WARN_ON(chip->of_gpio_n_cells < 2))
-		return -EINVAL;
-
-	if (WARN_ON(spec->args_count < chip->of_gpio_n_cells))
-		return -EINVAL;
-
-	port = spec->args[0] / 8;
-	pin = spec->args[0] % 8;
-
-	if (port >= gpio->soc->num_ports) {
-		dev_err(chip->parent, "invalid port number: %u\n", port);
-		return -EINVAL;
-	}
-
-	for (i = 0; i < port; i++)
-		offset += gpio->soc->ports[i].pins;
-
-	if (flags)
-		*flags = spec->args[1];
-
-	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] = 0;
-
-		return irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &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);
@@ -372,7 +287,7 @@ static void tegra186_gpio_irq(struct irq_desc *desc)
 	struct irq_domain *domain = gpio->gpio.irq.domain;
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 	unsigned int parent = irq_desc_get_irq(desc);
-	unsigned int i, offset = 0;
+	unsigned int i;
 
 	chained_irq_enter(chip, desc);
 
@@ -384,20 +299,17 @@ static void tegra186_gpio_irq(struct irq_desc *desc)
 
 		/* skip ports that are not associated with this controller */
 		if (parent != gpio->irq[port->irq])
-			goto skip;
+			continue;
 
 		value = readl(base + TEGRA186_GPIO_INTERRUPT_STATUS(1));
 
 		for_each_set_bit(pin, &value, port->pins) {
-			irq = irq_find_mapping(domain, offset + pin);
+			irq = irq_find_mapping(domain, i * 8 + pin);
 			if (WARN_ON(irq == 0))
 				continue;
 
 			generic_handle_irq(irq);
 		}
-
-skip:
-		offset += port->pins;
 	}
 
 	chained_irq_exit(chip, desc);
@@ -409,7 +321,7 @@ static int tegra186_gpio_irq_domain_translate(struct irq_domain *domain,
 					      unsigned int *type)
 {
 	struct tegra_gpio *gpio = gpiochip_get_data(domain->host_data);
-	unsigned int port, pin, i, offset = 0;
+	unsigned int port, pin;
 
 	if (WARN_ON(gpio->gpio.of_gpio_n_cells < 2))
 		return -EINVAL;
@@ -423,11 +335,8 @@ static int tegra186_gpio_irq_domain_translate(struct irq_domain *domain,
 	if (port >= gpio->soc->num_ports)
 		return -EINVAL;
 
-	for (i = 0; i < port; i++)
-		offset += gpio->soc->ports[i].pins;
-
 	*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
-	*hwirq = offset + pin;
+	*hwirq = port * 8 + pin;
 
 	return 0;
 }
@@ -466,6 +375,43 @@ static const struct irq_domain_ops tegra186_gpio_irq_domain_ops = {
 	.unmap = gpiochip_irq_unmap,
 };
 
+static void tegra186_gpio_set_valid_mask(struct tegra_gpio *gpio,
+					 unsigned long *mask)
+{
+	unsigned int i;
+
+	bitmap_zero(mask, gpio->gpio.ngpio);
+
+	for (i = 0; i < gpio->soc->num_ports; i++) {
+		const struct tegra_gpio_port *port = &gpio->soc->ports[i];
+
+		bitmap_set(mask, i * 8, port->pins);
+	}
+}
+
+static int tegra186_gpio_init_valid_mask(struct gpio_chip *chip)
+{
+	struct tegra_gpio *gpio = gpiochip_get_data(chip);
+
+	tegra186_gpio_set_valid_mask(gpio, chip->valid_mask);
+
+	return 0;
+}
+
+static unsigned long *tegra186_gpio_setup_valid_mask(struct tegra_gpio *gpio)
+{
+	unsigned long *mask;
+
+	mask = devm_kcalloc(gpio->gpio.parent, BITS_TO_LONGS(gpio->gpio.ngpio),
+			    sizeof(*mask), GFP_KERNEL);
+	if (!mask)
+		return ERR_PTR(-ENOMEM);
+
+	tegra186_gpio_set_valid_mask(gpio, mask);
+
+	return mask;
+}
+
 static const struct of_device_id tegra186_pmc_of_match[] = {
 	{ .compatible = "nvidia,tegra186-pmc" },
 	{ .compatible = "nvidia,tegra194-pmc" },
@@ -474,11 +420,11 @@ static const struct of_device_id tegra186_pmc_of_match[] = {
 
 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;
+	unsigned int i, j;
 	char **names;
 	int err;
 
@@ -521,17 +467,15 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
 	gpio->gpio.get = tegra186_gpio_get,
 	gpio->gpio.set = tegra186_gpio_set;
 
+	gpio->gpio.ngpio = gpio->soc->num_ports * 8;
 	gpio->gpio.base = -1;
 
-	for (i = 0; i < gpio->soc->num_ports; i++)
-		gpio->gpio.ngpio += gpio->soc->ports[i].pins;
-
 	names = devm_kcalloc(gpio->gpio.parent, gpio->gpio.ngpio,
 			     sizeof(*names), GFP_KERNEL);
 	if (!names)
 		return -ENOMEM;
 
-	for (i = 0, offset = 0; i < gpio->soc->num_ports; i++) {
+	for (i = 0; i < gpio->soc->num_ports; i++) {
 		const struct tegra_gpio_port *port = &gpio->soc->ports[i];
 		char *name;
 
@@ -541,18 +485,17 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
 			if (!name)
 				return -ENOMEM;
 
-			names[offset + j] = name;
+			names[i * 8 + j] = name;
 		}
-
-		offset += port->pins;
 	}
 
 	gpio->gpio.names = (const char * const *)names;
 
 	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->gpio.init_valid_mask = tegra186_gpio_init_valid_mask;
+	gpio->gpio.need_valid_mask = true;
 
 	gpio->intc.name = pdev->dev.of_node->name;
 	gpio->intc.irq_ack = tegra186_irq_ack;
@@ -580,18 +523,22 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
 			return -EPROBE_DEFER;
 	}
 
+	irq->valid_mask = tegra186_gpio_setup_valid_mask(gpio);
+	if (IS_ERR(irq->valid_mask))
+		return PTR_ERR(irq->valid_mask);
+
+	irq->need_valid_mask = true;
+
 	irq->map = devm_kcalloc(&pdev->dev, gpio->gpio.ngpio,
 				sizeof(*irq->map), GFP_KERNEL);
 	if (!irq->map)
 		return -ENOMEM;
 
-	for (i = 0, offset = 0; i < gpio->soc->num_ports; i++) {
+	for (i = 0; i < gpio->soc->num_ports; i++) {
 		const struct tegra_gpio_port *port = &gpio->soc->ports[i];
 
 		for (j = 0; j < port->pins; j++)
-			irq->map[offset + j] = irq->parents[port->irq];
-
-		offset += port->pins;
+			irq->map[i * 8 + j] = irq->parents[port->irq];
 	}
 
 	platform_set_drvdata(pdev, gpio);
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/5] genirq: Export irq_chip_set_wake_parent()
  2018-11-29 17:03 ` [PATCH v2 2/5] genirq: Export irq_chip_set_wake_parent() Thierry Reding
@ 2018-12-05 21:31   ` Linus Walleij
  2018-12-06 13:55     ` Marc Zyngier
  0 siblings, 1 reply; 15+ messages in thread
From: Linus Walleij @ 2018-12-05 21:31 UTC (permalink / raw)
  To: thierry.reding, Thomas Gleixner, Marc Zyngier
  Cc: open list:GPIO SUBSYSTEM, linux-tegra, linux-kernel

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

> From: Thierry Reding <treding@nvidia.com>
>
> Interrupt controllers in a hierarchy want to use this function to
> propogate ->irq_set_wake() operations to their parent domains.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Tglx/Marc: is this change OK? (ACK?)

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/5] genirq: Export irq_chip_set_wake_parent()
  2018-12-05 21:31   ` Linus Walleij
@ 2018-12-06 13:55     ` Marc Zyngier
  2018-12-06 23:12       ` Thomas Gleixner
  0 siblings, 1 reply; 15+ messages in thread
From: Marc Zyngier @ 2018-12-06 13:55 UTC (permalink / raw)
  To: Linus Walleij, thierry.reding, Thomas Gleixner
  Cc: open list:GPIO SUBSYSTEM, linux-tegra, linux-kernel

On 05/12/2018 21:31, Linus Walleij wrote:
> On Thu, Nov 29, 2018 at 6:03 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> 
>> From: Thierry Reding <treding@nvidia.com>
>>
>> Interrupt controllers in a hierarchy want to use this function to
>> propogate ->irq_set_wake() operations to their parent domains.
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> Tglx/Marc: is this change OK? (ACK?)

It looks OK to me, as it is consistent with irq_chip_set_type_parent,
among others.

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/5] genirq: Export irq_chip_set_wake_parent()
  2018-12-06 13:55     ` Marc Zyngier
@ 2018-12-06 23:12       ` Thomas Gleixner
  0 siblings, 0 replies; 15+ messages in thread
From: Thomas Gleixner @ 2018-12-06 23:12 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Linus Walleij, thierry.reding, open list:GPIO SUBSYSTEM,
	linux-tegra, linux-kernel

On Thu, 6 Dec 2018, Marc Zyngier wrote:

> On 05/12/2018 21:31, Linus Walleij wrote:
> > On Thu, Nov 29, 2018 at 6:03 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> >> From: Thierry Reding <treding@nvidia.com>
> >>
> >> Interrupt controllers in a hierarchy want to use this function to
> >> propogate ->irq_set_wake() operations to their parent domains.
> >>
> >> Signed-off-by: Thierry Reding <treding@nvidia.com>
> > 
> > Tglx/Marc: is this change OK? (ACK?)
> 
> It looks OK to me, as it is consistent with irq_chip_set_type_parent,
> among others.
> 
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>

No objections either:

Acked-by: Thomas Gleixner <tglx@linutronix.de>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/5] gpio: tegra186: Rename flow variable to type
  2018-11-29 17:03 ` [PATCH v2 3/5] gpio: tegra186: Rename flow variable to type Thierry Reding
@ 2018-12-14 13:34   ` Linus Walleij
  2018-12-14 13:36     ` Thierry Reding
  0 siblings, 1 reply; 15+ messages in thread
From: Linus Walleij @ 2018-12-14 13:34 UTC (permalink / raw)
  To: thierry.reding
  Cc: Thomas Gleixner, open list:GPIO SUBSYSTEM, linux-tegra, linux-kernel

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

> From: Thierry Reding <treding@nvidia.com>
>
> The IRQ core code refers to the interrupt type by that name, whereas the
> term flow is almost never used. Some GPIO controllers use the term
> flow_type, but it is most consistent to just go with the IRQ core
> terminology.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Patch applied.

No need to wait for genirq or other changes to apply this.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/5] gpio: tegra186: Rename flow variable to type
  2018-12-14 13:34   ` Linus Walleij
@ 2018-12-14 13:36     ` Thierry Reding
  2018-12-14 13:51       ` Linus Walleij
  0 siblings, 1 reply; 15+ messages in thread
From: Thierry Reding @ 2018-12-14 13:36 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Thomas Gleixner, open list:GPIO SUBSYSTEM, linux-tegra, linux-kernel

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

On Fri, Dec 14, 2018 at 02:34:43PM +0100, Linus Walleij wrote:
> On Thu, Nov 29, 2018 at 6:03 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > From: Thierry Reding <treding@nvidia.com>
> >
> > The IRQ core code refers to the interrupt type by that name, whereas the
> > term flow is almost never used. Some GPIO controllers use the term
> > flow_type, but it is most consistent to just go with the IRQ core
> > terminology.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> Patch applied.
> 
> No need to wait for genirq or other changes to apply this.

The genirq patch was acked by both Thomas and Marc. Were you waiting for
anything else on the rest of this series?

Thierry

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/5] gpio: Add support for hierarchical IRQ domains
  2018-11-29 17:03 ` [PATCH v2 1/5] gpio: Add support for hierarchical IRQ domains Thierry Reding
@ 2018-12-14 13:41   ` Linus Walleij
  2018-12-18 22:06     ` Thierry Reding
  0 siblings, 1 reply; 15+ messages in thread
From: Linus Walleij @ 2018-12-14 13:41 UTC (permalink / raw)
  To: thierry.reding
  Cc: Thomas Gleixner, open list:GPIO SUBSYSTEM, linux-tegra,
	linux-kernel, Brian Masney

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/5] gpio: tegra186: Rename flow variable to type
  2018-12-14 13:36     ` Thierry Reding
@ 2018-12-14 13:51       ` Linus Walleij
  0 siblings, 0 replies; 15+ messages in thread
From: Linus Walleij @ 2018-12-14 13:51 UTC (permalink / raw)
  To: thierry.reding
  Cc: Thomas Gleixner, open list:GPIO SUBSYSTEM, linux-tegra, linux-kernel

On Fri, Dec 14, 2018 at 2:36 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> On Fri, Dec 14, 2018 at 02:34:43PM +0100, Linus Walleij wrote:
> > On Thu, Nov 29, 2018 at 6:03 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > The IRQ core code refers to the interrupt type by that name, whereas the
> > > term flow is almost never used. Some GPIO controllers use the term
> > > flow_type, but it is most consistent to just go with the IRQ core
> > > terminology.
> > >
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> >
> > Patch applied.
> >
> > No need to wait for genirq or other changes to apply this.
>
> The genirq patch was acked by both Thomas and Marc. Were you waiting for
> anything else on the rest of this series?

Yeah I had some comments on patch 1 (sorry for slowness ...)
but it's something I think you can fix up and test in 30 min.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/5] gpio: Add support for hierarchical IRQ domains
  2018-12-14 13:41   ` Linus Walleij
@ 2018-12-18 22:06     ` Thierry Reding
  2018-12-21 13:20       ` Linus Walleij
  0 siblings, 1 reply; 15+ messages in thread
From: Thierry Reding @ 2018-12-18 22:06 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Thomas Gleixner, open list:GPIO SUBSYSTEM, linux-tegra,
	linux-kernel, Brian Masney

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

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

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/5] gpio: Add support for hierarchical IRQ domains
  2018-12-18 22:06     ` Thierry Reding
@ 2018-12-21 13:20       ` Linus Walleij
  0 siblings, 0 replies; 15+ messages in thread
From: Linus Walleij @ 2018-12-21 13:20 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Thomas Gleixner, open list:GPIO SUBSYSTEM, linux-tegra,
	linux-kernel, Brian Masney, NeilBrown, Marc Zyngier

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2018-12-21 13:20 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-29 17:03 [PATCH v2 0/5] Implement wake event support on Tegra186 and later Thierry Reding
2018-11-29 17:03 ` [PATCH v2 1/5] gpio: Add support for hierarchical IRQ domains Thierry Reding
2018-12-14 13:41   ` Linus Walleij
2018-12-18 22:06     ` Thierry Reding
2018-12-21 13:20       ` Linus Walleij
2018-11-29 17:03 ` [PATCH v2 2/5] genirq: Export irq_chip_set_wake_parent() Thierry Reding
2018-12-05 21:31   ` Linus Walleij
2018-12-06 13:55     ` Marc Zyngier
2018-12-06 23:12       ` Thomas Gleixner
2018-11-29 17:03 ` [PATCH v2 3/5] gpio: tegra186: Rename flow variable to type Thierry Reding
2018-12-14 13:34   ` Linus Walleij
2018-12-14 13:36     ` Thierry Reding
2018-12-14 13:51       ` Linus Walleij
2018-11-29 17:03 ` [PATCH v2 4/5] gpio: tegra186: Implement wake event support Thierry Reding
2018-11-29 17:03 ` [PATCH v2 5/5] gpio: tegra186: Use valid mask instead of sparse number space Thierry Reding

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).