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,
M.
--
Jazz is not dead. It just smells funny...
