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

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

[Vinod Koul]

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?

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

[Vinod Koul]

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.

[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
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Vinod Koul]

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 :)

[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>/
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 },
 	{},