[RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default

[RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default

[Peter Ujfalusi]

Hi,

the original version was sent 14.04.2018:
https://patchwork.kernel.org/patch/10344403/

Changes since then:
- rebased on currentl drm/next

The reset value of the register is 0, the soft reset does not reset this
register and if other kernel changed this the audio is going to be
distorted.

It was observed when - accidentally - booted the kernel from eMMC on BBB
which is 3.8.13-bone79 and it sets this register to 0x0a. After reboot and
tda998x_reset() it remains 0x0a.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 7f34601bb515..72f93802d209 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -722,6 +722,9 @@ tda998x_reset(struct tda998x_priv *priv)
 
 	/* Write the default value MUX register */
 	reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24);
+
+	/* Write the default to I2S_FORMAT register */
+	reg_write(priv, REG_I2S_FORMAT,   0x00);
 }
 
 /*
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default

[Peter Ujfalusi]

On 22/02/2019 15.47, Peter Ujfalusi wrote:
> Hi,
> 
> the original version was sent 14.04.2018:

17.04.2018

> https://patchwork.kernel.org/patch/10344403/
> 
> Changes since then:
> - rebased on currentl drm/next
> 
> The reset value of the register is 0, the soft reset does not reset this
> register and if other kernel changed this the audio is going to be
> distorted.
> 
> It was observed when - accidentally - booted the kernel from eMMC on BBB
> which is 3.8.13-bone79 and it sets this register to 0x0a. After reboot and
> tda998x_reset() it remains 0x0a.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 7f34601bb515..72f93802d209 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -722,6 +722,9 @@ tda998x_reset(struct tda998x_priv *priv)
>  
>  	/* Write the default value MUX register */
>  	reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24);
> +
> +	/* Write the default to I2S_FORMAT register */
> +	reg_write(priv, REG_I2S_FORMAT,   0x00);
>  }
>  
>  /*
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default

[Russell King - ARM Linux admin]

On Fri, Feb 22, 2019 at 03:47:14PM +0200, Peter Ujfalusi wrote:
> Hi,
> 
> the original version was sent 14.04.2018:
> https://patchwork.kernel.org/patch/10344403/
> 
> Changes since then:
> - rebased on currentl drm/next
> 
> The reset value of the register is 0, the soft reset does not reset this
> register and if other kernel changed this the audio is going to be
> distorted.
> 
> It was observed when - accidentally - booted the kernel from eMMC on BBB
> which is 3.8.13-bone79 and it sets this register to 0x0a. After reboot and
> tda998x_reset() it remains 0x0a.

Have you checked whether the input I2S stream is Philips or Left
Justified?  This is controlled by the LSB two bits.

It appears that 3.8.13-bone79 configures the TDA998x for left-
justified, whereas re-setting these two bits to zero will configure
it for Philips.

Bits 3:2 control the data size, but I have no information what their
values correspond to.

Can we nail down what is required for BBB and what it doesn't care
about - and I think we should implement at least setting the I2S
register format from the hdmi_codec_daifmt data.

It would also be good to know what Fs value(s) BBB uses, and what
sample sizes you have tested.

> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 7f34601bb515..72f93802d209 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -722,6 +722,9 @@ tda998x_reset(struct tda998x_priv *priv)
>  
>  	/* Write the default value MUX register */
>  	reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24);
> +
> +	/* Write the default to I2S_FORMAT register */
> +	reg_write(priv, REG_I2S_FORMAT,   0x00);
>  }
>  
>  /*
> -- 
> Peter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> 

-- 
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default

[Peter Ujfalusi]

Hi Russell,

On 22/02/2019 16.35, Russell King - ARM Linux admin wrote:
> On Fri, Feb 22, 2019 at 03:47:14PM +0200, Peter Ujfalusi wrote:
>> Hi,
>>
>> the original version was sent 14.04.2018:
>> https://patchwork.kernel.org/patch/10344403/
>>
>> Changes since then:
>> - rebased on currentl drm/next
>>
>> The reset value of the register is 0, the soft reset does not reset this
>> register and if other kernel changed this the audio is going to be
>> distorted.
>>
>> It was observed when - accidentally - booted the kernel from eMMC on BBB
>> which is 3.8.13-bone79 and it sets this register to 0x0a. After reboot and
>> tda998x_reset() it remains 0x0a.
> 
> Have you checked whether the input I2S stream is Philips or Left
> Justified?  This is controlled by the LSB two bits.

The am335x-boneblack-common.dtsi configures the link to i2s, which
corresponds to Philips format (the default

> 
> It appears that 3.8.13-bone79 configures the TDA998x for left-
> justified, whereas re-setting these two bits to zero will configure
> it for Philips.

The chip reset value for the register is 0 and software reset will not
reset it if it was modified.

> Bits 3:2 control the data size, but I have no information what their
> values correspond to.

I can not find the register descriptions, can not tell what are the bits
in there.

> Can we nail down what is required for BBB and what it doesn't care
> about.

atm the REG_I2S_FORMAT register needs to be reset to 0.

> and I think we should implement at least setting the I2S
> register format from the hdmi_codec_daifmt data.

Yes, that needs to be done for sure, but without data sheet with
register descriptions I would not attempt to do that.

> It would also be good to know what Fs value(s) BBB uses, and what
> sample sizes you have tested.

On BBB McASP is the clock master and as I recall I have tested 44.1, 48
KHz at least with 16 and 24 bits.

>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/gpu/drm/i2c/tda998x_drv.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
>> index 7f34601bb515..72f93802d209 100644
>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>> @@ -722,6 +722,9 @@ tda998x_reset(struct tda998x_priv *priv)
>>  
>>  	/* Write the default value MUX register */
>>  	reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24);
>> +
>> +	/* Write the default to I2S_FORMAT register */
>> +	reg_write(priv, REG_I2S_FORMAT,   0x00);
>>  }
>>  
>>  /*
>> -- 
>> Peter
>>
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>
>>
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default

[Russell King - ARM Linux admin]

On Fri, Feb 22, 2019 at 05:20:15PM +0200, Peter Ujfalusi wrote:
> Hi Russell,
> 
> On 22/02/2019 16.35, Russell King - ARM Linux admin wrote:
> > On Fri, Feb 22, 2019 at 03:47:14PM +0200, Peter Ujfalusi wrote:
> >> Hi,
> >>
> >> the original version was sent 14.04.2018:
> >> https://patchwork.kernel.org/patch/10344403/
> >>
> >> Changes since then:
> >> - rebased on currentl drm/next
> >>
> >> The reset value of the register is 0, the soft reset does not reset this
> >> register and if other kernel changed this the audio is going to be
> >> distorted.
> >>
> >> It was observed when - accidentally - booted the kernel from eMMC on BBB
> >> which is 3.8.13-bone79 and it sets this register to 0x0a. After reboot and
> >> tda998x_reset() it remains 0x0a.
> > 
> > Have you checked whether the input I2S stream is Philips or Left
> > Justified?  This is controlled by the LSB two bits.
> 
> The am335x-boneblack-common.dtsi configures the link to i2s, which
> corresponds to Philips format (the default
> 
> > 
> > It appears that 3.8.13-bone79 configures the TDA998x for left-
> > justified, whereas re-setting these two bits to zero will configure
> > it for Philips.
> 
> The chip reset value for the register is 0 and software reset will not
> reset it if it was modified.

So I wonder why 3.8.13-bone79 configures it for left-justified.

> > Bits 3:2 control the data size, but I have no information what their
> > values correspond to.
> 
> I can not find the register descriptions, can not tell what are the bits
> in there.
> 
> > Can we nail down what is required for BBB and what it doesn't care
> > about.
> 
> atm the REG_I2S_FORMAT register needs to be reset to 0.
> 
> > and I think we should implement at least setting the I2S
> > register format from the hdmi_codec_daifmt data.
> 
> Yes, that needs to be done for sure, but without data sheet with
> register descriptions I would not attempt to do that.
> 
> > It would also be good to know what Fs value(s) BBB uses, and what
> > sample sizes you have tested.
> 
> On BBB McASP is the clock master and as I recall I have tested 44.1, 48
> KHz at least with 16 and 24 bits.

Sorry, I wasn't clear enough... is the bus clocked at 32Fs for 16bit
samples and 64Fs for 24bit samples, or is it 64Fs for both?

> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> >> ---
> >>  drivers/gpu/drm/i2c/tda998x_drv.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> >> index 7f34601bb515..72f93802d209 100644
> >> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> >> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> >> @@ -722,6 +722,9 @@ tda998x_reset(struct tda998x_priv *priv)
> >>  
> >>  	/* Write the default value MUX register */
> >>  	reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24);
> >> +
> >> +	/* Write the default to I2S_FORMAT register */
> >> +	reg_write(priv, REG_I2S_FORMAT,   0x00);
> >>  }
> >>  
> >>  /*
> >> -- 
> >> Peter
> >>
> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> >>
> >>
> > 
> 
> - Péter
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 

-- 
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default

[Russell King - ARM Linux admin]

On Fri, Feb 22, 2019 at 05:20:15PM +0200, Peter Ujfalusi wrote:
> > and I think we should implement at least setting the I2S
> > register format from the hdmi_codec_daifmt data.
> 
> Yes, that needs to be done for sure, but without data sheet with
> register descriptions I would not attempt to do that.

How about this, although it's not particularly pretty.

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a7c39f39793f..c53128bb40fd 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -242,7 +242,9 @@ struct tda998x_priv {
 # define HVF_CNTRL_1_SEMI_PLANAR  (1 << 6)
 #define REG_RPT_CNTRL             REG(0x00, 0xf0)     /* write */
 #define REG_I2S_FORMAT            REG(0x00, 0xfc)     /* read/write */
