[PATCH 0/2] Allwinner V3s DMA support

This is a dedicated patchset of Allwinner V3s DMA support, which used
to be part of the audio codec support patchset.

It's a derivation of the DMA part of v3 of the codec patchset.

Icenowy Zheng (2):
  dmaengine: sun6i: make gate bit in sun8i's DMA engines a common quirk
  dmaengine: sun6i: support V3s SoC variant

 .../devicetree/bindings/dma/sun6i-dma.txt          |  1 +
 drivers/dma/sun6i-dma.c                            | 33 +++++++++++++++++-----
 2 files changed, 27 insertions(+), 7 deletions(-)

-- 
2.12.2

From: Icenowy Zheng <redacted>

Originally we enable a special gate bit when the compatible indicates
A23/33.

But according to BSP sources and user manuals, more SoCs will need this
gate bit.

So make it a common quirk configured in the config struct.

Signed-off-by: Icenowy Zheng <redacted>
---
Changes since original codec patchset v3:
- Refactored comments to cover some words found in official documents.
- Removed the comments when toggling the gate bit.

 drivers/dma/sun6i-dma.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index a2358780ab2c..252b59c1d1d5 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c

@@ -101,6 +101,17 @@ struct sun6i_dma_config {
 	u32 nr_max_channels;
 	u32 nr_max_requests;
 	u32 nr_max_vchans;
+	/*
+	 * In the datasheets/user manuals of newer Allwinner SoCs, a special
+	 * bit (bit 2 at register 0x20) is present.
+	 * It's named "DMA MCLK interface circuit auto gating bit" in the
+	 * documents, and the footnote of this register says that this bit
+	 * should be set up when initializing the DMA controller.
+	 * Allwinner A23/A33 user manuals do not have this bit documented,
+	 * however these SoCs really have and need this bit, as seen in the
+	 * BSP kernel source code.
+	 */
+	bool gate_needed;
 };
 
 /*

@@ -1009,6 +1020,7 @@ static struct sun6i_dma_config sun8i_a23_dma_cfg = {
 	.nr_max_channels = 8,
 	.nr_max_requests = 24,
 	.nr_max_vchans   = 37,
+	.gate_needed	 = true,
 };
 
 static struct sun6i_dma_config sun8i_a83t_dma_cfg = {

@@ -1174,13 +1186,7 @@ static int sun6i_dma_probe(struct platform_device *pdev)
 		goto err_dma_unregister;
 	}
 
-	/*
-	 * sun8i variant requires us to toggle a dma gating register,
-	 * as seen in Allwinner's SDK. This register is not documented
-	 * in the A23 user manual.
-	 */
-	if (of_device_is_compatible(pdev->dev.of_node,
-				    "allwinner,sun8i-a23-dma"))
+	if (sdc->cfg->gate_needed)
 		writel(SUN8I_DMA_GATE_ENABLE, sdc->base + SUN8I_DMA_GATE);
 
 	return 0;

-- 
2.12.2

From: Icenowy Zheng <redacted>

Allwinner V3s has a DMA engine similar to the ones from A31, but with
fewer channels and DRQs.

Add support for it.

Signed-off-by: Icenowy Zheng <redacted>
Acked-by: Chen-Yu Tsai <redacted>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes since the original codec patchset v3:
- Added Rob's ACK.

 Documentation/devicetree/bindings/dma/sun6i-dma.txt |  1 +
 drivers/dma/sun6i-dma.c                             | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/sun6i-dma.txt b/Documentation/devicetree/bindings/dma/sun6i-dma.txt
index 6b267045f522..98fbe1a5c6dd 100644
--- a/Documentation/devicetree/bindings/dma/sun6i-dma.txt
+++ b/Documentation/devicetree/bindings/dma/sun6i-dma.txt

@@ -9,6 +9,7 @@ Required properties:
 		  "allwinner,sun8i-a23-dma"
 		  "allwinner,sun8i-a83t-dma"
 		  "allwinner,sun8i-h3-dma"
+		  "allwinner,sun8i-v3s-dma"
 - reg:		Should contain the registers base address and length
 - interrupts:	Should contain a reference to the interrupt used by this device
 - clocks:	Should contain a reference to the parent AHB clock

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 252b59c1d1d5..bcd496edc70f 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c

@@ -1040,11 +1040,24 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
 	.nr_max_vchans   = 34,
 };
 
