Re: [PATCH v3 4/5] irqchip: qcom-pdc: Introduce irq_set_wake call

[Maulik Shah]

Hi,

On 7/14/2020 3:46 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <mkshah@xxxxxxxxxxxxxx> wrote:
> > Remove irq_disable callback to allow lazy disable for pdc interrupts.
> >
> > Add irq_set_wake callback that unmask interrupt in HW when drivers
> > mark interrupt for wakeup. Interrupt will be cleared in HW during
> > lazy disable if its not marked for wakeup.
> >
> > Signed-off-by: Maulik Shah <mkshah@xxxxxxxxxxxxxx>
> > ---
> >   drivers/irqchip/qcom-pdc.c | 34 ++++++++++++++++------------------
> >   1 file changed, 16 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> > index 6ae9e1f..8beb6f7 100644
> > --- a/drivers/irqchip/qcom-pdc.c
> > +++ b/drivers/irqchip/qcom-pdc.c
> > @@ -36,6 +36,7 @@ struct pdc_pin_region {
> >          u32 cnt;
> >   };
> >
> > +static DECLARE_BITMAP(pdc_wake_irqs, PDC_MAX_IRQS);
> >   static DEFINE_RAW_SPINLOCK(pdc_lock);
> >   static void __iomem *pdc_base;
> >   static struct pdc_pin_region *pdc_region;
> > @@ -87,22 +88,17 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
> >          raw_spin_unlock(&pdc_lock);
> >   }
> >
> > -static void qcom_pdc_gic_disable(struct irq_data *d)
> > +static int qcom_pdc_gic_set_wake(struct irq_data *d, unsigned int on)
> >   {
> > -       if (d->hwirq == GPIO_NO_WAKE_IRQ)
> > -               return;
> > -
> > -       pdc_enable_intr(d, false);
> > -       irq_chip_disable_parent(d);
> > -}
> > -
> > -static void qcom_pdc_gic_enable(struct irq_data *d)
> > -{
> > -       if (d->hwirq == GPIO_NO_WAKE_IRQ)
> > -               return;
> > +       if (on) {
> > +               pdc_enable_intr(d, true);
> > +               irq_chip_enable_parent(d);
> > +               set_bit(d->hwirq, pdc_wake_irqs);
> > +       } else {
> > +               clear_bit(d->hwirq, pdc_wake_irqs);
> > +       }
> >
> > -       pdc_enable_intr(d, true);
> > -       irq_chip_enable_parent(d);
> > +       return irq_chip_set_wake_parent(d, on);
> >   }
> >
> >   static void qcom_pdc_gic_mask(struct irq_data *d)
> > @@ -111,6 +107,9 @@ static void qcom_pdc_gic_mask(struct irq_data *d)
> >                  return;
> >
> >          irq_chip_mask_parent(d);
> > +
> > +       if (!test_bit(d->hwirq, pdc_wake_irqs))
> > +               pdc_enable_intr(d, false);
> I _think_ this will break masking, right?  In other words, consider
> the following (having nothing to do with suspend/resume):
>
> 1. Driver requests an interrupt.
> 2. Driver masks interrupt (calls disable_irq())
> 3. Interrupt fires while it is masked.
> 4. Driver unmasks interrupt (calls enable_irq().
>
> After step #4 the interrupt should fire since it was only masked, not
> disabled (yes, it's super confusing that the driver calls
> disable_irq() but it expecting it to be masked--as I understand it
> that's just how it is).  I haven't tested, but I suspect that's broken
> for you now (assuming you're working on a pin that wasn't a wakeup
> pin) because you won't track edges when you're "disabled".
No its not broken, it works as expected. after step #4, interrupt will fire.
> I suspect that the right thing to do here is to:
>
> a) Make qcom_pdc_gic_set_wake() just keep "pdc_wake_irqs" up to date
> and then call parent.
>
> b) Implement irq_suspend and irq_resume.  In irq_suspend() you disable
> all interrupts that aren't in "pdc_wake_irqs".  In irq_resume() you
> just re-enable all of them (masking will be handled by the parent).
>
> Would that work?
>
> ...oh, drat!  The .irq_suspend() callback is only there if you're
> using "irq/generic-chip.c".  OK, well unless we want to move over to
> using generic-chip we can just register for syscore ourselves.  OK, I
> tested and <https://crrev.com/c/2296160> works.

I too thought of using syscore ops earlier, but syscore ops won't work 
if device chooses to enter "s2idle" suspend state since they are not 
invoked in s2idle entry path.

even if you register for "irq/generic-chip.c" , this driver too 
registers for syscore ops only, it won't work if you enter s2idle 
suspend state.

Current patch works fine with both s2idle and deep suspend states.

Thanks,
Maulik
> >   }
> >
> >   static void qcom_pdc_gic_unmask(struct irq_data *d)
> > @@ -118,6 +117,7 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
> >          if (d->hwirq == GPIO_NO_WAKE_IRQ)
> >                  return;
> >
> > +       pdc_enable_intr(d, true);
> >          irq_chip_unmask_parent(d);
> >   }
> >
> > @@ -197,15 +197,13 @@ static struct irq_chip qcom_pdc_gic_chip = {
> >          .irq_eoi                = irq_chip_eoi_parent,
> >          .irq_mask               = qcom_pdc_gic_mask,
> >          .irq_unmask             = qcom_pdc_gic_unmask,
> > -       .irq_disable            = qcom_pdc_gic_disable,
> > -       .irq_enable             = qcom_pdc_gic_enable,
> >          .irq_get_irqchip_state  = qcom_pdc_gic_get_irqchip_state,
> >          .irq_set_irqchip_state  = qcom_pdc_gic_set_irqchip_state,
> >          .irq_retrigger          = irq_chip_retrigger_hierarchy,
> >          .irq_set_type           = qcom_pdc_gic_set_type,
> > +       .irq_set_wake           = qcom_pdc_gic_set_wake,
> >          .flags                  = IRQCHIP_MASK_ON_SUSPEND |
> > -                                 IRQCHIP_SET_TYPE_MASKED |
> > -                                 IRQCHIP_SKIP_SET_WAKE,
> > +                                 IRQCHIP_SET_TYPE_MASKED,
> >          .irq_set_vcpu_affinity  = irq_chip_set_vcpu_affinity_parent,
> >          .irq_set_affinity       = irq_chip_set_affinity_parent,
> >   };
> > --
> > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> > of Code Aurora Forum, hosted by The Linux Foundation
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation