[PATCH 00/12] Support wakeup capable GPIOs

[PATCH 00/12] Support wakeup capable GPIOs

[Lina Iyer]

Hi all,

Thanks for all the reviews.

Here is the next spin of the wakeup capable GPIO support. In order to
facilitate basic support available in the kernel, I have dropped the SPI
register configuration. The feature was added when this series was
restarted based on new hierarchy support in gpiolib. But, the SPI
configuration can be done in the firmware. This would avoid a whole lot
of code in linux that serve little to no purpose. Users of GPIO never
have the need to change the trigger type (level edge and vice-versa) and
the basic configuration can be set in the firmware before boot.

Changes in v1:
	- Address review comments
	- Add Reviewed-by tags
	- Drop SPI config patches
	- Rebase on top of Rajendra's PDC changes [6]

Changes in RFC v2[5]:
        - Address review comments #3, #4, #6, #7, #8, #9, #10
        - Rebased on top of linux-next GPIO latest patches [1],[3],[4]
        - Increase PDC max irqs in #2 (avoid merge conflicts with
          downstream)
        - Add Reviewed-by #5

Please consider reviewing these patches.

Thanks,
Lina

[1].
https://lore.kernel.org/linux-gpio/20190808123242.5359-1-linus.walleij@linaro.org/
[2].
https://lkml.org/lkml/2019/5/7/1173
[3].
https://lore.kernel.org/r/20190819084904.30027-1-linus.walleij@linaro.org
[4].
https://lore.kernel.org/r/20190724083828.7496-1-linus.walleij@linaro.org
[5].
https://lore.kernel.org/linux-gpio/5da6b849.1c69fb81.a9b04.1b9f@mx.google.com/t/
[6].
https://lore.kernel.org/linux-arm-msm/d622482d92059533f03b65af26c69b9b@www.loen.fr/

Lina Iyer (10):
  irqdomain: add bus token DOMAIN_BUS_WAKEUP
  drivers: irqchip: qcom-pdc: update max PDC interrupts
  drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask
  drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs
  of: irq: document properties for wakeup interrupt parent
  drivers: pinctrl: msm: setup GPIO chip in hierarchy
  drivers: pinctrl: sdm845: add PDC wakeup interrupt map for GPIOs
  arm64: dts: qcom: add PDC interrupt controller for SDM845
  arm64: dts: qcom: setup PDC as the wakeup parent for TLMM on SDM845
  arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845

Maulik Shah (2):
  genirq: Introduce irq_chip_get/set_parent_state calls
  drivers: irqchip: pdc: Add irqchip set/get state calls

 .../bindings/interrupt-controller/interrupts.txt   |  12 ++
 arch/arm64/boot/dts/qcom/sdm845.dtsi               |  10 ++
 arch/arm64/configs/defconfig                       |   1 +
 drivers/irqchip/qcom-pdc.c                         | 147 +++++++++++++++++++--
 drivers/pinctrl/qcom/pinctrl-msm.c                 | 113 ++++++++++++++++
 drivers/pinctrl/qcom/pinctrl-msm.h                 |  16 +++
 drivers/pinctrl/qcom/pinctrl-sdm845.c              |  23 +++-
 include/linux/irq.h                                |   6 +
 include/linux/irqdomain.h                          |   1 +
 include/linux/soc/qcom/irq.h                       |  34 +++++
 kernel/irq/chip.c                                  |  44 ++++++
 11 files changed, 393 insertions(+), 14 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 01/12] irqdomain: add bus token DOMAIN_BUS_WAKEUP

[Lina Iyer]

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 583e7ab..3c340db 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -83,6 +83,7 @@ enum irq_domain_bus_token {
 	DOMAIN_BUS_IPI,
 	DOMAIN_BUS_FSL_MC_MSI,
 	DOMAIN_BUS_TI_SCI_INTA_MSI,
+	DOMAIN_BUS_WAKEUP,
 };
 
 /**
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 01/12] irqdomain: add bus token DOMAIN_BUS_WAKEUP

[Stephen Boyd]

Quoting Lina Iyer (2019-11-14 10:35:10)
> 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>
> ---

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

[PATCH 02/12] drivers: irqchip: qcom-pdc: update max PDC interrupts

[Lina Iyer]

Newer SoCs have increased the number of interrupts routed to the PDC
interrupt controller. Update the definition of max PDC interrupts.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/irqchip/qcom-pdc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index c175333..690cf10 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2017-2019, The Linux Foundation. All rights reserved.
  */
 
 #include <linux/err.h>
@@ -18,7 +18,7 @@
 #include <linux/slab.h>
 #include <linux/types.h>
 
-#define PDC_MAX_IRQS		126
+#define PDC_MAX_IRQS		168
 
 #define CLEAR_INTR(reg, intr)	(reg & ~(1 << intr))
 #define ENABLE_INTR(reg, intr)	(reg | (1 << intr))
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 02/12] drivers: irqchip: qcom-pdc: update max PDC interrupts

[Stephen Boyd]

Quoting Lina Iyer (2019-11-14 10:35:11)
> Newer SoCs have increased the number of interrupts routed to the PDC
> interrupt controller. Update the definition of max PDC interrupts.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---

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

[PATCH 03/12] drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask

[Lina Iyer]

When an interrupt is to be serviced, the convention is to mask the
interrupt at the chip and unmask after servicing the interrupt. Enabling
and disabling the interrupt at the PDC irqchip causes an interrupt storm
due to the way dual edge interrupts are handled in hardware.

Skip configuring the PDC when the IRQ is masked and unmasked, instead
use the irq_enable/irq_disable callbacks to toggle the IRQ_ENABLE
register at the PDC. The PDC's IRQ_ENABLE register is only used during
the monitoring mode when the system is asleep and is not needed for
active mode detection.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/irqchip/qcom-pdc.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 690cf10..527c29e 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -63,15 +63,25 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
 	raw_spin_unlock(&pdc_lock);
 }
 
-static void qcom_pdc_gic_mask(struct irq_data *d)
+static void qcom_pdc_gic_disable(struct irq_data *d)
 {
 	pdc_enable_intr(d, false);
+	irq_chip_disable_parent(d);
+}
+
+static void qcom_pdc_gic_enable(struct irq_data *d)
+{
+	pdc_enable_intr(d, true);
+	irq_chip_enable_parent(d);
+}
+
+static void qcom_pdc_gic_mask(struct irq_data *d)
+{
 	irq_chip_mask_parent(d);
 }
 
 static void qcom_pdc_gic_unmask(struct irq_data *d)
 {
-	pdc_enable_intr(d, true);
 	irq_chip_unmask_parent(d);
 }
 
@@ -148,6 +158,8 @@ static struct irq_chip qcom_pdc_gic_chip = {
 	.irq_eoi		= irq_chip_eoi_parent,
 	.irq_mask		= qcom_pdc_gic_mask,
 	.irq_unmask		= qcom_pdc_gic_unmask,
+	.irq_disable		= qcom_pdc_gic_disable,
+	.irq_enable		= qcom_pdc_gic_enable,
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 	.irq_set_type		= qcom_pdc_gic_set_type,
 	.flags			= IRQCHIP_MASK_ON_SUSPEND |
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 03/12] drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask

[Stephen Boyd]

Quoting Lina Iyer (2019-11-14 10:35:12)
> When an interrupt is to be serviced, the convention is to mask the
> interrupt at the chip and unmask after servicing the interrupt. Enabling
> and disabling the interrupt at the PDC irqchip causes an interrupt storm
> due to the way dual edge interrupts are handled in hardware.
> 
> Skip configuring the PDC when the IRQ is masked and unmasked, instead
> use the irq_enable/irq_disable callbacks to toggle the IRQ_ENABLE
> register at the PDC. The PDC's IRQ_ENABLE register is only used during
> the monitoring mode when the system is asleep and is not needed for
> active mode detection.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---

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

[PATCH 04/12] 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: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v1:
	- Include irqdomain.h
Changes in RFC v2:
	- Move irq_domain_qcom_handle_wakeup to the patch where it is
	used
	- Replace #define definitons
	- Add Signed-off-by and other minor changes
---
 drivers/irqchip/qcom-pdc.c   | 104 +++++++++++++++++++++++++++++++++++++++----
 include/linux/soc/qcom/irq.h |  21 +++++++++
 2 files changed, 116 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 527c29e..4f2c762 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		168
+#define PDC_MAX_GPIO_IRQS	256
 
 #define CLEAR_INTR(reg, intr)	(reg & ~(1 << intr))
 #define ENABLE_INTR(reg, intr)	(reg | (1 << intr))
@@ -26,6 +27,8 @@
 #define IRQ_ENABLE_BANK		0x10
 #define IRQ_i_CFG		0x110
 
+#define PDC_NO_PARENT_IRQ	~0UL
+
 struct pdc_pin_region {
 	u32 pin_base;
 	u32 parent_base;
@@ -65,23 +68,35 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
 
 static void qcom_pdc_gic_disable(struct irq_data *d)
 {
+	if (d->hwirq == GPIO_NO_WAKE_IRQ)
+		return;
+
 	pdc_enable_intr(d, false);
 	irq_chip_disable_parent(d);
 }
 
 static void qcom_pdc_gic_enable(struct irq_data *d)
 {
+	if (d->hwirq == GPIO_NO_WAKE_IRQ)
+		return;
+
 	pdc_enable_intr(d, true);
 	irq_chip_enable_parent(d);
 }
 
 static void qcom_pdc_gic_mask(struct irq_data *d)
 {
+	if (d->hwirq == GPIO_NO_WAKE_IRQ)
+		return;
+
 	irq_chip_mask_parent(d);
 }
 
 static void qcom_pdc_gic_unmask(struct irq_data *d)
 {
+	if (d->hwirq == GPIO_NO_WAKE_IRQ)
+		return;
+
 	irq_chip_unmask_parent(d);
 }
 
@@ -124,6 +139,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
 	int pin_out = d->hwirq;
 	enum pdc_irq_config_bits pdc_type;
 
+	if (pin_out == GPIO_NO_WAKE_IRQ)
+		return 0;
+
 	switch (type) {
 	case IRQ_TYPE_EDGE_RISING:
 		pdc_type = PDC_EDGE_RISING;
@@ -181,8 +199,7 @@ static irq_hw_number_t get_parent_hwirq(int pin)
 			return (region->parent_base + pin - region->pin_base);
 	}
 
-	WARN_ON(1);
-	return ~0UL;
+	return PDC_NO_PARENT_IRQ;
 }
 
 static int qcom_pdc_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
@@ -211,17 +228,17 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq,
 
 	ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
 	if (ret)
-		return -EINVAL;
-
-	parent_hwirq = get_parent_hwirq(hwirq);
-	if (parent_hwirq == ~0UL)
-		return -EINVAL;
+		return ret;
 
 	ret  = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
 					     &qcom_pdc_gic_chip, NULL);
 	if (ret)
 		return ret;
 
+	parent_hwirq = get_parent_hwirq(hwirq);
+	if (parent_hwirq == PDC_NO_PARENT_IRQ)
+		return 0;
+
 	if (type & IRQ_TYPE_EDGE_BOTH)
 		type = IRQ_TYPE_EDGE_RISING;
 
@@ -244,6 +261,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 irq_fwspec *fwspec = data;
+	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 ret;
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					    &qcom_pdc_gic_chip, NULL);
+	if (ret)
+		return ret;
+
+	if (hwirq == GPIO_NO_WAKE_IRQ)
+		return 0;
+
+	parent_hwirq = get_parent_hwirq(hwirq);
+	if (parent_hwirq == PDC_NO_PARENT_IRQ)
+		return 0;
+
+	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;
@@ -282,7 +353,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);
@@ -313,8 +384,23 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
 		goto fail;
 	}
 
+	pdc_gpio_domain = irq_domain_create_hierarchy(parent_domain,
+					IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP,
+					PDC_MAX_GPIO_IRQS,
+					of_fwnode_handle(node),
+					&qcom_pdc_gpio_ops, NULL);
+	if (!pdc_gpio_domain) {
+		pr_err("%pOF: PDC domain add failed for GPIO domain\n", node);
+		ret = -ENOMEM;
+		goto remove;
+	}
+
+	irq_domain_update_bus_token(pdc_gpio_domain, DOMAIN_BUS_WAKEUP);
+
 	return 0;
 
+remove:
+	irq_domain_remove(pdc_domain);
 fail:
 	kfree(pdc_region);
 	iounmap(pdc_base);
diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h
new file mode 100644
index 0000000..637c0bf
--- /dev/null
+++ b/include/linux/soc/qcom/irq.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __QCOM_IRQ_H
+#define __QCOM_IRQ_H
+
+#include <linux/irqdomain.h>
+
+#define GPIO_NO_WAKE_IRQ	~0U
+
+/**
+ * QCOM specific IRQ domain flags that distinguishes the handling of wakeup
+ * capable interrupts by different interrupt controllers.
+ *
+ * IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP: Line must be masked at TLMM and the
+ *                                  interrupt configuration is done at PDC
+ * IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP: Interrupt configuration is handled at TLMM
+ */
+#define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP		(IRQ_DOMAIN_FLAG_NONCORE << 0)
+#define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP		(IRQ_DOMAIN_FLAG_NONCORE << 1)
+
+#endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 04/12] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs

[Stephen Boyd]

Quoting Lina Iyer (2019-11-14 10:35:13)
> 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: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---

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

[PATCH 05/12] of: irq: document properties for wakeup interrupt parent

[Lina Iyer]

Some interrupt controllers in a SoC, are always powered on and have a
select interrupts routed to them, so that they can wakeup the SoC from
suspend. Add wakeup-parent DT property to refer to these interrupt
controllers.

Cc: devicetree@vger.kernel.org
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes in v1:
	- Remove whitespace at end of patch
---
 .../devicetree/bindings/interrupt-controller/interrupts.txt  | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
index 4a3ee25..4ebfa00 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
@@ -108,3 +108,15 @@ commonly used:
 			sensitivity = <7>;
 		};
 	};
+
+3) Interrupt wakeup parent
+--------------------------
+
+Some interrupt controllers in a SoC, are always powered on and have a select
+interrupts routed to them, so that they can wakeup the SoC from suspend. These
+interrupt controllers do not fall into the category of a parent interrupt
+controller and can be specified by the "wakeup-parent" property and contain a
+single phandle referring to the wakeup capable interrupt controller.
+
+   Example:
+	wakeup-parent = <&pdc_intc>;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 05/12] of: irq: document properties for wakeup interrupt parent

[Stephen Boyd]

Quoting Lina Iyer (2019-11-14 10:35:14)
> Some interrupt controllers in a SoC, are always powered on and have a
> select interrupts routed to them, so that they can wakeup the SoC from
> suspend. Add wakeup-parent DT property to refer to these interrupt
> controllers.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---

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

[PATCH 06/12] genirq: Introduce irq_chip_get/set_parent_state calls

[Lina Iyer]

From: Maulik Shah <mkshah@codeaurora.org>

On certain QTI chipsets some GPIOs are direct-connect interrupts to the
GIC to be used as regular interrupt lines. When the GPIOs are not used
for interrupt generation the interrupt line is disabled. But disabling
the interrupt at GIC does not prevent the interrupt to be reported as
pending at GIC_ISPEND. Later, when drivers call enable_irq() on the
interrupt, an unwanted interrupt occurs.

Introduce get and set methods for irqchip's parent to clear it's pending
irq state. This then can be invoked by the GPIO interrupt controller on
the parents in it hierarchy to clear the interrupt before enabling the
interrupt.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
[updated commit text and minor code fixes]
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in RFC v2 -
	- Rephrase commit text
	- Address code review comments
