| Message ID | 20130808132201.2610aef3@armhf |
|---|---|
| State | New |
| Headers | show |
On 08/08/2013 01:22 PM, Jean-Francois Moine wrote: > This patch adds DT support to the audio subsystem of the mvebu family > (Kirkwood, Dove, Armada 370). > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > --- > .../devicetree/bindings/sound/mvebu-audio.txt | 29 ++++++++++++++++++++++ > sound/soc/kirkwood/kirkwood-i2s.c | 26 +++++++++++++++++----- > 2 files changed, 49 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/sound/mvebu-audio.txt b/Documentation/devicetree/bindings/sound/mvebu-audio.txt > new file mode 100644 > index 0000000..7e5fd37 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/mvebu-audio.txt > @@ -0,0 +1,29 @@ > +* mvebu (Kirkwood, Dove, Armada 370) audio controller > + > +Required properties: > + > +- compatible: "mrvl,mvebu-audio" Jean-Francois, we need at least two more compatibles for the audio controller found on Dove and Kirkwood respectively. This is how we are going to distinguish those two, e.g. Kirkwood has SPDIF in which Dove hasn't. Also, we have used "marvell" as prefix for a long time. I know there has been discussion about the stock ticker appreviation, has there been any decision on that already? > +- reg: physical base address of the controller and length of memory mapped > + region. > + > +- interrupts: list of two irq numbers. > + The first irq is used for data flow and the second one is used for errors. > + > +- clocks: one or two phandles. > + The first one is mandatory and defines the internal clock. > + The second one is optional and defines an external clock. > + > +- clock-names: names associated to the clocks: > + "internal" for the internal clock s/internal/dcoclk/ > + "extclk" for the external clock > + > +Example: > + > +i2s1: audio-controller@b4000 { > + compatible = "mrvl,mvebu-audio"; > + reg = <0xb4000 0x2210>; > + interrupts = <21>, <22>; > + clocks = <&gate_clk 13>; > + clock-names = "internal"; > +}; Also we will need some phandle reference to the audio codec here. As this property is ongoing work in ASoC core, I suggest we wait for it and propose a binding afterwards. Sebastian > diff --git a/sound/soc/kirkwood/kirkwood-i2s.c.next b/sound/soc/kirkwood/kirkwood-i2s.c > index e5f3f7a..a4170b4 100644 > --- a/sound/soc/kirkwood/kirkwood-i2s.c.next > +++ b/sound/soc/kirkwood/kirkwood-i2s.c > @@ -22,6 +22,8 @@ > #include <sound/pcm_params.h> > #include <sound/soc.h> > #include <linux/platform_data/asoc-kirkwood.h> > +#include <linux/of.h> > + > #include "kirkwood.h" > > #define DRV_NAME "mvebu-audio" > @@ -453,6 +455,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) > struct snd_soc_dai_driver *soc_dai = &kirkwood_i2s_dai; > struct kirkwood_dma_data *priv; > struct resource *mem; > + struct device_node *np = pdev->dev.of_node; > int err; > > priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > @@ -473,14 +476,16 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) > return -ENXIO; > } > > - if (!data) { > - dev_err(&pdev->dev, "no platform data ?!\n"); > + if (np) { > + priv->burst = 128; /* might be 32 or 128 */ > + } else if (data) { > + priv->burst = data->burst; > + } else { > + dev_err(&pdev->dev, "no DT nor platform data ?!\n"); > return -EINVAL; > } > > - priv->burst = data->burst; > - > - priv->clk = devm_clk_get(&pdev->dev, NULL); > + priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL); > if (IS_ERR(priv->clk)) { > dev_err(&pdev->dev, "no clock\n"); > return PTR_ERR(priv->clk); > @@ -507,7 +512,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) > priv->ctl_rec = KIRKWOOD_RECCTL_SIZE_24; > > /* Select the burst size */ > - if (data->burst == 32) { > + if (priv->burst == 32) { > priv->ctl_play |= KIRKWOOD_PLAYCTL_BURST_32; > priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_32; > } else { > @@ -552,12 +557,21 @@ static int kirkwood_i2s_dev_remove(struct platform_device *pdev) > return 0; > } > > +#ifdef CONFIG_OF > +static struct of_device_id kirkwood_i2s_of_match[] = { > + { .compatible = "mrvl,mvebu-audio" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, kirkwood_i2s_of_match); > +#endif > + > static struct platform_driver kirkwood_i2s_driver = { > .probe = kirkwood_i2s_dev_probe, > .remove = kirkwood_i2s_dev_remove, > .driver = { > .name = DRV_NAME, > .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(kirkwood_i2s_of_match), > }, > }; > >
On Fri, 09 Aug 2013 10:23:50 +0200 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > On 08/08/2013 01:22 PM, Jean-Francois Moine wrote: > > This patch adds DT support to the audio subsystem of the mvebu family > > (Kirkwood, Dove, Armada 370). > > > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > > --- > > .../devicetree/bindings/sound/mvebu-audio.txt | 29 ++++++++++++++++++++++ > > sound/soc/kirkwood/kirkwood-i2s.c | 26 +++++++++++++++++----- > > 2 files changed, 49 insertions(+), 6 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/sound/mvebu-audio.txt b/Documentation/devicetree/bindings/sound/mvebu-audio.txt > > new file mode 100644 > > index 0000000..7e5fd37 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/sound/mvebu-audio.txt > > @@ -0,0 +1,29 @@ > > +* mvebu (Kirkwood, Dove, Armada 370) audio controller > > + > > +Required properties: > > + > > +- compatible: "mrvl,mvebu-audio" > > Jean-Francois, > > we need at least two more compatibles for the audio controller found on > Dove and Kirkwood respectively. This is how we are going to distinguish > those two, e.g. Kirkwood has SPDIF in which Dove hasn't. Sebastian, s/has/hasn't & s/hasn't/has Are 2 compatibles enough, i.e. "mvebu-audio" and "mbevu-audio-spdif"? > Also, we have used "marvell" as prefix for a long time. I know there has > been discussion about the stock ticker appreviation, has there been any > decision on that already? Don't know, but, sure, there are still a lot of "mrvl". > > +- reg: physical base address of the controller and length of memory mapped > > + region. > > + > > +- interrupts: list of two irq numbers. > > + The first irq is used for data flow and the second one is used for errors. > > + > > +- clocks: one or two phandles. > > + The first one is mandatory and defines the internal clock. > > + The second one is optional and defines an external clock. > > + > > +- clock-names: names associated to the clocks: > > + "internal" for the internal clock > > s/internal/dcoclk/ Once, I got this message: On Fri, 26 Jul 2013 10:21:56 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, Jul 26, 2013 at 11:09:13AM +0200, Jean-Francois Moine wrote: > > The A510 documentation uses the names "DCO PLL" for the internal clock > > and "AU_EXTCLK" for the external clock. So, what about "dcopll" and > > "extclk"? > > Stop naming them according to their source. Their _consumer_ names > not _source_ names. But Russell did not tell clearly which name could be the best. BTW, as we are naming the clocks, the 'clk' in "xxxclk" seems redondant... > > + "extclk" for the external clock > > + > > +Example: > > + > > +i2s1: audio-controller@b4000 { > > + compatible = "mrvl,mvebu-audio"; > > + reg = <0xb4000 0x2210>; > > + interrupts = <21>, <22>; > > + clocks = <&gate_clk 13>; > > + clock-names = "internal"; > > +}; > > Also we will need some phandle reference to the audio codec here. As > this property is ongoing work in ASoC core, I suggest we wait for it > and propose a binding afterwards. I don't think that we need any reference to the codec here. The glue is done by the audio device. For example, using the (soon?) extended simple audio card: spdif: spdif { compatible = "linux,spdif-dit"; }; sound { compatible = "linux,simple-audio"; audio-controller = <&i2s1>; audio-codec = <&spdif>; codec-dai-name = "dit-hifi"; };
On Fri, Aug 09, 2013 at 10:23:50AM +0200, Sebastian Hesselbarth wrote: > On 08/08/2013 01:22 PM, Jean-Francois Moine wrote: > >+i2s1: audio-controller@b4000 { > >+ compatible = "mrvl,mvebu-audio"; > >+ reg = <0xb4000 0x2210>; > >+ interrupts = <21>, <22>; > >+ clocks = <&gate_clk 13>; > >+ clock-names = "internal"; > >+}; > Also we will need some phandle reference to the audio codec here. As > this property is ongoing work in ASoC core, I suggest we wait for it > and propose a binding afterwards. No, as discussed this should be in the binding for the audio subsystem not in the binding for an individual component in that subsystem.
On Fri, Aug 09, 2013 at 11:06:23AM +0200, Jean-Francois Moine wrote: > On Fri, 09 Aug 2013 10:23:50 +0200 > Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > > we need at least two more compatibles for the audio controller found on > > Dove and Kirkwood respectively. This is how we are going to distinguish > > those two, e.g. Kirkwood has SPDIF in which Dove hasn't. > > Sebastian, > > s/has/hasn't & s/hasn't/has No, you're both wrong. Some Kirkwood has SPDIF recording and playback. There are those audio blocks which have just I2S capture and playback. There are those which have I2S capture and playback, and SPDIF playback. There are those which have both I2S and SPDIF for both capture and playback. The only one I haven't yet seen is SPDIF capture without playback. > On Fri, 26 Jul 2013 10:21:56 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > On Fri, Jul 26, 2013 at 11:09:13AM +0200, Jean-Francois Moine wrote: > > > The A510 documentation uses the names "DCO PLL" for the internal clock > > > and "AU_EXTCLK" for the external clock. So, what about "dcopll" and > > > "extclk"? > > > > Stop naming them according to their source. Their _consumer_ names > > not _source_ names. > > But Russell did not tell clearly which name could be the best. > > BTW, as we are naming the clocks, the 'clk' in "xxxclk" seems redondant... "extclk" follows what's in the data sheet - the exact name of it is "AU_EXTCLK". Since we already know that this is the audio unit from the struct device, the "AU_" part is redundant. The input name for the internal clock is not given, instead they talk about where it comes from (it's producer) so your choice of "internal" is fine. Take a moment to think about it: if you call it "dcoclk" then what happens on a future SoC if it's not connected to the dcoclk anymore? > I don't think that we need any reference to the codec here. The glue is > done by the audio device. For example, using the (soon?) extended > simple audio card: > > spdif: spdif { > compatible = "linux,spdif-dit"; > }; > sound { > compatible = "linux,simple-audio"; > audio-controller = <&i2s1>; > audio-codec = <&spdif>; > codec-dai-name = "dit-hifi"; > }; As I understand the way DPCM works, that isn't going to work for this case. For DPCM as per Liam's example, it's going to require the audio controller to be bound to the dummy codec, and a dummy platform/dai bound to the SPDIF codec. You then use DAPM routing to connect the SPDIF codec to the appropriate CPU DAI output stream. So the above "simple" implementation won't do - it can't be used to specify two DAI links, and it can't specify the DAPM routing between them. This will be another challenge to solve in DT!
On 08/09/13 11:19, Mark Brown wrote: > On Fri, Aug 09, 2013 at 10:23:50AM +0200, Sebastian Hesselbarth wrote: >> On 08/08/2013 01:22 PM, Jean-Francois Moine wrote: > >>> +i2s1: audio-controller@b4000 { >>> + compatible = "mrvl,mvebu-audio"; >>> + reg = <0xb4000 0x2210>; >>> + interrupts = <21>, <22>; >>> + clocks = <&gate_clk 13>; >>> + clock-names = "internal"; >>> +}; > >> Also we will need some phandle reference to the audio codec here. As >> this property is ongoing work in ASoC core, I suggest we wait for it >> and propose a binding afterwards. > > No, as discussed this should be in the binding for the audio subsystem > not in the binding for an individual component in that subsystem. > Mark, I do understand there may be SoCs requiring sophisticated extra audio nodes, but Marvell SoCs don't. I prefer having a single node for the i2s controller *and* exploit the audio subsystem properties from that. For Marvell audio, we only need a single node for all three ASoC drivers. No other subsystem _requires_ you to have extra nodes for it's needs. If you can provide interrupts, just have an interrupt- controller property. If you can provide clocks, you can link to that very node - no virtual device node required. Even for media they do not insist on a virtual node but they do have generic properties you can exploit. If you insist on creating a virtual sound card node just because ASoC wants it that way - okay, your call. But I don't see any value in that. Sebastian
On Fri, Aug 09, 2013 at 11:34:30AM +0200, Sebastian Hesselbarth wrote: > Mark, > > I do understand there may be SoCs requiring sophisticated extra audio > nodes, but Marvell SoCs don't. I prefer having a single node for the > i2s controller *and* exploit the audio subsystem properties from that. > > For Marvell audio, we only need a single node for all three ASoC > drivers. No other subsystem _requires_ you to have extra nodes for > it's needs. If you can provide interrupts, just have an interrupt- > controller property. If you can provide clocks, you can link to that > very node - no virtual device node required. Even for media they > do not insist on a virtual node but they do have generic properties > you can exploit. Certainly from the "DT is a hardware description" you are completely correct. The audio controller _should_ link directly to the codec, because that's how the hardware is wired. Remember though that there are two outputs from the audio controller (please, call it audio controller, not I2S controller) so you need to specify which of those two outputs the "codec" is connected to. I would say that's required irrespective of whether or not we have a virtual node to aggregate the stuff together for ASoC's purposes (which should not be part of the DT hardware description - it can be part of the "software" configuration though.) We may be able to have kirkwood-i2s.c (which I plan to rename to mvebu-audio.c) automatically generate the snd_soc_card stuff from the audio controller node. Given that we need a more complex description than the "simple" thing for DPCM, that might be overall the best solution in any case (maybe calling out to a library which can be shared between CPU DAI drivers to do this.) Note that we do have another case not yet in tree, which is DRM, but this case is different from that, because ASoC can cope with components with independent initialisation.
On 08/09/2013 11:34 AM, Sebastian Hesselbarth wrote: > On 08/09/13 11:19, Mark Brown wrote: >> On Fri, Aug 09, 2013 at 10:23:50AM +0200, Sebastian Hesselbarth wrote: >>> On 08/08/2013 01:22 PM, Jean-Francois Moine wrote: >> >>>> +i2s1: audio-controller@b4000 { >>>> + compatible = "mrvl,mvebu-audio"; >>>> + reg = <0xb4000 0x2210>; >>>> + interrupts = <21>, <22>; >>>> + clocks = <&gate_clk 13>; >>>> + clock-names = "internal"; >>>> +}; >> >>> Also we will need some phandle reference to the audio codec here. As >>> this property is ongoing work in ASoC core, I suggest we wait for it >>> and propose a binding afterwards. >> >> No, as discussed this should be in the binding for the audio subsystem >> not in the binding for an individual component in that subsystem. >> > > Mark, > > I do understand there may be SoCs requiring sophisticated extra audio > nodes, but Marvell SoCs don't. I prefer having a single node for the > i2s controller *and* exploit the audio subsystem properties from that. It's not about SoCs, it's about the board. The audio fabric on a board can easily get complex enough to require its own driver. Speakers, mics, jacks and jack detection, external amplifiers, bluetooth, baseband, multiple CODECs. That's what the audio node describes. - Lars
On Fri, Aug 09, 2013 at 12:05:58PM +0200, Lars-Peter Clausen wrote: > On 08/09/2013 11:34 AM, Sebastian Hesselbarth wrote: > > I do understand there may be SoCs requiring sophisticated extra audio > > nodes, but Marvell SoCs don't. I prefer having a single node for the > > i2s controller *and* exploit the audio subsystem properties from that. > It's not about SoCs, it's about the board. The audio fabric on a board can > easily get complex enough to require its own driver. Speakers, mics, jacks > and jack detection, external amplifiers, bluetooth, baseband, multiple > CODECs. That's what the audio node describes. Exactly - as I said earlier on this week the issue is that in many cases there is enough going on in audio system design to make the PCB an interesting bit of hardware that's worth describing in the device tree. To repeat what I said before *please* go back and check the list archives, we've been through this several times.
On Fri, 9 Aug 2013 10:43:40 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > Note that we do have another case not yet in tree, which is DRM, but > this case is different from that, because ASoC can cope with components > with independent initialisation. No sure! Sometimes, with the tda998x in HDMI mode, I get a black screen on my TV set. And, surprise, when I start the audio (playing something with vlc), the image appears! I dumped all (known) registers in the audio and video subsystems (tda998x, dove lcd, kirkwood audio), and there is no difference between normal display and black screen. So, some audio event should interact with the HDMI display. I don't think that the problem comes from the TV set, because, when there is a black screen, turning the TV off/on never makes it display normally again. Rebooting the cubox also lets the black screen. Only powering off/on the cubox could change the display back to normal. Now, playing some audio stream gives me immediately a normal display. Any idea?
On 08/09/13 11:43, Russell King - ARM Linux wrote: > On Fri, Aug 09, 2013 at 11:34:30AM +0200, Sebastian Hesselbarth wrote: >> I do understand there may be SoCs requiring sophisticated extra audio >> nodes, but Marvell SoCs don't. I prefer having a single node for the >> i2s controller *and* exploit the audio subsystem properties from that. >> >> For Marvell audio, we only need a single node for all three ASoC >> drivers. No other subsystem _requires_ you to have extra nodes for >> it's needs. If you can provide interrupts, just have an interrupt- >> controller property. If you can provide clocks, you can link to that >> very node - no virtual device node required. Even for media they >> do not insist on a virtual node but they do have generic properties >> you can exploit. > > Certainly from the "DT is a hardware description" you are completely > correct. The audio controller _should_ link directly to the codec, > because that's how the hardware is wired. Remember though that there > are two outputs from the audio controller (please, call it audio > controller, not I2S controller) so you need to specify which of those > two outputs the "codec" is connected to. Exactly, we can solve that with multiple phandle/args audio-codecs property and audio-codec-names (or whatever property names are more appropriate). That is the common set of properties, I am talking about. But the properties are totally unrelated to what nodes you put them into. > I would say that's required irrespective of whether or not we have a > virtual node to aggregate the stuff together for ASoC's purposes (which > should not be part of the DT hardware description - it can be part of > the "software" configuration though.) And that is *the only thing* that keeps bugging me in Mark's replies - he *insists* on having that virtual audio nodes. I have nothing against it, except it should be *required* for every DT we have. DRM doesn't _need_ it, media doesn't _need_ it, but audio is so very special that it _requires_ you to have it described in DT? I understand that it may be required on some boards, especially if you create different sound-cards out of the IP available. Just like the DRM discussion we had - have a virtual node if there is no other sane way to describe it, but there is no strict requirement. Anyway, Mark keeps insisting on it, I'll obey. > We may be able to have kirkwood-i2s.c (which I plan to rename to > mvebu-audio.c) automatically generate the snd_soc_card stuff from the > audio controller node. Given that we need a more complex description > than the "simple" thing for DPCM, that might be overall the best > solution in any case (maybe calling out to a library which can be > shared between CPU DAI drivers to do this.) That is what I am doing on top of the audio-controller node and except that there is no helper to determine the names, yet. If ASoC would provide a snd_soc_simple_card_register_from_dt(...,device_node *), I wouldn't even have to parse the properties myself. > Note that we do have another case not yet in tree, which is DRM, but > this case is different from that, because ASoC can cope with components > with independent initialisation. True, and those should also probe themselves independent of the corresponding lcd driver. Now that we have the discussion on ASoC, that will also allow us to have an audio codec driver for the TDA998x audio part. We need that anyway to create correct AIF packets someday. Sebastian
On Fri, Aug 09, 2013 at 01:01:24PM +0200, Sebastian Hesselbarth wrote: > And that is *the only thing* that keeps bugging me in Mark's replies - > he *insists* on having that virtual audio nodes. I have nothing against > it, except it should be *required* for every DT we have. DRM doesn't > _need_ it, media doesn't _need_ it, but audio is so very special that it > _requires_ you to have it described in DT? > I understand that it may be required on some boards, especially if you > create different sound-cards out of the IP available. Just like the > DRM discussion we had - have a virtual node if there is no other sane > way to describe it, but there is no strict requirement. The problem here is that the extra effort comes from the board, not from the individual components - any component would have to optionally support having a card binding hanging off it (or the properties for a simple link) so it's more consistent just to say we link everything from a board node all the time. By the time you start adding all the things like jacks and connected pins I'd expect you'd find you'd want to have at least a sub node to organise everything. Even things like buses aren't that clear - an I2S link can be a fairly simple point to point link but it can also be a multi-master bus or share some signals with other links. Things like DRM generally have interconnects which are much more regular and less open to interesting board design decisions, though even for media the last time I looked at the bindings the individual links were getting DT nodes which is another way to try to deal with things. Let me once more renew my request that you go and read the previous discussions on this stuff, we've been through this loop repeatedly. > That is what I am doing on top of the audio-controller node and except > that there is no helper to determine the names, yet. If ASoC would > provide a snd_soc_simple_card_register_from_dt(...,device_node *), I > wouldn't even have to parse the properties myself. So extend Morimoto-san's work on the simple card for this - that's what it's there for, it's doing exactly this job for non-DT systems but it just didn't get DT support added yet. All the trivial cards should end up using this.
On Fri, Aug 09, 2013 at 12:39:40PM +0100, Mark Brown wrote: > So extend Morimoto-san's work on the simple card for this - that's what > it's there for, it's doing exactly this job for non-DT systems but it > just didn't get DT support added yet. All the trivial cards should end > up using this. It's quite rediculous to request that the simple card stuff is expanded at this time, when you're also telling us that we must use DPCM for Kirkwood, but DPCM is not yet in a working state in mainline. We don't yet know whether that's because of something the core is doing wrong or whether that's because something that I'm doing wrong. The reason we don't know that is because the _only_ person who knows anything about this is Liam, and as you have said, Liam is away on vacation. All that we presently know is that DPCM is supposed to be "something like this". However, when we do that, it doesn't work, which means we can't be certain what the end result is supposed to look like. It may be that Liam has changed some of the design decisions with DPCM. We know that various things are missing - like the .pcm_playback member in the DAI link. That points to Liam having additional patches to the core code in his tree to make DPCM work. That could impact our scenario here, and how the "simple" card should be implemented. So, what you're asking is for us to extend the design of something in a way that we know very little about how to make work. To that I say: No. Let's wait for Liam to return from vacation so that this can be sorted out properly with someone who knows this code.
On Fri, Aug 09, 2013 at 02:09:32PM +0100, Russell King - ARM Linux wrote: > On Fri, Aug 09, 2013 at 12:39:40PM +0100, Mark Brown wrote: > > So extend Morimoto-san's work on the simple card for this - that's what > > it's there for, it's doing exactly this job for non-DT systems but it > > just didn't get DT support added yet. All the trivial cards should end > > up using this. > It's quite rediculous to request that the simple card stuff is expanded at > this time, when you're also telling us that we must use DPCM for Kirkwood, That's the place to put the sort of shared infrastructure for trivial cards that Sebastian said he wanted to see - if you guys are saying that the machines should all be able to use a trivial binding with shared code that's where that code should be.
On Fri, Aug 09, 2013 at 07:00:58PM +0100, Mark Brown wrote: > On Fri, Aug 09, 2013 at 02:09:32PM +0100, Russell King - ARM Linux wrote: > > On Fri, Aug 09, 2013 at 12:39:40PM +0100, Mark Brown wrote: > > > > So extend Morimoto-san's work on the simple card for this - that's what > > > it's there for, it's doing exactly this job for non-DT systems but it > > > just didn't get DT support added yet. All the trivial cards should end > > > up using this. > > > It's quite rediculous to request that the simple card stuff is expanded at > > this time, when you're also telling us that we must use DPCM for Kirkwood, > > That's the place to put the sort of shared infrastructure for trivial > cards that Sebastian said he wanted to see - if you guys are saying that > the machines should all be able to use a trivial binding with shared > code that's where that code should be. Sigh, you completely miss the point. What all three of us are ultimately after is a DT description for the kirkwood stuff which covers all its use cases. The use case which all three of us have in common is the Cubox, which is the one which needs the spdif stuff to work. Now, what you've said to date is: 1. you want kirkwood to use DPCM. 2. you want kirkwood using people to use the simple card stuff with the kirkwood driver you want to use DPCM. To make it work with DPCM, we first need to know what DPCM requires, which means we either have to have the knowledge of DPCM and/or have a working implementation. We don't have either of those yet. So, I again state plainly that what you're asking is for people to come up with a DT description for a DPCM implementation when we haven't yet got a working DPCM implementation, even without DT. It is this which I assert is a completely rediculous request at this moment in time for the reasons stated in my previous email and repeated in this email.
On Fri, Aug 09, 2013 at 07:25:16PM +0100, Russell King - ARM Linux wrote: > Sigh, you completely miss the point. > What all three of us are ultimately after is a DT description for the > kirkwood stuff which covers all its use cases. The use case which all > three of us have in common is the Cubox, which is the one which needs > the spdif stuff to work. I think I get what needs to be done well enough, I'm not sure that matches up with what people are wanting the results to look like but that's another story. > Now, what you've said to date is: > 1. you want kirkwood to use DPCM. Yes. > 2. you want kirkwood using people to use the simple card stuff with the > kirkwood driver you want to use DPCM. No. What I'm saying is that if your end goal is a binding for cards that works for absolutely anything then it should be handled by the simple card driver since that's what it's there for. There's no requirement to do things that way though, certainly not in a first pass. > To make it work with DPCM, we first need to know what DPCM requires, > which means we either have to have the knowledge of DPCM and/or have a > working implementation. We don't have either of those yet. > So, I again state plainly that what you're asking is for people to come > up with a DT description for a DPCM implementation when we haven't yet > got a working DPCM implementation, even without DT. > It is this which I assert is a completely rediculous request at this > moment in time for the reasons stated in my previous email and repeated > in this email. I'm not sure I said that anyone need do anything right this very minute? In any case since the binding that pulls everything together into the audio subsystem is supposed to be separate to the bindings for the bindings for individual components it should be possible to proceed with those if someone wants to do that (as in the patch under discussion) and add the bits to tie things together later. If someone wants to it should also be possible to convert the existing platforms without S/PDIF support over to DT, providing you don't mind changing the code once the DPCM and S/PDIF support is added and a bit of thought is put into where the S/PDIF output will fit into the bindings. Since DPCM is a Linux internal thing it shouldn't have any impact on the bindings. But like I say there's no need to do anything in particular immediately. The patch under discussion seems basically fine for what it covers, modulo the fairly minor things Sebastian identified, and can be built on later.
On Fri, Aug 09, 2013 at 08:44:34PM +0100, Mark Brown wrote: > If someone wants to it should also be possible to convert the existing > platforms without S/PDIF support over to DT, providing you don't mind > changing the code once the DPCM and S/PDIF support is added and a bit of > thought is put into where the S/PDIF output will fit into the bindings. Okay, so you're thinking that the I2S output will be enabled in the absence of DPCM? If so, that tells me that you don't understand my patches, and this is getting *really* tiresome. One more time: - There are two outputs from the FIFO. - There is an I2S output, and there is a SPDIF output. - All hardware has an I2S output. - Some hardware also has the SPDIF output. - Each output is individually enable-able via separate bits. - When both outputs are used, both must be enabled simultaneously. - Otherwise, only one output must be enabled at any one time. - At least one output must be enabled for there to be any activity from the unit at all (that's obvious!) So, what I'm doing is providing _two_ AIF connection points, one for I2S and one for SPDIF. The appropriate AIF connection point must be attached to for the appropriate output to be enabled. That means that if you want to use the SPDIF output, the SPDIF AIF must be linked to the codec. If you want to use the I2S output, the I2S AIF must be linked to the codec. Without this, there's no way for the CPU DAI to know which output(s) are in use, and therefore which should be enabled. What this means is that the conventional setup where you have _one_ DAI link connecting the codec DAI to the CPU DAI won't work on Kirkwood anymore, because there is no way to know which of the two outputs should be enabled. I avoided that in my patches, but you've objected to that saying that it must use DPCM. That makes the whole of kirkwood entirely DPCM only, whether or not you have a single codec to connect. Surely you realise this, because you must have read the patches properly before you commented on them, and realised that one of spdifdo or i2sdo must be powered up to set either enable bit?
On Fri, Aug 09, 2013 at 09:38:47PM +0100, Russell King - ARM Linux wrote: > On Fri, Aug 09, 2013 at 08:44:34PM +0100, Mark Brown wrote: > > If someone wants to it should also be possible to convert the existing > > platforms without S/PDIF support over to DT, providing you don't mind > > changing the code once the DPCM and S/PDIF support is added and a bit of > > thought is put into where the S/PDIF output will fit into the bindings. > Okay, so you're thinking that the I2S output will be enabled in the > absence of DPCM? If so, that tells me that you don't understand my When I say that it should be possible to convert the existing machines over to DT I'm talking about the machines that are supported with the drivers currently in mainline. These machines presumably work just fine with the existing drivers which support the I2S only subset of the possible functionality in the hardware without using DCPM. These are the the machines that could have DT added now without implementing the driver improvements to enable the extra hardware functionality. When I say that they will need changing once DPCM and S/PDIF support is added what I mean is that those changes to the CPU side drivers should, as you correctly say, require all Kirkwood machine drivers to use DPCM even if only due to the handling of simultaneous startup of the S/PDIF and I2S interfaces. The "you" in the above quote was intended to refer to people working on Kirkwood in general, not you personally unless you want to. > What this means is that the conventional setup where you have _one_ DAI > link connecting the codec DAI to the CPU DAI won't work on Kirkwood > anymore, because there is no way to know which of the two outputs > should be enabled. I avoided that in my patches, but you've objected > to that saying that it must use DPCM. That makes the whole of kirkwood > entirely DPCM only, whether or not you have a single codec to connect. Right, that's where the drivers should end up. In terms of the device tree I would expect that it should be the case that there are distinct I2S and S/PDIF interfaces visible, or at least that there's some way to identify which interface on the SoC is being referred to - this is the thought about where the S/PDIF output will fit into the bindings that I mentioned being required. So long as only I2S is used in the system (or at least any S/PDIF that is present is ignored at runtime) it should be possible to continue using the existing driver infrastructure until S/PDIF and DPCM support is added and then transition the existing drivers (both non-DT and any DT ones that get added or converted) over to that as part of the conversion. In other words I was talking about a possible intermediate state where people are working with the DT bindings but the driver support for systems with S/PDIF in use is not there yet. That's not required but it should be possible if there's interest in progressing the DT bindings while the driver is as it stands.
Dear Jean-Francois Moine, On Fri, 9 Aug 2013 11:06:23 +0200, Jean-Francois Moine wrote: > > we need at least two more compatibles for the audio controller found on > > Dove and Kirkwood respectively. This is how we are going to distinguish > > those two, e.g. Kirkwood has SPDIF in which Dove hasn't. > > Sebastian, > > s/has/hasn't & s/hasn't/has > > Are 2 compatibles enough, i.e. "mvebu-audio" and "mbevu-audio-spdif"? Or: marvell,kirkwood-audio marvell,dove-audio > > Also, we have used "marvell" as prefix for a long time. I know there has > > been discussion about the stock ticker appreviation, has there been any > > decision on that already? > > Don't know, but, sure, there are still a lot of "mrvl". Yes, but they are being converted over to 'marvell', I've seen some patches posted. Also, as per Documentation/devicetree/bindings/vendor-prefixes.txt, 'marvell' is the right prefix for Marvell compatible strings. > > Also we will need some phandle reference to the audio codec here. As > > this property is ongoing work in ASoC core, I suggest we wait for it > > and propose a binding afterwards. > > I don't think that we need any reference to the codec here. The glue is > done by the audio device. For example, using the (soon?) extended > simple audio card: Have you looked at the Samsung DT bindings for audio: sound { compatible = "samsung,smdk-wm8994"; samsung,i2s-controller = <&i2s0>; samsung,audio-codec = <&wm8994>; }; it definitely has a codec phandle reference here. Thomas
On Sat, Aug 10, 2013 at 12:42:09AM +0100, Mark Brown wrote: > On Fri, Aug 09, 2013 at 09:38:47PM +0100, Russell King - ARM Linux wrote: > > On Fri, Aug 09, 2013 at 08:44:34PM +0100, Mark Brown wrote: > > > > If someone wants to it should also be possible to convert the existing > > > platforms without S/PDIF support over to DT, providing you don't mind > > > changing the code once the DPCM and S/PDIF support is added and a bit of > > > thought is put into where the S/PDIF output will fit into the bindings. > > > Okay, so you're thinking that the I2S output will be enabled in the > > absence of DPCM? If so, that tells me that you don't understand my > > When I say that it should be possible to convert the existing machines > over to DT I'm talking about the machines that are supported with the > drivers currently in mainline. These machines presumably work just fine > with the existing drivers which support the I2S only subset of the > possible functionality in the hardware without using DCPM. These are > the the machines that could have DT added now without implementing the > driver improvements to enable the extra hardware functionality. > > When I say that they will need changing once DPCM and S/PDIF support is > added what I mean is that those changes to the CPU side drivers should, > as you correctly say, require all Kirkwood machine drivers to use DPCM > even if only due to the handling of simultaneous startup of the S/PDIF > and I2S interfaces. > > The "you" in the above quote was intended to refer to people working on > Kirkwood in general, not you personally unless you want to. > > > What this means is that the conventional setup where you have _one_ DAI > > link connecting the codec DAI to the CPU DAI won't work on Kirkwood > > anymore, because there is no way to know which of the two outputs > > should be enabled. I avoided that in my patches, but you've objected > > to that saying that it must use DPCM. That makes the whole of kirkwood > > entirely DPCM only, whether or not you have a single codec to connect. > > Right, that's where the drivers should end up. In terms of the device > tree I would expect that it should be the case that there are distinct > I2S and S/PDIF interfaces visible, or at least that there's some way to > identify which interface on the SoC is being referred to - this is the > thought about where the S/PDIF output will fit into the bindings that I > mentioned being required. > > So long as only I2S is used in the system (or at least any S/PDIF that > is present is ignored at runtime) it should be possible to continue > using the existing driver infrastructure until S/PDIF and DPCM support > is added and then transition the existing drivers (both non-DT and any > DT ones that get added or converted) over to that as part of the > conversion. > > In other words I was talking about a possible intermediate state where > people are working with the DT bindings but the driver support for > systems with S/PDIF in use is not there yet. That's not required but it > should be possible if there's interest in progressing the DT bindings > while the driver is as it stands. Right, so what you're proposing is to come up with a DT description for the existing stuff, and then have to change (or at the very least augment) that description later when the DPCM stuff goes in. What should be done is to sort out DPCM, get that working and then sort out the DT side of it, so that everyone gets only one transition to DT, not a transition to a half-baked stop-gap DT and then have to go through another round of DT changes. "Because we can" is not a good enough reason not to get this sorted properly.
On Sat, Aug 10, 2013 at 10:31:37AM +0100, Russell King - ARM Linux wrote: > Right, so what you're proposing is to come up with a DT description for > the existing stuff, and then have to change (or at the very least augment) > that description later when the DPCM stuff goes in. > What should be done is to sort out DPCM, get that working and then sort out > the DT side of it, so that everyone gets only one transition to DT, not a > transition to a half-baked stop-gap DT and then have to go through another > round of DT changes. "Because we can" is not a good enough reason not to > get this sorted properly. Since we know what the hardware physically looks like we should be able to write a DT binding which can be stable for both before and after the DPCM transition. The bindings would have to be truly awful to require a rewrite here and as with all the DMA wrapper drivers if parts of DPCM that don't reflect actual hardware are appearing in the DT we're doing something wrong.
diff --git a/Documentation/devicetree/bindings/sound/mvebu-audio.txt b/Documentation/devicetree/bindings/sound/mvebu-audio.txt new file mode 100644 index 0000000..7e5fd37 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/mvebu-audio.txt @@ -0,0 +1,29 @@ +* mvebu (Kirkwood, Dove, Armada 370) audio controller + +Required properties: + +- compatible: "mrvl,mvebu-audio" + +- reg: physical base address of the controller and length of memory mapped + region. + +- interrupts: list of two irq numbers. + The first irq is used for data flow and the second one is used for errors. + +- clocks: one or two phandles. + The first one is mandatory and defines the internal clock. + The second one is optional and defines an external clock. + +- clock-names: names associated to the clocks: + "internal" for the internal clock + "extclk" for the external clock + +Example: + +i2s1: audio-controller@b4000 { + compatible = "mrvl,mvebu-audio"; + reg = <0xb4000 0x2210>; + interrupts = <21>, <22>; + clocks = <&gate_clk 13>; + clock-names = "internal"; +}; diff --git a/sound/soc/kirkwood/kirkwood-i2s.c.next b/sound/soc/kirkwood/kirkwood-i2s.c index e5f3f7a..a4170b4 100644 --- a/sound/soc/kirkwood/kirkwood-i2s.c.next +++ b/sound/soc/kirkwood/kirkwood-i2s.c @@ -22,6 +22,8 @@ #include <sound/pcm_params.h> #include <sound/soc.h> #include <linux/platform_data/asoc-kirkwood.h> +#include <linux/of.h> + #include "kirkwood.h" #define DRV_NAME "mvebu-audio" @@ -453,6 +455,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) struct snd_soc_dai_driver *soc_dai = &kirkwood_i2s_dai; struct kirkwood_dma_data *priv; struct resource *mem; + struct device_node *np = pdev->dev.of_node; int err; priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); @@ -473,14 +476,16 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) return -ENXIO; } - if (!data) { - dev_err(&pdev->dev, "no platform data ?!\n"); + if (np) { + priv->burst = 128; /* might be 32 or 128 */ + } else if (data) { + priv->burst = data->burst; + } else { + dev_err(&pdev->dev, "no DT nor platform data ?!\n"); return -EINVAL; } - priv->burst = data->burst; - - priv->clk = devm_clk_get(&pdev->dev, NULL); + priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL); if (IS_ERR(priv->clk)) { dev_err(&pdev->dev, "no clock\n"); return PTR_ERR(priv->clk); @@ -507,7 +512,7 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev) priv->ctl_rec = KIRKWOOD_RECCTL_SIZE_24; /* Select the burst size */ - if (data->burst == 32) { + if (priv->burst == 32) { priv->ctl_play |= KIRKWOOD_PLAYCTL_BURST_32; priv->ctl_rec |= KIRKWOOD_RECCTL_BURST_32; } else { @@ -552,12 +557,21 @@ static int kirkwood_i2s_dev_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_OF +static struct of_device_id kirkwood_i2s_of_match[] = { + { .compatible = "mrvl,mvebu-audio" }, + { } +}; +MODULE_DEVICE_TABLE(of, kirkwood_i2s_of_match); +#endif + static struct platform_driver kirkwood_i2s_driver = { .probe = kirkwood_i2s_dev_probe, .remove = kirkwood_i2s_dev_remove, .driver = { .name = DRV_NAME, .owner = THIS_MODULE, + .of_match_table = of_match_ptr(kirkwood_i2s_of_match), }, };
This patch adds DT support to the audio subsystem of the mvebu family (Kirkwood, Dove, Armada 370). Signed-off-by: Jean-Francois Moine <moinejf@free.fr> --- .../devicetree/bindings/sound/mvebu-audio.txt | 29 ++++++++++++++++++++++ sound/soc/kirkwood/kirkwood-i2s.c | 26 +++++++++++++++++----- 2 files changed, 49 insertions(+), 6 deletions(-)