Message-ID: <158456608487.152100.9336929115021414535@swboyd.mtv.corp.google.com>
Date:   Wed, 18 Mar 2020 14:14:44 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Maulik Shah <mkshah@...eaurora.org>, bjorn.andersson@...aro.org,
        evgreen@...omium.org, mka@...omium.org
Cc:     linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        agross@...nel.org, linus.walleij@...aro.org, tglx@...utronix.de,
        maz@...nel.org, jason@...edaemon.net, dianders@...omium.org,
        rnayak@...eaurora.org, ilina@...eaurora.org, lsrao@...eaurora.org
Subject: Re: [RFC v2] irqchip: qcom: pdc: Introduce irq_set_wake call

Quoting Maulik Shah (2020-03-16 23:47:21)
> Hi,
> 
> On 3/17/2020 7:34 AM, Stephen Boyd wrote:
> > Quoting Maulik Shah (2020-03-12 06:22:59)
> >> Change the way interrupts get enabled at wakeup capable PDC irq chip.
> >>
> >> Introduce irq_set_wake call which lets interrupts enabled at PDC with
> >> enable_irq_wake and disabled with disable_irq_wake with certain
> >> conditions.
> >>
> >> Interrupt will get enabled in HW at PDC and its parent GIC if they are
> >> either enabled is SW or marked as wake up capable.
> > Shouldn't we only enable in PDC and GIC if it's marked wakeup capable
> > and we're entering suspend? Otherwise we should let the hardware enable
> > state follow the software irq enable state?
> Not only during "sleep" but PDC (and GIC) have a role during "active" time as well.
> so we can not just enabled at PDC and GIC when entering to suspend, interrupt need
> to keep interrupt enabled at PDC and GIC HW when out of suspend as well.

Yes, but if an interrupt is only marked for wakeup and not actually
enabled we shouldn't deliver it to the GIC. That's what I'm asking
about.

> >
> >> interrupt will get disabled in HW at PDC and its parent GIC only if its
> >> disabled in SW and also marked as non-wake up capable.
> >>
> >> Signed-off-by: Maulik Shah <mkshah@...eaurora.org>
> >> ---
> >>  drivers/irqchip/qcom-pdc.c | 124 ++++++++++++++++++++++++++++++++++++++++++---
> >>  1 file changed, 117 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> >> index 6ae9e1f..d698cec 100644
> >> --- a/drivers/irqchip/qcom-pdc.c
> >> +++ b/drivers/irqchip/qcom-pdc.c
> >> @@ -1,6 +1,6 @@
[...]
> >
> >> +
> >>         if (d->hwirq == GPIO_NO_WAKE_IRQ)
> >>                 return;
> >>  
> >> -       pdc_enable_intr(d, false);
> >> -       irq_chip_disable_parent(d);
> >> +       raw_spin_lock(&pdc_lock);
> >> +
> >> +       clear_bit(d->hwirq, pdc_enabled_irqs);
> > clear_bit() is atomic, so why inside the lock?
> I will move it out of lock.
> >
> >> +       wake_status = test_bit(d->hwirq, pdc_wake_irqs);
> >> +
> >> +       /* Disable at PDC HW if wake_status also says same */
> >> +       if (!wake_status)
> > Should read as "if not wakeup_enabled".
> I will update comment.

Hopefully the comment isn't useful and can just be removed if the code
reads properly.

> >
> >> +               pdc_enable_intr(d, false);
> >> +
> >> +       raw_spin_unlock(&pdc_lock);
> >> +
> >> +       /* Disable at GIC HW if wake_status also says same */
> >> +       if (!wake_status)
> > This happens outside the lock, so I'm confused why any locking is needed
> > in this function.
> Okay, since test_bit() is also atomic so i will keep locking inside pc_enable_intr() as it is.
> >
> >> +               irq_chip_disable_parent(d);
> >>  }
> >>  
> >>  static void qcom_pdc_gic_enable(struct irq_data *d)
> >> @@ -101,7 +116,16 @@ static void qcom_pdc_gic_enable(struct irq_data *d)
> >>         if (d->hwirq == GPIO_NO_WAKE_IRQ)
> >>                 return;
> >>  
> >> +       raw_spin_lock(&pdc_lock);
> >> +
> >> +       set_bit(d->hwirq, pdc_enabled_irqs);
> >> +
> >> +       /* We can blindly enable at PDC HW as we are already in enable path */
> >>         pdc_enable_intr(d, true);
> >> +
> >> +       raw_spin_unlock(&pdc_lock);
> >> +
> >> +       /* We can blindly enable at GIC HW as we are already in enable path */
> >>         irq_chip_enable_parent(d);
> >>  }
> >>  
[...]
> >> + */
> >> +
> >> +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on)
> >> +{
> >> +       bool enabled_status;
> >> +
> >> +       if (d->hwirq == GPIO_NO_WAKE_IRQ)
> >> +               return 0;
> >> +
> >> +       raw_spin_lock(&pdc_lock);
> >> +       enabled_status = test_bit(d->hwirq, pdc_enabled_irqs);
> >> +       if (on) {
> >> +               set_bit(d->hwirq, pdc_wake_irqs);
> >> +               pdc_enable_intr(d, true);
> >> +       } else {
> >> +               clear_bit(d->hwirq, pdc_wake_irqs);
> >> +               pdc_enable_intr(d, enabled_status);
> >> +       }
> >> +
> >> +       raw_spin_unlock(&pdc_lock);
> >> +
> >> +       /* Either "wake" or "enabled" need same status at parent as well */
> >> +       if (on || enabled_status)
> >> +               irq_chip_enable_parent(d);
> >> +       else
> >> +               irq_chip_disable_parent(d);
> > What happens if irq is "disabled" in software, because this is the first
> > function called on the irq, and we aren't in suspend yet. Then we get
> > the irq. Won't we be interrupting the CPU because we've enabled in PDC
> > and GIC hardware? Why doesn't this function update the wake bit and then
> > leave the force on irq logic to suspend entry? Will we miss an interrupt
> > while entering suspend because of that?
> As PDC (and GIC) have a role during "active" time as well, interrupt should be
> enabled in PDC and GIC HW.

Sure. When the irq is enabled we want to enable at the GIC, but if it
isn't enabled and we're not in suspend I would think we don't want the
irq enabled at the GIC. But this code is doing that. Why? I'd think we
would want to make enable in the PDC driver enable the parent and then
make the set_wake path just update some bitmap tracking wakeup enabled
irqs.

Then when we enter suspend we will enable any pdc interrupts only in the
PDC so that we can wakeup from suspend if that interrupt comes in. When
we wakeup we'll resend the edge interrupts to the GIC on the resume path
and level interrupts will "just work" because they'll stay asserted
throughout resume.

The bigger problem would look to be suspend entry, but in that case we
leave any interrupts enabled at the GIC on the path to suspend (see
suspend_device_irq() and how it bails out early if it's marked for
wakeup) so we should be able to have some suspend entry hook in pdc that
enables the irq in the PDC if it's in the wakeup bitmap. Then on the
path to suspend the GIC can lose power at any point after we enable the
wakeup path in PDC and then the system should resume and get the
interrupt through the resend mechanism.

Powered by blists - more mailing lists