Message-ID: <158755213744.163502.17257131401798918469@swboyd.mtv.corp.google.com>
Date:   Wed, 22 Apr 2020 03:42:17 -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 v3] irqchip: qcom: pdc: Introduce irq_set_wake call

Quoting Maulik Shah (2020-04-20 23:35:09)
> Hi,
> 
> On 4/15/2020 1:36 PM, Stephen Boyd wrote:
> > Quoting Maulik Shah (2020-04-14 01:05:36)
> >> Hi,
> >>
> >> On 4/14/2020 6:05 AM, Stephen Boyd wrote:
> >>> Quoting Maulik Shah (2020-03-30 09:41:00)
> >>>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> >>>> index 6ae9e1f..c43715b 100644
> >>>> --- a/drivers/irqchip/qcom-pdc.c
> >>>> +++ b/drivers/irqchip/qcom-pdc.c
> >>>> +                        * the pending interrupt gets cleared at GIC before
> >>>> +                        * enabling it from msm_gpio_irq_enable(). So CPU will
> >>>> +                        * never see pending IRQ after resume if we disable them
> >>>> +                        * here.
> >>> Is there something that's doing this in the gpio driver? It sounds
> >>> like the bug lies in that driver. Maybe the gpio driver should use
> >>> irq_startup instead of irq_enable to clear anything pending? The comment
> >>> in msm_gpio_irq_enable() talks a lot but doesn't actually say why it's a
> >>> problem to be latched at the GIC as pending when the irq is enabled the
> >>> first time.
> >> This is to clear any erroneous interrupts that would have got latched
> >> when the interrupt is not in use.
> >>
> >> There may be devices like UART which can use the same gpio line for data
> >> rx as well as a wakeup gpio
> >>
> >> The data that was flowing on the line may latch the interrupt and when
> >> we enable the interrupt,we see IRQ pending and unexpected IRQ gets
> >> triggered.
> > Isn't the interrupt supposed to latch in the hardware in this scenario?
> > We wanted to wakeup from UART RX over GPIO, and we did, and we also
> > wanted to send that data through the pin to the UART core, so I suspect
> > we muxed it as a UART pin and also watched it for an irq wakeup in the
> > GPIO driver and PDC? The wakeup irq handler can be ignored by the UART
> > driver if it wants.
> I will check this if i can operate only on PDC and GIC IRQs and can 
> ignore PDC-GPIO domain IRQ.
> Working on it.

Ok.

> >
> >>>> +                               continue;
> >>>> +                       }
> >>>> +
> >>>> +                       irq_chip_disable_parent(d);
> >>>> +               }
> >>>> +       }
> >>>> +       p->from_pdc_suspend = false;
> >>>> +}
> >>>> +
> >>>> +static int pdc_cpu_pm_callback(struct notifier_block *nfb,
> >>>> +                              unsigned long action, void *v)
> >>>> +{
> >>>> +       struct pdc_pm_data *p = container_of(nfb, struct pdc_pm_data,
> >>>> +                                            pdc_cpu_pm_nfb);
> >>>> +       unsigned long flags;
> >>>> +
> >>>> +       if (!p->suspend_start)
> >>>> +               return NOTIFY_OK;
> >>>> +
> >>>> +       spin_lock_irqsave(&p->pm_lock, flags);
> >>>> +       switch (action) {
> >>>> +       case CPU_PM_ENTER:
> >>>> +               cpumask_set_cpu(raw_smp_processor_id(), &p->cpus_in_pc);
> >>>> +               if (cpumask_equal(&p->cpus_in_pc, cpu_online_mask))
> >>>> +                       pdc_suspend(p);
> >>>> +               break;
> >>>> +       case CPU_PM_ENTER_FAILED:
> >>>> +       case CPU_PM_EXIT:
> >>>> +               if (cpumask_equal(&p->cpus_in_pc, cpu_online_mask))
> >>>> +                       pdc_resume(p);
> >>>> +               cpumask_clear_cpu(raw_smp_processor_id(), &p->cpus_in_pc);
> >>>> +               break;
> >>>> +       }
> >>>> +       spin_unlock_irqrestore(&p->pm_lock, flags);
> >>> What is the point of this callback? Any irqs that we want to wakeup the
> >>> CPUs from deep idle should be enabled via enable_irq(). Otherwise, they
> >>> shouldn't wake up the system. That's the difference between idle and
> >>> suspend.
> >> We already discussed and agreed to treat IRQs differently in idle and
> >> suspend.
> >> In summary if a SW does disable and mark IRQ as wake up capable.
> >> 1.    irq_disable()
> >> 2.    enable_irq_wake()
> >>
> >> Since the HW don't understand wake, it only knows either enabled or
> >> disabled IRQ. So the HW should do below.
> >> 1.    if system is in suspend, the IRQ should be kept Enabled in HW
> >> 2.    if system is out of suspend, the IRQ should be kept disabled in HW
> > Why should it be kept disabled at the PDC and GIC when the system is out
> > of suspend? Is that because software hasn't enabled the irq yet upon
> > resume and we're waiting for the irq consumer driver (EC for example) to
> > do that?
> No, for validation of this change, i did below scenario
> 
> During a probe of a driver, requested an IRQ and then mark it as wakeup 
> capable and disabled it.
>  From driver, after probe is done with above operation, we never do any 
> enable/disable/wakeup operation.
> 
> In above scenario,
> 
> 1.    if system is in suspend, the IRQ should be kept Enabled in HW
>      (since it was marked for wakeup during probe, right?)

Yes.

> 2.    if system is out of suspend, the IRQ should be kept disabled in HW
>      (since it was disabled in probe, and never again enabled it, right?)

Not exactly. It was requested in probe, so it was enabled, and then it
was lazy disabled so it has basically always been enabled since it was
requested. If lazy disable isn't used then there's a problem.

> 
> i wanted to test if in HW its Enabled properly during suspend entry
> (since we differ status for idle and suspend, in above scenario IRQ 
> should be enabled in HW only during suspend
> and then when system is out of suspend it should get disabled in HW again)
> 
> of course, if we ever woke up from suspend with this wake up capable 
> IRQ, the handler will never get called
> since its forever marked as disabled in SW but yet the HW is still 
> allowed to wake from this IRQ.

Alright. The other case is request an irq without auto enable flag set,
which is pretty rare, and then mark it as wakeup capable and suspend. I
hope we can ignore this one.

> >
> > In the CPU idle path (i.e. the system isn't suspending) I'd expect the
> > PDC and GIC hwirqs to be enabled so that the interrupt can trigger at
> > anytime. This is the normal mode of operation.
> Correct
> > When the CPU goes to
> > idle, and for that matter all the CPUs go to idle, the PDC should still
> > be monitoring the irqs that are enabled with irq_enable() and wake up
> > the CPU to receive the irq by taking all the CPUs out of deep idle
> > states where the GIC is powered off. If the irq is disabled with
> > irq_disable(), I'd think we would want to disable in the PDC so that the
> > irq doesn't wake us up from idle unnecessarily.
> yes, we do all this with current patch.
> >
> > Long story short, CPUs going in and out of idle and system not
> > suspending or freezing for s2idle means the PDC enable state should
> > follow irq enable state and completely ignore wake state. During system
> > suspend or freezing for s2idle the PDC enable state should follow wake
> > state. The tricky part is making sure the suspend and resume path
> > doesn't miss some interrupt that would have woken us up.
> Agree, we do all settings in PDC / GIC HW during deep suspend/ s2idle 
> suspend
> and revert back to orignal state in HW when coming out of suspend
> >
> > I thought that maybe lazy irq disable would save us here.
> the lazy doesn't work for GPIO IRQs, gpiolib registers for .irq_disable 
> for every gpio chip.

Why? Can the gpio chip stop doing that?

> >>
> >> This answer's your below comment as well on why can't we use irqchip's
> >> irq_suspend() and irq_resume() calls,
> >> since they also get internally invoked from syscore ops only.
> > Actually no it doesn't. It looks like the irqchip irq_suspend() and
> > irq_resume() ops are only called when the driver uses the generic
> > irqchip implementation. That doesn't look to be used here.
> correct
> > Good point
> > about the syscore ops and s2idle interaction, but it's not the real
> > reason why these ops can't be used.
> No internally generic chip also looks using syscore ops 
> (irq_gc_syscore_ops)
> to invoke generic irq_chip's irq_suspend and irq_resume callbacks
> 
> since s2idle still don't call syscore ops, these callback will never get 
> invoked
> if s2idle suspend is entered.

Are you using the generic irqchip code? I don't see that used here so
talking about the usage of syscore and how that doesn't work for s2idle
doesn't seem relevant here.

> >
> > Maybe we can always call the ops from somewhere deep in the suspend path
> > instead of using a notifier. That may make it simpler to reason about
> > and fix the s2idle problem at the same time. Putting it into a notifier
> > is difficult to understand and could potentially have a problem if
> > something like the timer irq needs to stay enabled until the end of
> > suspend but our suspend ops turn it off.
> we are not using suspend ops for same reason, with notifiers this will 
> never happen, since we can determine last cpu
> 
> 1. during s2idle suspend
>      for all CPUs cpu_pm notifier will be called. we can determine which 
> one is last cpu calling it.
> 
> 2. during deep suspend
>      non-boot cpus get disabled and from last cpu, kernel/cpu_pm.c 
> registers for syscore ops and via
>     this ops it invokes cpu_pm notification. so we know it is last cpu.
> 
> so both cases these will be invoked at very last stage of suspend(be it 
> s2idle or deep) when all other drivers are done
> 
> and we will never run into above scenario.
> 

I hope the irqs can be lazy disabled and whatever problem exists in gpio
driver can be resolved so that irqs can be left enabled in PDC and GIC
during suspend and idle entry. It's sort of odd to rely on lazy disable
behavior in this way, so it would need genirq maintainer approval.

Powered by blists - more mailing lists