| Message ID | 20230918122411.237635-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Fix IRQ storm with GPIO interrupts | expand |
On Mon, 18 Sep 2023 13:24:10 +0100, Biju Das <biju.das.jz@bp.renesas.com> wrote: > > As per RZ/G2L hardware manual Rev.1.30 section 8.8.3 Precaution when > changing interrupt settings, we need to mask the interrupts for > any changes in below settings: > > * When changing the noise filter settings. > * When switching the GPIO pins to IRQ or GPIOINT. > * When changing the source of TINT. > * When changing the interrupt detection method. > > This patch masks the interrupts when there is a change in the interrupt > detection method and changing the source of TINT. > > Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver") > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > --- > drivers/irqchip/irq-renesas-rzg2l.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c > index 2cee5477be6b..33a22bafedcd 100644 > --- a/drivers/irqchip/irq-renesas-rzg2l.c > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > @@ -116,11 +116,13 @@ static void rzg2l_irqc_irq_disable(struct irq_data *d) > u8 tssr_index = TSSR_INDEX(offset); > u32 reg; > > + irq_chip_mask_parent(d); > raw_spin_lock(&priv->lock); > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > reg &= ~(TSSEL_MASK << TSSEL_SHIFT(tssr_offset)); > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > raw_spin_unlock(&priv->lock); > + irq_chip_unmask_parent(d); What guarantees that the parent irqchip state has been correctly restored? Nothing refcounts the nesting of mask/unmask. > } > irq_chip_disable_parent(d); I'd rather you start by *disabling* the parent, and then none of that matters at all. > } > @@ -137,11 +139,13 @@ static void rzg2l_irqc_irq_enable(struct irq_data *d) > u8 tssr_index = TSSR_INDEX(offset); > u32 reg; > > + irq_chip_mask_parent(d); > raw_spin_lock(&priv->lock); > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > reg |= (TIEN | tint) << TSSEL_SHIFT(tssr_offset); > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > raw_spin_unlock(&priv->lock); > + irq_chip_unmask_parent(d); > } > irq_chip_enable_parent(d); Same thing: if the parent was disabled, why do we need to do anything? > } > @@ -226,10 +230,12 @@ static int rzg2l_irqc_set_type(struct irq_data *d, unsigned int type) > unsigned int hw_irq = irqd_to_hwirq(d); > int ret = -EINVAL; > > + irq_chip_mask_parent(d); > if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT) > ret = rzg2l_irq_set_type(d, type); > else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) > ret = rzg2l_tint_set_edge(d, type); > + irq_chip_unmask_parent(d); This one is the only interesting one: why don't you mask the interrupt at the local level rather than on the parent? And this should be conditioned on the interrupt state itself (enabled or disabled), not done unconditionally. Thanks, M.
Hi Marc Zyngier, Thanks for the feedback. > Subject: Re: [PATCH 2/3] irqchip: renesas-rzg2l: Mask interrupts for > changing interrupt settings > > On Mon, 18 Sep 2023 13:24:10 +0100, > Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > > As per RZ/G2L hardware manual Rev.1.30 section 8.8.3 Precaution when > > changing interrupt settings, we need to mask the interrupts for any > > changes in below settings: > > > > * When changing the noise filter settings. > > * When switching the GPIO pins to IRQ or GPIOINT. > > * When changing the source of TINT. > > * When changing the interrupt detection method. > > > > This patch masks the interrupts when there is a change in the > > interrupt detection method and changing the source of TINT. > > > > Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller > > driver") > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > --- > > drivers/irqchip/irq-renesas-rzg2l.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c > > b/drivers/irqchip/irq-renesas-rzg2l.c > > index 2cee5477be6b..33a22bafedcd 100644 > > --- a/drivers/irqchip/irq-renesas-rzg2l.c > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > > @@ -116,11 +116,13 @@ static void rzg2l_irqc_irq_disable(struct irq_data > *d) > > u8 tssr_index = TSSR_INDEX(offset); > > u32 reg; > > > > + irq_chip_mask_parent(d); > > raw_spin_lock(&priv->lock); > > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > > reg &= ~(TSSEL_MASK << TSSEL_SHIFT(tssr_offset)); > > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > > raw_spin_unlock(&priv->lock); > > + irq_chip_unmask_parent(d); > > What guarantees that the parent irqchip state has been correctly restored? > Nothing refcounts the nesting of mask/unmask. > > > } > > irq_chip_disable_parent(d); > > I'd rather you start by *disabling* the parent, and then none of that > matters at all. Agreed. Will do this in next version. > > > } > > @@ -137,11 +139,13 @@ static void rzg2l_irqc_irq_enable(struct irq_data > *d) > > u8 tssr_index = TSSR_INDEX(offset); > > u32 reg; > > > > + irq_chip_mask_parent(d); > > raw_spin_lock(&priv->lock); > > reg = readl_relaxed(priv->base + TSSR(tssr_index)); > > reg |= (TIEN | tint) << TSSEL_SHIFT(tssr_offset); > > writel_relaxed(reg, priv->base + TSSR(tssr_index)); > > raw_spin_unlock(&priv->lock); > > + irq_chip_unmask_parent(d); > > } > > irq_chip_enable_parent(d); > > Same thing: if the parent was disabled, why do we need to do anything? OK. It is not required. > > > > } > > @@ -226,10 +230,12 @@ static int rzg2l_irqc_set_type(struct irq_data *d, > unsigned int type) > > unsigned int hw_irq = irqd_to_hwirq(d); > > int ret = -EINVAL; > > > > + irq_chip_mask_parent(d); > > if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT) > > ret = rzg2l_irq_set_type(d, type); > > else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) > > ret = rzg2l_tint_set_edge(d, type); > > + irq_chip_unmask_parent(d); > > This one is the only interesting one: why don't you mask the interrupt at > the local level rather than on the parent? And this should be conditioned > on the interrupt state itself (enabled or disabled), not done > unconditionally. OK. Will do this locally by conditioned on the interrupt state. Cheers, Biju
diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c index 2cee5477be6b..33a22bafedcd 100644 --- a/drivers/irqchip/irq-renesas-rzg2l.c +++ b/drivers/irqchip/irq-renesas-rzg2l.c @@ -116,11 +116,13 @@ static void rzg2l_irqc_irq_disable(struct irq_data *d) u8 tssr_index = TSSR_INDEX(offset); u32 reg; + irq_chip_mask_parent(d); raw_spin_lock(&priv->lock); reg = readl_relaxed(priv->base + TSSR(tssr_index)); reg &= ~(TSSEL_MASK << TSSEL_SHIFT(tssr_offset)); writel_relaxed(reg, priv->base + TSSR(tssr_index)); raw_spin_unlock(&priv->lock); + irq_chip_unmask_parent(d); } irq_chip_disable_parent(d); } @@ -137,11 +139,13 @@ static void rzg2l_irqc_irq_enable(struct irq_data *d) u8 tssr_index = TSSR_INDEX(offset); u32 reg; + irq_chip_mask_parent(d); raw_spin_lock(&priv->lock); reg = readl_relaxed(priv->base + TSSR(tssr_index)); reg |= (TIEN | tint) << TSSEL_SHIFT(tssr_offset); writel_relaxed(reg, priv->base + TSSR(tssr_index)); raw_spin_unlock(&priv->lock); + irq_chip_unmask_parent(d); } irq_chip_enable_parent(d); } @@ -226,10 +230,12 @@ static int rzg2l_irqc_set_type(struct irq_data *d, unsigned int type) unsigned int hw_irq = irqd_to_hwirq(d); int ret = -EINVAL; + irq_chip_mask_parent(d); if (hw_irq >= IRQC_IRQ_START && hw_irq <= IRQC_IRQ_COUNT) ret = rzg2l_irq_set_type(d, type); else if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) ret = rzg2l_tint_set_edge(d, type); + irq_chip_unmask_parent(d); if (ret) return ret;
As per RZ/G2L hardware manual Rev.1.30 section 8.8.3 Precaution when changing interrupt settings, we need to mask the interrupts for any changes in below settings: * When changing the noise filter settings. * When switching the GPIO pins to IRQ or GPIOINT. * When changing the source of TINT. * When changing the interrupt detection method. This patch masks the interrupts when there is a change in the interrupt detection method and changing the source of TINT. Fixes: 3fed09559cd8 ("irqchip: Add RZ/G2L IA55 Interrupt Controller driver") Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Tested-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> --- drivers/irqchip/irq-renesas-rzg2l.c | 6 ++++++ 1 file changed, 6 insertions(+)