Re: [PATCH RESEND v1 2/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend

From: Lina Iyer
Date: Mon Aug 20 2018 - 11:26:36 EST

On Sat, Aug 18 2018 at 07:13 -0600, Marc Zyngier wrote:
Hi Lina,

On Fri, 17 Aug 2018 20:10:23 +0100,
Lina Iyer <ilina@xxxxxxxxxxxxxx> wrote:

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

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

Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
---
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

Where is this value coming from? Is it guaranteed to be an
architectural maximum? Or something that is likely to vary in future
implementations?

#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);

I think we'll all survive the two extra lines if you write this as an
'if' statement (unless you're competing for the next IOCCC, and then
you need to up your game a bit).

Also, are you indexing the bitmap using a Linux irq number? If so,
that's an absolute NACK. Out of the box, a Linux IRQ can be up to
NR_IRQS+8196 on arm64, and there are plans to push that to be a much
larger space.

I didn't realize this. I have been using linux IRQ number on this
bitmask and I will need to fix this.

+ }

irq_set_irq_wake(pctrl->irq, on);

I'm a bit worried by the way you call into the irq subsystem with this
spinlock held. Have you run that code with lockdep enabled?

I have not tried lockdep. Will try it.
This specific line is already part of the driver. I added a similar line
irq_set_irq_wake(pdc_irqd->irq) just above following the same pattern.


@@ -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);

Could you explain what this is trying to do? I'm trying to understand
this code, but this function isn't in 4.18...

Oh, I have been able to test only on 4.14 so far. The flag does seem to
exist at least, I didn't get a compiler error.

I read this in kernel/irq/chip.c -

If the interrupt chip does not implement the irq_disable callback,
a driver can disable the lazy approach for a particular irq line by
calling 'irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY)'. This can
be used for devices which cannot disable the interrupt at the
device level under certain circumstances and have to use
disable_irq[_nosync] instead.

And interpreted this as something that this would prevent 'relaxed'
disable. I am enabling and disabling the IRQ in suspend path, that I
thought this would help avoid issues caused by late disable. Am I
mistaken?

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

So this is what I though. You're using the Linux IRQ, and not the pin
number (or whatever HW-dependent index that would otherwise make
sense). Please fix it.

Noted.

+ /*
+ * 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

I can't really review this code any further, as it seems that I'm
missing some crucial dependencies. But there is a number of things
that feel quite wrong in this code, and that need to be addressed
anyway.

Thanks for reviewing Marc.

-- Lina