Hi,
On Tue, Jul 14, 2020 at 3:54 AM Marc Zyngier <maz@xxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > 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
