[PATCH 0/7] qcom: support wakeup capable GPIOs

[PATCH 0/7] qcom: support wakeup capable GPIOs

[Lina Iyer]

Hi all,

This set of patches adds support for wakeup capable GPIOs. After discussions in
[1], this patches implement the recommendations suggested by Stephen.

The gist of the approach is that GPIO is setup in hierarchy with a
wakeup-parent irqchip. The key difference to [1] is that the TLMM GPIO driver
could mask the TLMM GPIO if requested by the wakeup-parent irqchip, thereby
supporing both MPM and PDC based architectures for QCOM SoCs. The work uses
Stephen's code pretty much as-is from [1].

Stephen,
I moved the GPIO-PDC pin map to a separate file to allow the follow-on chips to
add their SoC specific maps in the data file and avoid clobbering the PDC
driver.

Thanks to Thierry's patches to add support for hierarchy in GPIO framework. It
has been included here for completeness.

Kindly review these patches and let me know your commennts.

Thanks,
Lina

[1]. https://lkml.org/lkml/2018/11/21/168

Lina Iyer (5):
  irqdomain: add bus token DOMAIN_BUS_WAKEUP
  dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
  drivers: pinctrl: msm: setup GPIO irqchip hierarchy
  arm64: dts: msm: add PDC device bindings for sdm845
  arm64: dts: msm: setup PDC as wakeup parent for GPIOs for SDM845

Stephen Boyd (1):
  drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

Thierry Reding (1):
  gpio: Add support for hierarchical IRQ domains

 .../bindings/pinctrl/qcom,sdm845-pinctrl.txt  |   7 +-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |  10 ++
 drivers/gpio/gpiolib.c                        |  15 +-
 drivers/irqchip/Makefile                      |   2 +-
 drivers/irqchip/qcom-pdc-data.c               |  94 ++++++++++++
 drivers/irqchip/qcom-pdc.c                    | 111 ++++++++++++--
 drivers/irqchip/qcom-pdc.h                    |  24 ++++
 drivers/pinctrl/qcom/pinctrl-msm.c            | 136 ++++++++++++++++--
 include/linux/gpio/driver.h                   |   6 +
 include/linux/irqdomain.h                     |   1 +
 include/linux/soc/qcom/irq.h                  |  23 +++
 11 files changed, 401 insertions(+), 28 deletions(-)
 create mode 100644 drivers/irqchip/qcom-pdc-data.c
 create mode 100644 drivers/irqchip/qcom-pdc.h
 create mode 100644 include/linux/soc/qcom/irq.h

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Lina Iyer]

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 230e41562462..eb08de2e7c1d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1917,7 +1917,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;
@@ -1927,9 +1929,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 2db62b550b95..abcdb0c25a93 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:
 	 *
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/7] irqdomain: add bus token DOMAIN_BUS_WAKEUP

[Lina Iyer]

Add new bus token to describe domains that are wakeup capable.

Suggested-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 include/linux/irqdomain.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 068aa46f0d55..b9ea3c3998e2 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -81,6 +81,7 @@ enum irq_domain_bus_token {
 	DOMAIN_BUS_NEXUS,
 	DOMAIN_BUS_IPI,
 	DOMAIN_BUS_FSL_MC_MSI,
+	DOMAIN_BUS_WAKEUP,
 };
 
 /**
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/7] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Lina Iyer]

From: Stephen Boyd <sboyd@kernel.org>

Introduce a new domain for wakeup capable GPIOs. The domain can be
requested using the bus token DOMAIN_BUS_WAKEUP. In the following
patches, we will specify PDC as the wakeup-parent for the TLMM GPIO
irqchip. Requesting a wakeup GPIO will setup the GPIO and the
corresponding PDC interrupt as its parent.

Also, provide the map of the PDC pins for the GPIOs for SDM845.

Signed-off-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
[ilina: create separate file for PDC data]
---
 drivers/irqchip/Makefile        |   2 +-
 drivers/irqchip/qcom-pdc-data.c |  94 +++++++++++++++++++++++++++
 drivers/irqchip/qcom-pdc.c      | 111 +++++++++++++++++++++++++++++---
 drivers/irqchip/qcom-pdc.h      |  24 +++++++
 include/linux/soc/qcom/irq.h    |  23 +++++++
 5 files changed, 244 insertions(+), 10 deletions(-)
 create mode 100644 drivers/irqchip/qcom-pdc-data.c
 create mode 100644 drivers/irqchip/qcom-pdc.h
 create mode 100644 include/linux/soc/qcom/irq.h

diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 794c13d3ac3d..9c172f1328de 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -87,7 +87,7 @@ obj-$(CONFIG_ARCH_SYNQUACER)		+= irq-sni-exiu.o
 obj-$(CONFIG_MESON_IRQ_GPIO)		+= irq-meson-gpio.o
 obj-$(CONFIG_GOLDFISH_PIC) 		+= irq-goldfish-pic.o
 obj-$(CONFIG_NDS32)			+= irq-ativic32.o
-obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o
+obj-$(CONFIG_QCOM_PDC)			+= qcom-pdc.o qcom-pdc-data.o
 obj-$(CONFIG_CSKY_MPINTC)		+= irq-csky-mpintc.o
 obj-$(CONFIG_CSKY_APB_INTC)		+= irq-csky-apb-intc.o
 obj-$(CONFIG_SIFIVE_PLIC)		+= irq-sifive-plic.o
diff --git a/drivers/irqchip/qcom-pdc-data.c b/drivers/irqchip/qcom-pdc-data.c
new file mode 100644
index 000000000000..99b4be0af5db
--- /dev/null
+++ b/drivers/irqchip/qcom-pdc-data.c
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include "qcom-pdc.h"
+
+static const struct pdc_gpio_pin_map sdm845_gpio_pdc_map[] = {
+	{ 1, 30 },
+	{ 3, 31 },
+	{ 5, 32 },
+	{ 10, 33 },
+	{ 11, 34 },
+	{ 20, 35 },
+	{ 22, 36 },
+	{ 24, 37 },
+	{ 26, 38 },
+	{ 30, 39 },
+	{ 31, 117 },
+	{ 32, 41 },
+	{ 34, 42 },
+	{ 36, 43 },
+	{ 37, 44 },
+	{ 38, 45 },
+	{ 39, 46 },
+	{ 40, 47 },
+	{ 41, 115 },
+	{ 43, 49 },
+	{ 44, 50 },
+	{ 46, 51 },
+	{ 48, 52 },
+	{ 49, 118 },
+	{ 52, 54 },
+	{ 53, 55 },
+	{ 54, 56 },
+	{ 56, 57 },
+	{ 57, 58 },
+	{ 58, 59 },
+	{ 59, 60 },
+	{ 60, 61 },
+	{ 61, 62 },
+	{ 62, 63 },
+	{ 63, 64 },
+	{ 64, 65 },
+	{ 66, 66 },
+	{ 68, 67 },
+	{ 71, 68 },
+	{ 73, 69 },
+	{ 77, 70 },
+	{ 78, 71 },
+	{ 79, 72 },
+	{ 80, 73 },
+	{ 84, 74 },
+	{ 85, 75 },
+	{ 86, 76 },
+	{ 88, 77 },
+	{ 89, 116 },
+	{ 91, 79 },
+	{ 92, 80 },
+	{ 95, 81 },
+	{ 96, 82 },
+	{ 97, 83 },
+	{ 101, 84 },
+	{ 103, 85 },
+	{ 104, 86 },
+	{ 115, 90 },
+	{ 116, 91 },
+	{ 117, 92 },
+	{ 118, 93 },
+	{ 119, 94 },
+	{ 120, 95 },
+	{ 121, 96 },
+	{ 122, 97 },
+	{ 123, 98 },
+	{ 124, 99 },
+	{ 125, 100 },
+	{ 127, 102 },
+	{ 128, 103 },
+	{ 129, 104 },
+	{ 130, 105 },
+	{ 132, 106 },
+	{ 133, 107 },
+	{ 145, 108 },
+};
+
+static const struct pdc_gpio_pin_data sdm845_gpio_data = {
+	.size = ARRAY_SIZE(sdm845_gpio_pdc_map),
+	.map = sdm845_gpio_pdc_map,
+};
+
+const struct of_device_id pdc_gpio_match_table[] = {
+	{ .compatible = "qcom,845-pdc", .data = &sdm845_gpio_data },
+	{ },
+};
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index faa7d61b9d6c..1b9ec35958b1 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -13,12 +13,15 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
+#include <linux/soc/qcom/irq.h>
 #include <linux/spinlock.h>
-#include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 
+#include "qcom-pdc.h"
+
 #define PDC_MAX_IRQS		126
+#define PDC_MAX_GPIO_IRQS	256
 
 #define CLEAR_INTR(reg, intr)	(reg & ~(1 << intr))
 #define ENABLE_INTR(reg, intr)	(reg | (1 << intr))
@@ -47,9 +50,8 @@ static u32 pdc_reg_read(int reg, u32 i)
 	return readl_relaxed(pdc_base + reg + i * sizeof(u32));
 }
 
-static void pdc_enable_intr(struct irq_data *d, bool on)
+static void pdc_enable_intr(irq_hw_number_t pin_out, bool on)
 {
-	int pin_out = d->hwirq;
 	u32 index, mask;
 	u32 enable;
 
@@ -65,13 +67,23 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
 
 static void qcom_pdc_gic_mask(struct irq_data *d)
 {
-	pdc_enable_intr(d, false);
+	irq_hw_number_t hwirq = d->hwirq;
+
+	if (hwirq == ULONG_MAX)
+		return;
+
+	pdc_enable_intr(hwirq, false);
 	irq_chip_mask_parent(d);
 }
 
 static void qcom_pdc_gic_unmask(struct irq_data *d)
 {
-	pdc_enable_intr(d, true);
+	irq_hw_number_t hwirq = d->hwirq;
+
+	if (hwirq == ULONG_MAX)
+		return;
+
+	pdc_enable_intr(hwirq, true);
 	irq_chip_unmask_parent(d);
 }
 
@@ -111,9 +123,12 @@ enum pdc_irq_config_bits {
  */
 static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
 {
-	int pin_out = d->hwirq;
+	irq_hw_number_t pin_out = d->hwirq;
 	enum pdc_irq_config_bits pdc_type;
 
+	if (pin_out == ULONG_MAX)
+		return 0;
+
 	switch (type) {
 	case IRQ_TYPE_EDGE_RISING:
 		pdc_type = PDC_EDGE_RISING;
@@ -157,7 +172,7 @@ static struct irq_chip qcom_pdc_gic_chip = {
 	.irq_set_affinity	= irq_chip_set_affinity_parent,
 };
 
-static irq_hw_number_t get_parent_hwirq(int pin)
+static irq_hw_number_t get_parent_hwirq(irq_hw_number_t pin)
 {
 	int i;
 	struct pdc_pin_region *region;
@@ -169,7 +184,6 @@ static irq_hw_number_t get_parent_hwirq(int pin)
 			return (region->parent_base + pin - region->pin_base);
 	}
 
-	WARN_ON(1);
 	return ~0UL;
 }
 
@@ -232,6 +246,73 @@ static const struct irq_domain_ops qcom_pdc_ops = {
 	.free		= irq_domain_free_irqs_common,
 };
 
+static irq_hw_number_t qcom_gpio_to_pdc_pin(struct irq_domain *domain,
+					    unsigned int gpio)
+{
+	struct device_node *dn = irq_domain_get_of_node(domain);
+	const struct of_device_id *match_id;
+	const struct pdc_gpio_pin_data *data;
+	unsigned int i;
+
+	match_id = of_match_node(pdc_gpio_match_table, dn);
+	if (!match_id)
+		return ULONG_MAX;
+
+	data = match_id->data;
+	if (!data)
+		return ULONG_MAX;
+
+	for (i = 0; i < data->size; i++)
+		if (gpio == data->map[i].gpio)
+			return data->map[i].pdc_pin;
+
+	return ULONG_MAX;
+}
+
+static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq,
+			       unsigned int nr_irqs, void *data)
+{
+	struct qcom_irq_fwspec *qcom_fwspec = data;
+	struct irq_fwspec *fwspec = &qcom_fwspec->fwspec;
+	struct irq_fwspec parent_fwspec;
+	irq_hw_number_t hwirq, parent_hwirq;
+	unsigned int type;
+	int ret;
+
+	hwirq = qcom_gpio_to_pdc_pin(domain, fwspec->param[0]);
+	parent_hwirq = get_parent_hwirq(hwirq);
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					    &qcom_pdc_gic_chip, NULL);
+	if (ret)
+		return ret;
+
+	if (hwirq == ULONG_MAX)
+		return 0;
+
+	qcom_fwspec->mask = true;
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		type = IRQ_TYPE_EDGE_RISING;
+
+	if (type & IRQ_TYPE_LEVEL_MASK)
+		type = IRQ_TYPE_LEVEL_HIGH;
+
+	parent_fwspec.fwnode      = domain->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0]    = 0;
+	parent_fwspec.param[1]    = parent_hwirq;
+	parent_fwspec.param[2]    = type;
+
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+					    &parent_fwspec);
+}
+
+static const struct irq_domain_ops qcom_pdc_gpio_ops = {
+	.alloc		= qcom_pdc_gpio_alloc,
+	.free		= irq_domain_free_irqs_common,
+};
+
 static int pdc_setup_pin_mapping(struct device_node *np)
 {
 	int ret, n;
@@ -270,7 +351,7 @@ static int pdc_setup_pin_mapping(struct device_node *np)
 
 static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
 {
-	struct irq_domain *parent_domain, *pdc_domain;
+	struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain;
 	int ret;
 
 	pdc_base = of_iomap(node, 0);
@@ -301,6 +382,18 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
 		goto fail;
 	}
 
+	pdc_gpio_domain = irq_domain_create_hierarchy(parent_domain, 0,
+						      PDC_MAX_GPIO_IRQS,
+						      of_fwnode_handle(node),
+						      &qcom_pdc_gpio_ops, NULL);
+	if (!pdc_gpio_domain) {
+		pr_err("%pOF: GIC domain add failed for GPIO domain\n", node);
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	irq_domain_update_bus_token(pdc_gpio_domain, DOMAIN_BUS_WAKEUP);
+
 	return 0;
 
 fail:
diff --git a/drivers/irqchip/qcom-pdc.h b/drivers/irqchip/qcom-pdc.h
new file mode 100644
index 000000000000..3091db646004
--- /dev/null
+++ b/drivers/irqchip/qcom-pdc.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __QCOM_PDC_H
+#define __QCOM_PDC_H
+
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+
+struct pdc_gpio_pin_map {
+	unsigned int gpio;
+	u32 pdc_pin;
+};
+
+struct pdc_gpio_pin_data {
+	size_t size;
+	const struct pdc_gpio_pin_map *map;
+};
+
+extern const struct of_device_id pdc_gpio_match_table[];
+
+#endif /* __QCOM_PDC_H */
diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h
new file mode 100644
index 000000000000..bacc9edbce0d
--- /dev/null
+++ b/include/linux/soc/qcom/irq.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __QCOM_IRQ_H
+#define __QCOM_IRQ_H
+
+#include <linux/irqdomain.h>
+
+/**
+ * struct qcom_irq_fwspec - qcom specific irq fwspec wrapper
+ * @fwspec: irq fwspec
+ * @mask: if true, keep the irq masked in the gpio controller
+ *
+ * Use this structure to communicate between the parent irq chip, MPM or PDC,
+ * to the gpio chip, TLMM, about the gpio being allocated in the parent
+ * and if the gpio chip should keep the line masked because the parent irq
+ * chip is handling everything about the irq line.
+ */
+struct qcom_irq_fwspec {
+	struct irq_fwspec fwspec;
+	bool mask;
+};
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Lina Iyer]

SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO
routed to the PDC as interrupts that can be used to wake the system up
from deep low power modes and suspend.

Cc: devicetree@vger.kernel.org
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt    | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
index 665aadb5ea28..a522ca46667d 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
@@ -29,6 +29,11 @@ SDM845 platform.
 	Definition: must be 2. Specifying the pin number and flags, as defined
 		    in <dt-bindings/interrupt-controller/irq.h>
 
+- wakeup-parent:
+	Usage: optional
+	Value type: <phandle>
+	Definition: A phandle to the wakeup interrupt controller for the SoC.
+
 - gpio-controller:
 	Usage: required
 	Value type: <none>
@@ -53,7 +58,6 @@ pin, a group, or a list of pins or groups. This configuration can include the
 mux function to select on those pin(s)/group(s), and various pin configuration
 parameters, such as pull-up, drive strength, etc.
 
-
 PIN CONFIGURATION NODES:
 
 The name of each subnode is not important; all subnodes should be enumerated
@@ -160,6 +164,7 @@ Example:
 		#gpio-cells = <2>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
+		wakeup-parent = <&pdc>;
 
 		qup9_active: qup9-active {
 			mux {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 5/7] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Lina Iyer]

To allow GPIOs to wakeup the system from suspend or deep idle, the
wakeup capable GPIOs are setup in hierarchy with interrupts from the
wakeup-parent irqchip.

In older SoC's, the TLMM will handover detection to the parent irqchip
and in newer SoC's, the parent irqchip may also be active as well as the
TLMM and therefore the GPIOs need to be masked at TLMM to avoid
duplicate interrupts. To enable both these configurations to exist,
allow the parent irqchip to dictate the TLMM irqchip's behavior when
masking/unmasking the interrupt.

Signed-off-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 136 ++++++++++++++++++++++++++---
 1 file changed, 123 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 7c7d083e2c0d..c86117c981e3 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -17,6 +17,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinctrl.h>
@@ -27,6 +28,7 @@
 #include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
+#include <linux/soc/qcom/irq.h>
 #include <linux/reboot.h>
 #include <linux/pm.h>
 #include <linux/log2.h>
@@ -69,6 +71,7 @@ struct msm_pinctrl {
 
 	DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
 	DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
+	DECLARE_BITMAP(wakeup_masked_irqs, MAX_NR_GPIO);
 
 	const struct msm_pinctrl_soc_data *soc;
 	void __iomem *regs[MAX_NR_TILES];
@@ -703,6 +706,13 @@ static void msm_gpio_irq_mask(struct irq_data *d)
 
 	g = &pctrl->soc->groups[d->hwirq];
 
+	if (d->parent_data)
+		irq_chip_unmask_parent(d);
+
+	/* Monitored by parent wakeup controller? Keep masked */
+	if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+		return;
+
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	val = msm_readl_intr_cfg(pctrl, g);
@@ -735,6 +745,9 @@ static void msm_gpio_irq_mask(struct irq_data *d)
 	clear_bit(d->hwirq, pctrl->enabled_irqs);
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	if (d->parent_data)
+		irq_chip_mask_parent(d);
 }
 
 static void msm_gpio_irq_unmask(struct irq_data *d)
@@ -767,6 +780,10 @@ static void msm_gpio_irq_ack(struct irq_data *d)
 	unsigned long flags;
 	u32 val;
 
+	/* Handled by parent wakeup controller? Do nothing */
+	if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+		return;
+
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
@@ -794,6 +811,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 
 	g = &pctrl->soc->groups[d->hwirq];
 
+	if (d->parent_data)
+		irq_chip_set_type_parent(d, type);
+
+	/* Monitored by parent wakeup controller? Keep masked */
+	if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+		return 0;
+
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	/*
@@ -890,6 +914,9 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
+	if (d->parent_data)
+		irq_chip_set_wake_parent(d, on);
+
 	return 0;
 }
 
@@ -967,11 +994,86 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
 }
 
+static int msm_gpio_domain_translate(struct irq_domain *d,
+				     struct irq_fwspec *fwspec,
+				     unsigned long *hwirq, unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count < 2)
+			return -EINVAL;
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				 unsigned int nr_irqs, void *arg)
+{
+	int ret;
+	irq_hw_number_t hwirq;
+	struct gpio_chip *gc = domain->host_data;
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	struct irq_fwspec *fwspec = arg;
+	struct qcom_irq_fwspec parent = { };
+	unsigned int type;
+
+	ret = msm_gpio_domain_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					    &pctrl->irq_chip, gc);
+	if (ret < 0)
+		return ret;
+
+	if (!domain->parent)
+		return 0;
+
+	parent.fwspec.fwnode      = domain->parent->fwnode;
+	parent.fwspec.param_count = 2;
+	parent.fwspec.param[0]    = hwirq;
+	parent.fwspec.param[1]    = type;
+
+	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent);
+	if (ret)
+		return ret;
+
+	if (parent.mask)
+		set_bit(hwirq, pctrl->wakeup_masked_irqs);
+
+	return 0;
+}
+
+/*
+ * TODO: Get rid of this and push it into gpiochip_to_irq()
+ */
+static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct irq_fwspec fwspec;
+
+	fwspec.fwnode = of_node_to_fwnode(chip->of_node);
+	fwspec.param[0] = offset;
+	fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
+	fwspec.param_count = 2;
+
+	return irq_create_fwspec_mapping(&fwspec);
+}
+
+static const struct irq_domain_ops msm_gpio_domain_ops = {
+	.translate = msm_gpio_domain_translate,
+	.alloc     = msm_gpio_domain_alloc,
+	.free      = irq_domain_free_irqs_top,
+};
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;
 	int ret;
 	unsigned ngpio = pctrl->soc->ngpios;
