linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC
@ 2018-08-24 20:01 Lina Iyer
  2018-08-24 20:01 ` [PATCH v2 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO Lina Iyer
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Lina Iyer @ 2018-08-24 20:01 UTC (permalink / raw)
  To: marc.zyngier, bjorn.andersson, sboyd, evgreen, linus.walleij
  Cc: rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree,
	andy.gross, dianders, Lina Iyer

Hi,

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            | 164 ++++++++++++++++++
 drivers/pinctrl/qcom/pinctrl-msm.h            |   3 +
 drivers/pinctrl/qcom/pinctrl-sdm845.c         |   6 +
 5 files changed, 425 insertions(+), 4 deletions(-)

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO
  2018-08-24 20:01 [PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC Lina Iyer
@ 2018-08-24 20:01 ` Lina Iyer
  2018-08-27 22:35   ` Matthias Kaehlcke
  2018-08-24 20:01 ` [PATCH v2 2/5] dt-bindings: pinctrl: add wakeup capable GPIOs for SDM845 Lina Iyer
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Lina Iyer @ 2018-08-24 20:01 UTC (permalink / raw)
  To: marc.zyngier, bjorn.andersson, sboyd, evgreen, linus.walleij
  Cc: rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree,
	andy.gross, dianders, 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 the
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 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 | 96 ++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 0e22f52b2a19..b675ea56a4ff 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,92 @@ 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);
+
+	if (pdc_irqd) {
+		irq_set_handler_data(d->irq, NULL);
+		free_irq(pdc_irqd->irq, d);
+	}
+
+	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 +981,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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 2/5] dt-bindings: pinctrl: add wakeup capable GPIOs for SDM845
  2018-08-24 20:01 [PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC Lina Iyer
  2018-08-24 20:01 ` [PATCH v2 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO Lina Iyer
@ 2018-08-24 20:01 ` Lina Iyer
  2018-08-28 20:23   ` Rob Herring
  2018-08-24 20:01 ` [PATCH v2 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend Lina Iyer
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Lina Iyer @ 2018-08-24 20:01 UTC (permalink / raw)
  To: marc.zyngier, bjorn.andersson, sboyd, evgreen, linus.walleij
  Cc: rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree,
	andy.gross, dianders, 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend
  2018-08-24 20:01 [PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC Lina Iyer
  2018-08-24 20:01 ` [PATCH v2 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO Lina Iyer
  2018-08-24 20:01 ` [PATCH v2 2/5] dt-bindings: pinctrl: add wakeup capable GPIOs for SDM845 Lina Iyer
@ 2018-08-24 20:01 ` Lina Iyer
  2018-08-27 22:57   ` Matthias Kaehlcke
  2018-08-24 20:01 ` [PATCH v2 4/5] drivers: pinctrl: qcom: sdm845: support GPIO wakeup from suspend Lina Iyer
  2018-08-24 20:01 ` [PATCH v2 5/5] arm64: dts: qcom: add wake up interrupts for GPIOs for SDM845 Lina Iyer
  4 siblings, 1 reply; 15+ messages in thread
From: Lina Iyer @ 2018-08-24 20:01 UTC (permalink / raw)
  To: marc.zyngier, bjorn.andersson, sboyd, evgreen, linus.walleij
  Cc: rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree,
	andy.gross, dianders, 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 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 | 70 +++++++++++++++++++++++++++++-
 drivers/pinctrl/qcom/pinctrl-msm.h |  3 ++
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index b675ea56a4ff..a880cefbd248 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;
 }
@@ -1069,6 +1085,58 @@ 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.
+		 */
+		if (irqd_is_wakeup_set(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;
+	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);
+		/*
+		 * The TLMM will be operational now, so disable
+		 * the PDC IRQ.
+		 */
+		if (irqd_is_wakeup_set(irq_get_irq_data(irq))) {
+			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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 4/5] drivers: pinctrl: qcom: sdm845: support GPIO wakeup from suspend
  2018-08-24 20:01 [PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC Lina Iyer
                   ` (2 preceding siblings ...)
  2018-08-24 20:01 ` [PATCH v2 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend Lina Iyer
@ 2018-08-24 20:01 ` Lina Iyer
  2018-08-28  0:31   ` Bjorn Andersson
  2018-08-24 20:01 ` [PATCH v2 5/5] arm64: dts: qcom: add wake up interrupts for GPIOs for SDM845 Lina Iyer
  4 siblings, 1 reply; 15+ messages in thread
From: Lina Iyer @ 2018-08-24 20:01 UTC (permalink / raw)
  To: marc.zyngier, bjorn.andersson, sboyd, evgreen, linus.walleij
  Cc: rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree,
	andy.gross, dianders, 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>
---
 drivers/pinctrl/qcom/pinctrl-sdm845.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
index 2ab7a8885757..cc333b8afb99 100644
--- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
+++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
@@ -1297,10 +1297,16 @@ static const struct of_device_id sdm845_pinctrl_of_match[] = {
 	{ },
 };
 
+static const struct dev_pm_ops msm_pinctrl_dev_pm_ops = {
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(msm_pinctrl_suspend_late,
+				     msm_pinctrl_resume_late)
+};
+
 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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 5/5] arm64: dts: qcom: add wake up interrupts for GPIOs for SDM845
  2018-08-24 20:01 [PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC Lina Iyer
                   ` (3 preceding siblings ...)
  2018-08-24 20:01 ` [PATCH v2 4/5] drivers: pinctrl: qcom: sdm845: support GPIO wakeup from suspend Lina Iyer
@ 2018-08-24 20:01 ` Lina Iyer
  4 siblings, 0 replies; 15+ messages in thread
From: Lina Iyer @ 2018-08-24 20:01 UTC (permalink / raw)
  To: marc.zyngier, bjorn.andersson, sboyd, evgreen, linus.walleij
  Cc: rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree,
	andy.gross, dianders, 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>
---
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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO
  2018-08-24 20:01 ` [PATCH v2 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO Lina Iyer
@ 2018-08-27 22:35   ` Matthias Kaehlcke
  2018-09-04 17:51     ` Lina Iyer
  0 siblings, 1 reply; 15+ messages in thread
From: Matthias Kaehlcke @ 2018-08-27 22:35 UTC (permalink / raw)
  To: Lina Iyer
  Cc: marc.zyngier, bjorn.andersson, sboyd, evgreen, linus.walleij,
	rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree,
	andy.gross, dianders

Hi Lina,

On Fri, Aug 24, 2018 at 02:01:53PM -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 the
> its interrupt controller. Only select GPIOs that are deemed wakeup

wording nit: "are routed to the|its interrupt controller"

> 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 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 | 96 ++++++++++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 0e22f52b2a19..b675ea56a4ff 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,92 @@ 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;

Do I understand correctly that this is the case where the pin isn't
routed to the PDC?

> +	}
> +
> +	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);

'\n' is missing

> +		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);
> +
> +	if (pdc_irqd) {
> +		irq_set_handler_data(d->irq, NULL);
> +		free_irq(pdc_irqd->irq, d);

You need to free 'pin_name' allocated in msm_gpio_pdc_pin_request().
IIUC it should be available in irq_desc->action->name.

Cheers

Matthias


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend
  2018-08-24 20:01 ` [PATCH v2 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend Lina Iyer
@ 2018-08-27 22:57   ` Matthias Kaehlcke
  2018-09-04 17:47     ` Lina Iyer
  0 siblings, 1 reply; 15+ messages in thread
From: Matthias Kaehlcke @ 2018-08-27 22:57 UTC (permalink / raw)
  To: Lina Iyer
  Cc: marc.zyngier, bjorn.andersson, sboyd, evgreen, linus.walleij,
	rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree,
	andy.gross, dianders

Hi Lina,

On Fri, Aug 24, 2018 at 02:01:55PM -0600, Lina Iyer 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 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 | 70 +++++++++++++++++++++++++++++-
>  drivers/pinctrl/qcom/pinctrl-msm.h |  3 ++
>  2 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index b675ea56a4ff..a880cefbd248 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;
>  }
> @@ -1069,6 +1085,58 @@ 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.
> +		 */
> +		if (irqd_is_wakeup_set(irqd)) {
> +			disable_irq_wake(irqd->irq);
> +			disable_irq(irqd->irq);
> +			enable_irq(irq);
> +		}

Would it make sense to limit this to edge triggered interrupts since
the interrupt handler does nothing for level triggered ones?

Cheers

Matthias

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 4/5] drivers: pinctrl: qcom: sdm845: support GPIO wakeup from suspend
  2018-08-24 20:01 ` [PATCH v2 4/5] drivers: pinctrl: qcom: sdm845: support GPIO wakeup from suspend Lina Iyer
@ 2018-08-28  0:31   ` Bjorn Andersson
  2018-08-28  1:44     ` Lina Iyer
  0 siblings, 1 reply; 15+ messages in thread
From: Bjorn Andersson @ 2018-08-28  0:31 UTC (permalink / raw)
  To: Lina Iyer
  Cc: marc.zyngier, sboyd, evgreen, linus.walleij, rplsssn,
	linux-kernel, linux-arm-msm, rnayak, devicetree, andy.gross,
	dianders

On Fri 24 Aug 13:01 PDT 2018, Lina Iyer wrote:

> 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>
> ---
>  drivers/pinctrl/qcom/pinctrl-sdm845.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> index 2ab7a8885757..cc333b8afb99 100644
> --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
> +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
> @@ -1297,10 +1297,16 @@ static const struct of_device_id sdm845_pinctrl_of_match[] = {
>  	{ },
>  };
>  
> +static const struct dev_pm_ops msm_pinctrl_dev_pm_ops = {
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(msm_pinctrl_suspend_late,
> +				     msm_pinctrl_resume_late)
> +};
> +

I expect these four lines to be duplicated in every platform file, so I
think it would be better to just move it to pinctrl-msm.c and extern
declare it in pinctrl-msm.h.

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

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 4/5] drivers: pinctrl: qcom: sdm845: support GPIO wakeup from suspend
  2018-08-28  0:31   ` Bjorn Andersson
@ 2018-08-28  1:44     ` Lina Iyer
  0 siblings, 0 replies; 15+ messages in thread
From: Lina Iyer @ 2018-08-28  1:44 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: marc.zyngier, sboyd, evgreen, linus.walleij, rplsssn,
	linux-kernel, linux-arm-msm, rnayak, devicetree, andy.gross,
	dianders

On Mon, Aug 27 2018 at 18:31 -0600, Bjorn Andersson wrote:
>On Fri 24 Aug 13:01 PDT 2018, Lina Iyer wrote:
>
>> 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>
>> ---
>>  drivers/pinctrl/qcom/pinctrl-sdm845.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c b/drivers/pinctrl/qcom/pinctrl-sdm845.c
>> index 2ab7a8885757..cc333b8afb99 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
>> @@ -1297,10 +1297,16 @@ static const struct of_device_id sdm845_pinctrl_of_match[] = {
>>  	{ },
>>  };
>>
>> +static const struct dev_pm_ops msm_pinctrl_dev_pm_ops = {
>> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(msm_pinctrl_suspend_late,
>> +				     msm_pinctrl_resume_late)
>> +};
>> +
>
>I expect these four lines to be duplicated in every platform file, so I
>think it would be better to just move it to pinctrl-msm.c and extern
>declare it in pinctrl-msm.h.
>
Sure.

>>  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,
>
>Regards,
>Bjorn

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/5] dt-bindings: pinctrl: add wakeup capable GPIOs for SDM845
  2018-08-24 20:01 ` [PATCH v2 2/5] dt-bindings: pinctrl: add wakeup capable GPIOs for SDM845 Lina Iyer
@ 2018-08-28 20:23   ` Rob Herring
  0 siblings, 0 replies; 15+ messages in thread
From: Rob Herring @ 2018-08-28 20:23 UTC (permalink / raw)
  To: Lina Iyer
  Cc: marc.zyngier, bjorn.andersson, sboyd, evgreen, linus.walleij,
	rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree,
	andy.gross, dianders

On Fri, Aug 24, 2018 at 02:01:54PM -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
> ---

Please add acks/reviewed-bys when posting new versions. These changes 
look trivial enough to have kept my R-by.

>  .../bindings/pinctrl/qcom,sdm845-pinctrl.txt  | 104 +++++++++++++++++-
>  1 file changed, 101 insertions(+), 3 deletions(-)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend
  2018-08-27 22:57   ` Matthias Kaehlcke
@ 2018-09-04 17:47     ` Lina Iyer
  0 siblings, 0 replies; 15+ messages in thread
From: Lina Iyer @ 2018-09-04 17:47 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: marc.zyngier, bjorn.andersson, sboyd, evgreen, linus.walleij,
	rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree,
	andy.gross, dianders

On Mon, Aug 27 2018 at 16:57 -0600, Matthias Kaehlcke wrote:
>Hi Lina,
>
>On Fri, Aug 24, 2018 at 02:01:55PM -0600, Lina Iyer 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 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 | 70 +++++++++++++++++++++++++++++-
>>  drivers/pinctrl/qcom/pinctrl-msm.h |  3 ++
>>  2 files changed, 72 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index b675ea56a4ff..a880cefbd248 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;
>>  }
>> @@ -1069,6 +1085,58 @@ 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.
>> +		 */
>> +		if (irqd_is_wakeup_set(irqd)) {
>> +			disable_irq_wake(irqd->irq);
>> +			disable_irq(irqd->irq);
>> +			enable_irq(irq);
>> +		}
>
>Would it make sense to limit this to edge triggered interrupts since
>the interrupt handler does nothing for level triggered ones?
>
Sure.

-- Lina

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO
  2018-08-27 22:35   ` Matthias Kaehlcke
@ 2018-09-04 17:51     ` Lina Iyer
  0 siblings, 0 replies; 15+ messages in thread
From: Lina Iyer @ 2018-09-04 17:51 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: marc.zyngier, bjorn.andersson, sboyd, evgreen, linus.walleij,
	rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree,
	andy.gross, dianders

On Mon, Aug 27 2018 at 16:35 -0600, Matthias Kaehlcke wrote:
>Hi Lina,
>
>On Fri, Aug 24, 2018 at 02:01:53PM -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 the
>> its interrupt controller. Only select GPIOs that are deemed wakeup
>
>wording nit: "are routed to the|its interrupt controller"
>
Okay.

>> 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 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 | 96 ++++++++++++++++++++++++++++++
>>  1 file changed, 96 insertions(+)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 0e22f52b2a19..b675ea56a4ff 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,92 @@ 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;
>
>Do I understand correctly that this is the case where the pin isn't
>routed to the PDC?
>
Yes, correct.

>> +	}
>> +
>> +	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);
>
>'\n' is missing
>
ok.
>> +		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);
>> +
>> +	if (pdc_irqd) {
>> +		irq_set_handler_data(d->irq, NULL);
>> +		free_irq(pdc_irqd->irq, d);
>
>You need to free 'pin_name' allocated in msm_gpio_pdc_pin_request().
>IIUC it should be available in irq_desc->action->name.
>
Yes, I didn't realize that free_irq returns the name when I posted this
series (noted in the cover letter). Will fix.

-- Lina


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC
  2018-08-17 16:38 [PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC Lina Iyer
@ 2018-08-17 19:31 ` Lina Iyer
  0 siblings, 0 replies; 15+ messages in thread
From: Lina Iyer @ 2018-08-17 19:31 UTC (permalink / raw)
  To: marc.zyngier, bjorn.andersson, sboyd, evgreen, linus.walleij
  Cc: rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree,
	andy.gross, dianders


Please ignore this series. The series is incorrectly marked as v2. I am
resending it as v1.

On Fri, Aug 17 2018 at 10:39 -0600, Lina Iyer wrote:
>Hi,
>
>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)
>
>Dependencies:
>	https://lkml.org/lkml/2018/8/17/137
>	https://lkml.org/lkml/2018/8/15/289
>
>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
>
>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  |  58 ++++++-
> arch/arm64/boot/dts/qcom/sdm845.dtsi          |  57 ++++++-
> drivers/pinctrl/qcom/pinctrl-msm.c            | 155 ++++++++++++++++++
> drivers/pinctrl/qcom/pinctrl-msm.h            |   3 +
> drivers/pinctrl/qcom/pinctrl-sdm845.c         |   6 +
> 5 files changed, 275 insertions(+), 4 deletions(-)
>
>--
>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>a Linux Foundation Collaborative Project
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC
@ 2018-08-17 16:38 Lina Iyer
  2018-08-17 19:31 ` Lina Iyer
  0 siblings, 1 reply; 15+ messages in thread
From: Lina Iyer @ 2018-08-17 16:38 UTC (permalink / raw)
  To: marc.zyngier, bjorn.andersson, sboyd, evgreen, linus.walleij
  Cc: rplsssn, linux-kernel, linux-arm-msm, rnayak, devicetree,
	andy.gross, dianders, Lina Iyer

Hi,

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)

Dependencies:
	https://lkml.org/lkml/2018/8/17/137
	https://lkml.org/lkml/2018/8/15/289

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

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  |  58 ++++++-
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |  57 ++++++-
 drivers/pinctrl/qcom/pinctrl-msm.c            | 155 ++++++++++++++++++
 drivers/pinctrl/qcom/pinctrl-msm.h            |   3 +
 drivers/pinctrl/qcom/pinctrl-sdm845.c         |   6 +
 5 files changed, 275 insertions(+), 4 deletions(-)

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2018-09-04 17:51 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-24 20:01 [PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC Lina Iyer
2018-08-24 20:01 ` [PATCH v2 1/5] drivers: pinctrl: qcom: add wakeup capability to GPIO Lina Iyer
2018-08-27 22:35   ` Matthias Kaehlcke
2018-09-04 17:51     ` Lina Iyer
2018-08-24 20:01 ` [PATCH v2 2/5] dt-bindings: pinctrl: add wakeup capable GPIOs for SDM845 Lina Iyer
2018-08-28 20:23   ` Rob Herring
2018-08-24 20:01 ` [PATCH v2 3/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend Lina Iyer
2018-08-27 22:57   ` Matthias Kaehlcke
2018-09-04 17:47     ` Lina Iyer
2018-08-24 20:01 ` [PATCH v2 4/5] drivers: pinctrl: qcom: sdm845: support GPIO wakeup from suspend Lina Iyer
2018-08-28  0:31   ` Bjorn Andersson
2018-08-28  1:44     ` Lina Iyer
2018-08-24 20:01 ` [PATCH v2 5/5] arm64: dts: qcom: add wake up interrupts for GPIOs for SDM845 Lina Iyer
  -- strict thread matches above, loose matches on Subject: below --
2018-08-17 16:38 [PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC Lina Iyer
2018-08-17 19:31 ` Lina Iyer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).