| Message ID | 20130828113459.48ecbb34@armhf (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/28/2013 11:34 AM, Jean-Francois Moine wrote: > This patch adds the nodes to instantiate the audio devices of the Dove > boards. > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > --- > arch/arm/boot/dts/dove.dtsi | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi > index 499abad..78227e2 100644 > --- a/arch/arm/boot/dts/dove.dtsi > +++ b/arch/arm/boot/dts/dove.dtsi > @@ -573,6 +573,24 @@ > phy-handle = <ðphy>; > }; > }; > + > + i2s0: audio-controller@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 > + 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"; > + }; > }; > }; > }; > >
Sebastian, Jean-François, On Wed, 28 Aug 2013 12:13:07 +0200, Sebastian Hesselbarth wrote: > On 08/28/2013 11:34 AM, Jean-Francois Moine wrote: > > This patch adds the nodes to instantiate the audio devices of the Dove > > boards. > > > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > > --- > > arch/arm/boot/dts/dove.dtsi | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi > > index 499abad..78227e2 100644 > > --- a/arch/arm/boot/dts/dove.dtsi > > +++ b/arch/arm/boot/dts/dove.dtsi > > @@ -573,6 +573,24 @@ > > phy-handle = <ðphy>; > > }; > > }; > > + > > + i2s0: audio-controller@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? 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
On 08/28/2013 12:19 PM, Thomas Petazzoni wrote: > Sebastian, Jean-François, > > On Wed, 28 Aug 2013 12:13:07 +0200, Sebastian Hesselbarth wrote: >> On 08/28/2013 11:34 AM, Jean-Francois Moine wrote: >>> This patch adds the nodes to instantiate the audio devices of the Dove >>> boards. >>> >>> Signed-off-by: Jean-Francois Moine <moinejf@free.fr> >>> --- >>> arch/arm/boot/dts/dove.dtsi | 18 ++++++++++++++++++ >>> 1 file changed, 18 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi >>> index 499abad..78227e2 100644 >>> --- a/arch/arm/boot/dts/dove.dtsi >>> +++ b/arch/arm/boot/dts/dove.dtsi >>> @@ -573,6 +573,24 @@ >>> phy-handle = <ðphy>; >>> }; >>> }; >>> + >>> + i2s0: audio-controller@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? > > 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, On Wed, 28 Aug 2013 12:26:31 +0200, Sebastian Hesselbarth wrote: > >> 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. > > 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. > 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
On 08/28/13 13:15, Thomas Petazzoni wrote: > On Wed, 28 Aug 2013 12:26:31 +0200, Sebastian Hesselbarth wrote: >>>> 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. >> >> 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. >> 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, On Wed, 28 Aug 2013 13:44:51 +0200, Sebastian Hesselbarth wrote: > > 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? > >> 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. 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
On Wed, Aug 28, 2013 at 01:58:27PM +0200, Thomas Petazzoni wrote: > Dear Sebastian Hesselbarth, > > On Wed, 28 Aug 2013 13:44:51 +0200, Sebastian Hesselbarth wrote: > > > > 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. > 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"; > }; ... 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.
On 08/28/13 13:58, Thomas Petazzoni wrote: > On Wed, 28 Aug 2013 13:44:51 +0200, Sebastian Hesselbarth wrote: >>>> 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. > > 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; 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 > }; > > audio1 { > reg = <... ...>; > compatible = "marvell,kirkwood-audio"; > };
Dear Russell King - ARM Linux, On Wed, 28 Aug 2013 13:13:20 +0100, Russell King - ARM Linux wrote: > > > 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. > 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. > > 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"; > > }; > > ... 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"). > 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@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
On Wed, Aug 28, 2013 at 02:29:09PM +0200, Thomas Petazzoni wrote: > Dear Russell King - ARM Linux, > > On Wed, 28 Aug 2013 13:13:20 +0100, Russell King - ARM Linux wrote: > > > > > 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. We're not talking about replacing a pre-existing string, we're talking about adding one, which is a different situation. > > 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. > > > > 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"; > > > }; > > > > ... 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... > 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. > > 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, On Wed, 28 Aug 2013 13:42:55 +0100, Russell King - ARM Linux wrote: > > 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. > > 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. > > > ... 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... 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. > > 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). > > > 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. 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
On Wed, Aug 28, 2013 at 02:51:51PM +0200, Thomas Petazzoni wrote: > 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. > 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. > 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. :) > 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. On 08/28/2013 01:34 PM, Jean-Francois Moine wrote: > This patch adds the nodes to instantiate the audio devices of the Dove > boards. > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > --- > arch/arm/boot/dts/dove.dtsi | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi > index 499abad..78227e2 100644 > --- a/arch/arm/boot/dts/dove.dtsi > +++ b/arch/arm/boot/dts/dove.dtsi > @@ -573,6 +573,24 @@ > phy-handle = <ðphy>; > }; > }; > + > + i2s0: audio-controller@b0000 { According to ePAPR [1] the node should be called "sound", not "audio-controller". > + compatible = "marvell,mvebu-audio"; > + reg = <0xb0000 0x2210>; > + interrupts = <19>, <20>; > + clocks = <&gate_clk 12>; > + clock-names = "internal"; > + status = "disabled"; > + }; > + > + i2s1: audio-controller@b4000 { Same comment. > + 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
On Wed, 28 Aug 2013 23:49:12 +0400 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > > diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi > > index 499abad..78227e2 100644 > > --- a/arch/arm/boot/dts/dove.dtsi > > +++ b/arch/arm/boot/dts/dove.dtsi > > @@ -573,6 +573,24 @@ > > phy-handle = <ðphy>; > > }; > > }; > > + > > + i2s0: audio-controller@b0000 { > > According to ePAPR [1] the node should be called "sound", not > "audio-controller". > > > + 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"; };
On Wed, 28 Aug 2013 13:15:48 +0200 Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > 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.
On Wed, 28 Aug 2013 14:16:32 +0200 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: [snip] > > 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. 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.
On Thu, Aug 29, 2013 at 12:07:04PM +0200, Jean-Francois Moine wrote: > On Wed, 28 Aug 2013 14:16:32 +0200 > Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > > [snip] > > > 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. > > 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. 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.
On 08/29/13 12:13, Russell King - ARM Linux wrote: > On Thu, Aug 29, 2013 at 12:07:04PM +0200, Jean-Francois Moine wrote: >> On Wed, 28 Aug 2013 14:16:32 +0200 >> Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: >> >> [snip] >>>> 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. >> >> 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. > > 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. > 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 > 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? > 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. On 29-08-2013 13:38, Jean-Francois Moine wrote: >>> diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi >>> index 499abad..78227e2 100644 >>> --- a/arch/arm/boot/dts/dove.dtsi >>> +++ b/arch/arm/boot/dts/dove.dtsi >>> @@ -573,6 +573,24 @@ >>> phy-handle = <ðphy>; >>> }; >>> }; >>> + >>> + i2s0: audio-controller@b0000 { >> According to ePAPR [1] the node should be called "sound", not >> "audio-controller". >>> + 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"; > }; Well, then "sound-controller" sounds somewhat more appropriate. WBR, Sergei
On Wed, Aug 28, 2013 at 12:26:31PM +0200, Sebastian Hesselbarth wrote: > 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. > 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...
On Thu, Aug 29, 2013 at 05:12:17PM +0100, Mark Brown wrote: > 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.
On Thu, Aug 29, 2013 at 05:33:58PM +0100, Russell King - ARM Linux wrote: > On Thu, Aug 29, 2013 at 05:12:17PM +0100, Mark Brown wrote: > > On Wed, Aug 28, 2013 at 12:26:31PM +0200, Sebastian Hesselbarth wrote: > > > 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... > 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.
On 08/29/13 19:12, Mark Brown wrote: > On Thu, Aug 29, 2013 at 05:33:58PM +0100, Russell King - ARM Linux wrote: >> On Thu, Aug 29, 2013 at 05:12:17PM +0100, Mark Brown wrote: >>> On Wed, Aug 28, 2013 at 12:26:31PM +0200, Sebastian Hesselbarth wrote: > >>>> 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... > >> 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. > 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
On Thu, Aug 29, 2013 at 08:02:27PM +0200, Sebastian Hesselbarth wrote: > > 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? > 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.
On Thu, Aug 29, 2013 at 07:20:11PM +0100, Mark Brown wrote: > On Thu, Aug 29, 2013 at 08:02:27PM +0200, Sebastian Hesselbarth wrote: > > > > 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.
On Thu, Aug 29, 2013 at 01:01:23PM +0200, Sebastian Hesselbarth wrote: > 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. 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.
diff --git a/arch/arm/boot/dts/dove.dtsi b/arch/arm/boot/dts/dove.dtsi index 499abad..78227e2 100644 --- a/arch/arm/boot/dts/dove.dtsi +++ b/arch/arm/boot/dts/dove.dtsi @@ -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"; + }; }; }; };
This patch adds the nodes to instantiate the audio devices of the Dove boards. Signed-off-by: Jean-Francois Moine <moinejf@free.fr> --- arch/arm/boot/dts/dove.dtsi | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)