[alsa-devel] [PATCH] ASoC: davinci-mcasp: Correct rx format unit configuration

[alsa-devel] [PATCH] ASoC: davinci-mcasp: Correct rx format unit configuration

[Peter Ujfalusi]

In case of capture we should not use rotation. The reverse and mask is
enough to get the data align correctly from the bus to MCU:
Format	  data from bus    after reverse (XRBUF)
S16_LE:  |LSB|MSB|xxx|xxx|  |xxx|xxx|MSB|LSB|
S24_3LE: |LSB|DAT|MSB|xxx|  |xxx|MSB|DAT|LSB|
S24_LE:  |LSB|DAT|MSB|xxx|  |xxx|MSB|DAT|LSB|
S32_LE:  |LSB|DAT|DAT|MSB|  |MSB|DAT|DAT|LSB|

With this patch all supported formats will work for playback and capture.

Reported-by: Jyri Sarha <jsarha@ti.com> (broken S24_3LE capture)
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/davinci/davinci-mcasp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index 6a6b2ff7d7d7..68347b55f6e1 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -467,8 +467,17 @@ static int davinci_config_channel_size(struct davinci_mcasp *mcasp,
 {
 	u32 fmt;
 	u32 tx_rotate = (word_length / 4) & 0x7;
-	u32 rx_rotate = (32 - word_length) / 4;
 	u32 mask = (1ULL << word_length) - 1;
+	/*
+	 * For captured data we should not rotate, inversion and masking is
+	 * enoguh to get the data to the right position:
+	 * Format	  data from bus		after reverse (XRBUF)
+	 * S16_LE:	|LSB|MSB|xxx|xxx|	|xxx|xxx|MSB|LSB|
+	 * S24_3LE:	|LSB|DAT|MSB|xxx|	|xxx|MSB|DAT|LSB|
+	 * S24_LE:	|LSB|DAT|MSB|xxx|	|xxx|MSB|DAT|LSB|
+	 * S32_LE:	|LSB|DAT|DAT|MSB|	|MSB|DAT|DAT|LSB|
+	 */
+	u32 rx_rotate = 0;
 
 	/*
 	 * if s BCLK-to-LRCLK ratio has been configured via the set_clkdiv()
-- 
2.1.0

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: davinci-mcasp: Correct rx format unit configuration

[Mark Brown]

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

On Thu, Sep 04, 2014 at 10:52:53AM +0300, Peter Ujfalusi wrote:
> In case of capture we should not use rotation. The reverse and mask is
> enough to get the data align correctly from the bus to MCU:

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

Re: [PATCH] ASoC: davinci-mcasp: Correct rx format unit configuration

[Pascal Huerst]

Hey Peter, all,

the patch (8e3006d02b46a722729a3bbd6a774eae99c6c8b8), causes lots of
terrible noise on a custom built am33xx based board, if I map the input
to the output like so:

arecord -f S16_LE -r 48000 -c2 -D hw:0,1 -F 0 --period-size=102
4 -B 0 --buffer-size=4096 | aplay -D hw:0,0 -

If I undo your changes, everything works fine. Tested with 3.14.23 and
3.18.1.

Do you have any Idea, what might cause that behavior?


regards,
pascal

Re: [PATCH] ASoC: davinci-mcasp: Correct rx format unit configuration

[Peter Ujfalusi]

On 01/14/2015 01:11 PM, Pascal Huerst wrote:
> Hey Peter, all,
> 
> the patch (8e3006d02b46a722729a3bbd6a774eae99c6c8b8), causes lots of
> terrible noise on a custom built am33xx based board, if I map the input
> to the output like so:
> 
> arecord -f S16_LE -r 48000 -c2 -D hw:0,1 -F 0 --period-size=102
> 4 -B 0 --buffer-size=4096 | aplay -D hw:0,0 -

FWIW the -F0 and -B0 is redundant and you can use -fdat instead of -f S16_LE
-r 48000 -c2

> If I undo your changes, everything works fine. Tested with 3.14.23 and
> 3.18.1.

You mean only this single commit?

> Do you have any Idea, what might cause that behavior?

No, I don't. I quickly checked on am335x, am437x and on J6 boards and the
command you are using works w/o any issue or distortion in recorded audio.

what codec or codecs are you using? I see that you record from hw:0,1 and play
it back to hw:0,0
Can you check if the recorded sample alone is correct? Just record it to a
file and look at it on the PC. I did the same and the recorded sample looks
fine on my PC as well.

Looking at the patch again, I still think it is a valid fix.

-- 
Péter
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: davinci-mcasp: Correct rx format unit configuration

[Pascal Huerst]

On 14.01.2015 13:33, Peter Ujfalusi wrote:
> On 01/14/2015 01:11 PM, Pascal Huerst wrote:
>> Hey Peter, all,
>>
>> the patch (8e3006d02b46a722729a3bbd6a774eae99c6c8b8), causes lots of
>> terrible noise on a custom built am33xx based board, if I map the input
>> to the output like so:
>>
>> arecord -f S16_LE -r 48000 -c2 -D hw:0,1 -F 0 --period-size=102
>> 4 -B 0 --buffer-size=4096 | aplay -D hw:0,0 -
> 
> FWIW the -F0 and -B0 is redundant and you can use -fdat instead of -f S16_LE
> -r 48000 -c2

Ok, good to know.

>> If I undo your changes, everything works fine. Tested with 3.14.23 and
>> 3.18.1.
> 
> You mean only this single commit?

Yes, even this single line.

- u32 rx_rotate = 0;
+ u32 rx_rotate = (32 - word_length) / 4;

>> Do you have any Idea, what might cause that behavior?
> 
> No, I don't. I quickly checked on am335x, am437x and on J6 boards and the
> command you are using works w/o any issue or distortion in recorded audio.

> what codec or codecs are you using? I see that you record from hw:0,1 and play
> it back to hw:0,0
> Can you check if the recorded sample alone is correct? Just record it to a
> file and look at it on the PC. I did the same and the recorded sample looks
> fine on my PC as well.

if I record into a file, it behaves just the same. Noise if
rx_rotate == 0, correct otherwise. I guess it is a problem in the input
codec, then.

input codec:   AK5386
output codec:  TAS5086

> Looking at the patch again, I still think it is a valid fix.

Yes, I agree, the patch looks right.

I'll have a look into the ak5386 and see, what impact your change could
have there.


thanks so far!

Re: [PATCH] ASoC: davinci-mcasp: Correct rx format unit configuration

[Peter Ujfalusi]

On 01/14/2015 03:24 PM, Pascal Huerst wrote:
> Yes, even this single line.
> 
> - u32 rx_rotate = 0;
> + u32 rx_rotate = (32 - word_length) / 4;
> 
>>> Do you have any Idea, what might cause that behavior?
>>
>> No, I don't. I quickly checked on am335x, am437x and on J6 boards and the
>> command you are using works w/o any issue or distortion in recorded audio.
> 
>> what codec or codecs are you using? I see that you record from hw:0,1 and play
>> it back to hw:0,0
>> Can you check if the recorded sample alone is correct? Just record it to a
>> file and look at it on the PC. I did the same and the recorded sample looks
>> fine on my PC as well.
> 
> if I record into a file, it behaves just the same. Noise if
> rx_rotate == 0, correct otherwise. I guess it is a problem in the input
> codec, then.
> 
> input codec:   AK5386
> output codec:  TAS5086
> 
>> Looking at the patch again, I still think it is a valid fix.
> 
> Yes, I agree, the patch looks right.
> 
> I'll have a look into the ak5386 and see, what impact your change could
> have there.

Can you try S24_LE format as well? I think it should work.

I assume you are running the AK5386 as master which would explain the
symptoms. In this mode you have 64bclk per sample (32 per channel).

I believe this should fix your capture in case of S16_LE:
keep the (in linux-next):
fe0a29e163a5d045c73faab682a8dac71c2f8012 : ASoC: davinci-mcasp: Correct rx
format unit configuration

make sure you have (as in linux-next):
d742b925244ce91f16d380befdca473e4536359b : ASoC: davinci-mcasp: Fix rx format
when more bclk is used on the bus

add the following to your machine driver's hw_params callback:

...
snd_soc_dai_set_clkdiv(cpu_dai, 2, 64);
...

I think the issue with your setup is that McASP expects the you have 32bclk
per sample, but AK5386 generates 64 for you, this will mess up the data you
are receiving since in this case we need to shift the data with 16bits before
we invert it.

-- 
Péter
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: davinci-mcasp: Correct rx format unit configuration

[Pascal Huerst]

On 14.01.2015 15:17, Peter Ujfalusi wrote:
> On 01/14/2015 03:24 PM, Pascal Huerst wrote:
>> Yes, even this single line.
>>
>> - u32 rx_rotate = 0;
>> + u32 rx_rotate = (32 - word_length) / 4;
>>
>>>> Do you have any Idea, what might cause that behavior?
>>>
>>> No, I don't. I quickly checked on am335x, am437x and on J6 boards and the
>>> command you are using works w/o any issue or distortion in recorded audio.
>>
>>> what codec or codecs are you using? I see that you record from hw:0,1 and play
>>> it back to hw:0,0
>>> Can you check if the recorded sample alone is correct? Just record it to a
>>> file and look at it on the PC. I did the same and the recorded sample looks
>>> fine on my PC as well.
>>
>> if I record into a file, it behaves just the same. Noise if
>> rx_rotate == 0, correct otherwise. I guess it is a problem in the input
>> codec, then.
>>
>> input codec:   AK5386
>> output codec:  TAS5086
>>
>>> Looking at the patch again, I still think it is a valid fix.
>>
>> Yes, I agree, the patch looks right.
>>
>> I'll have a look into the ak5386 and see, what impact your change could
>> have there.
> 
> Can you try S24_LE format as well? I think it should work.

If I record to a file everything works fine, no noise. But I can not map
to the output, since the output codec does not support S24_LE

> I assume you are running the AK5386 as master which would explain the
> symptoms. In this mode you have 64bclk per sample (32 per channel).

no I'm not. it's running in slave mode. CKS[0-2] = 0 (GND).

> I believe this should fix your capture in case of S16_LE:
> keep the (in linux-next):
> fe0a29e163a5d045c73faab682a8dac71c2f8012 : ASoC: davinci-mcasp: Correct rx
> format unit configuration
> 
> make sure you have (as in linux-next):
> d742b925244ce91f16d380befdca473e4536359b : ASoC: davinci-mcasp: Fix rx format
> when more bclk is used on the bus

Yes. Although AK5386 is in slave mode, this fixed the issue!

> add the following to your machine driver's hw_params callback:
> 
> ...
> snd_soc_dai_set_clkdiv(cpu_dai, 2, 64);
> ...

That line was there already.

> I think the issue with your setup is that McASP expects the you have 32bclk
> per sample, but AK5386 generates 64 for you, this will mess up the data you
> are receiving since in this case we need to shift the data with 16bits before
> we invert it.

Ok. I think I will have to look deeper into that, to really understand,
where these 64bclk came from in my case (assuming its set in the machine
driver).

But anyways, thanks a lot for your help!

pascal