Is there a binding for IORESOURCE_DMA population?

Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

It's pretty common on ARM platforms that IORESOURCE_DMA is being used
by platform devices to tell DMA channel/request/event number (either
physical or virtual).  And then drivers simply call function
platform_get_resource() to get that.  Do we have any support for
IORESOURCE_DMA to make it migrate to device tree probe as easy as we
do for IORESOURCE_IRQ?  Or do we have to follow "dma-channel" binding
that is being used on powerpc?

I ask because I do not see too much sense to use dma-channel binding
on ARM platforms.  For specific example like imx-sdma, it has 32
physical channels which are not bonded to any specific devices.
Instead, hardware defines 48 dma events (something like virtual
channels).  Every single event is assigned to specific device by
hardware.  Whenever one device wants to do dma, it just tells its dma
request number.  sdma will find an available physical channel to
do dma.

First of all, using powerpc dma-channel binding on such dma event
seems not reflecting the hardware facts.  Secondly, the binding in
this case (below) looks silly.  Thirdly, device driver need to make
code change for simply getting a dma event number.

sdma@83fb0000 {
        #address-cells = <1>;
        #size-cells = <0>;
        compatible = "fsl,imx51-sdma", "fsl,imx35-sdma";
        reg = <0x83fb0000 0x4000>;
        interrupts = <6>;
        fsl,sdma-ram-script-name = "sdma-imx51.bin";

        dma00: dma-channel@0 {
                compatible = "fsl,sdma-channel";
                reg = <0>;
        };

        dma01: dma-channel@1 {
                compatible = "fsl,sdma-channel";
                reg = <1>;
        };

        dma02: dma-channel@2 {
                compatible = "fsl,sdma-channel";
                reg = <2>;
        };

        dma04: dma-channel@3 {
                compatible = "fsl,sdma-channel";
                reg = <3>;
        };

        ......

        dma47: dma-channel@47 {
                compatible = "fsl,sdma-channel";
                reg = <47>;
        };
};

ssi@83fcc000 { /* SSI1 */
        compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
        reg = <0x83fcc000 0x4000>;
        interrupts = <29>;
        fsl,ssi-uses-dma;
        fsl,ssi-tx-dma-channel = &dma02;
        fsl,ssi-rx-dma-channel = &dma03;
};

Should we get dt infrastructural auto-load IORESOURCE_DMA to make
the thing a lot easier?  Or did I miss anything?

-- 
Regards,
Shawn

Re: Is there a binding for IORESOURCE_DMA population?

[Tabi Timur-B04825]

On Fri, Jul 15, 2011 at 11:32 AM, Shawn Guo <shawn.guo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> First of all, using powerpc dma-channel binding on such dma event
> seems not reflecting the hardware facts.  Secondly, the binding in
> this case (below) looks silly.  Thirdly, device driver need to make
> code change for simply getting a dma event number.

Take a look at p1022ds.dts or mpc8610hpcd.dts.  We create separate DMA
channel nodes on Freescale PowerPC parts because the channels really
are independent.  On these two boards, some of the DMA channels are
used by the async DMA driver, and two of the channels are used by the
audio driver.  The compatible property is used to differentiate among
them.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Is there a binding for IORESOURCE_DMA population?

[Arnd Bergmann]

On Friday 15 July 2011 18:45:38 Tabi Timur-B04825 wrote:
> 
> On Fri, Jul 15, 2011 at 11:32 AM, Shawn Guo <shawn.guo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 
> > First of all, using powerpc dma-channel binding on such dma event
> > seems not reflecting the hardware facts.  Secondly, the binding in
> > this case (below) looks silly.  Thirdly, device driver need to make
> > code change for simply getting a dma event number.
> 
> Take a look at p1022ds.dts or mpc8610hpcd.dts.  We create separate DMA
> channel nodes on Freescale PowerPC parts because the channels really
> are independent.  On these two boards, some of the DMA channels are
> used by the async DMA driver, and two of the channels are used by the
> audio driver.

I think this is much better than using IORESOURCE_DMA fields, which
basically are only well-defined for PC-style i8237 DMA controllers
used in stuff like floppy drives and ISA sound cards.

However, we should find a way to standardize finding the dma_chan
for an platform_device based on a phandle. Maybe a little helper
that will scan a well-known property (dma-channels?) for phandles
to a device_node that is backing a struct dma_chan.

	Arnd

Re: Is there a binding for IORESOURCE_DMA population?

[Grant Likely]

On Fri, Jul 15, 2011 at 08:11:41PM +0200, Arnd Bergmann wrote:
> On Friday 15 July 2011 18:45:38 Tabi Timur-B04825 wrote:
> > 
> > On Fri, Jul 15, 2011 at 11:32 AM, Shawn Guo <shawn.guo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > 
> > > First of all, using powerpc dma-channel binding on such dma event
> > > seems not reflecting the hardware facts.  Secondly, the binding in
> > > this case (below) looks silly.  Thirdly, device driver need to make
> > > code change for simply getting a dma event number.
> > 
> > Take a look at p1022ds.dts or mpc8610hpcd.dts.  We create separate DMA
> > channel nodes on Freescale PowerPC parts because the channels really
> > are independent.  On these two boards, some of the DMA channels are
> > used by the async DMA driver, and two of the channels are used by the
> > audio driver.
> 
> I think this is much better than using IORESOURCE_DMA fields, which
> basically are only well-defined for PC-style i8237 DMA controllers
> used in stuff like floppy drives and ISA sound cards.
> 
> However, we should find a way to standardize finding the dma_chan
> for an platform_device based on a phandle. Maybe a little helper
> that will scan a well-known property (dma-channels?) for phandles
> to a device_node that is backing a struct dma_chan.

That sounds appropriate.

g.

Re: Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

On Fri, Jul 15, 2011 at 08:11:41PM +0200, Arnd Bergmann wrote:
> On Friday 15 July 2011 18:45:38 Tabi Timur-B04825 wrote:
> > 
> > On Fri, Jul 15, 2011 at 11:32 AM, Shawn Guo <shawn.guo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > 
> > > First of all, using powerpc dma-channel binding on such dma event
> > > seems not reflecting the hardware facts.  Secondly, the binding in
> > > this case (below) looks silly.  Thirdly, device driver need to make
> > > code change for simply getting a dma event number.
> > 
> > Take a look at p1022ds.dts or mpc8610hpcd.dts.  We create separate DMA
> > channel nodes on Freescale PowerPC parts because the channels really
> > are independent.  On these two boards, some of the DMA channels are
> > used by the async DMA driver, and two of the channels are used by the
> > audio driver.
> 
> I think this is much better than using IORESOURCE_DMA fields, which
> basically are only well-defined for PC-style i8237 DMA controllers
> used in stuff like floppy drives and ISA sound cards.
> 
> However, we should find a way to standardize finding the dma_chan
> for an platform_device based on a phandle. Maybe a little helper
> that will scan a well-known property (dma-channels?) for phandles
> to a device_node that is backing a struct dma_chan.
> 
If I understand this correctly, this is something I demonstrated in
the initial message.  I do not think we always have individual
device_node for each channel (struct dma_chan).  The most common
situation is all channels are backed by one device_node (dmaengine).

As I stated in the initial message, (for i.mx sdma example) the
problem is 'struct dma_chan' stands for a physical dma channel, while
all platform_device has is a virtual channel number (known as sdma
event) assigned by hardware.  This virtual channel can only be
dynamically mapped to a physical one by sdma driver.  That said there
is no way for us to specify a physical channel in device tree.  We can
only specify the virtual one there.  (I think this is kinda common,
and sdma is just one example.)

i.mx sdma has 32 physically channels and 48 virtual ones (events).
If we go the way you are suggesting, we will have to have 48 nodes
under sdma node just for giving event number, and then have
dma-channels property of device specifying the phanldles to these
event nodes belonging to it.

So we get something like below, which looks silly to me.

sdma@83fb0000 {
        #address-cells = <1>;
        #size-cells = <0>;
        compatible = "fsl,imx51-sdma", "fsl,imx35-sdma";
        reg = <0x83fb0000 0x4000>;
        interrupts = <6>;
        fsl,sdma-ram-script-name = "sdma-imx51.bin";

        dma00: dma-channel@0 {
                compatible = "fsl,sdma-channel";
                reg = <0>;
        };

        dma01: dma-channel@1 {
                compatible = "fsl,sdma-channel";
                reg = <1>;
        };

        ......

        dma47: dma-channel@47 {
                compatible = "fsl,sdma-channel";
                reg = <47>;
        };
};

ssi@83fcc000 { /* SSI1 */
        compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
        reg = <0x83fcc000 0x4000>;
        interrupts = <29>;
        fsl,ssi-uses-dma;
        dma-channels = <&dma00, &dma01>;
};

Did I state the problem clear?  Or am I missing anything?

-- 
Regards,
Shawn

Re: Is there a binding for IORESOURCE_DMA population?

[Arnd Bergmann]

On Saturday 16 July 2011 09:57:49 Shawn Guo wrote:
> If I understand this correctly, this is something I demonstrated in
> the initial message.  I do not think we always have individual
> device_node for each channel (struct dma_chan).  The most common
> situation is all channels are backed by one device_node (dmaengine).
> 
> As I stated in the initial message, (for i.mx sdma example) the
> problem is 'struct dma_chan' stands for a physical dma channel, while
> all platform_device has is a virtual channel number (known as sdma
> event) assigned by hardware.  This virtual channel can only be
> dynamically mapped to a physical one by sdma driver.  That said there
> is no way for us to specify a physical channel in device tree.  We can
> only specify the virtual one there.  (I think this is kinda common,
> and sdma is just one example.)

My assumption was that you would create a device_node for each channel
in the device tree source, but not necessarily have a platform_device
for each of them.

> i.mx sdma has 32 physically channels and 48 virtual ones (events).
> If we go the way you are suggesting, we will have to have 48 nodes
> under sdma node just for giving event number, and then have
> dma-channels property of device specifying the phanldles to these
> event nodes belonging to it.
> 
> So we get something like below, which looks silly to me.
> 
> sdma@83fb0000 {
>         #address-cells = <1>;
>         #size-cells = <0>;
>         compatible = "fsl,imx51-sdma", "fsl,imx35-sdma";
>         reg = <0x83fb0000 0x4000>;
>         interrupts = <6>;
>         fsl,sdma-ram-script-name = "sdma-imx51.bin";
> 
>         dma00: dma-channel@0 {
>                 compatible = "fsl,sdma-channel";
>                 reg = <0>;
>         };
> 
>         dma01: dma-channel@1 {
>                 compatible = "fsl,sdma-channel";
>                 reg = <1>;
>         };
> 
>         ......
> 
>         dma47: dma-channel@47 {
>                 compatible = "fsl,sdma-channel";
>                 reg = <47>;
>         };
> };
> 
> ssi@83fcc000 { /* SSI1 */
>         compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
>         reg = <0x83fcc000 0x4000>;
>         interrupts = <29>;
>         fsl,ssi-uses-dma;
>         dma-channels = <&dma00, &dma01>;
> };
> 
> Did I state the problem clear?  Or am I missing anything?

Right, this is what I had in mind. I think this is reasonable.
If you need so many channels, the device tree will be huge already,
so this doesn't add that much bloat either.

Another option would be to add a 'dma-parent' property akin to the
interrupt-parent property and then just refer to the channel
numbers within the parent dma controller.
I guess you would prefer something like that in order to keep the
device tree source more compact, right?

The only disadvantage I can see there is slighty more complex code
that is needed to find the appropriate channel, but certainly not
out of the ordinary.

	Arnd

Re: Is there a binding for IORESOURCE_DMA population?

[Tabi Timur-B04825]

Arnd Bergmann wrote:
> Right, this is what I had in mind. I think this is reasonable.
> If you need so many channels, the device tree will be huge already,
> so this doesn't add that much bloat either.

I'm not sure I agree.  It seems that specifying nodes for each DMA channel 
(virtual or physical) is meaningless because the only property in the node 
is the "reg", which is really just a cell-index in disguise.

My vote is to have a node only for the DMA engine, and let the DMA handle 
the virtual and physical channels internally.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

On Sat, Jul 16, 2011 at 02:09:46PM +0200, Arnd Bergmann wrote:
> On Saturday 16 July 2011 09:57:49 Shawn Guo wrote:
> > If I understand this correctly, this is something I demonstrated in
> > the initial message.  I do not think we always have individual
> > device_node for each channel (struct dma_chan).  The most common
> > situation is all channels are backed by one device_node (dmaengine).
> > 
> > As I stated in the initial message, (for i.mx sdma example) the
> > problem is 'struct dma_chan' stands for a physical dma channel, while
> > all platform_device has is a virtual channel number (known as sdma
> > event) assigned by hardware.  This virtual channel can only be
> > dynamically mapped to a physical one by sdma driver.  That said there
> > is no way for us to specify a physical channel in device tree.  We can
> > only specify the virtual one there.  (I think this is kinda common,
> > and sdma is just one example.)
> 
> My assumption was that you would create a device_node for each channel
> in the device tree source, but not necessarily have a platform_device
> for each of them.
> 
I did not make it clear.  But when I say platform_device I meant the
device that requests dma service (dma client).

Creating device_node for each channel with dt code will make dt and
non-dt diverged unnecessarily.  I would not do that.

> > i.mx sdma has 32 physically channels and 48 virtual ones (events).
> > If we go the way you are suggesting, we will have to have 48 nodes
> > under sdma node just for giving event number, and then have
> > dma-channels property of device specifying the phanldles to these
> > event nodes belonging to it.
> > 
> > So we get something like below, which looks silly to me.
> > 
> > sdma@83fb0000 {
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> >         compatible = "fsl,imx51-sdma", "fsl,imx35-sdma";
> >         reg = <0x83fb0000 0x4000>;
> >         interrupts = <6>;
> >         fsl,sdma-ram-script-name = "sdma-imx51.bin";
> > 
> >         dma00: dma-channel@0 {
> >                 compatible = "fsl,sdma-channel";
> >                 reg = <0>;
> >         };
> > 
> >         dma01: dma-channel@1 {
> >                 compatible = "fsl,sdma-channel";
> >                 reg = <1>;
> >         };
> > 
> >         ......
> > 
> >         dma47: dma-channel@47 {
> >                 compatible = "fsl,sdma-channel";
> >                 reg = <47>;
> >         };
> > };
> > 
> > ssi@83fcc000 { /* SSI1 */
> >         compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
> >         reg = <0x83fcc000 0x4000>;
> >         interrupts = <29>;
> >         fsl,ssi-uses-dma;
> >         dma-channels = <&dma00, &dma01>;
> > };
> > 
> > Did I state the problem clear?  Or am I missing anything?
> 
> Right, this is what I had in mind. I think this is reasonable.
> If you need so many channels, the device tree will be huge already,
> so this doesn't add that much bloat either.
> 
I think it's unreasonable not only because it's a huge list but more
importantly because it's just meaningless as the only valuable info
is the event number in 'reg' property.

> Another option would be to add a 'dma-parent' property akin to the
> interrupt-parent property and then just refer to the channel
> numbers within the parent dma controller.

Though I do not understand what you said about channel numbers within
within the parent dma controller, I do think the dma has something
in common with 'interrupt-controller', and we can has a property like
'dma-channels' under the node of devices that are dma clients to contain
the their channel/event numbers, just like property 'interrupts'
containing devices' IRQ number.

Then like that IRQ number is decoded and populated into IORESOURCE_IRQ
by device tree infrastructural code, we can also do the same into
IORESOURCE_DMA.  In that case, drivers do not need any code change for
getting dma channel/event numbers from device tree, as
platform_get_resource(pdev, IORESOURCE_DMA) still works for them.

Right now, I have the following as the solution.  I do not prefer to
it as much as I prefer to IORESOURCE_DMA solution, but I think it's
better than that huge and meaningless dma-channel node list.

ssi@83fcc000 { /* SSI1 */
	compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
	reg = <0x83fcc000 0x4000>;
	interrupts = <29>;
	fsl,dma-events = <29 28 27 26>; /* TX0 RX0 TX1 RX1 */
	fsl,ssi-uses-dma;
};

of_property_read_u32_array(pdev->dev.of_node, "fsl,dma-events",
			   dma_events, ARRAY_SIZE(dma_events));

-- 
Regards,
Shawn

Re: Is there a binding for IORESOURCE_DMA population?

[Tabi Timur-B04825]

Shawn Guo wrote:
> It's not about virtual or physical channel.  I'm asking suggestion on
> how we bind the channel number for each dma client device in device
> tree and how they will get the number from device tree when they get
> probed.

I guess I'm still confused.  Why do we need to bind the channel number? 
It's just a resource that can be owned by the driver without needing a 
device tree.

There is no value in doing this:

         dma00: dma-channel@0 {
                 compatible = "fsl,sdma-channel";
                 reg = <0>;
         };

This tells us literally nothing.

On PowerPC, we assign DMA channels to the SSI node because that mapping is 
hard-wired in the chip, and it was an easy way to assign some channels to 
the async driver and other channels to the audio driver.  Do you have the 
same problem?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

On Sat, Jul 16, 2011 at 08:55:41PM +0800, Tabi Timur-B04825 wrote:
> Arnd Bergmann wrote:
> > Right, this is what I had in mind. I think this is reasonable.
> > If you need so many channels, the device tree will be huge already,
> > so this doesn't add that much bloat either.
> 
> I'm not sure I agree.  It seems that specifying nodes for each DMA channel 
> (virtual or physical) is meaningless because the only property in the node 
> is the "reg", which is really just a cell-index in disguise.
> 
Yes, that's exactly why I do not appreciate the solution.

> My vote is to have a node only for the DMA engine, and let the DMA handle 
> the virtual and physical channels internally.
> 
It's not about virtual or physical channel.  I'm asking suggestion on
how we bind the channel number for each dma client device in device
tree and how they will get the number from device tree when they get
probed.

-- 
Regards,
Shawn

Re: Is there a binding for IORESOURCE_DMA population?

[Tabi Timur-B04825]

Shawn Guo wrote:
> ssi@83fcc000 { /* SSI1 */
> 	compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
> 	reg =<0x83fcc000 0x4000>;
> 	interrupts =<29>;
> 	fsl,dma-events =<29 28 27 26>; /* TX0 RX0 TX1 RX1 */
> 	fsl,ssi-uses-dma;
> };

This is similar to what we do on PowerPC:

             ssi@15000 {
                     compatible = "fsl,mpc8610-ssi";
                     cell-index = <0>;
                     reg = <0x15000 0x100>;
                     interrupts = <75 2 0 0>;
                     fsl,mode = "i2s-slave";
                     codec-handle = <&wm8776>;
                     fsl,playback-dma = <&dma00>;
                     fsl,capture-dma = <&dma01>;
                     fsl,fifo-depth = <15>;
                     fsl,ssi-asynchronous;
              };

Keep in mind that there already is a device-tree enabled SSI driver, so 
you need to find a way to use that driver, instead of adding device-tree 
support to the non-DT driver that's already there.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Is there a binding for IORESOURCE_DMA population?

[Arnd Bergmann]

On Saturday 16 July 2011, Shawn Guo wrote:
> Though I do not understand what you said about channel numbers within
> within the parent dma controller, I do think the dma has something
> in common with 'interrupt-controller', and we can has a property like
> 'dma-channels' under the node of devices that are dma clients to contain
> the their channel/event numbers, just like property 'interrupts'
> containing devices' IRQ number.

I think that would be ok, and it's basically what I tried to express.

> Then like that IRQ number is decoded and populated into IORESOURCE_IRQ
> by device tree infrastructural code, we can also do the same into
> IORESOURCE_DMA.  In that case, drivers do not need any code change for
> getting dma channel/event numbers from device tree, as
> platform_get_resource(pdev, IORESOURCE_DMA) still works for them.

