lkml.org 
[lkml]   [2020]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] pinctrl: qcom: Handle broken/missing PDC dual edge IRQs on sc7180
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@chromium.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,

M.
--
Jazz is not dead. It just smells funny...

\
 
 \ /
  Last update: 2020-07-14 12:54    [W:0.069 / U:0.516 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site