-# define I2S_FORMAT(x)            (((x) & 3) << 0)
+# define I2S_FORMAT_PHILIPS       (0 << 0)
+# define I2S_FORMAT_LEFT_J        (2 << 0)
+# define I2S_FORMAT_RIGHT_J       (3 << 0)
 #define REG_AIP_CLKSEL            REG(0x00, 0xfd)     /* write */
 # define AIP_CLKSEL_AIP_SPDIF	  (0 << 3)
 # define AIP_CLKSEL_AIP_I2S	  (1 << 3)
@@ -872,14 +874,14 @@ static int
 tda998x_configure_audio(struct tda998x_priv *priv,
 			struct tda998x_audio_params *params)
 {
-	u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv;
+	u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv, i2s_fmt;
 	u32 n;
 
 	/* Enable audio ports */
 	reg_write(priv, REG_ENA_AP, params->config);
 
 	/* Set audio input source */
-	switch (params->format) {
+	switch (params->format & AFMT_MASK) {
 	case AFMT_SPDIF:
 		reg_write(priv, REG_ENA_ACLK, 0);
 		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF);
@@ -907,6 +909,19 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 			cts_n = CTS_N_M(3) | CTS_N_K(3);
 			break;
 		}
+
+		switch (params->format & AFMT_I2S_MASK) {
+		case AFMT_I2S_LEFT_J:
+			i2s_fmt = I2S_FORMAT_LEFT_J;
+			break;
+		case AFMT_I2S_RIGHT_J:
+			i2s_fmt = I2S_FORMAT_RIGHT_J;
+			break;
+		default:
+			i2s_fmt = I2S_FORMAT_PHILIPS;
+			break;
+		}
+		reg_write(priv, REG_I2S_FORMAT, i2s_fmt);
 		break;
 
 	default:
@@ -992,23 +1007,15 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 
 	switch (daifmt->fmt) {
 	case HDMI_I2S:
-		if (daifmt->bit_clk_inv || daifmt->frame_clk_inv ||
-		    daifmt->bit_clk_master || daifmt->frame_clk_master) {
-			dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__,
-				daifmt->bit_clk_inv, daifmt->frame_clk_inv,
-				daifmt->bit_clk_master,
-				daifmt->frame_clk_master);
-			return -EINVAL;
-		}
-		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
-			if (priv->audio_port[i].format == AFMT_I2S)
-				audio.config = priv->audio_port[i].config;
-		audio.format = AFMT_I2S;
+		audio.format = AFMT_I2S | AFMT_I2S_PHILIPS;
+		break;
+	case HDMI_LEFT_J:
+		audio.format = AFMT_I2S | AFMT_I2S_LEFT_J;
+		break;
+	case HDMI_RIGHT_J:
+		audio.format = AFMT_I2S | AFMT_I2S_RIGHT_J;
 		break;
 	case HDMI_SPDIF:
-		for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
-			if (priv->audio_port[i].format == AFMT_SPDIF)
-				audio.config = priv->audio_port[i].config;
 		audio.format = AFMT_SPDIF;
 		break;
 	default:
@@ -1016,11 +1023,25 @@ static int tda998x_audio_hw_params(struct device *dev, void *data,
 		return -EINVAL;
 	}
 
+	for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
+		if (priv->audio_port[i].format == (audio.format & AFMT_MASK))
+			audio.config = priv->audio_port[i].config;
+
 	if (audio.config == 0) {
 		dev_err(dev, "%s: No audio configuration found\n", __func__);
 		return -EINVAL;
 	}
 
+	if ((audio.format & AFMT_MASK) == HDMI_I2S &&
+	    (daifmt->bit_clk_inv || daifmt->frame_clk_inv ||
+	     daifmt->bit_clk_master || daifmt->frame_clk_master)) {
+		dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__,
+			daifmt->bit_clk_inv, daifmt->frame_clk_inv,
+			daifmt->bit_clk_master,
+			daifmt->frame_clk_master);
+		return -EINVAL;
+	}
+
 	mutex_lock(&priv->audio_mutex);
 	if (priv->supports_infoframes && priv->sink_has_audio)
 		ret = tda998x_configure_audio(priv, &audio);
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index 3cb25ccbe5e6..b0864f0be017 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -6,9 +6,14 @@
 #include <dt-bindings/display/tda998x.h>
 
 enum {
-	AFMT_UNUSED =	0,
-	AFMT_SPDIF =	TDA998x_SPDIF,
-	AFMT_I2S =	TDA998x_I2S,
+	AFMT_UNUSED      = 0,
+	AFMT_SPDIF       = TDA998x_SPDIF,
+	AFMT_I2S         = TDA998x_I2S,
+	AFMT_MASK        = AFMT_SPDIF | AFMT_I2S,
+	AFMT_I2S_PHILIPS = 0 << 4,
+	AFMT_I2S_LEFT_J  = 1 << 4,
+	AFMT_I2S_RIGHT_J = 2 << 4,
+	AFMT_I2S_MASK    = 3 << 4,
 };
 
 struct tda998x_audio_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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default

