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

[PATCH 0/9] 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

Linus, I'm sending this as one series, but both the GPIO and PMC patches
should be applicable separately. There are no build-time dependencies in
the series. So once this has been duly reviewed, I think patches 1-5 can
go through the Tegra tree, while patches 6-9 should go through the GPIO
tree.

Thierry

Thierry Reding (9):
  dt-bindings: tegra186-pmc: Add interrupt controller properties
  soc/tegra: pmc: Add Tegra194 support
  soc/tegra: pmc: Add wake event support
  soc/tegra: pmc: Add initial Tegra186 wake events
  soc/tegra: pmc: Add initial Tegra194 wake events
  gpio: Add support for hierarchical IRQ domains
  dt-bindings: tegra186-gpio: Add wakeup parent support
  gpio: tegra186: Rename flow variable to type
  gpio: tegra186: Implement wake event support

 .../arm/tegra/nvidia,tegra186-pmc.txt         |   3 +
 .../bindings/gpio/nvidia,tegra186-gpio.txt    |   7 +
 drivers/gpio/gpio-tegra186.c                  | 109 +++++-
 drivers/gpio/gpiolib.c                        |  15 +-
 drivers/soc/tegra/pmc.c                       | 312 +++++++++++++++++-
 include/linux/gpio/driver.h                   |   6 +
 include/soc/tegra/pmc.h                       |  21 ++
 7 files changed, 451 insertions(+), 22 deletions(-)

-- 
2.19.0

[PATCH 1/9] dt-bindings: tegra186-pmc: Add interrupt controller properties

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The PMC can be a top-level interrupt controller that provides the top-
level controls for wake events. Add optional properties to mark the PMC
as interrupt controller.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt      | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt
index c9fd6d1de57e..2d89cdc39eb0 100644
--- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt
@@ -15,6 +15,9 @@ Required properties:
 
 Optional properties:
 - nvidia,invert-interrupt: If present, inverts the PMU interrupt signal.
+- interrupt-controller: Identifies the node as an interrupt controller.
+- #interrupt-cells: Specifies the number of cells needed to encode an
+  interrupt source. The value must be 2.
 
 Example:
 
-- 
2.19.0

Re: [PATCH 1/9] dt-bindings: tegra186-pmc: Add interrupt controller properties

[Rob Herring]

On Fri, 21 Sep 2018 12:25:38 +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The PMC can be a top-level interrupt controller that provides the top-
> level controls for wake events. Add optional properties to mark the PMC
> as interrupt controller.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt      | 3 +++
>  1 file changed, 3 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/9] dt-bindings: tegra186-pmc: Add interrupt controller properties

[Rob Herring]

On Fri, 21 Sep 2018 12:25:38 +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The PMC can be a top-level interrupt controller that provides the top-
> level controls for wake events. Add optional properties to mark the PMC
> as interrupt controller.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../devicetree/bindings/arm/tegra/nvidia,tegra186-pmc.txt      | 3 +++
>  1 file changed, 3 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 2/9] soc/tegra: pmc: Add Tegra194 support

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The PMC controller on Tegra194 has a couple of new I/O pads and drops
others compared to Tegra186.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 66 ++++++++++++++++++++++++++++++++++++++++-
 include/soc/tegra/pmc.h | 21 +++++++++++++
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index ab719fa90150..c08f0b942020 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -2138,8 +2138,72 @@ static const struct tegra_pmc_soc tegra186_pmc_soc = {
 	.setup_irq_polarity = tegra186_pmc_setup_irq_polarity,
 };
 
+static const struct tegra_io_pad_soc tegra194_io_pads[] = {
+	{ .id = TEGRA_IO_PAD_CSIA, .dpd = 0, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_CSIB, .dpd = 1, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_MIPI_BIAS, .dpd = 3, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_PEX_CLK_BIAS, .dpd = 4, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_PEX_CLK3, .dpd = 5, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_PEX_CLK2, .dpd = 6, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_PEX_CLK1, .dpd = 7, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_EQOS, .dpd = 8, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_PEX_CLK2_BIAS, .dpd = 9, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_PEX_CLK2, .dpd = 10, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_DAP3, .dpd = 11, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_DAP5, .dpd = 12, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_UART, .dpd = 14, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_PWR_CTL, .dpd = 15, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_SOC_GPIO53, .dpd = 16, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_AUDIO, .dpd = 17, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_GP_PWM2, .dpd = 18, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_GP_PWM3, .dpd = 19, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_SOC_GPIO12, .dpd = 20, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_SOC_GPIO13, .dpd = 21, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_SOC_GPIO10, .dpd = 22, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_UART4, .dpd = 23, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_UART5, .dpd = 24, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_DBG, .dpd = 25, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_HDMI_DP3, .dpd = 26, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_HDMI_DP2, .dpd = 27, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_HDMI_DP0, .dpd = 28, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_HDMI_DP1, .dpd = 29, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_PEX_CNTRL, .dpd = 32, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_PEX_CTL2, .dpd = 33, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_PEX_L0_RST_N, .dpd = 34, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_PEX_L1_RST_N, .dpd = 35, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_SDMMC4, .dpd = 36, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_PEX_L5_RST_N, .dpd = 37, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_CSIC, .dpd = 43, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_CSID, .dpd = 44, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_CSIE, .dpd = 45, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_CSIF, .dpd = 46, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_SPI, .dpd = 47, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_UFS, .dpd = 49, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_CSIG, .dpd = 50, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_CSIH, .dpd = 51, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_EDP, .dpd = 53, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_SDMMC1_HV, .dpd = 55, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_SDMMC3_HV, .dpd = 56, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_CONN, .dpd = 60, .voltage = UINT_MAX },
+	{ .id = TEGRA_IO_PAD_AUDIO_HV, .dpd = 61, .voltage = UINT_MAX },
+};
+
+static const struct tegra_pmc_soc tegra194_pmc_soc = {
+	.num_powergates = 0,
+	.powergates = NULL,
+	.num_cpu_powergates = 0,
+	.cpu_powergates = NULL,
+	.has_tsense_reset = false,
+	.has_gpu_clamps = false,
+	.num_io_pads = ARRAY_SIZE(tegra194_io_pads),
+	.io_pads = tegra194_io_pads,
+	.regs = &tegra186_pmc_regs,
+	.init = NULL,
+	.setup_irq_polarity = tegra186_pmc_setup_irq_polarity,
+};
+
 static const struct of_device_id tegra_pmc_match[] = {
-	{ .compatible = "nvidia,tegra194-pmc", .data = &tegra186_pmc_soc },
+	{ .compatible = "nvidia,tegra194-pmc", .data = &tegra194_pmc_soc },
 	{ .compatible = "nvidia,tegra186-pmc", .data = &tegra186_pmc_soc },
 	{ .compatible = "nvidia,tegra210-pmc", .data = &tegra210_pmc_soc },
 	{ .compatible = "nvidia,tegra132-pmc", .data = &tegra124_pmc_soc },
diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
index 562426812ab2..fd816f6aa9cc 100644
--- a/include/soc/tegra/pmc.h
+++ b/include/soc/tegra/pmc.h
@@ -90,6 +90,10 @@ enum tegra_io_pad {
 	TEGRA_IO_PAD_CSID,
 	TEGRA_IO_PAD_CSIE,
 	TEGRA_IO_PAD_CSIF,
+	TEGRA_IO_PAD_CSIG,
+	TEGRA_IO_PAD_CSIH,
+	TEGRA_IO_PAD_DAP3,
+	TEGRA_IO_PAD_DAP5,
 	TEGRA_IO_PAD_DBG,
 	TEGRA_IO_PAD_DEBUG_NONAO,
 	TEGRA_IO_PAD_DMIC,
@@ -102,10 +106,15 @@ enum tegra_io_pad {
 	TEGRA_IO_PAD_EDP,
 	TEGRA_IO_PAD_EMMC,
 	TEGRA_IO_PAD_EMMC2,
+	TEGRA_IO_PAD_EQOS,
 	TEGRA_IO_PAD_GPIO,
+	TEGRA_IO_PAD_GP_PWM2,
+	TEGRA_IO_PAD_GP_PWM3,
 	TEGRA_IO_PAD_HDMI,
 	TEGRA_IO_PAD_HDMI_DP0,
 	TEGRA_IO_PAD_HDMI_DP1,
+	TEGRA_IO_PAD_HDMI_DP2,
+	TEGRA_IO_PAD_HDMI_DP3,
 	TEGRA_IO_PAD_HSIC,
 	TEGRA_IO_PAD_HV,
 	TEGRA_IO_PAD_LVDS,
@@ -115,8 +124,14 @@ enum tegra_io_pad {
 	TEGRA_IO_PAD_PEX_CLK_BIAS,
 	TEGRA_IO_PAD_PEX_CLK1,
 	TEGRA_IO_PAD_PEX_CLK2,
+	TEGRA_IO_PAD_PEX_CLK2_BIAS,
 	TEGRA_IO_PAD_PEX_CLK3,
 	TEGRA_IO_PAD_PEX_CNTRL,
+	TEGRA_IO_PAD_PEX_CTL2,
+	TEGRA_IO_PAD_PEX_L0_RST_N,
+	TEGRA_IO_PAD_PEX_L1_RST_N,
+	TEGRA_IO_PAD_PEX_L5_RST_N,
+	TEGRA_IO_PAD_PWR_CTL,
 	TEGRA_IO_PAD_SDMMC1,
 	TEGRA_IO_PAD_SDMMC1_HV,
 	TEGRA_IO_PAD_SDMMC2,
@@ -124,10 +139,16 @@ enum tegra_io_pad {
 	TEGRA_IO_PAD_SDMMC3,
 	TEGRA_IO_PAD_SDMMC3_HV,
 	TEGRA_IO_PAD_SDMMC4,
+	TEGRA_IO_PAD_SOC_GPIO10,
+	TEGRA_IO_PAD_SOC_GPIO12,
+	TEGRA_IO_PAD_SOC_GPIO13,
+	TEGRA_IO_PAD_SOC_GPIO53,
 	TEGRA_IO_PAD_SPI,
 	TEGRA_IO_PAD_SPI_HV,
 	TEGRA_IO_PAD_SYS_DDC,
 	TEGRA_IO_PAD_UART,
+	TEGRA_IO_PAD_UART4,
+	TEGRA_IO_PAD_UART5,
 	TEGRA_IO_PAD_UFS,
 	TEGRA_IO_PAD_USB0,
 	TEGRA_IO_PAD_USB1,
-- 
2.19.0

[PATCH 3/9] soc/tegra: pmc: Add wake event support

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The power management controller has top-level controls that allow
certain interrupts (such as from the RTC or a subset of GPIOs) to
wake the system from sleep. Implement infrastructure to support
these wake events.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 230 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 230 insertions(+)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index c08f0b942020..eeef5a1f2837 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -29,9 +29,12 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_clk.h>
+#include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/pinctrl/pinconf.h>
@@ -48,6 +51,7 @@
 #include <soc/tegra/fuse.h>
 #include <soc/tegra/pmc.h>
 
+#include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/pinctrl/pinctrl-tegra-io-pad.h>
 
 #define PMC_CNTRL			0x0
@@ -126,6 +130,18 @@
 #define GPU_RG_CNTRL			0x2d4
 
 /* Tegra186 and later */
+#define WAKE_AOWAKE_CNTRL(x) (0x000 + ((x) << 2))
+#define WAKE_AOWAKE_CNTRL_LEVEL (1 << 3)
+#define WAKE_AOWAKE_MASK_W(x) (0x180 + ((x) << 2))
+#define WAKE_AOWAKE_MASK_R(x) (0x300 + ((x) << 2))
+#define WAKE_AOWAKE_STATUS_W(x) (0x30c + ((x) << 2))
+#define WAKE_AOWAKE_STATUS_R(x) (0x48c + ((x) << 2))
+#define WAKE_AOWAKE_TIER1_CTRL 0x4ac
+#define WAKE_AOWAKE_TIER2_CTRL 0x4b0
+#define WAKE_AOWAKE_TIER0_ROUTING(x) (0x4b4 + ((x) << 2))
+#define WAKE_AOWAKE_TIER1_ROUTING(x) (0x4c0 + ((x) << 2))
+#define WAKE_AOWAKE_TIER2_ROUTING(x) (0x4cc + ((x) << 2))
+
 #define WAKE_AOWAKE_CTRL 0x4f4
 #define  WAKE_AOWAKE_CTRL_INTR_POLARITY BIT(0)
 
@@ -153,6 +169,38 @@ struct tegra_pmc_regs {
 	unsigned int dpd2_status;
 };
 
+struct tegra_wake_event {
+	const char *name;
+	unsigned int id;
+	unsigned int irq;
+	struct {
+		unsigned int instance;
+		unsigned int pin;
+	} gpio;
+};
+
+#define TEGRA_WAKE_IRQ(_name, _id, _irq)		\
+	{						\
+		.name = _name,				\
+		.id = _id,				\
+		.irq = _irq,				\
+		.gpio = {				\
+			.instance = UINT_MAX,		\
+			.pin = UINT_MAX,		\
+		},					\
+	}
+
+#define TEGRA_WAKE_GPIO(_name, _id, _instance, _pin)	\
+	{						\
+		.name = _name,				\
+		.id = _id,				\
+		.irq = 0,				\
+		.gpio = {				\
+			.instance = _instance,		\
+			.pin = _pin,			\
+		},					\
+	}
+
 struct tegra_pmc_soc {
 	unsigned int num_powergates;
 	const char *const *powergates;
@@ -175,6 +223,9 @@ struct tegra_pmc_soc {
 	void (*setup_irq_polarity)(struct tegra_pmc *pmc,
 				   struct device_node *np,
 				   bool invert);
+
+	const struct tegra_wake_event *wake_events;
+	unsigned int num_wake_events;
 };
 
 /**
@@ -230,6 +281,9 @@ struct tegra_pmc {
 	struct mutex powergates_lock;
 
 	struct pinctrl_dev *pctl_dev;
+
+	struct irq_domain *domain;
+	struct irq_chip irq;
 };
 
 static struct tegra_pmc *pmc = &(struct tegra_pmc) {
@@ -1543,6 +1597,178 @@ static int tegra_pmc_pinctrl_init(struct tegra_pmc *pmc)
 	return err;
 }
 
+static int tegra_pmc_irq_translate(struct irq_domain *domain,
+				   struct irq_fwspec *fwspec,
+				   unsigned long *hwirq,
+				   unsigned int *type)
+{
+	if (WARN_ON(fwspec->param_count < 2))
+		return -EINVAL;
+
+	*hwirq = fwspec->param[0];
+	*type = fwspec->param[1];
+
+	return 0;
+}
+
+static int tegra_pmc_irq_alloc(struct irq_domain *domain, unsigned int virq,
+			       unsigned int num_irqs, void *data)
+{
+	struct tegra_pmc *pmc = domain->host_data;
+	struct irq_fwspec *fwspec = data;
+	unsigned int i;
+	int err = 0;
+
+	for (i = 0; i < pmc->soc->num_wake_events; i++) {
+		const struct tegra_wake_event *event = &pmc->soc->wake_events[i];
+
+		if (fwspec->param_count == 2) {
+			struct irq_fwspec spec;
+
+			if (event->id != fwspec->param[0])
+				continue;
+
+			err = irq_domain_set_hwirq_and_chip(domain, virq,
+							    event->id,
+							    &pmc->irq, pmc);
+			if (err < 0)
+				break;
+
+			spec.fwnode = &pmc->dev->of_node->fwnode;
+			spec.param_count = 3;
+			spec.param[0] = GIC_SPI;
+			spec.param[1] = event->irq;
+			spec.param[2] = fwspec->param[1];
+
+			err = irq_domain_alloc_irqs_parent(domain, virq,
+							   num_irqs, &spec);
+
+			break;
+		}
+
+		if (fwspec->param_count == 3) {
+			if (event->gpio.instance != fwspec->param[0] ||
+			    event->gpio.pin != fwspec->param[1])
+				continue;
+
+			err = irq_domain_set_hwirq_and_chip(domain, virq,
+							    event->id,
+							    &pmc->irq, pmc);
+
+			break;
+		}
+	}
+
+	if (i == pmc->soc->num_wake_events)
+		err = irq_domain_set_hwirq_and_chip(domain, virq, ULONG_MAX,
+						    &pmc->irq, pmc);
+
+	return err;
+}
+
+static const struct irq_domain_ops tegra_pmc_irq_domain_ops = {
+	.translate = tegra_pmc_irq_translate,
+	.alloc = tegra_pmc_irq_alloc,
+};
+
+static int tegra_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
+{
+	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
+	unsigned int offset, bit;
+	u32 value;
+
+	offset = data->hwirq / 32;
+	bit = data->hwirq % 32;
+
+	/* clear wake status */
+	writel(0x1, pmc->wake + WAKE_AOWAKE_STATUS_W(data->hwirq));
+
+	/* route wake to tier 2 (XXX conditionally enable this) */
+	value = readl(pmc->wake + WAKE_AOWAKE_TIER2_CTRL);
+	writel(0x1, pmc->wake + WAKE_AOWAKE_TIER2_CTRL);
+
+	value = readl(pmc->wake + WAKE_AOWAKE_TIER2_ROUTING(offset));
+
+	if (!on)
+		value &= ~(1 << bit);
+	else
+		value |= 1 << bit;
+
+	writel(value, pmc->wake + WAKE_AOWAKE_TIER2_ROUTING(offset));
+
+	/* enable wakeup event */
+	writel(!!on, pmc->wake + WAKE_AOWAKE_MASK_W(data->hwirq));
+
+	return 0;
+}
+
+static int tegra_pmc_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
+	u32 value;
+
+	if (data->hwirq == ULONG_MAX)
+		return 0;
+
+	value = readl(pmc->wake + WAKE_AOWAKE_CNTRL(data->hwirq));
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_LEVEL_HIGH:
+		value |= WAKE_AOWAKE_CNTRL_LEVEL;
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+	case IRQ_TYPE_LEVEL_LOW:
+		value &= ~WAKE_AOWAKE_CNTRL_LEVEL;
+		break;
+
+	case IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING:
+		value ^= WAKE_AOWAKE_CNTRL_LEVEL;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	writel(value, pmc->wake + WAKE_AOWAKE_CNTRL(data->hwirq));
+
+	return 0;
+}
+
+static int tegra_pmc_irq_init(struct tegra_pmc *pmc)
+{
+	struct irq_domain *parent = NULL;
+	struct device_node *np;
+
+	np = of_irq_find_parent(pmc->dev->of_node);
+	if (np) {
+		parent = irq_find_host(np);
+		of_node_put(np);
+	}
+
+	if (parent) {
+		pmc->irq.name = dev_name(pmc->dev);
+		pmc->irq.irq_mask = irq_chip_mask_parent;
+		pmc->irq.irq_unmask = irq_chip_unmask_parent;
+		pmc->irq.irq_eoi = irq_chip_eoi_parent;
+		pmc->irq.irq_set_affinity = irq_chip_set_affinity_parent;
+		pmc->irq.irq_set_type = tegra_pmc_irq_set_type;
+		pmc->irq.irq_set_wake = tegra_pmc_irq_set_wake;
+
+		pmc->domain = irq_domain_add_hierarchy(parent, 0, 96,
+						       pmc->dev->of_node,
+						       &tegra_pmc_irq_domain_ops,
+						       pmc);
+		if (!pmc->domain) {
+			dev_err(pmc->dev, "failed to allocate domain\n");
+			return -ENOMEM;
+		}
+	}
+
+	return 0;
+}
+
 static int tegra_pmc_probe(struct platform_device *pdev)
 {
 	void __iomem *base;
@@ -1629,6 +1855,10 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 	if (err)
 		goto cleanup_restart_handler;
 
+	err = tegra_pmc_irq_init(pmc);
+	if (err < 0)
+		goto cleanup_restart_handler;
+
 	mutex_lock(&pmc->powergates_lock);
 	iounmap(pmc->base);
 	pmc->base = base;
-- 
2.19.0

Re: [PATCH 3/9] soc/tegra: pmc: Add wake event support

[Mikko Perttunen]

On 21/09/2018 19.25, Thierry Reding wrote:
> ...
> +	/* route wake to tier 2 (XXX conditionally enable this) */
> +	value = readl(pmc->wake + WAKE_AOWAKE_TIER2_CTRL);
> +	writel(0x1, pmc->wake + WAKE_AOWAKE_TIER2_CTRL);

This doesn't seem right

Cheers,
Mikko

[PATCH 4/9] soc/tegra: pmc: Add initial Tegra186 wake events

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Tegra186 support 96 wake events in total. Many of them are never used,
so only the most common ones (RTC alarm and power key) are currently
defined.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index eeef5a1f2837..42e7deb6de74 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -53,6 +53,7 @@
 
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/pinctrl/pinctrl-tegra-io-pad.h>
+#include <dt-bindings/gpio/tegra186-gpio.h>
 
 #define PMC_CNTRL			0x0
 #define  PMC_CNTRL_INTR_POLARITY	BIT(17) /* inverts INTR polarity */
@@ -2351,6 +2352,11 @@ static void tegra186_pmc_setup_irq_polarity(struct tegra_pmc *pmc,
 	iounmap(wake);
 }
 
+static const struct tegra_wake_event tegra186_wake_events[] = {
+	TEGRA_WAKE_GPIO("power", 29, 1, TEGRA_AON_GPIO(FF, 0)),
+	TEGRA_WAKE_IRQ("rtc", 73, 10),
+};
+
 static const struct tegra_pmc_soc tegra186_pmc_soc = {
 	.num_powergates = 0,
 	.powergates = NULL,
@@ -2366,6 +2372,8 @@ static const struct tegra_pmc_soc tegra186_pmc_soc = {
 	.regs = &tegra186_pmc_regs,
 	.init = NULL,
 	.setup_irq_polarity = tegra186_pmc_setup_irq_polarity,
+	.num_wake_events = ARRAY_SIZE(tegra186_wake_events),
+	.wake_events = tegra186_wake_events,
 };
 
 static const struct tegra_io_pad_soc tegra194_io_pads[] = {
-- 
2.19.0

[PATCH 5/9] soc/tegra: pmc: Add initial Tegra194 wake events

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Tegra194 supports 96 wake events in total. Many of them are never used,
so only the most common ones (RTC alarm and power key) are currently
defined.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 42e7deb6de74..efe990985fe6 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -54,6 +54,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/pinctrl/pinctrl-tegra-io-pad.h>
 #include <dt-bindings/gpio/tegra186-gpio.h>
+#include <dt-bindings/gpio/tegra194-gpio.h>
 
 #define PMC_CNTRL			0x0
 #define  PMC_CNTRL_INTR_POLARITY	BIT(17) /* inverts INTR polarity */
@@ -2426,6 +2427,11 @@ static const struct tegra_io_pad_soc tegra194_io_pads[] = {
 	{ .id = TEGRA_IO_PAD_AUDIO_HV, .dpd = 61, .voltage = UINT_MAX },
 };
 
+static const struct tegra_wake_event tegra194_wake_events[] = {
+	TEGRA_WAKE_GPIO("power", 29, 1, TEGRA194_AON_GPIO(EE, 4)),
+	TEGRA_WAKE_IRQ("rtc", 73, 10),
+};
+
 static const struct tegra_pmc_soc tegra194_pmc_soc = {
 	.num_powergates = 0,
 	.powergates = NULL,
@@ -2438,6 +2444,8 @@ static const struct tegra_pmc_soc tegra194_pmc_soc = {
 	.regs = &tegra186_pmc_regs,
 	.init = NULL,
 	.setup_irq_polarity = tegra186_pmc_setup_irq_polarity,
+	.num_wake_events = ARRAY_SIZE(tegra194_wake_events),
+	.wake_events = tegra194_wake_events,
 };
 
 static const struct of_device_id tegra_pmc_match[] = {
-- 
2.19.0

[PATCH 6/9] 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>
---
 drivers/gpio/gpiolib.c      | 15 +++++++++++----
 include/linux/gpio/driver.h |  6 ++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a53d17745d21..94146093ee95 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1918,7 +1918,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 		type = IRQ_TYPE_NONE;
 	}
 
-	gpiochip->to_irq = gpiochip_to_irq;
+	if (!gpiochip->to_irq)
+		gpiochip->to_irq = gpiochip_to_irq;
+
 	gpiochip->irq.default_type = type;
 	gpiochip->irq.lock_key = lock_key;
 	gpiochip->irq.request_key = request_key;
@@ -1928,9 +1930,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 d8dcd0e44cab..fcd09a396d76 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.0

Re: [PATCH 6/9] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

Hi Thierry!

Thanks for the patch!

I am a bit ignorant about irqdomains so I will probably need an ACK
from some irq maintainer before I can apply this.

On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
<thierry.reding@gmail.com> wrote:

> From: Thierry Reding <treding@nvidia.com>
>
> Hierarchical IRQ domains can be used to stack different IRQ controllers
> on top of each other. One specific use-case where this can be useful is
> if a power management controller has top-level controls for wakeup
> interrupts. In such cases, the power management controller can be a
> parent to other interrupt controllers and program additional registers
> when an IRQ has its wake capability enabled or disabled.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

While I think it is really important that we start supporting hierarchical
irqdomains in the gpiolib core, I want a more complete approach,
so that drivers that need hierarchical handling of irqdomains
can get the same support from gpiolib as they get for simple
domains.

> @@ -1918,7 +1918,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
>                 type = IRQ_TYPE_NONE;
>         }
>
> -       gpiochip->to_irq = gpiochip_to_irq;
> +       if (!gpiochip->to_irq)
> +               gpiochip->to_irq = gpiochip_to_irq;

So here you let the drivers override the .to_irq() function and that
I think gets confusing as we are asking gpiolib to handle our
irqchip.


> -       gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> -                                                    gpiochip->irq.first,
> -                                                    ops, gpiochip);
> +       if (gpiochip->irq.parent_domain)
> +               gpiochip->irq.domain = irq_domain_add_hierarchy(gpiochip->irq.parent_domain,
> +                                                               0, gpiochip->ngpio,
> +                                                               np, ops, gpiochip);
> +       else
> +               gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> +                                                            gpiochip->irq.first,
> +                                                            ops, gpiochip);

So this part is great: if we pass in a parent domain the core helps us
create the hierarchy.

But the stuff in .to_irq() should also be handled in the gpiolib core:
the irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &spec) for
example. That way you should not need any external .to_irq() function.

I can't see if you need to pull more stuff into the core to accomplish
that, but I think in essence the core gpiolib needs to be more helpful
with hierarchies.

Yours,
Linus Walleij

Re: [PATCH 6/9] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

Hi Thierry!

Thanks for the patch!

I am a bit ignorant about irqdomains so I will probably need an ACK
from some irq maintainer before I can apply this.

On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
<thierry.reding@gmail.com> wrote:

> From: Thierry Reding <treding@nvidia.com>
>
> Hierarchical IRQ domains can be used to stack different IRQ controllers
> on top of each other. One specific use-case where this can be useful is
> if a power management controller has top-level controls for wakeup
> interrupts. In such cases, the power management controller can be a
> parent to other interrupt controllers and program additional registers
> when an IRQ has its wake capability enabled or disabled.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

While I think it is really important that we start supporting hierarchical
irqdomains in the gpiolib core, I want a more complete approach,
so that drivers that need hierarchical handling of irqdomains
can get the same support from gpiolib as they get for simple
domains.

> @@ -1918,7 +1918,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
>                 type = IRQ_TYPE_NONE;
>         }
>
> -       gpiochip->to_irq = gpiochip_to_irq;
> +       if (!gpiochip->to_irq)
> +               gpiochip->to_irq = gpiochip_to_irq;

So here you let the drivers override the .to_irq() function and that
I think gets confusing as we are asking gpiolib to handle our
irqchip.


> -       gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> -                                                    gpiochip->irq.first,
> -                                                    ops, gpiochip);
> +       if (gpiochip->irq.parent_domain)
> +               gpiochip->irq.domain = irq_domain_add_hierarchy(gpiochip->irq.parent_domain,
> +                                                               0, gpiochip->ngpio,
> +                                                               np, ops, gpiochip);
> +       else
> +               gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> +                                                            gpiochip->irq.first,
> +                                                            ops, gpiochip);

So this part is great: if we pass in a parent domain the core helps us
create the hierarchy.

But the stuff in .to_irq() should also be handled in the gpiolib core:
the irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &spec) for
example. That way you should not need any external .to_irq() function.