+/*
+ * The V3s have only 8 physical channels, a maximum DRQ port id of 23,
+ * and a total of 24 usable source and destination endpoints.
+ */
+
+static struct sun6i_dma_config sun8i_v3s_dma_cfg = {
+	.nr_max_channels = 8,
+	.nr_max_requests = 23,
+	.nr_max_vchans   = 24,
+	.gate_needed	 = true,
+};
+
 static const struct of_device_id sun6i_dma_match[] = {
 	{ .compatible = "allwinner,sun6i-a31-dma", .data = &sun6i_a31_dma_cfg },
 	{ .compatible = "allwinner,sun8i-a23-dma", .data = &sun8i_a23_dma_cfg },
 	{ .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 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, sun6i_dma_match);

-- 
2.12.2

On Mon, Jun 5, 2017 at 8:33 PM, Icenowy Zheng [off-list ref] wrote:

From: Icenowy Zheng <redacted>

Originally we enable a special gate bit when the compatible indicates
A23/33.

But according to BSP sources and user manuals, more SoCs will need this
gate bit.

So make it a common quirk configured in the config struct.

Signed-off-by: Icenowy Zheng <redacted>

Reviewed-by: Chen-Yu Tsai <redacted>

On Mon, Jun 05, 2017 at 08:33:47PM +0800, Icenowy Zheng wrote:

Since this is a hw property, why is this not added as an optional DT
property?

-- 
~Vinod

? 2017?6?14? GMT+08:00 ??4:32:52, Vinod Koul [off-list ref] ??:

On Mon, Jun 05, 2017 at 08:33:47PM +0800, Icenowy Zheng wrote:

quoted

From: Icenowy Zheng <redacted>

Originally we enable a special gate bit when the compatible indicates
A23/33.

But according to BSP sources and user manuals, more SoCs will need

this

quoted

gate bit.

So make it a common quirk configured in the config struct.

Signed-off-by: Icenowy Zheng <redacted>
---
Changes since original codec patchset v3:
- Refactored comments to cover some words found in official

documents.

quoted

- Removed the comments when toggling the gate bit.

 drivers/dma/sun6i-dma.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index a2358780ab2c..252b59c1d1d5 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c

@@ -101,6 +101,17 @@ struct sun6i_dma_config {
 	u32 nr_max_channels;
 	u32 nr_max_requests;
 	u32 nr_max_vchans;
+	/*
+	 * In the datasheets/user manuals of newer Allwinner SoCs, a

special

quoted

+	 * bit (bit 2 at register 0x20) is present.
+	 * It's named "DMA MCLK interface circuit auto gating bit" in the
+	 * documents, and the footnote of this register says that this bit
+	 * should be set up when initializing the DMA controller.
+	 * Allwinner A23/A33 user manuals do not have this bit documented,
+	 * however these SoCs really have and need this bit, as seen in the
+	 * BSP kernel source code.
+	 */
+	bool gate_needed;

Since this is a hw property, why is this not added as an optional DT
property?

As it's SoC-specified.

Some SoCs need it, and some don't.

SoC info is in compatible, so there's no reason to make it a property.

quoted

 };
 
 /*

@@ -1009,6 +1020,7 @@ static struct sun6i_dma_config

sun8i_a23_dma_cfg = {

quoted

 	.nr_max_channels = 8,
 	.nr_max_requests = 24,
 	.nr_max_vchans   = 37,
+	.gate_needed	 = true,
 };
 
 static struct sun6i_dma_config sun8i_a83t_dma_cfg = {

@@ -1174,13 +1186,7 @@ static int sun6i_dma_probe(struct

platform_device *pdev)

quoted

 		goto err_dma_unregister;
 	}
 
-	/*
-	 * sun8i variant requires us to toggle a dma gating register,
-	 * as seen in Allwinner's SDK. This register is not documented
-	 * in the A23 user manual.
-	 */
-	if (of_device_is_compatible(pdev->dev.of_node,
-				    "allwinner,sun8i-a23-dma"))
+	if (sdc->cfg->gate_needed)
 		writel(SUN8I_DMA_GATE_ENABLE, sdc->base + SUN8I_DMA_GATE);
 
 	return 0;
-- 
2.12.2

On Wed, Jun 14, 2017 at 04:32:57PM +0800, Icenowy Zheng wrote:

? 2017?6?14? GMT+08:00 ??4:32:52, Vinod Koul [off-list ref] ??:

quoted

On Mon, Jun 05, 2017 at 08:33:47PM +0800, Icenowy Zheng wrote:

quoted

From: Icenowy Zheng <redacted>

Originally we enable a special gate bit when the compatible indicates
A23/33.

But according to BSP sources and user manuals, more SoCs will need

this

quoted

gate bit.

So make it a common quirk configured in the config struct.

Signed-off-by: Icenowy Zheng <redacted>
---
Changes since original codec patchset v3:
- Refactored comments to cover some words found in official

documents.

quoted

- Removed the comments when toggling the gate bit.

 drivers/dma/sun6i-dma.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index a2358780ab2c..252b59c1d1d5 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c

@@ -101,6 +101,17 @@ struct sun6i_dma_config {
 	u32 nr_max_channels;
 	u32 nr_max_requests;
 	u32 nr_max_vchans;
+	/*
+	 * In the datasheets/user manuals of newer Allwinner SoCs, a

special

quoted

+	 * bit (bit 2 at register 0x20) is present.
+	 * It's named "DMA MCLK interface circuit auto gating bit" in the
+	 * documents, and the footnote of this register says that this bit
+	 * should be set up when initializing the DMA controller.
+	 * Allwinner A23/A33 user manuals do not have this bit documented,
+	 * however these SoCs really have and need this bit, as seen in the
+	 * BSP kernel source code.
+	 */
+	bool gate_needed;

Since this is a hw property, why is this not added as an optional DT
property?

As it's SoC-specified.

Some SoCs need it, and some don't.

and that is the reason it should be a property

SoC info is in compatible, so there's no reason to make it a property.

that's why it would need to be optional for the SoC's that needs these..

quoted

quoted

 };
 
 /*

@@ -1009,6 +1020,7 @@ static struct sun6i_dma_config

sun8i_a23_dma_cfg = {

quoted

 	.nr_max_channels = 8,
 	.nr_max_requests = 24,
 	.nr_max_vchans   = 37,
+	.gate_needed	 = true,
 };
 
 static struct sun6i_dma_config sun8i_a83t_dma_cfg = {

@@ -1174,13 +1186,7 @@ static int sun6i_dma_probe(struct

platform_device *pdev)

quoted

 		goto err_dma_unregister;
 	}
 
-	/*
-	 * sun8i variant requires us to toggle a dma gating register,
-	 * as seen in Allwinner's SDK. This register is not documented
-	 * in the A23 user manual.
-	 */
-	if (of_device_is_compatible(pdev->dev.of_node,
-				    "allwinner,sun8i-a23-dma"))
+	if (sdc->cfg->gate_needed)
 		writel(SUN8I_DMA_GATE_ENABLE, sdc->base + SUN8I_DMA_GATE);
 
 	return 0;
-- 
2.12.2

-- 
~Vinod

? 2017?6?14? GMT+08:00 ??4:45:29, Vinod Koul [off-list ref] ??:

On Wed, Jun 14, 2017 at 04:32:57PM +0800, Icenowy Zheng wrote:

quoted

? 2017?6?14? GMT+08:00 ??4:32:52, Vinod Koul [off-list ref]

??:

quoted

quoted

On Mon, Jun 05, 2017 at 08:33:47PM +0800, Icenowy Zheng wrote:

quoted

From: Icenowy Zheng <redacted>

Originally we enable a special gate bit when the compatible

indicates

quoted

quoted

quoted

A23/33.

But according to BSP sources and user manuals, more SoCs will need

this

quoted

gate bit.

So make it a common quirk configured in the config struct.

Signed-off-by: Icenowy Zheng <redacted>
---
Changes since original codec patchset v3:
- Refactored comments to cover some words found in official

documents.

quoted

- Removed the comments when toggling the gate bit.

 drivers/dma/sun6i-dma.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index a2358780ab2c..252b59c1d1d5 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c

@@ -101,6 +101,17 @@ struct sun6i_dma_config {
 	u32 nr_max_channels;
 	u32 nr_max_requests;
 	u32 nr_max_vchans;
+	/*
+	 * In the datasheets/user manuals of newer Allwinner SoCs, a

special

quoted

+	 * bit (bit 2 at register 0x20) is present.
+	 * It's named "DMA MCLK interface circuit auto gating bit" in

the

quoted

quoted

quoted

+	 * documents, and the footnote of this register says that this

bit

quoted

quoted

quoted

+	 * should be set up when initializing the DMA controller.
+	 * Allwinner A23/A33 user manuals do not have this bit

documented,

quoted

quoted

quoted

+	 * however these SoCs really have and need this bit, as seen in

the

quoted

quoted

quoted

+	 * BSP kernel source code.
+	 */
+	bool gate_needed;

Since this is a hw property, why is this not added as an optional DT
property?

As it's SoC-specified.

Some SoCs need it, and some don't.

and that is the reason it should be a property

quoted

SoC info is in compatible, so there's no reason to make it a

property.

that's why it would need to be optional for the SoC's that needs
these..

I don't think it proper to add block-specified properties
that can be bound to compatible.

I added Rob Herring to the recipient list.

Rob, do you think this can be added as a property?

This is SoC-specific and compatibles are also SoC-specific.

quoted

quoted

quoted

 };
 
 /*

@@ -1009,6 +1020,7 @@ static struct sun6i_dma_config

sun8i_a23_dma_cfg = {

quoted

 	.nr_max_channels = 8,
 	.nr_max_requests = 24,
 	.nr_max_vchans   = 37,
+	.gate_needed	 = true,
 };
 
 static struct sun6i_dma_config sun8i_a83t_dma_cfg = {

@@ -1174,13 +1186,7 @@ static int sun6i_dma_probe(struct

platform_device *pdev)

quoted

 		goto err_dma_unregister;
 	}
 
-	/*
-	 * sun8i variant requires us to toggle a dma gating register,
-	 * as seen in Allwinner's SDK. This register is not documented
-	 * in the A23 user manual.
-	 */
-	if (of_device_is_compatible(pdev->dev.of_node,
-				    "allwinner,sun8i-a23-dma"))
+	if (sdc->cfg->gate_needed)
 		writel(SUN8I_DMA_GATE_ENABLE, sdc->base + SUN8I_DMA_GATE);
 
 	return 0;
-- 
2.12.2

On Wed, Jun 14, 2017 at 02:15:29PM +0530, Vinod Koul wrote:

quoted

SoC info is in compatible, so there's no reason to make it a property.

that's why it would need to be optional for the SoC's that needs these..

There's nothing optional about that behaviour, it's mandatory for the
SoC that need it, and useless on the SoC that don't.

Plus, that would require changing the DT binding, which isn't
something we can do.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170614/803c9893/attachment.sig>

On Wed, Jun 14, 2017 at 11:04:39AM +0200, Maxime Ripard wrote:

On Wed, Jun 14, 2017 at 02:15:29PM +0530, Vinod Koul wrote:

quoted

quoted

SoC info is in compatible, so there's no reason to make it a property.

that's why it would need to be optional for the SoC's that needs these..

There's nothing optional about that behaviour, it's mandatory for the
SoC that need it, and useless on the SoC that don't.

And why should kernel put strings for each hw behaviour. I am expecting DT
to tell me if this SoC is a special case or not and kernel shall handle
accordingly

Plus, that would require changing the DT binding, which isn't
something we can do.

Any reason why bindings can't change..? I though this was support for new
SoC...

-- 
~Vinod

? 2017?6?15? GMT+08:00 ??11:54:08, Vinod Koul [off-list ref] ??:

On Wed, Jun 14, 2017 at 11:04:39AM +0200, Maxime Ripard wrote:

quoted

On Wed, Jun 14, 2017 at 02:15:29PM +0530, Vinod Koul wrote:

quoted

quoted

SoC info is in compatible, so there's no reason to make it a

property.

quoted

quoted

that's why it would need to be optional for the SoC's that needs

these..

quoted

There's nothing optional about that behaviour, it's mandatory for the
SoC that need it, and useless on the SoC that don't.

And why should kernel put strings for each hw behaviour. I am expecting
DT
to tell me if this SoC is a special case or not and kernel shall handle
accordingly

I don't think this kind of behavior should be described in DT.

Rob, do you agree?

quoted

Plus, that would require changing the DT binding, which isn't
something we can do.

Any reason why bindings can't change..? I though this was support for
new
SoC...

This is a behavior that exists on a SoC that is already
supported (A23/A33).

On Thu, Jun 15, 2017 at 09:24:08AM +0530, Vinod Koul wrote:

On Wed, Jun 14, 2017 at 11:04:39AM +0200, Maxime Ripard wrote:

quoted

On Wed, Jun 14, 2017 at 02:15:29PM +0530, Vinod Koul wrote:

quoted

quoted

SoC info is in compatible, so there's no reason to make it a property.

that's why it would need to be optional for the SoC's that needs these..

There's nothing optional about that behaviour, it's mandatory for the
SoC that need it, and useless on the SoC that don't.

And why should kernel put strings for each hw behaviour.

You will have strings in the kernel for each hw behaviour,
disregarding on whether you base the behaviour on the compatible or a
set of properties. In fact, you will have *much* more strings in the
kernel in the latter case.

I am expecting DT to tell me if this SoC is a special case or not
and kernel shall handle accordingly

How is this not the case here?

The DT tells you that this SoC is a special case through a compatible
already.

quoted

Plus, that would require changing the DT binding, which isn't
something we can do.

Any reason why bindings can't change..? I though this was support for new
SoC...

No, this is a rework of an existing code to support a new SoC. The
code is already there, and the binding too. It has been for 3 years.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170620/88a09bac/attachment.sig>