Re: [alsa-devel] [Linux-kernel] [PATCH v5 2/7] ASoC: tegra: Allow 24bit and 32bit samples

From: Ben Dooks <ben.dooks@codethink.co.uk>
To: Jon Hunter <jonathanh@nvidia.com>,
	Mark Brown <broonie@kernel.org>,
	Dmitry Osipenko <digetx@gmail.com>
Cc: linux-kernel@lists.codethink.co.uk, alsa-devel@alsa-project.org,
	Liam Girdwood <lgirdwood@gmail.com>,
	Takashi Iwai <tiwai@suse.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Edward Cragg <edward.cragg@codethink.co.uk>,
	linux-tegra@vger.kernel.org
Subject: Re: [alsa-devel] [Linux-kernel] [PATCH v5 2/7] ASoC: tegra: Allow 24bit and 32bit samples
Date: Thu, 30 Jan 2020 08:17:37 +0000	[thread overview]
Message-ID: <fe002ec7-ae6e-f770-b82a-49237e0b29c6@codethink.co.uk> (raw)
In-Reply-To: <586ea2b9-c204-2bd1-f8e2-875e0974e42d@nvidia.com>

On 29/01/2020 14:33, Jon Hunter wrote:
> 
> On 29/01/2020 10:49, Jon Hunter wrote:
>>
>> On 28/01/2020 12:13, Mark Brown wrote:
>>> I really don't understand why this is all taking so long, this thread
>>> just seems to be going round in interminable circles long after it
>>> looked like the issue was understood.  I have to admit I've not read
>>> every single message in the thread but it's difficult to see why it
>>> doesn't seem to be making any progress.
>>
>> Sorry about that. On reviewing this with the audio team at NVIDIA, I was
>> told we don't support S24_LE for I2S. The reason being that the crossbar
>> between the DMA and I2S is not able to extract the correct 24-bits from
>> the 32-bit sample to feed to the I2S interface. The Tegra documentation
>> does show support for 24-bits, but not state explicit support for S24_LE.
>>
>> Now Ben says that he has this working, but I am unable to reproduce
>> this, so before just dropping the S24_LE support, I would like to
>> understand how this is working for Ben in case there is something that
>> we have overlooked here.
> 
> Ah, I see that part of the problem is that patches 6 and 7 are yet to be
> applied and without these the audio is completely distorted because
> there is a mismatch in the data size between the APBIF and I2S
> controller. Applying these patches it is not distorted but now I am
> observing the clocking issue Ben reported and so the tone is not quite
> right.

I thought they had been applied? I probably dragged them back in when
putting in the support for the test channel on the colibri.

> Ben, I was able to workaround the clocking issue by making the I2S word
> clock 64 bits long and not 48.

Ok, that will work for I2S case, but maybe not TDM? I'd have to check.

> diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
> index bbf81b5aa723..3c9b4779e61b 100644
> --- a/sound/soc/tegra/tegra30_i2s.c
> +++ b/sound/soc/tegra/tegra30_i2s.c
> @@ -143,7 +143,7 @@ static int tegra30_i2s_hw_params(struct
> snd_pcm_substream *substream,
>          case SNDRV_PCM_FORMAT_S24_LE:
>                  val = TEGRA30_I2S_CTRL_BIT_SIZE_24;
>                  audio_bits = TEGRA30_AUDIOCIF_BITS_24;
> -               sample_size = 24;
> +               sample_size = 32;
>                  break;
>          case SNDRV_PCM_FORMAT_S32_LE:
>                  val = TEGRA30_I2S_CTRL_BIT_SIZE_32;
> 
> For I2S I believe we only care about the edges of the word clock and so
> we make the overall period of the word clock 64 bit clocks then we no
> longer have an issue with the bit clock frequency. I assume that this
> should also be fine for TDM modes as well.
> 
> Can you let me know if this works for you?

I'll be back in the office next week to test.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel