[PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC

[PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC

[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

[PATCH v2 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 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 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 | 97 ++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 0e22f52b2a19..03ef1d29d078 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,93 @@ 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);
+	unsigned irq;
+	unsigned long trigger;
+	const char *pin_name;
+	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;
+	}
+
+	trigger = irqd_get_trigger_type(d) | IRQF_ONESHOT | IRQF_NO_SUSPEND;
+	ret = request_irq(irq, wake_irq_gpio_handler, trigger, 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 +982,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 v2 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>
---
 .../bindings/pinctrl/qcom,sdm845-pinctrl.txt  | 58 ++++++++++++++++++-
 1 file changed, 55 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..d7408cc74e01 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,52 @@ 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 0>,
+			<&pdc 510 0>, <&pdc 511 0>, <&pdc 512 0>, <&pdc 513 0>,
+			<&pdc 514 0>, <&pdc 515 0>, <&pdc 516 0>, <&pdc 517 0>,
+			<&pdc 518 0>, <&pdc 519 0>, <&pdc 632 0>, <&pdc 639 0>,
+			<&pdc 521 0>, <&pdc 522 0>, <&pdc 523 0>, <&pdc 524 0>,
+			<&pdc 525 0>, <&pdc 526 0>, <&pdc 527 0>, <&pdc 630 0>,
+			<&pdc 637 0>, <&pdc 529 0>, <&pdc 530 0>, <&pdc 531 0>,
+			<&pdc 532 0>, <&pdc 633 0>, <&pdc 640 0>, <&pdc 534 0>,
+			<&pdc 535 0>, <&pdc 536 0>, <&pdc 537 0>, <&pdc 538 0>,
+			<&pdc 539 0>, <&pdc 540 0>, <&pdc 541 0>, <&pdc 542 0>,
+			<&pdc 543 0>, <&pdc 544 0>, <&pdc 545 0>, <&pdc 546 0>,
+			<&pdc 547 0>, <&pdc 548 0>, <&pdc 549 0>, <&pdc 550 0>,
+			<&pdc 551 0>, <&pdc 552 0>, <&pdc 553 0>, <&pdc 554 0>,
+			<&pdc 555 0>, <&pdc 556 0>, <&pdc 557 0>, <&pdc 631 0>,
+			<&pdc 638 0>, <&pdc 559 0>, <&pdc 560 0>, <&pdc 561 0>,
+			<&pdc 562 0>, <&pdc 563 0>, <&pdc 564 0>, <&pdc 565 0>,
+			<&pdc 566 0>, <&pdc 570 0>, <&pdc 571 0>, <&pdc 572 0>,
+			<&pdc 573 0>, <&pdc 609 0>, <&pdc 610 0>, <&pdc 611 0>,
+			<&pdc 612 0>, <&pdc 613 0>, <&pdc 614 0>, <&pdc 615 0>,
+			<&pdc 617 0>, <&pdc 618 0>, <&pdc 619 0>, <&pdc 620 0>,
+			<&pdc 621 0>, <&pdc 622 0>, <&pdc 623 0>;
+		interrupt-names = "summary-irq",
+			"gpio1", "gpio3", "gpio5", "gpio10",
+			"gpio11", "gpio20", "gpio22", "gpio24",
+			"gpio26", "gpio30", "gpio31", "gpio31",
+			"gpio32", "gpio34", "gpio36", "gpio37",
+			"gpio38", "gpio39", "gpio40", "gpio41",
+			"gpio41", "gpio43", "gpio44", "gpio46",
+			"gpio48", "gpio49", "gpio49", "gpio52",
+			"gpio53", "gpio54", "gpio56", "gpio57",
+			"gpio58", "gpio59", "gpio60", "gpio61",
+			"gpio62", "gpio63", "gpio64", "gpio66",
+			"gpio68", "gpio71", "gpio73", "gpio77",
+			"gpio78", "gpio79", "gpio80", "gpio84",
+			"gpio85", "gpio86", "gpio88", "gpio89",
+			"gpio89", "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";
 
 		qup9_active: qup9-active {
 			mux {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 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>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 60 +++++++++++++++++++++++++++++-
 drivers/pinctrl/qcom/pinctrl-msm.h |  3 ++
 2 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 03ef1d29d078..17e541f8f09d 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_IRQ 1024
 #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_irqs:       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,14 @@ struct msm_pinctrl {
 
 	DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
 	DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
+	DECLARE_BITMAP(pdc_irqs, MAX_PDC_IRQ);
 
 	const struct msm_pinctrl_soc_data *soc;
 	void __iomem *regs;
 };
 
+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 +792,11 @@ 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);
+		on ? set_bit(pdc_irqd->irq, pctrl->pdc_irqs) :
+		     clear_bit(pdc_irqd->irq, pctrl->pdc_irqs);
+	}
 
 	irq_set_irq_wake(pctrl->irq, on);
 
@@ -920,6 +928,8 @@ 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);
 	disable_irq(irq);
 
 	return 0;
@@ -1070,6 +1080,54 @@ 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;
+	int i;
+
+	in_suspend = true;
+	for_each_set_bit(i, pctrl->pdc_irqs, MAX_PDC_IRQ) {
+		irqd = irq_get_handler_data(i);
+		/*
+		 * 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)) {
+			irq_set_irq_wake(irqd->irq, false);
+			disable_irq(irqd->irq);
+			enable_irq(i);
+		}
+	}
+
+	return 0;
+}
+
+int __maybe_unused msm_pinctrl_resume_late(struct device *dev)
+{
+	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
+	struct irq_data *irqd;
+	int i;
+
+	for_each_set_bit(i, pctrl->pdc_irqs, MAX_PDC_IRQ) {
+		irqd = irq_get_handler_data(i);
+		/*
+		 * The TLMM will be operational now, so disable
+		 * the PDC IRQ.
+		 */
+		if (irqd_is_wakeup_set(irq_get_irq_data(i))) {
+			disable_irq_nosync(i);
+			irq_set_irq_wake(irqd->irq, true);
+			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 v2 4/5] drivers: pinctrl: qcom: sdm845: support GPIO wakeup from suspend

[Lina Iyer]

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

[PATCH v2 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>
---
Changes in v1:
	- Use interrupt-extended for all TLMM interrupts
	- Define GPIO-PDC map using interrupt-names
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 57 +++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 87ffc32dc597..2379684373d3 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -712,11 +712,66 @@
 		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 0>,
+				<&pdc 510 0>, <&pdc 511 0>, <&pdc 512 0>,
+				<&pdc 513 0>, <&pdc 514 0>, <&pdc 515 0>,
+				<&pdc 516 0>, <&pdc 517 0>, <&pdc 518 0>,
+				<&pdc 519 0>, <&pdc 632 0>, <&pdc 639 0>,
+				<&pdc 521 0>, <&pdc 522 0>, <&pdc 523 0>,
+				<&pdc 524 0>, <&pdc 525 0>, <&pdc 526 0>,
+				<&pdc 527 0>, <&pdc 630 0>, <&pdc 637 0>,
+				<&pdc 529 0>, <&pdc 530 0>, <&pdc 531 0>,
+				<&pdc 532 0>, <&pdc 633 0>, <&pdc 640 0>,
+				<&pdc 534 0>, <&pdc 535 0>, <&pdc 536 0>,
+				<&pdc 537 0>, <&pdc 538 0>, <&pdc 539 0>,
+				<&pdc 540 0>, <&pdc 541 0>, <&pdc 542 0>,
+				<&pdc 543 0>, <&pdc 544 0>, <&pdc 545 0>,
+				<&pdc 546 0>, <&pdc 547 0>, <&pdc 548 0>,
+				<&pdc 549 0>, <&pdc 550 0>, <&pdc 551 0>,
+				<&pdc 552 0>, <&pdc 553 0>, <&pdc 554 0>,
+				<&pdc 555 0>, <&pdc 556 0>, <&pdc 557 0>,
+				<&pdc 631 0>, <&pdc 638 0>, <&pdc 559 0>,
+				<&pdc 560 0>, <&pdc 561 0>, <&pdc 562 0>,
+				<&pdc 563 0>, <&pdc 564 0>, <&pdc 565 0>,
+				<&pdc 566 0>, <&pdc 570 0>, <&pdc 571 0>,
+				<&pdc 572 0>, <&pdc 573 0>, <&pdc 609 0>,
+				<&pdc 610 0>, <&pdc 611 0>, <&pdc 612 0>,
+				<&pdc 613 0>, <&pdc 614 0>, <&pdc 615 0>,
+				<&pdc 617 0>, <&pdc 618 0>, <&pdc 619 0>,
+				<&pdc 620 0>, <&pdc 621 0>, <&pdc 622 0>,
+				<&pdc 623 0>;
+			interrupt-names = "summary-irq",
+				"gpio1", "gpio3", "gpio5",
+				"gpio10", "gpio11", "gpio20",
+				"gpio22", "gpio24", "gpio26",
+				"gpio30", "gpio31", "gpio31",
+				"gpio32", "gpio34", "gpio36",
+				"gpio37", "gpio38", "gpio39",
+				"gpio40", "gpio41", "gpio41",
+				"gpio43", "gpio44", "gpio46",
+				"gpio48", "gpio49", "gpio49",
+				"gpio52", "gpio53", "gpio54",
+				"gpio56", "gpio57", "gpio58",
+				"gpio59", "gpio60", "gpio61",
+				"gpio62", "gpio63", "gpio64",
+				"gpio66", "gpio68", "gpio71",
+				"gpio73", "gpio77", "gpio78",
+				"gpio79", "gpio80", "gpio84",
+				"gpio85", "gpio86", "gpio88",
+				"gpio89", "gpio89", "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";
 
 			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 v2 0/5] Wakeup GPIO support for SDM845 SoC

[Lina Iyer]

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
>

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

[Bjorn Andersson]

On Fri 17 Aug 09:38 PDT 2018, Lina Iyer wrote:

Thanks Lina, I think this looks like a very reasonable approach!

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

Afaict we can model a driver for the MPM hardware - for previous
platforms - after your PDC driver and all of this logic will be reused.

As such I think it would be better to use the word "wake" instead of
"pdc" in the implementation. I don't see a problem with the commit
message being specific and talking about the PDC though, so keep that.

> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
> ---
> 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 | 97 ++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 0e22f52b2a19..03ef1d29d078 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);

Given that the TLMM summary logic isn't powered during a collapse, is
there really a point in toggling the wake of the summary irq? (I wrote
this, not sure it is correct)

Also, we're not modifying any tlmm state here, so we shouldn't need that
spinlock.

>  
>  	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> @@ -863,6 +871,93 @@ 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)) {

This deserves a comment in the code as well.

> +		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);
> +	unsigned irq;
> +	unsigned long trigger;
> +	const char *pin_name;
> +	int ret;
> +
> +	pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);

pin_name needs to be released in msm_gpio_pdc_pin_release() as well.

> +	if (!pin_name)
> +		return -ENOMEM;
> +
> +	irq = platform_get_irq_byname(pdev, pin_name);
> +	if (irq < 0) {
> +		kfree(pin_name);
> +		return 0;
> +	}
> +
> +	trigger = irqd_get_trigger_type(d) | IRQF_ONESHOT | IRQF_NO_SUSPEND;
> +	ret = request_irq(irq, wake_irq_gpio_handler, trigger, 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);

free_irq() returns what was "pin_name" in msm_gpio_pdc_pin_request(), so
you should be able to free that.

> +	}
> +
> +	return 0;
> +}
> +

Regards,
Bjorn

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

[Linus Walleij]

On Fri, Aug 17, 2018 at 6:39 PM 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 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 v1:
>         - Trigger GPIO in h/w from PDC IRQ handler
>         - Avoid big tables for GPIO-PDC map, pick from DT instead
>         - Use handler_data

Just for the record this is an impressive and much needed patch
set, no other SoC developer has yet taken on the task of making this
work so I very much appreciate that Qualcomm show the way.

> +static int msm_gpio_pdc_pin_request(struct irq_data *d)
> +static int msm_gpio_pdc_pin_release(struct irq_data *d)
> +static int msm_gpio_irq_reqres(struct irq_data *d)
> +{
(...)
> +       if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
(...)
> +static void msm_gpio_irq_relres(struct irq_data *d)
> +{
> +       gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
> +}

FYI Hans Verkuil is working on a patch set that moves the
lock/unlock as IRQ call to the irqchip request() and release()
functions so we can switch a GPIO irqchip line from IRQ
mode to say output at runtime without too much trouble.
(CEC needs this.)

I suspect that will make your work easier?

Hans can you include Lina in the loop for your patches
so she can take that into accoun because I think we might
need that as a base for this.

Yours,
Linus Walleij

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

[Lina Iyer]

On Mon, Aug 20 2018 at 00:05 -0600, Bjorn Andersson wrote:
>On Fri 17 Aug 09:38 PDT 2018, Lina Iyer wrote:
>
>Thanks Lina, I think this looks like a very reasonable approach!
>
>> 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.
>>
>
>Afaict we can model a driver for the MPM hardware - for previous
>platforms - after your PDC driver and all of this logic will be reused.
>
>As such I think it would be better to use the word "wake" instead of
>"pdc" in the implementation. I don't see a problem with the commit
>message being specific and talking about the PDC though, so keep that.
>
>> Signed-off-by: Lina Iyer <ilina@codeaurora.org>
>> ---
>> 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 | 97 ++++++++++++++++++++++++++++++
>>  1 file changed, 97 insertions(+)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 0e22f52b2a19..03ef1d29d078 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);
>
>Given that the TLMM summary logic isn't powered during a collapse, is
>there really a point in toggling the wake of the summary irq? (I wrote
>this, not sure it is correct)
>
>Also, we're not modifying any tlmm state here, so we shouldn't need that
>spinlock.
>
Okay.
>>
>>  	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>> @@ -863,6 +871,93 @@ 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)) {
>
>This deserves a comment in the code as well.
>
Will add.

>> +		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);
>> +	unsigned irq;
>> +	unsigned long trigger;
>> +	const char *pin_name;
>> +	int ret;
>> +
>> +	pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);
>
>pin_name needs to be released in msm_gpio_pdc_pin_release() as well.
>
>> +	if (!pin_name)
>> +		return -ENOMEM;
>> +
>> +	irq = platform_get_irq_byname(pdev, pin_name);
>> +	if (irq < 0) {
>> +		kfree(pin_name);
>> +		return 0;
>> +	}
>> +
>> +	trigger = irqd_get_trigger_type(d) | IRQF_ONESHOT | IRQF_NO_SUSPEND;
>> +	ret = request_irq(irq, wake_irq_gpio_handler, trigger, 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);
>
>free_irq() returns what was "pin_name" in msm_gpio_pdc_pin_request(), so
>you should be able to free that.
>
Just realized the return value. Will fix.