I can't see if you need to pull more stuff into the core to accomplish
that, but I think in essence the core gpiolib needs to be more helpful
with hierarchies.

Yours,
Linus Walleij

Re: [PATCH 6/9] gpio: Add support for hierarchical IRQ domains

[Thierry Reding]

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

On Tue, Sep 25, 2018 at 10:11:06AM +0200, Linus Walleij wrote:
> Hi Thierry!
> 
> Thanks for the patch!
> 
> I am a bit ignorant about irqdomains so I will probably need an ACK
> from some irq maintainer before I can apply this.
> 
> On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
> <thierry.reding@gmail.com> wrote:
> 
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Hierarchical IRQ domains can be used to stack different IRQ controllers
> > on top of each other. One specific use-case where this can be useful is
> > if a power management controller has top-level controls for wakeup
> > interrupts. In such cases, the power management controller can be a
> > parent to other interrupt controllers and program additional registers
> > when an IRQ has its wake capability enabled or disabled.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> While I think it is really important that we start supporting hierarchical
> irqdomains in the gpiolib core, I want a more complete approach,
> so that drivers that need hierarchical handling of irqdomains
> can get the same support from gpiolib as they get for simple
> domains.
> 
> > @@ -1918,7 +1918,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
> >                 type = IRQ_TYPE_NONE;
> >         }
> >
> > -       gpiochip->to_irq = gpiochip_to_irq;
> > +       if (!gpiochip->to_irq)
> > +               gpiochip->to_irq = gpiochip_to_irq;
> 
> So here you let the drivers override the .to_irq() function and that
> I think gets confusing as we are asking gpiolib to handle our
> irqchip.
> 
> 
> > -       gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> > -                                                    gpiochip->irq.first,
> > -                                                    ops, gpiochip);
> > +       if (gpiochip->irq.parent_domain)
> > +               gpiochip->irq.domain = irq_domain_add_hierarchy(gpiochip->irq.parent_domain,
> > +                                                               0, gpiochip->ngpio,
> > +                                                               np, ops, gpiochip);
> > +       else
> > +               gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> > +                                                            gpiochip->irq.first,
> > +                                                            ops, gpiochip);
> 
> So this part is great: if we pass in a parent domain the core helps us
> create the hierarchy.
> 
> But the stuff in .to_irq() should also be handled in the gpiolib core:
> the irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &spec) for
> example. That way you should not need any external .to_irq() function.
> 
> I can't see if you need to pull more stuff into the core to accomplish
> that, but I think in essence the core gpiolib needs to be more helpful
> with hierarchies.

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

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

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

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

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

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

Thierry

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

Re: [PATCH 6/9] gpio: Add support for hierarchical IRQ domains

[Thierry Reding]

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

On Tue, Sep 25, 2018 at 10:11:06AM +0200, Linus Walleij wrote:
> Hi Thierry!
> 
> Thanks for the patch!
> 
> I am a bit ignorant about irqdomains so I will probably need an ACK
> from some irq maintainer before I can apply this.
> 
> On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
> <thierry.reding@gmail.com> wrote:
> 
> > From: Thierry Reding <treding@nvidia.com>
> >
> > Hierarchical IRQ domains can be used to stack different IRQ controllers
> > on top of each other. One specific use-case where this can be useful is
> > if a power management controller has top-level controls for wakeup
> > interrupts. In such cases, the power management controller can be a
> > parent to other interrupt controllers and program additional registers
> > when an IRQ has its wake capability enabled or disabled.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> While I think it is really important that we start supporting hierarchical
> irqdomains in the gpiolib core, I want a more complete approach,
> so that drivers that need hierarchical handling of irqdomains
> can get the same support from gpiolib as they get for simple
> domains.
> 
> > @@ -1918,7 +1918,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
> >                 type = IRQ_TYPE_NONE;
> >         }
> >
> > -       gpiochip->to_irq = gpiochip_to_irq;
> > +       if (!gpiochip->to_irq)
> > +               gpiochip->to_irq = gpiochip_to_irq;
> 
> So here you let the drivers override the .to_irq() function and that
> I think gets confusing as we are asking gpiolib to handle our
> irqchip.
> 
> 
> > -       gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> > -                                                    gpiochip->irq.first,
> > -                                                    ops, gpiochip);
> > +       if (gpiochip->irq.parent_domain)
> > +               gpiochip->irq.domain = irq_domain_add_hierarchy(gpiochip->irq.parent_domain,
> > +                                                               0, gpiochip->ngpio,
> > +                                                               np, ops, gpiochip);
> > +       else
> > +               gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> > +                                                            gpiochip->irq.first,
> > +                                                            ops, gpiochip);
> 
> So this part is great: if we pass in a parent domain the core helps us
> create the hierarchy.
> 
> But the stuff in .to_irq() should also be handled in the gpiolib core:
> the irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, &spec) for
> example. That way you should not need any external .to_irq() function.
> 
> I can't see if you need to pull more stuff into the core to accomplish
> that, but I think in essence the core gpiolib needs to be more helpful
> with hierarchies.

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

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

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

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

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

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

Thierry

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

Re: [PATCH 6/9] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

On Tue, Sep 25, 2018 at 11:33 AM Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, Sep 25, 2018 at 10:11:06AM +0200, Linus Walleij wrote:

> > While I think it is really important that we start supporting hierarchical
> > irqdomains in the gpiolib core, I want a more complete approach,
> > so that drivers that need hierarchical handling of irqdomains
> > can get the same support from gpiolib as they get for simple
> > domains.
(...)
> > I can't see if you need to pull more stuff into the core to accomplish
> > that, but I think in essence the core gpiolib needs to be more helpful
> > with hierarchies.
>
> This is not as trivial as it sounds. I think we could probably provide a
> simple helper in the core that may work for the majority of GPIO
> controllers, and would be similar to irq_domain_xlate_twocell(). The
> problem is that ->gpio_to_irq() effectively needs to convert the offset
> of the pin in the GPIO controller to an IRQ specifier. If the IRQ domain
> can use irq_domain_xlate_twocell(), that should be easy, but if it does
> not, then we likely need a custom implementation as well.

This sounds a lot like the "gpio-ranges" we have in the
gpiochip DT bindings, mapping gpio offsets to pin offsets.

I assume that we could just introduce a cross-mapping
array from IRQ to IRQ in struct gpio_irq_chip for the
hierarchical irqchip? Is it any
more complicated than an array of [(int, int)] tuples?

I guess this is what you have in mind for twocell?

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

I guess this stuff is what you refer to?

#define TEGRA_MAIN_GPIO_PORT(port, base, count, controller)     \
        [TEGRA_MAIN_GPIO_PORT_##port] = {                       \
                .name = #port,                                  \
                .offset = base,                                 \
                .pins = count,                                  \
                .irq = controller,                              \
        }

