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

[kgunda]

On 2017-06-17 02:41, Stephen Boyd wrote:
> On 06/14, kgunda@xxxxxxxxxxxxxx wrote:
> > On 2017-06-01 02:09, Stephen Boyd wrote:
> > >On 05/30, Kiran Gunda wrote:
> > >>From: Abhijeet Dharmapurikar <adharmap@xxxxxxxxxxxxxx>
> > >>
> > >>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@xxxxxxxxxxxxxx>
> > >>Signed-off-by: Kiran Gunda <kgunda@xxxxxxxxxxxxxx>
> > >>---
> > >> 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.
Actually we are disabling the interrupt at peripheral level, so i 
believe
disabling the PIC_ACC_EN_BIT is not mandatory.
> > 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?
Yes.
> > >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?
Yes. Correct. Both SPMI_PIC_OWNERm_ACC_STATUSn and SPMI_PIC_IRQ_STATUSn
are getting set when the peripheral fires the interrupt, even though we
haven't enable that particular peripheral bit in SPMI_PIC_ACC_ENABLEn 
register.