| Message ID | 1371263570-9323-5-git-send-email-joelagnel@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Grant, Rob, Can one of you please take a look at this patch and see if you have any comments on the binding definition? Joel, Ideally the bindings are described before they are used or along with its usage. In that aspect, this patch is present too far back in the series. Can you please fix this if you get to posting another version. I think I gave the same comment on v9 as well. On 6/15/2013 8:02 AM, Joel A Fernandes wrote: > From: Matt Porter <mdp@ti.com> > > The binding definition is based on the generic DMA controller > binding. > > Joel: Droped reserved and queue DT entries from Documentation > for now from the original patch series. > > Signed-off-by: Matt Porter <mporter@ti.com> > Signed-off-by: Joel A Fernandes <joelagnel@ti.com> > --- > Documentation/devicetree/bindings/dma/ti-edma.txt | 26 +++++++++++++++++++++ > 1 file changed, 26 insertions(+) > create mode 100644 Documentation/devicetree/bindings/dma/ti-edma.txt > > diff --git a/Documentation/devicetree/bindings/dma/ti-edma.txt b/Documentation/devicetree/bindings/dma/ti-edma.txt > new file mode 100644 > index 0000000..ada0018 > --- /dev/null > +++ b/Documentation/devicetree/bindings/dma/ti-edma.txt > @@ -0,0 +1,26 @@ > +TI EDMA > + > +Required properties: > +- compatible : "ti,edma3" > +- ti,hwmods: Name of the hwmods associated to the EDMA ti,hwmods should be optional, no? hwmod is not present on DaVinci where EDMA is also used. If it is not optional then these bindings wont work there. Thanks, Sekhar
On Friday 14 June 2013 21:32:47 Joel A Fernandes wrote: > > diff --git a/Documentation/devicetree/bindings/dma/ti-edma.txt b/Documentation/devicetree/bindings/dma/ti-edma.txt > new file mode 100644 > index 0000000..ada0018 > --- /dev/null > +++ b/Documentation/devicetree/bindings/dma/ti-edma.txt > @@ -0,0 +1,26 @@ > +TI EDMA > + > +Required properties: > +- compatible : "ti,edma3" > +- ti,hwmods: Name of the hwmods associated to the EDMA > +- ti,edma-regions: Number of regions > +- ti,edma-slots: Number of slots > + > +Optional properties: > +- ti,edma-xbar-event-map: Crossbar event to channel map You need to list #dma-cells as required here, and which values are accepted by the driver (I suppose only <1>). You should also explain the format of the dma-specifier for a slave here for each possible value of #dma-cells. For each of the standard properties (reg, interrupts, dma-channels), list whether they are optional or required. Since the example has three interrupts, you should probably list how many interrupts need to be specified (minimum and maximum if the number is variable) and in what order to expect them. Arnd
Hi Sekhar, > -----Original Message----- > From: Nori, Sekhar > Sent: Monday, June 17, 2013 6:01 AM > To: Fernandes, Joel A; Grant Likely; Rob Herring > Cc: Tony Lindgren; Matt Porter; Vinod Koul; Mark Brown; Cousson, Benoit; > Russell King; Rob Landley; Andrew Morton; Jason Kridner; Koen Kooi; > Devicetree Discuss; Linux OMAP List; Linux ARM Kernel List; Linux DaVinci > Kernel List; Linux Kernel Mailing List; Linux Documentation List; Linux MMC > List; Linux SPI Devel List; Arnd Bergmann > Subject: Re: [PATCH v10 5/8] dmaengine: edma: Add TI EDMA device tree > binding > > Grant, Rob, > > Can one of you please take a look at this patch and see if you have any > comments on the binding definition? > > Joel, > > Ideally the bindings are described before they are used or along with its > usage. In that aspect, this patch is present too far back in the series. Can you > please fix this if you get to posting another version. I think I gave the same > comment on v9 as well. [Joel] Yes, sure, I will make a note to push this up the series during the next posting. Thanks, Joel
Hi Arnd, > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Monday, June 17, 2013 6:13 AM > To: Fernandes, Joel A > Cc: Tony Lindgren; Nori, Sekhar; Matt Porter; Grant Likely; Rob Herring; Vinod > Koul; Mark Brown; Cousson, Benoit; Russell King; Rob Landley; Andrew > Morton; Jason Kridner; Koen Kooi; Devicetree Discuss; Linux OMAP List; Linux > ARM Kernel List; Linux DaVinci Kernel List; Linux Kernel Mailing List; Linux > Documentation List; Linux MMC List; Linux SPI Devel List > Subject: Re: [PATCH v10 5/8] dmaengine: edma: Add TI EDMA device tree > binding > > On Friday 14 June 2013 21:32:47 Joel A Fernandes wrote: > > > > diff --git a/Documentation/devicetree/bindings/dma/ti-edma.txt > > b/Documentation/devicetree/bindings/dma/ti-edma.txt > > new file mode 100644 > > index 0000000..ada0018 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/dma/ti-edma.txt > > @@ -0,0 +1,26 @@ > > +TI EDMA > > + > > +Required properties: > > +- compatible : "ti,edma3" > > +- ti,hwmods: Name of the hwmods associated to the EDMA > > +- ti,edma-regions: Number of regions > > +- ti,edma-slots: Number of slots > > + > > +Optional properties: > > +- ti,edma-xbar-event-map: Crossbar event to channel map > > You need to list #dma-cells as required here, and which values are accepted > by the driver (I suppose only <1>). You should also explain the format of the > dma-specifier for a slave here for each possible value of #dma-cells. > > For each of the standard properties (reg, interrupts, dma-channels), list > whether they are optional or required. Since the example has three > interrupts, you should probably list how many interrupts need to be specified > (minimum and maximum if the number is variable) and in what order to > expect them. [Joel] Thanks for the suggestion, I updated it and it looks like this now: Required properties: - compatible : "ti,edma3" - ti,hwmods: Name of the hwmods associated to the EDMA - ti,edma-regions: Number of regions - ti,edma-slots: Number of slots - #dma-cells: Should be set to <1> Clients should use a single number per DMA channel request. - dma-channels: Specify total DMA channels per CC - reg: Memory map for accessing module - interrupt-parent: Interrupt controller the interrupt is routed through - interrupts: Exactly 3 interrupts need to be specified in the order: 1. Transfer completion interrupt. 2. Memory protection interrupt. 3. Error interrupt. Optional properties: - ti,edma-xbar-event-map: Crossbar event to channel map Hope this looks ok. I will respin this patch and repost it in the next series spin. Thanks, Joel
On Monday 17 June 2013, Fernandes, Joel A wrote: > [Joel] Thanks for the suggestion, I updated it and it looks like this now: > > Required properties: > - compatible : "ti,edma3" > - ti,hwmods: Name of the hwmods associated to the EDMA > - ti,edma-regions: Number of regions > - ti,edma-slots: Number of slots > - #dma-cells: Should be set to <1> > Clients should use a single number per DMA channel request. That still does not say what that number refers to. Is it a channel number, or a request line number, or something completely different? Arnd
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Monday, June 17, 2013 11:20 AM > To: Fernandes, Joel A > Cc: Tony Lindgren; Nori, Sekhar; Matt Porter; Grant Likely; Rob Herring; Vinod > Koul; Mark Brown; Cousson, Benoit; Russell King; Rob Landley; Andrew Morton; > Jason Kridner; Koen Kooi; Devicetree Discuss; Linux OMAP List; Linux ARM > Kernel List; Linux DaVinci Kernel List; Linux Kernel Mailing List; Linux > Documentation List; Linux MMC List; Linux SPI Devel List > Subject: Re: [PATCH v10 5/8] dmaengine: edma: Add TI EDMA device tree > binding > > On Monday 17 June 2013, Fernandes, Joel A wrote: > > [Joel] Thanks for the suggestion, I updated it and it looks like this now: > > > > Required properties: > > - compatible : "ti,edma3" > > - ti,hwmods: Name of the hwmods associated to the EDMA > > - ti,edma-regions: Number of regions > > - ti,edma-slots: Number of slots > > - #dma-cells: Should be set to <1> > > Clients should use a single number per DMA channel request. > > That still does not say what that number refers to. > Is it a channel number, or a request line number, or something completely > different? [Joel] Ah! Will fix, it's a channel number. Thanks, Joel
On 6/17/2013 9:10 PM, Fernandes, Joel A wrote: > Hi Arnd, > >> -----Original Message----- >> From: Arnd Bergmann [mailto:arnd@arndb.de] >> Sent: Monday, June 17, 2013 6:13 AM >> To: Fernandes, Joel A >> Cc: Tony Lindgren; Nori, Sekhar; Matt Porter; Grant Likely; Rob Herring; Vinod >> Koul; Mark Brown; Cousson, Benoit; Russell King; Rob Landley; Andrew >> Morton; Jason Kridner; Koen Kooi; Devicetree Discuss; Linux OMAP List; Linux >> ARM Kernel List; Linux DaVinci Kernel List; Linux Kernel Mailing List; Linux >> Documentation List; Linux MMC List; Linux SPI Devel List >> Subject: Re: [PATCH v10 5/8] dmaengine: edma: Add TI EDMA device tree >> binding >> >> On Friday 14 June 2013 21:32:47 Joel A Fernandes wrote: >>> >>> diff --git a/Documentation/devicetree/bindings/dma/ti-edma.txt >>> b/Documentation/devicetree/bindings/dma/ti-edma.txt >>> new file mode 100644 >>> index 0000000..ada0018 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/dma/ti-edma.txt >>> @@ -0,0 +1,26 @@ >>> +TI EDMA >>> + >>> +Required properties: >>> +- compatible : "ti,edma3" >>> +- ti,hwmods: Name of the hwmods associated to the EDMA >>> +- ti,edma-regions: Number of regions >>> +- ti,edma-slots: Number of slots >>> + >>> +Optional properties: >>> +- ti,edma-xbar-event-map: Crossbar event to channel map >> >> You need to list #dma-cells as required here, and which values are accepted >> by the driver (I suppose only <1>). You should also explain the format of the >> dma-specifier for a slave here for each possible value of #dma-cells. >> >> For each of the standard properties (reg, interrupts, dma-channels), list >> whether they are optional or required. Since the example has three >> interrupts, you should probably list how many interrupts need to be specified >> (minimum and maximum if the number is variable) and in what order to >> expect them. > > [Joel] Thanks for the suggestion, I updated it and it looks like this now: > > Required properties: > - compatible : "ti,edma3" > - ti,hwmods: Name of the hwmods associated to the EDMA I have already asked for ti,hwmods to be made optional. Please see my comment from yesterday. Thanks, Sekhar
diff --git a/Documentation/devicetree/bindings/dma/ti-edma.txt b/Documentation/devicetree/bindings/dma/ti-edma.txt new file mode 100644 index 0000000..ada0018 --- /dev/null +++ b/Documentation/devicetree/bindings/dma/ti-edma.txt @@ -0,0 +1,26 @@ +TI EDMA + +Required properties: +- compatible : "ti,edma3" +- ti,hwmods: Name of the hwmods associated to the EDMA +- ti,edma-regions: Number of regions +- ti,edma-slots: Number of slots + +Optional properties: +- ti,edma-xbar-event-map: Crossbar event to channel map + +Example: + +edma: edma@49000000 { + reg = <0x49000000 0x10000>; + interrupt-parent = <&intc>; + interrupts = <12 13 14>; + compatible = "ti,edma3"; + ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2"; + #dma-cells = <1>; + dma-channels = <64>; + ti,edma-regions = <4>; + ti,edma-slots = <256>; + ti,edma-xbar-event-map = <1 12 + 2 13>; +};