static const struct tegra_gpio_port tegra186_main_ports[] = {
        TEGRA_MAIN_GPIO_PORT( A, 0x2000, 7, 2),
        TEGRA_MAIN_GPIO_PORT( B, 0x3000, 7, 3),
        TEGRA_MAIN_GPIO_PORT( C, 0x3200, 7, 3),
        TEGRA_MAIN_GPIO_PORT( D, 0x3400, 6, 3),
(...)

Maybe things have changed slightly.

As I see it there are some ways to go about this:

1. Create one gpiochip per bank and just register the number of
GPIOs actually accessible per bank offset from 0. This works
if one does not insist on having one gpiochip covering all pins,
and as long as all usable pins are stacked from offset 0..n.
(Tegra186 doesn't do this, it is registering one chip for all.)

2. If the above cannot be met, register enough pins to cover all
(e.g. 32 pins for a 32bit GPIO register) then mask off the
unused pins in .valid_mask in the gpio_chip. This was fairly
recently introduced to add ACPI support for Qualcomm, as
there were valid, but unusable GPIO offsets, but it can be
used to cut arbitrary holes in any range of offsets.

3. Some driver-specific way. Which seems to be what Tegra186
is doing.

Would you consider to move over to using method (2) to
get a more linear numberspace? I.e. register 8 GPIOs/IRQs
per port/bank and then just mask off the invalid ones?
.valid_mask in gpio_chip can be used for the GPIOs and
.valid_mask in the gpio_irq_chip can be used for IRQs.

Or do you think it is nonelegant?

> Since there is no 1:1 relationship between the value in the specifier
> and the GPIO offset, we can't use irq_domain_xlate_twocell().

Am I right that if you switch to method (2) above this is solved
and we get rid of the custom tegra186 xlate function == big win?

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

And if what I assume (in my naive thinking) you can do with
.valid_mask is correct, then you can convert tegra186 to use
common twocell translation.

Sorry for being a pest, I just have a feeling we are reinventing
wheels here, I really want to pull as many fringe cases as
possible into gpiolib if I can so the maintenance gets
simpler.

Yours,
Linus Walleij

Re: [PATCH 6/9] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

On Tue, Sep 25, 2018 at 11:33 AM Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, Sep 25, 2018 at 10:11:06AM +0200, Linus Walleij wrote:

> > While I think it is really important that we start supporting hierarchical
> > irqdomains in the gpiolib core, I want a more complete approach,
> > so that drivers that need hierarchical handling of irqdomains
> > can get the same support from gpiolib as they get for simple
> > domains.
(...)
> > I can't see if you need to pull more stuff into the core to accomplish
> > that, but I think in essence the core gpiolib needs to be more helpful
> > with hierarchies.
>
> This is not as trivial as it sounds. I think we could probably provide a
> simple helper in the core that may work for the majority of GPIO
> controllers, and would be similar to irq_domain_xlate_twocell(). The
> problem is that ->gpio_to_irq() effectively needs to convert the offset
> of the pin in the GPIO controller to an IRQ specifier. If the IRQ domain
> can use irq_domain_xlate_twocell(), that should be easy, but if it does
> not, then we likely need a custom implementation as well.

This sounds a lot like the "gpio-ranges" we have in the
gpiochip DT bindings, mapping gpio offsets to pin offsets.

I assume that we could just introduce a cross-mapping
array from IRQ to IRQ in struct gpio_irq_chip for the
hierarchical irqchip? Is it any
more complicated than an array of [(int, int)] tuples?

I guess this is what you have in mind for twocell?

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

I guess this stuff is what you refer to?

#define TEGRA_MAIN_GPIO_PORT(port, base, count, controller)     \
        [TEGRA_MAIN_GPIO_PORT_##port] = {                       \
                .name = #port,                                  \
                .offset = base,                                 \
                .pins = count,                                  \
                .irq = controller,                              \
        }

static const struct tegra_gpio_port tegra186_main_ports[] = {
        TEGRA_MAIN_GPIO_PORT( A, 0x2000, 7, 2),
        TEGRA_MAIN_GPIO_PORT( B, 0x3000, 7, 3),
        TEGRA_MAIN_GPIO_PORT( C, 0x3200, 7, 3),
        TEGRA_MAIN_GPIO_PORT( D, 0x3400, 6, 3),
(...)

Maybe things have changed slightly.

As I see it there are some ways to go about this:

1. Create one gpiochip per bank and just register the number of
GPIOs actually accessible per bank offset from 0. This works
if one does not insist on having one gpiochip covering all pins,
and as long as all usable pins are stacked from offset 0..n.
(Tegra186 doesn't do this, it is registering one chip for all.)

2. If the above cannot be met, register enough pins to cover all
(e.g. 32 pins for a 32bit GPIO register) then mask off the
unused pins in .valid_mask in the gpio_chip. This was fairly
recently introduced to add ACPI support for Qualcomm, as
there were valid, but unusable GPIO offsets, but it can be
used to cut arbitrary holes in any range of offsets.

3. Some driver-specific way. Which seems to be what Tegra186
is doing.

Would you consider to move over to using method (2) to
get a more linear numberspace? I.e. register 8 GPIOs/IRQs
per port/bank and then just mask off the invalid ones?
.valid_mask in gpio_chip can be used for the GPIOs and
.valid_mask in the gpio_irq_chip can be used for IRQs.

Or do you think it is nonelegant?

> Since there is no 1:1 relationship between the value in the specifier
> and the GPIO offset, we can't use irq_domain_xlate_twocell().

Am I right that if you switch to method (2) above this is solved
and we get rid of the custom tegra186 xlate function == big win?

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

And if what I assume (in my naive thinking) you can do with
.valid_mask is correct, then you can convert tegra186 to use
common twocell translation.

Sorry for being a pest, I just have a feeling we are reinventing
wheels here, I really want to pull as many fringe cases as
possible into gpiolib if I can so the maintenance gets
simpler.

Yours,
Linus Walleij

Re: [PATCH 6/9] gpio: Add support for hierarchical IRQ domains

[Thierry Reding]

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

On Tue, Sep 25, 2018 at 12:33:54PM +0200, Linus Walleij wrote:
> On Tue, Sep 25, 2018 at 11:33 AM Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Tue, Sep 25, 2018 at 10:11:06AM +0200, Linus Walleij wrote:
> 
> > > While I think it is really important that we start supporting hierarchical
> > > irqdomains in the gpiolib core, I want a more complete approach,
> > > so that drivers that need hierarchical handling of irqdomains
> > > can get the same support from gpiolib as they get for simple
> > > domains.
> (...)
> > > I can't see if you need to pull more stuff into the core to accomplish
> > > that, but I think in essence the core gpiolib needs to be more helpful
> > > with hierarchies.
> >
> > This is not as trivial as it sounds. I think we could probably provide a
> > simple helper in the core that may work for the majority of GPIO
> > controllers, and would be similar to irq_domain_xlate_twocell(). The
> > problem is that ->gpio_to_irq() effectively needs to convert the offset
> > of the pin in the GPIO controller to an IRQ specifier. If the IRQ domain
> > can use irq_domain_xlate_twocell(), that should be easy, but if it does
> > not, then we likely need a custom implementation as well.
> 
> This sounds a lot like the "gpio-ranges" we have in the
> gpiochip DT bindings, mapping gpio offsets to pin offsets.
> 
> I assume that we could just introduce a cross-mapping
> array from IRQ to IRQ in struct gpio_irq_chip for the
> hierarchical irqchip? Is it any
> more complicated than an array of [(int, int)] tuples?
> 
> I guess this is what you have in mind for twocell?

For twocell I think it would be even easier, because the IRQ specifier
can just be reconstructed from (offset, irq_type). That's all the simple
two-cell does, right? It's a 1:1 mapping of specifier to GPIO offset to
IRQ offset.

> > For example, as you may remember, the Tegra186 GPIO controller is
> > somewhat quirky in that it has a number of banks, each of which can have
> > any number of pins up to 8. However, in order to prevent users from
> > attempting to use one of the non-existent GPIOs, we resorted to
> > compacting the GPIO number space so that the GPIO specifier uses
> > basically a (bank, pin) pair that is converted into a GPIO offset. The
> > same is done for interrupt specifiers.
> 
> I guess this stuff is what you refer to?
> 
> #define TEGRA_MAIN_GPIO_PORT(port, base, count, controller)     \
>         [TEGRA_MAIN_GPIO_PORT_##port] = {                       \
>                 .name = #port,                                  \
>                 .offset = base,                                 \
>                 .pins = count,                                  \
>                 .irq = controller,                              \
>         }
> 
> static const struct tegra_gpio_port tegra186_main_ports[] = {
>         TEGRA_MAIN_GPIO_PORT( A, 0x2000, 7, 2),
>         TEGRA_MAIN_GPIO_PORT( B, 0x3000, 7, 3),
>         TEGRA_MAIN_GPIO_PORT( C, 0x3200, 7, 3),
>         TEGRA_MAIN_GPIO_PORT( D, 0x3400, 6, 3),
> (...)
> 
> Maybe things have changed slightly.
> 
> As I see it there are some ways to go about this:
> 
> 1. Create one gpiochip per bank and just register the number of
> GPIOs actually accessible per bank offset from 0. This works
> if one does not insist on having one gpiochip covering all pins,
> and as long as all usable pins are stacked from offset 0..n.
> (Tegra186 doesn't do this, it is registering one chip for all.)
> 
> 2. If the above cannot be met, register enough pins to cover all
> (e.g. 32 pins for a 32bit GPIO register) then mask off the
> unused pins in .valid_mask in the gpio_chip. This was fairly
> recently introduced to add ACPI support for Qualcomm, as
> there were valid, but unusable GPIO offsets, but it can be
> used to cut arbitrary holes in any range of offsets.
> 
> 3. Some driver-specific way. Which seems to be what Tegra186
> is doing.
> 
> Would you consider to move over to using method (2) to
> get a more linear numberspace? I.e. register 8 GPIOs/IRQs
> per port/bank and then just mask off the invalid ones?
> .valid_mask in gpio_chip can be used for the GPIOs and
> .valid_mask in the gpio_irq_chip can be used for IRQs.
> 
> Or do you think it is nonelegant?

This is all pretty much the same discussion that I remember we had
earlier this year. Nothing's changed so far.

Back at the time I had pointed out that we'd be wasting a lot of memory
by registering 8 GPIOs/IRQs per bank, because on average only about 60-
75% of the GPIOs are actually used. In addition we waste processing
resources by having to check the GPIO offset against the valid mask
every time we want to access a GPIO.

I think that's inelegant, but from the rest of what you're saying you
don't see it that way.

> > Since there is no 1:1 relationship between the value in the specifier
> > and the GPIO offset, we can't use irq_domain_xlate_twocell().
> 
> Am I right that if you switch to method (2) above this is solved
> and we get rid of the custom tegra186 xlate function == big win?
> 
> > I think we can probably just implement the simple two-cell version in
> > gpiochip_to_irq() directly and leave it up to drivers that require
> > something more to override ->to_irq().
> 
> And if what I assume (in my naive thinking) you can do with
> .valid_mask is correct, then you can convert tegra186 to use
> common twocell translation.
> 
> Sorry for being a pest, I just have a feeling we are reinventing
> wheels here, I really want to pull as many fringe cases as
> possible into gpiolib if I can so the maintenance gets
> simpler.

Like I said, we had this very discussion a couple of months ago, and I
don't think I want to go through all of it again. I think, yes, I could
make Tegra186 work with .valid_mask, even if I consider it wasteful. So
if that's what you want, I'll go rewrite the driver so we'll never have
to repeat this again.

Thierry

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

Re: [PATCH 6/9] gpio: Add support for hierarchical IRQ domains

[Thierry Reding]

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

On Tue, Sep 25, 2018 at 12:33:54PM +0200, Linus Walleij wrote:
> On Tue, Sep 25, 2018 at 11:33 AM Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Tue, Sep 25, 2018 at 10:11:06AM +0200, Linus Walleij wrote:
> 
> > > While I think it is really important that we start supporting hierarchical
> > > irqdomains in the gpiolib core, I want a more complete approach,
> > > so that drivers that need hierarchical handling of irqdomains
> > > can get the same support from gpiolib as they get for simple
> > > domains.
> (...)
> > > I can't see if you need to pull more stuff into the core to accomplish
> > > that, but I think in essence the core gpiolib needs to be more helpful
> > > with hierarchies.
> >
> > This is not as trivial as it sounds. I think we could probably provide a
> > simple helper in the core that may work for the majority of GPIO
> > controllers, and would be similar to irq_domain_xlate_twocell(). The
> > problem is that ->gpio_to_irq() effectively needs to convert the offset
> > of the pin in the GPIO controller to an IRQ specifier. If the IRQ domain
> > can use irq_domain_xlate_twocell(), that should be easy, but if it does
> > not, then we likely need a custom implementation as well.
> 
> This sounds a lot like the "gpio-ranges" we have in the
> gpiochip DT bindings, mapping gpio offsets to pin offsets.
> 
> I assume that we could just introduce a cross-mapping
> array from IRQ to IRQ in struct gpio_irq_chip for the
> hierarchical irqchip? Is it any
> more complicated than an array of [(int, int)] tuples?
> 
> I guess this is what you have in mind for twocell?

For twocell I think it would be even easier, because the IRQ specifier
can just be reconstructed from (offset, irq_type). That's all the simple
two-cell does, right? It's a 1:1 mapping of specifier to GPIO offset to
IRQ offset.

> > For example, as you may remember, the Tegra186 GPIO controller is
> > somewhat quirky in that it has a number of banks, each of which can have
> > any number of pins up to 8. However, in order to prevent users from
> > attempting to use one of the non-existent GPIOs, we resorted to
> > compacting the GPIO number space so that the GPIO specifier uses
> > basically a (bank, pin) pair that is converted into a GPIO offset. The
> > same is done for interrupt specifiers.
> 
> I guess this stuff is what you refer to?
> 
> #define TEGRA_MAIN_GPIO_PORT(port, base, count, controller)     \
>         [TEGRA_MAIN_GPIO_PORT_##port] = {                       \
>                 .name = #port,                                  \
>                 .offset = base,                                 \
>                 .pins = count,                                  \
>                 .irq = controller,                              \
>         }
> 
> static const struct tegra_gpio_port tegra186_main_ports[] = {
>         TEGRA_MAIN_GPIO_PORT( A, 0x2000, 7, 2),
>         TEGRA_MAIN_GPIO_PORT( B, 0x3000, 7, 3),
>         TEGRA_MAIN_GPIO_PORT( C, 0x3200, 7, 3),
>         TEGRA_MAIN_GPIO_PORT( D, 0x3400, 6, 3),
> (...)
> 
> Maybe things have changed slightly.
> 
> As I see it there are some ways to go about this:
> 
> 1. Create one gpiochip per bank and just register the number of
> GPIOs actually accessible per bank offset from 0. This works
> if one does not insist on having one gpiochip covering all pins,
> and as long as all usable pins are stacked from offset 0..n.
> (Tegra186 doesn't do this, it is registering one chip for all.)
> 
> 2. If the above cannot be met, register enough pins to cover all
> (e.g. 32 pins for a 32bit GPIO register) then mask off the
> unused pins in .valid_mask in the gpio_chip. This was fairly
> recently introduced to add ACPI support for Qualcomm, as
> there were valid, but unusable GPIO offsets, but it can be
> used to cut arbitrary holes in any range of offsets.
> 
> 3. Some driver-specific way. Which seems to be what Tegra186
> is doing.
> 
> Would you consider to move over to using method (2) to
> get a more linear numberspace? I.e. register 8 GPIOs/IRQs
> per port/bank and then just mask off the invalid ones?
> .valid_mask in gpio_chip can be used for the GPIOs and
> .valid_mask in the gpio_irq_chip can be used for IRQs.
> 
> Or do you think it is nonelegant?

This is all pretty much the same discussion that I remember we had
earlier this year. Nothing's changed so far.

Back at the time I had pointed out that we'd be wasting a lot of memory
by registering 8 GPIOs/IRQs per bank, because on average only about 60-
75% of the GPIOs are actually used. In addition we waste processing
resources by having to check the GPIO offset against the valid mask
every time we want to access a GPIO.

I think that's inelegant, but from the rest of what you're saying you
don't see it that way.

> > Since there is no 1:1 relationship between the value in the specifier
> > and the GPIO offset, we can't use irq_domain_xlate_twocell().
> 
> Am I right that if you switch to method (2) above this is solved
> and we get rid of the custom tegra186 xlate function == big win?
> 
> > I think we can probably just implement the simple two-cell version in
> > gpiochip_to_irq() directly and leave it up to drivers that require
> > something more to override ->to_irq().
> 
> And if what I assume (in my naive thinking) you can do with
> .valid_mask is correct, then you can convert tegra186 to use
> common twocell translation.
> 
> Sorry for being a pest, I just have a feeling we are reinventing
> wheels here, I really want to pull as many fringe cases as
> possible into gpiolib if I can so the maintenance gets
> simpler.

Like I said, we had this very discussion a couple of months ago, and I
don't think I want to go through all of it again. I think, yes, I could
make Tegra186 work with .valid_mask, even if I consider it wasteful. So
if that's what you want, I'll go rewrite the driver so we'll never have
to repeat this again.

Thierry

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

Re: [PATCH 6/9] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

On Tue, Sep 25, 2018 at 1:17 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Sep 25, 2018 at 12:33:54PM +0200, Linus Walleij wrote:
> > On Tue, Sep 25, 2018 at 11:33 AM Thierry Reding
> > <thierry.reding@gmail.com> wrote:
> > > On Tue, Sep 25, 2018 at 10:11:06AM +0200, Linus Walleij wrote:

> > As I see it there are some ways to go about this:
> >
> > 1. Create one gpiochip per bank and just register the number of
> > GPIOs actually accessible per bank offset from 0. This works
> > if one does not insist on having one gpiochip covering all pins,
> > and as long as all usable pins are stacked from offset 0..n.
> > (Tegra186 doesn't do this, it is registering one chip for all.)
> >
> > 2. If the above cannot be met, register enough pins to cover all
> > (e.g. 32 pins for a 32bit GPIO register) then mask off the
> > unused pins in .valid_mask in the gpio_chip. This was fairly
> > recently introduced to add ACPI support for Qualcomm, as
> > there were valid, but unusable GPIO offsets, but it can be
> > used to cut arbitrary holes in any range of offsets.
> >
> > 3. Some driver-specific way. Which seems to be what Tegra186
> > is doing.
> >
> > Would you consider to move over to using method (2) to
> > get a more linear numberspace? I.e. register 8 GPIOs/IRQs
> > per port/bank and then just mask off the invalid ones?
> > .valid_mask in gpio_chip can be used for the GPIOs and
> > .valid_mask in the gpio_irq_chip can be used for IRQs.
> >
> > Or do you think it is nonelegant?
>
> This is all pretty much the same discussion that I remember we had
> earlier this year. Nothing's changed so far.
>
> Back at the time I had pointed out that we'd be wasting a lot of memory
> by registering 8 GPIOs/IRQs per bank, because on average only about 60-
> 75% of the GPIOs are actually used. In addition we waste processing
> resources by having to check the GPIO offset against the valid mask
> every time we want to access a GPIO.
>
> I think that's inelegant, but from the rest of what you're saying you
> don't see it that way.

(...)

> > Sorry for being a pest, I just have a feeling we are reinventing
> > wheels here, I really want to pull as many fringe cases as
> > possible into gpiolib if I can so the maintenance gets
> > simpler.
>
> Like I said, we had this very discussion a couple of months ago, and I
> don't think I want to go through all of it again. I think, yes, I could
> make Tegra186 work with .valid_mask, even if I consider it wasteful. So
> if that's what you want, I'll go rewrite the driver so we'll never have
> to repeat this again.

Well, IMO rolling custom code into every driver that needs a
hierarchical interrupt is pretty inelegant too, so I guess it is one
of those choose the lesser evil situations for me as a maintainer.

I am very happy if this hairy hierarchical irqdomain handling can
be standardized and pushed into the core, and as it seems this
memory optimization stops that and forces a local solution with
some necessarilily different code, also for xlate since a while
back and now for the hierarchical irqdomain.

So if I let this pass then I will just be grumpy again the next
time another local kludge needs to be added.

I am worried that there will be more and more of this special
code just to account for the nonlinear blocks of GPIOs.

I agree memory footprint matters, as does performance,
as does maintainability and reuse. So it's not like there is
a hard factor determining this.

How much memory are we talking about for a tegra186, and
how much is that in proportion to how much memory a
typical tegra186 system has connected to it? If it is significant
and hurting the kernel and/or applications on that platform
I would agree we need to look into memory optimizations.

Yours,
Linus Walleij

Re: [PATCH 6/9] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

On Tue, Sep 25, 2018 at 1:17 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> On Tue, Sep 25, 2018 at 12:33:54PM +0200, Linus Walleij wrote:
> > On Tue, Sep 25, 2018 at 11:33 AM Thierry Reding
> > <thierry.reding@gmail.com> wrote:
> > > On Tue, Sep 25, 2018 at 10:11:06AM +0200, Linus Walleij wrote:

> > As I see it there are some ways to go about this:
> >
> > 1. Create one gpiochip per bank and just register the number of
> > GPIOs actually accessible per bank offset from 0. This works
> > if one does not insist on having one gpiochip covering all pins,
> > and as long as all usable pins are stacked from offset 0..n.
> > (Tegra186 doesn't do this, it is registering one chip for all.)
> >
> > 2. If the above cannot be met, register enough pins to cover all
> > (e.g. 32 pins for a 32bit GPIO register) then mask off the
> > unused pins in .valid_mask in the gpio_chip. This was fairly
> > recently introduced to add ACPI support for Qualcomm, as
> > there were valid, but unusable GPIO offsets, but it can be
> > used to cut arbitrary holes in any range of offsets.
> >
> > 3. Some driver-specific way. Which seems to be what Tegra186
> > is doing.
> >
> > Would you consider to move over to using method (2) to
> > get a more linear numberspace? I.e. register 8 GPIOs/IRQs
> > per port/bank and then just mask off the invalid ones?
> > .valid_mask in gpio_chip can be used for the GPIOs and
> > .valid_mask in the gpio_irq_chip can be used for IRQs.
> >
> > Or do you think it is nonelegant?
>
> This is all pretty much the same discussion that I remember we had
> earlier this year. Nothing's changed so far.
>
> Back at the time I had pointed out that we'd be wasting a lot of memory
> by registering 8 GPIOs/IRQs per bank, because on average only about 60-
> 75% of the GPIOs are actually used. In addition we waste processing
> resources by having to check the GPIO offset against the valid mask
> every time we want to access a GPIO.
>
> I think that's inelegant, but from the rest of what you're saying you
> don't see it that way.

(...)

> > Sorry for being a pest, I just have a feeling we are reinventing
> > wheels here, I really want to pull as many fringe cases as
> > possible into gpiolib if I can so the maintenance gets
> > simpler.
>
> Like I said, we had this very discussion a couple of months ago, and I
> don't think I want to go through all of it again. I think, yes, I could
> make Tegra186 work with .valid_mask, even if I consider it wasteful. So
> if that's what you want, I'll go rewrite the driver so we'll never have
> to repeat this again.

Well, IMO rolling custom code into every driver that needs a
hierarchical interrupt is pretty inelegant too, so I guess it is one
of those choose the lesser evil situations for me as a maintainer.

I am very happy if this hairy hierarchical irqdomain handling can
be standardized and pushed into the core, and as it seems this
memory optimization stops that and forces a local solution with
some necessarilily different code, also for xlate since a while
back and now for the hierarchical irqdomain.

So if I let this pass then I will just be grumpy again the next
time another local kludge needs to be added.

I am worried that there will be more and more of this special
code just to account for the nonlinear blocks of GPIOs.

I agree memory footprint matters, as does performance,
as does maintainability and reuse. So it's not like there is
a hard factor determining this.

How much memory are we talking about for a tegra186, and
how much is that in proportion to how much memory a
typical tegra186 system has connected to it? If it is significant
and hurting the kernel and/or applications on that platform
I would agree we need to look into memory optimizations.

Yours,
Linus Walleij

[PATCH 7/9] dt-bindings: tegra186-gpio: Add wakeup parent support

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Tegra186 and later have some top-level controls for wake events in the
power management controller (PMC). In order to enable the system to wake
up from low power states, additional registers in the PMC need to be
programmed. Add a wakeup-parent property to establish this relationship
between the GPIO controller and the PMC.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../devicetree/bindings/gpio/nvidia,tegra186-gpio.txt      | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt b/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
index adff16c71d21..cbb51a8990c3 100644
--- a/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
@@ -127,6 +127,11 @@ Required properties:
             - 8: Active low level-sensitive.
             Valid combinations are 1, 2, 3, 4, 8.
 
+Optional properties:
+- wake-parent
+    A phandle to the Power Management Controller (PMC) that contains top-
+    level controls to enable the wake-up capabilities of some GPIOs.
+
 Example:
 
 #include <dt-bindings/interrupt-controller/irq.h>
@@ -148,6 +153,7 @@ gpio@2200000 {
 	#gpio-cells = <2>;
 	interrupt-controller;
 	#interrupt-cells = <2>;
+	wakeup-parent = <&pmc>;
 };
 
 gpio@c2f0000 {
@@ -162,4 +168,5 @@ gpio@c2f0000 {
 	#gpio-cells = <2>;
 	interrupt-controller;
 	#interrupt-cells = <2>;
+	wakeup-parent = <&pmc>;
 };
-- 
2.19.0

Re: [PATCH 7/9] dt-bindings: tegra186-gpio: Add wakeup parent support

[Mikko Perttunen]

On 21/09/2018 19.25, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Tegra186 and later have some top-level controls for wake events in the
> power management controller (PMC). In order to enable the system to wake
> up from low power states, additional registers in the PMC need to be
> programmed. Add a wakeup-parent property to establish this relationship
> between the GPIO controller and the PMC.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>   .../devicetree/bindings/gpio/nvidia,tegra186-gpio.txt      | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt b/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> index adff16c71d21..cbb51a8990c3 100644
> --- a/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> @@ -127,6 +127,11 @@ Required properties:
>               - 8: Active low level-sensitive.
>               Valid combinations are 1, 2, 3, 4, 8.
>   
> +Optional properties:
> +- wake-parent

wakeup-parent

Cheers,
Mikko

> +    A phandle to the Power Management Controller (PMC) that contains top-
> +    level controls to enable the wake-up capabilities of some GPIOs.
> +
>   Example:
>   
>   #include <dt-bindings/interrupt-controller/irq.h>
> @@ -148,6 +153,7 @@ gpio@2200000 {
>   	#gpio-cells = <2>;
>   	interrupt-controller;
>   	#interrupt-cells = <2>;
> +	wakeup-parent = <&pmc>;
>   };
>   
>   gpio@c2f0000 {
> @@ -162,4 +168,5 @@ gpio@c2f0000 {
>   	#gpio-cells = <2>;
>   	interrupt-controller;
>   	#interrupt-cells = <2>;
> +	wakeup-parent = <&pmc>;
>   };
> 

Re: [PATCH 7/9] dt-bindings: tegra186-gpio: Add wakeup parent support

[Rob Herring]

On Fri, Sep 21, 2018 at 12:25:44PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Tegra186 and later have some top-level controls for wake events in the
> power management controller (PMC). In order to enable the system to wake
> up from low power states, additional registers in the PMC need to be
> programmed. Add a wakeup-parent property to establish this relationship
> between the GPIO controller and the PMC.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../devicetree/bindings/gpio/nvidia,tegra186-gpio.txt      | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt b/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> index adff16c71d21..cbb51a8990c3 100644
> --- a/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> @@ -127,6 +127,11 @@ Required properties:
>              - 8: Active low level-sensitive.
>              Valid combinations are 1, 2, 3, 4, 8.
>  
> +Optional properties:
> +- wake-parent
> +    A phandle to the Power Management Controller (PMC) that contains top-
> +    level controls to enable the wake-up capabilities of some GPIOs.
> +
>  Example:
>  
>  #include <dt-bindings/interrupt-controller/irq.h>
> @@ -148,6 +153,7 @@ gpio@2200000 {
>  	#gpio-cells = <2>;
>  	interrupt-controller;
>  	#interrupt-cells = <2>;
> +	wakeup-parent = <&pmc>;
>  };
>  
>  gpio@c2f0000 {
> @@ -162,4 +168,5 @@ gpio@c2f0000 {
>  	#gpio-cells = <2>;
>  	interrupt-controller;
>  	#interrupt-cells = <2>;
> +	wakeup-parent = <&pmc>;

If all the GPIO instances point to the same PMC and have no per instance 
data, why do you need this in DT? You can just search for the compatible 
node.

Rob

Re: [PATCH 7/9] dt-bindings: tegra186-gpio: Add wakeup parent support

[Thierry Reding]

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

On Mon, Oct 15, 2018 at 09:46:12AM -0500, Rob Herring wrote:
> On Fri, Sep 21, 2018 at 12:25:44PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Tegra186 and later have some top-level controls for wake events in the
> > power management controller (PMC). In order to enable the system to wake
> > up from low power states, additional registers in the PMC need to be
> > programmed. Add a wakeup-parent property to establish this relationship
> > between the GPIO controller and the PMC.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  .../devicetree/bindings/gpio/nvidia,tegra186-gpio.txt      | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt b/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> > index adff16c71d21..cbb51a8990c3 100644
> > --- a/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> > +++ b/Documentation/devicetree/bindings/gpio/nvidia,tegra186-gpio.txt
> > @@ -127,6 +127,11 @@ Required properties:
> >              - 8: Active low level-sensitive.
> >              Valid combinations are 1, 2, 3, 4, 8.
> >  
> > +Optional properties:
> > +- wake-parent
> > +    A phandle to the Power Management Controller (PMC) that contains top-
> > +    level controls to enable the wake-up capabilities of some GPIOs.
> > +
> >  Example:
> >  
> >  #include <dt-bindings/interrupt-controller/irq.h>
> > @@ -148,6 +153,7 @@ gpio@2200000 {
> >  	#gpio-cells = <2>;
> >  	interrupt-controller;
> >  	#interrupt-cells = <2>;
> > +	wakeup-parent = <&pmc>;
> >  };
> >  
> >  gpio@c2f0000 {
> > @@ -162,4 +168,5 @@ gpio@c2f0000 {
> >  	#gpio-cells = <2>;
> >  	interrupt-controller;
> >  	#interrupt-cells = <2>;
> > +	wakeup-parent = <&pmc>;
> 
> If all the GPIO instances point to the same PMC and have no per instance 
> data, why do you need this in DT? You can just search for the compatible 
> node.

That would be slightly annoying to do. I mean, we'd have to somehow
construct the compatible string that we're looking for. I guess we could
get around this mostly by just looking for a device matching one of the
entries in a "pmc_gpio_of_match" table. That would potentially match a
Tegra194 GPIO against a Tegra186 PMC, but that should never happen in
practice because it'd be a bug in the DT.

Although that somewhat depends on exactly what we mean by "compatible".
Technically the Tegra194 PMC is compatible with the Tegra186 PMC in
terms of register layout and so on. However, since both chips have
undergone quite some changes with regards to the pins they expose, the
set of wake events exposed on Tegra186 and Tegra194 varies wildly. So if
that means that the compatible needs to be different (I think it should)
then we could go with the OF match table approach.

Actually, thinking about it some more, even if we had a Tegra186
fallback compatible string in the Tegra194 PMC's device tree node, we'd
still be matching on the correct instance, and therefore get the right
IRQ domain for the hierarchy.

I'll rework the patches accordingly.

Thierry

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

[PATCH 8/9] gpio: tegra186: Rename flow variable to type

[Thierry Reding]

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.0

[PATCH 9/9] 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>
---
 drivers/gpio/gpio-tegra186.c | 103 ++++++++++++++++++++++++++++++-----
 1 file changed, 89 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index 66ec38bb7954..240a26defe9b 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,67 @@ 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;
 
-	port = spec[0] / 8;
-	pin = spec[0] % 8;
+	if (WARN_ON(fwspec->param_count < gpio->gpio.of_gpio_n_cells))
+		return -EINVAL;
 
-	if (port >= gpio->soc->num_ports) {
-		dev_err(gpio->gpio.parent, "invalid port number: %u\n", port);
+	port = fwspec->param[0] / 8;
+	pin = fwspec->param[0] % 8;
+
+	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 int tegra186_gpio_probe(struct platform_device *pdev)
@@ -410,6 +471,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 +546,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 +565,15 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
 	irq->num_parents = gpio->num_irq;
 	irq->parents = gpio->irq;
 
+	np = of_parse_phandle(pdev->dev.of_node, "wakeup-parent", 0);
+	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 +710,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 +733,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.0

Re: [PATCH 0/9] Implement wake event support on Tegra186 and later

[Linus Walleij]

Hi Thierry,

thanks for working on the wakeup business!

This patch gets me a bit confused on our different approaches
toward wakeups in the kernel, so I included Lina, Marc and Ulf
to see if we can get some common understanding.

On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
<thierry.reding@gmail.com> wrote:

> 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

I am not sure if you are aware about Lina's series
"Wakeup GPIO support for SDM845 SoC"
that is now in v3:
https://patchwork.kernel.org/cover/10587965/

It appears to me, though I am blissfully ignorant of the
details, that there is a relationship between this patch
series and the other one.

Your approach is to insert an hiearchical PMC irq controller
and Lina's approach is to simply put a mechanism on the
side to inject IRQs into the GIC after sleep (IIUC).

I guess your hierarchy is in response to this mail from tglx:
https://lkml.org/lkml/2018/9/14/339

So from a birds eye point of view I don't see how the Tegra
PMC irq controller and Qualcomm's PDC power domain
controller are conceptually different. Are you doing the same
thing in two different ways for the same problem space
here?

Or are these hardwares so very different that they really
warrant two different approaches to wakeups?

I guess I miss a bit of hardware insight... is the key difference
that in Qualcomm's PDC the IRQs need to be replayed/injected
by software while Tegra's PMC will do this in hardware?

Yours,
Linus Walleij

Re: [PATCH 0/9] Implement wake event support on Tegra186 and later

[Linus Walleij]

Hi Thierry,

thanks for working on the wakeup business!

This patch gets me a bit confused on our different approaches
toward wakeups in the kernel, so I included Lina, Marc and Ulf
to see if we can get some common understanding.

On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
<thierry.reding@gmail.com> wrote:

> 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

I am not sure if you are aware about Lina's series
"Wakeup GPIO support for SDM845 SoC"
that is now in v3:
https://patchwork.kernel.org/cover/10587965/

It appears to me, though I am blissfully ignorant of the
details, that there is a relationship between this patch
series and the other one.

Your approach is to insert an hiearchical PMC irq controller
and Lina's approach is to simply put a mechanism on the
side to inject IRQs into the GIC after sleep (IIUC).

I guess your hierarchy is in response to this mail from tglx:
https://lkml.org/lkml/2018/9/14/339

So from a birds eye point of view I don't see how the Tegra
PMC irq controller and Qualcomm's PDC power domain
controller are conceptually different. Are you doing the same
thing in two different ways for the same problem space
here?

Or are these hardwares so very different that they really
warrant two different approaches to wakeups?

I guess I miss a bit of hardware insight... is the key difference
that in Qualcomm's PDC the IRQs need to be replayed/injected
by software while Tegra's PMC will do this in hardware?

Yours,
Linus Walleij

Re: [PATCH 0/9] Implement wake event support on Tegra186 and later

[Thierry Reding]

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

On Tue, Sep 25, 2018 at 10:48:39AM +0200, Linus Walleij wrote:
> Hi Thierry,
> 
> thanks for working on the wakeup business!
> 
> This patch gets me a bit confused on our different approaches
> toward wakeups in the kernel, so I included Lina, Marc and Ulf
> to see if we can get some common understanding.
> 
> On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
> <thierry.reding@gmail.com> wrote:
> 
> > 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
> 
> I am not sure if you are aware about Lina's series
> "Wakeup GPIO support for SDM845 SoC"
> that is now in v3:
> https://patchwork.kernel.org/cover/10587965/
> 
> It appears to me, though I am blissfully ignorant of the
> details, that there is a relationship between this patch
> series and the other one.
> 
> Your approach is to insert an hiearchical PMC irq controller
> and Lina's approach is to simply put a mechanism on the
> side to inject IRQs into the GIC after sleep (IIUC).

From a quick look at Lina's patches, I think it's more like adding a
demultiplex in the TLMM. So the TLMM effectively has interrupt handlers
for all wakeup interrupts so that when a wakeup interrupt happens, the
GPIO interrupts can be replayed (using the interrupt status bit in the
GPIO controller, if I'm reading things right).

From a very high level view both seem indeed to be very similar and have
the same goal. Both are in a partition that is always powered on and the
goal is to enable wake up from certain interrupts. One difference I see
is that the PMC on Tegra allows wake events to originate from sources
other than GPIOs. For example the RTC or PMIC interrupts (at the GIC)
can be a source for the wake event, as can a number of other special
signals. The PDC on the other hand seems to be limited to GPIOs as wake
events.

Another area, more low-level, where these setups seem to be different is
that the PMC isn't really a proper interrupt controller in itself. It is
more of a top-level interrupt gate. If you enable a given wake event
(that is, unmask the "interrupt"), that event will be able to

> I guess your hierarchy is in response to this mail from tglx:
> https://lkml.org/lkml/2018/9/14/339

Yes, there was some good discussion in that thread which helped me come
up with this solution. I think it's pretty elegant because it allows all
of this interaction to happen almost automatically via the existing
infrastructure. I'm not sure the same could be applied to the PDC,
though, because of the need to manually replay the interrupt. That's not
something I think can be done with just the simple parent/child
relationship that we use on Tegra.

On the other hand, I don't think implementing something akin to Lina's
proposal would work on Tegra because in our case the PMC doesn't
actually raise an interrupt on wake. The hardware will simply wake up
the system, at which point all the signals will be forwarded as normal,
so the GPIO or GIC will see the interrupts as if they happened during
normal runtime.

> So from a birds eye point of view I don't see how the Tegra
> PMC irq controller and Qualcomm's PDC power domain
> controller are conceptually different. Are you doing the same
> thing in two different ways for the same problem space
> here?
> 
> Or are these hardwares so very different that they really
> warrant two different approaches to wakeups?
> 
> I guess I miss a bit of hardware insight... is the key difference
> that in Qualcomm's PDC the IRQs need to be replayed/injected
> by software while Tegra's PMC will do this in hardware?

Yes, I think you're exactly right here. As I said above, I don't think
there's a way to replay interrupts with a pure parent/child hierarchy
because the hierarchy doesn't actually do anything at the interrupt
handler level. You'd need to set up additional demultiplexing at that
point to make it work, which is pretty much the equivalent of what Lina
has proposed.

On the other hand, since we don't get interrupts from the PMC for wake
events themselves, we can't replay interrupts on Tegra. And we don't
have to.

Unfortunately, these seem to be really similar pieces of hardware but
with just enough of a low-level difference to require completely
different solutions.

Thierry

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

Re: [PATCH 0/9] Implement wake event support on Tegra186 and later

[Thierry Reding]

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

On Tue, Sep 25, 2018 at 10:48:39AM +0200, Linus Walleij wrote:
> Hi Thierry,
> 
> thanks for working on the wakeup business!
> 
> This patch gets me a bit confused on our different approaches
> toward wakeups in the kernel, so I included Lina, Marc and Ulf
> to see if we can get some common understanding.
> 
> On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
> <thierry.reding@gmail.com> wrote:
> 
> > 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
> 
> I am not sure if you are aware about Lina's series
> "Wakeup GPIO support for SDM845 SoC"
> that is now in v3:
> https://patchwork.kernel.org/cover/10587965/
> 
> It appears to me, though I am blissfully ignorant of the
> details, that there is a relationship between this patch
> series and the other one.
> 
> Your approach is to insert an hiearchical PMC irq controller
> and Lina's approach is to simply put a mechanism on the
> side to inject IRQs into the GIC after sleep (IIUC).

From a quick look at Lina's patches, I think it's more like adding a
demultiplex in the TLMM. So the TLMM effectively has interrupt handlers
for all wakeup interrupts so that when a wakeup interrupt happens, the
GPIO interrupts can be replayed (using the interrupt status bit in the
GPIO controller, if I'm reading things right).

From a very high level view both seem indeed to be very similar and have
the same goal. Both are in a partition that is always powered on and the
goal is to enable wake up from certain interrupts. One difference I see
is that the PMC on Tegra allows wake events to originate from sources
other than GPIOs. For example the RTC or PMIC interrupts (at the GIC)
can be a source for the wake event, as can a number of other special
signals. The PDC on the other hand seems to be limited to GPIOs as wake
events.

Another area, more low-level, where these setups seem to be different is
that the PMC isn't really a proper interrupt controller in itself. It is
more of a top-level interrupt gate. If you enable a given wake event
(that is, unmask the "interrupt"), that event will be able to

> I guess your hierarchy is in response to this mail from tglx:
> https://lkml.org/lkml/2018/9/14/339

Yes, there was some good discussion in that thread which helped me come
up with this solution. I think it's pretty elegant because it allows all
of this interaction to happen almost automatically via the existing
infrastructure. I'm not sure the same could be applied to the PDC,
though, because of the need to manually replay the interrupt. That's not
something I think can be done with just the simple parent/child
relationship that we use on Tegra.

On the other hand, I don't think implementing something akin to Lina's
proposal would work on Tegra because in our case the PMC doesn't
actually raise an interrupt on wake. The hardware will simply wake up
the system, at which point all the signals will be forwarded as normal,
so the GPIO or GIC will see the interrupts as if they happened during
normal runtime.

> So from a birds eye point of view I don't see how the Tegra
> PMC irq controller and Qualcomm's PDC power domain
> controller are conceptually different. Are you doing the same
> thing in two different ways for the same problem space
> here?
> 
> Or are these hardwares so very different that they really
> warrant two different approaches to wakeups?
> 
> I guess I miss a bit of hardware insight... is the key difference
> that in Qualcomm's PDC the IRQs need to be replayed/injected
> by software while Tegra's PMC will do this in hardware?

Yes, I think you're exactly right here. As I said above, I don't think
there's a way to replay interrupts with a pure parent/child hierarchy
because the hierarchy doesn't actually do anything at the interrupt
handler level. You'd need to set up additional demultiplexing at that
point to make it work, which is pretty much the equivalent of what Lina
has proposed.

On the other hand, since we don't get interrupts from the PMC for wake
events themselves, we can't replay interrupts on Tegra. And we don't
have to.

Unfortunately, these seem to be really similar pieces of hardware but
with just enough of a low-level difference to require completely
different solutions.

Thierry

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

Re: [PATCH 0/9] Implement wake event support on Tegra186 and later

[Lina Iyer]

Thanks Linus, for bringing this to my attention.

Hi Thierry,

On Tue, Sep 25 2018 at 03:57 -0600, Thierry Reding wrote:
>On Tue, Sep 25, 2018 at 10:48:39AM +0200, Linus Walleij wrote:
>> Hi Thierry,
>>
>> thanks for working on the wakeup business!
>>
>> This patch gets me a bit confused on our different approaches
>> toward wakeups in the kernel, so I included Lina, Marc and Ulf
>> to see if we can get some common understanding.
>>
>> On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>>
>> > 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
>>
>> I am not sure if you are aware about Lina's series
>> "Wakeup GPIO support for SDM845 SoC"
>> that is now in v3:
>> https://patchwork.kernel.org/cover/10587965/
>>
>> It appears to me, though I am blissfully ignorant of the
>> details, that there is a relationship between this patch
>> series and the other one.
>>
>> Your approach is to insert an hiearchical PMC irq controller
>> and Lina's approach is to simply put a mechanism on the
>> side to inject IRQs into the GIC after sleep (IIUC).
>
From a quick look at Lina's patches, I think it's more like adding a
>demultiplex in the TLMM. So the TLMM effectively has interrupt handlers
>for all wakeup interrupts so that when a wakeup interrupt happens, the
>GPIO interrupts can be replayed (using the interrupt status bit in the
>GPIO controller, if I'm reading things right).
>
I don't really have to replay the interrupt at the GPIO controller. The
PDC (= PMC on Tegra) receives the same interrupt line as the GPIO
controller and can wake up the system. The reason for this replaying the
interrupt status at the GPIO is because, the action handler registered
for the GPIO, by the driver, needs to be invoked for the PDC interrupt.
I haven't found a clean way to use the same action handler on the PDC
interrupt line. I couldn't set up the PDC as parent of the GPIO, because
not all GPIOs are routed through the PDC and secondly, the summary line
(mux line) from the GPIO is routed directly to the GIC and not the PDC.

From a very high level view both seem indeed to be very similar and have
>the same goal. Both are in a partition that is always powered on and the
>goal is to enable wake up from certain interrupts. One difference I see
>is that the PMC on Tegra allows wake events to originate from sources
>other than GPIOs. For example the RTC or PMIC interrupts (at the GIC)
>can be a source for the wake event, as can a number of other special
>signals. The PDC on the other hand seems to be limited to GPIOs as wake
>events.
>
The PDC (= PMC on Tegra) can wake up GPIOs as well as the regular
interrupts. GIC is the parent of the PDC.

>Another area, more low-level, where these setups seem to be different is
>that the PMC isn't really a proper interrupt controller in itself. It is
>more of a top-level interrupt gate. If you enable a given wake event
>(that is, unmask the "interrupt"), that event will be able to
>
This is an area where the PDC and PMC seem to be different. PDC is an
interrupt controller that is always ON and if it detects an interrupt
from any source that is enabled, it can wake up the GIC and replay the
interrupt at the GIC.

>> I guess your hierarchy is in response to this mail from tglx:
>> https://lkml.org/lkml/2018/9/14/339
>
>Yes, there was some good discussion in that thread which helped me come
>up with this solution. I think it's pretty elegant because it allows all
>of this interaction to happen almost automatically via the existing
>infrastructure. I'm not sure the same could be applied to the PDC,
>though, because of the need to manually replay the interrupt. That's not
>something I think can be done with just the simple parent/child
>relationship that we use on Tegra.
>
I wasn't able to use the hierarchy because not all GPIOs and the summary
line are routed to the PDC. But I am exploring options of hierarchy as
well.

Thanks,
Lina

>On the other hand, I don't think implementing something akin to Lina's
>proposal would work on Tegra because in our case the PMC doesn't
>actually raise an interrupt on wake. The hardware will simply wake up
>the system, at which point all the signals will be forwarded as normal,
>so the GPIO or GIC will see the interrupts as if they happened during
>normal runtime.
>
>> So from a birds eye point of view I don't see how the Tegra
>> PMC irq controller and Qualcomm's PDC power domain
>> controller are conceptually different. Are you doing the same
>> thing in two different ways for the same problem space
>> here?
>>
>> Or are these hardwares so very different that they really
>> warrant two different approaches to wakeups?
>>
>> I guess I miss a bit of hardware insight... is the key difference
>> that in Qualcomm's PDC the IRQs need to be replayed/injected
>> by software while Tegra's PMC will do this in hardware?
>
>Yes, I think you're exactly right here. As I said above, I don't think
>there's a way to replay interrupts with a pure parent/child hierarchy
>because the hierarchy doesn't actually do anything at the interrupt
>handler level. You'd need to set up additional demultiplexing at that
>point to make it work, which is pretty much the equivalent of what Lina
>has proposed.
>
>On the other hand, since we don't get interrupts from the PMC for wake
>events themselves, we can't replay interrupts on Tegra. And we don't
>have to.
>
>Unfortunately, these seem to be really similar pieces of hardware but
>with just enough of a low-level difference to require completely
>different solutions.
>
>Thierry

Re: [PATCH 0/9] Implement wake event support on Tegra186 and later

[Lina Iyer]

Thanks Linus, for bringing this to my attention.

Hi Thierry,

On Tue, Sep 25 2018 at 03:57 -0600, Thierry Reding wrote:
>On Tue, Sep 25, 2018 at 10:48:39AM +0200, Linus Walleij wrote:
>> Hi Thierry,
>>
>> thanks for working on the wakeup business!
>>
>> This patch gets me a bit confused on our different approaches
>> toward wakeups in the kernel, so I included Lina, Marc and Ulf
>> to see if we can get some common understanding.
>>
>> On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>>
>> > 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
>>
>> I am not sure if you are aware about Lina's series
>> "Wakeup GPIO support for SDM845 SoC"
>> that is now in v3:
>> https://patchwork.kernel.org/cover/10587965/
>>
>> It appears to me, though I am blissfully ignorant of the
>> details, that there is a relationship between this patch
>> series and the other one.
>>
>> Your approach is to insert an hiearchical PMC irq controller
>> and Lina's approach is to simply put a mechanism on the
>> side to inject IRQs into the GIC after sleep (IIUC).
>
>From a quick look at Lina's patches, I think it's more like adding a
>demultiplex in the TLMM. So the TLMM effectively has interrupt handlers
>for all wakeup interrupts so that when a wakeup interrupt happens, the
>GPIO interrupts can be replayed (using the interrupt status bit in the
>GPIO controller, if I'm reading things right).
>
I don't really have to replay the interrupt at the GPIO controller. The
PDC (= PMC on Tegra) receives the same interrupt line as the GPIO
controller and can wake up the system. The reason for this replaying the
interrupt status at the GPIO is because, the action handler registered
for the GPIO, by the driver, needs to be invoked for the PDC interrupt.
I haven't found a clean way to use the same action handler on the PDC
interrupt line. I couldn't set up the PDC as parent of the GPIO, because
not all GPIOs are routed through the PDC and secondly, the summary line
(mux line) from the GPIO is routed directly to the GIC and not the PDC.

>From a very high level view both seem indeed to be very similar and have
>the same goal. Both are in a partition that is always powered on and the
>goal is to enable wake up from certain interrupts. One difference I see
>is that the PMC on Tegra allows wake events to originate from sources
>other than GPIOs. For example the RTC or PMIC interrupts (at the GIC)
>can be a source for the wake event, as can a number of other special
>signals. The PDC on the other hand seems to be limited to GPIOs as wake
>events.
>
The PDC (= PMC on Tegra) can wake up GPIOs as well as the regular
interrupts. GIC is the parent of the PDC.

>Another area, more low-level, where these setups seem to be different is
>that the PMC isn't really a proper interrupt controller in itself. It is
>more of a top-level interrupt gate. If you enable a given wake event
>(that is, unmask the "interrupt"), that event will be able to
>
This is an area where the PDC and PMC seem to be different. PDC is an
interrupt controller that is always ON and if it detects an interrupt
from any source that is enabled, it can wake up the GIC and replay the
interrupt at the GIC.

>> I guess your hierarchy is in response to this mail from tglx:
>> https://lkml.org/lkml/2018/9/14/339
>
>Yes, there was some good discussion in that thread which helped me come
>up with this solution. I think it's pretty elegant because it allows all
>of this interaction to happen almost automatically via the existing
>infrastructure. I'm not sure the same could be applied to the PDC,
>though, because of the need to manually replay the interrupt. That's not
>something I think can be done with just the simple parent/child
>relationship that we use on Tegra.
>
I wasn't able to use the hierarchy because not all GPIOs and the summary
line are routed to the PDC. But I am exploring options of hierarchy as
well.

Thanks,
Lina

>On the other hand, I don't think implementing something akin to Lina's
>proposal would work on Tegra because in our case the PMC doesn't
>actually raise an interrupt on wake. The hardware will simply wake up
>the system, at which point all the signals will be forwarded as normal,
>so the GPIO or GIC will see the interrupts as if they happened during
>normal runtime.
>
>> So from a birds eye point of view I don't see how the Tegra
>> PMC irq controller and Qualcomm's PDC power domain
>> controller are conceptually different. Are you doing the same
>> thing in two different ways for the same problem space
>> here?
>>
>> Or are these hardwares so very different that they really
>> warrant two different approaches to wakeups?
>>
>> I guess I miss a bit of hardware insight... is the key difference
>> that in Qualcomm's PDC the IRQs need to be replayed/injected
>> by software while Tegra's PMC will do this in hardware?
>
>Yes, I think you're exactly right here. As I said above, I don't think
>there's a way to replay interrupts with a pure parent/child hierarchy
>because the hierarchy doesn't actually do anything at the interrupt
>handler level. You'd need to set up additional demultiplexing at that
>point to make it work, which is pretty much the equivalent of what Lina
>has proposed.
>
>On the other hand, since we don't get interrupts from the PMC for wake
>events themselves, we can't replay interrupts on Tegra. And we don't
>have to.
>
>Unfortunately, these seem to be really similar pieces of hardware but
>with just enough of a low-level difference to require completely
>different solutions.
>
>Thierry

Re: [PATCH 0/9] Implement wake event support on Tegra186 and later

[Lina Iyer]

Thanks Linus, for bringing this to my attention.

Hi Thierry,

On Tue, Sep 25 2018 at 03:57 -0600, Thierry Reding wrote:
>On Tue, Sep 25, 2018 at 10:48:39AM +0200, Linus Walleij wrote:
>> Hi Thierry,
>>
>> thanks for working on the wakeup business!
>>
>> This patch gets me a bit confused on our different approaches
>> toward wakeups in the kernel, so I included Lina, Marc and Ulf
>> to see if we can get some common understanding.
>>
>> On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>>
>> > 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
>>
>> I am not sure if you are aware about Lina's series
>> "Wakeup GPIO support for SDM845 SoC"
>> that is now in v3:
>> https://patchwork.kernel.org/cover/10587965/
>>
>> It appears to me, though I am blissfully ignorant of the
>> details, that there is a relationship between this patch
>> series and the other one.
>>
>> Your approach is to insert an hiearchical PMC irq controller
>> and Lina's approach is to simply put a mechanism on the
>> side to inject IRQs into the GIC after sleep (IIUC).
>
>From a quick look at Lina's patches, I think it's more like adding a
>demultiplex in the TLMM. So the TLMM effectively has interrupt handlers
>for all wakeup interrupts so that when a wakeup interrupt happens, the
>GPIO interrupts can be replayed (using the interrupt status bit in the
>GPIO controller, if I'm reading things right).
>
I don't really have to replay the interrupt at the GPIO controller. The
PDC (= PMC on Tegra) receives the same interrupt line as the GPIO
controller and can wake up the system. The reason for this replaying the
interrupt status at the GPIO is because, the action handler registered
for the GPIO, by the driver, needs to be invoked for the PDC interrupt.
I haven't found a clean way to use the same action handler on the PDC
interrupt line. I couldn't set up the PDC as parent of the GPIO, because
not all GPIOs are routed through the PDC and secondly, the summary line
(mux line) from the GPIO is routed directly to the GIC and not the PDC.

>From a very high level view both seem indeed to be very similar and have
>the same goal. Both are in a partition that is always powered on and the
>goal is to enable wake up from certain interrupts. One difference I see
>is that the PMC on Tegra allows wake events to originate from sources
>other than GPIOs. For example the RTC or PMIC interrupts (at the GIC)
>can be a source for the wake event, as can a number of other special
>signals. The PDC on the other hand seems to be limited to GPIOs as wake
>events.
>
The PDC (= PMC on Tegra) can wake up GPIOs as well as the regular
interrupts. GIC is the parent of the PDC.

>Another area, more low-level, where these setups seem to be different is
>that the PMC isn't really a proper interrupt controller in itself. It is
>more of a top-level interrupt gate. If you enable a given wake event
>(that is, unmask the "interrupt"), that event will be able to
>
This is an area where the PDC and PMC seem to be different. PDC is an
interrupt controller that is always ON and if it detects an interrupt
from any source that is enabled, it can wake up the GIC and replay the
interrupt at the GIC.

>> I guess your hierarchy is in response to this mail from tglx:
>> https://lkml.org/lkml/2018/9/14/339
>
>Yes, there was some good discussion in that thread which helped me come
>up with this solution. I think it's pretty elegant because it allows all
>of this interaction to happen almost automatically via the existing
>infrastructure. I'm not sure the same could be applied to the PDC,
>though, because of the need to manually replay the interrupt. That's not
>something I think can be done with just the simple parent/child
>relationship that we use on Tegra.
>
I wasn't able to use the hierarchy because not all GPIOs and the summary
line are routed to the PDC. But I am exploring options of hierarchy as
well.

Thanks,
Lina

>On the other hand, I don't think implementing something akin to Lina's
>proposal would work on Tegra because in our case the PMC doesn't
>actually raise an interrupt on wake. The hardware will simply wake up
>the system, at which point all the signals will be forwarded as normal,
>so the GPIO or GIC will see the interrupts as if they happened during
>normal runtime.
>
>> So from a birds eye point of view I don't see how the Tegra
>> PMC irq controller and Qualcomm's PDC power domain
>> controller are conceptually different. Are you doing the same
>> thing in two different ways for the same problem space
>> here?
>>
>> Or are these hardwares so very different that they really
>> warrant two different approaches to wakeups?
>>
>> I guess I miss a bit of hardware insight... is the key difference
>> that in Qualcomm's PDC the IRQs need to be replayed/injected
>> by software while Tegra's PMC will do this in hardware?
>
>Yes, I think you're exactly right here. As I said above, I don't think
>there's a way to replay interrupts with a pure parent/child hierarchy
>because the hierarchy doesn't actually do anything at the interrupt
>handler level. You'd need to set up additional demultiplexing at that
>point to make it work, which is pretty much the equivalent of what Lina
>has proposed.
>
>On the other hand, since we don't get interrupts from the PMC for wake
>events themselves, we can't replay interrupts on Tegra. And we don't
>have to.
>
>Unfortunately, these seem to be really similar pieces of hardware but
>with just enough of a low-level difference to require completely
>different solutions.
>
>Thierry

Re: [PATCH 0/9] Implement wake event support on Tegra186 and later

[Stephen Boyd]

Quoting Lina Iyer (2018-09-25 10:16:05)
> Thanks Linus, for bringing this to my attention.
> 
> Hi Thierry,
> 
> On Tue, Sep 25 2018 at 03:57 -0600, Thierry Reding wrote:
> >On Tue, Sep 25, 2018 at 10:48:39AM +0200, Linus Walleij wrote:
> >> Hi Thierry,
> >>
> >> thanks for working on the wakeup business!
> >>
> >> This patch gets me a bit confused on our different approaches
> >> toward wakeups in the kernel, so I included Lina, Marc and Ulf
> >> to see if we can get some common understanding.
> >>
> >> On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
> >> <thierry.reding@gmail.com> wrote:
> >>
> >> > 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
[...]
> >
> >Yes, there was some good discussion in that thread which helped me come
> >up with this solution. I think it's pretty elegant because it allows all
> >of this interaction to happen almost automatically via the existing
> >infrastructure. I'm not sure the same could be applied to the PDC,
> >though, because of the need to manually replay the interrupt. That's not
> >something I think can be done with just the simple parent/child
> >relationship that we use on Tegra.
> >
> I wasn't able to use the hierarchy because not all GPIOs and the summary
> line are routed to the PDC. But I am exploring options of hierarchy as
> well.
> 

From reading this thread (and https://lkml.org/lkml/2018/9/17/756) it
looks almost exactly the same. The only difference is that Nvidia Tegra
does the replay in hardware whereas Qualcomm SDM845 decided to replay
the irq in software. Either way, the gpio controller has two parent
domains, one is wakeup capable (PDC or PMC) and the other is not (GIC)
and some wakeup capable irqs only go through the PDC/PMC and then to the
GIC (e.g. RTC) instead of through gpio first. And it sounds like not all
gpios are wakeup capable in both designs.

The plan to have the gpio to wakeup capable irq map live in DT for the
PMC sounds good too. That way, the wakeup domain alloc function can
figure things out and redirect gpios by itself while the gpio controller
doesn't need to do anything special besides ask for wakeup to be set and
fail if the parent can't support it.

Can hierarchical irq domains entirely replace the chained irqchip code
in gpiolib? That would be interesting.

Re: [PATCH 0/9] Implement wake event support on Tegra186 and later

[Stephen Boyd]

Quoting Lina Iyer (2018-09-25 10:16:05)
> Thanks Linus, for bringing this to my attention.
> 
> Hi Thierry,
> 
> On Tue, Sep 25 2018 at 03:57 -0600, Thierry Reding wrote:
> >On Tue, Sep 25, 2018 at 10:48:39AM +0200, Linus Walleij wrote:
> >> Hi Thierry,
> >>
> >> thanks for working on the wakeup business!
> >>
> >> This patch gets me a bit confused on our different approaches
> >> toward wakeups in the kernel, so I included Lina, Marc and Ulf
> >> to see if we can get some common understanding.
> >>
> >> On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
> >> <thierry.reding@gmail.com> wrote:
> >>
> >> > 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
[...]
> >
> >Yes, there was some good discussion in that thread which helped me come
> >up with this solution. I think it's pretty elegant because it allows all
> >of this interaction to happen almost automatically via the existing
> >infrastructure. I'm not sure the same could be applied to the PDC,
> >though, because of the need to manually replay the interrupt. That's not
> >something I think can be done with just the simple parent/child
> >relationship that we use on Tegra.
> >
> I wasn't able to use the hierarchy because not all GPIOs and the summary
> line are routed to the PDC. But I am exploring options of hierarchy as
> well.
> 

From reading this thread (and https://lkml.org/lkml/2018/9/17/756) it
looks almost exactly the same. The only difference is that Nvidia Tegra
does the replay in hardware whereas Qualcomm SDM845 decided to replay
the irq in software. Either way, the gpio controller has two parent
domains, one is wakeup capable (PDC or PMC) and the other is not (GIC)
and some wakeup capable irqs only go through the PDC/PMC and then to the
GIC (e.g. RTC) instead of through gpio first. And it sounds like not all
gpios are wakeup capable in both designs.

The plan to have the gpio to wakeup capable irq map live in DT for the
PMC sounds good too. That way, the wakeup domain alloc function can
figure things out and redirect gpios by itself while the gpio controller
doesn't need to do anything special besides ask for wakeup to be set and
fail if the parent can't support it.

Can hierarchical irq domains entirely replace the chained irqchip code
in gpiolib? That would be interesting.

Re: [PATCH 0/9] Implement wake event support on Tegra186 and later

[Marc Zyngier]

On Mon, 08 Oct 2018 08:14:53 +0100,
Stephen Boyd <swboyd@chromium.org> wrote:
> 
> Quoting Lina Iyer (2018-09-25 10:16:05)
> > Thanks Linus, for bringing this to my attention.
> > 
> > Hi Thierry,
> > 
> > On Tue, Sep 25 2018 at 03:57 -0600, Thierry Reding wrote:
> > >On Tue, Sep 25, 2018 at 10:48:39AM +0200, Linus Walleij wrote:
> > >> Hi Thierry,
> > >>
> > >> thanks for working on the wakeup business!
> > >>
> > >> This patch gets me a bit confused on our different approaches
> > >> toward wakeups in the kernel, so I included Lina, Marc and Ulf
> > >> to see if we can get some common understanding.
> > >>
> > >> On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
> > >> <thierry.reding@gmail.com> wrote:
> > >>
> > >> > 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
> [...]
> > >
> > >Yes, there was some good discussion in that thread which helped me come
> > >up with this solution. I think it's pretty elegant because it allows all
> > >of this interaction to happen almost automatically via the existing
> > >infrastructure. I'm not sure the same could be applied to the PDC,
> > >though, because of the need to manually replay the interrupt. That's not
> > >something I think can be done with just the simple parent/child
> > >relationship that we use on Tegra.
> > >
> > I wasn't able to use the hierarchy because not all GPIOs and the summary
> > line are routed to the PDC. But I am exploring options of hierarchy as
> > well.
> > 
> 
> From reading this thread (and https://lkml.org/lkml/2018/9/17/756) it
> looks almost exactly the same. The only difference is that Nvidia Tegra
> does the replay in hardware whereas Qualcomm SDM845 decided to replay
> the irq in software. Either way, the gpio controller has two parent
> domains, one is wakeup capable (PDC or PMC) and the other is not (GIC)
> and some wakeup capable irqs only go through the PDC/PMC and then to the
> GIC (e.g. RTC) instead of through gpio first. And it sounds like not all
> gpios are wakeup capable in both designs.
> 
> The plan to have the gpio to wakeup capable irq map live in DT for the
> PMC sounds good too. That way, the wakeup domain alloc function can
> figure things out and redirect gpios by itself while the gpio controller
> doesn't need to do anything special besides ask for wakeup to be set and
> fail if the parent can't support it.
> 
> Can hierarchical irq domains entirely replace the chained irqchip code
> in gpiolib? That would be interesting.

I'm not convinced this is generally doable. Most GPIO blocks multiplex
the signalling on a single parent interrupt, meaning that although you
may be able to have a hierarchy extending to that point, it can't go
any further (at which point you're back into chained-irq land). It
doesn't mean it invalidates the above design, but it probably requires
a bit of flexibility.

I must admit having slightly lost track of the intricacies of the QC
design, but we already have a set of interrupt controllers whose sole
task is to generate wake-up events. They are well behaved though, in
the sense that they will regenerate edges that the QC HW drops on the
floor.

The main issue I can see is that the QC HW relies on some signal other
than the normal interrupt we can handle, and this completely breaks
the very notion of a hierarchy. You need some "side-band signalling"
which will re-inject the lost edges. That, on its own, is a bit of a
deal-breaker.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH 0/9] Implement wake event support on Tegra186 and later

[Marc Zyngier]

On Mon, 08 Oct 2018 08:14:53 +0100,
Stephen Boyd <swboyd@chromium.org> wrote:
> 
> Quoting Lina Iyer (2018-09-25 10:16:05)
> > Thanks Linus, for bringing this to my attention.
> > 
> > Hi Thierry,
> > 
> > On Tue, Sep 25 2018 at 03:57 -0600, Thierry Reding wrote:
> > >On Tue, Sep 25, 2018 at 10:48:39AM +0200, Linus Walleij wrote:
> > >> Hi Thierry,
> > >>
> > >> thanks for working on the wakeup business!
> > >>
> > >> This patch gets me a bit confused on our different approaches
> > >> toward wakeups in the kernel, so I included Lina, Marc and Ulf
> > >> to see if we can get some common understanding.
> > >>
> > >> On Fri, Sep 21, 2018 at 12:25 PM Thierry Reding
> > >> <thierry.reding@gmail.com> wrote:
> > >>
> > >> > 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
> [...]
> > >
> > >Yes, there was some good discussion in that thread which helped me come
> > >up with this solution. I think it's pretty elegant because it allows all
> > >of this interaction to happen almost automatically via the existing
> > >infrastructure. I'm not sure the same could be applied to the PDC,
> > >though, because of the need to manually replay the interrupt. That's not
> > >something I think can be done with just the simple parent/child
> > >relationship that we use on Tegra.
> > >
> > I wasn't able to use the hierarchy because not all GPIOs and the summary
> > line are routed to the PDC. But I am exploring options of hierarchy as
> > well.
> > 
> 
> From reading this thread (and https://lkml.org/lkml/2018/9/17/756) it
> looks almost exactly the same. The only difference is that Nvidia Tegra
> does the replay in hardware whereas Qualcomm SDM845 decided to replay
> the irq in software. Either way, the gpio controller has two parent
> domains, one is wakeup capable (PDC or PMC) and the other is not (GIC)
> and some wakeup capable irqs only go through the PDC/PMC and then to the
> GIC (e.g. RTC) instead of through gpio first. And it sounds like not all
> gpios are wakeup capable in both designs.
> 
> The plan to have the gpio to wakeup capable irq map live in DT for the
> PMC sounds good too. That way, the wakeup domain alloc function can
> figure things out and redirect gpios by itself while the gpio controller
> doesn't need to do anything special besides ask for wakeup to be set and
> fail if the parent can't support it.
> 
> Can hierarchical irq domains entirely replace the chained irqchip code
> in gpiolib? That would be interesting.

I'm not convinced this is generally doable. Most GPIO blocks multiplex
the signalling on a single parent interrupt, meaning that although you
may be able to have a hierarchy extending to that point, it can't go
any further (at which point you're back into chained-irq land). It
doesn't mean it invalidates the above design, but it probably requires
a bit of flexibility.

I must admit having slightly lost track of the intricacies of the QC
design, but we already have a set of interrupt controllers whose sole
task is to generate wake-up events. They are well behaved though, in
the sense that they will regenerate edges that the QC HW drops on the
floor.

The main issue I can see is that the QC HW relies on some signal other
than the normal interrupt we can handle, and this completely breaks
the very notion of a hierarchy. You need some "side-band signalling"
which will re-inject the lost edges. That, on its own, is a bit of a
deal-breaker.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.