diff mbox series

[3/3] dmaengine: sun6i: Add support for Allwinner A64

Message ID 20170830233609.13855-4-stefan.bruens@rwth-aachen.de
State New
Headers show
Series dmaengine: Fix DMA on current allwinner SoCs, add A64 support | expand

Commit Message

Stefan Brüns Aug. 30, 2017, 11:36 p.m. UTC
The A64 SoC has the same dma engine as the H3 (sun8i), with a
reduced amount of physical channels. Add the proper config data
and compatible string to support it.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++
 drivers/dma/sun6i-dma.c                       | 7 +++++++
 2 files changed, 11 insertions(+)

Comments

Code Kipper Aug. 31, 2017, 11:44 a.m. UTC | #1
On 31 August 2017 at 01:36, Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> The A64 SoC has the same dma engine as the H3 (sun8i), with a
> reduced amount of physical channels. Add the proper config data
> and compatible string to support it.
>
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++
>  drivers/dma/sun6i-dma.c                       | 7 +++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index f96287d3043a..b86019238b77 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -494,6 +494,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";
Hi Stefan,
the dtsi parts should be in a separate patch

>                         pinctrl-names = "default";
>                         pinctrl-0 = <&spi0_pins>;
>                         resets = <&ccu RST_BUS_SPI0>;
> @@ -509,6 +511,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>;
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 5f4eee4513e5..6a17c5d63582 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
>         .nr_max_vchans   = 34,
>         .dmac_variant    = DMAC_VARIANT_H3,
>  };
> +
> +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
> +       .nr_max_channels = 8,
> +       .nr_max_requests = 27,
> +       .nr_max_vchans   = 38,
> +       .dmac_variant    = DMAC_VARIANT_H3,
>  };
>
>  static const struct of_device_id sun6i_dma_match[] = {
> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = {
>         { .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,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg },
This all looks fine...it's similar to my patch here
https://github.com/codekipper/linux-sunxi/commit/8c54d9852dfad6ceb478c579a1213f38fb12fa80
which I've been too lazy to post. I think the binding documentation
should go with this patch and this should also be the 1st patch in the
series, followed by the dtsi changes.
BR,
CK
>         { /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, sun6i_dma_match);
> --
> 2.14.1
>
Maxime Ripard Aug. 31, 2017, 2:52 p.m. UTC | #2
On Thu, Aug 31, 2017 at 01:36:09AM +0200, Stefan Brüns wrote:
> The A64 SoC has the same dma engine as the H3 (sun8i), with a
> reduced amount of physical channels. Add the proper config data
> and compatible string to support it.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++
>  drivers/dma/sun6i-dma.c                       | 7 +++++++
>  2 files changed, 11 insertions(+)

With what device did you test this? As far as I know, the SPI driver
doesn't use DMA at all.

Maxime
Code Kipper Aug. 31, 2017, 4:35 p.m. UTC | #3
On 31 August 2017 at 16:52, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Thu, Aug 31, 2017 at 01:36:09AM +0200, Stefan Brüns wrote:
>> The A64 SoC has the same dma engine as the H3 (sun8i), with a
>> reduced amount of physical channels. Add the proper config data
>> and compatible string to support it.
>>
>> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
>> ---
>>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 4 ++++
>>  drivers/dma/sun6i-dma.c                       | 7 +++++++
>>  2 files changed, 11 insertions(+)
>
> With what device did you test this? As far as I know, the SPI driver
> doesn't use DMA at all.
>
Hi Maxime,
I've tested this on spdif and i2s on the Pine64.
BR,
CK
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
Andre Przywara Sept. 1, 2017, 12:31 a.m. UTC | #4
Hi,

On 31/08/17 00:36, Stefan Brüns wrote:
> The A64 SoC has the same dma engine as the H3 (sun8i), with a
> reduced amount of physical channels. Add the proper config data
> and compatible string to support it.

...

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 5f4eee4513e5..6a17c5d63582 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
>  	.nr_max_vchans   = 34,
>  	.dmac_variant    = DMAC_VARIANT_H3,
>  };
> +
> +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
> +	.nr_max_channels = 8,
> +	.nr_max_requests = 27,
> +	.nr_max_vchans   = 38,
> +	.dmac_variant    = DMAC_VARIANT_H3,
>  };
>  
>  static const struct of_device_id sun6i_dma_match[] = {
> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = {
>  	{ .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,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, sun6i_dma_match);

I was wondering if should use the opportunity to expose those values as
DT properties instead of hard-wiring them to a compatible string in the
driver every time we add support for a new SoC?
We could introduce a new compatible string (say: "allwinner,sunxi-dma"),
then describe properties for the number of channels and requests and
vchans and parse those from the DT at probe time.
With this we might be able to support future SoCs without Linux *driver*
changes, by just providing the right DT. This would have worked already
for instance for the A83T support, which just changed those values.

For instance with this quick patch below (just compile tested, and without
your refactoring).
The DT node would then read something like:
	dma: dma-controller@01c02000 {
		compatible = "allwinner,sun50i-a64-dma",
			     "allwinner,sunxi-dma";
		reg = <0x01c02000 0x1000>;
		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
		clocks = <&ccu CLK_BUS_DMA>;
		resets = <&ccu RST_BUS_DMA>;
		#dma-cells = <1>;
		allwinner,max_channels = <8>;
		allwinner,max_requests = <27>;
		allwinner,max_vchans = <38>;
	};

Cheers,
Andre.

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index a2358780ab2c..5ae8032f2065 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -1033,6 +1033,7 @@ static const struct of_device_id sun6i_dma_match[] = {
 	{ .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,sunxi-dma", .data = NULL },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, sun6i_dma_match);
@@ -1051,7 +1052,43 @@ static int sun6i_dma_probe(struct platform_device *pdev)
 	device = of_match_device(sun6i_dma_match, &pdev->dev);
 	if (!device)
 		return -ENODEV;
-	sdc->cfg = device->data;
+	if (!device->data) {
+		struct sun6i_dma_config *config;
+
+		config = devm_kmalloc(&pdev->dev, sizeof(*config), GFP_KERNEL);
+		if (!config)
+			return -ENOMEM;
+
+		ret = of_property_read_u32(pdev->dev.of_node,
+					   "allwinner,max_channels",
+					   &config->nr_max_channels);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"missing allwinner,max_channels property\n");
+			return ret;
+		}
+
+		ret = of_property_read_u32(pdev->dev.of_node,
+					   "allwinner,max_requests",
+					   &config->nr_max_requests);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"missing allwinner,max_requests property\n");
+			return ret;
+		}
+
+		ret = of_property_read_u32(pdev->dev.of_node,
+					   "allwinner,max_vchans",
+					   &config->nr_max_vchans);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"missing allwinner,max_vchans property\n");
+			return ret;
+		}
+		sdc->cfg = config;
+	} else {
+		sdc->cfg = device->data;
+	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	sdc->base = devm_ioremap_resource(&pdev->dev, res);
Stefan Brüns Sept. 1, 2017, 1:19 a.m. UTC | #5
On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote:
> Hi,
> 
> On 31/08/17 00:36, Stefan Brüns wrote:
> > The A64 SoC has the same dma engine as the H3 (sun8i), with a
> > reduced amount of physical channels. Add the proper config data
> > and compatible string to support it.
> 
> ...
> 
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index 5f4eee4513e5..6a17c5d63582 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
> > 
> >  	.nr_max_vchans   = 34,
> >  	.dmac_variant    = DMAC_VARIANT_H3,
> >  
> >  };
> > 
> > +
> > +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
> > +	.nr_max_channels = 8,
> > +	.nr_max_requests = 27,
> > +	.nr_max_vchans   = 38,
> > +	.dmac_variant    = DMAC_VARIANT_H3,
> > 
> >  };
> >  
> >  static const struct of_device_id sun6i_dma_match[] = {
> > 
> > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] =
> > {> 
> >  	{ .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,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg
> > },> 
> >  	{ /* sentinel */ }
> >  
> >  };
> >  MODULE_DEVICE_TABLE(of, sun6i_dma_match);
> 
> I was wondering if should use the opportunity to expose those values as
> DT properties instead of hard-wiring them to a compatible string in the
> driver every time we add support for a new SoC?
> We could introduce a new compatible string (say: "allwinner,sunxi-dma"),
> then describe properties for the number of channels and requests and
> vchans and parse those from the DT at probe time.
> With this we might be able to support future SoCs without Linux *driver*
> changes, by just providing the right DT. This would have worked already
> for instance for the A83T support, which just changed those values.
> 
> For instance with this quick patch below (just compile tested, and without
> your refactoring).
> The DT node would then read something like:
> 	dma: dma-controller@01c02000 {
> 		compatible = "allwinner,sun50i-a64-dma",
> 			     "allwinner,sunxi-dma";
> 		reg = <0x01c02000 0x1000>;
> 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
> 		clocks = <&ccu CLK_BUS_DMA>;
> 		resets = <&ccu RST_BUS_DMA>;
> 		#dma-cells = <1>;
> 		allwinner,max_channels = <8>;
> 		allwinner,max_requests = <27>;
> 		allwinner,max_vchans = <38>;
> 	};

For these 3 properties it likely is a good idea, but we would IMHO still have 
to care for the differences in the register settings:

- A31 does not have a clock autogating register
- A23 and A83t does have one at offset 0x20
- A64, H3, H5 and R40 have it at offset 0x28

There are also the incompatibilities in the "DMA channel configuration 
register" (burst length; burst width; burst length field offset).

We can either have 3 different compatible strings, or another property for the 
register model.

For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction is a 
better option - it can encode the allowed DRQ numbers much better (e.g. for 
H3, the highest source DRQ is 24). The DRQ field in the channel configuration 
register is 5 bits, so the hightest port/DRQ number is 31.

For aw,max_channels my first thought is - why max? is it variable? is there a 
min_channels? My suggestion would be (in order of preference): "aw,channels", 
"aw,dma_channels", "aw,available_channels".

Kind regards,

Stefan
Maxime Ripard Sept. 1, 2017, 6:04 a.m. UTC | #6
On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote:
> Hi,
> 
> On 31/08/17 00:36, Stefan Brüns wrote:
> > The A64 SoC has the same dma engine as the H3 (sun8i), with a
> > reduced amount of physical channels. Add the proper config data
> > and compatible string to support it.
> 
> ...
> 
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index 5f4eee4513e5..6a17c5d63582 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
> >  	.nr_max_vchans   = 34,
> >  	.dmac_variant    = DMAC_VARIANT_H3,
> >  };
> > +
> > +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
> > +	.nr_max_channels = 8,
> > +	.nr_max_requests = 27,
> > +	.nr_max_vchans   = 38,
> > +	.dmac_variant    = DMAC_VARIANT_H3,
> >  };
> >  
> >  static const struct of_device_id sun6i_dma_match[] = {
> > @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = {
> >  	{ .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,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg },
> >  	{ /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, sun6i_dma_match);
> 
> I was wondering if should use the opportunity to expose those values as
> DT properties instead of hard-wiring them to a compatible string in the
> driver every time we add support for a new SoC?
> We could introduce a new compatible string (say: "allwinner,sunxi-dma"),
> then describe properties for the number of channels and requests and
> vchans and parse those from the DT at probe time.
> With this we might be able to support future SoCs without Linux *driver*
> changes, by just providing the right DT. This would have worked already
> for instance for the A83T support, which just changed those values.
> 
> For instance with this quick patch below (just compile tested, and without
> your refactoring).
> The DT node would then read something like:
> 	dma: dma-controller@01c02000 {
> 		compatible = "allwinner,sun50i-a64-dma",
> 			     "allwinner,sunxi-dma";
> 		reg = <0x01c02000 0x1000>;
> 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
> 		clocks = <&ccu CLK_BUS_DMA>;
> 		resets = <&ccu RST_BUS_DMA>;
> 		#dma-cells = <1>;
> 		allwinner,max_channels = <8>;
> 		allwinner,max_requests = <27>;
> 		allwinner,max_vchans = <38>;
> 	};

We're still going to need a different compatible anyway, so it's not
really like it would change anything.

Maxime
Andre Przywara Sept. 1, 2017, 10:32 p.m. UTC | #7
Hi,

On 01/09/17 02:19, Stefan Bruens wrote:
> On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote:
>> Hi,
>>
>> On 31/08/17 00:36, Stefan Brüns wrote:
>>> The A64 SoC has the same dma engine as the H3 (sun8i), with a
>>> reduced amount of physical channels. Add the proper config data
>>> and compatible string to support it.
>>
>> ...
>>
>>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
>>> index 5f4eee4513e5..6a17c5d63582 100644
>>> --- a/drivers/dma/sun6i-dma.c
>>> +++ b/drivers/dma/sun6i-dma.c
>>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
>>>
>>>  	.nr_max_vchans   = 34,
>>>  	.dmac_variant    = DMAC_VARIANT_H3,
>>>  
>>>  };
>>>
>>> +
>>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
>>> +	.nr_max_channels = 8,
>>> +	.nr_max_requests = 27,
>>> +	.nr_max_vchans   = 38,
>>> +	.dmac_variant    = DMAC_VARIANT_H3,
>>>
>>>  };
>>>  
>>>  static const struct of_device_id sun6i_dma_match[] = {
>>>
>>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] =
>>> {> 
>>>  	{ .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,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg
>>> },> 
>>>  	{ /* sentinel */ }
>>>  
>>>  };
>>>  MODULE_DEVICE_TABLE(of, sun6i_dma_match);
>>
>> I was wondering if should use the opportunity to expose those values as
>> DT properties instead of hard-wiring them to a compatible string in the
>> driver every time we add support for a new SoC?
>> We could introduce a new compatible string (say: "allwinner,sunxi-dma"),
>> then describe properties for the number of channels and requests and
>> vchans and parse those from the DT at probe time.
>> With this we might be able to support future SoCs without Linux *driver*
>> changes, by just providing the right DT. This would have worked already
>> for instance for the A83T support, which just changed those values.
>>
>> For instance with this quick patch below (just compile tested, and without
>> your refactoring).
>> The DT node would then read something like:
>> 	dma: dma-controller@01c02000 {
>> 		compatible = "allwinner,sun50i-a64-dma",
>> 			     "allwinner,sunxi-dma";
>> 		reg = <0x01c02000 0x1000>;
>> 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
>> 		clocks = <&ccu CLK_BUS_DMA>;
>> 		resets = <&ccu RST_BUS_DMA>;
>> 		#dma-cells = <1>;
>> 		allwinner,max_channels = <8>;
>> 		allwinner,max_requests = <27>;
>> 		allwinner,max_vchans = <38>;
>> 	};
> 
> For these 3 properties it likely is a good idea, but we would IMHO still have 
> to care for the differences in the register settings:
> 
> - A31 does not have a clock autogating register
> - A23 and A83t does have one at offset 0x20
> - A64, H3, H5 and R40 have it at offset 0x28

Fair enough - I didn't look too closely at your new changes, especially
why it apparently worked before.
But as your list shows, we would only need one compatible string per
line - in the driver - to cover all SoCs (and possibly future SoCs!),
and do the rest via the properties.
We can't use the existing h3 compatible string or touch the already
existing SoCs and compatible strings, of course, but I guess
the A64, R40 and future SoCs could make use of a new (generic?) string.

> There are also the incompatibilities in the "DMA channel configuration 
> register" (burst length; burst width; burst length field offset).
> 
> We can either have 3 different compatible strings, or another property for the 
> register model.

The latter is usually frowned upon, using separate compatible strings
for each group of SoCs is the way to go here.

> For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction is a 
> better option - it can encode the allowed DRQ numbers much better (e.g. for 
> H3, the highest source DRQ is 24). The DRQ field in the channel configuration 
> register is 5 bits, so the hightest port/DRQ number is 31.

So looking more closely at the manual and the code my understanding is
that nr_max_requests is more or less some rough molly guard to prevent
wrong settings? Derived from the DRQ table in the manual?
So that trying to program port 28 on an H3 would fail?
But source port 25 or dest port 26 wouldn't be caught by this check,
though they would still be "illegal" according to the manual. (Which we
are not sure of, I guess, it may just not be documented)
So I wonder if this nr_max_requests is useful at all, and we should just
check that it fits into 5 bits and assume that the DT has superior
knowledge of what's allowed and what not.
Now I see what you mean with the bitmask (to cover those gaps), but I am
bit sceptical if that is actually useful. After all the DRQ number would
be coming from the DT, which we can surely trust.

