[PATCH 0/8] RZN1 DMA support

[PATCH 0/8] RZN1 DMA support

[Miquel Raynal]

Hello,

Here is a first series bringing DMA support to RZN1 platforms. I'm not a
DMA expert at all so criticism is welcome.

Soon a second series will come with changes made to the UART controller
driver, in order to interact with the RZN1 DMA controller.

Cheers,
Miquèl

Miquel Raynal (7):
  dt-bindings: dma: Introduce RZN1 dmamux bindings
  dt-bindings: dma: Introduce RZN1 DMA compatible
  soc: renesas: rzn1-sysc: Export function to set dmamux
  dma: dmamux: Introduce RZN1 DMA router support
  dma: dw: Add RZN1 compatible
  ARM: dts: r9a06g032: Add the two DMA nodes
  ARM: dts: r9a06g032: Describe the DMA router

Phil Edworthy (1):
  dma: dw: Avoid partial transfers

 .../bindings/dma/renesas,rzn1-dmamux.yaml     |  42 +++++
 .../bindings/dma/snps,dma-spear1340.yaml      |   8 +-
 MAINTAINERS                                   |   1 +
 arch/arm/boot/dts/r9a06g032.dtsi              |  37 ++++
 drivers/clk/renesas/r9a06g032-clocks.c        |  27 +++
 drivers/dma/dw/Makefile                       |   2 +-
 drivers/dma/dw/core.c                         |   3 +
 drivers/dma/dw/dmamux.c                       | 175 ++++++++++++++++++
 drivers/dma/dw/platform.c                     |   1 +
 include/dt-bindings/clock/r9a06g032-sysctrl.h |   2 +
 include/linux/soc/renesas/r9a06g032-syscon.h  |  11 ++
 11 files changed, 307 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/renesas,rzn1-dmamux.yaml
 create mode 100644 drivers/dma/dw/dmamux.c
 create mode 100644 include/linux/soc/renesas/r9a06g032-syscon.h

-- 
2.27.0

[PATCH 1/8] dt-bindings: dma: Introduce RZN1 dmamux bindings

[Miquel Raynal]

The Renesas RZN1 DMA IP is a based on a DW core, with eg. an additional
dmamux register located in the system control area which can take up to
32 requests (16 per DMA controller). Each DMA channel can be wired to
two different peripherals.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../bindings/dma/renesas,rzn1-dmamux.yaml     | 42 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/renesas,rzn1-dmamux.yaml

