On Fri, Aug 09, 2013 at 11:06:23AM +0200, Jean-Francois Moine wrote:

On Fri, 09 Aug 2013 10:23:50 +0200 Sebastian Hesselbarth sebastian.hesselbarth@gmail.com wrote:

...

we need at least two more compatibles for the audio controller found on Dove and Kirkwood respectively. This is how we are going to distinguish those two, e.g. Kirkwood has SPDIF in which Dove hasn't.

Sebastian,

s/has/hasn't & s/hasn't/has

No, you're both wrong.

Some Kirkwood has SPDIF recording and playback. There are those audio blocks which have just I2S capture and playback. There are those which have I2S capture and playback, and SPDIF playback. There are those which have both I2S and SPDIF for both capture and playback. The only one I haven't yet seen is SPDIF capture without playback.

On Fri, 26 Jul 2013 10:21:56 +0100 Russell King - ARM Linux linux@arm.linux.org.uk wrote:

...

On Fri, Jul 26, 2013 at 11:09:13AM +0200, Jean-Francois Moine wrote:

...

The A510 documentation uses the names "DCO PLL" for the internal clock and "AU_EXTCLK" for the external clock. So, what about "dcopll" and "extclk"?

Stop naming them according to their source. Their _consumer_ names not _source_ names.

But Russell did not tell clearly which name could be the best.

BTW, as we are naming the clocks, the 'clk' in "xxxclk" seems redondant...

"extclk" follows what's in the data sheet - the exact name of it is "AU_EXTCLK". Since we already know that this is the audio unit from the struct device, the "AU_" part is redundant. The input name for the internal clock is not given, instead they talk about where it comes from (it's producer) so your choice of "internal" is fine.

Take a moment to think about it: if you call it "dcoclk" then what happens on a future SoC if it's not connected to the dcoclk anymore?

I don't think that we need any reference to the codec here. The glue is done by the audio device. For example, using the (soon?) extended simple audio card:

spdif: spdif { compatible = "linux,spdif-dit"; }; sound { compatible = "linux,simple-audio"; audio-controller = <&i2s1>; audio-codec = <&spdif>; codec-dai-name = "dit-hifi"; };

As I understand the way DPCM works, that isn't going to work for this case. For DPCM as per Liam's example, it's going to require the audio controller to be bound to the dummy codec, and a dummy platform/dai bound to the SPDIF codec. You then use DAPM routing to connect the SPDIF codec to the appropriate CPU DAI output stream.

So the above "simple" implementation won't do - it can't be used to specify two DAI links, and it can't specify the DAPM routing between them.

This will be another challenge to solve in DT!

On Fri, Aug 09, 2013 at 10:23:50AM +0200, Sebastian Hesselbarth wrote:

On 08/08/2013 01:22 PM, Jean-Francois Moine wrote:

...

+i2s1: audio-controller@b4000 {

• compatible = "mrvl,mvebu-audio";
• reg = <0xb4000 0x2210>;
• interrupts = <21>, <22>;
• clocks = <&gate_clk 13>;
• clock-names = "internal";

+};

Also we will need some phandle reference to the audio codec here. As this property is ongoing work in ASoC core, I suggest we wait for it and propose a binding afterwards.

No, as discussed this should be in the binding for the audio subsystem not in the binding for an individual component in that subsystem.

On Fri, Aug 09, 2013 at 11:34:30AM +0200, Sebastian Hesselbarth wrote:

Mark,

I do understand there may be SoCs requiring sophisticated extra audio nodes, but Marvell SoCs don't. I prefer having a single node for the i2s controller *and* exploit the audio subsystem properties from that.

For Marvell audio, we only need a single node for all three ASoC drivers. No other subsystem _requires_ you to have extra nodes for it's needs. If you can provide interrupts, just have an interrupt- controller property. If you can provide clocks, you can link to that very node - no virtual device node required. Even for media they do not insist on a virtual node but they do have generic properties you can exploit.

Certainly from the "DT is a hardware description" you are completely correct. The audio controller _should_ link directly to the codec, because that's how the hardware is wired. Remember though that there are two outputs from the audio controller (please, call it audio controller, not I2S controller) so you need to specify which of those two outputs the "codec" is connected to.