---
 include/linux/irq.h |  6 ++++++
 kernel/irq/chip.c   | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index fb301cf..7853eb9 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -610,6 +610,12 @@ extern int irq_chip_pm_put(struct irq_data *data);
 #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
 extern void handle_fasteoi_ack_irq(struct irq_desc *desc);
 extern void handle_fasteoi_mask_irq(struct irq_desc *desc);
+extern int irq_chip_set_parent_state(struct irq_data *data,
+				     enum irqchip_irq_state which,
+				     bool val);
+extern int irq_chip_get_parent_state(struct irq_data *data,
+				     enum irqchip_irq_state which,
+				     bool *state);
 extern void irq_chip_enable_parent(struct irq_data *data);
 extern void irq_chip_disable_parent(struct irq_data *data);
 extern void irq_chip_ack_parent(struct irq_data *data);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index b76703b..b3fa2d8 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1298,6 +1298,50 @@ EXPORT_SYMBOL_GPL(handle_fasteoi_mask_irq);
 #endif /* CONFIG_IRQ_FASTEOI_HIERARCHY_HANDLERS */
 
 /**
+ * irq_chip_set_parent_state - set the state of a parent interrupt.
+ *
+ * @data: Pointer to interrupt specific data
+ * @which: State to be restored (one of IRQCHIP_STATE_*)
+ * @val: Value corresponding to @which
+ *
+ * Conditional success, if the underlying irqchip does not implement it.
+ */
+int irq_chip_set_parent_state(struct irq_data *data,
+			      enum irqchip_irq_state which,
+			      bool val)
+{
+	data = data->parent_data;
+
+	if (!data || !data->chip->irq_set_irqchip_state)
+		return 0;
+
+	return data->chip->irq_set_irqchip_state(data, which, val);
+}
+EXPORT_SYMBOL_GPL(irq_chip_set_parent_state);
+
+/**
+ * irq_chip_get_parent_state - get the state of a parent interrupt.
+ *
+ * @data: Pointer to interrupt specific data
+ * @which: one of IRQCHIP_STATE_* the caller wants to know
+ * @state: a pointer to a boolean where the state is to be stored
+ *
+ * Conditional success, if the underlying irqchip does not implement it.
+ */
+int irq_chip_get_parent_state(struct irq_data *data,
+			      enum irqchip_irq_state which,
+			      bool *state)
+{
+	data = data->parent_data;
+
+	if (!data || !data->chip->irq_get_irqchip_state)
+		return 0;
+
+	return data->chip->irq_get_irqchip_state(data, which, state);
+}
+EXPORT_SYMBOL_GPL(irq_chip_get_parent_state);
+
+/**
  * irq_chip_enable_parent - Enable the parent interrupt (defaults to unmask if
  * NULL)
  * @data:	Pointer to interrupt specific data
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 06/12] genirq: Introduce irq_chip_get/set_parent_state calls

[Stephen Boyd]

Quoting Lina Iyer (2019-11-14 10:35:15)
> From: Maulik Shah <mkshah@codeaurora.org>
> 
> On certain QTI chipsets some GPIOs are direct-connect interrupts to the
> GIC to be used as regular interrupt lines. When the GPIOs are not used
> for interrupt generation the interrupt line is disabled. But disabling
> the interrupt at GIC does not prevent the interrupt to be reported as
> pending at GIC_ISPEND. Later, when drivers call enable_irq() on the
> interrupt, an unwanted interrupt occurs.
> 
> Introduce get and set methods for irqchip's parent to clear it's pending
> irq state. This then can be invoked by the GPIO interrupt controller on
> the parents in it hierarchy to clear the interrupt before enabling the
> interrupt.
> 
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> [updated commit text and minor code fixes]
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---

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

[PATCH 07/12] drivers: irqchip: pdc: Add irqchip set/get state calls

[Lina Iyer]

From: Maulik Shah <mkshah@codeaurora.org>

Add irqchip calls to set/get interrupt state from the parent interrupt
controller. When GPIOs are renabled as interrupt lines, it is desirable
to clear the interrupt state at the GIC. This avoids any unwanted
interrupt as a result of stale pending state recorded when the line was
used as a GPIO.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
[updated commit text, rearranged code]
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
 drivers/irqchip/qcom-pdc.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 4f2c762..6ae9e1f 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -5,6 +5,7 @@
 
 #include <linux/err.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqchip.h>
 #include <linux/irqdomain.h>
@@ -50,6 +51,26 @@ static u32 pdc_reg_read(int reg, u32 i)
 	return readl_relaxed(pdc_base + reg + i * sizeof(u32));
 }
 
+static int qcom_pdc_gic_get_irqchip_state(struct irq_data *d,
+					  enum irqchip_irq_state which,
+					  bool *state)
+{
+	if (d->hwirq == GPIO_NO_WAKE_IRQ)
+		return 0;
+
+	return irq_chip_get_parent_state(d, which, state);
+}
+
+static int qcom_pdc_gic_set_irqchip_state(struct irq_data *d,
+					  enum irqchip_irq_state which,
+					  bool value)
+{
+	if (d->hwirq == GPIO_NO_WAKE_IRQ)
+		return 0;
+
+	return irq_chip_set_parent_state(d, which, value);
+}
+
 static void pdc_enable_intr(struct irq_data *d, bool on)
 {
 	int pin_out = d->hwirq;
@@ -178,6 +199,8 @@ static struct irq_chip qcom_pdc_gic_chip = {
 	.irq_unmask		= qcom_pdc_gic_unmask,
 	.irq_disable		= qcom_pdc_gic_disable,
 	.irq_enable		= qcom_pdc_gic_enable,
+	.irq_get_irqchip_state	= qcom_pdc_gic_get_irqchip_state,
+	.irq_set_irqchip_state	= qcom_pdc_gic_set_irqchip_state,
 	.irq_retrigger		= irq_chip_retrigger_hierarchy,
 	.irq_set_type		= qcom_pdc_gic_set_type,
 	.flags			= IRQCHIP_MASK_ON_SUSPEND |
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 07/12] drivers: irqchip: pdc: Add irqchip set/get state calls

[Stephen Boyd]

Quoting Lina Iyer (2019-11-14 10:35:16)
> From: Maulik Shah <mkshah@codeaurora.org>
> 
> Add irqchip calls to set/get interrupt state from the parent interrupt
> controller. When GPIOs are renabled as interrupt lines, it is desirable
> to clear the interrupt state at the GIC. This avoids any unwanted
> interrupt as a result of stale pending state recorded when the line was
> used as a GPIO.
> 
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> [updated commit text, rearranged code]
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---

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

[PATCH 08/12] drivers: pinctrl: msm: setup GPIO chip in hierarchy

[Lina Iyer]

Some GPIOs are marked as wakeup capable and are routed to another
interrupt controller that is an always-domain and can detect interrupts
even most of the SoC is powered off. The wakeup interrupt controller
wakes up the GIC and replays the interrupt at the GIC.

Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller
and ensure the wakeup GPIOs are handled correctly.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
----
Changes in v1:
	- Address minor review comments
	- Remove redundant call to set irq handler
	- Move irq_domain_qcom_handle_wakeup() to this patch
Changes in RFC v2:
	- Rebase on top of GPIO hierarchy support in linux-next
	- Set the chained irq handler for summary line
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 113 +++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/qcom/pinctrl-msm.h |  16 ++++++
 include/linux/soc/qcom/irq.h       |  13 +++++
 3 files changed, 142 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 763da0b..c245686 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -23,6 +23,8 @@
 #include <linux/pm.h>
 #include <linux/log2.h>
 
+#include <linux/soc/qcom/irq.h>
+
 #include "../core.h"
 #include "../pinconf.h"
 #include "pinctrl-msm.h"
@@ -44,6 +46,7 @@
  * @enabled_irqs:   Bitmap of currently enabled irqs.
  * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
  *                  detection.
+ * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt contrroller
  * @soc;            Reference to soc_data of platform specific data.
  * @regs:           Base addresses for the TLMM tiles.
  */
