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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > Tested-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> > ---
> > 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
