[PATCH v2 00/10] dmaengine: sun6i: Fixes for H3/A83T, enable A64

[PATCH v2 00/10] dmaengine: sun6i: Fixes for H3/A83T, enable A64

[Stefan Brüns]

Commit 3a03ea763a67 ("dmaengine: sun6i: Add support for Allwinner A83T
(sun8i) variant") and commit f008db8c00c1 ("dmaengine: sun6i: Add support for
Allwinner H3 (sun8i) variant") added support for the A83T resp. H3, but missed
some differences between the original A31 and A83T/H3.

The first patch adds a callback to the controller config to set the clock
autogating register of different SoC generations, i.e. A31, A23+A83T, H3+later,
and uses it to for the correct clock autogating setting.

The second patch adds a callback for the burst length setting in the channel
config register, which has different field offsets and new burst widths/lengths,
which differs between H3 and earlier generations

The third patch restructures some code required for the fourth patch and adds the
burst lengths to the controller config.

The fourth patch adds the burst widths to the config and adds the handling of the
H3 specific burst widths.

Patch 5 restructures the code to decouple some controller details (e.g. channel
count) from the compatible string/the config.

Patches 6, 7 and 8 introduce and use the "dma-chans" property for the A64. Although
register compatible to the H3, the channel count differs and thus it requires a
new compatible. To avoid introduction of new compatibles for each minor variation,
anything but the register model is moved to devicetree properties. There
is at least one SoC (R40) which can then reuse the A64 compatible, the same
would have worked for A83T+V3s.

Patches 9 and 10 add the DMA controller node to the devicetree and add the DMA
controller reference to the SPI nodes.

This patch series could be called v2, but the patches were split and significantly
restructured, thus listing changes individually is not to meaningful.

Changes in v2:
- Use callback for autogating instead of variable for different SoC generations
- Use controller specific callback for burst length setting
- Store burst lengths in config instead of device structure
- Store burst widths in config
- Set default number of dma-request if not provided in config or devicetree

Stefan Brüns (10):
  dmaengine: sun6i: Correct setting of clock autogating register for
    A83T/H3
  dmaengine: sun6i: Correct burst length field offsets for H3
  dmaengine: sun6i: Restructure code to allow extension for new SoCs
  dmaengine: sun6i: Enable additional burst lengths/widths on H3
  dmaengine: sun6i: Move number of pchans/vchans/request to device
    struct
  arm64: allwinner: a64: Add devicetree binding for DMA controller
  dmaengine: sun6i: Retrieve channel count/max request from devicetree
  dmaengine: sun6i: Add support for Allwinner A64 and compatibles
  arm64: allwinner: a64: Add device node for DMA controller
  arm64: allwinner: a64: add dma controller references to spi nodes

 .../devicetree/bindings/dma/sun6i-dma.txt          |  26 ++
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi      |  15 ++
 drivers/dma/sun6i-dma.c                            | 265 ++++++++++++++++-----
 3 files changed, 248 insertions(+), 58 deletions(-)

-- 
2.14.1

[PATCH v2 01/10] dmaengine: sun6i: Correct setting of clock autogating register for A83T/H3

[Stefan Brüns]

The H83T uses a compatible string different from the A23, but requires
the same clock autogating register setting.

The H3 also requires setting the clock autogating register, but has
the register at a different offset.

Add three suitable callbacks for the existing controller generations
and set it in the controller config structure.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/dma/sun6i-dma.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index bcd496edc70f..45bcd5271d94 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -48,6 +48,9 @@
 #define SUN8I_DMA_GATE		0x20
 #define SUN8I_DMA_GATE_ENABLE	0x4
 
+#define SUNXI_H3_SECURE_REG		0x20
+#define SUNXI_H3_DMA_GATE		0x28
+#define SUNXI_H3_DMA_GATE_ENABLE	0x4
 /*
  * Channels specific registers
  */
@@ -111,7 +114,7 @@ struct sun6i_dma_config {
 	 * however these SoCs really have and need this bit, as seen in the
 	 * BSP kernel source code.
 	 */
-	bool gate_needed;
+	void (*clock_autogate_enable)();
 };
 
 /*
@@ -267,6 +270,20 @@ static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width)
 	return addr_width >> 1;
 }
 
+static void sun6i_enable_clock_autogate_noop(struct sun6i_dma_dev *sdev)
+{
+}
+
+static void sun6i_enable_clock_autogate_a23(struct sun6i_dma_dev *sdev)
+{
+	writel(SUN8I_DMA_GATE_ENABLE, sdev->base + SUN8I_DMA_GATE);
+}
+
+static void sun6i_enable_clock_autogate_h3(struct sun6i_dma_dev *sdev)
+{
+	writel(SUNXI_H3_DMA_GATE_ENABLE, sdev->base + SUNXI_H3_DMA_GATE);
+}
+
 static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
 {
 	struct sun6i_desc *txd = pchan->desc;
@@ -1009,6 +1026,7 @@ static struct sun6i_dma_config sun6i_a31_dma_cfg = {
 	.nr_max_channels = 16,
 	.nr_max_requests = 30,
 	.nr_max_vchans   = 53,
+	.clock_autogate_enable = sun6i_enable_clock_autogate_noop;
 };
 
 /*
@@ -1020,24 +1038,28 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = {
 	.nr_max_channels = 8,
 	.nr_max_requests = 24,
 	.nr_max_vchans   = 37,
-	.gate_needed	 = true,
+	.clock_autogate_enable = sun6i_enable_clock_autogate_a23;
 };
 
 static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
 	.nr_max_channels = 8,
 	.nr_max_requests = 28,
 	.nr_max_vchans   = 39,
+	.clock_autogate_enable = sun6i_enable_clock_autogate_a23;
 };
 
 /*
  * The H3 has 12 physical channels, a maximum DRQ port id of 27,
  * and a total of 34 usable source and destination endpoints.
+ * It also supports additional burst lengths and bus widths,
+ * and the burst length fields have different offsets.
  */
 
 static struct sun6i_dma_config sun8i_h3_dma_cfg = {
 	.nr_max_channels = 12,
 	.nr_max_requests = 27,
 	.nr_max_vchans   = 34,
+	.clock_autogate_enable = sun6i_enable_clock_autogate_h3;
 };
 
 /*
@@ -1049,7 +1071,7 @@ static struct sun6i_dma_config sun8i_v3s_dma_cfg = {
 	.nr_max_channels = 8,
 	.nr_max_requests = 23,
 	.nr_max_vchans   = 24,
-	.gate_needed	 = true,
+	.clock_autogate_enable = sun6i_enable_clock_autogate_a23;
 };
 
 static const struct of_device_id sun6i_dma_match[] = {
@@ -1199,8 +1221,7 @@ static int sun6i_dma_probe(struct platform_device *pdev)
 		goto err_dma_unregister;
 	}
 
-	if (sdc->cfg->gate_needed)
-		writel(SUN8I_DMA_GATE_ENABLE, sdc->base + SUN8I_DMA_GATE);
+	sdc->cfg->clock_autogate_enable();
 
 	return 0;
 
-- 
2.14.1

[PATCH v2 02/10] dmaengine: sun6i: Correct burst length field offsets for H3

[Stefan Brüns]

For the H3, the burst lengths field offsets in the channel configuration
register differs from earlier SoC generations.

Using the A31 register macros actually configured the H3 controller
do to bursts of length 1 always, which although working leads to higher
bus utilisation.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/dma/sun6i-dma.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 45bcd5271d94..a6fc066a0ac6 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -68,13 +68,15 @@
 #define DMA_CHAN_CFG_SRC_DRQ(x)		((x) & 0x1f)
 #define DMA_CHAN_CFG_SRC_IO_MODE	BIT(5)
 #define DMA_CHAN_CFG_SRC_LINEAR_MODE	(0 << 5)
-#define DMA_CHAN_CFG_SRC_BURST(x)	(((x) & 0x3) << 7)
+#define DMA_CHAN_CFG_SRC_BURST_A31(x)	(((x) & 0x3) << 7)
+#define DMA_CHAN_CFG_SRC_BURST_H3(x)	(((x) & 0x3) << 6)
 #define DMA_CHAN_CFG_SRC_WIDTH(x)	(((x) & 0x3) << 9)
 
 #define DMA_CHAN_CFG_DST_DRQ(x)		(DMA_CHAN_CFG_SRC_DRQ(x) << 16)
 #define DMA_CHAN_CFG_DST_IO_MODE	(DMA_CHAN_CFG_SRC_IO_MODE << 16)
 #define DMA_CHAN_CFG_DST_LINEAR_MODE	(DMA_CHAN_CFG_SRC_LINEAR_MODE << 16)
-#define DMA_CHAN_CFG_DST_BURST(x)	(DMA_CHAN_CFG_SRC_BURST(x) << 16)
+#define DMA_CHAN_CFG_DST_BURST_A31(x)	(DMA_CHAN_CFG_SRC_BURST_A31(x) << 16)
+#define DMA_CHAN_CFG_DST_BURST_H3(x)	(DMA_CHAN_CFG_SRC_BURST_H3(x) << 16)
 #define DMA_CHAN_CFG_DST_WIDTH(x)	(DMA_CHAN_CFG_SRC_WIDTH(x) << 16)
 
 #define DMA_CHAN_CUR_SRC	0x10
@@ -115,6 +117,7 @@ struct sun6i_dma_config {
 	 * BSP kernel source code.
 	 */
 	void (*clock_autogate_enable)();
+	void (*set_burst_length)(u32 *p_cfg, s8 src_burst, s8 dst_burst);
 };
 
 /*
@@ -284,6 +287,18 @@ static void sun6i_enable_clock_autogate_h3(struct sun6i_dma_dev *sdev)
 	writel(SUNXI_H3_DMA_GATE_ENABLE, sdev->base + SUNXI_H3_DMA_GATE);
 }
 
+static void sun6i_set_burst_length_a31(u32 *p_cfg, s8 src_burst, s8 dst_burst)
+{
+	*p_cfg |= DMA_CHAN_CFG_SRC_BURST_A31(src_burst) |
+		  DMA_CHAN_CFG_DST_BURST_A31(dst_burst);
+}
+
+static void sun6i_set_burst_length_h3(u32 *p_cfg, s8 src_burst, s8 dst_burst)
+{
+	*p_cfg |= DMA_CHAN_CFG_SRC_BURST_H3(src_burst) |
+		  DMA_CHAN_CFG_DST_BURST_H3(dst_burst);
+}
+
 static size_t sun6i_get_chan_size(struct sun6i_pchan *pchan)
 {
 	struct sun6i_desc *txd = pchan->desc;
@@ -563,11 +578,11 @@ static int set_config(struct sun6i_dma_dev *sdev,
 	if (dst_width < 0)
 		return dst_width;
 
-	*p_cfg = DMA_CHAN_CFG_SRC_BURST(src_burst) |
-		DMA_CHAN_CFG_SRC_WIDTH(src_width) |
-		DMA_CHAN_CFG_DST_BURST(dst_burst) |
+	*p_cfg = DMA_CHAN_CFG_SRC_WIDTH(src_width) |
 		DMA_CHAN_CFG_DST_WIDTH(dst_width);
 
+	sdev->cfg->set_burst_length(p_cfg, src_burst, dst_burst);
+
 	return 0;
 }
 
@@ -610,11 +625,11 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_memcpy(
 		DMA_CHAN_CFG_DST_DRQ(DRQ_SDRAM) |
 		DMA_CHAN_CFG_DST_LINEAR_MODE |
 		DMA_CHAN_CFG_SRC_LINEAR_MODE |
-		DMA_CHAN_CFG_SRC_BURST(burst) |
 		DMA_CHAN_CFG_SRC_WIDTH(width) |
-		DMA_CHAN_CFG_DST_BURST(burst) |
 		DMA_CHAN_CFG_DST_WIDTH(width);
 
+	sdev->cfg->set_burst_length(v_lli->cfg, burst, burst);
+
 	sun6i_dma_lli_add(NULL, v_lli, p_lli, txd);
 
 	sun6i_dma_dump_lli(vchan, v_lli);
@@ -1027,6 +1042,7 @@ static struct sun6i_dma_config sun6i_a31_dma_cfg = {
 	.nr_max_requests = 30,
 	.nr_max_vchans   = 53,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_noop;
+	.set_burst_length = sun6i_set_burst_length_a31;
 };
 
 /*
@@ -1039,6 +1055,7 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = {
 	.nr_max_requests = 24,
 	.nr_max_vchans   = 37,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_a23;
+	.set_burst_length = sun6i_set_burst_length_a31;
 };
 
 static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
@@ -1046,13 +1063,12 @@ static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
 	.nr_max_requests = 28,
 	.nr_max_vchans   = 39,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_a23;
+	.set_burst_length = sun6i_set_burst_length_a31;
 };
 
 /*
  * The H3 has 12 physical channels, a maximum DRQ port id of 27,
  * and a total of 34 usable source and destination endpoints.
- * It also supports additional burst lengths and bus widths,
- * and the burst length fields have different offsets.
  */
 
 static struct sun6i_dma_config sun8i_h3_dma_cfg = {
@@ -1060,6 +1076,7 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
 	.nr_max_requests = 27,
 	.nr_max_vchans   = 34,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_h3;
+	.set_burst_length = sun6i_set_burst_length_h3;
 };
 
 /*
@@ -1072,6 +1089,7 @@ static struct sun6i_dma_config sun8i_v3s_dma_cfg = {
 	.nr_max_requests = 23,
 	.nr_max_vchans   = 24,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_a23;
+	.set_burst_length = sun6i_set_burst_length_a31;
 };
 
 static const struct of_device_id sun6i_dma_match[] = {
-- 
2.14.1

[PATCH v2 03/10] dmaengine: sun6i: Restructure code to allow extension for new SoCs

[Stefan Brüns]

The current code mixes three distinct operations when transforming
the slave config to register settings:

  1. special handling of DMA_SLAVE_BUSWIDTH_UNDEFINED, maxburst == 0
  2. range checking
  3. conversion of raw to register values

As the range checks depend on the specific SoC, move these out of the
conversion to distinct operations.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/dma/sun6i-dma.c | 66 ++++++++++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 28 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index a6fc066a0ac6..663f4b0b450e 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -118,6 +118,8 @@ struct sun6i_dma_config {
 	 */
 	void (*clock_autogate_enable)();
 	void (*set_burst_length)(u32 *p_cfg, s8 src_burst, s8 dst_burst);
