[alsa-devel] [PATCH 1/2] powerpc: add platform registration for ALSA SoC drivers
An upcoming change in the architecture of ALSA SoC (ASoC) will require the MPC8610 HPCD's ASoC fabric driver to register as a standard platform driver. Therefore, we need to call platform_device_register_simple() from the board's platform code.
Signed-off-by: Timur Tabi timur@freescale.com ---
Kumar, the ASoC mainters are willing to pick up this patch, but they want an ACK from you first. Or, you could pick it up, since by itself it's harmless.
arch/powerpc/platforms/86xx/mpc8610_hpcd.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c index 5abe137..66afff3 100644 --- a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c +++ b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c @@ -98,6 +98,9 @@ static int __init mpc8610_declare_of_platform_devices(void) /* Without this call, the SSI device driver won't get probed. */ of_platform_bus_probe(NULL, mpc8610_ids, NULL);
+ /* Register the platform device for the ASoC fabric driver */ + platform_device_register_simple("snd-soc-mpc8610-hpcd", 0, NULL, 0); + return 0; } machine_device_initcall(mpc86xx_hpcd, mpc8610_declare_of_platform_devices);
On Mon, 2010-04-26 at 15:49 -0500, Timur Tabi wrote:
An upcoming change in the architecture of ALSA SoC (ASoC) will require the MPC8610 HPCD's ASoC fabric driver to register as a standard platform driver. Therefore, we need to call platform_device_register_simple() from the board's platform code.
Signed-off-by: Timur Tabi timur@freescale.com
Gross. Loses the linkage to device-tree etc... which is useful for audio especially. You should at least make sure the device node follows for the target driver to be able to use it :-) I'd like you to sync with Grant work on that matter if you are going to convert of_devices into platform_devices.
Cheers, Ben.
Kumar, the ASoC mainters are willing to pick up this patch, but they want an ACK from you first. Or, you could pick it up, since by itself it's harmless.
arch/powerpc/platforms/86xx/mpc8610_hpcd.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c index 5abe137..66afff3 100644 --- a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c +++ b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c @@ -98,6 +98,9 @@ static int __init mpc8610_declare_of_platform_devices(void) /* Without this call, the SSI device driver won't get probed. */ of_platform_bus_probe(NULL, mpc8610_ids, NULL);
- /* Register the platform device for the ASoC fabric driver */
- platform_device_register_simple("snd-soc-mpc8610-hpcd", 0, NULL, 0);
- return 0;
} machine_device_initcall(mpc86xx_hpcd, mpc8610_declare_of_platform_devices);
On Tue, 2010-04-27 at 16:36 +1000, Benjamin Herrenschmidt wrote:
On Mon, 2010-04-26 at 15:49 -0500, Timur Tabi wrote:
An upcoming change in the architecture of ALSA SoC (ASoC) will require the MPC8610 HPCD's ASoC fabric driver to register as a standard platform driver. Therefore, we need to call platform_device_register_simple() from the board's platform code.
Signed-off-by: Timur Tabi timur@freescale.com
Gross. Loses the linkage to device-tree etc... which is useful for audio especially. You should at least make sure the device node follows for the target driver to be able to use it :-) I'd like you to sync with Grant work on that matter if you are going to convert of_devices into platform_devices.
Timur, please correct my device tree understanding form our IRC conversation if I'm wrong here.
I think one of the difficulties encountered here is that the device tree root for sound in this case is the SSI (Digital Audio Interface) controller and not the sound card as in ASoC. So maybe the problem is how do we represent an ASoC sound card in the device tree ?
The sound card within ASoC is a compound device that is made up of the SSI, Codec and audio DMA engine components. The SSI/Codec/DMA components do not have to be the sound cards child devices wrt the driver model but do register with the ASoC core and are 'joined' with each other and the sound card driver to form a working audio device.
Now I don't know the mechanics of adding an ASoC sound card to the device tree, but the device tree should be able to define an ASoC sound card and also represent the relationships between the sound card and the SSI/Codec/DMA components. The components should also be represented in the device tree. Parsing the device tree should probe() all the ASoC components and sound card. The ASoC core would then instantiated them as a sound device.
Thanks
Liam
Liam Girdwood wrote:
I think one of the difficulties encountered here is that the device tree root for sound in this case is the SSI (Digital Audio Interface) controller and not the sound card as in ASoC. So maybe the problem is how do we represent an ASoC sound card in the device tree ?
Yes, that is a topic of concern.
If you look at the 8610 DTS, you'll see that the SSI (i2s) is effectively the master. In reality, I just modeled the board in the DTS. The SSI node, for example, lists the two DMA channels it needs. It also points to the codec that it's connected to. And the clock-frequency property is in the codec node because the oscillator is connected to the codec on the board.
In theory, would could probably get rid of the DMA node pointers if I can update the standard DMA driver to allow me to reserve two DMA channels. That's not an easy fix, though.
The sound card within ASoC is a compound device that is made up of the SSI, Codec and audio DMA engine components. The SSI/Codec/DMA components do not have to be the sound cards child devices wrt the driver model but do register with the ASoC core and are 'joined' with each other and the sound card driver to form a working audio device.
Yes, today I just walk the device tree looking for these connections implicitly. The code may be ugly, but the device tree is more streamlined.
Now I don't know the mechanics of adding an ASoC sound card to the device tree, but the device tree should be able to define an ASoC sound card and also represent the relationships between the sound card and the SSI/Codec/DMA components. The components should also be represented in the device tree. Parsing the device tree should probe() all the ASoC components and sound card. The ASoC core would then instantiated them as a sound device.
Another problem is that ASoC won't let me probe the DMA channels independently. That is, I cannot tell ASoC that I have a playback DMA and a capture DMA. ASoC does not recognize two DMA devices for a single SSI. If you can fix that, then I can turn the DMA driver into an OF driver.
On Tue, 2010-04-27 at 09:52 -0500, Timur Tabi wrote:
Liam Girdwood wrote:
Now I don't know the mechanics of adding an ASoC sound card to the device tree, but the device tree should be able to define an ASoC sound card and also represent the relationships between the sound card and the SSI/Codec/DMA components. The components should also be represented in the device tree. Parsing the device tree should probe() all the ASoC components and sound card. The ASoC core would then instantiated them as a sound device.
Another problem is that ASoC won't let me probe the DMA channels independently. That is, I cannot tell ASoC that I have a playback DMA and a capture DMA. ASoC does not recognize two DMA devices for a single SSI. If you can fix that, then I can turn the DMA driver into an OF driver.
Iirc, the SSI and DMA controllers on your SoC mean that each DMA device can only do one direction (either Playback or Capture). So I'm thinking we create two DAI link entries for your sound card (one for playback and the other for capture) and they both use the same SSI device but each would have it's own DMA device.
This would result in two separate pcm devices being exported to userspace i.e one for playback only and the other for capture only. I think this is also a more accurate representation of your hardware too (since we have different DMA devices for each pcm stream direction).
Liam
Liam Girdwood wrote:
Iirc, the SSI and DMA controllers on your SoC mean that each DMA device can only do one direction (either Playback or Capture). So I'm thinking we create two DAI link entries for your sound card (one for playback and the other for capture) and they both use the same SSI device but each would have it's own DMA device.
Do you mean here:
machine_data->card.probe = mpc8610_hpcd_machine_probe; machine_data->card.remove = mpc8610_hpcd_machine_remove; machine_data->card.name = "MPC8610 HPCD"; machine_data->card.num_links = 1; machine_data->card.dai_link = &machine_data->dai;
So that num_links would be 2 instead of 1?
I would need some way for fsl_dma_open() to get a pointer to private, DMA-specific data. I'm not sure how I can do that.
This would result in two separate pcm devices being exported to userspace i.e one for playback only and the other for capture only. I think this is also a more accurate representation of your hardware too (since we have different DMA devices for each pcm stream direction).
I can say for certain whether that will actually work. My gut tells me that I might run into problems trying to implement that. The only way to know for sure is to start hacking. Unfortunately, my window of opportunity to work on this just closed, and it may not open for a while. I know my current patch is less-than-ideal, but it does restore functionality to the 8610, and without needing any U-Boot or device-tree changes. So unless there are any real objections, I'd like it to be merged. Otherwise, 8610 audio will be broken in the next release.
On Tue, Apr 27, 2010 at 10:28 AM, Timur Tabi timur@freescale.com wrote:
I can say for certain whether that will actually work.
Ugh, I meant to write, "I can't say for certain".
On Tue, 2010-04-27 at 10:28 -0500, Timur Tabi wrote:
Liam Girdwood wrote:
Iirc, the SSI and DMA controllers on your SoC mean that each DMA device can only do one direction (either Playback or Capture). So I'm thinking we create two DAI link entries for your sound card (one for playback and the other for capture) and they both use the same SSI device but each would have it's own DMA device.
Do you mean here:
machine_data->card.probe = mpc8610_hpcd_machine_probe; machine_data->card.remove = mpc8610_hpcd_machine_remove; machine_data->card.name = "MPC8610 HPCD"; machine_data->card.num_links = 1; machine_data->card.dai_link = &machine_data->dai;
So that num_links would be 2 instead of 1?
Yes.
I would need some way for fsl_dma_open() to get a pointer to private, DMA-specific data. I'm not sure how I can do that.
In multi-component we now have platform_data and device private data (from the regular driver model).
We also have stream snd_soc_dai_set_dma_data() for runtime DMA config.
So it depends on who is setting your DMA data. If it's DTS then it would be the of_ platform equivalent, if it's your DMA probe() then dev data otherwise you can use the snd_soc_dai_set_dma_data().
This would result in two separate pcm devices being exported to userspace i.e one for playback only and the other for capture only. I think this is also a more accurate representation of your hardware too (since we have different DMA devices for each pcm stream direction).
I can say for certain whether that will actually work. My gut tells me that I might run into problems trying to implement that. The only way to know for sure is to start hacking. Unfortunately, my window of opportunity to work on this just closed, and it may not open for a while. I know my current patch is less-than-ideal, but it does restore functionality to the 8610, and without needing any U-Boot or device-tree changes. So unless there are any real objections, I'd like it to be merged. Otherwise, 8610 audio will be broken in the next release.
Ok, I can live with this providing we can mark it as a TODO: and have a PPC Ack. It may be easier to fix in the future too as the ASoC card registration clean-up should be complete/in-progress (i.e. card platform_data and private_data will be available for passing in anything you like).
Thanks
Liam
Liam Girdwood wrote:
I would need some way for fsl_dma_open() to get a pointer to private, DMA-specific data. I'm not sure how I can do that.
In multi-component we now have platform_data and device private data (from the regular driver model).
In that case, I still have the same problem with how to generate an 'id' based on a device tree node. We have the idea of a 'phandle', which is a unique integer for a node, but there's no way to create phandles from within Linux. They have to exist in the DTS first, and I'm loathe to modify the DTS.
On Tue, Apr 27, 2010 at 12:32 PM, Timur Tabi timur@freescale.com wrote:
Liam Girdwood wrote:
I would need some way for fsl_dma_open() to get a pointer to private, DMA-specific data. I'm not sure how I can do that.
In multi-component we now have platform_data and device private data (from the regular driver model).
In that case, I still have the same problem with how to generate an 'id' based on a device tree node. We have the idea of a 'phandle', which is a unique integer for a node, but there's no way to create phandles from within Linux. They have to exist in the DTS first, and I'm loathe to modify the DTS.
Can you not dynamically assign an id? If alsa soc needs a unique id number, then just create a lookup function. Something like of_asoc_phandle_to_codec_id() that will either return a previously assigned id, or will assign a new id. You shouldn't ever need to add data to the tree at runtime.
g.
Grant Likely wrote:
Can you not dynamically assign an id? If alsa soc needs a unique id number, then just create a lookup function.
What I need is something like a hashing function that can convert a "struct device_node *" into an "int". I'm going to have two functions that independently parse the device tree and locate a specific node. Both functions will "register the node" with asoc, but they'll use an integer ID to uniquely identify the node.
At least, that's the way ASoC likes to operate. AsoC takes a fixed string plus a unique integer. I could technically create a unique string for each DMA device, and have the integer always be 0.
Something like of_asoc_phandle_to_codec_id() that will either return a previously assigned id, or will assign a new id. You shouldn't ever need to add data to the tree at runtime.
I know. It just would have been nice if my nodes already had phandles in them.
On Tue, Apr 27, 2010 at 03:04:33PM -0500, Timur Tabi wrote:
[Reflowed into 80 columns]
At least, that's the way ASoC likes to operate. AsoC takes a fixed string plus a unique integer. I could technically create a unique string for each DMA device, and have the integer always be 0.
This seems like the most direct approach to your problem - as with other subsystems the .id is there mostly to provide deduping when the string is always the same, if you can easily come up with sensible names you can just use those.
On Tue, 2010-04-27 at 15:04 -0500, Timur Tabi wrote:
What I need is something like a hashing function that can convert a "struct device_node *" into an "int". I'm going to have two functions that independently parse the device tree and locate a specific node. Both functions will "register the node" with asoc, but they'll use an integer ID to uniquely identify the node.
At least, that's the way ASoC likes to operate. AsoC takes a fixed string plus a unique integer. I could technically create a unique string for each DMA device, and have the integer always be 0.
That's just plain gross and horrible. You could use phandles you know :-)
Or you could use path in your strings, or something like that.
Note that any time you have a struct device, you have a free device_node pointer as well.
Cheers, Ben.
On Tue, 2010-04-27 at 13:15 -0600, Grant Likely wrote:
Can you not dynamically assign an id? If alsa soc needs a unique id number, then just create a lookup function. Something like of_asoc_phandle_to_codec_id() that will either return a previously assigned id, or will assign a new id. You shouldn't ever need to add data to the tree at runtime.
Numerical magic IDs are evil... why not a name ? Or it's existing alsa breakage ? :-)
Cheers, Ben.
On Tue, Apr 27, 2010 at 10:20 AM, Liam Girdwood lrg@slimlogic.co.uk wrote:
Another problem is that ASoC won't let me probe the DMA channels independently. That is, I cannot tell ASoC that I have a playback DMA and a capture DMA. ASoC does not recognize two DMA devices for a single SSI. If you can fix that, then I can turn the DMA driver into an OF driver.
Iirc, the SSI and DMA controllers on your SoC mean that each DMA device can only do one direction (either Playback or Capture). So I'm thinking we create two DAI link entries for your sound card (one for playback and the other for capture) and they both use the same SSI device but each would have it's own DMA device.
This would result in two separate pcm devices being exported to userspace i.e one for playback only and the other for capture only. I think this is also a more accurate representation of your hardware too (since we have different DMA devices for each pcm stream direction).
Ok, I'm trying to do this now, and I'm running into problems.
So here's the device list:
One machine One SSI Two DMA channels One codec
So I create two dai_links in the machine driver. Each dai_link has two DAIs in it. The DAIs are identical, except for the platform_drv field. The platform_drv in the first DAI points to the first DMA channel, and the platform_drv of the second DAI points to the second DMA channel.
When I boot Linux, I get this:
asoc: cs4270 <-> /soc@e0000000/ssi@16000 mapping ok sysfs: cannot create duplicate filename '/devices/platform/soc-audio/cs4270'
so it looks like when asoc is processing the dai_link, it tries to create a sysfs device for the codec twice.
How do I avoid this?
On Fri, Apr 30, 2010 at 4:46 PM, Timur Tabi timur@freescale.com wrote:
When I boot Linux, I get this:
asoc: cs4270 <-> /soc@e0000000/ssi@16000 mapping ok sysfs: cannot create duplicate filename '/devices/platform/soc-audio/cs4270'
*sigh* never mind. The problem was that the stream_name was the same for both DAIs.
What exactly should the stream names be set to, anyway? I did this:
machine_data->dai[0].stream_name = "playback"; machine_data->dai[1].stream_name = "capture";
but I have a suspicion that this is too simplistic.
On Tue, Apr 27, 2010 at 2:07 AM, Liam Girdwood lrg@slimlogic.co.uk wrote:
On Tue, 2010-04-27 at 16:36 +1000, Benjamin Herrenschmidt wrote:
On Mon, 2010-04-26 at 15:49 -0500, Timur Tabi wrote:
An upcoming change in the architecture of ALSA SoC (ASoC) will require the MPC8610 HPCD's ASoC fabric driver to register as a standard platform driver. Therefore, we need to call platform_device_register_simple() from the board's platform code.
Signed-off-by: Timur Tabi timur@freescale.com
Gross. Loses the linkage to device-tree etc... which is useful for audio especially. You should at least make sure the device node follows for the target driver to be able to use it :-) I'd like you to sync with Grant work on that matter if you are going to convert of_devices into platform_devices.
Timur, please correct my device tree understanding form our IRC conversation if I'm wrong here.
I think one of the difficulties encountered here is that the device tree root for sound in this case is the SSI (Digital Audio Interface) controller and not the sound card as in ASoC. So maybe the problem is how do we represent an ASoC sound card in the device tree ?
Most likely this is currently a problem, yes. *however* the device tree is just data. It is convenient and useful to have a common representation between similar kinds of devices, but it isn't mandatory. The device tree structure does *not* need to have a 1:1 correspondence to the ASoC device registrations.
So, the current 86xx device tree binding assumes a simple layout with a node describing an DAI controller, and another node describing the codec with a single phandle (pointer) from the DAI to the codec. In this configuration, it is completely reasonable for the DAI node to trigger both the instantiation of the ASoC DAI controller device and the sound card device. Linux can treat them as separate even though the current device tree has a simplistic representation.
The sound card within ASoC is a compound device that is made up of the SSI, Codec and audio DMA engine components. The SSI/Codec/DMA components do not have to be the sound cards child devices wrt the driver model but do register with the ASoC core and are 'joined' with each other and the sound card driver to form a working audio device.
Right. This makes sense to me.
Now I don't know the mechanics of adding an ASoC sound card to the device tree, but the device tree should be able to define an ASoC sound card and also represent the relationships between the sound card and the SSI/Codec/DMA components. The components should also be represented in the device tree. Parsing the device tree should probe() all the ASoC components and sound card. The ASoC core would then instantiated them as a sound device.
I've tried very hard to maintain a distinction between device tree binding (representation) and Linux kernel internal implementation details. The real question is whether or not the binding provides sufficient detail for the operating system to figure out what to do. In the extreme minimalist case, the audio driver could decide how to configure itself solely on the board name property of the root node. There is nothing wrong with that, but it also means that no data is available to dynamically select common modules or modify connections; it all has to be hard coded.
The 8610 device tree looks something like this right now:
[...] cs4270:codec@4f { compatible = "cirrus,cs4270"; reg = <0x4f>; /* MCLK source is a stand-alone oscillator */ clock-frequency = <12288000>; }; [...] ssi@16000 { compatible = "fsl,mpc8610-ssi"; [...] fsl,mode = "i2s-slave"; codec-handle = <&cs4270>; fsl,playback-dma = <&dma00>; fsl,capture-dma = <&dma01>; fsl,fifo-depth = <8>; sleep = <&pmc 0 0x08000000>; }; [...]
(I've omitted the DMA nodes and some irrelevant details) This is enough information for a simplistic driver registration that probably makes a lot of assumptions. Such as the ssi represents a single logical sound device. It won't handle complex representations, but in a lot of cases that may be just fine.
However, as you and Mark rightly point out, it completely fails to represent complex sound devices with weird clocking and strange routes. Something like this is probably more appropriate:
[...] codec1 :codec@4f { compatible = "cirrus,cs4270"; reg = <0x4f>; /* MCLK source is a stand-alone oscillator */ clock-frequency = <12288000>; }; [...] ssi1: ssi@16000 { compatible = "fsl,mpc8610-ssi"; [...] fsl,mode = "i2s-slave"; fsl,playback-dma = <&dma00>; fsl,capture-dma = <&dma01>; fsl,fifo-depth = <8>; sleep = <&pmc 0 0x08000000>; }; [...] sound { compatible = "fsl,mpc8610-hpcd-sound"; /* maybe something like (totally off the top of my head) */ dai-links = <&ssi1 0 &codec 0 &ssi1 1 &codec 1>; [...] };
Where the 'sound' node is now the starting point for representing a logical sound device instead of the ssi node. This binding probably makes more sense (but I'm not committing to anything like this until I see a real proposal for a real device).
The question for the drivers is more around how to deal with the data provided. I've got zero issues with platform specific code to handle things that aren't easily described in a device tree. The point is to make the common cases data-driven so that common code will handle them. The complex corner cases are still complex corner cases that need platform specific code.
Or, in other words, the device tree should *not* be used to describe every possible detail and permutation. It is best used to describe the permutations that are common so that they don't need to be hard coded for each and every board.
I would solve the problem this way: In the ssi driver, if the codec-node property is present, then call a function to instantiate a simple or platform specific sound card instance that makes the assumptions listed above. If not, then just register the ssi and exit, which leaves the ssi available for a separate driver to pick it up. I wouldn't do this for new platforms, but it gracefully makes use of the data provided in the current 8610 device tree.
BTW Timur, there is nothing wrong with registering multiple devices that all have the of_node pointer set to the same node.
g.
Grant Likely wrote:
So, the current 86xx device tree binding assumes a simple layout with a node describing an DAI controller, and another node describing the codec with a single phandle (pointer) from the DAI to the codec. In this configuration, it is completely reasonable for the DAI node to trigger both the instantiation of the ASoC DAI controller device and the sound card device. Linux can treat them as separate even though the current device tree has a simplistic representation.
The only problem with this is that there is board-specific programming that needs to be done (look at mpc8610_hpcd_machine_probe), so we still need to have a fabric driver that is independent of the SSI, codec, or DMA drivers. The P1022 also has an SSI, and I'm hoping that all I need to do is create a new fabric driver, not hack up the SSI driver to support board programming.
So if the fabric driver still needs to exist, then it still needs a struct device, and it still needs to register with asoc. I don't see how I can register the sound card itself in the SSI driver, because it won't know anything about the board-specific code in the fabric driver.
I've tried very hard to maintain a distinction between device tree binding (representation) and Linux kernel internal implementation details. The real question is whether or not the binding provides sufficient detail for the operating system to figure out what to do.
I think it does, because it's working today.
In the extreme minimalist case, the audio driver could decide how to configure itself solely on the board name property of the root node. There is nothing wrong with that, but it also means that no data is available to dynamically select common modules or modify connections; it all has to be hard coded.
Well, asoc already has several hard-coded requirements:
machine_data->dai.cpu_dai_drv = &fsl_ssi_dai; machine_data->dai.codec_dai_drv = &cs4270_dai; machine_data->dai.codec_drv = &soc_codec_device_cs4270; machine_data->dai.ops = &mpc8610_hpcd_ops; machine_data->dai.platform_drv = &fsl_soc_platform;
So even though I probe for each device separately and register them separately, the fabric driver still needs to have hard-coded addresses
Maybe the asoc guys can tell me why I need to register the cpu_dai_drv structure via platform_device_add(), when it's already being registered via snd_soc_register_dai().
(I've omitted the DMA nodes and some irrelevant details) This is enough information for a simplistic driver registration that probably makes a lot of assumptions. Such as the ssi represents a single logical sound device. It won't handle complex representations, but in a lot of cases that may be just fine.
Why would I ever represent the SSI as anything but a single logical sound device? Let ALSA handle synchronizing multiple streams together if it wants to.
sound { compatible = "fsl,mpc8610-hpcd-sound"; /* maybe something like (totally off the top of my head) */ dai-links = <&ssi1 0 &codec 0 &ssi1 1 &codec 1>; [...] };
I don't know when I would ever do this. The two SSI devices are completely independent. Why would I bind them together into one "device"?
Where the 'sound' node is now the starting point for representing a logical sound device instead of the ssi node. This binding probably makes more sense (but I'm not committing to anything like this until I see a real proposal for a real device).
The only issue I have with this is all it does is turn the fabric driver from a platform driver that scans the OF tree, into an OF driver that still needs to query the device tree to get all the data it needs. For example, the fabric driver still needs to know the clock frequency and direction of the codec node, so that it can call snd_soc_dai_set_fmt() and snd_soc_dai_set_sysclk() properly. To eliminate that, we could have the fabric driver never call these functions, and expect the SSI and codec drivers to gather this information itself. But the codec driver is not an OF driver, so it has no expectation of being able to query the codec node.
I would solve the problem this way: In the ssi driver, if the codec-node property is present, then call a function to instantiate a simple or platform specific sound card instance that makes the assumptions listed above. If not, then just register the ssi and exit, which leaves the ssi available for a separate driver to pick it up. I wouldn't do this for new platforms, but it gracefully makes use of the data provided in the current 8610 device tree.
Eh, I'll have to think about that. The absence of a codec pointer in the SSI node means that the SSI is not connected to a codec, so it should just be ignored altogether. An SSI is useless if it's not connected to a codec.
BTW Timur, there is nothing wrong with registering multiple devices that all have the of_node pointer set to the same node.
Sorry, I don't understand what you're getting at.
On Tue, Apr 27, 2010 at 03:46:04PM -0500, Timur Tabi wrote:
[Reflowed into 80 columns; please fix your mail client.]
(I've omitted the DMA nodes and some irrelevant details) This is enough information for a simplistic driver registration that probably makes a lot of assumptions. Such as the ssi represents a single logical sound device. It won't handle complex representations, but in
Why would I ever represent the SSI as anything but a single logical sound device? Let ALSA handle synchronizing multiple streams together if it wants to.
...
dai-links = <&ssi1 0 &codec 0 &ssi1 1 &codec 1>; [...]
I don't know when I would ever do this. The two SSI devices are completely independent. Why would I bind them together into one "device"?
It's entirely possible that if the board designer intended the verious SSIs to be used in concert they've done something like cross wire the clocks which creates a board-specific interrelationship that needs to be dealt with.
Mark Brown wrote:
It's entirely possible that if the board designer intended the verious SSIs to be used in concert they've done something like cross wire the clocks which creates a board-specific interrelationship that needs to be dealt with.
Fine, but I don't see how that can be handled with the current code. Each SSI is independent, and audio is streamed to it via DMA. The current SSI driver would need to be completely rewritten in order to initiate both DMA operations simultaneously. The clocking is the least of my problems.
On Tue, Apr 27, 2010 at 04:03:27PM -0500, Timur Tabi wrote:
[Reflowing the text into 80 columns again]
Mark Brown wrote:
It's entirely possible that if the board designer intended the verious SSIs to be used in concert they've done something like cross wire the clocks which creates a board-specific interrelationship that needs to be dealt with.
Fine, but I don't see how that can be handled with the current code. Each SSI is independent, and audio is streamed to it via DMA. The current SSI driver would need to be completely rewritten in order to initiate both DMA operations simultaneously. The clocking is the least of my problems.
I believe the usual technique is to start the DMA then clock the bus - data doesn't flow over the bus until the clock appears and that appears everywhere simultaneously. Obviously some hardware really doesn't like having the DMA blocked like that, but not all.
On Tue, 2010-04-27 at 21:59 +0100, Mark Brown wrote:
It's entirely possible that if the board designer intended the verious SSIs to be used in concert they've done something like cross wire the clocks which creates a board-specific interrelationship that needs to be dealt with.
In which case, have that in some board specific code :-) Really, as I said earlier, I think there's no point to aim toward a uber-representation that can describe everything along with code that can cope with anything the tree can describe :-) That's just insane.
I'd stay stick to the basics and move incrementally up until it stops making sense:
- First, the basics: nodes for actual physical devices. i2c codecs on their i2c busses, DMA controllers, etc...
- From there, see what simple properties are better off being put in those respective nodes because they represent common enough variants of functionality or simply because they are specific attributes of a given device that aren't too concerned by the way things are interconnected.
- Provide at least one identifier (either in a "sound" virtual device, or just use the board's own "compatible" property) to have a .c file to put everything together.
From there, you may decide that 90% of the simple cases can be very
easily represented by a couple of properties in the said "sound" node, and henceforth, create a simple.c "board" file that matches against a list of boards known to fit within that simple model, and that pretty much exclusively use the device-tree to put things together.
But you don't have to.
The whole thing is a matter of common sense and a bit of taste :-)
Cheers, Ben.
On Wed, Apr 28, 2010 at 02:25:29PM +1000, Benjamin Herrenschmidt wrote:
I'd stay stick to the basics and move incrementally up until it stops making sense:
- First, the basics: nodes for actual physical devices. i2c codecs on
their i2c busses, DMA controllers, etc...
This is already supported by the ASoC core and has been for about a year or so, it just needs the device drivers to make use of it. IIRC the drivers relevant to PowerPC are pretty much doing so already, but I didn't actually check.
- From there, see what simple properties are better off being put in
those respective nodes because they represent common enough variants of functionality or simply because they are specific attributes of a given device that aren't too concerned by the way things are interconnected.
This one is just a case of writing boiler plate for the existing platform data devices have to convert that from device tree into the existing formats. It's not a monumentally edifying task, but I don't think we really need to worry about it here.
- Provide at least one identifier (either in a "sound" virtual device,
or just use the board's own "compatible" property) to have a .c file to put everything together.
This has been the big sticking point so far. It has been difficult to get people to accept this idea at all, the idea that there may be more to the machine specific setup than can be readily expressed in static data and might even vary dynamically at runtime is what seems to upset people. I've been told on a number of occasions that instantiating machine specific code that's not part of a specific chip is very difficult and would cause the device tree to be horribly buggy.
What you're suggesting here is exactly what is supposed to happen from an ASoC point of view.
The whole thing is a matter of common sense and a bit of taste :-)
The impression that has been created in the past is that there are inflexible device tree rules which can't be varied.
On Wed, 2010-04-28 at 14:00 +0100, Mark Brown wrote:
The whole thing is a matter of common sense and a bit of taste :-)
The impression that has been created in the past is that there are inflexible device tree rules which can't be varied.
I'm a bit sad this is how things have been perceived since that's clearly not the policy I've applied to the powerpc architecture.
Or rather, there are -some- inflexible rules yes, which are to:
- Have a device-tree :-)
- Have a /compatible property at the toplevel to identify your board
- Have the /cpus nodes for representing the CPUs.
That's pretty much the only absolute requirements from a code perspective.
Now I -do- require people to also have nodes for things like PCI host bridge, since that allows using a ton of existing code for handling most aspects of PCI, and I -do- complain if people just hard wire platform devices everywhere or interrupt numbers without even trying to consider using the device-tree appropriately.
However, I've always been against the one-bsp-fits-all approach, and it's always been my clear policy that there should be a per-machine .c file. I did bend when folks pushed the "simple" platform but with the understanding that it must contain an -explicit- list of boards it supports.
You'll also notice that all of my virtual interrupt handling stuff is such that you -can- use it without device-tree nodes, the DT just makes it easier. Same goes with PCI devices (only the PHB requires a DT node at this stage) etc...
Cheers, Ben.
On Tue, Apr 27, 2010 at 2:46 PM, Timur Tabi timur@freescale.com wrote:
Grant Likely wrote:
So, the current 86xx device tree binding assumes a simple layout with a node describing an DAI controller, and another node describing the codec with a single phandle (pointer) from the DAI to the codec. In this configuration, it is completely reasonable for the DAI node to trigger both the instantiation of the ASoC DAI controller device and the sound card device. Linux can treat them as separate even though the current device tree has a simplistic representation.
The only problem with this is that there is board-specific programming that needs to be done (look at mpc8610_hpcd_machine_probe), so we still need to have a fabric driver that is independent of the SSI, codec, or DMA drivers. The P1022 also has an SSI, and I'm hoping that all I need to do is create a new fabric driver, not hack up the SSI driver to support board programming.
So if the fabric driver still needs to exist, then it still needs a struct device, and it still needs to register with asoc. I don't see how I can register the sound card itself in the SSI driver, because it won't know anything about the board-specific code in the fabric driver.
Why not? Just have the ssi driver probe routine register the fabric device based on the existence of the codec-handle property. It is the best way to go about things with the data that you've got available, and it is no big deal. The relevant fabric driver can then bind against that. You should probably also stuff the ssi device node pointer into the fabric device of_node pointer.
I've tried very hard to maintain a distinction between device tree binding (representation) and Linux kernel internal implementation details. The real question is whether or not the binding provides sufficient detail for the operating system to figure out what to do.
I think it does, because it's working today.
In the extreme minimalist case, the audio driver could decide how to configure itself solely on the board name property of the root node. There is nothing wrong with that, but it also means that no data is available to dynamically select common modules or modify connections; it all has to be hard coded.
Well, asoc already has several hard-coded requirements:
machine_data->dai.cpu_dai_drv = &fsl_ssi_dai; machine_data->dai.codec_dai_drv = &cs4270_dai; machine_data->dai.codec_drv = &soc_codec_device_cs4270; machine_data->dai.ops = &mpc8610_hpcd_ops; machine_data->dai.platform_drv = &fsl_soc_platform;
So even though I probe for each device separately and register them separately, the fabric driver still needs to have hard-coded addresses
Maybe the asoc guys can tell me why I need to register the cpu_dai_drv structure via platform_device_add(), when it's already being registered via snd_soc_register_dai().
(I've omitted the DMA nodes and some irrelevant details) This is enough information for a simplistic driver registration that probably makes a lot of assumptions. Such as the ssi represents a single logical sound device. It won't handle complex representations, but in a lot of cases that may be just fine.
Why would I ever represent the SSI as anything but a single logical sound device? Let ALSA handle synchronizing multiple streams together if it wants to.
sound { compatible = "fsl,mpc8610-hpcd-sound"; /* maybe something like (totally off the top of my head) */ dai-links = <&ssi1 0 &codec 0 &ssi1 1 &codec 1>; [...] };
I don't know when I would ever do this. The two SSI devices are completely independent. Why would I bind them together into one "device"?
If there is no use case for binding them together, then you're right. The current binding is probably just fine. I cannot comment on whether or not it will be used that way by platform designers.
Where the 'sound' node is now the starting point for representing a logical sound device instead of the ssi node. This binding probably makes more sense (but I'm not committing to anything like this until I see a real proposal for a real device).
The only issue I have with this is all it does is turn the fabric driver from a platform driver that scans the OF tree, into an OF driver that still needs to query the device tree to get all the data it needs. For example, the fabric driver still needs to know the clock frequency and direction of the codec node, so that it can call snd_soc_dai_set_fmt() and snd_soc_dai_set_sysclk() properly. To eliminate that, we could have the fabric driver never call these functions, and expect the SSI and codec drivers to gather this information itself. But the codec driver is not an OF driver, so it has no expectation of being able to query the codec node.
I would solve the problem this way: In the ssi driver, if the codec-node property is present, then call a function to instantiate a simple or platform specific sound card instance that makes the assumptions listed above. If not, then just register the ssi and exit, which leaves the ssi available for a separate driver to pick it up. I wouldn't do this for new platforms, but it gracefully makes use of the data provided in the current 8610 device tree.
Eh, I'll have to think about that. The absence of a codec pointer in the SSI node means that the SSI is not connected to a codec, so it should just be ignored altogether. An SSI is useless if it's not connected to a codec.
I'm suggesting the case where a third node describes the mappings between codecs and SSIs.
BTW Timur, there is nothing wrong with registering multiple devices that all have the of_node pointer set to the same node.
Sorry, I don't understand what you're getting at.
Linux struct device registrations are cheap, and every struct device has a device_node pointer available. It is totally fine to have both the ssi device and the fabric device point to the same device node if that helps solve your problem of finding references to the right things in each driver. (Just as long as only one of them is an of_platform driver).
g.
On Wed, Apr 28, 2010 at 12:37 AM, Grant Likely grant.likely@secretlab.ca wrote:
Why not? Just have the ssi driver probe routine register the fabric device based on the existence of the codec-handle property. It is the best way to go about things with the data that you've got available, and it is no big deal. The relevant fabric driver can then bind against that. You should probably also stuff the ssi device node pointer into the fabric device of_node pointer.
And then where do I put the board-specific initialization code that's currently in the fabric driver? The programming information for that initialization is not in the device tree.
It sounds to me like you're saying I should take all the code from the fabric driver and shove it into the SSI driver, just so that I can avoid instantiating a platform driver.
Keep in mind that asoc likes to have a different struct device for the fabric driver and the SSI nodes, so I would need to manually create a struct device for the fabric device anyway.
I don't know when I would ever do this. The two SSI devices are completely independent. Why would I bind them together into one "device"?
If there is no use case for binding them together, then you're right. The current binding is probably just fine. I cannot comment on whether or not it will be used that way by platform designers.
Although the 8610 has two SSI devices, the reference board we ship only has one wired up. I have a prototype board in the lab that has both wired up, but even then, I don't think I can get the two SSIs synchronized in any meaningful fashion. Mark suggested a technique, but even if I could get that to work, the driver code would need to be rewritten to handle two SSI devices in concert. In short, it might be theoretically possible, but I'm not going to try to make it work.
Linux struct device registrations are cheap, and every struct device has a device_node pointer available. It is totally fine to have both the ssi device and the fabric device point to the same device node if that helps solve your problem of finding references to the right things in each driver. (Just as long as only one of them is an of_platform driver).
But I already have it set up like that. The SSI driver is an OF driver, and the fabric driver is a platform driver. I might be able to move some code from the fabric driver into the SSI driver to make it the fabric driver less obnoxious about scanning the device tree.
On Wed, Apr 28, 2010 at 7:35 AM, Timur Tabi timur@freescale.com wrote:
On Wed, Apr 28, 2010 at 12:37 AM, Grant Likely grant.likely@secretlab.ca wrote:
Why not? Just have the ssi driver probe routine register the fabric device based on the existence of the codec-handle property. It is the best way to go about things with the data that you've got available, and it is no big deal. The relevant fabric driver can then bind against that. You should probably also stuff the ssi device node pointer into the fabric device of_node pointer.
And then where do I put the board-specific initialization code that's currently in the fabric driver? The programming information for that initialization is not in the device tree.
In the fabric driver; where it is right now. I'm saying *instantiate* the device when the ssi driver gets probed. Use the top level board name when assigning the name so that the correct asoc machine driver gets bound to it.
It sounds to me like you're saying I should take all the code from the fabric driver and shove it into the SSI driver, just so that I can avoid instantiating a platform driver.
Nope.
Keep in mind that asoc likes to have a different struct device for the fabric driver and the SSI nodes, so I would need to manually create a struct device for the fabric device anyway.
You can do it this way too, but this is not what I'm saying.
Linux struct device registrations are cheap, and every struct device has a device_node pointer available. It is totally fine to have both the ssi device and the fabric device point to the same device node if that helps solve your problem of finding references to the right things in each driver. (Just as long as only one of them is an of_platform driver).
But I already have it set up like that. The SSI driver is an OF driver, and the fabric driver is a platform driver. I might be able to move some code from the fabric driver into the SSI driver to make it the fabric driver less obnoxious about scanning the device tree.
I'm just saying move the registration of the machine device out of arch/powerpc platform code and into the ssi driver. Then you've got a reasonable place to pass shared data (either the ssi device node or device instance or name. Whatever you need) to the machine driver.
g.
On Wed, Apr 28, 2010 at 8:57 AM, Grant Likely grant.likely@secretlab.ca wrote:
I'm just saying move the registration of the machine device out of arch/powerpc platform code and into the ssi driver.
But the SSI driver is an OF driver, and it gets probed for every SSI node in the device tree. On the 8610, that means being probed twice. But I should only call platform_device_register_simple() once.
Are you saying that I should call platform_device_register_simple() from the SSI's driver initialization function, fsl_ssi_init()?
Then you've got a reasonable place to pass shared data (either the ssi device node or device instance or name. Whatever you need) to the machine driver.
The problem is that the fabric driver needs much more information from the device tree than the SSI driver needs. So if the SSI driver is going to pass that information to the fabric driver via the platform data, it's going to have to know what information the fabric driver needs. Then the SSI driver is not board-independent.
On Wed, Apr 28, 2010 at 10:20 AM, Timur Tabi timur@freescale.com wrote:
On Wed, Apr 28, 2010 at 8:57 AM, Grant Likely grant.likely@secretlab.ca wrote:
I'm just saying move the registration of the machine device out of arch/powerpc platform code and into the ssi driver.
But the SSI driver is an OF driver, and it gets probed for every SSI node in the device tree. On the 8610, that means being probed twice. But I should only call platform_device_register_simple() once.
Didn't you just finish saying that you cannot see any situation where you would want the SSI devices linked into a single audio device? So then if both SSIs are being used for audio, then do you not need a machine driver for each ssi?
Are you saying that I should call platform_device_register_simple() from the SSI's driver initialization function, fsl_ssi_init()?
No, I'm saying call it from the probe hook.
However, you should probably do a two stage platform_device_alloc() / platform_device_add() so you can add data (node pointer) to the platform device before it gets probed by the machine driver.
Then you've got a reasonable place to pass shared data (either the ssi device node or device instance or name. Whatever you need) to the machine driver.
The problem is that the fabric driver needs much more information from the device tree than the SSI driver needs. So if the SSI driver is going to pass that information to the fabric driver via the platform data, it's going to have to know what information the fabric driver needs. Then the SSI driver is not board-independent.
I'm not talking about platform_data or about the ssi driver decoding things that the machine driver needs. I'm talking about giving the machine driver a pointer to the ssi device tree node so you don't need to go through weird gymnastics to find the correct node again.
g.
Grant Likely wrote:
Didn't you just finish saying that you cannot see any situation where you would want the SSI devices linked into a single audio device? So then if both SSIs are being used for audio, then do you not need a machine driver for each ssi?
That's right. But in order for my fabric/machine driver to be called at all, I thought I needed to make it a platform driver and use platform_device_register_simple() and platform_driver_register().
Are you saying that I should call platform_device_register_simple() from the SSI's driver initialization function, fsl_ssi_init()?
No, I'm saying call it from the probe hook.
But then platform_device_register_simple() will be called twice, and it should be called only once.
I must be missing something here.
However, you should probably do a two stage platform_device_alloc() / platform_device_add() so you can add data (node pointer) to the platform device before it gets probed by the machine driver.
Don't you mean platform_device_add_resources() instead of platform_device_add()? That is, use platform_device_add_resources() too add information about each SSI that gets probed, and then after all of the SSIs are probed, then call platform_device_add().
If that's what you mean, then where do I call platform_device_add()?
I'm not talking about platform_data or about the ssi driver decoding things that the machine driver needs. I'm talking about giving the machine driver a pointer to the ssi device tree node so you don't need to go through weird gymnastics to find the correct node again.
The only gymnastics I need to go through is this:
while ((np = of_find_compatible_node(np, NULL, "fsl,mpc8610-ssi"))) {
This finds every SSI node. I then extract the information from the SSI nodes and all the other nodes, and then register it with ASoC.
On Tue, Apr 27, 2010 at 02:24:34PM -0600, Grant Likely wrote:
However, as you and Mark rightly point out, it completely fails to represent complex sound devices with weird clocking and strange routes. Something like this is probably more appropriate:
[...] codec1 :codec@4f { compatible = "cirrus,cs4270"; reg = <0x4f>; /* MCLK source is a stand-alone oscillator */ clock-frequency = <12288000>; };
You also want to be representing unused pins here.
[...] ssi1: ssi@16000 { compatible = "fsl,mpc8610-ssi"; [...] fsl,mode = "i2s-slave";
I'd not include the master/slave decision; it's either implied by the fact that the CODEC has a standalone clock, a property of the link/card, or a policy decision that the running software can change on a whim.
sound { compatible = "fsl,mpc8610-hpcd-sound"; /* maybe something like (totally off the top of my head) */ dai-links = <&ssi1 0 &codec 0 &ssi1 1 &codec 1>;
I'm having a hard time loving this. I'd be looking for a lot more semantics on the links (things like information about separate clocks for the two directions, for example) which makes me think that that simple list format is far too simple and you want a list of more complex objects.
There's also analogue interconnects to deal with, and jacks (including detection and accessories). Jacks can be particularly entertaining here.
Or, in other words, the device tree should *not* be used to describe every possible detail and permutation. It is best used to describe the permutations that are common so that they don't need to be hard coded for each and every board.
I think the ideal is something that's purely descriptive of the hardware and doesn't include any policy decisions. Policy decisions covers an awful lot of the interesting issues, though - but they're the sort of things that can easily be changed with a new software load and therefore don't belong in the device tree.
On the other hand from a pragmatic point of view it's just much less hassle to just only provide the mechanism for instantiating a machine with custom code and use that for everything.
On Tue, Apr 27, 2010 at 4:29 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Tue, Apr 27, 2010 at 02:24:34PM -0600, Grant Likely wrote:
However, as you and Mark rightly point out, it completely fails to represent complex sound devices with weird clocking and strange routes. Something like this is probably more appropriate:
[...] codec1 :codec@4f { compatible = "cirrus,cs4270"; reg = <0x4f>; /* MCLK source is a stand-alone oscillator */ clock-frequency = <12288000>; };
You also want to be representing unused pins here.
[...] ssi1: ssi@16000 { compatible = "fsl,mpc8610-ssi"; [...] fsl,mode = "i2s-slave";
I'd not include the master/slave decision; it's either implied by the fact that the CODEC has a standalone clock, a property of the link/card, or a policy decision that the running software can change on a whim.
sound { compatible = "fsl,mpc8610-hpcd-sound"; /* maybe something like (totally off the top of my head) */ dai-links = <&ssi1 0 &codec 0 &ssi1 1 &codec 1>;
I'm having a hard time loving this. I'd be looking for a lot more semantics on the links (things like information about separate clocks for the two directions, for example) which makes me think that that simple list format is far too simple and you want a list of more complex objects.
Oh, absolutely. This example is no where near complete. Mostly I just wanted to give a concrete example of a 'virtual' device like Ben was talking about. I'm not going to even attempt to draft a complete binding until I've got time to properly go over the issues involved and discuss them with you and Liam.
There's also analogue interconnects to deal with, and jacks (including detection and accessories). Jacks can be particularly entertaining here.
Or, in other words, the device tree should *not* be used to describe every possible detail and permutation. It is best used to describe the permutations that are common so that they don't need to be hard coded for each and every board.
I think the ideal is something that's purely descriptive of the hardware and doesn't include any policy decisions. Policy decisions covers an awful lot of the interesting issues, though - but they're the sort of things that can easily be changed with a new software load and therefore don't belong in the device tree.
Agreed.
On the other hand from a pragmatic point of view it's just much less hassle to just only provide the mechanism for instantiating a machine with custom code and use that for everything.
Also true, but this approach carries with it an incremental cost that distributions feel the pain of. Ultimately I think we'll find a sweet spot somewhere in between.
Cheers, g.
On Tue, Apr 27, 2010 at 08:31:18PM -0600, Grant Likely wrote:
On Tue, Apr 27, 2010 at 4:29 PM, Mark Brown
On the other hand from a pragmatic point of view it's just much less hassle to just only provide the mechanism for instantiating a machine with custom code and use that for everything.
Also true, but this approach carries with it an incremental cost that distributions feel the pain of. Ultimately I think we'll find a sweet spot somewhere in between.
Meh, it's not really much hassle for the distributions - it's all handled by the kernel, they don't need to explicitly do anything.
None of the machine-specific stuff has ever been a hassle getting stuff merged, problems have always been in the drivers for the devices which device tree isn't going to make a blind bit of difference to (other than the usual discussions about what exactly the device tree should look like).
On Tue, 2010-04-27 at 23:29 +0100, Mark Brown wrote:
On the other hand from a pragmatic point of view it's just much less hassle to just only provide the mechanism for instantiating a machine with custom code and use that for everything.
Right, that's the balance to find. A too descriptive device-tree becomes a mess, and attempting to deal with any kind of representation in SW is horrible.
There is a fine balance to be found between how much goes into the device-tree and how much is eventually just a plain C file that puts things together.
For example, one could imagine a /sound node with simply a "compatible" property that matches what we call in AOA terminology a "fabric" driver. IE. The one thing specific to the board that puts bits and pieces together.
Now, the device-tree is still obviously useful to provide things like the actual i2c IDs of codecs, GPIOs used for various actions, link to from various components to their clock source devices, etc.. All these things simplify the code, avoids horrid board specific code in the actual drivers (codecs, busses, etc...) and overall help keeping the code more maintainable.
This is not an issue specific to audio. The same problem to some extent shows up at the arch level, which is why I was never too much in favor of doing a "generic" platform on powerpc, but still want to have a per board (or at least board family) platform .c file which has the upper hand, even if it ends up mostly using device-tree based "helpers" to put things together.
The device-tree helps keep the platform .c file simple and devoid of too horrible hacks, it allows to easily pass various configuration data to leaf drivers such as i2c thingies, PHY devices etc... without gross hooks between these and the platform, but the platform code still has the upper hand for doing ad-hoc bits and pieces (or overwriting the device-tree based behaviour) if necessary.
Cheers, Ben.
On Wed, Apr 28, 2010 at 02:10:11PM +1000, Benjamin Herrenschmidt wrote:
On Tue, 2010-04-27 at 23:29 +0100, Mark Brown wrote:
On the other hand from a pragmatic point of view it's just much less hassle to just only provide the mechanism for instantiating a machine with custom code and use that for everything.
Right, that's the balance to find. A too descriptive device-tree becomes a mess, and attempting to deal with any kind of representation in SW is horrible.
Sadly this often seems to be what the device tree folks are pushing for. I really want whatever you guys come up with to explicitly say that it's OK to just write standard code like we have at the minute and not faff around defining device tree representations for everything so that we can just point people at that when the discussion gets too bogged down.
For example, one could imagine a /sound node with simply a "compatible" property that matches what we call in AOA terminology a "fabric" driver. IE. The one thing specific to the board that puts bits and pieces together.
This is the current ASoC design (someone probably ought to look at merging AOA into ASoC, it's approximately the same hardware and at least the CPU driver ought to be useful for other systems).
Now, the device-tree is still obviously useful to provide things like the actual i2c IDs of codecs, GPIOs used for various actions, link to from various components to their clock source devices, etc.. All these things simplify the code, avoids horrid board specific code in the actual drivers (codecs, busses, etc...) and overall help keeping the code more maintainable.
You're preaching to the choir here. With ASoC all this system specific code ends up in the machine driver (which you guys are calling the fabric driver). All the design stuff you're talking about here is already dealt with as-is, it's just that the parameterisation is done with C code and data structures in the kernel (which can deal with multiple boards if it chooses to) rather than with device tree stuff. The chip specific drivers do not have any board specific code so there is no meaningful change that's being proposed to them.
The problems with the device tree have been that people have been hostile to the idea of there being any board specific code at all in the kernel (or a board specific device tree node for the audio), and that people keep wanting to define some OF stuff that is supposed to cover a wide range of boards but makes unrealistic simplifying assumptions about what general hardware looks like.
The device-tree helps keep the platform .c file simple and devoid of too horrible hacks, it allows to easily pass various configuration data to leaf drivers such as i2c thingies, PHY devices etc... without gross hooks between these and the platform, but the platform code still has the upper hand for doing ad-hoc bits and pieces (or overwriting the device-tree based behaviour) if necessary.
Once again, if you can get the device tree guys to buy into this and stick with it that sounds good but my experience has been that this isn't where any of these discussions end up.
On Wed, 2010-04-28 at 13:07 +0100, Mark Brown wrote:
The device-tree helps keep the platform .c file simple and devoid of too horrible hacks, it allows to easily pass various configuration data to leaf drivers such as i2c thingies, PHY devices etc... without gross hooks between these and the platform, but the platform code still has the upper hand for doing ad-hoc bits and pieces (or overwriting the device-tree based behaviour) if necessary.
Once again, if you can get the device tree guys to buy into this and stick with it that sounds good but my experience has been that this isn't where any of these discussions end up.
Well, as the person who came up with the flattened device-tree format in the first place I suppose I qualify as a "device-tree" guy here :-)
At the moment, I'd say Grant (and to some extent Jeremy Kerr) are the guys in charge though, but yes, I agree with you, there's a tendency to be too over-exited and to want to do "too much" with the DT and that is counter productive. It's a good tool but it's not going to solve world hunger and in some places an ad-hoc bit of C code is a better option :)
Now, I don't think Grant is totally off the tracks here but I must admit I haven't taken the time to ensure I understand perfectly everybody's position in that debate. At least I made mine clear, hope this helps :-)
Cheers, Ben.
On Wed, Apr 28, 2010 at 6:36 PM, Benjamin Herrenschmidt benh@kernel.crashing.org wrote:
On Wed, 2010-04-28 at 13:07 +0100, Mark Brown wrote:
The device-tree helps keep the platform .c file simple and devoid of too horrible hacks, it allows to easily pass various configuration data to leaf drivers such as i2c thingies, PHY devices etc... without gross hooks between these and the platform, but the platform code still has the upper hand for doing ad-hoc bits and pieces (or overwriting the device-tree based behaviour) if necessary.
Once again, if you can get the device tree guys to buy into this and stick with it that sounds good but my experience has been that this isn't where any of these discussions end up.
Well, as the person who came up with the flattened device-tree format in the first place I suppose I qualify as a "device-tree" guy here :-)
At the moment, I'd say Grant (and to some extent Jeremy Kerr) are the guys in charge though, but yes, I agree with you, there's a tendency to be too over-exited and to want to do "too much" with the DT and that is counter productive. It's a good tool but it's not going to solve world hunger and in some places an ad-hoc bit of C code is a better option :)
Now, I don't think Grant is totally off the tracks here but I must admit I haven't taken the time to ensure I understand perfectly everybody's position in that debate. At least I made mine clear, hope this helps :-)
After an IRC conversation with Timur, I think we've pretty much sorted out the best way to handle the mpc8610 use case that allows the ssi/dma/codec drivers to remain blissfully ignorant and bind in the appropriate ASoC machine driver for the board.
g.
On Tue, Apr 27, 2010 at 5:29 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
codec1 :codec@4f { compatible = "cirrus,cs4270"; reg = <0x4f>; /* MCLK source is a stand-alone oscillator */ clock-frequency = <12288000>; };
You also want to be representing unused pins here.
If they're unused, how do I represent them? Can you give me an example?
[...] ssi1: ssi@16000 { compatible = "fsl,mpc8610-ssi"; [...] fsl,mode = "i2s-slave";
I'd not include the master/slave decision; it's either implied by the fact that the CODEC has a standalone clock, a property of the link/card, or a policy decision that the running software can change on a whim.
I know it's redundant, but at the time, it seemed a lot simpler than walking the device tree.
Frankly, I'd rather not consider minor device tree changes at this point. I'm hoping I don't need to change the device tree at all.
sound { compatible = "fsl,mpc8610-hpcd-sound"; /* maybe something like (totally off the top of my head) */ dai-links = <&ssi1 0 &codec 0 &ssi1 1 &codec 1>;
I'm having a hard time loving this. I'd be looking for a lot more semantics on the links (things like information about separate clocks for the two directions, for example) which makes me think that that simple list format is far too simple and you want a list of more complex objects.
Yeah, I don't like it either. It seems arbitrary.
I think the ideal is something that's purely descriptive of the hardware and doesn't include any policy decisions.
I like to think that this is what we have today.
On Wed, Apr 28, 2010 at 08:19:00AM -0500, Timur Tabi wrote:
On Tue, Apr 27, 2010 at 5:29 PM, Mark Brown
You also want to be representing unused pins here.
If they're unused, how do I represent them? Can you give me an example?
You should arrange for something to call snd_soc_dapm_nc_pin() on any unconnected pins.
On Tue, Apr 27, 2010 at 04:36:08PM +1000, Benjamin Herrenschmidt wrote:
Gross. Loses the linkage to device-tree etc... which is useful for audio especially. You should at least make sure the device node follows for the target driver to be able to use it :-) I'd like you to sync with Grant work on that matter if you are going to convert of_devices into platform_devices.
So, Liam already sent a message explaining the technical side of this - the audio subsystem needs to glue together two or more components that don't immediately have anything to do with each other from a device tree point of view and may have rather complex interrelationships.
I'd just like to add that I *really* want to see you guys come to some sort of firm and documented conclusion about the way to handle situations like this. Some variant of this seems to come up every single time anyone tries to do anything to do with audio on a system using the device tree and it's getting really repetitive. What would be really useful for audio at this point would be if we could get some sort of decision about how to represent this stuff which we can point people at so that work on systems using the device tree can be done without having to deal with the device tree layout discussions that frequently seem to be involved.
On Tue, 2010-04-27 at 10:54 +0100, Mark Brown wrote:
I'd just like to add that I *really* want to see you guys come to some sort of firm and documented conclusion about the way to handle situations like this. Some variant of this seems to come up every single time anyone tries to do anything to do with audio on a system using the device tree and it's getting really repetitive. What would be really useful for audio at this point would be if we could get some sort of decision about how to represent this stuff which we can point people at so that work on systems using the device tree can be done without having to deal with the device tree layout discussions that frequently seem to be involved.
Agreed. Just seeing how Apple fucked it up so many times, it's not a simple problem :-)
The device-tree allows to express all of these relationship but we should be able to come up with a reasonably "standard" way to do so to avoid every SoC or platform doing it it's "own" way.
I think the main deal is to decide who gets to be the "master" node which contains the various properties doing the linkage. My gut feeling is that it could be the main transport, ie, the i2s or ac97, but people with more experience dealing with that stuff might have other ideas.
Keep in mind that it's perfectly kosher to create nodes for "virtual" devices. IE. We could imagine a node for the "sound subsystem" that doesn't actually correspond to any physical device but contain the necessary properties that binds everything together. You could even have multiple of these if you have separate set of sound HW that aren't directly dependant.
I don't have bandwidth to contribute much in this discussion right now, at least not to lead it, so I'm happy to let others do so, but I'm happy to provide feedback from my own experience as proposals are made.
Cheers, Ben.
On Tue, Apr 27, 2010 at 08:09:15PM +1000, Benjamin Herrenschmidt wrote:
I think the main deal is to decide who gets to be the "master" node which contains the various properties doing the linkage. My gut feeling is that it could be the main transport, ie, the i2s or ac97, but people with more experience dealing with that stuff might have other ideas.
You're not going to find a single master transport node on more complex systems - some systems have multiple audio interfaces to the CPU (to allow use of hardware mixing, for example), and in systems with things like offboard DSPs or basebands it's not always clear that the CPU Linux is running on is in charge of anything.
Keep in mind that it's perfectly kosher to create nodes for "virtual" devices. IE. We could imagine a node for the "sound subsystem" that doesn't actually correspond to any physical device but contain the necessary properties that binds everything together. You could even have multiple of these if you have separate set of sound HW that aren't directly dependant.
In terms of where to shove any data this is sort of the solution I favour, it's pretty much exactly what's implemented on other platforms at the moment and it seems to adequately represent the physical board (it's not a million miles away from what Timur has here).
The main problem is that trying to define a language which can represent the needs of modern mobile audio subsystems just doesn't seem worth the effort. The clocking arrangements for modern mobile audio subsystems aren't trivial, are normally highly device and system specific, and when you add on more exotic things like sharing the bit and frame clocks between multiple audio interfaces it gets even more involved.
On Tue, Apr 27, 2010 at 4:09 AM, Benjamin Herrenschmidt benh@kernel.crashing.org wrote:
On Tue, 2010-04-27 at 10:54 +0100, Mark Brown wrote:
I'd just like to add that I *really* want to see you guys come to some sort of firm and documented conclusion about the way to handle situations like this. Some variant of this seems to come up every single time anyone tries to do anything to do with audio on a system using the device tree and it's getting really repetitive. What would be really useful for audio at this point would be if we could get some sort of decision about how to represent this stuff which we can point people at so that work on systems using the device tree can be done without having to deal with the device tree layout discussions that frequently seem to be involved.
Yes, you're right. I completely agree.
[...]
Keep in mind that it's perfectly kosher to create nodes for "virtual" devices. IE. We could imagine a node for the "sound subsystem" that doesn't actually correspond to any physical device but contain the necessary properties that binds everything together. You could even have multiple of these if you have separate set of sound HW that aren't directly dependant.
I don't have bandwidth to contribute much in this discussion right now, at least not to lead it, so I'm happy to let others do so, but I'm happy to provide feedback from my own experience as proposals are made.
Unfortunately, I'm in the same boat. :-( However, I'll be at UDS in 2 weeks time, and I know audio is a big concern for the Ubuntu folks. A bunch of the ARM vendors will be there too. I'll schedule a session to talk about audio bindings and hopefully that way make some headway on defining a binding that makes sense and is actually useful.
g.
On Tue, Apr 27, 2010 at 02:27:42PM -0600, Grant Likely wrote:
On Tue, Apr 27, 2010 at 4:09 AM, Benjamin Herrenschmidt
I don't have bandwidth to contribute much in this discussion right now, at least not to lead it, so I'm happy to let others do so, but I'm happy to provide feedback from my own experience as proposals are made.
Unfortunately, I'm in the same boat. :-( However, I'll be at UDS in 2 weeks time, and I know audio is a big concern for the Ubuntu folks. A bunch of the ARM vendors will be there too. I'll schedule a session to talk about audio bindings and hopefully that way make some headway on defining a binding that makes sense and is actually useful.
Hrm, the only issue that's been raised upstream is multi-CODEC (there are one or two other things that boil down to multi-CODEC, but nothing else I'm aware of). If you schedule something please announce it here, I believe that UDS generally has arrangements for remote participation.
Note that most of the complexity explosion you'll see here is visible in things like phones more than the sort of devices you'd run Ubuntu on currently and much of it isn't really visible to a lot of the CPU vendors at all since it's generally all handled off the CPU.
Mark Brown wrote:
Hrm, the only issue that's been raised upstream is multi-CODEC (there are one or two other things that boil down to multi-CODEC, but nothing else I'm aware of). If you schedule something please announce it here, I believe that UDS generally has arrangements for remote participation.
Keep in mind that the phandle-type properties can accept multiple phandles. So for instance, if I had two codecs attached to an SSI, the SSI node would look like this:
ssi@16000 { compatible = "fsl,mpc8610-ssi"; ... codec-handle = <&cs4270_1 &cs4270_2>; };
On Tue, 2010-04-27 at 14:27 -0600, Grant Likely wrote:
Unfortunately, I'm in the same boat. :-( However, I'll be at UDS in 2 weeks time, and I know audio is a big concern for the Ubuntu folks. A bunch of the ARM vendors will be there too. I'll schedule a session to talk about audio bindings and hopefully that way make some headway on defining a binding that makes sense and is actually useful.
It's not clear if I'm going to UDS atm, but I'll join via IRC. Graeme is going to UDS so he will probably pop in to the discussion if he's free.
Liam
On Tue, Apr 27, 2010 at 5:09 AM, Benjamin Herrenschmidt benh@kernel.crashing.org wrote:
Keep in mind that it's perfectly kosher to create nodes for "virtual" devices. IE. We could imagine a node for the "sound subsystem" that doesn't actually correspond to any physical device but contain the necessary properties that binds everything together. You could even have multiple of these if you have separate set of sound HW that aren't directly dependant.
First, I want to officially retract this patch. I've talked with Grant, and we've come up with a different approach to this problem.
Second, how about this binding for the virtual sound node? It would be a root-level node.
sound-devices { sound0 { ssi = &ssi0; playback-dma = &dma00; capture-dma = &dma01; codec = &cs4270; } };
On Wed, Apr 28, 2010 at 2:35 PM, Timur Tabi timur.tabi@gmail.com wrote:
On Tue, Apr 27, 2010 at 5:09 AM, Benjamin Herrenschmidt benh@kernel.crashing.org wrote:
Keep in mind that it's perfectly kosher to create nodes for "virtual" devices. IE. We could imagine a node for the "sound subsystem" that doesn't actually correspond to any physical device but contain the necessary properties that binds everything together. You could even have multiple of these if you have separate set of sound HW that aren't directly dependant.
First, I want to officially retract this patch. I've talked with Grant, and we've come up with a different approach to this problem.
Second, how about this binding for the virtual sound node? It would be a root-level node.
sound-devices { sound0 { ssi = &ssi0; playback-dma = &dma00; capture-dma = &dma01; codec = &cs4270; } };
The sound0 node needs a compatible value, the sound-device node should probably have one too.
The sound0 node should have something board specific like "fsl,mpc8610hpcd-sound" to make it clear that the binding really only applies to this particular board. It would also be a good idea to prefix all of the property names with 'fsl,' to avoid conflicting with any future common bindings or conventions. Other boards can use the same binding, but they would get a different compatible value (the driver could bind on both).
I'm not a huge fan of the name "sound-devices" for the parent node. There are other sorts of things that we need 'virtual' device nodes to describe. It would be nice to have a single place for collecting nodes for stuff like this. Perhaps this:
system { compatible = "system-devices"; sound0 { compatible = "fsl,mpc8610hpcd-sound"; fsl,ssi = &ssi0; fsl,playback-dma = &dma00; fsl,capture-dma = &dma01; fsl,codec = &cs4270; }; };
But I really don't have any knowledge of what has been done previously in this regard or if any conventions have been established. Ben, any thoughts?
g.
On Wed, Apr 28, 2010 at 4:58 PM, Grant Likely grant.likely@secretlab.ca wrote:
The sound0 node needs a compatible value,
I knew I was forgetting something
the sound-device node should probably have one too.
The aliases, cpus, and memory node don't have a compatible property, and I was modeling the design after the aliases node.
The sound0 node should have something board specific like "fsl,mpc8610hpcd-sound" to make it clear that the binding really only applies to this particular board. It would also be a good idea to prefix all of the property names with 'fsl,' to avoid conflicting with any future common bindings or conventions. Other boards can use the same binding, but they would get a different compatible value (the driver could bind on both).
The aliases node doesn't have an fsl, prefix. I understand the need for the prefix, but I wonder why we don't do that for the aliases node.
I'm not a huge fan of the name "sound-devices" for the parent node. There are other sorts of things that we need 'virtual' device nodes to describe. It would be nice to have a single place for collecting nodes for stuff like this. Perhaps this:
system { compatible = "system-devices"; sound0 { compatible = "fsl,mpc8610hpcd-sound"; fsl,ssi = &ssi0; fsl,playback-dma = &dma00; fsl,capture-dma = &dma01; fsl,codec = &cs4270; }; };
I like that.
On Wed, Apr 28, 2010 at 4:13 PM, Timur Tabi timur.tabi@gmail.com wrote:
On Wed, Apr 28, 2010 at 4:58 PM, Grant Likely grant.likely@secretlab.ca wrote:
The sound0 node needs a compatible value,
I knew I was forgetting something
:-)
the sound-device node should probably have one too.
The aliases, cpus, and memory node don't have a compatible property, and I was modeling the design after the aliases node.
Well, there are typically three ways to find a node; by name, by device_type and by compatible. device_type is meaningless for the flattened tree, so that's out. Matching by name could potentially have namespace collisions, but I'm not sure. I'll defer to Ben & Mitch's judgment here.
The difference with aliases, cpus and memory nodes is that the conventions around them were defined and agreed on a very long time ago. We could get consensus to do the same here, but I cannot make that call.
The sound0 node should have something board specific like "fsl,mpc8610hpcd-sound" to make it clear that the binding really only applies to this particular board. It would also be a good idea to prefix all of the property names with 'fsl,' to avoid conflicting with any future common bindings or conventions. Other boards can use the same binding, but they would get a different compatible value (the driver could bind on both).
The aliases node doesn't have an fsl, prefix. I understand the need for the prefix, but I wonder why we don't do that for the aliases node.
aliases is not a vendor-specific or limited scope convention.
g.
On Wed, 2010-04-28 at 17:13 -0500, Timur Tabi wrote:
On Wed, Apr 28, 2010 at 4:58 PM, Grant Likely grant.likely@secretlab.ca wrote:
The sound0 node needs a compatible value,
I knew I was forgetting something
the sound-device node should probably have one too.
The aliases, cpus, and memory node don't have a compatible property, and I was modeling the design after the aliases node.
aliases is a bad choice, it's very very special and is neither a device nor a virtual device, like chosen.
cpus is more of a match in your case.
In any case, I agree, you may not really need a compatible prop for the virtual device. In fact, Grant, do we really need an enclosing node like that ? In any case, it's no big deal and shouldn't have much impact on the design.
Cheers, Ben.
The sound0 node should have something board specific like "fsl,mpc8610hpcd-sound" to make it clear that the binding really only applies to this particular board. It would also be a good idea to prefix all of the property names with 'fsl,' to avoid conflicting with any future common bindings or conventions. Other boards can use the same binding, but they would get a different compatible value (the driver could bind on both).
The aliases node doesn't have an fsl, prefix. I understand the need for the prefix, but I wonder why we don't do that for the aliases node.
I'm not a huge fan of the name "sound-devices" for the parent node. There are other sorts of things that we need 'virtual' device nodes to describe. It would be nice to have a single place for collecting nodes for stuff like this. Perhaps this:
system { compatible = "system-devices"; sound0 { compatible = "fsl,mpc8610hpcd-sound"; fsl,ssi = &ssi0; fsl,playback-dma = &dma00; fsl,capture-dma = &dma01; fsl,codec = &cs4270; }; };
I like that.
On Wed, Apr 28, 2010 at 6:52 PM, Benjamin Herrenschmidt benh@kernel.crashing.org wrote:
On Wed, 2010-04-28 at 17:13 -0500, Timur Tabi wrote:
On Wed, Apr 28, 2010 at 4:58 PM, Grant Likely grant.likely@secretlab.ca wrote:
The sound0 node needs a compatible value,
I knew I was forgetting something
the sound-device node should probably have one too.
The aliases, cpus, and memory node don't have a compatible property, and I was modeling the design after the aliases node.
aliases is a bad choice, it's very very special and is neither a device nor a virtual device, like chosen.
cpus is more of a match in your case.
In any case, I agree, you may not really need a compatible prop for the virtual device. In fact, Grant, do we really need an enclosing node like that ?
Mostly I'm concerned about 'polluting' the root node in a way that we'd regret later; but perhaps I'm being overly conservative. The sound node will still be uniquely identified by it's compatible property, so perhaps I'm fretting over nothing.
In any case, it's no big deal and shouldn't have much impact on the design.
Right, the point has been reached of quibbling over trivialities.
g.
On Wed, 2010-04-28 at 15:35 -0500, Timur Tabi wrote:
Second, how about this binding for the virtual sound node? It would be a root-level node.
sound-devices { sound0 { ssi = &ssi0; playback-dma = &dma00; capture-dma = &dma01; codec = &cs4270; } };
Make sure you also have a "compatible" property to uniquely identify the design. You could use the toplevel board one but I'd rather keep a separate one here. I've seen case where the exact same base board may have different sound components (because they are dautherboards for example, but there's a few other cases).
Cheers, Ben.
On Mon, Apr 26, 2010 at 2:49 PM, Timur Tabi timur@freescale.com wrote:
An upcoming change in the architecture of ALSA SoC (ASoC) will require the MPC8610 HPCD's ASoC fabric driver to register as a standard platform driver. Therefore, we need to call platform_device_register_simple() from the board's platform code.
Signed-off-by: Timur Tabi timur@freescale.com
Kumar, the ASoC mainters are willing to pick up this patch, but they want an ACK from you first. Or, you could pick it up, since by itself it's harmless.
arch/powerpc/platforms/86xx/mpc8610_hpcd.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c index 5abe137..66afff3 100644 --- a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c +++ b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c @@ -98,6 +98,9 @@ static int __init mpc8610_declare_of_platform_devices(void) /* Without this call, the SSI device driver won't get probed. */ of_platform_bus_probe(NULL, mpc8610_ids, NULL);
- /* Register the platform device for the ASoC fabric driver */
- platform_device_register_simple("snd-soc-mpc8610-hpcd", 0, NULL, 0);
Since this is a temporary measure, this device registration is probably best put into the .probe() hook of the i2s driver. That will keep everything contained in the same place until we can hammer out a reasonable binding.
g.
Grant Likely wrote:
/* Register the platform device for the ASoC fabric driver */
platform_device_register_simple("snd-soc-mpc8610-hpcd", 0, NULL, 0);Since this is a temporary measure, this device registration is probably best put into the .probe() hook of the i2s driver. That will keep everything contained in the same place until we can hammer out a reasonable binding.
This part is not temporary, I think. The fabric driver will always be a standard platform driver, not an OF driver, because there are no DT nodes that it can probe against. BenH is suggestion that maybe we can create one, but I'm not sure that's really the best approach.
participants (6)
-
Benjamin Herrenschmidt -
Grant Likely -
Liam Girdwood -
Mark Brown -
Timur Tabi -
Timur Tabi