Message-ID: <CAD=FV=Wa6mUAM_bMN7E-D1Wi9qbFNJWWjfVsgcAVUBmwGqBhSg@mail.gmail.com>
Date:   Mon, 13 Jul 2020 15:16:59 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Maulik Shah <mkshah@...eaurora.org>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Marc Zyngier <maz@...nel.org>,
        LinusW <linus.walleij@...aro.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Evan Green <evgreen@...omium.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Andy Gross <agross@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Lina Iyer <ilina@...eaurora.org>,
        Srinivas Rao L <lsrao@...eaurora.org>
Subject: Re: [PATCH v3 4/5] irqchip: qcom-pdc: Introduce irq_set_wake call

Hi,

On Mon, Jun 22, 2020 at 2:33 AM Maulik Shah <mkshah@...eaurora.org> 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@...eaurora.org>
> ---
>  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".

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.



>  }
>
>  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
>

Powered by blists - more mailing lists