| Message ID | 20190829181203.2660-3-ilina@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | qcom: support wakeup capable GPIOs | expand |
Quoting Lina Iyer (2019-08-29 11:11:51) > When an interrupt is to be serviced, the convention is to mask the > interrupt at the chip and unmask after servicing the interrupt. Enabling > and disabling the interrupt at the PDC irqchip causes an interrupt storm > due to the way dual edge interrupts are handled in hardware. > > Skip configuring the PDC when the IRQ is masked and unmasked, instead > use the irq_enable/irq_disable callbacks to toggle the IRQ_ENABLE > register at the PDC. The PDC's IRQ_ENABLE register is only used during > the monitoring mode when the system is asleep and is not needed for > active mode detection. I think this is saying that we want to always let the line be sent through the PDC to the parent irqchip, in this case GIC, so that we don't get an interrupt storm for dual edge interrupts? Why does dual edge interrupts cause a problem? > > Signed-off-by: Lina Iyer <ilina@codeaurora.org>
On Thu, Sep 05 2019 at 18:39 -0600, Stephen Boyd wrote: >Quoting Lina Iyer (2019-08-29 11:11:51) >> When an interrupt is to be serviced, the convention is to mask the >> interrupt at the chip and unmask after servicing the interrupt. Enabling >> and disabling the interrupt at the PDC irqchip causes an interrupt storm >> due to the way dual edge interrupts are handled in hardware. >> >> Skip configuring the PDC when the IRQ is masked and unmasked, instead >> use the irq_enable/irq_disable callbacks to toggle the IRQ_ENABLE >> register at the PDC. The PDC's IRQ_ENABLE register is only used during >> the monitoring mode when the system is asleep and is not needed for >> active mode detection. > >I think this is saying that we want to always let the line be sent >through the PDC to the parent irqchip, in this case GIC, so that we >don't get an interrupt storm for dual edge interrupts? Why does dual >edge interrupts cause a problem? > I am not sure about the hardware details, but the PDC designers did not expect enable and disable to be called whenever the interrupt is handled. This specially becomes a problem for dual edge interrupts which seems to generate a interrupt storm when enabled/disabled while handling the interrupt. --Lina
Quoting Lina Iyer (2019-09-11 09:15:57) > On Thu, Sep 05 2019 at 18:39 -0600, Stephen Boyd wrote: > >Quoting Lina Iyer (2019-08-29 11:11:51) > >> When an interrupt is to be serviced, the convention is to mask the > >> interrupt at the chip and unmask after servicing the interrupt. Enabling > >> and disabling the interrupt at the PDC irqchip causes an interrupt storm > >> due to the way dual edge interrupts are handled in hardware. > >> > >> Skip configuring the PDC when the IRQ is masked and unmasked, instead > >> use the irq_enable/irq_disable callbacks to toggle the IRQ_ENABLE > >> register at the PDC. The PDC's IRQ_ENABLE register is only used during > >> the monitoring mode when the system is asleep and is not needed for > >> active mode detection. > > > >I think this is saying that we want to always let the line be sent > >through the PDC to the parent irqchip, in this case GIC, so that we > >don't get an interrupt storm for dual edge interrupts? Why does dual > >edge interrupts cause a problem? > > > I am not sure about the hardware details, but the PDC designers did not > expect enable and disable to be called whenever the interrupt is > handled. This specially becomes a problem for dual edge interrupts which > seems to generate a interrupt storm when enabled/disabled while handling > the interrupt. > Ok. I just wanted to confirm that masking "doesn't matter" to the PDC because it assumes the irqchip closer to the CPU will be able to mask it anyway. Is that right?
On Fri, Sep 20 2019 at 16:22 -0600, Stephen Boyd wrote: >Quoting Lina Iyer (2019-09-11 09:15:57) >> On Thu, Sep 05 2019 at 18:39 -0600, Stephen Boyd wrote: >> >Quoting Lina Iyer (2019-08-29 11:11:51) >> >> When an interrupt is to be serviced, the convention is to mask the >> >> interrupt at the chip and unmask after servicing the interrupt. Enabling >> >> and disabling the interrupt at the PDC irqchip causes an interrupt storm >> >> due to the way dual edge interrupts are handled in hardware. >> >> >> >> Skip configuring the PDC when the IRQ is masked and unmasked, instead >> >> use the irq_enable/irq_disable callbacks to toggle the IRQ_ENABLE >> >> register at the PDC. The PDC's IRQ_ENABLE register is only used during >> >> the monitoring mode when the system is asleep and is not needed for >> >> active mode detection. >> > >> >I think this is saying that we want to always let the line be sent >> >through the PDC to the parent irqchip, in this case GIC, so that we >> >don't get an interrupt storm for dual edge interrupts? Why does dual >> >edge interrupts cause a problem? >> > >> I am not sure about the hardware details, but the PDC designers did not >> expect enable and disable to be called whenever the interrupt is >> handled. This specially becomes a problem for dual edge interrupts which >> seems to generate a interrupt storm when enabled/disabled while handling >> the interrupt. >> > >Ok. I just wanted to confirm that masking "doesn't matter" to the PDC >because it assumes the irqchip closer to the CPU will be able to mask it >anyway. Is that right? > That is correct.
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c index faa7d61b9d6c..338fae604af5 100644 --- a/drivers/irqchip/qcom-pdc.c +++ b/drivers/irqchip/qcom-pdc.c @@ -63,15 +63,25 @@ static void pdc_enable_intr(struct irq_data *d, bool on) raw_spin_unlock(&pdc_lock); } -static void qcom_pdc_gic_mask(struct irq_data *d) +static void qcom_pdc_gic_disable(struct irq_data *d) { pdc_enable_intr(d, false); + irq_chip_disable_parent(d); +} + +static void qcom_pdc_gic_enable(struct irq_data *d) +{ + pdc_enable_intr(d, true); + irq_chip_enable_parent(d); +} + +static void qcom_pdc_gic_mask(struct irq_data *d) +{ irq_chip_mask_parent(d); } static void qcom_pdc_gic_unmask(struct irq_data *d) { - pdc_enable_intr(d, true); irq_chip_unmask_parent(d); } @@ -148,6 +158,8 @@ static struct irq_chip qcom_pdc_gic_chip = { .irq_eoi = irq_chip_eoi_parent, .irq_mask = qcom_pdc_gic_mask, .irq_unmask = qcom_pdc_gic_unmask, + .irq_disable = qcom_pdc_gic_disable, + .irq_enable = qcom_pdc_gic_enable, .irq_retrigger = irq_chip_retrigger_hierarchy, .irq_set_type = qcom_pdc_gic_set_type, .flags = IRQCHIP_MASK_ON_SUSPEND |
When an interrupt is to be serviced, the convention is to mask the interrupt at the chip and unmask after servicing the interrupt. Enabling and disabling the interrupt at the PDC irqchip causes an interrupt storm due to the way dual edge interrupts are handled in hardware. Skip configuring the PDC when the IRQ is masked and unmasked, instead use the irq_enable/irq_disable callbacks to toggle the IRQ_ENABLE register at the PDC. The PDC's IRQ_ENABLE register is only used during the monitoring mode when the system is asleep and is not needed for active mode detection. Signed-off-by: Lina Iyer <ilina@codeaurora.org> --- drivers/irqchip/qcom-pdc.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)