[PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation

[PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation

[Sven Van Asbroeck]

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];
-- 
2.17.1

[PATCH 2/2] sound: hdmi-codec: propagate physical_width

[Sven Van Asbroeck]

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,
-- 
2.17.1

Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation

[Russell King - ARM Linux admin]

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) {
>  		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];
> -- 
> 2.17.1
> 
> 

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

Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation

[Sven Van Asbroeck]

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?

Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation

[Russell King - ARM Linux admin]

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

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

Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation

[Russell King - ARM Linux admin]

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;

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

Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation

[Sven Van Asbroeck]

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,
 };
-- 
2.17.1

Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation

[Russell King - ARM Linux admin]

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) {
>  		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,
>  };
> -- 
> 2.17.1
> 
> 

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

Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation

[Sven Van Asbroeck]

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.