Date:   Tue, 14 Jul 2020 08:04:42 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     LinusW <linus.walleij@...aro.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Cheng-Yi Chiang <cychiang@...omium.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Lina Iyer <ilina@...eaurora.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Andy Gross <agross@...nel.org>,
        Maulik Shah <mkshah@...eaurora.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] pinctrl: qcom: Handle broken/missing PDC dual edge
 IRQs on sc7180

Hi,

On Tue, Jul 14, 2020 at 3:54 AM Marc Zyngier <maz@...nel.org> wrote:
>
> Hi Doug,
>
> On 2020-07-13 16:26, Douglas Anderson wrote:
> > Depending on how you look at it, you can either say that:
> > a) There is a PDC hardware issue (with the specific IP rev that exists
> >    on sc7180) that causes the PDC not to work properly when configured
> >    to handle dual edges.
> > b) The dual edge feature of the PDC hardware was only added in later
> >    HW revisions and thus isn't in all hardware.
> >
> > Regardless of how you look at it, let's work around the lack of dual
> > edge support by only ever letting our parent see requests for single
> > edge interrupts on affected hardware.
> >
> > NOTE: it's possible that a driver requesting a dual edge interrupt
> > might get several edges coalesced into a single IRQ.  For instance if
> > a line starts low and then goes high and low again, the driver that
> > requested the IRQ is not guaranteed to be called twice.  However, it
> > is guaranteed that once the driver's interrupt handler starts running
> > its first instruction that any new edges coming in will cause the
> > interrupt to fire again.  This is relatively commonplace for dual-edge
> > gpio interrupts (many gpio controllers require software to emulate
> > dual edge with single edge) so client drivers should be setup to
> > handle it.
> >
> > Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
> > Signed-off-by: Douglas Anderson <dianders@...omium.org>
> > ---
> > As far as I can tell everything here should work and the limited
> > testing I'm able to give it shows that, in fact, I can detect both
> > edges.
> >
> > I specifically left off Reviewed-by and Tested-by tags from v2 becuase
> > I felt that the implementation had changed just enough to invalidate
> > previous reviews / testing.  Hopefully it's not too much of a hassle
> > for folks to re-review and re-test.
> >
> > Changes in v2:
> > - Use handle_fasteoi_ack_irq() and switch edges in the Ack now.
> > - If we change types, switch back to the normal handle_fasteoi_irq().
> > - No extra locking.
> > - Properly print an error if we hit 100 loops w/ no stability.
> > - Beefed up the commit message.
> >
> >  drivers/pinctrl/qcom/Kconfig          |  2 +
> >  drivers/pinctrl/qcom/pinctrl-msm.c    | 74 ++++++++++++++++++++++++++-
> >  drivers/pinctrl/qcom/pinctrl-msm.h    |  4 ++
> >  drivers/pinctrl/qcom/pinctrl-sc7180.c |  1 +
> >  4 files changed, 79 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pinctrl/qcom/Kconfig
> > b/drivers/pinctrl/qcom/Kconfig
> > index ff1ee159dca2..f8ff30cdafa6 100644
> > --- a/drivers/pinctrl/qcom/Kconfig
> > +++ b/drivers/pinctrl/qcom/Kconfig
> > @@ -7,6 +7,8 @@ config PINCTRL_MSM
> >       select PINCONF
> >       select GENERIC_PINCONF
> >       select GPIOLIB_IRQCHIP
> > +     select IRQ_DOMAIN_HIERARCHY
> > +     select IRQ_FASTEOI_HIERARCHY_HANDLERS
> >
> >  config PINCTRL_APQ8064
> >       tristate "Qualcomm APQ8064 pin controller driver"
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c
> > b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index 83b7d64bc4c1..eae8f421ff63 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -832,6 +832,52 @@ static void msm_gpio_irq_unmask(struct irq_data
> > *d)
> >       msm_gpio_irq_clear_unmask(d, false);
> >  }
> >
> > +/**
> > + * msm_gpio_update_dual_edge_parent() - Prime next edge for IRQs
> > handled by parent.
> > + * @d: The irq dta.
> > + *
> > + * This is much like msm_gpio_update_dual_edge_pos() but for IRQs that
> > are
> > + * normally handled by the parent irqchip.  The logic here is slightly
> > + * different due to what's easy to do with our parent, but in
> > principle it's
> > + * the same.
> > + */
> > +static void msm_gpio_update_dual_edge_parent(struct irq_data *d)
> > +{
> > +     struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +     struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> > +     const struct msm_pingroup *g = &pctrl->soc->groups[d->hwirq];
> > +     int loop_limit = 100;
> > +     unsigned int val;
> > +     unsigned int type;
> > +
> > +     /* Read the value and make a guess about what edge we need to catch
> > */
> > +     val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> > +     type = val ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> > +
> > +     do {
> > +             /* Set the parent to catch the next edge */
> > +             irq_chip_set_type_parent(d, type);
> > +
> > +             /*
> > +              * Possibly the line changed between when we last read "val"
> > +              * (and decided what edge we needed) and when set the edge.
> > +              * If the value didn't change (or changed and then changed
> > +              * back) then we're done.
> > +              */
> > +             val = msm_readl_io(pctrl, g) & BIT(g->in_bit);
> > +             if (type == IRQ_TYPE_EDGE_RISING) {
> > +                     if (!val)
> > +                             return;
> > +                     type = IRQ_TYPE_EDGE_FALLING;
> > +             } else if (type == IRQ_TYPE_EDGE_FALLING) {
> > +                     if (val)
> > +                             return;
> > +                     type = IRQ_TYPE_EDGE_RISING;
> > +             }
> > +     } while (loop_limit-- > 0);
> > +     dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
>
> I'd tone this down to a dev_warn_once(), if at all possible, or
> some other rate-limited variant.
>
> Otherwise, looks OK to me.

Thanks for your review.  I'm sending out v3 without any delay just to
get this fixed up since it's not controversial.  Hope that's OK.

-Doug

Powered by blists - more mailing lists