| Message ID | 1496147943-25822-8-git-send-email-kgunda@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On 05/30, Kiran Gunda wrote: > From: Abhijeet Dharmapurikar <adharmap@codeaurora.org> > > PMIC interrupts each have an internal latched status bit which is > not visible from any register. This status bit is set as soon as > the conditions specified in the interrupt type and polarity > registers are met even if the interrupt is not enabled. When it > is set, nothing else changes within the PMIC and no interrupt > notification packets are sent. If the internal latched status > bit is set when an interrupt is enabled, then the value is > immediately propagated into the interrupt latched status register > and an interrupt notification packet is sent out from the PMIC > over SPMI. > > This PMIC hardware behavior can lead to a situation where the > handler for a level triggered interrupt is called immediately > after enable_irq() is called even though the interrupt physically > triggered while it was disabled within the genirq framework. > This situation takes place if the the interrupt fires twice after Double 'the' > calling disable_irq(). The first time it fires, the level flow > handler will mask and disregard it. Unfortunately, the second > time it fires, the internal latched status bit is set within the > PMIC and no further notification is received. because the interrupt has been disabled. > When enable_irq() > is called later, the interrupt is unmasked (enabled in the PMIC) > which results in the PMIC immediately sending an interrupt > notification packet out over SPMI. This breaks the semantics > of level triggered interrupts within the genirq framework since > they should be completely ignored while disabled. Ok. I wonder why the hardware latches interrupts at all. > > The PMIC internal latched status behavior also affects how > interrupts are treated during suspend. While entering suspend, > all interrupts not specified as wakeup mode are masked. Upon > resume, these interrupts are unmasked. Thus if any of the > non-wakeup PMIC interrupts fired while the system was suspended, > then the PMIC will send interrupt notification packets out via > SPMI as soon as they are unmasked during resume. This behavior > violates genirq semantics as well since non-wakeup interrupts > should be completely ignored during suspend. > > Modify the qpnpint_irq_unmask() function so that the interrupt > latched status clear register is written immediately before the > interrupt enable register. This clears the internal latched > status bit of the interrupt so that it cannot trigger spuriously > immediately upon being enabled. > > Also, while resuming an irq, an unmask could be called even if it > was not previously masked. So, before writing these registers, > check if the interrupt is already enabled within the PMIC. If it > is, then no further register writes are required. This > condition check ensures that a valid latched status register bit > is not cleared until it is properly handled. > > Signed-off-by: Abhijeet Dharmapurikar <adharmap@codeaurora.org> > Signed-off-by: Kiran Gunda <kgunda@codeaurora.org> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
On 2017-06-01 03:33, Stephen Boyd wrote: > On 05/30, Kiran Gunda wrote: >> From: Abhijeet Dharmapurikar <adharmap@codeaurora.org> >> >> PMIC interrupts each have an internal latched status bit which is >> not visible from any register. This status bit is set as soon as >> the conditions specified in the interrupt type and polarity >> registers are met even if the interrupt is not enabled. When it >> is set, nothing else changes within the PMIC and no interrupt >> notification packets are sent. If the internal latched status >> bit is set when an interrupt is enabled, then the value is >> immediately propagated into the interrupt latched status register >> and an interrupt notification packet is sent out from the PMIC >> over SPMI. >> >> This PMIC hardware behavior can lead to a situation where the >> handler for a level triggered interrupt is called immediately >> after enable_irq() is called even though the interrupt physically >> triggered while it was disabled within the genirq framework. >> This situation takes place if the the interrupt fires twice after > > Double 'the' > >> calling disable_irq(). The first time it fires, the level flow >> handler will mask and disregard it. Unfortunately, the second >> time it fires, the internal latched status bit is set within the >> PMIC and no further notification is received. > > because the interrupt has been disabled. > >> When enable_irq() >> is called later, the interrupt is unmasked (enabled in the PMIC) >> which results in the PMIC immediately sending an interrupt >> notification packet out over SPMI. This breaks the semantics >> of level triggered interrupts within the genirq framework since >> they should be completely ignored while disabled. > > Ok. I wonder why the hardware latches interrupts at all. > >> >> The PMIC internal latched status behavior also affects how >> interrupts are treated during suspend. While entering suspend, >> all interrupts not specified as wakeup mode are masked. Upon >> resume, these interrupts are unmasked. Thus if any of the >> non-wakeup PMIC interrupts fired while the system was suspended, >> then the PMIC will send interrupt notification packets out via >> SPMI as soon as they are unmasked during resume. This behavior >> violates genirq semantics as well since non-wakeup interrupts >> should be completely ignored during suspend. >> >> Modify the qpnpint_irq_unmask() function so that the interrupt >> latched status clear register is written immediately before the >> interrupt enable register. This clears the internal latched >> status bit of the interrupt so that it cannot trigger spuriously >> immediately upon being enabled. >> >> Also, while resuming an irq, an unmask could be called even if it >> was not previously masked. So, before writing these registers, >> check if the interrupt is already enabled within the PMIC. If it >> is, then no further register writes are required. This >> condition check ensures that a valid latched status register bit >> is not cleared until it is properly handled. >> >> Signed-off-by: Abhijeet Dharmapurikar <adharmap@codeaurora.org> >> Signed-off-by: Kiran Gunda <kgunda@codeaurora.org> > > Reviewed-by: Stephen Boyd <sboyd@codeaurora.org> Thanks for reviewing and acking this. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c index bc03737..1d23df0 100644 --- a/drivers/spmi/spmi-pmic-arb.c +++ b/drivers/spmi/spmi-pmic-arb.c @@ -580,12 +580,22 @@ static void qpnpint_irq_unmask(struct irq_data *d) struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d); u8 irq = d->hwirq >> 8; u8 apid = d->hwirq; - u8 data = BIT(irq); + u8 buf[2]; writel_relaxed(SPMI_PIC_ACC_ENABLE_BIT, pa->intr + pa->ver_ops->acc_enable(apid)); - qpnpint_spmi_write(d, QPNPINT_REG_EN_CLR, &data, 1); + qpnpint_spmi_read(d, QPNPINT_REG_EN_SET, &buf[0], 1); + if (!(buf[0] & BIT(irq))) { + /* + * Since the interrupt is currently disabled, write to both the + * LATCHED_CLR and EN_SET registers so that a spurious interrupt + * cannot be triggered when the interrupt is enabled + */ + buf[0] = BIT(irq); + buf[1] = BIT(irq); + qpnpint_spmi_write(d, QPNPINT_REG_LATCHED_CLR, &buf, 2); + } } static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type)