Thanks,
Lina

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

[Lina Iyer]

On Sun, Aug 26 2018 at 08:33 -0600, Linus Walleij wrote:
>On Fri, Aug 17, 2018 at 6:39 PM 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 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 v1:
>>         - Trigger GPIO in h/w from PDC IRQ handler
>>         - Avoid big tables for GPIO-PDC map, pick from DT instead
>>         - Use handler_data
>
>Just for the record this is an impressive and much needed patch
>set, no other SoC developer has yet taken on the task of making this
>work so I very much appreciate that Qualcomm show the way.
>
>> +static int msm_gpio_pdc_pin_request(struct irq_data *d)
>> +static int msm_gpio_pdc_pin_release(struct irq_data *d)
>> +static int msm_gpio_irq_reqres(struct irq_data *d)
>> +{
>(...)
>> +       if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
>(...)
>> +static void msm_gpio_irq_relres(struct irq_data *d)
>> +{
>> +       gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
>> +}
>
>FYI Hans Verkuil is working on a patch set that moves the
>lock/unlock as IRQ call to the irqchip request() and release()
>functions so we can switch a GPIO irqchip line from IRQ
>mode to say output at runtime without too much trouble.
>(CEC needs this.)
>
Thanks, I will look into Hans's RFCv2. But what would help me would be
to avoid creating the IRQ for the GPIO itself (I have the latent IRQ),
if I could just return that instead in gpio_to_irq(), it might be
easier. I understand ->to_irq() is supposed to be a translate function
only, I can avoid the dance of enabling and diabling the PDC IRQ on
suspend and resume.

-- Lina

>I suspect that will make your work easier?
>
>Hans can you include Lina in the loop for your patches
>so she can take that into accoun because I think we might
>need that as a base for this.
>
>Yours,
>Linus Walleij

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

[Bjorn Andersson]

On Mon 27 Aug 09:56 PDT 2018, Lina Iyer wrote:

> On Sun, Aug 26 2018 at 08:33 -0600, Linus Walleij wrote:
> > On Fri, Aug 17, 2018 at 6:39 PM 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 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 v1:
> > >         - Trigger GPIO in h/w from PDC IRQ handler
> > >         - Avoid big tables for GPIO-PDC map, pick from DT instead
> > >         - Use handler_data
> > 
> > Just for the record this is an impressive and much needed patch
> > set, no other SoC developer has yet taken on the task of making this
> > work so I very much appreciate that Qualcomm show the way.
> > 
> > > +static int msm_gpio_pdc_pin_request(struct irq_data *d)
> > > +static int msm_gpio_pdc_pin_release(struct irq_data *d)
> > > +static int msm_gpio_irq_reqres(struct irq_data *d)
> > > +{
> > (...)
> > > +       if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
> > (...)
> > > +static void msm_gpio_irq_relres(struct irq_data *d)
> > > +{
> > > +       gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
> > > +}
> > 
> > FYI Hans Verkuil is working on a patch set that moves the
> > lock/unlock as IRQ call to the irqchip request() and release()
> > functions so we can switch a GPIO irqchip line from IRQ
> > mode to say output at runtime without too much trouble.
> > (CEC needs this.)
> > 
> Thanks, I will look into Hans's RFCv2. But what would help me would be
> to avoid creating the IRQ for the GPIO itself (I have the latent IRQ),
> if I could just return that instead in gpio_to_irq(), it might be
> easier. I understand ->to_irq() is supposed to be a translate function
> only, I can avoid the dance of enabling and diabling the PDC IRQ on
> suspend and resume.
> 

I did implement gpio_to_irq() like this in the PMIC gpio/mpp drivers and
we've since concluded that we need to move this to some hierarchical
interrupt controller, because people like Linus expect to be able to say

  interrupts = <&gpio_controller 1 IRQ_TYPE_EDGE_RISING> 

which is something used all over the place with the TLMM driver today.

Regards,
Bjorn

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

[Lina Iyer]

On Mon, Aug 27 2018 at 18:26 -0600, Bjorn Andersson wrote:
>On Mon 27 Aug 09:56 PDT 2018, Lina Iyer wrote:
>
>> On Sun, Aug 26 2018 at 08:33 -0600, Linus Walleij wrote:
>> > On Fri, Aug 17, 2018 at 6:39 PM 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 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 v1:
>> > >         - Trigger GPIO in h/w from PDC IRQ handler
>> > >         - Avoid big tables for GPIO-PDC map, pick from DT instead
>> > >         - Use handler_data
>> >
>> > Just for the record this is an impressive and much needed patch
>> > set, no other SoC developer has yet taken on the task of making this
>> > work so I very much appreciate that Qualcomm show the way.
>> >
>> > > +static int msm_gpio_pdc_pin_request(struct irq_data *d)
>> > > +static int msm_gpio_pdc_pin_release(struct irq_data *d)
>> > > +static int msm_gpio_irq_reqres(struct irq_data *d)
>> > > +{
>> > (...)
>> > > +       if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
>> > (...)
>> > > +static void msm_gpio_irq_relres(struct irq_data *d)
>> > > +{
>> > > +       gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
>> > > +}
>> >
>> > FYI Hans Verkuil is working on a patch set that moves the
>> > lock/unlock as IRQ call to the irqchip request() and release()
>> > functions so we can switch a GPIO irqchip line from IRQ
>> > mode to say output at runtime without too much trouble.
>> > (CEC needs this.)
>> >
>> Thanks, I will look into Hans's RFCv2. But what would help me would be
>> to avoid creating the IRQ for the GPIO itself (I have the latent IRQ),
>> if I could just return that instead in gpio_to_irq(), it might be
>> easier. I understand ->to_irq() is supposed to be a translate function
>> only, I can avoid the dance of enabling and diabling the PDC IRQ on
>> suspend and resume.
>>
>
>I did implement gpio_to_irq() like this in the PMIC gpio/mpp drivers and
>we've since concluded that we need to move this to some hierarchical
>interrupt controller, because people like Linus expect to be able to say
>
>  interrupts = <&gpio_controller 1 IRQ_TYPE_EDGE_RISING>
>
>which is something used all over the place with the TLMM driver today.

Does it have to be &gpio_controller, can it be another interrupt controller?

Say,
	interrupts-extended = <&pdc 1 IRQ_TYPE_EDGE_RISING>;

-- Lina

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

[Bjorn Andersson]

On Mon 27 Aug 18:46 PDT 2018, Lina Iyer wrote:
> On Mon, Aug 27 2018 at 18:26 -0600, Bjorn Andersson wrote:
> > On Mon 27 Aug 09:56 PDT 2018, Lina Iyer wrote:
[..]
> > > Thanks, I will look into Hans's RFCv2. But what would help me would be
> > > to avoid creating the IRQ for the GPIO itself (I have the latent IRQ),
> > > if I could just return that instead in gpio_to_irq(), it might be
> > > easier. I understand ->to_irq() is supposed to be a translate function
> > > only, I can avoid the dance of enabling and diabling the PDC IRQ on
> > > suspend and resume.
> > > 
> > 
> > I did implement gpio_to_irq() like this in the PMIC gpio/mpp drivers and
> > we've since concluded that we need to move this to some hierarchical
> > interrupt controller, because people like Linus expect to be able to say
> > 
> >  interrupts = <&gpio_controller 1 IRQ_TYPE_EDGE_RISING>
> > 
> > which is something used all over the place with the TLMM driver today.
> 
> Does it have to be &gpio_controller, can it be another interrupt controller?
> 
> Say,
> 	interrupts-extended = <&pdc 1 IRQ_TYPE_EDGE_RISING>;
> 

It would require that the GPIO interrupt number space of the PDC matches
the pin numbering of the TLMM, to be somewhat maintainable.

And it would still require DT-writers to know that if the implementation
of a compatible, that references a TLMM IRQ, wants to mark the IRQ wake
capable it needs to reference the PDC instead...while still having a
pinmux/pinconf setting for the TLMM.

And for gpio_to_irq() we would need to do the mapping that you suggest,
so the TLMM still needs to have all these references to the PDC.


So I think it would be nice if we could avoid this scenario, but I don't
have any good ideas of how to do this right now...

Regards,
Bjorn

[PATCH v2 0/5] Wakeup GPIO support for SDM845 SoC

[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