RE: [PATCH 4/7] irqchip/renesas-rzv2h: Add rzv2h_icu_register_dma_req_ack

[Fabrizio Castro <fabrizio.castro.jz@renesas.com>]

Hi Thomas,

Thank you for your feedback!

> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: 07 February 2025 07:49
> Subject: Re: [PATCH 4/7] irqchip/renesas-rzv2h: Add rzv2h_icu_register_dma_req_ack
> 
> On Thu, Feb 06 2025 at 22:03, Fabrizio Castro wrote:
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#function-references-in-changelogs
> 
> > On the Renesas RZ/V2H(P) family of SoCs, DMAC IPs are connected
> > to the Interrupt Control Unit (ICU).
> > +#define ICU_DMkSELy(k, y)			(0x420 + (k) * 0x20 + (y) * 4)
> > +#define ICU_DMACKSELk(k)			(0x500 + (k) * 4)
> >
> >  /* NMI */
> >  #define ICU_NMI_EDGE_FALLING			0
> > @@ -80,6 +83,19 @@
> >  #define ICU_TINT_EXTRACT_GPIOINT(x)		FIELD_GET(GENMASK(31, 16), (x))
> >  #define ICU_PB5_TINT				0x55
> >
> > +/* DMAC */
> > +#define ICU_DMAC_DkSEL_CLRON_MASK		BIT(15)
> > +#define ICU_DMAC_DkRQ_SEL_MASK			GENMASK(9, 0)
> > +#define ICU_DMAC_DMAREQ_MASK			(ICU_DMAC_DkRQ_SEL_MASK | \
> > +						 ICU_DMAC_DkSEL_CLRON_MASK)
> > +
> > +#define ICU_DMAC_PREP_DkSEL_CLRON(x)		FIELD_PREP(ICU_DMAC_DkSEL_CLRON_MASK, (x))
> > +#define ICU_DMAC_PREP_DkRQ_SEL(x)		FIELD_PREP(ICU_DMAC_DkRQ_SEL_MASK, (x))
> > +#define ICU_DMAC_PREP_DMAREQ(sel, clr)		(ICU_DMAC_PREP_DkRQ_SEL(sel) | \
> > +						ICU_DMAC_PREP_DkSEL_CLRON(clr))
> 
> That's a pretty convoluted way to create a mask whihc has the CLRON bit
> always set to 0 according to the only usage site.

Indeed, it can be simplified.

> 
> > +#define ICU_DMAC_DACK_SEL_MASK			GENMASK(6, 0)
> 
> > +void rzv2h_icu_register_dma_req_ack(struct platform_device *icu_dev, u8 dmac_index, u8
> dmac_channel,
> > +				    u16 req_no, u8 ack_no)
> > +{
> > +	struct rzv2h_icu_priv *priv = platform_get_drvdata(icu_dev);
> > +	u32 icu_dmackselk, dmaack, dmaack_mask;
> > +	u32 icu_dmksely, dmareq, dmareq_mask;
> > +	u8 k, field_no;
> > +	u8 y, upper;
> > +
> > +	if (req_no >= 0x1b5)
> 
> In the DMA part you use proper defines for this, but here you put magic
> numbers into the code. Please share the defines and use them consistently.

Thanks for pointing it out, I'll fix it in v2.

> 
> > +		req_no = RZV2H_ICU_DMAC_REQ_NO_DEFAULT;
> > +
> > +	if (ack_no >= 0x50)
> > +		ack_no = RZV2H_ICU_DMAC_ACK_NO_DEFAULT;
> > +
> > +	y = dmac_channel / 2;
> > +	upper = dmac_channel % 2;
> > +
> > +	dmareq = ICU_DMAC_PREP_DMAREQ(req_no, 0);
> > +	dmareq_mask = ICU_DMAC_DMAREQ_MASK;
> > +
> > +	if (upper) {
> > +		dmareq <<= 16;
> > +		dmareq_mask <<= 16;
> > +	}
> 
> You already have macros for this, so the obvious thing to do is to put
> the shift magic into them:
> 
> /* Two 16 bit fields per register */
> #define ICU_DMAC_DMAREQ_SHIFT(ch)		((ch & 0x01) * 16)
> 
> #define ICU_DMAC_PREP_DMAREQ(sel, ch)		(ICU_DMAC_PREP_DkRQ_SEL(sel)	\
>                                                  << ICU_DMAC_DMAREQ_SHIFT(ch))
> #define ICU_DMAC_DMAREQ_MASK(ch)		(ICU_DMAC_DkRQ_SEL_MASK		\
>                                                  << ICU_DMAC_DMAREQ_SHIFT(ch))
> 
>         dmareq = ICU_DMAC_PREP_DMAREQ(req_no, ch);
>         dmareq_mask = ICU_DMAC_DMAREQ_MASK(ch);

Thank you, I'll adjust accordingly.

> 
> > +	k  = ack_no / 4;
> > +	field_no = ack_no % 4;
> > +
> > +	dmaack_mask = ICU_DMAC_DACK_SEL_MASK << (field_no * 8);
> > +	dmaack = ack_no << (field_no * 8);
> 
> Same here.

Will do.

Cheers,
Fab

> 
> > +	guard(raw_spinlock_irqsave)(&priv->lock);
> > +
> > +	icu_dmksely = readl(priv->base + ICU_DMkSELy(dmac_index, y));
> > +	icu_dmksely = (icu_dmksely & ~dmareq_mask) | dmareq;
> > +	writel(icu_dmksely, priv->base + ICU_DMkSELy(dmac_index, y));
> > +
> > +	icu_dmackselk = readl(priv->base + ICU_DMACKSELk(k));
> > +	icu_dmackselk = (icu_dmackselk & ~dmaack_mask) | dmaack;
> > +	writel(icu_dmackselk, priv->base + ICU_DMACKSELk(k));
> 
> Thanks,
> 
>         tglx