diff --git a/Documentation/devicetree/bindings/dma/renesas,rzn1-dmamux.yaml b/Documentation/devicetree/bindings/dma/renesas,rzn1-dmamux.yaml
new file mode 100644
index 000000000000..e2c82e43b8b1
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/renesas,rzn1-dmamux.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/renesas,rzn1-dmamux.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/N1 DMA mux
+
+maintainers:
+  - Miquel Raynal <miquel.raynal@bootlin.com>
+
+allOf:
+  - $ref: "dma-router.yaml#"
+
+properties:
+  compatible:
+    const: renesas,rzn1-dmamux
+
+  '#dma-cells':
+    const: 6
+    description:
+      The first four cells are dedicated to the master DMA controller. The fifth
+      cell gives the DMA mux bit index that must be set starting from 0. The
+      sixth cell gives the binary value that must be written there, ie. 0 or 1.
+
+  dma-masters:
+    minItems: 1
+    maxItems: 2
+
+  dma-requests:
+    const: 32
+
+additionalProperties: false
+
+examples:
+  - |
+    dma-router {
+      compatible = "renesas,rzn1-dmamux";
+      #dma-cells = <6>;
+      dma-masters = <&dma0 &dma1>;
+      dma-requests = <32>;
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index ea3e6c914384..c70c9c39a2f3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18636,6 +18636,7 @@ SYNOPSYS DESIGNWARE DMAC DRIVER
 M:	Viresh Kumar <vireshk@kernel.org>
 R:	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
 S:	Maintained
+F:	Documentation/devicetree/bindings/dma/renesas,rzn1-dmamux.yaml
 F:	Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
 F:	drivers/dma/dw/
 F:	include/dt-bindings/dma/dw-dmac.h
-- 
2.27.0

[PATCH 2/8] dt-bindings: dma: Introduce RZN1 DMA compatible

[Miquel Raynal]

Just like for the NAND controller that is also on this SoC, let's
provide a SoC generic and a more specific couple of compatibles.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../devicetree/bindings/dma/snps,dma-spear1340.yaml       | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
index 6b35089ac017..c2e2dc637e0a 100644
--- a/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
+++ b/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
@@ -15,7 +15,13 @@ allOf:
 
 properties:
   compatible:
-    const: snps,dma-spear1340
+    oneOf:
+      - const: snps,dma-spear1340
+      - items:
+          - enum:
+              - renesas,r9a06g032-nandc
+          - const: renesas,rzn1-nandc
+
 
   "#dma-cells":
     minimum: 3
-- 
2.27.0

[PATCH 3/8] soc: renesas: rzn1-sysc: Export function to set dmamux

[Miquel Raynal]

The dmamux register is located within the system controller.

Without syscon, we need an extra helper in order to give write access to
this register to a dmamux driver.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/renesas/r9a06g032-clocks.c        | 27 +++++++++++++++++++
 include/dt-bindings/clock/r9a06g032-sysctrl.h |  2 ++
 include/linux/soc/renesas/r9a06g032-syscon.h  | 11 ++++++++
 3 files changed, 40 insertions(+)
 create mode 100644 include/linux/soc/renesas/r9a06g032-syscon.h

diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c
index c99942f0e4d4..3bca60fac21c 100644
--- a/drivers/clk/renesas/r9a06g032-clocks.c
+++ b/drivers/clk/renesas/r9a06g032-clocks.c
@@ -315,6 +315,27 @@ struct r9a06g032_priv {
 	void __iomem *reg;
 };
 
+/* Exported helper to access the DMAMUX register */
+static struct r9a06g032_priv *syscon_priv;
+int r9a06g032_syscon_set_dmamux(u32 mask, u32 val)
+{
+	u32 dmamux;
+
+	if (!syscon_priv)
+		return -EPROBE_DEFER;
+
+	spin_lock(&syscon_priv->lock);
+
+	dmamux = readl(syscon_priv->reg + R9A06G032_SYSCON_DMAMUX);
+	dmamux &= ~mask;
+	dmamux |= val & mask;
+	writel(dmamux, syscon_priv->reg + R9A06G032_SYSCON_DMAMUX);
+
+	spin_unlock(&syscon_priv->lock);
+
+	return 0;
+}
+
 /* register/bit pairs are encoded as an uint16_t */
 static void
 clk_rdesc_set(struct r9a06g032_priv *clocks,
@@ -922,6 +943,12 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev)
 	clocks->reg = of_iomap(np, 0);
 	if (WARN_ON(!clocks->reg))
 		return -ENOMEM;
+
+	if (syscon_priv)
+		return -EBUSY;
+
+	syscon_priv = clocks;
+
 	for (i = 0; i < ARRAY_SIZE(r9a06g032_clocks); ++i) {
 		const struct r9a06g032_clkdesc *d = &r9a06g032_clocks[i];
 		const char *parent_name = d->source ?
diff --git a/include/dt-bindings/clock/r9a06g032-sysctrl.h b/include/dt-bindings/clock/r9a06g032-sysctrl.h
index 90c0f3dc1ba1..609e7fe8fcb1 100644
--- a/include/dt-bindings/clock/r9a06g032-sysctrl.h
+++ b/include/dt-bindings/clock/r9a06g032-sysctrl.h
@@ -145,4 +145,6 @@
 #define R9A06G032_CLK_UART6		152
 #define R9A06G032_CLK_UART7		153
 
+#define R9A06G032_SYSCON_DMAMUX		0xA0
+
 #endif /* __DT_BINDINGS_R9A06G032_SYSCTRL_H__ */
diff --git a/include/linux/soc/renesas/r9a06g032-syscon.h b/include/linux/soc/renesas/r9a06g032-syscon.h
new file mode 100644
index 000000000000..d97e0e91cc6a
--- /dev/null
+++ b/include/linux/soc/renesas/r9a06g032-syscon.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_SOC_RENESAS_R9A06G032_SYSCON_H__
+#define __LINUX_SOC_RENESAS_R9A06G032_SYSCON_H__
+
+#ifdef CONFIG_CLK_R9A06G032
+int r9a06g032_syscon_set_dmamux(u32 mask, u32 val);
+#else
+static inline int r9a06g032_syscon_set_dmamux(u32 mask, u32 val) { return -ENODEV; }
+#endif
+
+#endif /* __LINUX_SOC_RENESAS_R9A06G032_SYSCON_H__ */
-- 
2.27.0

[PATCH 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Miquel Raynal]

The Renesas RZN1 DMA IP is a based on a DW core, with eg. an additional
dmamux register located in the system control area which can take up to
32 requests (16 per DMA controller). Each DMA channel can be wired to
two different peripherals.

We need two additional information from the 'dmas' property: the channel
(bit in the dmamux register) that must be accessed and the value of the
mux for this channel.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/dma/dw/Makefile |   2 +-
 drivers/dma/dw/dmamux.c | 175 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 176 insertions(+), 1 deletion(-)
 create mode 100644 drivers/dma/dw/dmamux.c

diff --git a/drivers/dma/dw/Makefile b/drivers/dma/dw/Makefile
index a6f358ad8591..d8cfbf36b381 100644
--- a/drivers/dma/dw/Makefile
+++ b/drivers/dma/dw/Makefile
@@ -4,7 +4,7 @@ dw_dmac_core-y			:= core.o dw.o idma32.o
 dw_dmac_core-$(CONFIG_ACPI)	+= acpi.o
 
 obj-$(CONFIG_DW_DMAC)		+= dw_dmac.o
-dw_dmac-y			:= platform.o
+dw_dmac-y			:= platform.o dmamux.o
 dw_dmac-$(CONFIG_OF)		+= of.o
 
 obj-$(CONFIG_DW_DMAC_PCI)	+= dw_dmac_pci.o
diff --git a/drivers/dma/dw/dmamux.c b/drivers/dma/dw/dmamux.c
new file mode 100644
index 000000000000..30de776f195e
--- /dev/null
+++ b/drivers/dma/dw/dmamux.c
@@ -0,0 +1,175 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Schneider-Electric
+ * Author: Miquel Raynal <miquel.raynal@bootlin.com
+ * Based on TI crossbar driver written by Peter Ujfalusi <peter.ujfalusi@ti.com>
+ */
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/io.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_dma.h>
+#include <linux/soc/renesas/r9a06g032-syscon.h>
+
+#define RZN1_DMAMUX_LINES	64
+
+struct rzn1_dmamux_data {
+	struct dma_router dmarouter;
+	unsigned int dmac_requests;
+	unsigned int dmamux_requests;
+	u32 used_chans;
+	struct mutex lock;
+};
+
+struct rzn1_dmamux_map {
+	unsigned int req_idx;
+};
+
+static void rzn1_dmamux_free(struct device *dev, void *route_data)
+{
+	struct rzn1_dmamux_data *dmamux = dev_get_drvdata(dev);
+	struct rzn1_dmamux_map *map = route_data;
+
+	dev_dbg(dev, "Unmapping DMAMUX request %u\n", map->req_idx);
+
+	mutex_lock(&dmamux->lock);
+	dmamux->used_chans &= ~BIT(map->req_idx);
+	mutex_unlock(&dmamux->lock);
+
+	kfree(map);
+}
+
+static void *rzn1_dmamux_route_allocate(struct of_phandle_args *dma_spec,
+					struct of_dma *ofdma)
+{
+	struct platform_device *pdev = of_find_device_by_node(ofdma->of_node);
+	struct rzn1_dmamux_data *dmamux = platform_get_drvdata(pdev);
+	struct rzn1_dmamux_map *map;
+	unsigned int master, chan, val;
+	int ret;
+
+	map = kzalloc(sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return ERR_PTR(-ENOMEM);
+
+	if (dma_spec->args_count != 6)
+		return ERR_PTR(-EINVAL);
+
+	chan = dma_spec->args[0];
+	map->req_idx = dma_spec->args[4];
+	val = dma_spec->args[5];
+	dma_spec->args_count -= 2;
+
+	if (chan >= dmamux->dmac_requests) {
+		dev_err(&pdev->dev, "Invalid DMA request line: %d\n", chan);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (map->req_idx >= dmamux->dmamux_requests ||
+	    map->req_idx % dmamux->dmac_requests != chan) {
+		dev_err(&pdev->dev, "Invalid MUX request line: %d\n", map->req_idx);
+		return ERR_PTR(-EINVAL);
+	}
+
+	/* The of_node_put() will be done in the core for the node */
+	master = map->req_idx < dmamux->dmac_requests ? 0 : 1;
+	dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", master);
+	if (!dma_spec->np) {
+		dev_err(&pdev->dev, "Can't get DMA master\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n",
+		map->req_idx, master, chan);
+
+	mutex_lock(&dmamux->lock);
+	dmamux->used_chans |= BIT(map->req_idx);
+	ret = r9a06g032_syscon_set_dmamux(BIT(map->req_idx),
+					  val ? BIT(map->req_idx) : 0);
+	mutex_unlock(&dmamux->lock);
+	if (ret) {
+		rzn1_dmamux_free(&pdev->dev, map);
+		return ERR_PTR(ret);
+	}
+
+	return map;
+}
+
+static const struct of_device_id rzn1_dmac_match[] __maybe_unused = {
+	{ .compatible = "renesas,rzn1-dma", },
+	{},
+};
+
+static int rzn1_dmamux_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	const struct of_device_id *match;
+	struct device_node *dmac_node;
+	struct rzn1_dmamux_data *dmamux;
+
+	if (!node)
+		return -ENODEV;
+
+	dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL);
+	if (!dmamux)
+		return -ENOMEM;
+
+	mutex_init(&dmamux->lock);
+
+	dmac_node = of_parse_phandle(node, "dma-masters", 0);
+	if (!dmac_node) {
+		dev_err(&pdev->dev, "Can't get DMA master node\n");
+		return -ENODEV;
+	}
+
+	match = of_match_node(rzn1_dmac_match, dmac_node);
+	if (!match) {
+		dev_err(&pdev->dev, "DMA master is not supported\n");
+		of_node_put(dmac_node);
+		return -EINVAL;
+	}
+
+	if (of_property_read_u32(dmac_node, "dma-requests",
+				 &dmamux->dmac_requests)) {
+		dev_err(&pdev->dev, "Missing DMAC requests information\n");
+		of_node_put(dmac_node);
+		return -EINVAL;
+	}
+	of_node_put(dmac_node);
+
+	if (of_property_read_u32(node, "dma-requests",
+				 &dmamux->dmamux_requests)) {
+		dev_err(&pdev->dev, "Missing DMA mux requests information\n");
+		return -EINVAL;
+	}
+
+	dmamux->dmarouter.dev = &pdev->dev;
+	dmamux->dmarouter.route_free = rzn1_dmamux_free;
+
+	platform_set_drvdata(pdev, dmamux);
+
+	return of_dma_router_register(node, rzn1_dmamux_route_allocate,
+				      &dmamux->dmarouter);
+}
+
+static const struct of_device_id rzn1_dmamux_match[] = {
+	{ .compatible = "renesas,rzn1-dmamux", },
+	{},
+};
+
+static struct platform_driver rzn1_dmamux_driver = {
+	.driver = {
+		.name = "renesas,rzn1-dmamux",
+		.of_match_table = rzn1_dmamux_match,
+	},
+	.probe	= rzn1_dmamux_probe,
+};
+
+static int rzn1_dmamux_init(void)
+{
+	return platform_driver_register(&rzn1_dmamux_driver);
+}
+arch_initcall(rzn1_dmamux_init);
-- 
2.27.0

[PATCH 5/8] dma: dw: Avoid partial transfers

[Miquel Raynal]

From: Phil Edworthy <phil.edworthy@renesas.com>

Pausing a partial transfer only causes data to be written to mem that is
a multiple of the memory width setting.

However, when a DMA client driver finishes DMA early, e.g. due to UART
char timeout interrupt, all data read from the DEV must be written to MEM.

Therefore, allow the slave to limit the memory width to ensure all data
read from the DEV is written to MEM when DMA is paused.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/dma/dw/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index 7ab83fe601ed..48cdefe997f1 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -705,6 +705,9 @@ dwc_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
 		ctllo |= sconfig->device_fc ? DWC_CTLL_FC(DW_DMA_FC_P_P2M) :
 			DWC_CTLL_FC(DW_DMA_FC_D_P2M);
 
+		if (sconfig->dst_addr_width && sconfig->dst_addr_width < data_width)
+			data_width = sconfig->dst_addr_width;
+
 		for_each_sg(sgl, sg, sg_len, i) {
 			struct dw_desc	*desc;
 			u32		len, mem;
-- 
2.27.0

[PATCH 6/8] dma: dw: Add RZN1 compatible

[Miquel Raynal]

The Renesas RZN1 DMA IP is very close to the original DW DMA IP, a DMA
routeur has been introduced to handle the wiring options that have been
added.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/dma/dw/platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index 246118955877..47f2292dba98 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -137,6 +137,7 @@ static void dw_shutdown(struct platform_device *pdev)
 #ifdef CONFIG_OF
 static const struct of_device_id dw_dma_of_id_table[] = {
 	{ .compatible = "snps,dma-spear1340", .data = &dw_dma_chip_pdata },
+	{ .compatible = "renesas,rzn1-dma", .data = &dw_dma_chip_pdata },
 	{}
 };
 MODULE_DEVICE_TABLE(of, dw_dma_of_id_table);
-- 
2.27.0

[PATCH 7/8] ARM: dts: r9a06g032: Add the two DMA nodes

[Miquel Raynal]

Describe the two DMA controllers available on this SoC.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm/boot/dts/r9a06g032.dtsi | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index db657224688a..640c3eb4bbcd 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -184,6 +184,36 @@ nand_controller: nand-controller@40102000 {
 			status = "disabled";
 		};
 
+		dma0: dma-controller@40104000 {
+			compatible = "renesas,r9a06g032-dma", "renesas,rzn1-dma";
+			reg = <0x40104000 0x1000>;
+			interrupts = <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
+			clock-names = "hclk";
+			clocks = <&sysctrl R9A06G032_HCLK_DMA0>;
+			dma-channels = <8>;
+			dma-requests = <16>;
+			dma-masters = <1>;
+			#dma-cells = <3>;
+			block_size = <0xfff>;
+			data_width = <3>;
+			status = "disabled";
+		};
+
+		dma1: dma-controller@40105000 {
+			compatible = "renesas,r9a06g032-dma", "renesas,rzn1-dma";
+			reg = <0x40105000 0x1000>;
+			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>;
+			clock-names = "hclk";
+			clocks = <&sysctrl R9A06G032_HCLK_DMA1>;
+			dma-channels = <8>;
+			dma-requests = <16>;
+			dma-masters = <1>;
+			#dma-cells = <3>;
+			block_size = <0xfff>;
+			data_width = <3>;
+			status = "disabled";
+		};
+
 		gic: interrupt-controller@44101000 {
 			compatible = "arm,gic-400", "arm,cortex-a7-gic";
 			interrupt-controller;
-- 
2.27.0

[PATCH 8/8] ARM: dts: r9a06g032: Describe the DMA router

[Miquel Raynal]

There is a dmamux on this SoC which allows picking two different sources
for a single DMA request.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm/boot/dts/r9a06g032.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index 640c3eb4bbcd..0eb12c3d9cfd 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -59,6 +59,13 @@ ext_rtc_clk: extrtcclk {
 		clock-frequency = <0>;
 	};
 
+	dmamux: dma-router {
+		compatible = "renesas,rzn1-dmamux";
+		#dma-cells = <6>;
+		dma-requests = <32>;
+		dma-masters = <&dma0 &dma1>;
+	};
+
 	soc {
 		compatible = "simple-bus";
 		#address-cells = <1>;
-- 
2.27.0

Re: [PATCH 5/8] dma: dw: Avoid partial transfers

[Andy Shevchenko]

On Fri, Feb 18, 2022 at 07:12:23PM +0100, Miquel Raynal wrote:
> From: Phil Edworthy <phil.edworthy@renesas.com>
> 
> Pausing a partial transfer only causes data to be written to mem that is
> a multiple of the memory width setting.
> 
> However, when a DMA client driver finishes DMA early, e.g. due to UART
> char timeout interrupt, all data read from the DEV must be written to MEM.
> 
> Therefore, allow the slave to limit the memory width to ensure all data
> read from the DEV is written to MEM when DMA is paused.

Is this a fix?
What happens to the data if you don't do this?
As far as I understood the Synopsys DesignWare specification the DMA controller
is capable of flushing FIFO in that case on byte-by-byte basis. Do you have an
HW integration bug?

TL;DR: tell us more about this.

...

> +		if (sconfig->dst_addr_width && sconfig->dst_addr_width < data_width)
> +			data_width = sconfig->dst_addr_width;

But here no check that you do it for explicitly peripheral to memory, so this
will affect memory to peripheral transfers as well.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Andy Shevchenko]

On Fri, Feb 18, 2022 at 07:12:22PM +0100, Miquel Raynal wrote:
> The Renesas RZN1 DMA IP is a based on a DW core, with eg. an additional
> dmamux register located in the system control area which can take up to
> 32 requests (16 per DMA controller). Each DMA channel can be wired to
> two different peripherals.
> 
> We need two additional information from the 'dmas' property: the channel
> (bit in the dmamux register) that must be accessed and the value of the
> mux for this channel.

...

> +dw_dmac-y			:= platform.o dmamux.o

We do not need this on other platforms, please make sure we have no dangling
code on, e.g., x86.

...

> +	/* The of_node_put() will be done in the core for the node */
> +	master = map->req_idx < dmamux->dmac_requests ? 0 : 1;

The opposite conditional will be better, no?`

...

> +	dmamux->used_chans |= BIT(map->req_idx);
> +	ret = r9a06g032_syscon_set_dmamux(BIT(map->req_idx),
> +					  val ? BIT(map->req_idx) : 0);


Cleaner to do

	u32 mask = BIT(...);
	...

	dmamux->used_chans |= mask;
	ret = r9a06g032_syscon_set_dmamux(mask, val ? mask : 0);

...

> +static const struct of_device_id rzn1_dmac_match[] __maybe_unused = {
> +	{ .compatible = "renesas,rzn1-dma", },
> +	{},

No comma for terminator entry.

> +};

...

> +	if (!node)
> +		return -ENODEV;

Dup check, why not to simply try for phandle first?

...

> +	if (of_property_read_u32(dmac_node, "dma-requests",
> +				 &dmamux->dmac_requests)) {

One line?

> +		dev_err(&pdev->dev, "Missing DMAC requests information\n");
> +		of_node_put(dmac_node);
> +		return -EINVAL;

First put node, then simply use dev_err_probe().

> +	}

...

> +static const struct of_device_id rzn1_dmamux_match[] = {
> +	{ .compatible = "renesas,rzn1-dmamux", },
> +	{},

No comma.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/8] soc: renesas: rzn1-sysc: Export function to set dmamux

[kernel test robot]

Hi Miquel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on geert-renesas-devel/next]
[also build test WARNING on geert-renesas-drivers/renesas-clk robh/for-next linus/master v5.17-rc4 next-20220217]
[cannot apply to vkoul-dmaengine/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Miquel-Raynal/RZN1-DMA-support/20220220-182519
base:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next
config: arc-randconfig-r043-20220220 (https://download.01.org/0day-ci/archive/20220221/202202210247.Ul5J6pwr-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ed9b880ea7f2b23b42feeed7a6ed898cd09ae2f1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Miquel-Raynal/RZN1-DMA-support/20220220-182519
        git checkout ed9b880ea7f2b23b42feeed7a6ed898cd09ae2f1
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash drivers/clk/renesas/ drivers/mtd/spi-nor/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/clk/renesas/r9a06g032-clocks.c:320:5: warning: no previous prototype for 'r9a06g032_syscon_set_dmamux' [-Wmissing-prototypes]
     320 | int r9a06g032_syscon_set_dmamux(u32 mask, u32 val)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/r9a06g032_syscon_set_dmamux +320 drivers/clk/renesas/r9a06g032-clocks.c

   317	
   318	/* Exported helper to access the DMAMUX register */
   319	static struct r9a06g032_priv *syscon_priv;
 > 320	int r9a06g032_syscon_set_dmamux(u32 mask, u32 val)
   321	{
   322		u32 dmamux;
   323	
   324		if (!syscon_priv)
   325			return -EPROBE_DEFER;
   326	
   327		spin_lock(&syscon_priv->lock);
   328	
   329		dmamux = readl(syscon_priv->reg + R9A06G032_SYSCON_DMAMUX);
   330		dmamux &= ~mask;
   331		dmamux |= val & mask;
   332		writel(dmamux, syscon_priv->reg + R9A06G032_SYSCON_DMAMUX);
   333	
   334		spin_unlock(&syscon_priv->lock);
   335	
   336		return 0;
   337	}
   338	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 3/8] soc: renesas: rzn1-sysc: Export function to set dmamux

[kernel test robot]

Hi Miquel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on geert-renesas-devel/next]
[also build test WARNING on geert-renesas-drivers/renesas-clk robh/for-next linus/master v5.17-rc4 next-20220217]
[cannot apply to vkoul-dmaengine/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Miquel-Raynal/RZN1-DMA-support/20220220-182519
base:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next
config: arm-randconfig-r022-20220220 (https://download.01.org/0day-ci/archive/20220221/202202210355.JzDJ9Lyz-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/ed9b880ea7f2b23b42feeed7a6ed898cd09ae2f1
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Miquel-Raynal/RZN1-DMA-support/20220220-182519
        git checkout ed9b880ea7f2b23b42feeed7a6ed898cd09ae2f1
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/clk/renesas/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/clk/renesas/r9a06g032-clocks.c:320:5: warning: no previous prototype for function 'r9a06g032_syscon_set_dmamux' [-Wmissing-prototypes]
   int r9a06g032_syscon_set_dmamux(u32 mask, u32 val)
       ^
   drivers/clk/renesas/r9a06g032-clocks.c:320:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int r9a06g032_syscon_set_dmamux(u32 mask, u32 val)
   ^
   static 
   1 warning generated.


vim +/r9a06g032_syscon_set_dmamux +320 drivers/clk/renesas/r9a06g032-clocks.c

   317	
   318	/* Exported helper to access the DMAMUX register */
   319	static struct r9a06g032_priv *syscon_priv;
 > 320	int r9a06g032_syscon_set_dmamux(u32 mask, u32 val)
   321	{
   322		u32 dmamux;
   323	
   324		if (!syscon_priv)
   325			return -EPROBE_DEFER;
   326	
   327		spin_lock(&syscon_priv->lock);
   328	
   329		dmamux = readl(syscon_priv->reg + R9A06G032_SYSCON_DMAMUX);
   330		dmamux &= ~mask;
   331		dmamux |= val & mask;
   332		writel(dmamux, syscon_priv->reg + R9A06G032_SYSCON_DMAMUX);
   333	
   334		spin_unlock(&syscon_priv->lock);
   335	
   336		return 0;
   337	}
   338	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 4/8] dma: dmamux: Introduce RZN1 DMA router support

[kernel test robot]

Hi Miquel,

I love your patch! Yet something to improve:

[auto build test ERROR on geert-renesas-devel/next]
[also build test ERROR on geert-renesas-drivers/renesas-clk robh/for-next linus/master v5.17-rc4 next-20220217]
[cannot apply to vkoul-dmaengine/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Miquel-Raynal/RZN1-DMA-support/20220220-182519
base:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next
config: microblaze-randconfig-r026-20220220 (https://download.01.org/0day-ci/archive/20220221/202202210543.GR8q2zw2-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/df0b7e58b46473e407c2c552f843d0628ad6875d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Miquel-Raynal/RZN1-DMA-support/20220220-182519
        git checkout df0b7e58b46473e407c2c552f843d0628ad6875d
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=microblaze SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   microblaze-linux-ld: drivers/dma/dw/dmamux.o: in function `rzn1_dmamux_init':
>> (.text+0x45c): multiple definition of `init_module'; drivers/dma/dw/platform.o:(.init.text+0x0): first defined here

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 2/8] dt-bindings: dma: Introduce RZN1 DMA compatible

[Rob Herring]

On Fri, 18 Feb 2022 19:12:20 +0100, Miquel Raynal wrote:
> Just like for the NAND controller that is also on this SoC, let's
> provide a SoC generic and a more specific couple of compatibles.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../devicetree/bindings/dma/snps,dma-spear1340.yaml       | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mtd/renesas-nandc.example.dt.yaml: nand-controller@40102000: '#dma-cells' is a required property
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mtd/renesas-nandc.example.dt.yaml: nand-controller@40102000: $nodename:0: 'nand-controller@40102000' does not match '^dma-controller(@.*)?$'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mtd/renesas-nandc.example.dt.yaml: nand-controller@40102000: clocks: [[4294967295, 117], [4294967295, 37]] is too long
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mtd/renesas-nandc.example.dt.yaml: nand-controller@40102000: clock-names: ['hclk', 'eclk'] is too long
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mtd/renesas-nandc.example.dt.yaml: nand-controller@40102000: Unevaluated properties are not allowed ('clocks', 'clock-names', '#address-cells', '#size-cells' were unexpected)
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mtd/renesas-nandc.example.dt.yaml: nand-controller@40102000: '#dma-cells' is a required property
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/snps,dma-spear1340.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1594847

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 4/8] dma: dmamux: Introduce RZN1 DMA router support

[kernel test robot]

Hi Miquel,

I love your patch! Yet something to improve:

[auto build test ERROR on geert-renesas-devel/next]
[also build test ERROR on geert-renesas-drivers/renesas-clk robh/for-next linus/master v5.17-rc5 next-20220217]
[cannot apply to vkoul-dmaengine/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Miquel-Raynal/RZN1-DMA-support/20220220-182519
base:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next
config: ia64-allmodconfig (https://download.01.org/0day-ci/archive/20220221/202202211236.07FjzSmp-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/df0b7e58b46473e407c2c552f843d0628ad6875d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Miquel-Raynal/RZN1-DMA-support/20220220-182519
        git checkout df0b7e58b46473e407c2c552f843d0628ad6875d
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ia64-linux-ld: drivers/dma/dw/dmamux.o: in function `rzn1_dmamux_init':
>> dmamux.c:(.text+0x9c0): multiple definition of `init_module'; drivers/dma/dw/platform.o:platform.c:(.init.text+0x0): first defined here

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

RE: [PATCH 5/8] dma: dw: Avoid partial transfers

[Phil Edworthy]

Hi Andy,

I wrote the patch a few years ago, but didn't get the time to upstream it.

I am not aware of a HW integration bug on the RZ/N1 device but can't rule it out. I am struggling to see what kind of HW issue this could be as, iirc, word accesses work fine when the size of the transfer is a multiple of the MEM width.

I found the issue when testing DMA with the UART transferring different amounts of data.

> > +		if (sconfig->dst_addr_width && sconfig->dst_addr_width <
> data_width)
> > +			data_width = sconfig->dst_addr_width;
> 
> But here no check that you do it for explicitly peripheral to memory, so
> this
> will affect memory to peripheral transfers as well.
No, this should be ok as this change is within:
	case DMA_DEV_TO_MEM:

BR
Phil

> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: 20 February 2022 10:50
> To: Miquel Raynal <miquel.raynal@bootlin.com>
> Cc: Viresh Kumar <vireshk@kernel.org>; Vinod Koul <vkoul@kernel.org>;
> Geert Uytterhoeven <geert+renesas@glider.be>; Magnus Damm
> <magnus.damm@gmail.com>; Michael Turquette <mturquette@baylibre.com>;
> Stephen Boyd <sboyd@kernel.org>; Rob Herring <robh+dt@kernel.org>;
> devicetree@vger.kernel.org; dmaengine@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; linux-clk@vger.kernel.org; Thomas Petazzoni
> <thomas.petazzoni@bootlin.com>; Milan Stevanovic
> <milan.stevanovic@se.com>; Jimmy Lalande <jimmy.lalande@se.com>; Laetitia
> MARIOTTINI <laetitia.mariottini@se.com>; Phil Edworthy
> <phil.edworthy@renesas.com>
> Subject: Re: [PATCH 5/8] dma: dw: Avoid partial transfers
> 
> On Fri, Feb 18, 2022 at 07:12:23PM +0100, Miquel Raynal wrote:
> > From: Phil Edworthy <phil.edworthy@renesas.com>
> >
> > Pausing a partial transfer only causes data to be written to mem that is
> > a multiple of the memory width setting.
> >
> > However, when a DMA client driver finishes DMA early, e.g. due to UART
> > char timeout interrupt, all data read from the DEV must be written to
> MEM.
> >
> > Therefore, allow the slave to limit the memory width to ensure all data
> > read from the DEV is written to MEM when DMA is paused.
> 
> Is this a fix?
> What happens to the data if you don't do this?
> As far as I understood the Synopsys DesignWare specification the DMA
> controller
> is capable of flushing FIFO in that case on byte-by-byte basis. Do you
> have an
> HW integration bug?
> 
> TL;DR: tell us more about this.
> 
> ...
> 
> > +		if (sconfig->dst_addr_width && sconfig->dst_addr_width <
> data_width)
> > +			data_width = sconfig->dst_addr_width;
> 
> But here no check that you do it for explicitly peripheral to memory, so
> this
> will affect memory to peripheral transfers as well.
> 
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH 3/8] soc: renesas: rzn1-sysc: Export function to set dmamux

[Geert Uytterhoeven]

Hi Miquel,

On Fri, Feb 18, 2022 at 7:12 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> The dmamux register is located within the system controller.
>
> Without syscon, we need an extra helper in order to give write access to
> this register to a dmamux driver.
>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks for your patch!

> --- a/drivers/clk/renesas/r9a06g032-clocks.c
> +++ b/drivers/clk/renesas/r9a06g032-clocks.c

Missing #include <linux/soc/renesas/r9a06g032-syscon.h>

> @@ -315,6 +315,27 @@ struct r9a06g032_priv {
>         void __iomem *reg;
>  };
>
> +/* Exported helper to access the DMAMUX register */
> +static struct r9a06g032_priv *syscon_priv;

I'd call this sysctrl_priv, as that matches the bindings and
binding header file name.

> +int r9a06g032_syscon_set_dmamux(u32 mask, u32 val)
> +{
> +       u32 dmamux;
> +
> +       if (!syscon_priv)
> +               return -EPROBE_DEFER;
> +
> +       spin_lock(&syscon_priv->lock);

This needs propection against interrupts => spin_lock_irqsave().

> +
> +       dmamux = readl(syscon_priv->reg + R9A06G032_SYSCON_DMAMUX);
> +       dmamux &= ~mask;
> +       dmamux |= val & mask;
> +       writel(dmamux, syscon_priv->reg + R9A06G032_SYSCON_DMAMUX);
> +
> +       spin_unlock(&syscon_priv->lock);
> +
> +       return 0;
> +}
> +
>  /* register/bit pairs are encoded as an uint16_t */
>  static void
>  clk_rdesc_set(struct r9a06g032_priv *clocks,

> --- a/include/dt-bindings/clock/r9a06g032-sysctrl.h
> +++ b/include/dt-bindings/clock/r9a06g032-sysctrl.h
> @@ -145,4 +145,6 @@
>  #define R9A06G032_CLK_UART6            152
>  #define R9A06G032_CLK_UART7            153
>
> +#define R9A06G032_SYSCON_DMAMUX                0xA0

I don't think this needs to be part of the bindings, so please move
it to the driver source file.

> --- /dev/null
> +++ b/include/linux/soc/renesas/r9a06g032-syscon.h

r9a06g032-sysctrl.h etc.

> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_SOC_RENESAS_R9A06G032_SYSCON_H__
> +#define __LINUX_SOC_RENESAS_R9A06G032_SYSCON_H__
> +
> +#ifdef CONFIG_CLK_R9A06G032
> +int r9a06g032_syscon_set_dmamux(u32 mask, u32 val);
> +#else
> +static inline int r9a06g032_syscon_set_dmamux(u32 mask, u32 val) { return -ENODEV; }
> +#endif
> +
> +#endif /* __LINUX_SOC_RENESAS_R9A06G032_SYSCON_H__ */
> --
> 2.27.0

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 5/8] dma: dw: Avoid partial transfers

[Miquel Raynal]

Hi Andy,

phil.edworthy@renesas.com wrote on Mon, 21 Feb 2022 08:14:47 +0000:

> Hi Andy,
> 
> I wrote the patch a few years ago, but didn't get the time to upstream it.
> 
> I am not aware of a HW integration bug on the RZ/N1 device but can't rule it out. I am struggling to see what kind of HW issue this could be as, iirc, word accesses work fine when the size of the transfer is a multiple of the MEM width.
> 
> I found the issue when testing DMA with the UART transferring different amounts of data.
> 
> > > +		if (sconfig->dst_addr_width && sconfig->dst_addr_width <  
> > data_width)  
> > > +			data_width = sconfig->dst_addr_width;  
> > 
> > But here no check that you do it for explicitly peripheral to memory, so
> > this
> > will affect memory to peripheral transfers as well.  
> No, this should be ok as this change is within:
> 	case DMA_DEV_TO_MEM:

I will add this to the commit log to clarify.

Thanks,
Miquèl

Re: [PATCH 2/8] dt-bindings: dma: Introduce RZN1 DMA compatible

[Miquel Raynal]

Hi Rob,

robh@kernel.org wrote on Sun, 20 Feb 2022 20:36:09 -0600:

> On Fri, 18 Feb 2022 19:12:20 +0100, Miquel Raynal wrote:
> > Just like for the NAND controller that is also on this SoC, let's
> > provide a SoC generic and a more specific couple of compatibles.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  .../devicetree/bindings/dma/snps,dma-spear1340.yaml       | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >   
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):

Copy-paste error. Now fixed. Soon the v2.

Thanks,
Miquèl

Re: [PATCH 3/8] soc: renesas: rzn1-sysc: Export function to set dmamux

[Miquel Raynal]

Hi Geert,

geert@linux-m68k.org wrote on Mon, 21 Feb 2022 10:16:24 +0100:

> Hi Miquel,
> 
> On Fri, Feb 18, 2022 at 7:12 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > The dmamux register is located within the system controller.
> >
> > Without syscon, we need an extra helper in order to give write access to
> > this register to a dmamux driver.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> Thanks for your patch!
> 
> > --- a/drivers/clk/renesas/r9a06g032-clocks.c
> > +++ b/drivers/clk/renesas/r9a06g032-clocks.c  
> 
> Missing #include <linux/soc/renesas/r9a06g032-syscon.h>
> 
> > @@ -315,6 +315,27 @@ struct r9a06g032_priv {
> >         void __iomem *reg;
> >  };
> >
> > +/* Exported helper to access the DMAMUX register */
> > +static struct r9a06g032_priv *syscon_priv;  
> 
> I'd call this sysctrl_priv, as that matches the bindings and
> binding header file name.

Ok.

> 
> > +int r9a06g032_syscon_set_dmamux(u32 mask, u32 val)
> > +{
> > +       u32 dmamux;
> > +
> > +       if (!syscon_priv)
> > +               return -EPROBE_DEFER;
> > +
> > +       spin_lock(&syscon_priv->lock);  
> 
> This needs propection against interrupts => spin_lock_irqsave().

Yes.

> 
> > +
> > +       dmamux = readl(syscon_priv->reg + R9A06G032_SYSCON_DMAMUX);
> > +       dmamux &= ~mask;
> > +       dmamux |= val & mask;
> > +       writel(dmamux, syscon_priv->reg + R9A06G032_SYSCON_DMAMUX);
> > +
> > +       spin_unlock(&syscon_priv->lock);
> > +
> > +       return 0;
> > +}
> > +
> >  /* register/bit pairs are encoded as an uint16_t */
> >  static void
> >  clk_rdesc_set(struct r9a06g032_priv *clocks,  
> 
> > --- a/include/dt-bindings/clock/r9a06g032-sysctrl.h
> > +++ b/include/dt-bindings/clock/r9a06g032-sysctrl.h
> > @@ -145,4 +145,6 @@
> >  #define R9A06G032_CLK_UART6            152
> >  #define R9A06G032_CLK_UART7            153
> >
> > +#define R9A06G032_SYSCON_DMAMUX                0xA0  
> 
> I don't think this needs to be part of the bindings, so please move
> it to the driver source file.

I've moved it to the top of the file. There definitions are a bit mixed
with the code, I don't like this, so I kept it at the top.

> 
> > --- /dev/null
> > +++ b/include/linux/soc/renesas/r9a06g032-syscon.h  
> 
> r9a06g032-sysctrl.h etc.

Done.

> 
> > @@ -0,0 +1,11 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __LINUX_SOC_RENESAS_R9A06G032_SYSCON_H__
> > +#define __LINUX_SOC_RENESAS_R9A06G032_SYSCON_H__
> > +
> > +#ifdef CONFIG_CLK_R9A06G032
> > +int r9a06g032_syscon_set_dmamux(u32 mask, u32 val);
> > +#else
> > +static inline int r9a06g032_syscon_set_dmamux(u32 mask, u32 val) { return -ENODEV; }
> > +#endif
> > +
> > +#endif /* __LINUX_SOC_RENESAS_R9A06G032_SYSCON_H__ */
> > --
> > 2.27.0  
> 
> 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


Thanks,
Miquèl

Re: [PATCH 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Miquel Raynal]

Hi Andy,

andriy.shevchenko@linux.intel.com wrote on Sun, 20 Feb 2022 12:56:02
+0200:

> On Fri, Feb 18, 2022 at 07:12:22PM +0100, Miquel Raynal wrote:
> > The Renesas RZN1 DMA IP is a based on a DW core, with eg. an additional
> > dmamux register located in the system control area which can take up to
> > 32 requests (16 per DMA controller). Each DMA channel can be wired to
> > two different peripherals.
> > 
> > We need two additional information from the 'dmas' property: the channel
> > (bit in the dmamux register) that must be accessed and the value of the
> > mux for this channel.  
> 
> ...

Thanks for the review!

> 
> > +dw_dmac-y			:= platform.o dmamux.o  
> 
> We do not need this on other platforms, please make sure we have no dangling
> code on, e.g., x86.
> 
> ...
> 
> > +	/* The of_node_put() will be done in the core for the node */
> > +	master = map->req_idx < dmamux->dmac_requests ? 0 : 1;  
> 
> The opposite conditional will be better, no?`

I guess this is a matter of taste. I prefer the current writing but I
will change it.

> 
> ...
> 
> > +	dmamux->used_chans |= BIT(map->req_idx);
> > +	ret = r9a06g032_syscon_set_dmamux(BIT(map->req_idx),
> > +					  val ? BIT(map->req_idx) : 0);  
> 
> 
> Cleaner to do
> 
> 	u32 mask = BIT(...);
> 	...
> 
> 	dmamux->used_chans |= mask;
> 	ret = r9a06g032_syscon_set_dmamux(mask, val ? mask : 0);

Fine.

> 
> ...
> 
> > +static const struct of_device_id rzn1_dmac_match[] __maybe_unused = {
> > +	{ .compatible = "renesas,rzn1-dma", },
> > +	{},  
> 
> No comma for terminator entry.

Mmh, ok.

> 
> > +};  
> 
> ...
> 
> > +	if (!node)
> > +		return -ENODEV;  
> 
> Dup check, why not to simply try for phandle first?

I'll drop it.

> 
> ...
> 
> > +	if (of_property_read_u32(dmac_node, "dma-requests",
> > +				 &dmamux->dmac_requests)) {  
> 
> One line?

Ok.

> 
> > +		dev_err(&pdev->dev, "Missing DMAC requests information\n");
> > +		of_node_put(dmac_node);
> > +		return -EINVAL;  
> 
> First put node, then simply use dev_err_probe().

I don't get the point here. dev_err_probe() is useful when -EPROBE_DEFER
can be returned, right? I don't understand what it would bring here nor
how I should use it to simplify error handling.

> 
> > +	}  
> 
> ...
> 
> > +static const struct of_device_id rzn1_dmamux_match[] = {
> > +	{ .compatible = "renesas,rzn1-dmamux", },
> > +	{},  
> 
> No comma.

Ok.

> 
> > +};  
> 


Thanks,
Miquèl

Re: [PATCH 4/8] dma: dmamux: Introduce RZN1 DMA router support

[Andy Shevchenko]

On Mon, Feb 21, 2022 at 04:13:20PM +0100, Miquel Raynal wrote:
> andriy.shevchenko@linux.intel.com wrote on Sun, 20 Feb 2022 12:56:02
> +0200:
> > On Fri, Feb 18, 2022 at 07:12:22PM +0100, Miquel Raynal wrote:

...

> > > +		dev_err(&pdev->dev, "Missing DMAC requests information\n");
> > > +		of_node_put(dmac_node);
> > > +		return -EINVAL;  
> > 
> > First put node, then simply use dev_err_probe().
> 
> I don't get the point here. dev_err_probe() is useful when -EPROBE_DEFER
> can be returned, right? I don't understand what it would bring here nor
> how I should use it to simplify error handling.

Less LOCs, and it's fine to call it here. This usecase is described in the
dev_err_probe() documentation.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/8] dma: dw: Avoid partial transfers

[Andy Shevchenko]

On Mon, Feb 21, 2022 at 08:14:47AM +0000, Phil Edworthy wrote:
> Hi Andy,
> 
> I wrote the patch a few years ago, but didn't get the time to upstream it.
> 
> I am not aware of a HW integration bug on the RZ/N1 device but can't rule it
> out. I am struggling to see what kind of HW issue this could be as, iirc,
> word accesses work fine when the size of the transfer is a multiple of the
> MEM width.
> 
> I found the issue when testing DMA with the UART transferring different amounts of data.

Can you tell more about the setup and test cases?

Also, which version of the DW DMAC IP is being used in this SoC?

...

> > > +		if (sconfig->dst_addr_width && sconfig->dst_addr_width <
> > data_width)
> > > +			data_width = sconfig->dst_addr_width;
> > 
> > But here no check that you do it for explicitly peripheral to memory, so
> > this
> > will affect memory to peripheral transfers as well.
> No, this should be ok as this change is within:
> 	case DMA_DEV_TO_MEM:

Ah, it's better. But still unclear to me why we need this.

P.S. Please avoid top-postings.

> > -----Original Message-----
> > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Sent: 20 February 2022 10:50
> > On Fri, Feb 18, 2022 at 07:12:23PM +0100, Miquel Raynal wrote:

> > > Pausing a partial transfer only causes data to be written to mem that is
> > > a multiple of the memory width setting.
> > >
> > > However, when a DMA client driver finishes DMA early, e.g. due to UART
> > > char timeout interrupt, all data read from the DEV must be written to
> > MEM.
> > >
> > > Therefore, allow the slave to limit the memory width to ensure all data
> > > read from the DEV is written to MEM when DMA is paused.
> > 
> > Is this a fix?
> > What happens to the data if you don't do this?
> > As far as I understood the Synopsys DesignWare specification the DMA
> > controller
> > is capable of flushing FIFO in that case on byte-by-byte basis. Do you
> > have an
> > HW integration bug?
> > 
> > TL;DR: tell us more about this.
> > 
> > ...
> > 
> > > +		if (sconfig->dst_addr_width && sconfig->dst_addr_width <
> > data_width)
> > > +			data_width = sconfig->dst_addr_width;
> > 
> > But here no check that you do it for explicitly peripheral to memory, so
> > this
> > will affect memory to peripheral transfers as well.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 5/8] dma: dw: Avoid partial transfers

[Phil Edworthy]

Hi Andy,

> > I found the issue when testing DMA with the UART transferring different
> amounts of data.
> 
> Can you tell more about the setup and test cases?
We had a loopback test from one uart to another. The test checks the
following transfer lengths (bytes):
1 to 33, 2043 to 2053, 4091 to 4101, 8187 to 8297, 16379 to 16389

I think 1 to 33 and 4095 to 4097 were enough to find the problem.

> Also, which version of the DW DMAC IP is being used in this SoC?
I'm still checking, but it looks to be 2.18b

Thanks
Phil

RE: [PATCH 5/8] dma: dw: Avoid partial transfers

[Phil Edworthy]

> > Also, which version of the DW DMAC IP is being used in this SoC?
> I'm still checking, but it looks to be 2.18b
Our HW people have told me it's v2.19a

Thanks
Phil

Re: [PATCH 5/8] dma: dw: Avoid partial transfers

[Andy Shevchenko]

On Wed, Feb 23, 2022 at 08:01:25AM +0000, Phil Edworthy wrote:
> > > Also, which version of the DW DMAC IP is being used in this SoC?
> > I'm still checking, but it looks to be 2.18b
> Our HW people have told me it's v2.19a

Thanks for both answers, I have commented the same patch in v2 with my
interpretation of what's going on. Let's continue there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/8] soc: renesas: rzn1-sysc: Export function to set dmamux

[Rob Herring]

On Fri, Feb 18, 2022 at 07:12:21PM +0100, Miquel Raynal wrote:
> The dmamux register is located within the system controller.
> 
> Without syscon, we need an extra helper in order to give write access to
> this register to a dmamux driver.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/clk/renesas/r9a06g032-clocks.c        | 27 +++++++++++++++++++
>  include/dt-bindings/clock/r9a06g032-sysctrl.h |  2 ++
>  include/linux/soc/renesas/r9a06g032-syscon.h  | 11 ++++++++
>  3 files changed, 40 insertions(+)
>  create mode 100644 include/linux/soc/renesas/r9a06g032-syscon.h

> diff --git a/include/dt-bindings/clock/r9a06g032-sysctrl.h b/include/dt-bindings/clock/r9a06g032-sysctrl.h
> index 90c0f3dc1ba1..609e7fe8fcb1 100644
> --- a/include/dt-bindings/clock/r9a06g032-sysctrl.h
> +++ b/include/dt-bindings/clock/r9a06g032-sysctrl.h
> @@ -145,4 +145,6 @@
>  #define R9A06G032_CLK_UART6		152
>  #define R9A06G032_CLK_UART7		153
>  
> +#define R9A06G032_SYSCON_DMAMUX		0xA0

That looks like a register offset? We generally don't put register 
offsets in DT.

> +
>  #endif /* __DT_BINDINGS_R9A06G032_SYSCTRL_H__ */

Re: [PATCH 3/8] soc: renesas: rzn1-sysc: Export function to set dmamux

[Miquel Raynal]

Hi Rob,

robh@kernel.org wrote on Fri, 25 Feb 2022 12:32:51 -0600:

> On Fri, Feb 18, 2022 at 07:12:21PM +0100, Miquel Raynal wrote:
> > The dmamux register is located within the system controller.
> > 
> > Without syscon, we need an extra helper in order to give write access to
> > this register to a dmamux driver.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/clk/renesas/r9a06g032-clocks.c        | 27 +++++++++++++++++++
> >  include/dt-bindings/clock/r9a06g032-sysctrl.h |  2 ++
> >  include/linux/soc/renesas/r9a06g032-syscon.h  | 11 ++++++++
> >  3 files changed, 40 insertions(+)
> >  create mode 100644 include/linux/soc/renesas/r9a06g032-syscon.h  
> 
> > diff --git a/include/dt-bindings/clock/r9a06g032-sysctrl.h b/include/dt-bindings/clock/r9a06g032-sysctrl.h
> > index 90c0f3dc1ba1..609e7fe8fcb1 100644
> > --- a/include/dt-bindings/clock/r9a06g032-sysctrl.h
> > +++ b/include/dt-bindings/clock/r9a06g032-sysctrl.h
> > @@ -145,4 +145,6 @@
> >  #define R9A06G032_CLK_UART6		152
> >  #define R9A06G032_CLK_UART7		153
> >  
> > +#define R9A06G032_SYSCON_DMAMUX		0xA0  
> 
> That looks like a register offset? We generally don't put register 
> offsets in DT.

This is a leftover, the offset is defined somewhere else now, I will
fix this.

> 
> > +
> >  #endif /* __DT_BINDINGS_R9A06G032_SYSCTRL_H__ */  


Thanks,
Miquèl