Date:   Fri, 22 Feb 2019 20:16:41 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Sven Van Asbroeck <thesven73@...il.com>,
        Mark Brown <broonie@...nel.org>
Cc:     David Airlie <airlied@...ux.ie>, Daniel Vetter <daniel@...ll.ch>,
        dri-devel@...ts.freedesktop.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Peter Rosin <peda@...ntia.se>
Subject: Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider
 calculation

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

Powered by blists - more mailing lists