+	struct device_node *dn;
 
 	if (WARN_ON(ngpio > MAX_NR_GPIO))
 		return -EINVAL;
@@ -986,6 +1088,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	chip->need_valid_mask = msm_gpio_needs_valid_mask(pctrl);
 
 	pctrl->irq_chip.name = "msmgpio";
+	pctrl->irq_chip.irq_eoi	= irq_chip_eoi_parent;
 	pctrl->irq_chip.irq_mask = msm_gpio_irq_mask;
 	pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask;
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
@@ -994,6 +1097,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
 	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
+	chip->irq.chip = &pctrl->irq_chip;
+	chip->irq.domain_ops = &msm_gpio_domain_ops;
+	chip->irq.handler = handle_edge_irq;
+	chip->irq.default_type = IRQ_TYPE_NONE;
+
+	dn = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
+	if (dn) {
+		chip->irq.parent_domain = irq_find_matching_host(dn,
+						 DOMAIN_BUS_WAKEUP);
+		of_node_put(dn);
+		if (!chip->irq.parent_domain)
+			return -EPROBE_DEFER;
+
+		chip->to_irq = msm_gpio_to_irq;
+	}
+
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
 		dev_err(pctrl->dev, "Failed register gpiochip\n");
@@ -1015,26 +1134,17 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 			dev_name(pctrl->dev), 0, 0, chip->ngpio);
 		if (ret) {
 			dev_err(pctrl->dev, "Failed to add pin range\n");
-			gpiochip_remove(&pctrl->chip);
-			return ret;
+			goto fail;
 		}
 	}
 
-	ret = gpiochip_irqchip_add(chip,
-				   &pctrl->irq_chip,
-				   0,
-				   handle_edge_irq,
-				   IRQ_TYPE_NONE);
-	if (ret) {
-		dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n");
-		gpiochip_remove(&pctrl->chip);
-		return -ENOSYS;
-	}
-
 	gpiochip_set_chained_irqchip(chip, &pctrl->irq_chip, pctrl->irq,
 				     msm_gpio_irq_handler);
 
 	return 0;
+fail:
+	gpiochip_remove(&pctrl->chip);
+	return ret;
 }
 
 static int msm_ps_hold_restart(struct notifier_block *nb, unsigned long action,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 6/7] arm64: dts: msm: add PDC device bindings for sdm845

[Lina Iyer]

Add PDC interrupt controller device bindings for SDM845.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v3:
	- Fix PDC map, use GIC SPI port number for hwirq
Changes in v2:
	- Order by address
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index b72bdb0a31a5..8e15392a6f64 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1278,6 +1278,15 @@
 			#reset-cells = <1>;
 		};
 
+		pdc: interrupt-controller@b220000 {
+			compatible = "qcom,sdm845-pdc";
+			reg = <0xb220000 0x30000>;
+			qcom,pdc-ranges = <0 480 94>, <94 609 15>, <115 630 7>;
+			#interrupt-cells = <2>;
+			interrupt-parent = <&intc>;
+			interrupt-controller;
+		};
+
 		spmi_bus: spmi@c440000 {
 			compatible = "qcom,spmi-pmic-arb";
 			reg = <0xc440000 0x1100>,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 7/7] arm64: dts: msm: setup PDC as wakeup parent for GPIOs for SDM845

[Lina Iyer]

Add PDC wakeup parent for TLMM for SDM845 SoC.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 8e15392a6f64..40dca655ba86 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -821,6 +821,7 @@
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
+			wakeup-parent = <&pdc>;
 
 			qup_i2c0_default: qup-i2c0-default {
 				pinmux {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 6/7] arm64: dts: msm: add PDC device bindings for sdm845

[Doug Anderson]

Hi,

On Wed, Dec 19, 2018 at 2:11 PM Lina Iyer <ilina@codeaurora.org> wrote:
>
> Add PDC interrupt controller device bindings for SDM845.
>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
> Changes in v3:
>         - Fix PDC map, use GIC SPI port number for hwirq
> Changes in v2:
>         - Order by address
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)

nit: ${SUBJECT} makes it sounds like you're adding something into the
"Documentation/devicetree/bindings" folder, but you're not.  Also you
probably want the prefix "qcom", not "msm" since it ends up in the
"qcom" dir.  Also, subject should say that this is the interrupt
controller.  How about:

arm64: dts: qcom: add PDC interrupt controller for sdm845


> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index b72bdb0a31a5..8e15392a6f64 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -1278,6 +1278,15 @@
>                         #reset-cells = <1>;
>                 };

nit: the above node looks like the tail end of pdc reset controller.
That has a unit address of b2e0000.  Your unit address is smaller than
the the pdc reset controller so you should be above it, not below it.


> +               pdc: interrupt-controller@b220000 {

nit: Maybe the label should be "pdc_intc" not just "pdc".  This is
just the node for the interrupt controller, not the whole pdc, right?


> +                       compatible = "qcom,sdm845-pdc";
> +                       reg = <0xb220000 0x30000>;

nit: apparently common practice for Quaclomm dts is to pad the address
in the "reg" field to all 8 digits.  So the above should be:

reg = <0x0b220000 0x30000>;

NOTE: it's important to _not_ pad the unit address in the node name
(so you got that right).  Only update the "reg".  For context:

https://lkml.kernel.org/r/CAD=FV=WrvRH6QpaQ67yw2MFz8RP59ozkSfQC4+OAM_8fAbGZuw@mail.gmail.com


-Doug

Re: [PATCH 5/7] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Stephen Boyd]

Quoting Lina Iyer (2018-12-19 14:11:03)
> To allow GPIOs to wakeup the system from suspend or deep idle, the
> wakeup capable GPIOs are setup in hierarchy with interrupts from the
> wakeup-parent irqchip.
> 
> In older SoC's, the TLMM will handover detection to the parent irqchip
> and in newer SoC's, the parent irqchip may also be active as well as the
> TLMM and therefore the GPIOs need to be masked at TLMM to avoid
> duplicate interrupts. To enable both these configurations to exist,
> allow the parent irqchip to dictate the TLMM irqchip's behavior when
> masking/unmasking the interrupt.
> 
> Signed-off-by: Stephen Boyd <sboyd@kernel.org>

