[PATCH RFC 0/3] tda998x updates for DAI formats and bclk_ratio

[PATCH RFC 0/3] tda998x updates for DAI formats and bclk_ratio

[Russell King - ARM Linux admin]

This series addresses two issues with TDA998x that have been
identified:

1) Peter found that the I2S format was not being explicitly set, and
   retains its value from whatever was previously running on the
   platform.  Work around this by implementing support for setting
   the I2S format from the DAI format, rather than merely defaulting
   the register back to its power-on value.

2) Sven found that TDA998x does not work on his Freescale platform,
   which always uses a 64·fs bitclock.  The TDA998x driver was
   deriving this information from the sample width, which, while it
   works for Beagle Bone Black, does not allow the driver to be used
   with other I2S sources that may have different behaviours.

   To work around that, we implement support for
   snd_soc_dai_set_bclk_ratio() in hdmi-codec, and propagate its
   value to TDA998x and other HDMI codecs via a new member.  However,
   since snd_soc_dai_set_bclk_ratio() is never called, we need to
   avoid breaking any existing users, so we detect the lack of call
   by an impossible zero value, and subsitute a value corresponding
   with the TDA998x's old behaviour.

   It is hoped that snd_soc_dai_set_bclk_ratio() will see more
   adoption in ASoC, and the TDA998x specific defaulting can be
   removed.

 drivers/gpu/drm/i2c/tda998x_drv.c | 75 ++++++++++++++++++++++++++-------------
 include/drm/i2c/tda998x.h         | 12 +++++--
 include/sound/hdmi-codec.h        |  1 +
 sound/soc/codecs/hdmi-codec.c     | 45 +++++++++++++++++++++--
 4 files changed, 104 insertions(+), 29 deletions(-)

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

[PATCH RFC 1/3] drm/i2c: tda998x: implement different I2S flavours

[Russell King]

Add support for the left and right justified I2S formats as well as the
more tranditional "Philips" I2S format.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 57 ++++++++++++++++++++++++++-------------
 include/drm/i2c/tda998x.h         | 11 +++++---
 2 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a7c39f39793f..645d884fb9e8 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 {
-- 
2.7.4

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

[PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Russell King]

Some HDMI codecs need to know the relationship between the I2S bit clock
and the I2S word clock in order to correctly generate the CTS value for
audio clock recovery on the sink.

Add support for this, but there are currently no callers of
snd_soc_dai_set_bclk_ratio(), we provide a default implementation that
uses the sample width to derive the ratio from the 8-bit aligned
sample size.  This reflects the derivation that is in TDA998x, which
we are going to convert to use this new support.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 include/sound/hdmi-codec.h    |  1 +
 sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 2 deletions(-)

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 e5b6769b9797..d71a7e5a2231 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -470,6 +470,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
 				struct snd_soc_dai *dai)
 {
 	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+	struct hdmi_codec_daifmt fmt;
 	struct hdmi_codec_params hp = {
 		.iec = {
 			.status = { 0 },
@@ -520,8 +521,43 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
 	hp.sample_rate = params_rate(params);
 	hp.channels = params_channels(params);
 
+	fmt = hcp->daifmt[dai->id];
+
+	/*
+	 * If the .set_bclk_ratio() has not been called, default it
+	 * using the sample width for compatibility for TDA998x.
+	 * Rather than changing this, drivers should arrange to make
+	 * an appropriate call to snd_soc_dai_set_bclk_ratio().
+	 */
+	if (fmt.bclk_ratio == 0) {
+		switch (hp.sample_width) {
+		case 16:
+			fmt.bclk_ratio = 32;
+			break;
+		case 18:
+		case 20:
+		case 24:
+			fmt.bclk_ratio = 48;
+			break;
+		default:
+			fmt.bclk_ratio = 64;
+			break;
+		}
+	}
+
 	return hcp->hcd.ops->hw_params(dai->dev->parent, hcp->hcd.data,
-				       &hcp->daifmt[dai->id], &hp);
+				       &fmt, &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,
@@ -593,7 +629,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 +655,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.7.4

[PATCH RFC 3/3] drm/i2c: tda998x: add support for bclk_ratio

[Russell King]

It appears that TDA998x derives the CTS value using the supplied I2S
bit clock (BCLK) rather than 128·fs.  TDA998x uses two constants named
m and k in the CTS generator such that we have this relationship
between the source BCLK and the sink fs:

	128·fs_sink = BCLK·m / k

Where BCLK = bclk_ratio·fs_source.

We have been lucky up to now that all users have scaled their bclk_ratio
to match the sample width - for example, on Beagle Bone Black, with a
16-bit sample width, BCLK = 32·fs, which increases to 64·fs for 32-bit
samples.  24-bit samples are sent as 32-bit.

We are now starting to see users whose I2S blocks send at 64·fs for
16-bit samples, which means TDA998x now breaks.

ASoC has a snd_soc_dai_set_bclk_ratio() call available which sets the
ratio of BCLK to the sample rate.  Implement support for this.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 20 +++++++++++++-------
 include/drm/i2c/tda998x.h         |  1 +
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 645d884fb9e8..f2d40235acf9 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -895,21 +895,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;
@@ -997,6 +1002,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 = {
+		.bclk_ratio = daifmt->bclk_ratio,
 		.sample_width = params->sample_width,
 		.sample_rate = params->sample_rate,
 		.cea = params->cea,
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;
-- 
2.7.4

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

Re: [PATCH RFC 1/3] drm/i2c: tda998x: implement different I2S flavours

[Jyri Sarha]

On 22/02/2019 23:27, Russell King wrote:
> Add support for the left and right justified I2S formats as well as the
> more tranditional "Philips" I2S format.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

I do not have a spec to check REG_I2S_FORMAT bits, but at least philips
and left justified formats work on BBB (McASP does not support right
justified format). With the above conditions:

Reviewed-by: Jyri Sarha <jsarha@ti.com>
Tested-by: Jyri Sarha <jsarha@ti.com>



> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 57 ++++++++++++++++++++++++++-------------
>  include/drm/i2c/tda998x.h         | 11 +++++---
>  2 files changed, 47 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index a7c39f39793f..645d884fb9e8 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 {
> 


-- 
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: [PATCH RFC 1/3] drm/i2c: tda998x: implement different I2S flavours

[Peter Ujfalusi]

hi Russell,

On 22/02/2019 23.27, Russell King wrote:
> Add support for the left and right justified I2S formats as well as the
> more tranditional "Philips" I2S format.

First of all, thank you for the patch, it works.

Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

There is however one thing I'm not sure about.
the 3.8 kernel configured the page0:0xfc register [1]:
/* select I2S format, and datasize */
reg_write(encoder, REG_I2S_FORMAT, 0x0a);

In theory this should select left_j and set bit3 which does something.
It looks like that the McASP is configured to I2S mode in 3.8 kernel
which would result channel swap at least there (I2S vs left_j).

Do you know what the bit3 is configuring and to what?

[1]
https://github.com/beagleboard/linux/blob/1f2f3402a6e4b8c90148c0ca2fd3acba91738eb3/drivers/gpu/drm/i2c/tda998x_drv.c#L851


> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 57 ++++++++++++++++++++++++++-------------
>  include/drm/i2c/tda998x.h         | 11 +++++---
>  2 files changed, 47 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index a7c39f39793f..645d884fb9e8 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 {
> 

- 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: [PATCH RFC 1/3] drm/i2c: tda998x: implement different I2S flavours

[Russell King - ARM Linux admin]

On Mon, Feb 25, 2019 at 03:28:51PM +0200, Peter Ujfalusi wrote:
> hi Russell,
> 
> On 22/02/2019 23.27, Russell King wrote:
> > Add support for the left and right justified I2S formats as well as the
> > more tranditional "Philips" I2S format.
> 
> First of all, thank you for the patch, it works.
> 
> Tested-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> There is however one thing I'm not sure about.
> the 3.8 kernel configured the page0:0xfc register [1]:
> /* select I2S format, and datasize */
> reg_write(encoder, REG_I2S_FORMAT, 0x0a);
> 
> In theory this should select left_j and set bit3 which does something.
> It looks like that the McASP is configured to I2S mode in 3.8 kernel
> which would result channel swap at least there (I2S vs left_j).
> 
> Do you know what the bit3 is configuring and to what?

Bits 2 and 3 are something to do with "data size" which is as much
information as I have on those two bits.  Maybe they apply to the
right justified mode as that would certainly need to know the width
of the supplied I2S sample data.

-- 
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: [PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Jyri Sarha]

On 22/02/2019 23:27, Russell King wrote:
> Some HDMI codecs need to know the relationship between the I2S bit clock
> and the I2S word clock in order to correctly generate the CTS value for
> audio clock recovery on the sink.
> 
> Add support for this, but there are currently no callers of
> snd_soc_dai_set_bclk_ratio(), we provide a default implementation that
> uses the sample width to derive the ratio from the 8-bit aligned
> sample size.  This reflects the derivation that is in TDA998x, which
> we are going to convert to use this new support.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  include/sound/hdmi-codec.h    |  1 +
>  sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 44 insertions(+), 2 deletions(-)
> 
> 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 e5b6769b9797..d71a7e5a2231 100644
> --- a/sound/soc/codecs/hdmi-codec.c
> +++ b/sound/soc/codecs/hdmi-codec.c
> @@ -470,6 +470,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
>  				struct snd_soc_dai *dai)
>  {
>  	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
> +	struct hdmi_codec_daifmt fmt;
>  	struct hdmi_codec_params hp = {
>  		.iec = {
>  			.status = { 0 },
> @@ -520,8 +521,43 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
>  	hp.sample_rate = params_rate(params);
>  	hp.channels = params_channels(params);
>  
> +	fmt = hcp->daifmt[dai->id];
> +
> +	/*
> +	 * If the .set_bclk_ratio() has not been called, default it
> +	 * using the sample width for compatibility for TDA998x.
> +	 * Rather than changing this, drivers should arrange to make
> +	 * an appropriate call to snd_soc_dai_set_bclk_ratio().
> +	 */
> +	if (fmt.bclk_ratio == 0) {
> +		switch (hp.sample_width) {
> +		case 16:
> +			fmt.bclk_ratio = 32;
> +			break;
> +		case 18:
> +		case 20:
> +		case 24:
> +			fmt.bclk_ratio = 48;
> +			break;

AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually,
the bclk_ratio is set to the exact frame length required by the sample
width without any padding. That is at least the case with
tlv320aic3x-driver and 20-bit sample width.


> +		default:
> +			fmt.bclk_ratio = 64;
> +			break;
> +		}
> +	}
> +
>  	return hcp->hcd.ops->hw_params(dai->dev->parent, hcp->hcd.data,
> -				       &hcp->daifmt[dai->id], &hp);
> +				       &fmt, &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,
> @@ -593,7 +629,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 +655,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,
>  };
> 


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

Re: [PATCH RFC 3/3] drm/i2c: tda998x: add support for bclk_ratio

[Jyri Sarha]

On 22/02/2019 23:27, Russell King wrote:
> It appears that TDA998x derives the CTS value using the supplied I2S
> bit clock (BCLK) rather than 128·fs.  TDA998x uses two constants named
> m and k in the CTS generator such that we have this relationship
> between the source BCLK and the sink fs:
> 
> 	128·fs_sink = BCLK·m / k
> 
> Where BCLK = bclk_ratio·fs_source.
> 
> We have been lucky up to now that all users have scaled their bclk_ratio
> to match the sample width - for example, on Beagle Bone Black, with a
> 16-bit sample width, BCLK = 32·fs, which increases to 64·fs for 32-bit
> samples.  24-bit samples are sent as 32-bit.
> 
> We are now starting to see users whose I2S blocks send at 64·fs for
> 16-bit samples, which means TDA998x now breaks.
> 
> ASoC has a snd_soc_dai_set_bclk_ratio() call available which sets the
> ratio of BCLK to the sample rate.  Implement support for this.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Works at least on Beaglebone-black.

Tested-by: Jyri Sarha <jsarha@ti.com>
Reviewed-by: Jyri Sarha <jsarha@ti.com>

> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 20 +++++++++++++-------
>  include/drm/i2c/tda998x.h         |  1 +
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 645d884fb9e8..f2d40235acf9 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -895,21 +895,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;
> @@ -997,6 +1002,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 = {
> +		.bclk_ratio = daifmt->bclk_ratio,
>  		.sample_width = params->sample_width,
>  		.sample_rate = params->sample_rate,
>  		.cea = params->cea,
> 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;
> 


-- 
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: [PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Russell King - ARM Linux admin]

On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
> On 22/02/2019 23:27, Russell King wrote:
> > Some HDMI codecs need to know the relationship between the I2S bit clock
> > and the I2S word clock in order to correctly generate the CTS value for
> > audio clock recovery on the sink.
> > 
> > Add support for this, but there are currently no callers of
> > snd_soc_dai_set_bclk_ratio(), we provide a default implementation that
> > uses the sample width to derive the ratio from the 8-bit aligned
> > sample size.  This reflects the derivation that is in TDA998x, which
> > we are going to convert to use this new support.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  include/sound/hdmi-codec.h    |  1 +
> >  sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 44 insertions(+), 2 deletions(-)
> > 
> > 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 e5b6769b9797..d71a7e5a2231 100644
> > --- a/sound/soc/codecs/hdmi-codec.c
> > +++ b/sound/soc/codecs/hdmi-codec.c
> > @@ -470,6 +470,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
> >  				struct snd_soc_dai *dai)
> >  {
> >  	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
> > +	struct hdmi_codec_daifmt fmt;
> >  	struct hdmi_codec_params hp = {
> >  		.iec = {
> >  			.status = { 0 },
> > @@ -520,8 +521,43 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
> >  	hp.sample_rate = params_rate(params);
> >  	hp.channels = params_channels(params);
> >  
> > +	fmt = hcp->daifmt[dai->id];
> > +
> > +	/*
> > +	 * If the .set_bclk_ratio() has not been called, default it
> > +	 * using the sample width for compatibility for TDA998x.
> > +	 * Rather than changing this, drivers should arrange to make
> > +	 * an appropriate call to snd_soc_dai_set_bclk_ratio().
> > +	 */
> > +	if (fmt.bclk_ratio == 0) {
> > +		switch (hp.sample_width) {
> > +		case 16:
> > +			fmt.bclk_ratio = 32;
> > +			break;
> > +		case 18:
> > +		case 20:
> > +		case 24:
> > +			fmt.bclk_ratio = 48;
> > +			break;
> 
> AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually,
> the bclk_ratio is set to the exact frame length required by the sample
> width without any padding. That is at least the case with
> tlv320aic3x-driver and 20-bit sample width.

As mentioned in the commit message, this is what TDA998x does today,
and I have to assume that the contributor tested this, who seems to
be one Jyri Sarha in commit 95db3b255fde ("drm/i2c: tda998x: Improve
tda998x_configure_audio() audio related pdata").

If we don't do it this way, then converting TDA998x to use this
defaulting would change the already established behaviour.  As there
are no users of this by hdmi-codec implementations, and as there are
no callers of snd_soc_dai_set_bclk_ratio(), the only way to maintain
the current behaviour in TDA998x is to either place a default in
hdmi-codec.c, or to have hdmi-codec.c pass zero into TDA998x and have
that encode the above.

I would much rather every user of hdmi-codec gained support for
snd_soc_dai_set_bclk_ratio() rather than relying on that default, but
that is beyond what I can do - I don't have the knowledge of which
sound setups would need it, and I don't have any platforms that are
configured to use I2S with TDA998x.

The Dove Cubox has spdif and i2s but is setup to use spdif (since
that is the most flexible, supporting compressed audio.)  I've a large
pile of unsubmittable patches there which makes kirkwood use DPCM, as
they would end up breaking all the existing users, and from what I can
see there's no way around that.  That makes submitting a patch to add
snd_soc_dai_set_bclk_ratio() very difficult.  That said, the above
defaults are not correct for kirkwood-i2s - that always uses bclk
running at 64fs, as per the TDA998x code prior to your patch I mention
above.

ARM Juno has two TDA998x, but the DT description lacks any audio
description, so it's not clear whether these are even wired.

The ARM MPS platform has a TDA998x connected to a HDLCD, but the HDLCD
does not have an interrupt - and the DRM driver requires an interrupt.
The conclusion that was come to there is basically "don't even bother".
Also unknown whether audio is wired up.

-- 
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 RFC 1/3] drm/i2c: tda998x: implement different I2S flavours

[Sven Van Asbroeck]

On Fri, Feb 22, 2019 at 4:27 PM Russell King <rmk+kernel@armlinux.org.uk> wrote:
>
> Add support for the left and right justified I2S formats as well as the
> more tranditional "Philips" I2S format.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

My imx6q ssi has i2s and left-justified in common with the tda998x.
Left justified
does not appear to work, but I believe this is because of missing support for
left-justified master mode in fsl_ssi ?

So on an imx6q ssi, using i2s philips format only:

Tested-by: Sven Van Asbroeck <TheSven73@gmail.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/3] drm/i2c: tda998x: add support for bclk_ratio

[Sven Van Asbroeck]

On Fri, Feb 22, 2019 at 4:27 PM Russell King <rmk+kernel@armlinux.org.uk> wrote:
>
> It appears that TDA998x derives the CTS value using the supplied I2S
> bit clock (BCLK) rather than 128·fs.  TDA998x uses two constants named
> m and k in the CTS generator such that we have this relationship
> between the source BCLK and the sink fs:
>
>         128·fs_sink = BCLK·m / k
>
> Where BCLK = bclk_ratio·fs_source.
>
> We have been lucky up to now that all users have scaled their bclk_ratio
> to match the sample width - for example, on Beagle Bone Black, with a
> 16-bit sample width, BCLK = 32·fs, which increases to 64·fs for 32-bit
> samples.  24-bit samples are sent as 32-bit.
>
> We are now starting to see users whose I2S blocks send at 64·fs for
> 16-bit samples, which means TDA998x now breaks.
>
> ASoC has a snd_soc_dai_set_bclk_ratio() call available which sets the
> ratio of BCLK to the sample rate.  Implement support for this.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Works with an imx6q ssi, but only if the card driver calls
set_bclk_ratio() with the correct value, which is 32/channel
for the fsl_ssi in master mode.

I will post an RFC patch shortly which adds bclk-ratio support
to simple-card.

Tested-by: Sven Van Asbroeck <TheSven73@gmail.com>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Jyri Sarha]

On 25/02/2019 16:03, Russell King - ARM Linux admin wrote:
> On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
>> On 22/02/2019 23:27, Russell King wrote:
>>> Some HDMI codecs need to know the relationship between the I2S bit clock
>>> and the I2S word clock in order to correctly generate the CTS value for
>>> audio clock recovery on the sink.
>>>
>>> Add support for this, but there are currently no callers of
>>> snd_soc_dai_set_bclk_ratio(), we provide a default implementation that
>>> uses the sample width to derive the ratio from the 8-bit aligned
>>> sample size.  This reflects the derivation that is in TDA998x, which
>>> we are going to convert to use this new support.
>>>
>>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>>> ---
>>>  include/sound/hdmi-codec.h    |  1 +
>>>  sound/soc/codecs/hdmi-codec.c | 45 +++++++++++++++++++++++++++++++++++++++++--
>>>  2 files changed, 44 insertions(+), 2 deletions(-)
>>>
>>> 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 e5b6769b9797..d71a7e5a2231 100644
>>> --- a/sound/soc/codecs/hdmi-codec.c
>>> +++ b/sound/soc/codecs/hdmi-codec.c
>>> @@ -470,6 +470,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
>>>  				struct snd_soc_dai *dai)
>>>  {
>>>  	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
>>> +	struct hdmi_codec_daifmt fmt;
>>>  	struct hdmi_codec_params hp = {
>>>  		.iec = {
>>>  			.status = { 0 },
>>> @@ -520,8 +521,43 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
>>>  	hp.sample_rate = params_rate(params);
>>>  	hp.channels = params_channels(params);
>>>  
>>> +	fmt = hcp->daifmt[dai->id];
>>> +
>>> +	/*
>>> +	 * If the .set_bclk_ratio() has not been called, default it
>>> +	 * using the sample width for compatibility for TDA998x.
>>> +	 * Rather than changing this, drivers should arrange to make
>>> +	 * an appropriate call to snd_soc_dai_set_bclk_ratio().
>>> +	 */
>>> +	if (fmt.bclk_ratio == 0) {
>>> +		switch (hp.sample_width) {
>>> +		case 16:
>>> +			fmt.bclk_ratio = 32;
>>> +			break;
>>> +		case 18:
>>> +		case 20:
>>> +		case 24:
>>> +			fmt.bclk_ratio = 48;
>>> +			break;
>>
>> AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually,
>> the bclk_ratio is set to the exact frame length required by the sample
>> width without any padding. That is at least the case with
>> tlv320aic3x-driver and 20-bit sample width.
> 
> As mentioned in the commit message, this is what TDA998x does today,
> and I have to assume that the contributor tested this, who seems to
> be one Jyri Sarha in commit 95db3b255fde ("drm/i2c: tda998x: Improve
> tda998x_configure_audio() audio related pdata").
> 

Yes, my original implementation was a bit optimistic. I only had limited
information about the chip and a somewhat working undocumented hackish
driver implementation. By now it's clear that rejecting anything but
16-, 24-, or 32-bit sample widths (32, 48, or 64 bclk ratios) would have
been right way to go.

> If we don't do it this way, then converting TDA998x to use this
> defaulting would change the already established behaviour.  As there
> are no users of this by hdmi-codec implementations, and as there are
> no callers of snd_soc_dai_set_bclk_ratio(), the only way to maintain
> the current behaviour in TDA998x is to either place a default in
> hdmi-codec.c, or to have hdmi-codec.c pass zero into TDA998x and have
> that encode the above.
> 

I think there are other options too. The obvious one would be passing
the blck_ratio callback trough HDMI-codec to HDMI-bridge-driver, but
yes, I do not like that either.

The other would be changing the implicit bclk_ratio to 2 x sample-width,
and accepting blck_ratios of 36 and 40 in tda998x, and set CTS_N_M = 3
and CTS_N_K = 2 for them too.

This way my bad choices would not spread to all hdmi-codec users.

Then again, I don't think anyone is using 18- or 20-bit samples with
tda998x. They most likely do not even work. So simply refusing the 36
and 40 blck_ratios would probably be just fine too.

> I would much rather every user of hdmi-codec gained support for
> snd_soc_dai_set_bclk_ratio() rather than relying on that default, but
> that is beyond what I can do - I don't have the knowledge of which
> sound setups would need it, and I don't have any platforms that are
> configured to use I2S with TDA998x.
> 

The little that I know how ASoC is generally used, it is no wonder that
so few drivers have implemented it. In 99% of the cases that I have
encountered the bclk_ratio is sample_width*channels. I cases where
something else is needed the blck_ratio is not the only missing piece.

> The Dove Cubox has spdif and i2s but is setup to use spdif (since
> that is the most flexible, supporting compressed audio.)  I've a large
> pile of unsubmittable patches there which makes kirkwood use DPCM, as
> they would end up breaking all the existing users, and from what I can
> see there's no way around that.  That makes submitting a patch to add
> snd_soc_dai_set_bclk_ratio() very difficult.  That said, the above
> defaults are not correct for kirkwood-i2s - that always uses bclk
> running at 64fs, as per the TDA998x code prior to your patch I mention
> above.
> 
> ARM Juno has two TDA998x, but the DT description lacks any audio
> description, so it's not clear whether these are even wired.
> 
> The ARM MPS platform has a TDA998x connected to a HDLCD, but the HDLCD
> does not have an interrupt - and the DRM driver requires an interrupt.
> The conclusion that was come to there is basically "don't even bother".
> Also unknown whether audio is wired up.
> 


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

Re: [PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Russell King - ARM Linux admin]

On Mon, Feb 25, 2019 at 10:58:34PM +0200, Jyri Sarha wrote:
> On 25/02/2019 16:03, Russell King - ARM Linux admin wrote:
> > As mentioned in the commit message, this is what TDA998x does today,
> > and I have to assume that the contributor tested this, who seems to
> > be one Jyri Sarha in commit 95db3b255fde ("drm/i2c: tda998x: Improve
> > tda998x_configure_audio() audio related pdata").
> 
> Yes, my original implementation was a bit optimistic. I only had limited
> information about the chip and a somewhat working undocumented hackish
> driver implementation. By now it's clear that rejecting anything but
> 16-, 24-, or 32-bit sample widths (32, 48, or 64 bclk ratios) would have
> been right way to go.
> 
> > If we don't do it this way, then converting TDA998x to use this
> > defaulting would change the already established behaviour.  As there
> > are no users of this by hdmi-codec implementations, and as there are
> > no callers of snd_soc_dai_set_bclk_ratio(), the only way to maintain
> > the current behaviour in TDA998x is to either place a default in
> > hdmi-codec.c, or to have hdmi-codec.c pass zero into TDA998x and have
> > that encode the above.
> 
> I think there are other options too. The obvious one would be passing
> the blck_ratio callback trough HDMI-codec to HDMI-bridge-driver, but
> yes, I do not like that either.
> 
> The other would be changing the implicit bclk_ratio to 2 x sample-width,
> and accepting blck_ratios of 36 and 40 in tda998x, and set CTS_N_M = 3
> and CTS_N_K = 2 for them too.
> 
> This way my bad choices would not spread to all hdmi-codec users.
> 
> Then again, I don't think anyone is using 18- or 20-bit samples with
> tda998x. They most likely do not even work. So simply refusing the 36
> and 40 blck_ratios would probably be just fine too.

Another would be keepign the existing code with an additional
WARN_ON_ONCE(1) if invoked, which would have the effect of encouraging
drivers to be fixed up to call snd_soc_dai_set_bclk_ratio() - which has
surely to be a good thing?  However, the risk is that no one reports
the problem cases...

-- 
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 RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Russell King - ARM Linux admin]

On Mon, Feb 25, 2019 at 10:58:34PM +0200, Jyri Sarha wrote:
> I think there are other options too. The obvious one would be passing
> the blck_ratio callback trough HDMI-codec to HDMI-bridge-driver, but
> yes, I do not like that either.
> 
> The other would be changing the implicit bclk_ratio to 2 x sample-width,
> and accepting blck_ratios of 36 and 40 in tda998x, and set CTS_N_M = 3
> and CTS_N_K = 2 for them too.

To put a bit of further information out there...

I really don't like that idea - what if we have a transmitter that
really does use a bclk_ratio of 36 or 40?  That would mess up the
CTS calculation in the TDA998x.

The equation I've come up to fit what we know for TDA998x is:

	k = m * bclk_ratio / 128

where k and m are parameters that we values we select for TDA998x.
This reflects the entire clock regeneration system including the sink.
The possible values of k are 1, 2, 3, 4, or 8.  m are 1, 2, 4, or 8.
I also have this equation which defines the fs regenerated at the sink:

	fso = bclk_ratio * fsi * m / (128 * k)

If we did have a ratio of 36 or 40, the first equation above fails
since k is not one of the possible integers.  If we round down and use
e.g. 2 for the 36fs case, then we'd actually end up with a sample
frequency on the sink of 1.125x faster than it should be, and the sink
would suffer starvation of audio samples.  If we rounded up to 3, then
the sample frequency will be 3/4 of it's true value, so the sink will
overflow and would have to discard audio samples.

We know this happens, because that's the root of the problem at hand:
wrongly setting the m and k values for the bclk_ratio being supplied
to the TDA998x causes audio to be corrupted when reproduced by the
sink.

Given that, we do need some way to validate the bclk_ratio when it is
set, and not during hw_params which would (a) lead to ALSA devices
failing when userspace is negotiating the parameters and (b) gives no
way in the kernel for set_bclk_ratio() to discover whether a particular
ratio is supported by the codec.

So, I think there's three possible ways around this:
1. adding a set_bclk_ratio() method to hdmi_codec_ops
2. calling hw_params() when our set_bclk_ratio() method is called
   (what if the rest of the format isn't set or incompatible, which
   may cause the hw_params() method to fail?)
3. adding a list of acceptable bclk_ratio values to hdmi_codec_pdata

-- 
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 RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Jyri Sarha]

On 27/02/2019 13:47, Russell King - ARM Linux admin wrote:
> On Mon, Feb 25, 2019 at 10:58:34PM +0200, Jyri Sarha wrote:
>> I think there are other options too. The obvious one would be passing
>> the blck_ratio callback trough HDMI-codec to HDMI-bridge-driver, but
>> yes, I do not like that either.
>>
>> The other would be changing the implicit bclk_ratio to 2 x sample-width,
>> and accepting blck_ratios of 36 and 40 in tda998x, and set CTS_N_M = 3
>> and CTS_N_K = 2 for them too.
> To put a bit of further information out there...
> 
> I really don't like that idea - what if we have a transmitter that
> really does use a bclk_ratio of 36 or 40?  That would mess up the
> CTS calculation in the TDA998x.
> 
> The equation I've come up to fit what we know for TDA998x is:
> 
> 	k = m * bclk_ratio / 128
> 
> where k and m are parameters that we values we select for TDA998x.
> This reflects the entire clock regeneration system including the sink.
> The possible values of k are 1, 2, 3, 4, or 8.  m are 1, 2, 4, or 8.
> I also have this equation which defines the fs regenerated at the sink:
> 
> 	fso = bclk_ratio * fsi * m / (128 * k)
> 
> If we did have a ratio of 36 or 40, the first equation above fails
> since k is not one of the possible integers.  If we round down and use
> e.g. 2 for the 36fs case, then we'd actually end up with a sample
> frequency on the sink of 1.125x faster than it should be, and the sink
> would suffer starvation of audio samples.  If we rounded up to 3, then
> the sample frequency will be 3/4 of it's true value, so the sink will
> overflow and would have to discard audio samples.
> 
> We know this happens, because that's the root of the problem at hand:
> wrongly setting the m and k values for the bclk_ratio being supplied
> to the TDA998x causes audio to be corrupted when reproduced by the
> sink.
> 
> Given that, we do need some way to validate the bclk_ratio when it is
> set, and not during hw_params which would (a) lead to ALSA devices
> failing when userspace is negotiating the parameters and (b) gives no
> way in the kernel for set_bclk_ratio() to discover whether a particular
> ratio is supported by the codec.
> 
> So, I think there's three possible ways around this:
> 1. adding a set_bclk_ratio() method to hdmi_codec_ops
> 2. calling hw_params() when our set_bclk_ratio() method is called
>    (what if the rest of the format isn't set or incompatible, which
>    may cause the hw_params() method to fail?)
> 3. adding a list of acceptable bclk_ratio values to hdmi_codec_pdata

Or leaving the bclk_ratio field zero in struct hdmi_codec_params if
set_bclk_ratio() is not called and leaving the bridge driver to decide
what to do in such a situation.

And in tda998x_audio_hw_params() doing something like this:

if (audio.bclk_ratio == 0)
	audio.bclk_ratio = DIV_ROUND_UP(params->sample_width, 8) * 8;

But then again I think it would be quite sane option to set bclk_ration
in hdmi-codec to 2*sample_width and simply refuse the ratios of 36 or 40
in tda998x.

Best regards,
Jyri


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

Re: [PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Russell King - ARM Linux admin]

On Wed, Feb 27, 2019 at 07:48:33PM +0200, Jyri Sarha wrote:
> On 27/02/2019 13:47, Russell King - ARM Linux admin wrote:
> > On Mon, Feb 25, 2019 at 10:58:34PM +0200, Jyri Sarha wrote:
> >> I think there are other options too. The obvious one would be passing
> >> the blck_ratio callback trough HDMI-codec to HDMI-bridge-driver, but
> >> yes, I do not like that either.
> >>
> >> The other would be changing the implicit bclk_ratio to 2 x sample-width,
> >> and accepting blck_ratios of 36 and 40 in tda998x, and set CTS_N_M = 3
> >> and CTS_N_K = 2 for them too.
> > To put a bit of further information out there...
> > 
> > I really don't like that idea - what if we have a transmitter that
> > really does use a bclk_ratio of 36 or 40?  That would mess up the
> > CTS calculation in the TDA998x.
> > 
> > The equation I've come up to fit what we know for TDA998x is:
> > 
> > 	k = m * bclk_ratio / 128
> > 
> > where k and m are parameters that we values we select for TDA998x.
> > This reflects the entire clock regeneration system including the sink.
> > The possible values of k are 1, 2, 3, 4, or 8.  m are 1, 2, 4, or 8.
> > I also have this equation which defines the fs regenerated at the sink:
> > 
> > 	fso = bclk_ratio * fsi * m / (128 * k)
> > 
> > If we did have a ratio of 36 or 40, the first equation above fails
> > since k is not one of the possible integers.  If we round down and use
> > e.g. 2 for the 36fs case, then we'd actually end up with a sample
> > frequency on the sink of 1.125x faster than it should be, and the sink
> > would suffer starvation of audio samples.  If we rounded up to 3, then
> > the sample frequency will be 3/4 of it's true value, so the sink will
> > overflow and would have to discard audio samples.
> > 
> > We know this happens, because that's the root of the problem at hand:
> > wrongly setting the m and k values for the bclk_ratio being supplied
> > to the TDA998x causes audio to be corrupted when reproduced by the
> > sink.
> > 
> > Given that, we do need some way to validate the bclk_ratio when it is
> > set, and not during hw_params which would (a) lead to ALSA devices
> > failing when userspace is negotiating the parameters and (b) gives no
> > way in the kernel for set_bclk_ratio() to discover whether a particular
> > ratio is supported by the codec.
> > 
> > So, I think there's three possible ways around this:
> > 1. adding a set_bclk_ratio() method to hdmi_codec_ops
> > 2. calling hw_params() when our set_bclk_ratio() method is called
> >    (what if the rest of the format isn't set or incompatible, which
> >    may cause the hw_params() method to fail?)
> > 3. adding a list of acceptable bclk_ratio values to hdmi_codec_pdata
> 
> Or leaving the bclk_ratio field zero in struct hdmi_codec_params if
> set_bclk_ratio() is not called and leaving the bridge driver to decide
> what to do in such a situation.
> 
> And in tda998x_audio_hw_params() doing something like this:
> 
> if (audio.bclk_ratio == 0)
> 	audio.bclk_ratio = DIV_ROUND_UP(params->sample_width, 8) * 8;
> 
> But then again I think it would be quite sane option to set bclk_ration
> in hdmi-codec to 2*sample_width and simply refuse the ratios of 36 or 40
> in tda998x.

... which then leads to breakage of userspace if 18 or 20 bit formats
were attempted, making the bridge driver's capabilities undiscoverable.

Returning an error from hw_params causes hard failures.

-- 
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 RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Sven Van Asbroeck]

On Wed, Feb 27, 2019 at 6:47 AM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> Given that, we do need some way to validate the bclk_ratio when it is
> set, and not during hw_params which would (a) lead to ALSA devices
> failing when userspace is negotiating the parameters and (b) gives no
> way in the kernel for set_bclk_ratio() to discover whether a particular
> ratio is supported by the codec.
>
> So, I think there's three possible ways around this:
> 1. adding a set_bclk_ratio() method to hdmi_codec_ops
> 2. calling hw_params() when our set_bclk_ratio() method is called
>    (what if the rest of the format isn't set or incompatible, which
>    may cause the hw_params() method to fail?)
> 3. adding a list of acceptable bclk_ratio values to hdmi_codec_pdata
>

Again excuse my obvious ignorance, but would these solutions work well in the
more general case?

Imagine a cpu dai that supports 16x2/16x2, 20x2/20x2, 24x2/24x2
(sample bits/frame bits -- wire format), sending audio to our tda998x
hdmi xmitter.
Depending on the type of samples that userspace chooses to play, one of the
above formats gets selected by the ASoC core, resulting in a bclk_ratio of
16x2, 20x2 or 32x2. It's up to the card driver to call set_bclk_ratio(), right?
So now this card driver needs intimate knowledge of bclk_ratios vs formats for
the cpu dai. Also it needs knowledge of which bclk_ratios are supported by the
hdmi xmitter, and a mechanism to filter our the 20x2 blk_ratio format.
Which may not be trivial, and also prevents it from being generic,
i.e. we can no longer use simple-card ?

But it gets worse. Imagine a hypothetical cpu dai that supports 20x2/20x2 and
20x2/24x2. When the dai is sending to a codec that doesn't care about
bclk_ratio,
it should pick 20x2/20x2, because that's most efficient, right? Except
on a tda998x
it should select 20x2/24x2. So how would a card driver now even begin to
deal with this, given that there appears to be no mechanism to even describe
these differences? Because the params_physical_width() describes the memory
format, and not the wire format, correct?

So all this kind of suggests to me that the bclk_ratio could be part of the
format description, or something?

static struct snd_soc_dai_driver acme_cpu_dai = {
        .playback = {
                .formats = SNDRV_PCM_FMTBIT_S20_3LE_20 |
                                SNDRV_PCM_FMTBIT_S20_3LE_24,
                                SNDRV_PCM_FMTBIT_S16_LE | //
bclk_ratio 16 implied
                                SNDRV_PCM_FMTBIT_S24_LE | //
bclk_ratio 24 implied
                                SNDRV_PCM_FMTBIT_S24_LE_32
        },
};

static struct snd_soc_dai_driver hdmi_dai = {
        .playback = {
                .formats = SNDRV_PCM_FMTBIT_S20_3LE_24 |
                                SNDRV_PCM_FMTBIT_S16_LE | //
bclk_ratio 16 implied
                                SNDRV_PCM_FMTBIT_S24_LE | //
bclk_ratio 24 implied
                                SNDRV_PCM_FMTBIT_S24_LE_32,
        },
};

In this case, the capabilities get negotiated correctly, and the tda998x's
hw_params() could just call params_bclk_ratio(params) to get the ratio, right?

And the fsl_ssi could resort to a rule to filter out all non-32x2 bclk_ratio
formats only in master mode.

As I said, I'm way out of my depth here. No idea how realistic or hypothetical
this is, or if this would go against the grain of the existing ASoC
architecture.

I really hope there's a much better solution than this that will solve the
general case.

Re: [PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Russell King - ARM Linux admin]

On Wed, Feb 27, 2019 at 01:01:05PM -0500, Sven Van Asbroeck wrote:
> On Wed, Feb 27, 2019 at 6:47 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > Given that, we do need some way to validate the bclk_ratio when it is
> > set, and not during hw_params which would (a) lead to ALSA devices
> > failing when userspace is negotiating the parameters and (b) gives no
> > way in the kernel for set_bclk_ratio() to discover whether a particular
> > ratio is supported by the codec.
> >
> > So, I think there's three possible ways around this:
> > 1. adding a set_bclk_ratio() method to hdmi_codec_ops
> > 2. calling hw_params() when our set_bclk_ratio() method is called
> >    (what if the rest of the format isn't set or incompatible, which
> >    may cause the hw_params() method to fail?)
> > 3. adding a list of acceptable bclk_ratio values to hdmi_codec_pdata
> >
> 
> Again excuse my obvious ignorance, but would these solutions work well in the
> more general case?
> 
> Imagine a cpu dai that supports 16x2/16x2, 20x2/20x2, 24x2/24x2
> (sample bits/frame bits -- wire format), sending audio to our tda998x
> hdmi xmitter.
> Depending on the type of samples that userspace chooses to play, one of the
> above formats gets selected by the ASoC core, resulting in a bclk_ratio of
> 16x2, 20x2 or 32x2. It's up to the card driver to call set_bclk_ratio(), right?
> So now this card driver needs intimate knowledge of bclk_ratios vs formats for
> the cpu dai. Also it needs knowledge of which bclk_ratios are supported by the
> hdmi xmitter, and a mechanism to filter our the 20x2 blk_ratio format.

That's why I said in a previous email that it would be good if there
was some way that the capabilities of the codec and cpu DAIs were
known to the ASoC core.

> Which may not be trivial, and also prevents it from being generic,
> i.e. we can no longer use simple-card ?

It seems trivial today that we have a complex system called ALSA PCM,
where lots of different parameters are negotiated between the kernel
driver and userspace, which include:

- sample rate
- number of channels
- buffer size
- period size
- frame bits (unfortunately, not on-wire!)
etc.

If you want to see the full list, see the SNDRV_PCM_HW_PARAM_*
definitions in include/uapi/sound/asound.h.

The kernel driver can arbitarily apply rules to these, even inter-
dependent rules - it's possible to specify in kernel space a rule such
as "we can support 48kHz with up to six channels, or 96kHz with two
channels" and ALSA will handle it.  Other rules are possible.

More than that, if we have a PCM device that supports (eg) only 48kHz,
44.1kHz and 32kHz sample rates with two channels, and we attempt to
play a 11.025kHz mono sample, userspace will negotiate one of the
hardware supported formats, and then automatically assemble within
libasound a set of plugins that convert the number of channels to
what the hardware supports, and performs sample rate conversion.

Yet, we seem to be saying that we can't solve the problem of the
sample-rate to bitclock ratio.

I suspect we could do using this infrastructure...

> But it gets worse. Imagine a hypothetical cpu dai that supports 20x2/20x2 and
> 20x2/24x2. When the dai is sending to a codec that doesn't care about
> bclk_ratio,
> it should pick 20x2/20x2, because that's most efficient, right? Except
> on a tda998x
> it should select 20x2/24x2. So how would a card driver now even begin to
> deal with this, given that there appears to be no mechanism to even describe
> these differences? Because the params_physical_width() describes the memory
> format, and not the wire format, correct?

If we were able to describe the on-wire frame size to ALSA's constraint
resolution core, I bet it can resolve this for us.

To take the above, for I2S, TDA998x would add a rules to ALSA saying:

1. "I support 2*N channels" where 1 < N < number of I2S inputs.
2. "I support sample widths from 16 to 24 bits"
3. "I support on-wire frame bits 32, 48, 64, 128"
4. "Depending on the sample width, I support <limited-from-above>
   on-wire frame bits".

We now have TDA998x's parameters described to the ALSA core.

The CPU on the other hand would say to the ALSA core (e.g.):

1. "I support 1-8 channels"
2. "I support sample widths from 8 to 32 bits"
3. "I support on-wire frame bits only of 64"

During ALSAs hardware parameter refining, this would result in ALSA
settling on a frame bits of 64 for everything.

If, on the other hand, we had a CPU DAI specifying these rules:

1. "I support 1-8 channels"
2. "I support sample widths from 8 to 32 bits"
3. "Depending on the sample width, I support on-wire frame bits only
   of sample width * 2"

Then ALSA if you ask for 16 bit samples, it would end up settling on
a frame bits of 32, for 24 bit, 48.  For the others, it would notice
that it can't do a sample width of 18 or 20 bits, and would probably
decide upon 24 bit.

And... userspace would automatically pick up a plugin to convert to
24 bit sample width (provided its using one of libasound's plughw
devices rather than the raw "no conversion" devices.)

Now, the problem is... there doesn't seem to be an existing hardware
parameter for the on-wire frame bits - it isn't
SNDRV_PCM_HW_PARAM_FRAME_BITS - sound/core/pcm_native.c already has
rules for this parameter that lock it to sample_bits * channels, and
it's used to determine the period bytes from the period size etc.
However, if you look down the list of rules in
snd_pcm_hw_constraints_init(), you'll see that these kinds of
relationships can be described to ALSA's contraint resolution core.

It does look like struct snd_pcm_hw_params has some space to be able
to extend the intervals, but I'm guessing that isn't going to be easy
given that userspace won't know about the new interval, although I'm
not sure whether that really matters for this case.  However, I can
see that there will be objections to exposing such a parameter to
userspace.

Another problem is hdmi-codec trying to abstract ALSA and ASoC, and
hide it from the actual HDMI bridge.  It means bridges have no access
to many of the ALSA facilities, and basically have to "do what they're
told or fail" causing a hard-failure in ALSA.

Rather than the nice "lets refine the hardware parameters and then
insitute whatever plugins are necessary" we get instead something
like this (for example, using a slightly hacked driver to show what
happens if we error out in hdmi_codec_hw_params()):

$ aplay -Dplughw:0,0 /usr/lib/libreoffice/share/gallery/sounds/theetone.wav
Playing WAVE '/usr/lib/libreoffice/share/gallery/sounds/theetone.wav' :
Signed 16 bit Little Endian, Rate 11025 Hz, Mono
aplay: set_params:1297: Unable to install hw params:
ACCESS:  RW_INTERLEAVED
FORMAT:  S16_LE
SUBFORMAT:  STD
SAMPLE_BITS: 16
FRAME_BITS: 16
CHANNELS: 1
RATE: 11025
PERIOD_TIME: (127709 127710)
PERIOD_SIZE: 1408
PERIOD_BYTES: 2816
PERIODS: (3 4)
BUFFER_TIME: (499229 499230)
BUFFER_SIZE: 5504
BUFFER_BYTES: 11008
TICK_TIME: 0

which is showing what userspace does when hdmi_codec_hw_params()
returns -EINVAL - ALSA just gives up.  It doesn't even bother trying
any of its format and sample rate conversion which _is_ available
via the "plughw" device.  If it knew ahead of time (iow, during the
constraint refining) then it would've used plugins to convert from
(e.g.) 11.025kHz mono to 48kHz stereo.  By way of illustration, here's
the difference on x86, HDA-Intel:

Playing WAVE '/usr/lib/libreoffice/share/gallery/sounds/theetone.wav' : Signed 16 bit Little Endian, Rate 11025 Hz, Mono
Plug PCM: Rate conversion PCM (44100, sformat=S16_LE)
Converter: linear-interpolation
Protocol version: 10002
Its setup is:
  stream       : PLAYBACK
  access       : RW_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 1
  rate         : 11025
  exact rate   : 11025 (11025/1)
  msbits       : 16
  buffer_size  : 4096
  period_size  : 1024
  period_time  : 92879
  tstamp_mode  : NONE
  period_step  : 1
  avail_min    : 1024
  period_event : 0
  start_threshold  : 4096
  stop_threshold   : 4096
  silence_threshold: 0
  silence_size : 0
  boundary     : 268435456
Slave: Route conversion PCM (sformat=S16_LE)
  Transformation table:
    0 <- 0
    1 <- 0
Its setup is:
  stream       : PLAYBACK
  access       : MMAP_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 1
  rate         : 44100
  exact rate   : 44100 (44100/1)
  msbits       : 16
  buffer_size  : 16384
  period_size  : 4096
  period_time  : 92879
  tstamp_mode  : NONE
  period_step  : 1
  avail_min    : 4096
  period_event : 0
  start_threshold  : 16384
  stop_threshold   : 16384
  silence_threshold: 0
  silence_size : 0
  boundary     : 1073741824
Slave: Hardware PCM card 0 'HDA Intel' device 0 subdevice 0
Its setup is:
  stream       : PLAYBACK
  access       : MMAP_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 2
  rate         : 44100
  exact rate   : 44100 (44100/1)
  msbits       : 16
  buffer_size  : 16384
  period_size  : 4096
  period_time  : 92879
  tstamp_mode  : NONE
  period_step  : 1
  avail_min    : 4096
  period_event : 0
  start_threshold  : 16384
  stop_threshold   : 16384
  silence_threshold: 0
  silence_size : 0
  boundary     : 1073741824
  appl_ptr     : 0
  hw_ptr       : 0

And there you have the hardware running at 44.1kHz stereo, playing
the very same 11.025kHz mono wav file with userspace plugins
clearly automatically picked up.

> So all this kind of suggests to me that the bclk_ratio could be part of the
> format description, or something?
> 
> static struct snd_soc_dai_driver acme_cpu_dai = {
>         .playback = {
>                 .formats = SNDRV_PCM_FMTBIT_S20_3LE_20 |
>                                 SNDRV_PCM_FMTBIT_S20_3LE_24,
>                                 SNDRV_PCM_FMTBIT_S16_LE | //
> bclk_ratio 16 implied
>                                 SNDRV_PCM_FMTBIT_S24_LE | //
> bclk_ratio 24 implied
>                                 SNDRV_PCM_FMTBIT_S24_LE_32

I don't think this is going to work.  Firstly, it'll break userspace,
becuase userspace would need to be taught about all these new format
identifiers.  Secondly, it massively increases the number of formats
to number_of_existing_formats * number_of_possible_bclk_ratios.

-- 
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 RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Sven Van Asbroeck]

Russell, thank you for the amazing explanation, and for your continued patience.

On Wed, Feb 27, 2019 at 2:56 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> Now, the problem is... there doesn't seem to be an existing hardware
> parameter for the on-wire frame bits

Could a case be made to make the resolution core "bclk_ratio" aware ?
Of course if this is just to support tda998x, it would be like a cannon to
kill a fly...

>
> > So all this kind of suggests to me that the bclk_ratio could be part of the
> > format description, or something?
> >
> > static struct snd_soc_dai_driver acme_cpu_dai = {
> >         .playback = {
> >                 .formats = SNDRV_PCM_FMTBIT_S20_3LE_20 |
> >                                 SNDRV_PCM_FMTBIT_S20_3LE_24,
> >                                 SNDRV_PCM_FMTBIT_S16_LE | //
> > bclk_ratio 16 implied
> >                                 SNDRV_PCM_FMTBIT_S24_LE | //
> > bclk_ratio 24 implied
> >                                 SNDRV_PCM_FMTBIT_S24_LE_32
>
> I don't think this is going to work.  Firstly, it'll break userspace,
> becuase userspace would need to be taught about all these new format
> identifiers.  Secondly, it massively increases the number of formats
> to number_of_existing_formats * number_of_possible_bclk_ratios.

How about

static struct snd_soc_dai_driver acme_dai_template = {
        .playback = {
                .stream_name = "CPU-Playback",
                .channels_min = 1,
                .channels_max = 32,
                .rates = SNDRV_PCM_RATE_CONTINUOUS,
                .formats = FSLSSI_I2S_FORMATS,

                .bclk_ratio_min = 8,
                .bclk_ratio_max = 64,
                or
                .bclk_ratios = RATIO_8 | RATIO_16 | RATIO_32 | RATIO_64,
        },
};

Then if codec/cpu cares about bclk_ratios, it would have to put rules in place
such as:
- SNDRV_PCM_FMTBIT_S16_LE -> bclk_ratio = 16x2 only
- SNDRV_PCM_FMTBIT_S24_LE -> slave : bclk_ratio = [ 24x2 to 32x2 ]
                          -> master: bclk_ratio = 32x2 only

Re: [PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Russell King - ARM Linux admin]

On Wed, Feb 27, 2019 at 07:56:00PM +0000, Russell King - ARM Linux admin wrote:
> On Wed, Feb 27, 2019 at 01:01:05PM -0500, Sven Van Asbroeck wrote:
> > On Wed, Feb 27, 2019 at 6:47 AM Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > >
> > > Given that, we do need some way to validate the bclk_ratio when it is
> > > set, and not during hw_params which would (a) lead to ALSA devices
> > > failing when userspace is negotiating the parameters and (b) gives no
> > > way in the kernel for set_bclk_ratio() to discover whether a particular
> > > ratio is supported by the codec.
> > >
> > > So, I think there's three possible ways around this:
> > > 1. adding a set_bclk_ratio() method to hdmi_codec_ops
> > > 2. calling hw_params() when our set_bclk_ratio() method is called
> > >    (what if the rest of the format isn't set or incompatible, which
> > >    may cause the hw_params() method to fail?)
> > > 3. adding a list of acceptable bclk_ratio values to hdmi_codec_pdata
> > >
> > 
> > Again excuse my obvious ignorance, but would these solutions work well in the
> > more general case?
> > 
> > Imagine a cpu dai that supports 16x2/16x2, 20x2/20x2, 24x2/24x2
> > (sample bits/frame bits -- wire format), sending audio to our tda998x
> > hdmi xmitter.
> > Depending on the type of samples that userspace chooses to play, one of the
> > above formats gets selected by the ASoC core, resulting in a bclk_ratio of
> > 16x2, 20x2 or 32x2. It's up to the card driver to call set_bclk_ratio(), right?
> > So now this card driver needs intimate knowledge of bclk_ratios vs formats for
> > the cpu dai. Also it needs knowledge of which bclk_ratios are supported by the
> > hdmi xmitter, and a mechanism to filter our the 20x2 blk_ratio format.
> 
> That's why I said in a previous email that it would be good if there
> was some way that the capabilities of the codec and cpu DAIs were
> known to the ASoC core.
> 
> > Which may not be trivial, and also prevents it from being generic,
> > i.e. we can no longer use simple-card ?
> 
> It seems trivial today that we have a complex system called ALSA PCM,
> where lots of different parameters are negotiated between the kernel
> driver and userspace, which include:
> 
> - sample rate
> - number of channels
> - buffer size
> - period size
> - frame bits (unfortunately, not on-wire!)
> etc.
> 
> If you want to see the full list, see the SNDRV_PCM_HW_PARAM_*
> definitions in include/uapi/sound/asound.h.
> 
> The kernel driver can arbitarily apply rules to these, even inter-
> dependent rules - it's possible to specify in kernel space a rule such
> as "we can support 48kHz with up to six channels, or 96kHz with two
> channels" and ALSA will handle it.  Other rules are possible.
> 
> More than that, if we have a PCM device that supports (eg) only 48kHz,
> 44.1kHz and 32kHz sample rates with two channels, and we attempt to
> play a 11.025kHz mono sample, userspace will negotiate one of the
> hardware supported formats, and then automatically assemble within
> libasound a set of plugins that convert the number of channels to
> what the hardware supports, and performs sample rate conversion.
> 
> Yet, we seem to be saying that we can't solve the problem of the
> sample-rate to bitclock ratio.
> 
> I suspect we could do using this infrastructure...
> 
> > But it gets worse. Imagine a hypothetical cpu dai that supports 20x2/20x2 and
> > 20x2/24x2. When the dai is sending to a codec that doesn't care about
> > bclk_ratio,
> > it should pick 20x2/20x2, because that's most efficient, right? Except
> > on a tda998x
> > it should select 20x2/24x2. So how would a card driver now even begin to
> > deal with this, given that there appears to be no mechanism to even describe
> > these differences? Because the params_physical_width() describes the memory
> > format, and not the wire format, correct?
> 
> If we were able to describe the on-wire frame size to ALSA's constraint
> resolution core, I bet it can resolve this for us.
> 
> To take the above, for I2S, TDA998x would add a rules to ALSA saying:
> 
> 1. "I support 2*N channels" where 1 < N < number of I2S inputs.
> 2. "I support sample widths from 16 to 24 bits"
> 3. "I support on-wire frame bits 32, 48, 64, 128"
> 4. "Depending on the sample width, I support <limited-from-above>
>    on-wire frame bits".
> 
> We now have TDA998x's parameters described to the ALSA core.
> 
> The CPU on the other hand would say to the ALSA core (e.g.):
> 
> 1. "I support 1-8 channels"
> 2. "I support sample widths from 8 to 32 bits"
> 3. "I support on-wire frame bits only of 64"
> 
> During ALSAs hardware parameter refining, this would result in ALSA
> settling on a frame bits of 64 for everything.
> 
> If, on the other hand, we had a CPU DAI specifying these rules:
> 
> 1. "I support 1-8 channels"
> 2. "I support sample widths from 8 to 32 bits"
> 3. "Depending on the sample width, I support on-wire frame bits only
>    of sample width * 2"
> 
> Then ALSA if you ask for 16 bit samples, it would end up settling on
> a frame bits of 32, for 24 bit, 48.  For the others, it would notice
> that it can't do a sample width of 18 or 20 bits, and would probably
> decide upon 24 bit.
> 
> And... userspace would automatically pick up a plugin to convert to
> 24 bit sample width (provided its using one of libasound's plughw
> devices rather than the raw "no conversion" devices.)
> 
> Now, the problem is... there doesn't seem to be an existing hardware
> parameter for the on-wire frame bits - it isn't
> SNDRV_PCM_HW_PARAM_FRAME_BITS - sound/core/pcm_native.c already has
> rules for this parameter that lock it to sample_bits * channels, and
> it's used to determine the period bytes from the period size etc.
> However, if you look down the list of rules in
> snd_pcm_hw_constraints_init(), you'll see that these kinds of
> relationships can be described to ALSA's contraint resolution core.
> 
> It does look like struct snd_pcm_hw_params has some space to be able
> to extend the intervals, but I'm guessing that isn't going to be easy
> given that userspace won't know about the new interval, although I'm
> not sure whether that really matters for this case.  However, I can
> see that there will be objections to exposing such a parameter to
> userspace.
> 
> Another problem is hdmi-codec trying to abstract ALSA and ASoC, and
> hide it from the actual HDMI bridge.  It means bridges have no access
> to many of the ALSA facilities, and basically have to "do what they're
> told or fail" causing a hard-failure in ALSA.
> 
> Rather than the nice "lets refine the hardware parameters and then
> insitute whatever plugins are necessary" we get instead something
> like this (for example, using a slightly hacked driver to show what
> happens if we error out in hdmi_codec_hw_params()):
> 
> $ aplay -Dplughw:0,0 /usr/lib/libreoffice/share/gallery/sounds/theetone.wav
> Playing WAVE '/usr/lib/libreoffice/share/gallery/sounds/theetone.wav' :
> Signed 16 bit Little Endian, Rate 11025 Hz, Mono
> aplay: set_params:1297: Unable to install hw params:
> ACCESS:  RW_INTERLEAVED
> FORMAT:  S16_LE
> SUBFORMAT:  STD
> SAMPLE_BITS: 16
> FRAME_BITS: 16
> CHANNELS: 1
> RATE: 11025
> PERIOD_TIME: (127709 127710)
> PERIOD_SIZE: 1408
> PERIOD_BYTES: 2816
> PERIODS: (3 4)
> BUFFER_TIME: (499229 499230)
> BUFFER_SIZE: 5504
> BUFFER_BYTES: 11008
> TICK_TIME: 0
> 
> which is showing what userspace does when hdmi_codec_hw_params()
> returns -EINVAL - ALSA just gives up.  It doesn't even bother trying
> any of its format and sample rate conversion which _is_ available
> via the "plughw" device.  If it knew ahead of time (iow, during the
> constraint refining) then it would've used plugins to convert from
> (e.g.) 11.025kHz mono to 48kHz stereo.  By way of illustration, here's
> the difference on x86, HDA-Intel:
> 
> Playing WAVE '/usr/lib/libreoffice/share/gallery/sounds/theetone.wav' : Signed 16 bit Little Endian, Rate 11025 Hz, Mono
> Plug PCM: Rate conversion PCM (44100, sformat=S16_LE)
> Converter: linear-interpolation
> Protocol version: 10002
> Its setup is:
>   stream       : PLAYBACK
>   access       : RW_INTERLEAVED
>   format       : S16_LE
>   subformat    : STD
>   channels     : 1
>   rate         : 11025
>   exact rate   : 11025 (11025/1)
>   msbits       : 16
>   buffer_size  : 4096
>   period_size  : 1024
>   period_time  : 92879
>   tstamp_mode  : NONE
>   period_step  : 1
>   avail_min    : 1024
>   period_event : 0
>   start_threshold  : 4096
>   stop_threshold   : 4096
>   silence_threshold: 0
>   silence_size : 0
>   boundary     : 268435456
> Slave: Route conversion PCM (sformat=S16_LE)
>   Transformation table:
>     0 <- 0
>     1 <- 0
> Its setup is:
>   stream       : PLAYBACK
>   access       : MMAP_INTERLEAVED
>   format       : S16_LE
>   subformat    : STD
>   channels     : 1
>   rate         : 44100
>   exact rate   : 44100 (44100/1)
>   msbits       : 16
>   buffer_size  : 16384
>   period_size  : 4096
>   period_time  : 92879
>   tstamp_mode  : NONE
>   period_step  : 1
>   avail_min    : 4096
>   period_event : 0
>   start_threshold  : 16384
>   stop_threshold   : 16384
>   silence_threshold: 0
>   silence_size : 0
>   boundary     : 1073741824
> Slave: Hardware PCM card 0 'HDA Intel' device 0 subdevice 0
> Its setup is:
>   stream       : PLAYBACK
>   access       : MMAP_INTERLEAVED
>   format       : S16_LE
>   subformat    : STD
>   channels     : 2
>   rate         : 44100
>   exact rate   : 44100 (44100/1)
>   msbits       : 16
>   buffer_size  : 16384
>   period_size  : 4096
>   period_time  : 92879
>   tstamp_mode  : NONE
>   period_step  : 1
>   avail_min    : 4096
>   period_event : 0
>   start_threshold  : 16384
>   stop_threshold   : 16384
>   silence_threshold: 0
>   silence_size : 0
>   boundary     : 1073741824
>   appl_ptr     : 0
>   hw_ptr       : 0
> 
> And there you have the hardware running at 44.1kHz stereo, playing
> the very same 11.025kHz mono wav file with userspace plugins
> clearly automatically picked up.
> 
> > So all this kind of suggests to me that the bclk_ratio could be part of the
> > format description, or something?
> > 
> > static struct snd_soc_dai_driver acme_cpu_dai = {
> >         .playback = {
> >                 .formats = SNDRV_PCM_FMTBIT_S20_3LE_20 |
> >                                 SNDRV_PCM_FMTBIT_S20_3LE_24,
> >                                 SNDRV_PCM_FMTBIT_S16_LE | //
> > bclk_ratio 16 implied
> >                                 SNDRV_PCM_FMTBIT_S24_LE | //
> > bclk_ratio 24 implied
> >                                 SNDRV_PCM_FMTBIT_S24_LE_32
> 
> I don't think this is going to work.  Firstly, it'll break userspace,
> becuase userspace would need to be taught about all these new format
> identifiers.  Secondly, it massively increases the number of formats
> to number_of_existing_formats * number_of_possible_bclk_ratios.

I'll add that yes, I do think that solving this bclk_ratio issue is
not going to be an easy job, and I am hoping that those who know
ALSA and ASoC better than I will make some suggestions on a way
forward that looks sensible, otherwise I fear that this issue isn't
going to be resolvable.

What I _do_ want to avoid are hard-failures (as illustrated above)
that cause ALSA userspace to basically give up - which results in
audio completely failing to work.

-- 
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 RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Jyri Sarha]

On 27/02/2019 20:00, Russell King - ARM Linux admin wrote:
> On Wed, Feb 27, 2019 at 07:48:33PM +0200, Jyri Sarha wrote:
>> On 27/02/2019 13:47, Russell King - ARM Linux admin wrote:
>>> On Mon, Feb 25, 2019 at 10:58:34PM +0200, Jyri Sarha wrote:
>>>> I think there are other options too. The obvious one would be passing
>>>> the blck_ratio callback trough HDMI-codec to HDMI-bridge-driver, but
>>>> yes, I do not like that either.
>>>>
>>>> The other would be changing the implicit bclk_ratio to 2 x sample-width,
>>>> and accepting blck_ratios of 36 and 40 in tda998x, and set CTS_N_M = 3
>>>> and CTS_N_K = 2 for them too.
>>> To put a bit of further information out there...
>>>
>>> I really don't like that idea - what if we have a transmitter that
>>> really does use a bclk_ratio of 36 or 40?  That would mess up the
>>> CTS calculation in the TDA998x.
>>>
>>> The equation I've come up to fit what we know for TDA998x is:
>>>
>>> 	k = m * bclk_ratio / 128
>>>
>>> where k and m are parameters that we values we select for TDA998x.
>>> This reflects the entire clock regeneration system including the sink.
>>> The possible values of k are 1, 2, 3, 4, or 8.  m are 1, 2, 4, or 8.
>>> I also have this equation which defines the fs regenerated at the sink:
>>>
>>> 	fso = bclk_ratio * fsi * m / (128 * k)
>>>
>>> If we did have a ratio of 36 or 40, the first equation above fails
>>> since k is not one of the possible integers.  If we round down and use
>>> e.g. 2 for the 36fs case, then we'd actually end up with a sample
>>> frequency on the sink of 1.125x faster than it should be, and the sink
>>> would suffer starvation of audio samples.  If we rounded up to 3, then
>>> the sample frequency will be 3/4 of it's true value, so the sink will
>>> overflow and would have to discard audio samples.
>>>
>>> We know this happens, because that's the root of the problem at hand:
>>> wrongly setting the m and k values for the bclk_ratio being supplied
>>> to the TDA998x causes audio to be corrupted when reproduced by the
>>> sink.
>>>
>>> Given that, we do need some way to validate the bclk_ratio when it is
>>> set, and not during hw_params which would (a) lead to ALSA devices
>>> failing when userspace is negotiating the parameters and (b) gives no
>>> way in the kernel for set_bclk_ratio() to discover whether a particular
>>> ratio is supported by the codec.
>>>
>>> So, I think there's three possible ways around this:
>>> 1. adding a set_bclk_ratio() method to hdmi_codec_ops
>>> 2. calling hw_params() when our set_bclk_ratio() method is called
>>>    (what if the rest of the format isn't set or incompatible, which
>>>    may cause the hw_params() method to fail?)
>>> 3. adding a list of acceptable bclk_ratio values to hdmi_codec_pdata
>>

The suggestion below would be my first choice. Packing the same
information that we already have in sample_width to bclk_ratio does not
make too much sense.

>> Or leaving the bclk_ratio field zero in struct hdmi_codec_params if
>> set_bclk_ratio() is not called and leaving the bridge driver to decide
>> what to do in such a situation.
>>
>> And in tda998x_audio_hw_params() doing something like this:
>>
>> if (audio.bclk_ratio == 0)
>> 	audio.bclk_ratio = DIV_ROUND_UP(params->sample_width, 8) * 8;
>>>> But then again I think it would be quite sane option to set bclk_ration
>> in hdmi-codec to 2*sample_width and simply refuse the ratios of 36 or 40
>> in tda998x.
> 
> ... which then leads to breakage of userspace if 18 or 20 bit formats
> were attempted, making the bridge driver's capabilities undiscoverable.
> 
> Returning an error from hw_params causes hard failures.
> 

Knowing how limited the support to those formats is in Linux userspace,
I would say that it is terribly unlikely anybody uses those formats.
Before the currently worked on bclk_ratio, those formats would only have
worked if the CPU DAI would implicitly use 48-bit bclk_ratio. I would
say that is quite unlikely too. I such a case exists or emerges later,
then they just need start using the bclk_ratio callback in their machine
driver. But still, I think leaving the bclk_ratio to zero if it is not
set is a better choice.

Best regards,
Jyri

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

Re: [PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1678 bytes --]

On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
> On 22/02/2019 23:27, Russell King wrote:

> > +	/*
> > +	 * If the .set_bclk_ratio() has not been called, default it
> > +	 * using the sample width for compatibility for TDA998x.
> > +	 * Rather than changing this, drivers should arrange to make
> > +	 * an appropriate call to snd_soc_dai_set_bclk_ratio().
> > +	 */
> > +	if (fmt.bclk_ratio == 0) {
> > +		switch (hp.sample_width) {
> > +		case 16:
> > +			fmt.bclk_ratio = 32;
> > +			break;
> > +		case 18:
> > +		case 20:
> > +		case 24:
> > +			fmt.bclk_ratio = 48;
> > +			break;

> AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually,
> the bclk_ratio is set to the exact frame length required by the sample
> width without any padding. That is at least the case with
> tlv320aic3x-driver and 20-bit sample width.

So, this is true.  On the other hand like Russell says further down the
thread it's preserving the existing behaviour so it would be surprising
if it actually broke anything and it will help systems that explicitly
set the ratio so I don't think we should let perfect be the enemy of
good here.

As Russell outlined there's quite a bit of hopeful assumption in how
ASoC handles the mapping of memory formats onto wire formats which works
almost all the time but not always and definitely not through robust
design, that should be a lot easier to address once the component
conversion has been done as we'll actually have all the links in the
system directly visible rather than bundled up together and implied as
they are currently.  Sadly that's a lot of work with not many people
working on it so progress is super slow.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Jyri Sarha]

[-- Attachment #1.1: Type: text/plain, Size: 3895 bytes --]

On 01/03/2019 14:36, Mark Brown wrote:
> On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
>> On 22/02/2019 23:27, Russell King wrote:
> 
>>> +	/*
>>> +	 * If the .set_bclk_ratio() has not been called, default it
>>> +	 * using the sample width for compatibility for TDA998x.
>>> +	 * Rather than changing this, drivers should arrange to make
>>> +	 * an appropriate call to snd_soc_dai_set_bclk_ratio().
>>> +	 */
>>> +	if (fmt.bclk_ratio == 0) {
>>> +		switch (hp.sample_width) {
>>> +		case 16:
>>> +			fmt.bclk_ratio = 32;
>>> +			break;
>>> +		case 18:
>>> +		case 20:
>>> +		case 24:
>>> +			fmt.bclk_ratio = 48;
>>> +			break;
> 
>> AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually,
>> the bclk_ratio is set to the exact frame length required by the sample
>> width without any padding. That is at least the case with
>> tlv320aic3x-driver and 20-bit sample width.
> 
> So, this is true.  On the other hand like Russell says further down the
> thread it's preserving the existing behaviour so it would be surprising
> if it actually broke anything and it will help systems that explicitly
> set the ratio so I don't think we should let perfect be the enemy of
> good here.
> 

Sure. Still, the requirement for having bclk_ratio of either 32, 48, or
64 is coming from tda998x, so that requirement should be satisfied in
tda998x driver, not in hdmi-codec that is supposed to be generic and
imposes that behaviour to all other user too.

Of course we should not make things overly complicated just because of
such principle. But in this case we are trying to preserve existing
behaviour that has hardly ever worked, and the new behaviour will
potentially cause more trouble. And if we really want to preserve the
existing behaviour there is another way that affects only tda998x driver:

- hdmi-codec leaves the struct hdmi_codec_params bclk_ratio to 0 if
  snd_soc_dai_set_bclk_ratio() is not called

- tda998x_audio_hw_params() checks if bclk_ratio == 0 and makes the
  implicit bclk_ratio setting the same way as the code above does it

Of course the configurations that we are trying to fix would only work
if the cpu-dai would by some luck (or intentional hack) use bclk_ratio
of 48 for 18- and 20-bit sample formats.

This is the problem with implicit blck_ratio setting at frame clock
slave end. Currently there is only a way to set bclk_ratio from the
machine driver, but there is no way ask what a dai driver would prefer
to use. That is why it is unlikely that 18- or 20-bit sample formats
have worked with any platform with tda998x, or will work in future with
just the implicit bclk_ratio setting.

But, after (hopefully) making my point, if both you and Russell want to
use Russell's original approach, please go ahead. As you said, it is
unlikely that it breaks anything.

> As Russell outlined there's quite a bit of hopeful assumption in how
> ASoC handles the mapping of memory formats onto wire formats which works
> almost all the time but not always and definitely not through robust
> design, that should be a lot easier to address once the component
> conversion has been done as we'll actually have all the links in the
> system directly visible rather than bundled up together and implied as
> they are currently.  Sadly that's a lot of work with not many people
> working on it so progress is super slow.
> 

Yes, there is lot more that could be done there. Ideally there would be
an option to let the dai drivers on the wire to negotiate the i2s format
and related parameters by them selves based on sample width, channels,
and sample rate.... but yes that is not a simple thing to implement.

Best regards,
Jyri

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Russell King - ARM Linux admin]

On Fri, Mar 01, 2019 at 04:05:25PM +0200, Jyri Sarha wrote:
> On 01/03/2019 14:36, Mark Brown wrote:
> > On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
> >> On 22/02/2019 23:27, Russell King wrote:
> > 
> >>> +	/*
> >>> +	 * If the .set_bclk_ratio() has not been called, default it
> >>> +	 * using the sample width for compatibility for TDA998x.
> >>> +	 * Rather than changing this, drivers should arrange to make
> >>> +	 * an appropriate call to snd_soc_dai_set_bclk_ratio().
> >>> +	 */
> >>> +	if (fmt.bclk_ratio == 0) {
> >>> +		switch (hp.sample_width) {
> >>> +		case 16:
> >>> +			fmt.bclk_ratio = 32;
> >>> +			break;
> >>> +		case 18:
> >>> +		case 20:
> >>> +		case 24:
> >>> +			fmt.bclk_ratio = 48;
> >>> +			break;
> > 
> >> AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually,
> >> the bclk_ratio is set to the exact frame length required by the sample
> >> width without any padding. That is at least the case with
> >> tlv320aic3x-driver and 20-bit sample width.
> > 
> > So, this is true.  On the other hand like Russell says further down the
> > thread it's preserving the existing behaviour so it would be surprising
> > if it actually broke anything and it will help systems that explicitly
> > set the ratio so I don't think we should let perfect be the enemy of
> > good here.
> > 
> 
> Sure. Still, the requirement for having bclk_ratio of either 32, 48, or
> 64 is coming from tda998x, so that requirement should be satisfied in
> tda998x driver, not in hdmi-codec that is supposed to be generic and
> imposes that behaviour to all other user too.
> 
> Of course we should not make things overly complicated just because of
> such principle. But in this case we are trying to preserve existing
> behaviour that has hardly ever worked, and the new behaviour will
> potentially cause more trouble.

Err, what?

With respect to TDA998x, my patches do not change the behaviour in
any way of the TDA998x setup.  As I have already established:

1. I'm introducing a new bclk_ratio member into the hdmi daifmt
   structure.  There can be no other users of this except the ones
   that are introduced from this point forth.

2. No one calls the function that sets the bclk_ratio in the
   entire codebase that is sound/soc, so no one will set bclk_ratio
   to anything non-zero, except any new callers that get introduced.

So, at the point that patch 2 is merged, no one is using the values
that the new code derives.  No one is supplying bclk_ratio values to
the code either.

As I have already stated, the way I see this code is it is a stop-gap
to allow TDA998x to continue working as well as it does _today_ on
BBB when we convert TDA998x to start using the supplied bclk_ratio.
However, what we should strive to do is to eliminate that code as
soon as possible - in other words, making BBB and all the other
users set the bclk_ratio explicitly for I2S links.

However, you bring up another issue in what you have said above -
you appear to be claiming that the "existing behaviour has hardly
ever worked".  If it hardly ever works, this seems to imply that
audio support on BBB is currently broken.

If BBB audio is broken, then we don't need this at all, and we can
just change TDA998x to error out on a zero bclk_ratio value.

> And if we really want to preserve the
> existing behaviour there is another way that affects only tda998x driver:
> 
> - hdmi-codec leaves the struct hdmi_codec_params bclk_ratio to 0 if
>   snd_soc_dai_set_bclk_ratio() is not called
> 
> - tda998x_audio_hw_params() checks if bclk_ratio == 0 and makes the
>   implicit bclk_ratio setting the same way as the code above does it

I'm afraid I completely disagree.  Pushing that into the tda998x
driver means that:

1. there is no motivation to set a correct bclk_ratio value (no one
   sets it today.)
2. because no one sets the bclk_ratio, users of hdmi-codec will see
   bclk_ratio as zero, so if they care they will implement their
   drivers with their own defaults, which may be to always default
   to 64fs, or may be to default to 2x sample_width.
3. since the defaults match what the HDMI bridges are used with,
   no one will bother implementing a call to the ASoC function to
   set bclk_ratio.

So, we'll continue on as we have done, with no one calling the ASoC
function, HDMI bridges making their own randomly different bclk_ratio
assumptions, and the whole thing generally decending into an
unmaintainable mess.

With the defaulting in the hdmi-codec code, it gives a bit more
motivation for things to get fixed up: if the default in hdmi-codec
is not suitable, it forces the I2S source to explicitly set the
bclk_ratio accurately.

It becomes even better when we remove the defaulting, because we
then require everyone to call it for I2S to be functional with
hdmi-codec with a HDMI bridge that requires this information.

Even better than that would be to make hdmi-codec require that
the bclk_ratio is set, which means we have a hope that we're not
going to end up in the endless situation where we have to retrofit
the bclk_ratio call this into future I2S source.

... where "I2S source" above I mean the non-codec parts of the
subsystem.

-- 
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 RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Jyri Sarha]

On 01/03/2019 16:59, Russell King - ARM Linux admin wrote:
> On Fri, Mar 01, 2019 at 04:05:25PM +0200, Jyri Sarha wrote:
>> On 01/03/2019 14:36, Mark Brown wrote:
>>> On Mon, Feb 25, 2019 at 03:45:44PM +0200, Jyri Sarha wrote:
>>>> On 22/02/2019 23:27, Russell King wrote:
>>>
>>>>> +	/*
>>>>> +	 * If the .set_bclk_ratio() has not been called, default it
>>>>> +	 * using the sample width for compatibility for TDA998x.
>>>>> +	 * Rather than changing this, drivers should arrange to make
>>>>> +	 * an appropriate call to snd_soc_dai_set_bclk_ratio().
>>>>> +	 */
>>>>> +	if (fmt.bclk_ratio == 0) {
>>>>> +		switch (hp.sample_width) {
>>>>> +		case 16:
>>>>> +			fmt.bclk_ratio = 32;
>>>>> +			break;
>>>>> +		case 18:
>>>>> +		case 20:
>>>>> +		case 24:
>>>>> +			fmt.bclk_ratio = 48;
>>>>> +			break;
>>>
>>>> AFAIK, this is not the usual choice for 18- or 20-bit samples. Usually,
>>>> the bclk_ratio is set to the exact frame length required by the sample
>>>> width without any padding. That is at least the case with
>>>> tlv320aic3x-driver and 20-bit sample width.
>>>
>>> So, this is true.  On the other hand like Russell says further down the
>>> thread it's preserving the existing behaviour so it would be surprising
>>> if it actually broke anything and it will help systems that explicitly
>>> set the ratio so I don't think we should let perfect be the enemy of
>>> good here.
>>>
>>
>> Sure. Still, the requirement for having bclk_ratio of either 32, 48, or
>> 64 is coming from tda998x, so that requirement should be satisfied in
>> tda998x driver, not in hdmi-codec that is supposed to be generic and
>> imposes that behaviour to all other user too.
>>
>> Of course we should not make things overly complicated just because of
>> such principle. But in this case we are trying to preserve existing
>> behaviour that has hardly ever worked, and the new behaviour will
>> potentially cause more trouble.
> 
> Err, what?
> 
> With respect to TDA998x, my patches do not change the behaviour in
> any way of the TDA998x setup.  As I have already established:
> 
> 1. I'm introducing a new bclk_ratio member into the hdmi daifmt
>    structure.  There can be no other users of this except the ones
>    that are introduced from this point forth.
> 
> 2. No one calls the function that sets the bclk_ratio in the
>    entire codebase that is sound/soc, so no one will set bclk_ratio
>    to anything non-zero, except any new callers that get introduced.
> 
> So, at the point that patch 2 is merged, no one is using the values
> that the new code derives.  No one is supplying bclk_ratio values to
> the code either.
> 
> As I have already stated, the way I see this code is it is a stop-gap
> to allow TDA998x to continue working as well as it does _today_ on
> BBB when we convert TDA998x to start using the supplied bclk_ratio.
> However, what we should strive to do is to eliminate that code as
> soon as possible - in other words, making BBB and all the other
> users set the bclk_ratio explicitly for I2S links.
> 
> However, you bring up another issue in what you have said above -
> you appear to be claiming that the "existing behaviour has hardly
> ever worked".  If it hardly ever works, this seems to imply that
> audio support on BBB is currently broken.
> 
> If BBB audio is broken, then we don't need this at all, and we can
> just change TDA998x to error out on a zero bclk_ratio value.
> 
>> And if we really want to preserve the
>> existing behaviour there is another way that affects only tda998x driver:
>>
>> - hdmi-codec leaves the struct hdmi_codec_params bclk_ratio to 0 if
>>   snd_soc_dai_set_bclk_ratio() is not called
>>
>> - tda998x_audio_hw_params() checks if bclk_ratio == 0 and makes the
>>   implicit bclk_ratio setting the same way as the code above does it
> 
> I'm afraid I completely disagree.  Pushing that into the tda998x
> driver means that:
> 
> 1. there is no motivation to set a correct bclk_ratio value (no one
>    sets it today.)

I fail to see how that is different when the implicit setting of
bclk_ratio is done in hdmi-codec.c.

> 2. because no one sets the bclk_ratio, users of hdmi-codec will see
>    bclk_ratio as zero, so if they care they will implement their
>    drivers with their own defaults, which may be to always default
>    to 64fs, or may be to default to 2x sample_width.
> 3. since the defaults match what the HDMI bridges are used with,
>    no one will bother implementing a call to the ASoC function to
>    set bclk_ratio.
> 

Setting the bclk_ratio implicitly to what ever in tda998x or hdmi-codec
does not really help the problem at all, since it is not visible outside
it really has nothing to do with the bclk_ratio used by the cpu-dai.

> So, we'll continue on as we have done, with no one calling the ASoC
> function, HDMI bridges making their own randomly different bclk_ratio
> assumptions, and the whole thing generally decending into an
> unmaintainable mess.
> 

The i2s slaves, which HDMI encoders most often are, do not usually care
too much about bclk_ratio in the first place. So normally there is no
need for any defaults. tda998x-family, of course, is an exception to
this rule, but for instance sii9022 does not use bclk for CTS values and
does not care about bclk as long as it is big enough to fit all sample
bits in to frame.

> With the defaulting in the hdmi-codec code, it gives a bit more
> motivation for things to get fixed up: if the default in hdmi-codec
> is not suitable, it forces the I2S source to explicitly set the
> bclk_ratio accurately.
> 

Ok, I see. The potential cause future of problems is the intention :).

The implementation of set_bclk_ratio() callback is only a small part of
the problem. We need something more we want to have a generic solution
to the situation where i2s slave needs to know, or even have a say to
what the bclk-ratio should be.

Currently (AFAIK, I have not followed lately) in ASoC the i2s
bclk-master generates the bclk to the best of its ability for the given
sample-rate and sample-format (and number channels and possibly of
tdm-slots etc.), usually it is also the frame-master and chooses the
matching bclk-ratio for the situation. Forcing the sample clock from the
machine driver (like simple-card) changes this behaviour a lot, and
requires relatively big changes to the dai-drivers acting as bclk and
frame masters, before this can work. This would at least be the case
with McASP that I am familiar with.

> It becomes even better when we remove the defaulting, because we
> then require everyone to call it for I2S to be functional with
> hdmi-codec with a HDMI bridge that requires this information.
> 
> Even better than that would be to make hdmi-codec require that
> the bclk_ratio is set, which means we have a hope that we're not
> going to end up in the endless situation where we have to retrofit
> the bclk_ratio call this into future I2S source.
> 

This would effectively break hdmi-codec, for all platforms that do not
have the above problem solved.

> ... where "I2S source" above I mean the non-codec parts of the
> subsystem.
> 

For a (more) complete solution to this problem I would propose to add
something to ASoC API for asking the chosen bclk-ratio from DAI drivers.
Machine drivers could then ask that from the i2s frame masters and
deliver the information to i2s slaves.

Best regards,
Jyri


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

Re: [PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Sven Van Asbroeck]

On Fri, Mar 1, 2019 at 7:36 AM Mark Brown <broonie@kernel.org> wrote:
>
>
> As Russell outlined there's quite a bit of hopeful assumption in how
> ASoC handles the mapping of memory formats onto wire formats which works
> almost all the time but not always and definitely not through robust
> design

Yes, many of the painful tradeoffs in this discussion would go away, if the
alsa negotiation infrastructure was 'bclk_ratio aware'.

They say that a 100-line patch is often better than 1000 words. To help kick-
start a discussion, I offer a simple patch which adds bclk_ratio to
hw_params, which lets alsa 'negotiate' it the same way as the format,
channels, etc.

Obviously this is completely flawed, as it exposes bclk_ratio to userspace thru
snd_pcm_hw_params. Userspace has no business even knowing about this on-wire
property.

It may be flawed in a lot of other ways I can't see rn :)

As far as a flawed suggestion goes, it seems to behave quite well on my system.
The bclk_ratio is nicely constrained within the cpu and codec's supported
ranges and rules, and the lowest supported value is picked before hw_params()
gets called. Which is at least channels * sample_width.

Would there be a way to hide the bclk_ratio from userspace?

Is it even worth taking this forward? How likely is it for alsa components
to care about the bclk_ratio? Maybe this is a tda998x one-off?

Sven

Re: [PATCH RFC 2/3] ASoC: hdmi-codec: add support for bclk_ratio

[Jyri Sarha]

On 04/03/2019 18:59, Sven Van Asbroeck wrote:
> On Fri, Mar 1, 2019 at 7:36 AM Mark Brown <broonie@kernel.org> wrote:
>>
>>
>> As Russell outlined there's quite a bit of hopeful assumption in how
>> ASoC handles the mapping of memory formats onto wire formats which works
>> almost all the time but not always and definitely not through robust
>> design
> 
> Yes, many of the painful tradeoffs in this discussion would go away, if the
> alsa negotiation infrastructure was 'bclk_ratio aware'.
> 
> They say that a 100-line patch is often better than 1000 words. To help kick-
> start a discussion, I offer a simple patch which adds bclk_ratio to
> hw_params, which lets alsa 'negotiate' it the same way as the format,
> channels, etc.
> 
> Obviously this is completely flawed, as it exposes bclk_ratio to userspace thru
> snd_pcm_hw_params. Userspace has no business even knowing about this on-wire
> property.
> 
> It may be flawed in a lot of other ways I can't see rn :)
> 
> As far as a flawed suggestion goes, it seems to behave quite well on my system.
> The bclk_ratio is nicely constrained within the cpu and codec's supported
> ranges and rules, and the lowest supported value is picked before hw_params()
> gets called. Which is at least channels * sample_width.
> 
> Would there be a way to hide the bclk_ratio from userspace?
> 
> Is it even worth taking this forward? How likely is it for alsa components
> to care about the bclk_ratio? Maybe this is a tda998x one-off?
> 

I am not the right person say if this should be taken forward.

However, if we are going forward, I think we need to add bclk_rate to
snd_pcm_hw_params too, since the available bclks affects the choice of
bclk_ratios that are available. For instance if the bclk is generated
from static clock with a simple divider then following equatios should
always be satisfied:

bclk_rate = clk_source_rate / clk_divider, where clk_divider = 0, 1, ..n
sample_rate = bclk_rate / bclk_ratio

The good news is that snd_pcm_hw_rule_add() should be able to cope with
such cross dependencies.

BR,
Jyri

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