[PATCH v2 0/4] dmaengine: rcar-dmac: Add support for R-Car V3U

[PATCH v2 0/4] dmaengine: rcar-dmac: Add support for R-Car V3U

[Geert Uytterhoeven]

	Hi Vinod,

This patch series adds support for the Direct Memory Access Controller
variant in the Renesas R-Car V3U (R8A779A0) SoC, to both DT bindings and
driver.

Changes compared to v1:
  - Add Reviewed-by,
  - Put the full loop control of for_each_rcar_dmac_chan() on a single
    line, to improve readability,
  - Use two separate named regions instead of array,
  - Drop rcar_dmac_of_data.chan_reg_block, check for
    !rcar_dmac_of_data.chan_offset_base instead,
  - Precalculate chan_base in rcar_dmac_probe().

This has been tested on the Renesas Falcon board, using external SPI
loopback (spi-loopback-test) on MSIOF1 and MSIOF2.

Thanks!

Geert Uytterhoeven (4):
  dt-bindings: renesas,rcar-dmac: Add r8a779a0 support
  dmaengine: rcar-dmac: Add for_each_rcar_dmac_chan() helper
  dmaengine: rcar-dmac: Add helpers for clearing DMA channel status
  dmaengine: rcar-dmac: Add support for R-Car V3U

 .../bindings/dma/renesas,rcar-dmac.yaml       |  76 ++++++++-----
 drivers/dma/sh/rcar-dmac.c                    | 105 +++++++++++++-----
 2 files changed, 123 insertions(+), 58 deletions(-)

-- 
2.25.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH v2 1/4] dt-bindings: renesas,rcar-dmac: Add r8a779a0 support

[Geert Uytterhoeven]

Document the compatible value for the Direct Memory Access Controller
blocks in the Renesas R-Car V3U (R8A779A0) SoC.

The most visible difference with DMAC blocks on other R-Car SoCs is the
move of the per-channel registers to a separate register block.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v2:
  - Add Reviewed-by.
---
 .../bindings/dma/renesas,rcar-dmac.yaml       | 76 ++++++++++++-------
 1 file changed, 48 insertions(+), 28 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.yaml b/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.yaml
index c07eb6f2fc8d2f12..7f2a54bc732d3a19 100644
--- a/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.yaml
+++ b/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.yaml
@@ -14,34 +14,37 @@ allOf:
 
 properties:
   compatible:
-    items:
-      - enum:
-          - renesas,dmac-r8a7742  # RZ/G1H
-          - renesas,dmac-r8a7743  # RZ/G1M
-          - renesas,dmac-r8a7744  # RZ/G1N
-          - renesas,dmac-r8a7745  # RZ/G1E
-          - renesas,dmac-r8a77470 # RZ/G1C
-          - renesas,dmac-r8a774a1 # RZ/G2M
-          - renesas,dmac-r8a774b1 # RZ/G2N
-          - renesas,dmac-r8a774c0 # RZ/G2E
-          - renesas,dmac-r8a774e1 # RZ/G2H
-          - renesas,dmac-r8a7790  # R-Car H2
-          - renesas,dmac-r8a7791  # R-Car M2-W
-          - renesas,dmac-r8a7792  # R-Car V2H
-          - renesas,dmac-r8a7793  # R-Car M2-N
-          - renesas,dmac-r8a7794  # R-Car E2
-          - renesas,dmac-r8a7795  # R-Car H3
-          - renesas,dmac-r8a7796  # R-Car M3-W
-          - renesas,dmac-r8a77961 # R-Car M3-W+
-          - renesas,dmac-r8a77965 # R-Car M3-N
-          - renesas,dmac-r8a77970 # R-Car V3M
-          - renesas,dmac-r8a77980 # R-Car V3H
-          - renesas,dmac-r8a77990 # R-Car E3
-          - renesas,dmac-r8a77995 # R-Car D3
-      - const: renesas,rcar-dmac
-
-  reg:
-    maxItems: 1
+    oneOf:
+      - items:
+          - enum:
+              - renesas,dmac-r8a7742  # RZ/G1H
+              - renesas,dmac-r8a7743  # RZ/G1M
+              - renesas,dmac-r8a7744  # RZ/G1N
+              - renesas,dmac-r8a7745  # RZ/G1E
+              - renesas,dmac-r8a77470 # RZ/G1C
+              - renesas,dmac-r8a774a1 # RZ/G2M
+              - renesas,dmac-r8a774b1 # RZ/G2N
+              - renesas,dmac-r8a774c0 # RZ/G2E
+              - renesas,dmac-r8a774e1 # RZ/G2H
+              - renesas,dmac-r8a7790  # R-Car H2
+              - renesas,dmac-r8a7791  # R-Car M2-W
+              - renesas,dmac-r8a7792  # R-Car V2H
+              - renesas,dmac-r8a7793  # R-Car M2-N
+              - renesas,dmac-r8a7794  # R-Car E2
+              - renesas,dmac-r8a7795  # R-Car H3
+              - renesas,dmac-r8a7796  # R-Car M3-W
+              - renesas,dmac-r8a77961 # R-Car M3-W+
+              - renesas,dmac-r8a77965 # R-Car M3-N
+              - renesas,dmac-r8a77970 # R-Car V3M
+              - renesas,dmac-r8a77980 # R-Car V3H
+              - renesas,dmac-r8a77990 # R-Car E3
+              - renesas,dmac-r8a77995 # R-Car D3
+          - const: renesas,rcar-dmac
+
+      - items:
+          - const: renesas,dmac-r8a779a0 # R-Car V3U
+
+  reg: true
 
   interrupts:
     minItems: 9
@@ -110,6 +113,23 @@ required:
   - power-domains
   - resets
 
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - renesas,dmac-r8a779a0
+then:
+  properties:
+    reg:
+      items:
+        - description: Base register block
+        - description: Channel register block
+else:
+  properties:
+    reg:
+      maxItems: 1
+
 additionalProperties: false
 
 examples:
-- 
2.25.1

[PATCH v2 2/4] dmaengine: rcar-dmac: Add for_each_rcar_dmac_chan() helper

[Geert Uytterhoeven]

Add and helper macro for iterating over all DMAC channels, taking into
account the channel mask.  Use it where appropriate, to simplify code.

Restore "reverse Christmas tree" order of local variables while adding a
new variable.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Put the full loop control of for_each_rcar_dmac_chan() on a single
    line, to improve readability.
---
 drivers/dma/sh/rcar-dmac.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index a57705356e8bb796..537550b4121bbc22 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -209,6 +209,10 @@ struct rcar_dmac {
 
 #define to_rcar_dmac(d)		container_of(d, struct rcar_dmac, engine)
 
+#define for_each_rcar_dmac_chan(i, chan, dmac)						\
+	for (i = 0, chan = &(dmac)->channels[0]; i < (dmac)->n_channels; i++, chan++)	\
+		if (!((dmac)->channels_mask & BIT(i))) continue; else
+
 /*
  * struct rcar_dmac_of_data - This driver's OF data
  * @chan_offset_base: DMAC channels base offset
@@ -817,15 +821,11 @@ static void rcar_dmac_chan_reinit(struct rcar_dmac_chan *chan)
 
 static void rcar_dmac_stop_all_chan(struct rcar_dmac *dmac)
 {
+	struct rcar_dmac_chan *chan;
 	unsigned int i;
 
 	/* Stop all channels. */
-	for (i = 0; i < dmac->n_channels; ++i) {
-		struct rcar_dmac_chan *chan = &dmac->channels[i];
-
-		if (!(dmac->channels_mask & BIT(i)))
-			continue;
-
+	for_each_rcar_dmac_chan(i, chan, dmac) {
 		/* Stop and reinitialize the channel. */
 		spin_lock_irq(&chan->lock);
 		rcar_dmac_chan_halt(chan);
@@ -1828,9 +1828,10 @@ static int rcar_dmac_probe(struct platform_device *pdev)
 		DMA_SLAVE_BUSWIDTH_2_BYTES | DMA_SLAVE_BUSWIDTH_4_BYTES |
 		DMA_SLAVE_BUSWIDTH_8_BYTES | DMA_SLAVE_BUSWIDTH_16_BYTES |
 		DMA_SLAVE_BUSWIDTH_32_BYTES | DMA_SLAVE_BUSWIDTH_64_BYTES;
+	const struct rcar_dmac_of_data *data;
+	struct rcar_dmac_chan *chan;
 	struct dma_device *engine;
 	struct rcar_dmac *dmac;
-	const struct rcar_dmac_of_data *data;
 	unsigned int i;
 	int ret;
 
@@ -1916,11 +1917,8 @@ static int rcar_dmac_probe(struct platform_device *pdev)
 
 	INIT_LIST_HEAD(&engine->channels);
 
-	for (i = 0; i < dmac->n_channels; ++i) {
-		if (!(dmac->channels_mask & BIT(i)))
-			continue;
-
-		ret = rcar_dmac_chan_probe(dmac, &dmac->channels[i], data, i);
+	for_each_rcar_dmac_chan(i, chan, dmac) {
+		ret = rcar_dmac_chan_probe(dmac, chan, data, i);
 		if (ret < 0)
 			goto error;
 	}
-- 
2.25.1

[PATCH v2 3/4] dmaengine: rcar-dmac: Add helpers for clearing DMA channel status

[Geert Uytterhoeven]

Extract the code to clear the status of one or all channels into their
own helpers, to prepare for the different handling of the R-Car V3U SoC.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - No changes.
---
 drivers/dma/sh/rcar-dmac.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 537550b4121bbc22..7a0f802c61e5152d 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -336,6 +336,17 @@ static void rcar_dmac_chan_write(struct rcar_dmac_chan *chan, u32 reg, u32 data)
 		writel(data, chan->iomem + reg);
 }
 
+static void rcar_dmac_chan_clear(struct rcar_dmac *dmac,
+				 struct rcar_dmac_chan *chan)
+{
+	rcar_dmac_write(dmac, RCAR_DMACHCLR, BIT(chan->index));
+}
+
+static void rcar_dmac_chan_clear_all(struct rcar_dmac *dmac)
+{
+	rcar_dmac_write(dmac, RCAR_DMACHCLR, dmac->channels_mask);
+}
+
 /* -----------------------------------------------------------------------------
  * Initialization and configuration
  */
@@ -451,7 +462,7 @@ static int rcar_dmac_init(struct rcar_dmac *dmac)
 	u16 dmaor;
 
 	/* Clear all channels and enable the DMAC globally. */
-	rcar_dmac_write(dmac, RCAR_DMACHCLR, dmac->channels_mask);
+	rcar_dmac_chan_clear_all(dmac);
 	rcar_dmac_write(dmac, RCAR_DMAOR,
 			RCAR_DMAOR_PRI_FIXED | RCAR_DMAOR_DME);
 
@@ -1566,7 +1577,7 @@ static irqreturn_t rcar_dmac_isr_channel(int irq, void *dev)
 		 * because channel is already stopped in error case.
 		 * We need to clear register and check DE bit as recovery.
 		 */
-		rcar_dmac_write(dmac, RCAR_DMACHCLR, 1 << chan->index);
+		rcar_dmac_chan_clear(dmac, chan);
 		rcar_dmac_chcr_de_barrier(chan);
 		reinit = true;
 		goto spin_lock_end;
-- 
2.25.1

[PATCH v2 4/4] dmaengine: rcar-dmac: Add support for R-Car V3U

[Geert Uytterhoeven]

The DMACs (both SYS-DMAC and RT-DMAC) on R-Car V3U differ slightly from
the DMACs on R-Car Gen2 and other R-Car Gen3 SoCs:
  1. The per-channel registers are located in a second register block.
     Add support for mapping the second block, using the appropriate
     offsets and stride.
  2. The common Channel Clear Register (DMACHCLR) was replaced by a
     per-channel register.
     Update rcar_dmac_chan_clear{,_all}() to handle this.
     As rcar_dmac_init() needs to clear the status before the individual
     channels are probed, channel index and base address initialization
     are moved forward.

Inspired by a patch in the BSP by Phong Hoang
<phong.hoang.wz@renesas.com>.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Use two separate named regions instead of an iomem[] array,
  - Drop rcar_dmac_of_data.chan_reg_block, check for
    !rcar_dmac_of_data.chan_offset_base instead,
  - Precalculate chan_base in rcar_dmac_probe().
---
 drivers/dma/sh/rcar-dmac.c | 74 ++++++++++++++++++++++++++++----------
 1 file changed, 55 insertions(+), 19 deletions(-)

diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
index 7a0f802c61e5152d..d9589eea98083215 100644
--- a/drivers/dma/sh/rcar-dmac.c
+++ b/drivers/dma/sh/rcar-dmac.c
@@ -189,7 +189,8 @@ struct rcar_dmac_chan {
  * struct rcar_dmac - R-Car Gen2 DMA Controller
  * @engine: base DMA engine object
  * @dev: the hardware device
- * @iomem: remapped I/O memory base
+ * @dmac_base: remapped base register block
+ * @chan_base: remapped channel register block (optional)
  * @n_channels: number of available channels
  * @channels: array of DMAC channels
  * @channels_mask: bitfield of which DMA channels are managed by this driver
@@ -198,7 +199,8 @@ struct rcar_dmac_chan {
 struct rcar_dmac {
 	struct dma_device engine;
 	struct device *dev;
-	void __iomem *iomem;
+	void __iomem *dmac_base;
+	void __iomem *chan_base;
 
 	unsigned int n_channels;
 	struct rcar_dmac_chan *channels;
@@ -234,7 +236,7 @@ struct rcar_dmac_of_data {
 #define RCAR_DMAOR_PRI_ROUND_ROBIN	(3 << 8)
 #define RCAR_DMAOR_AE			(1 << 2)
 #define RCAR_DMAOR_DME			(1 << 0)
-#define RCAR_DMACHCLR			0x0080
+#define RCAR_DMACHCLR			0x0080	/* Not on R-Car V3U */
 #define RCAR_DMADPSEC			0x00a0
 
 #define RCAR_DMASAR			0x0000
@@ -297,6 +299,9 @@ struct rcar_dmac_of_data {
 #define RCAR_DMAFIXDAR			0x0014
 #define RCAR_DMAFIXDPBASE		0x0060
 
+/* For R-Car V3U */
+#define RCAR_V3U_DMACHCLR		0x0100
+
 /* Hardcode the MEMCPY transfer size to 4 bytes. */
 #define RCAR_DMAC_MEMCPY_XFER_SIZE	4
 
@@ -307,17 +312,17 @@ struct rcar_dmac_of_data {
 static void rcar_dmac_write(struct rcar_dmac *dmac, u32 reg, u32 data)
 {
 	if (reg == RCAR_DMAOR)
-		writew(data, dmac->iomem + reg);
+		writew(data, dmac->dmac_base + reg);
 	else
-		writel(data, dmac->iomem + reg);
+		writel(data, dmac->dmac_base + reg);
 }
 
 static u32 rcar_dmac_read(struct rcar_dmac *dmac, u32 reg)
 {
 	if (reg == RCAR_DMAOR)
-		return readw(dmac->iomem + reg);
+		return readw(dmac->dmac_base + reg);
 	else
-		return readl(dmac->iomem + reg);
+		return readl(dmac->dmac_base + reg);
 }
 
 static u32 rcar_dmac_chan_read(struct rcar_dmac_chan *chan, u32 reg)
@@ -339,12 +344,23 @@ static void rcar_dmac_chan_write(struct rcar_dmac_chan *chan, u32 reg, u32 data)
 static void rcar_dmac_chan_clear(struct rcar_dmac *dmac,
 				 struct rcar_dmac_chan *chan)
 {
-	rcar_dmac_write(dmac, RCAR_DMACHCLR, BIT(chan->index));
+	if (dmac->chan_base)
+		rcar_dmac_chan_write(chan, RCAR_V3U_DMACHCLR, 1);
+	else
+		rcar_dmac_write(dmac, RCAR_DMACHCLR, BIT(chan->index));
 }
 
 static void rcar_dmac_chan_clear_all(struct rcar_dmac *dmac)
 {
-	rcar_dmac_write(dmac, RCAR_DMACHCLR, dmac->channels_mask);
+	struct rcar_dmac_chan *chan;
+	unsigned int i;
+
+	if (dmac->chan_base) {
+		for_each_rcar_dmac_chan(i, chan, dmac)
+			rcar_dmac_chan_write(chan, RCAR_V3U_DMACHCLR, 1);
+	} else {
+		rcar_dmac_write(dmac, RCAR_DMACHCLR, dmac->channels_mask);
+	}
 }
 
 /* -----------------------------------------------------------------------------
@@ -1744,7 +1760,6 @@ static const struct dev_pm_ops rcar_dmac_pm = {
 
 static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
 				struct rcar_dmac_chan *rchan,
-				const struct rcar_dmac_of_data *data,
 				unsigned int index)
 {
 	struct platform_device *pdev = to_platform_device(dmac->dev);
@@ -1753,9 +1768,6 @@ static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
 	char *irqname;
 	int ret;
 
-	rchan->index = index;
-	rchan->iomem = dmac->iomem + data->chan_offset_base +
-		       data->chan_offset_stride * index;
 	rchan->mid_rid = -EINVAL;
 
 	spin_lock_init(&rchan->lock);
@@ -1842,6 +1854,7 @@ static int rcar_dmac_probe(struct platform_device *pdev)
 	const struct rcar_dmac_of_data *data;
 	struct rcar_dmac_chan *chan;
 	struct dma_device *engine;
+	void __iomem *chan_base;
 	struct rcar_dmac *dmac;
 	unsigned int i;
 	int ret;
@@ -1880,9 +1893,24 @@ static int rcar_dmac_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	/* Request resources. */
-	dmac->iomem = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(dmac->iomem))
-		return PTR_ERR(dmac->iomem);
+	dmac->dmac_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(dmac->dmac_base))
+		return PTR_ERR(dmac->dmac_base);
+
+	if (!data->chan_offset_base) {
+		dmac->chan_base = devm_platform_ioremap_resource(pdev, 1);
+		if (IS_ERR(dmac->chan_base))
+			return PTR_ERR(dmac->chan_base);
+
+		chan_base = dmac->chan_base;
+	} else {
+		chan_base = dmac->dmac_base + data->chan_offset_base;
+	}
+
+	for_each_rcar_dmac_chan(i, chan, dmac) {
+		chan->index = i;
+		chan->iomem = chan_base + i * data->chan_offset_stride;
+	}
 
 	/* Enable runtime PM and initialize the device. */
 	pm_runtime_enable(&pdev->dev);
@@ -1929,7 +1957,7 @@ static int rcar_dmac_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&engine->channels);
 
 	for_each_rcar_dmac_chan(i, chan, dmac) {
-		ret = rcar_dmac_chan_probe(dmac, chan, data, i);
+		ret = rcar_dmac_chan_probe(dmac, chan, i);
 		if (ret < 0)
 			goto error;
 	}
@@ -1977,14 +2005,22 @@ static void rcar_dmac_shutdown(struct platform_device *pdev)
 }
 
 static const struct rcar_dmac_of_data rcar_dmac_data = {
-	.chan_offset_base = 0x8000,
-	.chan_offset_stride = 0x80,
+	.chan_offset_base	= 0x8000,
+	.chan_offset_stride	= 0x80,
+};
+
+static const struct rcar_dmac_of_data rcar_v3u_dmac_data = {
+	.chan_offset_base	= 0x0,
+	.chan_offset_stride	= 0x1000,
 };
 
 static const struct of_device_id rcar_dmac_of_ids[] = {
 	{
 		.compatible = "renesas,rcar-dmac",
 		.data = &rcar_dmac_data,
+	}, {
+		.compatible = "renesas,dmac-r8a779a0",
+		.data = &rcar_v3u_dmac_data,
 	},
 	{ /* Sentinel */ }
 };
-- 
2.25.1

Re: [PATCH v2 2/4] dmaengine: rcar-dmac: Add for_each_rcar_dmac_chan() helper

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On Mon, Jan 25, 2021 at 03:24:29PM +0100, Geert Uytterhoeven wrote:
> Add and helper macro for iterating over all DMAC channels, taking into
> account the channel mask.  Use it where appropriate, to simplify code.
> 
> Restore "reverse Christmas tree" order of local variables while adding a
> new variable.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Looks good and works fine with I2C + DMA on V3U:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/4] dmaengine: rcar-dmac: Add helpers for clearing DMA channel status

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

On Mon, Jan 25, 2021 at 03:24:30PM +0100, Geert Uytterhoeven wrote:
> Extract the code to clear the status of one or all channels into their
> own helpers, to prepare for the different handling of the R-Car V3U SoC.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Looks good and works fine with I2C + DMA on V3U:

Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/4] dmaengine: rcar-dmac: Add support for R-Car V3U

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

On Mon, Jan 25, 2021 at 03:24:31PM +0100, Geert Uytterhoeven wrote:
> The DMACs (both SYS-DMAC and RT-DMAC) on R-Car V3U differ slightly from
> the DMACs on R-Car Gen2 and other R-Car Gen3 SoCs:
>   1. The per-channel registers are located in a second register block.
>      Add support for mapping the second block, using the appropriate
>      offsets and stride.
>   2. The common Channel Clear Register (DMACHCLR) was replaced by a
>      per-channel register.
>      Update rcar_dmac_chan_clear{,_all}() to handle this.
>      As rcar_dmac_init() needs to clear the status before the individual
>      channels are probed, channel index and base address initialization
>      are moved forward.
> 
> Inspired by a patch in the BSP by Phong Hoang
> <phong.hoang.wz@renesas.com>.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Apporach looks good, didn't check the gory details. However, it still
works fine with I2C + DMA on V3U, so:

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] dt-bindings: renesas,rcar-dmac: Add r8a779a0 support

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Mon, Jan 25, 2021 at 03:24:28PM +0100, Geert Uytterhoeven wrote:
> Document the compatible value for the Direct Memory Access Controller
> blocks in the Renesas R-Car V3U (R8A779A0) SoC.
> 
> The most visible difference with DMAC blocks on other R-Car SoCs is the
> move of the per-channel registers to a separate register block.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Reviewed-by: Rob Herring <robh@kernel.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v2:
>   - Add Reviewed-by.
> ---
>  .../bindings/dma/renesas,rcar-dmac.yaml       | 76 ++++++++++++-------
>  1 file changed, 48 insertions(+), 28 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.yaml b/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.yaml
> index c07eb6f2fc8d2f12..7f2a54bc732d3a19 100644
> --- a/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.yaml
> +++ b/Documentation/devicetree/bindings/dma/renesas,rcar-dmac.yaml
> @@ -14,34 +14,37 @@ allOf:
>  
>  properties:
>    compatible:
> -    items:
> -      - enum:
> -          - renesas,dmac-r8a7742  # RZ/G1H
> -          - renesas,dmac-r8a7743  # RZ/G1M
> -          - renesas,dmac-r8a7744  # RZ/G1N
> -          - renesas,dmac-r8a7745  # RZ/G1E
> -          - renesas,dmac-r8a77470 # RZ/G1C
> -          - renesas,dmac-r8a774a1 # RZ/G2M
> -          - renesas,dmac-r8a774b1 # RZ/G2N
> -          - renesas,dmac-r8a774c0 # RZ/G2E
> -          - renesas,dmac-r8a774e1 # RZ/G2H
> -          - renesas,dmac-r8a7790  # R-Car H2
> -          - renesas,dmac-r8a7791  # R-Car M2-W
> -          - renesas,dmac-r8a7792  # R-Car V2H
> -          - renesas,dmac-r8a7793  # R-Car M2-N
> -          - renesas,dmac-r8a7794  # R-Car E2
> -          - renesas,dmac-r8a7795  # R-Car H3
> -          - renesas,dmac-r8a7796  # R-Car M3-W
> -          - renesas,dmac-r8a77961 # R-Car M3-W+
> -          - renesas,dmac-r8a77965 # R-Car M3-N
> -          - renesas,dmac-r8a77970 # R-Car V3M
> -          - renesas,dmac-r8a77980 # R-Car V3H
> -          - renesas,dmac-r8a77990 # R-Car E3
> -          - renesas,dmac-r8a77995 # R-Car D3
> -      - const: renesas,rcar-dmac
> -
> -  reg:
> -    maxItems: 1
> +    oneOf:
> +      - items:
> +          - enum:
> +              - renesas,dmac-r8a7742  # RZ/G1H
> +              - renesas,dmac-r8a7743  # RZ/G1M
> +              - renesas,dmac-r8a7744  # RZ/G1N
> +              - renesas,dmac-r8a7745  # RZ/G1E
> +              - renesas,dmac-r8a77470 # RZ/G1C
> +              - renesas,dmac-r8a774a1 # RZ/G2M
> +              - renesas,dmac-r8a774b1 # RZ/G2N
> +              - renesas,dmac-r8a774c0 # RZ/G2E
> +              - renesas,dmac-r8a774e1 # RZ/G2H
> +              - renesas,dmac-r8a7790  # R-Car H2
> +              - renesas,dmac-r8a7791  # R-Car M2-W
> +              - renesas,dmac-r8a7792  # R-Car V2H
> +              - renesas,dmac-r8a7793  # R-Car M2-N
> +              - renesas,dmac-r8a7794  # R-Car E2
> +              - renesas,dmac-r8a7795  # R-Car H3
> +              - renesas,dmac-r8a7796  # R-Car M3-W
> +              - renesas,dmac-r8a77961 # R-Car M3-W+
> +              - renesas,dmac-r8a77965 # R-Car M3-N
> +              - renesas,dmac-r8a77970 # R-Car V3M
> +              - renesas,dmac-r8a77980 # R-Car V3H
> +              - renesas,dmac-r8a77990 # R-Car E3
> +              - renesas,dmac-r8a77995 # R-Car D3
> +          - const: renesas,rcar-dmac
> +
> +      - items:
> +          - const: renesas,dmac-r8a779a0 # R-Car V3U
> +
> +  reg: true
>  
>    interrupts:
>      minItems: 9
> @@ -110,6 +113,23 @@ required:
>    - power-domains
>    - resets
>  
> +if:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - renesas,dmac-r8a779a0
> +then:
> +  properties:
> +    reg:
> +      items:
> +        - description: Base register block
> +        - description: Channel register block
> +else:
> +  properties:
> +    reg:
> +      maxItems: 1
> +
>  additionalProperties: false
>  
>  examples:

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/4] dmaengine: rcar-dmac: Add for_each_rcar_dmac_chan() helper

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Mon, Jan 25, 2021 at 03:24:29PM +0100, Geert Uytterhoeven wrote:
> Add and helper macro for iterating over all DMAC channels, taking into

s/and helper/a helper/

> account the channel mask.  Use it where appropriate, to simplify code.
> 
> Restore "reverse Christmas tree" order of local variables while adding a
> new variable.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - Put the full loop control of for_each_rcar_dmac_chan() on a single
>     line, to improve readability.
> ---
>  drivers/dma/sh/rcar-dmac.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index a57705356e8bb796..537550b4121bbc22 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -209,6 +209,10 @@ struct rcar_dmac {
>  
>  #define to_rcar_dmac(d)		container_of(d, struct rcar_dmac, engine)
>  
> +#define for_each_rcar_dmac_chan(i, chan, dmac)						\

I would have placed the iterator (chan) after the container being
iterated (dmac), but it seems there are some for_each_* macros doing it
the other way around (they may be older though).

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	for (i = 0, chan = &(dmac)->channels[0]; i < (dmac)->n_channels; i++, chan++)	\
> +		if (!((dmac)->channels_mask & BIT(i))) continue; else
> +
>  /*
>   * struct rcar_dmac_of_data - This driver's OF data
>   * @chan_offset_base: DMAC channels base offset
> @@ -817,15 +821,11 @@ static void rcar_dmac_chan_reinit(struct rcar_dmac_chan *chan)
>  
>  static void rcar_dmac_stop_all_chan(struct rcar_dmac *dmac)
>  {
> +	struct rcar_dmac_chan *chan;
>  	unsigned int i;
>  
>  	/* Stop all channels. */
> -	for (i = 0; i < dmac->n_channels; ++i) {
> -		struct rcar_dmac_chan *chan = &dmac->channels[i];
> -
> -		if (!(dmac->channels_mask & BIT(i)))
> -			continue;
> -
> +	for_each_rcar_dmac_chan(i, chan, dmac) {
>  		/* Stop and reinitialize the channel. */
>  		spin_lock_irq(&chan->lock);
>  		rcar_dmac_chan_halt(chan);
> @@ -1828,9 +1828,10 @@ static int rcar_dmac_probe(struct platform_device *pdev)
>  		DMA_SLAVE_BUSWIDTH_2_BYTES | DMA_SLAVE_BUSWIDTH_4_BYTES |
>  		DMA_SLAVE_BUSWIDTH_8_BYTES | DMA_SLAVE_BUSWIDTH_16_BYTES |
>  		DMA_SLAVE_BUSWIDTH_32_BYTES | DMA_SLAVE_BUSWIDTH_64_BYTES;
> +	const struct rcar_dmac_of_data *data;
> +	struct rcar_dmac_chan *chan;
>  	struct dma_device *engine;
>  	struct rcar_dmac *dmac;
> -	const struct rcar_dmac_of_data *data;
>  	unsigned int i;
>  	int ret;
>  
> @@ -1916,11 +1917,8 @@ static int rcar_dmac_probe(struct platform_device *pdev)
>  
>  	INIT_LIST_HEAD(&engine->channels);
>  
> -	for (i = 0; i < dmac->n_channels; ++i) {
> -		if (!(dmac->channels_mask & BIT(i)))
> -			continue;
> -
> -		ret = rcar_dmac_chan_probe(dmac, &dmac->channels[i], data, i);
> +	for_each_rcar_dmac_chan(i, chan, dmac) {
> +		ret = rcar_dmac_chan_probe(dmac, chan, data, i);
>  		if (ret < 0)
>  			goto error;
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/4] dmaengine: rcar-dmac: Add helpers for clearing DMA channel status

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Mon, Jan 25, 2021 at 03:24:30PM +0100, Geert Uytterhoeven wrote:
> Extract the code to clear the status of one or all channels into their
> own helpers, to prepare for the different handling of the R-Car V3U SoC.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v2:
>   - No changes.
> ---
>  drivers/dma/sh/rcar-dmac.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 537550b4121bbc22..7a0f802c61e5152d 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -336,6 +336,17 @@ static void rcar_dmac_chan_write(struct rcar_dmac_chan *chan, u32 reg, u32 data)
>  		writel(data, chan->iomem + reg);
>  }
>  
> +static void rcar_dmac_chan_clear(struct rcar_dmac *dmac,
> +				 struct rcar_dmac_chan *chan)
> +{
> +	rcar_dmac_write(dmac, RCAR_DMACHCLR, BIT(chan->index));
> +}
> +
> +static void rcar_dmac_chan_clear_all(struct rcar_dmac *dmac)
> +{
> +	rcar_dmac_write(dmac, RCAR_DMACHCLR, dmac->channels_mask);
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Initialization and configuration
>   */
> @@ -451,7 +462,7 @@ static int rcar_dmac_init(struct rcar_dmac *dmac)
>  	u16 dmaor;
>  
>  	/* Clear all channels and enable the DMAC globally. */
> -	rcar_dmac_write(dmac, RCAR_DMACHCLR, dmac->channels_mask);
> +	rcar_dmac_chan_clear_all(dmac);
>  	rcar_dmac_write(dmac, RCAR_DMAOR,
>  			RCAR_DMAOR_PRI_FIXED | RCAR_DMAOR_DME);
>  
> @@ -1566,7 +1577,7 @@ static irqreturn_t rcar_dmac_isr_channel(int irq, void *dev)
>  		 * because channel is already stopped in error case.
>  		 * We need to clear register and check DE bit as recovery.
>  		 */
> -		rcar_dmac_write(dmac, RCAR_DMACHCLR, 1 << chan->index);
> +		rcar_dmac_chan_clear(dmac, chan);
>  		rcar_dmac_chcr_de_barrier(chan);
>  		reinit = true;
>  		goto spin_lock_end;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/4] dmaengine: rcar-dmac: Add support for R-Car V3U

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Mon, Jan 25, 2021 at 03:24:31PM +0100, Geert Uytterhoeven wrote:
> The DMACs (both SYS-DMAC and RT-DMAC) on R-Car V3U differ slightly from
> the DMACs on R-Car Gen2 and other R-Car Gen3 SoCs:
>   1. The per-channel registers are located in a second register block.
>      Add support for mapping the second block, using the appropriate
>      offsets and stride.
>   2. The common Channel Clear Register (DMACHCLR) was replaced by a
>      per-channel register.
>      Update rcar_dmac_chan_clear{,_all}() to handle this.
>      As rcar_dmac_init() needs to clear the status before the individual
>      channels are probed, channel index and base address initialization
>      are moved forward.
> 
> Inspired by a patch in the BSP by Phong Hoang
> <phong.hoang.wz@renesas.com>.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - Use two separate named regions instead of an iomem[] array,
>   - Drop rcar_dmac_of_data.chan_reg_block, check for
>     !rcar_dmac_of_data.chan_offset_base instead,
>   - Precalculate chan_base in rcar_dmac_probe().
> ---
>  drivers/dma/sh/rcar-dmac.c | 74 ++++++++++++++++++++++++++++----------
>  1 file changed, 55 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/dma/sh/rcar-dmac.c b/drivers/dma/sh/rcar-dmac.c
> index 7a0f802c61e5152d..d9589eea98083215 100644
> --- a/drivers/dma/sh/rcar-dmac.c
> +++ b/drivers/dma/sh/rcar-dmac.c
> @@ -189,7 +189,8 @@ struct rcar_dmac_chan {
>   * struct rcar_dmac - R-Car Gen2 DMA Controller
>   * @engine: base DMA engine object
>   * @dev: the hardware device
> - * @iomem: remapped I/O memory base
> + * @dmac_base: remapped base register block
> + * @chan_base: remapped channel register block (optional)
>   * @n_channels: number of available channels
>   * @channels: array of DMAC channels
>   * @channels_mask: bitfield of which DMA channels are managed by this driver
> @@ -198,7 +199,8 @@ struct rcar_dmac_chan {
>  struct rcar_dmac {
>  	struct dma_device engine;
>  	struct device *dev;
> -	void __iomem *iomem;
> +	void __iomem *dmac_base;
> +	void __iomem *chan_base;
>  
>  	unsigned int n_channels;
>  	struct rcar_dmac_chan *channels;
> @@ -234,7 +236,7 @@ struct rcar_dmac_of_data {
>  #define RCAR_DMAOR_PRI_ROUND_ROBIN	(3 << 8)
>  #define RCAR_DMAOR_AE			(1 << 2)
>  #define RCAR_DMAOR_DME			(1 << 0)
> -#define RCAR_DMACHCLR			0x0080
> +#define RCAR_DMACHCLR			0x0080	/* Not on R-Car V3U */
>  #define RCAR_DMADPSEC			0x00a0
>  
>  #define RCAR_DMASAR			0x0000
> @@ -297,6 +299,9 @@ struct rcar_dmac_of_data {
>  #define RCAR_DMAFIXDAR			0x0014
>  #define RCAR_DMAFIXDPBASE		0x0060
>  
> +/* For R-Car V3U */
> +#define RCAR_V3U_DMACHCLR		0x0100
> +
>  /* Hardcode the MEMCPY transfer size to 4 bytes. */
>  #define RCAR_DMAC_MEMCPY_XFER_SIZE	4
>  
> @@ -307,17 +312,17 @@ struct rcar_dmac_of_data {
>  static void rcar_dmac_write(struct rcar_dmac *dmac, u32 reg, u32 data)
>  {
>  	if (reg == RCAR_DMAOR)
> -		writew(data, dmac->iomem + reg);
> +		writew(data, dmac->dmac_base + reg);
>  	else
> -		writel(data, dmac->iomem + reg);
> +		writel(data, dmac->dmac_base + reg);
>  }
>  
>  static u32 rcar_dmac_read(struct rcar_dmac *dmac, u32 reg)
>  {
>  	if (reg == RCAR_DMAOR)
> -		return readw(dmac->iomem + reg);
> +		return readw(dmac->dmac_base + reg);
>  	else
> -		return readl(dmac->iomem + reg);
> +		return readl(dmac->dmac_base + reg);
>  }
>  
>  static u32 rcar_dmac_chan_read(struct rcar_dmac_chan *chan, u32 reg)
> @@ -339,12 +344,23 @@ static void rcar_dmac_chan_write(struct rcar_dmac_chan *chan, u32 reg, u32 data)
>  static void rcar_dmac_chan_clear(struct rcar_dmac *dmac,
>  				 struct rcar_dmac_chan *chan)
>  {
> -	rcar_dmac_write(dmac, RCAR_DMACHCLR, BIT(chan->index));
> +	if (dmac->chan_base)

Using dmac->chan_base to check if the device is a V3U seems a bit of a
hack (especially given that the field is otherwise unused). I'd prefer
adding a model field to struct rcar_dmac_of_data and struct rcar_dmac.

> +		rcar_dmac_chan_write(chan, RCAR_V3U_DMACHCLR, 1);
> +	else
> +		rcar_dmac_write(dmac, RCAR_DMACHCLR, BIT(chan->index));
>  }
>  
>  static void rcar_dmac_chan_clear_all(struct rcar_dmac *dmac)
>  {
> -	rcar_dmac_write(dmac, RCAR_DMACHCLR, dmac->channels_mask);
> +	struct rcar_dmac_chan *chan;
> +	unsigned int i;
> +
> +	if (dmac->chan_base) {
> +		for_each_rcar_dmac_chan(i, chan, dmac)
> +			rcar_dmac_chan_write(chan, RCAR_V3U_DMACHCLR, 1);
> +	} else {
> +		rcar_dmac_write(dmac, RCAR_DMACHCLR, dmac->channels_mask);
> +	}
>  }
>  
>  /* -----------------------------------------------------------------------------
> @@ -1744,7 +1760,6 @@ static const struct dev_pm_ops rcar_dmac_pm = {
>  
>  static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
>  				struct rcar_dmac_chan *rchan,
> -				const struct rcar_dmac_of_data *data,
>  				unsigned int index)
>  {
>  	struct platform_device *pdev = to_platform_device(dmac->dev);
> @@ -1753,9 +1768,6 @@ static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
>  	char *irqname;
>  	int ret;
>  
> -	rchan->index = index;
> -	rchan->iomem = dmac->iomem + data->chan_offset_base +
> -		       data->chan_offset_stride * index;
>  	rchan->mid_rid = -EINVAL;
>  
>  	spin_lock_init(&rchan->lock);
> @@ -1842,6 +1854,7 @@ static int rcar_dmac_probe(struct platform_device *pdev)
>  	const struct rcar_dmac_of_data *data;
>  	struct rcar_dmac_chan *chan;
>  	struct dma_device *engine;
> +	void __iomem *chan_base;
>  	struct rcar_dmac *dmac;
>  	unsigned int i;
>  	int ret;
> @@ -1880,9 +1893,24 @@ static int rcar_dmac_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	/* Request resources. */
> -	dmac->iomem = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(dmac->iomem))
> -		return PTR_ERR(dmac->iomem);
> +	dmac->dmac_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(dmac->dmac_base))
> +		return PTR_ERR(dmac->dmac_base);
> +
> +	if (!data->chan_offset_base) {
> +		dmac->chan_base = devm_platform_ioremap_resource(pdev, 1);
> +		if (IS_ERR(dmac->chan_base))
> +			return PTR_ERR(dmac->chan_base);
> +
> +		chan_base = dmac->chan_base;
> +	} else {
> +		chan_base = dmac->dmac_base + data->chan_offset_base;
> +	}
> +
> +	for_each_rcar_dmac_chan(i, chan, dmac) {
> +		chan->index = i;

Now that chan->indew is set before calling rcar_dmac_chan_probe(), you
don't have to pass the index to rcar_dmac_chan_probe() anymore.

> +		chan->iomem = chan_base + i * data->chan_offset_stride;
> +	}
>  
>  	/* Enable runtime PM and initialize the device. */
>  	pm_runtime_enable(&pdev->dev);
> @@ -1929,7 +1957,7 @@ static int rcar_dmac_probe(struct platform_device *pdev)
>  	INIT_LIST_HEAD(&engine->channels);
>  
>  	for_each_rcar_dmac_chan(i, chan, dmac) {
> -		ret = rcar_dmac_chan_probe(dmac, chan, data, i);
> +		ret = rcar_dmac_chan_probe(dmac, chan, i);
>  		if (ret < 0)
>  			goto error;
>  	}
> @@ -1977,14 +2005,22 @@ static void rcar_dmac_shutdown(struct platform_device *pdev)
>  }
>  
>  static const struct rcar_dmac_of_data rcar_dmac_data = {
> -	.chan_offset_base = 0x8000,
> -	.chan_offset_stride = 0x80,
> +	.chan_offset_base	= 0x8000,
> +	.chan_offset_stride	= 0x80,
> +};
> +
> +static const struct rcar_dmac_of_data rcar_v3u_dmac_data = {
> +	.chan_offset_base	= 0x0,
> +	.chan_offset_stride	= 0x1000,
>  };
>  
>  static const struct of_device_id rcar_dmac_of_ids[] = {
>  	{
>  		.compatible = "renesas,rcar-dmac",
>  		.data = &rcar_dmac_data,
> +	}, {
> +		.compatible = "renesas,dmac-r8a779a0",
> +		.data = &rcar_v3u_dmac_data,
>  	},
>  	{ /* Sentinel */ }
>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/4] dmaengine: rcar-dmac: Add for_each_rcar_dmac_chan() helper

[Geert Uytterhoeven]

Hi Laurent,

On Tue, Jan 26, 2021 at 10:55 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Jan 25, 2021 at 03:24:29PM +0100, Geert Uytterhoeven wrote:
> > Add and helper macro for iterating over all DMAC channels, taking into
>
> s/and helper/a helper/

Oops.

> > account the channel mask.  Use it where appropriate, to simplify code.
> >
> > Restore "reverse Christmas tree" order of local variables while adding a
> > new variable.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > --- a/drivers/dma/sh/rcar-dmac.c
> > +++ b/drivers/dma/sh/rcar-dmac.c
> > @@ -209,6 +209,10 @@ struct rcar_dmac {
> >
> >  #define to_rcar_dmac(d)              container_of(d, struct rcar_dmac, engine)
> >
> > +#define for_each_rcar_dmac_chan(i, chan, dmac)                                               \
>
> I would have placed the iterator (chan) after the container being
> iterated (dmac), but it seems there are some for_each_* macros doing it
> the other way around (they may be older though).

Makes sense.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 4/4] dmaengine: rcar-dmac: Add support for R-Car V3U

[Geert Uytterhoeven]

Hi Laurent,

On Tue, Jan 26, 2021 at 11:01 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Mon, Jan 25, 2021 at 03:24:31PM +0100, Geert Uytterhoeven wrote:
> > The DMACs (both SYS-DMAC and RT-DMAC) on R-Car V3U differ slightly from
> > the DMACs on R-Car Gen2 and other R-Car Gen3 SoCs:
> >   1. The per-channel registers are located in a second register block.
> >      Add support for mapping the second block, using the appropriate
> >      offsets and stride.
> >   2. The common Channel Clear Register (DMACHCLR) was replaced by a
> >      per-channel register.
> >      Update rcar_dmac_chan_clear{,_all}() to handle this.
> >      As rcar_dmac_init() needs to clear the status before the individual
> >      channels are probed, channel index and base address initialization
> >      are moved forward.
> >
> > Inspired by a patch in the BSP by Phong Hoang
> > <phong.hoang.wz@renesas.com>.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > --- a/drivers/dma/sh/rcar-dmac.c
> > +++ b/drivers/dma/sh/rcar-dmac.c
> > @@ -189,7 +189,8 @@ struct rcar_dmac_chan {
> >   * struct rcar_dmac - R-Car Gen2 DMA Controller
> >   * @engine: base DMA engine object
> >   * @dev: the hardware device
> > - * @iomem: remapped I/O memory base
> > + * @dmac_base: remapped base register block
> > + * @chan_base: remapped channel register block (optional)
> >   * @n_channels: number of available channels
> >   * @channels: array of DMAC channels
> >   * @channels_mask: bitfield of which DMA channels are managed by this driver
> > @@ -198,7 +199,8 @@ struct rcar_dmac_chan {
> >  struct rcar_dmac {
> >       struct dma_device engine;
> >       struct device *dev;
> > -     void __iomem *iomem;
> > +     void __iomem *dmac_base;
> > +     void __iomem *chan_base;
> >
> >       unsigned int n_channels;
> >       struct rcar_dmac_chan *channels;

> > @@ -339,12 +344,23 @@ static void rcar_dmac_chan_write(struct rcar_dmac_chan *chan, u32 reg, u32 data)
> >  static void rcar_dmac_chan_clear(struct rcar_dmac *dmac,
> >                                struct rcar_dmac_chan *chan)
> >  {
> > -     rcar_dmac_write(dmac, RCAR_DMACHCLR, BIT(chan->index));
> > +     if (dmac->chan_base)
>
> Using dmac->chan_base to check if the device is a V3U seems a bit of a
> hack (especially given that the field is otherwise unused). I'd prefer
> adding a model field to struct rcar_dmac_of_data and struct rcar_dmac.

The check is not a check for R-Car V3U in particular, but a check for
the presence of a separate register block for channel registers.
I expect to see more SoCs having this, so IMHO checking for this feature,
instead of checking a model field, makes sense.

It's indeed unused otherwise, as beyond probe(), where per-channel bases
are calculated, no access to this pointer is needed anymore, (you can
blame devm_*() for not needing the pointer ;-)
Note that a model field would be "otherwise unused", too ;-)

> > +             rcar_dmac_chan_write(chan, RCAR_V3U_DMACHCLR, 1);
> > +     else
> > +             rcar_dmac_write(dmac, RCAR_DMACHCLR, BIT(chan->index));
> >  }
> >
> >  static void rcar_dmac_chan_clear_all(struct rcar_dmac *dmac)
> >  {
> > -     rcar_dmac_write(dmac, RCAR_DMACHCLR, dmac->channels_mask);
> > +     struct rcar_dmac_chan *chan;
> > +     unsigned int i;
> > +
> > +     if (dmac->chan_base) {
> > +             for_each_rcar_dmac_chan(i, chan, dmac)
> > +                     rcar_dmac_chan_write(chan, RCAR_V3U_DMACHCLR, 1);
> > +     } else {
> > +             rcar_dmac_write(dmac, RCAR_DMACHCLR, dmac->channels_mask);
> > +     }
> >  }
> >
> >  /* -----------------------------------------------------------------------------
> > @@ -1744,7 +1760,6 @@ static const struct dev_pm_ops rcar_dmac_pm = {
> >
> >  static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
> >                               struct rcar_dmac_chan *rchan,
> > -                             const struct rcar_dmac_of_data *data,
> >                               unsigned int index)
> >  {
> >       struct platform_device *pdev = to_platform_device(dmac->dev);
> > @@ -1753,9 +1768,6 @@ static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
> >       char *irqname;
> >       int ret;
> >
> > -     rchan->index = index;
> > -     rchan->iomem = dmac->iomem + data->chan_offset_base +
> > -                    data->chan_offset_stride * index;
> >       rchan->mid_rid = -EINVAL;
> >
> >       spin_lock_init(&rchan->lock);
> > @@ -1842,6 +1854,7 @@ static int rcar_dmac_probe(struct platform_device *pdev)
> >       const struct rcar_dmac_of_data *data;
> >       struct rcar_dmac_chan *chan;
> >       struct dma_device *engine;
> > +     void __iomem *chan_base;
> >       struct rcar_dmac *dmac;
> >       unsigned int i;
> >       int ret;
> > @@ -1880,9 +1893,24 @@ static int rcar_dmac_probe(struct platform_device *pdev)
> >               return -ENOMEM;
> >
> >       /* Request resources. */
> > -     dmac->iomem = devm_platform_ioremap_resource(pdev, 0);
> > -     if (IS_ERR(dmac->iomem))
> > -             return PTR_ERR(dmac->iomem);
> > +     dmac->dmac_base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(dmac->dmac_base))
> > +             return PTR_ERR(dmac->dmac_base);
> > +
> > +     if (!data->chan_offset_base) {
> > +             dmac->chan_base = devm_platform_ioremap_resource(pdev, 1);
> > +             if (IS_ERR(dmac->chan_base))
> > +                     return PTR_ERR(dmac->chan_base);
> > +
> > +             chan_base = dmac->chan_base;
> > +     } else {
> > +             chan_base = dmac->dmac_base + data->chan_offset_base;
> > +     }
> > +
> > +     for_each_rcar_dmac_chan(i, chan, dmac) {
> > +             chan->index = i;
>
> Now that chan->indew is set before calling rcar_dmac_chan_probe(), you
> don't have to pass the index to rcar_dmac_chan_probe() anymore.

Right, will fix.

> > +             chan->iomem = chan_base + i * data->chan_offset_stride;
> > +     }
> >
> >       /* Enable runtime PM and initialize the device. */
> >       pm_runtime_enable(&pdev->dev);
> > @@ -1929,7 +1957,7 @@ static int rcar_dmac_probe(struct platform_device *pdev)
> >       INIT_LIST_HEAD(&engine->channels);
> >
> >       for_each_rcar_dmac_chan(i, chan, dmac) {
> > -             ret = rcar_dmac_chan_probe(dmac, chan, data, i);
> > +             ret = rcar_dmac_chan_probe(dmac, chan, i);
> >               if (ret < 0)
> >                       goto error;
> >       }

Thanks for your comments!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 4/4] dmaengine: rcar-dmac: Add support for R-Car V3U

[Laurent Pinchart]

Hi Geert,

On Wed, Jan 27, 2021 at 09:10:40AM +0100, Geert Uytterhoeven wrote:
> On Tue, Jan 26, 2021 at 11:01 PM Laurent Pinchart wrote:
> > On Mon, Jan 25, 2021 at 03:24:31PM +0100, Geert Uytterhoeven wrote:
> > > The DMACs (both SYS-DMAC and RT-DMAC) on R-Car V3U differ slightly from
> > > the DMACs on R-Car Gen2 and other R-Car Gen3 SoCs:
> > >   1. The per-channel registers are located in a second register block.
> > >      Add support for mapping the second block, using the appropriate
> > >      offsets and stride.
> > >   2. The common Channel Clear Register (DMACHCLR) was replaced by a
> > >      per-channel register.
> > >      Update rcar_dmac_chan_clear{,_all}() to handle this.
> > >      As rcar_dmac_init() needs to clear the status before the individual
> > >      channels are probed, channel index and base address initialization
> > >      are moved forward.
> > >
> > > Inspired by a patch in the BSP by Phong Hoang
> > > <phong.hoang.wz@renesas.com>.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> > > --- a/drivers/dma/sh/rcar-dmac.c
> > > +++ b/drivers/dma/sh/rcar-dmac.c
> > > @@ -189,7 +189,8 @@ struct rcar_dmac_chan {
> > >   * struct rcar_dmac - R-Car Gen2 DMA Controller
> > >   * @engine: base DMA engine object
> > >   * @dev: the hardware device
> > > - * @iomem: remapped I/O memory base
> > > + * @dmac_base: remapped base register block
> > > + * @chan_base: remapped channel register block (optional)
> > >   * @n_channels: number of available channels
> > >   * @channels: array of DMAC channels
> > >   * @channels_mask: bitfield of which DMA channels are managed by this driver
> > > @@ -198,7 +199,8 @@ struct rcar_dmac_chan {
> > >  struct rcar_dmac {
> > >       struct dma_device engine;
> > >       struct device *dev;
> > > -     void __iomem *iomem;
> > > +     void __iomem *dmac_base;
> > > +     void __iomem *chan_base;
> > >
> > >       unsigned int n_channels;
> > >       struct rcar_dmac_chan *channels;
> 
> > > @@ -339,12 +344,23 @@ static void rcar_dmac_chan_write(struct rcar_dmac_chan *chan, u32 reg, u32 data)
> > >  static void rcar_dmac_chan_clear(struct rcar_dmac *dmac,
> > >                                struct rcar_dmac_chan *chan)
> > >  {
> > > -     rcar_dmac_write(dmac, RCAR_DMACHCLR, BIT(chan->index));
> > > +     if (dmac->chan_base)
> >
> > Using dmac->chan_base to check if the device is a V3U seems a bit of a
> > hack (especially given that the field is otherwise unused). I'd prefer
> > adding a model field to struct rcar_dmac_of_data and struct rcar_dmac.
> 
> The check is not a check for R-Car V3U in particular, but a check for
> the presence of a separate register block for channel registers.
> I expect to see more SoCs having this, so IMHO checking for this feature,
> instead of checking a model field, makes sense.
> 
> It's indeed unused otherwise, as beyond probe(), where per-channel bases
> are calculated, no access to this pointer is needed anymore, (you can
> blame devm_*() for not needing the pointer ;-)
> Note that a model field would be "otherwise unused", too ;-)

I agree that this isn't a V3U check, but a DMAC
"model/generation/version" check. With V3U as the only SoC we know of
that uses this new DMAC model, it's a bit difficult to come up with a
proper name, but conceptually I think a model check would be better than
checking chan_base.

> > > +             rcar_dmac_chan_write(chan, RCAR_V3U_DMACHCLR, 1);
> > > +     else
> > > +             rcar_dmac_write(dmac, RCAR_DMACHCLR, BIT(chan->index));
> > >  }
> > >
> > >  static void rcar_dmac_chan_clear_all(struct rcar_dmac *dmac)
> > >  {
> > > -     rcar_dmac_write(dmac, RCAR_DMACHCLR, dmac->channels_mask);
> > > +     struct rcar_dmac_chan *chan;
> > > +     unsigned int i;
> > > +
> > > +     if (dmac->chan_base) {
> > > +             for_each_rcar_dmac_chan(i, chan, dmac)
> > > +                     rcar_dmac_chan_write(chan, RCAR_V3U_DMACHCLR, 1);
> > > +     } else {
> > > +             rcar_dmac_write(dmac, RCAR_DMACHCLR, dmac->channels_mask);
> > > +     }
> > >  }
> > >
> > >  /* -----------------------------------------------------------------------------
> > > @@ -1744,7 +1760,6 @@ static const struct dev_pm_ops rcar_dmac_pm = {
> > >
> > >  static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
> > >                               struct rcar_dmac_chan *rchan,
> > > -                             const struct rcar_dmac_of_data *data,
> > >                               unsigned int index)
> > >  {
> > >       struct platform_device *pdev = to_platform_device(dmac->dev);
> > > @@ -1753,9 +1768,6 @@ static int rcar_dmac_chan_probe(struct rcar_dmac *dmac,
> > >       char *irqname;
> > >       int ret;
> > >
> > > -     rchan->index = index;
> > > -     rchan->iomem = dmac->iomem + data->chan_offset_base +
> > > -                    data->chan_offset_stride * index;
> > >       rchan->mid_rid = -EINVAL;
> > >
> > >       spin_lock_init(&rchan->lock);
> > > @@ -1842,6 +1854,7 @@ static int rcar_dmac_probe(struct platform_device *pdev)
> > >       const struct rcar_dmac_of_data *data;
> > >       struct rcar_dmac_chan *chan;
> > >       struct dma_device *engine;
> > > +     void __iomem *chan_base;
> > >       struct rcar_dmac *dmac;
> > >       unsigned int i;
> > >       int ret;
> > > @@ -1880,9 +1893,24 @@ static int rcar_dmac_probe(struct platform_device *pdev)
> > >               return -ENOMEM;
> > >
> > >       /* Request resources. */
> > > -     dmac->iomem = devm_platform_ioremap_resource(pdev, 0);
> > > -     if (IS_ERR(dmac->iomem))
> > > -             return PTR_ERR(dmac->iomem);
> > > +     dmac->dmac_base = devm_platform_ioremap_resource(pdev, 0);
> > > +     if (IS_ERR(dmac->dmac_base))
> > > +             return PTR_ERR(dmac->dmac_base);
> > > +
> > > +     if (!data->chan_offset_base) {
> > > +             dmac->chan_base = devm_platform_ioremap_resource(pdev, 1);
> > > +             if (IS_ERR(dmac->chan_base))
> > > +                     return PTR_ERR(dmac->chan_base);
> > > +
> > > +             chan_base = dmac->chan_base;
> > > +     } else {
> > > +             chan_base = dmac->dmac_base + data->chan_offset_base;
> > > +     }
> > > +
> > > +     for_each_rcar_dmac_chan(i, chan, dmac) {
> > > +             chan->index = i;
> >
> > Now that chan->indew is set before calling rcar_dmac_chan_probe(), you
> > don't have to pass the index to rcar_dmac_chan_probe() anymore.
> 
> Right, will fix.
> 
> > > +             chan->iomem = chan_base + i * data->chan_offset_stride;
> > > +     }
> > >
> > >       /* Enable runtime PM and initialize the device. */
> > >       pm_runtime_enable(&pdev->dev);
> > > @@ -1929,7 +1957,7 @@ static int rcar_dmac_probe(struct platform_device *pdev)
> > >       INIT_LIST_HEAD(&engine->channels);
> > >
> > >       for_each_rcar_dmac_chan(i, chan, dmac) {
> > > -             ret = rcar_dmac_chan_probe(dmac, chan, data, i);
> > > +             ret = rcar_dmac_chan_probe(dmac, chan, i);
> > >               if (ret < 0)
> > >                       goto error;
> > >       }
> 
> Thanks for your comments!

-- 
Regards,

Laurent Pinchart