I don't think I gave a signed-off-by, so you need to ask to forge my
sign off here. Please change it to be:

 Signed-off-by: Stephen Boyd <swboyd@chromium.org>

and I'm not sure how much I wrote vs. you wrote anymore so perhaps also
add a

 Co-developed-by: Stephen Boyd <swboyd@chromium.org>

And finally, please just email my chromium.org email for this series
because I apparently messed up the address once and now it's all going
to the wrong inbox. Thanks!

> Signed-off-by: Lina Iyer <ilina@codeaurora.org>

Can you Cc Linus Walleij and Bjorn Andersson on the whole patch series
next time? Would be good to have their review on major pinctrl driver
changes.

> @@ -967,11 +994,86 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
>         return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
>  }
>  
> +static int msm_gpio_domain_translate(struct irq_domain *d,
> +                                    struct irq_fwspec *fwspec,
> +                                    unsigned long *hwirq, unsigned int *type)
> +{
> +       if (is_of_node(fwspec->fwnode)) {
> +               if (fwspec->param_count < 2)
> +                       return -EINVAL;
> +               *hwirq = fwspec->param[0];
> +               *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +               return 0;
> +       }
> +
> +       return -EINVAL;
> +}

Maybe this can be a generic function in gpiolib?

> +
> +static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +                                unsigned int nr_irqs, void *arg)
> +{
> +       int ret;
> +       irq_hw_number_t hwirq;
> +       struct gpio_chip *gc = domain->host_data;
> +       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +       struct irq_fwspec *fwspec = arg;
> +       struct qcom_irq_fwspec parent = { };
> +       unsigned int type;
> +
> +       ret = msm_gpio_domain_translate(domain, fwspec, &hwirq, &type);
> +       if (ret)
> +               return ret;
> +
> +       ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +                                           &pctrl->irq_chip, gc);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (!domain->parent)
> +               return 0;
> +
> +       parent.fwspec.fwnode      = domain->parent->fwnode;
> +       parent.fwspec.param_count = 2;
> +       parent.fwspec.param[0]    = hwirq;
> +       parent.fwspec.param[1]    = type;
> +
> +       ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent);
> +       if (ret)
> +               return ret;
> +
> +       if (parent.mask)
> +               set_bit(hwirq, pctrl->wakeup_masked_irqs);
> +
> +       return 0;
> +}
> +
> +/*
> + * TODO: Get rid of this and push it into gpiochip_to_irq()

Hmm.. yeah we need to do this still. I think we can have a generic two
cell function similar to irq_domain_xlate_twocell() that does the fwspec
creation and uses some of the things that we pass to
gpiochip_irqchip_add(), like the default level type. This existing
function is not good to have, so there's work to do to get rid of this.

I was also thinking that maybe we can make the alloc function above take
a struct gpio_irq_fwspec structure that tells the alloc function what
gpiochip the irq is for. That would mean that we need to change the
gpio_to_irq() function below to be generic and stuff the chip inside the
fwspec wrapper structure:

	struct gpio_irq_fwspec {
		struct irq_fwspec fwspec;
		struct gpio_chip *chip;
		unsigned int offset;
	};

but I seem to recall that was not working for some reason.

> + */
> +static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct irq_fwspec fwspec;
> +
> +       fwspec.fwnode = of_node_to_fwnode(chip->of_node);
> +       fwspec.param[0] = offset;
> +       fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
> +       fwspec.param_count = 2;
> +
> +       return irq_create_fwspec_mapping(&fwspec);
> +}
> +
> +static const struct irq_domain_ops msm_gpio_domain_ops = {
> +       .translate = msm_gpio_domain_translate,
> +       .alloc     = msm_gpio_domain_alloc,
> +       .free      = irq_domain_free_irqs_top,
> +};
> +

Re: [PATCH 3/7] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Stephen Boyd]

Quoting Lina Iyer (2018-12-19 14:11:01)
> diff --git a/drivers/irqchip/qcom-pdc-data.c b/drivers/irqchip/qcom-pdc-data.c
> new file mode 100644
> index 000000000000..99b4be0af5db
> --- /dev/null
> +++ b/drivers/irqchip/qcom-pdc-data.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include "qcom-pdc.h"
> +
> +static const struct pdc_gpio_pin_map sdm845_gpio_pdc_map[] = {
> +       { 1, 30 },
> +       { 3, 31 },
> +       { 5, 32 },
> +       { 10, 33 },
> +       { 11, 34 },
> +       { 20, 35 },
> +       { 22, 36 },
> +       { 24, 37 },
> +       { 26, 38 },
> +       { 30, 39 },
> +       { 31, 117 },
> +       { 32, 41 },
> +       { 34, 42 },
> +       { 36, 43 },
> +       { 37, 44 },
> +       { 38, 45 },
> +       { 39, 46 },
> +       { 40, 47 },
> +       { 41, 115 },
> +       { 43, 49 },
> +       { 44, 50 },
> +       { 46, 51 },
> +       { 48, 52 },
> +       { 49, 118 },
> +       { 52, 54 },
> +       { 53, 55 },
> +       { 54, 56 },
> +       { 56, 57 },
> +       { 57, 58 },
> +       { 58, 59 },
> +       { 59, 60 },
> +       { 60, 61 },
> +       { 61, 62 },
> +       { 62, 63 },
> +       { 63, 64 },
> +       { 64, 65 },
> +       { 66, 66 },
> +       { 68, 67 },
> +       { 71, 68 },
> +       { 73, 69 },
> +       { 77, 70 },
> +       { 78, 71 },
> +       { 79, 72 },
> +       { 80, 73 },
> +       { 84, 74 },
> +       { 85, 75 },
> +       { 86, 76 },
> +       { 88, 77 },
> +       { 89, 116 },
> +       { 91, 79 },
> +       { 92, 80 },
> +       { 95, 81 },
> +       { 96, 82 },
> +       { 97, 83 },
> +       { 101, 84 },
> +       { 103, 85 },
> +       { 104, 86 },
> +       { 115, 90 },
> +       { 116, 91 },
> +       { 117, 92 },
> +       { 118, 93 },
> +       { 119, 94 },
> +       { 120, 95 },
> +       { 121, 96 },
> +       { 122, 97 },
> +       { 123, 98 },
> +       { 124, 99 },
> +       { 125, 100 },
> +       { 127, 102 },
> +       { 128, 103 },
> +       { 129, 104 },
> +       { 130, 105 },
> +       { 132, 106 },
> +       { 133, 107 },
> +       { 145, 108 },
> +};
> +
> +static const struct pdc_gpio_pin_data sdm845_gpio_data = {
> +       .size = ARRAY_SIZE(sdm845_gpio_pdc_map),
> +       .map = sdm845_gpio_pdc_map,
> +};
> +
> +const struct of_device_id pdc_gpio_match_table[] = {
> +       { .compatible = "qcom,845-pdc", .data = &sdm845_gpio_data },

Why not qcom,sdm845-pdc?

> +       { },
> +};

I wonder why we wouldn't just put this all into the qcom-pdc.c file at
the bottom and then have that IRQCHIP_DECLARE() macros call small
functions that pass the pdc to gpio mapping table to qcom_pdc_init()
that takes a third argument?

I really hope that in the future all the gpios can be wakeup capable so
that we don't need to have the table at all!

Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Rob Herring]

On Wed, Dec 19, 2018 at 03:11:02PM -0700, Lina Iyer wrote:
> SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO
> routed to the PDC as interrupts that can be used to wake the system up
> from deep low power modes and suspend.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
>  .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt    | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> index 665aadb5ea28..a522ca46667d 100644
> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> @@ -29,6 +29,11 @@ SDM845 platform.
>  	Definition: must be 2. Specifying the pin number and flags, as defined
>  		    in <dt-bindings/interrupt-controller/irq.h>
>  
> +- wakeup-parent:
> +	Usage: optional
> +	Value type: <phandle>
> +	Definition: A phandle to the wakeup interrupt controller for the SoC.

Is this really necessary? Is there more than one possible wakeup-parent 
node?

> +
>  - gpio-controller:
>  	Usage: required
>  	Value type: <none>
> @@ -53,7 +58,6 @@ pin, a group, or a list of pins or groups. This configuration can include the
>  mux function to select on those pin(s)/group(s), and various pin configuration
>  parameters, such as pull-up, drive strength, etc.
>  
> -
>  PIN CONFIGURATION NODES:
>  
>  The name of each subnode is not important; all subnodes should be enumerated
> @@ -160,6 +164,7 @@ Example:
>  		#gpio-cells = <2>;
>  		interrupt-controller;
>  		#interrupt-cells = <2>;
> +		wakeup-parent = <&pdc>;
>  
>  		qup9_active: qup9-active {
>  			mux {
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH 3/7] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Lina Iyer]

On Thu, Dec 20 2018 at 13:19 -0700, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-12-19 14:11:01)
>> diff --git a/drivers/irqchip/qcom-pdc-data.c b/drivers/irqchip/qcom-pdc-data.c
>> new file mode 100644
>> index 000000000000..99b4be0af5db
>> --- /dev/null
>> +++ b/drivers/irqchip/qcom-pdc-data.c
>> @@ -0,0 +1,94 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include "qcom-pdc.h"
>> +
[...]

>> +static const struct pdc_gpio_pin_data sdm845_gpio_data = {
>> +       .size = ARRAY_SIZE(sdm845_gpio_pdc_map),
>> +       .map = sdm845_gpio_pdc_map,
>> +};
>> +
>> +const struct of_device_id pdc_gpio_match_table[] = {
>> +       { .compatible = "qcom,845-pdc", .data = &sdm845_gpio_data },
>
>Why not qcom,sdm845-pdc?
>
The compatible matches the compatible specified in the PDC driver. Not
sure why the 'sdm' was left out at that time.

>> +       { },
>> +};
>
>I wonder why we wouldn't just put this all into the qcom-pdc.c file at
>the bottom and then have that IRQCHIP_DECLARE() macros call small
>functions that pass the pdc to gpio mapping table to qcom_pdc_init()
>that takes a third argument?
>
We could. I feel we would be adding tables like this and it just
clutters up the driver file. May be in the future we could move to
target specific data file like the gpios, but that could be excessive
too. Thought this might be a compromise. I am open to change.

>I really hope that in the future all the gpios can be wakeup capable so
>that we don't need to have the table at all!
>
I doubt there are plans to support that in hardware. We should plan for
supporting tables like this for other chipsets based on the PDC
architecture.

Thanks,
Lina

Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Lina Iyer]

On Fri, Dec 28 2018 at 17:07 -0700, Rob Herring wrote:
>On Wed, Dec 19, 2018 at 03:11:02PM -0700, Lina Iyer wrote:
>> SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO
>> routed to the PDC as interrupts that can be used to wake the system up
>> from deep low power modes and suspend.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>>  .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt    | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
>> index 665aadb5ea28..a522ca46667d 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
>> @@ -29,6 +29,11 @@ SDM845 platform.
>>  	Definition: must be 2. Specifying the pin number and flags, as defined
>>  		    in <dt-bindings/interrupt-controller/irq.h>
>>
>> +- wakeup-parent:
>> +	Usage: optional
>> +	Value type: <phandle>
>> +	Definition: A phandle to the wakeup interrupt controller for the SoC.
>
>Is this really necessary? Is there more than one possible wakeup-parent
>node?
>
No.  There is only one but depending on the architecture, the wakeup
interrupt controller could be different device like PDC on SDM845 or MPM
on SDM820.

What do you have in mind? Let me know if you have a better idea than
referencing in DT.

Thanks,
Lina

