diff mbox series

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

Message ID 20250206220308.76669-5-fabrizio.castro.jz@renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series Add DMAC support to the RZ/V2H(P) | expand

Commit Message

Fabrizio Castro Feb. 6, 2025, 10:03 p.m. UTC 
On the Renesas RZ/V2H(P) family of SoCs, DMAC IPs are connected
to the Interrupt Control Unit (ICU).
For DMA transfers, a request number and an ack number must be
registered with the ICU, which means that the DMAC driver has
to be able to instruct the ICU driver with the registration of
such ids.

Export rzv2h_icu_register_dma_req_ack so that the DMA driver
can register both ids in one go.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
---
 drivers/irqchip/irq-renesas-rzv2h.c       | 61 +++++++++++++++++++++++
 include/linux/irqchip/irq-renesas-rzv2h.h | 19 +++++++
 2 files changed, 80 insertions(+)
 create mode 100644 include/linux/irqchip/irq-renesas-rzv2h.h

Comments

Thomas Gleixner Feb. 7, 2025, 7:49 a.m. UTC | #1 
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
Fabrizio Castro Feb. 7, 2025, 4:27 p.m. UTC | #2 
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
diff mbox series

Patch

diff --git a/drivers/irqchip/irq-renesas-rzv2h.c b/drivers/irqchip/irq-renesas-rzv2h.c
index fe2d29e91026..0fdbf9ce89a9 100644
--- a/drivers/irqchip/irq-renesas-rzv2h.c
+++ b/drivers/irqchip/irq-renesas-rzv2h.c
@@ -15,6 +15,7 @@ 
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/irqchip.h>
+#include <linux/irqchip/irq-renesas-rzv2h.h>
 #include <linux/irqdomain.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
@@ -41,6 +42,8 @@ 
 #define ICU_TSCLR				0x24
 #define ICU_TITSR(k)				(0x28 + (k) * 4)
 #define ICU_TSSR(k)				(0x30 + (k) * 4)
+#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))
+
+#define ICU_DMAC_DACK_SEL_MASK			GENMASK(6, 0)
+
 /**
  * struct rzv2h_icu_priv - Interrupt Control Unit controller private data structure.
  * @base:	Controller's base address
@@ -94,6 +110,50 @@  struct rzv2h_icu_priv {
 	raw_spinlock_t			lock;
 };
 
+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)
+		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;
+	}
+
+	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);
+
+	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));
+}
+EXPORT_SYMBOL_GPL(rzv2h_icu_register_dma_req_ack);
+
 static inline struct rzv2h_icu_priv *irq_data_to_priv(struct irq_data *data)
 {
 	return data->domain->host_data;
@@ -446,6 +506,7 @@  static int rzv2h_icu_init(struct device_node *node, struct device_node *parent)
 		goto put_dev;
 	}
 
+	platform_set_drvdata(pdev, rzv2h_icu_data);
 	rzv2h_icu_data->irqchip = &rzv2h_icu_chip;
 
 	rzv2h_icu_data->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
diff --git a/include/linux/irqchip/irq-renesas-rzv2h.h b/include/linux/irqchip/irq-renesas-rzv2h.h
new file mode 100644
index 000000000000..686d050a239a
--- /dev/null
+++ b/include/linux/irqchip/irq-renesas-rzv2h.h
@@ -0,0 +1,19 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Renesas RZ/V2H(P) Interrupt Control Unit (ICU)
+ *
+ * Copyright (C) 2025 Renesas Electronics Corporation.
+ */
+
+#ifndef __LINUX_IRQ_RENESAS_RZV2H
+#define __LINUX_IRQ_RENESAS_RZV2H
+
+#include <linux/platform_device.h>
+
+#define RZV2H_ICU_DMAC_REQ_NO_DEFAULT		0x3ff
+#define RZV2H_ICU_DMAC_ACK_NO_DEFAULT		0x7f
+
+void rzv2h_icu_register_dma_req_ack(struct platform_device *icu_dev, u8 dmac_index, u8 dmac_channel,
+				    u16 req_no, u8 ack_no);
+
+#endif