RE: [PATCH 5/6] irqchip: Add RZ/V2H(P) Interrupt Control Unit (ICU) driver

[Fabrizio Castro]

Hi Thomas,

thanks for your feedback!

> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Sent: Wednesday, October 2, 2024 11:51 AM
> Subject: Re: [PATCH 5/6] irqchip: Add RZ/V2H(P) Interrupt Control Unit (ICU) driver
> 
> On Tue, Sep 17 2024 at 18:32, Fabrizio Castro wrote:
> > +
> > +/* DT "interrupts" indexes */
> > +#define ICU_IRQ_START				1
> > +#define ICU_IRQ_COUNT				16
> > +#define ICU_TINT_START				(ICU_IRQ_START + ICU_IRQ_COUNT)
> > +#define ICU_TINT_COUNT				32
> > +#define ICU_NUM_IRQ				(ICU_TINT_START + ICU_TINT_COUNT)
> > +
> > +/* Registers */
> > +#define ICU_NSCNT				0x00
> > +#define ICU_NSCLR				0x04
> > +#define ICU_NITSR				0x08
> > +#define ICU_ISCTR				0x10
> > +#define ICU_ISCLR				0x14
> > +#define ICU_IITSR				0x18
> > +#define ICU_TSCTR				0x20
> > +#define ICU_TSCLR				0x24
> > +#define ICU_TITSR(k)				(0x28 + (k) * 4)
> > +#define ICU_TSSR(k)				(0x30 + (k) * 4)
> > +
> > +/* NMI */
> > +#define ICU_NMI_EDGE_FALLING			0
> > +#define ICU_NMI_EDGE_RISING			1
> > +
> > +#define ICU_NSCNT_NSTAT				BIT(0)
> > +#define ICU_NSCNT_NSTAT_DETECTED		1
> > +
> > +#define ICU_NSCLR_NCLR				BIT(0)
> > +
> > +/* IRQ */
> > +#define ICU_IRQ_LEVEL_LOW			0
> > +#define ICU_IRQ_EDGE_FALLING			1
> > +#define ICU_IRQ_EDGE_RISING			2
> > +#define ICU_IRQ_EDGE_BOTH			3
> > +
> > +#define ICU_IITSR_IITSEL_PREP(iitsel, n)	((iitsel) << ((n) * 2))
> > +#define ICU_IITSR_IITSEL_GET(iitsr, n)		(((iitsr) >> ((n) * 2)) & 0x03)
> > +#define ICU_IITSR_IITSEL_MASK(n)		ICU_IITSR_IITSEL_PREP(0x03, n)
> > +
> > +/* TINT */
> > +#define ICU_TINT_EDGE_RISING			0
> > +#define ICU_TINT_EDGE_FALLING			1
> > +#define ICU_TINT_LEVEL_HIGH			2
> > +#define ICU_TINT_LEVEL_LOW			3
> > +
> > +#define ICU_TSSR_K(tint_nr)			((tint_nr) / 4)
> > +#define ICU_TSSR_TSSEL_N(tint_nr)		((tint_nr) % 4)
> > +#define ICU_TSSR_TSSEL_PREP(tssel, n)		((tssel) << ((n) * 8))
> > +#define ICU_TSSR_TSSEL_MASK(n)			ICU_TSSR_TSSEL_PREP(0x7F, n)
> > +#define ICU_TSSR_TIEN(n)			(BIT(7) << ((n) * 8))
> > +
> > +#define ICU_TITSR_K(tint_nr)			((tint_nr) / 16)
> > +#define ICU_TITSR_TITSEL_N(tint_nr)		((tint_nr) % 16)
> > +#define ICU_TITSR_TITSEL_PREP(titsel, n)	ICU_IITSR_IITSEL_PREP(titsel, n)
> > +#define ICU_TITSR_TITSEL_MASK(n)		ICU_IITSR_IITSEL_MASK(n)
> > +#define ICU_TITSR_TITSEL_GET(titsr, n)		ICU_IITSR_IITSEL_GET(titsr, n)
> > +
> > +#define ICU_TINT_EXTRACT_HWIRQ(x)		FIELD_GET(GENMASK(15, 0), (x))
> > +#define ICU_TINT_EXTRACT_GPIOINT(x)		FIELD_GET(GENMASK(31, 16), (x))
> > +#define ICU_PB5_TINT				0x55
> > +
> > +/**
> > + * struct rzv2h_icu_priv - Interrupt Control Unit controller private data
> > + * structure.
> 
> If you need a line break, then please format it so:
> 
>  * struct rzv2h_icu_priv - Interrupt Control Unit controller private data
>  *			   structure.

Since I have 100 chars, I'll just get rid of this line break.

> 
> This makes it readable.
> 
> > +static void rzv2h_clear_nmi_int(struct rzv2h_icu_priv *priv)
> > +{
> > +	u32 nscnt = readl_relaxed(priv->base + ICU_NSCNT);
> > +
> > +	if ((nscnt & ICU_NSCNT_NSTAT) == ICU_NSCNT_NSTAT_DETECTED)
> > +		writel_relaxed(ICU_NSCLR_NCLR, priv->base + ICU_NSCLR);
> > +}
> > +
> > +static void rzv2h_clear_irq_int(struct rzv2h_icu_priv *priv, unsigned int hwirq)
> > +{
> > +	unsigned int irq_nr = hwirq - ICU_IRQ_START;
> > +	u32 isctr, iitsr, iitsel;
> > +	u32 bit = BIT(irq_nr);
> > +
> > +	isctr = readl_relaxed(priv->base + ICU_ISCTR);
> > +	iitsr = readl_relaxed(priv->base + ICU_IITSR);
> > +	iitsel = ICU_IITSR_IITSEL_GET(iitsr, irq_nr);
> 
> Not that I care about the performance of your device, but why do you
> need to read back the type configuration. It's known and cached in
> irq_data, no?
> 
> Also this is invoked from eoi(), so why would the bit not be set when
> the interrupt is type edge and has fired? It should be set which means
> the ISCTR read is pointless too. Unless I'm missing something,

Both rzv2h_clear_irq_int and rzv2h_clear_tint_int are also called from
the irq_set_type path, that's why all the checks.

As you pointed out, no need for all those checks from within the eoi
path, therefore I'll implement whatever is required straight in
rzv2h_icu_eoi, to improve on performance.

> 
> > +static void rzv2h_clear_tint_int(struct rzv2h_icu_priv *priv,
> > +				 unsigned int hwirq)
> 
> No line break required.

Agreed.

> 
> > +{
> > +	unsigned int tint_nr = hwirq - ICU_TINT_START;
> > +	int titsel_n = ICU_TITSR_TITSEL_N(tint_nr);
> > +	u32 tsctr, titsr, titsel;
> > +	u32 bit = BIT(tint_nr);
> > +	int k = tint_nr / 16;
> > +
> > +	tsctr = readl_relaxed(priv->base + ICU_TSCTR);
> > +	titsr = readl_relaxed(priv->base + ICU_TITSR(k));
> > +	titsel = ICU_TITSR_TITSEL_GET(titsr, titsel_n);
> 
> Same comment.

Agreed.

> 
> > +static void rzv2h_icu_eoi(struct irq_data *d)
> > +{
> > +	struct rzv2h_icu_priv *priv = irq_data_to_priv(d);
> > +	unsigned int hw_irq = irqd_to_hwirq(d);
> > +
> > +	raw_spin_lock(&priv->lock);
> 
>   scoped_guard(raw_spinlock, ....) {

Good point!

> 
> > +	if (hw_irq >= ICU_TINT_START)
> > +		rzv2h_clear_tint_int(priv, hw_irq);
> > +	else if (hw_irq >= ICU_IRQ_START)
> > +		rzv2h_clear_irq_int(priv, hw_irq);
> > +	else
> > +		rzv2h_clear_nmi_int(priv);
> 
> Huch. Is NMI a real NMI or just some unmaskable regular interrupt?
> 
> If it's a real NMI, then you _cannot_ take the spinlock here.

It's not a real NMI, as it's mapped to SPI 0.

> 
> 
> > +	raw_spin_unlock(&priv->lock);
> > +
> > +	irq_chip_eoi_parent(d);
> > +}
> > +
> > +static void rzv2h_tint_irq_endisable(struct irq_data *d, bool enable)
> > +{
> > +	struct rzv2h_icu_priv *priv = irq_data_to_priv(d);
> > +	unsigned int hw_irq = irqd_to_hwirq(d);
> > +	u32 tint_nr, tssel_n, k, tssr;
> > +
> > +	if (hw_irq < ICU_TINT_START)
> > +		return;
> > +
> > +	tint_nr = hw_irq - ICU_TINT_START;
> > +	k = ICU_TSSR_K(tint_nr);
> > +	tssel_n = ICU_TSSR_TSSEL_N(tint_nr);
> > +
> > +	raw_spin_lock(&priv->lock);
> 
> guard()

Agreed.

> 
> > +	tssr = readl_relaxed(priv->base + ICU_TSSR(k));
> > +	if (enable)
> > +		tssr |= ICU_TSSR_TIEN(tssel_n);
> > +	else
> > +		tssr &= ~ICU_TSSR_TIEN(tssel_n);
> > +	writel_relaxed(tssr, priv->base + ICU_TSSR(k));
> > +	raw_spin_unlock(&priv->lock);
> > +}
> 
> > +	raw_spin_lock(&priv->lock);
> 
> guard()

Agreed.

> 
> > +static const struct irq_domain_ops rzv2h_icu_domain_ops = {
> > +	.alloc = rzv2h_icu_alloc,
> > +	.free = irq_domain_free_irqs_common,
> > +	.translate = irq_domain_translate_twocell,
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

Will fix.

> 
> > +};
> > +
> > +static int rzv2h_icu_parse_interrupts(struct rzv2h_icu_priv *priv,
> > +				       struct device_node *np)
> 
> Please get rid of the line breaks. You have 100 characters.

Thanks for pointing this out, I'll go through the whole file adjust accordingly.

I'll send a new version soon.

Thanks,
Fab

> 
> Thanks,
> 
>         tglx