[Russell King - ARM Linux admin]

On Fri, Feb 22, 2019 at 03:27:43PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Feb 22, 2019 at 05:20:15PM +0200, Peter Ujfalusi wrote:
> > Hi Russell,
> > 
> > On 22/02/2019 16.35, Russell King - ARM Linux admin wrote:
> > > It would also be good to know what Fs value(s) BBB uses, and what
> > > sample sizes you have tested.
> > 
> > On BBB McASP is the clock master and as I recall I have tested 44.1, 48
> > KHz at least with 16 and 24 bits.
> 
> Sorry, I wasn't clear enough... is the bus clocked at 32Fs for 16bit
> samples and 64Fs for 24bit samples, or is it 64Fs for both?

To be clear, the reason I'm asking for this information is that
Sven Van Asbroeck is trying to use TDA998x, and he has a problem with
the current implementation that adjusts CTS_N_M and CTS_N_K parameters
according to the _sample_ size.

This appears to be wrong, these settings should be set according to
the BCLK ratio (Fs multiplier) and not the sample size.

If you say that the existing code works for you, but your device
always produces a bclk at 64fs, then we have a problem - it means
that to add support for Sven's platform, we're probably going to end
up causing a regression for your platform.

The next issue is with snd_soc_dai_set_bclk_ratio().  Today, no one
calls that function, so none of the DAI .set_bclk_ratio
implementations are used (and probably completely untested.)  If your
CPU DAI changes the bclk ratio depending on the sample size, I will
need something to call that function at the appropriate time to set
the clocking ratio.