But I really don't think there is value in using IORESOURCE_DMA for this.
We don't have the code to manage DMA resources for more than one DMA
controller AFAICT, and I think we should not add it. Globally
unique interrupt numbers cause us a lot of trouble and we go to great
lengths to fake them on modern devices. It would be much better
not to have them visible in the OS, and I don't want to add them to
a modern API like the dmaengine.

> Right now, I have the following as the solution.  I do not prefer to
> it as much as I prefer to IORESOURCE_DMA solution, but I think it's
> better than that huge and meaningless dma-channel node list.
> 
> ssi@83fcc000 { /* SSI1 */
>         compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
>         reg = <0x83fcc000 0x4000>;
>         interrupts = <29>;
>         fsl,dma-events = <29 28 27 26>; /* TX0 RX0 TX1 RX1 */
>         fsl,ssi-uses-dma;
> };
> 
> of_property_read_u32_array(pdev->dev.of_node, "fsl,dma-events",
>                            dma_events, ARRAY_SIZE(dma_events));
> 

Why fsl,dma-events? A driver consuming a DMA channel should not
need to care if it's an freescale DMA controller or something else.

I also don't like the drivers having to decode that property themselves.
If you want bindings for dma channels, they should be implementation
indepedent and work as easily as possible with the dmaengine API.

I would turn the above into 

ssi@83fcc000 { /* SSI1 */
         compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
         reg = <0x83fcc000 0x4000>;
         interrupts = <29>;
	 dma-parent = &fsldma1;
         dma-channels = <29 28 27 26>; /* TX0 RX0 TX1 RX1 */
};

struct dma_chan *tx0_chan = of_dma_find_chan(ssidev->dev, 0);
struct dma_chan *rx0_chan = of_dma_find_chan(ssidev->dev, 1);
struct dma_chan *tx1_chan = of_dma_find_chan(ssidev->dev, 2);
struct dma_chan *rx1_chan = of_dma_find_chan(ssidev->dev, 3);

	Arnd

Re: Is there a binding for IORESOURCE_DMA population?

[Tabi Timur-B04825]

Arnd Bergmann wrote:
> Why fsl,dma-events? A driver consuming a DMA channel should not
> need to care if it's an freescale DMA controller or something else.

The SSI, unfortunately, works very closely with the internal mechanics of 
the DMA device, so it's not possible to completely separate them.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Is there a binding for IORESOURCE_DMA population?

[Arnd Bergmann]

On Saturday 16 July 2011 17:27:02 Tabi Timur-B04825 wrote:
> 
> Arnd Bergmann wrote:
> > Why fsl,dma-events? A driver consuming a DMA channel should not
> > need to care if it's an freescale DMA controller or something else.
> 
> The SSI, unfortunately, works very closely with the internal mechanics of 
> the DMA device, so it's not possible to completely separate them.

That's ok. My point is that we should have bindings that work for other
devices, too. There are a lot of dma engines and a lot of drivers using
them, some are closely related and others are not.

	Arnd

Re: Is there a binding for IORESOURCE_DMA population?

[Rob Herring]

On 07/16/2011 09:40 AM, Shawn Guo wrote:
> On Sat, Jul 16, 2011 at 08:55:41PM +0800, Tabi Timur-B04825 wrote:
>> Arnd Bergmann wrote:
>>> Right, this is what I had in mind. I think this is reasonable.
>>> If you need so many channels, the device tree will be huge already,
>>> so this doesn't add that much bloat either.
>>
>> I'm not sure I agree.  It seems that specifying nodes for each DMA channel 
>> (virtual or physical) is meaningless because the only property in the node 
>> is the "reg", which is really just a cell-index in disguise.
>>
> Yes, that's exactly why I do not appreciate the solution.
> 
>> My vote is to have a node only for the DMA engine, and let the DMA handle 
>> the virtual and physical channels internally.
>>
> It's not about virtual or physical channel.  I'm asking suggestion on
> how we bind the channel number for each dma client device in device
> tree and how they will get the number from device tree when they get
> probed.
> 

In this case, it is the event (or request line) that is important not
the channel assignment. This is how the hardware is wired up and the
main variation between chips with SDMA. Since DT describes the h/w, this
needs to be described.

For some DMA controllers, the channel and request are the same thing.
Sometimes they are not but the channel assignment is still important. On
i.MX21, the channel number defines the priority for example.

Regards,
Rob

Re: Is there a binding for IORESOURCE_DMA population?

[Tabi Timur-B04825]

Shawn Guo wrote:
> It's not about virtual or physical channel.  I'm asking suggestion on
> how we bind the channel number for each dma client device in device
> tree and how they will get the number from device tree when they get
> probed.

IMHO, a simple property that lists the DMA channels to use should be okay. 
  Arnd's example looks good to me:

ssi@83fcc000 { /* SSI1 */
          compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
          reg = <0x83fcc000 0x4000>;
          interrupts = <29>;
	 dma-parent = &fsldma1;
          dma-channels = <29 28 27 26>; /* TX0 RX0 TX1 RX1 */
};

However, I would use less generic property names, like 
"fsl,ssi-dma-parent" or something like that.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Is there a binding for IORESOURCE_DMA population?

[Arnd Bergmann]

On Saturday 16 July 2011 22:18:53 Tabi Timur-B04825 wrote:
> ssi@83fcc000 { /* SSI1 */
>           compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
>           reg = <0x83fcc000 0x4000>;
>           interrupts = <29>;
>          dma-parent = &fsldma1;
>           dma-channels = <29 28 27 26>; /* TX0 RX0 TX1 RX1 */
> };
> 
> However, I would use less generic property names, like 
> "fsl,ssi-dma-parent" or something like that.

But that would mean that the code interpreting that property has to
know about the specific dmaengine implementation, and needs to be
changed every time we add a new one.

Alternatively, you'd end up with the device drivers having to
read the properties themselves to get to the number and pass
that to the common dmaengine code.

Neither way looks like a sufficiently generic solution to me.

	Arnd

Re: Is there a binding for IORESOURCE_DMA population?

[Tabi Timur-B04825]

Arnd Bergmann wrote:
> But that would mean that the code interpreting that property has to
> know about the specific dmaengine implementation, and needs to be
> changed every time we add a new one.

And like I said, the SSI driver will already need to know the specifics of 
the DMA controller.  The PowerPC device tree node for the SSI already 
works this way:

             ssi@15000 {
                     compatible = "fsl,mpc8610-ssi";
                     cell-index = <0>;
                     reg = <0x15000 0x100>;
                     interrupts = <75 2 0 0>;
                     fsl,mode = "i2s-slave";
                     codec-handle = <&wm8776>;
                     fsl,playback-dma = <&dma00>;
                     fsl,capture-dma = <&dma01>;
                     fsl,fifo-depth = <15>;
                     fsl,ssi-asynchronous;
              };

