Patchwork [v7,1/3] DMA: Freescale: revise device tree binding document

login
register
mail settings
Submitter Hongbo Zhang
Date July 29, 2013, 10:49 a.m.
Message ID <1375094944-3343-2-git-send-email-hongbo.zhang@freescale.com>
Download mbox | patch
Permalink /patch/262712/
State Superseded
Headers show

Comments

Hongbo Zhang - July 29, 2013, 10:49 a.m.
From: Hongbo Zhang <hongbo.zhang@freescale.com>

This patch updates the discription of each type of DMA controller and its
channels, it is preparation for adding another new DMA controller binding, it
also fixes some defects of indent for text alignment at the same time.

Signed-off-by: Hongbo Zhang <hongbo.zhang@freescale.com>
---
 .../devicetree/bindings/powerpc/fsl/dma.txt        |   48 ++++++++------------
 1 file changed, 20 insertions(+), 28 deletions(-)
Scott Wood - July 29, 2013, 10:09 p.m.
On 07/29/2013 05:49:02 AM, hongbo.zhang@freescale.com wrote:
> From: Hongbo Zhang <hongbo.zhang@freescale.com>
> 
> This patch updates the discription of each type of DMA controller and  
> its
> channels, it is preparation for adding another new DMA controller  
> binding, it
> also fixes some defects of indent for text alignment at the same time.
> 
> Signed-off-by: Hongbo Zhang <hongbo.zhang@freescale.com>

ACK, but next time please do the whitespace fixes as a separate patch  
so that it's easier to see what's actually changing.

-Scott
Stephen Warren - Aug. 21, 2013, 10:33 p.m.
On 07/29/2013 04:49 AM, hongbo.zhang@freescale.com wrote:
> From: Hongbo Zhang <hongbo.zhang@freescale.com>
> 
> This patch updates the discription of each type of DMA controller and its
> channels, it is preparation for adding another new DMA controller binding, it
> also fixes some defects of indent for text alignment at the same time.

> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt

> -- compatible        : compatible list, contains 2 entries, first is
> -		 "fsl,CHIP-dma", where CHIP is the processor
> -		 (mpc8349, mpc8360, etc.) and the second is
> -		 "fsl,elo-dma"
> +- compatible        : must include "fsl,elo-dma"

Why remove the list of supported compatible values. Lately it seems that
we're moving towards listing more/all the values rather than removing
their documentation...

> -- ranges		: Should be defined as specified in 1) to describe the
> -		  DMA controller channels.
> +- ranges            : describes the mapping between the address space of the
> +                      DMA channels and the address space of the DMA controller

What is "the address space of the DMA controller". Perhaps this should
say "the CPU-visible address space" instead?
Scott Wood - Aug. 21, 2013, 10:45 p.m.
On Wed, 2013-08-21 at 16:33 -0600, Stephen Warren wrote:
> On 07/29/2013 04:49 AM, hongbo.zhang@freescale.com wrote:
> > From: Hongbo Zhang <hongbo.zhang@freescale.com>
> > 
> > This patch updates the discription of each type of DMA controller and its
> > channels, it is preparation for adding another new DMA controller binding, it
> > also fixes some defects of indent for text alignment at the same time.
> 
> > diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
> 
> > -- compatible        : compatible list, contains 2 entries, first is
> > -		 "fsl,CHIP-dma", where CHIP is the processor
> > -		 (mpc8349, mpc8360, etc.) and the second is
> > -		 "fsl,elo-dma"
> > +- compatible        : must include "fsl,elo-dma"
> 
> Why remove the list of supported compatible values. Lately it seems that
> we're moving towards listing more/all the values rather than removing
> their documentation...

Previous versions had language that required fsl,CHIP-dma for 83xx (and
maybe 85xx?) but not the new chip.  I asked for it to be consistent.
The reason that 83xx still has fsl,CHIP-dma is not because of anything
special to 83xx, but that most other chips with this device have been
converted to dtsi and it's much more of a pain to specify the specific
SoC in that context.  The existing language does not match actual device
trees when it comes to 85xx.