I suspect most codecs don't care about it, but TDA998x does, because
it _looks_ like it uses the BCLK to generate the CTS value to be
passed to the HDMI sink.  Since CTS = f(tmds) * N / (128 * fs),
using BCLK to derive the CTS value requires TDA998x to know the
BCLK ratio.

So, knowing this information ahead of time is very advantageous.

-- 
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default

[Jyri Sarha]

On 22/02/2019 18:40, Russell King - ARM Linux admin wrote:
> On Fri, Feb 22, 2019 at 03:27:43PM +0000, Russell King - ARM Linux admin wrote:
>> On Fri, Feb 22, 2019 at 05:20:15PM +0200, Peter Ujfalusi wrote:
>>> Hi Russell,
>>>
>>> On 22/02/2019 16.35, Russell King - ARM Linux admin wrote:
>>>> It would also be good to know what Fs value(s) BBB uses, and what
>>>> sample sizes you have tested.
>>>
>>> On BBB McASP is the clock master and as I recall I have tested 44.1, 48
>>> KHz at least with 16 and 24 bits.
>>

Peter, I doubt you you were able to natively play 24bit 48kHz stereo, or
anything at 44.1kHz. Did you have ALSA plug -plugin configured?

BBB has 24.576MHz McASP mclk, which divides nicely by 48kHz to 512 clock
cycles / sample. 512 is divisible by 32 (= 16-bit stereo) and 64 (=
32-bit stereo), but not by 48 e.g. 24-bit stereo.

