[PATCH 00/14] dma-jz4780 improvements

[PATCH 00/14] dma-jz4780 improvements

[Paul Cercueil]

Hi,

This set of patches by myself and Daniel extends the dma-jz4780 driver
to support other SoCs (JZ4770, JZ4740, JZ4725B).

Some fixes are also included, for proper residue reporting, which fixes
errors with ALSA.

Finally, the last two patches update the devicetree bindings for the
JZ4780 SoC and add a binding for the JZ4770 SoC.

This patchset was tested on JZ4780, JZ4770 and JZ4725B, but was not tested
on the JZ4740, so it would be fantastic if somebody could test it there.

Regards,
-Paul

[PATCH 01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels

[Paul Cercueil]

As part of the work to support various other Ingenic JZ47xx SoC versions,
which don't feature the same number of DMA channels per core, we now
deduce the number of DMA channels available from the devicetree
compatible string.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/dma/dma-jz4780.c | 53 +++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 85820a2d69d4..b40f491f0367 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -16,6 +16,7 @@
 #include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/of_dma.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -23,8 +24,6 @@
 #include "dmaengine.h"
 #include "virt-dma.h"
 
-#define JZ_DMA_NR_CHANNELS	32
-
 /* Global registers. */
 #define JZ_DMA_REG_DMAC		0x1000
 #define JZ_DMA_REG_DIRQP	0x1004
@@ -135,14 +134,20 @@ struct jz4780_dma_chan {
 	unsigned int curr_hwdesc;
 };
 
+enum jz_version {
+	ID_JZ4780,
+};
+
 struct jz4780_dma_dev {
 	struct dma_device dma_device;
 	void __iomem *base;
 	struct clk *clk;
 	unsigned int irq;
+	unsigned int nb_channels;
+	enum jz_version version;
 
 	uint32_t chan_reserved;
-	struct jz4780_dma_chan chan[JZ_DMA_NR_CHANNELS];
+	struct jz4780_dma_chan chan[];
 };
 
 struct jz4780_dma_filter_data {
@@ -648,7 +653,7 @@ static irqreturn_t jz4780_dma_irq_handler(int irq, void *data)
 
 	pending = jz4780_dma_readl(jzdma, JZ_DMA_REG_DIRQP);
 
-	for (i = 0; i < JZ_DMA_NR_CHANNELS; i++) {
+	for (i = 0; i < jzdma->nb_channels; i++) {
 		if (!(pending & (1<<i)))
 			continue;
 
@@ -728,7 +733,7 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
 	data.channel = dma_spec->args[1];
 
 	if (data.channel > -1) {
-		if (data.channel >= JZ_DMA_NR_CHANNELS) {
+		if (data.channel >= jzdma->nb_channels) {
 			dev_err(jzdma->dma_device.dev,
 				"device requested non-existent channel %u\n",
 				data.channel);
@@ -752,19 +757,45 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
 	}
 }
 
+static const unsigned int jz4780_dma_nb_channels[] = {
+	[ID_JZ4780] = 32,
+};
+
+static const struct of_device_id jz4780_dma_dt_match[] = {
+	{ .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
+	{},
+};
+MODULE_DEVICE_TABLE(of, jz4780_dma_dt_match);
+
 static int jz4780_dma_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	const struct of_device_id *of_id = of_match_device(
+			jz4780_dma_dt_match, dev);
 	struct jz4780_dma_dev *jzdma;
 	struct jz4780_dma_chan *jzchan;
 	struct dma_device *dd;
 	struct resource *res;
+	enum jz_version version;
+	unsigned int nb_channels;
 	int i, ret;
 
-	jzdma = devm_kzalloc(dev, sizeof(*jzdma), GFP_KERNEL);
+	if (of_id)
+		version = (enum jz_version)of_id->data;
+	else
+		version = ID_JZ4780; /* Default when not probed from DT */
+
+	nb_channels = jz4780_dma_nb_channels[version];
+
+	jzdma = devm_kzalloc(dev, sizeof(*jzdma)
+				+ sizeof(*jzdma->chan) * nb_channels,
+				GFP_KERNEL);
 	if (!jzdma)
 		return -ENOMEM;
 
+	jzdma->nb_channels = nb_channels;
+	jzdma->version = version;
+
 	platform_set_drvdata(pdev, jzdma);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -839,7 +870,7 @@ static int jz4780_dma_probe(struct platform_device *pdev)
 
 	INIT_LIST_HEAD(&dd->channels);
 
-	for (i = 0; i < JZ_DMA_NR_CHANNELS; i++) {
+	for (i = 0; i < jzdma->nb_channels; i++) {
 		jzchan = &jzdma->chan[i];
 		jzchan->id = i;
 
@@ -884,19 +915,13 @@ static int jz4780_dma_remove(struct platform_device *pdev)
 
 	free_irq(jzdma->irq, jzdma);
 
-	for (i = 0; i < JZ_DMA_NR_CHANNELS; i++)
+	for (i = 0; i < jzdma->nb_channels; i++)
 		tasklet_kill(&jzdma->chan[i].vchan.task);
 
 	dma_async_device_unregister(&jzdma->dma_device);
 	return 0;
 }
 
-static const struct of_device_id jz4780_dma_dt_match[] = {
-	{ .compatible = "ingenic,jz4780-dma", .data = NULL },
-	{},
-};
-MODULE_DEVICE_TABLE(of, jz4780_dma_dt_match);
-
 static struct platform_driver jz4780_dma_driver = {
 	.probe		= jz4780_dma_probe,
 	.remove		= jz4780_dma_remove,
-- 
2.18.0

Re: [PATCH 01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels

[PrasannaKumar Muralidharan]

Hi Paul,

On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote:
> As part of the work to support various other Ingenic JZ47xx SoC versions,
> which don't feature the same number of DMA channels per core, we now
> deduce the number of DMA channels available from the devicetree
> compatible string.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/dma/dma-jz4780.c | 53 +++++++++++++++++++++++++++++-----------
>  1 file changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 85820a2d69d4..b40f491f0367 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -16,6 +16,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/of_dma.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> @@ -23,8 +24,6 @@
>  #include "dmaengine.h"
>  #include "virt-dma.h"
>
> -#define JZ_DMA_NR_CHANNELS     32
> -
>  /* Global registers. */
>  #define JZ_DMA_REG_DMAC                0x1000
>  #define JZ_DMA_REG_DIRQP       0x1004
> @@ -135,14 +134,20 @@ struct jz4780_dma_chan {
>         unsigned int curr_hwdesc;
>  };
>
> +enum jz_version {
> +       ID_JZ4780,
> +};
> +
>  struct jz4780_dma_dev {
>         struct dma_device dma_device;
>         void __iomem *base;
>         struct clk *clk;
>         unsigned int irq;
> +       unsigned int nb_channels;
> +       enum jz_version version;
>
>         uint32_t chan_reserved;
> -       struct jz4780_dma_chan chan[JZ_DMA_NR_CHANNELS];
> +       struct jz4780_dma_chan chan[];

Looks like a variable length array in struct. I think there is some
effort to remove the usage of VLA. Can you revisit this? I may be
wrong, please feel free to correct.

>  };
>
>  struct jz4780_dma_filter_data {
> @@ -648,7 +653,7 @@ static irqreturn_t jz4780_dma_irq_handler(int irq, void *data)
>
>         pending = jz4780_dma_readl(jzdma, JZ_DMA_REG_DIRQP);
>
> -       for (i = 0; i < JZ_DMA_NR_CHANNELS; i++) {
> +       for (i = 0; i < jzdma->nb_channels; i++) {
>                 if (!(pending & (1<<i)))
>                         continue;
>
> @@ -728,7 +733,7 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
>         data.channel = dma_spec->args[1];
>
>         if (data.channel > -1) {
> -               if (data.channel >= JZ_DMA_NR_CHANNELS) {
> +               if (data.channel >= jzdma->nb_channels) {
>                         dev_err(jzdma->dma_device.dev,
>                                 "device requested non-existent channel %u\n",
>                                 data.channel);
> @@ -752,19 +757,45 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
>         }
>  }
>
> +static const unsigned int jz4780_dma_nb_channels[] = {
> +       [ID_JZ4780] = 32,
> +};
> +
> +static const struct of_device_id jz4780_dma_dt_match[] = {
> +       { .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, jz4780_dma_dt_match);
> +
>  static int jz4780_dma_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> +       const struct of_device_id *of_id = of_match_device(
> +                       jz4780_dma_dt_match, dev);
>         struct jz4780_dma_dev *jzdma;
>         struct jz4780_dma_chan *jzchan;
>         struct dma_device *dd;
>         struct resource *res;
> +       enum jz_version version;
> +       unsigned int nb_channels;
>         int i, ret;
>
> -       jzdma = devm_kzalloc(dev, sizeof(*jzdma), GFP_KERNEL);
> +       if (of_id)
> +               version = (enum jz_version)of_id->data;
> +       else
> +               version = ID_JZ4780; /* Default when not probed from DT */
> +
> +       nb_channels = jz4780_dma_nb_channels[version];
> +
> +       jzdma = devm_kzalloc(dev, sizeof(*jzdma)
> +                               + sizeof(*jzdma->chan) * nb_channels,
> +                               GFP_KERNEL);
>         if (!jzdma)
>                 return -ENOMEM;
>
> +       jzdma->nb_channels = nb_channels;
> +       jzdma->version = version;
> +
>         platform_set_drvdata(pdev, jzdma);
>
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -839,7 +870,7 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>
>         INIT_LIST_HEAD(&dd->channels);
>
> -       for (i = 0; i < JZ_DMA_NR_CHANNELS; i++) {
> +       for (i = 0; i < jzdma->nb_channels; i++) {
>                 jzchan = &jzdma->chan[i];
>                 jzchan->id = i;
>
> @@ -884,19 +915,13 @@ static int jz4780_dma_remove(struct platform_device *pdev)
>
>         free_irq(jzdma->irq, jzdma);
>
> -       for (i = 0; i < JZ_DMA_NR_CHANNELS; i++)
> +       for (i = 0; i < jzdma->nb_channels; i++)
>                 tasklet_kill(&jzdma->chan[i].vchan.task);
>
>         dma_async_device_unregister(&jzdma->dma_device);
>         return 0;
>  }
>
> -static const struct of_device_id jz4780_dma_dt_match[] = {
> -       { .compatible = "ingenic,jz4780-dma", .data = NULL },
> -       {},
> -};
> -MODULE_DEVICE_TABLE(of, jz4780_dma_dt_match);
> -
>  static struct platform_driver jz4780_dma_driver = {
>         .probe          = jz4780_dma_probe,
>         .remove         = jz4780_dma_remove,
> --
> 2.18.0
>
>

Re: [PATCH 01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels

[Paul Cercueil]

Hi PrasannaKumar,

> Hi Paul,
> 
> On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote:
>>  As part of the work to support various other Ingenic JZ47xx SoC 
>> versions,
>>  which don't feature the same number of DMA channels per core, we now
>>  deduce the number of DMA channels available from the devicetree
>>  compatible string.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   drivers/dma/dma-jz4780.c | 53 
>> +++++++++++++++++++++++++++++-----------
>>   1 file changed, 39 insertions(+), 14 deletions(-)
>> 
>>  diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
>>  index 85820a2d69d4..b40f491f0367 100644
>>  --- a/drivers/dma/dma-jz4780.c
>>  +++ b/drivers/dma/dma-jz4780.c
>>  @@ -16,6 +16,7 @@
>>   #include <linux/interrupt.h>
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>>  +#include <linux/of_device.h>
>>   #include <linux/of_dma.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/slab.h>
>>  @@ -23,8 +24,6 @@
>>   #include "dmaengine.h"
>>   #include "virt-dma.h"
>> 
>>  -#define JZ_DMA_NR_CHANNELS     32
>>  -
>>   /* Global registers. */
>>   #define JZ_DMA_REG_DMAC                0x1000
>>   #define JZ_DMA_REG_DIRQP       0x1004
>>  @@ -135,14 +134,20 @@ struct jz4780_dma_chan {
>>          unsigned int curr_hwdesc;
>>   };
>> 
>>  +enum jz_version {
>>  +       ID_JZ4780,
>>  +};
>>  +
>>   struct jz4780_dma_dev {
>>          struct dma_device dma_device;
>>          void __iomem *base;
>>          struct clk *clk;
>>          unsigned int irq;
>>  +       unsigned int nb_channels;
>>  +       enum jz_version version;
>> 
>>          uint32_t chan_reserved;
>>  -       struct jz4780_dma_chan chan[JZ_DMA_NR_CHANNELS];
>>  +       struct jz4780_dma_chan chan[];
> 
> Looks like a variable length array in struct. I think there is some
> effort to remove the usage of VLA. Can you revisit this? I may be
> wrong, please feel free to correct.

Are you sure? It's the first time I hear about it.
Could anybody confirm?

>>   };
>> 
>>   struct jz4780_dma_filter_data {
>>  @@ -648,7 +653,7 @@ static irqreturn_t jz4780_dma_irq_handler(int 
>> irq, void *data)
>> 
>>          pending = jz4780_dma_readl(jzdma, JZ_DMA_REG_DIRQP);
>> 
>>  -       for (i = 0; i < JZ_DMA_NR_CHANNELS; i++) {
>>  +       for (i = 0; i < jzdma->nb_channels; i++) {
>>                  if (!(pending & (1<<i)))
>>                          continue;
>> 
>>  @@ -728,7 +733,7 @@ static struct dma_chan 
>> *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
>>          data.channel = dma_spec->args[1];
>> 
>>          if (data.channel > -1) {
>>  -               if (data.channel >= JZ_DMA_NR_CHANNELS) {
>>  +               if (data.channel >= jzdma->nb_channels) {
>>                          dev_err(jzdma->dma_device.dev,
>>                                  "device requested non-existent 
>> channel %u\n",
>>                                  data.channel);
>>  @@ -752,19 +757,45 @@ static struct dma_chan 
>> *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
>>          }
>>   }
>> 
>>  +static const unsigned int jz4780_dma_nb_channels[] = {
>>  +       [ID_JZ4780] = 32,
>>  +};
>>  +
>>  +static const struct of_device_id jz4780_dma_dt_match[] = {
>>  +       { .compatible = "ingenic,jz4780-dma", .data = (void 
>> *)ID_JZ4780 },
>>  +       {},
>>  +};
>>  +MODULE_DEVICE_TABLE(of, jz4780_dma_dt_match);
>>  +
>>   static int jz4780_dma_probe(struct platform_device *pdev)
>>   {
>>          struct device *dev = &pdev->dev;
>>  +       const struct of_device_id *of_id = of_match_device(
>>  +                       jz4780_dma_dt_match, dev);
>>          struct jz4780_dma_dev *jzdma;
>>          struct jz4780_dma_chan *jzchan;
>>          struct dma_device *dd;
>>          struct resource *res;
>>  +       enum jz_version version;
>>  +       unsigned int nb_channels;
>>          int i, ret;
>> 
>>  -       jzdma = devm_kzalloc(dev, sizeof(*jzdma), GFP_KERNEL);
>>  +       if (of_id)
>>  +               version = (enum jz_version)of_id->data;
>>  +       else
>>  +               version = ID_JZ4780; /* Default when not probed 
>> from DT */
>>  +
>>  +       nb_channels = jz4780_dma_nb_channels[version];
>>  +
>>  +       jzdma = devm_kzalloc(dev, sizeof(*jzdma)
>>  +                               + sizeof(*jzdma->chan) * 
>> nb_channels,
>>  +                               GFP_KERNEL);
>>          if (!jzdma)
>>                  return -ENOMEM;
>> 
>>  +       jzdma->nb_channels = nb_channels;
>>  +       jzdma->version = version;
>>  +
>>          platform_set_drvdata(pdev, jzdma);
>> 
>>          res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  @@ -839,7 +870,7 @@ static int jz4780_dma_probe(struct 
>> platform_device *pdev)
>> 
>>          INIT_LIST_HEAD(&dd->channels);
>> 
>>  -       for (i = 0; i < JZ_DMA_NR_CHANNELS; i++) {
>>  +       for (i = 0; i < jzdma->nb_channels; i++) {
>>                  jzchan = &jzdma->chan[i];
>>                  jzchan->id = i;
>> 
>>  @@ -884,19 +915,13 @@ static int jz4780_dma_remove(struct 
>> platform_device *pdev)
>> 
>>          free_irq(jzdma->irq, jzdma);
>> 
>>  -       for (i = 0; i < JZ_DMA_NR_CHANNELS; i++)
>>  +       for (i = 0; i < jzdma->nb_channels; i++)
>>                  tasklet_kill(&jzdma->chan[i].vchan.task);
>> 
>>          dma_async_device_unregister(&jzdma->dma_device);
>>          return 0;
>>   }
>> 
>>  -static const struct of_device_id jz4780_dma_dt_match[] = {
>>  -       { .compatible = "ingenic,jz4780-dma", .data = NULL },
>>  -       {},
>>  -};
>>  -MODULE_DEVICE_TABLE(of, jz4780_dma_dt_match);
>>  -
>>   static struct platform_driver jz4780_dma_driver = {
>>          .probe          = jz4780_dma_probe,
>>          .remove         = jz4780_dma_remove,
>>  --
>>  2.18.0
>> 
>> 

Re: [PATCH 01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels

[PrasannaKumar Muralidharan]

On 5 July 2018 at 23:56, Paul Cercueil <paul@crapouillou.net> wrote:
> Hi PrasannaKumar,
>
>
>> Hi Paul,
>>
>> On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote:
>>>
>>>  As part of the work to support various other Ingenic JZ47xx SoC
>>> versions,
>>>  which don't feature the same number of DMA channels per core, we now
>>>  deduce the number of DMA channels available from the devicetree
>>>  compatible string.
>>>
>>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>  ---
>>>   drivers/dma/dma-jz4780.c | 53 +++++++++++++++++++++++++++++-----------
>>>   1 file changed, 39 insertions(+), 14 deletions(-)
>>>
>>>  diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
>>>  index 85820a2d69d4..b40f491f0367 100644
>>>  --- a/drivers/dma/dma-jz4780.c
>>>  +++ b/drivers/dma/dma-jz4780.c
>>>  @@ -16,6 +16,7 @@
>>>   #include <linux/interrupt.h>
>>>   #include <linux/module.h>
>>>   #include <linux/of.h>
>>>  +#include <linux/of_device.h>
>>>   #include <linux/of_dma.h>
>>>   #include <linux/platform_device.h>
>>>   #include <linux/slab.h>
>>>  @@ -23,8 +24,6 @@
>>>   #include "dmaengine.h"
>>>   #include "virt-dma.h"
>>>
>>>  -#define JZ_DMA_NR_CHANNELS     32
>>>  -
>>>   /* Global registers. */
>>>   #define JZ_DMA_REG_DMAC                0x1000
>>>   #define JZ_DMA_REG_DIRQP       0x1004
>>>  @@ -135,14 +134,20 @@ struct jz4780_dma_chan {
>>>          unsigned int curr_hwdesc;
>>>   };
>>>
>>>  +enum jz_version {
>>>  +       ID_JZ4780,
>>>  +};
>>>  +
>>>   struct jz4780_dma_dev {
>>>          struct dma_device dma_device;
>>>          void __iomem *base;
>>>          struct clk *clk;
>>>          unsigned int irq;
>>>  +       unsigned int nb_channels;
>>>  +       enum jz_version version;
>>>
>>>          uint32_t chan_reserved;
>>>  -       struct jz4780_dma_chan chan[JZ_DMA_NR_CHANNELS];
>>>  +       struct jz4780_dma_chan chan[];
>>
>>
>> Looks like a variable length array in struct. I think there is some
>> effort to remove the usage of VLA. Can you revisit this? I may be
>> wrong, please feel free to correct.
>
>
> Are you sure? It's the first time I hear about it.
> Could anybody confirm?

Please see [1] for info.

Variable Length Arrays in struct is expressly forbidden in C99, C11.
Clang does not support it. To make kernel compile with Clang few
people are trying to remove/reduce VLAIS usage.

1. https://blog.linuxplumbersconf.org/2013/ocw/system/presentations/1221/original/VLAIS.pdf

Re: [PATCH 01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels

[Paul Cercueil]

Le sam. 7 juil. 2018 à 9:34, PrasannaKumar Muralidharan 
<prasannatsmkumar@gmail.com> a écrit :
> On 5 July 2018 at 23:56, Paul Cercueil <paul@crapouillou.net> wrote:
>>  Hi PrasannaKumar,
>> 
>> 
>>>  Hi Paul,
>>> 
>>>  On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> 
>>> wrote:
>>>> 
>>>>   As part of the work to support various other Ingenic JZ47xx SoC
>>>>  versions,
>>>>   which don't feature the same number of DMA channels per core, we 
>>>> now
>>>>   deduce the number of DMA channels available from the devicetree
>>>>   compatible string.
>>>> 
>>>>   Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>>   ---
>>>>    drivers/dma/dma-jz4780.c | 53 
>>>> +++++++++++++++++++++++++++++-----------
>>>>    1 file changed, 39 insertions(+), 14 deletions(-)
>>>> 
>>>>   diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
>>>>   index 85820a2d69d4..b40f491f0367 100644
>>>>   --- a/drivers/dma/dma-jz4780.c
>>>>   +++ b/drivers/dma/dma-jz4780.c
>>>>   @@ -16,6 +16,7 @@
>>>>    #include <linux/interrupt.h>
>>>>    #include <linux/module.h>
>>>>    #include <linux/of.h>
>>>>   +#include <linux/of_device.h>
>>>>    #include <linux/of_dma.h>
>>>>    #include <linux/platform_device.h>
>>>>    #include <linux/slab.h>
>>>>   @@ -23,8 +24,6 @@
>>>>    #include "dmaengine.h"
>>>>    #include "virt-dma.h"
>>>> 
>>>>   -#define JZ_DMA_NR_CHANNELS     32
>>>>   -
>>>>    /* Global registers. */
>>>>    #define JZ_DMA_REG_DMAC                0x1000
>>>>    #define JZ_DMA_REG_DIRQP       0x1004
>>>>   @@ -135,14 +134,20 @@ struct jz4780_dma_chan {
>>>>           unsigned int curr_hwdesc;
>>>>    };
>>>> 
>>>>   +enum jz_version {
>>>>   +       ID_JZ4780,
>>>>   +};
>>>>   +
>>>>    struct jz4780_dma_dev {
>>>>           struct dma_device dma_device;
>>>>           void __iomem *base;
>>>>           struct clk *clk;
>>>>           unsigned int irq;
>>>>   +       unsigned int nb_channels;
>>>>   +       enum jz_version version;
>>>> 
>>>>           uint32_t chan_reserved;
>>>>   -       struct jz4780_dma_chan chan[JZ_DMA_NR_CHANNELS];
>>>>   +       struct jz4780_dma_chan chan[];
>>> 
>>> 
>>>  Looks like a variable length array in struct. I think there is some
>>>  effort to remove the usage of VLA. Can you revisit this? I may be
>>>  wrong, please feel free to correct.
>> 
>> 
>>  Are you sure? It's the first time I hear about it.
>>  Could anybody confirm?
> 
> Please see [1] for info.
> 
> Variable Length Arrays in struct is expressly forbidden in C99, C11.
> Clang does not support it. To make kernel compile with Clang few
> people are trying to remove/reduce VLAIS usage.
> 
> 1. 
> https://blog.linuxplumbersconf.org/2013/ocw/system/presentations/1221/original/VLAIS.pdf

I read it, and my structure is not a VLAIS; my "chan" array is a 
flexible
array, its sizeof() is 0, so the sizeof() of the structure is constant.

See page 6 of the PDF, about alternatives to VLAIS:
"If possible use a flexible array member and move the array to the end 
of
the struct"
Which is what I am doing here.

-Paul

Re: [PATCH 01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels

[Vinod]

On 03-07-18, 14:32, Paul Cercueil wrote:

>  struct jz4780_dma_dev {
>  	struct dma_device dma_device;
>  	void __iomem *base;
>  	struct clk *clk;
>  	unsigned int irq;
> +	unsigned int nb_channels;
> +	enum jz_version version;
>  
>  	uint32_t chan_reserved;
> -	struct jz4780_dma_chan chan[JZ_DMA_NR_CHANNELS];
> +	struct jz4780_dma_chan chan[];

why array, why not channel pointer?

> +static const unsigned int jz4780_dma_nb_channels[] = {
> +	[ID_JZ4780] = 32,
> +};
> +
> +static const struct of_device_id jz4780_dma_dt_match[] = {
> +	{ .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
> +	{},
> +};

Looking at description I was hoping that channels would be in DT,
channels is hardware information, so should come from DT rather than
coding the kernel...

> -	jzdma = devm_kzalloc(dev, sizeof(*jzdma), GFP_KERNEL);
> +	if (of_id)
> +		version = (enum jz_version)of_id->data;
> +	else
> +		version = ID_JZ4780; /* Default when not probed from DT */

where else would it be probed from.... ?
-- 
~Vinod

Re: [PATCH 01/14] dmaengine: dma-jz4780: Avoid hardcoding number of channels

[Vinod]

Hi Paul,

On 10-07-18, 17:29, Paul Cercueil wrote:
> > >  +static const unsigned int jz4780_dma_nb_channels[] = {
> > >  +	[ID_JZ4780] = 32,
> > >  +};
> > >  +
> > >  +static const struct of_device_id jz4780_dma_dt_match[] = {
> > >  +	{ .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
> > >  +	{},
> > >  +};
> > 
> > Looking at description I was hoping that channels would be in DT,
> > channels is hardware information, so should come from DT rather than
> > coding the kernel...
> 
> I had a talk with Linus Walleij (GPIO maintainer) about that:
> http://lkml.iu.edu/hypermail/linux/kernel/1701.3/05422.html
> 
> And I agree with him, we shouldn't have in devicetree what we can deduce
> from the compatible string. But there doesn't seem to be an enforced
> policy about it.

Looking at this, yes that can be done as you have implemented but adding
new compatible and tables every time seems not so great to me.

If DT can describe these hardware features then driver can take action generically
and we avoid these tables and skip some patches here..

> 
> @Rob, what do you think?

Rob what is the recommendation here?

> 
> > >  -	jzdma = devm_kzalloc(dev, sizeof(*jzdma), GFP_KERNEL);
> > >  +	if (of_id)
> > >  +		version = (enum jz_version)of_id->data;
> > >  +	else
> > >  +		version = ID_JZ4780; /* Default when not probed from DT */
> > 
> > where else would it be probed from.... ?
> 
> Platform, MFD driver, etc. But not likely to happen.
> I can remove these lines if you want.

Lets add when we land support for those.

-- 
~Vinod

[PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[Paul Cercueil]

The register area of the JZ4780 DMA core can be split into different
sections for different purposes:

* one set of registers is used to perform actions at the DMA core level,
that will generally affect all channels;

* one set of registers per DMA channel, to perform actions at the DMA
channel level, that will only affect the channel in question.

The problem rises when trying to support new versions of the JZ47xx
Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one
with six DMA channels, and the register sets are interleaved:
<DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs>

By using one memory resource for the channel-specific registers and
one memory resource for the core-specific registers, we can support
the JZ4770, by initializing the driver once per DMA core with different
addresses.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 .../devicetree/bindings/dma/jz4780-dma.txt    |   6 +-
 drivers/dma/dma-jz4780.c                      | 106 +++++++++++-------
 2 files changed, 69 insertions(+), 43 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
index f25feee62b15..f9b1864f5b77 100644
--- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
+++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
@@ -3,7 +3,8 @@
 Required properties:
 
 - compatible: Should be "ingenic,jz4780-dma"
-- reg: Should contain the DMA controller registers location and length.
+- reg: Should contain the DMA channel registers location and length, followed
+  by the DMA controller registers location and length.
 - interrupts: Should contain the interrupt specifier of the DMA controller.
 - interrupt-parent: Should be the phandle of the interrupt controller that
 - clocks: Should contain a clock specifier for the JZ4780 PDMA clock.
@@ -22,7 +23,8 @@ Example:
 
 dma: dma@13420000 {
 	compatible = "ingenic,jz4780-dma";
-	reg = <0x13420000 0x10000>;
+	reg = <0x13420000 0x400
+	       0x13421000 0x40>;
 
 	interrupt-parent = <&intc>;
 	interrupts = <10>;
diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index b40f491f0367..4d234caf5d62 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -25,26 +25,26 @@
 #include "virt-dma.h"
 
 /* Global registers. */
-#define JZ_DMA_REG_DMAC		0x1000
-#define JZ_DMA_REG_DIRQP	0x1004
-#define JZ_DMA_REG_DDR		0x1008
-#define JZ_DMA_REG_DDRS		0x100c
-#define JZ_DMA_REG_DMACP	0x101c
-#define JZ_DMA_REG_DSIRQP	0x1020
-#define JZ_DMA_REG_DSIRQM	0x1024
-#define JZ_DMA_REG_DCIRQP	0x1028
-#define JZ_DMA_REG_DCIRQM	0x102c
+#define JZ_DMA_REG_DMAC		0x00
+#define JZ_DMA_REG_DIRQP	0x04
+#define JZ_DMA_REG_DDR		0x08
+#define JZ_DMA_REG_DDRS		0x0c
+#define JZ_DMA_REG_DMACP	0x1c
+#define JZ_DMA_REG_DSIRQP	0x20
+#define JZ_DMA_REG_DSIRQM	0x24
+#define JZ_DMA_REG_DCIRQP	0x28
+#define JZ_DMA_REG_DCIRQM	0x2c
 
 /* Per-channel registers. */
 #define JZ_DMA_REG_CHAN(n)	(n * 0x20)
-#define JZ_DMA_REG_DSA(n)	(0x00 + JZ_DMA_REG_CHAN(n))
-#define JZ_DMA_REG_DTA(n)	(0x04 + JZ_DMA_REG_CHAN(n))
-#define JZ_DMA_REG_DTC(n)	(0x08 + JZ_DMA_REG_CHAN(n))
-#define JZ_DMA_REG_DRT(n)	(0x0c + JZ_DMA_REG_CHAN(n))
-#define JZ_DMA_REG_DCS(n)	(0x10 + JZ_DMA_REG_CHAN(n))
-#define JZ_DMA_REG_DCM(n)	(0x14 + JZ_DMA_REG_CHAN(n))
-#define JZ_DMA_REG_DDA(n)	(0x18 + JZ_DMA_REG_CHAN(n))
-#define JZ_DMA_REG_DSD(n)	(0x1c + JZ_DMA_REG_CHAN(n))
+#define JZ_DMA_REG_DSA		0x00
+#define JZ_DMA_REG_DTA		0x04
+#define JZ_DMA_REG_DTC		0x08
+#define JZ_DMA_REG_DRT		0x0c
+#define JZ_DMA_REG_DCS		0x10
+#define JZ_DMA_REG_DCM		0x14
+#define JZ_DMA_REG_DDA		0x18
+#define JZ_DMA_REG_DSD		0x1c
 
 #define JZ_DMA_DMAC_DMAE	BIT(0)
 #define JZ_DMA_DMAC_AR		BIT(2)
@@ -140,7 +140,8 @@ enum jz_version {
 
 struct jz4780_dma_dev {
 	struct dma_device dma_device;
-	void __iomem *base;
+	void __iomem *chn_base;
+	void __iomem *ctrl_base;
 	struct clk *clk;
 	unsigned int irq;
 	unsigned int nb_channels;
@@ -174,16 +175,28 @@ static inline struct jz4780_dma_dev *jz4780_dma_chan_parent(
 			    dma_device);
 }
 
-static inline uint32_t jz4780_dma_readl(struct jz4780_dma_dev *jzdma,
+static inline uint32_t jz4780_dma_chn_readl(struct jz4780_dma_dev *jzdma,
+	unsigned int chn, unsigned int reg)
+{
+	return readl(jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn));
+}
+
+static inline void jz4780_dma_chn_writel(struct jz4780_dma_dev *jzdma,
+	unsigned int chn, unsigned int reg, uint32_t val)
+{
+	writel(val, jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn));
+}
+
+static inline uint32_t jz4780_dma_ctrl_readl(struct jz4780_dma_dev *jzdma,
 	unsigned int reg)
 {
-	return readl(jzdma->base + reg);
+	return readl(jzdma->ctrl_base + reg);
 }
 
-static inline void jz4780_dma_writel(struct jz4780_dma_dev *jzdma,
+static inline void jz4780_dma_ctrl_writel(struct jz4780_dma_dev *jzdma,
 	unsigned int reg, uint32_t val)
 {
-	writel(val, jzdma->base + reg);
+	writel(val, jzdma->ctrl_base + reg);
 }
 
 static struct jz4780_dma_desc *jz4780_dma_desc_alloc(
@@ -478,17 +491,18 @@ static void jz4780_dma_begin(struct jz4780_dma_chan *jzchan)
 	}
 
 	/* Use 8-word descriptors. */
-	jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), JZ_DMA_DCS_DES8);
+	jz4780_dma_chn_writel(jzdma, jzchan->id,
+			      JZ_DMA_REG_DCS, JZ_DMA_DCS_DES8);
 
 	/* Write descriptor address and initiate descriptor fetch. */
 	desc_phys = jzchan->desc->desc_phys +
 		    (jzchan->curr_hwdesc * sizeof(*jzchan->desc->desc));
-	jz4780_dma_writel(jzdma, JZ_DMA_REG_DDA(jzchan->id), desc_phys);
-	jz4780_dma_writel(jzdma, JZ_DMA_REG_DDRS, BIT(jzchan->id));
+	jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DDA, desc_phys);
+	jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DDRS, BIT(jzchan->id));
 
 	/* Enable the channel. */
-	jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id),
-			  JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE);
+	jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS,
+			      JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE);
 }
 
 static void jz4780_dma_issue_pending(struct dma_chan *chan)
@@ -514,7 +528,7 @@ static int jz4780_dma_terminate_all(struct dma_chan *chan)
 	spin_lock_irqsave(&jzchan->vchan.lock, flags);
 
 	/* Clear the DMA status and stop the transfer. */
-	jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0);
+	jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
 	if (jzchan->desc) {
 		vchan_terminate_vdesc(&jzchan->desc->vdesc);
 		jzchan->desc = NULL;
@@ -563,8 +577,8 @@ static size_t jz4780_dma_desc_residue(struct jz4780_dma_chan *jzchan,
 		residue += desc->desc[i].dtc << jzchan->transfer_shift;
 
 	if (next_sg != 0) {
-		count = jz4780_dma_readl(jzdma,
-					 JZ_DMA_REG_DTC(jzchan->id));
+		count = jz4780_dma_chn_readl(jzdma, jzchan->id,
+					 JZ_DMA_REG_DTC);
 		residue += count << jzchan->transfer_shift;
 	}
 
@@ -611,8 +625,8 @@ static void jz4780_dma_chan_irq(struct jz4780_dma_dev *jzdma,
 
 	spin_lock(&jzchan->vchan.lock);
 
-	dcs = jz4780_dma_readl(jzdma, JZ_DMA_REG_DCS(jzchan->id));
-	jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0);
+	dcs = jz4780_dma_chn_readl(jzdma, jzchan->id, JZ_DMA_REG_DCS);
+	jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
 
 	if (dcs & JZ_DMA_DCS_AR) {
 		dev_warn(&jzchan->vchan.chan.dev->device,
@@ -651,7 +665,7 @@ static irqreturn_t jz4780_dma_irq_handler(int irq, void *data)
 	uint32_t pending, dmac;
 	int i;
 
-	pending = jz4780_dma_readl(jzdma, JZ_DMA_REG_DIRQP);
+	pending = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DIRQP);
 
 	for (i = 0; i < jzdma->nb_channels; i++) {
 		if (!(pending & (1<<i)))
@@ -661,12 +675,12 @@ static irqreturn_t jz4780_dma_irq_handler(int irq, void *data)
 	}
 
 	/* Clear halt and address error status of all channels. */
-	dmac = jz4780_dma_readl(jzdma, JZ_DMA_REG_DMAC);
+	dmac = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DMAC);
 	dmac &= ~(JZ_DMA_DMAC_HLT | JZ_DMA_DMAC_AR);
-	jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC, dmac);
+	jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, dmac);
 
 	/* Clear interrupt pending status. */
-	jz4780_dma_writel(jzdma, JZ_DMA_REG_DIRQP, 0);
+	jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DIRQP, 0);
 
 	return IRQ_HANDLED;
 }
@@ -804,9 +818,19 @@ static int jz4780_dma_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	jzdma->base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(jzdma->base))
-		return PTR_ERR(jzdma->base);
+	jzdma->chn_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(jzdma->chn_base))
+		return PTR_ERR(jzdma->chn_base);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res) {
+		dev_err(dev, "failed to get I/O memory\n");
+		return -EINVAL;
+	}
+
+	jzdma->ctrl_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(jzdma->ctrl_base))
+		return PTR_ERR(jzdma->ctrl_base);
 
 	ret = platform_get_irq(pdev, 0);
 	if (ret < 0) {
@@ -864,9 +888,9 @@ static int jz4780_dma_probe(struct platform_device *pdev)
 	 * Also set the FMSC bit - it increases MSC performance, so it makes
 	 * little sense not to enable it.
 	 */
-	jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC,
+	jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC,
 			  JZ_DMA_DMAC_DMAE | JZ_DMA_DMAC_FMSC);
-	jz4780_dma_writel(jzdma, JZ_DMA_REG_DMACP, 0);
+	jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0);
 
 	INIT_LIST_HEAD(&dd->channels);
 
-- 
2.18.0

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[Paul Burton]

Hi Paul,

On Tue, Jul 03, 2018 at 02:32:02PM +0200, Paul Cercueil wrote:
> @@ -804,9 +818,19 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	jzdma->base = devm_ioremap_resource(dev, res);
> -	if (IS_ERR(jzdma->base))
> -		return PTR_ERR(jzdma->base);
> +	jzdma->chn_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(jzdma->chn_base))
> +		return PTR_ERR(jzdma->chn_base);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res) {
> +		dev_err(dev, "failed to get I/O memory\n");
> +		return -EINVAL;
> +	}
> +
> +	jzdma->ctrl_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(jzdma->ctrl_base))
> +		return PTR_ERR(jzdma->ctrl_base);

Could we have this failure case fall back to the existing behaviour
where we only have a single resource covering all the registers? That
would avoid breaking bisection between this patch & the one that updates
the JZ4780 DT.

For example:

	#define JZ4780_DMA_CTRL_OFFSET	0x1000

	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
	if (res) {
		jzdma->ctrl_base = devm_ioremap_resource(dev, res);
		if (IS_ERR(jzdma->ctrl_base))
			return PTR_ERR(jzdma->ctrl_base);
	} else {
		jzdma->ctrl_base = jzdma->chn_base + JZ4780_DMA_CTRL_OFFSET;
	}

Then you could remove the fallback after patch 13, to end up with the
same code you have now but without breaking bisection.

Most correct might be to move patch 13 to right after this one, so that
the JZ4780-specific fallback can be removed before adding support for
any of the other SoCs.

Thanks,
    Paul

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[Paul Cercueil]

Hi Paul,

> Hi Paul,
> 
> On Tue, Jul 03, 2018 at 02:32:02PM +0200, Paul Cercueil wrote:
>>  @@ -804,9 +818,19 @@ static int jz4780_dma_probe(struct 
>> platform_device *pdev)
>>   		return -EINVAL;
>>   	}
>> 
>>  -	jzdma->base = devm_ioremap_resource(dev, res);
>>  -	if (IS_ERR(jzdma->base))
>>  -		return PTR_ERR(jzdma->base);
>>  +	jzdma->chn_base = devm_ioremap_resource(dev, res);
>>  +	if (IS_ERR(jzdma->chn_base))
>>  +		return PTR_ERR(jzdma->chn_base);
>>  +
>>  +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>  +	if (!res) {
>>  +		dev_err(dev, "failed to get I/O memory\n");
>>  +		return -EINVAL;
>>  +	}
>>  +
>>  +	jzdma->ctrl_base = devm_ioremap_resource(dev, res);
>>  +	if (IS_ERR(jzdma->ctrl_base))
>>  +		return PTR_ERR(jzdma->ctrl_base);
> 
> Could we have this failure case fall back to the existing behaviour
> where we only have a single resource covering all the registers? That
> would avoid breaking bisection between this patch & the one that 
> updates
> the JZ4780 DT.
> 
> For example:
> 
> 	#define JZ4780_DMA_CTRL_OFFSET	0x1000
> 
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> 	if (res) {
> 		jzdma->ctrl_base = devm_ioremap_resource(dev, res);
> 		if (IS_ERR(jzdma->ctrl_base))
> 			return PTR_ERR(jzdma->ctrl_base);
> 	} else {
> 		jzdma->ctrl_base = jzdma->chn_base + JZ4780_DMA_CTRL_OFFSET;
> 	}
> 
> Then you could remove the fallback after patch 13, to end up with the
> same code you have now but without breaking bisection.
> 
> Most correct might be to move patch 13 to right after this one, so 
> that
> the JZ4780-specific fallback can be removed before adding support for
> any of the other SoCs.

Sure, I can do that for the V2.

Thanks,
-Paul

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[PrasannaKumar Muralidharan]

Paul,

On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote:
> The register area of the JZ4780 DMA core can be split into different
> sections for different purposes:
>
> * one set of registers is used to perform actions at the DMA core level,
> that will generally affect all channels;
>
> * one set of registers per DMA channel, to perform actions at the DMA
> channel level, that will only affect the channel in question.
>
> The problem rises when trying to support new versions of the JZ47xx
> Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one
> with six DMA channels, and the register sets are interleaved:
> <DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs>
>
> By using one memory resource for the channel-specific registers and
> one memory resource for the core-specific registers, we can support
> the JZ4770, by initializing the driver once per DMA core with different
> addresses.

As per my understanding device tree should be modified only when
hardware changes. This looks the other way around. It must be possible
to achieve what you are trying to do in this patch without changing
the device tree.

> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  .../devicetree/bindings/dma/jz4780-dma.txt    |   6 +-
>  drivers/dma/dma-jz4780.c                      | 106 +++++++++++-------
>  2 files changed, 69 insertions(+), 43 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> index f25feee62b15..f9b1864f5b77 100644
> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> @@ -3,7 +3,8 @@
>  Required properties:
>
>  - compatible: Should be "ingenic,jz4780-dma"
> -- reg: Should contain the DMA controller registers location and length.
> +- reg: Should contain the DMA channel registers location and length, followed
> +  by the DMA controller registers location and length.
>  - interrupts: Should contain the interrupt specifier of the DMA controller.
>  - interrupt-parent: Should be the phandle of the interrupt controller that
>  - clocks: Should contain a clock specifier for the JZ4780 PDMA clock.
> @@ -22,7 +23,8 @@ Example:
>
>  dma: dma@13420000 {
>         compatible = "ingenic,jz4780-dma";
> -       reg = <0x13420000 0x10000>;
> +       reg = <0x13420000 0x400
> +              0x13421000 0x40>;
>
>         interrupt-parent = <&intc>;
>         interrupts = <10>;
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index b40f491f0367..4d234caf5d62 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -25,26 +25,26 @@
>  #include "virt-dma.h"
>
>  /* Global registers. */
> -#define JZ_DMA_REG_DMAC                0x1000
> -#define JZ_DMA_REG_DIRQP       0x1004
> -#define JZ_DMA_REG_DDR         0x1008
> -#define JZ_DMA_REG_DDRS                0x100c
> -#define JZ_DMA_REG_DMACP       0x101c
> -#define JZ_DMA_REG_DSIRQP      0x1020
> -#define JZ_DMA_REG_DSIRQM      0x1024
> -#define JZ_DMA_REG_DCIRQP      0x1028
> -#define JZ_DMA_REG_DCIRQM      0x102c
> +#define JZ_DMA_REG_DMAC                0x00
> +#define JZ_DMA_REG_DIRQP       0x04
> +#define JZ_DMA_REG_DDR         0x08
> +#define JZ_DMA_REG_DDRS                0x0c
> +#define JZ_DMA_REG_DMACP       0x1c
> +#define JZ_DMA_REG_DSIRQP      0x20
> +#define JZ_DMA_REG_DSIRQM      0x24
> +#define JZ_DMA_REG_DCIRQP      0x28
> +#define JZ_DMA_REG_DCIRQM      0x2c
>
>  /* Per-channel registers. */
>  #define JZ_DMA_REG_CHAN(n)     (n * 0x20)
> -#define JZ_DMA_REG_DSA(n)      (0x00 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DTA(n)      (0x04 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DTC(n)      (0x08 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DRT(n)      (0x0c + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DCS(n)      (0x10 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DCM(n)      (0x14 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DDA(n)      (0x18 + JZ_DMA_REG_CHAN(n))
> -#define JZ_DMA_REG_DSD(n)      (0x1c + JZ_DMA_REG_CHAN(n))
> +#define JZ_DMA_REG_DSA         0x00
> +#define JZ_DMA_REG_DTA         0x04
> +#define JZ_DMA_REG_DTC         0x08
> +#define JZ_DMA_REG_DRT         0x0c
> +#define JZ_DMA_REG_DCS         0x10
> +#define JZ_DMA_REG_DCM         0x14
> +#define JZ_DMA_REG_DDA         0x18
> +#define JZ_DMA_REG_DSD         0x1c
>
>  #define JZ_DMA_DMAC_DMAE       BIT(0)
>  #define JZ_DMA_DMAC_AR         BIT(2)
> @@ -140,7 +140,8 @@ enum jz_version {
>
>  struct jz4780_dma_dev {
>         struct dma_device dma_device;
> -       void __iomem *base;
> +       void __iomem *chn_base;
> +       void __iomem *ctrl_base;
>         struct clk *clk;
>         unsigned int irq;
>         unsigned int nb_channels;
> @@ -174,16 +175,28 @@ static inline struct jz4780_dma_dev *jz4780_dma_chan_parent(
>                             dma_device);
>  }
>
> -static inline uint32_t jz4780_dma_readl(struct jz4780_dma_dev *jzdma,
> +static inline uint32_t jz4780_dma_chn_readl(struct jz4780_dma_dev *jzdma,
> +       unsigned int chn, unsigned int reg)
> +{
> +       return readl(jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn));
> +}
> +
> +static inline void jz4780_dma_chn_writel(struct jz4780_dma_dev *jzdma,
> +       unsigned int chn, unsigned int reg, uint32_t val)
> +{
> +       writel(val, jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn));
> +}
> +
> +static inline uint32_t jz4780_dma_ctrl_readl(struct jz4780_dma_dev *jzdma,
>         unsigned int reg)
>  {
> -       return readl(jzdma->base + reg);
> +       return readl(jzdma->ctrl_base + reg);
>  }
>
> -static inline void jz4780_dma_writel(struct jz4780_dma_dev *jzdma,
> +static inline void jz4780_dma_ctrl_writel(struct jz4780_dma_dev *jzdma,
>         unsigned int reg, uint32_t val)
>  {
> -       writel(val, jzdma->base + reg);
> +       writel(val, jzdma->ctrl_base + reg);
>  }
>
>  static struct jz4780_dma_desc *jz4780_dma_desc_alloc(
> @@ -478,17 +491,18 @@ static void jz4780_dma_begin(struct jz4780_dma_chan *jzchan)
>         }
>
>         /* Use 8-word descriptors. */
> -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), JZ_DMA_DCS_DES8);
> +       jz4780_dma_chn_writel(jzdma, jzchan->id,
> +                             JZ_DMA_REG_DCS, JZ_DMA_DCS_DES8);
>
>         /* Write descriptor address and initiate descriptor fetch. */
>         desc_phys = jzchan->desc->desc_phys +
>                     (jzchan->curr_hwdesc * sizeof(*jzchan->desc->desc));
> -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DDA(jzchan->id), desc_phys);
> -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DDRS, BIT(jzchan->id));
> +       jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DDA, desc_phys);
> +       jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DDRS, BIT(jzchan->id));
>
>         /* Enable the channel. */
> -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id),
> -                         JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE);
> +       jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS,
> +                             JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE);
>  }
>
>  static void jz4780_dma_issue_pending(struct dma_chan *chan)
> @@ -514,7 +528,7 @@ static int jz4780_dma_terminate_all(struct dma_chan *chan)
>         spin_lock_irqsave(&jzchan->vchan.lock, flags);
>
>         /* Clear the DMA status and stop the transfer. */
> -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0);
> +       jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
>         if (jzchan->desc) {
>                 vchan_terminate_vdesc(&jzchan->desc->vdesc);
>                 jzchan->desc = NULL;
> @@ -563,8 +577,8 @@ static size_t jz4780_dma_desc_residue(struct jz4780_dma_chan *jzchan,
>                 residue += desc->desc[i].dtc << jzchan->transfer_shift;
>
>         if (next_sg != 0) {
> -               count = jz4780_dma_readl(jzdma,
> -                                        JZ_DMA_REG_DTC(jzchan->id));
> +               count = jz4780_dma_chn_readl(jzdma, jzchan->id,
> +                                        JZ_DMA_REG_DTC);
>                 residue += count << jzchan->transfer_shift;
>         }
>
> @@ -611,8 +625,8 @@ static void jz4780_dma_chan_irq(struct jz4780_dma_dev *jzdma,
>
>         spin_lock(&jzchan->vchan.lock);
>
> -       dcs = jz4780_dma_readl(jzdma, JZ_DMA_REG_DCS(jzchan->id));
> -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0);
> +       dcs = jz4780_dma_chn_readl(jzdma, jzchan->id, JZ_DMA_REG_DCS);
> +       jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
>
>         if (dcs & JZ_DMA_DCS_AR) {
>                 dev_warn(&jzchan->vchan.chan.dev->device,
> @@ -651,7 +665,7 @@ static irqreturn_t jz4780_dma_irq_handler(int irq, void *data)
>         uint32_t pending, dmac;
>         int i;
>
> -       pending = jz4780_dma_readl(jzdma, JZ_DMA_REG_DIRQP);
> +       pending = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DIRQP);
>
>         for (i = 0; i < jzdma->nb_channels; i++) {
>                 if (!(pending & (1<<i)))
> @@ -661,12 +675,12 @@ static irqreturn_t jz4780_dma_irq_handler(int irq, void *data)
>         }
>
>         /* Clear halt and address error status of all channels. */
> -       dmac = jz4780_dma_readl(jzdma, JZ_DMA_REG_DMAC);
> +       dmac = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DMAC);
>         dmac &= ~(JZ_DMA_DMAC_HLT | JZ_DMA_DMAC_AR);
> -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC, dmac);
> +       jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, dmac);
>
>         /* Clear interrupt pending status. */
> -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DIRQP, 0);
> +       jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DIRQP, 0);
>
>         return IRQ_HANDLED;
>  }
> @@ -804,9 +818,19 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>                 return -EINVAL;
>         }
>
> -       jzdma->base = devm_ioremap_resource(dev, res);
> -       if (IS_ERR(jzdma->base))
> -               return PTR_ERR(jzdma->base);
> +       jzdma->chn_base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(jzdma->chn_base))
> +               return PTR_ERR(jzdma->chn_base);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +       if (!res) {
> +               dev_err(dev, "failed to get I/O memory\n");
> +               return -EINVAL;
> +       }
> +
> +       jzdma->ctrl_base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(jzdma->ctrl_base))
> +               return PTR_ERR(jzdma->ctrl_base);
>
>         ret = platform_get_irq(pdev, 0);
>         if (ret < 0) {
> @@ -864,9 +888,9 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>          * Also set the FMSC bit - it increases MSC performance, so it makes
>          * little sense not to enable it.
>          */
> -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC,
> +       jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC,
>                           JZ_DMA_DMAC_DMAE | JZ_DMA_DMAC_FMSC);
> -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DMACP, 0);
> +       jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0);
>
>         INIT_LIST_HEAD(&dd->channels);
>
> --
> 2.18.0
>
>

