linux-kernel - Re: [PATCH 4/7] irqchip/renesas-rzv2h: Add rzv2h_icu_register_dma_req

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87ed0amby0.ffs@tglx>
Date: Fri, 07 Feb 2025 08:49:27 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Fabrizio Castro <fabrizio.castro.jz@...esas.com>, Geert Uytterhoeven
 <geert+renesas@...der.be>
Cc: Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
 linux-kernel@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>, Lad
 Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
 linux-renesas-soc@...r.kernel.org
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.

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