[alsa-devel] [RFC PATCH 1/2] ASoC: simple-card: add support for bclk_ratio
In some situations, codec configuration will depend on the bclk_ratio generated by the cpu dai. The tda998x hdmi transmitter is an example of this.
Allow simple-card to set the bclk_ratio via the 'bclk-slot-ratio' devicetree property, which describes the bclk to slot rate ratio.
This value is converted to the bclk to sample ratio before being passed into set_bclk_ratio().
Signed-off-by: Sven Van Asbroeck TheSven73@gmail.com --- sound/soc/generic/simple-card.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 3fe34417ec89..61e9ba4e9b58 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -25,6 +25,7 @@ struct simple_card_data { struct asoc_simple_card_data adata; struct snd_soc_codec_conf *codec_conf; unsigned int mclk_fs; + unsigned int bclk_slot_ratio; } *dai_props; struct asoc_simple_jack hp_jack; struct asoc_simple_jack mic_jack; @@ -97,7 +98,7 @@ static int asoc_simple_card_hw_params(struct snd_pcm_substream *substream, struct simple_card_data *priv = snd_soc_card_get_drvdata(rtd->card); struct simple_dai_props *dai_props = simple_priv_to_props(priv, rtd->num); - unsigned int mclk, mclk_fs = 0; + unsigned int mclk, bclk_ratio, mclk_fs = 0, bclk_slot_ratio = 0; int ret = 0;
if (dai_props->mclk_fs) @@ -124,6 +125,23 @@ static int asoc_simple_card_hw_params(struct snd_pcm_substream *substream, if (ret && ret != -ENOTSUPP) goto err; } + + if (dai_props->bclk_slot_ratio) + bclk_slot_ratio = dai_props->bclk_slot_ratio; + + if (bclk_slot_ratio) { + /* FIXME do we need to care about TDM slots here ? */ + bclk_ratio = snd_soc_calc_frame_size(bclk_slot_ratio, + params_channels(params), 1); + + ret = snd_soc_dai_set_bclk_ratio(codec_dai, bclk_ratio); + if (ret && ret != -ENOTSUPP) + goto err; + + ret = snd_soc_dai_set_bclk_ratio(cpu_dai, bclk_ratio); + if (ret && ret != -ENOTSUPP) + goto err; + } return 0; err: return ret; @@ -277,6 +295,12 @@ static int asoc_simple_card_dai_link_of_dpcm(struct device_node *top, of_property_read_u32(node, prop, &dai_props->mclk_fs); of_property_read_u32(np, prop, &dai_props->mclk_fs);
+ snprintf(prop, sizeof(prop), "%sbclk-slot-ratio", prefix); + of_property_read_u32(top, PREFIX "bclk-slot-ratio", + &dai_props->bclk_slot_ratio); + of_property_read_u32(node, prop, &dai_props->bclk_slot_ratio); + of_property_read_u32(np, prop, &dai_props->bclk_slot_ratio); + ret = asoc_simple_card_parse_daifmt(dev, node, codec, prefix, &dai_link->dai_fmt); if (ret < 0) @@ -349,6 +373,13 @@ static int asoc_simple_card_dai_link_of(struct device_node *top, of_property_read_u32(cpu, prop, &dai_props->mclk_fs); of_property_read_u32(codec, prop, &dai_props->mclk_fs);
+ snprintf(prop, sizeof(prop), "%sbclk-slot-ratio", prefix); + of_property_read_u32(top, PREFIX "bclk-slot-ratio", + &dai_props->bclk_slot_ratio); + of_property_read_u32(node, prop, &dai_props->bclk_slot_ratio); + of_property_read_u32(cpu, prop, &dai_props->bclk_slot_ratio); + of_property_read_u32(codec, prop, &dai_props->bclk_slot_ratio); + ret = asoc_simple_card_parse_cpu(cpu, dai_link, DAI, CELL, &single_cpu); if (ret < 0)
snd_soc_dai_set_bclk_ratio() should behave like other snd_soc_dai_XXX functions, and return -ENOTSUPP if the callback in driver->ops is NULL.
Signed-off-by: Sven Van Asbroeck TheSven73@gmail.com --- sound/soc/soc-core.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 50617db05c46..5611caf25ea3 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -2550,10 +2550,11 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_pll); */ int snd_soc_dai_set_bclk_ratio(struct snd_soc_dai *dai, unsigned int ratio) { - if (dai->driver->ops->set_bclk_ratio) - return dai->driver->ops->set_bclk_ratio(dai, ratio); - else + if (dai->driver == NULL) return -EINVAL; + if (dai->driver->ops->set_bclk_ratio == NULL) + return -ENOTSUPP; + return dai->driver->ops->set_bclk_ratio(dai, ratio); } EXPORT_SYMBOL_GPL(snd_soc_dai_set_bclk_ratio);
Hi Sven
Thank you for your patch
In some situations, codec configuration will depend on the bclk_ratio generated by the cpu dai. The tda998x hdmi transmitter is an example of this.
Allow simple-card to set the bclk_ratio via the 'bclk-slot-ratio' devicetree property, which describes the bclk to slot rate ratio.
This value is converted to the bclk to sample ratio before being passed into set_bclk_ratio().
Signed-off-by: Sven Van Asbroeck TheSven73@gmail.com
Please update Documentation, too
@@ -97,7 +98,7 @@ static int asoc_simple_card_hw_params(struct snd_pcm_substream *substream, struct simple_card_data *priv = snd_soc_card_get_drvdata(rtd->card); struct simple_dai_props *dai_props = simple_priv_to_props(priv, rtd->num);
- unsigned int mclk, mclk_fs = 0;
unsigned int mclk, bclk_ratio, mclk_fs = 0, bclk_slot_ratio = 0; int ret = 0;
if (dai_props->mclk_fs)
(snip)
- if (bclk_slot_ratio) {
/* FIXME do we need to care about TDM slots here ? */bclk_ratio = snd_soc_calc_frame_size(bclk_slot_ratio,params_channels(params), 1);ret = snd_soc_dai_set_bclk_ratio(codec_dai, bclk_ratio);if (ret && ret != -ENOTSUPP)goto err;ret = snd_soc_dai_set_bclk_ratio(cpu_dai, bclk_ratio);if (ret && ret != -ENOTSUPP)goto err;- }
Not a big deal, but "bclk_ratio" is used only here. We can define it here
On Tue, Feb 26, 2019 at 09:35:47AM +0900, Kuninori Morimoto wrote:
@@ -97,7 +98,7 @@ static int asoc_simple_card_hw_params(struct snd_pcm_substream *substream, struct simple_card_data *priv = snd_soc_card_get_drvdata(rtd->card); struct simple_dai_props *dai_props = simple_priv_to_props(priv, rtd->num);
- unsigned int mclk, mclk_fs = 0;
unsigned int mclk, bclk_ratio, mclk_fs = 0, bclk_slot_ratio = 0; int ret = 0;
if (dai_props->mclk_fs)
(snip)
- if (bclk_slot_ratio) {
/* FIXME do we need to care about TDM slots here ? */bclk_ratio = snd_soc_calc_frame_size(bclk_slot_ratio,params_channels(params), 1);ret = snd_soc_dai_set_bclk_ratio(codec_dai, bclk_ratio);if (ret && ret != -ENOTSUPP)goto err;ret = snd_soc_dai_set_bclk_ratio(cpu_dai, bclk_ratio);if (ret && ret != -ENOTSUPP)goto err;- }
Not a big deal, but "bclk_ratio" is used only here. We can define it here
This code actually requires a lot more thought - while it may solve Sven's issue, it isn't generic. So far, I have this table put together from various sources of information:
bclk_ratio sample width current mcasp fslssi kirkwood 16 32 32 64 64 24 48 48 64 64 32 64 64 64 64
Let's also consider whether it should depend on the number of channels. I2S uses a WS/LRCLK signal to differentiate each channel and to demark where the MSB bit is.
If userspace negotiates one channel, what happens - if WS becomes static, then there is no signal indicating where the MSB bit is in the stream, so there is no way for a receiver to synchronise. So, it is highly unlikely that bclk_ratio = channels * sample_width.
If the signal continues toggling, what does it do for the inactive channel - is that just one BCLK period long or does it assume there is still the second channel? If the former, it means we could end up with bclk_ratios of 17, 25, 33 which imho is unlikely, and would mess up TDA998x's CTS measurement.
What about setups where we have multiple I2S data signals (to support multi-channel audio) - if we have four channels transmitted on two data lines (as would be required by the TDA998x), that doesn't mean BCLK becomes sample_width * 4.
So, I don't think the number of channels comes into the bclk_ratio calculation at all. Given the above code, it effectively means we'd always specify channels = 2 to snd_soc_calc_frame_size().
Given that it is normal to talk about I2S being clocked at "64fs", "32fs" etc, wouldn't it just be much neater to specify _this_ value in DT, rather than half that value?
On Tue, Feb 26, 2019 at 4:11 AM Russell King - ARM Linux admin linux@armlinux.org.uk wrote:
This code actually requires a lot more thought - while it may solve Sven's issue, it isn't generic.
Wholeheartedly agree. My patches are marked RFC in the hope that they will be picked apart by folks much more in touch with the "big audio picture" than I am. It's great to learn from this process.
So far, I have this table put together from various sources of information:
bclk_ratiosample width current mcasp fslssi kirkwood 16 32 32 64 64 24 48 48 64 64 32 64 64 64 64
Let's also consider whether it should depend on the number of channels. I2S uses a WS/LRCLK signal to differentiate each channel and to demark where the MSB bit is.
If userspace negotiates one channel, what happens - if WS becomes static, then there is no signal indicating where the MSB bit is in the stream, so there is no way for a receiver to synchronise. So, it is highly unlikely that bclk_ratio = channels * sample_width.
If the signal continues toggling, what does it do for the inactive channel - is that just one BCLK period long or does it assume there is still the second channel? If the former, it means we could end up with bclk_ratios of 17, 25, 33 which imho is unlikely, and would mess up TDA998x's CTS measurement.
Good point. i2s with a single channel makes no sense.
What about setups where we have multiple I2S data signals (to support multi-channel audio) - if we have four channels transmitted on two data lines (as would be required by the TDA998x), that doesn't mean BCLK becomes sample_width * 4.
So, I don't think the number of channels comes into the bclk_ratio calculation at all. Given the above code, it effectively means we'd always specify channels = 2 to snd_soc_calc_frame_size().
I notice that hdmi-codec.c supports up to 8 channels in hdmi multi-channel playback mode. If we had a _theoretical_ hdmi xmitter with 8chan support, would the bclk_ratio not be 8 x slot_size - or 8 x 32 if using an fsl_ssi in master mode?
This will of course never happen with the tda998x, because .max_i2s_channels = 2, but we are thinking about a generic solution here.
Given that it is normal to talk about I2S being clocked at "64fs", "32fs" etc, wouldn't it just be much neater to specify _this_ value in DT, rather than half that value?
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
On Tue, Feb 26, 2019 at 09:53:22AM -0500, Sven Van Asbroeck wrote:
On Tue, Feb 26, 2019 at 4:11 AM Russell King - ARM Linux admin
If the signal continues toggling, what does it do for the inactive channel - is that just one BCLK period long or does it assume there is still the second channel? If the former, it means we could end up with bclk_ratios of 17, 25, 33 which imho is unlikely, and would mess up TDA998x's CTS measurement.
Good point. i2s with a single channel makes no sense.
As with every bad idea you can think of that never stopped anyone making such hardware. Mono is usually implemented by either leaving the right channel idle or playing the same signal on both channels.
On Tue, Feb 26, 2019 at 09:53:22AM -0500, Sven Van Asbroeck wrote:
I notice that hdmi-codec.c supports up to 8 channels in hdmi multi-channel playback mode. If we had a _theoretical_ hdmi xmitter with 8chan support, would the bclk_ratio not be 8 x slot_size - or 8 x 32 if using an fsl_ssi in master mode?
This will of course never happen with the tda998x, because .max_i2s_channels = 2, but we are thinking about a generic solution here.
The way TDA998x supports multichannel audio with I2S is as follows:
"The TDA9983B supports the NXP I2S-bus format. There are four I2S-bus stereo input channels (AP1 to AP4), which enable 8 uncompressed audio channels to be carried."
There is only one WS input and one SCK (bclk) input, which are common to each of the I2S buses.
The TDA19988 reduces this down to two I2S buses, which means it supports only up to 4 uncompressed channels. hdmi-codec doesn't take account of these restrictions, and just assumes the maximal number of channels are always available.
So, given this parallel bus architecture, it means that whether we have 2, 4, 6, or 8 channels is irrelevant to the number of bitclocks per sample - the number of bitclocks would be the same.
I can't see how you'd extend a single I2S setup to support multi- channel audio without either adding more I2S data lines or adding additional WS signals (so making it e.g., a binary number).
Adding more WS signals makes the bus deviate from the I2S standard, thereby making it impossible to connect a set of standard DACs to such a source, whereas adding more I2S data lines, you just connect each DAC to each I2S data line and common up the bit clock and WS signals across all.
In other words, the TDA998x approach is really the only sane way forward.
Now, as far as transmitter support, I believe TI Davinci SoCs use this - my Onkyo TX-NR609 AV receiver uses a DA830 SoC as a DSP to do the surround decode, which feeds multi-channel audio out to a set of DACs over a parallel I2S bus. The "mcasp" audio driver has multiple serialisers to cope with this - see Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt.
On Tue, Feb 26, 2019 at 03:45:19PM +0000, Russell King - ARM Linux admin wrote:
Adding more WS signals makes the bus deviate from the I2S standard, thereby making it impossible to connect a set of standard DACs to such a source, whereas adding more I2S data lines, you just connect each DAC to each I2S data line and common up the bit clock and WS signals across all.
In other words, the TDA998x approach is really the only sane way forward.
Right. You do also see some things doing this by stuffing all the left samples together in the left clock cycle and all the right samples together in the right clock cycle but that's usually more programmable hardware that also supports DSP modes and it's just falling out of the implementation with little effort rather than someone sitting down and thinking this is a good idea AFAICT.
Now, as far as transmitter support, I believe TI Davinci SoCs use this - my Onkyo TX-NR609 AV receiver uses a DA830 SoC as a DSP to do the surround decode, which feeds multi-channel audio out to a set of DACs over a parallel I2S bus. The "mcasp" audio driver has multiple serialisers to cope with this - see Documentation/devicetree/bindings/sound/davinci-mcasp-audio.txt.
Samsung LSI have multi-channel I2S with multiple data lines in their v5 and later controllers as well. I can't think of any other examples upstream off the top of my head.
On Tue, Feb 26, 2019 at 10:45 AM Russell King - ARM Linux admin linux@armlinux.org.uk wrote:
I can't see how you'd extend a single I2S setup to support multi- channel audio without either adding more I2S data lines or adding additional WS signals (so making it e.g., a binary number).
That's a very good point too. In light of this, I struggle to understand how the ssl_ssi can specify this:
static struct snd_soc_dai_driver fsl_ssi_dai_template = { .playback = { .stream_name = "CPU-Playback", .channels_min = 1, .channels_max = 32, },
There is talk in the manual about "network mode", which could work by changing the LRCLK only at the first slot - thereby allowing clients to receive all slots just by counting, as long as they know the slot size?
LRCLK _____/-----------------_____/--------- DATA SLOT1|SLOT2|SLOT3|SLOT4|SLOT1
Well idk, I'm way out of my depth here.
On Tue, Feb 26, 2019 at 11:31:15AM -0500, Sven Van Asbroeck wrote:
That's a very good point too. In light of this, I struggle to understand how the ssl_ssi can specify this:
static struct snd_soc_dai_driver fsl_ssi_dai_template = { .playback = { .stream_name = "CPU-Playback", .channels_min = 1, .channels_max = 32, },
There is talk in the manual about "network mode", which could work by changing the LRCLK only at the first slot - thereby allowing clients to receive all slots just by counting, as long as they know the slot size?
That's basically what the FSL controllers want to support - they're generic programmable serial ports, their most native formats are the DSP modes which only pay attention to one edge of LRCLK and then just have however many samples one after another. Several other devices like the PXA SSP ports are similar. They can generate I2S style LRCLKs but many implementations struggle to interpret incoming I2S properly, they often have issues with triggering on both rising and falling edges so get confused if there's any unused clock cycles.
For a DSP mode it's more:
LRCLK /______________________/____ DATA |SLOT1|SLOT2|SLOT3|SLOT4|SLOT1
I2S style it'd end up as more like:
LRCLK /-------------_____________/------ DATA |SLOTL1|SLOTL2|SLOTR1|SLOTR2|SLOTL1
This sort of stuff is why ASoC has a preference for exact clocking - it improves interoperability with hardware that's generic serial ports as it means that you only need to pay attention to one edge of the LRCLK even if it's doing something more than just a simple pulse.
On Tue, Feb 26, 2019 at 11:31:15AM -0500, Sven Van Asbroeck wrote:
On Tue, Feb 26, 2019 at 10:45 AM Russell King - ARM Linux admin linux@armlinux.org.uk wrote:
I can't see how you'd extend a single I2S setup to support multi- channel audio without either adding more I2S data lines or adding additional WS signals (so making it e.g., a binary number).
That's a very good point too. In light of this, I struggle to understand how the ssl_ssi can specify this:
static struct snd_soc_dai_driver fsl_ssi_dai_template = { .playback = { .stream_name = "CPU-Playback", .channels_min = 1, .channels_max = 32, },
There is talk in the manual about "network mode", which could work by changing the LRCLK only at the first slot - thereby allowing clients to receive all slots just by counting, as long as they know the slot size?
LRCLK _____/-----------------_____/--------- DATA SLOT1|SLOT2|SLOT3|SLOT4|SLOT1
From what I gather, these are described using SND_SOC_DAIFMT_DSP_A
and SND_SOC_DAIFMT_DSP_B dai formats, and the parameters are controlled not through snd_soc_dai_set_bclk_ratio() but via snd_soc_dai_set_tdm_slot().
So, IMHO, the TDM formats should be disregarded from consideration here. Mark, ack?
On Tue, Feb 26, 2019 at 05:03:49PM +0000, Russell King - ARM Linux admin wrote:
On Tue, Feb 26, 2019 at 11:31:15AM -0500, Sven Van Asbroeck wrote:
There is talk in the manual about "network mode", which could work by changing the LRCLK only at the first slot - thereby allowing clients to receive all slots just by counting, as long as they know the slot size?
LRCLK _____/-----------------_____/--------- DATA SLOT1|SLOT2|SLOT3|SLOT4|SLOT1
From what I gather, these are described using SND_SOC_DAIFMT_DSP_A and SND_SOC_DAIFMT_DSP_B dai formats, and the parameters are
Roughly, yes - the DSP formats are formats which only care about one edge of the LRCLK, the difference being if the first data bit is the same clock cycle as the transition or the next clock cycle.
controlled not through snd_soc_dai_set_bclk_ratio() but via snd_soc_dai_set_tdm_slot().
So, IMHO, the TDM formats should be disregarded from consideration here. Mark, ack?
I'd expect set_bclk_ratio() to cover all formats - usually the hardware either has zero flexibility when it comes to the LRCLK format or isn't super bothered, this stuff is mainly used to adapt the more flexible hardware to the less flexible hardware.
TDM is definitely something that only really makes sense with DSP formats though, I2S TDM is just weird.
On Tue, Feb 26, 2019 at 4:11 AM Russell King - ARM Linux admin linux@armlinux.org.uk wrote:
Given that it is normal to talk about I2S being clocked at "64fs", "32fs" etc, wouldn't it just be much neater to specify _this_ value in DT, rather than half that value?
Would we be able to reach consensus on Russell's suggestion ?
participants (4)
-
Kuninori Morimoto -
Mark Brown -
Russell King - ARM Linux admin -
Sven Van Asbroeck