Wakeup GPIO support for SDM845

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

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

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

Signed-off-by Lina Iyer <[email protected]>

---
Changes in v3:
- free action->name
Changes in v2:
- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
Changes in v1:
- Trigger GPIO in h/w from PDC IRQ handler
- Avoid big tables for GPIO-PDC map, pick from DT instead
- Use handler_data
---
drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++

1 file changed, 98 insertions(+)

drivers/pinctrl/qcom/pinctrl-msm.c +41 -0

index 0e22f52b2a19..6527a0a9edd1 100644
@@ -687,11 +687,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	const struct msm_pingroup *g;
 	unsigned long flags;
 	u32 val;
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
 
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	if (pdc_irqd)
+		irq_set_irq_type(pdc_irqd->irq, type);
+
 	/*
 	 * For hw without possibility of detecting both edges
 	 */

@@ -779,9 +783,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
 	unsigned long flags;
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	if (pdc_irqd)
+		irq_set_irq_wake(pdc_irqd->irq, on);
+
 	irq_set_irq_wake(pctrl->irq, on);
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);

@@ -863,6 +871,94 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
 }
 
+static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
+{
+	struct irq_data *irqd = data;
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	const struct msm_pingroup *g;
+	unsigned long flags;
+	u32 val;
+
+	if (!irqd_is_level_type(irqd)) {
+		g = &pctrl->soc->groups[irqd->hwirq];
+		raw_spin_lock_irqsave(&pctrl->lock, flags);
+		val = BIT(g->intr_status_bit);
+		writel(val, pctrl->regs + g->intr_status_reg);
+		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int msm_gpio_pdc_pin_request(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	struct platform_device *pdev = to_platform_device(pctrl->dev);
+	const char *pin_name;
+	int irq;
+	int ret;
+
+	pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);
+	if (!pin_name)
+		return -ENOMEM;
+

drivers/pinctrl/qcom/pinctrl-msm.c +47 -1

+		kfree(pin_name);
+		return ret;
+	}
+
+	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
+	disable_irq(irq);
+
+	return 0;
+}
+
+static int msm_gpio_pdc_pin_release(struct irq_data *d)
+{
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
+	const void *name;
+
+	if (pdc_irqd) {
+		irq_set_handler_data(d->irq, NULL);
+		name = free_irq(pdc_irqd->irq, d);
+		kfree(name);
+	}
+
+	return 0;
+}
+
+static int msm_gpio_irq_reqres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
+		dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
+			irqd_to_hwirq(d));
+		return -EINVAL;
+	}
+
+	return msm_gpio_pdc_pin_request(d);
+}
+
+static void msm_gpio_irq_relres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	msm_gpio_pdc_pin_release(d);
+	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
+}
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;

@@ -887,6 +983,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
 	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
 	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
+	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
+	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
--

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

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

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

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

Signed-off-by Lina Iyer <[email protected]>

---
Changes in v3:
- free action->name
Changes in v2:
- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
Changes in v1:
- Trigger GPIO in h/w from PDC IRQ handler
- Avoid big tables for GPIO-PDC map, pick from DT instead
- Use handler_data

---
 drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

drivers/pinctrl/qcom/pinctrl-msm.c +12 -0

index 0e22f52b2a19..6527a0a9edd1 100644
@@ -687,11 +687,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 	const struct msm_pingroup *g;
 	unsigned long flags;
 	u32 val;
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
 
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	if (pdc_irqd)
+		irq_set_irq_type(pdc_irqd->irq, type);
+
 	/*
 	 * For hw without possibility of detecting both edges
 	 */

@@ -779,9 +783,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
 	unsigned long flags;
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	if (pdc_irqd)
+		irq_set_irq_wake(pdc_irqd->irq, on);
+
 	irq_set_irq_wake(pctrl->irq, on);
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);

@@ -863,6 +871,94 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
 }
 
+static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
+{
+	struct irq_data *irqd = data;
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);

