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

[Russell King - ARM Linux admin]

(Adding Mark, ASoC maintainer.)

On Fri, Feb 22, 2019 at 10:47:35AM -0500, Sven Van Asbroeck wrote:
> On Fri, Feb 22, 2019 at 8:21 AM Russell King - ARM Linux admin
> <linux@xxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, Feb 21, 2019 at 01:18:13PM -0500, Sven Van Asbroeck wrote:
> >
> > >       [SNDRV_PCM_FORMAT_S24_LE] = {
> > >               .width = 24, .phys = 32, .le = 1, .signd = 1,
> > >               .silence = {},
> > >       },
> >
> > The above table describes the memory format, not the wire format.
> > Look further down for SNDRV_PCM_FORMAT_S24_3LE, which is 24-bit
> > packed into three bytes (see include/uapi/sound/asound.h for
> > the comment specifying that.)
> >
> > ASoC uses DAIFMT to specify the on-wire format in connection with
> > the above.
> >
> 
> Interesting ! So you're saying that currently, nobody strictly defines the
> layout of the on-wire format, correct? I'm not sure how this works in practice,
> because codec and cpu dai should agree on the on-wire format? Except if the
> formats used have enough flexibility so you don't have to care.

SNDRV_PCM_FORMAT_xxx more defines the in-memory format, rather than
the on-wire format.

As I've said, the on-wire format is defined in ASoC using a completely
different mechanism, using the definitions in include/sound/soc-dai.h.
This describes parameters such as the polarity of clocks on the i2s
bus, the justification of the data, etc.

Bear in mind that SNDRV_PCM_FORMAT_S24_LE in memory may be right
justified (using the least significant 3 bytes of every 32-bit word),
but on the wire may be left justified - using the most significant
bits.

SND_SOC_DAIFMT_I2S defines the format to be "Philips" format, where
the MSB bit is sent one BCLK _after_ the LRCLK signal changes state.
There is also SND_SOC_DAIFMT_LEFT_J, where the MSB bit is sent with
no delay, and extra padding zeros are sent in the "LSB" bits.
SND_SOC_DAIFMT_RIGHT_J is similar, but the padding is in the "MSB"
bits.

Then there's the polarity of the BCLK and LRCLK (frame) signals.
Finally, there's whether the codec (TDA998x in this case) is the
origin of the LRCLK and/or BCLK.

This information is passed via a call to snd_soc_dai_set_fmt()
which takes the DAI and the format - this calls into hdmi-codec.c
hdmi_codec_set_fmt().  This will be handled by the core ASoC code
if the DAI has a .dai_fmt member set (which can be set by DT -
see snd_soc_of_parse_daifmt().)

Then there is snd_soc_dai_set_bclk_ratio() which sets the BCLK
to sample-rate ratio, as I explained earlier.  hdmi-codec doesn't
have an implementation for this, and afaics no one calls this
function.  So, it seems assumptions are made throughout ASoC
on that point (probably because most codecs don't care.)

> > This doesn't really help in terms of working out what the correct
> > settings should be, and other information I have laying around does not
> > provide any further enlightenment.
> 
> I have access to the NXP software library shipped with the tda19988.

Yes, I'm aware of it.

> The library's release notes have the following entry:
> 
>   . "I2S audio does not work, CTS value is not good"
>     Check the audio I2S format <snip>
>     CTS is automatically computed by the TDA accordingly to the audio input
>     so accordingly to the upstream settings (like an OMAP ;)
>     For example, I2S 16 bits or 32 bits do not produce the same CTS value
> 
> The config structure which you need to fill in to init the audio has a
> "i2s qualifier" field, where you have the choice between 16 and 32 bits.
> This then maps to a "Clock Time Stamp factor x" called CTSX, which maps to
> the following CTS_N register settings:
> 
> CTSX -> CTS_N (m,k)
> -----------------------------------
> 16 -> (3,0)
> 32 -> (3,1) (i2s qualifier = 16 bits)
> 48 -> (3,2)
> 64 -> (3,3) (i2s qualifier = 32 bits)
> 128 -> (0,0)
> 
> Does this information bring us any closer to our assumption that CTS_N needs
> to be calculated off the bclk to sample rate ratio ?

I'm aware of other users of TDA998x, and I'm attempting to get out of
them what ratios their implementations use - they've said that they
have confirmed that 16bit and 24bit works for them, but that's rather
incomplete in terms of what I wanted to know... waiting for another
response!
 
> I'd love to take a shot at this, but first I'd like to understand what you're
> suggesting :)
> 
> Currently there is set_bclk_ratio() support, but no-one is actually using it.
> If hdmi-codec is to retrieve the ratio, wouldn't we need to add .GET_blk_ratio
> to snd_soc_dai_ops ?
> 
> I could add this to fsl_ssi in master mode, but what if somebody connects the
> tda to a cpu dai for which no-one implemented .GET_bclk_ratio ? Do we guess?
> Or just error out?
> 
> Also, what would a proposed snd_soc_dai_GET_bclk_ratio() return e.g. on
> fsl_ssi in slave mode, where the value arguably doesn't exist because the ssi
> will accept pretty much anything you throw at it?

