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

[kgunda@codeaurora.org]

On 2017-06-17 02:41, Stephen Boyd wrote:
> 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.
> 
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.