[PATCH v3 0/9] qcom: support wakeup capable GPIOs

[PATCH v3 0/9] qcom: support wakeup capable GPIOs

[Lina Iyer]

Hi all,

This series is based on idea of setting up a wakeup parent interrupt controller
for GPIOs that are wakeup capable. The patch is based on Thierry's hierarchical
GPIO irqdomains. Much of the idea stem's from Stephen's suggestions in [1] with
some fixes.

I am posting this series to help with the discussions around this approach.
Please take a look at the series and let me know your comments. We need to find
a good place to add the document the generic irqdomain-map bindings.

Thanks,
Lina

[1]. https://lkml.org/lkml/2018/12/19/813

Lina Iyer (7):
  irqdomain: add bus token DOMAIN_BUS_WAKEUP
  drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs
  dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
  drivers: pinctrl: msm: setup GPIO irqchip hierarchy
  arm64: dts: qcom: add PDC interrupt controller for SDM845
  arm64: dts: qcom: setup PDC as wakeup parent for GPIOs for SDM845
  arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845

Stephen Boyd (1):
  of: irq: add helper to remap interrupts to another irqdomain

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

 .../bindings/pinctrl/qcom,sdm845-pinctrl.txt  |   7 +-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |  87 +++++++++++
 arch/arm64/configs/defconfig                  |   1 +
 drivers/gpio/gpiolib.c                        |  15 +-
 drivers/irqchip/qcom-pdc.c                    | 106 +++++++++++--
 drivers/of/irq.c                              | 125 ++++++++++++++++
 drivers/pinctrl/qcom/pinctrl-msm.c            | 141 ++++++++++++++++--
 include/linux/gpio/driver.h                   |   6 +
 include/linux/irqdomain.h                     |   1 +
 include/linux/of_irq.h                        |   1 +
 include/linux/soc/qcom/irq.h                  |  23 +++
 11 files changed, 486 insertions(+), 27 deletions(-)
 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 v3 1/9] 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>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 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 d1adfdf50fb3..46ccb9b174b9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1895,7 +1895,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;
@@ -1905,9 +1907,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 07cddbf45186..88ef196f96ec 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -48,6 +48,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 v3 2/9] irqdomain: add bus token DOMAIN_BUS_WAKEUP

[Lina Iyer]

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

Suggested-by: Stephen Boyd <swboyd@chromium.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 35965f41d7be..05055bc992ab 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -82,6 +82,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 v3 3/9] of: irq: add helper to remap interrupts to another irqdomain

[Lina Iyer]

From: Stephen Boyd <swboyd@chromium.org>

Sometimes interrupts are routed from an interrupt controller to another
in no specific order. Having these in the drives makes it difficult to
maintain when the same drivers supports multiple variants with different
mapping. Also, specifying them in DT makes little sense with a
bunch of numbers like -
	<0, 13>, <5, 32>,
It makes more sense when we can have the parent handle along with
interrupt specificers for the incoming interrupt as well as that of the
outgoing interrupt like -
	<22 0 &intc 36 0>,
	<24 0 &intc 37 0>,
	<26 0 &intc 38 0>,
And the interrupt specifiers can be interpred using these properties -
	irqdomain-map-mask = <0xff 0>;
	irqdomain-map-pass-thru = <0 0xff>;

Let's add a helper function to parse this from DT.

Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/of/irq.c       | 125 +++++++++++++++++++++++++++++++++++++++++
 include/linux/of_irq.h |   1 +
 2 files changed, 126 insertions(+)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index e1f6f392a4c0..a1534f947ed4 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -273,6 +273,131 @@ 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(cur);
+	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/include/linux/of_irq.h b/include/linux/of_irq.h
index 1214cabb2247..86342502a62a 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -32,6 +32,7 @@ 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);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 4/9] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Lina Iyer]

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.

Co-developed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v3:
	- Remove PDC GPIO map data (moved to DT)
	- hwirq passed in .alloc() is a PDC pin now
Changes in v2:
	- Remove separate file for PDC GPIO map data
	- Error checks and return
	- Whitespace fixes
---
 drivers/irqchip/qcom-pdc.c   | 106 ++++++++++++++++++++++++++++++++---
 include/linux/soc/qcom/irq.h |  23 ++++++++
 2 files changed, 120 insertions(+), 9 deletions(-)
 create mode 100644 include/linux/soc/qcom/irq.h

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index faa7d61b9d6c..5a64cbeec410 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -13,12 +13,13 @@
 #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>
 
 #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))
@@ -32,6 +33,16 @@ struct pdc_pin_region {
 	u32 cnt;
 };
 
+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;
+};
+
 static DEFINE_RAW_SPINLOCK(pdc_lock);
 static void __iomem *pdc_base;
 static struct pdc_pin_region *pdc_region;
@@ -47,9 +58,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 +75,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 +131,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 +180,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 +192,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 +254,60 @@ static const struct irq_domain_ops qcom_pdc_ops = {
 	.free		= irq_domain_free_irqs_common,
 };
 
+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;
+
+	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return -EINVAL;
+
+	parent_hwirq = get_parent_hwirq(hwirq);
+	if (parent_hwirq == ~0UL)
+		return -EINVAL;
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					    &qcom_pdc_gic_chip, NULL);
+	if (ret)
+		return ret;
+
+	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 int qcom_pdc_gpio_domain_select(struct irq_domain *d,
+				       struct irq_fwspec *fwspec,
+				       enum irq_domain_bus_token bus_token)
+{
+	return (bus_token == DOMAIN_BUS_WAKEUP);
+}
+
+static const struct irq_domain_ops qcom_pdc_gpio_ops = {
+	.select		= qcom_pdc_gpio_domain_select,
+	.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 +346,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 +377,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/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 v3 5/9] 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..f0fedbc5d41a 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>;
+		wake-parent = <&pdc_intc>;
 
 		qup9_active: qup9-active {
 			mux {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 6/9] 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.

Co-developed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v4:
	- Use of_irq_domain_map() and pass PDC pin to parent irqdomain
Changes in v3:
	- Call parent mask when masking GPIO interrupt
Changes in v2:
	- Fix bug when unmasking PDC interrupt
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 141 ++++++++++++++++++++++++++---
 1 file changed, 128 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index ee8119879c4c..83053b45982e 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_mask_parent(d);
+
+	/* Monitored by parent wakeup controller?*/
+	if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+		return;
+
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	val = msm_readl_intr_cfg(pctrl, g);
@@ -747,6 +757,13 @@ static void msm_gpio_irq_unmask(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);
@@ -767,6 +784,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 +815,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 +918,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 +998,87 @@ 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;
+
+	ret = of_irq_domain_map(fwspec, &parent.fwspec);
+	if (ret)
+		return ret;
+
+	parent.fwspec.fwnode = domain->parent->fwnode;
+
+	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 +1093,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 +1102,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 +1139,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 v3 7/9] arm64: dts: qcom: add PDC interrupt controller for SDM845

[Lina Iyer]

Add PDC interrupt controller device bindings for SDM845.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v2:
	- Use updated address specification in reg
	- Rename to pdc_intc
	- Sort per address in DT
---
 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 c27cbd3bcb0a..e55100c2705e 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1534,6 +1534,15 @@
 			#power-domain-cells = <1>;
 		};
 
+		pdc_intc: interrupt-controller@b220000 {
+			compatible = "qcom,sdm845-pdc";
+			reg = <0 0x0b220000 0 0x30000>;
+			qcom,pdc-ranges = <0 480 94>, <94 609 15>, <115 630 7>;
+			#interrupt-cells = <2>;
+			interrupt-parent = <&intc>;
+			interrupt-controller;
+		};
+
 		tsens0: thermal-sensor@c263000 {
 			compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
 			reg = <0xc263000 0x1ff>, /* TM */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 8/9] arm64: dts: qcom: setup PDC as wakeup parent for GPIOs for SDM845

[Lina Iyer]

Setup PDC wakeup parent for TLMM for SDM845 SoC.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v4:
	- Provide irqdomain-map for GPIOs that map to PDC
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 78 ++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index e55100c2705e..58f9f2111449 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -994,6 +994,84 @@
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
+			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>;
 
 			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

[PATCH v3 9/9] arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845

[Lina Iyer]

Enable PDC interrupt controller for SDM845 devices. The interrupt
controller can detect wakeup capable interrupts when the SoC is in a low
power state.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index c8432e24207e..943f312772ee 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -656,6 +656,7 @@ CONFIG_QCOM_SMEM=y
 CONFIG_QCOM_SMD_RPM=y
 CONFIG_QCOM_SMP2P=y
 CONFIG_QCOM_SMSM=y
+CONFIG_QCOM_PDC=y
 CONFIG_ROCKCHIP_PM_DOMAINS=y
 CONFIG_ARCH_TEGRA_132_SOC=y
 CONFIG_ARCH_TEGRA_210_SOC=y
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 6/9] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Stephen Boyd]

Quoting Lina Iyer (2019-02-22 14:18:47)
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index ee8119879c4c..83053b45982e 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -890,6 +918,9 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
>  
>         raw_spin_unlock_irqrestore(&pctrl->lock, flags);

Can we remove the above hunk of code in this function? From what I can
tell we're trying to set wake on the GIC SPI line and that won't ever
work because it doesn't support wakeup support. Furthermore, I get a
lockdep splat when I try to set wake on a GPIO irq with this patchset
applied. It's probably a false positive though because it's warning
about an AA deadlock with the irq descriptor lock, which is aliased
between the two controllers.

>  
> +       if (d->parent_data)
> +               irq_chip_set_wake_parent(d, on);
> +
>         return 0;
>  }
>  

Here's a patch on top. If you need my signed-off-by please feel free to
add it:

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

-----8<-------
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index cdbc06ec5e96..4e81dad80a4d 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -852,20 +852,10 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 
 static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 {
-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
-	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
-	unsigned long flags;
-
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
-
-	irq_set_irq_wake(pctrl->irq, on);
-
-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
-
 	if (d->parent_data)
-		irq_chip_set_wake_parent(d, on);
+		return irq_chip_set_wake_parent(d, on);
 
-	return 0;
+	return -ENOSYS;
 }
 
 static int msm_gpio_irq_reqres(struct irq_data *d)

Re: [PATCH v3 6/9] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Lina Iyer]

On Thu, Mar 07 2019 at 17:59 -0700, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-02-22 14:18:47)
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index ee8119879c4c..83053b45982e 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> @@ -890,6 +918,9 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
>>
>>         raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>
>Can we remove the above hunk of code in this function? From what I can
>tell we're trying to set wake on the GIC SPI line and that won't ever
>work because it doesn't support wakeup support. Furthermore, I get a
>lockdep splat when I try to set wake on a GPIO irq with this patchset
>applied. It's probably a false positive though because it's warning
>about an AA deadlock with the irq descriptor lock, which is aliased
>between the two controllers.
>
>>
>> +       if (d->parent_data)
>> +               irq_chip_set_wake_parent(d, on);
>> +
>>         return 0;
>>  }
>>
>
>Here's a patch on top. If you need my signed-off-by please feel free to
>add it:
>
I will fold it in in my patch. It has your Co-developed-by anyways.

Thanks,
Lina

>Signed-off-by: Stephen Boyd <swboyd@chromium.org>
>
>-----8<-------
>diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>index cdbc06ec5e96..4e81dad80a4d 100644
>--- a/drivers/pinctrl/qcom/pinctrl-msm.c
>+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>@@ -852,20 +852,10 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>
> static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
> {
>-	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>-	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>-	unsigned long flags;
>-
>-	raw_spin_lock_irqsave(&pctrl->lock, flags);
>-
>-	irq_set_irq_wake(pctrl->irq, on);
>-
>-	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>-
> 	if (d->parent_data)
>-		irq_chip_set_wake_parent(d, on);
>+		return irq_chip_set_wake_parent(d, on);
>
>-	return 0;
>+	return -ENOSYS;
> }
>
> static int msm_gpio_irq_reqres(struct irq_data *d)

Re: [PATCH v3 2/9] irqdomain: add bus token DOMAIN_BUS_WAKEUP

[Marc Zyngier]

On 22/02/2019 22:18, Lina Iyer wrote:
> Add new bus token to describe domains that are wakeup capable.

This doesn't quite match what this bus token is trying to describe. How
about something along the lines of the following:

A single controller can handle normal interrupts and wake-up interrupts
independently, with a different numbering space. It is thus crucial to
allow the driver for such a controller discriminate between the two.

A simple way to do so is to tag the wake-up irqdomain with a "bus token"
that indicates the wake-up domain. This slightly abuses the notion of
bus, but also radically simplifies the design of such a driver. Between
two evils, we choose the least damaging.