>> +
>>  - gpio-controller:
>>  	Usage: required
>>  	Value type: <none>
>> @@ -53,7 +58,6 @@ pin, a group, or a list of pins or groups. This configuration can include the
>>  mux function to select on those pin(s)/group(s), and various pin configuration
>>  parameters, such as pull-up, drive strength, etc.
>>
>> -
>>  PIN CONFIGURATION NODES:
>>
>>  The name of each subnode is not important; all subnodes should be enumerated
>> @@ -160,6 +164,7 @@ Example:
>>  		#gpio-cells = <2>;
>>  		interrupt-controller;
>>  		#interrupt-cells = <2>;
>> +		wakeup-parent = <&pdc>;
>>
>>  		qup9_active: qup9-active {
>>  			mux {
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>

Re: [PATCH 6/7] arm64: dts: msm: add PDC device bindings for sdm845

[Lina Iyer]

On Thu, Dec 20 2018 at 11:14 -0700, Doug Anderson wrote:
>Hi,
>
>On Wed, Dec 19, 2018 at 2:11 PM Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> Add PDC interrupt controller device bindings for SDM845.
>>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>> Changes in v3:
>>         - Fix PDC map, use GIC SPI port number for hwirq
>> Changes in v2:
>>         - Order by address
>> ---
>>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>
>nit: ${SUBJECT} makes it sounds like you're adding something into the
>"Documentation/devicetree/bindings" folder, but you're not.  Also you
>probably want the prefix "qcom", not "msm" since it ends up in the
>"qcom" dir.  Also, subject should say that this is the interrupt
>controller.  How about:
>
>arm64: dts: qcom: add PDC interrupt controller for sdm845
>
>
Sure.

>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> index b72bdb0a31a5..8e15392a6f64 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>> @@ -1278,6 +1278,15 @@
>>                         #reset-cells = <1>;
>>                 };
>
>nit: the above node looks like the tail end of pdc reset controller.
>That has a unit address of b2e0000.  Your unit address is smaller than
>the the pdc reset controller so you should be above it, not below it.
>
Ok.
>
>> +               pdc: interrupt-controller@b220000 {
>
>nit: Maybe the label should be "pdc_intc" not just "pdc".  This is
>just the node for the interrupt controller, not the whole pdc, right?
>
>
>> +                       compatible = "qcom,sdm845-pdc";
>> +                       reg = <0xb220000 0x30000>;
>
>nit: apparently common practice for Quaclomm dts is to pad the address
>in the "reg" field to all 8 digits.  So the above should be:
>
>reg = <0x0b220000 0x30000>;
>
>NOTE: it's important to _not_ pad the unit address in the node name
>(so you got that right).  Only update the "reg".  For context:
>
Ok.

Will take care of these in the next spin.

Thanks,
Lina

>https://lkml.kernel.org/r/CAD=FV=WrvRH6QpaQ67yw2MFz8RP59ozkSfQC4+OAM_8fAbGZuw@mail.gmail.com
>
>
>-Doug

Re: [PATCH 5/7] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Lina Iyer]

On Thu, Dec 20 2018 at 13:03 -0700, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-12-19 14:11:03)
>> To allow GPIOs to wakeup the system from suspend or deep idle, the
>> wakeup capable GPIOs are setup in hierarchy with interrupts from the
>> wakeup-parent irqchip.
>>
>> In older SoC's, the TLMM will handover detection to the parent irqchip
>> and in newer SoC's, the parent irqchip may also be active as well as the
>> TLMM and therefore the GPIOs need to be masked at TLMM to avoid
>> duplicate interrupts. To enable both these configurations to exist,
>> allow the parent irqchip to dictate the TLMM irqchip's behavior when
>> masking/unmasking the interrupt.
>>
>> Signed-off-by: Stephen Boyd <sboyd@kernel.org>
>
>I don't think I gave a signed-off-by, so you need to ask to forge my
>sign off here. Please change it to be:
>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>
>and I'm not sure how much I wrote vs. you wrote anymore so perhaps also
>add a
>
> Co-developed-by: Stephen Boyd <swboyd@chromium.org>
>
I meant to do that. I will add this tag instead. Sorry about that.

>And finally, please just email my chromium.org email for this series
>because I apparently messed up the address once and now it's all going
>to the wrong inbox. Thanks!
>
Sure.

>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>
>Can you Cc Linus Walleij and Bjorn Andersson on the whole patch series
>next time? Would be good to have their review on major pinctrl driver
>changes.
>
Bjorn is already included. Will make sure to include Linus in the next
spin.

>> @@ -967,11 +994,86 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
>>         return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
>>  }
>>
>> +static int msm_gpio_domain_translate(struct irq_domain *d,
>> +                                    struct irq_fwspec *fwspec,
>> +                                    unsigned long *hwirq, unsigned int *type)
>> +{
>> +       if (is_of_node(fwspec->fwnode)) {
>> +               if (fwspec->param_count < 2)
>> +                       return -EINVAL;
>> +               *hwirq = fwspec->param[0];
>> +               *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
>> +               return 0;
>> +       }
>> +
>> +       return -EINVAL;
>> +}
>
>Maybe this can be a generic function in gpiolib?
>
>> +
>> +static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> +                                unsigned int nr_irqs, void *arg)
>> +{
>> +       int ret;
>> +       irq_hw_number_t hwirq;
>> +       struct gpio_chip *gc = domain->host_data;
>> +       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>> +       struct irq_fwspec *fwspec = arg;
>> +       struct qcom_irq_fwspec parent = { };
>> +       unsigned int type;
>> +
>> +       ret = msm_gpio_domain_translate(domain, fwspec, &hwirq, &type);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
>> +                                           &pctrl->irq_chip, gc);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       if (!domain->parent)
>> +               return 0;
>> +
>> +       parent.fwspec.fwnode      = domain->parent->fwnode;
>> +       parent.fwspec.param_count = 2;
>> +       parent.fwspec.param[0]    = hwirq;
>> +       parent.fwspec.param[1]    = type;
>> +
>> +       ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (parent.mask)
>> +               set_bit(hwirq, pctrl->wakeup_masked_irqs);
>> +
>> +       return 0;
>> +}
>> +
>> +/*
>> + * TODO: Get rid of this and push it into gpiochip_to_irq()
>
>Hmm.. yeah we need to do this still. I think we can have a generic two
>cell function similar to irq_domain_xlate_twocell() that does the fwspec
>creation and uses some of the things that we pass to
>gpiochip_irqchip_add(), like the default level type. This existing
>function is not good to have, so there's work to do to get rid of this.
>
>I was also thinking that maybe we can make the alloc function above take
>a struct gpio_irq_fwspec structure that tells the alloc function what
>gpiochip the irq is for. That would mean that we need to change the
>gpio_to_irq() function below to be generic and stuff the chip inside the
>fwspec wrapper structure:
>
>	struct gpio_irq_fwspec {
>		struct irq_fwspec fwspec;
>		struct gpio_chip *chip;
>		unsigned int offset;
>	};
>
>but I seem to recall that was not working for some reason.
>
I didn't attemp this. I am hoping we can solve this even after this
patch set separately. That said, I will try this.

>> + */
>> +static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       struct irq_fwspec fwspec;
>> +
>> +       fwspec.fwnode = of_node_to_fwnode(chip->of_node);
>> +       fwspec.param[0] = offset;
>> +       fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
>> +       fwspec.param_count = 2;
>> +
>> +       return irq_create_fwspec_mapping(&fwspec);
>> +}
>> +
>> +static const struct irq_domain_ops msm_gpio_domain_ops = {
>> +       .translate = msm_gpio_domain_translate,
>> +       .alloc     = msm_gpio_domain_alloc,
>> +       .free      = irq_domain_free_irqs_top,
>> +};
>> +

Thanks,
Lina

Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Rob Herring]

On Mon, Jan 7, 2019 at 12:51 PM Lina Iyer <ilina@codeaurora.org> wrote:
>
> On Fri, Dec 28 2018 at 17:07 -0700, Rob Herring wrote:
> >On Wed, Dec 19, 2018 at 03:11:02PM -0700, Lina Iyer wrote:
> >> SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO
> >> routed to the PDC as interrupts that can be used to wake the system up
> >> from deep low power modes and suspend.
> >>
> >> Cc: devicetree@vger.kernel.org
> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> >> ---
> >>  .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt    | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> >> index 665aadb5ea28..a522ca46667d 100644
> >> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> >> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> >> @@ -29,6 +29,11 @@ SDM845 platform.
> >>      Definition: must be 2. Specifying the pin number and flags, as defined
> >>                  in <dt-bindings/interrupt-controller/irq.h>
> >>
> >> +- wakeup-parent:
> >> +    Usage: optional
> >> +    Value type: <phandle>
> >> +    Definition: A phandle to the wakeup interrupt controller for the SoC.
> >
> >Is this really necessary? Is there more than one possible wakeup-parent
> >node?
> >
> No.  There is only one but depending on the architecture, the wakeup
> interrupt controller could be different device like PDC on SDM845 or MPM
> on SDM820.
>
> What do you have in mind? Let me know if you have a better idea than
> referencing in DT.

If there's only one possibility for a given platform, then you can
just use of_find_compatible_node(). I don't think it matters that
different platforms have a different device here. It's not going to be
a large table and you may need to know the differences if there's not
an abstracted interface to it (seems there is in your case).
Alternatively, if the PDC/MPM code knows what interrupt controller it
is associated with, then it could setup that relationship and the
interrupt controller code could retrieve that. Maybe the stacked
domain support doesn't work in that direction (I haven't looked at the
irq code much since that was added).

However, my main concern is documenting something genericish in a
device specific binding. It looks like Tegra is trying to add the same
thing, so this needs to be documented in a common place. One question
is whether wakeup is the only use or if this should be more generally
a secondary interrupt parent?

Rob

Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Lina Iyer]

On Tue, Jan 08 2019 at 07:49 -0700, Rob Herring wrote:
>On Mon, Jan 7, 2019 at 12:51 PM Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> On Fri, Dec 28 2018 at 17:07 -0700, Rob Herring wrote:
>> >On Wed, Dec 19, 2018 at 03:11:02PM -0700, Lina Iyer wrote:
>> >> SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO
>> >> routed to the PDC as interrupts that can be used to wake the system up
>> >> from deep low power modes and suspend.
>> >>
>> >> Cc: devicetree@vger.kernel.org
>> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> >> ---
>> >>  .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt    | 7 ++++++-
>> >>  1 file changed, 6 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
>> >> index 665aadb5ea28..a522ca46667d 100644
>> >> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
>> >> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
>> >> @@ -29,6 +29,11 @@ SDM845 platform.
>> >>      Definition: must be 2. Specifying the pin number and flags, as defined
>> >>                  in <dt-bindings/interrupt-controller/irq.h>
>> >>
>> >> +- wakeup-parent:
>> >> +    Usage: optional
>> >> +    Value type: <phandle>
>> >> +    Definition: A phandle to the wakeup interrupt controller for the SoC.
>> >
>> >Is this really necessary? Is there more than one possible wakeup-parent
>> >node?
>> >
>> No.  There is only one but depending on the architecture, the wakeup
>> interrupt controller could be different device like PDC on SDM845 or MPM
>> on SDM820.
>>
>> What do you have in mind? Let me know if you have a better idea than
>> referencing in DT.
>
>If there's only one possibility for a given platform, then you can
>just use of_find_compatible_node(). I don't think it matters that
>different platforms have a different device here. It's not going to be
>a large table and you may need to know the differences if there's not
>an abstracted interface to it (seems there is in your case).
The GPIO irqchip would be in hierarchy with the wakeup-parent
irqchip and no device specific functions would be called directly.
We could achieve this with compatible strings to the irqchip.

>Alternatively, if the PDC/MPM code knows what interrupt controller it
>is associated with, then it could setup that relationship and the
>interrupt controller code could retrieve that. Maybe the stacked
>domain support doesn't work in that direction (I haven't looked at the
>irq code much since that was added).
>
The PDC/MPM do not know about the association.

>However, my main concern is documenting something genericish in a
>device specific binding. It looks like Tegra is trying to add the same
>thing, so this needs to be documented in a common place. One question
>is whether wakeup is the only use or if this should be more generally
>a secondary interrupt parent?
>
Yes, wakeup is the only use of this interrupt parent. It is powered by
an always-on rail and therefore can detect some interrupts that are
routed to it even when the GIC is powered off. Though Tegra's
implementation of the irqchip is a bit different from QCOM, the idea is
generally the same. It would be helpful, if we could make this a
generic enough binding.

-- Lina

Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Rob Herring]

On Wed, Jan 9, 2019 at 11:31 AM Lina Iyer <ilina@codeaurora.org> wrote:
>
> On Tue, Jan 08 2019 at 07:49 -0700, Rob Herring wrote:
> >On Mon, Jan 7, 2019 at 12:51 PM Lina Iyer <ilina@codeaurora.org> wrote:
> >>
> >> On Fri, Dec 28 2018 at 17:07 -0700, Rob Herring wrote:
> >> >On Wed, Dec 19, 2018 at 03:11:02PM -0700, Lina Iyer wrote:
> >> >> SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO
> >> >> routed to the PDC as interrupts that can be used to wake the system up
> >> >> from deep low power modes and suspend.
> >> >>
> >> >> Cc: devicetree@vger.kernel.org
> >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> >> >> ---
> >> >>  .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt    | 7 ++++++-
> >> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> >> >> index 665aadb5ea28..a522ca46667d 100644
> >> >> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> >> >> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
> >> >> @@ -29,6 +29,11 @@ SDM845 platform.
> >> >>      Definition: must be 2. Specifying the pin number and flags, as defined
> >> >>                  in <dt-bindings/interrupt-controller/irq.h>
> >> >>
> >> >> +- wakeup-parent:
> >> >> +    Usage: optional
> >> >> +    Value type: <phandle>
> >> >> +    Definition: A phandle to the wakeup interrupt controller for the SoC.
> >> >
> >> >Is this really necessary? Is there more than one possible wakeup-parent
> >> >node?
> >> >
> >> No.  There is only one but depending on the architecture, the wakeup
> >> interrupt controller could be different device like PDC on SDM845 or MPM
> >> on SDM820.
> >>
> >> What do you have in mind? Let me know if you have a better idea than
> >> referencing in DT.
> >
> >If there's only one possibility for a given platform, then you can
> >just use of_find_compatible_node(). I don't think it matters that
> >different platforms have a different device here. It's not going to be
> >a large table and you may need to know the differences if there's not
> >an abstracted interface to it (seems there is in your case).
> The GPIO irqchip would be in hierarchy with the wakeup-parent
> irqchip and no device specific functions would be called directly.
> We could achieve this with compatible strings to the irqchip.
>
> >Alternatively, if the PDC/MPM code knows what interrupt controller it
> >is associated with, then it could setup that relationship and the
> >interrupt controller code could retrieve that. Maybe the stacked
> >domain support doesn't work in that direction (I haven't looked at the
> >irq code much since that was added).
> >
> The PDC/MPM do not know about the association.

Neither does the main interrupt controller. The association is part of
SoC integration. You can describe that association in either direction
and that is sufficient from a DT standpoint. You've probably picked
putting this in the GIC(?) based on what works more easily with the
Linux irqdomain code.

