Issues using simple-audio-card driver with Xilinx Audio Formatter

Issues using simple-audio-card driver with Xilinx Audio Formatter

[Robert Hancock]

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.

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: Issues using simple-audio-card driver with Xilinx Audio Formatter

[Kuninori Morimoto]

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

Re: Issues using simple-audio-card driver with Xilinx Audio Formatter

[Robert Hancock]

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.

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: Issues using simple-audio-card driver with Xilinx Audio Formatter

[Kuninori Morimoto]

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

Re: Issues using simple-audio-card driver with Xilinx Audio Formatter

[Robert Hancock]

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..

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: Issues using simple-audio-card driver with Xilinx Audio Formatter

[Kuninori Morimoto]

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%2FXilinx%2Flinux-xlnx%2Ftree%2Fmaster%2Fsound%2Fsoc%2Fxilinx&amp;data=04%7C01%7Ckuninori.morimoto.gx%40renesas.com%7C48c9c4633dae479df6a408d9424d9abd%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C637613723347710960%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=nMnJI78dvM12Yj8mqB8TzJfxtm3V1tUeJpVQ1FR1bhE%3D&amp;reserved=0) 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

Re: Issues using simple-audio-card driver with Xilinx Audio Formatter

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Issues using simple-audio-card driver with Xilinx Audio Formatter

[Robert Hancock]

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..

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: Issues using simple-audio-card driver with Xilinx Audio Formatter

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Issues using simple-audio-card driver with Xilinx Audio Formatter

[Robert Hancock]

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?

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

Re: Issues using simple-audio-card driver with Xilinx Audio Formatter

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]