We have a problem if we use gpio-keys and configure wakeups such that
we only want one edge to wake us up. AKA:
wakeup-event-action = <EV_ACT_DEASSERTED>;
wakeup-source;
Specifically we end up with a phantom interrupt that blocks suspend if
the line was already high and we want wakeups on rising edges (AKA we
want the GPIO to go low and then high again before we wake up). The
opposite is also problematic.
Specifically, here's what's happening today:
1. Normally, gpio-keys configures to look for both edges. Due to the
current workaround introduced in commit c3c0c2e18d94 ("pinctrl:
qcom: Handle broken/missing PDC dual edge IRQs on sc7180"), if the
line was high we'd configure for falling edges.
2. At suspend time, we change to look for rising edges.
3. After qcom_pdc_gic_set_type() runs, we get a phantom interrupt.
We can solve this by just clearing the phantom interrupt.
NOTE: it is possible that this could cause problems for a client with
very specific needs, but there's not much we can do with this
hardware. As an example, let's say the interrupt signal is currently
high and the client is looking for falling edges. The client now
changes to look for rising edges. The client could possibly expect
that if the line has a short pulse low (and back high) that it would
always be detected. Specifically no matter when the pulse happened,
it should either have tripped the (old) falling edge trigger or the
(new) rising edge trigger. We will simply not trip it. We could
narrow down the race a bit by polling our parent before changing
types, but no matter what we do there will still be a period of time
where we can't tell the difference between a real transition (or more
than one transition) and the phantom.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/irqchip/qcom-pdc.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index bd39e9de6ecf..7d097164aadc 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -159,6 +159,8 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
{
int pin_out = d->hwirq;
enum pdc_irq_config_bits pdc_type;
+ enum pdc_irq_config_bits old_pdc_type;
+ int ret;
if (pin_out == GPIO_NO_WAKE_IRQ)
return 0;
@@ -187,9 +189,24 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
return -EINVAL;
}
+ old_pdc_type = pdc_reg_read(IRQ_i_CFG, pin_out);
pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
- return irq_chip_set_type_parent(d, type);
+ ret = irq_chip_set_type_parent(d, type);
+
+ /*
+ * When we change types the PDC can give a phantom interrupt.
+ * Clear it. Specifically the phantom shows up if a line is already
+ * high and we change to rising or if a line is already low and we
+ * change to falling but let's be consistent and clear it always.
+ *
+ * Doing this works because we have IRQCHIP_SET_TYPE_MASKED so the
+ * interrupt will be cleared before the rest of the system sees it.
+ */
+ if (old_pdc_type != pdc_type)
+ irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
+
+ return ret;
}
static struct irq_chip qcom_pdc_gic_chip = {
--
2.29.2.454.gaff20da3a2-goog
Conceptually, we can envision the input on Qualcomm SoCs to pass
through a bunch of blocks between coming into the chip and becoming a
GPIO interrupt. From guessing and running a handful of tests, I
believe that we can represent the state of the world with a drawing
that looks something like this:
+-----------------+ +-----------------+ +-----------------+
| INPUT | --> | PINMUX | | IS_INPUT |
+-----------------+ | | --> | |
| output bogus (?)| | output bogus (?)|
| if not muxed | | if input disab. |
+-----------------+ +-----------------+
|
+---------------------------------------------------+--> to PDC
|
V
+-----------------+ +-----------------+ +-----------------+
| INTR RAW ENABLE | | DETECTION LOGIC | | STATUS REGISTER |
| | | | | |
| output bogus (?)| --> | maybe handles | | latches inputs |
| if disabled | | polarity diffs | --> | that are high |
+-----------------+ | | | |
| maybe debounces | | write 1 to clr |
| level irqs | +-----------------+
+-----------------+ |
|
+---------------------------------------------------+
|
V
+-----------------+
| ENABLE |
| | +-----------------+
| nothing passes | --> | SUMMARY IRQ |
| through if | +-----------------+
| disabled |
+-----------------+
The above might not be 100% exact, but for the purpose of this
discussion, the point is that there are a whole bunch of gates and
transformations on the input before it gets to the circuitry that
generates interrupts.
As you might guess, if you reconfigure one of the gates in the above
diagram while the system is configured to detect interrupts things get
a little wacky. Specifically it appears that if you gate the input at
any step it can cause various glitches in the later steps because they
are still paying attention to their input but their input isn't really
sane anymore.
I did some poking and I found that I could generate bogus interrupts
in the system both when muxing away from GPIO mode and also when
muxing back to GPIO mode. When configured to use the PDC path for
generating interrupts I found that if the external input on the GPIO
was low that I'd get what looked like a rising edge when unmuxing and
a falling edge when muxing. When configured away from the PDC path I
got slightly different glitch interrupts when changing muxing.
These glitches when remuxing matter in reality, not just in theory.
To be concrete, let's take the special "wakeup_irq" in
qcom_geni_serial.c as an example. In sc7180-trogdor.dtsi we configure
the uart3 to have two pinctrl states, sleep and default, and mux
between the two during runtime PM and system suspend (see
geni_se_resources_{on,off}() for more details). The difference
between the sleep and default state is that the RX pin is muxed to a
GPIO during sleep and muxed to the UART otherwise. When we switch
between these two states we can generate the glitches talked about
above because we're configured to look for edges but the transition
from the gated input (which is bogus) to the real input can look like
an edge.
Historically the UART case above was handled by the fact that the
"enable" function in the MSM GPIO driver did an "unmask and clear".
This relied on the fact that the system happens to have the interrupt
disabled until suspend time and that it would enable it after the
pinmux change happened, thus clearing the interrupt.
The historical solution, however, had a few problems. The first
problem (that nobody seemed to have tripped) is that we can still get
bogus interrupts if we remux when the interrupt isn't disabled during
the muxing and re-enabled after. The second problem is that it
violates how I believe that the interrupt enable path is supposed to
work.
In Linux, if a driver does disable_irq() and later does enable_irq()
on its interrupt, I believe it's expecting these properties:
* If an interrupt was pending when the driver disabled then it will
still be pending after the driver re-enables.
* If an edge-triggered interrupt comes in while an interrupt is
disabled it should assert when the interrupt is re-enabled.
If you think that the above sounds a lot like the disable_irq() and
enable_irq() are supposed to be masking/unmasking the interrupt
instead of disabling/enabling it then you've made an astute
observation. Specifically when talking about interrupts, "mask"
usually means to stop posting interrupts but keep tracking them and
"disable" means to fully shut off interrupt detection. It's
unfortunate that this is so confusing, but presumably this is all the
way it is for historical reasons.
Perhaps more confusing than the above is that, even though clients of
IRQs themselves don't have a way to request mask/unmask
vs. disable/enable calls, IRQ chips themselves can implement both.
...and yet more confusing is that if an IRQ chip implements
disable/enable then they will be called when a client driver calls
disable_irq() / enable_irq().
It does feel like some of the above could be cleared up. However,
without any other core interrupt changes it should be clear that when
an IRQ chip gets a request to "disable" an IRQ that it has to treat it
like a mask of that IRQ.
In any case, after that long interlude you can see that the "unmask
and clear" can break things. Maulik tried to fix it so that we no
longer did "unmask and clear" in commit 71266d9d3936 ("pinctrl: qcom:
Move clearing pending IRQ to .irq_request_resources callback"), but it
didn't work for two reasons:
* It only tried to address the problem for interrupts that had parents
(like the PDC).
* It regressed the problem that the original clearing was trying to
solve.
I think we can safely assume that if someone muxes a pin to be
something other than a GPIO and then muxes it back that we can clear
any interrupts that were pending on it without violating any
assumptions that client drivers are making. Presumably the client
drivers are intentionally remuxing the pin away from a dedicated
purpose to be a plain GPIO so they don't care what the pin state was
before the mux switch and they don't expect to see the pin change
level during this switch. Let's move the clearing of the IRQ to the
pin muxing routine so that we'll clear a pending IRQ if we're muxing
from some non-GPIO mode to a GPIO mode.
Fixes: 71266d9d3936 ("pinctrl: qcom: Move clearing pending IRQ to .irq_request_resources callback")
Signed-off-by: Douglas Anderson <[email protected]>
---
This is a pretty hairy little patch and presumably needs a good amount
of testing / discussion before landing. If this patch is totally
broken / wrong feel free to consider it as an RFC and suggest how it
should be better.
Also note: I wanted to put this in the same series as patch #1, but
IMO that patch can stand on its own. If it looks Ok but we want to
have lots of debate about this one, please land patch #1 on its own
and we can split the series.
I have done most of this patch testing on the Chrome OS 5.4 kernel
tree (with many backports) but have sanity checked it on mainline.
drivers/pinctrl/qcom/pinctrl-msm.c | 104 ++++++++++++++++++-----------
1 file changed, 64 insertions(+), 40 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 588df91274e2..e7c3927c7d54 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -166,14 +166,42 @@ static int msm_get_function_groups(struct pinctrl_dev *pctldev,
return 0;
}
+static void msm_pinctrl_clear_pending_irq(struct msm_pinctrl *pctrl,
+ unsigned int group,
+ unsigned int irq)
+{
+ struct irq_data *d = irq_get_irq_data(irq);
+ const struct msm_pingroup *g;
+ unsigned long flags;
+ u32 val;
+
+ if (!d)
+ return;
+
+ if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
+ irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
+
+ g = &pctrl->soc->groups[group];
+
+ raw_spin_lock_irqsave(&pctrl->lock, flags);
+ val = msm_readl_intr_status(pctrl, g);
+ val &= ~BIT(g->intr_status_bit);
+ msm_writel_intr_status(val, pctrl, g);
+ raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+}
+
static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
unsigned function,
unsigned group)
{
struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+ struct gpio_chip *gc = &pctrl->chip;
+ unsigned int irq = irq_find_mapping(gc->irq.domain, group);
const struct msm_pingroup *g;
unsigned long flags;
u32 val, mask;
+ u32 oldval;
+ u32 old_i;
int i;
g = &pctrl->soc->groups[group];
@@ -187,15 +215,26 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
if (WARN_ON(i == g->nfuncs))
return -EINVAL;
- raw_spin_lock_irqsave(&pctrl->lock, flags);
+ disable_irq(irq);
- val = msm_readl_ctl(pctrl, g);
+ raw_spin_lock_irqsave(&pctrl->lock, flags);
+ oldval = val = msm_readl_ctl(pctrl, g);
val &= ~mask;
val |= i << g->mux_bit;
msm_writel_ctl(val, pctrl, g);
-
raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+ /*
+ * Clear IRQs if switching to/from GPIO mode since muxing to/from
+ * the GPIO path can cause phantom edges.
+ */
+ old_i = (oldval & mask) >> g->mux_bit;
+ if (old_i != i &&
+ (i == pctrl->soc->gpio_func || old_i == pctrl->soc->gpio_func))
+ msm_pinctrl_clear_pending_irq(pctrl, group, irq);
+
+ enable_irq(irq);
+
return 0;
}
@@ -456,32 +495,45 @@ static const struct pinconf_ops msm_pinconf_ops = {
static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
{
const struct msm_pingroup *g;
+ unsigned int irq = irq_find_mapping(chip->irq.domain, offset);
struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
unsigned long flags;
+ u32 oldval;
u32 val;
g = &pctrl->soc->groups[offset];
+ disable_irq(irq);
+
raw_spin_lock_irqsave(&pctrl->lock, flags);
- val = msm_readl_ctl(pctrl, g);
+ oldval = val = msm_readl_ctl(pctrl, g);
val &= ~BIT(g->oe_bit);
msm_writel_ctl(val, pctrl, g);
raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+ if (oldval != val)
+ msm_pinctrl_clear_pending_irq(pctrl, offset, irq);
+
+ enable_irq(irq);
+
return 0;
}
static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, int value)
{
const struct msm_pingroup *g;
+ unsigned int irq = irq_find_mapping(chip->irq.domain, offset);
struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
unsigned long flags;
+ u32 oldval;
u32 val;
g = &pctrl->soc->groups[offset];
+ disable_irq(irq);
+
raw_spin_lock_irqsave(&pctrl->lock, flags);
val = msm_readl_io(pctrl, g);
@@ -491,12 +543,17 @@ static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, in
val &= ~BIT(g->out_bit);
msm_writel_io(val, pctrl, g);
- val = msm_readl_ctl(pctrl, g);
+ oldval = msm_readl_ctl(pctrl, g);
val |= BIT(g->oe_bit);
msm_writel_ctl(val, pctrl, g);
raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+ if (oldval != val)
+ msm_pinctrl_clear_pending_irq(pctrl, offset, irq);
+
+ enable_irq(irq);
+
return 0;
}
@@ -774,7 +831,7 @@ static void msm_gpio_irq_mask(struct irq_data *d)
raw_spin_unlock_irqrestore(&pctrl->lock, flags);
}
-static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
+static void msm_gpio_irq_unmask(struct irq_data *d)
{
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
@@ -792,17 +849,6 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
raw_spin_lock_irqsave(&pctrl->lock, flags);
- if (status_clear) {
- /*
- * clear the interrupt status bit before unmask to avoid
- * any erroneous interrupts that would have got latched
- * when the interrupt is not in use.
- */
- val = msm_readl_intr_status(pctrl, g);
- val &= ~BIT(g->intr_status_bit);
- msm_writel_intr_status(val, pctrl, g);
- }
-
val = msm_readl_intr_cfg(pctrl, g);
val |= BIT(g->intr_raw_status_bit);
val |= BIT(g->intr_enable_bit);
@@ -815,14 +861,10 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
static void msm_gpio_irq_enable(struct irq_data *d)
{
- struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
- struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
-
if (d->parent_data)
irq_chip_enable_parent(d);
- if (!test_bit(d->hwirq, pctrl->skip_wake_irqs))
- msm_gpio_irq_clear_unmask(d, true);
+ msm_gpio_irq_unmask(d);
}
static void msm_gpio_irq_disable(struct irq_data *d)
@@ -837,11 +879,6 @@ static void msm_gpio_irq_disable(struct irq_data *d)
msm_gpio_irq_mask(d);
}
-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.
@@ -1097,19 +1134,6 @@ static int msm_gpio_irq_reqres(struct irq_data *d)
ret = -EINVAL;
goto out;
}
-
- /*
- * Clear the interrupt that may be pending before we enable
- * the line.
- * This is especially a problem with the GPIOs routed to the
- * PDC. These GPIOs are direct-connect interrupts to the GIC.
- * Disabling the interrupt line at the PDC does not prevent
- * the interrupt from being latched at the GIC. The state at
- * GIC needs to be cleared before enabling.
- */
- if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
- irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
-
return 0;
out:
module_put(gc->owner);
--
2.29.2.454.gaff20da3a2-goog
On Tue, Nov 24, 2020 at 1:02 AM Douglas Anderson <[email protected]> wrote:
> Conceptually, we can envision the input on Qualcomm SoCs to pass
> through a bunch of blocks between coming into the chip and becoming a
> GPIO interrupt.
This looks like really good detective engineering, something
I do myself from time to time.
Bjorn does this look OK to you?
I'm wondering about patch 1/3, does it need to be applied
with the rest?
Yours,
Linus Walleij
On 2020-11-24 00:01, Douglas Anderson wrote:
> We have a problem if we use gpio-keys and configure wakeups such that
> we only want one edge to wake us up. AKA:
> wakeup-event-action = <EV_ACT_DEASSERTED>;
> wakeup-source;
>
> Specifically we end up with a phantom interrupt that blocks suspend if
> the line was already high and we want wakeups on rising edges (AKA we
> want the GPIO to go low and then high again before we wake up). The
> opposite is also problematic.
>
> Specifically, here's what's happening today:
> 1. Normally, gpio-keys configures to look for both edges. Due to the
> current workaround introduced in commit c3c0c2e18d94 ("pinctrl:
> qcom: Handle broken/missing PDC dual edge IRQs on sc7180"), if the
> line was high we'd configure for falling edges.
> 2. At suspend time, we change to look for rising edges.
> 3. After qcom_pdc_gic_set_type() runs, we get a phantom interrupt.
>
> We can solve this by just clearing the phantom interrupt.
>
> NOTE: it is possible that this could cause problems for a client with
> very specific needs, but there's not much we can do with this
> hardware. As an example, let's say the interrupt signal is currently
> high and the client is looking for falling edges. The client now
> changes to look for rising edges. The client could possibly expect
> that if the line has a short pulse low (and back high) that it would
> always be detected. Specifically no matter when the pulse happened,
> it should either have tripped the (old) falling edge trigger or the
> (new) rising edge trigger. We will simply not trip it. We could
> narrow down the race a bit by polling our parent before changing
> types, but no matter what we do there will still be a period of time
> where we can't tell the difference between a real transition (or more
> than one transition) and the phantom.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> drivers/irqchip/qcom-pdc.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index bd39e9de6ecf..7d097164aadc 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -159,6 +159,8 @@ static int qcom_pdc_gic_set_type(struct irq_data
> *d, unsigned int type)
> {
> int pin_out = d->hwirq;
> enum pdc_irq_config_bits pdc_type;
> + enum pdc_irq_config_bits old_pdc_type;
> + int ret;
>
> if (pin_out == GPIO_NO_WAKE_IRQ)
> return 0;
> @@ -187,9 +189,24 @@ static int qcom_pdc_gic_set_type(struct irq_data
> *d, unsigned int type)
> return -EINVAL;
> }
>
> + old_pdc_type = pdc_reg_read(IRQ_i_CFG, pin_out);
> pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
>
> - return irq_chip_set_type_parent(d, type);
> + ret = irq_chip_set_type_parent(d, type);
> +
> + /*
> + * When we change types the PDC can give a phantom interrupt.
> + * Clear it. Specifically the phantom shows up if a line is already
> + * high and we change to rising or if a line is already low and we
> + * change to falling but let's be consistent and clear it always.
> + *
> + * Doing this works because we have IRQCHIP_SET_TYPE_MASKED so the
> + * interrupt will be cleared before the rest of the system sees it.
> + */
> + if (old_pdc_type != pdc_type)
> + irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
nit: s/0/false/.
You could also make it conditional on the parent side having been
successful.
And while we're looking at this: do you need to rollback the PDC state
if the GIC side has failed? It's all very hypothetical, but just in
case...
> +
> + return ret;
> }
>
> static struct irq_chip qcom_pdc_gic_chip = {
It otherwise looks sensible. Is that a fix for 5.10?
M.
--
Jazz is not dead. It just smells funny...
On 2020-11-24 10:37, Maulik Shah wrote:
[...]
>> static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>> unsigned function,
>> unsigned group)
>> {
>> struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>> + struct gpio_chip *gc = &pctrl->chip;
>> + unsigned int irq = irq_find_mapping(gc->irq.domain, group);
>> const struct msm_pingroup *g;
>> unsigned long flags;
>> u32 val, mask;
>> + u32 oldval;
>> + u32 old_i;
>> int i;
>> g = &pctrl->soc->groups[group];
>> @@ -187,15 +215,26 @@ static int msm_pinmux_set_mux(struct pinctrl_dev
>> *pctldev,
>> if (WARN_ON(i == g->nfuncs))
>> return -EINVAL;
>> - raw_spin_lock_irqsave(&pctrl->lock, flags);
>> + disable_irq(irq);
>> - val = msm_readl_ctl(pctrl, g);
>> + raw_spin_lock_irqsave(&pctrl->lock, flags);
>> + oldval = val = msm_readl_ctl(pctrl, g);
>> val &= ~mask;
>> val |= i << g->mux_bit;
>> msm_writel_ctl(val, pctrl, g);
>> -
>> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>> + /*
>> + * Clear IRQs if switching to/from GPIO mode since muxing to/from
>> + * the GPIO path can cause phantom edges.
>> + */
>> + old_i = (oldval & mask) >> g->mux_bit;
>> + if (old_i != i &&
>> + (i == pctrl->soc->gpio_func || old_i == pctrl->soc->gpio_func))
>> + msm_pinctrl_clear_pending_irq(pctrl, group, irq);
>
> disable_irq() and enable_irq() should be moved inside this if loop. as
> only use for this is to mask the IRQ when switching back to gpio IRQ
> mode?
>
> i also don't think we should leave IRQ enabled at the end of this
> function by default, probably need to check if IRQ was already
> unmasked before disabling it, then only call enable_irq().
Why? It looks to me that this reproduces the behaviour of
IRQCHIP_SET_TYPE_MASKED, which is highly desirable. What
problem are you trying to address with this?
>
>> +
>> + enable_irq(irq);
>> +
>> return 0;
>> }
>> @@ -456,32 +495,45 @@ static const struct pinconf_ops
>> msm_pinconf_ops = {
>> static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned
>> offset)
>> {
>> const struct msm_pingroup *g;
>> + unsigned int irq = irq_find_mapping(chip->irq.domain, offset);
>> struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
>> unsigned long flags;
>> + u32 oldval;
>> u32 val;
>> g = &pctrl->soc->groups[offset];
>> + disable_irq(irq);
>> +
>> raw_spin_lock_irqsave(&pctrl->lock, flags);
>> - val = msm_readl_ctl(pctrl, g);
>> + oldval = val = msm_readl_ctl(pctrl, g);
>> val &= ~BIT(g->oe_bit);
>> msm_writel_ctl(val, pctrl, g);
>> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>> + if (oldval != val)
>> + msm_pinctrl_clear_pending_irq(pctrl, offset, irq);
>> +
>> + enable_irq(irq);
>
> i do not think we need disable_irq() and enable_irq() here, changing
> direction to input does not mean its being used for interrupt only, it
> may be set to use something like Rx mode in UART.
>
> the client driver should enable IRQ when needed.
And the kernel doesn't expect random interrupts to fire. Again, what
are you trying to fix by removing these?
M.
--
Jazz is not dead. It just smells funny...
Hi Marc,
On 11/24/2020 4:45 PM, Marc Zyngier wrote:
> On 2020-11-24 10:37, Maulik Shah wrote:
>
> [...]
>
>>> static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>>> unsigned function,
>>> unsigned group)
>>> {
>>> struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>>> + struct gpio_chip *gc = &pctrl->chip;
>>> + unsigned int irq = irq_find_mapping(gc->irq.domain, group);
>>> const struct msm_pingroup *g;
>>> unsigned long flags;
>>> u32 val, mask;
>>> + u32 oldval;
>>> + u32 old_i;
>>> int i;
>>> g = &pctrl->soc->groups[group];
>>> @@ -187,15 +215,26 @@ static int msm_pinmux_set_mux(struct
>>> pinctrl_dev *pctldev,
>>> if (WARN_ON(i == g->nfuncs))
>>> return -EINVAL;
>>> - raw_spin_lock_irqsave(&pctrl->lock, flags);
>>> + disable_irq(irq);
>>> - val = msm_readl_ctl(pctrl, g);
>>> + raw_spin_lock_irqsave(&pctrl->lock, flags);
>>> + oldval = val = msm_readl_ctl(pctrl, g);
>>> val &= ~mask;
>>> val |= i << g->mux_bit;
>>> msm_writel_ctl(val, pctrl, g);
>>> -
>>> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>>> + /*
>>> + * Clear IRQs if switching to/from GPIO mode since muxing to/from
>>> + * the GPIO path can cause phantom edges.
>>> + */
>>> + old_i = (oldval & mask) >> g->mux_bit;
>>> + if (old_i != i &&
>>> + (i == pctrl->soc->gpio_func || old_i ==
>>> pctrl->soc->gpio_func))
>>> + msm_pinctrl_clear_pending_irq(pctrl, group, irq);
>>
>> disable_irq() and enable_irq() should be moved inside this if loop. as
>> only use for this is to mask the IRQ when switching back to gpio IRQ
>> mode?
>>
>> i also don't think we should leave IRQ enabled at the end of this
>> function by default, probably need to check if IRQ was already
>> unmasked before disabling it, then only call enable_irq().
>
> Why? It looks to me that this reproduces the behaviour of
> IRQCHIP_SET_TYPE_MASKED, which is highly desirable. What
> problem are you trying to address with this?
Correct, here trying to reproduce the behaviour of
IRQCHIP_SET_TYPE_MASKED which i guess is ok once its moved inside if
loop as this is the place its switching to IRQ mode.
but there is a problem to leave it enabled at the end of set_direction
callbacks, see below.
>
>>
>>> +
>>> + enable_irq(irq);
>>> +
>>> return 0;
>>> }
>>> @@ -456,32 +495,45 @@ static const struct pinconf_ops
>>> msm_pinconf_ops = {
>>> static int msm_gpio_direction_input(struct gpio_chip *chip,
>>> unsigned offset)
>>> {
>>> const struct msm_pingroup *g;
>>> + unsigned int irq = irq_find_mapping(chip->irq.domain, offset);
>>> struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
>>> unsigned long flags;
>>> + u32 oldval;
>>> u32 val;
>>> g = &pctrl->soc->groups[offset];
>>> + disable_irq(irq);
>>> +
>>> raw_spin_lock_irqsave(&pctrl->lock, flags);
>>> - val = msm_readl_ctl(pctrl, g);
>>> + oldval = val = msm_readl_ctl(pctrl, g);
>>> val &= ~BIT(g->oe_bit);
>>> msm_writel_ctl(val, pctrl, g);
>>> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>>> + if (oldval != val)
>>> + msm_pinctrl_clear_pending_irq(pctrl, offset, irq);
>>> +
>>> + enable_irq(irq);
>>
>> i do not think we need disable_irq() and enable_irq() here, changing
>> direction to input does not mean its being used for interrupt only, it
>> may be set to use something like Rx mode in UART.
>>
>> the client driver should enable IRQ when needed.
>
> And the kernel doesn't expect random interrupts to fire. Again, what
> are you trying to fix by removing these?
I see leaving IRQ enabled here can cause problems. For example in
qcom_geni_serial.c driver before requesting IRQ, it sets the
IRQ_NOAUTOEN flag to not keep it enabled.
see the below snippet
irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
ret = devm_request_irq(uport->dev, uport->irq,
qcom_geni_serial_isr,
IRQF_TRIGGER_HIGH, port->name, uport);
later when this devm_request_irq() invokes .irq_request_resources
callback it will reach msm_gpio_irq_reqres() from
where msm_gpio_direction_input() is called which leaves the irq enabled
at the end with enable_irq() which was not expected by driver.
It will cause is IRQ storm since the UART geni driver uses GPIO in Rx
mode when out of suspend. The IRQ mode in GPIO is enabled
with suspend entry only. During resume the IRQ will again be disabled
and GPIO will be switched to Rx mode.
Thanks,
Maulik
>
> M.
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi,
On Tue, Nov 24, 2020 at 12:28 AM Linus Walleij <[email protected]> wrote:
>
> On Tue, Nov 24, 2020 at 1:02 AM Douglas Anderson <[email protected]> wrote:
>
> > We have a problem if we use gpio-keys and configure wakeups such that
> > we only want one edge to wake us up. AKA:
> > wakeup-event-action = <EV_ACT_DEASSERTED>;
> > wakeup-source;
>
> I would need Marc's ACK to apply this with the other patches
> to the pinctrl tree, but I can't really see if maybe it is OK to
> apply it separately?
I'll make an explicit note after the cut in the patch, but to also
respond here: we can apply this patch on its own. The only reason I
sent as one series is because they address similar issues, this patch
stands on its own. Patch #3 needs #2 but patch #2/#3 don't need patch
#1.
> Also are these patches supposed to all go in as fixes or
> for v5.11?
Wherever it makes sense.
-Doug
>Hi,
On Tue, Nov 24, 2020 at 2:37 AM Maulik Shah <[email protected]> wrote:
>
> > +static void msm_pinctrl_clear_pending_irq(struct msm_pinctrl *pctrl,
> > + unsigned int group,
> > + unsigned int irq)
> > +{
> > + struct irq_data *d = irq_get_irq_data(irq);
> > + const struct msm_pingroup *g;
> > + unsigned long flags;
> > + u32 val;
> > +
> > + if (!d)
> > + return;
> > +
> > + if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
> > + irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
> > +
>
> can you add return here if IRQ has parent PDC?
Sure, done.
> also replace 0 with false as Marc suggested in patch 1 of this series.
Thanks for the reminder.
> > @@ -187,15 +215,26 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> > if (WARN_ON(i == g->nfuncs))
> > return -EINVAL;
> >
> > - raw_spin_lock_irqsave(&pctrl->lock, flags);
> > + disable_irq(irq);
> >
> > - val = msm_readl_ctl(pctrl, g);
> > + raw_spin_lock_irqsave(&pctrl->lock, flags);
> > + oldval = val = msm_readl_ctl(pctrl, g);
> > val &= ~mask;
> > val |= i << g->mux_bit;
> > msm_writel_ctl(val, pctrl, g);
> > -
> > raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> >
> > + /*
> > + * Clear IRQs if switching to/from GPIO mode since muxing to/from
> > + * the GPIO path can cause phantom edges.
> > + */
> > + old_i = (oldval & mask) >> g->mux_bit;
> > + if (old_i != i &&
> > + (i == pctrl->soc->gpio_func || old_i == pctrl->soc->gpio_func))
> > + msm_pinctrl_clear_pending_irq(pctrl, group, irq);
>
> disable_irq() and enable_irq() should be moved inside this if loop. as
> only use for this is to mask the IRQ when switching back to gpio IRQ mode?
That totally breaks things. Specifically the glitch actually
introduced above when we write the mux. If the interrupt wasn't
disabled then we'd race our Ack-ing of it with it firing, right? So
the disable _has_ to be above the mux change.
> i also don't think we should leave IRQ enabled at the end of this
> function by default, probably need to check if IRQ was already unmasked
> before disabling it, then only call enable_irq().
Marc already replied, but that's not how it works. disable_irq() and
enable_irq() are reference counted, so as long as we match them then
we're leaving the state exactly the same as when we started, right?
To enumerate the possibilities:
a) If the GPIO wasn't configured as an interrupt at all, the
disable/enable are no-ops.
b) If the interrupt was already disabled, we'll increase the reference
count and then decrease it at the end.
c) If the interrupt wasn't already disabled, we'll disable it and then
re-enable it.
> > @@ -456,32 +495,45 @@ static const struct pinconf_ops msm_pinconf_ops = {
> > static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> > {
> > const struct msm_pingroup *g;
> > + unsigned int irq = irq_find_mapping(chip->irq.domain, offset);
> > struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
> > unsigned long flags;
> > + u32 oldval;
> > u32 val;
> >
> > g = &pctrl->soc->groups[offset];
> >
> > + disable_irq(irq);
> > +
> > raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> > - val = msm_readl_ctl(pctrl, g);
> > + oldval = val = msm_readl_ctl(pctrl, g);
> > val &= ~BIT(g->oe_bit);
> > msm_writel_ctl(val, pctrl, g);
> >
> > raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> >
> > + if (oldval != val)
> > + msm_pinctrl_clear_pending_irq(pctrl, offset, irq);
> > +
> > + enable_irq(irq);
>
> i do not think we need disable_irq() and enable_irq() here, changing
> direction to input does not mean its being used for interrupt only, it
> may be set to use something like Rx mode in UART.
>
> the client driver should enable IRQ when needed.
Check the implementation of disable_irq() and enable_irq(). They are
silent no-ops if the interrupt isn't setup, so this doesn't hurt.
If we don't need the disable_irq() and enable_irq() here then we also
don't need to clear the pending irq. If we need to clear the pending
irq then we need them to avoid the interrupt handler getting called.
> > @@ -774,7 +831,7 @@ static void msm_gpio_irq_mask(struct irq_data *d)
> > raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> > }
> >
> > -static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
> > +static void msm_gpio_irq_unmask(struct irq_data *d)
> > {
> > struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> > @@ -792,17 +849,6 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
> >
> > raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> > - if (status_clear) {
> > - /*
> > - * clear the interrupt status bit before unmask to avoid
> > - * any erroneous interrupts that would have got latched
> > - * when the interrupt is not in use.
> > - */
> can you please carry this comment in msm_pinctrl_clear_pending_irq()?
I could, but as per the big long description of this patch I think
that comment is wrong / misleading. It implies that the interrupt
controller was somehow paying attention even when the input was muxed
away (or similar). After my recent tests I don't believe that's
actually what was happening. I believe the glitches you were trying
to clear here were actually introduced by changing the muxing.
> > @@ -815,14 +861,10 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
> >
> > static void msm_gpio_irq_enable(struct irq_data *d)
> > {
> > - struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > - struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> > -
> > if (d->parent_data)
> > irq_chip_enable_parent(d);
> >
> > - if (!test_bit(d->hwirq, pctrl->skip_wake_irqs))
> > - msm_gpio_irq_clear_unmask(d, true);
> > + msm_gpio_irq_unmask(d);
> > }
> >
> > static void msm_gpio_irq_disable(struct irq_data *d)
> > @@ -837,11 +879,6 @@ static void msm_gpio_irq_disable(struct irq_data *d)
> > msm_gpio_irq_mask(d);
> > }
> >
> > -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.
> > @@ -1097,19 +1134,6 @@ static int msm_gpio_irq_reqres(struct irq_data *d)
> > ret = -EINVAL;
> > goto out;
> > }
> > -
> > - /*
> > - * Clear the interrupt that may be pending before we enable
> > - * the line.
> > - * This is especially a problem with the GPIOs routed to the
> > - * PDC. These GPIOs are direct-connect interrupts to the GIC.
> > - * Disabling the interrupt line at the PDC does not prevent
> > - * the interrupt from being latched at the GIC. The state at
> > - * GIC needs to be cleared before enabling.
> > - */
> can you please carry this comment in msm_pinctrl_clear_pending_irq()?
I also think this comment is wrong / misleading. Specifically:
1. I added a test patch to remux a line away from GPIO mode.
2. I toggled this line back and forth.
3. I then muxed it back.
4. I did not see any indication that the GIC was observing the line
when it was muxed away. No interrupts were pending until I muxed it
back and caused the glitch. Also: muxing it back could cause a glitch
regardless of whether I toggled it.
I will certainly admit that I could have messed up in my testing. If
you have tested this yourself and you're certain that there is a case
where that comment is correct then please let me know and I'll
re-test. One of the test patches I used when poking at this can be
found at <https://crrev.com/c/2556012>.
-Doug
On 2020-11-24 12:43, Maulik Shah wrote:
> Hi Marc,
>
> On 11/24/2020 4:45 PM, Marc Zyngier wrote:
>> On 2020-11-24 10:37, Maulik Shah wrote:
>>
>> [...]
>>
>>>> static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
>>>> unsigned function,
>>>> unsigned group)
>>>> {
>>>> struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>>>> + struct gpio_chip *gc = &pctrl->chip;
>>>> + unsigned int irq = irq_find_mapping(gc->irq.domain, group);
>>>> const struct msm_pingroup *g;
>>>> unsigned long flags;
>>>> u32 val, mask;
>>>> + u32 oldval;
>>>> + u32 old_i;
>>>> int i;
>>>> g = &pctrl->soc->groups[group];
>>>> @@ -187,15 +215,26 @@ static int msm_pinmux_set_mux(struct
>>>> pinctrl_dev *pctldev,
>>>> if (WARN_ON(i == g->nfuncs))
>>>> return -EINVAL;
>>>> - raw_spin_lock_irqsave(&pctrl->lock, flags);
>>>> + disable_irq(irq);
>>>> - val = msm_readl_ctl(pctrl, g);
>>>> + raw_spin_lock_irqsave(&pctrl->lock, flags);
>>>> + oldval = val = msm_readl_ctl(pctrl, g);
>>>> val &= ~mask;
>>>> val |= i << g->mux_bit;
>>>> msm_writel_ctl(val, pctrl, g);
>>>> -
>>>> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>>>> + /*
>>>> + * Clear IRQs if switching to/from GPIO mode since muxing
>>>> to/from
>>>> + * the GPIO path can cause phantom edges.
>>>> + */
>>>> + old_i = (oldval & mask) >> g->mux_bit;
>>>> + if (old_i != i &&
>>>> + (i == pctrl->soc->gpio_func || old_i ==
>>>> pctrl->soc->gpio_func))
>>>> + msm_pinctrl_clear_pending_irq(pctrl, group, irq);
>>>
>>> disable_irq() and enable_irq() should be moved inside this if loop.
>>> as
>>> only use for this is to mask the IRQ when switching back to gpio IRQ
>>> mode?
>>>
>>> i also don't think we should leave IRQ enabled at the end of this
>>> function by default, probably need to check if IRQ was already
>>> unmasked before disabling it, then only call enable_irq().
>>
>> Why? It looks to me that this reproduces the behaviour of
>> IRQCHIP_SET_TYPE_MASKED, which is highly desirable. What
>> problem are you trying to address with this?
>
> Correct, here trying to reproduce the behaviour of
> IRQCHIP_SET_TYPE_MASKED which i guess is ok once its moved inside if
> loop as this is the place its switching to IRQ mode.
>
> but there is a problem to leave it enabled at the end of set_direction
> callbacks, see below.
>
>>
>>>
>>>> +
>>>> + enable_irq(irq);
>>>> +
>>>> return 0;
>>>> }
>>>> @@ -456,32 +495,45 @@ static const struct pinconf_ops
>>>> msm_pinconf_ops = {
>>>> static int msm_gpio_direction_input(struct gpio_chip *chip,
>>>> unsigned offset)
>>>> {
>>>> const struct msm_pingroup *g;
>>>> + unsigned int irq = irq_find_mapping(chip->irq.domain, offset);
>>>> struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
>>>> unsigned long flags;
>>>> + u32 oldval;
>>>> u32 val;
>>>> g = &pctrl->soc->groups[offset];
>>>> + disable_irq(irq);
>>>> +
>>>> raw_spin_lock_irqsave(&pctrl->lock, flags);
>>>> - val = msm_readl_ctl(pctrl, g);
>>>> + oldval = val = msm_readl_ctl(pctrl, g);
>>>> val &= ~BIT(g->oe_bit);
>>>> msm_writel_ctl(val, pctrl, g);
>>>> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>>>> + if (oldval != val)
>>>> + msm_pinctrl_clear_pending_irq(pctrl, offset, irq);
>>>> +
>>>> + enable_irq(irq);
>>>
>>> i do not think we need disable_irq() and enable_irq() here, changing
>>> direction to input does not mean its being used for interrupt only,
>>> it
>>> may be set to use something like Rx mode in UART.
>>>
>>> the client driver should enable IRQ when needed.
>>
>> And the kernel doesn't expect random interrupts to fire. Again, what
>> are you trying to fix by removing these?
>
> I see leaving IRQ enabled here can cause problems. For example in
> qcom_geni_serial.c driver before requesting IRQ, it sets the
> IRQ_NOAUTOEN flag to not keep it enabled.
>
> see the below snippet
> irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
> ret = devm_request_irq(uport->dev, uport->irq,
> qcom_geni_serial_isr,
> IRQF_TRIGGER_HIGH, port->name, uport);
>
> later when this devm_request_irq() invokes .irq_request_resources
> callback it will reach msm_gpio_irq_reqres() from
> where msm_gpio_direction_input() is called which leaves the irq
> enabled at the end with enable_irq() which was not expected by driver.
No it doesn't. disable_irq()/enable_irq() are designed to nest.
If the interrupt line was disabled before the disable/enable
sequence, it will still be disabled after.
> It will cause is IRQ storm since the UART geni driver uses GPIO in Rx
> mode when out of suspend. The IRQ mode in GPIO is enabled
> with suspend entry only. During resume the IRQ will again be disabled
> and GPIO will be switched to Rx mode.
I don't see how this contradicts what is above. If the interrupt was
disabled before hitting this sequence, it will still be disabled after.
Am I missing something? Have you actually seen the problem on HW?
M.
--
Jazz is not dead. It just smells funny...
Hi,
On Tue, Nov 24, 2020 at 9:42 AM Maulik Shah <[email protected]> wrote:
>
> Hi Linus,
>
> + * When we change types the PDC can give a phantom interrupt.
> + * Clear it. Specifically the phantom shows up if a line is already
> + * high and we change to rising or if a line is already low and we
> + * change to falling but let's be consistent and clear it always.
> + *
>
> Can you please hold this change. I am checking with HW folks if above
> commented behaviour is expected/is valid case to set the irq type rising
> edge when the line is already high.
>
> Will keep posting update here.
>
> Thanks,
> Maulik
Thanks for the update. I'm still going to post a v2 because I think
patch #1 in the series should land and it seems nice to keep them
together. I'll add a note to the patch indicating your request to
wait for an ack before landing.
-Doug
Hi Linus,
+ * When we change types the PDC can give a phantom interrupt.
+ * Clear it. Specifically the phantom shows up if a line is already
+ * high and we change to rising or if a line is already low and we
+ * change to falling but let's be consistent and clear it always.
+ *
Can you please hold this change. I am checking with HW folks if above
commented behaviour is expected/is valid case to set the irq type rising
edge when the line is already high.
Will keep posting update here.
Thanks,
Maulik
On 11/24/2020 10:25 PM, Doug Anderson wrote:
> Hi,
>
> On Tue, Nov 24, 2020 at 12:28 AM Linus Walleij <[email protected]> wrote:
>> On Tue, Nov 24, 2020 at 1:02 AM Douglas Anderson <[email protected]> wrote:
>>
>>> We have a problem if we use gpio-keys and configure wakeups such that
>>> we only want one edge to wake us up. AKA:
>>> wakeup-event-action = <EV_ACT_DEASSERTED>;
>>> wakeup-source;
>> I would need Marc's ACK to apply this with the other patches
>> to the pinctrl tree, but I can't really see if maybe it is OK to
>> apply it separately?
> I'll make an explicit note after the cut in the patch, but to also
> respond here: we can apply this patch on its own. The only reason I
> sent as one series is because they address similar issues, this patch
> stands on its own. Patch #3 needs #2 but patch #2/#3 don't need patch
> #1.
>
>> Also are these patches supposed to all go in as fixes or
>> for v5.11?
> Wherever it makes sense.
>
> -Doug
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi Doug,
Thanks for the patch. Looks good to me and tested.
Reviewed-by: Maulik Shah <[email protected]>
Tested-by: Maulik Shah <[email protected]>
Thanks,
Maulik
On 11/24/2020 5:31 AM, Douglas Anderson wrote:
> We have a problem if we use gpio-keys and configure wakeups such that
> we only want one edge to wake us up. AKA:
> wakeup-event-action = <EV_ACT_DEASSERTED>;
> wakeup-source;
>
> Specifically we end up with a phantom interrupt that blocks suspend if
> the line was already high and we want wakeups on rising edges (AKA we
> want the GPIO to go low and then high again before we wake up). The
> opposite is also problematic.
>
> Specifically, here's what's happening today:
> 1. Normally, gpio-keys configures to look for both edges. Due to the
> current workaround introduced in commit c3c0c2e18d94 ("pinctrl:
> qcom: Handle broken/missing PDC dual edge IRQs on sc7180"), if the
> line was high we'd configure for falling edges.
> 2. At suspend time, we change to look for rising edges.
> 3. After qcom_pdc_gic_set_type() runs, we get a phantom interrupt.
>
> We can solve this by just clearing the phantom interrupt.
>
> NOTE: it is possible that this could cause problems for a client with
> very specific needs, but there's not much we can do with this
> hardware. As an example, let's say the interrupt signal is currently
> high and the client is looking for falling edges. The client now
> changes to look for rising edges. The client could possibly expect
> that if the line has a short pulse low (and back high) that it would
> always be detected. Specifically no matter when the pulse happened,
> it should either have tripped the (old) falling edge trigger or the
> (new) rising edge trigger. We will simply not trip it. We could
> narrow down the race a bit by polling our parent before changing
> types, but no matter what we do there will still be a period of time
> where we can't tell the difference between a real transition (or more
> than one transition) and the phantom.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> drivers/irqchip/qcom-pdc.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index bd39e9de6ecf..7d097164aadc 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -159,6 +159,8 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
> {
> int pin_out = d->hwirq;
> enum pdc_irq_config_bits pdc_type;
> + enum pdc_irq_config_bits old_pdc_type;
> + int ret;
>
> if (pin_out == GPIO_NO_WAKE_IRQ)
> return 0;
> @@ -187,9 +189,24 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
> return -EINVAL;
> }
>
> + old_pdc_type = pdc_reg_read(IRQ_i_CFG, pin_out);
> pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
>
> - return irq_chip_set_type_parent(d, type);
> + ret = irq_chip_set_type_parent(d, type);
> +
> + /*
> + * When we change types the PDC can give a phantom interrupt.
> + * Clear it. Specifically the phantom shows up if a line is already
> + * high and we change to rising or if a line is already low and we
> + * change to falling but let's be consistent and clear it always.
> + *
> + * Doing this works because we have IRQCHIP_SET_TYPE_MASKED so the
> + * interrupt will be cleared before the rest of the system sees it.
> + */
> + if (old_pdc_type != pdc_type)
> + irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
> +
> + return ret;
> }
>
> static struct irq_chip qcom_pdc_gic_chip = {
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
On Tue, Nov 24, 2020 at 1:02 AM Douglas Anderson <[email protected]> wrote:
> We have a problem if we use gpio-keys and configure wakeups such that
> we only want one edge to wake us up. AKA:
> wakeup-event-action = <EV_ACT_DEASSERTED>;
> wakeup-source;
I would need Marc's ACK to apply this with the other patches
to the pinctrl tree, but I can't really see if maybe it is OK to
apply it separately?
Also are these patches supposed to all go in as fixes or
for v5.11?
Yours,
Linus Walleij
Hi Doug,
On 11/24/2020 5:31 AM, Douglas Anderson wrote:
> Conceptually, we can envision the input on Qualcomm SoCs to pass
> through a bunch of blocks between coming into the chip and becoming a
> GPIO interrupt. From guessing and running a handful of tests, I
> believe that we can represent the state of the world with a drawing
> that looks something like this:
>
> +-----------------+ +-----------------+ +-----------------+
> | INPUT | --> | PINMUX | | IS_INPUT |
> +-----------------+ | | --> | |
> | output bogus (?)| | output bogus (?)|
> | if not muxed | | if input disab. |
> +-----------------+ +-----------------+
> |
> +---------------------------------------------------+--> to PDC
> |
> V
> +-----------------+ +-----------------+ +-----------------+
> | INTR RAW ENABLE | | DETECTION LOGIC | | STATUS REGISTER |
> | | | | | |
> | output bogus (?)| --> | maybe handles | | latches inputs |
> | if disabled | | polarity diffs | --> | that are high |
> +-----------------+ | | | |
> | maybe debounces | | write 1 to clr |
> | level irqs | +-----------------+
> +-----------------+ |
> |
> +---------------------------------------------------+
> |
> V
> +-----------------+
> | ENABLE |
> | | +-----------------+
> | nothing passes | --> | SUMMARY IRQ |
> | through if | +-----------------+
> | disabled |
> +-----------------+
>
> The above might not be 100% exact, but for the purpose of this
> discussion, the point is that there are a whole bunch of gates and
> transformations on the input before it gets to the circuitry that
> generates interrupts.
>
> As you might guess, if you reconfigure one of the gates in the above
> diagram while the system is configured to detect interrupts things get
> a little wacky. Specifically it appears that if you gate the input at
> any step it can cause various glitches in the later steps because they
> are still paying attention to their input but their input isn't really
> sane anymore.
>
> I did some poking and I found that I could generate bogus interrupts
> in the system both when muxing away from GPIO mode and also when
> muxing back to GPIO mode. When configured to use the PDC path for
> generating interrupts I found that if the external input on the GPIO
> was low that I'd get what looked like a rising edge when unmuxing and
> a falling edge when muxing. When configured away from the PDC path I
> got slightly different glitch interrupts when changing muxing.
>
> These glitches when remuxing matter in reality, not just in theory.
> To be concrete, let's take the special "wakeup_irq" in
> qcom_geni_serial.c as an example. In sc7180-trogdor.dtsi we configure
> the uart3 to have two pinctrl states, sleep and default, and mux
> between the two during runtime PM and system suspend (see
> geni_se_resources_{on,off}() for more details). The difference
> between the sleep and default state is that the RX pin is muxed to a
> GPIO during sleep and muxed to the UART otherwise. When we switch
> between these two states we can generate the glitches talked about
> above because we're configured to look for edges but the transition
> from the gated input (which is bogus) to the real input can look like
> an edge.
>
> Historically the UART case above was handled by the fact that the
> "enable" function in the MSM GPIO driver did an "unmask and clear".
> This relied on the fact that the system happens to have the interrupt
> disabled until suspend time and that it would enable it after the
> pinmux change happened, thus clearing the interrupt.
>
> The historical solution, however, had a few problems. The first
> problem (that nobody seemed to have tripped) is that we can still get
> bogus interrupts if we remux when the interrupt isn't disabled during
> the muxing and re-enabled after. The second problem is that it
> violates how I believe that the interrupt enable path is supposed to
> work.
>
> In Linux, if a driver does disable_irq() and later does enable_irq()
> on its interrupt, I believe it's expecting these properties:
> * If an interrupt was pending when the driver disabled then it will
> still be pending after the driver re-enables.
> * If an edge-triggered interrupt comes in while an interrupt is
> disabled it should assert when the interrupt is re-enabled.
>
> If you think that the above sounds a lot like the disable_irq() and
> enable_irq() are supposed to be masking/unmasking the interrupt
> instead of disabling/enabling it then you've made an astute
> observation. Specifically when talking about interrupts, "mask"
> usually means to stop posting interrupts but keep tracking them and
> "disable" means to fully shut off interrupt detection. It's
> unfortunate that this is so confusing, but presumably this is all the
> way it is for historical reasons.
>
> Perhaps more confusing than the above is that, even though clients of
> IRQs themselves don't have a way to request mask/unmask
> vs. disable/enable calls, IRQ chips themselves can implement both.
> ...and yet more confusing is that if an IRQ chip implements
> disable/enable then they will be called when a client driver calls
> disable_irq() / enable_irq().
>
> It does feel like some of the above could be cleared up. However,
> without any other core interrupt changes it should be clear that when
> an IRQ chip gets a request to "disable" an IRQ that it has to treat it
> like a mask of that IRQ.
>
> In any case, after that long interlude you can see that the "unmask
> and clear" can break things. Maulik tried to fix it so that we no
> longer did "unmask and clear" in commit 71266d9d3936 ("pinctrl: qcom:
> Move clearing pending IRQ to .irq_request_resources callback"), but it
> didn't work for two reasons:
> * It only tried to address the problem for interrupts that had parents
> (like the PDC).
> * It regressed the problem that the original clearing was trying to
> solve.
>
> I think we can safely assume that if someone muxes a pin to be
> something other than a GPIO and then muxes it back that we can clear
> any interrupts that were pending on it without violating any
> assumptions that client drivers are making. Presumably the client
> drivers are intentionally remuxing the pin away from a dedicated
> purpose to be a plain GPIO so they don't care what the pin state was
> before the mux switch and they don't expect to see the pin change
> level during this switch. Let's move the clearing of the IRQ to the
> pin muxing routine so that we'll clear a pending IRQ if we're muxing
> from some non-GPIO mode to a GPIO mode.
>
> Fixes: 71266d9d3936 ("pinctrl: qcom: Move clearing pending IRQ to .irq_request_resources callback")
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
> This is a pretty hairy little patch and presumably needs a good amount
> of testing / discussion before landing. If this patch is totally
> broken / wrong feel free to consider it as an RFC and suggest how it
> should be better.
>
> Also note: I wanted to put this in the same series as patch #1, but
> IMO that patch can stand on its own. If it looks Ok but we want to
> have lots of debate about this one, please land patch #1 on its own
> and we can split the series.
>
> I have done most of this patch testing on the Chrome OS 5.4 kernel
> tree (with many backports) but have sanity checked it on mainline.
>
> drivers/pinctrl/qcom/pinctrl-msm.c | 104 ++++++++++++++++++-----------
> 1 file changed, 64 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 588df91274e2..e7c3927c7d54 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -166,14 +166,42 @@ static int msm_get_function_groups(struct pinctrl_dev *pctldev,
> return 0;
> }
>
> +static void msm_pinctrl_clear_pending_irq(struct msm_pinctrl *pctrl,
> + unsigned int group,
> + unsigned int irq)
> +{
> + struct irq_data *d = irq_get_irq_data(irq);
> + const struct msm_pingroup *g;
> + unsigned long flags;
> + u32 val;
> +
> + if (!d)
> + return;
> +
> + if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
> + irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
> +
can you add return here if IRQ has parent PDC?
also replace 0 with false as Marc suggested in patch 1 of this series.
if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs)) {
irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
return;
}
> + g = &pctrl->soc->groups[group];
> +
> + raw_spin_lock_irqsave(&pctrl->lock, flags);
> + val = msm_readl_intr_status(pctrl, g);
> + val &= ~BIT(g->intr_status_bit);
> + msm_writel_intr_status(val, pctrl, g);
> + raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +}
> +
> static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> unsigned function,
> unsigned group)
> {
> struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> + struct gpio_chip *gc = &pctrl->chip;
> + unsigned int irq = irq_find_mapping(gc->irq.domain, group);
> const struct msm_pingroup *g;
> unsigned long flags;
> u32 val, mask;
> + u32 oldval;
> + u32 old_i;
> int i;
>
> g = &pctrl->soc->groups[group];
> @@ -187,15 +215,26 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
> if (WARN_ON(i == g->nfuncs))
> return -EINVAL;
>
> - raw_spin_lock_irqsave(&pctrl->lock, flags);
> + disable_irq(irq);
>
> - val = msm_readl_ctl(pctrl, g);
> + raw_spin_lock_irqsave(&pctrl->lock, flags);
> + oldval = val = msm_readl_ctl(pctrl, g);
> val &= ~mask;
> val |= i << g->mux_bit;
> msm_writel_ctl(val, pctrl, g);
> -
> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>
> + /*
> + * Clear IRQs if switching to/from GPIO mode since muxing to/from
> + * the GPIO path can cause phantom edges.
> + */
> + old_i = (oldval & mask) >> g->mux_bit;
> + if (old_i != i &&
> + (i == pctrl->soc->gpio_func || old_i == pctrl->soc->gpio_func))
> + msm_pinctrl_clear_pending_irq(pctrl, group, irq);
disable_irq() and enable_irq() should be moved inside this if loop. as
only use for this is to mask the IRQ when switching back to gpio IRQ mode?
i also don't think we should leave IRQ enabled at the end of this
function by default, probably need to check if IRQ was already unmasked
before disabling it, then only call enable_irq().
> +
> + enable_irq(irq);
> +
> return 0;
> }
>
> @@ -456,32 +495,45 @@ static const struct pinconf_ops msm_pinconf_ops = {
> static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> {
> const struct msm_pingroup *g;
> + unsigned int irq = irq_find_mapping(chip->irq.domain, offset);
> struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
> unsigned long flags;
> + u32 oldval;
> u32 val;
>
> g = &pctrl->soc->groups[offset];
>
> + disable_irq(irq);
> +
> raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> - val = msm_readl_ctl(pctrl, g);
> + oldval = val = msm_readl_ctl(pctrl, g);
> val &= ~BIT(g->oe_bit);
> msm_writel_ctl(val, pctrl, g);
>
> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>
> + if (oldval != val)
> + msm_pinctrl_clear_pending_irq(pctrl, offset, irq);
> +
> + enable_irq(irq);
i do not think we need disable_irq() and enable_irq() here, changing
direction to input does not mean its being used for interrupt only, it
may be set to use something like Rx mode in UART.
the client driver should enable IRQ when needed.
> +
> return 0;
> }
>
> static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, int value)
> {
> const struct msm_pingroup *g;
> + unsigned int irq = irq_find_mapping(chip->irq.domain, offset);
> struct msm_pinctrl *pctrl = gpiochip_get_data(chip);
> unsigned long flags;
> + u32 oldval;
> u32 val;
>
> g = &pctrl->soc->groups[offset];
>
> + disable_irq(irq);
> +
> raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> val = msm_readl_io(pctrl, g);
> @@ -491,12 +543,17 @@ static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, in
> val &= ~BIT(g->out_bit);
> msm_writel_io(val, pctrl, g);
>
> - val = msm_readl_ctl(pctrl, g);
> + oldval = msm_readl_ctl(pctrl, g);
> val |= BIT(g->oe_bit);
> msm_writel_ctl(val, pctrl, g);
>
> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>
> + if (oldval != val)
> + msm_pinctrl_clear_pending_irq(pctrl, offset, irq);
> +
> + enable_irq(irq);
> +
same as above, we changing the direction to output so should not
enable_irq().
> return 0;
> }
>
> @@ -774,7 +831,7 @@ static void msm_gpio_irq_mask(struct irq_data *d)
> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> }
>
> -static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
> +static void msm_gpio_irq_unmask(struct irq_data *d)
> {
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> @@ -792,17 +849,6 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
>
> raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> - if (status_clear) {
> - /*
> - * clear the interrupt status bit before unmask to avoid
> - * any erroneous interrupts that would have got latched
> - * when the interrupt is not in use.
> - */
can you please carry this comment in msm_pinctrl_clear_pending_irq()?
> - val = msm_readl_intr_status(pctrl, g);
> - val &= ~BIT(g->intr_status_bit);
> - msm_writel_intr_status(val, pctrl, g);
> - }
> -
> val = msm_readl_intr_cfg(pctrl, g);
> val |= BIT(g->intr_raw_status_bit);
> val |= BIT(g->intr_enable_bit);
> @@ -815,14 +861,10 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d, bool status_clear)
>
> static void msm_gpio_irq_enable(struct irq_data *d)
> {
> - struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> - struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> -
> if (d->parent_data)
> irq_chip_enable_parent(d);
>
> - if (!test_bit(d->hwirq, pctrl->skip_wake_irqs))
> - msm_gpio_irq_clear_unmask(d, true);
> + msm_gpio_irq_unmask(d);
> }
>
> static void msm_gpio_irq_disable(struct irq_data *d)
> @@ -837,11 +879,6 @@ static void msm_gpio_irq_disable(struct irq_data *d)
> msm_gpio_irq_mask(d);
> }
>
> -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.
> @@ -1097,19 +1134,6 @@ static int msm_gpio_irq_reqres(struct irq_data *d)
> ret = -EINVAL;
> goto out;
> }
> -
> - /*
> - * Clear the interrupt that may be pending before we enable
> - * the line.
> - * This is especially a problem with the GPIOs routed to the
> - * PDC. These GPIOs are direct-connect interrupts to the GIC.
> - * Disabling the interrupt line at the PDC does not prevent
> - * the interrupt from being latched at the GIC. The state at
> - * GIC needs to be cleared before enabling.
> - */
can you please carry this comment in msm_pinctrl_clear_pending_irq()?
> - if (d->parent_data && test_bit(d->hwirq, pctrl->skip_wake_irqs))
> - irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
> -
> return 0;
> out:
> module_put(gc->owner);
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi,
On Tue, Nov 24, 2020 at 1:00 AM Marc Zyngier <[email protected]> wrote:
>
> > @@ -187,9 +189,24 @@ static int qcom_pdc_gic_set_type(struct irq_data
> > *d, unsigned int type)
> > return -EINVAL;
> > }
> >
> > + old_pdc_type = pdc_reg_read(IRQ_i_CFG, pin_out);
> > pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
> >
> > - return irq_chip_set_type_parent(d, type);
> > + ret = irq_chip_set_type_parent(d, type);
> > +
> > + /*
> > + * When we change types the PDC can give a phantom interrupt.
> > + * Clear it. Specifically the phantom shows up if a line is already
> > + * high and we change to rising or if a line is already low and we
> > + * change to falling but let's be consistent and clear it always.
> > + *
> > + * Doing this works because we have IRQCHIP_SET_TYPE_MASKED so the
> > + * interrupt will be cleared before the rest of the system sees it.
> > + */
> > + if (old_pdc_type != pdc_type)
> > + irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
>
> nit: s/0/false/.
I'll fix this.
> You could also make it conditional on the parent side having been
> successful.
Good idea.
> And while we're looking at this: do you need to rollback the PDC state
> if the GIC side has failed? It's all very hypothetical, but just in
> case...
I'm going to go ahead and say "no", but I'll make this change if you
insist. Specifically:
* We're still leaving things in a self-consistent state if we fail to
clear the parent, we'll just get a spurious interrupt. It won't cause
any crashes / hangs / whatever.
* Since it seems very unlikely we'd ever trip this and if we ever do
it's not the end of the world, I'd rather not add extra code.
Hopefully that's OK.
-Doug