> >However, my main concern is documenting something genericish in a
> >device specific binding. It looks like Tegra is trying to add the same
> >thing, so this needs to be documented in a common place. One question
> >is whether wakeup is the only use or if this should be more generally
> >a secondary interrupt parent?
> >
> Yes, wakeup is the only use of this interrupt parent.

Maybe for you, but I was wondering about this more generally. Should
we encode what the function (e.g. wakeup) is in the property name or
have something like aux-interrupt-controller? Maybe some platforms
have some need for a secondary interrupt-controller which is not
wakeup. Routing interrupts to other cores perhaps?

> It is powered by
> an always-on rail and therefore can detect some interrupts that are
> routed to it even when the GIC is powered off. Though Tegra's
> implementation of the irqchip is a bit different from QCOM, the idea is
> generally the same. It would be helpful, if we could make this a
> generic enough binding.
>
> -- Lina
>

Re: [PATCH 3/7] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Stephen Boyd]

Quoting Lina Iyer (2019-01-07 10:48:36)
> On Thu, Dec 20 2018 at 13:19 -0700, Stephen Boyd wrote:
> >Quoting Lina Iyer (2018-12-19 14:11:01)
> 
> >> +static const struct pdc_gpio_pin_data sdm845_gpio_data = {
> >> +       .size = ARRAY_SIZE(sdm845_gpio_pdc_map),
> >> +       .map = sdm845_gpio_pdc_map,
> >> +};
> >> +
> >> +const struct of_device_id pdc_gpio_match_table[] = {
> >> +       { .compatible = "qcom,845-pdc", .data = &sdm845_gpio_data },
> >
> >Why not qcom,sdm845-pdc?
> >
> The compatible matches the compatible specified in the PDC driver. Not
> sure why the 'sdm' was left out at that time.

Are you going to add sdm?

> 
> >> +       { },
> >> +};
> >
> >I wonder why we wouldn't just put this all into the qcom-pdc.c file at
> >the bottom and then have that IRQCHIP_DECLARE() macros call small
> >functions that pass the pdc to gpio mapping table to qcom_pdc_init()
> >that takes a third argument?
> >
> We could. I feel we would be adding tables like this and it just
> clutters up the driver file. May be in the future we could move to
> target specific data file like the gpios, but that could be excessive
> too. Thought this might be a compromise. I am open to change.

Ok. The benefit to my approach is that we don't do the string match
twice. We do it once and sacrifice a little code space to jump to the
real init function with the data we want. We can then put those chip
tables inside some #ifdef to save space and allow configurations to turn
off everything in EXPERT mode but leave everything default enabled
otherwise.

> 
> >I really hope that in the future all the gpios can be wakeup capable so
> >that we don't need to have the table at all!
> >
> I doubt there are plans to support that in hardware. We should plan for
> supporting tables like this for other chipsets based on the PDC
> architecture.
> 

Uh oh. That's sad.

Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Stephen Boyd]

Quoting Rob Herring (2019-01-09 11:36:56)
> 
> > >However, my main concern is documenting something genericish in a
> > >device specific binding. It looks like Tegra is trying to add the same
> > >thing, so this needs to be documented in a common place. One question
> > >is whether wakeup is the only use or if this should be more generally
> > >a secondary interrupt parent?
> > >
> > Yes, wakeup is the only use of this interrupt parent.
> 
> Maybe for you, but I was wondering about this more generally. Should
> we encode what the function (e.g. wakeup) is in the property name or
> have something like aux-interrupt-controller? Maybe some platforms
> have some need for a secondary interrupt-controller which is not
> wakeup. Routing interrupts to other cores perhaps?
> 

I'd say it's not the interrupt-parent, but a secondary-interrupt-parent,
because it's another path that some GPIO interrupts will go through vs.
the "normal" summary irq line that uses the interrupt-parent. Maybe
that's similar to the interrupt partitioning that ARM is doing for PPIs
that only go to some CPUs?

We don't really specify that some GPIO is corresponding to the secondary
or primary interrupt controller for the GPIO controller in DT. If we
did, then we could do something like the interrupt-map binding and have
the index of that property be the gpio number and the interrupt parent
that it maps to (either summary from the GIC or MPM pin number).

 interrupt-map = <0 0 &gic GIC_SPI 208 0>,
                 <1 0 &pdc 3 0>;
 interrupt-map-mask = <0xfffffff 0>;

And then we would pass the 2-cell GPIO interrupt specifier (gpio# and
flags) through the table and remap it to arbitrary domain parents. We
could use this same design for the SSBI and SPMI gpio interrupt
controller where we're currently looking at hardcoding the base
interrupt number in the driver (0xc0) and then adding the GPIO number to
that to get the parent interrupt specifier.

It's sort of an abuse of interrupt-map, but I don't know if it really
matters because there isn't a child of the gpio controller that is going
to go through this table.

Re: [PATCH 3/7] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Lina Iyer]

On Fri, Jan 11 2019 at 15:55 -0700, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-01-07 10:48:36)
>> On Thu, Dec 20 2018 at 13:19 -0700, Stephen Boyd wrote:
>> >Quoting Lina Iyer (2018-12-19 14:11:01)
>>
>> >> +static const struct pdc_gpio_pin_data sdm845_gpio_data = {
>> >> +       .size = ARRAY_SIZE(sdm845_gpio_pdc_map),
>> >> +       .map = sdm845_gpio_pdc_map,
>> >> +};
>> >> +
>> >> +const struct of_device_id pdc_gpio_match_table[] = {
>> >> +       { .compatible = "qcom,845-pdc", .data = &sdm845_gpio_data },
>> >
>> >Why not qcom,sdm845-pdc?
>> >
>> The compatible matches the compatible specified in the PDC driver. Not
>> sure why the 'sdm' was left out at that time.
>
>Are you going to add sdm?
>
Realized this is a bug. Will fix.
>>
>> >> +       { },
>> >> +};
>> >
>> >I wonder why we wouldn't just put this all into the qcom-pdc.c file at
>> >the bottom and then have that IRQCHIP_DECLARE() macros call small
>> >functions that pass the pdc to gpio mapping table to qcom_pdc_init()
>> >that takes a third argument?
>> >
>> We could. I feel we would be adding tables like this and it just
>> clutters up the driver file. May be in the future we could move to
>> target specific data file like the gpios, but that could be excessive
>> too. Thought this might be a compromise. I am open to change.
>
>Ok. The benefit to my approach is that we don't do the string match
>twice. We do it once and sacrifice a little code space to jump to the
>real init function with the data we want. We can then put those chip
>tables inside some #ifdef to save space and allow configurations to turn
>off everything in EXPERT mode but leave everything default enabled
>otherwise.
>
Sure. I will use this idea.

>>
>> >I really hope that in the future all the gpios can be wakeup capable so
>> >that we don't need to have the table at all!
>> >
>> I doubt there are plans to support that in hardware. We should plan for
>> supporting tables like this for other chipsets based on the PDC
>> architecture.
>>
>
>Uh oh. That's sad.
>

Re: [PATCH 5/7] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Lina Iyer]

On Thu, Dec 20 2018 at 13:03 -0700, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-12-19 14:11:03)
>> +
>> +static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> +                                unsigned int nr_irqs, void *arg)
>> +{
>> +       int ret;
>> +       irq_hw_number_t hwirq;
>> +       struct gpio_chip *gc = domain->host_data;
>> +       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>> +       struct irq_fwspec *fwspec = arg;
>> +       struct qcom_irq_fwspec parent = { };
>> +       unsigned int type;
>> +
>> +       ret = msm_gpio_domain_translate(domain, fwspec, &hwirq, &type);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
>> +                                           &pctrl->irq_chip, gc);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       if (!domain->parent)
>> +               return 0;
>> +
>> +       parent.fwspec.fwnode      = domain->parent->fwnode;
>> +       parent.fwspec.param_count = 2;
>> +       parent.fwspec.param[0]    = hwirq;
>> +       parent.fwspec.param[1]    = type;
>> +
>> +       ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (parent.mask)
>> +               set_bit(hwirq, pctrl->wakeup_masked_irqs);
>> +
>> +       return 0;
>> +}
>> +
>> +/*
>> + * TODO: Get rid of this and push it into gpiochip_to_irq()
>
>Hmm.. yeah we need to do this still. I think we can have a generic two
>cell function similar to irq_domain_xlate_twocell() that does the fwspec
>creation and uses some of the things that we pass to
>gpiochip_irqchip_add(), like the default level type. This existing
>function is not good to have, so there's work to do to get rid of this.
>

>I was also thinking that maybe we can make the alloc function above take
>a struct gpio_irq_fwspec structure that tells the alloc function what
>gpiochip the irq is for. That would mean that we need to change the
>gpio_to_irq() function below to be generic and stuff the chip inside the
>fwspec wrapper structure:
>
>	struct gpio_irq_fwspec {
>		struct irq_fwspec fwspec;
>		struct gpio_chip *chip;
>		unsigned int offset;
>	};
>
>but I seem to recall that was not working for some reason.
>
I was thinking about this. If I understand you correctly, we want to
generalize the .translate and .alloc functions. We could move the
.translate to generic however, the alloc would still need to be specific
for the parent.mask. But we can do this without the gpio_irq_fwspec. I
presume you suggest this structure so we could pass the hwirq and type
to the .alloc function. but we have that in the fwspec. What am I
missing?

>> + */
>> +static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       struct irq_fwspec fwspec;
>> +
>> +       fwspec.fwnode = of_node_to_fwnode(chip->of_node);
>> +       fwspec.param[0] = offset;
>> +       fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
>> +       fwspec.param_count = 2;
>> +
>> +       return irq_create_fwspec_mapping(&fwspec);
>> +}
>> +
>> +static const struct irq_domain_ops msm_gpio_domain_ops = {
>> +       .translate = msm_gpio_domain_translate,
>> +       .alloc     = msm_gpio_domain_alloc,
>> +       .free      = irq_domain_free_irqs_top,
>> +};
>> +

Re: [PATCH 6/7] arm64: dts: msm: add PDC device bindings for sdm845

[Doug Anderson]

Hi,

On Mon, Jan 7, 2019 at 10:52 AM Lina Iyer <ilina@codeaurora.org> wrote:
> >> +                       compatible = "qcom,sdm845-pdc";
> >> +                       reg = <0xb220000 0x30000>;
> >
> >nit: apparently common practice for Quaclomm dts is to pad the address
> >in the "reg" field to all 8 digits.  So the above should be:
> >
> >reg = <0x0b220000 0x30000>;
> >
> >NOTE: it's important to _not_ pad the unit address in the node name
> >(so you got that right).  Only update the "reg".  For context:
> >
> Ok.
>
> Will take care of these in the next spin.

This will now need to be adjusted to:

reg = <0 0x0b220000 0 0x30000>;

This fixes things up to deal with the patch ("arm64: dts: qcom:
sdm845: Increase address and size cells for soc"), AKA:
- https://patchwork.kernel.org/patch/10767511/
- https://lkml.kernel.org/r/20190117042940.25487-2-bjorn.andersson@linaro.org

Thanks!

-Doug

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

[Doug Anderson]

Hi,

On Wed, Dec 19, 2018 at 2:12 PM Lina Iyer <ilina@codeaurora.org> 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>
> ---

Drive-by-nit: If you are re-posting someone else's patch you need to
add your Signed-off-by.

-Doug

Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Stephen Boyd]

Quoting Stephen Boyd (2019-01-11 15:20:48)
> Quoting Rob Herring (2019-01-09 11:36:56)
> > 
> > > >However, my main concern is documenting something genericish in a
> > > >device specific binding. It looks like Tegra is trying to add the same
> > > >thing, so this needs to be documented in a common place. One question
> > > >is whether wakeup is the only use or if this should be more generally
> > > >a secondary interrupt parent?
> > > >
> > > Yes, wakeup is the only use of this interrupt parent.
> > 
> > Maybe for you, but I was wondering about this more generally. Should
> > we encode what the function (e.g. wakeup) is in the property name or
> > have something like aux-interrupt-controller? Maybe some platforms
> > have some need for a secondary interrupt-controller which is not
> > wakeup. Routing interrupts to other cores perhaps?
> > 
> 
> I'd say it's not the interrupt-parent, but a secondary-interrupt-parent,
> because it's another path that some GPIO interrupts will go through vs.
> the "normal" summary irq line that uses the interrupt-parent. Maybe
> that's similar to the interrupt partitioning that ARM is doing for PPIs
> that only go to some CPUs?
> 
> We don't really specify that some GPIO is corresponding to the secondary
> or primary interrupt controller for the GPIO controller in DT. If we
> did, then we could do something like the interrupt-map binding and have
> the index of that property be the gpio number and the interrupt parent
> that it maps to (either summary from the GIC or MPM pin number).
> 
>  interrupt-map = <0 0 &gic GIC_SPI 208 0>,
>                  <1 0 &pdc 3 0>;
>  interrupt-map-mask = <0xfffffff 0>;
> 
> And then we would pass the 2-cell GPIO interrupt specifier (gpio# and
> flags) through the table and remap it to arbitrary domain parents. We
> could use this same design for the SSBI and SPMI gpio interrupt
> controller where we're currently looking at hardcoding the base
> interrupt number in the driver (0xc0) and then adding the GPIO number to
> that to get the parent interrupt specifier.
> 
> It's sort of an abuse of interrupt-map, but I don't know if it really
> matters because there isn't a child of the gpio controller that is going
> to go through this table.
> 

Rob, can you please respond?

Re: [PATCH 5/7] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Stephen Boyd]

