ARM: Dove: Add the audio devices in DT
PATCH 1/2 ARM: Dove: Add the audio devices in DT
This patch adds the nodes to instantiate the audio devices of the Dove boards.
--- arch/arm/boot/dts/dove.dtsi | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
arch/arm/boot/dts/dove.dtsi +18 -0
@@ -573,6 +573,24 @@ phy-handle = <ðphy>; }; }; + + i2s0: audio-controller@b0000 { + compatible = "marvell,mvebu-audio"; + reg = <0xb0000 0x2210>; + interrupts = <19>, <20>; + clocks = <&gate_clk 12>; + clock-names = "internal"; + status = "disabled"; + }; + + i2s1: audio-controller@b4000 { + compatible = "marvell,mvebu-audio"; + reg = <0xb4000 0x2210>; + interrupts = <21>, <22>; + clocks = <&gate_clk 13>; + clock-names = "internal"; + status = "disabled"; + }; }; }; };
Signature · 2 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
Quoted 13 lines · Jean-Francois Moine
This patch adds the nodes to instantiate the audio devices of the Dove boards.
Signed-off-by Jean-Francois Moine <[email protected]>--- arch/arm/boot/dts/dove.dtsi | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)arch/arm/boot/dts/dove.dtsi +0
-0
arch/arm/boot/dts/dove.dtsi line 578
Quoted 6 lines
arch/arm/boot/dts/dove.dtsi +3
-0@@ -573,6 +573,24 @@ phy-handle = <ðphy>; }; }; + + i2s0: audio-controller at b0000 { + compatible = "marvell,mvebu-audio";
[added Gregory to Cc]
Jean-Francois,
as Mark Brown already took the bindings patch for above generic compatible, how are we going to discriminate different implementations/features of Dove, Kirkwood, and Armada 370?
Sebastian
Quoted 20 lines
arch/arm/boot/dts/dove.dtsi +15
-0+ reg = <0xb0000 0x2210>; + interrupts = <19>, <20>; + clocks = <&gate_clk 12>; + clock-names = "internal"; + status = "disabled"; + }; + + i2s1: audio-controller at b4000 { + compatible = "marvell,mvebu-audio"; + reg = <0xb4000 0x2210>; + interrupts = <21>, <22>; + clocks = <&gate_clk 13>; + clock-names = "internal"; + status = "disabled"; + }; }; }; };
Sebastian, Jean-Fran?ois,
Quoted 23 lines · Sebastian Hesselbarth
Quoted 13 lines · Jean-Francois Moine
This patch adds the nodes to instantiate the audio devices of the Dove boards.
Signed-off-by Jean-Francois Moine <[email protected]>--- arch/arm/boot/dts/dove.dtsi | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)arch/arm/boot/dts/dove.dtsi +0
-0arch/arm/boot/dts/dove.dtsi line 578
Quoted 6 lines
arch/arm/boot/dts/dove.dtsi +3
-0@@ -573,6 +573,24 @@ phy-handle = <ðphy>; }; }; + + i2s0: audio-controller at b0000 { + compatible = "marvell,mvebu-audio";[added Gregory to Cc]
Quoted 5 lines
Jean-Francois,
as Mark Brown already took the bindings patch for above generic compatible, how are we going to discriminate different implementations/features of Dove, Kirkwood, and Armada 370?
I agree that mvebu-audio is not a really good compatible string. It should use the first SoC that introduced the IP block, so that if future SOCs have variations, we can introduce separate compatible strings.
So for now, the compatible string should be kirkwood-audio.
Thanks,
Thomas
Signature · 4 lines
Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
Quoted 32 lines · Thomas Petazzoni
Sebastian, Jean-Fran?ois,
Quoted 23 lines · Sebastian Hesselbarth
Quoted 13 lines · Jean-Francois Moine
This patch adds the nodes to instantiate the audio devices of the Dove boards.
Signed-off-by Jean-Francois Moine <[email protected]>--- arch/arm/boot/dts/dove.dtsi | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)arch/arm/boot/dts/dove.dtsi +0
-0arch/arm/boot/dts/dove.dtsi line 578
Quoted 6 lines
arch/arm/boot/dts/dove.dtsi +3
-0@@ -573,6 +573,24 @@ phy-handle = <ðphy>; }; }; + + i2s0: audio-controller at b0000 { + compatible = "marvell,mvebu-audio";[added Gregory to Cc]
Quoted 5 lines
Jean-Francois,
as Mark Brown already took the bindings patch for above generic compatible, how are we going to discriminate different implementations/features of Dove, Kirkwood, and Armada 370?
Quoted 6 lines
I agree that mvebu-audio is not a really good compatible string. It should use the first SoC that introduced the IP block, so that if future SOCs have variations, we can introduce separate compatible strings.
So for now, the compatible string should be kirkwood-audio.
Unfortunately, mvebu-audio has already been taken by Mark. Also, we know the differences for the three SoCs now and should have a compatible for each (and maybe mvebu-audio for fallback).
Also, we'll need to distinguish between the different audio controllers on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg base passed.
Sebastian
Dear Sebastian Hesselbarth,
Quoted 9 lines · Sebastian Hesselbarth
Quoted 9 lines
Quoted 3 lines
as Mark Brown already took the bindings patch for above generic compatible, how are we going to discriminate different implementations/features of Dove, Kirkwood, and Armada 370?
I agree that mvebu-audio is not a really good compatible string. It should use the first SoC that introduced the IP block, so that if future SOCs have variations, we can introduce separate compatible strings.
Quoted 5 lines
Quoted 1 line
So for now, the compatible string should be kirkwood-audio.
Unfortunately, mvebu-audio has already been taken by Mark. Also, we know the differences for the three SoCs now and should have a compatible for each (and maybe mvebu-audio for fallback).
For 3.12, right? So 3.12 hasn't been released yet, so it's still time to fix this.
Quoted 3 lines
Also, we'll need to distinguish between the different audio controllers on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg base passed.
For what reason does the driver needs to know whether it's the instance 0 or instance 1 ? If it's needed for some specific reason, then there should probably be something like marvell,i2s-channel-id = <0> and marvell,i2s-channel-id = <1>.
Best regards,
Thomas
Signature · 4 lines
Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
Quoted 10 lines · Thomas Petazzoni
Quoted 9 lines · Sebastian Hesselbarth
Quoted 9 lines
Quoted 3 lines
as Mark Brown already took the bindings patch for above generic compatible, how are we going to discriminate different implementations/features of Dove, Kirkwood, and Armada 370?
I agree that mvebu-audio is not a really good compatible string. It should use the first SoC that introduced the IP block, so that if future SOCs have variations, we can introduce separate compatible strings.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
So for now, the compatible string should be kirkwood-audio.
Unfortunately, mvebu-audio has already been taken by Mark. Also, we know the differences for the three SoCs now and should have a compatible for each (and maybe mvebu-audio for fallback).
For 3.12, right? So 3.12 hasn't been released yet, so it's still time to fix this.
I guess, yes.
Quoted 8 lines
Quoted 3 lines
Also, we'll need to distinguish between the different audio controllers on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg base passed.
For what reason does the driver needs to know whether it's the instance 0 or instance 1 ? If it's needed for some specific reason, then there should probably be something like marvell,i2s-channel-id = <0> and marvell,i2s-channel-id = <1>.
On Dove, audio1 has SPDIF out, audio0 hasn't. Russell also mentioned to get rid of "i2s" and use "audio" instead. Most SoC's controllers are i2s only but as soon as SPDIF comes into play, it is a different interface protocol.
I am fine with having a "marvell,channel-id" (no "i2s") to discriminate the instances, although reg offset should be sufficient.
Jean-Francois: Can you please also rename the DT nodes to "audio0" and "audio1" instead?
Sebastian
Dear Sebastian Hesselbarth,
Quoted 4 lines · Sebastian Hesselbarth
Quoted 2 lines
For 3.12, right? So 3.12 hasn't been released yet, so it's still time to fix this.
I guess, yes.
Jean-Fran?ois, could you cook and submit a patch to change the compatible string?
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
Also, we'll need to distinguish between the different audio controllers on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg base passed.
For what reason does the driver needs to know whether it's the instance 0 or instance 1 ? If it's needed for some specific reason, then there should probably be something like marvell,i2s-channel-id = <0> and marvell,i2s-channel-id = <1>.
Quoted 7 lines
On Dove, audio1 has SPDIF out, audio0 hasn't. Russell also mentioned to get rid of "i2s" and use "audio" instead. Most SoC's controllers are i2s only but as soon as SPDIF comes into play, it is a different interface protocol.
I am fine with having a "marvell,channel-id" (no "i2s") to discriminate the instances, although reg offset should be sufficient.
Well, the reg offset is a possibility, but it's not really nice, and would have to be adapted to each and every SoC even if the reset of the audio IP is the same.
Though, if the difference between the two units is the availability of SPDIF support, then we shouldn't encode the channel number, but instead the availability of SPDIF, i.e:
audio0 {
reg = <... ...>;
compatible = "marvell,kirkwood-audio";
marvell,has-spdif;
};
audio1 {
reg = <... ...>;
compatible = "marvell,kirkwood-audio";
};Thomas
Signature · 4 lines
Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
Quoted 11 lines · Thomas Petazzoni
Dear Sebastian Hesselbarth,
Quoted 4 lines · Sebastian Hesselbarth
Quoted 2 lines
For 3.12, right? So 3.12 hasn't been released yet, so it's still time to fix this.
I guess, yes.
Jean-Fran?ois, could you cook and submit a patch to change the compatible string?
I don't think this is a good idea. The configuration of this IP is not based on the SoC as a single SoC can have a mixture of different configurations.
I think marvell,mvebu-audio is a reasonable compatible string for this, and that the different configurations should be described by properties indicating which inputs and outputs have been implemented.
For instance, on the Dove, there are two of these blocks. One has I2S in and out only, but the other block has I2S in and out, and SPDIF out. On some other Marvell devices, this block has I2S in and out and SPDIF in and out.
Otherwise, they're functionally the same.
Quoted 4 lines
Though, if the difference between the two units is the availability of SPDIF support, then we shouldn't encode the channel number, but instead the availability of SPDIF, i.e:
Quoted 10 lines
audio0 {
reg = <... ...>;
compatible = "marvell,kirkwood-audio";
marvell,has-spdif;
};
audio1 {
reg = <... ...>;
compatible = "marvell,kirkwood-audio";
};... which means there's no problem with using marvell,mvebu-audio as the compatible string if you're going to use properties to describe what facilities are available.
In any case "marvell,has-spdif" is too generic - as I've indicated above, there's versions with spdif out, and other versions with spdif in and out.
Quoted 22 lines · Thomas Petazzoni
Quoted 9 lines · Sebastian Hesselbarth
Quoted 8 lines
Quoted 3 lines
Also, we'll need to distinguish between the different audio controllers on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg base passed.
For what reason does the driver needs to know whether it's the instance 0 or instance 1 ? If it's needed for some specific reason, then there should probably be something like marvell,i2s-channel-id = <0> and marvell,i2s-channel-id = <1>.
Quoted 7 lines
On Dove, audio1 has SPDIF out, audio0 hasn't. Russell also mentioned to get rid of "i2s" and use "audio" instead. Most SoC's controllers are i2s only but as soon as SPDIF comes into play, it is a different interface protocol.
I am fine with having a "marvell,channel-id" (no "i2s") to discriminate the instances, although reg offset should be sufficient.
Well, the reg offset is a possibility, but it's not really nice, and would have to be adapted to each and every SoC even if the reset of the audio IP is the same.
Quoted 8 lines
Though, if the difference between the two units is the availability of SPDIF support, then we shouldn't encode the channel number, but instead the availability of SPDIF, i.e:
audio0 { reg = <... ...>; compatible = "marvell,kirkwood-audio"; marvell,has-spdif;
Agree, if you make it "marvell,has-spdif-in" and "marvell,has-spdif-out" Dove has either i2s-only or i2s+spdifo, kirkwood has i2s+spdifo+spdifi for the one audio controller available. Can't tell for Armada 370.
BTW, you might have followed some of the DT discussions with Mark before; as he insists on having a separate sound card node, he might argue that above property should be part of that node instead.
Last patch discussion [1] I followed on some spdif sound nodes, took the patch up to v11 or so. Mainly, because the author updated it too quickly, but looks like audio bindings are (still) worth a lot of discussion.
[1] http://comments.gmane.org/gmane.linux.alsa.devel/112004
Sebastian
Quoted 6 lines
};
audio1 {
reg = <... ...>;
compatible = "marvell,kirkwood-audio";
};Dear Russell King - ARM Linux,
Quoted 8 lines · Russell King - ARM Linux
Quoted 4 lines
Quoted 1 line
I guess, yes.
Jean-Fran?ois, could you cook and submit a patch to change the compatible string?
I don't think this is a good idea. The configuration of this IP is not based on the SoC as a single SoC can have a mixture of different configurations.
Using the name of the oldest SoC in the family that had the IP block is the norm, because it's really what "compatible" means: the IP block in Dove is *compatible* with the one that was originally introduced in Kirkwood.
See what Rob Herring (one of the DT maintainer) says in
http://lists.infradead.org/pipermail/linux-mtd/2012-March/040417.html:
"""
There is no reason all machines can't use "st,spear600-smi" in their dts. It doesn't have to be a spear600, just compatible with it. Really you want the string to be the oldest SOC the block is in and then newer SOCs can claim compatibility with the old version.
"""
The thread was precisely about replacing a SoC-specific compatible string "st,spear600-smi" by a more generic "st,spear-smi" and Rob Herring (above) was opposing to that.
Quoted 10 lines
I think marvell,mvebu-audio is a reasonable compatible string for this, and that the different configurations should be described by properties indicating which inputs and outputs have been implemented.
For instance, on the Dove, there are two of these blocks. One has I2S in and out only, but the other block has I2S in and out, and SPDIF out. On some other Marvell devices, this block has I2S in and out and SPDIF in and out.
Otherwise, they're functionally the same.
Right, that's why they can both use "kirkwood-audio" as the compatible string.
Quoted 10 lines
Quoted 10 lines
Though, if the difference between the two units is the availability of SPDIF support, then we shouldn't encode the channel number, but instead the availability of SPDIF, i.e:
audio0 { reg = <... ...>; compatible = "marvell,kirkwood-audio"; marvell,has-spdif; };
Quoted 8 lines
Quoted 4 lines
audio1 { reg = <... ...>; compatible = "marvell,kirkwood-audio"; };... which means there's no problem with using marvell,mvebu-audio as the compatible string if you're going to use properties to describe what facilities are available.
I disagree, because how do you know if a future "mvebu" SOC such as Armada 370, or one that doesn't exist yet, will not have a different audio IP block? It will still be audio, it will still be mvebu, but it will not be able to use a "marvell,mvebu-audio" driver. Or maybe it can use the same driver, but with a few variations, so a different compatible string will be needed to identify the original IP ("marvell,kirkwood-audio", used on Kirkwood/Dove) and slightly newer versions of the IP ("marvell,some-funky-soc-audio").
Quoted 3 lines
In any case "marvell,has-spdif" is too generic - as I've indicated above, there's versions with spdif out, and other versions with spdif in and out.
Right, the above was just an example to illustrate that we can have additional properties to encode the differences between each instance of the audio devices.
For example, for XOR engines, we have:
xor at 60900 {
compatible = "marvell,orion-xor";
reg = <0x60900 0x100
0x60b00 0x100>;
clocks = <&gateclk 22>;
status = "okay";
xor10 {
interrupts = <51>;
dmacap,memcpy;
dmacap,xor;
};
xor11 {
interrupts = <52>;
dmacap,memcpy;
dmacap,xor;
dmacap,memset;
};
};because the first channel of each XOR engine has only memcpy and xor capabilities, while the second channel has memcpy, xor and memset capabilities.
Thanks,
Thomas
Signature · 4 lines
Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
Quoted 28 lines · Thomas Petazzoni
Dear Russell King - ARM Linux,
Quoted 8 lines · Russell King - ARM Linux
Quoted 4 lines
Quoted 1 line
I guess, yes.
Jean-Fran?ois, could you cook and submit a patch to change the compatible string?
I don't think this is a good idea. The configuration of this IP is not based on the SoC as a single SoC can have a mixture of different configurations.
Using the name of the oldest SoC in the family that had the IP block is the norm, because it's really what "compatible" means: the IP block in Dove is *compatible* with the one that was originally introduced in Kirkwood.
See what Rob Herring (one of the DT maintainer) says in
http://lists.infradead.org/pipermail/linux-mtd/2012-March/040417.html:"""
There is no reason all machines can't use "st,spear600-smi" in their dts. It doesn't have to be a spear600, just compatible with it. Really you want the string to be the oldest SOC the block is in and then newer SOCs can claim compatibility with the old version.
"""
Quoted 3 lines
The thread was precisely about replacing a SoC-specific compatible string "st,spear600-smi" by a more generic "st,spear-smi" and Rob Herring (above) was opposing to that.
We're not talking about replacing a pre-existing string, we're talking about adding one, which is a different situation.
Quoted 29 lines
Quoted 10 lines
I think marvell,mvebu-audio is a reasonable compatible string for this, and that the different configurations should be described by properties indicating which inputs and outputs have been implemented.
For instance, on the Dove, there are two of these blocks. One has I2S in and out only, but the other block has I2S in and out, and SPDIF out. On some other Marvell devices, this block has I2S in and out and SPDIF in and out.
Otherwise, they're functionally the same.
Right, that's why they can both use "kirkwood-audio" as the compatible string.
Quoted 15 lines
Quoted 14 lines
Though, if the difference between the two units is the availability of SPDIF support, then we shouldn't encode the channel number, but instead the availability of SPDIF, i.e:
audio0 { reg = <... ...>; compatible = "marvell,kirkwood-audio"; marvell,has-spdif; }; audio1 { reg = <... ...>; compatible = "marvell,kirkwood-audio"; };
Quoted 7 lines
Quoted 3 lines
... which means there's no problem with using marvell,mvebu-audio as the compatible string if you're going to use properties to describe what facilities are available.
I disagree, because how do you know if a future "mvebu" SOC such as Armada 370, or one that doesn't exist yet, will not have a different audio IP block?
The Dove already contains _three_ audio blocks, two of which are this one, and another which is block for driving an AC'97 codec (which doesn't have a driver.) That's no problem because you won't call that one an "audio" block but an AC'97 block. So...
Quoted 6 lines
It will still be audio, it will still be mvebu, but it
will not be able to use a "marvell,mvebu-audio" driver. Or maybe it can use the same driver, but with a few variations, so a different compatible string will be needed to identify the original IP ("marvell,kirkwood-audio", used on Kirkwood/Dove) and slightly newer versions of the IP ("marvell,some-funky-soc-audio").
I don't think this really applies.
Quoted 7 lines
Quoted 3 lines
In any case "marvell,has-spdif" is too generic - as I've indicated above, there's versions with spdif out, and other versions with spdif in and out.
Right, the above was just an example to illustrate that we can have additional properties to encode the differences between each instance of the audio devices.
I think this is a mistake too: these properties will just tell us what may be possible, and the driver will take no real action on them. I suppose that a property specifying whether there is a SPDIF output could be used to control whether the IEC958 channel status controls are registered. However...
What's more important is which outputs are actually wired up, and therefore which bits of this hardware are enabled. Even then, we wouldn't want to expose (eg) the IEC958 channel status controls if the SPDIF output isn't wired. So all in all, I don't see any point to a set of properties saying "we have SPDIF" etc. That information should come solely from whether the SPDIF output has been "wired up".
Let me put that another way: we _can_ provide those properties to indicate what facilities the hardware has, we just wouldn't use them at all - and to provide them seems like over-design to me.
Dear Russell King - ARM Linux,
Quoted 15 lines · Russell King - ARM Linux
Quoted 15 lines
Using the name of the oldest SoC in the family that had the IP block is the norm, because it's really what "compatible" means: the IP block in Dove is *compatible* with the one that was originally introduced in Kirkwood.
See what Rob Herring (one of the DT maintainer) says in
http://lists.infradead.org/pipermail/linux-mtd/2012-March/040417.html:"""
There is no reason all machines can't use "st,spear600-smi" in their dts. It doesn't have to be a spear600, just compatible with it. Really you want the string to be the oldest SOC the block is in and then newer SOCs can claim compatibility with the old version.
"""
Quoted 6 lines
Quoted 3 lines
The thread was precisely about replacing a SoC-specific compatible string "st,spear600-smi" by a more generic "st,spear-smi" and Rob Herring (above) was opposing to that.
We're not talking about replacing a pre-existing string, we're talking about adding one, which is a different situation.
I don't see how this makes this a different situation. See for example
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/161065.html
and
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-March/087702.html
where Arnd also said using the oldest SoC that has the same IP block as the compatible string was the right thing to do.
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
... which means there's no problem with using marvell,mvebu-audio as the compatible string if you're going to use properties to describe what facilities are available.
Quoted 8 lines
Quoted 3 lines
I disagree, because how do you know if a future "mvebu" SOC such as Armada 370, or one that doesn't exist yet, will not have a different audio IP block?
The Dove already contains _three_ audio blocks, two of which are this one, and another which is block for driving an AC'97 codec (which doesn't have a driver.) That's no problem because you won't call that one an "audio" block but an AC'97 block. So...
And? If that's a different IP block, it'll have a different compatible string, that's it.
That doesn't change my point: using "marvell,mvebu-audio" as the compatible string is stupid, because you have absolutely no idea what the future of audio in mvebu SOCs will be. However, you do know, *today* that Kirkwood and Dove have compatible IP blocks for audio, and that they were first introduced with Kirkwood.
Quoted 8 lines
Quoted 6 lines
It will still be audio, it will still be mvebu, but it
will not be able to use a "marvell,mvebu-audio" driver. Or maybe it can use the same driver, but with a few variations, so a different compatible string will be needed to identify the original IP ("marvell,kirkwood-audio", used on Kirkwood/Dove) and slightly newer versions of the IP ("marvell,some-funky-soc-audio").I don't think this really applies.
It does. We're exactly in this situation, as I will soon be working on Armada 370 audio support, and while the IP looks similar, I have checked all the details to see if it's exactly identical.
And Armada 370 is really a mvebu architecture: it's even supported in mach-mvebu/, while Kirkwood and Dove are not (yet).
Quoted 21 lines
Quoted 7 lines
Quoted 3 lines
In any case "marvell,has-spdif" is too generic - as I've indicated above, there's versions with spdif out, and other versions with spdif in and out.
Right, the above was just an example to illustrate that we can have additional properties to encode the differences between each instance of the audio devices.
I think this is a mistake too: these properties will just tell us what may be possible, and the driver will take no real action on them. I suppose that a property specifying whether there is a SPDIF output could be used to control whether the IEC958 channel status controls are registered. However...
What's more important is which outputs are actually wired up, and therefore which bits of this hardware are enabled. Even then, we wouldn't want to expose (eg) the IEC958 channel status controls if the SPDIF output isn't wired. So all in all, I don't see any point to a set of properties saying "we have SPDIF" etc. That information should come solely from whether the SPDIF output has been "wired up".
Quoted 3 lines
Let me put that another way: we _can_ provide those properties to indicate what facilities the hardware has, we just wouldn't use them at all - and to provide them seems like over-design to me.
I am not arguing about the properties, as I haven't looked at the specific problem that needs to be solved. By suggesting properties, I was merely suggesting one possible solution to the problem that Sebastian was raising, where the different instances of the IP block don't have the same capabilities.
What I am however strongly arguing on is the choice of the compatible string. marvell,mvebu-audio is a wrong choice.
Best regards,
Thomas
Signature · 4 lines
Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
Quoted 6 lines · Thomas Petazzoni
I don't see how this makes this a different situation. See for example
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-April/161065.html
and
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-March/087702.html
where Arnd also said using the oldest SoC that has the same IP block as the compatible string was the right thing to do.
Well, I have a different opinion, and that's all there is to it. However, differing opinions aren't tolerated within the kernel community even if they are equally valid, so I'll just shut up about after this mail.
Quoted 2 lines
And? If that's a different IP block, it'll have a different compatible string, that's it.
There you go, you've just said why there's no problem with using a generic compatible string.
Quoted 3 lines
It does. We're exactly in this situation, as I will soon be working on Armada 370 audio support, and while the IP looks similar, I have checked all the details to see if it's exactly identical.
You seem to have missed something in that paragraph. If it's exactly identical, there isn't a problem. :)
Quoted 2 lines
And Armada 370 is really a mvebu architecture: it's even supported in mach-mvebu/, while Kirkwood and Dove are not (yet).
Maybe it should be mach-kirkwood and not mach-mvebu by your reasoning then!
Hello.
Quoted 13 lines · Jean-Francois Moine
This patch adds the nodes to instantiate the audio devices of the Dove boards.
Signed-off-by Jean-Francois Moine <[email protected]>--- arch/arm/boot/dts/dove.dtsi | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)arch/arm/boot/dts/dove.dtsi +0
-0
arch/arm/boot/dts/dove.dtsi line 577
Quoted 5 lines
arch/arm/boot/dts/dove.dtsi +2
-0@@ -573,6 +573,24 @@ phy-handle = <ðphy>; }; }; + + i2s0: audio-controller at b0000 {
According to ePAPR [1] the node should be called "sound", not
"audio-controller".
arch/arm/boot/dts/dove.dtsi
Quoted 9 lines
arch/arm/boot/dts/dove.dtsi +9
-0+ compatible = "marvell,mvebu-audio"; + reg = <0xb0000 0x2210>; + interrupts = <19>, <20>; + clocks = <&gate_clk 12>; + clock-names = "internal"; + status = "disabled"; + }; + + i2s1: audio-controller at b4000 {
Same comment.
arch/arm/boot/dts/dove.dtsi line 593
Quoted 7 lines
arch/arm/boot/dts/dove.dtsi +7
-0+ compatible = "marvell,mvebu-audio"; + reg = <0xb4000 0x2210>; + interrupts = <21>, <22>; + clocks = <&gate_clk 13>; + clock-names = "internal"; + status = "disabled"; + };
[1] http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf
WBR, Sergei
Quoted 14 lines · On Wed, 28 Aug 2013 23:49:12 +0400 Sergei Shtylyov…
arch/arm/boot/dts/dove.dtsi line 577
Quoted 10 lines
arch/arm/boot/dts/dove.dtsi +2
-0@@ -573,6 +573,24 @@ phy-handle = <ðphy>; }; }; + + i2s0: audio-controller@b0000 {According to ePAPR [1] the node should be called "sound", not
"audio-controller".
Quoted 7 lines
arch/arm/boot/dts/dove.dtsi line 584
Quoted 7 lines
arch/arm/boot/dts/dove.dtsi +7
-0+ compatible = "marvell,mvebu-audio"; + reg = <0xb0000 0x2210>; + interrupts = <19>, <20>; + clocks = <&gate_clk 12>; + clock-names = "internal"; + status = "disabled"; + };
AFAIK, "sound" describes the global audio subsystem. For the Cubox, this will be done by something like:
sound {
compatible = "simple-audio";
audio-controller = <&i2s1>;
audio-codec = <&spdif>;
codec-dai-name = "dit-hifi";
};Signature · 2 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
Quoted 4 lines · On Wed, 28 Aug 2013 13:15:48 +0200 Thomas Petazzoni…
For what reason does the driver needs to know whether it's the instance 0 or instance 1 ? If it's needed for some specific reason, then there should probably be something like marvell,i2s-channel-id = <0> and marvell,i2s-channel-id = <1>.
It seems simpler to use aliases.
Signature · 2 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
On Wed, 28 Aug 2013 14:16:32 +0200
Sebastian Hesselbarth <[email protected]> wrote:
[snip]
Quoted 9 lines
Quoted 8 lines
Though, if the difference between the two units is the availability of SPDIF support, then we shouldn't encode the channel number, but instead the availability of SPDIF, i.e:
audio0 { reg = <... ...>; compatible = "marvell,kirkwood-audio"; marvell,has-spdif;
Quoted 7 lines
Agree, if you make it "marvell,has-spdif-in" and "marvell,has-spdif-out" Dove has either i2s-only or i2s+spdifo, kirkwood has i2s+spdifo+spdifi for the one audio controller available. Can't tell for Armada 370.
BTW, you might have followed some of the DT discussions with Mark before; as he insists on having a separate sound card node, he might argue that above property should be part of that node instead.
Yes. For the Cubox, the card will be described by something like:
sound {
compatible = "simple-audio";
audio-controller = <&audio1>;
audio-codec = <&spdif>;
codec-dai-name = "dit-hifi";
};with:
spdif: spdif {
compatible = "linux,spdif-dit";
};Then, the audio driver will know about s/pdif on the first open.
Signature · 2 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
Quoted 30 lines · Jean-Francois Moine
On Wed, 28 Aug 2013 14:16:32 +0200
Sebastian Hesselbarth <[email protected]> wrote:[snip]Quoted 9 lines
Quoted 8 lines
Though, if the difference between the two units is the availability of SPDIF support, then we shouldn't encode the channel number, but instead the availability of SPDIF, i.e:
audio0 { reg = <... ...>; compatible = "marvell,kirkwood-audio"; marvell,has-spdif;Quoted 7 lines
Agree, if you make it "marvell,has-spdif-in" and "marvell,has-spdif-out" Dove has either i2s-only or i2s+spdifo, kirkwood has i2s+spdifo+spdifi for the one audio controller available. Can't tell for Armada 370.
BTW, you might have followed some of the DT discussions with Mark before; as he insists on having a separate sound card node, he might argue that above property should be part of that node instead.
Yes. For the Cubox, the card will be described by something like:
sound { compatible = "simple-audio"; audio-controller = <&audio1>; audio-codec = <&spdif>; codec-dai-name = "dit-hifi"; };
Quoted 7 lines
with:
spdif: spdif { compatible = "linux,spdif-dit"; };Then, the audio driver will know about s/pdif on the first open.
I can tell that neither of you have taken notice of what I said about the "has" stuff - it's completely useless to the driver, it conveys no useful information.
Moreover, the above isn't going to be the answer to this. With DPCM you need:
1. two DAI links to be setup:
1a. one to connect the CPU DAI to the dummy codec
1b. one to connect the dummy platform/CPU DAI to the SPDIF codec
2. DAPM routes to connect the CPU DAI audio stream(s) to the Codec stream(s)
If you have both I2S and SPDIF, then you need another DAI link and a few more DAPM routes. Even if you have just one codec connected, you will need this structure so that the CPU DAI knows which audio outputs are to be enabled. If no DAPM routes exist, the CPU DAI will not enable any outputs.
Or at least that's the theory when ASoC DPCM eventually works.
Quoted 37 lines · Russell King - ARM Linux
Quoted 36 lines · Jean-Francois Moine
On Wed, 28 Aug 2013 14:16:32 +0200
Sebastian Hesselbarth <[email protected]> wrote:[snip]Quoted 9 lines
Quoted 8 lines
Though, if the difference between the two units is the availability of SPDIF support, then we shouldn't encode the channel number, but instead the availability of SPDIF, i.e:
audio0 { reg = <... ...>; compatible = "marvell,kirkwood-audio"; marvell,has-spdif;Quoted 7 lines
Agree, if you make it "marvell,has-spdif-in" and "marvell,has-spdif-out" Dove has either i2s-only or i2s+spdifo, kirkwood has i2s+spdifo+spdifi for the one audio controller available. Can't tell for Armada 370.
BTW, you might have followed some of the DT discussions with Mark before; as he insists on having a separate sound card node, he might argue that above property should be part of that node instead.
Yes. For the Cubox, the card will be described by something like:
sound { compatible = "simple-audio"; audio-controller = <&audio1>; audio-codec = <&spdif>; codec-dai-name = "dit-hifi"; };with:
spdif: spdif { compatible = "linux,spdif-dit"; };
Quoted 5 lines
Quoted 1 line
Then, the audio driver will know about s/pdif on the first open.
I can tell that neither of you have taken notice of what I said about the "has" stuff - it's completely useless to the driver, it conveys no useful information.
You are right, of course, that has-spdif-foo properties are useless, for the driver and the DT. For a working audio setup, we need to link the codecs anyway and we can put that information in there.
Quoted 6 lines
Moreover, the above isn't going to be the answer to this. With DPCM you need:
1. two DAI links to be setup:
1a. one to connect the CPU DAI to the dummy codec
1b. one to connect the dummy platform/CPU DAI to the SPDIF codec
Taking the sound node above:
sound {
compatible = "whatever-audio";
...
audio-codec = <&rt1234 1>, <&spdif 0>;
audio-codec-names = "i2s", "spdifo";
...
};Each audio-codec phandle with arg links to one specific "port" at some "codec". The audio-codec-names property allows the ASoC driver to determine the local "ports" where the above codecs are connected. The dummy codecs are ASoC specific and must be added by the driver.
Basically, it is the same approach as we have for clocks, maybe the property names are not best suited, but it should give an impression of how it _could_ be done.
Unfortunately, the fsl spdif discussion I referenced earlier ended up with
a) a special machine driver and its own DT binding
b) using that "has" properties approach
Quoted 1 line
2. DAPM routes to connect the CPU DAI audio stream(s) to the Codec stream(s)
I am not yet sure how that could be solved easily with DT, nor if it should go in there at all. But I have not dug into ASoC and DAPM enough.
Can you point out, given the above linking, what you think is missing to allow the driver to find all it needs wrt to DAIs and DAPM?
Quoted 8 lines
If you have both I2S and SPDIF, then you need another DAI link and a few more DAPM routes. Even if you have just one codec connected, you will need this structure so that the CPU DAI knows which audio outputs are to be enabled. If no DAPM routes exist, the CPU DAI will not enable any outputs.
Or at least that's the theory when ASoC DPCM eventually works.
Hello.
Quoted 25 lines · Jean-Francois Moine
Quoted 14 lines
arch/arm/boot/dts/dove.dtsi line 577
Quoted 10 lines
arch/arm/boot/dts/dove.dtsi +2
-0@@ -573,6 +573,24 @@ phy-handle = <ðphy>; }; }; + + i2s0: audio-controller at b0000 {According to ePAPR [1] the node should be called "sound", not
"audio-controller".Quoted 7 lines
arch/arm/boot/dts/dove.dtsi line 584
Quoted 7 lines
arch/arm/boot/dts/dove.dtsi +7
-0+ compatible = "marvell,mvebu-audio"; + reg = <0xb0000 0x2210>; + interrupts = <19>, <20>; + clocks = <&gate_clk 12>; + clock-names = "internal"; + status = "disabled"; + };AFAIK, "sound" describes the global audio subsystem. For the Cubox, this will be done by something like:
Quoted 6 lines
sound {
compatible = "simple-audio";
audio-controller = <&i2s1>;
audio-codec = <&spdif>;
codec-dai-name = "dit-hifi";
};Well, then "sound-controller" sounds somewhat more appropriate.
WBR, Sergei
Quoted 3 lines · Sebastian Hesselbarth
Unfortunately, mvebu-audio has already been taken by Mark. Also, we know the differences for the three SoCs now and should have a compatible for each (and maybe mvebu-audio for fallback).
We're not at the merge window yet so we can always change it. We can also add additional compatible strings to the drivers at any point - the device can list as many as it likes, this is useful if you've got new controllers which are supersets of old ones for example.
Quoted 3 lines
Also, we'll need to distinguish between the different audio controllers on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg base passed.
Why is this required - ideally this would have been mentioned in some of
the previous reviews...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130829/c7c8411d/attachment-0001.sig>
Quoted 2 lines · Mark Brown
Why is this required - ideally this would have been mentioned in some of the previous reviews...
I've mentioned the differences between the blocks to you repeatedly in our massive thread, including that some contain the block with different configs, but it seems you've been completely closed to everything technical that I've ever said.
Quoted 7 lines · Russell King - ARM Linux
Quoted 5 lines · Mark Brown
Quoted 3 lines · Sebastian Hesselbarth
Also, we'll need to distinguish between the different audio controllers on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg base passed.
Quoted 5 lines
Quoted 2 lines
Why is this required - ideally this would have been mentioned in some of the previous reviews...
I've mentioned the differences between the blocks to you repeatedly in our massive thread, including that some contain the block with different
You have described some additional features which will require additional driver support. I would expect that the device tree bindings for these features would be added as the features are added and the DTS files updated, for example by listing additional compatible strings if that was the binding update, as is the normal practice. Obviously any hardware which is not compatible with the current binding should not be being registered using the current binding.
It is not clear from the above comment by Sebastian if he is referring
to the same set of hardware differences or something new - doing things
based on device address is highly unusual, it sounds like something to
do with the integration into the SoC rather than to do with the IP.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130829/45a367c1/attachment.sig>
Quoted 22 lines · Mark Brown
Quoted 7 lines · Russell King - ARM Linux
Quoted 5 lines · Mark Brown
Quoted 3 lines · Sebastian Hesselbarth
Also, we'll need to distinguish between the different audio controllers on a single SoC, i.e. i2s0 and i2s1. I suggest checking the (phys) reg base passed.
Quoted 5 lines
Quoted 2 lines
Why is this required - ideally this would have been mentioned in some of the previous reviews...
I've mentioned the differences between the blocks to you repeatedly in our massive thread, including that some contain the block with different
You have described some additional features which will require additional driver support. I would expect that the device tree bindings for these features would be added as the features are added and the DTS files updated, for example by listing additional compatible strings if that was the binding update, as is the normal practice. Obviously any hardware which is not compatible with the current binding should not be being registered using the current binding.
Quoted 5 lines
It is not clear from the above comment by Sebastian if he is referring to the same set of hardware differences or something new - doing things based on device address is highly unusual, it sounds like something to do with the integration into the SoC rather than to do with the IP.
Mark,
it is referring the same differences Russell already mentioned. But I already came to the conclusion, that we don't need the information in the binding. For example, if you use that controller on Dove and you hook it up for SPDIF-in (which it hasn't), than I consider this a DT bug. No need to double-check that in the driver. From that p-o-v, please just let the current binding as is.
Thomas Petazzoni mentioned earlier, that the _usual_ procedure to name the compatibles is to pick the SoC that the IP appeared in first. But I am also fine with "marvell,mvebu-audio" and adding compatibles for dove or kirkwood _if_ we will ever need them.
Please, just stop fighting over this again - it is not getting anything any further.
Sebastian
Quoted 7 lines · Sebastian Hesselbarth
it is referring the same differences Russell already mentioned. But I already came to the conclusion, that we don't need the information in the binding. For example, if you use that controller on Dove and you hook it up for SPDIF-in (which it hasn't), than I consider this a DT bug. No need to double-check that in the driver. From that p-o-v, please just let the current binding as is.
OK, great - none of these devices have any differences which are visible only within the controller, they're all extra external interfaces?
Quoted 4 lines
Thomas Petazzoni mentioned earlier, that the _usual_ procedure to name the compatibles is to pick the SoC that the IP appeared in first. But I am also fine with "marvell,mvebu-audio" and adding compatibles for dove or kirkwood _if_ we will ever need them.
Yeah, it doesn't make much difference either way so long as the base
name isn't utterly confusing.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130829/45a71d28/attachment.sig>
Quoted 11 lines · Mark Brown
Quoted 7 lines · Sebastian Hesselbarth
it is referring the same differences Russell already mentioned. But I already came to the conclusion, that we don't need the information in the binding. For example, if you use that controller on Dove and you hook it up for SPDIF-in (which it hasn't), than I consider this a DT bug. No need to double-check that in the driver. From that p-o-v, please just let the current binding as is.
OK, great - none of these devices have any differences which are visible only within the controller, they're all extra external interfaces?
Dove Audio 0:
- I2S in
- I2S out
- No SPDIF
Dove Audio 1:
- I2S in
- I2S out
- SPDIF out only
Both these variants occur on the same SoC.
Kirkwood:
- I2S in
- SPDIF in
- I2S out
- SPDIF out
Preconditions:
1. Only one input can be used at any one time.
2. One output can be used or both if enabled simultaneously.
Quoted 10 lines · Sebastian Hesselbarth
Taking the sound node above:
sound { compatible = "whatever-audio"; ... audio-codec = <&rt1234 1>, <&spdif 0>; audio-codec-names = "i2s", "spdifo"; ... };
Quoted 4 lines
Each audio-codec phandle with arg links to one specific "port" at some "codec". The audio-codec-names property allows the ASoC driver to determine the local "ports" where the above codecs are connected. The dummy codecs are ASoC specific and must be added by the driver.
I think it's slightly more complicated than that. Here's what needs to be setup with DPCM - when it eventually works:
1. Two DAI links need to be setup:
1a: .name = "S/PDIF1",
.stream_name = "Audio Playback",
.platform_name = "mvebu-audio.1",
.cpu_dai_name = "mvebu-audio.1",
.codec_name = "snd-soc-dummy",
.codec_dai_name = "snd-soc-dummy-dai",
.dynamic = 1,1b: .name = "Codec",
.stream_name = "IEC958 Playback",
.cpu_dai_name = "snd-soc-dummy-dai",
.platform_name = "snd-soc-dummy",
.no_pcm = 1,
.codec_dai_name = "dit-hifi",
.codec_name = "spdif-dit",This separates the CPU DAI from the codec DAI - the important thing seems to be that the first entry (which connects the CPU DAI to the dummy codec) breaks the automatic DAPM routes between the CPU streams and the codec streams.
2. DAPM routes need to be created between the CPU DAI and Codec DAIs to wire the back together:
static const struct snd_soc_dapm_route routes[] = {
{ "spdif-Playback", NULL, "spdifdo" },
};"spdif-Playback" is the stream name in the spdif-dit codec (I had to change the name from merely "Playback" as otherwise the route gets bound to the dummy codec instead.) "spdifdo" is the audio interface widget name in the CPU DAI, which is connected to the CPU DAI playback stream.
So, if we wanted two codecs, one spdif and one i2s, we would need something like this:
DAI links:
.name = "CPU",
.stream_name = "Audio Playback",
.platform_name = "mvebu-audio.1",
.cpu_dai_name = "mvebu-audio.1",
.codec_name = "snd-soc-dummy",
.codec_dai_name = "snd-soc-dummy-dai",
.dynamic = 1,
.name = "I2S",
.stream_name = "PCM Playback",
.cpu_dai_name = "snd-soc-dummy-dai",
.platform_name = "snd-soc-dummy",
.no_pcm = 1,
.codec_dai_name = "i2s-codec-dai-name",
.codec_name = "i2s-codec-name",
.ops = &..._ops,
.name = "SPDIF",
.stream_name = "IEC958 Playback",
.cpu_dai_name = "snd-soc-dummy-dai",
.platform_name = "snd-soc-dummy",
.no_pcm = 1,
.codec_dai_name = "dit-hifi",
.codec_name = "spdif-dit",with routes like this:
static const struct snd_soc_dapm_route routes[] = {
{ "i2sdi", NULL, "i2s-codec-Capture" },
{ "i2s-codec-Playback", NULL, "i2sdo" },
{ "spdif-Playback", NULL, "spdifdo" },
};If we were to add support for SPDIF recording mode, then we'd need to add an additional route for that.
The pitfalls here are:
1. if we are going to refer to codec streams in DAPM routes, we need all codec implementations to have unique names, or some way for DAPM routes to refer to specific codec streams.
2. being able to effectively represent this in DT.
I think we need a flag in DT to indicate a DPCM configuration, which would trigger the creation of the first DAI link. The second and (optionally) third can be generated from knowing the codec DAI name and the codec name. I think we also need a way to specify arbitary DAPM routes in DT as well.