Plus, the exact SoC name is of dubious value for integrated devices.  It
doesn't uniquely identify the hardware because different versions of the
SoC could have different versions of the subdevice.  As such, on our
chips we've been moving away from including a compatible that specifies
the exact SoC.  If it turns out we made a mistake in naming different
versions of the device, or if there are errata, the exact SoC can still
be determined at runtime using SVR.

> > -- ranges		: Should be defined as specified in 1) to describe the
> > -		  DMA controller channels.
> > +- ranges            : describes the mapping between the address space of the
> > +                      DMA channels and the address space of the DMA controller
> 
> What is "the address space of the DMA controller". Perhaps this should
> say "the CPU-visible address space" instead?

It's translating from the addresses used in the child nodes to a CCSR
offset.  It's really just a convenience for the readability and
macro-ability of the device tree that we do this translation at all,
versus having an empty ranges and using CCSR offsets in the children.

It's not about translating between the DMA controller's view and the
CPU's view or anything like that.

-Scott
Stephen Warren - Aug. 21, 2013, 11:12 p.m.
On 08/21/2013 04:45 PM, Scott Wood wrote:
> On Wed, 2013-08-21 at 16:33 -0600, Stephen Warren wrote:
>> On 07/29/2013 04:49 AM, hongbo.zhang@freescale.com wrote:
>>> From: Hongbo Zhang <hongbo.zhang@freescale.com>
>>>
>>> This patch updates the discription of each type of DMA controller and its
>>> channels, it is preparation for adding another new DMA controller binding, it
>>> also fixes some defects of indent for text alignment at the same time.
>>
>>> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
>>
>>> -- compatible        : compatible list, contains 2 entries, first is
>>> -		 "fsl,CHIP-dma", where CHIP is the processor
>>> -		 (mpc8349, mpc8360, etc.) and the second is
>>> -		 "fsl,elo-dma"
>>> +- compatible        : must include "fsl,elo-dma"
>>
>> Why remove the list of supported compatible values. Lately it seems that
>> we're moving towards listing more/all the values rather than removing
>> their documentation...
> 
> Previous versions had language that required fsl,CHIP-dma for 83xx (and
> maybe 85xx?) but not the new chip.  I asked for it to be consistent.
> The reason that 83xx still has fsl,CHIP-dma is not because of anything
> special to 83xx, but that most other chips with this device have been
> converted to dtsi and it's much more of a pain to specify the specific
> SoC in that context.  The existing language does not match actual device
> trees when it comes to 85xx.
> 
> Plus, the exact SoC name is of dubious value for integrated devices.  It
> doesn't uniquely identify the hardware because different versions of the
> SoC could have different versions of the subdevice.  As such, on our
> chips we've been moving away from including a compatible that specifies
> the exact SoC.  If it turns out we made a mistake in naming different
> versions of the device, or if there are errata, the exact SoC can still
> be determined at runtime using SVR.

OK, if there's some alternative run-time way of enabling chip-specific
quirking, it's probably fine to remove the extra compatible values.

Now, that does rather assume that this DMA IP block will only ever be
used within SoCs that have that SVR concept, but perhaps if that's ever
not the case, we can simply go back to requiring extra compatible values
in those specific cases?
Scott Wood - Aug. 21, 2013, 11:33 p.m.
On Wed, 2013-08-21 at 17:12 -0600, Stephen Warren wrote:
> OK, if there's some alternative run-time way of enabling chip-specific
> quirking, it's probably fine to remove the extra compatible values.
> 
> Now, that does rather assume that this DMA IP block will only ever be
> used within SoCs that have that SVR concept, but perhaps if that's ever
> not the case, we can simply go back to requiring extra compatible values
> in those specific cases?