+	u32 src_burst_lengths;
+	u32 dst_burst_lengths;
 };
 
 /*
@@ -266,10 +268,6 @@ static inline s8 convert_burst(u32 maxburst)
 
 static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width)
 {
-	if ((addr_width < DMA_SLAVE_BUSWIDTH_1_BYTE) ||
-	    (addr_width > DMA_SLAVE_BUSWIDTH_4_BYTES))
-		return -EINVAL;
-
 	return addr_width >> 1;
 }
 
@@ -542,41 +540,43 @@ static int set_config(struct sun6i_dma_dev *sdev,
 			enum dma_transfer_direction direction,
 			u32 *p_cfg)
 {
+	enum dma_slave_buswidth src_addr_width, dst_addr_width;
+	u32 src_maxburst, dst_maxburst;
 	s8 src_width, dst_width, src_burst, dst_burst;
 
+	src_addr_width = sconfig->src_addr_width;
+	dst_addr_width = sconfig->dst_addr_width;
+	src_maxburst = sconfig->src_maxburst;
+	dst_maxburst = sconfig->dst_maxburst;
+
 	switch (direction) {
 	case DMA_MEM_TO_DEV:
-		src_burst = convert_burst(sconfig->src_maxburst ?
-					sconfig->src_maxburst : 8);
-		src_width = convert_buswidth(sconfig->src_addr_width !=
-						DMA_SLAVE_BUSWIDTH_UNDEFINED ?
-				sconfig->src_addr_width :
-				DMA_SLAVE_BUSWIDTH_4_BYTES);
-		dst_burst = convert_burst(sconfig->dst_maxburst);
-		dst_width = convert_buswidth(sconfig->dst_addr_width);
+		if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
+			src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		src_maxburst = src_maxburst ? src_maxburst : 8;
 		break;
 	case DMA_DEV_TO_MEM:
-		src_burst = convert_burst(sconfig->src_maxburst);
-		src_width = convert_buswidth(sconfig->src_addr_width);
-		dst_burst = convert_burst(sconfig->dst_maxburst ?
-					sconfig->dst_maxburst : 8);
-		dst_width = convert_buswidth(sconfig->dst_addr_width !=
-						DMA_SLAVE_BUSWIDTH_UNDEFINED ?
-				sconfig->dst_addr_width :
-				DMA_SLAVE_BUSWIDTH_4_BYTES);
+		if (dst_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
+			dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		dst_maxburst = dst_maxburst ? dst_maxburst : 8;
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	if (src_burst < 0)
-		return src_burst;
-	if (src_width < 0)
-		return src_width;
-	if (dst_burst < 0)
-		return dst_burst;
-	if (dst_width < 0)
-		return dst_width;
+	if (!(BIT(src_addr_width) & sdev->slave.src_addr_widths))
+		return -EINVAL;
+	if (!(BIT(dst_addr_width) & sdev->slave.dst_addr_widths))
+		return -EINVAL;
+	if (!(BIT(src_maxburst) & sdev->cfg->src_burst_lengths))
+		return -EINVAL;
+	if (!(BIT(dst_maxburst) & sdev->cfg->dst_burst_lengths))
+		return -EINVAL;
+
+	src_width = convert_buswidth(src_addr_width);
+	dst_width = convert_buswidth(dst_addr_width);
+	dst_burst = convert_burst(dst_maxburst);
+	src_burst = convert_burst(src_maxburst);
 
 	*p_cfg = DMA_CHAN_CFG_SRC_WIDTH(src_width) |
 		DMA_CHAN_CFG_DST_WIDTH(dst_width);
@@ -1043,6 +1043,8 @@ static struct sun6i_dma_config sun6i_a31_dma_cfg = {
 	.nr_max_vchans   = 53,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_noop;
 	.set_burst_length = sun6i_set_burst_length_a31;
+	.src_burst_lengths = BIT(1) | BIT(8);
+	.dst_burst_lengths = BIT(1) | BIT(8);
 };
 
 /*
@@ -1056,6 +1058,8 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = {
 	.nr_max_vchans   = 37,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_a23;
 	.set_burst_length = sun6i_set_burst_length_a31;
+	.src_burst_lengths = BIT(1) | BIT(8);
+	.dst_burst_lengths = BIT(1) | BIT(8);
 };
 
 static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
@@ -1064,6 +1068,8 @@ static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
 	.nr_max_vchans   = 39,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_a23;
 	.set_burst_length = sun6i_set_burst_length_a31;
+	.src_burst_lengths = BIT(1) | BIT(8);
+	.dst_burst_lengths = BIT(1) | BIT(8);
 };
 
 /*
@@ -1077,6 +1083,8 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
 	.nr_max_vchans   = 34,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_h3;
 	.set_burst_length = sun6i_set_burst_length_h3;
+	.src_burst_lengths = BIT(1) | BIT(8);
+	.dst_burst_lengths = BIT(1) | BIT(8);
 };
 
 /*
@@ -1090,6 +1098,8 @@ static struct sun6i_dma_config sun8i_v3s_dma_cfg = {
 	.nr_max_vchans   = 24,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_a23;
 	.set_burst_length = sun6i_set_burst_length_a31;
+	.src_burst_lengths = BIT(1) | BIT(8);
+	.dst_burst_lengths = BIT(1) | BIT(8);
 };
 
 static const struct of_device_id sun6i_dma_match[] = {
-- 
2.14.1

[PATCH v2 04/10] dmaengine: sun6i: Enable additional burst lengths/widths on H3

[Stefan Brüns]

The H3 supports bursts lengths of 1, 4, 8 and 16 transfers, each with
a width of 1, 2, 4 or 8 bytes.

The register value for the the width is log2-encoded, change the
conversion function to provide the correct value for width == 8.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/dma/sun6i-dma.c | 54 ++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 663f4b0b450e..f2ee914cd755 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -120,6 +120,8 @@ struct sun6i_dma_config {
 	void (*set_burst_length)(u32 *p_cfg, s8 src_burst, s8 dst_burst);
 	u32 src_burst_lengths;
 	u32 dst_burst_lengths;
+	u32 src_addr_widths;
+	u32 dst_addr_widths;
 };
 
 /*
@@ -259,8 +261,12 @@ static inline s8 convert_burst(u32 maxburst)
 	switch (maxburst) {
 	case 1:
 		return 0;
+	case 4:
+		return 1;
 	case 8:
 		return 2;
+	case 16:
+		return 3;
 	default:
 		return -EINVAL;
 	}
@@ -268,7 +274,7 @@ static inline s8 convert_burst(u32 maxburst)
 
 static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width)
 {
-	return addr_width >> 1;
+	return ilog2(addr_width);
 }
 
 static void sun6i_enable_clock_autogate_noop(struct sun6i_dma_dev *sdev)
@@ -1045,6 +1051,12 @@ static struct sun6i_dma_config sun6i_a31_dma_cfg = {
 	.set_burst_length = sun6i_set_burst_length_a31;
 	.src_burst_lengths = BIT(1) | BIT(8);
 	.dst_burst_lengths = BIT(1) | BIT(8);
+	.src_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
 };
 
 /*
@@ -1060,6 +1072,12 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = {
 	.set_burst_length = sun6i_set_burst_length_a31;
 	.src_burst_lengths = BIT(1) | BIT(8);
 	.dst_burst_lengths = BIT(1) | BIT(8);
+	.src_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
 };
 
 static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
@@ -1070,11 +1088,19 @@ static struct sun6i_dma_config sun8i_a83t_dma_cfg = {
 	.set_burst_length = sun6i_set_burst_length_a31;
 	.src_burst_lengths = BIT(1) | BIT(8);
 	.dst_burst_lengths = BIT(1) | BIT(8);
+	.src_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
 };
 
 /*
  * The H3 has 12 physical channels, a maximum DRQ port id of 27,
  * and a total of 34 usable source and destination endpoints.
+ * It also supports additional burst lengths and bus widths,
+ * and the burst length fields have different offsets.
  */
 
 static struct sun6i_dma_config sun8i_h3_dma_cfg = {
@@ -1083,8 +1109,16 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
 	.nr_max_vchans   = 34,
 	.clock_autogate_enable = sun6i_enable_clock_autogate_h3;
 	.set_burst_length = sun6i_set_burst_length_h3;
-	.src_burst_lengths = BIT(1) | BIT(8);
-	.dst_burst_lengths = BIT(1) | BIT(8);
+	.src_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16);
+	.dst_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16);
+	.src_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
+	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
 };
 
 /*
@@ -1100,6 +1134,12 @@ static struct sun6i_dma_config sun8i_v3s_dma_cfg = {
 	.set_burst_length = sun6i_set_burst_length_a31;
 	.src_burst_lengths = BIT(1) | BIT(8);
 	.dst_burst_lengths = BIT(1) | BIT(8);
+	.src_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
 };
 
 static const struct of_device_id sun6i_dma_match[] = {
@@ -1179,12 +1219,8 @@ static int sun6i_dma_probe(struct platform_device *pdev)
 	sdc->slave.device_pause			= sun6i_dma_pause;
 	sdc->slave.device_resume		= sun6i_dma_resume;
 	sdc->slave.device_terminate_all		= sun6i_dma_terminate_all;
-	sdc->slave.src_addr_widths		= BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
-						  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
-						  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
-	sdc->slave.dst_addr_widths		= BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
-						  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
-						  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	sdc->slave.src_addr_widths		= sdc->cfg->src_addr_widths;
+	sdc->slave.dst_addr_widths		= sdc->cfg->dst_addr_widths;
 	sdc->slave.directions			= BIT(DMA_DEV_TO_MEM) |
 						  BIT(DMA_MEM_TO_DEV);
 	sdc->slave.residue_granularity		= DMA_RESIDUE_GRANULARITY_BURST;
-- 
2.14.1

[PATCH v2 05/10] dmaengine: sun6i: Move number of pchans/vchans/request to device struct

[Stefan Brüns]

Preparatory patch: If the same compatible is used for different SoCs which
have a common register layout, but different number of channels, the
channel count can no longer be stored in the config. Store it in the
device structure instead.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/dma/sun6i-dma.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index f2ee914cd755..245a147f718f 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -185,6 +185,9 @@ struct sun6i_dma_dev {
 	struct sun6i_pchan	*pchans;
 	struct sun6i_vchan	*vchans;
 	const struct sun6i_dma_config *cfg;
+	u32			num_pchans;
+	u32			num_vchans;
+	u32			max_request;
 };
 
 static struct device *chan2dev(struct dma_chan *chan)
@@ -435,7 +438,6 @@ static int sun6i_dma_start_desc(struct sun6i_vchan *vchan)
 static void sun6i_dma_tasklet(unsigned long data)
 {
 	struct sun6i_dma_dev *sdev = (struct sun6i_dma_dev *)data;
-	const struct sun6i_dma_config *cfg = sdev->cfg;
 	struct sun6i_vchan *vchan;
 	struct sun6i_pchan *pchan;
 	unsigned int pchan_alloc = 0;
@@ -463,7 +465,7 @@ static void sun6i_dma_tasklet(unsigned long data)
 	}
 
 	spin_lock_irq(&sdev->lock);
-	for (pchan_idx = 0; pchan_idx < cfg->nr_max_channels; pchan_idx++) {
+	for (pchan_idx = 0; pchan_idx < sdev->num_pchans; pchan_idx++) {
 		pchan = &sdev->pchans[pchan_idx];
 
 		if (pchan->vchan || list_empty(&sdev->pending))
@@ -484,7 +486,7 @@ static void sun6i_dma_tasklet(unsigned long data)
 	}
 	spin_unlock_irq(&sdev->lock);
 
-	for (pchan_idx = 0; pchan_idx < cfg->nr_max_channels; pchan_idx++) {
+	for (pchan_idx = 0; pchan_idx < sdev->num_pchans; pchan_idx++) {
 		if (!(pchan_alloc & BIT(pchan_idx)))
 			continue;
 
@@ -506,7 +508,7 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
 	int i, j, ret = IRQ_NONE;
 	u32 status;
 
-	for (i = 0; i < sdev->cfg->nr_max_channels / DMA_IRQ_CHAN_NR; i++) {
+	for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {
 		status = readl(sdev->base + DMA_IRQ_STAT(i));
 		if (!status)
 			continue;
@@ -986,7 +988,7 @@ static struct dma_chan *sun6i_dma_of_xlate(struct of_phandle_args *dma_spec,
 	struct dma_chan *chan;
 	u8 port = dma_spec->args[0];
 
-	if (port > sdev->cfg->nr_max_requests)
+	if (port > sdev->max_request)
 		return NULL;
 
 	chan = dma_get_any_slave_channel(&sdev->slave);
@@ -1019,7 +1021,7 @@ static inline void sun6i_dma_free(struct sun6i_dma_dev *sdev)
 {
 	int i;
 
-	for (i = 0; i < sdev->cfg->nr_max_vchans; i++) {
+	for (i = 0; i < sdev->num_vchans; i++) {
 		struct sun6i_vchan *vchan = &sdev->vchans[i];
 
 		list_del(&vchan->vc.chan.device_node);
@@ -1226,26 +1228,30 @@ static int sun6i_dma_probe(struct platform_device *pdev)
 	sdc->slave.residue_granularity		= DMA_RESIDUE_GRANULARITY_BURST;
 	sdc->slave.dev = &pdev->dev;
 
-	sdc->pchans = devm_kcalloc(&pdev->dev, sdc->cfg->nr_max_channels,
+	sdc->num_pchans = sdc->cfg->nr_max_channels;
+	sdc->num_vchans = sdc->cfg->nr_max_vchans;
+	sdc->max_request = sdc->cfg->nr_max_requests;
+
+	sdc->pchans = devm_kcalloc(&pdev->dev, sdc->num_pchans,
 				   sizeof(struct sun6i_pchan), GFP_KERNEL);
 	if (!sdc->pchans)
 		return -ENOMEM;
 
-	sdc->vchans = devm_kcalloc(&pdev->dev, sdc->cfg->nr_max_vchans,
+	sdc->vchans = devm_kcalloc(&pdev->dev, sdc->num_vchans,
 				   sizeof(struct sun6i_vchan), GFP_KERNEL);
 	if (!sdc->vchans)
 		return -ENOMEM;
 
 	tasklet_init(&sdc->task, sun6i_dma_tasklet, (unsigned long)sdc);
 
-	for (i = 0; i < sdc->cfg->nr_max_channels; i++) {
+	for (i = 0; i < sdc->num_pchans; i++) {
 		struct sun6i_pchan *pchan = &sdc->pchans[i];
 
 		pchan->idx = i;
 		pchan->base = sdc->base + 0x100 + i * 0x40;
 	}
 
-	for (i = 0; i < sdc->cfg->nr_max_vchans; i++) {
+	for (i = 0; i < sdc->num_vchans; i++) {
 		struct sun6i_vchan *vchan = &sdc->vchans[i];
 
 		INIT_LIST_HEAD(&vchan->node);
-- 
2.14.1

[PATCH v2 06/10] arm64: allwinner: a64: Add devicetree binding for DMA controller

[Stefan Brüns]

The A64 is register compatible with the H3, but has a different number
of dma channels and request ports.

Attach additional properties to the node to allow future reuse of the
compatible for controllers with different number of channels/requests.

If dma-requests is not specified, the register layout defined maximum
of 32 is used.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 .../devicetree/bindings/dma/sun6i-dma.txt          | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
index 98fbe1a5c6dd..6ebc79f95202 100644
--- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
+++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
@@ -27,6 +27,32 @@ Example:
 		#dma-cells = <1>;
 	};
 
+------------------------------------------------------------------------------
+For A64 DMA controller:
+
+Required properties:
+- compatible:	"allwinner,sun50i-a64-dma"
+- dma-channels: Number of DMA channels supported by the controller.
+		Refer to Documentation/devicetree/bindings/dma/dma.txt
+- all properties above, i.e. reg, interrupts, clocks, resets and #dma-cells
+
+Optional properties:
+- dma-requests: Number of DMA request signals supported by the controller.
+		Refer to Documentation/devicetree/bindings/dma/dma.txt
+
+Example:
+	dma: dma-controller@01c02000 {
+		compatible = "allwinner,sun50i-a64-dma";
+		reg = <0x01c02000 0x1000>;
+		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&ccu CLK_BUS_DMA>;
+		dma-channels = <8>;
+		dma-requests = <27>;
+		resets = <&ccu RST_BUS_DMA>;
+		#dma-cells = <1>;
+	};
+------------------------------------------------------------------------------
+
 Clients:
 
 DMA clients connected to the A31 DMA controller must use the format
-- 
2.14.1

[PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

[Stefan Brüns]

To avoid introduction of a new compatible for each small SoC/DMA controller
variation, move the definition of the channel count to the devicetree.

The number of vchans is no longer explicit, but limited by the highest
port/DMA request number. The result is a slight overallocation for SoCs
with a sparse port mapping.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/dma/sun6i-dma.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 245a147f718f..b5ecc97a0d5a 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -42,6 +42,9 @@
 
 #define DMA_STAT		0x30
 
+/* Offset between DMA_IRQ_EN and DMA_IRQ_STAT limits number of channels */
+#define DMA_MAX_CHANNELS	(DMA_IRQ_CHAN_NR * 0x10 / 4)
+
 /*
  * sun8i specific registers
  */
@@ -65,7 +68,8 @@
 #define DMA_CHAN_LLI_ADDR	0x08
 
 #define DMA_CHAN_CUR_CFG	0x0c
-#define DMA_CHAN_CFG_SRC_DRQ(x)		((x) & 0x1f)
+#define DMA_CHAN_MAX_DRQ		0x1f
+#define DMA_CHAN_CFG_SRC_DRQ(x)		((x) & DMA_CHAN_MAX_DRQ)
 #define DMA_CHAN_CFG_SRC_IO_MODE	BIT(5)
 #define DMA_CHAN_CFG_SRC_LINEAR_MODE	(0 << 5)
 #define DMA_CHAN_CFG_SRC_BURST_A31(x)	(((x) & 0x3) << 7)
@@ -1157,6 +1161,7 @@ MODULE_DEVICE_TABLE(of, sun6i_dma_match);
 static int sun6i_dma_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *device;
+	struct device_node *np = pdev->dev.of_node;
 	struct sun6i_dma_dev *sdc;
 	struct resource *res;
 	int ret, i;
@@ -1232,6 +1237,36 @@ static int sun6i_dma_probe(struct platform_device *pdev)
 	sdc->num_vchans = sdc->cfg->nr_max_vchans;
 	sdc->max_request = sdc->cfg->nr_max_requests;
 
