by Hongbo Zhang

[permalink] [raw]

Subject: Re: [PATCH v8 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes

On 08/28/2013 08:51 PM, Mark Rutland wrote:
> On Wed, Aug 28, 2013 at 07:54:01AM +0100, Hongbo Zhang wrote:
>> On 08/27/2013 07:35 PM, Mark Rutland wrote:
>>> On Tue, Aug 27, 2013 at 11:42:02AM +0100, [email protected] wrote:
>>>> From: Hongbo Zhang <[email protected]>
>>>>
>>>> Freescale QorIQ T4 and B4 introduce new 8-channel DMA engines, this patch adds
>>>> the device tree nodes for them.
>>>>
>>>> Signed-off-by: Hongbo Zhang <[email protected]>
>>>> ---
>>>> .../devicetree/bindings/powerpc/fsl/dma.txt | 66 ++++++++++++++++
>>>> arch/powerpc/boot/dts/fsl/b4si-post.dtsi | 4 +-
>>>> arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi | 81 ++++++++++++++++++++
>>>> arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi | 81 ++++++++++++++++++++
>>>> arch/powerpc/boot/dts/fsl/t4240si-post.dtsi | 4 +-
>>>> 5 files changed, 232 insertions(+), 4 deletions(-)
>>>> create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi
>>>> create mode 100644 arch/powerpc/boot/dts/fsl/elo3-dma-1.dtsi
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
>>>> index ddf17af..10fd031 100644
>>>> --- a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
>>>> +++ b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
>>>> @@ -126,6 +126,72 @@ Example:
>>>> };
>>>> };
>>>>
>>>> +** Freescale Elo3 DMA Controller
>>>> + This is EloPlus controller with 8 channels, used in Freescale Txxx and Bxxx
>>>> + series chips, such as t1040, t4240, b4860.
>>>> +
>>>> +Required properties:
>>>> +
>>>> +- compatible : must include "fsl,elo3-dma"
>>>> +- reg : <registers specifier for DMA general status reg>
>>>> +- ranges : describes the mapping between the address space of the
>>>> + DMA channels and the address space of the DMA controller
>>>> +
>>>> +- DMA channel nodes:
>>>> + - compatible : must include "fsl,eloplus-dma-channel"
>>>> + - reg : <registers specifier for channel>
>>>> + - interrupts : <interrupt specifier for DMA channel IRQ>
>>>> + - interrupt-parent : optional, if needed for interrupt mapping
>>>> +
>>>> +Example:
>>>> +dma@100300 {
>>>> + #address-cells = <1>;
>>>> + #size-cells = <1>;
>>>> + compatible = "fsl,elo3-dma";
>>>> + reg = <0x100300 0x4 0x100600 0x4>;
>>> Is that one reg entry where #size-cells=2 and #address-cells=2?
>>>
>>> That's what the binding implies (given it only describes a single reg
>>> entry).
>>>
>>> if it's two entries, we should make that explicit (both in the binding
>>> and example):
>>>
>>> reg = <0x100300 0x4>,
>>> <0x100600 0x4>;
>> Yes they are two entries, I will change it this way.
> Ok. Could you make sure you document what the two reg entries correspond
> to? That's not clear from "<registers specifier for channel>".
Yes I am sure, we have reg for DMA controller and also reg for each DMA
channel.
these two reg entries are "registers specifier for DMA general status
reg", not "registers specifier for channel"
because this is an 8-channel DMA controller, we have two general status
registers (vs. one status register for 4-chanel DMA controller previously )
>>>> + ranges = <0x0 0x100100 0x500>;
>>> If it is one reg entry then the example ranges property isn't big enough
>>> to contain the parent-bus-address.
>> They are two reg entries, so the range is big enough.
> Ok.
>
>>>> + dma-channel@0 {
>>>> + compatible = "fsl,eloplus-dma-channel";
>>>> + reg = <0x0 0x80>;
>>>> + interrupts = <28 2 0 0>;
>>>> + };
>>>> + dma-channel@80 {
>>>> + compatible = "fsl,eloplus-dma-channel";
>>>> + reg = <0x80 0x80>;
>>>> + interrupts = <29 2 0 0>;
>>>> + };
>>>> + dma-channel@100 {
>>>> + compatible = "fsl,eloplus-dma-channel";
>>>> + reg = <0x100 0x80>;
>>>> + interrupts = <30 2 0 0>;
>>>> + };
>>>> + dma-channel@180 {
>>>> + compatible = "fsl,eloplus-dma-channel";
>>>> + reg = <0x180 0x80>;
>>>> + interrupts = <31 2 0 0>;
>>>> + };
>>>> + dma-channel@300 {
>>>> + compatible = "fsl,eloplus-dma-channel";
>>>> + reg = <0x300 0x80>;
>>>> + interrupts = <76 2 0 0>;
>>>> + };
>>>> + dma-channel@380 {
>>>> + compatible = "fsl,eloplus-dma-channel";
>>>> + reg = <0x380 0x80>;
>>>> + interrupts = <77 2 0 0>;
>>>> + };
>>>> + dma-channel@400 {
>>>> + compatible = "fsl,eloplus-dma-channel";
>>>> + reg = <0x400 0x80>;
>>>> + interrupts = <78 2 0 0>;
>>>> + };
>>>> + dma-channel@480 {
>>>> + compatible = "fsl,eloplus-dma-channel";
>>>> + reg = <0x480 0x80>;
>>>> + interrupts = <79 2 0 0>;
>>>> + };
>>>> +};
>>>> +
>>>> Note on DMA channel compatible properties: The compatible property must say
>>>> "fsl,elo-dma-channel" or "fsl,eloplus-dma-channel" to be used by the Elo DMA
>>>> driver (fsldma). Any DMA channel used by fsldma cannot be used by another
>>>> diff --git a/arch/powerpc/boot/dts/fsl/b4si-post.dtsi b/arch/powerpc/boot/dts/fsl/b4si-post.dtsi
>>>> index 7399154..ea53ea1 100644
>>>> --- a/arch/powerpc/boot/dts/fsl/b4si-post.dtsi
>>>> +++ b/arch/powerpc/boot/dts/fsl/b4si-post.dtsi
>>>> @@ -223,13 +223,13 @@
>>>> reg = <0xe2000 0x1000>;
>>>> };
>>>>
>>>> -/include/ "qoriq-dma-0.dtsi"
>>>> +/include/ "elo3-dma-0.dtsi"
>>>> dma@100300 {
>>>> fsl,iommu-parent = <&pamu0>;
>>>> fsl,liodn-reg = <&guts 0x580>; /* DMA1LIODNR */
>>>> };
>>>>
>>>> -/include/ "qoriq-dma-1.dtsi"
>>>> +/include/ "elo3-dma-1.dtsi"
>>>> dma@101300 {
>>>> fsl,iommu-parent = <&pamu0>;
>>>> fsl,liodn-reg = <&guts 0x584>; /* DMA2LIODNR */
>>>> diff --git a/arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi b/arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi
>>>> new file mode 100644
>>>> index 0000000..69a3277
>>>> --- /dev/null
>>>> +++ b/arch/powerpc/boot/dts/fsl/elo3-dma-0.dtsi
>>>> @@ -0,0 +1,81 @@
>>>> +/*
>>>> + * QorIQ DMA device tree stub [ controller @ offset 0x100000 ]
>>> Copy-pasted?
>>>
>>> Presumably should be "Elo3 DMA devicetree stub", or similar?
>>>
>>> Similarly for elo3-dma-1.dtsi.
>> Yes copy-pasted, but QorIQ isn't wrong, it is name of Freescale series
>> chips.
>> To be more specific, I'd like to use "QorIQ Elo3 DMA devicetree stub"
> That sounds good to me.
>
> Cheers,
> Mark.
>

2013-09-06 15:55:34

by Scott Wood

[permalink] [raw]

Subject: Re: [PATCH v8 1/3] DMA: Freescale: revise device tree binding document

On Wed, 2013-08-28 at 13:48 +0100, Mark Rutland wrote:
> On Wed, Aug 28, 2013 at 09:18:55AM +0100, Hongbo Zhang wrote:
> > On 08/27/2013 07:25 PM, Mark Rutland wrote:
> > > On Tue, Aug 27, 2013 at 11:42:01AM +0100, [email protected] wrote:
> > >> From: Hongbo Zhang <[email protected]>
> > >>
> > >> 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 <[email protected]>
> > >> ---
> > >> .../devicetree/bindings/powerpc/fsl/dma.txt | 62 +++++++++-----------
> > >> 1 file changed, 27 insertions(+), 35 deletions(-)
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
> > >> index 2a4b4bc..ddf17af 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"
> > >> -- reg : <registers mapping for DMA general status reg>
> > >> -- ranges : Should be defined as specified in 1) to describe the
> > >> - DMA controller channels.
> > >> +- compatible : must include "fsl,elo-dma"
> > > We should list the other values that may be in the list also, unless
> > > they are really of no consequence, in which case their presence in dt is
> > > questionable.
> > Hmm. Stephen questioned here too, it seems this is a default rule.
> > Although Scott@freescale had explained our thoughts, I'd like to edit
> > this item like this:
> >
> > "must include "fsl,eloplus-dma", and a "fsl,CHIP-dma" is optional, where
> > CHIP is the processor name"
> >
> > We don't list all the chip name because we have tens of them and we
> > cannot list all of them, and it is unnecessary to list them because we
> > even don't use "fsl,CHIP-dma" in the new driver, add "fsl,CHIP-dma" here
> > just make it questionable when it presents in example and old dts files.
> >
> > I remove the examples in bracket "(mpc8349, mpc8360, etc.)" because we
> > can see the real example below.
> > I don't say" if "fsl,CHIP-dma" presents, it should be the first one, and
> > the "fsl,eloplus-dma" should be the second" because it is common rule.
> > the description language should be clear and concise too I think.
>
> Actually, you've convinced me for the form as you originally converted
> it (must include "fsl,elo-dma"), given that the other strings aren't
> used to give information anywhere and "fsl,CHIP-dma" doesn't fully
> define a valid string.

More generally, bindings should always specify "must include" rather
than "must be" for compatible, since by the nature of compatible you
could have a derivative device with more features that is compatible
with the device described in the binding, and the binding shouldn't
preclude identifying the derivative device.

-Scott