From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [PATCH V1 09/15] spmi: pmic-arb: check apid enabled before calling the handler Date: Fri, 16 Jun 2017 14:11:14 -0700 Message-ID: <20170616211114.GM20170@codeaurora.org> References: <1496147943-25822-1-git-send-email-kgunda@codeaurora.org> <1496147943-25822-10-git-send-email-kgunda@codeaurora.org> <20170531203909.GG20170@codeaurora.org> <09e72f239b5cbf615ab828a32f34f9b5@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <09e72f239b5cbf615ab828a32f34f9b5@codeaurora.org> Sender: linux-kernel-owner@vger.kernel.org To: kgunda@codeaurora.org Cc: Abhijeet Dharmapurikar , David Collins , Subbaraman Narayanamurthy , Christophe JAILLET , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, adharmap@quicinc.com, aghayal@qti.qualcomm.com, linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.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 > >> > >>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 > >>Signed-off-by: Kiran Gunda > >>--- > >> 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