[v1,1/2] dt-bindings: dmaengine: dw-dmac: add protection control property

[v1,1/2] dt-bindings: dmaengine: dw-dmac: add protection control property

[Christian Lamparter]

This patch adds the per-channel dma protection control
prop-encoded-array and dt-binding definitions (including
definitions for the existing channel allocation and priority
order values based on include/linux/platform_data/dma-dw.h)
for the DesignWare AHB Central Direct Memory Access Controller.

Note: The protection control signals are one-to-one mapped to
the AHB HPROT[1:3] signals.  The HPROT0 (Data Access) is
always hardwired to 1 for this controller.

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 .../devicetree/bindings/dma/snps-dma.txt      |  4 ++++
 MAINTAINERS                                   |  4 +++-
 include/dt-bindings/dma/dw-dmac.h             | 20 +++++++++++++++++++
 3 files changed, 27 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/dma/dw-dmac.h

diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
index 39e2b26be344..72b4984a4c18 100644
--- a/Documentation/devicetree/bindings/dma/snps-dma.txt
+++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
@@ -27,6 +27,10 @@ Optional properties:
   general purpose DMA channel allocator. False if not passed.
 - multi-block: Multi block transfers supported by hardware. Array property with
   one cell per channel. 0: not supported, 1 (default): supported.
+- snps,dma-protection-control: Channel's AHB HPROT[3:1] protection setting.
+  Array property with one cell per channel. The default value for a channel
+  is 0 (for non-cacheable, strongly ordered, unprivileged data access).
+  Refer to include/dt-bindings/dma/dw-dmac.h for possible values.
 
 Example:
 
diff --git a/MAINTAINERS b/MAINTAINERS
index dacba23b80b4..c35998e20e9d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14107,9 +14107,11 @@ 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/snps-dma.txt
+F:	drivers/dma/dw/
+F:	include/dt-bindings/dma/dw-dmac.h
 F:	include/linux/dma/dw.h
 F:	include/linux/platform_data/dma-dw.h
-F:	drivers/dma/dw/
 
 SYNOPSYS DESIGNWARE ENTERPRISE ETHERNET DRIVER
 M:	Jose Abreu <Jose.Abreu@synopsys.com>
diff --git a/include/dt-bindings/dma/dw-dmac.h b/include/dt-bindings/dma/dw-dmac.h
new file mode 100644
index 000000000000..9152a6e2406c
--- /dev/null
+++ b/include/dt-bindings/dma/dw-dmac.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+
+#ifndef __DT_BINDINGS_DMA_DW_DMAC_H__
+#define __DT_BINDINGS_DMA_DW_DMAC_H__
+
+#define DW_DMAC_CHAN_ALLOCATION_ASCENDING       0       /* zero to seven */
+#define DW_DMAC_CHAN_ALLOCATION_DESCENDING      1       /* seven to zero */
+#define DW_DMAC_CHAN_PRIORITY_ASCENDING         0       /* chan0 highest */
+#define DW_DMAC_CHAN_PRIORITY_DESCENDING        1       /* chan7 highest */
+
+/*
+ * Protection Control bits provide protection against illegal transactions.
+ * The protection bits[0:2] are one-to-one mapped to AHB HPROT[3:1] signals.
+ * The AHB HPROT[0] bit is hardwired to 1: Data Access.
+ */
+#define DW_DMAC_HPROT1_PRIVILEGED_MODE	(1 << 0)	/* Privileged Mode */
+#define DW_DMAC_HPROT2_BUFFERABLE	(1 << 1)	/* DMA is bufferable */
+#define DW_DMAC_HPROT3_CACHEABLE	(1 << 2)	/* DMA is cacheable */
+
+#endif /* __DT_BINDINGS_DMA_DW_DMAC_H__ */

[PATCH v1 1/2] dt-bindings: dmaengine: dw-dmac: add protection control property

[Christian Lamparter]

This patch adds the per-channel dma protection control
prop-encoded-array and dt-binding definitions (including
definitions for the existing channel allocation and priority
order values based on include/linux/platform_data/dma-dw.h)
for the DesignWare AHB Central Direct Memory Access Controller.

Note: The protection control signals are one-to-one mapped to
the AHB HPROT[1:3] signals.  The HPROT0 (Data Access) is
always hardwired to 1 for this controller.

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 .../devicetree/bindings/dma/snps-dma.txt      |  4 ++++
 MAINTAINERS                                   |  4 +++-
 include/dt-bindings/dma/dw-dmac.h             | 20 +++++++++++++++++++
 3 files changed, 27 insertions(+), 1 deletion(-)
 create mode 100644 include/dt-bindings/dma/dw-dmac.h

diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
index 39e2b26be344..72b4984a4c18 100644
--- a/Documentation/devicetree/bindings/dma/snps-dma.txt
+++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
@@ -27,6 +27,10 @@ Optional properties:
   general purpose DMA channel allocator. False if not passed.
 - multi-block: Multi block transfers supported by hardware. Array property with
   one cell per channel. 0: not supported, 1 (default): supported.
+- snps,dma-protection-control: Channel's AHB HPROT[3:1] protection setting.
+  Array property with one cell per channel. The default value for a channel
+  is 0 (for non-cacheable, strongly ordered, unprivileged data access).
+  Refer to include/dt-bindings/dma/dw-dmac.h for possible values.
 
 Example:
 
diff --git a/MAINTAINERS b/MAINTAINERS
index dacba23b80b4..c35998e20e9d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14107,9 +14107,11 @@ 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/snps-dma.txt
+F:	drivers/dma/dw/
+F:	include/dt-bindings/dma/dw-dmac.h
 F:	include/linux/dma/dw.h
 F:	include/linux/platform_data/dma-dw.h
-F:	drivers/dma/dw/
 
 SYNOPSYS DESIGNWARE ENTERPRISE ETHERNET DRIVER
 M:	Jose Abreu <Jose.Abreu@synopsys.com>
diff --git a/include/dt-bindings/dma/dw-dmac.h b/include/dt-bindings/dma/dw-dmac.h
new file mode 100644
index 000000000000..9152a6e2406c
--- /dev/null
+++ b/include/dt-bindings/dma/dw-dmac.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
+
+#ifndef __DT_BINDINGS_DMA_DW_DMAC_H__
+#define __DT_BINDINGS_DMA_DW_DMAC_H__
+
+#define DW_DMAC_CHAN_ALLOCATION_ASCENDING       0       /* zero to seven */
+#define DW_DMAC_CHAN_ALLOCATION_DESCENDING      1       /* seven to zero */
+#define DW_DMAC_CHAN_PRIORITY_ASCENDING         0       /* chan0 highest */
+#define DW_DMAC_CHAN_PRIORITY_DESCENDING        1       /* chan7 highest */
+
+/*
+ * Protection Control bits provide protection against illegal transactions.
+ * The protection bits[0:2] are one-to-one mapped to AHB HPROT[3:1] signals.
+ * The AHB HPROT[0] bit is hardwired to 1: Data Access.
+ */
+#define DW_DMAC_HPROT1_PRIVILEGED_MODE	(1 << 0)	/* Privileged Mode */
+#define DW_DMAC_HPROT2_BUFFERABLE	(1 << 1)	/* DMA is bufferable */
+#define DW_DMAC_HPROT3_CACHEABLE	(1 << 2)	/* DMA is cacheable */
+
+#endif /* __DT_BINDINGS_DMA_DW_DMAC_H__ */
-- 
2.19.1

[v1,2/2] dmaengine: dw: implement per-channel protection control setting

[Christian Lamparter]

This patch adds a new device-tree property that allows to
specify the protection control bits for each DMA channel
individually.

Setting the "correct" bits can have a huge impact on the
PPC460EX and APM82181 that use this DMA engine in combination
with a DesignWare' SATA-II core (sata_dwc_460ex driver).

In the OpenWrt Forum, the user takimata reported that:
|<https://forum.lede-project.org/t/wd-mybook-live-duo-two-disks/16195/55>
|It seems your patch unleashed the full power of the SATA port.
|Where I was previously hitting a really hard limit at around
|82 MB/s for reading and 27 MB/s for writing, I am now getting this:
|
|root@OpenWrt:/mnt# time dd if=/dev/zero of=tempfile bs=1M count=1024
|1024+0 records in
|1024+0 records out
|real    0m 13.65s
|user    0m 0.01s
|sys     0m 11.89s
|
|root@OpenWrt:/mnt# time dd if=tempfile of=/dev/null bs=1M count=1024
|1024+0 records in
|1024+0 records out
|real    0m 8.41s
|user    0m 0.01s
|sys     0m 4.70s
|
|This means: 121 MB/s reading and 75 MB/s writing!
|
|The drive is a WD Green WD10EARX taken from an older MBL Single.
|I repeated the test a few times with even larger files to rule out
|any caching, I'm still seeing the same great performance. OpenWrt is
|now completely on par with the original MBL firmware's performance.

Another user And.short reported in the same thread:
|<https://forum.openwrt.org/t/solved-wd-mybook-live-duo-two-disks/16195/50>
|I can report that your fix worked! Boots up fine with two
|drives even with more partitions, and no more reboot on
|concurrent disk access!