Regards,
PrasannaKumar

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[Paul Cercueil]

> Paul,
> 
> On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote:
>>  The register area of the JZ4780 DMA core can be split into different
>>  sections for different purposes:
>> 
>>  * one set of registers is used to perform actions at the DMA core 
>> level,
>>  that will generally affect all channels;
>> 
>>  * one set of registers per DMA channel, to perform actions at the 
>> DMA
>>  channel level, that will only affect the channel in question.
>> 
>>  The problem rises when trying to support new versions of the JZ47xx
>>  Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one
>>  with six DMA channels, and the register sets are interleaved:
>>  <DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs>
>> 
>>  By using one memory resource for the channel-specific registers and
>>  one memory resource for the core-specific registers, we can support
>>  the JZ4770, by initializing the driver once per DMA core with 
>> different
>>  addresses.
> 
> As per my understanding device tree should be modified only when
> hardware changes. This looks the other way around. It must be possible
> to achieve what you are trying to do in this patch without changing
> the device tree.

I would agree that devicetree has an ABI that we shouldn't break if
possible.

However DTS support for all the Ingenic SoCs/boards is far from being
complete, and more importantly, all Ingenic-based boards compile the DTS
file within the kernel; so breaking the ABI is not (yet) a problem, and
we should push the big changes right now while it's still possible.

>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   .../devicetree/bindings/dma/jz4780-dma.txt    |   6 +-
>>   drivers/dma/dma-jz4780.c                      | 106 
>> +++++++++++-------
>>   2 files changed, 69 insertions(+), 43 deletions(-)
>> 
>>  diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt 
>> b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
>>  index f25feee62b15..f9b1864f5b77 100644
>>  --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
>>  +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
>>  @@ -3,7 +3,8 @@
>>   Required properties:
>> 
>>   - compatible: Should be "ingenic,jz4780-dma"
>>  -- reg: Should contain the DMA controller registers location and 
>> length.
>>  +- reg: Should contain the DMA channel registers location and 
>> length, followed
>>  +  by the DMA controller registers location and length.
>>   - interrupts: Should contain the interrupt specifier of the DMA 
>> controller.
>>   - interrupt-parent: Should be the phandle of the interrupt 
>> controller that
>>   - clocks: Should contain a clock specifier for the JZ4780 PDMA 
>> clock.
>>  @@ -22,7 +23,8 @@ Example:
>> 
>>   dma: dma@13420000 {
>>          compatible = "ingenic,jz4780-dma";
>>  -       reg = <0x13420000 0x10000>;
>>  +       reg = <0x13420000 0x400
>>  +              0x13421000 0x40>;
>> 
>>          interrupt-parent = <&intc>;
>>          interrupts = <10>;
>>  diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
>>  index b40f491f0367..4d234caf5d62 100644
>>  --- a/drivers/dma/dma-jz4780.c
>>  +++ b/drivers/dma/dma-jz4780.c
>>  @@ -25,26 +25,26 @@
>>   #include "virt-dma.h"
>> 
>>   /* Global registers. */
>>  -#define JZ_DMA_REG_DMAC                0x1000
>>  -#define JZ_DMA_REG_DIRQP       0x1004
>>  -#define JZ_DMA_REG_DDR         0x1008
>>  -#define JZ_DMA_REG_DDRS                0x100c
>>  -#define JZ_DMA_REG_DMACP       0x101c
>>  -#define JZ_DMA_REG_DSIRQP      0x1020
>>  -#define JZ_DMA_REG_DSIRQM      0x1024
>>  -#define JZ_DMA_REG_DCIRQP      0x1028
>>  -#define JZ_DMA_REG_DCIRQM      0x102c
>>  +#define JZ_DMA_REG_DMAC                0x00
>>  +#define JZ_DMA_REG_DIRQP       0x04
>>  +#define JZ_DMA_REG_DDR         0x08
>>  +#define JZ_DMA_REG_DDRS                0x0c
>>  +#define JZ_DMA_REG_DMACP       0x1c
>>  +#define JZ_DMA_REG_DSIRQP      0x20
>>  +#define JZ_DMA_REG_DSIRQM      0x24
>>  +#define JZ_DMA_REG_DCIRQP      0x28
>>  +#define JZ_DMA_REG_DCIRQM      0x2c
>> 
>>   /* Per-channel registers. */
>>   #define JZ_DMA_REG_CHAN(n)     (n * 0x20)
>>  -#define JZ_DMA_REG_DSA(n)      (0x00 + JZ_DMA_REG_CHAN(n))
>>  -#define JZ_DMA_REG_DTA(n)      (0x04 + JZ_DMA_REG_CHAN(n))
>>  -#define JZ_DMA_REG_DTC(n)      (0x08 + JZ_DMA_REG_CHAN(n))
>>  -#define JZ_DMA_REG_DRT(n)      (0x0c + JZ_DMA_REG_CHAN(n))
>>  -#define JZ_DMA_REG_DCS(n)      (0x10 + JZ_DMA_REG_CHAN(n))
>>  -#define JZ_DMA_REG_DCM(n)      (0x14 + JZ_DMA_REG_CHAN(n))
>>  -#define JZ_DMA_REG_DDA(n)      (0x18 + JZ_DMA_REG_CHAN(n))
>>  -#define JZ_DMA_REG_DSD(n)      (0x1c + JZ_DMA_REG_CHAN(n))
>>  +#define JZ_DMA_REG_DSA         0x00
>>  +#define JZ_DMA_REG_DTA         0x04
>>  +#define JZ_DMA_REG_DTC         0x08
>>  +#define JZ_DMA_REG_DRT         0x0c
>>  +#define JZ_DMA_REG_DCS         0x10
>>  +#define JZ_DMA_REG_DCM         0x14
>>  +#define JZ_DMA_REG_DDA         0x18
>>  +#define JZ_DMA_REG_DSD         0x1c
>> 
>>   #define JZ_DMA_DMAC_DMAE       BIT(0)
>>   #define JZ_DMA_DMAC_AR         BIT(2)
>>  @@ -140,7 +140,8 @@ enum jz_version {
>> 
>>   struct jz4780_dma_dev {
>>          struct dma_device dma_device;
>>  -       void __iomem *base;
>>  +       void __iomem *chn_base;
>>  +       void __iomem *ctrl_base;
>>          struct clk *clk;
>>          unsigned int irq;
>>          unsigned int nb_channels;
>>  @@ -174,16 +175,28 @@ static inline struct jz4780_dma_dev 
>> *jz4780_dma_chan_parent(
>>                              dma_device);
>>   }
>> 
>>  -static inline uint32_t jz4780_dma_readl(struct jz4780_dma_dev 
>> *jzdma,
>>  +static inline uint32_t jz4780_dma_chn_readl(struct jz4780_dma_dev 
>> *jzdma,
>>  +       unsigned int chn, unsigned int reg)
>>  +{
>>  +       return readl(jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn));
>>  +}
>>  +
>>  +static inline void jz4780_dma_chn_writel(struct jz4780_dma_dev 
>> *jzdma,
>>  +       unsigned int chn, unsigned int reg, uint32_t val)
>>  +{
>>  +       writel(val, jzdma->chn_base + reg + JZ_DMA_REG_CHAN(chn));
>>  +}
>>  +
>>  +static inline uint32_t jz4780_dma_ctrl_readl(struct jz4780_dma_dev 
>> *jzdma,
>>          unsigned int reg)
>>   {
>>  -       return readl(jzdma->base + reg);
>>  +       return readl(jzdma->ctrl_base + reg);
>>   }
>> 
>>  -static inline void jz4780_dma_writel(struct jz4780_dma_dev *jzdma,
>>  +static inline void jz4780_dma_ctrl_writel(struct jz4780_dma_dev 
>> *jzdma,
>>          unsigned int reg, uint32_t val)
>>   {
>>  -       writel(val, jzdma->base + reg);
>>  +       writel(val, jzdma->ctrl_base + reg);
>>   }
>> 
>>   static struct jz4780_dma_desc *jz4780_dma_desc_alloc(
>>  @@ -478,17 +491,18 @@ static void jz4780_dma_begin(struct 
>> jz4780_dma_chan *jzchan)
>>          }
>> 
>>          /* Use 8-word descriptors. */
>>  -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 
>> JZ_DMA_DCS_DES8);
>>  +       jz4780_dma_chn_writel(jzdma, jzchan->id,
>>  +                             JZ_DMA_REG_DCS, JZ_DMA_DCS_DES8);
>> 
>>          /* Write descriptor address and initiate descriptor fetch. 
>> */
>>          desc_phys = jzchan->desc->desc_phys +
>>                      (jzchan->curr_hwdesc * 
>> sizeof(*jzchan->desc->desc));
>>  -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DDA(jzchan->id), 
>> desc_phys);
>>  -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DDRS, BIT(jzchan->id));
>>  +       jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DDA, 
>> desc_phys);
>>  +       jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DDRS, 
>> BIT(jzchan->id));
>> 
>>          /* Enable the channel. */
>>  -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id),
>>  -                         JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE);
>>  +       jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS,
>>  +                             JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE);
>>   }
>> 
>>   static void jz4780_dma_issue_pending(struct dma_chan *chan)
>>  @@ -514,7 +528,7 @@ static int jz4780_dma_terminate_all(struct 
>> dma_chan *chan)
>>          spin_lock_irqsave(&jzchan->vchan.lock, flags);
>> 
>>          /* Clear the DMA status and stop the transfer. */
>>  -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0);
>>  +       jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
>>          if (jzchan->desc) {
>>                  vchan_terminate_vdesc(&jzchan->desc->vdesc);
>>                  jzchan->desc = NULL;
>>  @@ -563,8 +577,8 @@ static size_t jz4780_dma_desc_residue(struct 
>> jz4780_dma_chan *jzchan,
>>                  residue += desc->desc[i].dtc << 
>> jzchan->transfer_shift;
>> 
>>          if (next_sg != 0) {
>>  -               count = jz4780_dma_readl(jzdma,
>>  -                                        
>> JZ_DMA_REG_DTC(jzchan->id));
>>  +               count = jz4780_dma_chn_readl(jzdma, jzchan->id,
>>  +                                        JZ_DMA_REG_DTC);
>>                  residue += count << jzchan->transfer_shift;
>>          }
>> 
>>  @@ -611,8 +625,8 @@ static void jz4780_dma_chan_irq(struct 
>> jz4780_dma_dev *jzdma,
>> 
>>          spin_lock(&jzchan->vchan.lock);
>> 
>>  -       dcs = jz4780_dma_readl(jzdma, JZ_DMA_REG_DCS(jzchan->id));
>>  -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DCS(jzchan->id), 0);
>>  +       dcs = jz4780_dma_chn_readl(jzdma, jzchan->id, 
>> JZ_DMA_REG_DCS);
>>  +       jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
>> 
>>          if (dcs & JZ_DMA_DCS_AR) {
>>                  dev_warn(&jzchan->vchan.chan.dev->device,
>>  @@ -651,7 +665,7 @@ static irqreturn_t jz4780_dma_irq_handler(int 
>> irq, void *data)
>>          uint32_t pending, dmac;
>>          int i;
>> 
>>  -       pending = jz4780_dma_readl(jzdma, JZ_DMA_REG_DIRQP);
>>  +       pending = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DIRQP);
>> 
>>          for (i = 0; i < jzdma->nb_channels; i++) {
>>                  if (!(pending & (1<<i)))
>>  @@ -661,12 +675,12 @@ static irqreturn_t jz4780_dma_irq_handler(int 
>> irq, void *data)
>>          }
>> 
>>          /* Clear halt and address error status of all channels. */
>>  -       dmac = jz4780_dma_readl(jzdma, JZ_DMA_REG_DMAC);
>>  +       dmac = jz4780_dma_ctrl_readl(jzdma, JZ_DMA_REG_DMAC);
>>          dmac &= ~(JZ_DMA_DMAC_HLT | JZ_DMA_DMAC_AR);
>>  -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC, dmac);
>>  +       jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, dmac);
>> 
>>          /* Clear interrupt pending status. */
>>  -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DIRQP, 0);
>>  +       jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DIRQP, 0);
>> 
>>          return IRQ_HANDLED;
>>   }
>>  @@ -804,9 +818,19 @@ static int jz4780_dma_probe(struct 
>> platform_device *pdev)
>>                  return -EINVAL;
>>          }
>> 
>>  -       jzdma->base = devm_ioremap_resource(dev, res);
>>  -       if (IS_ERR(jzdma->base))
>>  -               return PTR_ERR(jzdma->base);
>>  +       jzdma->chn_base = devm_ioremap_resource(dev, res);
>>  +       if (IS_ERR(jzdma->chn_base))
>>  +               return PTR_ERR(jzdma->chn_base);
>>  +
>>  +       res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>  +       if (!res) {
>>  +               dev_err(dev, "failed to get I/O memory\n");
>>  +               return -EINVAL;
>>  +       }
>>  +
>>  +       jzdma->ctrl_base = devm_ioremap_resource(dev, res);
>>  +       if (IS_ERR(jzdma->ctrl_base))
>>  +               return PTR_ERR(jzdma->ctrl_base);
>> 
>>          ret = platform_get_irq(pdev, 0);
>>          if (ret < 0) {
>>  @@ -864,9 +888,9 @@ static int jz4780_dma_probe(struct 
>> platform_device *pdev)
>>           * Also set the FMSC bit - it increases MSC performance, so 
>> it makes
>>           * little sense not to enable it.
>>           */
>>  -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DMAC,
>>  +       jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC,
>>                            JZ_DMA_DMAC_DMAE | JZ_DMA_DMAC_FMSC);
>>  -       jz4780_dma_writel(jzdma, JZ_DMA_REG_DMACP, 0);
>>  +       jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0);
>> 
>>          INIT_LIST_HEAD(&dd->channels);
>> 
>>  --
>>  2.18.0
>> 
>> 
> 
> Regards,
> PrasannaKumar

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[PrasannaKumar Muralidharan]

