[PATCH v3 0/5] Wakeup GPIO support for SDM845

[PATCH v3 0/5] Wakeup GPIO support for SDM845

[Lina Iyer]

Hi,

Changes in v3:
	- Release memory allocated for IRQ anme
	- Minor fixes as suggested by Mathias, Bjorn, Evan

Changes in v2:
        - Compile and test on 4.18 on SDM845
        - Fix IRQ map in patch #3
        - Address review comments
        (I still need to find a way to free memory allocated for PDC IRQ.)
        - Specify type for IRQ in DT
        - This series needs V3 of the PDC DT bingings [4]
        - gic-v3 settings are also needed [5]

Changes in v1:
        - Avoid GPIO-PDC map in .c file
        - Trigger GPIO by writing to the hardware
        - Hooked up to suspend/resume callbacks
        - Dropped PDC DT bindings (see dependencies)

This is an attempt at a solution to enable wake up from suspend and deep idle
using GPIO as a wakeup source. The 845 uses a new interrupt controller (PDC)
that lies in the always-on domain and can sense interrupts that are routed to
it, when the GIC is powered off. It would then wakeup the GIC and replay the
interrupt which would then be relayed to the AP. The PDC interrupt controller
driver is merged upstream [1],[2]. The following set of patches extends the
wakeup capability to GPIOs using the PDC. The TLMM pinctrl driver for the SoC
available at [3].

The complexity with the solution stems from the fact that only a selected few
GPIO lines are routed to the PDC in addition the TLMMs. They are also from
different banks on the pinctrl and the TLMM summary line is not routed to the
PDC. Hence the PDC cannot be considered as parent of the TLMM irqchip (or can
we ?). This is what it looks like -

   [ PIN ] -----[ TLMM ]---------------> [ GIC ] ---> [ CPU ]
      |                  <summary IRQ>       ^
      |                                      |
      ----------------------------------> [ PDC ]
                <wakeup IRQs>

I had a brief discussion with Linus on this and the idea implemented is based
on his suggestion.

When an IRQ (let's call this latent IRQ) for a GPIO is requested, the
->irq_request_resources() is used by the TLMM driver to request a PDC pin. The
PDC pin associated with the GPIO is read from a  static map available in the
pinctrl-sdm845.c. (I think there should be a better location than a static map,
more on that later). Knowing the PDC pin from the map, we could look up the DT
bindings and request the PDC interrupt with the same trigger mask as the
interrupt requested. The ->set_type and ->set_wake are also trapped to set the
PDC IRQ's polarity and enable it when the latent IRQ is requested. When the PDC
detects the interrupt at suspend, it wakes up the GIC and replays the wakeup
IRQ. The GPIO handler function for the latent IRQ is invoked in turn.

Please review these patches and your inputs would be greatly appreciated and
(kindly) let me know if I have committed any blunders with this approach. There
is definitely opportunity to improve the location of the static GPIO-PDC pin
map. We could possibly put it as an data argument in the interrupts definition
of the PDC or with interrupt names. Also, I am still sorting out some issues
with the IRQ handling part of these patches. And I am unsure of how to set the
polarity of the PDC pin without locking, since we are not in hierarchy with the
PDC interrupt controller. Again, your inputs on these would be greatly helpful.

Thanks,
Lina

[1]. drivers/irqchip/qcom-pdc.c
[2]. Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
[3]. drivers/pinctrl/qcom/pinctrl-msm.c
[4]. https://lore.kernel.org/patchwork/patch/977589/
[5]. https://lore.kernel.org/patchwork/patch/975425/

Lina Iyer (5):
  drivers: pinctrl: qcom: add wakeup capability to GPIO
  dt-bindings: pinctrl: add wakeup capable GPIOs for SDM845
  drivers: pinctrl: msm: enable PDC interrupt only during suspend
  drivers: pinctrl: qcom: sdm845: support GPIO wakeup from suspend
  arm64: dts: qcom: add wake up interrupts for GPIOs for SDM845

 .../bindings/pinctrl/qcom,sdm845-pinctrl.txt  | 104 ++++++++++-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          | 152 ++++++++++++++-
 drivers/pinctrl/qcom/pinctrl-msm.c            | 174 ++++++++++++++++++
 drivers/pinctrl/qcom/pinctrl-msm.h            |   5 +
 drivers/pinctrl/qcom/pinctrl-sdm845.c         |   1 +
 5 files changed, 432 insertions(+), 4 deletions(-)

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

[PATCH v3 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
domain can wakeup the SoC, when interrupts and GPIOs are routed to its
interrupt controller. Only select GPIOs that are deemed wakeup capable
are routed to specific PDC pins. During low power state, the pinmux
interrupt controller may be non-functional but the PDC would be. The PDC
can detect the wakeup GPIO is triggered and bring the TLMM to an
operational state.

Interrupts that are level triggered will be detected at the TLMM when
the controller becomes operational. Edge interrupts however need to be
replayed again.

Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
but keep it disabled. During suspend, we can enable the PDC IRQ instead
of the GPIO IRQ, which may or not be detected.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v3:
	- free action->name
Changes in v2:
	- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
Changes in v1:
	- Trigger GPIO in h/w from PDC IRQ handler
	- Avoid big tables for GPIO-PDC map, pick from DT instead
	- Use handler_data
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 0e22f52b2a19..6527a0a9edd1 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -687,11 +687,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	const struct msm_pingroup *g;
 	unsigned long flags;
 	u32 val;
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
 
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	if (pdc_irqd)
+		irq_set_irq_type(pdc_irqd->irq, type);
+
 	/*
 	 * For hw without possibility of detecting both edges
 	 */
@@ -779,9 +783,13 @@ 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;
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	if (pdc_irqd)
+		irq_set_irq_wake(pdc_irqd->irq, on);
+
 	irq_set_irq_wake(pctrl->irq, on);
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
@@ -863,6 +871,94 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
 }
 
+static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
+{
+	struct irq_data *irqd = data;
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	const struct msm_pingroup *g;
+	unsigned long flags;
+	u32 val;
+
+	if (!irqd_is_level_type(irqd)) {
+		g = &pctrl->soc->groups[irqd->hwirq];
+		raw_spin_lock_irqsave(&pctrl->lock, flags);
+		val = BIT(g->intr_status_bit);
+		writel(val, pctrl->regs + g->intr_status_reg);
+		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int msm_gpio_pdc_pin_request(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	struct platform_device *pdev = to_platform_device(pctrl->dev);
+	const char *pin_name;
+	int irq;
+	int ret;
+
+	pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);
+	if (!pin_name)
+		return -ENOMEM;
+
+	irq = platform_get_irq_byname(pdev, pin_name);
+	if (irq < 0) {
+		kfree(pin_name);
+		return 0;
+	}
+
+	ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d),
+			  pin_name, d);
+	if (ret) {
+		pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq);
+		kfree(pin_name);
+		return ret;
+	}
+
+	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
+	disable_irq(irq);
+
+	return 0;
+}
+
+static int msm_gpio_pdc_pin_release(struct irq_data *d)
+{
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
+	const void *name;
+
+	if (pdc_irqd) {
+		irq_set_handler_data(d->irq, NULL);
+		name = free_irq(pdc_irqd->irq, d);
+		kfree(name);
+	}
+
+	return 0;
+}
+
+static int msm_gpio_irq_reqres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
+		dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
+			irqd_to_hwirq(d));
+		return -EINVAL;
+	}
+
+	return msm_gpio_pdc_pin_request(d);
+}
+
+static void msm_gpio_irq_relres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	msm_gpio_pdc_pin_release(d);
+	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
+}
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;
@@ -887,6 +983,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
 	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;
 
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 2/5] dt-bindings: pinctrl: add wakeup capable GPIOs for SDM845

[Lina Iyer]

Update the documentation to use interrupts-extended format for
specifying the TLMM summary IRQ line that is requested from GIC and the
PDC interrupts corresponding to the wakeup capable GPIOs.

Update the example to show PDC interrupts for the wakeup capable GPIOs
for SDM845.

Cc: devicetree@vger.kernel.org
Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v2:
	- Fix PDC IRQ number in example
	- Describe IRQ trigger type in example
---
 .../bindings/pinctrl/qcom,sdm845-pinctrl.txt  | 104 +++++++++++++++++-
 1 file changed, 101 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
index 665aadb5ea28..c96417b291d1 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
@@ -13,10 +13,21 @@ SDM845 platform.
 	Value type: <prop-encoded-array>
 	Definition: the base address and size of the TLMM register space.
 
-- interrupts:
+- interrupts-extended:
 	Usage: required
 	Value type: <prop-encoded-array>
-	Definition: should specify the TLMM summary IRQ.
+	Definition: should specify the TLMM summary IRQ as the first
+		    interrupt. Optionally, wake up capable GPIOs may list
+		    their corresponding PDC interrupts here.
+
+- interrupt-names:
+	Usage: required
+	Value type: <string>
+	Definition: the names matching the interrupt definition in the
+		    interrupts-extended property. The first interrupt name
+		    must be "summary-irq" for the TLMM summary IRQ. PDC
+		    interrupts must be described by "gpioN", where N is the
+		    GPIO line corresponding to the PDC IRQ.
 
 - interrupt-controller:
 	Usage: required
