From mboxrd@z Thu Jan 1 00:00:00 1970 From: kgunda@codeaurora.org Subject: Re: [PATCH V1 09/15] spmi: pmic-arb: check apid enabled before calling the handler Date: Wed, 21 Jun 2017 10:32:56 +0530 Message-ID: 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> <20170616211114.GM20170@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:35438 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750876AbdFUFC5 (ORCPT ); Wed, 21 Jun 2017 01:02:57 -0400 In-Reply-To: <20170616211114.GM20170@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Stephen Boyd 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 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 >> >> >> >>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. > 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.