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

[Thomas Gleixner <tglx@linutronix.de>]

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.

> +#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.

> +		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);

> +	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.

> +	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