On PowerPC, the SSI driver knows that "fsl,playback-dma" points to a 
Freescale e500 DMA controller channel node.  The driver currently does not 
work with any other DMA controller.  This is what needs to be fixed.

> Alternatively, you'd end up with the device drivers having to
> read the properties themselves to get to the number and pass
> that to the common dmaengine code.

The SSI won't be able to use any common DMA engine code, because DMA needs 
to be set up in a very specific way for audio.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

On Sat, Jul 16, 2011 at 10:38:11PM +0800, Tabi Timur-B04825 wrote:
> Shawn Guo wrote:
> > It's not about virtual or physical channel.  I'm asking suggestion on
> > how we bind the channel number for each dma client device in device
> > tree and how they will get the number from device tree when they get
> > probed.
> 
> I guess I'm still confused.  Why do we need to bind the channel number? 
> It's just a resource that can be owned by the driver without needing a 
> device tree.

I should have said it's about how we bind sdma event number for each
client device and how the client device drivers retrieve the number
from device tree.  To be clear, the sdma event number is a sdma client
device specific resource, which is defined by soc.  You may ask why
this virtual number matters, since physical channel number is
dynamically assigned to the client device.  It's related to sdma
hardware design.  sdma is a custom RISC core with SRAM inside.  It
executes firmware to move data between peripheral and system memory.
The sdma event number is actually used to identify the peripheral by
sdma core and firmware.  So whenever a peripheral (sdma client device)
requests sdma service, it has to tell sdma its event number to let
sdma identify it.

For existing non-dt case, the sdma event number is being passed into
sdma client device driver by platform code as resource of
IORESOURCE_DMA.  But the approach is not working for dt case.

-- 
Regards,
Shawn

Re: Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

On Sat, Jul 16, 2011 at 02:16:40PM -0500, Rob Herring wrote:
> On 07/16/2011 09:40 AM, Shawn Guo wrote:
> > On Sat, Jul 16, 2011 at 08:55:41PM +0800, Tabi Timur-B04825 wrote:
> >> Arnd Bergmann wrote:
> >>> Right, this is what I had in mind. I think this is reasonable.
> >>> If you need so many channels, the device tree will be huge already,
> >>> so this doesn't add that much bloat either.
> >>
> >> I'm not sure I agree.  It seems that specifying nodes for each DMA channel 
> >> (virtual or physical) is meaningless because the only property in the node 
> >> is the "reg", which is really just a cell-index in disguise.
> >>
> > Yes, that's exactly why I do not appreciate the solution.
> > 
> >> My vote is to have a node only for the DMA engine, and let the DMA handle 
> >> the virtual and physical channels internally.
> >>
> > It's not about virtual or physical channel.  I'm asking suggestion on
> > how we bind the channel number for each dma client device in device
> > tree and how they will get the number from device tree when they get
> > probed.
> > 
> 
> In this case, it is the event (or request line) that is important not
> the channel assignment.

Exactly.

> This is how the hardware is wired up and the
> main variation between chips with SDMA. Since DT describes the h/w, this
> needs to be described.
> 
We absolutely need to describe the events.  The topic here is how.

-- 
Regards,
Shawn

Re: Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

On Sun, Jul 17, 2011 at 04:54:21AM +0800, Tabi Timur-B04825 wrote:
> Arnd Bergmann wrote:
> > But that would mean that the code interpreting that property has to
> > know about the specific dmaengine implementation, and needs to be
> > changed every time we add a new one.
> 
> And like I said, the SSI driver will already need to know the specifics of 
> the DMA controller.  The PowerPC device tree node for the SSI already 
> works this way:
> 
>              ssi@15000 {
>                      compatible = "fsl,mpc8610-ssi";
>                      cell-index = <0>;
>                      reg = <0x15000 0x100>;
>                      interrupts = <75 2 0 0>;
>                      fsl,mode = "i2s-slave";
>                      codec-handle = <&wm8776>;
>                      fsl,playback-dma = <&dma00>;
>                      fsl,capture-dma = <&dma01>;
>                      fsl,fifo-depth = <15>;
>                      fsl,ssi-asynchronous;
>               };
> 
> On PowerPC, the SSI driver knows that "fsl,playback-dma" points to a 
> Freescale e500 DMA controller channel node.  The driver currently does not 
> work with any other DMA controller.  This is what needs to be fixed.
> 
> > Alternatively, you'd end up with the device drivers having to
> > read the properties themselves to get to the number and pass
> > that to the common dmaengine code.
> 
> The SSI won't be able to use any common DMA engine code, because DMA needs 
> to be set up in a very specific way for audio.
> 
i.mx sdma was implemented as a generic dmaengine driver, and i.mx ssi
driver is working with the generic dmaengine api.

-- 
Regards,
Shawn

Re: Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

On Sat, Jul 16, 2011 at 10:43:31PM +0800, Tabi Timur-B04825 wrote:
> Shawn Guo wrote:
> > ssi@83fcc000 { /* SSI1 */
> > 	compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
> > 	reg =<0x83fcc000 0x4000>;
> > 	interrupts =<29>;
> > 	fsl,dma-events =<29 28 27 26>; /* TX0 RX0 TX1 RX1 */
> > 	fsl,ssi-uses-dma;
> > };
> 
> This is similar to what we do on PowerPC:
> 
>              ssi@15000 {
>                      compatible = "fsl,mpc8610-ssi";
>                      cell-index = <0>;
>                      reg = <0x15000 0x100>;
>                      interrupts = <75 2 0 0>;
>                      fsl,mode = "i2s-slave";
>                      codec-handle = <&wm8776>;
>                      fsl,playback-dma = <&dma00>;
>                      fsl,capture-dma = <&dma01>;
>                      fsl,fifo-depth = <15>;
>                      fsl,ssi-asynchronous;
>               };
> 
> Keep in mind that there already is a device-tree enabled SSI driver, so 
> you need to find a way to use that driver, instead of adding device-tree 
> support to the non-DT driver that's already there.
> 
Between a) making DT enabled SSI driver (mpc version) work for i.mx
and b) adding DT probe support for imx-ssi driver, I would absolutely
go for b).  Apparently it's much easier.

-- 
Regards,
Shawn

Re: Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

On Sat, Jul 16, 2011 at 05:19:32PM +0200, Arnd Bergmann wrote:
> On Saturday 16 July 2011, Shawn Guo wrote:
> > Though I do not understand what you said about channel numbers within
> > within the parent dma controller, I do think the dma has something
> > in common with 'interrupt-controller', and we can has a property like
> > 'dma-channels' under the node of devices that are dma clients to contain
> > the their channel/event numbers, just like property 'interrupts'
> > containing devices' IRQ number.
> 
> I think that would be ok, and it's basically what I tried to express.
> 
> > Then like that IRQ number is decoded and populated into IORESOURCE_IRQ
> > by device tree infrastructural code, we can also do the same into
> > IORESOURCE_DMA.  In that case, drivers do not need any code change for
> > getting dma channel/event numbers from device tree, as
> > platform_get_resource(pdev, IORESOURCE_DMA) still works for them.
> 
> But I really don't think there is value in using IORESOURCE_DMA for this.
> We don't have the code to manage DMA resources for more than one DMA
> controller AFAICT, and I think we should not add it. Globally
> unique interrupt numbers cause us a lot of trouble and we go to great
> lengths to fake them on modern devices. It would be much better
> not to have them visible in the OS, and I don't want to add them to
> a modern API like the dmaengine.
> 
Yeah, I guessed that there are reasons we do not have the support
at infrastructural level :)