You appear to be thinking that the codec should ask something else for
the bclk ratio - however ASoC is designed from the point of view of
the codecs being told the appropriate operating parameters by the
"card" or core at the appropriate time(s).  Hence, something needs
to call snd_soc_dai_set_bclk_ratio().

hdmi-codec then needs to supply an implementation for the
set_bclk_ratio() method in struct snd_soc_dai_ops, just like it
already does for the set_fmt method, and pass the bclk ratio to the
actual HDMI codec.

Something like the below would probably be moving in the right
direction:

diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
index 9483c55f871b..0fca69880dc3 100644
--- a/include/sound/hdmi-codec.h
+++ b/include/sound/hdmi-codec.h
@@ -42,6 +42,7 @@ struct hdmi_codec_daifmt {
 	unsigned int frame_clk_inv:1;
 	unsigned int bit_clk_master:1;
 	unsigned int frame_clk_master:1;
+	unsigned int bclk_ratio;
 };
 
 /*
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index d00734d31e04..f0f08b7a073f 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -524,6 +524,17 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
 				       &hcp->daifmt[dai->id], &hp);
 }
 
+static int hdmi_codec_set_bclk_ratio(struct snd_soc_dai *dai,
+				     unsigned int ratio)
+{
+	struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai);
+
+	/* FIXME: some validation here would be good? */
+	hcp->daifmt[dai->id].bclk_ratio = ratio;
+
+	return 0;
+}
+
 static int hdmi_codec_set_fmt(struct snd_soc_dai *dai,
 			      unsigned int fmt)
 {
@@ -593,7 +604,11 @@ static int hdmi_codec_set_fmt(struct snd_soc_dai *dai,
 		}
 	}
 
-	hcp->daifmt[dai->id] = cf;
+	hcp->daifmt[dai->id].fmt = cf.fmt;
+	hcp->daifmt[dai->id].bit_clk_inv = cf.bit_clk_inv;
+	hcp->daifmt[dai->id].frame_clk_inv = cf.frame_clk_inv;
+	hcp->daifmt[dai->id].bit_clk_master = cf.bit_clk_master;
+	hcp->daifmt[dai->id].frame_clk_master = cf.frame_clk_master;
 
 	return ret;
 }
@@ -615,6 +630,7 @@ static const struct snd_soc_dai_ops hdmi_dai_ops = {
 	.startup	= hdmi_codec_startup,
 	.shutdown	= hdmi_codec_shutdown,
 	.hw_params	= hdmi_codec_hw_params,
+	.set_bclk_ratio	= hdmi_codec_set_bclk_ratio,
 	.set_fmt	= hdmi_codec_set_fmt,
 	.digital_mute	= hdmi_codec_digital_mute,
 };
@@ -795,6 +811,8 @@ static int hdmi_codec_probe(struct platform_device *pdev)
 	if (hcd->spdif)
 		hcp->daidrv[i] = hdmi_spdif_dai;
 
+	hcp->daifmt[DAI_ID_I2S].bclk_ratio = 64;
+
 	ret = devm_snd_soc_register_component(dev, &hdmi_driver, hcp->daidrv,
 				     dai_count);
 	if (ret) {

Then tda998x can look at daifmt->bclk_ratio in
tda998x_audio_hw_params().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up