Messages in this thread | | | Subject | Re: [PATCH 1/3] irqchip: qcom-pdc: Fix phantom irq when changing between rising/falling | From | Maulik Shah <> | Date | Tue, 24 Nov 2020 12:33:36 +0530 |
| |
Hi Doug,
Thanks for the patch. Looks good to me and tested.
Reviewed-by: Maulik Shah <mkshah@codeaurora.org> Tested-by: Maulik Shah <mkshah@codeaurora.org>
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 <dianders@chromium.org> > --- > > 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
| |