@@ -155,11 +166,98 @@ Example:
 	tlmm: pinctrl@3400000 {
 		compatible = "qcom,sdm845-pinctrl";
 		reg = <0x03400000 0xc00000>;
-		interrupts = <GIC_SPI 208 0>;
 		gpio-controller;
 		#gpio-cells = <2>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
+		interrupts-extended =
+			<&intc GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 30 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 31 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 32 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 33 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 34 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 35 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 36 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 37 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 38 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 39 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 41 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 42 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 43 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 44 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 45 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 46 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 47 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 49 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 50 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 51 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 52 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 54 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 55 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 56 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 57 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 58 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 59 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 60 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 61 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 62 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 63 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 64 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 65 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 66 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 67 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 68 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 69 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 70 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 71 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 72 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 73 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 74 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 75 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 76 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 77 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 79 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 80 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 81 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 82 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 83 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 84 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 85 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 86 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 90 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 91 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 92 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 95 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 96 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 97 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 98 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 99 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 100 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 102 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 103 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 104 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 105 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 106 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 107 IRQ_TYPE_LEVEL_HIGH>,
+			<&pdc 108 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "summary-irq",
+			"gpio1", "gpio3", "gpio5", "gpio10", "gpio11",
+			"gpio20", "gpio22", "gpio24", "gpio26", "gpio30",
+			"gpio32", "gpio34", "gpio36", "gpio37", "gpio38",
+			"gpio39", "gpio40", "gpio43", "gpio44", "gpio46",
+			"gpio48", "gpio52", "gpio53", "gpio54", "gpio56",
+			"gpio57", "gpio58", "gpio59", "gpio60", "gpio61",
+			"gpio62", "gpio63", "gpio64", "gpio66", "gpio68",
+			"gpio71", "gpio73", "gpio77", "gpio78", "gpio79",
+			"gpio80", "gpio84", "gpio85", "gpio86", "gpio88",
+			"gpio91", "gpio92", "gpio95", "gpio96", "gpio97",
+			"gpio101", "gpio103", "gpio104", "gpio115", "gpio116",
+			"gpio117", "gpio118", "gpio119", "gpio120", "gpio121",
+			"gpio122", "gpio123", "gpio124", "gpio125", "gpio127",
+			"gpio128", "gpio129", "gpio130", "gpio132", "gpio133",
+			"gpio145", "gpio41", "gpio89", "gpio31", "gpio49",
+			"gpio41", "gpio89", "gpio31", "gpio49";
 
 		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 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend

[Lina Iyer]

During suspend the system may power down some of the system rails. As a
result, the TLMM hw block may not be operational anymore and wakeup
capable GPIOs will not be detected. The PDC however will be operational
and the GPIOs that are routed to the PDC as IRQs can wake the system up.

To avoid being interrupted twice (for TLMM and once for PDC IRQ) when a
GPIO trips, use TLMM for active and switch to PDC for suspend. When
entering suspend, disable the TLMM wakeup interrupt and instead enable
the PDC IRQ and revert upon resume.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v3:
	- Enable PDC-IRQ swap only for edge interrupts
Changes in v2:
	- Fix PDC IRQ max port, 126 is the max supported in h/w
	- Use PDC hwirq in bitmap, linux numbers could be large
	- Setup DISABLE_UNLAZY for both TLMM and PDC IRQs
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 73 +++++++++++++++++++++++++++++-
 drivers/pinctrl/qcom/pinctrl-msm.h |  3 ++
 2 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 6527a0a9edd1..01a455f86fcd 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -37,6 +37,7 @@
 #include "../pinctrl-utils.h"
 
 #define MAX_NR_GPIO 300
+#define MAX_PDC_HWIRQ 126
 #define PS_HOLD_OFFSET 0x820
 
 /**
@@ -51,6 +52,7 @@
  * @enabled_irqs:   Bitmap of currently enabled irqs.
  * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
  *                  detection.
+ * @pdc_hwirqs:     Bitmap of wakeup capable irqs.
  * @soc;            Reference to soc_data of platform specific data.
  * @regs:           Base address for the TLMM register map.
  */
@@ -68,11 +70,15 @@ struct msm_pinctrl {
 
 	DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
 	DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
+	DECLARE_BITMAP(pdc_hwirqs, MAX_PDC_HWIRQ);
 
 	const struct msm_pinctrl_soc_data *soc;
 	void __iomem *regs;
+	struct irq_domain *pdc_irq_domain;
 };
 
+static bool in_suspend;
+
 static int msm_get_groups_count(struct pinctrl_dev *pctldev)
 {
 	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
@@ -787,8 +793,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
-	if (pdc_irqd)
+	if (pdc_irqd && !in_suspend) {
 		irq_set_irq_wake(pdc_irqd->irq, on);
+		if (on)
+			set_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs);
+		else
+			clear_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs);
+	}
 
 	irq_set_irq_wake(pctrl->irq, on);
 
@@ -919,7 +930,12 @@ static int msm_gpio_pdc_pin_request(struct irq_data *d)
 	}
 
 	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
+	irq_set_handler_data(irq, d);
+	irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY);
+	irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY);
 	disable_irq(irq);
+	if (!pctrl->pdc_irq_domain)
+		pctrl->pdc_irq_domain = irq_get_irq_data(irq)->domain;
 
 	return 0;
 }
@@ -1071,6 +1087,61 @@ static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl)
 		}
 }
 
+int __maybe_unused msm_pinctrl_suspend_late(struct device *dev)
+{
+	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
+	struct irq_data *irqd;
+	unsigned int irq;
+	int i;
+
+	in_suspend = true;
+	for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
+		irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
+		irqd = irq_get_handler_data(irq);
+		/*
+		 * We don't know if the TLMM will be functional
+		 * or not, during the suspend. If its functional,
+		 * we do not want duplicate interrupts from PDC.
+		 * Hence disable the GPIO IRQ and enable PDC IRQ
+		 * for edge interrupt only.
+		 */
+		if (irqd_is_wakeup_set(irqd) && !irqd_is_level_type(irqd)) {
+			disable_irq_wake(irqd->irq);
+			disable_irq(irqd->irq);
+			enable_irq(irq);
+		}
+	}
+
+	return 0;
+}
+
+int __maybe_unused msm_pinctrl_resume_late(struct device *dev)
+{
+	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
+	struct irq_data *irqd, *pdc_irqd;
+	unsigned int irq;
+	int i;
+
+	for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
+		irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
+		irqd = irq_get_handler_data(irq);
+		pdc_irqd = irq_get_irq_data(irq);
+		/*
+		 * The TLMM will be operational now, so disable
+		 * the PDC IRQ for edge interrupts only.
+		 */
+		if (irqd_is_wakeup_set(pdc_irqd) &&
+		    !irqd_is_level_type(pdc_irqd)) {
+			disable_irq_nosync(irq);
+			enable_irq_wake(irqd->irq);
+			enable_irq(irqd->irq);
+		}
+	}
+	in_suspend = false;
+
+	return 0;
+}
+
 int msm_pinctrl_probe(struct platform_device *pdev,
 		      const struct msm_pinctrl_soc_data *soc_data)
 {
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index 9b9feea540ff..21b56fb5dae9 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -123,4 +123,7 @@ int msm_pinctrl_probe(struct platform_device *pdev,
 		      const struct msm_pinctrl_soc_data *soc_data);
 int msm_pinctrl_remove(struct platform_device *pdev);
 
+int msm_pinctrl_suspend_late(struct device *dev);
+int msm_pinctrl_resume_late(struct device *dev);
+
 #endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 4/5] drivers: pinctrl: qcom: sdm845: support GPIO wakeup from suspend

[Lina Iyer]

Enable TLMM IRQs to be sensed by PDC when we enter suspend. It is
possible that the TLMM may be powered off and not detect GPIOs that are
configured as wake up interrupts. By hooking into suspend callbacks, we
allow PDC IRQs to take over and wake up the system if wakeup interrupts
are triggered.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
---
Changes in v3:
	- Move the common suspend ops definition to pinctrl-msm.c
---
 drivers/pinctrl/qcom/pinctrl-msm.c    | 5 +++++
 drivers/pinctrl/qcom/pinctrl-msm.h    | 2 ++
 drivers/pinctrl/qcom/pinctrl-sdm845.c | 1 +
 3 files changed, 8 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 01a455f86fcd..92887c075230 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1142,6 +1142,11 @@ int __maybe_unused msm_pinctrl_resume_late(struct device *dev)
 	return 0;
 }
 