drivers/pinctrl/qcom/pinctrl-msm.c +17 -0

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

drivers/pinctrl/qcom/pinctrl-msm.c +41 -0

+
+	return 0;
+}
+
+static int msm_gpio_pdc_pin_release(struct irq_data *d)
+{
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
+	const void *name;
+
+	if (pdc_irqd) {
+		irq_set_handler_data(d->irq, NULL);
+		name = free_irq(pdc_irqd->irq, d);
+		kfree(name);
+	}
+
+	return 0;
+}
+
+static int msm_gpio_irq_reqres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
+		dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
+			irqd_to_hwirq(d));
+		return -EINVAL;
+	}
+
+	return msm_gpio_pdc_pin_request(d);
+}
+
+static void msm_gpio_irq_relres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	msm_gpio_pdc_pin_release(d);
+	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
+}
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;

@@ -887,6 +983,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
 	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
 	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
+	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
+	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {

Signature · 2 lines

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

Hi Lina,

On Tue, 04 Sep 2018 22:18:06 +0100,
Lina Iyer <[email protected]> wrote:

Quoted 4 lines

drivers/pinctrl/qcom/pinctrl-msm.c +4 -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);

drivers/pinctrl/qcom/pinctrl-msm.c static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) · line 887

Quoted 10 lines

drivers/pinctrl/qcom/pinctrl-msm.c +10 -0

+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	const struct msm_pingroup *g;
+	unsigned long flags;
+	u32 val;
+
+	if (!irqd_is_level_type(irqd)) {
+		g = &pctrl->soc->groups[irqd->hwirq];
+		raw_spin_lock_irqsave(&pctrl->lock, flags);
+		val = BIT(g->intr_status_bit);
+		writel(val, pctrl->regs + g->intr_status_reg);

write_relaxed, please.

Review

Quoted 2 lines

(quoted diff) +2 -0

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

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

Quoted 21 lines

drivers/pinctrl/qcom/pinctrl-msm.c +21 -0

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

drivers/pinctrl/qcom/pinctrl-msm.c static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) · line 916

Quoted 6 lines

drivers/pinctrl/qcom/pinctrl-msm.c +6 -0

+	}
+
+	ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d),
+			  pin_name, d);
+	if (ret) {
+		pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq);

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

drivers/pinctrl/qcom/pinctrl-msm.c static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) · line 922

Quoted 6 lines

drivers/pinctrl/qcom/pinctrl-msm.c +6 -0

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

Who enables this interrupt?

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

A slightly better way of doing that would be:

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

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

Quoted 52 lines

drivers/pinctrl/qcom/pinctrl-msm.c +41 -1

+
+	return 0;
+}
+
+static int msm_gpio_pdc_pin_release(struct irq_data *d)
+{
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
+	const void *name;
+
+	if (pdc_irqd) {
+		irq_set_handler_data(d->irq, NULL);
+		name = free_irq(pdc_irqd->irq, d);
+		kfree(name);
+	}
+
+	return 0;
+}
+
+static int msm_gpio_irq_reqres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
+		dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
+			irqd_to_hwirq(d));
+		return -EINVAL;
+	}
+
+	return msm_gpio_pdc_pin_request(d);
+}
+
+static void msm_gpio_irq_relres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	msm_gpio_pdc_pin_release(d);
+	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
+}
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;

@@ -887,6 +983,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
 	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
 	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
+	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
+	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
--

Quoted 3 lines

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

Thanks,

	M.

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

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

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

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

Signed-off-by Lina Iyer <[email protected]>

---
Changes in v3:
- free action->name
Changes in v2:
- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
Changes in v1:
- Trigger GPIO in h/w from PDC IRQ handler
- Avoid big tables for GPIO-PDC map, pick from DT instead
- Use handler_data
---
drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++

1 file changed, 98 insertions(+)

drivers/pinctrl/qcom/pinctrl-msm.c +0 -0

index 0e22f52b2a19..6527a0a9edd1 100644
@@ -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;

drivers/pinctrl/qcom/pinctrl-msm.c +14 -0

 	/*
 	 * For hw without possibility of detecting both edges
 	 */

@@ -779,9 +783,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
 	unsigned long flags;
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	if (pdc_irqd)
+		irq_set_irq_wake(pdc_irqd->irq, on);
+
 	irq_set_irq_wake(pctrl->irq, on);
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);

@@ -863,6 +871,94 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
 }
 
+static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
+{
+	struct irq_data *irqd = data;
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	const struct msm_pingroup *g;
+	unsigned long flags;
+	u32 val;
+
+	if (!irqd_is_level_type(irqd)) {

drivers/pinctrl/qcom/pinctrl-msm.c +70 -1

+static int msm_gpio_pdc_pin_request(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	struct platform_device *pdev = to_platform_device(pctrl->dev);
+	const char *pin_name;
+	int irq;
+	int ret;
+
+	pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);
+	if (!pin_name)
+		return -ENOMEM;
+
+	irq = platform_get_irq_byname(pdev, pin_name);
+	if (irq < 0) {
+		kfree(pin_name);
+		return 0;
+	}
+
+	ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d),
+			  pin_name, d);
+	if (ret) {
+		pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq);
+		kfree(pin_name);
+		return ret;
+	}
+
+	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
+	disable_irq(irq);
+
+	return 0;
+}
+
+static int msm_gpio_pdc_pin_release(struct irq_data *d)
+{
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
+	const void *name;
+
+	if (pdc_irqd) {
+		irq_set_handler_data(d->irq, NULL);
+		name = free_irq(pdc_irqd->irq, d);
+		kfree(name);
+	}
+
+	return 0;
+}
+
+static int msm_gpio_irq_reqres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
+		dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
+			irqd_to_hwirq(d));
+		return -EINVAL;
+	}
+
+	return msm_gpio_pdc_pin_request(d);
+}
+
+static void msm_gpio_irq_relres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	msm_gpio_pdc_pin_release(d);
+	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
+}
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;

@@ -887,6 +983,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
 	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
 	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
+	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
+	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
--

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

Marc,

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

Quoted 37 lines · Lina Iyer

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

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

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

Signed-off-by Lina Iyer <[email protected]>

---
Changes in v3:
- free action->name
Changes in v2:
- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
Changes in v1:
- Trigger GPIO in h/w from PDC IRQ handler
- Avoid big tables for GPIO-PDC map, pick from DT instead
- Use handler_data
---
drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++

1 file changed, 98 insertions(+)

drivers/pinctrl/qcom/pinctrl-msm.c +0 -0

index 0e22f52b2a19..6527a0a9edd1 100644
@@ -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;

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) · line 698

Quoted 10 lines

drivers/pinctrl/qcom/pinctrl-msm.c +4 -0

 	u32 val;
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
 
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	if (pdc_irqd)
+		irq_set_irq_type(pdc_irqd->irq, type);
+

I skip over the TLMM configuration for the GPIO interrupt and just set the IRQ handler for the GPIO interrupt here..

Quoted 31 lines

drivers/pinctrl/qcom/pinctrl-msm.c +14 -0

 	/*
 	 * For hw without possibility of detecting both edges
 	 */

@@ -779,9 +783,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
 	unsigned long flags;
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	if (pdc_irqd)
+		irq_set_irq_wake(pdc_irqd->irq, on);
+
 	irq_set_irq_wake(pctrl->irq, on);
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);

@@ -863,6 +871,94 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
 }
 
+static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
+{
+	struct irq_data *irqd = data;
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	const struct msm_pingroup *g;
+	unsigned long flags;
+	u32 val;
+
+	if (!irqd_is_level_type(irqd)) {

drivers/pinctrl/qcom/pinctrl-msm.c static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) · line 893

Quoted 10 lines

drivers/pinctrl/qcom/pinctrl-msm.c +10 -0

+		g = &pctrl->soc->groups[irqd->hwirq];
+		raw_spin_lock_irqsave(&pctrl->lock, flags);
+		val = BIT(g->intr_status_bit);
+		writel(val, pctrl->regs + g->intr_status_reg);
+		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	}
+
+	return IRQ_HANDLED;
+}
+

Then ...

static irqreturn_t wake_irq_gpio_handler(int irq, void *data)

{
     struct irq_data *irqd = data;
     struct irq_desc *desc = irq_to_desc(irqd->irq);

     desc->handle_irq(desc);

     return IRQ_HANDLED;
}

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

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

Thoughts?

Thanks,
Lina

Quoted 84 lines

drivers/pinctrl/qcom/pinctrl-msm.c +70 -1

+static int msm_gpio_pdc_pin_request(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	struct platform_device *pdev = to_platform_device(pctrl->dev);
+	const char *pin_name;
+	int irq;
+	int ret;
+
+	pin_name = kasprintf(GFP_KERNEL, "gpio%lu", d->hwirq);
+	if (!pin_name)
+		return -ENOMEM;
+
+	irq = platform_get_irq_byname(pdev, pin_name);
+	if (irq < 0) {
+		kfree(pin_name);
+		return 0;
+	}
+
+	ret = request_irq(irq, wake_irq_gpio_handler, irqd_get_trigger_type(d),
+			  pin_name, d);
+	if (ret) {
+		pr_warn("GPIO-%lu could not be set up as wakeup", d->hwirq);
+		kfree(pin_name);
+		return ret;
+	}
+
+	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
+	disable_irq(irq);
+
+	return 0;
+}
+
+static int msm_gpio_pdc_pin_release(struct irq_data *d)
+{
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
+	const void *name;
+
+	if (pdc_irqd) {
+		irq_set_handler_data(d->irq, NULL);
+		name = free_irq(pdc_irqd->irq, d);
+		kfree(name);
+	}
+
+	return 0;
+}
+
+static int msm_gpio_irq_reqres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	if (gpiochip_lock_as_irq(gc, irqd_to_hwirq(d))) {
+		dev_err(gc->parent, "unable to lock HW IRQ %lu for IRQ\n",
+			irqd_to_hwirq(d));
+		return -EINVAL;
+	}
+
+	return msm_gpio_pdc_pin_request(d);
+}
+
+static void msm_gpio_irq_relres(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+	msm_gpio_pdc_pin_release(d);
+	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
+}
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;

@@ -887,6 +983,8 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	pctrl->irq_chip.irq_ack = msm_gpio_irq_ack;
 	pctrl->irq_chip.irq_set_type = msm_gpio_irq_set_type;
 	pctrl->irq_chip.irq_set_wake = msm_gpio_irq_set_wake;
+	pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
+	pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
 
 	ret = gpiochip_add_data(&pctrl->chip, pctrl);
 	if (ret) {
--

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

Quoted 116 lines · Lina Iyer

Marc,

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

Quoted 37 lines · Lina Iyer

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

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

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

Signed-off-by Lina Iyer <[email protected]>

---
Changes in v3:
- free action->name
Changes in v2:
- Remove IRQF_NO_SUSPEND and IRQF_ONE_SHOT from PDC IRQ
Changes in v1:
- Trigger GPIO in h/w from PDC IRQ handler
- Avoid big tables for GPIO-PDC map, pick from DT instead
- Use handler_data
---
drivers/pinctrl/qcom/pinctrl-msm.c | 98 ++++++++++++++++++++++++++++++

1 file changed, 98 insertions(+)

drivers/pinctrl/qcom/pinctrl-msm.c +0 -0

index 0e22f52b2a19..6527a0a9edd1 100644
@@ -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;

drivers/pinctrl/qcom/pinctrl-msm.c static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) · line 698

Quoted 10 lines

drivers/pinctrl/qcom/pinctrl-msm.c +4 -0

 	u32 val;
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
 
 	g = &pctrl->soc->groups[d->hwirq];
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	if (pdc_irqd)
+		irq_set_irq_type(pdc_irqd->irq, type);
+

I skip over the TLMM configuration for the GPIO interrupt and just set the IRQ handler for the GPIO interrupt here..

Quoted 31 lines

drivers/pinctrl/qcom/pinctrl-msm.c +14 -0

 	/*
 	 * For hw without possibility of detecting both edges
 	 */

@@ -779,9 +783,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
 	unsigned long flags;
+	struct irq_data *pdc_irqd = irq_get_handler_data(d->irq);
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	if (pdc_irqd)
+		irq_set_irq_wake(pdc_irqd->irq, on);
+
 	irq_set_irq_wake(pctrl->irq, on);
 
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);

@@ -863,6 +871,94 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
 	return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
 }
 
+static irqreturn_t wake_irq_gpio_handler(int irq, void *data)
+{
+	struct irq_data *irqd = data;
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
+	struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+	const struct msm_pingroup *g;
+	unsigned long flags;
+	u32 val;
+
+	if (!irqd_is_level_type(irqd)) {

drivers/pinctrl/qcom/pinctrl-msm.c static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) · line 893

Quoted 10 lines

drivers/pinctrl/qcom/pinctrl-msm.c +10 -0

+		g = &pctrl->soc->groups[irqd->hwirq];
+		raw_spin_lock_irqsave(&pctrl->lock, flags);
+		val = BIT(g->intr_status_bit);
+		writel(val, pctrl->regs + g->intr_status_reg);
+		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+	}
+
+	return IRQ_HANDLED;
+}
+

Then ...

static irqreturn_t wake_irq_gpio_handler(int irq, void *data)

{
      struct irq_data *irqd = data;
      struct irq_desc *desc = irq_to_desc(irqd->irq);

      desc->handle_irq(desc);

      return IRQ_HANDLED;
}

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

Quoted 7 lines

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

Thoughts?

Would it help if I submit a RFC patch based on this idea?

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 [email protected]

Signed-off-by Lina Iyer <[email protected]>

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

Quoted 9 lines · 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 [email protected]

Signed-off-by Lina Iyer <[email protected]>

Quoted 7 lines

---
Changes in v2:
- Fix PDC IRQ number in example
- Describe IRQ trigger type in example

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

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

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 <[email protected]>

---
Changes in v3:
- Enable PDC-IRQ swap only for edge interrupts
Changes in v2:
- Fix PDC IRQ max port, 126 is the max supported in h/w
- Use PDC hwirq in bitmap, linux numbers could be large
- Setup DISABLE_UNLAZY for both TLMM and PDC IRQs

---
 drivers/pinctrl/qcom/pinctrl-msm.c | 73 +++++++++++++++++++++++++++++-
 drivers/pinctrl/qcom/pinctrl-msm.h |  3 ++
 2 files changed, 75 insertions(+), 1 deletion(-)

drivers/pinctrl/qcom/pinctrl-msm.c +27 -1

index 6527a0a9edd1..01a455f86fcd 100644
@@ -37,6 +37,7 @@
 #include "../pinctrl-utils.h"
 
 #define MAX_NR_GPIO 300
+#define MAX_PDC_HWIRQ 126
 #define PS_HOLD_OFFSET 0x820
 
 /**

@@ -51,6 +52,7 @@
  * @enabled_irqs:   Bitmap of currently enabled irqs.
  * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
  *                  detection.
+ * @pdc_hwirqs:     Bitmap of wakeup capable irqs.
  * @soc;            Reference to soc_data of platform specific data.
  * @regs:           Base address for the TLMM register map.
  */

@@ -68,11 +70,15 @@ struct msm_pinctrl {
 
 	DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
 	DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
+	DECLARE_BITMAP(pdc_hwirqs, MAX_PDC_HWIRQ);
 
 	const struct msm_pinctrl_soc_data *soc;
 	void __iomem *regs;
+	struct irq_domain *pdc_irq_domain;
 };
 
+static bool in_suspend;
+
 static int msm_get_groups_count(struct pinctrl_dev *pctldev)
 {
 	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);

@@ -787,8 +793,13 @@ static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
-	if (pdc_irqd)
+	if (pdc_irqd && !in_suspend) {
 		irq_set_irq_wake(pdc_irqd->irq, on);
+		if (on)
+			set_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs);
+		else
+			clear_bit(pdc_irqd->hwirq, pctrl->pdc_hwirqs);
+	}
 
 	irq_set_irq_wake(pctrl->irq, on);
 

@@ -919,7 +930,12 @@ static int msm_gpio_pdc_pin_request(struct irq_data *d)
 	}
 
 	irq_set_handler_data(d->irq, irq_get_irq_data(irq));
+	irq_set_handler_data(irq, d);
+	irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY);
+	irq_set_status_flags(d->irq, IRQ_DISABLE_UNLAZY);
 	disable_irq(irq);
+	if (!pctrl->pdc_irq_domain)
+		pctrl->pdc_irq_domain = irq_get_irq_data(irq)->domain;
 
 	return 0;
 }

@@ -1071,6 +1087,61 @@ static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl)
 		}
 }
 
+int __maybe_unused msm_pinctrl_suspend_late(struct device *dev)
+{
+	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
+	struct irq_data *irqd;
+	unsigned int irq;
+	int i;
+
+	in_suspend = true;
+	for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
+		irq = irq_find_mapping(pctrl->pdc_irq_domain, i);

drivers/pinctrl/qcom/pinctrl-msm.c +17 -0

+
+	return 0;
+}
+
+int __maybe_unused msm_pinctrl_resume_late(struct device *dev)
+{
+	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
+	struct irq_data *irqd, *pdc_irqd;
+	unsigned int irq;
+	int i;
+
+	for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
+		irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
+		irqd = irq_get_handler_data(irq);
+		pdc_irqd = irq_get_irq_data(irq);
+		/*
+		 * The TLMM will be operational now, so disable

drivers/pinctrl/qcom/pinctrl-msm.c +4 -0

+
+	return 0;
+}
+
 int msm_pinctrl_probe(struct platform_device *pdev,
 		      const struct msm_pinctrl_soc_data *soc_data)
 {

drivers/pinctrl/qcom/pinctrl-msm.h +3 -0

index 9b9feea540ff..21b56fb5dae9 100644
@@ -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

Signature · 2 lines

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

Hi Lina,

Quoted 20 lines · 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 <[email protected]>

---
Changes in v3:
- Enable PDC-IRQ swap only for edge interrupts
Changes in v2:
- Fix PDC IRQ max port, 126 is the max supported in h/w
- Use PDC hwirq in bitmap, linux numbers could be large
- Setup DISABLE_UNLAZY for both TLMM and PDC IRQs
---

Quoted 17 lines

drivers/pinctrl/qcom/pinctrl-msm.c +17 -0

+int __maybe_unused msm_pinctrl_suspend_late(struct device *dev)
+{
+	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
+	struct irq_data *irqd;
+	unsigned int irq;
+	int i;
+
+	in_suspend = true;
+	for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
+		irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
+		irqd = irq_get_handler_data(irq);
+		/*
+		 * We don't know if the TLMM will be functional
+		 * or not, during the suspend. If its functional,
+		 * we do not want duplicate interrupts from PDC.
+		 * Hence disable the GPIO IRQ and enable PDC IRQ
+		 * for edge interrupt only.

drivers/pinctrl/qcom/pinctrl-msm.c static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) · line 1109

Quoted 3 lines

drivers/pinctrl/qcom/pinctrl-msm.c +3 -0

+		 */
+		if (irqd_is_wakeup_set(irqd) && !irqd_is_level_type(irqd)) {
+			disable_irq_wake(irqd->irq);

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

drivers/pinctrl/qcom/pinctrl-msm.c static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) · line 1113

Quoted 4 lines

drivers/pinctrl/qcom/pinctrl-msm.c +4 -0

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

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

Quoted 21 lines

drivers/pinctrl/qcom/pinctrl-msm.c +21 -0

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

drivers/pinctrl/qcom/pinctrl-msm.c static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) · line 1140

Quoted 6 lines

drivers/pinctrl/qcom/pinctrl-msm.c +6 -0

+			disable_irq_nosync(irq);
+			enable_irq_wake(irqd->irq);
+			enable_irq(irqd->irq);
+		}
+	}
+	in_suspend = false;

Same remark about the lock.

Quoted 24 lines · Marc Zyngier

Hi Lina,

Quoted 20 lines · 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 <[email protected]>

---
Changes in v3:
- Enable PDC-IRQ swap only for edge interrupts
Changes in v2:
- Fix PDC IRQ max port, 126 is the max supported in h/w
- Use PDC hwirq in bitmap, linux numbers could be large
- Setup DISABLE_UNLAZY for both TLMM and PDC IRQs
---

[...]

Quoted 17 lines

Quoted 17 lines

drivers/pinctrl/qcom/pinctrl-msm.c +17 -0

+int __maybe_unused msm_pinctrl_suspend_late(struct device *dev)
+{
+	struct msm_pinctrl *pctrl = dev_get_drvdata(dev);
+	struct irq_data *irqd;
+	unsigned int irq;
+	int i;
+
+	in_suspend = true;
+	for_each_set_bit(i, pctrl->pdc_hwirqs, MAX_PDC_HWIRQ) {
+		irq = irq_find_mapping(pctrl->pdc_irq_domain, i);
+		irqd = irq_get_handler_data(irq);
+		/*
+		 * We don't know if the TLMM will be functional
+		 * or not, during the suspend. If its functional,
+		 * we do not want duplicate interrupts from PDC.
+		 * Hence disable the GPIO IRQ and enable PDC IRQ
+		 * for edge interrupt only.

Quoted 9 lines

drivers/pinctrl/qcom/pinctrl-msm.c static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) · line 1109

Quoted 3 lines

drivers/pinctrl/qcom/pinctrl-msm.c +3 -0

+		 */
+		if (irqd_is_wakeup_set(irqd) && !irqd_is_level_type(irqd)) {
+			disable_irq_wake(irqd->irq);

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

True, it need not be. I could just avoid setting the wakeup on the TLMM and just use the PDC interrupt as wakeup.

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

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

	GPIO-IRQ --> PDC --> GIC

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

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

Quoted 10 lines

drivers/pinctrl/qcom/pinctrl-msm.c static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) · line 1113

Quoted 6 lines

drivers/pinctrl/qcom/pinctrl-msm.c +4 -0

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

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

Since we are the the only CPU running and suspend/resume (and even idle) would be serialized I didn't see a reason for needing a lock.

Quoted 18 lines · Lina Iyer

Quoted 4 lines · Marc Zyngier

Hi Lina,

On Tue, 04 Sep 2018 22:18:08 +0100,
Lina Iyer <[email protected]> wrote:

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

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

Quoted 7 lines

	GPIO-IRQ --> PDC --> GIC

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

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

[me tries to page it all in again]

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

	GPIO -> PDC -> TLMM -> GIC

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

The lack of interrupt
replaying on the PDC is quite annoying (I have much stronger words in mind though), and I'm not sure we can easily fix that one without this parallel interrupt hack (you need something to inject edge interrupts in the TLMM).

Quoted 10 lines

Quoted 10 lines

drivers/pinctrl/qcom/pinctrl-msm.c static void msm_pinctrl_setup_pm_reset(struct msm_pinctrl *pctrl) · line 1113

Quoted 6 lines

drivers/pinctrl/qcom/pinctrl-msm.c +4 -0

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

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

Quoted 2 lines

Since we are the the only CPU running and suspend/resume (and even idle) would be serialized I didn't see a reason for needing a lock.

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