> > Right now, I have the following as the solution.  I do not prefer to
> > it as much as I prefer to IORESOURCE_DMA solution, but I think it's
> > better than that huge and meaningless dma-channel node list.
> > 
> > ssi@83fcc000 { /* SSI1 */
> >         compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
> >         reg = <0x83fcc000 0x4000>;
> >         interrupts = <29>;
> >         fsl,dma-events = <29 28 27 26>; /* TX0 RX0 TX1 RX1 */
> >         fsl,ssi-uses-dma;
> > };
> > 
> > of_property_read_u32_array(pdev->dev.of_node, "fsl,dma-events",
> >                            dma_events, ARRAY_SIZE(dma_events));
> > 
> 
> Why fsl,dma-events? A driver consuming a DMA channel should not
> need to care if it's an freescale DMA controller or something else.
> 
It's not a physical DMA channel, but just an event (request line) for
sdma core and firmware to identify the peripheral.

> I also don't like the drivers having to decode that property themselves.
> If you want bindings for dma channels, they should be implementation
> indepedent and work as easily as possible with the dmaengine API.
> 
> I would turn the above into 
> 
> ssi@83fcc000 { /* SSI1 */
>          compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
>          reg = <0x83fcc000 0x4000>;
>          interrupts = <29>;
> 	 dma-parent = &fsldma1;
>          dma-channels = <29 28 27 26>; /* TX0 RX0 TX1 RX1 */
> };
> 
> struct dma_chan *tx0_chan = of_dma_find_chan(ssidev->dev, 0);
> struct dma_chan *rx0_chan = of_dma_find_chan(ssidev->dev, 1);
> struct dma_chan *tx1_chan = of_dma_find_chan(ssidev->dev, 2);
> struct dma_chan *rx1_chan = of_dma_find_chan(ssidev->dev, 3);
> 
Yes, I agree it's absolutely the right way to go if we are binding
a physical channel.  But for sdma case here, it's an event (request
line), which we can take it as an virtual channel number.  That said
there is no actual 'struct dma_chan' backing it.  Instead,
'struct dma_chan' is backing physical channel which is totally dynamic
in the mapping/assigning to event.

-- 
Regards,
Shawn

Re: Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

On Sat, Jul 16, 2011 at 11:27:02PM +0800, Tabi Timur-B04825 wrote:
> Arnd Bergmann wrote:
> > Why fsl,dma-events? A driver consuming a DMA channel should not
> > need to care if it's an freescale DMA controller or something else.
> 
> The SSI, unfortunately, works very closely with the internal mechanics of 
> the DMA device, so it's not possible to completely separate them.
> 
Again, it's not the case for i.MX.  i.MX SDMA was implemented as the
generic dma driver, and all its client device driver talks to it with
the generic interface.

-- 
Regards,
Shawn

Re: Is there a binding for IORESOURCE_DMA population?

[Tabi Timur-B04825]

Shawn Guo wrote:
>> >
> Between a) making DT enabled SSI driver (mpc version) work for i.mx
> and b) adding DT probe support for imx-ssi driver, I would absolutely
> go for b).  Apparently it's much easier.

Absolutely not!  It's ridiculous to have two drivers for the same exact 
device that are both device-tree enabled.  It's already bad enough that we 
have fsl-ssi.c and imx-ssi.c.  imx-ssi.c should never have been created, 
but there's no way I will approve enhancing imx-ssi.c to also support 
device trees, when fsl-ssi.c already does this.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

On Sun, Jul 17, 2011 at 09:41:02PM +0800, Tabi Timur-B04825 wrote:
> Shawn Guo wrote:
> >> >
> > Between a) making DT enabled SSI driver (mpc version) work for i.mx
> > and b) adding DT probe support for imx-ssi driver, I would absolutely
> > go for b).  Apparently it's much easier.
> 
> Absolutely not!  It's ridiculous to have two drivers for the same exact 
> device that are both device-tree enabled.  It's already bad enough that we 
> have fsl-ssi.c and imx-ssi.c.  imx-ssi.c should never have been created, 
> but there's no way I will approve enhancing imx-ssi.c to also support 
> device trees, when fsl-ssi.c already does this.
> 
I was not part of creating imx-ssi.  But I guess that Sascha (Cc-ed)
might have strong reasons for creating it rather than reusing fsl-ssi.
I really doubt that missing device-tree on ARM platform is the only
reason resulting two ssi drivers.

I'm also not a fan of consolidating device driver between fsl mpc and
imx family, especially I had a try on eSDHC and saw something ugly
and negative feedback from people.

http://article.gmane.org/gmane.linux.kernel.mmc/7832
http://article.gmane.org/gmane.linux.kernel.mmc/8202

So unless someone initiates the consolidation of fsl-ssi and imx-ssi,
I will keep going option b).  If the consolidation is reasonable and
possible, it can happen anytime no matter whether device-tree is
added for imx-ssi or not.

-- 
Regards,
Shawn

Re: Is there a binding for IORESOURCE_DMA population?

[Tabi Timur-B04825]

Shawn Guo wrote:
> I was not part of creating imx-ssi.  But I guess that Sascha (Cc-ed)
> might have strong reasons for creating it rather than reusing fsl-ssi.

Not really:

http://mailman.alsa-project.org/pipermail/alsa-devel/2009-November/023425.html

Sascha created imx-ssi because he didn't even notice my driver was already 
there!

> I really doubt that missing device-tree on ARM platform is the only
> reason resulting two ssi drivers.

Yes, it is!  If we had had device tree support on ARM back then, imx-ssi 
would never have existed.

> I'm also not a fan of consolidating device driver between fsl mpc and
> imx family, especially I had a try on eSDHC and saw something ugly
> and negative feedback from people.
>
> http://article.gmane.org/gmane.linux.kernel.mmc/7832
> http://article.gmane.org/gmane.linux.kernel.mmc/8202

That feedback would not have stopped me from merging the two.  It looks 
like some minor concerns that could have been addressed.

> So unless someone initiates the consolidation of fsl-ssi and imx-ssi,
> I will keep going option b).  If the consolidation is reasonable and
> possible, it can happen anytime no matter whether device-tree is
> added for imx-ssi or not.

