lkml.org 
[lkml]   [2020]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] pinctrl: qcom: Handle broken PDC dual edge case on sc7180
    From
    Date

    On 7/9/2020 2:46 AM, Douglas Anderson wrote:
    > As per Qualcomm, 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.
    >
    > Let's work around this by emulating only ever letting our parent see
    > requests for single edge interrupts on affected hardware.
    >
    > Fixes: e35a6ae0eb3a ("pinctrl/msm: Setup GPIO chip in hierarchy")
    > Signed-off-by: Douglas Anderson <dianders@chromium.org>

    Thanks Doug, this looks like a much better solution than what I was
    proposing :)

    Reviewed-by: Rajendra Nayak <rnayak@codeaurora.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.
    >
    > Please give this an extra thorough review since it's trying to find
    > the exact right place to insert this code and I'm not massively
    > familiar with all the frameworks.
    >
    > If someone has hardware where it's easy to stress test this that'd be
    > wonderful too. The board I happen to have in front of me doesn't have
    > any easy-to-toggle GPIOs where I can just poke a button or a switch to
    > generate edges. My testing was done by hacking the "write protect"
    > GPIO on my board into gpio-keys as a dual-edge interrupt and then
    > sending commands to our security chip to toggle it--not exactly great
    > for testing to make sure there are no race conditions if the interrupt
    > bounces a lot.
    >
    > drivers/pinctrl/qcom/pinctrl-msm.c | 80 +++++++++++++++++++++++++++
    > drivers/pinctrl/qcom/pinctrl-msm.h | 4 ++
    > drivers/pinctrl/qcom/pinctrl-sc7180.c | 1 +
    > 3 files changed, 85 insertions(+)
    >
    > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
    > index 83b7d64bc4c1..45ca09ebb7b3 100644
    > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
    > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
    > @@ -860,6 +860,79 @@ static void msm_gpio_irq_ack(struct irq_data *d)
    > raw_spin_unlock_irqrestore(&pctrl->lock, flags);
    > }
    >
    > +/**
    > + * 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];
    > + unsigned long flags;
    > + 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;
    > +
    > + raw_spin_lock_irqsave(&pctrl->lock, flags);
    > + 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)
    > + break;
    > + type = IRQ_TYPE_EDGE_FALLING;
    > + } else if (type == IRQ_TYPE_EDGE_FALLING) {
    > + if (val)
    > + break;
    > + type = IRQ_TYPE_EDGE_RISING;
    > + }
    > + } while (loop_limit-- > 0);
    > + raw_spin_unlock_irqrestore(&pctrl->lock, flags);
    > +
    > + if (!loop_limit)
    > + dev_err(pctrl->dev, "dual-edge irq failed to stabilize\n");
    > +}
    > +
    > +void msm_gpio_handle_dual_edge_parent_irq(struct irq_desc *desc)
    > +{
    > + struct irq_data *d = &desc->irq_data;
    > +
    > + /* Make sure we're primed for the next edge */
    > + msm_gpio_update_dual_edge_parent(d);
    > +
    > + /* Pass on to the normal interrupt handler */
    > + handle_fasteoi_irq(desc);
    > +}
    > +
    > +static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
    > + unsigned int type)
    > +{
    > + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
    > + struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
    > +
    > + return type == IRQ_TYPE_EDGE_BOTH &&
    > + pctrl->soc->wakeirq_dual_edge_errata && d->parent_data &&
    > + test_bit(d->hwirq, pctrl->skip_wake_irqs);
    > +}
    > +
    > static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
    > {
    > struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
    > @@ -868,6 +941,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
    > unsigned long flags;
    > u32 val;
    >
    > + if (msm_gpio_needs_dual_edge_parent_workaround(d, type)) {
    > + irq_set_handler_locked(d, msm_gpio_handle_dual_edge_parent_irq);
    > + msm_gpio_update_dual_edge_parent(d);
    > +
    > + return 0;
    > + }
    > +
    > if (d->parent_data)
    > irq_chip_set_type_parent(d, type);
    >
    > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
    > index 9452da18a78b..7486fe08eb9b 100644
    > --- a/drivers/pinctrl/qcom/pinctrl-msm.h
    > +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
    > @@ -113,6 +113,9 @@ struct msm_gpio_wakeirq_map {
    > * @pull_no_keeper: The SoC does not support keeper bias.
    > * @wakeirq_map: The map of wakeup capable GPIOs and the pin at PDC/MPM
    > * @nwakeirq_map: The number of entries in @wakeirq_map
    > + * @wakeirq_dual_edge_errata: If true then GPIOs using the wakeirq_map need
    > + * to be aware that their parent can't handle dual
    > + * edge interrupts.
    > */
    > struct msm_pinctrl_soc_data {
    > const struct pinctrl_pin_desc *pins;
    > @@ -128,6 +131,7 @@ struct msm_pinctrl_soc_data {
    > const int *reserved_gpios;
    > const struct msm_gpio_wakeirq_map *wakeirq_map;
    > unsigned int nwakeirq_map;
    > + bool wakeirq_dual_edge_errata;
    > };
    >
    > extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
    > diff --git a/drivers/pinctrl/qcom/pinctrl-sc7180.c b/drivers/pinctrl/qcom/pinctrl-sc7180.c
    > index 1b6465a882f2..1d9acad3c1ce 100644
    > --- a/drivers/pinctrl/qcom/pinctrl-sc7180.c
    > +++ b/drivers/pinctrl/qcom/pinctrl-sc7180.c
    > @@ -1147,6 +1147,7 @@ static const struct msm_pinctrl_soc_data sc7180_pinctrl = {
    > .ntiles = ARRAY_SIZE(sc7180_tiles),
    > .wakeirq_map = sc7180_pdc_map,
    > .nwakeirq_map = ARRAY_SIZE(sc7180_pdc_map),
    > + .wakeirq_dual_edge_errata = true,
    > };
    >
    > static int sc7180_pinctrl_probe(struct platform_device *pdev)
    >

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

    \
     
     \ /
      Last update: 2020-07-10 06:29    [W:5.693 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site