The only situation I can see where SVR would be absent is if we were to
integrate this device into an ARM chip, in which case I'd expect there
to be some equivalent way to find the SoC identification.  If the driver
knows what SoC version it expects, it will know the way that that SoC
advertises its version.

-Scott

Patch

diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
index 2a4b4bc..6e9384b 100644
--- a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
+++ b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
@@ -1,33 +1,29 @@ 
-* Freescale 83xx DMA Controller
+* Freescale DMA Controllers
 
-Freescale PowerPC 83xx have on chip general purpose DMA controllers.
+** Freescale Elo DMA Controller
+   This is a little-endian DMA controller, used in Freescale mpc83xx series
+   chips such as mpc8315, mpc8349, mpc8379 etc.
 
 Required properties:
 
-- compatible        : compatible list, contains 2 entries, first is
-		 "fsl,CHIP-dma", where CHIP is the processor
-		 (mpc8349, mpc8360, etc.) and the second is
-		 "fsl,elo-dma"
+- compatible        : must include "fsl,elo-dma"
 - reg               : <registers mapping for DMA general status reg>
-- ranges		: Should be defined as specified in 1) to describe the
-		  DMA controller channels.
+- ranges            : describes the mapping between the address space of the
+                      DMA channels and the address space of the DMA controller
 - cell-index        : controller index.  0 for controller @ 0x8100
 - interrupts        : <interrupt mapping for DMA IRQ>
 - interrupt-parent  : optional, if needed for interrupt mapping
 
-
 - DMA channel nodes:
-        - compatible        : compatible list, contains 2 entries, first is
-			 "fsl,CHIP-dma-channel", where CHIP is the processor
-			 (mpc8349, mpc8350, etc.) and the second is
-			 "fsl,elo-dma-channel". However, see note below.
+        - compatible        : must include "fsl,elo-dma-channel"
+                              However, see note below.
         - reg               : <registers mapping for channel>
         - cell-index        : dma channel index starts at 0.
 
 Optional properties:
         - interrupts        : <interrupt mapping for DMA channel IRQ>
-			  (on 83xx this is expected to be identical to
-			   the interrupts property of the parent node)
+                              (on 83xx this is expected to be identical to
+                              the interrupts property of the parent node)
         - interrupt-parent  : optional, if needed for interrupt mapping
 
 Example:
@@ -70,27 +66,23 @@  Example:
 		};
 	};
 
-* Freescale 85xx/86xx DMA Controller
-
-Freescale PowerPC 85xx/86xx have on chip general purpose DMA controllers.
+** Freescale EloPlus DMA Controller
+   This is DMA controller with extended addresses and chaining, mainly used in
+   Freescale mpc85xx/86xx, Pxxx and BSC series chips, such as mpc8540, mpc8641
+   p4080, bsc9131 etc.
 
 Required properties:
 
-- compatible        : compatible list, contains 2 entries, first is
-		 "fsl,CHIP-dma", where CHIP is the processor
-		 (mpc8540, mpc8540, etc.) and the second is
-		 "fsl,eloplus-dma"
+- compatible        : must include "fsl,eloplus-dma"
 - reg               : <registers mapping for DMA general status reg>
 - cell-index        : controller index.  0 for controller @ 0x21000,
                                          1 for controller @ 0xc000
-- ranges		: Should be defined as specified in 1) to describe the
-		  DMA controller channels.
+- ranges            : describes the mapping between the address space of the
+                      DMA channels and the address space of the DMA controller
 
 - DMA channel nodes:
-        - compatible        : compatible list, contains 2 entries, first is
-			 "fsl,CHIP-dma-channel", where CHIP is the processor
-			 (mpc8540, mpc8560, etc.) and the second is
-			 "fsl,eloplus-dma-channel". However, see note below.
+        - compatible        : must include "fsl,eloplus-dma-channel"
+                              However, see note below.
         - cell-index        : dma channel index starts at 0.
         - reg               : <registers mapping for channel>
         - interrupts        : <interrupt mapping for DMA channel IRQ>