Quoting Lina Iyer (2019-01-16 15:13:28)
> On Thu, Dec 20 2018 at 13:03 -0700, Stephen Boyd wrote:
> >Quoting Lina Iyer (2018-12-19 14:11:03)
> >> +
> >> +static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
> >> +                                unsigned int nr_irqs, void *arg)
> >> +{
> >> +       int ret;
> >> +       irq_hw_number_t hwirq;
> >> +       struct gpio_chip *gc = domain->host_data;
> >> +       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> >> +       struct irq_fwspec *fwspec = arg;
> >> +       struct qcom_irq_fwspec parent = { };
> >> +       unsigned int type;
> >> +
> >> +       ret = msm_gpio_domain_translate(domain, fwspec, &hwirq, &type);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> >> +                                           &pctrl->irq_chip, gc);
> >> +       if (ret < 0)
> >> +               return ret;
> >> +
> >> +       if (!domain->parent)
> >> +               return 0;
> >> +
> >> +       parent.fwspec.fwnode      = domain->parent->fwnode;
> >> +       parent.fwspec.param_count = 2;
> >> +       parent.fwspec.param[0]    = hwirq;
> >> +       parent.fwspec.param[1]    = type;
> >> +
> >> +       ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +       if (parent.mask)
> >> +               set_bit(hwirq, pctrl->wakeup_masked_irqs);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +/*
> >> + * TODO: Get rid of this and push it into gpiochip_to_irq()
> >
> >Hmm.. yeah we need to do this still. I think we can have a generic two
> >cell function similar to irq_domain_xlate_twocell() that does the fwspec
> >creation and uses some of the things that we pass to
> >gpiochip_irqchip_add(), like the default level type. This existing
> >function is not good to have, so there's work to do to get rid of this.
> >
> 
> >I was also thinking that maybe we can make the alloc function above take
> >a struct gpio_irq_fwspec structure that tells the alloc function what
> >gpiochip the irq is for. That would mean that we need to change the
> >gpio_to_irq() function below to be generic and stuff the chip inside the
> >fwspec wrapper structure:
> >
> >       struct gpio_irq_fwspec {
> >               struct irq_fwspec fwspec;
> >               struct gpio_chip *chip;
> >               unsigned int offset;
> >       };
> >
> >but I seem to recall that was not working for some reason.
> >
> I was thinking about this. If I understand you correctly, we want to
> generalize the .translate and .alloc functions. We could move the
> .translate to generic however, the alloc would still need to be specific
> for the parent.mask. But we can do this without the gpio_irq_fwspec. I
> presume you suggest this structure so we could pass the hwirq and type
> to the .alloc function. but we have that in the fwspec. What am I
> missing?

I'm trying to flatten the irq number space into a single integer while
letting it cross multiple gpio chips. Similar to how gpiolib flattens
the gpio number space across multiple gpio chips. The goal being to make
gpiochip_to_irq() do this all for us without having to implement it in
each gpio driver, but maybe that isn't needed or wise if the goal is to
move drivers away from taking a gpio number and converting it into an
irq with gpio_to_irq() to begin with.

FWIW, the SPMI PMIC gpio chip conversion patches didn't fix this problem
and Brian just called irq_create_fwspec_mapping() from the to_irq()
hook. So maybe this can be cleaned up later.

Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Stephen Boyd]

Quoting Stephen Boyd (2019-01-23 12:52:09)
> Quoting Stephen Boyd (2019-01-11 15:20:48)
> > Quoting Rob Herring (2019-01-09 11:36:56)
> > > 
> > > > >However, my main concern is documenting something genericish in a
> > > > >device specific binding. It looks like Tegra is trying to add the same
> > > > >thing, so this needs to be documented in a common place. One question
> > > > >is whether wakeup is the only use or if this should be more generally
> > > > >a secondary interrupt parent?
> > > > >
> > > > Yes, wakeup is the only use of this interrupt parent.
> > > 
> > > Maybe for you, but I was wondering about this more generally. Should
> > > we encode what the function (e.g. wakeup) is in the property name or
> > > have something like aux-interrupt-controller? Maybe some platforms
> > > have some need for a secondary interrupt-controller which is not
> > > wakeup. Routing interrupts to other cores perhaps?
> > > 
> > 
> > I'd say it's not the interrupt-parent, but a secondary-interrupt-parent,
> > because it's another path that some GPIO interrupts will go through vs.
> > the "normal" summary irq line that uses the interrupt-parent. Maybe
> > that's similar to the interrupt partitioning that ARM is doing for PPIs
> > that only go to some CPUs?
> > 
> > We don't really specify that some GPIO is corresponding to the secondary
> > or primary interrupt controller for the GPIO controller in DT. If we
> > did, then we could do something like the interrupt-map binding and have
> > the index of that property be the gpio number and the interrupt parent
> > that it maps to (either summary from the GIC or MPM pin number).
> > 
> >  interrupt-map = <0 0 &gic GIC_SPI 208 0>,
> >                  <1 0 &pdc 3 0>;
> >  interrupt-map-mask = <0xfffffff 0>;
> > 
> > And then we would pass the 2-cell GPIO interrupt specifier (gpio# and
> > flags) through the table and remap it to arbitrary domain parents. We
> > could use this same design for the SSBI and SPMI gpio interrupt
> > controller where we're currently looking at hardcoding the base
> > interrupt number in the driver (0xc0) and then adding the GPIO number to
> > that to get the parent interrupt specifier.
> > 
> > It's sort of an abuse of interrupt-map, but I don't know if it really
> > matters because there isn't a child of the gpio controller that is going
> > to go through this table.
> > 
> 
> Rob, can you please respond?
> 

Thinking more about this it doesn't seem too beneficial to use the
interrupt-map binding to figure out the parent domain. When we create
the irqdomain in the gpio controller, we have to specify the parent
domain at the same time, and there can only be one parent of an
irqdomain. Furthermore, we can have many irqdomains per device node, and
the DT binding doesn't indicate which irqdomain we want to parent to,
just the device node for the parent.

The nice part about using a DT binding to map the incoming irq specifier
to the parent specifier is that we don't have to put that mapping
somewhere in the driver. Instead, we can look it up in DT. This is
especially helpful with these qcom devices where they seem to randomly
assign gpios to PDC pins, and change it every time they make a new SoC.
Having those data tables in the kernel is annoying to maintain, and
having the child to parent hwirq numbers in DT isn't too great either
when it's just a bunch of numbers:

	<0 208>, <1 3>, etc.

It makes more sense when we at least have the parent phandle and can
assume the incoming irq specifier is for the current node.

	<[#interrupt-cells] [phandle to parent to irq remap] [parents #interrupt-cells]>

	<0 0 &pdc 208 0>, <1 0 &pdc 3 0>, etc.

But then, the parent phandle is always the same because a domain can't
have more than one parent.

In theory, we could also do simple trigger type inverting or collapsing
if we wanted to while translating the irq specifier to the parent.
Currently, drivers do that with some code to translate the specifier to
a hwirq and flags and then overwrite the incoming flags from the child
to be what the parent can accept.

So should we make some new binding like 'irqdomain-<foo>-map' that maps
the irq specifiers coming into the containing node's 'foo' irqdomain to
a parent's irq specifier, instead of writing that in each irqchip
driver? Or is this too verbose because each irq needs to be specified in
the mapping table?

I'm prototyping out some code to do the remapping based on this type of
DT property, because it will make the irqdomain::alloc function a little
simpler to implement by passing in a struct irq_fwspec and getting out a
parent irq_fwspec and it will make the patches to add these mapping
tables in C irrelevant. Plus, I think Lina will be happy that the
pinctrl driver will know if some pin maps to the PDC or not without
having to see if it fails to allocate in the parent irqdomain. But there
will still need to be a property for 'wakeup-parent' unless we do
something to expose irqdomain tokens into DT.

Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Stephen Boyd]

Quoting Stephen Boyd (2019-01-31 13:53:42)
> 
> I'm prototyping out some code to do the remapping based on this type of
> DT property, because it will make the irqdomain::alloc function a little
> simpler to implement by passing in a struct irq_fwspec and getting out a
> parent irq_fwspec and it will make the patches to add these mapping
> tables in C irrelevant. Plus, I think Lina will be happy that the
> pinctrl driver will know if some pin maps to the PDC or not without
> having to see if it fails to allocate in the parent irqdomain. But there
> will still need to be a property for 'wakeup-parent' unless we do
> something to expose irqdomain tokens into DT.
> 

And here's the code to do this remapping idea, heavily based on the
phandle remapping code. I didn't properly fix up the irq alloc function
for the pinctrl driver here, but it should work out properly without
much more diff. I realize now that the pass-thru mechanism will fail
horribly when a specifier changes size types. I guess we'll need to keep
that in mind if we convert the PDC driver to this too. Or we can leave
the PDC driver as is and not worry about listing out the individual
pins.

-----8<-----
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index a2241dd9c185..6b3a4227f433 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1213,6 +1213,83 @@
 			#interrupt-cells = <2>;
 			gpio-ranges = <&tlmm 0 0 150>;
 			wakeup-parent = <&pdc_intc>;
+			irqdomain-map = <1 0 &pdc_intc 30 0>,
+					<3 0 &pdc_intc 31 0>,
+					<5 0 &pdc_intc 32 0>,
+					<10 0 &pdc_intc 33 0>,
+					<11 0 &pdc_intc 34 0>,
+					<20 0 &pdc_intc 35 0>,
+					<22 0 &pdc_intc 36 0>,
+					<24 0 &pdc_intc 37 0>,
+					<26 0 &pdc_intc 38 0>,
+					<30 0 &pdc_intc 39 0>,
+					<31 0 &pdc_intc 117 0>,
+					<32 0 &pdc_intc 41 0>,
+					<34 0 &pdc_intc 42 0>,
+					<36 0 &pdc_intc 43 0>,
+					<37 0 &pdc_intc 44 0>,
+					<38 0 &pdc_intc 45 0>,
+					<39 0 &pdc_intc 46 0>,
+					<40 0 &pdc_intc 47 0>,
+					<41 0 &pdc_intc 115 0>,
+					<43 0 &pdc_intc 49 0>,
+					<44 0 &pdc_intc 50 0>,
+					<46 0 &pdc_intc 51 0>,
+					<48 0 &pdc_intc 52 0>,
+					<49 0 &pdc_intc 118 0>,
+					<52 0 &pdc_intc 54 0>,
+					<53 0 &pdc_intc 55 0>,
+					<54 0 &pdc_intc 56 0>,
+					<56 0 &pdc_intc 57 0>,
+					<57 0 &pdc_intc 58 0>,
+					<58 0 &pdc_intc 59 0>,
+					<59 0 &pdc_intc 60 0>,
+					<60 0 &pdc_intc 61 0>,
+					<61 0 &pdc_intc 62 0>,
+					<62 0 &pdc_intc 63 0>,
+					<63 0 &pdc_intc 64 0>,
+					<64 0 &pdc_intc 65 0>,
+					<66 0 &pdc_intc 66 0>,
+					<68 0 &pdc_intc 67 0>,
+					<71 0 &pdc_intc 68 0>,
+					<73 0 &pdc_intc 69 0>,
+					<77 0 &pdc_intc 70 0>,
+					<78 0 &pdc_intc 71 0>,
+					<79 0 &pdc_intc 72 0>,
+					<80 0 &pdc_intc 73 0>,
+					<84 0 &pdc_intc 74 0>,
+					<85 0 &pdc_intc 75 0>,
+					<86 0 &pdc_intc 76 0>,
+					<88 0 &pdc_intc 77 0>,
+					<89 0 &pdc_intc 116 0>,
+					<91 0 &pdc_intc 79 0>,
+					<92 0 &pdc_intc 80 0>,
+					<95 0 &pdc_intc 81 0>,
+					<96 0 &pdc_intc 82 0>,
+					<97 0 &pdc_intc 83 0>,
+					<101 0 &pdc_intc 84 0>,
+					<103 0 &pdc_intc 85 0>,
+					<104 0 &pdc_intc 86 0>,
+					<115 0 &pdc_intc 90 0>,
+					<116 0 &pdc_intc 91 0>,
+					<117 0 &pdc_intc 92 0>,
+					<118 0 &pdc_intc 93 0>,
+					<119 0 &pdc_intc 94 0>,
+					<120 0 &pdc_intc 95 0>,
+					<121 0 &pdc_intc 96 0>,
+					<122 0 &pdc_intc 97 0>,
+					<123 0 &pdc_intc 98 0>,
+					<124 0 &pdc_intc 99 0>,
+					<125 0 &pdc_intc 100 0>,
+					<127 0 &pdc_intc 102 0>,
+					<128 0 &pdc_intc 103 0>,
+					<129 0 &pdc_intc 104 0>,
+					<130 0 &pdc_intc 105 0>,
+					<132 0 &pdc_intc 106 0>,
+					<133 0 &pdc_intc 107 0>,
+					<145 0 &pdc_intc 108 0>;
+			irqdomain-map-mask = <0xff 0>;
+			irqdomain-map-pass-thru = <0 0xff>;
 
 			qspi_clk: qspi-clk {
 				pinmux {
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 02ad93a304a4..b37f4cdfda53 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -274,6 +274,130 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
 }
 EXPORT_SYMBOL_GPL(of_irq_parse_raw);
 
+int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out)
+{
+	char *stem_name;
+	char *cells_name, *map_name = NULL, *mask_name = NULL;
+	char *pass_name = NULL;
+	struct device_node *cur, *new = NULL;
+	const __be32 *map, *mask, *pass;
+	static const __be32 dummy_mask[] = { [0 ... MAX_PHANDLE_ARGS] = ~0 };
+	static const __be32 dummy_pass[] = { [0 ... MAX_PHANDLE_ARGS] = 0 };
+	__be32 initial_match_array[MAX_PHANDLE_ARGS];
+	const __be32 *match_array = initial_match_array;
+	int i, ret, map_len, match;
+	u32 in_size, out_size;
+
+	stem_name = "";
+	cells_name = "#interrupt-cells";
+
+	ret = -ENOMEM;
+	map_name = kasprintf(GFP_KERNEL, "irqdomain%s-map", stem_name);
+	if (!map_name)
+		goto free;
+
+	mask_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-mask", stem_name);
+	if (!mask_name)
+		goto free;
+
+	pass_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-pass-thru", stem_name);
+	if (!pass_name)
+		goto free;
+
+	/* Get the #interrupt-cells property */
+	cur = to_of_node(in->fwnode);
+	ret = of_property_read_u32(cur, cells_name, &in_size);
+	if (ret < 0)
+		goto put;
+
+	/* Precalculate the match array - this simplifies match loop */
+	for (i = 0; i < in_size; i++)
+		initial_match_array[i] = cpu_to_be32(in->param[i]);
+
+	ret = -EINVAL;
+	/* Get the irqdomain-map property */
+	map = of_get_property(cur, map_name, &map_len);
+	if (!map) {
+		ret = 0;
+		goto free;
+	}
+	map_len /= sizeof(u32);
+
+	/* Get the irqdomain-map-mask property (optional) */
+	mask = of_get_property(cur, mask_name, NULL);
+	if (!mask)
+		mask = dummy_mask;
+	/* Iterate through irqdomain-map property */
+	match = 0;
+	while (map_len > (in_size + 1) && !match) {
+		/* Compare specifiers */
+		match = 1;
+		for (i = 0; i < in_size; i++, map_len--)
+			match &= !((match_array[i] ^ *map++) & mask[i]);
+
+		of_node_put(new);
+		new = of_find_node_by_phandle(be32_to_cpup(map));
+		map++;
+		map_len--;
+
+		/* Check if not found */
+		if (!new)
+			goto put;
+
+		if (!of_device_is_available(new))
+			match = 0;
+
+		ret = of_property_read_u32(new, cells_name, &out_size);
+		if (ret)
+			goto put;
+
+		/* Check for malformed properties */
+		if (WARN_ON(out_size > MAX_PHANDLE_ARGS))
+			goto put;
+		if (map_len < out_size)
+			goto put;
+
+		/* Move forward by new node's #interrupt-cells amount */
+		map += out_size;
+		map_len -= out_size;
+	}
+	if (match) {
+		/* Get the irqdomain-map-pass-thru property (optional) */
+		pass = of_get_property(cur, pass_name, NULL);
+		if (!pass)
+			pass = dummy_pass;
+
+		/*
+		 * Successfully parsed a irqdomain-map translation; copy new
+		 * specifier into the out structure, keeping the
+		 * bits specified in irqdomain-map-pass-thru.
+		 */
+		match_array = map - out_size;
+		for (i = 0; i < out_size; i++) {
+			__be32 val = *(map - out_size + i);
+
+			out->param[i] = in->param[i];
+			if (i < in_size) {
+				val &= ~pass[i];
+				val |= cpu_to_be32(out->param[i]) & pass[i];
+			}
+
+			out->param[i] = be32_to_cpu(val);
+		}
+		out->param_count = in_size = out_size;
+		out->fwnode = of_node_to_fwnode(new);
+	}
+put:
+	of_node_put(new);
+free:
+	kfree(mask_name);
+	kfree(map_name);
+	kfree(pass_name);
+
+	return ret;
+}
+EXPORT_SYMBOL(of_irq_domain_map);
+
 /**
  * of_irq_parse_one - Resolve an interrupt for a device
  * @device: the device whose interrupt is to be resolved
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 9b45219893bd..0473b180dc8d 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1011,6 +1011,12 @@ static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	if (!domain->parent)
 		return 0;
 
+	ret = of_irq_domain_map(fwspec, &parent.fwspec);
+	if (ret)
+		return ret;
+
+	pr_info("incoming is %d %#x and outgoing is %d %#x\n", fwspec->param[0], fwspec->param[1], parent.fwspec.param[0], parent.fwspec.param[1]);
+
 	parent.fwspec.fwnode      = domain->parent->fwnode;
 	parent.fwspec.param_count = 2;
 	parent.fwspec.param[0]    = hwirq;
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 1214cabb2247..80ba1e9be68d 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -32,6 +32,8 @@ static inline int of_irq_parse_oldworld(struct device_node *device, int index,
 }
 #endif /* CONFIG_PPC32 && CONFIG_PPC_PMAC */
 