On 6 July 2018 at 03:15, Paul Cercueil <paul@crapouillou.net> wrote:
>
>
>> Paul,
>>
>> On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote:
>>>
>>>  The register area of the JZ4780 DMA core can be split into different
>>>  sections for different purposes:
>>>
>>>  * one set of registers is used to perform actions at the DMA core level,
>>>  that will generally affect all channels;
>>>
>>>  * one set of registers per DMA channel, to perform actions at the DMA
>>>  channel level, that will only affect the channel in question.
>>>
>>>  The problem rises when trying to support new versions of the JZ47xx
>>>  Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one
>>>  with six DMA channels, and the register sets are interleaved:
>>>  <DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs>
>>>
>>>  By using one memory resource for the channel-specific registers and
>>>  one memory resource for the core-specific registers, we can support
>>>  the JZ4770, by initializing the driver once per DMA core with different
>>>  addresses.
>>
>>
>> As per my understanding device tree should be modified only when
>> hardware changes. This looks the other way around. It must be possible
>> to achieve what you are trying to do in this patch without changing
>> the device tree.
>
>
> I would agree that devicetree has an ABI that we shouldn't break if
> possible.
>
> However DTS support for all the Ingenic SoCs/boards is far from being
> complete, and more importantly, all Ingenic-based boards compile the DTS
> file within the kernel; so breaking the ABI is not (yet) a problem, and
> we should push the big changes right now while it's still possible.

Completely agree with you in this. Let's wait and see what DT maintainer's view.

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[Vinod]

On 03-07-18, 14:32, Paul Cercueil wrote:
> The register area of the JZ4780 DMA core can be split into different
> sections for different purposes:
> 
> * one set of registers is used to perform actions at the DMA core level,
> that will generally affect all channels;
> 
> * one set of registers per DMA channel, to perform actions at the DMA
> channel level, that will only affect the channel in question.
> 
> The problem rises when trying to support new versions of the JZ47xx
> Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one
> with six DMA channels, and the register sets are interleaved:
> <DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs>
> 
> By using one memory resource for the channel-specific registers and
> one memory resource for the core-specific registers, we can support
> the JZ4770, by initializing the driver once per DMA core with different
> addresses.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  .../devicetree/bindings/dma/jz4780-dma.txt    |   6 +-

Pls move to separate patch.

>  drivers/dma/dma-jz4780.c                      | 106 +++++++++++-------
>  2 files changed, 69 insertions(+), 43 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> index f25feee62b15..f9b1864f5b77 100644
> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> @@ -3,7 +3,8 @@
>  Required properties:
>  
>  - compatible: Should be "ingenic,jz4780-dma"
> -- reg: Should contain the DMA controller registers location and length.
> +- reg: Should contain the DMA channel registers location and length, followed
> +  by the DMA controller registers location and length.
>  - interrupts: Should contain the interrupt specifier of the DMA controller.
>  - interrupt-parent: Should be the phandle of the interrupt controller that
>  - clocks: Should contain a clock specifier for the JZ4780 PDMA clock.
> @@ -22,7 +23,8 @@ Example:
>  
>  dma: dma@13420000 {
>  	compatible = "ingenic,jz4780-dma";
> -	reg = <0x13420000 0x10000>;
> +	reg = <0x13420000 0x400
> +	       0x13421000 0x40>;

Second should be optional or we break platform which may not have
updated DT..

> -	jzdma->base = devm_ioremap_resource(dev, res);
> -	if (IS_ERR(jzdma->base))
> -		return PTR_ERR(jzdma->base);
> +	jzdma->chn_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(jzdma->chn_base))
> +		return PTR_ERR(jzdma->chn_base);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res) {
> +		dev_err(dev, "failed to get I/O memory\n");
> +		return -EINVAL;
> +	}