And nr_max_vchans seems to describe the sum of documented DRQs, to just
limit the memory allocation? So this could become just 64 to cover all
possible cases without SoC specific configuration at all?

> For aw,max_channels my first thought is - why max? is it variable? is there a 
> min_channels? My suggestion would be (in order of preference): "aw,channels", 
> "aw,dma_channels", "aw,available_channels".

Sure, actually looking at Documentation/devicetree/bindings/dma/dma.txt
I think we can even use the generic "dma-channels" property described
there. And possibly the same with "dma-requests", should we keep this.

So summarizing this:
- We create a new compatible string, which drives an H3 compatible DMA
(clock autogating at 0x28, 64-bit data width capable). This name could
either be generic, or actually "allwinner,sun50i-a64-dma".
- This one sets nr_max_requests to 31 and nr_max_vchans to 64.
- Alternatively we expose those values in the DT as properties.
- We take the number of DMA channels from the (now required)
"dma-channels" property.
- We let the A64 (and R40?) use this new binding.
- Any future SoC which is close enough can then just piggy-back on this.
- Any future *changes* in the Allwinner DMA device which require driver
changes create a new compatible string, but still keep the above
semantics. Chances are that there are more than one SoC using this kind
of new DMA device, so they would work out of the box.

Does that make sense?
I am happy to provide the code for that, based on your H3 rework.

Cheers,
Andre.
Andre Przywara Sept. 1, 2017, 10:35 p.m. UTC | #8
On 01/09/17 07:04, Maxime Ripard wrote:
> On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote:
>> Hi,
>>
>> On 31/08/17 00:36, Stefan Brüns wrote:
>>> The A64 SoC has the same dma engine as the H3 (sun8i), with a
>>> reduced amount of physical channels. Add the proper config data
>>> and compatible string to support it.
>>
>> ...
>>
>>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
>>> index 5f4eee4513e5..6a17c5d63582 100644
>>> --- a/drivers/dma/sun6i-dma.c
>>> +++ b/drivers/dma/sun6i-dma.c
>>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
>>>  	.nr_max_vchans   = 34,
>>>  	.dmac_variant    = DMAC_VARIANT_H3,
>>>  };
>>> +
>>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
>>> +	.nr_max_channels = 8,
>>> +	.nr_max_requests = 27,
>>> +	.nr_max_vchans   = 38,
>>> +	.dmac_variant    = DMAC_VARIANT_H3,
>>>  };
>>>  
>>>  static const struct of_device_id sun6i_dma_match[] = {
>>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = {
>>>  	{ .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,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg },
>>>  	{ /* sentinel */ }
>>>  };
>>>  MODULE_DEVICE_TABLE(of, sun6i_dma_match);
>>
>> I was wondering if should use the opportunity to expose those values as
>> DT properties instead of hard-wiring them to a compatible string in the
>> driver every time we add support for a new SoC?
>> We could introduce a new compatible string (say: "allwinner,sunxi-dma"),
>> then describe properties for the number of channels and requests and
>> vchans and parse those from the DT at probe time.
>> With this we might be able to support future SoCs without Linux *driver*
>> changes, by just providing the right DT. This would have worked already
>> for instance for the A83T support, which just changed those values.
>>
>> For instance with this quick patch below (just compile tested, and without
>> your refactoring).
>> The DT node would then read something like:
>> 	dma: dma-controller@01c02000 {
>> 		compatible = "allwinner,sun50i-a64-dma",
>> 			     "allwinner,sunxi-dma";
>> 		reg = <0x01c02000 0x1000>;
>> 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
>> 		clocks = <&ccu CLK_BUS_DMA>;
>> 		resets = <&ccu RST_BUS_DMA>;
>> 		#dma-cells = <1>;
>> 		allwinner,max_channels = <8>;
>> 		allwinner,max_requests = <27>;
>> 		allwinner,max_vchans = <38>;
>> 	};
> 
> We're still going to need a different compatible anyway, so it's not
> really like it would change anything.

Well, not for now, but possibly in the future. And we should start with
this at one point. If we would have had this type of binding already for
H3, we could have added the A64 support without driver changes just by a
DT change.

Cheers,
Andre.
Stefan Brüns Sept. 2, 2017, 12:38 a.m. UTC | #9
On Samstag, 2. September 2017 00:32:50 CEST André Przywara wrote:
> Hi,
> 
> On 01/09/17 02:19, Stefan Bruens wrote:
> > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote:
> >> Hi,
> >> 
> >> On 31/08/17 00:36, Stefan Brüns wrote:
[...]
> > 
> > For these 3 properties it likely is a good idea, but we would IMHO still
> > have to care for the differences in the register settings:
> > 
> > - A31 does not have a clock autogating register
> > - A23 and A83t does have one at offset 0x20
> > - A64, H3, H5 and R40 have it at offset 0x28
> 
> Fair enough - I didn't look too closely at your new changes, especially
> why it apparently worked before.
> But as your list shows, we would only need one compatible string per
> line - in the driver - to cover all SoCs (and possibly future SoCs!),
> and do the rest via the properties.
> We can't use the existing h3 compatible string or touch the already
> existing SoCs and compatible strings, of course, but I guess
> the A64, R40 and future SoCs could make use of a new (generic?) string.
> 
> > There are also the incompatibilities in the "DMA channel configuration
> > register" (burst length; burst width; burst length field offset).
> > 
> > We can either have 3 different compatible strings, or another property for
> > the register model.
> 
> The latter is usually frowned upon, using separate compatible strings
> for each group of SoCs is the way to go here.
> 
> > For the aw,max_requests and aw,max_vchans, maybe a bitmask per direction
> > is a better option - it can encode the allowed DRQ numbers much better
> > (e.g. for H3, the highest source DRQ is 24). The DRQ field in the channel
> > configuration register is 5 bits, so the hightest port/DRQ number is 31.
> 
> So looking more closely at the manual and the code my understanding is
> that nr_max_requests is more or less some rough molly guard to prevent
> wrong settings? Derived from the DRQ table in the manual?
> So that trying to program port 28 on an H3 would fail?
> But source port 25 or dest port 26 wouldn't be caught by this check,
> though they would still be "illegal" according to the manual. (Which we
> are not sure of, I guess, it may just not be documented)
> So I wonder if this nr_max_requests is useful at all, and we should just
> check that it fits into 5 bits and assume that the DT has superior
> knowledge of what's allowed and what not.
> Now I see what you mean with the bitmask (to cover those gaps), but I am
> bit sceptical if that is actually useful. After all the DRQ number would
> be coming from the DT, which we can surely trust.
> 
> And nr_max_vchans seems to describe the sum of documented DRQs, to just
> limit the memory allocation? So this could become just 64 to cover all
> possible cases without SoC specific configuration at all?

Yes, thats my understanding as well. Allocating a few excess channels would 
waste a few kBytes (AFAICS 304 bytes per channel on 64bit).
 
> > For aw,max_channels my first thought is - why max? is it variable? is
> > there a min_channels? My suggestion would be (in order of preference):
> > "aw,channels", "aw,dma_channels", "aw,available_channels".
> 
> Sure, actually looking at Documentation/devicetree/bindings/dma/dma.txt
> I think we can even use the generic "dma-channels" property described
> there. And possibly the same with "dma-requests", should we keep this.
> 
> So summarizing this:
> - We create a new compatible string, which drives an H3 compatible DMA
> (clock autogating at 0x28, 64-bit data width capable). This name could
> either be generic, or actually "allwinner,sun50i-a64-dma".
> - This one sets nr_max_requests to 31 and nr_max_vchans to 64.
> - Alternatively we expose those values in the DT as properties.
> - We take the number of DMA channels from the (now required)
> "dma-channels" property.
> - We let the A64 (and R40?) use this new binding.
> - Any future SoC which is close enough can then just piggy-back on this.
> - Any future *changes* in the Allwinner DMA device which require driver
> changes create a new compatible string, but still keep the above
> semantics. Chances are that there are more than one SoC using this kind
> of new DMA device, so they would work out of the box.
> 
> Does that make sense?
> I am happy to provide the code for that, based on your H3 rework.

Sounds good for me. I will send a cleaned up series later.

Kind regards,

Stefan
Stefan Brüns Sept. 2, 2017, 2:02 a.m. UTC | #10
On Samstag, 2. September 2017 00:32:50 CEST André Przywara wrote:
> Hi,
> 
> On 01/09/17 02:19, Stefan Bruens wrote:
> > On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote:
> >> Hi,
> >> 
> >> On 31/08/17 00:36, Stefan Brüns wrote:
> >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a
> >>> reduced amount of physical channels. Add the proper config data
> >>> and compatible string to support it.
> >> 
> >> ...
> >> 
> >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> >>> index 5f4eee4513e5..6a17c5d63582 100644
> >>> --- a/drivers/dma/sun6i-dma.c
> >>> +++ b/drivers/dma/sun6i-dma.c
> >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg =
> >>> {
> >>> 
> >>>  	.nr_max_vchans   = 34,
> >>>  	.dmac_variant    = DMAC_VARIANT_H3,
> >>>  
> >>>  };
> >>> 
> >>> +
> >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
> >>> +	.nr_max_channels = 8,
> >>> +	.nr_max_requests = 27,
> >>> +	.nr_max_vchans   = 38,
> >>> +	.dmac_variant    = DMAC_VARIANT_H3,
> >>> 
> >>>  };
> >>>  
[...]
> > There are also the incompatibilities in the "DMA channel configuration
> > register" (burst length; burst width; burst length field offset).
> > 
> > We can either have 3 different compatible strings, or another property for
> > the register model.
> 
> The latter is usually frowned upon, using separate compatible strings
> for each group of SoCs is the way to go here.

Just for clarification, I was not talking about a property in the devicetree, 
but about a struct member in the config data, i.e. the .dmac_variant above.

Kind regards,

Stefan
Andre Przywara Sept. 3, 2017, 11:14 p.m. UTC | #11
On 02/09/17 03:02, Stefan Bruens wrote:
> On Samstag, 2. September 2017 00:32:50 CEST André Przywara wrote:
>> Hi,
>>
>> On 01/09/17 02:19, Stefan Bruens wrote:
>>> On Freitag, 1. September 2017 02:31:35 CEST Andre Przywara wrote:
>>>> Hi,
>>>>
>>>> On 31/08/17 00:36, Stefan Brüns wrote:
>>>>> The A64 SoC has the same dma engine as the H3 (sun8i), with a
>>>>> reduced amount of physical channels. Add the proper config data
>>>>> and compatible string to support it.
>>>>
>>>> ...
>>>>
>>>>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
>>>>> index 5f4eee4513e5..6a17c5d63582 100644
>>>>> --- a/drivers/dma/sun6i-dma.c
>>>>> +++ b/drivers/dma/sun6i-dma.c
>>>>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg =
>>>>> {
>>>>>
>>>>>  	.nr_max_vchans   = 34,
>>>>>  	.dmac_variant    = DMAC_VARIANT_H3,
>>>>>  
>>>>>  };
>>>>>
>>>>> +
>>>>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
>>>>> +	.nr_max_channels = 8,
>>>>> +	.nr_max_requests = 27,
>>>>> +	.nr_max_vchans   = 38,
>>>>> +	.dmac_variant    = DMAC_VARIANT_H3,
>>>>>
>>>>>  };
>>>>>  
> [...]
>>> There are also the incompatibilities in the "DMA channel configuration
>>> register" (burst length; burst width; burst length field offset).
>>>
>>> We can either have 3 different compatible strings, or another property for
>>> the register model.
>>
>> The latter is usually frowned upon, using separate compatible strings
>> for each group of SoCs is the way to go here.
> 
> Just for clarification, I was not talking about a property in the devicetree, 
> but about a struct member in the config data, i.e. the .dmac_variant above.

Ah, I see. I was indeed talking about device tree nodes.

So in this case I would lean towards mapping the actual properties to
member names in struct sun6i_dma_config, in this case something like:
	.auto_clock_gate = 0x28;
	.max_burst_width = 16;

This looks more flexible to me and avoids hard to read code where
property A is used in SoC X and Y, but property B in SoC X and Z, for
instance.
In the auto clock gate case this would result in an easy-to-read:
	writel(SUN8I_DMA_GATE_ENABLE,
	       sdc->base + sdc->cfg->auto_clock_gate);
(possibly guarded somehow to catch that A31 case)

Sorry for the delay in this answer, I see that you kept the
DMAC_VARIANT_ style for your new post, and the end result doesn't look
too bad. But maybe still changing this is not too hard now that you have
more fine grained patches?

Cheers,
Andre.
Maxime Ripard Sept. 4, 2017, 7:04 a.m. UTC | #12
On Fri, Sep 01, 2017 at 11:35:40PM +0100, André Przywara wrote:
> On 01/09/17 07:04, Maxime Ripard wrote:
> > On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote:
> >> Hi,
> >>
> >> On 31/08/17 00:36, Stefan Brüns wrote:
> >>> The A64 SoC has the same dma engine as the H3 (sun8i), with a
> >>> reduced amount of physical channels. Add the proper config data
> >>> and compatible string to support it.
> >>
> >> ...
> >>
> >>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> >>> index 5f4eee4513e5..6a17c5d63582 100644
> >>> --- a/drivers/dma/sun6i-dma.c
> >>> +++ b/drivers/dma/sun6i-dma.c
> >>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
> >>>  	.nr_max_vchans   = 34,
> >>>  	.dmac_variant    = DMAC_VARIANT_H3,
> >>>  };
> >>> +
> >>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
> >>> +	.nr_max_channels = 8,
> >>> +	.nr_max_requests = 27,
> >>> +	.nr_max_vchans   = 38,
> >>> +	.dmac_variant    = DMAC_VARIANT_H3,
> >>>  };
> >>>  
> >>>  static const struct of_device_id sun6i_dma_match[] = {
> >>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = {
> >>>  	{ .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,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg },
> >>>  	{ /* sentinel */ }
> >>>  };
> >>>  MODULE_DEVICE_TABLE(of, sun6i_dma_match);
> >>
> >> I was wondering if should use the opportunity to expose those values as
> >> DT properties instead of hard-wiring them to a compatible string in the
> >> driver every time we add support for a new SoC?
> >> We could introduce a new compatible string (say: "allwinner,sunxi-dma"),
> >> then describe properties for the number of channels and requests and
> >> vchans and parse those from the DT at probe time.
> >> With this we might be able to support future SoCs without Linux *driver*
> >> changes, by just providing the right DT. This would have worked already
> >> for instance for the A83T support, which just changed those values.
> >>
> >> For instance with this quick patch below (just compile tested, and without
> >> your refactoring).
> >> The DT node would then read something like:
> >> 	dma: dma-controller@01c02000 {
> >> 		compatible = "allwinner,sun50i-a64-dma",
> >> 			     "allwinner,sunxi-dma";
> >> 		reg = <0x01c02000 0x1000>;
> >> 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
> >> 		clocks = <&ccu CLK_BUS_DMA>;
> >> 		resets = <&ccu RST_BUS_DMA>;
> >> 		#dma-cells = <1>;
> >> 		allwinner,max_channels = <8>;
> >> 		allwinner,max_requests = <27>;
> >> 		allwinner,max_vchans = <38>;
> >> 	};
> > 
> > We're still going to need a different compatible anyway, so it's not
> > really like it would change anything.
> 
> Well, not for now, but possibly in the future. And we should start with
> this at one point. If we would have had this type of binding already for
> H3, we could have added the A64 support without driver changes just by a
> DT change.

That's not true. As usual with these kinds of generic binding
arguments (it's definitely not personal, you're far from the only one
making them), it completely ignores the fact that the IP itself might
change from one revision to another, and its behaviour might too.

The A64 for example moved at least one register off, and has a
different set of burst size / width.

It's something you can't account for when you initially define the
binding, unless you describe all the registers, plus all their
parameters and the way you interact with them, which isn't going to
happen if you want to keep your sanity.

And obviously, while maintaining the stability of the binding of those
hundreds properties.

Or, you can base all this on the compatible, and be done with it once
and for all.

Maxime
Andre Przywara Sept. 4, 2017, 8:14 a.m. UTC | #13
Salut,

On 04/09/17 08:04, Maxime Ripard wrote:
> On Fri, Sep 01, 2017 at 11:35:40PM +0100, André Przywara wrote:
>> On 01/09/17 07:04, Maxime Ripard wrote:
>>> On Fri, Sep 01, 2017 at 01:31:35AM +0100, Andre Przywara wrote:
>>>> Hi,
>>>>
>>>> On 31/08/17 00:36, Stefan Brüns wrote:
>>>>> The A64 SoC has the same dma engine as the H3 (sun8i), with a
>>>>> reduced amount of physical channels. Add the proper config data
>>>>> and compatible string to support it.
>>>>
>>>> ...
>>>>
>>>>> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
>>>>> index 5f4eee4513e5..6a17c5d63582 100644
>>>>> --- a/drivers/dma/sun6i-dma.c
>>>>> +++ b/drivers/dma/sun6i-dma.c
>>>>> @@ -1068,6 +1068,12 @@ static struct sun6i_dma_config sun8i_h3_dma_cfg = {
>>>>>  	.nr_max_vchans   = 34,
>>>>>  	.dmac_variant    = DMAC_VARIANT_H3,
>>>>>  };
>>>>> +
>>>>> +static struct sun6i_dma_config sun50i_a64_dma_cfg = {
>>>>> +	.nr_max_channels = 8,
>>>>> +	.nr_max_requests = 27,
>>>>> +	.nr_max_vchans   = 38,
>>>>> +	.dmac_variant    = DMAC_VARIANT_H3,
>>>>>  };
>>>>>  
>>>>>  static const struct of_device_id sun6i_dma_match[] = {
>>>>> @@ -1075,6 +1081,7 @@ static const struct of_device_id sun6i_dma_match[] = {
>>>>>  	{ .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,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg },
>>>>>  	{ /* sentinel */ }
>>>>>  };
>>>>>  MODULE_DEVICE_TABLE(of, sun6i_dma_match);
>>>>
>>>> I was wondering if should use the opportunity to expose those values as
>>>> DT properties instead of hard-wiring them to a compatible string in the
>>>> driver every time we add support for a new SoC?
>>>> We could introduce a new compatible string (say: "allwinner,sunxi-dma"),
>>>> then describe properties for the number of channels and requests and
>>>> vchans and parse those from the DT at probe time.
>>>> With this we might be able to support future SoCs without Linux *driver*
>>>> changes, by just providing the right DT. This would have worked already
>>>> for instance for the A83T support, which just changed those values.
>>>>
>>>> For instance with this quick patch below (just compile tested, and without
>>>> your refactoring).
>>>> The DT node would then read something like:
>>>> 	dma: dma-controller@01c02000 {
>>>> 		compatible = "allwinner,sun50i-a64-dma",
>>>> 			     "allwinner,sunxi-dma";
>>>> 		reg = <0x01c02000 0x1000>;
>>>> 		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
>>>> 		clocks = <&ccu CLK_BUS_DMA>;
>>>> 		resets = <&ccu RST_BUS_DMA>;
>>>> 		#dma-cells = <1>;
>>>> 		allwinner,max_channels = <8>;
>>>> 		allwinner,max_requests = <27>;
>>>> 		allwinner,max_vchans = <38>;
>>>> 	};
>>>
>>> We're still going to need a different compatible anyway, so it's not
>>> really like it would change anything.
>>
>> Well, not for now, but possibly in the future. And we should start with
>> this at one point. If we would have had this type of binding already for
>> H3, we could have added the A64 support without driver changes just by a
>> DT change.
> 
> That's not true. As usual with these kinds of generic binding
> arguments (it's definitely not personal, you're far from the only one
> making them), it completely ignores the fact that the IP itself might
> change from one revision to another, and its behaviour might too.

Not arguing that, and I totally see that those cases indeed require a
new compatible string.

> The A64 for example moved at least one register off, and has a
> different set of burst size / width.

But that is only compared to the original A23/A31 model? AFAICT compared
to the H3 DMA it's really only the number of supported DMA channels that
has changed. For which we don't even need to invent a property name.
My point was that as the driver is *at the moment* all those different
compatible strings behaved the same (apart from the missing A23 clock
gating), see commits f008db8c00c1 and 3a03ea763a67. The differences we
coded in struct sun6i_dma_config were more or less artificial.
Now thanks to Stefan we learned that the H3 is more different than we
thought, so this argument doesn't hold anymore (but see below).

> It's something you can't account for when you initially define the
> binding, unless you describe all the registers, plus all their
> parameters and the way you interact with them, which isn't going to
> happen if you want to keep your sanity.

I agree on that.

> And obviously, while maintaining the stability of the binding of those
> hundreds properties.
> 
> Or, you can base all this on the compatible, and be done with it once
> and for all.

What I am after is to cover SoCs which *don't* have differences in their
register layout, for instance A83T, H3, A64, R40.
In an ideal world we could have reused the H3 compatible string,
adjusting the number of channels for each SoC in the DT.

So I see that having a generic compatible name will not fly, as we now
have differences which should not be modelled by DT properties.
But I still think we should try to cover those non-register differences
(number of channels) with a DT property, to allow reusing the existing
driver code whenever possible. As is stands with this series, the R40
support should just be a matter of:
	compatible = "allwinner,sun8i-r40-dma",
		     "allwinner,sun50i-a64-dma";

I am aware that future SoCs might require new compatibles (I actually
hope that they introduce 64-bit memory addresses at some point), but
having the "dma-channels" DT parsing code in should then reduce the
frequency of driver changes (for SoCs just copying existing DMA IP).

Cheers,
Andre.
Maxime Ripard Sept. 8, 2017, 2:39 p.m. UTC | #14
Hi,

On Mon, Sep 04, 2017 at 09:14:52AM +0100, André Przywara wrote:
> > And obviously, while maintaining the stability of the binding of those
> > hundreds properties.
> > 
> > Or, you can base all this on the compatible, and be done with it once
> > and for all.
> 
> What I am after is to cover SoCs which *don't* have differences in their
> register layout, for instance A83T, H3, A64, R40.
> In an ideal world we could have reused the H3 compatible string,
> adjusting the number of channels for each SoC in the DT.
> 
> So I see that having a generic compatible name will not fly, as we now
> have differences which should not be modelled by DT properties.
> But I still think we should try to cover those non-register differences
> (number of channels) with a DT property, to allow reusing the existing
> driver code whenever possible. As is stands with this series, the R40
> support should just be a matter of:
> 	compatible = "allwinner,sun8i-r40-dma",
> 		     "allwinner,sun50i-a64-dma";

I just suggested the exact same thing, and then saw your mail, so I
guess we have an agreement :)

Maxime
Andre Przywara Sept. 8, 2017, 2:57 p.m. UTC | #15
Hi Maxime,

On 08/09/17 15:39, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Sep 04, 2017 at 09:14:52AM +0100, André Przywara wrote:
>>> And obviously, while maintaining the stability of the binding of those
>>> hundreds properties.
>>>
>>> Or, you can base all this on the compatible, and be done with it once
>>> and for all.
>>
>> What I am after is to cover SoCs which *don't* have differences in their
>> register layout, for instance A83T, H3, A64, R40.
>> In an ideal world we could have reused the H3 compatible string,
>> adjusting the number of channels for each SoC in the DT.
>>
>> So I see that having a generic compatible name will not fly, as we now
>> have differences which should not be modelled by DT properties.
>> But I still think we should try to cover those non-register differences
>> (number of channels) with a DT property, to allow reusing the existing
>> driver code whenever possible. As is stands with this series, the R40
>> support should just be a matter of:
>> 	compatible = "allwinner,sun8i-r40-dma",
>> 		     "allwinner,sun50i-a64-dma";
> 
> I just suggested the exact same thing, and then saw your mail, so I
> guess we have an agreement :)

Yes, I was thinking so as well.
Since my DeLorean is in the garage ;-) we have no other choice than
doing so.
My original suggestion for a generic name was based on my naive reading
of the existing code, which *looked like* it would be all compatible.
But as we know better now, this is the way to go.

Merci,
André
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index f96287d3043a..b86019238b77 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -494,6 +494,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>;
@@ -509,6 +511,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>;
diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 5f4eee4513e5..6a17c5d63582 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -1068,6 +1068,12 @@  static struct sun6i_dma_config sun8i_h3_dma_cfg = {
 	.nr_max_vchans   = 34,
 	.dmac_variant    = DMAC_VARIANT_H3,
 };
+
+static struct sun6i_dma_config sun50i_a64_dma_cfg = {
+	.nr_max_channels = 8,
+	.nr_max_requests = 27,
+	.nr_max_vchans   = 38,
+	.dmac_variant    = DMAC_VARIANT_H3,
 };
 
 static const struct of_device_id sun6i_dma_match[] = {
@@ -1075,6 +1081,7 @@  static const struct of_device_id sun6i_dma_match[] = {
 	{ .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,sun50i-a64-dma", .data = &sun50i_a64_dma_cfg },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, sun6i_dma_match);