Issues using simple-audio-card driver with Xilinx Audio Formatter
I'm working with a Xilinx UltraScale+ MPSoC platform and trying to get some drivers working for the Xilinx Audio Formatter, I2S Transmitter and I2S Receiver FPGA logic cores. The Audio Formatter is what handles audio DMA and the I2S Transmitter/Receiver handle converting between an AXI stream and I2S.
Right now as a prototype, I just have the I2S Transmitter looped back directly to the I2S Receiver and both of them connected to the Audio Formatter, with the idea that audio played back can be immediately recorded in again. The drivers for these cores are in mainline (sound/soc/xilinx/xlnx_i2s.c and sound/soc/xilinx/xlnx_formatter_pcm.c), but it's the next step of putting them together into a functioning audio device which I am having trouble with. It seems the usual way one would do this is using the simple-audio-card driver and adding some device tree entries to hook everything up. In this case there is no real "codec" so I'm using the SPDIF transmitter and receiver codecs for this - the device tree entries for the simple-audio-card I have look like this:
/* Use S/PDIF transmitter as codec required by simple-audio-card */ playback_codec: playback-codec { compatible = "linux,spdif-dit"; #sound-dai-cells = <0>; };
/* Use S/PDIF receiver as codec required by simple-audio-card */ record_codec: record-codec { compatible = "linux,spdif-dir"; #sound-dai-cells = <0>; };
irxs-audio { compatible = "simple-audio-card"; simple-audio-card,name = "IRXS-Audio"; #address-cells = <1>; #size-cells = <0>;
playback_link: simple-audio-card,dai-link@0 { reg = <0>; format = "i2s";
bitclock-master = <&p_codec_dai>; frame-master = <&p_codec_dai>;
p_cpu_dai: cpu { sound-dai = <&test_audio_i2s_transmitter_0>; };
p_platform_dai: plat { sound-dai = <&test_audio_audio_formatter_0>; };
p_codec_dai: codec { sound-dai = <&playback_codec>; clocks = <&misc_clk_4>; }; };
record_link: simple-audio-card,dai-link@1 { reg = <1>; format = "i2s";
bitclock-master = <&r_codec_dai>; frame-master = <&r_codec_dai>;
r_cpu_dai: cpu { sound-dai = <&test_audio_i2s_receiver_0>; };
r_platform_dai: plat { sound-dai = <&test_audio_audio_formatter_0>; };
r_codec_dai: codec { sound-dai = <&record_codec>; clocks = <&misc_clk_4>; }; }; };
I _think_ this should be a reasonable setup as far as I can tell? However, testing this out just results in:
asoc-simple-card irxs-audio: parse error -22 asoc-simple-card: probe of irxs-audio failed with error -22
After adding in a bunch of debug output, it seems that the issue is that through this sequence of calls:
asoc_simple_probe simple_parse_of simple_for_each_link simple_dai_link_of asoc_simple_parse_platform (aka asoc_simple_parse_dai) snd_soc_of_get_dai_name snd_soc_get_dai_name
Inside snd_soc_get_dai_name, snd_soc_component_of_xlate_dai_name is called and returns -ENOTSUPP, so we fall into the if block and end up failing out here:
if (id < 0 || id >= pos->num_dai) { ret = -EINVAL; continue; }
That, in turn, seems to be because the Audio Formatter driver doesn't register any DAIs in its call to devm_snd_soc_register_component in xlnx_formatter_pcm_probe. I'm a bit unsure whether that is supposed to be the case, or if not, what should be done to fix it. Can anyone provide some advice?
My tests are on kernel 5.10.45, but there's been no changes in the Xilinx ASoC drivers since then, and I'm not sure anything relevant to this has changed in the rest of ASoC either.
Hi Robert
Thank you for your reporting
asoc-simple-card irxs-audio: parse error -22 asoc-simple-card: probe of irxs-audio failed with error -22
(snip)
Inside snd_soc_get_dai_name, snd_soc_component_of_xlate_dai_name is called and returns -ENOTSUPP, so we fall into the if block and end up failing out here:
if (id < 0 || id >= pos->num_dai) { ret = -EINVAL; continue; }
Platform support was added to simple-audio-card by someone (I forgot detail), and I have never use it unfortunately. But in my quick check, the purpose of asoc_simple_parse_platform() is setup dlc->of_node
asoc_simple_parse_dai(...) { ... => dlc->of_node = args.np; ... }
and it will be checked at asoc_simple_canonicalize_platform()
asoc_simple_canonicalize_platform(...) { /* Assumes platform == cpu */ => if (!dai_link->platforms->of_node) => dai_link->platforms->of_node = dai_link->cpus->of_node; ... }
and will be used at soc-core
(A) soc_dai_link_sanity_check(...) { ... for_each_link_platforms(link, i, platform) { => if (!!platform->name == !!platform->of_node) { ... }
snd_soc_add_pcm_runtime(...) { ... (A) ret = soc_dai_link_sanity_check(); ...
for_each_link_cpus(dai_link, i, cpu) { (X) asoc_rtd_to_cpu(rtd, i) = snd_soc_find_dai(cpu); ... }
for_each_link_codecs(dai_link, i, codec) { (X) asoc_rtd_to_codec(rtd, i) = snd_soc_find_dai(codec); ... }
for_each_link_platforms(dai_link, i, platform) { (Y) for_each_component(component) { => if (!snd_soc_is_matching_component(platform, component)) ... }
But, at snd_soc_add_pcm_runtime(), CPU/Codec needs of_node and DAI name (= X) Platform needs of_node (= Y)
So maybe (I didn't confirm) for platform, asoc_simple_parse_dai() don't need to call snd_soc_of_get_dai_name() ?
Thank you for your help !!
Best regards --- Kuninori Morimoto
On Wed, 2021-07-07 at 10:19 +0900, Kuninori Morimoto wrote:
Hi Robert
Thank you for your reporting
asoc-simple-card irxs-audio: parse error -22 asoc-simple-card: probe of irxs-audio failed with error -22
(snip)
Inside snd_soc_get_dai_name, snd_soc_component_of_xlate_dai_name is called and returns -ENOTSUPP, so we fall into the if block and end up failing out here:
if (id < 0 || id >= pos->num_dai) { ret = -EINVAL; continue; }Platform support was added to simple-audio-card by someone (I forgot detail), and I have never use it unfortunately. But in my quick check, the purpose of asoc_simple_parse_platform() is setup dlc->of_node
asoc_simple_parse_dai(...) { ... => dlc->of_node = args.np; ... }
and it will be checked at asoc_simple_canonicalize_platform()
asoc_simple_canonicalize_platform(...) { /* Assumes platform == cpu */ => if (!dai_link->platforms->of_node) => dai_link->platforms->of_node = dai_link->cpus->of_node; ... }
and will be used at soc-core
(A) soc_dai_link_sanity_check(...) { ... for_each_link_platforms(link, i, platform) { => if (!!platform->name == !!platform->of_node) { ... }
snd_soc_add_pcm_runtime(...) { ... (A) ret = soc_dai_link_sanity_check(); ...
for_each_link_cpus(dai_link, i, cpu) {(X) asoc_rtd_to_cpu(rtd, i) = snd_soc_find_dai(cpu); ... }
for_each_link_codecs(dai_link, i, codec) {(X) asoc_rtd_to_codec(rtd, i) = snd_soc_find_dai(codec); ... }
for_each_link_platforms(dai_link, i, platform) {(Y) for_each_component(component) { => if (!snd_soc_is_matching_component(platform, component)) ... }
But, at snd_soc_add_pcm_runtime(), CPU/Codec needs of_node and DAI name (= X) Platform needs of_node (= Y)
So maybe (I didn't confirm) for platform, asoc_simple_parse_dai() don't need to call snd_soc_of_get_dai_name() ?
I think you're probably right - I made a change to basically ignore a failure of snd_soc_of_get_dai_name in the platform case and the driver seems to probe OK. Possibly it should just skip the call entirely and not even try to populate the name for platform if it's never needed?
I have some other issues to work through to try and get a working setup, but once I get things working in my test setup I can put a patch together.
Hi Robert
I think you're probably right - I made a change to basically ignore a failure of snd_soc_of_get_dai_name in the platform case and the driver seems to probe OK. Possibly it should just skip the call entirely and not even try to populate the name for platform if it's never needed?
I have some other issues to work through to try and get a working setup, but once I get things working in my test setup I can put a patch together.
Great ! Nice to know.
Thank you for your help !!
Best regards --- Kuninori Morimoto
On Thu, 2021-07-08 at 08:44 +0900, Kuninori Morimoto wrote:
Hi Robert
I think you're probably right - I made a change to basically ignore a failure of snd_soc_of_get_dai_name in the platform case and the driver seems to probe OK. Possibly it should just skip the call entirely and not even try to populate the name for platform if it's never needed?
I have some other issues to work through to try and get a working setup, but once I get things working in my test setup I can put a patch together.
Great ! Nice to know.
Thank you for your help !!
So the next issue I'm now facing is that the MCLK to SCLK divider is not being set properly in either the Audio Formatter (MM2S Fs Multiplier register) or in the I2S Transmitter (I2S Timing Control register). The xlnx_i2s driver has a set_clkdiv function defined in its snd_soc_dai_ops structure, however that doesn't appear to be getting called. And the xlnx_formatter_pcm driver doesn't seem to have any code to set XLNX_AUD_FS_MULTIPLIER at all.
In this case I have a sample rate to MCLK divider of 256, so it looks like I should add mclk-fs = <256> into the dai-link nodes in the device tree, but there will need to be some code added to the xlnx_formatter_pcm to do something with that information? And then should that driver have code to trigger the call to set_clkdiv on the CPU DAI as well?
These drivers originated in the Xilinx kernel tree ( https://github.com/Xilinx/linux-xlnx/tree/master/sound/soc/xilinx) and in that tree they've got a top-level xlnx_pl_snd_card.c driver which is defining the MCLK divider and instantiating the other components, however that driver is not in mainline and seems like it is kind of a hack. It seems like this SCLK divider setting is the main thing that is still needed to getting the Xilinx audio cores working in mainline using simple-sound-card..
Hi Robert
So the next issue I'm now facing is that the MCLK to SCLK divider is not being set properly in either the Audio Formatter (MM2S Fs Multiplier register) or in the I2S Transmitter (I2S Timing Control register). The xlnx_i2s driver has a set_clkdiv function defined in its snd_soc_dai_ops structure, however that doesn't appear to be getting called. And the xlnx_formatter_pcm driver doesn't seem to have any code to set XLNX_AUD_FS_MULTIPLIER at all.
In this case I have a sample rate to MCLK divider of 256, so it looks like I should add mclk-fs = <256> into the dai-link nodes in the device tree, but there will need to be some code added to the xlnx_formatter_pcm to do something with that information? And then should that driver have code to trigger the call to set_clkdiv on the CPU DAI as well?
These drivers originated in the Xilinx kernel tree ( https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com...) and in that tree they've got a top-level xlnx_pl_snd_card.c driver which is defining the MCLK divider and instantiating the other components, however that driver is not in mainline and seems like it is kind of a hack. It seems like this SCLK divider setting is the main thing that is still needed to getting the Xilinx audio cores working in mainline using simple-sound-card..
Hmm... clock is one of difficult point to be generic, I guess. audio-graph / audio-graph2 has customize feature in such case, but simple-card doesn't.
- create generic clock handling way on simple-card ? - add customize feature to simple-card ? - switch to audio-graph / audio-graph2, and use customize feature ?
Thank you for your help !!
Best regards --- Kuninori Morimoto
On Fri, Jul 09, 2021 at 10:16:45AM +0900, Kuninori Morimoto wrote:
So the next issue I'm now facing is that the MCLK to SCLK divider is not being set properly in either the Audio Formatter (MM2S Fs Multiplier register) or in the I2S Transmitter (I2S Timing Control register). The xlnx_i2s driver has a set_clkdiv function defined in its snd_soc_dai_ops structure, however that doesn't appear to be getting called. And the xlnx_formatter_pcm driver doesn't seem to have any code to set XLNX_AUD_FS_MULTIPLIER at all.
In this case I have a sample rate to MCLK divider of 256, so it looks like I should add mclk-fs = <256> into the dai-link nodes in the device tree, but there will need to be some code added to the xlnx_formatter_pcm to do something with that information? And then should that driver have code to trigger the call to set_clkdiv on the CPU DAI as well?
Hmm... clock is one of difficult point to be generic, I guess. audio-graph / audio-graph2 has customize feature in such case, but simple-card doesn't.
- create generic clock handling way on simple-card ?
- add customize feature to simple-card ?
- switch to audio-graph / audio-graph2, and use customize feature ?
Thank you for your help !!
For something like this I think the driver should be able to figure out the ratio based on the configured MCLK and sample rate. For the most part set_clkdiv() should be a legacy thing, it's very manual and hard to see why a system would do something different to the obvious ratio usually.
On Fri, 2021-07-09 at 13:38 +0100, Mark Brown wrote:
On Fri, Jul 09, 2021 at 10:16:45AM +0900, Kuninori Morimoto wrote:
So the next issue I'm now facing is that the MCLK to SCLK divider is not being set properly in either the Audio Formatter (MM2S Fs Multiplier register) or in the I2S Transmitter (I2S Timing Control register). The xlnx_i2s driver has a set_clkdiv function defined in its snd_soc_dai_ops structure, however that doesn't appear to be getting called. And the xlnx_formatter_pcm driver doesn't seem to have any code to set XLNX_AUD_FS_MULTIPLIER at all. In this case I have a sample rate to MCLK divider of 256, so it looks like I should add mclk-fs = <256> into the dai-link nodes in the device tree, but there will need to be some code added to the xlnx_formatter_pcm to do something with that information? And then should that driver have code to trigger the call to set_clkdiv on the CPU DAI as well?
Hmm... clock is one of difficult point to be generic, I guess. audio-graph / audio-graph2 has customize feature in such case, but simple-card doesn't.
- create generic clock handling way on simple-card ?
- add customize feature to simple-card ?
- switch to audio-graph / audio-graph2, and use customize feature ?
Thank you for your help !!
For something like this I think the driver should be able to figure out the ratio based on the configured MCLK and sample rate. For the most part set_clkdiv() should be a legacy thing, it's very manual and hard to see why a system would do something different to the obvious ratio usually.
Possibly the I2S transmitter should be implementing set_sysclk rather than set_clkdiv then? simple-audio-card seems like it would already propagate that through into the CPU DAI in asoc_simple_hw_params and then it could figure out the right divider value to use.
The tricky part is that the Audio Formatter (used as the "plat" component here) also needs to know what the mclk-fs value is. (I really don't know why it cares, I would think it would just push data to the output stream as fast as it was accepted, but indeed it does have a register to set the sample rate to MCLK divider, and if it's not set properly the I2S transmitter downstream seems to constantly get underruns.) I'm not sure if there's any mechanism for it to determine the value right now, or if something new would need to be added?
Our actual product using this Xilinx logic is only using capture functionality - the playback side (which is where these issues come in) is just being used as a loopback to the capture side for testing purposes - but I can poke at this further if there's some spare time..
On Fri, Jul 09, 2021 at 05:05:47PM +0000, Robert Hancock wrote:
On Fri, 2021-07-09 at 13:38 +0100, Mark Brown wrote:
For something like this I think the driver should be able to figure out the ratio based on the configured MCLK and sample rate. For the most part set_clkdiv() should be a legacy thing, it's very manual and hard to see why a system would do something different to the obvious ratio usually.
Possibly the I2S transmitter should be implementing set_sysclk rather than set_clkdiv then? simple-audio-card seems like it would already propagate that through into the CPU DAI in asoc_simple_hw_params and then it could figure out the right divider value to use.
I think so.
The tricky part is that the Audio Formatter (used as the "plat" component here) also needs to know what the mclk-fs value is. (I really don't know why it cares, I would think it would just push data to the output stream as fast as it was accepted, but indeed it does have a register to set the sample rate to MCLK divider, and if it's not set properly the I2S transmitter downstream seems to constantly get underruns.) I'm not sure if there's any mechanism for it to determine the value right now, or if something new would need to be added?
Given that it knows the MCLK if set_sysclk() is used and it knows the sample rate it should just be able to calculate the ratio?
On Fri, 2021-07-09 at 19:02 +0100, Mark Brown wrote:
On Fri, Jul 09, 2021 at 05:05:47PM +0000, Robert Hancock wrote:
On Fri, 2021-07-09 at 13:38 +0100, Mark Brown wrote:
For something like this I think the driver should be able to figure out the ratio based on the configured MCLK and sample rate. For the most part set_clkdiv() should be a legacy thing, it's very manual and hard to see why a system would do something different to the obvious ratio usually.
Possibly the I2S transmitter should be implementing set_sysclk rather than set_clkdiv then? simple-audio-card seems like it would already propagate that through into the CPU DAI in asoc_simple_hw_params and then it could figure out the right divider value to use.
I think so.
The tricky part is that the Audio Formatter (used as the "plat" component here) also needs to know what the mclk-fs value is. (I really don't know why it cares, I would think it would just push data to the output stream as fast as it was accepted, but indeed it does have a register to set the sample rate to MCLK divider, and if it's not set properly the I2S transmitter downstream seems to constantly get underruns.) I'm not sure if there's any mechanism for it to determine the value right now, or if something new would need to be added?
Given that it knows the MCLK if set_sysclk() is used and it knows the sample rate it should just be able to calculate the ratio?
I see that snd_soc_component_driver has a set_sysclk callback as well, so that allows the formatter to handle setting the divider. However, right now with simple-audio-card that callback is not being invoked on the formatter, though it is on the I2S transmitter.
I'm thinking something needs to be added to asoc_simple_hw_params to call snd_soc_component_set_sysclk on the platform component(s) like it calls snd_soc_dai_set_sysclk for the codec DAI and CPU DAI.
Not sure exactly how that should be done though - we could use for_each_rtd_components to iterate through all of the components and call snd_soc_component_set_sysclk on all of them, though that would also potentially duplicate some settings already done by the snd_soc_dai_set_sysclk calls on the CPU and codec DAIs. I'm not sure if that really hurts anything though?
On Fri, Jul 09, 2021 at 08:25:17PM +0000, Robert Hancock wrote:
On Fri, 2021-07-09 at 19:02 +0100, Mark Brown wrote:
Given that it knows the MCLK if set_sysclk() is used and it knows the sample rate it should just be able to calculate the ratio?
I see that snd_soc_component_driver has a set_sysclk callback as well, so that allows the formatter to handle setting the divider. However, right now with simple-audio-card that callback is not being invoked on the formatter, though it is on the I2S transmitter.
I'm thinking something needs to be added to asoc_simple_hw_params to call snd_soc_component_set_sysclk on the platform component(s) like it calls snd_soc_dai_set_sysclk for the codec DAI and CPU DAI.
Not sure exactly how that should be done though - we could use for_each_rtd_components to iterate through all of the components and call snd_soc_component_set_sysclk on all of them, though that would also potentially duplicate some settings already done by the snd_soc_dai_set_sysclk calls on the CPU and codec DAIs. I'm not sure if that really hurts anything though?
Yeah, I don't think that's likely to hurt anything - I'd be surprised if there were that many things that actually have set_sysclk() to even notice.
participants (3)
-
Kuninori Morimoto -
Mark Brown -
Robert Hancock