That is why I originally insisted in allowing 32-bit sample-format in
HDMI-codec (that is used in tda998x) and simply marking in struct
snd_soc_dai_driver that there is only 24 significant bits per a sample.

The other alternative would have been using set_bclk_ratio(), but it was
a quite new thing back then and had even less support in the drivers
than the currently.

>> Sorry, I wasn't clear enough... is the bus clocked at 32Fs for 16bit
>> samples and 64Fs for 24bit samples, or is it 64Fs for both?
> 

It is 32Fs for 16-bit and 64Fs for 32-bit, but 24-bit format is not
supported as such.

> To be clear, the reason I'm asking for this information is that
> Sven Van Asbroeck is trying to use TDA998x, and he has a problem with
> the current implementation that adjusts CTS_N_M and CTS_N_K parameters
> according to the _sample_ size.
> 
> This appears to be wrong, these settings should be set according to
> the BCLK ratio (Fs multiplier) and not the sample size.
> 
> If you say that the existing code works for you, but your device
> always produces a bclk at 64fs, then we have a problem - it means
> that to add support for Sven's platform, we're probably going to end
> up causing a regression for your platform.
> 
> The next issue is with snd_soc_dai_set_bclk_ratio().  Today, no one
> calls that function, so none of the DAI .set_bclk_ratio
> implementations are used (and probably completely untested.)  If your
> CPU DAI changes the bclk ratio depending on the sample size, I will
> need something to call that function at the appropriate time to set
> the clocking ratio.
> 
> I suspect most codecs don't care about it, but TDA998x does, because
> it _looks_ like it uses the BCLK to generate the CTS value to be
> passed to the HDMI sink.  Since CTS = f(tmds) * N / (128 * fs),
> using BCLK to derive the CTS value requires TDA998x to know the
> BCLK ratio.
> 
> So, knowing this information ahead of time is very advantageous.
> 

Sound to me that .set_bclk_ratio() support should be added to tda998x
(and HDMI-codec), but with the default behaviour that assumes the
bclk-ratio to be 2*sample-width if .set_bclk_ratio() is not called.
bcm2835-i2s codec driver appears to already implement .set_bclk_ratio()
like that.

Or am I missing something?

Best regards,
Jyri

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND] drm/i2c: tda998x: Reset the I2S_FORMAT (Page0, Reg 0xfc) to it's default

[Peter Ujfalusi]

Hi Russell,

On 22/02/2019 17.27, Russell King - ARM Linux admin wrote:
>> On BBB McASP is the clock master and as I recall I have tested 44.1, 48
>> KHz at least with 16 and 24 bits.
> 
> Sorry, I wasn't clear enough... is the bus clocked at 32Fs for 16bit
> samples and 64Fs for 24bit samples, or is it 64Fs for both?

Now that I have dug out the board:
Playback of the following formats works w/o plughw conversion:
32KHz/48KHz/96KHz, stereo, 16bit (32fs on bus)
32KHz/48KHz/96KHz, stereo, 24bit (S24_LE or S24_3LE - 48fs on bus)

McASP is only putting the needed amount of bclk on the bus for the given
format - uses params_width() for divider calculation.

>>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>>> ---
>>>>  drivers/gpu/drm/i2c/tda998x_drv.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
>>>> index 7f34601bb515..72f93802d209 100644
>>>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>>>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>>>> @@ -722,6 +722,9 @@ tda998x_reset(struct tda998x_priv *priv)
>>>>  
>>>>  	/* Write the default value MUX register */
>>>>  	reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24);
>>>> +
>>>> +	/* Write the default to I2S_FORMAT register */
>>>> +	reg_write(priv, REG_I2S_FORMAT,   0x00);
>>>>  }
>>>>  
>>>>  /*
>>>> -- 
>>>> Peter
>>>>
>>>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>>>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>>>
>>>>
>>>
>>
>> - Péter
>>
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
>> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel