Messages in this thread | | | Date | Mon, 20 Aug 2018 09:26:29 -0600 | From | Lina Iyer <> | Subject | Re: [PATCH RESEND v1 2/5] drivers: pinctrl: msm: enable PDC interrupt only during suspend |
| |
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@codeaurora.org> wrote: >> >> During suspend the system may power down some of the system rails. As a >> result, the TLMM hw block may not be operational anymore and wakeup >> capable GPIOs will not be detected. The PDC however will be operational >> and the GPIOs that are routed to the PDC as IRQs can wake the system up. >> >> To avoid being interrupted twice (for TLMM and once for PDC IRQ) when a >> GPIO trips, use TLMM for active and switch to PDC for suspend. When >> entering suspend, disable the TLMM wakeup interrupt and instead enable >> the PDC IRQ and revert upon resume. >> >> Signed-off-by: Lina Iyer <ilina@codeaurora.org> >> --- >> 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
| |