| Message ID | 1397559250-17680-6-git-send-email-a13xp0p0v88@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Vinod Koul |
| Headers | show |
On Tue, 2014-04-15 at 14:54 +0400, Alexander Popov wrote: > > Introduce a device tree binding document for the MPC512x DMA controller > > Signed-off-by: Gerhard Sittig <gsi@denx.de> > Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com> I'm not certain whether the attribution is right. Is the S-o-b appropriate when the patch is not "from" me? As I've stated before, it's OK if you pick up and extend what I provide, but please don't pretend that I wrote what you did, and don't pretend that I ACKed or passed along your submission when I didn't. This binding certainly needs further improvement to become a good one. As I've communicated in the past, I was rather ignorant "back then" when I wrote v1 and v2 of the RFC. We have learned something in the meantime. Though I admit having gone silent after several review iterations. Assumed you would pick up information that showed up several times on public lists. > --- /dev/null > +++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt > @@ -0,0 +1,51 @@ > +* Freescale MPC512x and MPC8308 DMA Controller > + > +The DMA controller in the Freescale MPC512x and MPC8308 SoCs can move > +blocks of memory contents between memory and peripherals or > +from memory to memory. > + > +Refer to the "Generic DMA Controller and DMA request bindings" in > +the dma/dma.txt file for a more detailed description of binding. > + > +* DMA controller > + > +Required properties: > +- compatible: Should be one of > + "fsl,mpc5121-dma" > + "fsl,mpc8308-dma", "fsl,mpc5121-dma" is this a duplicate? looks funny, needs a fix or is it a requirement that for MPC8308 you need to provide both compatible strings? that would be wrong, as MPC8308 certainly is not an MPC5121 a quick search reveals: the drivers/dma/mpc512x_dma.c Linux driver implementation is wrong, it should match on both strings; expecting the MPC8308 to disguise as an MPC5121 when it's not is inappropriate (and only went unnoticed because of missing bindings, I guess) > +- reg: Address and size of the DMA controller's register set > +- interrupts: Interrupt for the DMA controller. Generic interrupt client node > + is described in interrupt-controller/interrupts.txt 'interrupts' only works in combinations with 'interrupt-parent', that actual .dts files don't have the latter in the nodes is an implementation detail but not a binding's requirement and an alternative method of specifying interrupts was introduced recently, a reference to the common binding without naming one specific property name could be most appropriate > + > +Optional properties: > +- #dma-cells: The length of the DMA specifier, must be <1> since > + the DMA controller uses a fixed assignment of request lines > + per channel. Refer to dma/dma.txt for the detailed description > + of this property I'm afraid that a generic/common document does not and cannot describe the specific semantics of this provider's cells this binding should explicitly mention that the number of cells needs to be one, and that this one cell is the DMA channel (which translates to "peripheral request line"), because these assigments are fixed in hardware > + > +Example: > + > + dma0: dma@14000 { > + compatible = "fsl,mpc5121-dma"; > + reg = <0x14000 0x1800>; > + interrupts = <65 0x8>; > + #dma-cells = <1>; > + }; > + > +* DMA client the DMA provider's binding probably need not discuss client specs, a reference to the common binding should suffice if it's appropriate at all virtually yours Gerhard Sittig
Thanks for your reply, Gerhard 2014-04-17 0:44 GMT+04:00 Gerhard Sittig <gsi@denx.de>: > On Tue, 2014-04-15 at 14:54 +0400, Alexander Popov wrote: >> >> Introduce a device tree binding document for the MPC512x DMA controller >> >> Signed-off-by: Gerhard Sittig <gsi@denx.de> >> Signed-off-by: Alexander Popov <a13xp0p0v88@gmail.com> > > I'm not certain whether the attribution is right. Is the S-o-b > appropriate when the patch is not "from" me? As I've stated > before, it's OK if you pick up and extend what I provide, but > please don't pretend that I wrote what you did, Thanks. I've read the corresponding part of Documentation/SubmittingPatches once again and now I see my mistake. > and don't pretend > that I ACKed or passed along your submission when I didn't. I didn't have any malicious intent. > This binding certainly needs further improvement to become a good > one. As I've communicated in the past, I was rather ignorant > "back then" when I wrote v1 and v2 of the RFC. We have learned > something in the meantime. Though I admit having gone silent > after several review iterations. Assumed you would pick up > information that showed up several times on public lists. > >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt >> @@ -0,0 +1,51 @@ >> +* Freescale MPC512x and MPC8308 DMA Controller >> + >> +The DMA controller in the Freescale MPC512x and MPC8308 SoCs can move >> +blocks of memory contents between memory and peripherals or >> +from memory to memory. >> + >> +Refer to the "Generic DMA Controller and DMA request bindings" in >> +the dma/dma.txt file for a more detailed description of binding. >> + >> +* DMA controller >> + >> +Required properties: >> +- compatible: Should be one of >> + "fsl,mpc5121-dma" >> + "fsl,mpc8308-dma", "fsl,mpc5121-dma" > > is this a duplicate? looks funny, needs a fix > > or is it a requirement that for MPC8308 you need to provide both > compatible strings? that would be wrong, as MPC8308 certainly is > not an MPC5121 > > a quick search reveals: the drivers/dma/mpc512x_dma.c Linux > driver implementation is wrong, it should match on both strings; > expecting the MPC8308 to disguise as an MPC5121 when it's not is > inappropriate (and only went unnoticed because of missing > bindings, I guess) I can try to fix that and add a new patch to the series. >> +- reg: Address and size of the DMA controller's register set >> +- interrupts: Interrupt for the DMA controller. Generic interrupt client node >> + is described in interrupt-controller/interrupts.txt > > 'interrupts' only works in combinations with 'interrupt-parent', > that actual .dts files don't have the latter in the nodes is an > implementation detail but not a binding's requirement Excuse me, I didn't understand your point. > and an alternative method of specifying interrupts was introduced > recently, a reference to the common binding without naming one > specific property name could be most appropriate Excuse me, I haven't found such an example. >> + >> +Optional properties: >> +- #dma-cells: The length of the DMA specifier, must be <1> since >> + the DMA controller uses a fixed assignment of request lines >> + per channel. Refer to dma/dma.txt for the detailed description >> + of this property > > I'm afraid that a generic/common document does not and cannot > describe the specific semantics of this provider's cells Ok, I see. > this binding should explicitly mention that the number of cells > needs to be one, and that this one cell is the DMA channel (which > translates to "peripheral request line"), because these > assigments are fixed in hardware Ok. >> + >> +Example: >> + >> + dma0: dma@14000 { >> + compatible = "fsl,mpc5121-dma"; >> + reg = <0x14000 0x1800>; >> + interrupts = <65 0x8>; >> + #dma-cells = <1>; >> + }; >> + >> +* DMA client > > the DMA provider's binding probably need not discuss client > specs, a reference to the common binding should suffice if it's > appropriate at all Ok. Best regards, Alexander -- 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
On Fri, 2014-04-18 at 15:29 +0400, Alexander Popov wrote: > > 2014-04-17 0:44 GMT+04:00 Gerhard Sittig <gsi@denx.de>: > > On Tue, 2014-04-15 at 14:54 +0400, Alexander Popov wrote: > >> > >> +- reg: Address and size of the DMA controller's register set > >> +- interrupts: Interrupt for the DMA controller. Generic interrupt client node > >> + is described in interrupt-controller/interrupts.txt > > > > 'interrupts' only works in combinations with 'interrupt-parent', > > that actual .dts files don't have the latter in the nodes is an > > implementation detail but not a binding's requirement > Excuse me, I didn't understand your point. > > > and an alternative method of specifying interrupts was introduced > > recently, a reference to the common binding without naming one > > specific property name could be most appropriate > Excuse me, I haven't found such an example. The 'interrupts' property is not enough in itself, it always needs the 'interrupt-parent' property, too. Because the parent (the interrupt controller) often is the same for multiple interrupts, it usually gets "factored out" into a parent node in the tree, and thus often gets missed in discussions. Still the 'interrupt-parent' is strictly required for 'interrupts' to work. Splitting both properties and putting them into nodes that are rather distant from each other is just an implementation detail of .dts files. This should not be reflected in bindings. The 'interrupts-extended' property was introduced only recently. See bindings/interrupt-controller/interrupts.txt for details. Although the motivation was to reference several parents from one client node, I very much like the idea of having all of the interrupt spec within a single property. Being explicit is a good thing, especially in setups with cascades. I consider this approach an improvement in readability and maintenance. You might just want to document in the binding that interrupt specs are required (or optional), which interrupts these are (the above text could be sufficient if there is only one interrupt for this IP block), and refer to the common binding for the syntax. virtually yours Gerhard Sittig
diff --git a/Documentation/devicetree/bindings/dma/mpc512x-dma.txt b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt new file mode 100644 index 0000000..92eb0d9 --- /dev/null +++ b/Documentation/devicetree/bindings/dma/mpc512x-dma.txt @@ -0,0 +1,51 @@ +* Freescale MPC512x and MPC8308 DMA Controller + +The DMA controller in the Freescale MPC512x and MPC8308 SoCs can move +blocks of memory contents between memory and peripherals or +from memory to memory. + +Refer to the "Generic DMA Controller and DMA request bindings" in +the dma/dma.txt file for a more detailed description of binding. + +* DMA controller + +Required properties: +- compatible: Should be one of + "fsl,mpc5121-dma" + "fsl,mpc8308-dma", "fsl,mpc5121-dma" +- reg: Address and size of the DMA controller's register set +- interrupts: Interrupt for the DMA controller. Generic interrupt client node + is described in interrupt-controller/interrupts.txt + +Optional properties: +- #dma-cells: The length of the DMA specifier, must be <1> since + the DMA controller uses a fixed assignment of request lines + per channel. Refer to dma/dma.txt for the detailed description + of this property + +Example: + + dma0: dma@14000 { + compatible = "fsl,mpc5121-dma"; + reg = <0x14000 0x1800>; + interrupts = <65 0x8>; + #dma-cells = <1>; + }; + +* DMA client + +Required properties: +- dmas: List of one or more DMA request specifiers. One DMA request specifier + consists of a phandle to the DMA controller node followed by + the integer specifying the request line +- dma-names: Contains an identifier string for each DMA request specifier. + Refer to dma/dma.txt for the description of this property + +Example: + + sdhc@1500 { + compatible = "fsl,mpc5121-sdhc"; + /* ... */ + dmas = <&dma0 30>; + dma-names = "rx-tx"; + };