okay and this breaks if you happen to get probed on older DT. I think DT
is treated as ABI so you need to continue support older method while
finding if DT has split resources

-- 
~Vinod

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[Paul Cercueil]

Le lun. 9 juil. 2018 à 19:03, Vinod <vkoul@kernel.org> a écrit :
> On 03-07-18, 14:32, Paul Cercueil wrote:
>>  The register area of the JZ4780 DMA core can be split into different
>>  sections for different purposes:
>> 
>>  * one set of registers is used to perform actions at the DMA core 
>> level,
>>  that will generally affect all channels;
>> 
>>  * one set of registers per DMA channel, to perform actions at the 
>> DMA
>>  channel level, that will only affect the channel in question.
>> 
>>  The problem rises when trying to support new versions of the JZ47xx
>>  Ingenic SoC. For instance, the JZ4770 has two DMA cores, each one
>>  with six DMA channels, and the register sets are interleaved:
>>  <DMA0 chan regs> <DMA1 chan regs> <DMA0 ctrl regs> <DMA1 ctrl regs>
>> 
>>  By using one memory resource for the channel-specific registers and
>>  one memory resource for the core-specific registers, we can support
>>  the JZ4770, by initializing the driver once per DMA core with 
>> different
>>  addresses.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   .../devicetree/bindings/dma/jz4780-dma.txt    |   6 +-
> 
> Pls move to separate patch.

OK.

>>   drivers/dma/dma-jz4780.c                      | 106 
>> +++++++++++-------
>>   2 files changed, 69 insertions(+), 43 deletions(-)
>> 
>>  diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt 
>> b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
>>  index f25feee62b15..f9b1864f5b77 100644
>>  --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
>>  +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
>>  @@ -3,7 +3,8 @@
>>   Required properties:
>> 
>>   - compatible: Should be "ingenic,jz4780-dma"
>>  -- reg: Should contain the DMA controller registers location and 
>> length.
>>  +- reg: Should contain the DMA channel registers location and 
>> length, followed
>>  +  by the DMA controller registers location and length.
>>   - interrupts: Should contain the interrupt specifier of the DMA 
>> controller.
>>   - interrupt-parent: Should be the phandle of the interrupt 
>> controller that
>>   - clocks: Should contain a clock specifier for the JZ4780 PDMA 
>> clock.
>>  @@ -22,7 +23,8 @@ Example:
>> 
>>   dma: dma@13420000 {
>>   	compatible = "ingenic,jz4780-dma";
>>  -	reg = <0x13420000 0x10000>;
>>  +	reg = <0x13420000 0x400
>>  +	       0x13421000 0x40>;
> 
> Second should be optional or we break platform which may not have
> updated DT..

See comment below.

>>  -	jzdma->base = devm_ioremap_resource(dev, res);
>>  -	if (IS_ERR(jzdma->base))
>>  -		return PTR_ERR(jzdma->base);
>>  +	jzdma->chn_base = devm_ioremap_resource(dev, res);
>>  +	if (IS_ERR(jzdma->chn_base))
>>  +		return PTR_ERR(jzdma->chn_base);
>>  +
>>  +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>  +	if (!res) {
>>  +		dev_err(dev, "failed to get I/O memory\n");
>>  +		return -EINVAL;
>>  +	}
> 
> okay and this breaks if you happen to get probed on older DT. I think 
> DT
> is treated as ABI so you need to continue support older method while
> finding if DT has split resources

See my response to PrasannaKumar. All the Ingenic-based boards do 
compile
the devicetree within the kernel, so I think it's still fine to add 
breaking
changes. I'll wait on @Rob to give his point of view on this, though.

(It's not something hard to change, but I'd like to know what's the 
policy
in that case. I have other DT-breaking patches to submit)

> --
> ~Vinod

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[Vinod]

On 10-07-18, 17:36, Paul Cercueil wrote:

> > >  @@ -3,7 +3,8 @@
> > >   Required properties:
> > > 
> > >   - compatible: Should be "ingenic,jz4780-dma"
> > >  -- reg: Should contain the DMA controller registers location and
> > > length.
> > >  +- reg: Should contain the DMA channel registers location and
> > > length, followed
> > >  +  by the DMA controller registers location and length.
> > >   - interrupts: Should contain the interrupt specifier of the DMA
> > > controller.
> > >   - interrupt-parent: Should be the phandle of the interrupt
> > > controller that
> > >   - clocks: Should contain a clock specifier for the JZ4780 PDMA
> > > clock.
> > >  @@ -22,7 +23,8 @@ Example:
> > > 
> > >   dma: dma@13420000 {
> > >   	compatible = "ingenic,jz4780-dma";
> > >  -	reg = <0x13420000 0x10000>;
> > >  +	reg = <0x13420000 0x400
> > >  +	       0x13421000 0x40>;
> > 
> > Second should be optional or we break platform which may not have
> > updated DT..
> 
> See comment below.
> 
> > >  -	jzdma->base = devm_ioremap_resource(dev, res);
> > >  -	if (IS_ERR(jzdma->base))
> > >  -		return PTR_ERR(jzdma->base);
> > >  +	jzdma->chn_base = devm_ioremap_resource(dev, res);
> > >  +	if (IS_ERR(jzdma->chn_base))
> > >  +		return PTR_ERR(jzdma->chn_base);
> > >  +
> > >  +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > >  +	if (!res) {
> > >  +		dev_err(dev, "failed to get I/O memory\n");
> > >  +		return -EINVAL;
> > >  +	}
> > 
> > okay and this breaks if you happen to get probed on older DT. I think DT
> > is treated as ABI so you need to continue support older method while
> > finding if DT has split resources
> 
> See my response to PrasannaKumar. All the Ingenic-based boards do compile
> the devicetree within the kernel, so I think it's still fine to add breaking
> changes. I'll wait on @Rob to give his point of view on this, though.
> 
> (It's not something hard to change, but I'd like to know what's the policy
> in that case. I have other DT-breaking patches to submit)

The policy is that DT is an ABI and should not break :)

Who maintains Ingenic arch. MAINTAINERS doesn't tell me.

-- 
~Vinod

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[Paul Cercueil]

Le mer. 11 juil. 2018 à 14:16, Vinod <vkoul@kernel.org> a écrit :
> On 10-07-18, 17:36, Paul Cercueil wrote:
> 
>>  > >  @@ -3,7 +3,8 @@
>>  > >   Required properties:
>>  > >
>>  > >   - compatible: Should be "ingenic,jz4780-dma"
>>  > >  -- reg: Should contain the DMA controller registers location 
>> and
>>  > > length.
>>  > >  +- reg: Should contain the DMA channel registers location and
>>  > > length, followed
>>  > >  +  by the DMA controller registers location and length.
>>  > >   - interrupts: Should contain the interrupt specifier of the 
>> DMA
>>  > > controller.
>>  > >   - interrupt-parent: Should be the phandle of the interrupt
>>  > > controller that
>>  > >   - clocks: Should contain a clock specifier for the JZ4780 PDMA
>>  > > clock.
>>  > >  @@ -22,7 +23,8 @@ Example:
>>  > >
>>  > >   dma: dma@13420000 {
>>  > >   	compatible = "ingenic,jz4780-dma";
>>  > >  -	reg = <0x13420000 0x10000>;
>>  > >  +	reg = <0x13420000 0x400
>>  > >  +	       0x13421000 0x40>;
>>  >
>>  > Second should be optional or we break platform which may not have
>>  > updated DT..
>> 
>>  See comment below.
>> 
>>  > >  -	jzdma->base = devm_ioremap_resource(dev, res);
>>  > >  -	if (IS_ERR(jzdma->base))
>>  > >  -		return PTR_ERR(jzdma->base);
>>  > >  +	jzdma->chn_base = devm_ioremap_resource(dev, res);
>>  > >  +	if (IS_ERR(jzdma->chn_base))
>>  > >  +		return PTR_ERR(jzdma->chn_base);
>>  > >  +
>>  > >  +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>  > >  +	if (!res) {
>>  > >  +		dev_err(dev, "failed to get I/O memory\n");
>>  > >  +		return -EINVAL;
>>  > >  +	}
>>  >
>>  > okay and this breaks if you happen to get probed on older DT. I 
>> think DT
>>  > is treated as ABI so you need to continue support older method 
>> while
>>  > finding if DT has split resources
>> 
>>  See my response to PrasannaKumar. All the Ingenic-based boards do 
>> compile
>>  the devicetree within the kernel, so I think it's still fine to add 
>> breaking
>>  changes. I'll wait on @Rob to give his point of view on this, 
>> though.
>> 
>>  (It's not something hard to change, but I'd like to know what's the 
>> policy
>>  in that case. I have other DT-breaking patches to submit)
> 
> The policy is that DT is an ABI and should not break :)
> 
> Who maintains Ingenic arch. MAINTAINERS doesn't tell me.

Unofficially that would be me :)

Otherwise that would be the MIPS maintainers, Ralf and Paul (Burton).

> --
> ~Vinod

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[Paul Burton]

Hi Vinod,

On Wed, Jul 11, 2018 at 05:46:55PM +0530, Vinod wrote:
> > > >  -	jzdma->base = devm_ioremap_resource(dev, res);
> > > >  -	if (IS_ERR(jzdma->base))
> > > >  -		return PTR_ERR(jzdma->base);
> > > >  +	jzdma->chn_base = devm_ioremap_resource(dev, res);
> > > >  +	if (IS_ERR(jzdma->chn_base))
> > > >  +		return PTR_ERR(jzdma->chn_base);
> > > >  +
> > > >  +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > >  +	if (!res) {
> > > >  +		dev_err(dev, "failed to get I/O memory\n");
> > > >  +		return -EINVAL;
> > > >  +	}
> > > 
> > > okay and this breaks if you happen to get probed on older DT. I think DT
> > > is treated as ABI so you need to continue support older method while
> > > finding if DT has split resources
> > 
> > See my response to PrasannaKumar. All the Ingenic-based boards do compile
> > the devicetree within the kernel, so I think it's still fine to add breaking
> > changes. I'll wait on @Rob to give his point of view on this, though.
> > 
> > (It's not something hard to change, but I'd like to know what's the policy
> > in that case. I have other DT-breaking patches to submit)
> 
> The policy is that DT is an ABI and should not break :)

I think in general that's a good policy to have for compatibility, but
if it's known for certain that the DT for all users of a driver is
always built into the kernel then I don't see why we shouldn't feel free
to change a binding. I agree with Paul that it'd be interesting to hear
the DT binding maintainers take on this.

Thanks,
    Paul

Re: [PATCH 02/14] dmaengine: dma-jz4780: Separate chan/ctrl registers

[Rob Herring]

On Wed, Jul 11, 2018 at 04:27:15PM -0700, Paul Burton wrote:
> Hi Vinod,
> 
> On Wed, Jul 11, 2018 at 05:46:55PM +0530, Vinod wrote:
> > > > >  -	jzdma->base = devm_ioremap_resource(dev, res);
> > > > >  -	if (IS_ERR(jzdma->base))
> > > > >  -		return PTR_ERR(jzdma->base);
> > > > >  +	jzdma->chn_base = devm_ioremap_resource(dev, res);
> > > > >  +	if (IS_ERR(jzdma->chn_base))
> > > > >  +		return PTR_ERR(jzdma->chn_base);
> > > > >  +
> > > > >  +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > > > >  +	if (!res) {
> > > > >  +		dev_err(dev, "failed to get I/O memory\n");
> > > > >  +		return -EINVAL;
> > > > >  +	}
> > > > 
> > > > okay and this breaks if you happen to get probed on older DT. I think DT
> > > > is treated as ABI so you need to continue support older method while
> > > > finding if DT has split resources
> > > 
> > > See my response to PrasannaKumar. All the Ingenic-based boards do compile
> > > the devicetree within the kernel, so I think it's still fine to add breaking
> > > changes. I'll wait on @Rob to give his point of view on this, though.
> > > 
> > > (It's not something hard to change, but I'd like to know what's the policy
> > > in that case. I have other DT-breaking patches to submit)
> > 
> > The policy is that DT is an ABI and should not break :)
> 
> I think in general that's a good policy to have for compatibility, but
> if it's known for certain that the DT for all users of a driver is
> always built into the kernel then I don't see why we shouldn't feel free
> to change a binding. I agree with Paul that it'd be interesting to hear
> the DT binding maintainers take on this.

If the platform maintainers (and their users) don't care, then I don't 
have an issue with the change. It should still be an exception and not 
just any change goes. The commit message should still highlight that 
compatibility is being broken and why.

Rob

[PATCH 03/14] dmaengine: dma-jz4780: Use 4-word descriptors

[Paul Cercueil]

The only information we use in the 8-word version of the hardware DMA
descriptor that is not present in the 4-word version is the transfer
type, aka. the ID of the source or recipient device.

Since the transfer type will never change for a DMA channel in use,
we can just set it once for all in the corresponding DMA register
before starting any transfer.

This has several benefits:

* the driver will handle twice as many hardware DMA descriptors;

* the driver is closer to support the JZ4740, which only supports 4-word
  hardware DMA descriptors;

* the JZ4770 SoC needs the transfer type to be set in the corresponding
  DMA register anyway, even if 8-word descriptors are in use.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/dma/dma-jz4780.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 4d234caf5d62..cd2cd70fd843 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -93,17 +93,12 @@
  * @dtc: transfer count (number of blocks of the transfer size specified in DCM
  * to transfer) in the low 24 bits, offset of the next descriptor from the
  * descriptor base address in the upper 8 bits.
- * @sd: target/source stride difference (in stride transfer mode).
- * @drt: request type
  */
 struct jz4780_dma_hwdesc {
 	uint32_t dcm;
 	uint32_t dsa;
 	uint32_t dta;
 	uint32_t dtc;
-	uint32_t sd;
-	uint32_t drt;
-	uint32_t reserved[2];
 };
 
 /* Size of allocations for hardware descriptor blocks. */
@@ -280,7 +275,6 @@ static int jz4780_dma_setup_hwdesc(struct jz4780_dma_chan *jzchan,
 		desc->dcm = JZ_DMA_DCM_SAI;
 		desc->dsa = addr;
 		desc->dta = config->dst_addr;
-		desc->drt = jzchan->transfer_type;
 
 		width = config->dst_addr_width;
 		maxburst = config->dst_maxburst;
@@ -288,7 +282,6 @@ static int jz4780_dma_setup_hwdesc(struct jz4780_dma_chan *jzchan,
 		desc->dcm = JZ_DMA_DCM_DAI;
 		desc->dsa = config->src_addr;
 		desc->dta = addr;
-		desc->drt = jzchan->transfer_type;
 
 		width = config->src_addr_width;
 		maxburst = config->src_maxburst;
@@ -433,9 +426,10 @@ static struct dma_async_tx_descriptor *jz4780_dma_prep_dma_memcpy(
 	tsz = jz4780_dma_transfer_size(dest | src | len,
 				       &jzchan->transfer_shift);
 
+	jzchan->transfer_type = JZ_DMA_DRT_AUTO;
+
 	desc->desc[0].dsa = src;
 	desc->desc[0].dta = dest;
-	desc->desc[0].drt = JZ_DMA_DRT_AUTO;
 	desc->desc[0].dcm = JZ_DMA_DCM_TIE | JZ_DMA_DCM_SAI | JZ_DMA_DCM_DAI |
 			    tsz << JZ_DMA_DCM_TSZ_SHIFT |
 			    JZ_DMA_WIDTH_32_BIT << JZ_DMA_DCM_SP_SHIFT |
@@ -490,9 +484,12 @@ static void jz4780_dma_begin(struct jz4780_dma_chan *jzchan)
 			(jzchan->curr_hwdesc + 1) % jzchan->desc->count;
 	}
 
-	/* Use 8-word descriptors. */
-	jz4780_dma_chn_writel(jzdma, jzchan->id,
-			      JZ_DMA_REG_DCS, JZ_DMA_DCS_DES8);
+	/* Use 4-word descriptors. */
+	jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
+
+	/* Set transfer type. */
+	jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DRT,
+			      jzchan->transfer_type);
 
 	/* Write descriptor address and initiate descriptor fetch. */
 	desc_phys = jzchan->desc->desc_phys +
@@ -502,7 +499,7 @@ static void jz4780_dma_begin(struct jz4780_dma_chan *jzchan)
 
 	/* Enable the channel. */
 	jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS,
-			      JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE);
+			      JZ_DMA_DCS_CTE);
 }
 
 static void jz4780_dma_issue_pending(struct dma_chan *chan)
-- 
2.18.0

Re: [PATCH 03/14] dmaengine: dma-jz4780: Use 4-word descriptors

[PrasannaKumar Muralidharan]

Hi Paul,

On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote:
> The only information we use in the 8-word version of the hardware DMA
> descriptor that is not present in the 4-word version is the transfer
> type, aka. the ID of the source or recipient device.
>
> Since the transfer type will never change for a DMA channel in use,
> we can just set it once for all in the corresponding DMA register
> before starting any transfer.
>
> This has several benefits:
>
> * the driver will handle twice as many hardware DMA descriptors;
>
> * the driver is closer to support the JZ4740, which only supports 4-word
>   hardware DMA descriptors;
>
> * the JZ4770 SoC needs the transfer type to be set in the corresponding
>   DMA register anyway, even if 8-word descriptors are in use.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/dma/dma-jz4780.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 4d234caf5d62..cd2cd70fd843 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -93,17 +93,12 @@
>   * @dtc: transfer count (number of blocks of the transfer size specified in DCM
>   * to transfer) in the low 24 bits, offset of the next descriptor from the
>   * descriptor base address in the upper 8 bits.
> - * @sd: target/source stride difference (in stride transfer mode).
> - * @drt: request type
>   */
>  struct jz4780_dma_hwdesc {
>         uint32_t dcm;
>         uint32_t dsa;
>         uint32_t dta;
>         uint32_t dtc;
> -       uint32_t sd;
> -       uint32_t drt;
> -       uint32_t reserved[2];
>  };
>
>  /* Size of allocations for hardware descriptor blocks. */
> @@ -280,7 +275,6 @@ static int jz4780_dma_setup_hwdesc(struct jz4780_dma_chan *jzchan,
>                 desc->dcm = JZ_DMA_DCM_SAI;
>                 desc->dsa = addr;
>                 desc->dta = config->dst_addr;
> -               desc->drt = jzchan->transfer_type;
>
>                 width = config->dst_addr_width;
>                 maxburst = config->dst_maxburst;
> @@ -288,7 +282,6 @@ static int jz4780_dma_setup_hwdesc(struct jz4780_dma_chan *jzchan,
>                 desc->dcm = JZ_DMA_DCM_DAI;
>                 desc->dsa = config->src_addr;
>                 desc->dta = addr;
> -               desc->drt = jzchan->transfer_type;
>
>                 width = config->src_addr_width;
>                 maxburst = config->src_maxburst;
> @@ -433,9 +426,10 @@ static struct dma_async_tx_descriptor *jz4780_dma_prep_dma_memcpy(
>         tsz = jz4780_dma_transfer_size(dest | src | len,
>                                        &jzchan->transfer_shift);
>
> +       jzchan->transfer_type = JZ_DMA_DRT_AUTO;
> +
>         desc->desc[0].dsa = src;
>         desc->desc[0].dta = dest;
> -       desc->desc[0].drt = JZ_DMA_DRT_AUTO;
>         desc->desc[0].dcm = JZ_DMA_DCM_TIE | JZ_DMA_DCM_SAI | JZ_DMA_DCM_DAI |
>                             tsz << JZ_DMA_DCM_TSZ_SHIFT |
>                             JZ_DMA_WIDTH_32_BIT << JZ_DMA_DCM_SP_SHIFT |
> @@ -490,9 +484,12 @@ static void jz4780_dma_begin(struct jz4780_dma_chan *jzchan)
>                         (jzchan->curr_hwdesc + 1) % jzchan->desc->count;
>         }
>
> -       /* Use 8-word descriptors. */
> -       jz4780_dma_chn_writel(jzdma, jzchan->id,
> -                             JZ_DMA_REG_DCS, JZ_DMA_DCS_DES8);
> +       /* Use 4-word descriptors. */
> +       jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
> +
> +       /* Set transfer type. */
> +       jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DRT,
> +                             jzchan->transfer_type);
>
>         /* Write descriptor address and initiate descriptor fetch. */
>         desc_phys = jzchan->desc->desc_phys +
> @@ -502,7 +499,7 @@ static void jz4780_dma_begin(struct jz4780_dma_chan *jzchan)
>
>         /* Enable the channel. */
>         jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS,
> -                             JZ_DMA_DCS_DES8 | JZ_DMA_DCS_CTE);
> +                             JZ_DMA_DCS_CTE);
>  }
>
>  static void jz4780_dma_issue_pending(struct dma_chan *chan)
> --
> 2.18.0
>
>

Patch looks good to me.
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>

Regards,
PrasannaKumar

[PATCH 04/14] dmaengine: dma-jz4780: Add support for the JZ4770 SoC

[Paul Cercueil]

The JZ4770 SoC has two DMA cores, each one featuring six DMA channels.
The major change is that each channel's clock can be enabled or disabled
through register writes.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 .../devicetree/bindings/dma/jz4780-dma.txt    |  4 +-
 drivers/dma/Kconfig                           |  2 +-
 drivers/dma/dma-jz4780.c                      | 48 ++++++++++++++++---
 3 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
index f9b1864f5b77..0fd0759053be 100644
--- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
+++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
@@ -2,7 +2,9 @@
 
 Required properties:
 
-- compatible: Should be "ingenic,jz4780-dma"
+- compatible: Should be one of:
+  * ingenic,jz4780-dma
+  * ingenic,jz4770-dma
 - reg: Should contain the DMA channel registers location and length, followed
   by the DMA controller registers location and length.
 - interrupts: Should contain the interrupt specifier of the DMA controller.
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index ca1680afa20a..48d25dccedb7 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -143,7 +143,7 @@ config DMA_JZ4740
 
 config DMA_JZ4780
 	tristate "JZ4780 DMA support"
-	depends on MACH_JZ4780 || COMPILE_TEST
+	depends on MACH_JZ4780 || MACH_JZ4770 || COMPILE_TEST
 	select DMA_ENGINE
 	select DMA_VIRTUAL_CHANNELS
 	help
diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index cd2cd70fd843..7b8b2dcd119e 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -29,6 +29,9 @@
 #define JZ_DMA_REG_DIRQP	0x04
 #define JZ_DMA_REG_DDR		0x08
 #define JZ_DMA_REG_DDRS		0x0c
+#define JZ_DMA_REG_DCKE		0x10
+#define JZ_DMA_REG_DCKES	0x14
+#define JZ_DMA_REG_DCKEC	0x18
 #define JZ_DMA_REG_DMACP	0x1c
 #define JZ_DMA_REG_DSIRQP	0x20
 #define JZ_DMA_REG_DSIRQM	0x24
@@ -130,6 +133,7 @@ struct jz4780_dma_chan {
 };
 
 enum jz_version {
+	ID_JZ4770,
 	ID_JZ4780,
 };
 
@@ -194,6 +198,20 @@ static inline void jz4780_dma_ctrl_writel(struct jz4780_dma_dev *jzdma,
 	writel(val, jzdma->ctrl_base + reg);
 }
 
+static inline void jz4780_dma_chan_enable(struct jz4780_dma_dev *jzdma,
+	unsigned int chn)
+{
+	if (jzdma->version == ID_JZ4770)
+		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
+}
+
+static inline void jz4780_dma_chan_disable(struct jz4780_dma_dev *jzdma,
+	unsigned int chn)
+{
+	if (jzdma->version == ID_JZ4770)
+		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKEC, BIT(chn));
+}
+
 static struct jz4780_dma_desc *jz4780_dma_desc_alloc(
 	struct jz4780_dma_chan *jzchan, unsigned int count,
 	enum dma_transaction_type type)
@@ -228,8 +246,15 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
 	kfree(desc);
 }
 
-static uint32_t jz4780_dma_transfer_size(unsigned long val, uint32_t *shift)
+static const unsigned int jz4780_dma_ord_max[] = {
+	[ID_JZ4770] = 6,
+	[ID_JZ4780] = 7,
+};
+
+static uint32_t jz4780_dma_transfer_size(struct jz4780_dma_chan *jzchan,
+	unsigned long val, uint32_t *shift)
 {
+	struct jz4780_dma_dev *jzdma = jz4780_dma_chan_parent(jzchan);
 	int ord = ffs(val) - 1;
 
 	/*
@@ -241,8 +266,8 @@ static uint32_t jz4780_dma_transfer_size(unsigned long val, uint32_t *shift)
 	 */
 	if (ord == 3)
 		ord = 2;
-	else if (ord > 7)
-		ord = 7;
+	else if (ord > jz4780_dma_ord_max[jzdma->version])
+		ord = jz4780_dma_ord_max[jzdma->version];
 
 	*shift = ord;
 
@@ -294,7 +319,7 @@ static int jz4780_dma_setup_hwdesc(struct jz4780_dma_chan *jzchan,
 	 * divisible by the transfer size, and we must not use more than the
 	 * maximum burst specified by the user.
 	 */
-	tsz = jz4780_dma_transfer_size(addr | len | (width * maxburst),
+	tsz = jz4780_dma_transfer_size(jzchan, addr | len | (width * maxburst),
 				       &jzchan->transfer_shift);
 
 	switch (width) {
@@ -423,7 +448,7 @@ static struct dma_async_tx_descriptor *jz4780_dma_prep_dma_memcpy(
 	if (!desc)
 		return NULL;
 
-	tsz = jz4780_dma_transfer_size(dest | src | len,
+	tsz = jz4780_dma_transfer_size(jzchan, dest | src | len,
 				       &jzchan->transfer_shift);
 
 	jzchan->transfer_type = JZ_DMA_DRT_AUTO;
@@ -484,6 +509,9 @@ static void jz4780_dma_begin(struct jz4780_dma_chan *jzchan)
 			(jzchan->curr_hwdesc + 1) % jzchan->desc->count;
 	}
 
+	/* Enable the channel's clock. */
+	jz4780_dma_chan_enable(jzdma, jzchan->id);
+
 	/* Use 4-word descriptors. */
 	jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DCS, 0);
 
@@ -531,6 +559,8 @@ static int jz4780_dma_terminate_all(struct dma_chan *chan)
 		jzchan->desc = NULL;
 	}
 
+	jz4780_dma_chan_disable(jzdma, jzchan->id);
+
 	vchan_get_all_descriptors(&jzchan->vchan, &head);
 
 	spin_unlock_irqrestore(&jzchan->vchan.lock, flags);
@@ -542,8 +572,10 @@ static int jz4780_dma_terminate_all(struct dma_chan *chan)
 static void jz4780_dma_synchronize(struct dma_chan *chan)
 {
 	struct jz4780_dma_chan *jzchan = to_jz4780_dma_chan(chan);
+	struct jz4780_dma_dev *jzdma = jz4780_dma_chan_parent(jzchan);
 
 	vchan_synchronize(&jzchan->vchan);
+	jz4780_dma_chan_disable(jzdma, jzchan->id);
 }
 
 static int jz4780_dma_config(struct dma_chan *chan,
@@ -769,10 +801,12 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
 }
 
 static const unsigned int jz4780_dma_nb_channels[] = {
+	[ID_JZ4770] = 6,
 	[ID_JZ4780] = 32,
 };
 
 static const struct of_device_id jz4780_dma_dt_match[] = {
+	{ .compatible = "ingenic,jz4770-dma", .data = (void *)ID_JZ4770 },
 	{ .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
 	{},
 };
@@ -887,7 +921,9 @@ static int jz4780_dma_probe(struct platform_device *pdev)
 	 */
 	jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC,
 			  JZ_DMA_DMAC_DMAE | JZ_DMA_DMAC_FMSC);
-	jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0);
+
+	if (jzdma->version == ID_JZ4780)
+		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0);
 
 	INIT_LIST_HEAD(&dd->channels);
 
-- 
2.18.0

Re: [PATCH 04/14] dmaengine: dma-jz4780: Add support for the JZ4770 SoC

[Vinod]

On 03-07-18, 14:32, Paul Cercueil wrote:

> +static inline void jz4780_dma_chan_disable(struct jz4780_dma_dev *jzdma,
> +	unsigned int chn)
> +{
> +	if (jzdma->version == ID_JZ4770)
> +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKEC, BIT(chn));
> +}

this sounds as hardware behaviour, so why not describe as a property in
DT?

> +
>  static struct jz4780_dma_desc *jz4780_dma_desc_alloc(
>  	struct jz4780_dma_chan *jzchan, unsigned int count,
>  	enum dma_transaction_type type)
> @@ -228,8 +246,15 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
>  	kfree(desc);
>  }
>  
> -static uint32_t jz4780_dma_transfer_size(unsigned long val, uint32_t *shift)
> +static const unsigned int jz4780_dma_ord_max[] = {
> +	[ID_JZ4770] = 6,
> +	[ID_JZ4780] = 7,
> +};

So this gives the transfer length supported?
-- 
~Vinod

Re: [PATCH 04/14] dmaengine: dma-jz4780: Add support for the JZ4770 SoC

[Paul Cercueil]

Le lun. 9 juil. 2018 à 19:10, Vinod <vkoul@kernel.org> a écrit :
> On 03-07-18, 14:32, Paul Cercueil wrote:
> 
>>  +static inline void jz4780_dma_chan_disable(struct jz4780_dma_dev 
>> *jzdma,
>>  +	unsigned int chn)
>>  +{
>>  +	if (jzdma->version == ID_JZ4770)
>>  +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKEC, BIT(chn));
>>  +}
> 
> this sounds as hardware behaviour, so why not describe as a property 
> in
> DT?

See my response to your message on patch 1.

>>  +
>>   static struct jz4780_dma_desc *jz4780_dma_desc_alloc(
>>   	struct jz4780_dma_chan *jzchan, unsigned int count,
>>   	enum dma_transaction_type type)
>>  @@ -228,8 +246,15 @@ static void jz4780_dma_desc_free(struct 
>> virt_dma_desc *vdesc)
>>   	kfree(desc);
>>   }
>> 
>>  -static uint32_t jz4780_dma_transfer_size(unsigned long val, 
>> uint32_t *shift)
>>  +static const unsigned int jz4780_dma_ord_max[] = {
>>  +	[ID_JZ4770] = 6,
>>  +	[ID_JZ4780] = 7,
>>  +};
> 
> So this gives the transfer length supported?

Yes, exactly. The maximum transfer size is (1 << ord).

> --
> ~Vinod

[PATCH 05/14] dmaengine: dma-jz4780: Add support for the JZ4740 SoC

[Paul Cercueil]

The JZ4740 SoC has a single DMA core starring six DMA channels.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 Documentation/devicetree/bindings/dma/jz4780-dma.txt | 1 +
 drivers/dma/Kconfig                                  | 2 +-
 drivers/dma/dma-jz4780.c                             | 4 ++++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
index 0fd0759053be..d7ca3f925fdf 100644
--- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
+++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
@@ -5,6 +5,7 @@ Required properties:
 - compatible: Should be one of:
   * ingenic,jz4780-dma
   * ingenic,jz4770-dma
+  * ingenic,jz4740-dma
 - reg: Should contain the DMA channel registers location and length, followed
   by the DMA controller registers location and length.
 - interrupts: Should contain the interrupt specifier of the DMA controller.
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 48d25dccedb7..a935d15ec581 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -143,7 +143,7 @@ config DMA_JZ4740
 
 config DMA_JZ4780
 	tristate "JZ4780 DMA support"
-	depends on MACH_JZ4780 || MACH_JZ4770 || COMPILE_TEST
+	depends on MACH_INGENIC || COMPILE_TEST
 	select DMA_ENGINE
 	select DMA_VIRTUAL_CHANNELS
 	help
diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 7b8b2dcd119e..ccadbe61dde7 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -133,6 +133,7 @@ struct jz4780_dma_chan {
 };
 
 enum jz_version {
+	ID_JZ4740,
 	ID_JZ4770,
 	ID_JZ4780,
 };
@@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
 }
 
 static const unsigned int jz4780_dma_ord_max[] = {
+	[ID_JZ4740] = 5,
 	[ID_JZ4770] = 6,
 	[ID_JZ4780] = 7,
 };
@@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
 }
 
 static const unsigned int jz4780_dma_nb_channels[] = {
+	[ID_JZ4740] = 6,
 	[ID_JZ4770] = 6,
 	[ID_JZ4780] = 32,
 };
 
 static const struct of_device_id jz4780_dma_dt_match[] = {
+	{ .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 },
 	{ .compatible = "ingenic,jz4770-dma", .data = (void *)ID_JZ4770 },
 	{ .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
 	{},
-- 
2.18.0

Re: [PATCH 05/14] dmaengine: dma-jz4780: Add support for the JZ4740 SoC

[PrasannaKumar Muralidharan]

On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote:
> The JZ4740 SoC has a single DMA core starring six DMA channels.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  Documentation/devicetree/bindings/dma/jz4780-dma.txt | 1 +
>  drivers/dma/Kconfig                                  | 2 +-
>  drivers/dma/dma-jz4780.c                             | 4 ++++
>  3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> index 0fd0759053be..d7ca3f925fdf 100644
> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> @@ -5,6 +5,7 @@ Required properties:
>  - compatible: Should be one of:
>    * ingenic,jz4780-dma
>    * ingenic,jz4770-dma
> +  * ingenic,jz4740-dma
>  - reg: Should contain the DMA channel registers location and length, followed
>    by the DMA controller registers location and length.
>  - interrupts: Should contain the interrupt specifier of the DMA controller.
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 48d25dccedb7..a935d15ec581 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -143,7 +143,7 @@ config DMA_JZ4740
>
>  config DMA_JZ4780
>         tristate "JZ4780 DMA support"
> -       depends on MACH_JZ4780 || MACH_JZ4770 || COMPILE_TEST
> +       depends on MACH_INGENIC || COMPILE_TEST
>         select DMA_ENGINE
>         select DMA_VIRTUAL_CHANNELS
>         help
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 7b8b2dcd119e..ccadbe61dde7 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -133,6 +133,7 @@ struct jz4780_dma_chan {
>  };
>
>  enum jz_version {
> +       ID_JZ4740,
>         ID_JZ4770,
>         ID_JZ4780,
>  };
> @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
>  }
>
>  static const unsigned int jz4780_dma_ord_max[] = {
> +       [ID_JZ4740] = 5,
>         [ID_JZ4770] = 6,
>         [ID_JZ4780] = 7,
>  };
> @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
>  }
>
>  static const unsigned int jz4780_dma_nb_channels[] = {
> +       [ID_JZ4740] = 6,
>         [ID_JZ4770] = 6,
>         [ID_JZ4780] = 32,
>  };
>
>  static const struct of_device_id jz4780_dma_dt_match[] = {
> +       { .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 },
>         { .compatible = "ingenic,jz4770-dma", .data = (void *)ID_JZ4770 },
>         { .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
>         {},
> --
> 2.18.0
>
>

Patch looks good to me.
Reviewed-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>/

Re: [PATCH 05/14] dmaengine: dma-jz4780: Add support for the JZ4740 SoC

[Vinod]

On 03-07-18, 14:32, Paul Cercueil wrote:

>  enum jz_version {
> +	ID_JZ4740,
>  	ID_JZ4770,
>  	ID_JZ4780,
>  };
> @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
>  }
>  
>  static const unsigned int jz4780_dma_ord_max[] = {
> +	[ID_JZ4740] = 5,
>  	[ID_JZ4770] = 6,
>  	[ID_JZ4780] = 7,
>  };
> @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
>  }
>  
>  static const unsigned int jz4780_dma_nb_channels[] = {
> +	[ID_JZ4740] = 6,
>  	[ID_JZ4770] = 6,
>  	[ID_JZ4780] = 32,
>  };

I feel these should be done away with if we describe hardware in DT

>  
>  static const struct of_device_id jz4780_dma_dt_match[] = {
> +	{ .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 },

adding .compatible should be the only thing required, if at all for this
addition :)

-- 
~Vinod

Re: [PATCH 05/14] dmaengine: dma-jz4780: Add support for the JZ4740 SoC

[Rob Herring]

On Mon, Jul 09, 2018 at 10:42:26PM +0530, Vinod wrote:
> On 03-07-18, 14:32, Paul Cercueil wrote:
> 
> >  enum jz_version {
> > +	ID_JZ4740,
> >  	ID_JZ4770,
> >  	ID_JZ4780,
> >  };
> > @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
> >  }
> >  
> >  static const unsigned int jz4780_dma_ord_max[] = {
> > +	[ID_JZ4740] = 5,
> >  	[ID_JZ4770] = 6,
> >  	[ID_JZ4780] = 7,
> >  };
> > @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
> >  }
> >  
> >  static const unsigned int jz4780_dma_nb_channels[] = {
> > +	[ID_JZ4740] = 6,
> >  	[ID_JZ4770] = 6,
> >  	[ID_JZ4780] = 32,
> >  };
> 
> I feel these should be done away with if we describe hardware in DT

The compatible property can imply things like this.

But how this is structured is a bit strange. Normally you have a per 
compatible struct with these as elements and the compatible matching 
selects the struct.

> 
> >  
> >  static const struct of_device_id jz4780_dma_dt_match[] = {
> > +	{ .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 },
> 
> adding .compatible should be the only thing required, if at all for this
> addition :)
> 
> -- 
> ~Vinod

Re: [PATCH 05/14] dmaengine: dma-jz4780: Add support for the JZ4740 SoC

[Paul Cercueil]

Hi,

Le lun. 16 juil. 2018 à 23:33, Rob Herring <robh@kernel.org> a écrit :
> On Mon, Jul 09, 2018 at 10:42:26PM +0530, Vinod wrote:
>>  On 03-07-18, 14:32, Paul Cercueil wrote:
>> 
>>  >  enum jz_version {
>>  > +	ID_JZ4740,
>>  >  	ID_JZ4770,
>>  >  	ID_JZ4780,
>>  >  };
>>  > @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct 
>> virt_dma_desc *vdesc)
>>  >  }
>>  >
>>  >  static const unsigned int jz4780_dma_ord_max[] = {
>>  > +	[ID_JZ4740] = 5,
>>  >  	[ID_JZ4770] = 6,
>>  >  	[ID_JZ4780] = 7,
>>  >  };
>>  > @@ -801,11 +803,13 @@ static struct dma_chan 
>> *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
>>  >  }
>>  >
>>  >  static const unsigned int jz4780_dma_nb_channels[] = {
>>  > +	[ID_JZ4740] = 6,
>>  >  	[ID_JZ4770] = 6,
>>  >  	[ID_JZ4780] = 32,
>>  >  };
>> 
>>  I feel these should be done away with if we describe hardware in DT
> 
> The compatible property can imply things like this.
> 
> But how this is structured is a bit strange. Normally you have a per
> compatible struct with these as elements and the compatible matching
> selects the struct.

You're right, I'll change that.

>> 
>>  >
>>  >  static const struct of_device_id jz4780_dma_dt_match[] = {
>>  > +	{ .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 
>> },
>> 
>>  adding .compatible should be the only thing required, if at all for 
>> this
>>  addition :)
>> 
>>  --
>>  ~Vinod

Re: [PATCH 05/14] dmaengine: dma-jz4780: Add support for the JZ4740 SoC

[Vinod]

On 16-07-18, 15:33, Rob Herring wrote:
> On Mon, Jul 09, 2018 at 10:42:26PM +0530, Vinod wrote:
> > On 03-07-18, 14:32, Paul Cercueil wrote:
> > 
> > >  enum jz_version {
> > > +	ID_JZ4740,
> > >  	ID_JZ4770,
> > >  	ID_JZ4780,
> > >  };
> > > @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
> > >  }
> > >  
> > >  static const unsigned int jz4780_dma_ord_max[] = {
> > > +	[ID_JZ4740] = 5,
> > >  	[ID_JZ4770] = 6,
> > >  	[ID_JZ4780] = 7,
> > >  };
> > > @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
> > >  }
> > >  
> > >  static const unsigned int jz4780_dma_nb_channels[] = {
> > > +	[ID_JZ4740] = 6,
> > >  	[ID_JZ4770] = 6,
> > >  	[ID_JZ4780] = 32,
> > >  };
> > 
> > I feel these should be done away with if we describe hardware in DT
> 
> The compatible property can imply things like this.

So what is the general recommendation, let DT describe hardware
including version delta or use compatible to code that in driver?

Is it documented anywhere?

-- 
~Vinod

Re: [PATCH 05/14] dmaengine: dma-jz4780: Add support for the JZ4740 SoC

[Rob Herring]

On Tue, Jul 17, 2018 at 9:34 AM Vinod <vkoul@kernel.org> wrote:
>
> On 16-07-18, 15:33, Rob Herring wrote:
> > On Mon, Jul 09, 2018 at 10:42:26PM +0530, Vinod wrote:
> > > On 03-07-18, 14:32, Paul Cercueil wrote:
> > >
> > > >  enum jz_version {
> > > > + ID_JZ4740,
> > > >   ID_JZ4770,
> > > >   ID_JZ4780,
> > > >  };
> > > > @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
> > > >  }
> > > >
> > > >  static const unsigned int jz4780_dma_ord_max[] = {
> > > > + [ID_JZ4740] = 5,
> > > >   [ID_JZ4770] = 6,
> > > >   [ID_JZ4780] = 7,
> > > >  };
> > > > @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
> > > >  }
> > > >
> > > >  static const unsigned int jz4780_dma_nb_channels[] = {
> > > > + [ID_JZ4740] = 6,
> > > >   [ID_JZ4770] = 6,
> > > >   [ID_JZ4780] = 32,
> > > >  };
> > >
> > > I feel these should be done away with if we describe hardware in DT
> >
> > The compatible property can imply things like this.
>
> So what is the general recommendation, let DT describe hardware
> including version delta or use compatible to code that in driver?

Compatible is the version. Looking at the above, the version or ID
isn't even stable.

> Is it documented anywhere?

Not really. It's a judgment call generally. Maybe # of DMA channels
should be a property because that is something most controllers have.
But you really have to define the property up front, not when the 2nd
version of h/w shows up with different properties.

To start defining guidelines, a couple of things come to mind:

- Define properties for parameters that vary from board to board (for one SoC).
- You can't add new required properties to existing bindings, so the
not present default must work for all existing compatibles (or you
need per compatible driver data).
- Bugs/quirks/errata should be handled by compatible, not adding a
property. Because bugs should be fixable without a dtb update and only
a kernel update.

Rob

Re: [PATCH 05/14] dmaengine: dma-jz4780: Add support for the JZ4740 SoC

[Vinod]

On 17-07-18, 11:40, Rob Herring wrote:
> On Tue, Jul 17, 2018 at 9:34 AM Vinod <vkoul@kernel.org> wrote:
> >
> > On 16-07-18, 15:33, Rob Herring wrote:
> > > On Mon, Jul 09, 2018 at 10:42:26PM +0530, Vinod wrote:
> > > > On 03-07-18, 14:32, Paul Cercueil wrote:
> > > >
> > > > >  enum jz_version {
> > > > > + ID_JZ4740,
> > > > >   ID_JZ4770,
> > > > >   ID_JZ4780,
> > > > >  };
> > > > > @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
> > > > >  }
> > > > >
> > > > >  static const unsigned int jz4780_dma_ord_max[] = {
> > > > > + [ID_JZ4740] = 5,
> > > > >   [ID_JZ4770] = 6,
> > > > >   [ID_JZ4780] = 7,
> > > > >  };
> > > > > @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
> > > > >  }
> > > > >
> > > > >  static const unsigned int jz4780_dma_nb_channels[] = {
> > > > > + [ID_JZ4740] = 6,
> > > > >   [ID_JZ4770] = 6,
> > > > >   [ID_JZ4780] = 32,
> > > > >  };
> > > >
> > > > I feel these should be done away with if we describe hardware in DT
> > >
> > > The compatible property can imply things like this.
> >
> > So what is the general recommendation, let DT describe hardware
> > including version delta or use compatible to code that in driver?
> 
> Compatible is the version. Looking at the above, the version or ID
> isn't even stable.
> 
> > Is it documented anywhere?
> 
> Not really. It's a judgment call generally. Maybe # of DMA channels
> should be a property because that is something most controllers have.
> But you really have to define the property up front, not when the 2nd
> version of h/w shows up with different properties.
> 
> To start defining guidelines, a couple of things come to mind:
> 
> - Define properties for parameters that vary from board to board (for one SoC).
> - You can't add new required properties to existing bindings, so the
> not present default must work for all existing compatibles (or you
> need per compatible driver data).
> - Bugs/quirks/errata should be handled by compatible, not adding a
> property. Because bugs should be fixable without a dtb update and only
> a kernel update.

Sounds good to me, thanks for the guide.

-- 
~Vinod

[PATCH 06/14] dmaengine: dma-jz4780: Add support for the JZ4725B SoC

[Paul Cercueil]

The JZ4725B has one DMA core starring six DMA channels.
As for the JZ4770, each DMA channel's clock can be enabled with
a register write, the difference here being that once started, it
is not possible to turn it off.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 Documentation/devicetree/bindings/dma/jz4780-dma.txt | 1 +
 drivers/dma/dma-jz4780.c                             | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
index d7ca3f925fdf..5d302b488e88 100644
--- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
+++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
@@ -5,6 +5,7 @@ Required properties:
 - compatible: Should be one of:
   * ingenic,jz4780-dma
   * ingenic,jz4770-dma
+  * ingenic,jz4725b-dma
   * ingenic,jz4740-dma
 - reg: Should contain the DMA channel registers location and length, followed
   by the DMA controller registers location and length.
diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index ccadbe61dde7..922e4031e70e 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -134,6 +134,7 @@ struct jz4780_dma_chan {
 
 enum jz_version {
 	ID_JZ4740,
+	ID_JZ4725B,
 	ID_JZ4770,
 	ID_JZ4780,
 };
@@ -204,6 +205,8 @@ static inline void jz4780_dma_chan_enable(struct jz4780_dma_dev *jzdma,
 {
 	if (jzdma->version == ID_JZ4770)
 		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
+	else if (jzdma->version == ID_JZ4725B)
+		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKE, BIT(chn));
 }
 
 static inline void jz4780_dma_chan_disable(struct jz4780_dma_dev *jzdma,
@@ -249,6 +252,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
 
 static const unsigned int jz4780_dma_ord_max[] = {
 	[ID_JZ4740] = 5,
+	[ID_JZ4725B] = 5,
 	[ID_JZ4770] = 6,
 	[ID_JZ4780] = 7,
 };
@@ -804,12 +808,14 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
 
 static const unsigned int jz4780_dma_nb_channels[] = {
 	[ID_JZ4740] = 6,
+	[ID_JZ4725B] = 6,
 	[ID_JZ4770] = 6,
 	[ID_JZ4780] = 32,
 };
 
 static const struct of_device_id jz4780_dma_dt_match[] = {
 	{ .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 },
+	{ .compatible = "ingenic,jz4725b-dma", .data = (void *)ID_JZ4725B },
 	{ .compatible = "ingenic,jz4770-dma", .data = (void *)ID_JZ4770 },
 	{ .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
 	{},
-- 
2.18.0

Re: [PATCH 06/14] dmaengine: dma-jz4780: Add support for the JZ4725B SoC

[PrasannaKumar Muralidharan]

On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote:
> The JZ4725B has one DMA core starring six DMA channels.
> As for the JZ4770, each DMA channel's clock can be enabled with
> a register write, the difference here being that once started, it
> is not possible to turn it off.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  Documentation/devicetree/bindings/dma/jz4780-dma.txt | 1 +
>  drivers/dma/dma-jz4780.c                             | 6 ++++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> index d7ca3f925fdf..5d302b488e88 100644
> --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt
> @@ -5,6 +5,7 @@ Required properties:
>  - compatible: Should be one of:
>    * ingenic,jz4780-dma
>    * ingenic,jz4770-dma
> +  * ingenic,jz4725b-dma
>    * ingenic,jz4740-dma
>  - reg: Should contain the DMA channel registers location and length, followed
>    by the DMA controller registers location and length.
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index ccadbe61dde7..922e4031e70e 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -134,6 +134,7 @@ struct jz4780_dma_chan {
>
>  enum jz_version {
>         ID_JZ4740,
> +       ID_JZ4725B,
>         ID_JZ4770,
>         ID_JZ4780,
>  };
> @@ -204,6 +205,8 @@ static inline void jz4780_dma_chan_enable(struct jz4780_dma_dev *jzdma,
>  {
>         if (jzdma->version == ID_JZ4770)
>                 jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
> +       else if (jzdma->version == ID_JZ4725B)
> +               jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKE, BIT(chn));
>  }
>
>  static inline void jz4780_dma_chan_disable(struct jz4780_dma_dev *jzdma,
> @@ -249,6 +252,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc)
>
>  static const unsigned int jz4780_dma_ord_max[] = {
>         [ID_JZ4740] = 5,
> +       [ID_JZ4725B] = 5,
>         [ID_JZ4770] = 6,
>         [ID_JZ4780] = 7,
>  };
> @@ -804,12 +808,14 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec,
>
>  static const unsigned int jz4780_dma_nb_channels[] = {
>         [ID_JZ4740] = 6,
> +       [ID_JZ4725B] = 6,
>         [ID_JZ4770] = 6,
>         [ID_JZ4780] = 32,
>  };
>
>  static const struct of_device_id jz4780_dma_dt_match[] = {
>         { .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 },
> +       { .compatible = "ingenic,jz4725b-dma", .data = (void *)ID_JZ4725B },
>         { .compatible = "ingenic,jz4770-dma", .data = (void *)ID_JZ4770 },
>         { .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 },
>         {},
> --
> 2.18.0
>
>

Reviewed-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>

Re: [PATCH 06/14] dmaengine: dma-jz4780: Add support for the JZ4725B SoC

[Vinod]

On 03-07-18, 14:32, Paul Cercueil wrote:
> The JZ4725B has one DMA core starring six DMA channels.
> As for the JZ4770, each DMA channel's clock can be enabled with
> a register write, the difference here being that once started, it
> is not possible to turn it off.

ok so disable for this, right..

> @@ -204,6 +205,8 @@ static inline void jz4780_dma_chan_enable(struct jz4780_dma_dev *jzdma,
>  {
>  	if (jzdma->version == ID_JZ4770)
>  		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
> +	else if (jzdma->version == ID_JZ4725B)
> +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKE, BIT(chn));

but you are writing to a different register here.. 

-- 
~Vinod

Re: [PATCH 06/14] dmaengine: dma-jz4780: Add support for the JZ4725B SoC

[Paul Cercueil]

Le lun. 9 juil. 2018 à 19:14, Vinod <vkoul@kernel.org> a écrit :
> On 03-07-18, 14:32, Paul Cercueil wrote:
>>  The JZ4725B has one DMA core starring six DMA channels.
>>  As for the JZ4770, each DMA channel's clock can be enabled with
>>  a register write, the difference here being that once started, it
>>  is not possible to turn it off.
> 
> ok so disable for this, right..
> 
>>  @@ -204,6 +205,8 @@ static inline void 
>> jz4780_dma_chan_enable(struct jz4780_dma_dev *jzdma,
>>   {
>>   	if (jzdma->version == ID_JZ4770)
>>   		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
>>  +	else if (jzdma->version == ID_JZ4725B)
>>  +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKE, BIT(chn));
> 
> but you are writing to a different register here..

Yes. SoCs >= JZ4770 have the DCKE read-only register, and DCKES/DCKEC 
to set/clear bits in DCKE.
On JZ4725B, DCKE is read/write, but the zeros written are ignored (at 
least that's what the
documentation says).

> --
> ~Vinod

Thanks,
-Paul

Re: [PATCH 06/14] dmaengine: dma-jz4780: Add support for the JZ4725B SoC

[Vinod]

On 10-07-18, 17:45, Paul Cercueil wrote:
> 
> 
> Le lun. 9 juil. 2018 à 19:14, Vinod <vkoul@kernel.org> a écrit :
> > On 03-07-18, 14:32, Paul Cercueil wrote:
> > >  The JZ4725B has one DMA core starring six DMA channels.
> > >  As for the JZ4770, each DMA channel's clock can be enabled with
> > >  a register write, the difference here being that once started, it
> > >  is not possible to turn it off.
> > 
> > ok so disable for this, right..
> > 
> > >  @@ -204,6 +205,8 @@ static inline void
> > > jz4780_dma_chan_enable(struct jz4780_dma_dev *jzdma,
> > >   {
> > >   	if (jzdma->version == ID_JZ4770)
> > >   		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
> > >  +	else if (jzdma->version == ID_JZ4725B)
> > >  +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKE, BIT(chn));
> > 
> > but you are writing to a different register here..
> 
> Yes. SoCs >= JZ4770 have the DCKE read-only register, and DCKES/DCKEC to
> set/clear bits in DCKE.
> On JZ4725B, DCKE is read/write, but the zeros written are ignored (at least
> that's what the
> documentation says).

and that was not documented in the log... so i though it maybe a typo.

-- 
~Vinod

Re: [PATCH 06/14] dmaengine: dma-jz4780: Add support for the JZ4725B SoC

[Paul Cercueil]

Le mer. 11 juil. 2018 à 14:18, Vinod <vkoul@kernel.org> a écrit :
> On 10-07-18, 17:45, Paul Cercueil wrote:
>> 
>> 
>>  Le lun. 9 juil. 2018 à 19:14, Vinod <vkoul@kernel.org> a écrit :
>>  > On 03-07-18, 14:32, Paul Cercueil wrote:
>>  > >  The JZ4725B has one DMA core starring six DMA channels.
>>  > >  As for the JZ4770, each DMA channel's clock can be enabled with
>>  > >  a register write, the difference here being that once started, 
>> it
>>  > >  is not possible to turn it off.
>>  >
>>  > ok so disable for this, right..
>>  >
>>  > >  @@ -204,6 +205,8 @@ static inline void
>>  > > jz4780_dma_chan_enable(struct jz4780_dma_dev *jzdma,
>>  > >   {
>>  > >   	if (jzdma->version == ID_JZ4770)
>>  > >   		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKES, BIT(chn));
>>  > >  +	else if (jzdma->version == ID_JZ4725B)
>>  > >  +		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DCKE, BIT(chn));
>>  >
>>  > but you are writing to a different register here..
>> 
>>  Yes. SoCs >= JZ4770 have the DCKE read-only register, and 
>> DCKES/DCKEC to
>>  set/clear bits in DCKE.
>>  On JZ4725B, DCKE is read/write, but the zeros written are ignored 
>> (at least
>>  that's what the
>>  documentation says).
> 
> and that was not documented in the log... so i though it maybe a typo.

Right, I will add a comment in-code to explain that it's normal.

> --
> ~Vinod

Thanks,
-Paul

[PATCH 07/14] dmaengine: dma-jz4780: Enable Fast DMA to the AIC

[Paul Cercueil]

With the fast DMA bit set, the DMA will transfer twice as much data
per clock period to the AIC, so there is little point not to set it.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 drivers/dma/dma-jz4780.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 922e4031e70e..7ee2c121948f 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -52,6 +52,7 @@
 #define JZ_DMA_DMAC_DMAE	BIT(0)
 #define JZ_DMA_DMAC_AR		BIT(2)
 #define JZ_DMA_DMAC_HLT		BIT(3)
+#define JZ_DMA_DMAC_FAIC	BIT(27)
 #define JZ_DMA_DMAC_FMSC	BIT(31)
 
 #define JZ_DMA_DRT_AUTO		0x8
@@ -929,8 +930,8 @@ static int jz4780_dma_probe(struct platform_device *pdev)
 	 * Also set the FMSC bit - it increases MSC performance, so it makes
 	 * little sense not to enable it.
 	 */
-	jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC,
-			  JZ_DMA_DMAC_DMAE | JZ_DMA_DMAC_FMSC);
+	jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, JZ_DMA_DMAC_DMAE |
+			       JZ_DMA_DMAC_FAIC | JZ_DMA_DMAC_FMSC);
 
 	if (jzdma->version == ID_JZ4780)
 		jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0);
-- 
2.18.0

Re: [PATCH 07/14] dmaengine: dma-jz4780: Enable Fast DMA to the AIC

[PrasannaKumar Muralidharan]

On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote:
> With the fast DMA bit set, the DMA will transfer twice as much data
> per clock period to the AIC, so there is little point not to set it.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  drivers/dma/dma-jz4780.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 922e4031e70e..7ee2c121948f 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -52,6 +52,7 @@
>  #define JZ_DMA_DMAC_DMAE       BIT(0)
>  #define JZ_DMA_DMAC_AR         BIT(2)
>  #define JZ_DMA_DMAC_HLT                BIT(3)
> +#define JZ_DMA_DMAC_FAIC       BIT(27)
>  #define JZ_DMA_DMAC_FMSC       BIT(31)
>
>  #define JZ_DMA_DRT_AUTO                0x8
> @@ -929,8 +930,8 @@ static int jz4780_dma_probe(struct platform_device *pdev)
>          * Also set the FMSC bit - it increases MSC performance, so it makes
>          * little sense not to enable it.
>          */
> -       jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC,
> -                         JZ_DMA_DMAC_DMAE | JZ_DMA_DMAC_FMSC);
> +       jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMAC, JZ_DMA_DMAC_DMAE |
> +                              JZ_DMA_DMAC_FAIC | JZ_DMA_DMAC_FMSC);
>
>         if (jzdma->version == ID_JZ4780)
>                 jz4780_dma_ctrl_writel(jzdma, JZ_DMA_REG_DMACP, 0);
> --
> 2.18.0
>
>

Reviewed-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>.

[PATCH 08/14] dmaengine: dma-jz4780: Add missing residue DTC mask

[Paul Cercueil]

From: Daniel Silsby <dansilsby@gmail.com>

The 'dtc' word in jz DMA descriptors contains two fields: The
lowest 24 bits are the transfer count, and upper 8 bits are the DOA
offset to next descriptor. The upper 8 bits are now correctly masked
off when computing residue in jz4780_dma_desc_residue(). Note that
reads of the DTCn hardware reg are automatically masked this way.

Signed-off-by: Daniel Silsby <dansilsby@gmail.com>
---
 drivers/dma/dma-jz4780.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 7ee2c121948f..7b2e305e28fb 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -610,7 +610,8 @@ static size_t jz4780_dma_desc_residue(struct jz4780_dma_chan *jzchan,
 	residue = 0;
 
 	for (i = next_sg; i < desc->count; i++)
-		residue += desc->desc[i].dtc << jzchan->transfer_shift;
+		residue += (desc->desc[i].dtc & 0xffffff) <<
+			jzchan->transfer_shift;
 
 	if (next_sg != 0) {
 		count = jz4780_dma_chn_readl(jzdma, jzchan->id,
-- 
2.18.0

[PATCH 09/14] dmaengine: dma-jz4780: Simplify jz4780_dma_desc_residue()

[Paul Cercueil]

From: Daniel Silsby <dansilsby@gmail.com>

Simple cleanup, no changes to actual logic here.

Signed-off-by: Daniel Silsby <dansilsby@gmail.com>
---
 drivers/dma/dma-jz4780.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 7b2e305e28fb..adada2a3a067 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -604,22 +604,17 @@ static size_t jz4780_dma_desc_residue(struct jz4780_dma_chan *jzchan,
 	struct jz4780_dma_desc *desc, unsigned int next_sg)
 {
 	struct jz4780_dma_dev *jzdma = jz4780_dma_chan_parent(jzchan);
-	unsigned int residue, count;
+	unsigned int count = 0;
 	unsigned int i;
 
-	residue = 0;
-
 	for (i = next_sg; i < desc->count; i++)
-		residue += (desc->desc[i].dtc & 0xffffff) <<
-			jzchan->transfer_shift;
+		count += desc->desc[i].dtc & 0xffffff;
 
-	if (next_sg != 0) {
-		count = jz4780_dma_chn_readl(jzdma, jzchan->id,
+	if (next_sg != 0)
+		count += jz4780_dma_chn_readl(jzdma, jzchan->id,
 					 JZ_DMA_REG_DTC);
-		residue += count << jzchan->transfer_shift;
-	}
 
-	return residue;
+	return count << jzchan->transfer_shift;
 }
 
 static enum dma_status jz4780_dma_tx_status(struct dma_chan *chan,
-- 
2.18.0

[PATCH 10/14] dmaengine: dma-jz4780: Set DTCn register explicitly

[Paul Cercueil]

From: Daniel Silsby <dansilsby@gmail.com>

Normally, we wouldn't set the channel transfer count register directly
when using descriptor-driven transfers. However, there is no harm in
doing so, and it allows jz4780_dma_desc_residue() to report the correct
residue of an ongoing transfer, no matter when it is called.

Signed-off-by: Daniel Silsby <dansilsby@gmail.com>
---
 drivers/dma/dma-jz4780.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index adada2a3a067..64270d53ba57 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -526,6 +526,15 @@ static void jz4780_dma_begin(struct jz4780_dma_chan *jzchan)
 	jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DRT,
 			      jzchan->transfer_type);
 
+	/*
+	 * Set the transfer count. This is redundant for a descriptor-driven
+	 * transfer. However, there can be a delay between the transfer start
+	 * time and when DTCn reg contains the new transfer count. Setting
+	 * it explicitly ensures residue is computed correctly at all times.
+	 */
+	jz4780_dma_chn_writel(jzdma, jzchan->id, JZ_DMA_REG_DTC,
+				jzchan->desc->desc[jzchan->curr_hwdesc].dtc);
+
 	/* Write descriptor address and initiate descriptor fetch. */
 	desc_phys = jzchan->desc->desc_phys +
 		    (jzchan->curr_hwdesc * sizeof(*jzchan->desc->desc));
-- 
2.18.0

[PATCH 11/14] dmaengine: dma-jz4780: Further residue status fix

[Paul Cercueil]

From: Daniel Silsby <dansilsby@gmail.com>

Func jz4780_dma_desc_residue() expects the index to the next hw
descriptor as its last parameter. Caller func jz4780_dma_tx_status(),
however, applied modulus before passing it. When the current hw
descriptor was last in the list, the index passed became zero.

The resulting excess of reported residue especially caused problems
with cyclic DMA transfer clients, i.e. ALSA AIC audio output, which
rely on this for determining current DMA location within buffer.

Combined with the recent and related residue-reporting fixes, spurious
ALSA audio underruns on jz4770 hardware are now fixed.

Signed-off-by: Daniel Silsby <dansilsby@gmail.com>
---
 drivers/dma/dma-jz4780.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 64270d53ba57..690b853977b2 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -647,7 +647,7 @@ static enum dma_status jz4780_dma_tx_status(struct dma_chan *chan,
 					to_jz4780_dma_desc(vdesc), 0);
 	} else if (cookie == jzchan->desc->vdesc.tx.cookie) {
 		txstate->residue = jz4780_dma_desc_residue(jzchan, jzchan->desc,
-			  (jzchan->curr_hwdesc + 1) % jzchan->desc->count);
+					jzchan->curr_hwdesc + 1);
 	} else
 		txstate->residue = 0;
 
-- 
2.18.0

[PATCH 12/14] dmaengine: dma-jz4780: Use dma_set_residue()

[Paul Cercueil]

From: Daniel Silsby <dansilsby@gmail.com>

This is the standard method provided by dmaengine header.

Signed-off-by: Daniel Silsby <dansilsby@gmail.com>
---
 drivers/dma/dma-jz4780.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
index 690b853977b2..084b7a46a68d 100644
--- a/drivers/dma/dma-jz4780.c
+++ b/drivers/dma/dma-jz4780.c
@@ -633,6 +633,7 @@ static enum dma_status jz4780_dma_tx_status(struct dma_chan *chan,
 	struct virt_dma_desc *vdesc;
 	enum dma_status status;
 	unsigned long flags;
+	unsigned long residue = 0;
 
 	status = dma_cookie_status(chan, cookie, txstate);
 	if ((status == DMA_COMPLETE) || (txstate == NULL))
@@ -643,13 +644,13 @@ static enum dma_status jz4780_dma_tx_status(struct dma_chan *chan,
 	vdesc = vchan_find_desc(&jzchan->vchan, cookie);
 	if (vdesc) {
 		/* On the issued list, so hasn't been processed yet */
-		txstate->residue = jz4780_dma_desc_residue(jzchan,
+		residue = jz4780_dma_desc_residue(jzchan,
 					to_jz4780_dma_desc(vdesc), 0);
 	} else if (cookie == jzchan->desc->vdesc.tx.cookie) {
-		txstate->residue = jz4780_dma_desc_residue(jzchan, jzchan->desc,
+		residue = jz4780_dma_desc_residue(jzchan, jzchan->desc,
 					jzchan->curr_hwdesc + 1);
-	} else
-		txstate->residue = 0;
+	}
+	dma_set_residue(txstate, residue);
 
 	if (vdesc && jzchan->desc && vdesc == &jzchan->desc->vdesc
 	    && jzchan->desc->status & (JZ_DMA_DCS_AR | JZ_DMA_DCS_HLT))
-- 
2.18.0

[PATCH 13/14] MIPS: JZ4780: DTS: Update DMA node to match driver changes

[Paul Cercueil]

The driver now requires two memory resources to be supplied, the first
one for the channel-specific registers, the second one for the
controller-specific registers.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index aa4e8f75ff5d..ad3b1f827cf5 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -247,7 +247,8 @@
 
 	dma: dma@13420000 {
 		compatible = "ingenic,jz4780-dma";
-		reg = <0x13420000 0x10000>;
+		reg = <0x13420000 0x400
+		       0x13421000 0x40>;
 		#dma-cells = <2>;
 
 		interrupt-parent = <&intc>;
-- 
2.18.0

[PATCH 14/14] MIPS: JZ4770: DTS: Add DMA nodes

[Paul Cercueil]

Add the two devicetree nodes for the two DMA cores of the JZ4770 SoC,
disabled by default, as currently there are no clients for the DMA
driver (until the MMC driver and/or others get a devicetree node).

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 arch/mips/boot/dts/ingenic/jz4770.dtsi | 30 ++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/jz4770.dtsi b/arch/mips/boot/dts/ingenic/jz4770.dtsi
index 7c2804f3f5f1..fda17beeb08b 100644
--- a/arch/mips/boot/dts/ingenic/jz4770.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4770.dtsi
@@ -196,6 +196,36 @@
 		status = "disabled";
 	};
 
+	dmac0: jz4770-dma@13420000 {
+		compatible = "ingenic,jz4770-dma";
+		reg = <0x13420000 0xC0
+		       0x13420300 0x20>;
+
+		#dma-cells = <1>;
+
+		clocks = <&cgu JZ4770_CLK_DMA>;
+		interrupt-parent = <&intc>;
+		interrupts = <24>;
+
+		/* Disable dmac0 until we have something that uses it */
+		status = "disabled";
+	};
+
+	dmac1: jz4770-dma@13420100 {
+		compatible = "ingenic,jz4770-dma";
+		reg = <0x13420100 0xC0
+		       0x13420400 0x20>;
+
+		#dma-cells = <1>;
+
+		clocks = <&cgu JZ4770_CLK_DMA>;
+		interrupt-parent = <&intc>;
+		interrupts = <23>;
+
+		/* Disable dmac1 until we have something that uses it */
+		status = "disabled";
+	};
+
 	uhc: uhc@13430000 {
 		compatible = "generic-ohci";
 		reg = <0x13430000 0x1000>;
-- 
2.18.0

Re: [PATCH 00/14] dma-jz4780 improvements

[Mathieu Malaterre]

On Tue, Jul 3, 2018 at 2:32 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Hi,
>
> This set of patches by myself and Daniel extends the dma-jz4780 driver
> to support other SoCs (JZ4770, JZ4740, JZ4725B).
>
> Some fixes are also included, for proper residue reporting, which fixes
> errors with ALSA.
>
> Finally, the last two patches update the devicetree bindings for the
> JZ4780 SoC and add a binding for the JZ4770 SoC.
>
> This patchset was tested on JZ4780, JZ4770 and JZ4725B, but was not tested
> on the JZ4740, so it would be fantastic if somebody could test it there.

For the entire series, everything works as expected on JZ4780 (Creator
CI20), including reading from sdcard.

Tested-by: Mathieu Malaterre <malat@debian.org>

> Regards,
> -Paul
>