+const struct dev_pm_ops msm_pinctrl_dev_pm_ops = {
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(msm_pinctrl_suspend_late,
+				     msm_pinctrl_resume_late)
+};
+
 int msm_pinctrl_probe(struct platform_device *pdev,
 		      const struct msm_pinctrl_soc_data *soc_data)
 {
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index 21b56fb5dae9..9be1baa878a3 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -126,4 +126,6 @@ int msm_pinctrl_remove(struct platform_device *pdev);
 int msm_pinctrl_suspend_late(struct device *dev);
 int msm_pinctrl_resume_late(struct device *dev);
 
+extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
+
 #endif
diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
index 2ab7a8885757..0c82dc403268 100644
--- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
+++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
@@ -1301,6 +1301,7 @@ static struct platform_driver sdm845_pinctrl_driver = {
 	.driver = {
 		.name = "sdm845-pinctrl",
 		.of_match_table = sdm845_pinctrl_of_match,
+		.pm = &msm_pinctrl_dev_pm_ops,
 	},
 	.probe = sdm845_pinctrl_probe,
 	.remove = msm_pinctrl_remove,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 5/5] arm64: dts: qcom: add wake up interrupts for GPIOs for SDM845

[Lina Iyer]

GPIOs that are wakeup capable have interrupt lines that are routed to
the always-on interrupt controller (PDC) in parallel to the pinctrl. The
interrupts listed here are the wake up lines corresponding to GPIOs.

Signed-off-by: Lina Iyer <ilina@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v2:
	- Define IRQ trigger type in DT
Changes in v1:
	- Use interrupt-extended for all TLMM interrupts
	- Define GPIO-PDC map using interrupt-names
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 152 ++++++++++++++++++++++++++-
 1 file changed, 151 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 0208f8557ffa..8d87794092b0 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -712,11 +712,161 @@
 		tlmm: pinctrl@3400000 {
 			compatible = "qcom,sdm845-pinctrl";
 			reg = <0x03400000 0xc00000>;
-			interrupts = <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>;
 			gpio-controller;
 			#gpio-cells = <2>;
 			interrupt-controller;
 			#interrupt-cells = <2>;
+			interrupts-extended =
+				<&intc GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 30 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 31 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 32 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 33 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 34 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 35 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 36 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 37 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 38 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 39 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 41 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 42 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 43 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 44 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 45 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 46 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 47 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 49 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 50 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 51 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 52 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 54 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 55 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 56 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 57 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 58 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 59 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 60 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 61 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 62 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 63 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 64 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 65 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 66 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 67 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 68 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 69 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 70 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 71 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 72 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 73 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 74 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 75 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 76 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 77 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 79 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 80 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 81 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 82 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 83 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 84 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 85 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 86 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 90 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 91 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 92 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 95 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 96 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 97 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 98 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 99 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 100 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 102 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 103 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 104 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 105 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 106 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 107 IRQ_TYPE_LEVEL_HIGH>,
+				<&pdc 108 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "summary-irq",
+				"gpio1",
+				"gpio3",
+				"gpio5",
+				"gpio10",
+				"gpio11",
+				"gpio20",
+				"gpio22",
+				"gpio24",
+				"gpio26",
+				"gpio30",
+				"gpio32",
+				"gpio34",
+				"gpio36",
+				"gpio37",
+				"gpio38",
+				"gpio39",
+				"gpio40",
+				"gpio43",
+				"gpio44",
+				"gpio46",
+				"gpio48",
+				"gpio52",
+				"gpio53",
+				"gpio54",
+				"gpio56",
+				"gpio57",
+				"gpio58",
+				"gpio59",
+				"gpio60",
+				"gpio61",
+				"gpio62",
+				"gpio63",
+				"gpio64",
+				"gpio66",
+				"gpio68",
+				"gpio71",
+				"gpio73",
+				"gpio77",
+				"gpio78",
+				"gpio79",
+				"gpio80",
+				"gpio84",
+				"gpio85",
+				"gpio86",
+				"gpio88",
+				"gpio91",
+				"gpio92",
+				"gpio95",
+				"gpio96",
+				"gpio97",
+				"gpio101",
+				"gpio103",
+				"gpio104",
+				"gpio115",
+				"gpio116",
+				"gpio117",
+				"gpio118",
+				"gpio119",
+				"gpio120",
+				"gpio121",
+				"gpio122",
+				"gpio123",
+				"gpio124",
+				"gpio125",
+				"gpio127",
+				"gpio128",
+				"gpio129",
+				"gpio130",
+				"gpio132",
+				"gpio133",
+				"gpio145",
+				"gpio41",
+				"gpio89",
+				"gpio31",
+				"gpio49",
+				"gpio41",
+				"gpio89",
+				"gpio31",
+				"gpio49";
 
 			qup_i2c0_default: qup-i2c0-default {
 				pinmux {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

On Tue, Sep 04 2018 at 15:18 -0600, Lina Iyer wrote:
>QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
>domain can wakeup the SoC, when interrupts and GPIOs are routed to its
>interrupt controller. Only select GPIOs that are deemed wakeup capable
>are routed to specific PDC pins. During low power state, the pinmux
>interrupt controller may be non-functional but the PDC would be. The PDC
>can detect the wakeup GPIO is triggered and bring the TLMM to an
>operational state.
>
>Interrupts that are level triggered will be detected at the TLMM when
>the controller becomes operational. Edge interrupts however need to be
>replayed again.
>
>Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
>but keep it disabled. During suspend, we can enable the PDC IRQ instead
>of the GPIO IRQ, which may or not be detected.
>
>Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>---
>Changes in v3:
>	- free action->name
>Changes in v2:
>	- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
>Changes in v1:
>	- Trigger GPIO in h/w from PDC IRQ handler
>	- Avoid big tables for GPIO-PDC map, pick from DT instead
>	- Use handler_data
>---
> drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++
> 1 file changed, 98 insertions(+)
>
>diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>index 0e22f52b2a19..6527a0a9edd1 100644
>--- a/drivers/pinctrl/qcom/pinctrl-msm.c
>+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>@@ -687,11 +687,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> 	const struct msm_pingroup *g;
> 	unsigned long flags;
> 	u32 val;
>+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>
> 	g = &pctrl->soc->groups[d->hwirq];
>
> 	raw_spin_lock_irqsave(&pctrl->lock, flags);
>
>+	if (pdc_irqd)
>+		irq_set_irq_type(pdc_irqd->irq, type);
>+
> 	/*
> 	 * For hw without possibility of detecting both edges
> 	 */
>@@ -779,9 +783,13 @@ 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;
>+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>
> 	raw_spin_lock_irqsave(&pctrl->lock, flags);
>
>+	if (pdc_irqd)
>+		irq_set_irq_wake(pdc_irqd->irq, on);
>+
> 	irq_set_irq_wake(pctrl->irq, on);
>
> 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>@@ -863,6 +871,94 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
> 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
> }
>
>+static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
>+{
>+	struct irq_data *irqd = data;
>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
>+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>+	const struct msm_pingroup *g;
>+	unsigned long flags;
>+	u32 val;
>+
>+	if (!irqd_is_level_type(irqd)) {
>+		g = &pctrl->soc->groups[irqd->hwirq];
>+		raw_spin_lock_irqsave(&pctrl->lock, flags);
>+		val = BIT(g->intr_status_bit);
>+		writel(val, pctrl->regs + g->intr_status_reg);
>+		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>+	}
>+
>+	return IRQ_HANDLED;
>+}
>+
>+static int msm_gpio_pdc_pin_request(struct irq_data *d)
>+{
>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>+	struct platform_device *pdev = to_platform_device(pctrl->dev);
>+	const char *pin_name;
>+	int irq;
>+	int ret;
>+
>+	pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);
>+	if (!pin_name)
>+		return -ENOMEM;
>+
>+	irq = platform_get_irq_byname(pdev, pin_name);
>+	if (irq < 0) {
>+		kfree(pin_name);
>+		return 0;
>+	}
>+
>+	ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d),
>+			  pin_name, d);
>+	if (ret) {
>+		pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq);
Need to end with '\n'.

>+		kfree(pin_name);
>+		return ret;
>+	}
>+
>+	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
>+	disable_irq(irq);
>+
>+	return 0;
>+}
>+
>+static int msm_gpio_pdc_pin_release(struct irq_data *d)
>+{
>+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>+	const void *name;
>+
>+	if (pdc_irqd) {
>+		irq_set_handler_data(d->irq, NULL);
>+		name = free_irq(pdc_irqd->irq, d);
>+		kfree(name);
>+	}
>+
>+	return 0;
>+}
>+
>+static int msm_gpio_irq_reqres(struct irq_data *d)
>+{
>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>+
>+	if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
>+		dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
>+			irqd_to_hwirq(d));
>+		return -EINVAL;
>+	}
>+
>+	return msm_gpio_pdc_pin_request(d);
>+}
>+
>+static void msm_gpio_irq_relres(struct irq_data *d)
>+{
>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>+
>+	msm_gpio_pdc_pin_release(d);
>+	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
>+}
>+
> static int msm_gpio_init(struct msm_pinctrl *pctrl)
> {
> 	struct gpio_chip *chip;
>@@ -887,6 +983,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
> 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
> 	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;
>
> 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
> 	if (ret) {
>--
>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>a Linux Foundation Collaborative Project
>

Re: [PATCH v3 2/5] dt-bindings: pinctrl: add wakeup capable GPIOs for SDM845

[Rob Herring]

On Tue, Sep 04, 2018 at 03:18:07PM -0600, Lina Iyer wrote:
> Update the documentation to use interrupts-extended format for
> specifying the TLMM summary IRQ line that is requested from GIC and the
> PDC interrupts corresponding to the wakeup capable GPIOs.
> 
> Update the example to show PDC interrupts for the wakeup capable GPIOs
> for SDM845.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
> Changes in v2:
> 	- Fix PDC IRQ number in example
> 	- Describe IRQ trigger type in example
> ---
>  .../bindings/pinctrl/qcom,sdm845-pinctrl.txt  | 104 +++++++++++++++++-
>  1 file changed, 101 insertions(+), 3 deletions(-)

As I also mentioned in v2. Please add acks/reviewed-bys when posting new 
versions.

Rob

Re: [PATCH v3 2/5] dt-bindings: pinctrl: add wakeup capable GPIOs for SDM845

[Lina Iyer]

On Mon, Sep 10 2018 at 10:28 -0600, Rob Herring wrote:
>On Tue, Sep 04, 2018 at 03:18:07PM -0600, Lina Iyer wrote:
>> Update the documentation to use interrupts-extended format for
>> specifying the TLMM summary IRQ line that is requested from GIC and the
>> PDC interrupts corresponding to the wakeup capable GPIOs.
>>
>> Update the example to show PDC interrupts for the wakeup capable GPIOs
>> for SDM845.
>>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>> Changes in v2:
>> 	- Fix PDC IRQ number in example
>> 	- Describe IRQ trigger type in example
>> ---
>>  .../bindings/pinctrl/qcom,sdm845-pinctrl.txt  | 104 +++++++++++++++++-
>>  1 file changed, 101 insertions(+), 3 deletions(-)
>
>As I also mentioned in v2. Please add acks/reviewed-bys when posting new
>versions.
>
Sorry Rob. I added to patch 5, but forgot this one. Thanks for pointing
out. Will add it in the next spin. Sorry for wasting your time.

-- Lina

Re: [PATCH v3 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Marc Zyngier]

Hi Lina,

On Tue, 04 Sep 2018 22:18:06 +0100,
Lina Iyer <ilina@codeaurora.org> wrote:
> 
> QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
> domain can wakeup the SoC, when interrupts and GPIOs are routed to its
> interrupt controller. Only select GPIOs that are deemed wakeup capable
> are routed to specific PDC pins. During low power state, the pinmux
> interrupt controller may be non-functional but the PDC would be. The PDC
> can detect the wakeup GPIO is triggered and bring the TLMM to an
> operational state.
> 
> Interrupts that are level triggered will be detected at the TLMM when
> the controller becomes operational. Edge interrupts however need to be
> replayed again.
> 
> Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
> but keep it disabled. During suspend, we can enable the PDC IRQ instead
> of the GPIO IRQ, which may or not be detected.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
> Changes in v3:
> 	- free action->name
> Changes in v2:
> 	- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
> Changes in v1:
> 	- Trigger GPIO in h/w from PDC IRQ handler
> 	- Avoid big tables for GPIO-PDC map, pick from DT instead
> 	- Use handler_data
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 0e22f52b2a19..6527a0a9edd1 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -687,11 +687,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>  	const struct msm_pingroup *g;
>  	unsigned long flags;
>  	u32 val;
> +	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>  
>  	g = &pctrl->soc->groups[d->hwirq];
>  
>  	raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
> +	if (pdc_irqd)
> +		irq_set_irq_type(pdc_irqd->irq, type);
> +
>  	/*
>  	 * For hw without possibility of detecting both edges
>  	 */
> @@ -779,9 +783,13 @@ 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;
> +	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>  
>  	raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
> +	if (pdc_irqd)
> +		irq_set_irq_wake(pdc_irqd->irq, on);
> +
>  	irq_set_irq_wake(pctrl->irq, on);
>  
>  	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> @@ -863,6 +871,94 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
>  	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
>  }
>  
> +static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
> +{
> +	struct irq_data *irqd = data;
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +	const struct msm_pingroup *g;
> +	unsigned long flags;
> +	u32 val;
> +
> +	if (!irqd_is_level_type(irqd)) {
> +		g = &pctrl->soc->groups[irqd->hwirq];
> +		raw_spin_lock_irqsave(&pctrl->lock, flags);
> +		val = BIT(g->intr_status_bit);
> +		writel(val, pctrl->regs + g->intr_status_reg);

write_relaxed, please.

> +		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +	}

Overall, this requires some form of documentation (I'll have forgotten
about the whole thing quickly enough).

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int msm_gpio_pdc_pin_request(struct irq_data *d)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> +	struct platform_device *pdev = to_platform_device(pctrl->dev);
> +	const char *pin_name;
> +	int irq;
> +	int ret;
> +
> +	pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);
> +	if (!pin_name)
> +		return -ENOMEM;
> +
> +	irq = platform_get_irq_byname(pdev, pin_name);
> +	if (irq < 0) {
> +		kfree(pin_name);
> +		return 0;
> +	}
> +
> +	ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d),
> +			  pin_name, d);
> +	if (ret) {
> +		pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq);

This message doesn't correspond to what you're doing here.

> +		kfree(pin_name);
> +		return ret;
> +	}
> +
> +	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
> +	disable_irq(irq);

Who enables this interrupt?

There is a gap between request_irq and disable_irq, where you can take
the interrupt, and not having set the handler data. Horrible things
will happen in this situation.

A slightly better way of doing that would be:

	// Prevent the interrupt from being enabled on request
	irq_set_status_flags(d->irq, IRQ_NOAUTOEN);
	ret = request_irq(...);
	irq_set_handler(...);

and let the enable_irq() do its thing when it happens (where?).

> +
> +	return 0;
> +}
> +
> +static int msm_gpio_pdc_pin_release(struct irq_data *d)
> +{
> +	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
> +	const void *name;
> +
> +	if (pdc_irqd) {
> +		irq_set_handler_data(d->irq, NULL);
> +		name = free_irq(pdc_irqd->irq, d);
> +		kfree(name);
> +	}
> +
> +	return 0;
> +}
> +
> +static int msm_gpio_irq_reqres(struct irq_data *d)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +
> +	if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
> +		dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
> +			irqd_to_hwirq(d));
> +		return -EINVAL;
> +	}
> +
> +	return msm_gpio_pdc_pin_request(d);
> +}
> +
> +static void msm_gpio_irq_relres(struct irq_data *d)
> +{
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +
> +	msm_gpio_pdc_pin_release(d);
> +	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
> +}
> +
>  static int msm_gpio_init(struct msm_pinctrl *pctrl)
>  {
>  	struct gpio_chip *chip;
> @@ -887,6 +983,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>  	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
>  	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;
>  
>  	ret = gpiochip_add_data(&pctrl->chip, pctrl);
>  	if (ret) {
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Thanks,

	M.

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

Re: [PATCH v3 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

On Fri, Sep 21 2018 at 17:11 -0600, Marc Zyngier wrote:
>Hi Lina,
>
>On Tue, 04 Sep 2018 22:18:06 +0100,
>Lina Iyer <ilina@codeaurora.org> wrote:
[...]
>> +static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
>> +{
>> +	struct irq_data *irqd = data;
>> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
>> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>> +	const struct msm_pingroup *g;
>> +	unsigned long flags;
>> +	u32 val;
>> +
>> +	if (!irqd_is_level_type(irqd)) {
>> +		g = &pctrl->soc->groups[irqd->hwirq];
>> +		raw_spin_lock_irqsave(&pctrl->lock, flags);
>> +		val = BIT(g->intr_status_bit);
>> +		writel(val, pctrl->regs + g->intr_status_reg);
>
>write_relaxed, please.
>
>> +		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>> +	}
>
>Overall, this requires some form of documentation (I'll have forgotten
>about the whole thing quickly enough).
>
Sure, I will address them both.
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int msm_gpio_pdc_pin_request(struct irq_data *d)
>> +{
>> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>> +	struct platform_device *pdev = to_platform_device(pctrl->dev);
>> +	const char *pin_name;
>> +	int irq;
>> +	int ret;
>> +
>> +	pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);
>> +	if (!pin_name)
>> +		return -ENOMEM;
>> +
>> +	irq = platform_get_irq_byname(pdev, pin_name);
>> +	if (irq < 0) {
>> +		kfree(pin_name);
>> +		return 0;
>> +	}
>> +
>> +	ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d),
>> +			  pin_name, d);
>> +	if (ret) {
>> +		pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq);
>
>This message doesn't correspond to what you're doing here.
>
Well, I thought the message is most relevant because, we will not be
able to wake up from this GPIO IRQ. But, I will change it.

>> +		kfree(pin_name);
>> +		return ret;
>> +	}
>> +
>> +	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
>> +	disable_irq(irq);
>
>Who enables this interrupt?
>
Suspend/resume code does the swap of enable/disable of PDC IRQ vs GPIO
IRQ in patch #4.

>There is a gap between request_irq and disable_irq, where you can take
>the interrupt, and not having set the handler data. Horrible things
>will happen in this situation.
>
>A slightly better way of doing that would be:
>
>	// Prevent the interrupt from being enabled on request
>	irq_set_status_flags(d->irq, IRQ_NOAUTOEN);
>	ret = request_irq(...);
>	irq_set_handler(...);
>
>and let the enable_irq() do its thing when it happens (where?).
>
Oh. This is better. Will amend.

Thanks for the review.

-- Lina

>> +
>> +	return 0;
>> +}
>> +
>> +static int msm_gpio_pdc_pin_release(struct irq_data *d)
>> +{
>> +	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>> +	const void *name;
>> +
>> +	if (pdc_irqd) {
>> +		irq_set_handler_data(d->irq, NULL);
>> +		name = free_irq(pdc_irqd->irq, d);
>> +		kfree(name);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int msm_gpio_irq_reqres(struct irq_data *d)
>> +{
>> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +
>> +	if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
>> +		dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
>> +			irqd_to_hwirq(d));
>> +		return -EINVAL;
>> +	}
>> +
>> +	return msm_gpio_pdc_pin_request(d);
>> +}
>> +
>> +static void msm_gpio_irq_relres(struct irq_data *d)
>> +{
>> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> +
>> +	msm_gpio_pdc_pin_release(d);
>> +	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
>> +}
>> +
>>  static int msm_gpio_init(struct msm_pinctrl *pctrl)
>>  {
>>  	struct gpio_chip *chip;
>> @@ -887,6 +983,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>>  	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
>>  	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;
>>
>>  	ret = gpiochip_add_data(&pctrl->chip, pctrl);
>>  	if (ret) {
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
>
>Thanks,
>
>	M.
>
>--
>Jazz is not dead, it just smell funny.

Re: [PATCH v3 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend

[Marc Zyngier]

Hi Lina,

On Tue, 04 Sep 2018 22:18:08 +0100,
Lina Iyer <ilina@codeaurora.org> wrote:
> 
> During suspend the system may power down some of the system rails. As a
> result, the TLMM hw block may not be operational anymore and wakeup
> capable GPIOs will not be detected. The PDC however will be operational
> and the GPIOs that are routed to the PDC as IRQs can wake the system up.
> 
> To avoid being interrupted twice (for TLMM and once for PDC IRQ) when a
> GPIO trips, use TLMM for active and switch to PDC for suspend. When
> entering suspend, disable the TLMM wakeup interrupt and instead enable
> the PDC IRQ and revert upon resume.
> 
> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
> Changes in v3:
> 	- Enable PDC-IRQ swap only for edge interrupts
> Changes in v2:
> 	- Fix PDC IRQ max port, 126 is the max supported in h/w
> 	- Use PDC hwirq in bitmap, linux numbers could be large
> 	- Setup DISABLE_UNLAZY for both TLMM and PDC IRQs
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 73 +++++++++++++++++++++++++++++-
>  drivers/pinctrl/qcom/pinctrl-msm.h |  3 ++
>  2 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 6527a0a9edd1..01a455f86fcd 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -37,6 +37,7 @@
>  #include "../pinctrl-utils.h"
>  
>  #define MAX_NR_GPIO 300
> +#define MAX_PDC_HWIRQ 126
>  #define PS_HOLD_OFFSET 0x820
>  
>  /**
> @@ -51,6 +52,7 @@
>   * @enabled_irqs:   Bitmap of currently enabled irqs.
>   * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
>   *                  detection.
> + * @pdc_hwirqs:     Bitmap of wakeup capable irqs.
>   * @soc;            Reference to soc_data of platform specific data.
>   * @regs:           Base address for the TLMM register map.
>   */
> @@ -68,11 +70,15 @@ struct msm_pinctrl {
>  
>  	DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
>  	DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
> +	DECLARE_BITMAP(pdc_hwirqs, MAX_PDC_HWIRQ);
>  
>  	const struct msm_pinctrl_soc_data *soc;
>  	void __iomem *regs;
> +	struct irq_domain *pdc_irq_domain;
>  };
>  
> +static bool in_suspend;
> +
>  static int msm_get_groups_count(struct pinctrl_dev *pctldev)
>  {
>  	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> @@ -787,8 +793,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
>  
>  	raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
> -	if (pdc_irqd)
> +	if (pdc_irqd && !in_suspend) {
>  		irq_set_irq_wake(pdc_irqd->irq, on);
> +		if (on)
> +			set_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs);
> +		else
> +			clear_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs);
> +	}
>  
>  	irq_set_irq_wake(pctrl->irq, on);
>  
> @@ -919,7 +930,12 @@ static int msm_gpio_pdc_pin_request(struct irq_data *d)
>  	}
>  
>  	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
> +	irq_set_handler_data(irq, d);
> +	irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY);
> +	irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY);
>  	disable_irq(irq);
> +	if (!pctrl->pdc_irq_domain)
> +		pctrl->pdc_irq_domain = irq_get_irq_data(irq)->domain;
>  
>  	return 0;
>  }
> @@ -1071,6 +1087,61 @@ static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl)
>  		}
>  }
>  
> +int __maybe_unused msm_pinctrl_suspend_late(struct device *dev)
> +{
> +	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
> +	struct irq_data *irqd;
> +	unsigned int irq;
> +	int i;
> +
> +	in_suspend = true;
> +	for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
> +		irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
> +		irqd = irq_get_handler_data(irq);
> +		/*
> +		 * We don't know if the TLMM will be functional
> +		 * or not, during the suspend. If its functional,
> +		 * we do not want duplicate interrupts from PDC.
> +		 * Hence disable the GPIO IRQ and enable PDC IRQ
> +		 * for edge interrupt only.
> +		 */
> +		if (irqd_is_wakeup_set(irqd) && !irqd_is_level_type(irqd)) {
> +			disable_irq_wake(irqd->irq);

There is something I don't quite get here. If the PDC is used to wake
up the platform, why is the TLMM interrupt configured as a wakeup
source the first place? Or is it just to keep things simple and not
have to track it in the TLMM driver itself?

> +			disable_irq(irqd->irq);
> +			enable_irq(irq);
> +		}
> +	}

