| Message ID | 1496147943-25822-10-git-send-email-kgunda@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
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()? > 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? > > 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. 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?
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. 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. > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)); + if (enable & SPMI_PIC_ACC_ENABLE_BIT) + periph_interrupt(pa, apid); } }