> 
> Suggested-by: Stephen Boyd <swboyd@chromium.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 35965f41d7be..05055bc992ab 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -82,6 +82,7 @@ enum irq_domain_bus_token {
>  	DOMAIN_BUS_NEXUS,
>  	DOMAIN_BUS_IPI,
>  	DOMAIN_BUS_FSL_MC_MSI,
> +	DOMAIN_BUS_WAKEUP,
>  };
>  
>  /**
> 

Thanks,

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

Re: [PATCH v3 4/9] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Marc Zyngier]

On 22/02/2019 22:18, Lina Iyer wrote:
> 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.
> 
> Co-developed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
> Changes in v3:
> 	- Remove PDC GPIO map data (moved to DT)
> 	- hwirq passed in .alloc() is a PDC pin now
> Changes in v2:
> 	- Remove separate file for PDC GPIO map data
> 	- Error checks and return
> 	- Whitespace fixes
> ---
>  drivers/irqchip/qcom-pdc.c   | 106 ++++++++++++++++++++++++++++++++---
>  include/linux/soc/qcom/irq.h |  23 ++++++++
>  2 files changed, 120 insertions(+), 9 deletions(-)
>  create mode 100644 include/linux/soc/qcom/irq.h
> 
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index faa7d61b9d6c..5a64cbeec410 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -13,12 +13,13 @@
>  #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>
>  
>  #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))
> @@ -32,6 +33,16 @@ struct pdc_pin_region {
>  	u32 cnt;
>  };
>  
> +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;
> +};

I can't see this being used anywhere.

> +
>  static DEFINE_RAW_SPINLOCK(pdc_lock);
>  static void __iomem *pdc_base;
>  static struct pdc_pin_region *pdc_region;
> @@ -47,9 +58,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;

Why do you need this change? As far as I can tell, you can always use
the irq_data here, and it would reduce the size of the patch.

>  	u32 index, mask;
>  	u32 enable;
>  
> @@ -65,13 +75,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)

Can you define a specific constant here? PDC_WAKEUP_IRQ? Or something?
Also, I can't work out where d->hwirq gets set to such value.

> +		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 +131,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;
> +

It is quite odd that you're accepting any odd trigger, even if they are
illegal. The API should be consistent, even if you're not applying any
change.

>  	switch (type) {
>  	case IRQ_TYPE_EDGE_RISING:
>  		pdc_type = PDC_EDGE_RISING;
> @@ -157,7 +180,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 +192,6 @@ static irq_hw_number_t get_parent_hwirq(int pin)
>  			return (region->parent_base + pin - region->pin_base);
>  	}
>  
> -	WARN_ON(1);
>  	return ~0UL;

Above, you're using ULONG_MAX, and here ~0UL. You need to make up your
mind and use one single identifier. Or maybe these are not the same
thing? In any case, you need to lift the ambiguity.

>  }
>  
> @@ -232,6 +254,60 @@ static const struct irq_domain_ops qcom_pdc_ops = {
>  	.free		= irq_domain_free_irqs_common,
>  };
>  
> +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;
> +
> +	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return -EINVAL;
> +
> +	parent_hwirq = get_parent_hwirq(hwirq);
> +	if (parent_hwirq == ~0UL)
> +		return -EINVAL;
> +
> +	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +					    &qcom_pdc_gic_chip, NULL);
> +	if (ret)
> +		return ret;
> +
> +	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 int qcom_pdc_gpio_domain_select(struct irq_domain *d,
> +				       struct irq_fwspec *fwspec,
> +				       enum irq_domain_bus_token bus_token)
> +{
> +	return (bus_token == DOMAIN_BUS_WAKEUP);
> +}
> +
> +static const struct irq_domain_ops qcom_pdc_gpio_ops = {
> +	.select		= qcom_pdc_gpio_domain_select,
> +	.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 +346,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 +377,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/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
> 

Thanks,

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

Re: [PATCH v3 3/9] of: irq: add helper to remap interrupts to another irqdomain

[Marc Zyngier]

On 22/02/2019 22:18, Lina Iyer wrote:
> From: Stephen Boyd <swboyd@chromium.org>
> 
> Sometimes interrupts are routed from an interrupt controller to another
> in no specific order. Having these in the drives makes it difficult to

s/drives/driver/ ?

> maintain when the same drivers supports multiple variants with different
> mapping. Also, specifying them in DT makes little sense with a
> bunch of numbers like -
> 	<0, 13>, <5, 32>,
> It makes more sense when we can have the parent handle along with
> interrupt specificers for the incoming interrupt as well as that of the

specifiers

> outgoing interrupt like -
> 	<22 0 &intc 36 0>,
> 	<24 0 &intc 37 0>,
> 	<26 0 &intc 38 0>,
> And the interrupt specifiers can be interpred using these properties -

interpreted

> 	irqdomain-map-mask = <0xff 0>;
> 	irqdomain-map-pass-thru = <0 0xff>;

Talking about interpretation, can you please expand on how these should
be used? You definitely want to have this documented as a generic
binding somewhere.

Thanks,

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

Re: [PATCH v3 6/9] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Marc Zyngier]

On 22/02/2019 22:18, Lina Iyer wrote:
> 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.
> 
> Co-developed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
> Changes in v4:
> 	- Use of_irq_domain_map() and pass PDC pin to parent irqdomain
> Changes in v3:
> 	- Call parent mask when masking GPIO interrupt
> Changes in v2:
> 	- Fix bug when unmasking PDC interrupt
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 141 ++++++++++++++++++++++++++---
>  1 file changed, 128 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index ee8119879c4c..83053b45982e 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_mask_parent(d);
> +
> +	/* Monitored by parent wakeup controller?*/
> +	if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
> +		return;
> +
>  	raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
>  	val = msm_readl_intr_cfg(pctrl, g);
> @@ -747,6 +757,13 @@ static void msm_gpio_irq_unmask(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);
> @@ -767,6 +784,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 +815,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 +918,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 +998,87 @@ 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;
> +
> +	ret = of_irq_domain_map(fwspec, &parent.fwspec);
> +	if (ret)
> +		return ret;
> +
> +	parent.fwspec.fwnode = domain->parent->fwnode;
> +
> +	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 +1093,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 +1102,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;