+extern int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out);
+
 extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq);
 extern int of_irq_parse_one(struct device_node *device, int index,
 			  struct of_phandle_args *out_irq);

Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Lina Iyer]

Thanks for the patch Stephen. Sorry, it took a while to get to this and
understand how this works.

On Thu, Jan 31 2019 at 00:10 -0700, Stephen Boyd wrote:
>Quoting Stephen Boyd (2019-01-31 13:53:42)
>>
>> I'm prototyping out some code to do the remapping based on this type of
>> DT property, because it will make the irqdomain::alloc function a little
>> simpler to implement by passing in a struct irq_fwspec and getting out a
>> parent irq_fwspec and it will make the patches to add these mapping
>> tables in C irrelevant. Plus, I think Lina will be happy that the
>> pinctrl driver will know if some pin maps to the PDC or not without
>> having to see if it fails to allocate in the parent irqdomain. But there
>> will still need to be a property for 'wakeup-parent' unless we do
>> something to expose irqdomain tokens into DT.
>>
>
>And here's the code to do this remapping idea, heavily based on the
>phandle remapping code. I didn't properly fix up the irq alloc function
>for the pinctrl driver here, but it should work out properly without
>much more diff. I realize now that the pass-thru mechanism will fail
>horribly when a specifier changes size types.
Could you explain?
>I guess we'll need to keep
>that in mind if we convert the PDC driver to this too. Or we can leave
>the PDC driver as is and not worry about listing out the individual
>pins.
The PDC pins are more sequential and it looks clean as it. I would
prefer that it be left as is.
>
>-----8<-----
>diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>index a2241dd9c185..6b3a4227f433 100644
>--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>@@ -1213,6 +1213,83 @@
> 			#interrupt-cells = <2>;
> 			gpio-ranges = <&tlmm 0 0 150>;
> 			wakeup-parent = <&pdc_intc>;
>+			irqdomain-map = <1 0 &pdc_intc 30 0>,
>+					<3 0 &pdc_intc 31 0>,
>+					<5 0 &pdc_intc 32 0>,
>+					<10 0 &pdc_intc 33 0>,
>+					<11 0 &pdc_intc 34 0>,
>+					<20 0 &pdc_intc 35 0>,
>+					<22 0 &pdc_intc 36 0>,
>+					<24 0 &pdc_intc 37 0>,
>+					<26 0 &pdc_intc 38 0>,
>+					<30 0 &pdc_intc 39 0>,
>+					<31 0 &pdc_intc 117 0>,
>+					<32 0 &pdc_intc 41 0>,
>+					<34 0 &pdc_intc 42 0>,
>+					<36 0 &pdc_intc 43 0>,
>+					<37 0 &pdc_intc 44 0>,
>+					<38 0 &pdc_intc 45 0>,
>+					<39 0 &pdc_intc 46 0>,
>+					<40 0 &pdc_intc 47 0>,
>+					<41 0 &pdc_intc 115 0>,
>+					<43 0 &pdc_intc 49 0>,
>+					<44 0 &pdc_intc 50 0>,
>+					<46 0 &pdc_intc 51 0>,
>+					<48 0 &pdc_intc 52 0>,
>+					<49 0 &pdc_intc 118 0>,
>+					<52 0 &pdc_intc 54 0>,
>+					<53 0 &pdc_intc 55 0>,
>+					<54 0 &pdc_intc 56 0>,
>+					<56 0 &pdc_intc 57 0>,
>+					<57 0 &pdc_intc 58 0>,
>+					<58 0 &pdc_intc 59 0>,
>+					<59 0 &pdc_intc 60 0>,
>+					<60 0 &pdc_intc 61 0>,
>+					<61 0 &pdc_intc 62 0>,
>+					<62 0 &pdc_intc 63 0>,
>+					<63 0 &pdc_intc 64 0>,
>+					<64 0 &pdc_intc 65 0>,
>+					<66 0 &pdc_intc 66 0>,
>+					<68 0 &pdc_intc 67 0>,
>+					<71 0 &pdc_intc 68 0>,
>+					<73 0 &pdc_intc 69 0>,
>+					<77 0 &pdc_intc 70 0>,
>+					<78 0 &pdc_intc 71 0>,
>+					<79 0 &pdc_intc 72 0>,
>+					<80 0 &pdc_intc 73 0>,
>+					<84 0 &pdc_intc 74 0>,
>+					<85 0 &pdc_intc 75 0>,
>+					<86 0 &pdc_intc 76 0>,
>+					<88 0 &pdc_intc 77 0>,
>+					<89 0 &pdc_intc 116 0>,
>+					<91 0 &pdc_intc 79 0>,
>+					<92 0 &pdc_intc 80 0>,
>+					<95 0 &pdc_intc 81 0>,
>+					<96 0 &pdc_intc 82 0>,
>+					<97 0 &pdc_intc 83 0>,
>+					<101 0 &pdc_intc 84 0>,
>+					<103 0 &pdc_intc 85 0>,
>+					<104 0 &pdc_intc 86 0>,
>+					<115 0 &pdc_intc 90 0>,
>+					<116 0 &pdc_intc 91 0>,
>+					<117 0 &pdc_intc 92 0>,
>+					<118 0 &pdc_intc 93 0>,
>+					<119 0 &pdc_intc 94 0>,
>+					<120 0 &pdc_intc 95 0>,
>+					<121 0 &pdc_intc 96 0>,
>+					<122 0 &pdc_intc 97 0>,
>+					<123 0 &pdc_intc 98 0>,
>+					<124 0 &pdc_intc 99 0>,
>+					<125 0 &pdc_intc 100 0>,
>+					<127 0 &pdc_intc 102 0>,
>+					<128 0 &pdc_intc 103 0>,
>+					<129 0 &pdc_intc 104 0>,
>+					<130 0 &pdc_intc 105 0>,
>+					<132 0 &pdc_intc 106 0>,
>+					<133 0 &pdc_intc 107 0>,
>+					<145 0 &pdc_intc 108 0>;
>+			irqdomain-map-mask = <0xff 0>;
>+			irqdomain-map-pass-thru = <0 0xff>;
Where do we document these bindings?
>
> 			qspi_clk: qspi-clk {
> 				pinmux {
>diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>index 02ad93a304a4..b37f4cdfda53 100644
>--- a/drivers/of/irq.c
>+++ b/drivers/of/irq.c
>@@ -274,6 +274,130 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
> }
> EXPORT_SYMBOL_GPL(of_irq_parse_raw);
>
I would think this would be a separate patch and I presume, I can add
you as the author with your signed-off-by?

>+int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out)
>+{
>+	char *stem_name;
>+	char *cells_name, *map_name = NULL, *mask_name = NULL;
>+	char *pass_name = NULL;
>+	struct device_node *cur, *new = NULL;
>+	const __be32 *map, *mask, *pass;
>+	static const __be32 dummy_mask[] = { [0 ... MAX_PHANDLE_ARGS] = ~0 };
>+	static const __be32 dummy_pass[] = { [0 ... MAX_PHANDLE_ARGS] = 0 };
>+	__be32 initial_match_array[MAX_PHANDLE_ARGS];
>+	const __be32 *match_array = initial_match_array;
>+	int i, ret, map_len, match;
>+	u32 in_size, out_size;
>+
>+	stem_name = "";
>+	cells_name = "#interrupt-cells";
>+
>+	ret = -ENOMEM;
>+	map_name = kasprintf(GFP_KERNEL, "irqdomain%s-map", stem_name);
>+	if (!map_name)
>+		goto free;
>+
>+	mask_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-mask", stem_name);
>+	if (!mask_name)
>+		goto free;
>+
>+	pass_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-pass-thru", stem_name);
>+	if (!pass_name)
>+		goto free;
>+
>+	/* Get the #interrupt-cells property */
>+	cur = to_of_node(in->fwnode);
>+	ret = of_property_read_u32(cur, cells_name, &in_size);
>+	if (ret < 0)
>+		goto put;
>+
>+	/* Precalculate the match array - this simplifies match loop */
>+	for (i = 0; i < in_size; i++)
>+		initial_match_array[i] = cpu_to_be32(in->param[i]);
>+
>+	ret = -EINVAL;
>+	/* Get the irqdomain-map property */
>+	map = of_get_property(cur, map_name, &map_len);
>+	if (!map) {
>+		ret = 0;
>+		goto free;
>+	}
>+	map_len /= sizeof(u32);
>+
>+	/* Get the irqdomain-map-mask property (optional) */
>+	mask = of_get_property(cur, mask_name, NULL);
>+	if (!mask)
>+		mask = dummy_mask;
>+	/* Iterate through irqdomain-map property */
>+	match = 0;
>+	while (map_len > (in_size + 1) && !match) {
>+		/* Compare specifiers */
>+		match = 1;
>+		for (i = 0; i < in_size; i++, map_len--)
>+			match &= !((match_array[i] ^ *map++) & mask[i]);
>+
>+		of_node_put(new);
>+		new = of_find_node_by_phandle(be32_to_cpup(map));
>+		map++;
>+		map_len--;
>+
>+		/* Check if not found */
>+		if (!new)
>+			goto put;
>+
>+		if (!of_device_is_available(new))
>+			match = 0;
>+
>+		ret = of_property_read_u32(new, cells_name, &out_size);
>+		if (ret)
>+			goto put;
>+
>+		/* Check for malformed properties */
>+		if (WARN_ON(out_size > MAX_PHANDLE_ARGS))
>+			goto put;
>+		if (map_len < out_size)
>+			goto put;
>+
>+		/* Move forward by new node's #interrupt-cells amount */
>+		map += out_size;
>+		map_len -= out_size;
Nit: Could make this a bit simpler to understand if you broke out the
loop on match instead of continuing the loop.
>+	}
>+	if (match) {
>+		/* Get the irqdomain-map-pass-thru property (optional) */
>+		pass = of_get_property(cur, pass_name, NULL);
>+		if (!pass)
>+			pass = dummy_pass;
>+
>+		/*
>+		 * Successfully parsed a irqdomain-map translation; copy new
>+		 * specifier into the out structure, keeping the
>+		 * bits specified in irqdomain-map-pass-thru.
>+		 */
>+		match_array = map - out_size;
>+		for (i = 0; i < out_size; i++) {
>+			__be32 val = *(map - out_size + i);
>+
>+			out->param[i] = in->param[i];
>+			if (i < in_size) {
>+				val &= ~pass[i];
I don't get why the mask is inverted and reversed here again?
>+				val |= cpu_to_be32(out->param[i]) & pass[i];
>+			}
>+
>+			out->param[i] = be32_to_cpu(val);
>+		}
>+		out->param_count = in_size = out_size;
>+		out->fwnode = of_node_to_fwnode(new);
>+	}
>+put:
>+	of_node_put(new);
>+free:
>+	kfree(mask_name);
>+	kfree(map_name);
>+	kfree(pass_name);
>+
>+	return ret;
>+}
>+EXPORT_SYMBOL(of_irq_domain_map);
>+
> /**
>  * of_irq_parse_one - Resolve an interrupt for a device
>  * @device: the device whose interrupt is to be resolved
>diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>index 9b45219893bd..0473b180dc8d 100644
>--- a/drivers/pinctrl/qcom/pinctrl-msm.c
>+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>@@ -1011,6 +1011,12 @@ static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
> 	if (!domain->parent)
> 		return 0;
>
>+	ret = of_irq_domain_map(fwspec, &parent.fwspec);
>+	if (ret)
>+		return ret;
>+
>+	pr_info("incoming is %d %#x and outgoing is %d %#x\n", fwspec->param[0], fwspec->param[1], parent.fwspec.param[0], parent.fwspec.param[1]);
>+
> 	parent.fwspec.fwnode      = domain->parent->fwnode;
> 	parent.fwspec.param_count = 2;
> 	parent.fwspec.param[0]    = hwirq;
>diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
>index 1214cabb2247..80ba1e9be68d 100644
>--- a/include/linux/of_irq.h
>+++ b/include/linux/of_irq.h
>@@ -32,6 +32,8 @@ static inline int of_irq_parse_oldworld(struct device_node *device, int index,
> }
> #endif /* CONFIG_PPC32 && CONFIG_PPC_PMAC */
>
>+extern int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out);
>+
> extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq);
> extern int of_irq_parse_one(struct device_node *device, int index,
> 			  struct of_phandle_args *out_irq);

Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Stephen Boyd]