@@ -61,6 +64,7 @@ struct msm_pinctrl {
 
 	DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
 	DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
+	DECLARE_BITMAP(skip_wake_irqs, MAX_NR_GPIO);
 
 	const struct msm_pinctrl_soc_data *soc;
 	void __iomem *regs[MAX_NR_TILES];
@@ -707,6 +711,12 @@ static void msm_gpio_irq_mask(struct irq_data *d)
 	unsigned long flags;
 	u32 val;
 
+	if (d->parent_data)
+		irq_chip_mask_parent(d);
+
+	if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+		return;
+
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
@@ -751,6 +761,12 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
 	unsigned long flags;
 	u32 val;
 
+	if (d->parent_data)
+		irq_chip_unmask_parent(d);
+
+	if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+		return;
+
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
@@ -778,10 +794,37 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
 
 static void msm_gpio_irq_enable(struct irq_data *d)
 {
+	/*
+	 * Clear the interrupt that may be pending before we enable
+	 * the line.
+	 * This is especially a problem with the GPIOs routed to the
+	 * PDC. These GPIOs are direct-connect interrupts to the GIC.
+	 * Disabling the interrupt line at the PDC does not prevent
+	 * the interrupt from being latched at the GIC. The state at
+	 * GIC needs to be cleared before enabling.
+	 */
+	if (d->parent_data) {
+		irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
+		irq_chip_enable_parent(d);
+	}
 
 	msm_gpio_irq_clear_unmask(d, true);
 }
 
+static void msm_gpio_irq_disable(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+
+	if (d->parent_data)
+		irq_chip_disable_parent(d);
+
+	if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+		return;
+
+	msm_gpio_irq_mask(d);
+}
+
 static void msm_gpio_irq_unmask(struct irq_data *d)
 {
 	msm_gpio_irq_clear_unmask(d, false);
@@ -795,6 +838,9 @@ static void msm_gpio_irq_ack(struct irq_data *d)
 	unsigned long flags;
 	u32 val;
 
+	if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+		return;
+
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
@@ -820,6 +866,12 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	unsigned long flags;
 	u32 val;
 
+	if (d->parent_data)
+		irq_chip_set_type_parent(d, type);
+
+	if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+		return 0;
+
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
@@ -912,6 +964,15 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
 	unsigned long flags;
 
+	if (d->parent_data)
+		irq_chip_set_wake_parent(d, on);
+
+	/*
+	 * While they may not wake up when the TLMM is powered off,
+	 * some GPIOs would like to wakeup the system from suspend
+	 * when TLMM is powered on. To allow that, enable the GPIO
+	 * summary line to be wakeup capable at GIC.
+	 */
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	irq_set_irq_wake(pctrl->irq, on);
@@ -990,6 +1051,30 @@ static void msm_gpio_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static int msm_gpio_wakeirq(struct gpio_chip *gc,
+			    unsigned int child,
+			    unsigned int child_type,
+			    unsigned int *parent,
+			    unsigned int *parent_type)
+{
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	const struct msm_gpio_wakeirq_map *map;
+	int i;
+
+	*parent = GPIO_NO_WAKE_IRQ;
+	*parent_type = IRQ_TYPE_EDGE_RISING;
+
+	for (i = 0; i < pctrl->soc->nwakeirq_map; i++) {
+		map = &pctrl->soc->wakeirq_map[i];
+		if (map->gpio == child) {
+			*parent = map->wakeirq;
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
 {
 	if (pctrl->soc->reserved_gpios)
@@ -1004,6 +1089,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	struct gpio_irq_chip *girq;
 	int ret;
 	unsigned ngpio = pctrl->soc->ngpios;
+	struct device_node *np;
 
 	if (WARN_ON(ngpio > MAX_NR_GPIO))
 		return -EINVAL;
@@ -1020,17 +1106,44 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 
 	pctrl->irq_chip.name = "msmgpio";
 	pctrl->irq_chip.irq_enable = msm_gpio_irq_enable;
+	pctrl->irq_chip.irq_disable = msm_gpio_irq_disable;
 	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;
+	pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent;
 	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
 	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
 	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
 	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
+	np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
+	if (np) {
+		int i;
+		bool skip;
+		unsigned int gpio;
+
+		chip->irq.parent_domain = irq_find_matching_host(np,
+						 DOMAIN_BUS_WAKEUP);
+		of_node_put(np);
+		if (!chip->irq.parent_domain)
+			return -EPROBE_DEFER;
+		chip->irq.child_to_parent_hwirq = msm_gpio_wakeirq;
+
+		/*
+		 * Let's skip handling the GPIOs, if the parent irqchip
+		 * handling the direct connect IRQ of the GPIO.
+		 */
+		skip = irq_domain_qcom_handle_wakeup(chip->irq.parent_domain);
+		for (i = 0; skip && i < pctrl->soc->nwakeirq_map; i++) {
+			gpio = pctrl->soc->wakeirq_map[i].gpio;
+			set_bit(gpio, pctrl->skip_wake_irqs);
+		}
+	}
+
 	girq = &chip->irq;
 	girq->chip = &pctrl->irq_chip;
 	girq->parent_handler = msm_gpio_irq_handler;
+	girq->fwnode = pctrl->dev->fwnode;
 	girq->num_parents = 1;
 	girq->parents = devm_kcalloc(pctrl->dev, 1, sizeof(*girq->parents),
 				     GFP_KERNEL);
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index 48569cda8..1547020 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -5,6 +5,8 @@
 #ifndef __PINCTRL_MSM_H__
 #define __PINCTRL_MSM_H__
 
+#include <linux/gpio/driver.h>
+
 struct pinctrl_pin_desc;
 
 /**
@@ -92,6 +94,16 @@ struct msm_pingroup {
 };
 
 /**
+ * struct msm_gpio_wakeirq_map - Map of GPIOs and their wakeup pins
+ * @gpio:          The GPIOs that are wakeup capable
+ * @wakeirq:       The interrupt at the always-on interrupt controller
+ */
+struct msm_gpio_wakeirq_map {
+	unsigned int gpio;
+	unsigned int wakeirq;
+};
+
+/**
  * struct msm_pinctrl_soc_data - Qualcomm pin controller driver configuration
  * @pins:	    An array describing all pins the pin controller affects.
  * @npins:	    The number of entries in @pins.
@@ -101,6 +113,8 @@ struct msm_pingroup {
  * @ngroups:	    The numbmer of entries in @groups.
  * @ngpio:	    The number of pingroups the driver should expose as GPIOs.
  * @pull_no_keeper: The SoC does not support keeper bias.
+ * @wakeirq_map:    The map of wakeup capable GPIOs and the pin at PDC/MPM
+ * @nwakeirq_map:   The number of entries in @hierarchy_map
  */
 struct msm_pinctrl_soc_data {
 	const struct pinctrl_pin_desc *pins;
@@ -114,6 +128,8 @@ struct msm_pinctrl_soc_data {
 	const char *const *tiles;
 	unsigned int ntiles;
 	const int *reserved_gpios;
+	const struct msm_gpio_wakeirq_map *wakeirq_map;
+	unsigned int nwakeirq_map;
 };
 
 extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h
index 637c0bf..e01391c 100644
--- a/include/linux/soc/qcom/irq.h
+++ b/include/linux/soc/qcom/irq.h
@@ -18,4 +18,17 @@
 #define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP		(IRQ_DOMAIN_FLAG_NONCORE << 0)
 #define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP		(IRQ_DOMAIN_FLAG_NONCORE << 1)
 
+/**
+ * irq_domain_qcom_handle_wakeup: Return if the domain handles interrupt
+ *                                configuration
+ * @d: irq domain
+ *
+ * This QCOM specific irq domain call returns if the interrupt controller
+ * requires the interrupt be masked at the child interrupt controller.
+ */
+static inline bool irq_domain_qcom_handle_wakeup(struct irq_domain *d)
+{
+	return (d->flags & IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP);
+}
+
 #endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 08/12] drivers: pinctrl: msm: setup GPIO chip in hierarchy

[Linus Walleij]

On Thu, Nov 14, 2019 at 7:35 PM Lina Iyer <ilina@codeaurora.org> wrote:

> Some GPIOs are marked as wakeup capable and are routed to another
> interrupt controller that is an always-domain and can detect interrupts
> even most of the SoC is powered off. The wakeup interrupt controller
> wakes up the GIC and replays the interrupt at the GIC.
>
> Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller
> and ensure the wakeup GPIOs are handled correctly.
>
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>

This looks finished, and elegant.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 08/12] drivers: pinctrl: msm: setup GPIO chip in hierarchy

[Lina Iyer]

On Thu, Nov 14 2019 at 11:47 -0700, Linus Walleij wrote:
>On Thu, Nov 14, 2019 at 7:35 PM Lina Iyer <ilina@codeaurora.org> wrote:
>
>> Some GPIOs are marked as wakeup capable and are routed to another
>> interrupt controller that is an always-domain and can detect interrupts
>> even most of the SoC is powered off. The wakeup interrupt controller
>> wakes up the GIC and replays the interrupt at the GIC.
>>
>> Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller
>> and ensure the wakeup GPIOs are handled correctly.
>>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>
>This looks finished, and elegant.
>Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
Thanks Linus.

-- Lina

Re: [PATCH 08/12] drivers: pinctrl: msm: setup GPIO chip in hierarchy

[Stephen Boyd]

Quoting Lina Iyer (2019-11-14 10:35:17)
> Some GPIOs are marked as wakeup capable and are routed to another
> interrupt controller that is an always-domain and can detect interrupts
> even most of the SoC is powered off. The wakeup interrupt controller

even when?

> wakes up the GIC and replays the interrupt at the GIC.
> 
> Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller
> and ensure the wakeup GPIOs are handled correctly.
> 
> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>

Is it Co-developed-by for Maulik?

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

Some minor comments. Shouldn't be hard to fix and resend quickly I hope.

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 763da0b..c245686 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -44,6 +46,7 @@
>   * @enabled_irqs:   Bitmap of currently enabled irqs.
>   * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
>   *                  detection.
> + * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt contrroller

s/contrroller/controller/

>   * @soc;            Reference to soc_data of platform specific data.
>   * @regs:           Base addresses for the TLMM tiles.
>   */
> @@ -778,10 +794,37 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
>  
>  static void msm_gpio_irq_enable(struct irq_data *d)
>  {
> +       /*
> +        * Clear the interrupt that may be pending before we enable
> +        * the line.
> +        * This is especially a problem with the GPIOs routed to the
> +        * PDC. These GPIOs are direct-connect interrupts to the GIC.
> +        * Disabling the interrupt line at the PDC does not prevent
> +        * the interrupt from being latched at the GIC. The state at
> +        * GIC needs to be cleared before enabling.
> +        */
> +       if (d->parent_data) {
> +               irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
> +               irq_chip_enable_parent(d);
> +       }
>  
>         msm_gpio_irq_clear_unmask(d, true);
>  }
>  
> +static void msm_gpio_irq_disable(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +
> +       if (d->parent_data)
> +               irq_chip_disable_parent(d);
> +
> +       if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
> +               return;
> +
> +       msm_gpio_irq_mask(d);

Why not

	if (!test_bit(...)
		msm_gpio_irq_mask(d);

> +}
> +
>  static void msm_gpio_irq_unmask(struct irq_data *d)
>  {
>         msm_gpio_irq_clear_unmask(d, false);
> @@ -912,6 +964,15 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
>         struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>         unsigned long flags;
>  
> +       if (d->parent_data)
> +               irq_chip_set_wake_parent(d, on);
> +
> +       /*
> +        * While they may not wake up when the TLMM is powered off,
> +        * some GPIOs would like to wakeup the system from suspend
> +        * when TLMM is powered on. To allow that, enable the GPIO
> +        * summary line to be wakeup capable at GIC.
> +        */

Can this comment go above the irq_set_irq_wake() line below instead of
this spinlock?

>         raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
>         irq_set_irq_wake(pctrl->irq, on);
> @@ -990,6 +1051,30 @@ static void msm_gpio_irq_handler(struct irq_desc *desc)
>         chained_irq_exit(chip, desc);
>  }
>  
> +static int msm_gpio_wakeirq(struct gpio_chip *gc,
> +                           unsigned int child,
> +                           unsigned int child_type,
> +                           unsigned int *parent,
> +                           unsigned int *parent_type)
> +{
> +       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +       const struct msm_gpio_wakeirq_map *map;
> +       int i;
> +
> +       *parent = GPIO_NO_WAKE_IRQ;
> +       *parent_type = IRQ_TYPE_EDGE_RISING;
> +
> +       for (i = 0; i < pctrl->soc->nwakeirq_map; i++) {
> +               map = &pctrl->soc->wakeirq_map[i];
> +               if (map->gpio == child) {
> +                       *parent = map->wakeirq;
> +                       break;
> +               }
> +       }
> +
> +       return 0;

Shouldn't we return -EINVAL if we can't translate the gpio irq to the
parent domain? I would expect to see return -EINVAL here and the above
if condition to return 0 instead of break.

> +}
> +
>  static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
>  {
>         if (pctrl->soc->reserved_gpios)
> @@ -1004,6 +1089,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>         struct gpio_irq_chip *girq;
>         int ret;
>         unsigned ngpio = pctrl->soc->ngpios;
> +       struct device_node *np;
>  
>         if (WARN_ON(ngpio > MAX_NR_GPIO))
>                 return -EINVAL;
> @@ -1020,17 +1106,44 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>  
>         pctrl->irq_chip.name = "msmgpio";
>         pctrl->irq_chip.irq_enable = msm_gpio_irq_enable;
> +       pctrl->irq_chip.irq_disable = msm_gpio_irq_disable;
>         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;
> +       pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent;
>         pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
>         pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
>         pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
>         pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
>  
> +       np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
> +       if (np) {
> +               int i;
> +               bool skip;
> +               unsigned int gpio;

Can these be placed at the top of this function instead of buried
halfway down here?

> +
> +               chip->irq.parent_domain = irq_find_matching_host(np,
> +                                                DOMAIN_BUS_WAKEUP);
> +               of_node_put(np);
> +               if (!chip->irq.parent_domain)
> +                       return -EPROBE_DEFER;
> +               chip->irq.child_to_parent_hwirq = msm_gpio_wakeirq;
> +
> +               /*
> +                * Let's skip handling the GPIOs, if the parent irqchip
> +                * handling the direct connect IRQ of the GPIO.

is handling?

> +                */
> +               skip = irq_domain_qcom_handle_wakeup(chip->irq.parent_domain);
> +               for (i = 0; skip && i < pctrl->soc->nwakeirq_map; i++) {
> +                       gpio = pctrl->soc->wakeirq_map[i].gpio;
> +                       set_bit(gpio, pctrl->skip_wake_irqs);
> +               }
> +       }
> +
>         girq = &chip->irq;
>         girq->chip = &pctrl->irq_chip;
>         girq->parent_handler = msm_gpio_irq_handler;
> +       girq->fwnode = pctrl->dev->fwnode;
>         girq->num_parents = 1;
>         girq->parents = devm_kcalloc(pctrl->dev, 1, sizeof(*girq->parents),
>                                      GFP_KERNEL);
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index 48569cda8..1547020 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -5,6 +5,8 @@
>  #ifndef __PINCTRL_MSM_H__
>  #define __PINCTRL_MSM_H__
>  
> +#include <linux/gpio/driver.h>

What is this include for?

> +
>  struct pinctrl_pin_desc;
>  
>  /**
> @@ -101,6 +113,8 @@ struct msm_pingroup {
>   * @ngroups:       The numbmer of entries in @groups.
>   * @ngpio:         The number of pingroups the driver should expose as GPIOs.
>   * @pull_no_keeper: The SoC does not support keeper bias.
> + * @wakeirq_map:    The map of wakeup capable GPIOs and the pin at PDC/MPM
> + * @nwakeirq_map:   The number of entries in @hierarchy_map

Is it 'number of entries in @wakeirq_map"?

>   */
>  struct msm_pinctrl_soc_data {
>         const struct pinctrl_pin_desc *pins;
> @@ -114,6 +128,8 @@ struct msm_pinctrl_soc_data {
>         const char *const *tiles;
>         unsigned int ntiles;
>         const int *reserved_gpios;
> +       const struct msm_gpio_wakeirq_map *wakeirq_map;
> +       unsigned int nwakeirq_map;
>  };
>  
>  extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
> diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h
> index 637c0bf..e01391c 100644
> --- a/include/linux/soc/qcom/irq.h
> +++ b/include/linux/soc/qcom/irq.h
> @@ -18,4 +18,17 @@
>  #define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP                (IRQ_DOMAIN_FLAG_NONCORE << 0)
>  #define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP                (IRQ_DOMAIN_FLAG_NONCORE << 1)
>  
> +/**
> + * irq_domain_qcom_handle_wakeup: Return if the domain handles interrupt
> + *                                configuration
> + * @d: irq domain
> + *
> + * This QCOM specific irq domain call returns if the interrupt controller
> + * requires the interrupt be masked at the child interrupt controller.
> + */
> +static inline bool irq_domain_qcom_handle_wakeup(struct irq_domain *d)

could be const irq_domain here.

> +{
> +       return (d->flags & IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP);
> +}
> +

Re: [PATCH 08/12] drivers: pinctrl: msm: setup GPIO chip in hierarchy

[Lina Iyer]

>Quoting Lina Iyer (2019-11-14 10:35:17)
>> Some GPIOs are marked as wakeup capable and are routed to another
>> interrupt controller that is an always-domain and can detect interrupts
>> even most of the SoC is powered off. The wakeup interrupt controller
>
>even when?
>
>> wakes up the GIC and replays the interrupt at the GIC.
>>
>> Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller
>> and ensure the wakeup GPIOs are handled correctly.
>>
>> Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
>
>Is it Co-developed-by for Maulik?
>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>
>Some minor comments. Shouldn't be hard to fix and resend quickly I hope.
>
Thanks for the review Stephen. I will fix these and resend.

>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 763da0b..c245686 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> @@ -44,6 +46,7 @@
>>   * @enabled_irqs:   Bitmap of currently enabled irqs.
>>   * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
>>   *                  detection.
>> + * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt contrroller
>
>s/contrroller/controller/
>
>>   * @soc;            Reference to soc_data of platform specific data.
>>   * @regs:           Base addresses for the TLMM tiles.
>>   */
>> @@ -778,10 +794,37 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
>>
>>  static void msm_gpio_irq_enable(struct irq_data *d)
>>  {
>> +       /*
>> +        * Clear the interrupt that may be pending before we enable
>> +        * the line.
>> +        * This is especially a problem with the GPIOs routed to the
>> +        * PDC. These GPIOs are direct-connect interrupts to the GIC.
>> +        * Disabling the interrupt line at the PDC does not prevent
>> +        * the interrupt from being latched at the GIC. The state at
>> +        * GIC needs to be cleared before enabling.
>> +        */
>> +       if (d->parent_data) {
>> +               irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
>> +               irq_chip_enable_parent(d);
>> +       }
>>
>>         msm_gpio_irq_clear_unmask(d, true);
>>  }
>>
>> +static void msm_gpio_irq_disable(struct irq_data *d)
>> +{
>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>> +
>> +       if (d->parent_data)
>> +               irq_chip_disable_parent(d);
>> +
>> +       if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
>> +               return;
>> +
>> +       msm_gpio_irq_mask(d);
>
>Why not
>
>	if (!test_bit(...)
>		msm_gpio_irq_mask(d);
>
>> +}
>> +
>>  static void msm_gpio_irq_unmask(struct irq_data *d)
>>  {
>>         msm_gpio_irq_clear_unmask(d, false);
>> @@ -912,6 +964,15 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
>>         struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>>         unsigned long flags;
>>
>> +       if (d->parent_data)
>> +               irq_chip_set_wake_parent(d, on);
>> +
>> +       /*
>> +        * While they may not wake up when the TLMM is powered off,
>> +        * some GPIOs would like to wakeup the system from suspend
>> +        * when TLMM is powered on. To allow that, enable the GPIO
>> +        * summary line to be wakeup capable at GIC.
>> +        */
>
>Can this comment go above the irq_set_irq_wake() line below instead of
>this spinlock?
>
Sure.

>>         raw_spin_lock_irqsave(&pctrl->lock, flags);
>>
>>         irq_set_irq_wake(pctrl->irq, on);
>> @@ -990,6 +1051,30 @@ static void msm_gpio_irq_handler(struct irq_desc *desc)
>>         chained_irq_exit(chip, desc);
>>  }
>>
>> +static int msm_gpio_wakeirq(struct gpio_chip *gc,
>> +                           unsigned int child,
>> +                           unsigned int child_type,
>> +                           unsigned int *parent,
>> +                           unsigned int *parent_type)
>> +{
>> +       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>> +       const struct msm_gpio_wakeirq_map *map;
>> +       int i;
>> +
>> +       *parent = GPIO_NO_WAKE_IRQ;
>> +       *parent_type = IRQ_TYPE_EDGE_RISING;
>> +
>> +       for (i = 0; i < pctrl->soc->nwakeirq_map; i++) {
>> +               map = &pctrl->soc->wakeirq_map[i];
>> +               if (map->gpio == child) {
>> +                       *parent = map->wakeirq;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       return 0;
>
>Shouldn't we return -EINVAL if we can't translate the gpio irq to the
>parent domain? I would expect to see return -EINVAL here and the above
>if condition to return 0 instead of break.
>
Good catch. Sure.
>> +}
>> +
>>  static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
>>  {
>>         if (pctrl->soc->reserved_gpios)
>> @@ -1004,6 +1089,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>>         struct gpio_irq_chip *girq;
>>         int ret;
>>         unsigned ngpio = pctrl->soc->ngpios;
>> +       struct device_node *np;
>>
>>         if (WARN_ON(ngpio > MAX_NR_GPIO))
>>                 return -EINVAL;
>> @@ -1020,17 +1106,44 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>>
>>         pctrl->irq_chip.name = "msmgpio";
>>         pctrl->irq_chip.irq_enable = msm_gpio_irq_enable;
>> +       pctrl->irq_chip.irq_disable = msm_gpio_irq_disable;
>>         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;
>> +       pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent;
>>         pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
>>         pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
>>         pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
>>         pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
>>
>> +       np = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0);
>> +       if (np) {
>> +               int i;
>> +               bool skip;
>> +               unsigned int gpio;
>
>Can these be placed at the top of this function instead of buried
>halfway down here?
>
>> +
>> +               chip->irq.parent_domain = irq_find_matching_host(np,
>> +                                                DOMAIN_BUS_WAKEUP);
>> +               of_node_put(np);
>> +               if (!chip->irq.parent_domain)
>> +                       return -EPROBE_DEFER;
>> +               chip->irq.child_to_parent_hwirq = msm_gpio_wakeirq;
>> +
>> +               /*
>> +                * Let's skip handling the GPIOs, if the parent irqchip
>> +                * handling the direct connect IRQ of the GPIO.
>
>is handling?
>
>> +                */
>> +               skip = irq_domain_qcom_handle_wakeup(chip->irq.parent_domain);
>> +               for (i = 0; skip && i < pctrl->soc->nwakeirq_map; i++) {
>> +                       gpio = pctrl->soc->wakeirq_map[i].gpio;
>> +                       set_bit(gpio, pctrl->skip_wake_irqs);
>> +               }
>> +       }
>> +
>>         girq = &chip->irq;
>>         girq->chip = &pctrl->irq_chip;
>>         girq->parent_handler = msm_gpio_irq_handler;
>> +       girq->fwnode = pctrl->dev->fwnode;
>>         girq->num_parents = 1;
>>         girq->parents = devm_kcalloc(pctrl->dev, 1, sizeof(*girq->parents),
>>                                      GFP_KERNEL);
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
>> index 48569cda8..1547020 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
>> @@ -5,6 +5,8 @@
>>  #ifndef __PINCTRL_MSM_H__
>>  #define __PINCTRL_MSM_H__
>>
>> +#include <linux/gpio/driver.h>
>
>What is this include for?
>
Must be from an older version. Will remove.

>> +
>>  struct pinctrl_pin_desc;
>>
>>  /**
>> @@ -101,6 +113,8 @@ struct msm_pingroup {
>>   * @ngroups:       The numbmer of entries in @groups.
>>   * @ngpio:         The number of pingroups the driver should expose as GPIOs.
>>   * @pull_no_keeper: The SoC does not support keeper bias.
>> + * @wakeirq_map:    The map of wakeup capable GPIOs and the pin at PDC/MPM
>> + * @nwakeirq_map:   The number of entries in @hierarchy_map
>
>Is it 'number of entries in @wakeirq_map"?
>
Yes. Thanks.
>>   */
>>  struct msm_pinctrl_soc_data {
>>         const struct pinctrl_pin_desc *pins;
>> @@ -114,6 +128,8 @@ struct msm_pinctrl_soc_data {
>>         const char *const *tiles;
>>         unsigned int ntiles;
>>         const int *reserved_gpios;
>> +       const struct msm_gpio_wakeirq_map *wakeirq_map;
>> +       unsigned int nwakeirq_map;
>>  };
>>
>>  extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
>> diff --git a/include/linux/soc/qcom/irq.h b/include/linux/soc/qcom/irq.h
>> index 637c0bf..e01391c 100644
>> --- a/include/linux/soc/qcom/irq.h
>> +++ b/include/linux/soc/qcom/irq.h
>> @@ -18,4 +18,17 @@
>>  #define IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP                (IRQ_DOMAIN_FLAG_NONCORE << 0)
>>  #define IRQ_DOMAIN_FLAG_QCOM_MPM_WAKEUP                (IRQ_DOMAIN_FLAG_NONCORE << 1)
>>
>> +/**
>> + * irq_domain_qcom_handle_wakeup: Return if the domain handles interrupt
>> + *                                configuration
>> + * @d: irq domain
>> + *
>> + * This QCOM specific irq domain call returns if the interrupt controller
>> + * requires the interrupt be masked at the child interrupt controller.
>> + */
>> +static inline bool irq_domain_qcom_handle_wakeup(struct irq_domain *d)
>
>could be const irq_domain here.
>
Ok.
>> +{
>> +       return (d->flags & IRQ_DOMAIN_FLAG_QCOM_PDC_WAKEUP);
>> +}
>> +

Thanks,
Lina

Re: [PATCH 08/12] drivers: pinctrl: msm: setup GPIO chip in hierarchy

[Lina Iyer]

On Fri, Nov 15 2019 at 13:55 -0700, Lina Iyer wrote:
>>Quoting Lina Iyer (2019-11-14 10:35:17)

>>>+static int msm_gpio_wakeirq(struct gpio_chip *gc,
>>>+                           unsigned int child,
>>>+                           unsigned int child_type,
>>>+                           unsigned int *parent,
>>>+                           unsigned int *parent_type)
>>>+{
>>>+       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>>>+       const struct msm_gpio_wakeirq_map *map;
>>>+       int i;
>>>+
>>>+       *parent = GPIO_NO_WAKE_IRQ;
>>>+       *parent_type = IRQ_TYPE_EDGE_RISING;
>>>+
>>>+       for (i = 0; i < pctrl->soc->nwakeirq_map; i++) {
>>>+               map = &pctrl->soc->wakeirq_map[i];
>>>+               if (map->gpio == child) {
>>>+                       *parent = map->wakeirq;
>>>+                       break;
>>>+               }
>>>+       }
>>>+
>>>+       return 0;
>>
>>Shouldn't we return -EINVAL if we can't translate the gpio irq to the
>>parent domain? I would expect to see return -EINVAL here and the above
>>if condition to return 0 instead of break.
>>
>Good catch. Sure.
>>>+}
>>>+
Looking into this, we have been setting GPIO in hierarchy with PDC and
the return 0 is appropriate as it would set the GPIO_NO_WAKE_IRQ as the
parent and setup the IRQ correctly. We fail to setup GPIOs otherwise.

Re: [PATCH 08/12] drivers: pinctrl: msm: setup GPIO chip in hierarchy

[Stephen Boyd]

Quoting Lina Iyer (2019-11-15 13:57:37)
> On Fri, Nov 15 2019 at 13:55 -0700, Lina Iyer wrote:
> >>Quoting Lina Iyer (2019-11-14 10:35:17)
> 
> >>>+static int msm_gpio_wakeirq(struct gpio_chip *gc,
> >>>+                           unsigned int child,
> >>>+                           unsigned int child_type,
> >>>+                           unsigned int *parent,
> >>>+                           unsigned int *parent_type)
> >>>+{
> >>>+       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> >>>+       const struct msm_gpio_wakeirq_map *map;
> >>>+       int i;
> >>>+
> >>>+       *parent = GPIO_NO_WAKE_IRQ;
> >>>+       *parent_type = IRQ_TYPE_EDGE_RISING;
> >>>+
> >>>+       for (i = 0; i < pctrl->soc->nwakeirq_map; i++) {
> >>>+               map = &pctrl->soc->wakeirq_map[i];
> >>>+               if (map->gpio == child) {
> >>>+                       *parent = map->wakeirq;
> >>>+                       break;
> >>>+               }
> >>>+       }
> >>>+
> >>>+       return 0;
> >>
> >>Shouldn't we return -EINVAL if we can't translate the gpio irq to the
> >>parent domain? I would expect to see return -EINVAL here and the above
> >>if condition to return 0 instead of break.
> >>
> >Good catch. Sure.
> >>>+}
> >>>+
> Looking into this, we have been setting GPIO in hierarchy with PDC and
> the return 0 is appropriate as it would set the GPIO_NO_WAKE_IRQ as the
> parent and setup the IRQ correctly. We fail to setup GPIOs otherwise.

Ah ok so by default we will set the parent irq to ~0 and that means
bypass pdc and go directly to GIC?

Where do we fail to setup a GPIO otherwise? It sounds like we can always
translate to either something in the map or to ~0.

Re: [PATCH 08/12] drivers: pinctrl: msm: setup GPIO chip in hierarchy

[Lina Iyer]

On Fri, Nov 15 2019 at 15:09 -0700, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-11-15 13:57:37)
>> On Fri, Nov 15 2019 at 13:55 -0700, Lina Iyer wrote:
>> >>Quoting Lina Iyer (2019-11-14 10:35:17)
>>
>> >>>+static int msm_gpio_wakeirq(struct gpio_chip *gc,
>> >>>+                           unsigned int child,
>> >>>+                           unsigned int child_type,
>> >>>+                           unsigned int *parent,
>> >>>+                           unsigned int *parent_type)
>> >>>+{
>> >>>+       struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>> >>>+       const struct msm_gpio_wakeirq_map *map;
>> >>>+       int i;
>> >>>+
>> >>>+       *parent = GPIO_NO_WAKE_IRQ;
>> >>>+       *parent_type = IRQ_TYPE_EDGE_RISING;
>> >>>+
>> >>>+       for (i = 0; i < pctrl->soc->nwakeirq_map; i++) {
>> >>>+               map = &pctrl->soc->wakeirq_map[i];
>> >>>+               if (map->gpio == child) {
>> >>>+                       *parent = map->wakeirq;
>> >>>+                       break;
>> >>>+               }
>> >>>+       }
>> >>>+
>> >>>+       return 0;
>> >>
>> >>Shouldn't we return -EINVAL if we can't translate the gpio irq to the
>> >>parent domain? I would expect to see return -EINVAL here and the above
>> >>if condition to return 0 instead of break.
>> >>
>> >Good catch. Sure.
>> >>>+}
>> >>>+
>> Looking into this, we have been setting GPIO in hierarchy with PDC and
>> the return 0 is appropriate as it would set the GPIO_NO_WAKE_IRQ as the
>> parent and setup the IRQ correctly. We fail to setup GPIOs otherwise.
>
>Ah ok so by default we will set the parent irq to ~0 and that means
>bypass pdc and go directly to GIC?
>
>Where do we fail to setup a GPIO otherwise? It sounds like we can always
>translate to either something in the map or to ~0.
>
We do not, may be other architectures can use this check better.

--lina

[PATCH 09/12] drivers: pinctrl: sdm845: add PDC wakeup interrupt map for GPIOs

[Lina Iyer]

Add interrupt parents for wakeup capable GPIOs for Qualcomm SDM845 SoC.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes in RFC v2:
	- Rearranged GPIO wakeup parent map
---
 drivers/pinctrl/qcom/pinctrl-sdm845.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
index ce49597..2834d2c 100644
--- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
+++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2016-2019, The Linux Foundation. All rights reserved.
  */
 
 #include <linux/acpi.h>
@@ -1282,6 +1282,24 @@ static const int sdm845_acpi_reserved_gpios[] = {
 	0, 1, 2, 3, 81, 82, 83, 84, -1
 };
 
+static const struct msm_gpio_wakeirq_map sdm845_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 msm_pinctrl_soc_data sdm845_pinctrl = {
 	.pins = sdm845_pins,
 	.npins = ARRAY_SIZE(sdm845_pins),
@@ -1290,6 +1308,9 @@ static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
 	.groups = sdm845_groups,
 	.ngroups = ARRAY_SIZE(sdm845_groups),
 	.ngpios = 151,
+	.wakeirq_map = sdm845_pdc_map,
+	.nwakeirq_map = ARRAY_SIZE(sdm845_pdc_map),
+
 };
 
 static const struct msm_pinctrl_soc_data sdm845_acpi_pinctrl = {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 09/12] drivers: pinctrl: sdm845: add PDC wakeup interrupt map for GPIOs

[Stephen Boyd]

Quoting Lina Iyer (2019-11-14 10:35:18)
> Add interrupt parents for wakeup capable GPIOs for Qualcomm SDM845 SoC.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---

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

[PATCH 10/12] 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>
---
 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 da8aa59..fb060a4 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2939,6 +2939,15 @@
 			#power-domain-cells = <1>;
 		};
 
+		pdc_intc: interrupt-controller@b220000 {
+			compatible = "qcom,sdm845-pdc", "qcom,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;
+		};
+
 		pdc_reset: reset-controller@b2e0000 {
 			compatible = "qcom,sdm845-pdc-global";
 			reg = <0 0x0b2e0000 0 0x20000>;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 10/12] arm64: dts: qcom: add PDC interrupt controller for SDM845

[Stephen Boyd]

Quoting Lina Iyer (2019-11-14 10:35:19)
> Add PDC interrupt controller device bindings for SDM845.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---

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

[PATCH 11/12] arm64: dts: qcom: setup PDC as the wakeup parent for TLMM on SDM845

[Lina Iyer]

PDC always-on interrupt controller can detect certain GPIOs even when
the TLMM interrupt controller is powered off. Link the PDC as TLMM's
wakeup parent.

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 fb060a4..6d2dfd7 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1447,6 +1447,7 @@
 			interrupt-controller;
 			#interrupt-cells = <2>;
 			gpio-ranges = <&tlmm 0 0 150>;
+			wakeup-parent = <&pdc_intc>;
 
 			qspi_clk: qspi-clk {
 				pinmux {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 11/12] arm64: dts: qcom: setup PDC as the wakeup parent for TLMM on SDM845

[Stephen Boyd]

Quoting Lina Iyer (2019-11-14 10:35:20)
> PDC always-on interrupt controller can detect certain GPIOs even when
> the TLMM interrupt controller is powered off. Link the PDC as TLMM's
> wakeup parent.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---

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

[PATCH 12/12] 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 c9a867a..8d8d4d5 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -749,6 +749,7 @@ CONFIG_ARCH_R8A77970=y
 CONFIG_ARCH_R8A77980=y
 CONFIG_ARCH_R8A77990=y
 CONFIG_ARCH_R8A77995=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 12/12] arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845

[Stephen Boyd]

Quoting Lina Iyer (2019-11-14 10:35:21)
> 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>
> ---

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

Re: [PATCH 00/12] Support wakeup capable GPIOs

[Marc Zyngier]

Hi Lina,

On 2019-11-14 18:35, Lina Iyer wrote:
> Hi all,
>
> Thanks for all the reviews.
>
> Here is the next spin of the wakeup capable GPIO support. In order to
> facilitate basic support available in the kernel, I have dropped the 
> SPI
> register configuration. The feature was added when this series was
> restarted based on new hierarchy support in gpiolib. But, the SPI
> configuration can be done in the firmware. This would avoid a whole 
> lot
> of code in linux that serve little to no purpose. Users of GPIO never
> have the need to change the trigger type (level edge and vice-versa) 
> and
> the basic configuration can be set in the firmware before boot.
>
> Changes in v1:
> 	- Address review comments
> 	- Add Reviewed-by tags
> 	- Drop SPI config patches
> 	- Rebase on top of Rajendra's PDC changes [6]
>
> Changes in RFC v2[5]:
>         - Address review comments #3, #4, #6, #7, #8, #9, #10
>         - Rebased on top of linux-next GPIO latest patches 
> [1],[3],[4]
>         - Increase PDC max irqs in #2 (avoid merge conflicts with
>           downstream)
>         - Add Reviewed-by #5
>
> Please consider reviewing these patches.

It has been a long time coming, and I'm minded to take the first 9
patches into the irqchip tree. Anyone objects? The last 3 patches
can go via the platform maintainer tree.

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

Re: [PATCH 00/12] Support wakeup capable GPIOs

[Lina Iyer]

On Fri, Nov 15 2019 at 03:36 -0700, Marc Zyngier wrote:
>Hi Lina,
>
>On 2019-11-14 18:35, Lina Iyer wrote:
>>Hi all,
>>
>>Thanks for all the reviews.
>>
>>Here is the next spin of the wakeup capable GPIO support. In order to
>>facilitate basic support available in the kernel, I have dropped the
>>SPI
>>register configuration. The feature was added when this series was
>>restarted based on new hierarchy support in gpiolib. But, the SPI
>>configuration can be done in the firmware. This would avoid a whole
>>lot
>>of code in linux that serve little to no purpose. Users of GPIO never
>>have the need to change the trigger type (level edge and vice-versa)
>>and
>>the basic configuration can be set in the firmware before boot.
>>
>>Changes in v1:
>>	- Address review comments
>>	- Add Reviewed-by tags
>>	- Drop SPI config patches
>>	- Rebase on top of Rajendra's PDC changes [6]
>>
>>Changes in RFC v2[5]:
>>        - Address review comments #3, #4, #6, #7, #8, #9, #10
>>        - Rebased on top of linux-next GPIO latest patches
>>[1],[3],[4]
>>        - Increase PDC max irqs in #2 (avoid merge conflicts with
>>          downstream)
>>        - Add Reviewed-by #5
>>
>>Please consider reviewing these patches.
>
>It has been a long time coming, and I'm minded to take the first 9
>patches into the irqchip tree. Anyone objects? The last 3 patches
>can go via the platform maintainer tree.
>
Sounds good Marc.

Thanks,
Lina

Re: [PATCH 00/12] Support wakeup capable GPIOs

[Stephen Boyd]

Quoting Lina Iyer (2019-11-14 10:35:09)
> Hi all,
> 
> Thanks for all the reviews.
> 
> Here is the next spin of the wakeup capable GPIO support. In order to
> facilitate basic support available in the kernel, I have dropped the SPI
> register configuration. The feature was added when this series was
> restarted based on new hierarchy support in gpiolib. But, the SPI
> configuration can be done in the firmware. This would avoid a whole lot
> of code in linux that serve little to no purpose. Users of GPIO never
> have the need to change the trigger type (level edge and vice-versa) and
> the basic configuration can be set in the firmware before boot.

Awesome! I'm glad we don't need to worry about configuring that in the
kernel.