A closer look into the sata_dwc_460ex code revealed that
the driver did initally set the correct protection control
bits. However, this feature was lost when the sata_dwc_460ex
driver was converted to the generic DMA driver framework with:
8b3444852a2 ("sata_dwc_460ex: move to generic DMA driver").

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 drivers/dma/dw/core.c                |  3 +++
 drivers/dma/dw/platform.c            | 12 +++++++++---
 drivers/dma/dw/regs.h                |  4 ++++
 include/linux/platform_data/dma-dw.h |  6 ++++++
 4 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index f43e6dafe446..2db15e9b33a8 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -160,12 +160,15 @@ static void dwc_initialize_chan_idma32(struct dw_dma_chan *dwc)
 
 static void dwc_initialize_chan_dw(struct dw_dma_chan *dwc)
 {
+	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
+	size_t chanidx = (size_t)(dwc - dw->chan);
 	u32 cfghi = DWC_CFGH_FIFO_MODE;
 	u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority);
 	bool hs_polarity = dwc->dws.hs_polarity;
 
 	cfghi |= DWC_CFGH_DST_PER(dwc->dws.dst_id);
 	cfghi |= DWC_CFGH_SRC_PER(dwc->dws.src_id);
+	cfghi |= DWC_CFGH_PROTCTL(dw->pdata->protctl[chanidx]);
 
 	/* Set polarity of handshake interface */
 	cfglo |= hs_polarity ? DWC_CFGL_HS_DST_POL | DWC_CFGL_HS_SRC_POL : 0;
diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index f62dd0944908..078cca6576c3 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -102,7 +102,7 @@ dw_dma_parse_dt(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct dw_dma_platform_data *pdata;
-	u32 tmp, arr[DW_DMA_MAX_NR_MASTERS], mb[DW_DMA_MAX_NR_CHANNELS];
+	u32 tmp, arr[DW_DMA_MAX_NR_MASTERS], val[DW_DMA_MAX_NR_CHANNELS];
 	u32 nr_masters;
 	u32 nr_channels;
 
@@ -154,14 +154,20 @@ dw_dma_parse_dt(struct platform_device *pdev)
 			pdata->data_width[tmp] = BIT(arr[tmp] & 0x07);
 	}
 
-	if (!of_property_read_u32_array(np, "multi-block", mb, nr_channels)) {
+	if (!of_property_read_u32_array(np, "multi-block", val, nr_channels)) {
 		for (tmp = 0; tmp < nr_channels; tmp++)
-			pdata->multi_block[tmp] = mb[tmp];
+			pdata->multi_block[tmp] = val[tmp];
 	} else {
 		for (tmp = 0; tmp < nr_channels; tmp++)
 			pdata->multi_block[tmp] = 1;
 	}
 
+	if (!of_property_read_u32_array(np, "snps,dma-protection-control",
+					val, nr_channels)) {
+		for (tmp = 0; tmp < nr_channels; tmp++)
+			pdata->protctl[tmp] = val[tmp];
+	}
+
 	return pdata;
 }
 #else
diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h
index 09e7dfdbb790..646c9c960c07 100644
--- a/drivers/dma/dw/regs.h
+++ b/drivers/dma/dw/regs.h
@@ -200,6 +200,10 @@ enum dw_dma_msize {
 #define DWC_CFGH_FCMODE		(1 << 0)
 #define DWC_CFGH_FIFO_MODE	(1 << 1)
 #define DWC_CFGH_PROTCTL(x)	((x) << 2)
+#define DWC_CFGH_PROTCTL_DATA	(0 << 2)	/* data access - always set */
+#define DWC_CFGH_PROTCTL_PRIV	(1 << 2)	/* privileged -> AHB HPROT[1] */
+#define DWC_CFGH_PROTCTL_BUFFER	(2 << 2)	/* bufferable -> AHB HPROT[2] */
+#define DWC_CFGH_PROTCTL_CACHE	(4 << 2)	/* cacheable  -> AHB HPROT[3] */
 #define DWC_CFGH_DS_UPD_EN	(1 << 5)
 #define DWC_CFGH_SS_UPD_EN	(1 << 6)
 #define DWC_CFGH_SRC_PER(x)	((x) << 7)
diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
index 896cb71a382c..df65e3311a56 100644
--- a/include/linux/platform_data/dma-dw.h
+++ b/include/linux/platform_data/dma-dw.h
@@ -49,6 +49,7 @@ struct dw_dma_slave {
  * @data_width: Maximum data width supported by hardware per AHB master
  *		(in bytes, power of 2)
  * @multi_block: Multi block transfers supported by hardware per channel.
+ * @protctl:	Protection control signals setting per channel.
  */
 struct dw_dma_platform_data {
 	unsigned int	nr_channels;
@@ -65,6 +66,11 @@ struct dw_dma_platform_data {
 	unsigned char	nr_masters;
 	unsigned char	data_width[DW_DMA_MAX_NR_MASTERS];
 	unsigned char	multi_block[DW_DMA_MAX_NR_CHANNELS];
+#define CHAN_PROTCTL_PRIVILEGED		BIT(0)
+#define CHAN_PROTCTL_BUFFERABLE		BIT(1)
+#define CHAN_PROTCTL_CACHEABLE		BIT(2)
+#define	CHAN_PROTCTL_MASK		0x7
+	unsigned char	protctl[DW_DMA_MAX_NR_CHANNELS];
 };
 
 #endif /* _PLATFORM_DATA_DMA_DW_H */

[PATCH v1 2/2] dmaengine: dw: implement per-channel protection control setting

[Christian Lamparter]

This patch adds a new device-tree property that allows to
specify the protection control bits for each DMA channel
individually.

Setting the "correct" bits can have a huge impact on the
PPC460EX and APM82181 that use this DMA engine in combination
with a DesignWare' SATA-II core (sata_dwc_460ex driver).

In the OpenWrt Forum, the user takimata reported that:
|<https://forum.lede-project.org/t/wd-mybook-live-duo-two-disks/16195/55>
|It seems your patch unleashed the full power of the SATA port.
|Where I was previously hitting a really hard limit at around
|82 MB/s for reading and 27 MB/s for writing, I am now getting this:
|
|root@OpenWrt:/mnt# time dd if=/dev/zero of=tempfile bs=1M count=1024
|1024+0 records in
|1024+0 records out
|real    0m 13.65s
|user    0m 0.01s
|sys     0m 11.89s
|
|root@OpenWrt:/mnt# time dd if=tempfile of=/dev/null bs=1M count=1024
|1024+0 records in
|1024+0 records out
|real    0m 8.41s
|user    0m 0.01s
|sys     0m 4.70s
|
|This means: 121 MB/s reading and 75 MB/s writing!
|
|The drive is a WD Green WD10EARX taken from an older MBL Single.
|I repeated the test a few times with even larger files to rule out
|any caching, I'm still seeing the same great performance. OpenWrt is
|now completely on par with the original MBL firmware's performance.

Another user And.short reported in the same thread:
|<https://forum.openwrt.org/t/solved-wd-mybook-live-duo-two-disks/16195/50>
|I can report that your fix worked! Boots up fine with two
|drives even with more partitions, and no more reboot on
|concurrent disk access!

A closer look into the sata_dwc_460ex code revealed that
the driver did initally set the correct protection control
bits. However, this feature was lost when the sata_dwc_460ex
driver was converted to the generic DMA driver framework with:
8b3444852a2 ("sata_dwc_460ex: move to generic DMA driver").

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
 drivers/dma/dw/core.c                |  3 +++
 drivers/dma/dw/platform.c            | 12 +++++++++---
 drivers/dma/dw/regs.h                |  4 ++++
 include/linux/platform_data/dma-dw.h |  6 ++++++
 4 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
index f43e6dafe446..2db15e9b33a8 100644
--- a/drivers/dma/dw/core.c
+++ b/drivers/dma/dw/core.c
@@ -160,12 +160,15 @@ static void dwc_initialize_chan_idma32(struct dw_dma_chan *dwc)
 
 static void dwc_initialize_chan_dw(struct dw_dma_chan *dwc)
 {
+	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
+	size_t chanidx = (size_t)(dwc - dw->chan);
 	u32 cfghi = DWC_CFGH_FIFO_MODE;
 	u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority);
 	bool hs_polarity = dwc->dws.hs_polarity;
 
 	cfghi |= DWC_CFGH_DST_PER(dwc->dws.dst_id);
 	cfghi |= DWC_CFGH_SRC_PER(dwc->dws.src_id);
+	cfghi |= DWC_CFGH_PROTCTL(dw->pdata->protctl[chanidx]);
 
 	/* Set polarity of handshake interface */
 	cfglo |= hs_polarity ? DWC_CFGL_HS_DST_POL | DWC_CFGL_HS_SRC_POL : 0;
diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
index f62dd0944908..078cca6576c3 100644
--- a/drivers/dma/dw/platform.c
+++ b/drivers/dma/dw/platform.c
@@ -102,7 +102,7 @@ dw_dma_parse_dt(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct dw_dma_platform_data *pdata;
-	u32 tmp, arr[DW_DMA_MAX_NR_MASTERS], mb[DW_DMA_MAX_NR_CHANNELS];
+	u32 tmp, arr[DW_DMA_MAX_NR_MASTERS], val[DW_DMA_MAX_NR_CHANNELS];
 	u32 nr_masters;
 	u32 nr_channels;
 
@@ -154,14 +154,20 @@ dw_dma_parse_dt(struct platform_device *pdev)
 			pdata->data_width[tmp] = BIT(arr[tmp] & 0x07);
 	}
 
-	if (!of_property_read_u32_array(np, "multi-block", mb, nr_channels)) {
+	if (!of_property_read_u32_array(np, "multi-block", val, nr_channels)) {
 		for (tmp = 0; tmp < nr_channels; tmp++)
-			pdata->multi_block[tmp] = mb[tmp];
+			pdata->multi_block[tmp] = val[tmp];
 	} else {
 		for (tmp = 0; tmp < nr_channels; tmp++)
 			pdata->multi_block[tmp] = 1;
 	}
 
+	if (!of_property_read_u32_array(np, "snps,dma-protection-control",
+					val, nr_channels)) {
+		for (tmp = 0; tmp < nr_channels; tmp++)
+			pdata->protctl[tmp] = val[tmp];
+	}
+
 	return pdata;
 }
 #else
diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h
index 09e7dfdbb790..646c9c960c07 100644
--- a/drivers/dma/dw/regs.h
+++ b/drivers/dma/dw/regs.h
@@ -200,6 +200,10 @@ enum dw_dma_msize {
 #define DWC_CFGH_FCMODE		(1 << 0)
 #define DWC_CFGH_FIFO_MODE	(1 << 1)
 #define DWC_CFGH_PROTCTL(x)	((x) << 2)
+#define DWC_CFGH_PROTCTL_DATA	(0 << 2)	/* data access - always set */
+#define DWC_CFGH_PROTCTL_PRIV	(1 << 2)	/* privileged -> AHB HPROT[1] */
+#define DWC_CFGH_PROTCTL_BUFFER	(2 << 2)	/* bufferable -> AHB HPROT[2] */
+#define DWC_CFGH_PROTCTL_CACHE	(4 << 2)	/* cacheable  -> AHB HPROT[3] */
 #define DWC_CFGH_DS_UPD_EN	(1 << 5)
 #define DWC_CFGH_SS_UPD_EN	(1 << 6)
 #define DWC_CFGH_SRC_PER(x)	((x) << 7)
diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
index 896cb71a382c..df65e3311a56 100644
--- a/include/linux/platform_data/dma-dw.h
+++ b/include/linux/platform_data/dma-dw.h
@@ -49,6 +49,7 @@ struct dw_dma_slave {
  * @data_width: Maximum data width supported by hardware per AHB master
  *		(in bytes, power of 2)
  * @multi_block: Multi block transfers supported by hardware per channel.
+ * @protctl:	Protection control signals setting per channel.
  */
 struct dw_dma_platform_data {
 	unsigned int	nr_channels;
@@ -65,6 +66,11 @@ struct dw_dma_platform_data {
 	unsigned char	nr_masters;
 	unsigned char	data_width[DW_DMA_MAX_NR_MASTERS];
 	unsigned char	multi_block[DW_DMA_MAX_NR_CHANNELS];
+#define CHAN_PROTCTL_PRIVILEGED		BIT(0)
+#define CHAN_PROTCTL_BUFFERABLE		BIT(1)
+#define CHAN_PROTCTL_CACHEABLE		BIT(2)
+#define	CHAN_PROTCTL_MASK		0x7
+	unsigned char	protctl[DW_DMA_MAX_NR_CHANNELS];
 };
 
 #endif /* _PLATFORM_DATA_DMA_DW_H */
-- 
2.19.1

[v1,2/2] dmaengine: dw: implement per-channel protection control setting

[Andy Shevchenko]

Thanks for the patch, my comments below.

On Sun, Nov 04, 2018 at 06:01:39PM +0100, Christian Lamparter wrote:
> This patch adds a new device-tree property that allows to
> specify the protection control bits for each DMA channel
> individually.
> 
> Setting the "correct" bits can have a huge impact on the
> PPC460EX and APM82181 that use this DMA engine in combination
> with a DesignWare' SATA-II core (sata_dwc_460ex driver).
> 
> In the OpenWrt Forum, the user takimata reported that:
> |<https://forum.lede-project.org/t/wd-mybook-live-duo-two-disks/16195/55>

You may use BugLink: tag at the end of commit message.

> |It seems your patch unleashed the full power of the SATA port.
> |Where I was previously hitting a really hard limit at around
> |82 MB/s for reading and 27 MB/s for writing, I am now getting this:
> |
> |root@OpenWrt:/mnt# time dd if=/dev/zero of=tempfile bs=1M count=1024
> |1024+0 records in
> |1024+0 records out
> |real    0m 13.65s
> |user    0m 0.01s
> |sys     0m 11.89s
> |
> |root@OpenWrt:/mnt# time dd if=tempfile of=/dev/null bs=1M count=1024
> |1024+0 records in
> |1024+0 records out
> |real    0m 8.41s
> |user    0m 0.01s
> |sys     0m 4.70s
> |
> |This means: 121 MB/s reading and 75 MB/s writing!
> |
> |The drive is a WD Green WD10EARX taken from an older MBL Single.
> |I repeated the test a few times with even larger files to rule out
> |any caching, I'm still seeing the same great performance. OpenWrt is
> |now completely on par with the original MBL firmware's performance.
> 
> Another user And.short reported in the same thread:
> |<https://forum.openwrt.org/t/solved-wd-mybook-live-duo-two-disks/16195/50>

Another BugLink: tag entry :-)

> |I can report that your fix worked! Boots up fine with two
> |drives even with more partitions, and no more reboot on
> |concurrent disk access!
> 
> A closer look into the sata_dwc_460ex code revealed that
> the driver did initally set the correct protection control
> bits. 

> However, this feature was lost when the sata_dwc_460ex
> driver was converted to the generic DMA driver framework with:
> 8b3444852a2 ("sata_dwc_460ex: move to generic DMA driver").

Fixes: tag.

> 
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
>  drivers/dma/dw/core.c                |  3 +++
>  drivers/dma/dw/platform.c            | 12 +++++++++---
>  drivers/dma/dw/regs.h                |  4 ++++
>  include/linux/platform_data/dma-dw.h |  6 ++++++
>  4 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index f43e6dafe446..2db15e9b33a8 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -160,12 +160,15 @@ static void dwc_initialize_chan_idma32(struct dw_dma_chan *dwc)
>  
>  static void dwc_initialize_chan_dw(struct dw_dma_chan *dwc)
>  {
> +	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> +	size_t chanidx = (size_t)(dwc - dw->chan);

We have mask field, so, index is a first set bit out of mask, __ffs(mask).

	unsigned int protctl = dw->pdata->protctl[__ffs(mask)];


>  	u32 cfghi = DWC_CFGH_FIFO_MODE;
>  	u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority);
>  	bool hs_polarity = dwc->dws.hs_polarity;
>  
>  	cfghi |= DWC_CFGH_DST_PER(dwc->dws.dst_id);
>  	cfghi |= DWC_CFGH_SRC_PER(dwc->dws.src_id);
> +	cfghi |= DWC_CFGH_PROTCTL(dw->pdata->protctl[chanidx]);
>  
>  	/* Set polarity of handshake interface */
>  	cfglo |= hs_polarity ? DWC_CFGL_HS_DST_POL | DWC_CFGL_HS_SRC_POL : 0;
> diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
> index f62dd0944908..078cca6576c3 100644
> --- a/drivers/dma/dw/platform.c
> +++ b/drivers/dma/dw/platform.c
> @@ -102,7 +102,7 @@ dw_dma_parse_dt(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
>  	struct dw_dma_platform_data *pdata;
> -	u32 tmp, arr[DW_DMA_MAX_NR_MASTERS], mb[DW_DMA_MAX_NR_CHANNELS];
> +	u32 tmp, arr[DW_DMA_MAX_NR_MASTERS], val[DW_DMA_MAX_NR_CHANNELS];
>  	u32 nr_masters;
>  	u32 nr_channels;
>  
> @@ -154,14 +154,20 @@ dw_dma_parse_dt(struct platform_device *pdev)
>  			pdata->data_width[tmp] = BIT(arr[tmp] & 0x07);
>  	}
>  
> -	if (!of_property_read_u32_array(np, "multi-block", mb, nr_channels)) {
> +	if (!of_property_read_u32_array(np, "multi-block", val, nr_channels)) {
>  		for (tmp = 0; tmp < nr_channels; tmp++)
> -			pdata->multi_block[tmp] = mb[tmp];
> +			pdata->multi_block[tmp] = val[tmp];
>  	} else {
>  		for (tmp = 0; tmp < nr_channels; tmp++)
>  			pdata->multi_block[tmp] = 1;
>  	}
>  
> +	if (!of_property_read_u32_array(np, "snps,dma-protection-control",
> +					val, nr_channels)) {
> +		for (tmp = 0; tmp < nr_channels; tmp++)
> +			pdata->protctl[tmp] = val[tmp];
> +	}
> +
>  	return pdata;
>  }
>  #else
> diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h
> index 09e7dfdbb790..646c9c960c07 100644
> --- a/drivers/dma/dw/regs.h
> +++ b/drivers/dma/dw/regs.h
> @@ -200,6 +200,10 @@ enum dw_dma_msize {
>  #define DWC_CFGH_FCMODE		(1 << 0)
>  #define DWC_CFGH_FIFO_MODE	(1 << 1)
>  #define DWC_CFGH_PROTCTL(x)	((x) << 2)

> +#define DWC_CFGH_PROTCTL_DATA	(0 << 2)	/* data access - always set */
> +#define DWC_CFGH_PROTCTL_PRIV	(1 << 2)	/* privileged -> AHB HPROT[1] */
> +#define DWC_CFGH_PROTCTL_BUFFER	(2 << 2)	/* bufferable -> AHB HPROT[2] */
> +#define DWC_CFGH_PROTCTL_CACHE	(4 << 2)	/* cacheable  -> AHB HPROT[3] */

>  #define DWC_CFGH_DS_UPD_EN	(1 << 5)
>  #define DWC_CFGH_SS_UPD_EN	(1 << 6)
>  #define DWC_CFGH_SRC_PER(x)	((x) << 7)
> diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
> index 896cb71a382c..df65e3311a56 100644
> --- a/include/linux/platform_data/dma-dw.h
> +++ b/include/linux/platform_data/dma-dw.h
> @@ -49,6 +49,7 @@ struct dw_dma_slave {
>   * @data_width: Maximum data width supported by hardware per AHB master
>   *		(in bytes, power of 2)
>   * @multi_block: Multi block transfers supported by hardware per channel.
> + * @protctl:	Protection control signals setting per channel.
>   */
>  struct dw_dma_platform_data {
>  	unsigned int	nr_channels;
> @@ -65,6 +66,11 @@ struct dw_dma_platform_data {
>  	unsigned char	nr_masters;
>  	unsigned char	data_width[DW_DMA_MAX_NR_MASTERS];
>  	unsigned char	multi_block[DW_DMA_MAX_NR_CHANNELS];
> +#define CHAN_PROTCTL_PRIVILEGED		BIT(0)
> +#define CHAN_PROTCTL_BUFFERABLE		BIT(1)
> +#define CHAN_PROTCTL_CACHEABLE		BIT(2)

> +#define	CHAN_PROTCTL_MASK		0x7

GENMASK()

> +	unsigned char	protctl[DW_DMA_MAX_NR_CHANNELS];
>  };
>  
>  #endif /* _PLATFORM_DATA_DMA_DW_H */
> -- 
> 2.19.1
>

Re: [PATCH v1 2/2] dmaengine: dw: implement per-channel protection control setting

[Andy Shevchenko]

Thanks for the patch, my comments below.

On Sun, Nov 04, 2018 at 06:01:39PM +0100, Christian Lamparter wrote:
> This patch adds a new device-tree property that allows to
> specify the protection control bits for each DMA channel
> individually.
> 
> Setting the "correct" bits can have a huge impact on the
> PPC460EX and APM82181 that use this DMA engine in combination
> with a DesignWare' SATA-II core (sata_dwc_460ex driver).
> 
> In the OpenWrt Forum, the user takimata reported that:
> |<https://forum.lede-project.org/t/wd-mybook-live-duo-two-disks/16195/55>

You may use BugLink: tag at the end of commit message.

> |It seems your patch unleashed the full power of the SATA port.
> |Where I was previously hitting a really hard limit at around
> |82 MB/s for reading and 27 MB/s for writing, I am now getting this:
> |
> |root@OpenWrt:/mnt# time dd if=/dev/zero of=tempfile bs=1M count=1024
> |1024+0 records in
> |1024+0 records out
> |real    0m 13.65s
> |user    0m 0.01s
> |sys     0m 11.89s
> |
> |root@OpenWrt:/mnt# time dd if=tempfile of=/dev/null bs=1M count=1024
> |1024+0 records in
> |1024+0 records out
> |real    0m 8.41s
> |user    0m 0.01s
> |sys     0m 4.70s
> |
> |This means: 121 MB/s reading and 75 MB/s writing!
> |
> |The drive is a WD Green WD10EARX taken from an older MBL Single.
> |I repeated the test a few times with even larger files to rule out
> |any caching, I'm still seeing the same great performance. OpenWrt is
> |now completely on par with the original MBL firmware's performance.
> 
> Another user And.short reported in the same thread:
> |<https://forum.openwrt.org/t/solved-wd-mybook-live-duo-two-disks/16195/50>

Another BugLink: tag entry :-)

> |I can report that your fix worked! Boots up fine with two
> |drives even with more partitions, and no more reboot on
> |concurrent disk access!
> 
> A closer look into the sata_dwc_460ex code revealed that
> the driver did initally set the correct protection control
> bits. 

> However, this feature was lost when the sata_dwc_460ex
> driver was converted to the generic DMA driver framework with:
> 8b3444852a2 ("sata_dwc_460ex: move to generic DMA driver").

Fixes: tag.

> 
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
>  drivers/dma/dw/core.c                |  3 +++
>  drivers/dma/dw/platform.c            | 12 +++++++++---
>  drivers/dma/dw/regs.h                |  4 ++++
>  include/linux/platform_data/dma-dw.h |  6 ++++++
>  4 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index f43e6dafe446..2db15e9b33a8 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -160,12 +160,15 @@ static void dwc_initialize_chan_idma32(struct dw_dma_chan *dwc)
>  
>  static void dwc_initialize_chan_dw(struct dw_dma_chan *dwc)
>  {
> +	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> +	size_t chanidx = (size_t)(dwc - dw->chan);

We have mask field, so, index is a first set bit out of mask, __ffs(mask).

	unsigned int protctl = dw->pdata->protctl[__ffs(mask)];


>  	u32 cfghi = DWC_CFGH_FIFO_MODE;
>  	u32 cfglo = DWC_CFGL_CH_PRIOR(dwc->priority);
>  	bool hs_polarity = dwc->dws.hs_polarity;
>  
>  	cfghi |= DWC_CFGH_DST_PER(dwc->dws.dst_id);
>  	cfghi |= DWC_CFGH_SRC_PER(dwc->dws.src_id);
> +	cfghi |= DWC_CFGH_PROTCTL(dw->pdata->protctl[chanidx]);
>  
>  	/* Set polarity of handshake interface */
>  	cfglo |= hs_polarity ? DWC_CFGL_HS_DST_POL | DWC_CFGL_HS_SRC_POL : 0;
> diff --git a/drivers/dma/dw/platform.c b/drivers/dma/dw/platform.c
> index f62dd0944908..078cca6576c3 100644
> --- a/drivers/dma/dw/platform.c
> +++ b/drivers/dma/dw/platform.c
> @@ -102,7 +102,7 @@ dw_dma_parse_dt(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
>  	struct dw_dma_platform_data *pdata;
> -	u32 tmp, arr[DW_DMA_MAX_NR_MASTERS], mb[DW_DMA_MAX_NR_CHANNELS];
> +	u32 tmp, arr[DW_DMA_MAX_NR_MASTERS], val[DW_DMA_MAX_NR_CHANNELS];
>  	u32 nr_masters;
>  	u32 nr_channels;
>  
> @@ -154,14 +154,20 @@ dw_dma_parse_dt(struct platform_device *pdev)
>  			pdata->data_width[tmp] = BIT(arr[tmp] & 0x07);
>  	}
>  
> -	if (!of_property_read_u32_array(np, "multi-block", mb, nr_channels)) {
> +	if (!of_property_read_u32_array(np, "multi-block", val, nr_channels)) {
>  		for (tmp = 0; tmp < nr_channels; tmp++)
> -			pdata->multi_block[tmp] = mb[tmp];
> +			pdata->multi_block[tmp] = val[tmp];
>  	} else {
>  		for (tmp = 0; tmp < nr_channels; tmp++)
>  			pdata->multi_block[tmp] = 1;
>  	}
>  
> +	if (!of_property_read_u32_array(np, "snps,dma-protection-control",
> +					val, nr_channels)) {
> +		for (tmp = 0; tmp < nr_channels; tmp++)
> +			pdata->protctl[tmp] = val[tmp];
> +	}
> +
>  	return pdata;
>  }
>  #else
> diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h
> index 09e7dfdbb790..646c9c960c07 100644
> --- a/drivers/dma/dw/regs.h
> +++ b/drivers/dma/dw/regs.h
> @@ -200,6 +200,10 @@ enum dw_dma_msize {
>  #define DWC_CFGH_FCMODE		(1 << 0)
>  #define DWC_CFGH_FIFO_MODE	(1 << 1)
>  #define DWC_CFGH_PROTCTL(x)	((x) << 2)

> +#define DWC_CFGH_PROTCTL_DATA	(0 << 2)	/* data access - always set */
> +#define DWC_CFGH_PROTCTL_PRIV	(1 << 2)	/* privileged -> AHB HPROT[1] */
> +#define DWC_CFGH_PROTCTL_BUFFER	(2 << 2)	/* bufferable -> AHB HPROT[2] */
> +#define DWC_CFGH_PROTCTL_CACHE	(4 << 2)	/* cacheable  -> AHB HPROT[3] */

>  #define DWC_CFGH_DS_UPD_EN	(1 << 5)
>  #define DWC_CFGH_SS_UPD_EN	(1 << 6)
>  #define DWC_CFGH_SRC_PER(x)	((x) << 7)
> diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
> index 896cb71a382c..df65e3311a56 100644
> --- a/include/linux/platform_data/dma-dw.h
> +++ b/include/linux/platform_data/dma-dw.h
> @@ -49,6 +49,7 @@ struct dw_dma_slave {
>   * @data_width: Maximum data width supported by hardware per AHB master
>   *		(in bytes, power of 2)
>   * @multi_block: Multi block transfers supported by hardware per channel.
> + * @protctl:	Protection control signals setting per channel.
>   */
>  struct dw_dma_platform_data {
>  	unsigned int	nr_channels;
> @@ -65,6 +66,11 @@ struct dw_dma_platform_data {
>  	unsigned char	nr_masters;
>  	unsigned char	data_width[DW_DMA_MAX_NR_MASTERS];
>  	unsigned char	multi_block[DW_DMA_MAX_NR_CHANNELS];
> +#define CHAN_PROTCTL_PRIVILEGED		BIT(0)
> +#define CHAN_PROTCTL_BUFFERABLE		BIT(1)
> +#define CHAN_PROTCTL_CACHEABLE		BIT(2)

> +#define	CHAN_PROTCTL_MASK		0x7

GENMASK()

> +	unsigned char	protctl[DW_DMA_MAX_NR_CHANNELS];
>  };
>  
>  #endif /* _PLATFORM_DATA_DMA_DW_H */
> -- 
> 2.19.1
> 

-- 
With Best Regards,
Andy Shevchenko

[v1,1/2] dt-bindings: dmaengine: dw-dmac: add protection control property

[Andy Shevchenko]

On Sun, Nov 04, 2018 at 06:01:38PM +0100, Christian Lamparter wrote:
> This patch adds the per-channel dma protection control
> prop-encoded-array and dt-binding definitions (including
> definitions for the existing channel allocation and priority
> order values based on include/linux/platform_data/dma-dw.h)
> for the DesignWare AHB Central Direct Memory Access Controller.
> 
> Note: The protection control signals are one-to-one mapped to
> the AHB HPROT[1:3] signals.  The HPROT0 (Data Access) is
> always hardwired to 1 for this controller.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
>  .../devicetree/bindings/dma/snps-dma.txt      |  4 ++++
>  MAINTAINERS                                   |  4 +++-
>  include/dt-bindings/dma/dw-dmac.h             | 20 +++++++++++++++++++
>  3 files changed, 27 insertions(+), 1 deletion(-)
>  create mode 100644 include/dt-bindings/dma/dw-dmac.h
> 
> diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
> index 39e2b26be344..72b4984a4c18 100644
> --- a/Documentation/devicetree/bindings/dma/snps-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
> @@ -27,6 +27,10 @@ Optional properties:
>    general purpose DMA channel allocator. False if not passed.
>  - multi-block: Multi block transfers supported by hardware. Array property with
>    one cell per channel. 0: not supported, 1 (default): supported.
> +- snps,dma-protection-control: Channel's AHB HPROT[3:1] protection setting.
> +  Array property with one cell per channel. The default value for a channel
> +  is 0 (for non-cacheable, strongly ordered, unprivileged data access).
> +  Refer to include/dt-bindings/dma/dw-dmac.h for possible values.
>  
>  Example:
>  
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dacba23b80b4..c35998e20e9d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14107,9 +14107,11 @@ 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/snps-dma.txt
> +F:	drivers/dma/dw/
> +F:	include/dt-bindings/dma/dw-dmac.h
>  F:	include/linux/dma/dw.h
>  F:	include/linux/platform_data/dma-dw.h
> -F:	drivers/dma/dw/
>  
>  SYNOPSYS DESIGNWARE ENTERPRISE ETHERNET DRIVER
>  M:	Jose Abreu <Jose.Abreu@synopsys.com>
> diff --git a/include/dt-bindings/dma/dw-dmac.h b/include/dt-bindings/dma/dw-dmac.h
> new file mode 100644
> index 000000000000..9152a6e2406c
> --- /dev/null
> +++ b/include/dt-bindings/dma/dw-dmac.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +
> +#ifndef __DT_BINDINGS_DMA_DW_DMAC_H__
> +#define __DT_BINDINGS_DMA_DW_DMAC_H__
> +
> +#define DW_DMAC_CHAN_ALLOCATION_ASCENDING       0       /* zero to seven */
> +#define DW_DMAC_CHAN_ALLOCATION_DESCENDING      1       /* seven to zero */
> +#define DW_DMAC_CHAN_PRIORITY_ASCENDING         0       /* chan0 highest */
> +#define DW_DMAC_CHAN_PRIORITY_DESCENDING        1       /* chan7 highest */
> +
> +/*
> + * Protection Control bits provide protection against illegal transactions.
> + * The protection bits[0:2] are one-to-one mapped to AHB HPROT[3:1] signals.
> + * The AHB HPROT[0] bit is hardwired to 1: Data Access.
> + */
> +#define DW_DMAC_HPROT1_PRIVILEGED_MODE	(1 << 0)	/* Privileged Mode */
> +#define DW_DMAC_HPROT2_BUFFERABLE	(1 << 1)	/* DMA is bufferable */
> +#define DW_DMAC_HPROT3_CACHEABLE	(1 << 2)	/* DMA is cacheable */
> +
> +#endif /* __DT_BINDINGS_DMA_DW_DMAC_H__ */
> -- 
> 2.19.1
>

Re: [PATCH v1 1/2] dt-bindings: dmaengine: dw-dmac: add protection control property

[Andy Shevchenko]

On Sun, Nov 04, 2018 at 06:01:38PM +0100, Christian Lamparter wrote:
> This patch adds the per-channel dma protection control
> prop-encoded-array and dt-binding definitions (including
> definitions for the existing channel allocation and priority
> order values based on include/linux/platform_data/dma-dw.h)
> for the DesignWare AHB Central Direct Memory Access Controller.
> 
> Note: The protection control signals are one-to-one mapped to
> the AHB HPROT[1:3] signals.  The HPROT0 (Data Access) is
> always hardwired to 1 for this controller.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
>  .../devicetree/bindings/dma/snps-dma.txt      |  4 ++++
>  MAINTAINERS                                   |  4 +++-
>  include/dt-bindings/dma/dw-dmac.h             | 20 +++++++++++++++++++
>  3 files changed, 27 insertions(+), 1 deletion(-)
>  create mode 100644 include/dt-bindings/dma/dw-dmac.h
> 
> diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
> index 39e2b26be344..72b4984a4c18 100644
> --- a/Documentation/devicetree/bindings/dma/snps-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
> @@ -27,6 +27,10 @@ Optional properties:
>    general purpose DMA channel allocator. False if not passed.
>  - multi-block: Multi block transfers supported by hardware. Array property with
>    one cell per channel. 0: not supported, 1 (default): supported.
> +- snps,dma-protection-control: Channel's AHB HPROT[3:1] protection setting.
> +  Array property with one cell per channel. The default value for a channel
> +  is 0 (for non-cacheable, strongly ordered, unprivileged data access).
> +  Refer to include/dt-bindings/dma/dw-dmac.h for possible values.
>  
>  Example:
>  
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dacba23b80b4..c35998e20e9d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14107,9 +14107,11 @@ 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/snps-dma.txt
> +F:	drivers/dma/dw/
> +F:	include/dt-bindings/dma/dw-dmac.h
>  F:	include/linux/dma/dw.h
>  F:	include/linux/platform_data/dma-dw.h
> -F:	drivers/dma/dw/
>  
>  SYNOPSYS DESIGNWARE ENTERPRISE ETHERNET DRIVER
>  M:	Jose Abreu <Jose.Abreu@synopsys.com>
> diff --git a/include/dt-bindings/dma/dw-dmac.h b/include/dt-bindings/dma/dw-dmac.h
> new file mode 100644
> index 000000000000..9152a6e2406c
> --- /dev/null
> +++ b/include/dt-bindings/dma/dw-dmac.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +
> +#ifndef __DT_BINDINGS_DMA_DW_DMAC_H__
> +#define __DT_BINDINGS_DMA_DW_DMAC_H__
> +
> +#define DW_DMAC_CHAN_ALLOCATION_ASCENDING       0       /* zero to seven */
> +#define DW_DMAC_CHAN_ALLOCATION_DESCENDING      1       /* seven to zero */
> +#define DW_DMAC_CHAN_PRIORITY_ASCENDING         0       /* chan0 highest */
> +#define DW_DMAC_CHAN_PRIORITY_DESCENDING        1       /* chan7 highest */
> +
> +/*
> + * Protection Control bits provide protection against illegal transactions.
> + * The protection bits[0:2] are one-to-one mapped to AHB HPROT[3:1] signals.
> + * The AHB HPROT[0] bit is hardwired to 1: Data Access.
> + */
> +#define DW_DMAC_HPROT1_PRIVILEGED_MODE	(1 << 0)	/* Privileged Mode */
> +#define DW_DMAC_HPROT2_BUFFERABLE	(1 << 1)	/* DMA is bufferable */
> +#define DW_DMAC_HPROT3_CACHEABLE	(1 << 2)	/* DMA is cacheable */
> +
> +#endif /* __DT_BINDINGS_DMA_DW_DMAC_H__ */
> -- 
> 2.19.1
> 

-- 
With Best Regards,
Andy Shevchenko

[v1,2/2] dmaengine: dw: implement per-channel protection control setting

[Andy Shevchenko]

On Mon, Nov 05, 2018 at 04:22:54PM +0200, Andy Shevchenko wrote:
> On Sun, Nov 04, 2018 at 06:01:39PM +0100, Christian Lamparter wrote:

> > +	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> > +	size_t chanidx = (size_t)(dwc - dw->chan);
> 
> We have mask field, so, index is a first set bit out of mask, __ffs(mask).
> 
> 	unsigned int protctl = dw->pdata->protctl[__ffs(mask)];

dwc->mask, of course.

Also, it's possible to use (though better to check) dwc->chan.chan_id, though I
dunno if it's reliable.

Re: [PATCH v1 2/2] dmaengine: dw: implement per-channel protection control setting

[Andy Shevchenko]

On Mon, Nov 05, 2018 at 04:22:54PM +0200, Andy Shevchenko wrote:
> On Sun, Nov 04, 2018 at 06:01:39PM +0100, Christian Lamparter wrote:

> > +	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> > +	size_t chanidx = (size_t)(dwc - dw->chan);
> 
> We have mask field, so, index is a first set bit out of mask, __ffs(mask).
> 
> 	unsigned int protctl = dw->pdata->protctl[__ffs(mask)];

dwc->mask, of course.

Also, it's possible to use (though better to check) dwc->chan.chan_id, though I
dunno if it's reliable.

-- 
With Best Regards,
Andy Shevchenko

[v1,2/2] dmaengine: dw: implement per-channel protection control setting

[Christian Lamparter]

On Monday, November 5, 2018 3:27:41 PM CET Andy Shevchenko wrote:
> On Mon, Nov 05, 2018 at 04:22:54PM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 04, 2018 at 06:01:39PM +0100, Christian Lamparter wrote:
> 
> > > +	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> > > +	size_t chanidx = (size_t)(dwc - dw->chan);
> > 
> > We have mask field, so, index is a first set bit out of mask, __ffs(mask).
> > 
> > 	unsigned int protctl = dw->pdata->protctl[__ffs(mask)];
> 
> dwc->mask, of course.
Ok, will do. I'll sent a v2 later this week.

> Also, it's possible to use (though better to check) dwc->chan.chan_id,
> though I dunno if it's reliable.
dwc->chan.chan_id is subjected to the chan_allocation setting. 
So, if it's set to CHAN_ALLOCATION_DESCENDING the dt prop array values
for the protctl would need to be reversed as well in order to match the
other per-channel settings (for example multiblock).
So, let's not do that since this gets very confusing.

Re: [PATCH v1 2/2] dmaengine: dw: implement per-channel protection control setting

[Christian Lamparter]

On Monday, November 5, 2018 3:27:41 PM CET Andy Shevchenko wrote:
> On Mon, Nov 05, 2018 at 04:22:54PM +0200, Andy Shevchenko wrote:
> > On Sun, Nov 04, 2018 at 06:01:39PM +0100, Christian Lamparter wrote:
> 
> > > +	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> > > +	size_t chanidx = (size_t)(dwc - dw->chan);
> > 
> > We have mask field, so, index is a first set bit out of mask, __ffs(mask).
> > 
> > 	unsigned int protctl = dw->pdata->protctl[__ffs(mask)];
> 
> dwc->mask, of course.
Ok, will do. I'll sent a v2 later this week.

> Also, it's possible to use (though better to check) dwc->chan.chan_id,
> though I dunno if it's reliable.
dwc->chan.chan_id is subjected to the chan_allocation setting. 
So, if it's set to CHAN_ALLOCATION_DESCENDING the dt prop array values
for the protctl would need to be reversed as well in order to match the
other per-channel settings (for example multiblock).
So, let's not do that since this gets very confusing.

[v1,2/2] dmaengine: dw: implement per-channel protection control setting

[Andy Shevchenko]

On Mon, Nov 05, 2018 at 05:06:20PM +0100, Christian Lamparter wrote:
> On Monday, November 5, 2018 3:27:41 PM CET Andy Shevchenko wrote:
> > On Mon, Nov 05, 2018 at 04:22:54PM +0200, Andy Shevchenko wrote:
> > > On Sun, Nov 04, 2018 at 06:01:39PM +0100, Christian Lamparter wrote:
> > 
> > > > +	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> > > > +	size_t chanidx = (size_t)(dwc - dw->chan);
> > > 
> > > We have mask field, so, index is a first set bit out of mask, __ffs(mask).
> > > 
> > > 	unsigned int protctl = dw->pdata->protctl[__ffs(mask)];
> > 
> > dwc->mask, of course.
> Ok, will do. I'll sent a v2 later this week.
> 
> > Also, it's possible to use (though better to check) dwc->chan.chan_id,
> > though I dunno if it's reliable.
> dwc->chan.chan_id is subjected to the chan_allocation setting. 
> So, if it's set to CHAN_ALLOCATION_DESCENDING the dt prop array values
> for the protctl would need to be reversed as well in order to match the
> other per-channel settings (for example multiblock).
> So, let's not do that since this gets very confusing.

Yes, that's what I suspected. So, __ffs(dwc->mask) seems feasible approach.

Re: [PATCH v1 2/2] dmaengine: dw: implement per-channel protection control setting

[Andy Shevchenko]

On Mon, Nov 05, 2018 at 05:06:20PM +0100, Christian Lamparter wrote:
> On Monday, November 5, 2018 3:27:41 PM CET Andy Shevchenko wrote:
> > On Mon, Nov 05, 2018 at 04:22:54PM +0200, Andy Shevchenko wrote:
> > > On Sun, Nov 04, 2018 at 06:01:39PM +0100, Christian Lamparter wrote:
> > 
> > > > +	struct dw_dma *dw = to_dw_dma(dwc->chan.device);
> > > > +	size_t chanidx = (size_t)(dwc - dw->chan);
> > > 
> > > We have mask field, so, index is a first set bit out of mask, __ffs(mask).
> > > 
> > > 	unsigned int protctl = dw->pdata->protctl[__ffs(mask)];
> > 
> > dwc->mask, of course.
> Ok, will do. I'll sent a v2 later this week.
> 
> > Also, it's possible to use (though better to check) dwc->chan.chan_id,
> > though I dunno if it's reliable.
> dwc->chan.chan_id is subjected to the chan_allocation setting. 
> So, if it's set to CHAN_ALLOCATION_DESCENDING the dt prop array values
> for the protctl would need to be reversed as well in order to match the
> other per-channel settings (for example multiblock).
> So, let's not do that since this gets very confusing.

Yes, that's what I suspected. So, __ffs(dwc->mask) seems feasible approach.


-- 
With Best Regards,
Andy Shevchenko

[v1,1/2] dt-bindings: dmaengine: dw-dmac: add protection control property

[Rob Herring]

On Sun, Nov 04, 2018 at 06:01:38PM +0100, Christian Lamparter wrote:
> This patch adds the per-channel dma protection control
> prop-encoded-array and dt-binding definitions (including
> definitions for the existing channel allocation and priority
> order values based on include/linux/platform_data/dma-dw.h)
> for the DesignWare AHB Central Direct Memory Access Controller.
> 
> Note: The protection control signals are one-to-one mapped to
> the AHB HPROT[1:3] signals.  The HPROT0 (Data Access) is
> always hardwired to 1 for this controller.
> 
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
>  .../devicetree/bindings/dma/snps-dma.txt      |  4 ++++
>  MAINTAINERS                                   |  4 +++-
>  include/dt-bindings/dma/dw-dmac.h             | 20 +++++++++++++++++++
>  3 files changed, 27 insertions(+), 1 deletion(-)
>  create mode 100644 include/dt-bindings/dma/dw-dmac.h
> 
> diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
> index 39e2b26be344..72b4984a4c18 100644
> --- a/Documentation/devicetree/bindings/dma/snps-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
> @@ -27,6 +27,10 @@ Optional properties:
>    general purpose DMA channel allocator. False if not passed.
>  - multi-block: Multi block transfers supported by hardware. Array property with
>    one cell per channel. 0: not supported, 1 (default): supported.
> +- snps,dma-protection-control: Channel's AHB HPROT[3:1] protection setting.
> +  Array property with one cell per channel. The default value for a channel
> +  is 0 (for non-cacheable, strongly ordered, unprivileged data access).

IIRC, 'strongly ordered' is outside the scope of AHB (and AXI) 
definitions. There is a mapping of SO page table entries to bus control 
signals, but that's part of the cpu and outside the scope of this doc.

> +  Refer to include/dt-bindings/dma/dw-dmac.h for possible values.

Do you really need this to be per channel rather than just per platform? 
If anything, the optimal setting is probably based on the memory address 
(device, on-chip RAM, or DRAM), not the channel. 

I'd expect for most platforms, bufferable works without any s/w issue
(and should be more correct in that coherent allocations are bufferable 
(at least for ARM). Cacheable probably has no effect as most systems 
don't have coherent i/o (again, at least for ARM).


>  Example:
>  
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dacba23b80b4..c35998e20e9d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14107,9 +14107,11 @@ 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/snps-dma.txt
> +F:	drivers/dma/dw/
> +F:	include/dt-bindings/dma/dw-dmac.h
>  F:	include/linux/dma/dw.h
>  F:	include/linux/platform_data/dma-dw.h
> -F:	drivers/dma/dw/
>  
>  SYNOPSYS DESIGNWARE ENTERPRISE ETHERNET DRIVER
>  M:	Jose Abreu <Jose.Abreu@synopsys.com>
> diff --git a/include/dt-bindings/dma/dw-dmac.h b/include/dt-bindings/dma/dw-dmac.h
> new file mode 100644
> index 000000000000..9152a6e2406c
> --- /dev/null
> +++ b/include/dt-bindings/dma/dw-dmac.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +
> +#ifndef __DT_BINDINGS_DMA_DW_DMAC_H__
> +#define __DT_BINDINGS_DMA_DW_DMAC_H__
> +
> +#define DW_DMAC_CHAN_ALLOCATION_ASCENDING       0       /* zero to seven */
> +#define DW_DMAC_CHAN_ALLOCATION_DESCENDING      1       /* seven to zero */
> +#define DW_DMAC_CHAN_PRIORITY_ASCENDING         0       /* chan0 highest */
> +#define DW_DMAC_CHAN_PRIORITY_DESCENDING        1       /* chan7 highest */

These seem unrelated?

> +
> +/*
> + * Protection Control bits provide protection against illegal transactions.
> + * The protection bits[0:2] are one-to-one mapped to AHB HPROT[3:1] signals.
> + * The AHB HPROT[0] bit is hardwired to 1: Data Access.
> + */
> +#define DW_DMAC_HPROT1_PRIVILEGED_MODE	(1 << 0)	/* Privileged Mode */
> +#define DW_DMAC_HPROT2_BUFFERABLE	(1 << 1)	/* DMA is bufferable */
> +#define DW_DMAC_HPROT3_CACHEABLE	(1 << 2)	/* DMA is cacheable */
> +
> +#endif /* __DT_BINDINGS_DMA_DW_DMAC_H__ */
> -- 
> 2.19.1
>

Re: [PATCH v1 1/2] dt-bindings: dmaengine: dw-dmac: add protection control property

[Rob Herring]

On Sun, Nov 04, 2018 at 06:01:38PM +0100, Christian Lamparter wrote:
> This patch adds the per-channel dma protection control
> prop-encoded-array and dt-binding definitions (including
> definitions for the existing channel allocation and priority
> order values based on include/linux/platform_data/dma-dw.h)
> for the DesignWare AHB Central Direct Memory Access Controller.
> 
> Note: The protection control signals are one-to-one mapped to
> the AHB HPROT[1:3] signals.  The HPROT0 (Data Access) is
> always hardwired to 1 for this controller.
> 
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
>  .../devicetree/bindings/dma/snps-dma.txt      |  4 ++++
>  MAINTAINERS                                   |  4 +++-
>  include/dt-bindings/dma/dw-dmac.h             | 20 +++++++++++++++++++
>  3 files changed, 27 insertions(+), 1 deletion(-)
>  create mode 100644 include/dt-bindings/dma/dw-dmac.h
> 
> diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
> index 39e2b26be344..72b4984a4c18 100644
> --- a/Documentation/devicetree/bindings/dma/snps-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
> @@ -27,6 +27,10 @@ Optional properties:
>    general purpose DMA channel allocator. False if not passed.
>  - multi-block: Multi block transfers supported by hardware. Array property with
>    one cell per channel. 0: not supported, 1 (default): supported.
> +- snps,dma-protection-control: Channel's AHB HPROT[3:1] protection setting.
> +  Array property with one cell per channel. The default value for a channel
> +  is 0 (for non-cacheable, strongly ordered, unprivileged data access).

IIRC, 'strongly ordered' is outside the scope of AHB (and AXI) 
definitions. There is a mapping of SO page table entries to bus control 
signals, but that's part of the cpu and outside the scope of this doc.

> +  Refer to include/dt-bindings/dma/dw-dmac.h for possible values.

Do you really need this to be per channel rather than just per platform? 
If anything, the optimal setting is probably based on the memory address 
(device, on-chip RAM, or DRAM), not the channel. 

I'd expect for most platforms, bufferable works without any s/w issue
(and should be more correct in that coherent allocations are bufferable 
(at least for ARM). Cacheable probably has no effect as most systems 
don't have coherent i/o (again, at least for ARM).


>  Example:
>  
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dacba23b80b4..c35998e20e9d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14107,9 +14107,11 @@ 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/snps-dma.txt
> +F:	drivers/dma/dw/
> +F:	include/dt-bindings/dma/dw-dmac.h
>  F:	include/linux/dma/dw.h
>  F:	include/linux/platform_data/dma-dw.h
> -F:	drivers/dma/dw/
>  
>  SYNOPSYS DESIGNWARE ENTERPRISE ETHERNET DRIVER
>  M:	Jose Abreu <Jose.Abreu@synopsys.com>
> diff --git a/include/dt-bindings/dma/dw-dmac.h b/include/dt-bindings/dma/dw-dmac.h
> new file mode 100644
> index 000000000000..9152a6e2406c
> --- /dev/null
> +++ b/include/dt-bindings/dma/dw-dmac.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +
> +#ifndef __DT_BINDINGS_DMA_DW_DMAC_H__
> +#define __DT_BINDINGS_DMA_DW_DMAC_H__
> +
> +#define DW_DMAC_CHAN_ALLOCATION_ASCENDING       0       /* zero to seven */
> +#define DW_DMAC_CHAN_ALLOCATION_DESCENDING      1       /* seven to zero */
> +#define DW_DMAC_CHAN_PRIORITY_ASCENDING         0       /* chan0 highest */
> +#define DW_DMAC_CHAN_PRIORITY_DESCENDING        1       /* chan7 highest */

These seem unrelated?

> +
> +/*
> + * Protection Control bits provide protection against illegal transactions.
> + * The protection bits[0:2] are one-to-one mapped to AHB HPROT[3:1] signals.
> + * The AHB HPROT[0] bit is hardwired to 1: Data Access.
> + */
> +#define DW_DMAC_HPROT1_PRIVILEGED_MODE	(1 << 0)	/* Privileged Mode */
> +#define DW_DMAC_HPROT2_BUFFERABLE	(1 << 1)	/* DMA is bufferable */
> +#define DW_DMAC_HPROT3_CACHEABLE	(1 << 2)	/* DMA is cacheable */
> +
> +#endif /* __DT_BINDINGS_DMA_DW_DMAC_H__ */
> -- 
> 2.19.1
> 

[v1,1/2] dt-bindings: dmaengine: dw-dmac: add protection control property

[Christian Lamparter]

On Tuesday, November 6, 2018 12:06:52 AM CET Rob Herring wrote:
> On Sun, Nov 04, 2018 at 06:01:38PM +0100, Christian Lamparter wrote:
> > This patch adds the per-channel dma protection control
> > prop-encoded-array and dt-binding definitions (including
> > definitions for the existing channel allocation and priority
> > order values based on include/linux/platform_data/dma-dw.h)
> > for the DesignWare AHB Central Direct Memory Access Controller.
> > 
> > Note: The protection control signals are one-to-one mapped to
> > the AHB HPROT[1:3] signals.  The HPROT0 (Data Access) is
> > always hardwired to 1 for this controller.
> > 
> > Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> > ---
> >  .../devicetree/bindings/dma/snps-dma.txt      |  4 ++++
> >  MAINTAINERS                                   |  4 +++-
> >  include/dt-bindings/dma/dw-dmac.h             | 20 +++++++++++++++++++
> >  3 files changed, 27 insertions(+), 1 deletion(-)
> >  create mode 100644 include/dt-bindings/dma/dw-dmac.h
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
> > index 39e2b26be344..72b4984a4c18 100644
> > --- a/Documentation/devicetree/bindings/dma/snps-dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
> > @@ -27,6 +27,10 @@ Optional properties:
> >    general purpose DMA channel allocator. False if not passed.
> >  - multi-block: Multi block transfers supported by hardware. Array property with
> >    one cell per channel. 0: not supported, 1 (default): supported.
> > +- snps,dma-protection-control: Channel's AHB HPROT[3:1] protection setting.
> > +  Array property with one cell per channel. The default value for a channel
> > +  is 0 (for non-cacheable, strongly ordered, unprivileged data access).
> 
> IIRC, 'strongly ordered' is outside the scope of AHB (and AXI) 
> definitions. There is a mapping of SO page table entries to bus control 
> signals, but that's part of the cpu and outside the scope of this doc.
Ok, the "stongly ordered" is the term I got from the ARM AMBA specification as
the AMCC/APM Hardware document links to it. However, in the PROTCTL register
description it reads "non-buffered":

"The AMBA Specification recommends that the default value of HPROT indicates
a non-cached, non-buffered, privileged data access. The reset value is
used to indicate such an access. HPROT[0] is tied high because all transfers
are data accesses, as there are no opcode fetches."
 
> > +  Refer to include/dt-bindings/dma/dw-dmac.h for possible values.
> 
> Do you really need this to be per channel rather than just per platform? 
> If anything, the optimal setting is probably based on the memory address 
> (device, on-chip RAM, or DRAM), not the channel. 
For my APM82181 and PPC460EX SoCs "the platform" approach would work just
fine. I made this "per-channel" because the HPROT bits can be configured
for each dma channel individually. But as far as the APM82181 (and PPC460EX)
are concerned: They both are PowerPC-SoCs and the AHB is not even the main bus.
Instead the dw-dmac DMA-Controller is bundled together with two 
SATA-II Controllers behind a bidirectional PLB4-To-AHB-bridge that 
interfaces with the rest of the system. The funkyness stems from the
bridge that tries to translate, buffer and modify the data (including
the HPROT signals) to make everything "fit" more or less.

So, I'll make the snps,dma-protection-control property a single "u32"
and apply it to all the channels for the v2.

> I'd expect for most platforms, bufferable works without any s/w issue
> (and should be more correct in that coherent allocations are bufferable 
> (at least for ARM). Cacheable probably has no effect as most systems 
> don't have coherent i/o (again, at least for ARM).
I think I would it leave this decision to the arch. Because from what I
can tell (based on grepping the kernel source): the dw-dmac is used by
various ARM, ARC, PowerPC and x86 machines. (The avr32 arch used to be
a user too, but it has been dropped in 4.12.). And I don't want to 
accidentally break those. Or, @Viresh Kumar what's your opinion?

> >  Example:
> >  
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index dacba23b80b4..c35998e20e9d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -14107,9 +14107,11 @@ 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/snps-dma.txt
> > +F:	drivers/dma/dw/
> > +F:	include/dt-bindings/dma/dw-dmac.h
> >  F:	include/linux/dma/dw.h
> >  F:	include/linux/platform_data/dma-dw.h
> > -F:	drivers/dma/dw/
> >  
> >  SYNOPSYS DESIGNWARE ENTERPRISE ETHERNET DRIVER
> >  M:	Jose Abreu <Jose.Abreu@synopsys.com>
> > diff --git a/include/dt-bindings/dma/dw-dmac.h b/include/dt-bindings/dma/dw-dmac.h
> > new file mode 100644
> > index 000000000000..9152a6e2406c
> > --- /dev/null
> > +++ b/include/dt-bindings/dma/dw-dmac.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> > +
> > +#ifndef __DT_BINDINGS_DMA_DW_DMAC_H__
> > +#define __DT_BINDINGS_DMA_DW_DMAC_H__
> > +
> > +#define DW_DMAC_CHAN_ALLOCATION_ASCENDING       0       /* zero to seven */
> > +#define DW_DMAC_CHAN_ALLOCATION_DESCENDING      1       /* seven to zero */
> > +#define DW_DMAC_CHAN_PRIORITY_ASCENDING         0       /* chan0 highest */
> > +#define DW_DMAC_CHAN_PRIORITY_DESCENDING        1       /* chan7 highest */
> 
> These seem unrelated?
they belong to the existing chan_allocation_order and the chan_priority DT
properties. The values are matching what is already in the existing
include/linux/platform_data/dma-dw.h. But Ok, I'll leave them out in the next
version.
 
> > +
> > +/*
> > + * Protection Control bits provide protection against illegal transactions.
> > + * The protection bits[0:2] are one-to-one mapped to AHB HPROT[3:1] signals.
> > + * The AHB HPROT[0] bit is hardwired to 1: Data Access.
> > + */
> > +#define DW_DMAC_HPROT1_PRIVILEGED_MODE	(1 << 0)	/* Privileged Mode */
> > +#define DW_DMAC_HPROT2_BUFFERABLE	(1 << 1)	/* DMA is bufferable */
> > +#define DW_DMAC_HPROT3_CACHEABLE	(1 << 2)	/* DMA is cacheable */
> > +
> > +#endif /* __DT_BINDINGS_DMA_DW_DMAC_H__ */
>

Re: [PATCH v1 1/2] dt-bindings: dmaengine: dw-dmac: add protection control property

[Christian Lamparter]

On Tuesday, November 6, 2018 12:06:52 AM CET Rob Herring wrote:
> On Sun, Nov 04, 2018 at 06:01:38PM +0100, Christian Lamparter wrote:
> > This patch adds the per-channel dma protection control
> > prop-encoded-array and dt-binding definitions (including
> > definitions for the existing channel allocation and priority
> > order values based on include/linux/platform_data/dma-dw.h)
> > for the DesignWare AHB Central Direct Memory Access Controller.
> > 
> > Note: The protection control signals are one-to-one mapped to
> > the AHB HPROT[1:3] signals.  The HPROT0 (Data Access) is
> > always hardwired to 1 for this controller.
> > 
> > Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> > ---
> >  .../devicetree/bindings/dma/snps-dma.txt      |  4 ++++
> >  MAINTAINERS                                   |  4 +++-
> >  include/dt-bindings/dma/dw-dmac.h             | 20 +++++++++++++++++++
> >  3 files changed, 27 insertions(+), 1 deletion(-)
> >  create mode 100644 include/dt-bindings/dma/dw-dmac.h
> > 
> > diff --git a/Documentation/devicetree/bindings/dma/snps-dma.txt b/Documentation/devicetree/bindings/dma/snps-dma.txt
> > index 39e2b26be344..72b4984a4c18 100644
> > --- a/Documentation/devicetree/bindings/dma/snps-dma.txt
> > +++ b/Documentation/devicetree/bindings/dma/snps-dma.txt
> > @@ -27,6 +27,10 @@ Optional properties:
> >    general purpose DMA channel allocator. False if not passed.
> >  - multi-block: Multi block transfers supported by hardware. Array property with
> >    one cell per channel. 0: not supported, 1 (default): supported.
> > +- snps,dma-protection-control: Channel's AHB HPROT[3:1] protection setting.
> > +  Array property with one cell per channel. The default value for a channel
> > +  is 0 (for non-cacheable, strongly ordered, unprivileged data access).
> 
> IIRC, 'strongly ordered' is outside the scope of AHB (and AXI) 
> definitions. There is a mapping of SO page table entries to bus control 
> signals, but that's part of the cpu and outside the scope of this doc.
Ok, the "stongly ordered" is the term I got from the ARM AMBA specification as
the AMCC/APM Hardware document links to it. However, in the PROTCTL register
description it reads "non-buffered":

"The AMBA Specification recommends that the default value of HPROT indicates
a non-cached, non-buffered, privileged data access. The reset value is
used to indicate such an access. HPROT[0] is tied high because all transfers
are data accesses, as there are no opcode fetches."
 
> > +  Refer to include/dt-bindings/dma/dw-dmac.h for possible values.
> 
> Do you really need this to be per channel rather than just per platform? 
> If anything, the optimal setting is probably based on the memory address 
> (device, on-chip RAM, or DRAM), not the channel. 
For my APM82181 and PPC460EX SoCs "the platform" approach would work just
fine. I made this "per-channel" because the HPROT bits can be configured
for each dma channel individually. But as far as the APM82181 (and PPC460EX)
are concerned: They both are PowerPC-SoCs and the AHB is not even the main bus.
Instead the dw-dmac DMA-Controller is bundled together with two 
SATA-II Controllers behind a bidirectional PLB4-To-AHB-bridge that 
interfaces with the rest of the system. The funkyness stems from the
bridge that tries to translate, buffer and modify the data (including
the HPROT signals) to make everything "fit" more or less.

So, I'll make the snps,dma-protection-control property a single "u32"
and apply it to all the channels for the v2.

> I'd expect for most platforms, bufferable works without any s/w issue
> (and should be more correct in that coherent allocations are bufferable 
> (at least for ARM). Cacheable probably has no effect as most systems 
> don't have coherent i/o (again, at least for ARM).
I think I would it leave this decision to the arch. Because from what I
can tell (based on grepping the kernel source): the dw-dmac is used by
various ARM, ARC, PowerPC and x86 machines. (The avr32 arch used to be
a user too, but it has been dropped in 4.12.). And I don't want to 
accidentally break those. Or, @Viresh Kumar what's your opinion?

> >  Example:
> >  
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index dacba23b80b4..c35998e20e9d 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -14107,9 +14107,11 @@ 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/snps-dma.txt
> > +F:	drivers/dma/dw/
> > +F:	include/dt-bindings/dma/dw-dmac.h
> >  F:	include/linux/dma/dw.h
> >  F:	include/linux/platform_data/dma-dw.h
> > -F:	drivers/dma/dw/
> >  
> >  SYNOPSYS DESIGNWARE ENTERPRISE ETHERNET DRIVER
> >  M:	Jose Abreu <Jose.Abreu@synopsys.com>
> > diff --git a/include/dt-bindings/dma/dw-dmac.h b/include/dt-bindings/dma/dw-dmac.h
> > new file mode 100644
> > index 000000000000..9152a6e2406c
> > --- /dev/null
> > +++ b/include/dt-bindings/dma/dw-dmac.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> > +
> > +#ifndef __DT_BINDINGS_DMA_DW_DMAC_H__
> > +#define __DT_BINDINGS_DMA_DW_DMAC_H__
> > +
> > +#define DW_DMAC_CHAN_ALLOCATION_ASCENDING       0       /* zero to seven */
> > +#define DW_DMAC_CHAN_ALLOCATION_DESCENDING      1       /* seven to zero */
> > +#define DW_DMAC_CHAN_PRIORITY_ASCENDING         0       /* chan0 highest */
> > +#define DW_DMAC_CHAN_PRIORITY_DESCENDING        1       /* chan7 highest */
> 
> These seem unrelated?
they belong to the existing chan_allocation_order and the chan_priority DT
properties. The values are matching what is already in the existing
include/linux/platform_data/dma-dw.h. But Ok, I'll leave them out in the next
version.
 
> > +
> > +/*
> > + * Protection Control bits provide protection against illegal transactions.
> > + * The protection bits[0:2] are one-to-one mapped to AHB HPROT[3:1] signals.
> > + * The AHB HPROT[0] bit is hardwired to 1: Data Access.
> > + */
> > +#define DW_DMAC_HPROT1_PRIVILEGED_MODE	(1 << 0)	/* Privileged Mode */
> > +#define DW_DMAC_HPROT2_BUFFERABLE	(1 << 1)	/* DMA is bufferable */
> > +#define DW_DMAC_HPROT3_CACHEABLE	(1 << 2)	/* DMA is cacheable */
> > +
> > +#endif /* __DT_BINDINGS_DMA_DW_DMAC_H__ */
>