I would say that's required irrespective of whether or not we have a virtual node to aggregate the stuff together for ASoC's purposes (which should not be part of the DT hardware description - it can be part of the "software" configuration though.)

We may be able to have kirkwood-i2s.c (which I plan to rename to mvebu-audio.c) automatically generate the snd_soc_card stuff from the audio controller node. Given that we need a more complex description than the "simple" thing for DPCM, that might be overall the best solution in any case (maybe calling out to a library which can be shared between CPU DAI drivers to do this.)

Note that we do have another case not yet in tree, which is DRM, but this case is different from that, because ASoC can cope with components with independent initialisation.

On 08/09/13 11:43, Russell King - ARM Linux wrote:

On Fri, Aug 09, 2013 at 11:34:30AM +0200, Sebastian Hesselbarth wrote:

...

I do understand there may be SoCs requiring sophisticated extra audio nodes, but Marvell SoCs don't. I prefer having a single node for the i2s controller *and* exploit the audio subsystem properties from that.

For Marvell audio, we only need a single node for all three ASoC drivers. No other subsystem _requires_ you to have extra nodes for it's needs. If you can provide interrupts, just have an interrupt- controller property. If you can provide clocks, you can link to that very node - no virtual device node required. Even for media they do not insist on a virtual node but they do have generic properties you can exploit.

Certainly from the "DT is a hardware description" you are completely correct. The audio controller _should_ link directly to the codec, because that's how the hardware is wired. Remember though that there are two outputs from the audio controller (please, call it audio controller, not I2S controller) so you need to specify which of those two outputs the "codec" is connected to.

Exactly, we can solve that with multiple phandle/args audio-codecs property and audio-codec-names (or whatever property names are more appropriate). That is the common set of properties, I am talking about. But the properties are totally unrelated to what nodes you put them into.

I would say that's required irrespective of whether or not we have a virtual node to aggregate the stuff together for ASoC's purposes (which should not be part of the DT hardware description - it can be part of the "software" configuration though.)

And that is *the only thing* that keeps bugging me in Mark's replies - he *insists* on having that virtual audio nodes. I have nothing against it, except it should be *required* for every DT we have. DRM doesn't _need_ it, media doesn't _need_ it, but audio is so very special that it _requires_ you to have it described in DT?

I understand that it may be required on some boards, especially if you create different sound-cards out of the IP available. Just like the DRM discussion we had - have a virtual node if there is no other sane way to describe it, but there is no strict requirement.

Anyway, Mark keeps insisting on it, I'll obey.

We may be able to have kirkwood-i2s.c (which I plan to rename to mvebu-audio.c) automatically generate the snd_soc_card stuff from the audio controller node. Given that we need a more complex description than the "simple" thing for DPCM, that might be overall the best solution in any case (maybe calling out to a library which can be shared between CPU DAI drivers to do this.)

That is what I am doing on top of the audio-controller node and except that there is no helper to determine the names, yet. If ASoC would provide a snd_soc_simple_card_register_from_dt(...,device_node *), I wouldn't even have to parse the properties myself.

Note that we do have another case not yet in tree, which is DRM, but this case is different from that, because ASoC can cope with components with independent initialisation.

True, and those should also probe themselves independent of the corresponding lcd driver. Now that we have the discussion on ASoC, that will also allow us to have an audio codec driver for the TDA998x audio part. We need that anyway to create correct AIF packets someday.

Sebastian

On Fri, Aug 09, 2013 at 12:39:40PM +0100, Mark Brown wrote:

So extend Morimoto-san's work on the simple card for this - that's what it's there for, it's doing exactly this job for non-DT systems but it just didn't get DT support added yet. All the trivial cards should end up using this.

It's quite rediculous to request that the simple card stuff is expanded at this time, when you're also telling us that we must use DPCM for Kirkwood, but DPCM is not yet in a working state in mainline. We don't yet know whether that's because of something the core is doing wrong or whether that's because something that I'm doing wrong. The reason we don't know that is because the _only_ person who knows anything about this is Liam, and as you have said, Liam is away on vacation.

All that we presently know is that DPCM is supposed to be "something like this". However, when we do that, it doesn't work, which means we can't be certain what the end result is supposed to look like. It may be that Liam has changed some of the design decisions with DPCM.

We know that various things are missing - like the .pcm_playback member in the DAI link. That points to Liam having additional patches to the core code in his tree to make DPCM work. That could impact our scenario here, and how the "simple" card should be implemented.

So, what you're asking is for us to extend the design of something in a way that we know very little about how to make work.

To that I say: No. Let's wait for Liam to return from vacation so that this can be sorted out properly with someone who knows this code.

On Fri, Aug 09, 2013 at 07:00:58PM +0100, Mark Brown wrote:

On Fri, Aug 09, 2013 at 02:09:32PM +0100, Russell King - ARM Linux wrote:

...

On Fri, Aug 09, 2013 at 12:39:40PM +0100, Mark Brown wrote:

...

...

So extend Morimoto-san's work on the simple card for this - that's what it's there for, it's doing exactly this job for non-DT systems but it just didn't get DT support added yet. All the trivial cards should end up using this.

...

It's quite rediculous to request that the simple card stuff is expanded at this time, when you're also telling us that we must use DPCM for Kirkwood,

That's the place to put the sort of shared infrastructure for trivial cards that Sebastian said he wanted to see - if you guys are saying that the machines should all be able to use a trivial binding with shared code that's where that code should be.

Sigh, you completely miss the point.

What all three of us are ultimately after is a DT description for the kirkwood stuff which covers all its use cases. The use case which all three of us have in common is the Cubox, which is the one which needs the spdif stuff to work.

Now, what you've said to date is:

1. you want kirkwood to use DPCM. 2. you want kirkwood using people to use the simple card stuff with the kirkwood driver you want to use DPCM.

To make it work with DPCM, we first need to know what DPCM requires, which means we either have to have the knowledge of DPCM and/or have a working implementation. We don't have either of those yet.

So, I again state plainly that what you're asking is for people to come up with a DT description for a DPCM implementation when we haven't yet got a working DPCM implementation, even without DT.

It is this which I assert is a completely rediculous request at this moment in time for the reasons stated in my previous email and repeated in this email.

On Fri, Aug 09, 2013 at 08:44:34PM +0100, Mark Brown wrote:

If someone wants to it should also be possible to convert the existing platforms without S/PDIF support over to DT, providing you don't mind changing the code once the DPCM and S/PDIF support is added and a bit of thought is put into where the S/PDIF output will fit into the bindings.

Okay, so you're thinking that the I2S output will be enabled in the absence of DPCM? If so, that tells me that you don't understand my patches, and this is getting *really* tiresome.

One more time: - There are two outputs from the FIFO. - There is an I2S output, and there is a SPDIF output. - All hardware has an I2S output. - Some hardware also has the SPDIF output. - Each output is individually enable-able via separate bits. - When both outputs are used, both must be enabled simultaneously. - Otherwise, only one output must be enabled at any one time. - At least one output must be enabled for there to be any activity from the unit at all (that's obvious!)

So, what I'm doing is providing _two_ AIF connection points, one for I2S and one for SPDIF. The appropriate AIF connection point must be attached to for the appropriate output to be enabled.

That means that if you want to use the SPDIF output, the SPDIF AIF must be linked to the codec. If you want to use the I2S output, the I2S AIF must be linked to the codec.

Without this, there's no way for the CPU DAI to know which output(s) are in use, and therefore which should be enabled.

What this means is that the conventional setup where you have _one_ DAI link connecting the codec DAI to the CPU DAI won't work on Kirkwood anymore, because there is no way to know which of the two outputs should be enabled. I avoided that in my patches, but you've objected to that saying that it must use DPCM. That makes the whole of kirkwood entirely DPCM only, whether or not you have a single codec to connect.

Surely you realise this, because you must have read the patches properly before you commented on them, and realised that one of spdifdo or i2sdo must be powered up to set either enable bit?

On Sat, Aug 10, 2013 at 12:42:09AM +0100, Mark Brown wrote:

On Fri, Aug 09, 2013 at 09:38:47PM +0100, Russell King - ARM Linux wrote:

...

On Fri, Aug 09, 2013 at 08:44:34PM +0100, Mark Brown wrote:

...

...

If someone wants to it should also be possible to convert the existing platforms without S/PDIF support over to DT, providing you don't mind changing the code once the DPCM and S/PDIF support is added and a bit of thought is put into where the S/PDIF output will fit into the bindings.

...

Okay, so you're thinking that the I2S output will be enabled in the absence of DPCM? If so, that tells me that you don't understand my

When I say that it should be possible to convert the existing machines over to DT I'm talking about the machines that are supported with the drivers currently in mainline. These machines presumably work just fine with the existing drivers which support the I2S only subset of the possible functionality in the hardware without using DCPM. These are the the machines that could have DT added now without implementing the driver improvements to enable the extra hardware functionality.

When I say that they will need changing once DPCM and S/PDIF support is added what I mean is that those changes to the CPU side drivers should, as you correctly say, require all Kirkwood machine drivers to use DPCM even if only due to the handling of simultaneous startup of the S/PDIF and I2S interfaces.

The "you" in the above quote was intended to refer to people working on Kirkwood in general, not you personally unless you want to.

...

What this means is that the conventional setup where you have _one_ DAI link connecting the codec DAI to the CPU DAI won't work on Kirkwood anymore, because there is no way to know which of the two outputs should be enabled. I avoided that in my patches, but you've objected to that saying that it must use DPCM. That makes the whole of kirkwood entirely DPCM only, whether or not you have a single codec to connect.

Right, that's where the drivers should end up. In terms of the device tree I would expect that it should be the case that there are distinct I2S and S/PDIF interfaces visible, or at least that there's some way to identify which interface on the SoC is being referred to - this is the thought about where the S/PDIF output will fit into the bindings that I mentioned being required.

So long as only I2S is used in the system (or at least any S/PDIF that is present is ignored at runtime) it should be possible to continue using the existing driver infrastructure until S/PDIF and DPCM support is added and then transition the existing drivers (both non-DT and any DT ones that get added or converted) over to that as part of the conversion.

In other words I was talking about a possible intermediate state where people are working with the DT bindings but the driver support for systems with S/PDIF in use is not there yet. That's not required but it should be possible if there's interest in progressing the DT bindings while the driver is as it stands.

Right, so what you're proposing is to come up with a DT description for the existing stuff, and then have to change (or at the very least augment) that description later when the DPCM stuff goes in.

What should be done is to sort out DPCM, get that working and then sort out the DT side of it, so that everyone gets only one transition to DT, not a transition to a half-baked stop-gap DT and then have to go through another round of DT changes. "Because we can" is not a good enough reason not to get this sorted properly.

On 08/09/2013 11:34 AM, Sebastian Hesselbarth wrote:

On 08/09/13 11:19, Mark Brown wrote:

...

On Fri, Aug 09, 2013 at 10:23:50AM +0200, Sebastian Hesselbarth wrote:

...

On 08/08/2013 01:22 PM, Jean-Francois Moine wrote:

...

...

+i2s1: audio-controller@b4000 {

• compatible = "mrvl,mvebu-audio";
• reg = <0xb4000 0x2210>;
• interrupts = <21>, <22>;
• clocks = <&gate_clk 13>;
• clock-names = "internal";

+};

...

Also we will need some phandle reference to the audio codec here. As this property is ongoing work in ASoC core, I suggest we wait for it and propose a binding afterwards.

No, as discussed this should be in the binding for the audio subsystem not in the binding for an individual component in that subsystem.

Mark,

I do understand there may be SoCs requiring sophisticated extra audio nodes, but Marvell SoCs don't. I prefer having a single node for the i2s controller *and* exploit the audio subsystem properties from that.

It's not about SoCs, it's about the board. The audio fabric on a board can easily get complex enough to require its own driver. Speakers, mics, jacks and jack detection, external amplifiers, bluetooth, baseband, multiple CODECs. That's what the audio node describes.

- Lars