If you try to update the imx-ssi driver to support device trees, you'll 
have to convince the ASoC team to override my NAKs.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Is there a binding for IORESOURCE_DMA population?

[Arnd Bergmann]

On Sunday 17 July 2011 16:28:39 Shawn Guo wrote:
> I was not part of creating imx-ssi.  But I guess that Sascha (Cc-ed)
> might have strong reasons for creating it rather than reusing fsl-ssi.
> I really doubt that missing device-tree on ARM platform is the only
> reason resulting two ssi drivers.
> 
> I'm also not a fan of consolidating device driver between fsl mpc and
> imx family, especially I had a try on eSDHC and saw something ugly
> and negative feedback from people.
> 
> http://article.gmane.org/gmane.linux.kernel.mmc/7832
> http://article.gmane.org/gmane.linux.kernel.mmc/8202
> 
> So unless someone initiates the consolidation of fsl-ssi and imx-ssi,
> I will keep going option b).  If the consolidation is reasonable and
> possible, it can happen anytime no matter whether device-tree is
> added for imx-ssi or not.

I think this sort of driver consolidation is generally a good thing, but
it requires that the consolidated driver is clearly better than either
of the two original drivers. People are generally very attached to the
code they maintain and don't like to see their work getting replaced
by the same thing done differently. However, there are lots of
disadvantages to having multiple drivers for the same hardware and
we clearly want to have only one if we only need one.

How we get there depends a lot on the situation, and with the esdhc
driver, there wasn't that much duplication: Your patch actually
added more code than it removed and it added a number of #ifdef
sections were not present in the original drivers.

	Arnd

Re: Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

On Sun, Jul 17, 2011 at 05:04:17PM +0200, Arnd Bergmann wrote:
> On Sunday 17 July 2011 16:28:39 Shawn Guo wrote:
> > I was not part of creating imx-ssi.  But I guess that Sascha (Cc-ed)
> > might have strong reasons for creating it rather than reusing fsl-ssi.
> > I really doubt that missing device-tree on ARM platform is the only
> > reason resulting two ssi drivers.
> > 
> > I'm also not a fan of consolidating device driver between fsl mpc and
> > imx family, especially I had a try on eSDHC and saw something ugly
> > and negative feedback from people.
> > 
> > http://article.gmane.org/gmane.linux.kernel.mmc/7832
> > http://article.gmane.org/gmane.linux.kernel.mmc/8202
> > 
> > So unless someone initiates the consolidation of fsl-ssi and imx-ssi,
> > I will keep going option b).  If the consolidation is reasonable and
> > possible, it can happen anytime no matter whether device-tree is
> > added for imx-ssi or not.
> 
> I think this sort of driver consolidation is generally a good thing, but
> it requires that the consolidated driver is clearly better than either
> of the two original drivers. People are generally very attached to the
> code they maintain and don't like to see their work getting replaced
> by the same thing done differently. However, there are lots of
> disadvantages to having multiple drivers for the same hardware and
> we clearly want to have only one if we only need one.
> 
> How we get there depends a lot on the situation, and with the esdhc
> driver, there wasn't that much duplication: Your patch actually
> added more code than it removed and it added a number of #ifdef
> sections were not present in the original drivers.
> 
Thanks, Arnd.  This helped me get a better understanding on the driver
consolidation thing.

-- 
Regards,
Shawn

Re: Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

On Sun, Jul 17, 2011 at 10:57:44PM +0800, Tabi Timur-B04825 wrote:
> Shawn Guo wrote:
> > I was not part of creating imx-ssi.  But I guess that Sascha (Cc-ed)
> > might have strong reasons for creating it rather than reusing fsl-ssi.
> 
> Not really:
> 
> http://mailman.alsa-project.org/pipermail/alsa-devel/2009-November/023425.html
> 
> Sascha created imx-ssi because he didn't even notice my driver was already 
> there!
> 
> > I really doubt that missing device-tree on ARM platform is the only
> > reason resulting two ssi drivers.
> 
> Yes, it is!  If we had had device tree support on ARM back then, imx-ssi 
> would never have existed.
> 
> > I'm also not a fan of consolidating device driver between fsl mpc and
> > imx family, especially I had a try on eSDHC and saw something ugly
> > and negative feedback from people.
> >
> > http://article.gmane.org/gmane.linux.kernel.mmc/7832
> > http://article.gmane.org/gmane.linux.kernel.mmc/8202
> 
> That feedback would not have stopped me from merging the two.  It looks 
> like some minor concerns that could have been addressed.
> 
> > So unless someone initiates the consolidation of fsl-ssi and imx-ssi,
> > I will keep going option b).  If the consolidation is reasonable and
> > possible, it can happen anytime no matter whether device-tree is
> > added for imx-ssi or not.
> 
> If you try to update the imx-ssi driver to support device trees, you'll 
> have to convince the ASoC team to override my NAKs.
> 
Ok, I will take this as a serious warning, and start evaluating the
driver consolidation.  I will absolutely need you help along the way.
At least, when we get there, I need your favor to test mpc platform,
as I do not have the access at all.

Sigh, that may be the reason people love device tree but hates to work
it out, because before you add actual device tree support, you need to
do consolidation and cleanup stuff first :)

-- 
Regards,
Shawn

Re: Is there a binding for IORESOURCE_DMA population?

[Tabi Timur-B04825]

Shawn Guo wrote:
> Ok, I will take this as a serious warning, and start evaluating the
> driver consolidation.  I will absolutely need you help along the way.
> At least, when we get there, I need your favor to test mpc platform,
> as I do not have the access at all.

Absolutely.  I've been waiting a long time for someone to come along and 
help me unify the drivers.  As you know, there's very little collaboration 
between the ARM and PowerPC groups inside Freescale.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Is there a binding for IORESOURCE_DMA population?

[Grant Likely]

