My cpu dai driving the tda998x in master mode outputs SNDRV_PCM_FORMAT_S24_LE, i2s left justified, two channels:
[SNDRV_PCM_FORMAT_S24_LE] = { .width = 24, .phys = 32, .le = 1, .signd = 1, .silence = {}, },
This format has a sample width of 24 bits, but a physical size of 32 bits per channel. The redundant bits are padded with zeros. This gives a total frame width of 64 bits. According to the tda19988 datasheet, this format is supported:
"Audio samples with a precision better than 24-bit are truncated to 24-bit. [...] If the input clock has a frequency of 64fs and is left justified or Philips, the audio word is truncated to 24-bit format and other bits padded with zeros." (tda19988 product datasheet Rev. 3 - 21 July 2011)
However, supplying this format to the chip results in distorted audio. I noticed that the audio problem disappears when the CTS_N pre-divider is calculated from the physical width, _not_ the sample width.
This patch adjusts the CTS_N calculation so that the audio distortion goes away.
Caveats: - I am only capable of generating audio with a frame width of 64fs. So I cannot test if 32fs or 48fs audio formats still work. Such as S16_LE or S20_LE. - I have no access to a datasheet which accurately describes the CTS_N register. So I cannot check if my assumption is correct.
Tested with an imx6 ssi.
Cc: Peter Rosin peda@axentia.se Signed-off-by: Sven Van Asbroeck TheSven73@gmail.com --- drivers/gpu/drm/i2c/tda998x_drv.c | 4 ++-- include/drm/i2c/tda998x.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index a7c39f39793f..ba9f55176e1b 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -893,7 +893,7 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S); clksel_aip = AIP_CLKSEL_AIP_I2S; clksel_fs = AIP_CLKSEL_FS_ACLK; - switch (params->sample_width) { + switch (params->physical_width) { case 16: cts_n = CTS_N_M(3) | CTS_N_K(1); break; @@ -982,7 +982,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, struct tda998x_priv *priv = dev_get_drvdata(dev); int i, ret; struct tda998x_audio_params audio = { - .sample_width = params->sample_width, + .physical_width = params->physical_width, .sample_rate = params->sample_rate, .cea = params->cea, }; diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index 3cb25ccbe5e6..7e9fddcc4ddd 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -14,7 +14,7 @@ enum { struct tda998x_audio_params { u8 config; u8 format; - unsigned sample_width; + unsigned int physical_width; unsigned sample_rate; struct hdmi_audio_infoframe cea; u8 status[5];
Infrastructure patch which allows the tda998x patch to build.
This patch will be submitted for review if/when the tda998x patch gets accepted.
Signed-off-by: Sven Van Asbroeck TheSven73@gmail.com --- include/sound/hdmi-codec.h | 1 + sound/soc/codecs/hdmi-codec.c | 1 + 2 files changed, 2 insertions(+)
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 9483c55f871b..5ba3db37e349 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -52,6 +52,7 @@ struct hdmi_codec_params { struct snd_aes_iec958 iec; int sample_rate; int sample_width; + int physical_width; int channels; };
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index e5b6769b9797..007f20f2c5ac 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -518,6 +518,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
hp.sample_width = params_width(params); hp.sample_rate = params_rate(params); + hp.physical_width = params_physical_width(params); hp.channels = params_channels(params);
return hcp->hcd.ops->hw_params(dai->dev->parent, hcp->hcd.data,
On Thu, Feb 21, 2019 at 01:18:13PM -0500, Sven Van Asbroeck wrote:
My cpu dai driving the tda998x in master mode outputs SNDRV_PCM_FORMAT_S24_LE, i2s left justified, two channels:
[SNDRV_PCM_FORMAT_S24_LE] = { .width = 24, .phys = 32, .le = 1, .signd = 1, .silence = {}, },
This format has a sample width of 24 bits, but a physical size of 32 bits per channel. The redundant bits are padded with zeros. This gives a total frame width of 64 bits.
The above table describes the memory format, not the wire format. Look further down for SNDRV_PCM_FORMAT_S24_3LE, which is 24-bit packed into three bytes (see include/uapi/sound/asound.h for the comment specifying that.)
ASoC uses DAIFMT to specify the on-wire format in connection with the above.
However, having 32-clocks per sample is quite normal with I2S.
According to the tda19988 datasheet, this format is supported:
"Audio samples with a precision better than 24-bit are truncated to 24-bit. [...] If the input clock has a frequency of 64fs and is left justified or Philips, the audio word is truncated to 24-bit format and other bits padded with zeros." (tda19988 product datasheet Rev. 3 - 21 July 2011)
However, supplying this format to the chip results in distorted audio. I noticed that the audio problem disappears when the CTS_N pre-divider is calculated from the physical width, _not_ the sample width.
It is not correct to use the physical width - as can be seen from the table, if you check the 3-byte memory format, you'll see that it has a physical width of 24. That doesn't mean that the bus has 24 clocks per sample.
Looking at kirkwood-i2s, which is one of the setups that the I2S mode was apparently originally tested (not by me), it seems to do the same thing as the FSL SSI - 32 clocks per sample with BCLK being 64Fs - there is a comment "I2S always uses 32 bits per channel" which implies 64Fs.
When I2S mode patches appeared, I did wonder about that code which depends on the sample width rather than the Fs value, but I assumed that it had been tested with all different formats, etc. Maybe that was a false assumption to make.
As it is possible to have 16-bit samples at 64Fs or 32Fs, and if the TDA998x is counting BCLKs, knowing the Fs value is necessary to know how to program the TDA998x to generate the CTS value.
Now, ASoC has a bunch of functions that allows the wire format to be controlled. E.g., snd_soc_dai_set_fmt() sets which end provides the master clock, the polarity of the clocks, whether the samples are left or right justified. There is also snd_soc_dai_set_bclk_ratio(), which is documented as:
/** * snd_soc_dai_set_bclk_ratio - configure BCLK to sample rate ratio. * @dai: DAI * @ratio: Ratio of BCLK to Sample rate. * * Configures the DAI for a preset BCLK to sample rate ratio. */
which would have ratio=64 for 64Fs. The problem is, though, that no one appears to call this function, and fewer implement the method (hdmi-codec being one of those which does not.)
This patch adjusts the CTS_N calculation so that the audio distortion goes away.
Caveats:
- I am only capable of generating audio with a frame width of 64fs. So I cannot test if 32fs or 48fs audio formats still work. Such as S16_LE or S20_LE.
- I have no access to a datasheet which accurately describes the CTS_N register. So I cannot check if my assumption is correct.
Don't think that any of the information that is available is much better! There's a few register descriptions but nothing that really describes how all the various register settings hang together.
For example, the CTS_N register M and K values are:
M select: postdivider mts (measured time stamp) 0 CTS = mts 1 CTS = mts / 2 2 CTS = mts / 4 3 CTS = mts / 8
K select: predivider (scales n) 0 k=1 1 k=2 2 k=3 3 k=4 4+ k=8
Then there's the SEL_FS field in the AIP_CLKSEL register:
select fs: CTS reference
which is surely the mts reference, if CTS is derived from mts.
This doesn't really help in terms of working out what the correct settings should be, and other information I have laying around does not provide any further enlightenment.
I think what I'd like to see is passing of the Fs value into the driver from hdmi-codec, but I suspect that requires a bit of work in multiple drivers.
Tested with an imx6 ssi.
Cc: Peter Rosin peda@axentia.se Signed-off-by: Sven Van Asbroeck TheSven73@gmail.com
drivers/gpu/drm/i2c/tda998x_drv.c | 4 ++-- include/drm/i2c/tda998x.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index a7c39f39793f..ba9f55176e1b 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -893,7 +893,7 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S); clksel_aip = AIP_CLKSEL_AIP_I2S; clksel_fs = AIP_CLKSEL_FS_ACLK;
switch (params->sample_width) {
switch (params->physical_width) {@@ -982,7 +982,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, struct tda998x_priv *priv = dev_get_drvdata(dev); int i, ret; struct tda998x_audio_params audio = {
.sample_width = params->sample_width,
.physical_width = params->physical_width,diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index 3cb25ccbe5e6..7e9fddcc4ddd 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -14,7 +14,7 @@ enum { struct tda998x_audio_params { u8 config; u8 format;
- unsigned sample_width;
- unsigned int physical_width; unsigned sample_rate; struct hdmi_audio_infoframe cea; u8 status[5];
-- 2.17.1
On Fri, Feb 22, 2019 at 8:21 AM Russell King - ARM Linux admin linux@armlinux.org.uk wrote:
On Thu, Feb 21, 2019 at 01:18:13PM -0500, Sven Van Asbroeck wrote:
[SNDRV_PCM_FORMAT_S24_LE] = { .width = 24, .phys = 32, .le = 1, .signd = 1, .silence = {}, },The above table describes the memory format, not the wire format. Look further down for SNDRV_PCM_FORMAT_S24_3LE, which is 24-bit packed into three bytes (see include/uapi/sound/asound.h for the comment specifying that.)
ASoC uses DAIFMT to specify the on-wire format in connection with the above.
Interesting ! So you're saying that currently, nobody strictly defines the layout of the on-wire format, correct? I'm not sure how this works in practice, because codec and cpu dai should agree on the on-wire format? Except if the formats used have enough flexibility so you don't have to care.
If so, we don't seem to have this luxury here :(
This doesn't really help in terms of working out what the correct settings should be, and other information I have laying around does not provide any further enlightenment.
I have access to the NXP software library shipped with the tda19988. The library's release notes have the following entry:
. "I2S audio does not work, CTS value is not good" Check the audio I2S format <snip> CTS is automatically computed by the TDA accordingly to the audio input so accordingly to the upstream settings (like an OMAP ;) For example, I2S 16 bits or 32 bits do not produce the same CTS value
The config structure which you need to fill in to init the audio has a "i2s qualifier" field, where you have the choice between 16 and 32 bits. This then maps to a "Clock Time Stamp factor x" called CTSX, which maps to the following CTS_N register settings:
CTSX -> CTS_N (m,k) ----------------------------------- 16 -> (3,0) 32 -> (3,1) (i2s qualifier = 16 bits) 48 -> (3,2) 64 -> (3,3) (i2s qualifier = 32 bits) 128 -> (0,0)
Does this information bring us any closer to our assumption that CTS_N needs to be calculated off the bclk to sample rate ratio ?
I think what I'd like to see is passing of the Fs value into the driver from hdmi-codec, but I suspect that requires a bit of work in multiple drivers.
I'd love to take a shot at this, but first I'd like to understand what you're suggesting :)
Currently there is set_bclk_ratio() support, but no-one is actually using it. If hdmi-codec is to retrieve the ratio, wouldn't we need to add .GET_blk_ratio to snd_soc_dai_ops ?
I could add this to fsl_ssi in master mode, but what if somebody connects the tda to a cpu dai for which no-one implemented .GET_bclk_ratio ? Do we guess? Or just error out?
Also, what would a proposed snd_soc_dai_GET_bclk_ratio() return e.g. on fsl_ssi in slave mode, where the value arguably doesn't exist because the ssi will accept pretty much anything you throw at it?
(Adding Mark, ASoC maintainer.)
On Fri, Feb 22, 2019 at 10:47:35AM -0500, Sven Van Asbroeck wrote:
On Fri, Feb 22, 2019 at 8:21 AM Russell King - ARM Linux admin linux@armlinux.org.uk wrote:
On Thu, Feb 21, 2019 at 01:18:13PM -0500, Sven Van Asbroeck wrote:
[SNDRV_PCM_FORMAT_S24_LE] = { .width = 24, .phys = 32, .le = 1, .signd = 1, .silence = {}, },The above table describes the memory format, not the wire format. Look further down for SNDRV_PCM_FORMAT_S24_3LE, which is 24-bit packed into three bytes (see include/uapi/sound/asound.h for the comment specifying that.)
ASoC uses DAIFMT to specify the on-wire format in connection with the above.
Interesting ! So you're saying that currently, nobody strictly defines the layout of the on-wire format, correct? I'm not sure how this works in practice, because codec and cpu dai should agree on the on-wire format? Except if the formats used have enough flexibility so you don't have to care.
SNDRV_PCM_FORMAT_xxx more defines the in-memory format, rather than the on-wire format.
As I've said, the on-wire format is defined in ASoC using a completely different mechanism, using the definitions in include/sound/soc-dai.h. This describes parameters such as the polarity of clocks on the i2s bus, the justification of the data, etc.
Bear in mind that SNDRV_PCM_FORMAT_S24_LE in memory may be right justified (using the least significant 3 bytes of every 32-bit word), but on the wire may be left justified - using the most significant bits.
SND_SOC_DAIFMT_I2S defines the format to be "Philips" format, where the MSB bit is sent one BCLK _after_ the LRCLK signal changes state. There is also SND_SOC_DAIFMT_LEFT_J, where the MSB bit is sent with no delay, and extra padding zeros are sent in the "LSB" bits. SND_SOC_DAIFMT_RIGHT_J is similar, but the padding is in the "MSB" bits.
Then there's the polarity of the BCLK and LRCLK (frame) signals. Finally, there's whether the codec (TDA998x in this case) is the origin of the LRCLK and/or BCLK.
This information is passed via a call to snd_soc_dai_set_fmt() which takes the DAI and the format - this calls into hdmi-codec.c hdmi_codec_set_fmt(). This will be handled by the core ASoC code if the DAI has a .dai_fmt member set (which can be set by DT - see snd_soc_of_parse_daifmt().)
Then there is snd_soc_dai_set_bclk_ratio() which sets the BCLK to sample-rate ratio, as I explained earlier. hdmi-codec doesn't have an implementation for this, and afaics no one calls this function. So, it seems assumptions are made throughout ASoC on that point (probably because most codecs don't care.)
This doesn't really help in terms of working out what the correct settings should be, and other information I have laying around does not provide any further enlightenment.
I have access to the NXP software library shipped with the tda19988.
Yes, I'm aware of it.
The library's release notes have the following entry:
. "I2S audio does not work, CTS value is not good" Check the audio I2S format <snip> CTS is automatically computed by the TDA accordingly to the audio input so accordingly to the upstream settings (like an OMAP ;) For example, I2S 16 bits or 32 bits do not produce the same CTS value
The config structure which you need to fill in to init the audio has a "i2s qualifier" field, where you have the choice between 16 and 32 bits. This then maps to a "Clock Time Stamp factor x" called CTSX, which maps to the following CTS_N register settings:
CTSX -> CTS_N (m,k)
16 -> (3,0) 32 -> (3,1) (i2s qualifier = 16 bits) 48 -> (3,2) 64 -> (3,3) (i2s qualifier = 32 bits) 128 -> (0,0)
Does this information bring us any closer to our assumption that CTS_N needs to be calculated off the bclk to sample rate ratio ?
I'm aware of other users of TDA998x, and I'm attempting to get out of them what ratios their implementations use - they've said that they have confirmed that 16bit and 24bit works for them, but that's rather incomplete in terms of what I wanted to know... waiting for another response!
I'd love to take a shot at this, but first I'd like to understand what you're suggesting :)
Currently there is set_bclk_ratio() support, but no-one is actually using it. If hdmi-codec is to retrieve the ratio, wouldn't we need to add .GET_blk_ratio to snd_soc_dai_ops ?
I could add this to fsl_ssi in master mode, but what if somebody connects the tda to a cpu dai for which no-one implemented .GET_bclk_ratio ? Do we guess? Or just error out?
Also, what would a proposed snd_soc_dai_GET_bclk_ratio() return e.g. on fsl_ssi in slave mode, where the value arguably doesn't exist because the ssi will accept pretty much anything you throw at it?
You appear to be thinking that the codec should ask something else for the bclk ratio - however ASoC is designed from the point of view of the codecs being told the appropriate operating parameters by the "card" or core at the appropriate time(s). Hence, something needs to call snd_soc_dai_set_bclk_ratio().
hdmi-codec then needs to supply an implementation for the set_bclk_ratio() method in struct snd_soc_dai_ops, just like it already does for the set_fmt method, and pass the bclk ratio to the actual HDMI codec.
Something like the below would probably be moving in the right direction:
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 9483c55f871b..0fca69880dc3 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -42,6 +42,7 @@ struct hdmi_codec_daifmt { unsigned int frame_clk_inv:1; unsigned int bit_clk_master:1; unsigned int frame_clk_master:1; + unsigned int bclk_ratio; };
/* diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index d00734d31e04..f0f08b7a073f 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -524,6 +524,17 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, &hcp->daifmt[dai->id], &hp); }
+static int hdmi_codec_set_bclk_ratio(struct snd_soc_dai *dai, + unsigned int ratio) +{ + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); + + /* FIXME: some validation here would be good? */ + hcp->daifmt[dai->id].bclk_ratio = ratio; + + return 0; +} + static int hdmi_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) { @@ -593,7 +604,11 @@ static int hdmi_codec_set_fmt(struct snd_soc_dai *dai, } }
- hcp->daifmt[dai->id] = cf; + hcp->daifmt[dai->id].fmt = cf.fmt; + hcp->daifmt[dai->id].bit_clk_inv = cf.bit_clk_inv; + hcp->daifmt[dai->id].frame_clk_inv = cf.frame_clk_inv; + hcp->daifmt[dai->id].bit_clk_master = cf.bit_clk_master; + hcp->daifmt[dai->id].frame_clk_master = cf.frame_clk_master;
return ret; } @@ -615,6 +630,7 @@ static const struct snd_soc_dai_ops hdmi_dai_ops = { .startup = hdmi_codec_startup, .shutdown = hdmi_codec_shutdown, .hw_params = hdmi_codec_hw_params, + .set_bclk_ratio = hdmi_codec_set_bclk_ratio, .set_fmt = hdmi_codec_set_fmt, .digital_mute = hdmi_codec_digital_mute, }; @@ -795,6 +811,8 @@ static int hdmi_codec_probe(struct platform_device *pdev) if (hcd->spdif) hcp->daidrv[i] = hdmi_spdif_dai;
+ hcp->daifmt[DAI_ID_I2S].bclk_ratio = 64; + ret = devm_snd_soc_register_component(dev, &hdmi_driver, hcp->daidrv, dai_count); if (ret) {
Then tda998x can look at daifmt->bclk_ratio in tda998x_audio_hw_params().
On Fri, Feb 22, 2019 at 04:27:35PM +0000, Russell King - ARM Linux admin wrote:
On Fri, Feb 22, 2019 at 10:47:35AM -0500, Sven Van Asbroeck wrote:
The config structure which you need to fill in to init the audio has a "i2s qualifier" field, where you have the choice between 16 and 32 bits. This then maps to a "Clock Time Stamp factor x" called CTSX, which maps to the following CTS_N register settings:
CTSX -> CTS_N (m,k)
16 -> (3,0) 32 -> (3,1) (i2s qualifier = 16 bits) 48 -> (3,2) 64 -> (3,3) (i2s qualifier = 32 bits) 128 -> (0,0)
Okay, this is my hypothesis about how the TDA998x works:
In the HDMI spec, the CTS value is generated using:
128*fS ---- divide ---> CTS counter -----> CTS value (clock) ^ ^ | | fTMDS_clock -----------------+------------> TMDS clock | N value -------+--------------------------> N value
What this does is count the number of TMDS clocks for every output of the divider to produce a value for CTS, effectively implementing
CTS = fTMDS_clock·N / 128·fS_source
The sink regenerates the 128·fS clock at the sink by reversing the process - this is the equation given in the HDMI spec:
128·fS_sink = fTMDS_clock·N / CTS
Using the "actual" values for 'm' and 'k' rather than the register values, if we subsitute BCLK for the 128·fS input, assume that instead of a CTS counter it is the mts counter which has to be divided by 'm' to get the CTS value, and take account of 'k' as an extra prescaler, what we end up with is:
mts = fTMDS_clock·N·k / BCLK CTS = fTMDS_clock·N·k / (BCLK·m)
Throw this into the reverse process, and we end up with:
128·fS_sink = BCLK·m / k
From the table of values you've given above, the CTSX value looks very
much like the BCLK fS ratio. For the 64·fS ratio, we have m=8 (reg value 3) k=4 (reg value 3):
BCLK ratio m k 128·fS_sink is in terms of fS_source 16 8 1 BCLK·8 128·fS_source 32 8 2 BCLK·4 128·fS_source 48 8 3 BCLK·8/3 128·fS_source 64 8 4 BCLK·2 128·fS_source 128 1 1 BCLK 128·fS_source
What this shows is that we end up with the same sample rate on the sink as the source despite the different BCLK ratios with this assumption.
What we also know is that SPDIF uses a 64·fS clock, and is programmed with m=8 k=4, which corresponds nicely with the above.
In light of that, what about this, which rejigs the driver to use a bclk_ratio rather than the sample width. We then just need to work out what to do about getting the bclk_ratio value into the driver in a way that we don't end up breaking existing users.
A possible solution to that would be for hdmi-codec to default that to zero unless it's been definitively provided by the ASoC "card", which would allow the old behaviour of selecting the CTS_N M/K values depending on the sample width, which we know works for some people.
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 9300469dbec0..3d5eb5024b2c 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -930,21 +930,26 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S); clksel_aip = AIP_CLKSEL_AIP_I2S; clksel_fs = AIP_CLKSEL_FS_ACLK; - switch (params->sample_width) { + switch (params->bclk_ratio) { case 16: + cts_n = CTS_N_M(3) | CTS_N_K(0); + break; + case 32: cts_n = CTS_N_M(3) | CTS_N_K(1); break; - case 18: - case 20: - case 24: + case 48: cts_n = CTS_N_M(3) | CTS_N_K(2); break; - default: - case 32: + case 64: cts_n = CTS_N_M(3) | CTS_N_K(3); break; + case 128: + cts_n = CTS_N_M(0) | CTS_N_K(0); + break; + default: + dev_err(&priv->hdmi->dev, "unsupported I2S bclk ratio\n"); + return -EINVAL; } - switch (params->format & AFMT_I2S_MASK) { case AFMT_I2S_LEFT_J: i2s_fmt = I2S_FORMAT_LEFT_J; @@ -1040,6 +1045,22 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, memcpy(audio.status, params->iec.status, min(sizeof(audio.status), sizeof(params->iec.status)));
+ /* Compatibility */ + switch (params->sample_width) { + case 16: + audio.bclk_ratio = 32; + break; + case 18: + case 20: + case 24: + audio.bclk_ratio = 48; + break; + default: + case 32: + audio.bclk_ratio = 64; + break; + } + switch (daifmt->fmt) { case HDMI_I2S: audio.format = AFMT_I2S | AFMT_I2S_PHILIPS; diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index b0864f0be017..4e0f0cd2d428 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -19,6 +19,7 @@ enum { struct tda998x_audio_params { u8 config; u8 format; + u8 bclk_ratio; unsigned sample_width; unsigned sample_rate; struct hdmi_audio_infoframe cea;
Russell, thank you so much for your patience, help and explanation, I really appreciate it !
On Fri, Feb 22, 2019 at 3:16 PM Russell King - ARM Linux admin linux@armlinux.org.uk wrote:
A possible solution to that would be for hdmi-codec to default that to zero unless it's been definitively provided by the ASoC "card", which would allow the old behaviour of selecting the CTS_N M/K values depending on the sample width, which we know works for some people.
Yes, that would keep the current users in business without them having to change anything.
Of course, poor souls like myself would have to patch, say, simple-card so we could provide a bclk ratio in the devicetree. Which would then propagate down to the tda998x via hdmi-codec. Which is fine by me.
Combining your two previous suggestions, I get the following. Now sample_width can be removed from tda998x_audio_params, as it's no longer used.
Would this be a good start?
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index a7c39f39793f..a306994ecc79 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -893,19 +893,25 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S); clksel_aip = AIP_CLKSEL_AIP_I2S; clksel_fs = AIP_CLKSEL_FS_ACLK; - switch (params->sample_width) { + switch (params->bclk_ratio) { case 16: + cts_n = CTS_N_M(3) | CTS_N_K(0); + break; + case 32: cts_n = CTS_N_M(3) | CTS_N_K(1); break; - case 18: - case 20: - case 24: + case 48: cts_n = CTS_N_M(3) | CTS_N_K(2); break; - default: - case 32: + case 64: cts_n = CTS_N_M(3) | CTS_N_K(3); break; + case 128: + cts_n = CTS_N_M(0) | CTS_N_K(0); + break; + default: + dev_err(&priv->hdmi->dev, "unsupported I2S bclk ratio\n"); + return -EINVAL; } break;
@@ -982,7 +988,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, struct tda998x_priv *priv = dev_get_drvdata(dev); int i, ret; struct tda998x_audio_params audio = { - .sample_width = params->sample_width, + .bclk_ratio = daifmt->bclk_ratio, .sample_rate = params->sample_rate, .cea = params->cea, }; @@ -1004,6 +1010,23 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, if (priv->audio_port[i].format == AFMT_I2S) audio.config = priv->audio_port[i].config; audio.format = AFMT_I2S; + if (!audio.bclk_ratio) { + /* compatibility */ + switch (params->sample_width) { + case 16: + audio.bclk_ratio = 32; + break; + case 18: + case 20: + case 24: + audio.bclk_ratio = 48; + break; + default: + case 32: + audio.bclk_ratio = 64; + break; + } + } break; case HDMI_SPDIF: for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index 3cb25ccbe5e6..039e1d9af2e0 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -14,7 +14,7 @@ enum { struct tda998x_audio_params { u8 config; u8 format; - unsigned sample_width; + u8 bclk_ratio; unsigned sample_rate; struct hdmi_audio_infoframe cea; u8 status[5]; diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 9483c55f871b..0fca69880dc3 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -42,6 +42,7 @@ struct hdmi_codec_daifmt { unsigned int frame_clk_inv:1; unsigned int bit_clk_master:1; unsigned int frame_clk_master:1; + unsigned int bclk_ratio; };
/* diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index d00734d31e04..d82f26854c90 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -524,6 +524,17 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, &hcp->daifmt[dai->id], &hp); }
+static int hdmi_codec_set_bclk_ratio(struct snd_soc_dai *dai, + unsigned int ratio) +{ + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); + + /* FIXME: some validation here would be good? */ + hcp->daifmt[dai->id].bclk_ratio = ratio; + + return 0; +} + static int hdmi_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) { @@ -593,7 +604,11 @@ static int hdmi_codec_set_fmt(struct snd_soc_dai *dai, } }
- hcp->daifmt[dai->id] = cf; + hcp->daifmt[dai->id].fmt = cf.fmt; + hcp->daifmt[dai->id].bit_clk_inv = cf.bit_clk_inv; + hcp->daifmt[dai->id].frame_clk_inv = cf.frame_clk_inv; + hcp->daifmt[dai->id].bit_clk_master = cf.bit_clk_master; + hcp->daifmt[dai->id].frame_clk_master = cf.frame_clk_master;
return ret; } @@ -615,6 +630,7 @@ static const struct snd_soc_dai_ops hdmi_dai_ops = { .startup = hdmi_codec_startup, .shutdown = hdmi_codec_shutdown, .hw_params = hdmi_codec_hw_params, + .set_bclk_ratio = hdmi_codec_set_bclk_ratio, .set_fmt = hdmi_codec_set_fmt, .digital_mute = hdmi_codec_digital_mute, };
On Fri, Feb 22, 2019 at 04:18:33PM -0500, Sven Van Asbroeck wrote:
Russell, thank you so much for your patience, help and explanation, I really appreciate it !
Yes, that would keep the current users in business without them having to change anything.
Of course, poor souls like myself would have to patch, say, simple-card so we could provide a bclk ratio in the devicetree. Which would then propagate down to the tda998x via hdmi-codec. Which is fine by me.
It probably would be better to try and find some generic way to deal with this.
After all, the I2S source probably knows which ratios it supports. Given that many sinks support a limited set of values as well, if ASoC core knew the supported set at each end of an I2S DAI format link, it could probably select a working bclk ratio automatically.
Combining your two previous suggestions, I get the following. Now sample_width can be removed from tda998x_audio_params, as it's no longer used.
Would this be a good start?
There's actually two threads of conversation going, and I recently had a reply from the maintainer of hdmi-codec suggesting a way forward - so I've coded that up as the three RFC patches you should have just received.
That should allow you to merely add a snd_soc_dai_set_bclk_ratio() call to set the bclk ratio while avoiding any breakage for existing users.
It does still contain my "FIXME" comment, so it isn't the final solution yet. One of the down-sides to the hdmi-codec shim is that it doesn't know which DAI formats nor which bclk ratios the hdmi-codec it's attached to supports, which makes validation against the codec capabilities rather awkward.
Sorry to have cut across your patch below.
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index a7c39f39793f..a306994ecc79 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -893,19 +893,25 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S); clksel_aip = AIP_CLKSEL_AIP_I2S; clksel_fs = AIP_CLKSEL_FS_ACLK;
switch (params->sample_width) {
switch (params->bclk_ratio) {cts_n = CTS_N_M(3) | CTS_N_K(0);break;case 32: cts_n = CTS_N_M(3) | CTS_N_K(1); break;
case 18:case 20:case 24:
case 48: cts_n = CTS_N_M(3) | CTS_N_K(2); break;
default:case 32:
case 64: cts_n = CTS_N_M(3) | CTS_N_K(3); break;case 128:cts_n = CTS_N_M(0) | CTS_N_K(0);break;default:dev_err(&priv->hdmi->dev, "unsupported I2S bclk ratio\n");return -EINVAL;@@ -982,7 +988,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, struct tda998x_priv *priv = dev_get_drvdata(dev); int i, ret; struct tda998x_audio_params audio = {
.sample_width = params->sample_width,
.bclk_ratio = daifmt->bclk_ratio,@@ -1004,6 +1010,23 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, if (priv->audio_port[i].format == AFMT_I2S) audio.config = priv->audio_port[i].config; audio.format = AFMT_I2S;
if (!audio.bclk_ratio) {/* compatibility */switch (params->sample_width) {case 16:audio.bclk_ratio = 32;break;case 18:case 20:case 24:audio.bclk_ratio = 48;break;default:case 32:audio.bclk_ratio = 64;break;}}diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index 3cb25ccbe5e6..039e1d9af2e0 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -14,7 +14,7 @@ enum { struct tda998x_audio_params { u8 config; u8 format;
- unsigned sample_width;
- u8 bclk_ratio; unsigned sample_rate; struct hdmi_audio_infoframe cea; u8 status[5];
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index 9483c55f871b..0fca69880dc3 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -42,6 +42,7 @@ struct hdmi_codec_daifmt { unsigned int frame_clk_inv:1; unsigned int bit_clk_master:1; unsigned int frame_clk_master:1;
- unsigned int bclk_ratio;
};
/* diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index d00734d31e04..d82f26854c90 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -524,6 +524,17 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, &hcp->daifmt[dai->id], &hp); }
+static int hdmi_codec_set_bclk_ratio(struct snd_soc_dai *dai,
unsigned int ratio)+{
- struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
- /* FIXME: some validation here would be good? */
- hcp->daifmt[dai->id].bclk_ratio = ratio;
- return 0;
+}
static int hdmi_codec_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) { @@ -593,7 +604,11 @@ static int hdmi_codec_set_fmt(struct snd_soc_dai *dai, } }
- hcp->daifmt[dai->id] = cf;
hcp->daifmt[dai->id].fmt = cf.fmt;
hcp->daifmt[dai->id].bit_clk_inv = cf.bit_clk_inv;
hcp->daifmt[dai->id].frame_clk_inv = cf.frame_clk_inv;
hcp->daifmt[dai->id].bit_clk_master = cf.bit_clk_master;
hcp->daifmt[dai->id].frame_clk_master = cf.frame_clk_master;
return ret;
} @@ -615,6 +630,7 @@ static const struct snd_soc_dai_ops hdmi_dai_ops = { .startup = hdmi_codec_startup, .shutdown = hdmi_codec_shutdown, .hw_params = hdmi_codec_hw_params,
- .set_bclk_ratio = hdmi_codec_set_bclk_ratio, .set_fmt = hdmi_codec_set_fmt, .digital_mute = hdmi_codec_digital_mute,
};
2.17.1
On Fri, Feb 22, 2019 at 4:36 PM Russell King - ARM Linux admin linux@armlinux.org.uk wrote:
There's actually two threads of conversation going, and I recently had a reply from the maintainer of hdmi-codec suggesting a way forward - so I've coded that up as the three RFC patches you should have just received.
Thank you, that's awesome !
It probably would be better to try and find some generic way to deal with this.
After all, the I2S source probably knows which ratios it supports. Given that many sinks support a limited set of values as well, if ASoC core knew the supported set at each end of an I2S DAI format link, it could probably select a working bclk ratio automatically.
Agree, possibly the same way the ASoC core auto-matches both sides when they are connected with a dai_link? Pardon my ignorance.
Of course the auto-matching should only happen when both sides provide a bclk ratio range - to avoid having to retro-fit every single dai.
dri-devel@lists.freedesktop.org
-
Russell King - ARM Linux admin -
Sven Van Asbroeck