I know you're only moving this around, but can you explain why you're
setting IRQ_TYPE_NONE in combination with handle_edge_irq? The two
really should go together. If this doesn't work for some reason, please
document it.

> +
> +	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 +1139,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,
> 

Thanks,

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

Re: [PATCH v3 0/9] qcom: support wakeup capable GPIOs

[Marc Zyngier]

On 22/02/2019 22:18, Lina Iyer wrote:
> Hi all,
> 
> This series is based on idea of setting up a wakeup parent interrupt controller
> for GPIOs that are wakeup capable. The patch is based on Thierry's hierarchical
> GPIO irqdomains. Much of the idea stem's from Stephen's suggestions in [1] with
> some fixes.

Given how long it took to get there, I'm really keen on moving forward
with this as the first implementation. There is a number of things that
needs fixing, but nothing that looks major so far.

> I am posting this series to help with the discussions around this approach.
> Please take a look at the series and let me know your comments. We need to find
> a good place to add the document the generic irqdomain-map bindings.

How about the obvious
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt.

I'd really like to see a respin of this by -rc1.

Thanks,

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

Re: [PATCH v3 0/9] qcom: support wakeup capable GPIOs

[Lina Iyer]

On Mon, Mar 11 2019 at 05:09 -0600, Marc Zyngier wrote:
>On 22/02/2019 22:18, Lina Iyer wrote:
>> Hi all,
>>
>> This series is based on idea of setting up a wakeup parent interrupt controller
>> for GPIOs that are wakeup capable. The patch is based on Thierry's hierarchical
>> GPIO irqdomains. Much of the idea stem's from Stephen's suggestions in [1] with
>> some fixes.
>
>Given how long it took to get there, I'm really keen on moving forward
>with this as the first implementation. There is a number of things that
>needs fixing, but nothing that looks major so far.
>
>> I am posting this series to help with the discussions around this approach.
>> Please take a look at the series and let me know your comments. We need to find
>> a good place to add the document the generic irqdomain-map bindings.
>
>How about the obvious
>Documentation/devicetree/bindings/interrupt-controller/interrupts.txt.
>
>I'd really like to see a respin of this by -rc1.
>
Thanks for the review Marc. Will do a respin in a couple of days.

Thanks,
Lina

Re: [PATCH v3 6/9] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

[Lina Iyer]

On Mon, Mar 11 2019 at 04:55 -0600, Marc Zyngier wrote:
>On 22/02/2019 22:18, Lina Iyer wrote:
>> 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.
>>
>> Co-developed-by: Stephen Boyd <swboyd@chromium.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>

[...]

>> @@ -986,6 +1093,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 +1102,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;
>
>I know you're only moving this around, but can you explain why you're
>setting IRQ_TYPE_NONE in combination with handle_edge_irq? The two
>really should go together. If this doesn't work for some reason, please
>document it.
>
Yes, it was a carry-over from the existing code. I am not sure why that
seems to be a common practice among GPIO drivers.
IRQ_TYPE_EDGE_RISING would be a better option ?