Given that you're changing in_suspend and parsing the bitmap,
shouldn't take the pdc spinlock?

> +
> +	return 0;
> +}
> +
> +int __maybe_unused msm_pinctrl_resume_late(struct device *dev)
> +{
> +	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
> +	struct irq_data *irqd, *pdc_irqd;
> +	unsigned int irq;
> +	int i;
> +
> +	for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
> +		irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
> +		irqd = irq_get_handler_data(irq);
> +		pdc_irqd = irq_get_irq_data(irq);
> +		/*
> +		 * The TLMM will be operational now, so disable
> +		 * the PDC IRQ for edge interrupts only.
> +		 */
> +		if (irqd_is_wakeup_set(pdc_irqd) &&
> +		    !irqd_is_level_type(pdc_irqd)) {
> +			disable_irq_nosync(irq);
> +			enable_irq_wake(irqd->irq);
> +			enable_irq(irqd->irq);
> +		}
> +	}
> +	in_suspend = false;

Same remark about the lock.

> +
> +	return 0;
> +}
> +
>  int msm_pinctrl_probe(struct platform_device *pdev,
>  		      const struct msm_pinctrl_soc_data *soc_data)
>  {
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index 9b9feea540ff..21b56fb5dae9 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -123,4 +123,7 @@ int msm_pinctrl_probe(struct platform_device *pdev,
>  		      const struct msm_pinctrl_soc_data *soc_data);
>  int msm_pinctrl_remove(struct platform_device *pdev);
>  
> +int msm_pinctrl_suspend_late(struct device *dev);
> +int msm_pinctrl_resume_late(struct device *dev);
> +
>  #endif
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Thanks,

	M.

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

Re: [PATCH v3 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend

[Lina Iyer]

On Sat, Sep 22 2018 at 10:29 -0600, Marc Zyngier wrote:
>Hi Lina,
>
>On Tue, 04 Sep 2018 22:18:08 +0100,
>Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> During suspend the system may power down some of the system rails. As a
>> result, the TLMM hw block may not be operational anymore and wakeup
>> capable GPIOs will not be detected. The PDC however will be operational
>> and the GPIOs that are routed to the PDC as IRQs can wake the system up.
>>
>> To avoid being interrupted twice (for TLMM and once for PDC IRQ) when a
>> GPIO trips, use TLMM for active and switch to PDC for suspend. When
>> entering suspend, disable the TLMM wakeup interrupt and instead enable
>> the PDC IRQ and revert upon resume.
>>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>> Changes in v3:
>> 	- Enable PDC-IRQ swap only for edge interrupts
>> Changes in v2:
>> 	- Fix PDC IRQ max port, 126 is the max supported in h/w
>> 	- Use PDC hwirq in bitmap, linux numbers could be large
>> 	- Setup DISABLE_UNLAZY for both TLMM and PDC IRQs
>> ---
[...]

>> +int __maybe_unused msm_pinctrl_suspend_late(struct device *dev)
>> +{
>> +	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
>> +	struct irq_data *irqd;
>> +	unsigned int irq;
>> +	int i;
>> +
>> +	in_suspend = true;
>> +	for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
>> +		irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
>> +		irqd = irq_get_handler_data(irq);
>> +		/*
>> +		 * We don't know if the TLMM will be functional
>> +		 * or not, during the suspend. If its functional,
>> +		 * we do not want duplicate interrupts from PDC.
>> +		 * Hence disable the GPIO IRQ and enable PDC IRQ
>> +		 * for edge interrupt only.
>> +		 */
>> +		if (irqd_is_wakeup_set(irqd) && !irqd_is_level_type(irqd)) {
>> +			disable_irq_wake(irqd->irq);
>
>There is something I don't quite get here. If the PDC is used to wake
>up the platform, why is the TLMM interrupt configured as a wakeup
>source the first place? Or is it just to keep things simple and not
>have to track it in the TLMM driver itself?
>
True, it need not be. I could just avoid setting the wakeup on the TLMM
and just use the PDC interrupt as wakeup.

Also, I am exploring an option that was suggested by Stephen [1] to just
use the PDC interrupt as a parent of the GPIO IRQ and use a different
irqchip for the PDC interrupt. I ran into some issue with accessing
irqchip and irqdata of the PDC interrupt, since the irqchip was not in
hierarchy with the GPIO's irqchip. I haven't been able to find time to
resolve the issue that the set_parent_ functions, because of the
hierarchy.

Essentially, we have two different mechanisms for GPIO IRQs based on
whether they can be woken up by the PDC interrupt.

	GPIO-IRQ --> PDC --> GIC

	GPIO-IRQ --> TLMM SUMMARY --> GIC

Do you think the idea is feasible? It would avoid doing all this
enable/disable at every suspend and even during idle, when the TLMM
could be powered off.


>> +			disable_irq(irqd->irq);
>> +			enable_irq(irq);
>> +		}
>> +	}
>
>Given that you're changing in_suspend and parsing the bitmap,
>shouldn't take the pdc spinlock?
>
Since we are the the only CPU running and suspend/resume (and even idle)
would be serialized I didn't see a reason for needing a lock.

>> +
>> +	return 0;
>> +}
>> +
>> +int __maybe_unused msm_pinctrl_resume_late(struct device *dev)
>> +{
>> +	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
>> +	struct irq_data *irqd, *pdc_irqd;
>> +	unsigned int irq;
>> +	int i;
>> +
>> +	for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
>> +		irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
>> +		irqd = irq_get_handler_data(irq);
>> +		pdc_irqd = irq_get_irq_data(irq);
>> +		/*
>> +		 * The TLMM will be operational now, so disable
>> +		 * the PDC IRQ for edge interrupts only.
>> +		 */
>> +		if (irqd_is_wakeup_set(pdc_irqd) &&
>> +		    !irqd_is_level_type(pdc_irqd)) {
>> +			disable_irq_nosync(irq);
>> +			enable_irq_wake(irqd->irq);
>> +			enable_irq(irqd->irq);
>> +		}
>> +	}
>> +	in_suspend = false;
>
>Same remark about the lock.
>

Thanks,
Lina

[1]. https://lore.kernel.org/patchwork/patch/975423/

Re: [PATCH v3 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend

[Marc Zyngier]

On Sat, 22 Sep 2018 18:09:09 +0100,
Lina Iyer <ilina@codeaurora.org> wrote:
> 
> On Sat, Sep 22 2018 at 10:29 -0600, Marc Zyngier wrote:
> > Hi Lina,
> > 
> > On Tue, 04 Sep 2018 22:18:08 +0100,
> > Lina Iyer <ilina@codeaurora.org> wrote:
> >> 
> >> During suspend the system may power down some of the system rails. As a
> >> result, the TLMM hw block may not be operational anymore and wakeup
> >> capable GPIOs will not be detected. The PDC however will be operational
> >> and the GPIOs that are routed to the PDC as IRQs can wake the system up.
> >> 
> >> To avoid being interrupted twice (for TLMM and once for PDC IRQ) when a
> >> GPIO trips, use TLMM for active and switch to PDC for suspend. When
> >> entering suspend, disable the TLMM wakeup interrupt and instead enable
> >> the PDC IRQ and revert upon resume.
> >> 
> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> >> ---
> >> Changes in v3:
> >> 	- Enable PDC-IRQ swap only for edge interrupts
> >> Changes in v2:
> >> 	- Fix PDC IRQ max port, 126 is the max supported in h/w
> >> 	- Use PDC hwirq in bitmap, linux numbers could be large
> >> 	- Setup DISABLE_UNLAZY for both TLMM and PDC IRQs
> >> ---
> [...]
> 
> >> +int __maybe_unused msm_pinctrl_suspend_late(struct device *dev)
> >> +{
> >> +	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
> >> +	struct irq_data *irqd;
> >> +	unsigned int irq;
> >> +	int i;
> >> +
> >> +	in_suspend = true;
> >> +	for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
> >> +		irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
> >> +		irqd = irq_get_handler_data(irq);
> >> +		/*
> >> +		 * We don't know if the TLMM will be functional
> >> +		 * or not, during the suspend. If its functional,
> >> +		 * we do not want duplicate interrupts from PDC.
> >> +		 * Hence disable the GPIO IRQ and enable PDC IRQ
> >> +		 * for edge interrupt only.
> >> +		 */
> >> +		if (irqd_is_wakeup_set(irqd) && !irqd_is_level_type(irqd)) {
> >> +			disable_irq_wake(irqd->irq);
> > 
> > There is something I don't quite get here. If the PDC is used to wake
> > up the platform, why is the TLMM interrupt configured as a wakeup
> > source the first place? Or is it just to keep things simple and not
> > have to track it in the TLMM driver itself?
> > 
> True, it need not be. I could just avoid setting the wakeup on the TLMM
> and just use the PDC interrupt as wakeup.
> 
> Also, I am exploring an option that was suggested by Stephen [1] to just
> use the PDC interrupt as a parent of the GPIO IRQ and use a different
> irqchip for the PDC interrupt. I ran into some issue with accessing
> irqchip and irqdata of the PDC interrupt, since the irqchip was not in
> hierarchy with the GPIO's irqchip. I haven't been able to find time to
> resolve the issue that the set_parent_ functions, because of the
> hierarchy.
> 
> Essentially, we have two different mechanisms for GPIO IRQs based on
> whether they can be woken up by the PDC interrupt.
> 
> 	GPIO-IRQ --> PDC --> GIC
> 
> 	GPIO-IRQ --> TLMM SUMMARY --> GIC
> 
> Do you think the idea is feasible? It would avoid doing all this
> enable/disable at every suspend and even during idle, when the TLMM
> could be powered off.

[me tries to page it all in again]

You could have the PDC as part of the GPIO hierarchy:

	GPIO -> PDC -> TLMM -> GIC

and always configure the PDC as a wake-up source. I just wonder if you
can do that without setting up a parallel hierarchy between the PDC
and the GIC. We already have similar things in the tree (see OMAP's
wakeupgen), and it may be worth having a look. The lack of interrupt
replaying on the PDC is quite annoying (I have much stronger words in
mind though), and I'm not sure we can easily fix that one without this
parallel interrupt hack (you need something to inject edge interrupts
in the TLMM).

> 
> 
> >> +			disable_irq(irqd->irq);
> >> +			enable_irq(irq);
> >> +		}
> >> +	}
> > 
> > Given that you're changing in_suspend and parsing the bitmap,
> > shouldn't take the pdc spinlock?
> > 
> Since we are the the only CPU running and suspend/resume (and even idle)
> would be serialized I didn't see a reason for needing a lock.

In that case, what is the purpose of 'in_suspend' if
msm_gpio_irq_set_wake cannot happen during the suspend/resume phases?
It all seems a bit inconsistent.

Thanks,

	N,

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

Re: [PATCH v3 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend

[Lina Iyer]

On Sun, Sep 23 2018 at 03:48 -0600, Marc Zyngier wrote:
>On Sat, 22 Sep 2018 18:09:09 +0100,
>Lina Iyer <ilina@codeaurora.org> wrote:
>>
>> On Sat, Sep 22 2018 at 10:29 -0600, Marc Zyngier wrote:
>> > Hi Lina,
>> >
>> > On Tue, 04 Sep 2018 22:18:08 +0100,
>> > Lina Iyer <ilina@codeaurora.org> wrote:

>> Also, I am exploring an option that was suggested by Stephen [1] to just
>> use the PDC interrupt as a parent of the GPIO IRQ and use a different
>> irqchip for the PDC interrupt. I ran into some issue with accessing
>> irqchip and irqdata of the PDC interrupt, since the irqchip was not in
>> hierarchy with the GPIO's irqchip. I haven't been able to find time to
>> resolve the issue that the set_parent_ functions, because of the
>> hierarchy.
>>
>> Essentially, we have two different mechanisms for GPIO IRQs based on
>> whether they can be woken up by the PDC interrupt.
>>
>> 	GPIO-IRQ --> PDC --> GIC
>>
>> 	GPIO-IRQ --> TLMM SUMMARY --> GIC
>>
>> Do you think the idea is feasible? It would avoid doing all this
>> enable/disable at every suspend and even during idle, when the TLMM
>> could be powered off.
>
>[me tries to page it all in again]
>
>You could have the PDC as part of the GPIO hierarchy:
>
>	GPIO -> PDC -> TLMM -> GIC
>
The PDC irqchip's parent is the GIC as set up in
drivers/irqchip/qcom-pdc.c. Wouldn't that conflict with the established
hierarchy?

>and always configure the PDC as a wake-up source. I just wonder if you
>can do that without setting up a parallel hierarchy between the PDC
>and the GIC. We already have similar things in the tree (see OMAP's
>wakeupgen), and it may be worth having a look.
Sure, will take a  look.

>The lack of interrupt
>replaying on the PDC is quite annoying (I have much stronger words in
>mind though), and I'm not sure we can easily fix that one without this
>parallel interrupt hack (you need something to inject edge interrupts
>in the TLMM).
>
The PDC replays the intterupt at the GIC, not the at the TLMM. So the
hierachy you recommended may not work as well here.
>>
>>
>> >> +			disable_irq(irqd->irq);
>> >> +			enable_irq(irq);
>> >> +		}
>> >> +	}
>> >
>> > Given that you're changing in_suspend and parsing the bitmap,
>> > shouldn't take the pdc spinlock?
>> >
>> Since we are the the only CPU running and suspend/resume (and even idle)
>> would be serialized I didn't see a reason for needing a lock.
>
>In that case, what is the purpose of 'in_suspend' if
>msm_gpio_irq_set_wake cannot happen during the suspend/resume phases?
>It all seems a bit inconsistent.
>
Well the disable_irq_wake that I call here, calls into the set_wake
callbacks. Hence the flag to indicate that we should ignore the PDC
interrupt configuration at that time. Let me see if I need to disable
the disable_irq_wake at all.

Thanks,
Lina

Re: [PATCH v3 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

Marc,

I am exploring an option where we don't do this enable/disable every
suspend/resume and in that process, I was able to just use the PDC
interrupt instead of the TLMM for triggering the GPIO. The PDC interrupt
(which takes over for the GPIO) has an handler like this -

On Tue, Sep 04 2018 at 15:18 -0600, Lina Iyer wrote:
>QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
>domain can wakeup the SoC, when interrupts and GPIOs are routed to its
>interrupt controller. Only select GPIOs that are deemed wakeup capable
>are routed to specific PDC pins. During low power state, the pinmux
>interrupt controller may be non-functional but the PDC would be. The PDC
>can detect the wakeup GPIO is triggered and bring the TLMM to an
>operational state.
>
>Interrupts that are level triggered will be detected at the TLMM when
>the controller becomes operational. Edge interrupts however need to be
>replayed again.
>
>Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
>but keep it disabled. During suspend, we can enable the PDC IRQ instead
>of the GPIO IRQ, which may or not be detected.
>
>Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>---
>Changes in v3:
>	- free action->name
>Changes in v2:
>	- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
>Changes in v1:
>	- Trigger GPIO in h/w from PDC IRQ handler
>	- Avoid big tables for GPIO-PDC map, pick from DT instead
>	- Use handler_data
>---
> drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++
> 1 file changed, 98 insertions(+)
>
>diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>index 0e22f52b2a19..6527a0a9edd1 100644
>--- a/drivers/pinctrl/qcom/pinctrl-msm.c
>+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>@@ -687,11 +687,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> 	const struct msm_pingroup *g;
> 	unsigned long flags;
> 	u32 val;
>+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>
> 	g = &pctrl->soc->groups[d->hwirq];
>
> 	raw_spin_lock_irqsave(&pctrl->lock, flags);
>
>+	if (pdc_irqd)
>+		irq_set_irq_type(pdc_irqd->irq, type);
>+
I skip over the TLMM configuration for the GPIO interrupt and just set
the IRQ handler for the GPIO interrupt here..

> 	/*
> 	 * For hw without possibility of detecting both edges
> 	 */
>@@ -779,9 +783,13 @@ 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;
>+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>
> 	raw_spin_lock_irqsave(&pctrl->lock, flags);
>
>+	if (pdc_irqd)
>+		irq_set_irq_wake(pdc_irqd->irq, on);
>+
> 	irq_set_irq_wake(pctrl->irq, on);
>
> 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>@@ -863,6 +871,94 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
> 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
> }
>
>+static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
>+{
>+	struct irq_data *irqd = data;
>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
>+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>+	const struct msm_pingroup *g;
>+	unsigned long flags;
>+	u32 val;
>+
>+	if (!irqd_is_level_type(irqd)) {
>+		g = &pctrl->soc->groups[irqd->hwirq];
>+		raw_spin_lock_irqsave(&pctrl->lock, flags);
>+		val = BIT(g->intr_status_bit);
>+		writel(val, pctrl->regs + g->intr_status_reg);
>+		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>+	}
>+
>+	return IRQ_HANDLED;
>+}
>+

Then ...

static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
{
       struct irq_data *irqd = data;
       struct irq_desc *desc = irq_to_desc(irqd->irq);

       desc->handle_irq(desc);

       return IRQ_HANDLED;
}

I am following check_irq_resend() but I need to call the handler for
both level and edge interrupts. Firstly, is it okay to call the
handler_irq() directly?

My other question is check_irq_resend() indicates that it should be
called with desc->lock held. Since we are invoking the handler directly
and not modifying any core state of the irq_desc, is it safe here? (Also
the locking API are not exposed, I am sure there must be a reason for it).

Thoughts?

Thanks,
Lina

>+static int msm_gpio_pdc_pin_request(struct irq_data *d)
>+{
>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>+	struct platform_device *pdev = to_platform_device(pctrl->dev);
>+	const char *pin_name;
>+	int irq;
>+	int ret;
>+
>+	pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);
>+	if (!pin_name)
>+		return -ENOMEM;
>+
>+	irq = platform_get_irq_byname(pdev, pin_name);
>+	if (irq < 0) {
>+		kfree(pin_name);
>+		return 0;
>+	}
>+
>+	ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d),
>+			  pin_name, d);
>+	if (ret) {
>+		pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq);
>+		kfree(pin_name);
>+		return ret;
>+	}
>+
>+	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
>+	disable_irq(irq);
>+
>+	return 0;
>+}
>+
>+static int msm_gpio_pdc_pin_release(struct irq_data *d)
>+{
>+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>+	const void *name;
>+
>+	if (pdc_irqd) {
>+		irq_set_handler_data(d->irq, NULL);
>+		name = free_irq(pdc_irqd->irq, d);
>+		kfree(name);
>+	}
>+
>+	return 0;
>+}
>+
>+static int msm_gpio_irq_reqres(struct irq_data *d)
>+{
>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>+
>+	if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
>+		dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
>+			irqd_to_hwirq(d));
>+		return -EINVAL;
>+	}
>+
>+	return msm_gpio_pdc_pin_request(d);
>+}
>+
>+static void msm_gpio_irq_relres(struct irq_data *d)
>+{
>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>+
>+	msm_gpio_pdc_pin_release(d);
>+	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
>+}
>+
> static int msm_gpio_init(struct msm_pinctrl *pctrl)
> {
> 	struct gpio_chip *chip;
>@@ -887,6 +983,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
> 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
> 	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;
>
> 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
> 	if (ret) {
>--
>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>a Linux Foundation Collaborative Project
>

Re: [PATCH v3 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Lina Iyer]

On Tue, Oct 02 2018 at 11:06 -0600, Lina Iyer wrote:
>Marc,
>
>I am exploring an option where we don't do this enable/disable every
>suspend/resume and in that process, I was able to just use the PDC
>interrupt instead of the TLMM for triggering the GPIO. The PDC interrupt
>(which takes over for the GPIO) has an handler like this -
>
>On Tue, Sep 04 2018 at 15:18 -0600, Lina Iyer wrote:
>>QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
>>domain can wakeup the SoC, when interrupts and GPIOs are routed to its
>>interrupt controller. Only select GPIOs that are deemed wakeup capable
>>are routed to specific PDC pins. During low power state, the pinmux
>>interrupt controller may be non-functional but the PDC would be. The PDC
>>can detect the wakeup GPIO is triggered and bring the TLMM to an
>>operational state.
>>
>>Interrupts that are level triggered will be detected at the TLMM when
>>the controller becomes operational. Edge interrupts however need to be
>>replayed again.
>>
>>Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
>>but keep it disabled. During suspend, we can enable the PDC IRQ instead
>>of the GPIO IRQ, which may or not be detected.
>>
>>Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>>---
>>Changes in v3:
>>	- free action->name
>>Changes in v2:
>>	- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
>>Changes in v1:
>>	- Trigger GPIO in h/w from PDC IRQ handler
>>	- Avoid big tables for GPIO-PDC map, pick from DT instead
>>	- Use handler_data
>>---
>>drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++
>>1 file changed, 98 insertions(+)
>>
>>diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>>index 0e22f52b2a19..6527a0a9edd1 100644
>>--- a/drivers/pinctrl/qcom/pinctrl-msm.c
>>+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>>@@ -687,11 +687,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>>	const struct msm_pingroup *g;
>>	unsigned long flags;
>>	u32 val;
>>+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>>
>>	g = &pctrl->soc->groups[d->hwirq];
>>
>>	raw_spin_lock_irqsave(&pctrl->lock, flags);
>>
>>+	if (pdc_irqd)
>>+		irq_set_irq_type(pdc_irqd->irq, type);
>>+
>I skip over the TLMM configuration for the GPIO interrupt and just set
>the IRQ handler for the GPIO interrupt here..
>
>>	/*
>>	 * For hw without possibility of detecting both edges
>>	 */
>>@@ -779,9 +783,13 @@ 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;
>>+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>>
>>	raw_spin_lock_irqsave(&pctrl->lock, flags);
>>
>>+	if (pdc_irqd)
>>+		irq_set_irq_wake(pdc_irqd->irq, on);
>>+
>>	irq_set_irq_wake(pctrl->irq, on);
>>
>>	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>>@@ -863,6 +871,94 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
>>	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
>>}
>>
>>+static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
>>+{
>>+	struct irq_data *irqd = data;
>>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
>>+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>>+	const struct msm_pingroup *g;
>>+	unsigned long flags;
>>+	u32 val;
>>+
>>+	if (!irqd_is_level_type(irqd)) {
>>+		g = &pctrl->soc->groups[irqd->hwirq];
>>+		raw_spin_lock_irqsave(&pctrl->lock, flags);
>>+		val = BIT(g->intr_status_bit);
>>+		writel(val, pctrl->regs + g->intr_status_reg);
>>+		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>>+	}
>>+
>>+	return IRQ_HANDLED;
>>+}
>>+
>
>Then ...
>
>static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
>{
>      struct irq_data *irqd = data;
>      struct irq_desc *desc = irq_to_desc(irqd->irq);
>
>      desc->handle_irq(desc);
>
>      return IRQ_HANDLED;
>}
>
>I am following check_irq_resend() but I need to call the handler for
>both level and edge interrupts. Firstly, is it okay to call the
>handler_irq() directly?
>
>My other question is check_irq_resend() indicates that it should be
>called with desc->lock held. Since we are invoking the handler directly
>and not modifying any core state of the irq_desc, is it safe here? (Also
>the locking API are not exposed, I am sure there must be a reason for it).
>
>Thoughts?
>
Would it help if I submit a RFC patch based on this idea?

>Thanks,
>Lina
>
>>+static int msm_gpio_pdc_pin_request(struct irq_data *d)
>>+{
>>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>>+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>>+	struct platform_device *pdev = to_platform_device(pctrl->dev);
>>+	const char *pin_name;
>>+	int irq;
>>+	int ret;
>>+
>>+	pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);
>>+	if (!pin_name)
>>+		return -ENOMEM;
>>+
>>+	irq = platform_get_irq_byname(pdev, pin_name);
>>+	if (irq < 0) {
>>+		kfree(pin_name);
>>+		return 0;
>>+	}
>>+
>>+	ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d),
>>+			  pin_name, d);
>>+	if (ret) {
>>+		pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq);
>>+		kfree(pin_name);
>>+		return ret;
>>+	}
>>+
>>+	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
>>+	disable_irq(irq);
>>+
>>+	return 0;
>>+}
>>+
>>+static int msm_gpio_pdc_pin_release(struct irq_data *d)
>>+{
>>+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>>+	const void *name;
>>+
>>+	if (pdc_irqd) {
>>+		irq_set_handler_data(d->irq, NULL);
>>+		name = free_irq(pdc_irqd->irq, d);
>>+		kfree(name);
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>>+static int msm_gpio_irq_reqres(struct irq_data *d)
>>+{
>>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>>+
>>+	if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
>>+		dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
>>+			irqd_to_hwirq(d));
>>+		return -EINVAL;
>>+	}
>>+
>>+	return msm_gpio_pdc_pin_request(d);
>>+}
>>+
>>+static void msm_gpio_irq_relres(struct irq_data *d)
>>+{
>>+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>>+
>>+	msm_gpio_pdc_pin_release(d);
>>+	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
>>+}
>>+
>>static int msm_gpio_init(struct msm_pinctrl *pctrl)
>>{
>>	struct gpio_chip *chip;
>>@@ -887,6 +983,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
>>	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
>>	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;
>>
>>	ret = gpiochip_add_data(&pctrl->chip, pctrl);
>>	if (ret) {
>>--
>>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>>a Linux Foundation Collaborative Project
>>

Re: [PATCH v3 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO

[Marc Zyngier]

On 09/10/18 18:07, Lina Iyer wrote:
> On Tue, Oct 02 2018 at 11:06 -0600, Lina Iyer wrote:
>> Marc,
>>
>> I am exploring an option where we don't do this enable/disable every
>> suspend/resume and in that process, I was able to just use the PDC
>> interrupt instead of the TLMM for triggering the GPIO. The PDC interrupt
>> (which takes over for the GPIO) has an handler like this -
>>
>> On Tue, Sep 04 2018 at 15:18 -0600, Lina Iyer wrote:
>>> QCOM SoC's that have Power Domain Controller (PDC) chip in the always-on
>>> domain can wakeup the SoC, when interrupts and GPIOs are routed to its
>>> interrupt controller. Only select GPIOs that are deemed wakeup capable
>>> are routed to specific PDC pins. During low power state, the pinmux
>>> interrupt controller may be non-functional but the PDC would be. The PDC
>>> can detect the wakeup GPIO is triggered and bring the TLMM to an
>>> operational state.
>>>
>>> Interrupts that are level triggered will be detected at the TLMM when
>>> the controller becomes operational. Edge interrupts however need to be
>>> replayed again.
>>>
>>> Request the corresponding PDC IRQ, when the GPIO is requested as an IRQ,
>>> but keep it disabled. During suspend, we can enable the PDC IRQ instead
>>> of the GPIO IRQ, which may or not be detected.
>>>
>>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>>> ---
>>> Changes in v3:
>>> 	- free action->name
>>> Changes in v2:
>>> 	- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
>>> Changes in v1:
>>> 	- Trigger GPIO in h/w from PDC IRQ handler
>>> 	- Avoid big tables for GPIO-PDC map, pick from DT instead
>>> 	- Use handler_data
>>> ---
>>> drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++
>>> 1 file changed, 98 insertions(+)
>>>
>>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>>> index 0e22f52b2a19..6527a0a9edd1 100644
>>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>>> @@ -687,11 +687,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>>> 	const struct msm_pingroup *g;
>>> 	unsigned long flags;
>>> 	u32 val;
>>> +	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>>>
>>> 	g = &pctrl->soc->groups[d->hwirq];
>>>
>>> 	raw_spin_lock_irqsave(&pctrl->lock, flags);
>>>
>>> +	if (pdc_irqd)
>>> +		irq_set_irq_type(pdc_irqd->irq, type);
>>> +
>> I skip over the TLMM configuration for the GPIO interrupt and just set
>> the IRQ handler for the GPIO interrupt here..
>>
>>> 	/*
>>> 	 * For hw without possibility of detecting both edges
>>> 	 */
>>> @@ -779,9 +783,13 @@ 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;
>>> +	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
>>>
>>> 	raw_spin_lock_irqsave(&pctrl->lock, flags);
>>>
>>> +	if (pdc_irqd)
>>> +		irq_set_irq_wake(pdc_irqd->irq, on);
>>> +
>>> 	irq_set_irq_wake(pctrl->irq, on);
>>>
>>> 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>>> @@ -863,6 +871,94 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
>>> 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
>>> }
>>>
>>> +static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
>>> +{
>>> +	struct irq_data *irqd = data;
>>> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
>>> +	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>>> +	const struct msm_pingroup *g;
>>> +	unsigned long flags;
>>> +	u32 val;
>>> +
>>> +	if (!irqd_is_level_type(irqd)) {
>>> +		g = &pctrl->soc->groups[irqd->hwirq];
>>> +		raw_spin_lock_irqsave(&pctrl->lock, flags);
>>> +		val = BIT(g->intr_status_bit);
>>> +		writel(val, pctrl->regs + g->intr_status_reg);
>>> +		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>>> +	}
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>
>> Then ...
>>
>> static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
>> {
>>       struct irq_data *irqd = data;
>>       struct irq_desc *desc = irq_to_desc(irqd->irq);
>>
>>       desc->handle_irq(desc);
>>
>>       return IRQ_HANDLED;
>> }
>>
>> I am following check_irq_resend() but I need to call the handler for
>> both level and edge interrupts. Firstly, is it okay to call the
>> handler_irq() directly?
>>
>> My other question is check_irq_resend() indicates that it should be
>> called with desc->lock held. Since we are invoking the handler directly
>> and not modifying any core state of the irq_desc, is it safe here? (Also
>> the locking API are not exposed, I am sure there must be a reason for it).
>>
>> Thoughts?
>>
> Would it help if I submit a RFC patch based on this idea?

Yes please. At this stage, I've completely lost track of the various 
ideas, and I'd like to restart with a clean slate.

Thanks,

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