DT DMA channel binding for Tegra I2S

DT DMA channel binding for Tegra I2S

[Stephen Warren]

Rob, Grant,

Re: http://www.spinics.net/lists/arm-kernel/msg148899.html

Background: Tegra's DMA controller needs to be told which DMA "request
select" to use for each transfer. The identifies which peripheral to
transfer to/from.

The Tegra I2S driver needs to know its own "request select" value to
pass to the audio driver DMA path. The board files typically provide
this to the driver as an IORESOURCE_DMA. I don't think there's any
standard binding that creates such a resource when instantiating a
platform device from device tree, and the discussions I found when
looking for one didn't seem to reach conclusion that there should be
one. So, I proposed the following property in the I2S driver's DT
binding for this:

Doc:

- dma-channel : The Tegra DMA controller's channel ID for this I2S controller

Example:

i2s@70002800 {
	compatible = "nvidia,tegra20-i2s";
	reg = <0x70002800 0x200>;
	interrupts = < 45 >;
	dma-channel = < 2 >;
};

Does that look reasonable? Or, should I pursue some more standardized
solution?

(although perhaps dma-request-select would be a better name, since each
of the DMA controller's 16 channels can be used for any of the DMA
selects)

For reference, the code turns out as below; it might be nicer if the
DT parsing code could create this resource for us, although there was
some previous discussion about IORESOURCE_DMA not being the correct
representation for this anyway.

	dmareq = platform_get_resource(pdev, IORESOURCE_DMA, 0);
	if (!dmareq) {
		if (of_property_read_u32(pdev->dev.of_node,
					 "nvidia,dma-channel",
					 &dma_ch) < 0) {
...

Thanks.

-- 
nvpublic

Re: DT DMA channel binding for Tegra I2S

[Rob Herring]

On 11/23/2011 03:25 PM, Stephen Warren wrote:
> Rob, Grant,
> 
> Re: http://www.spinics.net/lists/arm-kernel/msg148899.html
> 
> Background: Tegra's DMA controller needs to be told which DMA "request
> select" to use for each transfer. The identifies which peripheral to
> transfer to/from.
> 
> The Tegra I2S driver needs to know its own "request select" value to
> pass to the audio driver DMA path. The board files typically provide
> this to the driver as an IORESOURCE_DMA. I don't think there's any
> standard binding that creates such a resource when instantiating a
> platform device from device tree, and the discussions I found when
> looking for one didn't seem to reach conclusion that there should be
> one. So, I proposed the following property in the I2S driver's DT
> binding for this:

I think this is a case of every platform being different and
IORESOURCE_DMA is not too widely used. Even on PPC, there's not a
standard way.

> 
> Doc:
> 
> - dma-channel : The Tegra DMA controller's channel ID for this I2S controller
> 
> Example:
> 
> i2s@70002800 {
> 	compatible = "nvidia,tegra20-i2s";
> 	reg = <0x70002800 0x200>;
> 	interrupts = < 45 >;
> 	dma-channel = < 2 >;
> };
> 
> Does that look reasonable? Or, should I pursue some more standardized
> solution?

I had some discussion with Thomas Abraham about this for the pl330 dma:

http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-September/008269.html

So I would just add a phandle to the dma controller here.

> 
> (although perhaps dma-request-select would be a better name, since each
> of the DMA controller's 16 channels can be used for any of the DMA
> selects)

You'll find h/w with all sorts of relationships between requests and
channels on DMA controller h/w...

> 
> For reference, the code turns out as below; it might be nicer if the
> DT parsing code could create this resource for us, although there was
> some previous discussion about IORESOURCE_DMA not being the correct
> representation for this anyway.
> 
> 	dmareq = platform_get_resource(pdev, IORESOURCE_DMA, 0);
> 	if (!dmareq) {
> 		if (of_property_read_u32(pdev->dev.of_node,
> 					 "nvidia,dma-channel",
> 					 &dma_ch) < 0) {

It would, but I think IORESOURCE_DMA is supposed to be for ISA bus DMA.
Also, I think often you need more information than just a request line
like a mode or type of transfer or burst length parameters.

Rob

RE: DT DMA channel binding for Tegra I2S

[Stephen Warren]

Rob Herring wrote at Wednesday, November 23, 2011 2:55 PM:
> On 11/23/2011 03:25 PM, Stephen Warren wrote:
> > Rob, Grant,
> >
> > Re: http://www.spinics.net/lists/arm-kernel/msg148899.html
> >
> > Background: Tegra's DMA controller needs to be told which DMA "request
> > select" to use for each transfer. The identifies which peripheral to
> > transfer to/from.
> >
> > The Tegra I2S driver needs to know its own "request select" value to
> > pass to the audio driver DMA path. The board files typically provide
> > this to the driver as an IORESOURCE_DMA. I don't think there's any
> > standard binding that creates such a resource when instantiating a
> > platform device from device tree, and the discussions I found when
> > looking for one didn't seem to reach conclusion that there should be
> > one. So, I proposed the following property in the I2S driver's DT
> > binding for this:
> 
> I think this is a case of every platform being different and
> IORESOURCE_DMA is not too widely used. Even on PPC, there's not a
> standard way.
> 
> > Doc:
> >
> > - dma-channel : The Tegra DMA controller's channel ID for this I2S controller
> >
> > Example:
> >
> > i2s@70002800 {
> > 	compatible = "nvidia,tegra20-i2s";
> > 	reg = <0x70002800 0x200>;
> > 	interrupts = < 45 >;
> > 	dma-channel = < 2 >;
> > };
> >
> > Does that look reasonable? Or, should I pursue some more standardized
> > solution?
> 
> I had some discussion with Thomas Abraham about this for the pl330 dma:
> 
> http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-September/008269.html
> 
> So I would just add a phandle to the dma controller here.

So you mean just modify the binding as follows:

apbdma: dma@NNNNN { ... };

i2s@70002800 {
	dma-channel = < &apbdma, 2 >;
};

... and leave the code I quoted as-is?

I /think/ the rest of the patch you linked isn't really relevant to Tegra
at present, since it isn't ported to the DMA engine (yet?).

Thanks.

-- 
nvpublic

Re: DT DMA channel binding for Tegra I2S

[Rob Herring]

On 11/23/2011 05:26 PM, Stephen Warren wrote:
> Rob Herring wrote at Wednesday, November 23, 2011 2:55 PM:
>> On 11/23/2011 03:25 PM, Stephen Warren wrote:
>>> Rob, Grant,
>>>
>>> Re: http://www.spinics.net/lists/arm-kernel/msg148899.html
>>>
>>> Background: Tegra's DMA controller needs to be told which DMA "request
>>> select" to use for each transfer. The identifies which peripheral to
>>> transfer to/from.
>>>
>>> The Tegra I2S driver needs to know its own "request select" value to
>>> pass to the audio driver DMA path. The board files typically provide
>>> this to the driver as an IORESOURCE_DMA. I don't think there's any
>>> standard binding that creates such a resource when instantiating a
>>> platform device from device tree, and the discussions I found when
>>> looking for one didn't seem to reach conclusion that there should be
>>> one. So, I proposed the following property in the I2S driver's DT
>>> binding for this:
>>
>> I think this is a case of every platform being different and
>> IORESOURCE_DMA is not too widely used. Even on PPC, there's not a
>> standard way.
>>
>>> Doc:
>>>
>>> - dma-channel : The Tegra DMA controller's channel ID for this I2S controller
>>>
>>> Example:
>>>
>>> i2s@70002800 {
>>> 	compatible = "nvidia,tegra20-i2s";
>>> 	reg = <0x70002800 0x200>;
>>> 	interrupts = < 45 >;
>>> 	dma-channel = < 2 >;
>>> };
>>>
>>> Does that look reasonable? Or, should I pursue some more standardized
>>> solution?
>>
>> I had some discussion with Thomas Abraham about this for the pl330 dma:
>>
>> http://lists.ozlabs.org/pipermail/devicetree-discuss/2011-September/008269.html
>>
>> So I would just add a phandle to the dma controller here.
>
> So you mean just modify the binding as follows:
>
> apbdma: dma@NNNNN { ... };
>
> i2s@70002800 {
> 	dma-channel = < &apbdma, 2 >;
> };
>
> ... and leave the code I quoted as-is?

Right.

Although, I think you are possibly missing some other properties for
i2s mode like word size and master/slave mode. I think the ideal case
is that a single ASoC platform driver for DT could handle multiple
SoCs.

>
> I /think/ the rest of the patch you linked isn't really relevant to Tegra
> at present, since it isn't ported to the DMA engine (yet?).
>

Yes, of course..

Rob

Re: DT DMA channel binding for Tegra I2S

[Mark Brown]

On Fri, Nov 25, 2011 at 09:55:23AM -0600, Rob Herring wrote:

> Although, I think you are possibly missing some other properties for
> i2s mode like word size and master/slave mode. I think the ideal case
> is that a single ASoC platform driver for DT could handle multiple
> SoCs.

No.  You're not going to be able to get a single binding which handles
the setup for all systems based off a given SoC let alone multiple SoCs.
It is possible to come up with some subsets which work over multiple
boards but that's going to be outside the binding for the I2S controller
and in a board audio hardware binding for the subset.

This has been discussed repeatedly, we really need to work out how to
educate device tree people about this stuff, it gets *very* repetitive
having the discussion over and over again.

Re: DT DMA channel binding for Tegra I2S

[Rob Herring]

Mark,

On 11/25/2011 10:09 AM, Mark Brown wrote:
> On Fri, Nov 25, 2011 at 09:55:23AM -0600, Rob Herring wrote:
> 
>> Although, I think you are possibly missing some other properties for
>> i2s mode like word size and master/slave mode. I think the ideal case
>> is that a single ASoC platform driver for DT could handle multiple
>> SoCs.
> 
> No.  You're not going to be able to get a single binding which handles
> the setup for all systems based off a given SoC let alone multiple SoCs.
> It is possible to come up with some subsets which work over multiple
> boards but that's going to be outside the binding for the I2S controller
> and in a board audio hardware binding for the subset.

I never said a single binding. Part of device tree is describing how
devices are connected together which is a large part of the audio
configuration. Whether there's one common driver or not doesn't really
matter. The important part right now is to define the bindings and do so
in a common way where possible.

My original point really was that when defining a binding, it should be
as complete as possible rather than done incrementally over time. You
are describing the h/w, and that's not changing. For an i2s block, only
having a dma channel is definitely not complete.

> This has been discussed repeatedly, we really need to work out how to
> educate device tree people about this stuff, it gets *very* repetitive
> having the discussion over and over again.

As you might guess, audio is not an area I have been spending time on
recently. Perhaps seeing a complete binding with all the pieces (codec,
i2s, misc gpio's, etc.) would help educate us.

Rob

Re: DT DMA channel binding for Tegra I2S

[Mark Brown]

On Sat, Nov 26, 2011 at 10:07:01PM -0600, Rob Herring wrote:
> On 11/25/2011 10:09 AM, Mark Brown wrote:
> > On Fri, Nov 25, 2011 at 09:55:23AM -0600, Rob Herring wrote:

> >> Although, I think you are possibly missing some other properties for
> >> i2s mode like word size and master/slave mode. I think the ideal case
> >> is that a single ASoC platform driver for DT could handle multiple
> >> SoCs.

> > No.  You're not going to be able to get a single binding which handles
> > the setup for all systems based off a given SoC let alone multiple SoCs.
> > It is possible to come up with some subsets which work over multiple
> > boards but that's going to be outside the binding for the I2S controller
> > and in a board audio hardware binding for the subset.

> I never said a single binding. Part of device tree is describing how
> devices are connected together which is a large part of the audio
> configuration. Whether there's one common driver or not doesn't really
> matter. The important part right now is to define the bindings and do so
> in a common way where possible.

That's what I'm hearing when I hear you talk about things like defining
a binding that works for multiple SoCs or which defines stuff like word
size and master/slave mode (word size especially needs to be a runtime
property).

> My original point really was that when defining a binding, it should be
> as complete as possible rather than done incrementally over time. You
> are describing the h/w, and that's not changing. For an i2s block, only
> having a dma channel is definitely not complete.

The binding posted had DMA, registers and IRQ which seems like exactly
what I'd expect for an I2S controller - the reason only DMA was being
highlighted is that that's the only bit with a lack of clarity on the
bindings.

> > This has been discussed repeatedly, we really need to work out how to
> > educate device tree people about this stuff, it gets *very* repetitive
> > having the discussion over and over again.

> As you might guess, audio is not an area I have been spending time on
> recently. Perhaps seeing a complete binding with all the pieces (codec,
> i2s, misc gpio's, etc.) would help educate us.

Stephen's patches are the closest I've seen yet to what's been decided.

This is exactly the sort of concern I have with education - we've been
through this discussion repeatedly and yet every time a new person from
the OF side turns up we have to go through the same explanations and
discussion again.  Like I say this really does get very repetitive.