On Sat, Jul 16, 2011 at 02:09:46PM +0200, Arnd Bergmann wrote:
> On Saturday 16 July 2011 09:57:49 Shawn Guo wrote:
> > If I understand this correctly, this is something I demonstrated in
> > the initial message.  I do not think we always have individual
> > device_node for each channel (struct dma_chan).  The most common
> > situation is all channels are backed by one device_node (dmaengine).
> > 
> > As I stated in the initial message, (for i.mx sdma example) the
> > problem is 'struct dma_chan' stands for a physical dma channel, while
> > all platform_device has is a virtual channel number (known as sdma
> > event) assigned by hardware.  This virtual channel can only be
> > dynamically mapped to a physical one by sdma driver.  That said there
> > is no way for us to specify a physical channel in device tree.  We can
> > only specify the virtual one there.  (I think this is kinda common,
> > and sdma is just one example.)
> 
> My assumption was that you would create a device_node for each channel
> in the device tree source, but not necessarily have a platform_device
> for each of them.
> 
> > i.mx sdma has 32 physically channels and 48 virtual ones (events).
> > If we go the way you are suggesting, we will have to have 48 nodes
> > under sdma node just for giving event number, and then have
> > dma-channels property of device specifying the phanldles to these
> > event nodes belonging to it.
> > 
> > So we get something like below, which looks silly to me.
> > 
> > sdma@83fb0000 {
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> >         compatible = "fsl,imx51-sdma", "fsl,imx35-sdma";
> >         reg = <0x83fb0000 0x4000>;
> >         interrupts = <6>;
> >         fsl,sdma-ram-script-name = "sdma-imx51.bin";
> > 
> >         dma00: dma-channel@0 {
> >                 compatible = "fsl,sdma-channel";
> >                 reg = <0>;
> >         };
> > 
> >         dma01: dma-channel@1 {
> >                 compatible = "fsl,sdma-channel";
> >                 reg = <1>;
> >         };
> > 
> >         ......
> > 
> >         dma47: dma-channel@47 {
> >                 compatible = "fsl,sdma-channel";
> >                 reg = <47>;
> >         };
> > };
> > 
> > ssi@83fcc000 { /* SSI1 */
> >         compatible = "fsl,imx51-ssi", "fsl,imx1-ssi";
> >         reg = <0x83fcc000 0x4000>;
> >         interrupts = <29>;
> >         fsl,ssi-uses-dma;
> >         dma-channels = <&dma00, &dma01>;
> > };
> > 
> > Did I state the problem clear?  Or am I missing anything?
> 
> Right, this is what I had in mind. I think this is reasonable.
> If you need so many channels, the device tree will be huge already,
> so this doesn't add that much bloat either.
> 
> Another option would be to add a 'dma-parent' property akin to the
> interrupt-parent property and then just refer to the channel
> numbers within the parent dma controller.
> I guess you would prefer something like that in order to keep the
> device tree source more compact, right?

Rather than a dma-parent property, I'd recommend following approach
that the gpio binding uses and have:

	#dma-channel-cells = <n>

...in the dma controller, and...

	dma-channels = <&dma-controller (dma-specifier)>;

...in the dma user.  Then the dma controller binding can specify
how much data is required for a dma-specifier (number of cells), and
what each cell means.

g.

Re: Is there a binding for IORESOURCE_DMA population?

[Grant Likely]

On Sun, Jul 17, 2011 at 01:41:02PM +0000, Tabi Timur-B04825 wrote:
> Shawn Guo wrote:
> >> >
> > Between a) making DT enabled SSI driver (mpc version) work for i.mx
> > and b) adding DT probe support for imx-ssi driver, I would absolutely
> > go for b).  Apparently it's much easier.
> 
> Absolutely not!  It's ridiculous to have two drivers for the same exact 
> device that are both device-tree enabled.  It's already bad enough that we 
> have fsl-ssi.c and imx-ssi.c.  imx-ssi.c should never have been created, 
> but there's no way I will approve enhancing imx-ssi.c to also support 
> device trees, when fsl-ssi.c already does this.

I would disagree here.  I absolutely do think that the drivers should
be consolidated if they are the same device, but until someone
actually steps up to do that work, and as long as the current drivers
use different interfaces, then I have no problem adding the DT
bindings to the imx-ssi.c driver.

That said, since the fsl-ssi.c driver already implements a DT binding,
the imx driver definitely should implement the same binding unless
there is a really strong technical reason not to.

g.

Re: Is there a binding for IORESOURCE_DMA population?

[Tabi Timur-B04825]

Grant Likely wrote:
> That said, since the fsl-ssi.c driver already implements a DT binding,
> the imx driver definitely should implement the same binding unless
> there is a really strong technical reason not to.

What's the point of having two drivers for the same device that implement 
the same DT binding?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Is there a binding for IORESOURCE_DMA population?

[Arnd Bergmann]

On Sunday 17 July 2011, Tabi Timur-B04825 wrote:
> Shawn Guo wrote:
> > Ok, I will take this as a serious warning, and start evaluating the
> > driver consolidation.  I will absolutely need you help along the way.
> > At least, when we get there, I need your favor to test mpc platform,
> > as I do not have the access at all.
> 
> Absolutely.  I've been waiting a long time for someone to come along and 
> help me unify the drivers.  As you know, there's very little collaboration 
> between the ARM and PowerPC groups inside Freescale.

I've looked a bit at the two drivers, and it seems to me that both
would benefit from taking aspects from the other one.

Shawn, when you start the consolidation, I think it would be good
to first eliminate all the minor differences like variable names
and header file contents, always using the cleaner or more generic
version for each aspect. I've had some good success using 'vimdiff'
for similar work, which gives you a side-by-side representation of
the two files. There are evidently some more fundamental differences
like the DMA usage, but I think it's easier to leave them for
a later stage.

In some cases, you will have to abstract the differences between
the hardware like the mmio accessors that are not common between
powerpc and arm yet.

I also think that it would be useful to add the same device tree based
probing to the imx driver that is in the fsl one.

	Arnd

Re: Is there a binding for IORESOURCE_DMA population?

[Shawn Guo]

On Tue, Jul 19, 2011 at 05:16:56PM +0200, Arnd Bergmann wrote:
> On Sunday 17 July 2011, Tabi Timur-B04825 wrote:
> > Shawn Guo wrote:
> > > Ok, I will take this as a serious warning, and start evaluating the
> > > driver consolidation.  I will absolutely need you help along the way.
> > > At least, when we get there, I need your favor to test mpc platform,
> > > as I do not have the access at all.
> > 
> > Absolutely.  I've been waiting a long time for someone to come along and 
> > help me unify the drivers.  As you know, there's very little collaboration 
> > between the ARM and PowerPC groups inside Freescale.
> 
> I've looked a bit at the two drivers, and it seems to me that both
> would benefit from taking aspects from the other one.
> 
> Shawn, when you start the consolidation, I think it would be good
> to first eliminate all the minor differences like variable names
> and header file contents, always using the cleaner or more generic
> version for each aspect. I've had some good success using 'vimdiff'
> for similar work, which gives you a side-by-side representation of
> the two files. There are evidently some more fundamental differences
> like the DMA usage, but I think it's easier to leave them for
> a later stage.
> 
> In some cases, you will have to abstract the differences between
> the hardware like the mmio accessors that are not common between
> powerpc and arm yet.
> 
> I also think that it would be useful to add the same device tree based
> probing to the imx driver that is in the fsl one.
> 
Arnd, thanks for all these points.

As the audio device tree support on i.mx still has a few other issues
to address besides this one, plus that I have got several board files
to convert to device tree, I guess it's going to be a while before I
can actually start it.

-- 
Regards,
Shawn