Quoting Lina Iyer (2019-02-06 09:07:30)
> Thanks for the patch Stephen. Sorry, it took a while to get to this and
> understand how this works.
> 
> On Thu, Jan 31 2019 at 00:10 -0700, Stephen Boyd wrote:
> >Quoting Stephen Boyd (2019-01-31 13:53:42)
> >>
> >> I'm prototyping out some code to do the remapping based on this type of
> >> DT property, because it will make the irqdomain::alloc function a little
> >> simpler to implement by passing in a struct irq_fwspec and getting out a
> >> parent irq_fwspec and it will make the patches to add these mapping
> >> tables in C irrelevant. Plus, I think Lina will be happy that the
> >> pinctrl driver will know if some pin maps to the PDC or not without
> >> having to see if it fails to allocate in the parent irqdomain. But there
> >> will still need to be a property for 'wakeup-parent' unless we do
> >> something to expose irqdomain tokens into DT.
> >>
> >
> >And here's the code to do this remapping idea, heavily based on the
> >phandle remapping code. I didn't properly fix up the irq alloc function
> >for the pinctrl driver here, but it should work out properly without
> >much more diff. I realize now that the pass-thru mechanism will fail
> >horribly when a specifier changes size types.
> Could you explain?

The pass-thru code maps an incoming specifier directly to an outgoing
specifier, so it won't work well if the incoming specifier size is
different than the outgoing specifier. For example, converting a GPIO
two cell to a GIC 3 cell specifier doesn't work well.

	incoming: <GPIO# flags>
	outgoing: <GIC_SPI SPI# flags>

And the pass-thru and mask properties can't "shift" or swap a u32
element of the specifier. All they can do is copy from the incoming to
the outgoing specifier. Furthermore, we only copy over how ever many
cells there are in the incoming specifier so we don't do anything with
the last cell. So if GPIO32 corresponds to GIC SPI 14 we can't copy over
the flags.

	<32 IRQ_TYPE_LEVEL_HIGH> becomes <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>

but the code only sees <32 4> and needs to copy each element over one by
one to <0 14 4>. One solution is to match on the incoming flags and then
remap them to the outgoing flags, but then pass-thru property is
unusable and we have to list all possible flag combinations on the
incoming specifier side of the mapping.


> >I guess we'll need to keep
> >that in mind if we convert the PDC driver to this too. Or we can leave
> >the PDC driver as is and not worry about listing out the individual
> >pins.
> The PDC pins are more sequential and it looks clean as it. I would
> prefer that it be left as is.

Sure. I don't see any problems keeping the status quo in the PDC driver.

> >
> >-----8<-----
> >diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> >index a2241dd9c185..6b3a4227f433 100644
> >--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> >+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
[...]
> >+                                      <122 0 &pdc_intc 97 0>,
> >+                                      <123 0 &pdc_intc 98 0>,
> >+                                      <124 0 &pdc_intc 99 0>,
> >+                                      <125 0 &pdc_intc 100 0>,
> >+                                      <127 0 &pdc_intc 102 0>,
> >+                                      <128 0 &pdc_intc 103 0>,
> >+                                      <129 0 &pdc_intc 104 0>,
> >+                                      <130 0 &pdc_intc 105 0>,
> >+                                      <132 0 &pdc_intc 106 0>,
> >+                                      <133 0 &pdc_intc 107 0>,
> >+                                      <145 0 &pdc_intc 108 0>;
> >+                      irqdomain-map-mask = <0xff 0>;
> >+                      irqdomain-map-pass-thru = <0 0xff>;
> Where do we document these bindings?

In the DT spec itself or at
Documentation/devicetree/booting-without-of.txt I guess.

> >
> >                       qspi_clk: qspi-clk {
> >                               pinmux {
> >diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> >index 02ad93a304a4..b37f4cdfda53 100644
> >--- a/drivers/of/irq.c
> >+++ b/drivers/of/irq.c
> >@@ -274,6 +274,130 @@ int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq)
> > }
> > EXPORT_SYMBOL_GPL(of_irq_parse_raw);
> >
> I would think this would be a separate patch and I presume, I can add
> you as the author with your signed-off-by?

Sure, but I'd rather see if Rob has any views or opinions here.

> 
> >+int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out)
> >+{
[...]
> >+      while (map_len > (in_size + 1) && !match) {
> >+              /* Compare specifiers */
> >+              match = 1;
> >+              for (i = 0; i < in_size; i++, map_len--)
> >+                      match &= !((match_array[i] ^ *map++) & mask[i]);
> >+
> >+              of_node_put(new);
> >+              new = of_find_node_by_phandle(be32_to_cpup(map));
> >+              map++;
> >+              map_len--;
> >+
> >+              /* Check if not found */
> >+              if (!new)
> >+                      goto put;
> >+
> >+              if (!of_device_is_available(new))
> >+                      match = 0;
> >+
> >+              ret = of_property_read_u32(new, cells_name, &out_size);
> >+              if (ret)
> >+                      goto put;
> >+
> >+              /* Check for malformed properties */
> >+              if (WARN_ON(out_size > MAX_PHANDLE_ARGS))
> >+                      goto put;
> >+              if (map_len < out_size)
> >+                      goto put;
> >+
> >+              /* Move forward by new node's #interrupt-cells amount */
> >+              map += out_size;
> >+              map_len -= out_size;
> Nit: Could make this a bit simpler to understand if you broke out the
> loop on match instead of continuing the loop.

Instead of just doing that in the loop condition?

> >+      }
> >+      if (match) {
> >+              /* Get the irqdomain-map-pass-thru property (optional) */
> >+              pass = of_get_property(cur, pass_name, NULL);
> >+              if (!pass)
> >+                      pass = dummy_pass;
> >+
> >+              /*
> >+               * Successfully parsed a irqdomain-map translation; copy new
> >+               * specifier into the out structure, keeping the
> >+               * bits specified in irqdomain-map-pass-thru.
> >+               */
> >+              match_array = map - out_size;
> >+              for (i = 0; i < out_size; i++) {
> >+                      __be32 val = *(map - out_size + i);
> >+
> >+                      out->param[i] = in->param[i];
> >+                      if (i < in_size) {
> >+                              val &= ~pass[i];
> I don't get why the mask is inverted and reversed here again?

The mask is inverted to clear out the bits in the outgoing specifier for
whatever is there on the incoming side, per what the pass-thru mask
indicates should be copied over from incoming to outgoing.

> >+                              val |= cpu_to_be32(out->param[i]) & pass[i];
> >+                      }
> >+

Re: [PATCH 4/7] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

[Lina Iyer]

On Wed, Jan 09 2019 at 12:37 -0700, Rob Herring wrote:
>On Wed, Jan 9, 2019 at 11:31 AM Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> On Tue, Jan 08 2019 at 07:49 -0700, Rob Herring wrote:
>> >On Mon, Jan 7, 2019 at 12:51 PM Lina Iyer <ilina@codeaurora.org> wrote:
>> >>
>> >> On Fri, Dec 28 2018 at 17:07 -0700, Rob Herring wrote:
>> >> >On Wed, Dec 19, 2018 at 03:11:02PM -0700, Lina Iyer wrote:
>> >> >> SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO
>> >> >> routed to the PDC as interrupts that can be used to wake the system up
>> >> >> from deep low power modes and suspend.
>> >> >>
>> >> >> Cc: devicetree@vger.kernel.org
>> >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> >> >> ---
>> >> >>  .../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt    | 7 ++++++-
>> >> >>  1 file changed, 6 insertions(+), 1 deletion(-)
>> >> >>
>> >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
>> >> >> index 665aadb5ea28..a522ca46667d 100644
>> >> >> --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
>> >> >> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
>> >> >> @@ -29,6 +29,11 @@ SDM845 platform.
>> >> >>      Definition: must be 2. Specifying the pin number and flags, as defined
>> >> >>                  in <dt-bindings/interrupt-controller/irq.h>
>> >> >>
>> >> >> +- wakeup-parent:
>> >> >> +    Usage: optional
>> >> >> +    Value type: <phandle>
>> >> >> +    Definition: A phandle to the wakeup interrupt controller for the SoC.
>> >> >
>> >> >Is this really necessary? Is there more than one possible wakeup-parent
>> >> >node?
>> >> >
>> >> No.  There is only one but depending on the architecture, the wakeup
>> >> interrupt controller could be different device like PDC on SDM845 or MPM
>> >> on SDM820.
>> >>
>> >> What do you have in mind? Let me know if you have a better idea than
>> >> referencing in DT.
>> >
>> >If there's only one possibility for a given platform, then you can
>> >just use of_find_compatible_node(). I don't think it matters that
>> >different platforms have a different device here. It's not going to be
>> >a large table and you may need to know the differences if there's not
>> >an abstracted interface to it (seems there is in your case).
>> The GPIO irqchip would be in hierarchy with the wakeup-parent
>> irqchip and no device specific functions would be called directly.
>> We could achieve this with compatible strings to the irqchip.
>>
>> >Alternatively, if the PDC/MPM code knows what interrupt controller it
>> >is associated with, then it could setup that relationship and the
>> >interrupt controller code could retrieve that. Maybe the stacked
>> >domain support doesn't work in that direction (I haven't looked at the
>> >irq code much since that was added).
>> >
>> The PDC/MPM do not know about the association.
>
>Neither does the main interrupt controller. The association is part of
>SoC integration. You can describe that association in either direction
>and that is sufficient from a DT standpoint. You've probably picked
>putting this in the GIC(?) based on what works more easily with the
>Linux irqdomain code.
>
>> >However, my main concern is documenting something genericish in a
>> >device specific binding. It looks like Tegra is trying to add the same
>> >thing, so this needs to be documented in a common place. One question
>> >is whether wakeup is the only use or if this should be more generally
>> >a secondary interrupt parent?
>> >
>> Yes, wakeup is the only use of this interrupt parent.
>
>Maybe for you, but I was wondering about this more generally. Should
>we encode what the function (e.g. wakeup) is in the property name or
>have something like aux-interrupt-controller? Maybe some platforms
>have some need for a secondary interrupt-controller which is not
>wakeup. Routing interrupts to other cores perhaps?
>
Rob,

Would like to know your opinion on Stephen's idea. Could you take a look
at this thread again please?

Thanks,
Lina

>> It is powered by
>> an always-on rail and therefore can detect some interrupts that are
>> routed to it even when the GIC is powered off. Though Tegra's
>> implementation of the irqchip is a bit different from QCOM, the idea is
>> generally the same. It would be helpful, if we could make this a
>> generic enough binding.
>>
>> -- Lina
>>