>> +
>> +	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 +1139,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;
>>  }
>>

Thanks,
Lina

Re: [PATCH v3 4/9] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Lina Iyer]

On Mon, Mar 11 2019 at 04:42 -0600, Marc Zyngier wrote:
>On 22/02/2019 22:18, Lina Iyer wrote:
>> 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.
>>
>> Co-developed-by: Stephen Boyd <swboyd@chromium.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>> Changes in v3:
>> 	- Remove PDC GPIO map data (moved to DT)
>> 	- hwirq passed in .alloc() is a PDC pin now
>> Changes in v2:
>> 	- Remove separate file for PDC GPIO map data
>> 	- Error checks and return
>> 	- Whitespace fixes
>> ---
>>  drivers/irqchip/qcom-pdc.c   | 106 ++++++++++++++++++++++++++++++++---
>>  include/linux/soc/qcom/irq.h |  23 ++++++++
>>  2 files changed, 120 insertions(+), 9 deletions(-)
>>  create mode 100644 include/linux/soc/qcom/irq.h
>>
>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>> index faa7d61b9d6c..5a64cbeec410 100644
>> --- a/drivers/irqchip/qcom-pdc.c
>> +++ b/drivers/irqchip/qcom-pdc.c
>> @@ -13,12 +13,13 @@
>>  #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>
>>
>>  #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))
>> @@ -32,6 +33,16 @@ struct pdc_pin_region {
>>  	u32 cnt;
>>  };
>>
>> +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;
>> +};
>
>I can't see this being used anywhere.
>
Thanks for catching this.

This and the ULONG_MAX changes are from a previous idea. We don't need
them anymore.. I will remove them.

>> +
>>  static DEFINE_RAW_SPINLOCK(pdc_lock);
>>  static void __iomem *pdc_base;
>>  static struct pdc_pin_region *pdc_region;
>> @@ -47,9 +58,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;
>
>Why do you need this change? As far as I can tell, you can always use
>the irq_data here, and it would reduce the size of the patch.
>
Agreed

>>  	u32 index, mask;
>>  	u32 enable;
>>
>> @@ -65,13 +75,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)
>
>Can you define a specific constant here? PDC_WAKEUP_IRQ? Or something?
>Also, I can't work out where d->hwirq gets set to such value.
>
With an earlier approach, it could, not with this. We could remove them
all.

>> +		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 +131,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;
>> +
>
>It is quite odd that you're accepting any odd trigger, even if they are
>illegal. The API should be consistent, even if you're not applying any
>change.
>
Ok. This should be remvoed.

>>  	switch (type) {
>>  	case IRQ_TYPE_EDGE_RISING:
>>  		pdc_type = PDC_EDGE_RISING;
>> @@ -157,7 +180,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 +192,6 @@ static irq_hw_number_t get_parent_hwirq(int pin)
>>  			return (region->parent_base + pin - region->pin_base);
>>  	}
>>
>> -	WARN_ON(1);
>>  	return ~0UL;
>
>Above, you're using ULONG_MAX, and here ~0UL. You need to make up your
>mind and use one single identifier. Or maybe these are not the same
>thing? In any case, you need to lift the ambiguity.
>
Will do.

>>  }
>>
>> @@ -232,6 +254,60 @@ static const struct irq_domain_ops qcom_pdc_ops = {
>>  	.free		= irq_domain_free_irqs_common,
>>  };
>>
>> +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;
>> +
>> +	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
>> +	if (ret)
>> +		return -EINVAL;
>> +
>> +	parent_hwirq = get_parent_hwirq(hwirq);
>> +	if (parent_hwirq == ~0UL)
>> +		return -EINVAL;
>> +
>> +	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
>> +					    &qcom_pdc_gic_chip, NULL);
>> +	if (ret)
>> +		return ret;
>> +
>> +	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 int qcom_pdc_gpio_domain_select(struct irq_domain *d,
>> +				       struct irq_fwspec *fwspec,
>> +				       enum irq_domain_bus_token bus_token)
>> +{
>> +	return (bus_token == DOMAIN_BUS_WAKEUP);
>> +}
>> +
>> +static const struct irq_domain_ops qcom_pdc_gpio_ops = {
>> +	.select		= qcom_pdc_gpio_domain_select,
>> +	.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 +346,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 +377,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/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
>>
Thanks,
Lina