Re: [PATCH V1 09/15] spmi: pmic-arb: check apid enabled before calling the handler

[Stephen Boyd <sboyd@codeaurora.org>]

On 06/14, kgunda@codeaurora.org wrote:
> On 2017-06-01 02:09, Stephen Boyd wrote:
> >On 05/30, Kiran Gunda wrote:
> >>From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> >>
> >>The driver currently invokes the apid handler (periph_handler())
> >
> >You mean periph_interrupt()?
> >
> Yes.
> >>once it sees that the summary status bit for that apid is set.
> >>
> >>However the hardware is designed to set that bit even if the apid
> >>interrupts are disabled. The driver should check whether the apid
> >>is indeed enabled before calling the apid handler.
> >
> >Really? Wow that is awful. Or is this because ACC_ENABLE bit is
> >always set now and never cleared?
> >
> Yes. It is awful. It is not because of the ACC_ENABLE bit is set.
> >>
> >>Signed-off-by: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
> >>Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
> >>---
> >> drivers/spmi/spmi-pmic-arb.c | 10 +++++++---
> >> 1 file changed, 7 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/spmi/spmi-pmic-arb.c
> >>b/drivers/spmi/spmi-pmic-arb.c
> >>index ad34491..f8638fa 100644
> >>--- a/drivers/spmi/spmi-pmic-arb.c
> >>+++ b/drivers/spmi/spmi-pmic-arb.c
> >>@@ -536,8 +536,8 @@ static void pmic_arb_chained_irq(struct
> >>irq_desc *desc)
> >> 	void __iomem *intr = pa->intr;
> >> 	int first = pa->min_apid >> 5;
> >> 	int last = pa->max_apid >> 5;
> >>-	u32 status;
> >>-	int i, id;
> >>+	u32 status, enable;
> >>+	int i, id, apid;
> >>
> >> 	chained_irq_enter(chip, desc);
> >>
> >>@@ -547,7 +547,11 @@ static void pmic_arb_chained_irq(struct
> >>irq_desc *desc)
> >> 		while (status) {
> >> 			id = ffs(status) - 1;
> >> 			status &= ~BIT(id);
> >>-			periph_interrupt(pa, id + i * 32);
> >>+			apid = id + i * 32;
> >>+			enable = readl_relaxed(intr +
> >>+					pa->ver_ops->acc_enable(apid));
> >
> >Do we need to read the hardware to figure this out? After earlier
> >patches in this series we would never clear the
> >SPMI_PIC_ACC_ENABLE_BIT after one of the irqs in a peripheral is
> >unmasked for the first time (which looks to be fixing a bug in
> >the existing driver BTW). So in practice, this should almost
> >always be true.
> >
> yes. We have removed clearing the SPMI_PIC_ACC_ENABLE_BIT from the
> irq_mask,
> because if we disable this BIT it disables all the peripheral IRQs,
> which we don't want.

Right, we could reference count it though and only clear and set
the bit when we mask and unmask the last irq in the peripheral.

> 
> Once the peripheral fires the interrupt the summary status bit for
> that peripheral
> is set even though the SPMI_PIC_ACC_ENABLE_BIT is not enabled.
> That's why we have to
> read this register to not service the interrupt that is not
> requested/enabled yet.
> This SPMI_PIC_ACC_ENABLE_BIT is enabled during the irq_unmask which
> is called from request_irq.

Ok. So this is again about handling the case where an interrupt
is pending out of the bootloader?

> 
> >In the one case that it isn't true, we'll be handling some other
> >irq for another peripheral and then hardware will tell us there's
> >an interrupt for a peripheral that doesn't have any interrupts
> >unmasked. We would call periph_interrupt() and then that
> >shouldn't see any interrupts in the status register for that
> >APID. So we do some more work, but nothing happens still. Did I
> >miss something? What is this fixing?
> 
> Yes. As you said this fixes the issue of calling the periph_interrupt
> for some other irq that is not yet requested and enabled yet.


Hmm. I seemed to miss the fact that periph_interrupt() will see
an unmasked interrupt and think it's valid. I thought that only
SPMI_PIC_ACC_ENABLE_BIT was broken, but you're saying that the
status register for a particular peripheral will always latch
interrupts even if we haven't enabled them?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project