+	ret = of_property_read_u32(np, "dma-channels", &sdc->num_pchans);
+	if (ret && !sdc->num_pchans) {
+		dev_err(&pdev->dev, "Can't get dma-channels.\n");
+		return ret;
+	}
+
+	if (sdc->num_pchans > DMA_MAX_CHANNELS) {
+		dev_err(&pdev->dev, "Number of dma-channels out of range.\n");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u32(np, "dma-requests", &sdc->max_request);
+	if (ret && !sdc->max_request) {
+		dev_info(&pdev->dev, "Missing dma-requests, using %u.\n",
+			 DMA_CHAN_MAX_DRQ);
+		sdc->max_request = DMA_CHAN_MAX_DRQ;
+	}
+
+	if (sdc->max_request > DMA_CHAN_MAX_DRQ) {
+		dev_err(&pdev->dev, "Value of dma-requests out of range.\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * If the number of vchans is not specified, derive it from the
+	 * highest port number, at most one channel per port and direction.
+	 */
+	if (!sdc->num_vchans)
+		sdc->num_vchans = 2 * (sdc->max_request + 1);
+
 	sdc->pchans = devm_kcalloc(&pdev->dev, sdc->num_pchans,
 				   sizeof(struct sun6i_pchan), GFP_KERNEL);
 	if (!sdc->pchans)
-- 
2.14.1

[PATCH v2 08/10] dmaengine: sun6i: Add support for Allwinner A64 and compatibles

[Stefan Brüns]

The A64 SoC has the same dma engine as the H3 (sun8i), with a
reduced amount of physical channels. To allow future reuse of the
compatible, leave the channel count etc. in the config data blank
and retrieve it from the devicetree.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/dma/sun6i-dma.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index b5ecc97a0d5a..118b29bb1eac 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -1127,6 +1127,28 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
 			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
 };
 
+/*
+ * The A64 binding uses the number of dma channels from the
+ * device tree node.
+ */
+static struct sun6i_dma_config sun50i_a64_dma_cfg = {
+	.nr_max_channels = 0,
+	.nr_max_requests = 0,
+	.nr_max_vchans   = 0,
+	.clock_autogate_enable = sun6i_enable_clock_autogate_h3;
+	.set_burst_length = sun6i_set_burst_length_h3;
+	.src_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16);
+	.dst_burst_lengths = BIT(1) | BIT(4) | BIT(8) | BIT(16);
+	.src_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
+	.dst_addr_widths   = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			     BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
+			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
+};
+
 /*
  * The V3s have only 8 physical channels, a maximum DRQ port id of 23,
  * and a total of 24 usable source and destination endpoints.
@@ -1154,6 +1176,7 @@ static const struct of_device_id sun6i_dma_match[] = {
 	{ .compatible = "allwinner,sun8i-a83t-dma", .data = &sun8i_a83t_dma_cfg },
 	{ .compatible = "allwinner,sun8i-h3-dma", .data = &sun8i_h3_dma_cfg },
 	{ .compatible = "allwinner,sun8i-v3s-dma", .data = &sun8i_v3s_dma_cfg },
+	{ .compatible = "allwinner,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, sun6i_dma_match);
-- 
2.14.1

[PATCH v2 09/10] arm64: allwinner: a64: Add device node for DMA controller

[Stefan Brüns]

The A64 SoC has a DMA controller that supports 8 DMA channels
to and from various peripherals. The last used DRQ port is 27.

Add a device node for it.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index e9a9d7fb01c8..4f060ecdb061 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -136,6 +136,17 @@
 			reg = <0x01c00000 0x1000>;
 		};
 
+		dma: dma-controller@01c02000 {
+			compatible = "allwinner,sun50i-a64-dma";
+			reg = <0x01c02000 0x1000>;
+			interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_DMA>;
+			dma-channels = <8>;
+			dma-requests = <27>;
+			resets = <&ccu RST_BUS_DMA>;
+			#dma-cells = <1>;
+		};
+
 		mmc0: mmc@1c0f000 {
 			compatible = "allwinner,sun50i-a64-mmc";
 			reg = <0x01c0f000 0x1000>;
-- 
2.14.1

[PATCH v2 10/10] arm64: allwinner: a64: add dma controller references to spi nodes

[Stefan Brüns]

The spi controller nodes omit the dma controller/channel references, add
it.

This does not yet enable DMA for SPI transfers, as the spi-sun6i driver
lacks support for DMA, but always uses PIO to the FIFO.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 4f060ecdb061..ec71c48b393d 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -476,6 +476,8 @@
 			interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&ccu CLK_BUS_SPI0>, <&ccu CLK_SPI0>;
 			clock-names = "ahb", "mod";
+			dmas = <&dma 23>, <&dma 23>;
+			dma-names = "rx", "tx";
 			pinctrl-names = "default";
 			pinctrl-0 = <&spi0_pins>;
 			resets = <&ccu RST_BUS_SPI0>;
@@ -491,6 +493,8 @@
 			interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&ccu CLK_BUS_SPI1>, <&ccu CLK_SPI1>;
 			clock-names = "ahb", "mod";
+			dmas = <&dma 24>, <&dma 24>;
+			dma-names = "rx", "tx";
 			pinctrl-names = "default";
 			pinctrl-0 = <&spi1_pins>;
 			resets = <&ccu RST_BUS_SPI1>;
-- 
2.14.1

Re: [PATCH v2 01/10] dmaengine: sun6i: Correct setting of clock autogating register for A83T/H3

[Maxime Ripard]

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

Hi,

On Sun, Sep 17, 2017 at 05:19:47AM +0200, Stefan Brüns wrote:
> The H83T uses a compatible string different from the A23, but requires
> the same clock autogating register setting.
> 
> The H3 also requires setting the clock autogating register, but has
> the register at a different offset.
> 
> Add three suitable callbacks for the existing controller generations
> and set it in the controller config structure.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
>  drivers/dma/sun6i-dma.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index bcd496edc70f..45bcd5271d94 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -48,6 +48,9 @@
>  #define SUN8I_DMA_GATE		0x20
>  #define SUN8I_DMA_GATE_ENABLE	0x4
>  
> +#define SUNXI_H3_SECURE_REG		0x20
> +#define SUNXI_H3_DMA_GATE		0x28
> +#define SUNXI_H3_DMA_GATE_ENABLE	0x4
>  /*
>   * Channels specific registers
>   */
> @@ -111,7 +114,7 @@ struct sun6i_dma_config {
>  	 * however these SoCs really have and need this bit, as seen in the
>  	 * BSP kernel source code.
>  	 */
> -	bool gate_needed;
> +	void (*clock_autogate_enable)();
>  };
>  
>  /*
> @@ -267,6 +270,20 @@ static inline s8 convert_buswidth(enum dma_slave_buswidth addr_width)
>  	return addr_width >> 1;
>  }
>  
> +static void sun6i_enable_clock_autogate_noop(struct sun6i_dma_dev *sdev)
> +{
> +}

I guess instead of that one, we could just test for the pointer and
not call the function if it's NULL?

Looks good otherwise, once fixed, you have
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 02/10] dmaengine: sun6i: Correct burst length field offsets for H3

[Maxime Ripard]

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

On Sun, Sep 17, 2017 at 05:19:48AM +0200, Stefan Brüns wrote:
> For the H3, the burst lengths field offsets in the channel configuration
> register differs from earlier SoC generations.
> 
> Using the A31 register macros actually configured the H3 controller
> do to bursts of length 1 always, which although working leads to higher
> bus utilisation.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 03/10] dmaengine: sun6i: Restructure code to allow extension for new SoCs

[Maxime Ripard]

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

On Sun, Sep 17, 2017 at 05:19:49AM +0200, Stefan Brüns wrote:
> The current code mixes three distinct operations when transforming
> the slave config to register settings:
> 
>   1. special handling of DMA_SLAVE_BUSWIDTH_UNDEFINED, maxburst == 0
>   2. range checking
>   3. conversion of raw to register values
> 
> As the range checks depend on the specific SoC, move these out of the
> conversion to distinct operations.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 04/10] dmaengine: sun6i: Enable additional burst lengths/widths on H3

[Maxime Ripard]

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

On Sun, Sep 17, 2017 at 05:19:50AM +0200, Stefan Brüns wrote:
> The H3 supports bursts lengths of 1, 4, 8 and 16 transfers, each with
> a width of 1, 2, 4 or 8 bytes.
> 
> The register value for the the width is log2-encoded, change the
> conversion function to provide the correct value for width == 8.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 06/10] arm64: allwinner: a64: Add devicetree binding for DMA controller

[Maxime Ripard]

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

On Sun, Sep 17, 2017 at 05:19:52AM +0200, Stefan Brüns wrote:
> The A64 is register compatible with the H3, but has a different number
> of dma channels and request ports.
> 
> Attach additional properties to the node to allow future reuse of the
> compatible for controllers with different number of channels/requests.
> 
> If dma-requests is not specified, the register layout defined maximum
> of 32 is used.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
>  .../devicetree/bindings/dma/sun6i-dma.txt          | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> index 98fbe1a5c6dd..6ebc79f95202 100644
> --- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> @@ -27,6 +27,32 @@ Example:
>  		#dma-cells = <1>;
>  	};
>  
> +------------------------------------------------------------------------------
> +For A64 DMA controller:
> +
> +Required properties:
> +- compatible:	"allwinner,sun50i-a64-dma"
> +- dma-channels: Number of DMA channels supported by the controller.
> +		Refer to Documentation/devicetree/bindings/dma/dma.txt
> +- all properties above, i.e. reg, interrupts, clocks, resets and #dma-cells
> +
> +Optional properties:
> +- dma-requests: Number of DMA request signals supported by the controller.
> +		Refer to Documentation/devicetree/bindings/dma/dma.txt

You're error'ing out if dma-requests, so it isn't optional. I guess we
should just make it mandatory.

Once done,
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 05/10] dmaengine: sun6i: Move number of pchans/vchans/request to device struct

[Maxime Ripard]

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

1;4803;0c
On Sun, Sep 17, 2017 at 05:19:51AM +0200, Stefan Brüns wrote:
> Preparatory patch: If the same compatible is used for different SoCs which
> have a common register layout, but different number of channels, the
> channel count can no longer be stored in the config. Store it in the
> device structure instead.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

[Maxime Ripard]

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

Hi,

On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:
> +	ret = of_property_read_u32(np, "dma-channels", &sdc->num_pchans);
> +	if (ret && !sdc->num_pchans) {
> +		dev_err(&pdev->dev, "Can't get dma-channels.\n");
> +		return ret;
> +	}
> +
> +	if (sdc->num_pchans > DMA_MAX_CHANNELS) {
> +		dev_err(&pdev->dev, "Number of dma-channels out of range.\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_property_read_u32(np, "dma-requests", &sdc->max_request);
> +	if (ret && !sdc->max_request) {
> +		dev_info(&pdev->dev, "Missing dma-requests, using %u.\n",
> +			 DMA_CHAN_MAX_DRQ);
> +		sdc->max_request = DMA_CHAN_MAX_DRQ;
> +	}
> +
> +	if (sdc->max_request > DMA_CHAN_MAX_DRQ) {
> +		dev_err(&pdev->dev, "Value of dma-requests out of range.\n");
> +		return -EINVAL;
> +	}

I'm not really convinced about these two checks. They don't catch all
errors (the range between the actual number of channels / DRQ and the
maximum allowed per the registers), they might increase in the future
too, and if we want to make that check actually working, we would have
to duplicate the number of requests and channels into the driver.

Which is kind of the opposite of what we're trying to do here :)

Once removed,
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime



-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 08/10] dmaengine: sun6i: Add support for Allwinner A64 and compatibles

[Maxime Ripard]

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

On Sun, Sep 17, 2017 at 05:19:54AM +0200, Stefan Brüns wrote:
> The A64 SoC has the same dma engine as the H3 (sun8i), with a
> reduced amount of physical channels. To allow future reuse of the
> compatible, leave the channel count etc. in the config data blank
> and retrieve it from the devicetree.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
>  drivers/dma/sun6i-dma.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index b5ecc97a0d5a..118b29bb1eac 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -1127,6 +1127,28 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
>  			     BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
>  };
>  
> +/*
> + * The A64 binding uses the number of dma channels from the
> + * device tree node.
> + */
> +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
> +	.nr_max_channels = 0,
> +	.nr_max_requests = 0,
> +	.nr_max_vchans   = 0,

Those are the default values. I appreciate that you wanted them here
for documentation, but the comment above already fills up that role
fine.

Once removed,

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 06/10] arm64: allwinner: a64: Add devicetree binding for DMA controller

[Brüns, Stefan]

On Montag, 18. September 2017 10:11:34 CEST Maxime Ripard wrote:
> On Sun, Sep 17, 2017 at 05:19:52AM +0200, Stefan Brüns wrote:
> > The A64 is register compatible with the H3, but has a different number
> > of dma channels and request ports.
> > 
> > Attach additional properties to the node to allow future reuse of the
> > compatible for controllers with different number of channels/requests.
> > 
> > If dma-requests is not specified, the register layout defined maximum
> > of 32 is used.
> > 
> > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > ---
> > 
> >  .../devicetree/bindings/dma/sun6i-dma.txt          | 26
> >  ++++++++++++++++++++++ 1 file changed, 26 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > b/Documentation/devicetree/bindings/dma/sun6i-dma.txt index
> > 98fbe1a5c6dd..6ebc79f95202 100644
> > --- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > 
> > @@ -27,6 +27,32 @@ Example:
> >  		#dma-cells = <1>;
> >  	
> >  	};
> > 
> > +-------------------------------------------------------------------------
> > ----- +For A64 DMA controller:
> > +
> > +Required properties:
> > +- compatible:	"allwinner,sun50i-a64-dma"
> > +- dma-channels: Number of DMA channels supported by the controller.
> > +		Refer to Documentation/devicetree/bindings/dma/dma.txt
> > +- all properties above, i.e. reg, interrupts, clocks, resets and
> > #dma-cells +
> > +Optional properties:
> > +- dma-requests: Number of DMA request signals supported by the
> > controller.
> > +		Refer to Documentation/devicetree/bindings/dma/dma.txt
> 
> You're error'ing out if dma-requests, so it isn't optional. I guess we
> should just make it mandatory.

No, it defaults to DMA_CHAN_MAX_DRQ = 31, see patch 07/10.

Kind regards,

Stefan

Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

[Brüns, Stefan]

On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:
> On Mon, Sep 18, 2017 at 02:09:43PM +0000, Brüns, Stefan wrote:
> > On Montag, 18. September 2017 10:18:24 CEST you wrote:
> > > Hi,
> > > 
> > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:
> > > > +	ret = of_property_read_u32(np, "dma-channels", &sdc->num_pchans);
> > > > +	if (ret && !sdc->num_pchans) {
> > > > +		dev_err(&pdev->dev, "Can't get dma-channels.\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	if (sdc->num_pchans > DMA_MAX_CHANNELS) {
> > > > +		dev_err(&pdev->dev, "Number of dma-channels out of range.\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	ret = of_property_read_u32(np, "dma-requests", &sdc->max_request);
> > > > +	if (ret && !sdc->max_request) {
> > > > +		dev_info(&pdev->dev, "Missing dma-requests, using %u.\n",
> > > > +			 DMA_CHAN_MAX_DRQ);
> > > > +		sdc->max_request = DMA_CHAN_MAX_DRQ;
> > > > +	}
> > > > +
> > > > +	if (sdc->max_request > DMA_CHAN_MAX_DRQ) {
> > > > +		dev_err(&pdev->dev, "Value of dma-requests out of range.\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > 
> > > I'm not really convinced about these two checks. They don't catch all
> > > errors (the range between the actual number of channels / DRQ and the
> > > maximum allowed per the registers), they might increase in the future
> > > too, and if we want to make that check actually working, we would have
> > > to duplicate the number of requests and channels into the driver.
> > 
> > 1. If these values increase, we have a new register layout and and
> > need a new compatible anyway.
> 
> And you want to store a new maximum attached to the compatible? Isn't
> that exactly the situation you're trying to get away from?

Yes, and no. H3, H5, A64 and R40 have the exact same register layout, but 
different number of channels and ports. They could share a compatible (if DMA 
channels were generalized), and we already have several register offsets/
widths (implicitly via the callbacks) attached to the compatible (so these 
don't need generalization via DT).

Now, we could also move everything that is currently attached to the 
compatible, i.e. clock gate register offset, burst widths/lengths etc. into 
the devicetree binding, but that would just be too much.

The idea is to find a middle ground here, using common patterns in the 
existing SoCs. The register layout has hardly changed, while the number of DMA 
channels and ports changes all the time. Moving the number of DMA channels and 
ports to the DT is trivial, and a pattern also found in other DMA controller 
drivers. *If* the number of dma channels and ports is ever increased, 
exceeding the current maximum, this would amount to major changes in the 
driver and maybe even warrant a completely new driver.

> > 2. As long as the the limits are adhered to, no other registers/register
> > fields are overwritten. As the channel number and port are used to
> > calculate memory offsets bounds checking is IMHO a good idea.
> 
> And this is true for many other resources, starting with the one
> defined in reg. We don't error check every register range, clock
> index, reset line, interrupt, DMA channel, the memory size, etc. yet
> you could make the same argument.
> 
> The DT has to be right, and we have to trust it. Otherwise we can just
> throw it away.

So your argument here basically is - don't do any checks on DT provided 
values, these are always correct. So, following this argument, not only the 
range check, but also the of_property_read return values should be ignored, as 
the DT is correct, thus of_property_read will never return an error.

That clearly does not match the implementation of drivers throughout the 
various subsystems for DT properties, which is in general - do all the checks 
that can be done, trust everything you can not verify.

Kind regards,

Stefan

Re: [PATCH v2 06/10] arm64: allwinner: a64: Add devicetree binding for DMA controller

[Rob Herring]

On Sun, Sep 17, 2017 at 05:19:52AM +0200, Stefan Brüns wrote:
> The A64 is register compatible with the H3, but has a different number
> of dma channels and request ports.
> 
> Attach additional properties to the node to allow future reuse of the
> compatible for controllers with different number of channels/requests.
> 
> If dma-requests is not specified, the register layout defined maximum
> of 32 is used.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
>  .../devicetree/bindings/dma/sun6i-dma.txt          | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> index 98fbe1a5c6dd..6ebc79f95202 100644
> --- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> @@ -27,6 +27,32 @@ Example:
>  		#dma-cells = <1>;
>  	};
>  
> +------------------------------------------------------------------------------
> +For A64 DMA controller:
> +
> +Required properties:
> +- compatible:	"allwinner,sun50i-a64-dma"
> +- dma-channels: Number of DMA channels supported by the controller.
> +		Refer to Documentation/devicetree/bindings/dma/dma.txt
> +- all properties above, i.e. reg, interrupts, clocks, resets and #dma-cells
> +
> +Optional properties:
> +- dma-requests: Number of DMA request signals supported by the controller.
> +		Refer to Documentation/devicetree/bindings/dma/dma.txt
> +
> +Example:
> +	dma: dma-controller@01c02000 {

Drop the leading 0. Building dtbs with W=2 will tell you this.

With that,

Acked-by: Rob Herring <robh@kernel.org>

> +		compatible = "allwinner,sun50i-a64-dma";
> +		reg = <0x01c02000 0x1000>;
> +		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&ccu CLK_BUS_DMA>;
> +		dma-channels = <8>;
> +		dma-requests = <27>;
> +		resets = <&ccu RST_BUS_DMA>;
> +		#dma-cells = <1>;
> +	};
> +------------------------------------------------------------------------------
> +
>  Clients:
>  
>  DMA clients connected to the A31 DMA controller must use the format
> -- 
> 2.14.1
> 

Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

[Maxime Ripard]

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

On Tue, Sep 19, 2017 at 04:17:59PM +0000, Brüns, Stefan wrote:
> On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:
> > On Mon, Sep 18, 2017 at 02:09:43PM +0000, Brüns, Stefan wrote:
> > > On Montag, 18. September 2017 10:18:24 CEST you wrote:
> > > > Hi,
> > > > 
> > > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:
> > > > > +	ret = of_property_read_u32(np, "dma-channels", &sdc->num_pchans);
> > > > > +	if (ret && !sdc->num_pchans) {
> > > > > +		dev_err(&pdev->dev, "Can't get dma-channels.\n");
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	if (sdc->num_pchans > DMA_MAX_CHANNELS) {
> > > > > +		dev_err(&pdev->dev, "Number of dma-channels out of range.\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	ret = of_property_read_u32(np, "dma-requests", &sdc->max_request);
> > > > > +	if (ret && !sdc->max_request) {
> > > > > +		dev_info(&pdev->dev, "Missing dma-requests, using %u.\n",
> > > > > +			 DMA_CHAN_MAX_DRQ);
> > > > > +		sdc->max_request = DMA_CHAN_MAX_DRQ;
> > > > > +	}
> > > > > +
> > > > > +	if (sdc->max_request > DMA_CHAN_MAX_DRQ) {
> > > > > +		dev_err(&pdev->dev, "Value of dma-requests out of range.\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > 
> > > > I'm not really convinced about these two checks. They don't catch all
> > > > errors (the range between the actual number of channels / DRQ and the
> > > > maximum allowed per the registers), they might increase in the future
> > > > too, and if we want to make that check actually working, we would have
> > > > to duplicate the number of requests and channels into the driver.
> > > 
> > > 1. If these values increase, we have a new register layout and and
> > > need a new compatible anyway.
> > 
> > And you want to store a new maximum attached to the compatible? Isn't
> > that exactly the situation you're trying to get away from?
> 
> Yes, and no. H3, H5, A64 and R40 have the exact same register layout, but 
> different number of channels and ports. They could share a compatible (if DMA 
> channels were generalized), and we already have several register offsets/
> widths (implicitly via the callbacks) attached to the compatible (so these 
> don't need generalization via DT).
> 
> Now, we could also move everything that is currently attached to the 
> compatible, i.e. clock gate register offset, burst widths/lengths etc. into 
> the devicetree binding, but that would just be too much.
> 
> The idea is to find a middle ground here, using common patterns in the 
> existing SoCs. The register layout has hardly changed, while the number of DMA 
> channels and ports changes all the time. Moving the number of DMA channels and 
> ports to the DT is trivial, and a pattern also found in other DMA controller 
> drivers.

I'm sorry, but the code is inconsistent here. You basically have two
variables from one SoC to the other, the number of channels and
requests.

In one case (channels), it mandates that the property is provided in
the device tree, and doesn't default to anything.

In the other case (requests), the property is optional and it will
provide a default. All that in 20 lines.

I guess we already reached that middle ground by providing them
through the DT, we just have to make sure we remain consistent.

> *If* the number of dma channels and ports is ever increased,
> exceeding the current maximum, this would amount to major changes in
> the driver and maybe even warrant a completely new driver.
> 
> > > 2. As long as the the limits are adhered to, no other registers/register
> > > fields are overwritten. As the channel number and port are used to
> > > calculate memory offsets bounds checking is IMHO a good idea.
> > 
> > And this is true for many other resources, starting with the one
> > defined in reg. We don't error check every register range, clock
> > index, reset line, interrupt, DMA channel, the memory size, etc. yet
> > you could make the same argument.
> > 
> > The DT has to be right, and we have to trust it. Otherwise we can just
> > throw it away.
> 
> So your argument here basically is - don't do any checks on DT provided 
> values, these are always correct. So, following this argument, not only the 
> range check, but also the of_property_read return values should be ignored, as 
> the DT is correct, thus of_property_read will never return an error.

No, my argument is don't do a check if you can catch only half of the
errors, and with no hope of fixing it.

The functions you mentionned have a 100% error catch rate. This is the
difference.

> That clearly does not match the implementation of drivers throughout the 
> various subsystems for DT properties, which is in general - do all the checks 
> that can be done, trust everything you can not verify.

And my point is that we're falling into the latter here. You cannot
verify it properly.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

[Brüns, Stefan]

On Freitag, 22. September 2017 23:30:27 CEST Maxime Ripard wrote:
> On Tue, Sep 19, 2017 at 04:17:59PM +0000, Brüns, Stefan wrote:
> > On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:
> > > On Mon, Sep 18, 2017 at 02:09:43PM +0000, Brüns, Stefan wrote:
> > > > On Montag, 18. September 2017 10:18:24 CEST you wrote:
> > > > > Hi,
> > > > > 
> > > > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:
> > > > > > +	ret = of_property_read_u32(np, "dma-channels",
> > > > > > &sdc->num_pchans);
> > > > > > +	if (ret && !sdc->num_pchans) {
> > > > > > +		dev_err(&pdev->dev, "Can't get dma-channels.\n");
> > > > > > +		return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (sdc->num_pchans > DMA_MAX_CHANNELS) {
> > > > > > +		dev_err(&pdev->dev, "Number of dma-channels out of range.
\n");
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > > > +
> > > > > > +	ret = of_property_read_u32(np, "dma-requests",
> > > > > > &sdc->max_request);
> > > > > > +	if (ret && !sdc->max_request) {
> > > > > > +		dev_info(&pdev->dev, "Missing dma-requests, using %u.\n",
> > > > > > +			 DMA_CHAN_MAX_DRQ);
> > > > > > +		sdc->max_request = DMA_CHAN_MAX_DRQ;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (sdc->max_request > DMA_CHAN_MAX_DRQ) {
> > > > > > +		dev_err(&pdev->dev, "Value of dma-requests out of range.\n");
> > > > > > +		return -EINVAL;
> > > > > > +	}
> > > > > 
> > > > > I'm not really convinced about these two checks. They don't catch
> > > > > all
> > > > > errors (the range between the actual number of channels / DRQ and
> > > > > the
> > > > > maximum allowed per the registers), they might increase in the
> > > > > future
> > > > > too, and if we want to make that check actually working, we would
> > > > > have
> > > > > to duplicate the number of requests and channels into the driver.
> > > > 
> > > > 1. If these values increase, we have a new register layout and and
> > > > need a new compatible anyway.
> > > 
> > > And you want to store a new maximum attached to the compatible? Isn't
> > > that exactly the situation you're trying to get away from?
> > 
> > Yes, and no. H3, H5, A64 and R40 have the exact same register layout, but
> > different number of channels and ports. They could share a compatible (if
> > DMA channels were generalized), and we already have several register
> > offsets/ widths (implicitly via the callbacks) attached to the compatible
> > (so these don't need generalization via DT).
> > 
> > Now, we could also move everything that is currently attached to the
> > compatible, i.e. clock gate register offset, burst widths/lengths etc.
> > into
> > the devicetree binding, but that would just be too much.
> > 
> > The idea is to find a middle ground here, using common patterns in the
> > existing SoCs. The register layout has hardly changed, while the number of
> > DMA channels and ports changes all the time. Moving the number of DMA
> > channels and ports to the DT is trivial, and a pattern also found in
> > other DMA controller drivers.
> 
> I'm sorry, but the code is inconsistent here. You basically have two
> variables from one SoC to the other, the number of channels and
> requests.
> 
> In one case (channels), it mandates that the property is provided in
> the device tree, and doesn't default to anything.
> 
> In the other case (requests), the property is optional and it will
> provide a default. All that in 20 lines.

The channel number is a hardware property. Using more channels than the 
hardware provides is a bug. There is no default.

The port/request is just some lax property to limit the resource allocation 
upfront. As long as the bindings of the different IP blocks (SPI, audio, ...) 
provide the correct port numbers, all required information is available.
 
> I guess we already reached that middle ground by providing them
> through the DT, we just have to make sure we remain consistent.
> 
> > *If* the number of dma channels and ports is ever increased,
> > exceeding the current maximum, this would amount to major changes in
> > the driver and maybe even warrant a completely new driver.
> > 
> > > > 2. As long as the the limits are adhered to, no other
> > > > registers/register
> > > > fields are overwritten. As the channel number and port are used to
> > > > calculate memory offsets bounds checking is IMHO a good idea.
> > > 
> > > And this is true for many other resources, starting with the one
> > > defined in reg. We don't error check every register range, clock
> > > index, reset line, interrupt, DMA channel, the memory size, etc. yet
> > > you could make the same argument.
> > > 
> > > The DT has to be right, and we have to trust it. Otherwise we can just
> > > throw it away.
> > 
> > So your argument here basically is - don't do any checks on DT provided
> > values, these are always correct. So, following this argument, not only
> > the
> > range check, but also the of_property_read return values should be
> > ignored, as the DT is correct, thus of_property_read will never return an
> > error.
> No, my argument is don't do a check if you can catch only half of the
> errors, and with no hope of fixing it.
> 
> The functions you mentionned have a 100% error catch rate. This is the
> difference.
> 
> > That clearly does not match the implementation of drivers throughout the
> > various subsystems for DT properties, which is in general - do all the
> > checks that can be done, trust everything you can not verify.
> 
> And my point is that we're falling into the latter here. You cannot
> verify it properly.

Please check the following line:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/
drivers/dma/sun6i-dma.c#n951

Thats far from 100% - the highest allowed port for each SoC differs between RX 
and TX, and port allocation is sparse.

Regards,

Stefan

Re: [PATCH v2 06/10] arm64: allwinner: a64: Add devicetree binding for DMA controller

[Stefan Bruens]

On Mittwoch, 20. September 2017 22:53:00 CEST Rob Herring wrote:
> On Sun, Sep 17, 2017 at 05:19:52AM +0200, Stefan Brüns wrote:
> > The A64 is register compatible with the H3, but has a different number
> > of dma channels and request ports.
> > 
> > Attach additional properties to the node to allow future reuse of the
> > compatible for controllers with different number of channels/requests.
> > 
> > If dma-requests is not specified, the register layout defined maximum
> > of 32 is used.
> > 
> > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> > ---
> > 
> >  .../devicetree/bindings/dma/sun6i-dma.txt          | 26
> >  ++++++++++++++++++++++ 1 file changed, 26 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > b/Documentation/devicetree/bindings/dma/sun6i-dma.txt index
> > 98fbe1a5c6dd..6ebc79f95202 100644
> > --- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
> > 
> > @@ -27,6 +27,32 @@ Example:
> >  		#dma-cells = <1>;
> >  	
> >  	};
> > 
> > +-------------------------------------------------------------------------
> > ----- +For A64 DMA controller:
> > +
> > +Required properties:
> > +- compatible:	"allwinner,sun50i-a64-dma"
> > +- dma-channels: Number of DMA channels supported by the controller.
> > +		Refer to Documentation/devicetree/bindings/dma/dma.txt
> > +- all properties above, i.e. reg, interrupts, clocks, resets and
> > #dma-cells +
> > +Optional properties:
> > +- dma-requests: Number of DMA request signals supported by the
> > controller.
> > +		Refer to Documentation/devicetree/bindings/dma/dma.txt
> > +
> > +Example:
> > +	dma: dma-controller@01c02000 {
> 
> Drop the leading 0. Building dtbs with W=2 will tell you this.
> 
> With that,
> 
> Acked-by: Rob Herring <robh@kernel.org>

The leading 0 was copied from the A31 example just a few lines above. Should I 
also correct that one, or should that go in a separate patch?

Kind regards,

Stefan
-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

Re: [PATCH v2 06/10] arm64: allwinner: a64: Add devicetree binding for DMA controller

[Rob Herring]

On Sat, Sep 23, 2017 at 6:34 PM, Stefan Bruens
<stefan.bruens@rwth-aachen.de> wrote:
> On Mittwoch, 20. September 2017 22:53:00 CEST Rob Herring wrote:
>> On Sun, Sep 17, 2017 at 05:19:52AM +0200, Stefan Brüns wrote:
>> > The A64 is register compatible with the H3, but has a different number
>> > of dma channels and request ports.
>> >
>> > Attach additional properties to the node to allow future reuse of the
>> > compatible for controllers with different number of channels/requests.
>> >
>> > If dma-requests is not specified, the register layout defined maximum
>> > of 32 is used.
>> >
>> > Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
>> > ---
>> >
>> >  .../devicetree/bindings/dma/sun6i-dma.txt          | 26
>> >  ++++++++++++++++++++++ 1 file changed, 26 insertions(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
>> > b/Documentation/devicetree/bindings/dma/sun6i-dma.txt index
>> > 98fbe1a5c6dd..6ebc79f95202 100644
>> > --- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
>> > +++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
>> >
>> > @@ -27,6 +27,32 @@ Example:
>> >             #dma-cells = <1>;
>> >
>> >     };
>> >
>> > +-------------------------------------------------------------------------
>> > ----- +For A64 DMA controller:
>> > +
>> > +Required properties:
>> > +- compatible:      "allwinner,sun50i-a64-dma"
>> > +- dma-channels: Number of DMA channels supported by the controller.
>> > +           Refer to Documentation/devicetree/bindings/dma/dma.txt
>> > +- all properties above, i.e. reg, interrupts, clocks, resets and
>> > #dma-cells +
>> > +Optional properties:
>> > +- dma-requests: Number of DMA request signals supported by the
>> > controller.
>> > +           Refer to Documentation/devicetree/bindings/dma/dma.txt
>> > +
>> > +Example:
>> > +   dma: dma-controller@01c02000 {
>>
>> Drop the leading 0. Building dtbs with W=2 will tell you this.
>>
>> With that,
>>
>> Acked-by: Rob Herring <robh@kernel.org>
>
> The leading 0 was copied from the A31 example just a few lines above. Should I
> also correct that one, or should that go in a separate patch?

A separate patch.

Rob

Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

[Maxime Ripard]

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

On Sat, Sep 23, 2017 at 12:00:15AM +0000, Brüns, Stefan wrote:
> On Freitag, 22. September 2017 23:30:27 CEST Maxime Ripard wrote:
> > On Tue, Sep 19, 2017 at 04:17:59PM +0000, Brüns, Stefan wrote:
> > > On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:
> > > > On Mon, Sep 18, 2017 at 02:09:43PM +0000, Brüns, Stefan wrote:
> > > > > On Montag, 18. September 2017 10:18:24 CEST you wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:
> > > > > > > +	ret = of_property_read_u32(np, "dma-channels",
> > > > > > > &sdc->num_pchans);
> > > > > > > +	if (ret && !sdc->num_pchans) {
> > > > > > > +		dev_err(&pdev->dev, "Can't get dma-channels.\n");
> > > > > > > +		return ret;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	if (sdc->num_pchans > DMA_MAX_CHANNELS) {
> > > > > > > +		dev_err(&pdev->dev, "Number of dma-channels out of range.
> \n");
> > > > > > > +		return -EINVAL;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	ret = of_property_read_u32(np, "dma-requests",
> > > > > > > &sdc->max_request);
> > > > > > > +	if (ret && !sdc->max_request) {
> > > > > > > +		dev_info(&pdev->dev, "Missing dma-requests, using %u.\n",
> > > > > > > +			 DMA_CHAN_MAX_DRQ);
> > > > > > > +		sdc->max_request = DMA_CHAN_MAX_DRQ;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	if (sdc->max_request > DMA_CHAN_MAX_DRQ) {
> > > > > > > +		dev_err(&pdev->dev, "Value of dma-requests out of range.\n");
> > > > > > > +		return -EINVAL;
> > > > > > > +	}
> > > > > > 
> > > > > > I'm not really convinced about these two checks. They don't catch
> > > > > > all
> > > > > > errors (the range between the actual number of channels / DRQ and
> > > > > > the
> > > > > > maximum allowed per the registers), they might increase in the
> > > > > > future
> > > > > > too, and if we want to make that check actually working, we would
> > > > > > have
> > > > > > to duplicate the number of requests and channels into the driver.
> > > > > 
> > > > > 1. If these values increase, we have a new register layout and and
> > > > > need a new compatible anyway.
> > > > 
> > > > And you want to store a new maximum attached to the compatible? Isn't
> > > > that exactly the situation you're trying to get away from?
> > > 
> > > Yes, and no. H3, H5, A64 and R40 have the exact same register layout, but
> > > different number of channels and ports. They could share a compatible (if
> > > DMA channels were generalized), and we already have several register
> > > offsets/ widths (implicitly via the callbacks) attached to the compatible
> > > (so these don't need generalization via DT).
> > > 
> > > Now, we could also move everything that is currently attached to the
> > > compatible, i.e. clock gate register offset, burst widths/lengths etc.
> > > into
> > > the devicetree binding, but that would just be too much.
> > > 
> > > The idea is to find a middle ground here, using common patterns in the
> > > existing SoCs. The register layout has hardly changed, while the number of
> > > DMA channels and ports changes all the time. Moving the number of DMA
> > > channels and ports to the DT is trivial, and a pattern also found in
> > > other DMA controller drivers.
> > 
> > I'm sorry, but the code is inconsistent here. You basically have two
> > variables from one SoC to the other, the number of channels and
> > requests.
> > 
> > In one case (channels), it mandates that the property is provided in
> > the device tree, and doesn't default to anything.
> > 
> > In the other case (requests), the property is optional and it will
> > provide a default. All that in 20 lines.
> 
> The channel number is a hardware property. Using more channels than the 
> hardware provides is a bug. There is no default.
> 
> The port/request is just some lax property to limit the resource allocation 
> upfront. As long as the bindings of the different IP blocks (SPI, audio, ...) 
> provide the correct port numbers, all required information is available.

Using an improper request ID or out of bounds will be just as much as
a bug. You will not get your DMA transfer to the proper device you
were trying to, the data will not reach the device or memory, your
driver will not work => a bug.

It will not be for the same reasons, you will not overwrite other
registers, but the end result is just the same: your transfer will not
work.

> > I guess we already reached that middle ground by providing them
> > through the DT, we just have to make sure we remain consistent.
> > 
> > > *If* the number of dma channels and ports is ever increased,
> > > exceeding the current maximum, this would amount to major changes in
> > > the driver and maybe even warrant a completely new driver.
> > > 
> > > > > 2. As long as the the limits are adhered to, no other
> > > > > registers/register
> > > > > fields are overwritten. As the channel number and port are used to
> > > > > calculate memory offsets bounds checking is IMHO a good idea.
> > > > 
> > > > And this is true for many other resources, starting with the one
> > > > defined in reg. We don't error check every register range, clock
> > > > index, reset line, interrupt, DMA channel, the memory size, etc. yet
> > > > you could make the same argument.
> > > > 
> > > > The DT has to be right, and we have to trust it. Otherwise we can just
> > > > throw it away.
> > > 
> > > So your argument here basically is - don't do any checks on DT provided
> > > values, these are always correct. So, following this argument, not only
> > > the
> > > range check, but also the of_property_read return values should be
> > > ignored, as the DT is correct, thus of_property_read will never return an
> > > error.
> > No, my argument is don't do a check if you can catch only half of the
> > errors, and with no hope of fixing it.
> > 
> > The functions you mentionned have a 100% error catch rate. This is the
> > difference.
> > 
> > > That clearly does not match the implementation of drivers throughout the
> > > various subsystems for DT properties, which is in general - do all the
> > > checks that can be done, trust everything you can not verify.
> > 
> > And my point is that we're falling into the latter here. You cannot
> > verify it properly.
> 
> Please check the following line:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/dma/sun6i-dma.c#n951
> 
> Thats far from 100% - the highest allowed port for each SoC differs between RX 
> and TX, and port allocation is sparse.

But until your patches, you *could* fix it and reach that 100%.

And I guess now we could indeed remove it.

Look, this discussion is going nowhere. I told you what the condition
for my Acked-by was already.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v2 07/10] dmaengine: sun6i: Retrieve channel count/max request from devicetree

[Stefan Bruens]

On Mittwoch, 27. September 2017 11:09:22 CEST Maxime Ripard wrote:
> On Sat, Sep 23, 2017 at 12:00:15AM +0000, Brüns, Stefan wrote:
> > On Freitag, 22. September 2017 23:30:27 CEST Maxime Ripard wrote:
> > > On Tue, Sep 19, 2017 at 04:17:59PM +0000, Brüns, Stefan wrote:
> > > > On Dienstag, 19. September 2017 16:25:08 CEST Maxime Ripard wrote:
> > > > > On Mon, Sep 18, 2017 at 02:09:43PM +0000, Brüns, Stefan wrote:
> > > > > > On Montag, 18. September 2017 10:18:24 CEST you wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Sun, Sep 17, 2017 at 05:19:53AM +0200, Stefan Brüns wrote:
> > > > > > > > +	ret = of_property_read_u32(np, "dma-channels",
> > > > > > > > &sdc->num_pchans);
> > > > > > > > +	if (ret && !sdc->num_pchans) {
> > > > > > > > +		dev_err(&pdev->dev, "Can't get dma-channels.\n");
> > > > > > > > +		return ret;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	if (sdc->num_pchans > DMA_MAX_CHANNELS) {
> > > > > > > > +		dev_err(&pdev->dev, "Number of dma-channels out of range.
> > 
> > \n");
> > 
> > > > > > > > +		return -EINVAL;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	ret = of_property_read_u32(np, "dma-requests",
> > > > > > > > &sdc->max_request);
> > > > > > > > +	if (ret && !sdc->max_request) {
> > > > > > > > +		dev_info(&pdev->dev, "Missing dma-requests, using %u.\n",
> > > > > > > > +			 DMA_CHAN_MAX_DRQ);
> > > > > > > > +		sdc->max_request = DMA_CHAN_MAX_DRQ;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	if (sdc->max_request > DMA_CHAN_MAX_DRQ) {
> > > > > > > > +		dev_err(&pdev->dev, "Value of dma-requests out of
> > > > > > > > range.\n");
> > > > > > > > +		return -EINVAL;
> > > > > > > > +	}
> > > > > > > 
> > > > > > > I'm not really convinced about these two checks. They don't
> > > > > > > catch
> > > > > > > all
> > > > > > > errors (the range between the actual number of channels / DRQ
> > > > > > > and
> > > > > > > the
> > > > > > > maximum allowed per the registers), they might increase in the
> > > > > > > future
> > > > > > > too, and if we want to make that check actually working, we
> > > > > > > would
> > > > > > > have
> > > > > > > to duplicate the number of requests and channels into the
> > > > > > > driver.
> > > > > > 
> > > > > > 1. If these values increase, we have a new register layout and and
> > > > > > need a new compatible anyway.
> > > > > 
> > > > > And you want to store a new maximum attached to the compatible?
> > > > > Isn't
> > > > > that exactly the situation you're trying to get away from?
> > > > 
> > > > Yes, and no. H3, H5, A64 and R40 have the exact same register layout,
> > > > but
> > > > different number of channels and ports. They could share a compatible
> > > > (if
> > > > DMA channels were generalized), and we already have several register
> > > > offsets/ widths (implicitly via the callbacks) attached to the
> > > > compatible
> > > > (so these don't need generalization via DT).
> > > > 
> > > > Now, we could also move everything that is currently attached to the
> > > > compatible, i.e. clock gate register offset, burst widths/lengths etc.
> > > > into
> > > > the devicetree binding, but that would just be too much.
> > > > 
> > > > The idea is to find a middle ground here, using common patterns in the
> > > > existing SoCs. The register layout has hardly changed, while the
> > > > number of
> > > > DMA channels and ports changes all the time. Moving the number of DMA
> > > > channels and ports to the DT is trivial, and a pattern also found in
> > > > other DMA controller drivers.
> > > 
> > > I'm sorry, but the code is inconsistent here. You basically have two
> > > variables from one SoC to the other, the number of channels and
> > > requests.
> > > 
> > > In one case (channels), it mandates that the property is provided in
> > > the device tree, and doesn't default to anything.
> > > 
> > > In the other case (requests), the property is optional and it will
> > > provide a default. All that in 20 lines.
> > 
> > The channel number is a hardware property. Using more channels than the
> > hardware provides is a bug. There is no default.
> > 
> > The port/request is just some lax property to limit the resource
> > allocation
> > upfront. As long as the bindings of the different IP blocks (SPI, audio,
> > ...) provide the correct port numbers, all required information is
> > available.
> Using an improper request ID or out of bounds will be just as much as
> a bug. You will not get your DMA transfer to the proper device you
> were trying to, the data will not reach the device or memory, your
> driver will not work => a bug.
> 
> It will not be for the same reasons, you will not overwrite other
> registers, but the end result is just the same: your transfer will not
> work.

Writing adjacent registers breaks other users of the DMA controller. 
"Everytime I play a sound, my MMC breaks" - oh, what fun.

> > > I guess we already reached that middle ground by providing them
> > > through the DT, we just have to make sure we remain consistent.
> > > 
> > > > *If* the number of dma channels and ports is ever increased,
> > > > exceeding the current maximum, this would amount to major changes in
> > > > the driver and maybe even warrant a completely new driver.
> > > > 
> > > > > > 2. As long as the the limits are adhered to, no other
> > > > > > registers/register
> > > > > > fields are overwritten. As the channel number and port are used to
> > > > > > calculate memory offsets bounds checking is IMHO a good idea.
> > > > > 
> > > > > And this is true for many other resources, starting with the one
> > > > > defined in reg. We don't error check every register range, clock
> > > > > index, reset line, interrupt, DMA channel, the memory size, etc. yet
> > > > > you could make the same argument.
> > > > > 
> > > > > The DT has to be right, and we have to trust it. Otherwise we can
> > > > > just
> > > > > throw it away.
> > > > 
> > > > So your argument here basically is - don't do any checks on DT
> > > > provided
> > > > values, these are always correct. So, following this argument, not
> > > > only
> > > > the
> > > > range check, but also the of_property_read return values should be
> > > > ignored, as the DT is correct, thus of_property_read will never return
> > > > an
> > > > error.
> > > 
> > > No, my argument is don't do a check if you can catch only half of the
> > > errors, and with no hope of fixing it.
> > > 
> > > The functions you mentionned have a 100% error catch rate. This is the
> > > difference.
> > > 
> > > > That clearly does not match the implementation of drivers throughout
> > > > the
> > > > various subsystems for DT properties, which is in general - do all the
> > > > checks that can be done, trust everything you can not verify.
> > > 
> > > And my point is that we're falling into the latter here. You cannot
> > > verify it properly.
> > 
> > Please check the following line:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/dr
> > ivers/dma/sun6i-dma.c#n951
> > 
> > Thats far from 100% - the highest allowed port for each SoC differs
> > between RX and TX, and port allocation is sparse.
> 
> But until your patches, you *could* fix it and reach that 100%.

1. You had 3 years to do that, but you never cared.
2. Its still possible to do, just add a property to the devicetree.

> And I guess now we could indeed remove it.
> 
> Look, this discussion is going nowhere. I told you what the condition
> for my Acked-by was already.

Yeah, and its your power as a so called maintainer to force your opinion on 
anyone crossing your way. Fine, go for it ...

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019