Re: [RFC v3] irqchip: qcom: pdc: Introduce irq_set_wake call

[Maulik Shah <mkshah@codeaurora.org>]

Hi,

On 4/24/2020 8:07 AM, Stephen Boyd wrote:
> Quoting Maulik Shah (2020-04-22 05:14:21)
>> We discussed this in RFC v2, pasting your reply from same.
>>
>>> That looks like a bug. It appears that gpiolib is only hooking the irq
>>> disable path here so that it can keep track of what irqs are not in use
>>> by gpiolib and allow them to be used for GPIO purposes when the irqs are
>>> disabled. See commit 461c1a7d4733 ("gpiolib: override
>>> irq_enable/disable"). That code in gpiolib should probably see if lazy
>>> mode has been disabled on the irq and do similar things to what genirq
>>> does and then let genirq mask the gpios if they trigger during the time
>>> when the irq is disabled. Regardless of this design though, I don't
>>> understand why this matters.
> Yeah. I'm saying that the gpiolib code that forces all gpio irqs to be
> non-lazy is broken. Please send a patch to fix gpiolib so that irqs can
> be lazy again.

Let me reiterate.

In below genirq code, if irq_chip did not choose to implement 
.irq_disable (and the mask argument is coming as false by default during 
first disable) from the caller of this,

Then yes control takes lazy path since it doesn't enter to  if 
(desc->irq_data.chip->irq_disable)  or else if (mask) case and the 
__irq_disable call simply returns  without executing anything.

This leaves IRQ enabled in HW during suspend even though driver done 
disable_irq()

static void __irq_disable(struct irq_desc *desc, bool mask)
{
         if (irqd_irq_disabled(&desc->irq_data)) {
                 if (mask)
                         mask_irq(desc);
         } else {
                 irq_state_set_disabled(desc);
                 if (desc->irq_data.chip->irq_disable) {
desc->irq_data.chip->irq_disable(&desc->irq_data);
                         irq_state_set_masked(desc);
                 } else if (mask) {
                         mask_irq(desc);
                 }
         }
}

With above understanding, we have two more points to consider

1. The gpiolib registers for .irq_disable for every gpio chip, making it 
unlazy for every gpio chip.

2. The irq_chip that registers with gpiolib (from pinctrl-msm.c, also 
has .irq_disable implemented, and its parent PDC also has .irq_disable 
implemented)

Even if we fix point (1), by making gpiolib replicating genirq code,
It still will call irq_disable (from replicated code in gpiolib now) 
since irq_chip that registers with gpiolib has chosen to implement 
.irq_disable.

For other irq_chip's which don't implement .irq_disable, it will help.
But since from point (2) we know that PDC irq_chip implements 
.irq_disable, and would disable in HW the moment .irq_disable call comes in.


> I thought it didn't matter before but now that I've
> learned more it sounds like we have to use lazy irqs here so that irqs
> stay enabled on the path to suspend while they're disabled but marked
> for wakeup.

The description for irq_disable() says...

  * If the chip does not implement the irq_disable callback, we
  * use a lazy disable approach. That means we mark the interrupt
  * disabled, but leave the hardware unmasked. That's an
  * optimization...

Since PDC irq_chip implements .irq_disable callback, IRQ gets disabled 
in HW immediatly.

The comment says that lazy approch will be used if .irq_disable callback 
is not implemented.

IMO, client driver's should not assume/expect that underlying irq_chip 
will take lazy path only,
As it depends on whether irq_chip implemented .irq_disable callback or not.

If a driver is relying on lazy disable approach till date, then the 
driver should get fixed.
(by removing the unnecessary disable_irq() during suspend)

i suspect this might be the case in EC driver, on the platforms where it 
worked fine till today,
irq may not have been disabled in HW (may be that working platform's 
irq_chip may not have implemented irq_disable
hence support lazy disable which leaves IRQ unmasked in HW even after 
disable_irq() and able to resume from suspend)


> Otherwise I don't know how to make this work.
There are 3 possible options to make it work

1. Fix the EC driver (to stop calling disable_irq() during suspend) / 
support unlazy disable in EC driver

2. Support lazy disable (requires fixes in gpiolib and irq_chip also 
should not implement .irq_disable callback)

With above explanation, The PDC irq_chip want to keep continue implement 
.irq_disable callback
(we have different functionality in .irq_mask and .irq_disable 
implementation)

so option 2 is ruled out IMO.

3. We use this patch

The current patch makes this work by re-enabling such IRQs in HW during 
suspend's very end point, and restores state during resume.

i can not think of any other option.

Let me know if you now agree to go with option (1)

Otherwise, for option (3),

i can post next revision after addressing some minor fixes/suggestions.
I have tried to make this further simpler by not operating on both 
PDC-GPIO and PDC irq domains,
but it seems we need to use both irq domains and take the action 
accordingly during pdc_suspend() and pdc_resume().

I have missed the default case in switch (noticed same during rpmh patch 
doug has posted :-)), as it can get called for CLUSTER notification.
Will fix in next revision.

Thanks,
Maulik

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation