[PATCH] ASoC: fsl_esai: Clear the xPM bit when using xFP

[PATCH] ASoC: fsl_esai: Clear the xPM bit when using xFP

[Marek Vasut]

When setting xFP directly, set the xPM predivider to 1, otherwise
it could remain set to previously set incorrect value and interfere
with the correct clocking.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Gustavo A. R. Silva <garsilva@embeddedor.com>
Cc: Mark Brown <broonie@kernel.org>
---
 sound/soc/fsl/fsl_esai.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index 40a700493f4c..9f69823b50d7 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -128,8 +128,11 @@ static int fsl_esai_divisor_cal(struct snd_soc_dai *dai, bool tx, u32 ratio,
 
 	maxfp = usefp ? 16 : 1;
 
-	if (usefp && fp)
+	if (usefp && fp) {
+		regmap_update_bits(esai_priv->regmap, REG_ESAI_xCCR(tx),
+				   ESAI_xCCR_xPM_MASK, 0);
 		goto out_fp;
+	}
 
 	if (ratio > 2 * 8 * 256 * maxfp || ratio < 2) {
 		dev_err(dai->dev, "the ratio is out of range (2 ~ %d)\n",
-- 
2.16.2

Re: [PATCH] ASoC: fsl_esai: Clear the xPM bit when using xFP

[Fabio Estevam]

On Sat, Apr 7, 2018 at 10:02 AM, Marek Vasut <marex@denx.de> wrote:
> When setting xFP directly, set the xPM predivider to 1, otherwise
> it could remain set to previously set incorrect value and interfere
> with the correct clocking.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Gustavo A. R. Silva <garsilva@embeddedor.com>
> Cc: Mark Brown <broonie@kernel.org>

Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com>

Re: [PATCH] ASoC: fsl_esai: Clear the xPM bit when using xFP

[Nicolin Chen]

On Sat, Apr 07, 2018 at 03:02:21PM +0200, Marek Vasut wrote:
> When setting xFP directly, set the xPM predivider to 1, otherwise
> it could remain set to previously set incorrect value and interfere
> with the correct clocking.

This doesn't sound right to me. Could you please provide a failed
instance? It's been a while since I wrote the code. But I can tell
that PM is supposed to be called by set_sysclk() only, while FP is
used for bclk. If you clear PM when setting FP, the output of HCK
could be messed.

Thanks
Nicolin

> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Gustavo A. R. Silva <garsilva@embeddedor.com>
> Cc: Mark Brown <broonie@kernel.org>
> ---
>  sound/soc/fsl/fsl_esai.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> index 40a700493f4c..9f69823b50d7 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -128,8 +128,11 @@ static int fsl_esai_divisor_cal(struct snd_soc_dai *dai, bool tx, u32 ratio,
>  
>  	maxfp = usefp ? 16 : 1;
>  
> -	if (usefp && fp)
> +	if (usefp && fp) {
> +		regmap_update_bits(esai_priv->regmap, REG_ESAI_xCCR(tx),
> +				   ESAI_xCCR_xPM_MASK, 0);
>  		goto out_fp;
> +	}
>  
>  	if (ratio > 2 * 8 * 256 * maxfp || ratio < 2) {
>  		dev_err(dai->dev, "the ratio is out of range (2 ~ %d)\n",
> -- 
> 2.16.2
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: fsl_esai: Clear the xPM bit when using xFP

[Marek Vasut]

On 04/08/2018 03:14 AM, Nicolin Chen wrote:
> On Sat, Apr 07, 2018 at 03:02:21PM +0200, Marek Vasut wrote:
>> When setting xFP directly, set the xPM predivider to 1, otherwise
>> it could remain set to previously set incorrect value and interfere
>> with the correct clocking.
> 
> This doesn't sound right to me.

OK

> Could you please provide a failed
> instance? It's been a while since I wrote the code. But I can tell
> that PM is supposed to be called by set_sysclk() only, while FP is
> used for bclk. If you clear PM when setting FP, the output of HCK
> could be messed.

Try feeding it the following values, The codec I use is PCM1808.

fsl_esai_set_dai_sysclk[227] clk_id=0 freq=24576000 dir=1
fsl_esai_divisor_cal[131] tx=0 ratio=2 usefp=0 fp=0
fsl_esai_set_bclk[322] tx=0 freq=3072000
fsl_esai_divisor_cal[131] tx=0 ratio=8 usefp=1 fp=8

Also, I think there is another bug in the fsl_esai_divisor_cal() now
that I look at it. If usefp = 0, then maxfp = 1 , then savesub = 0 and
then in the loop sub is never < savesub , although the loop will
terminate on savesub == 0 immediately with pm = 999 and the driver will
fail.

> Thanks
> Nicolin
> 
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Fabio Estevam <fabio.estevam@nxp.com>
>> Cc: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> Cc: Mark Brown <broonie@kernel.org>
>> ---
>>  sound/soc/fsl/fsl_esai.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
>> index 40a700493f4c..9f69823b50d7 100644
>> --- a/sound/soc/fsl/fsl_esai.c
>> +++ b/sound/soc/fsl/fsl_esai.c
>> @@ -128,8 +128,11 @@ static int fsl_esai_divisor_cal(struct snd_soc_dai *dai, bool tx, u32 ratio,
>>  
>>  	maxfp = usefp ? 16 : 1;
>>  
>> -	if (usefp && fp)
>> +	if (usefp && fp) {
>> +		regmap_update_bits(esai_priv->regmap, REG_ESAI_xCCR(tx),
>> +				   ESAI_xCCR_xPM_MASK, 0);
>>  		goto out_fp;
>> +	}
>>  
>>  	if (ratio > 2 * 8 * 256 * maxfp || ratio < 2) {
>>  		dev_err(dai->dev, "the ratio is out of range (2 ~ %d)\n",
>> -- 
>> 2.16.2
>>
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel


-- 
Best regards,
Marek Vasut

Re: [PATCH] ASoC: fsl_esai: Clear the xPM bit when using xFP

[Nicolin Chen]

On Sun, Apr 08, 2018 at 04:16:39AM +0200, Marek Vasut wrote:

> > Could you please provide a failed
> > instance? It's been a while since I wrote the code. But I can tell
> > that PM is supposed to be called by set_sysclk() only, while FP is
> > used for bclk. If you clear PM when setting FP, the output of HCK
> > could be messed.
> 
> Try feeding it the following values, The codec I use is PCM1808.

> [1] fsl_esai_set_dai_sysclk[227] clk_id=0 freq=24576000 dir=1
> [2] fsl_esai_divisor_cal[131] tx=0 ratio=2 usefp=0 fp=0
> [3] fsl_esai_set_bclk[322] tx=0 freq=3072000
> [4] fsl_esai_divisor_cal[131] tx=0 ratio=8 usefp=1 fp=8

So [1] tries to output HCKT (using FSYS) and to set its freq
at 24MHz. If [2] is correct, you'd have HCKT outputting 24MHz.

Let's put this in the graph:
FSYS (48MHz) --> PSR & PM (input_rate / 2) --> HCKT (24MHz)

[3] tries to output SCKT (bit clock) at 3MHz. So you got fp=8
and usefp=1. The combination of usefp=1 and fp=8 should bypass
PSR/PM settings because both should be correctly configured:

HCKT (24MHz) --> FP (input_rate / 8) --> SCKT (3MHz)

If you set PM=0 at [4], the HCKT at [2] would be changed. So
your change shouldn't be a good fix. Meanwhile, I don't see
any problem of the driver at the FP/PM part. I think you can
help me figure it out as it might because of something else.
You may dump the PSR, PM here to check if HCKT is 24MHz.

> Also, I think there is another bug in the fsl_esai_divisor_cal() now
> that I look at it.

> If usefp = 0, then maxfp = 1 , then savesub = 0 and

I see. This actually would happen when PSR=0. And the ratio in
this case is <= 256 (which could be satisfied by PM only). Btw,
is this the reason why you got a "dirty" PM?

Anyway, this part is a bug. Would you like to fix it?

Thanks
Nicolin

Re: [PATCH] ASoC: fsl_esai: Clear the xPM bit when using xFP

[Marek Vasut]

On 04/08/2018 06:01 AM, Nicolin Chen wrote:
> On Sun, Apr 08, 2018 at 04:16:39AM +0200, Marek Vasut wrote:
> 
>>> Could you please provide a failed
>>> instance? It's been a while since I wrote the code. But I can tell
>>> that PM is supposed to be called by set_sysclk() only, while FP is
>>> used for bclk. If you clear PM when setting FP, the output of HCK
>>> could be messed.
>>
>> Try feeding it the following values, The codec I use is PCM1808.
> 
>> [1] fsl_esai_set_dai_sysclk[227] clk_id=0 freq=24576000 dir=1
>> [2] fsl_esai_divisor_cal[131] tx=0 ratio=2 usefp=0 fp=0
>> [3] fsl_esai_set_bclk[322] tx=0 freq=3072000
>> [4] fsl_esai_divisor_cal[131] tx=0 ratio=8 usefp=1 fp=8

Sorry for confusing you, clk_id in [1] should be 3 .
[...]
>> Also, I think there is another bug in the fsl_esai_divisor_cal() now
>> that I look at it.
> 
>> If usefp = 0, then maxfp = 1 , then savesub = 0 and
> 
> I see. This actually would happen when PSR=0. And the ratio in
> this case is <= 256 (which could be satisfied by PM only). Btw,
> is this the reason why you got a "dirty" PM?
> 
> Anyway, this part is a bug. Would you like to fix it?

Clearly I'm getting a bit lost on this convoluted clock calculation.
What is the code trying so elaborately to come up with, ideal
configuration for each of the clock ?

-- 
Best regards,
Marek Vasut

Re: [PATCH] ASoC: fsl_esai: Clear the xPM bit when using xFP

[Nicolin Chen]

On Sun, Apr 08, 2018 at 01:00:07PM +0200, Marek Vasut wrote:
> On 04/08/2018 06:01 AM, Nicolin Chen wrote:
> > On Sun, Apr 08, 2018 at 04:16:39AM +0200, Marek Vasut wrote:
> > 
> >>> Could you please provide a failed
> >>> instance? It's been a while since I wrote the code. But I can tell
> >>> that PM is supposed to be called by set_sysclk() only, while FP is
> >>> used for bclk. If you clear PM when setting FP, the output of HCK
> >>> could be messed.
> >>
> >> Try feeding it the following values, The codec I use is PCM1808.
> > 
> >> [1] fsl_esai_set_dai_sysclk[227] clk_id=0 freq=24576000 dir=1
> >> [2] fsl_esai_divisor_cal[131] tx=0 ratio=2 usefp=0 fp=0
> >> [3] fsl_esai_set_bclk[322] tx=0 freq=3072000
> >> [4] fsl_esai_divisor_cal[131] tx=0 ratio=8 usefp=1 fp=8
> 
> Sorry for confusing you, clk_id in [1] should be 3 .

No worries, it doesn't change the program flow. But it makes
sense now as the input is 48MHz.

> [...]
> >> Also, I think there is another bug in the fsl_esai_divisor_cal() now
> >> that I look at it.
> > 
> >> If usefp = 0, then maxfp = 1 , then savesub = 0 and
> > 
> > I see. This actually would happen when PSR=0. And the ratio in
> > this case is <= 256 (which could be satisfied by PM only). Btw,
> > is this the reason why you got a "dirty" PM?
> > 
> > Anyway, this part is a bug. Would you like to fix it?
> 
> Clearly I'm getting a bit lost on this convoluted clock calculation.
> What is the code trying so elaborately to come up with, ideal
> configuration for each of the clock ?

It just tires to get the closet ratio to set three divisors
correspondingly. Your case should be simpler but the program
didn't cover. It's a design flaw. I will figure out the best
fix and send it soon -- will put you in Reported-by and Cc.

Thanks

Re: [PATCH] ASoC: fsl_esai: Clear the xPM bit when using xFP

[Marek Vasut]

On 04/08/2018 09:27 PM, Nicolin Chen wrote:
> On Sun, Apr 08, 2018 at 01:00:07PM +0200, Marek Vasut wrote:
>> On 04/08/2018 06:01 AM, Nicolin Chen wrote:
>>> On Sun, Apr 08, 2018 at 04:16:39AM +0200, Marek Vasut wrote:
>>>
>>>>> Could you please provide a failed
>>>>> instance? It's been a while since I wrote the code. But I can tell
>>>>> that PM is supposed to be called by set_sysclk() only, while FP is
>>>>> used for bclk. If you clear PM when setting FP, the output of HCK
>>>>> could be messed.
>>>>
>>>> Try feeding it the following values, The codec I use is PCM1808.
>>>
>>>> [1] fsl_esai_set_dai_sysclk[227] clk_id=0 freq=24576000 dir=1
>>>> [2] fsl_esai_divisor_cal[131] tx=0 ratio=2 usefp=0 fp=0
>>>> [3] fsl_esai_set_bclk[322] tx=0 freq=3072000
>>>> [4] fsl_esai_divisor_cal[131] tx=0 ratio=8 usefp=1 fp=8
>>
>> Sorry for confusing you, clk_id in [1] should be 3 .
> 
> No worries, it doesn't change the program flow. But it makes
> sense now as the input is 48MHz.

Except with this configuration , the audio doesn't work without this
patch. I'm happy to receive any feedback on why or what is the problem.

>> [...]
>>>> Also, I think there is another bug in the fsl_esai_divisor_cal() now
>>>> that I look at it.
>>>
>>>> If usefp = 0, then maxfp = 1 , then savesub = 0 and
>>>
>>> I see. This actually would happen when PSR=0. And the ratio in
>>> this case is <= 256 (which could be satisfied by PM only). Btw,
>>> is this the reason why you got a "dirty" PM?
>>>
>>> Anyway, this part is a bug. Would you like to fix it?
>>
>> Clearly I'm getting a bit lost on this convoluted clock calculation.
>> What is the code trying so elaborately to come up with, ideal
>> configuration for each of the clock ?
> 
> It just tires to get the closet ratio to set three divisors
> correspondingly. Your case should be simpler but the program
> didn't cover. It's a design flaw. I will figure out the best
> fix and send it soon -- will put you in Reported-by and Cc.

OK. I wonder though, don't we have code to calculate dividers in the clk
framework already? Why is it reinvented in here ?

> Thanks
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH] ASoC: fsl_esai: Clear the xPM bit when using xFP

[Nicolin Chen]

On Sun, Apr 08, 2018 at 09:33:52PM +0200, Marek Vasut wrote:

> >>>> Try feeding it the following values, The codec I use is PCM1808.
> >>>
> >>>> [1] fsl_esai_set_dai_sysclk[227] clk_id=0 freq=24576000 dir=1
> >>>> [2] fsl_esai_divisor_cal[131] tx=0 ratio=2 usefp=0 fp=0
> >>>> [3] fsl_esai_set_bclk[322] tx=0 freq=3072000
> >>>> [4] fsl_esai_divisor_cal[131] tx=0 ratio=8 usefp=1 fp=8
> >>
> >> Sorry for confusing you, clk_id in [1] should be 3 .
> > 
> > No worries, it doesn't change the program flow. But it makes
> > sense now as the input is 48MHz.
> 
> Except with this configuration , the audio doesn't work without this
> patch. I'm happy to receive any feedback on why or what is the problem.

I sent a patch to you and in the maillist for review. Would
you please test it and sent a Tested-by to the patch? Thanks

> >> [...]
> >>>> Also, I think there is another bug in the fsl_esai_divisor_cal() now
> >>>> that I look at it.
> >>>
> >>>> If usefp = 0, then maxfp = 1 , then savesub = 0 and
> >>>
> >>> I see. This actually would happen when PSR=0. And the ratio in
> >>> this case is <= 256 (which could be satisfied by PM only). Btw,
> >>> is this the reason why you got a "dirty" PM?
> >>>
> >>> Anyway, this part is a bug. Would you like to fix it?
> >>
> >> Clearly I'm getting a bit lost on this convoluted clock calculation.
> >> What is the code trying so elaborately to come up with, ideal
> >> configuration for each of the clock ?
> > 
> > It just tires to get the closet ratio to set three divisors
> > correspondingly. Your case should be simpler but the program
> > didn't cover. It's a design flaw. I will figure out the best
> > fix and send it soon -- will put you in Reported-by and Cc.
> 
> OK. I wonder though, don't we have code to calculate dividers in the clk
> framework already? Why is it reinvented in here ?

Well, I wrote the code four years ago, based on a much older
downstream kernel; the code also got upstream that time too.
Even NXP's latest code doesn't seemly have some diff at this
part. I guess no one had reported this bug to them or to me
until you did yesterday...

Re: [PATCH] ASoC: fsl_esai: Clear the xPM bit when using xFP

[Marek Vasut]

On 04/09/2018 02:07 AM, Nicolin Chen wrote:
> On Sun, Apr 08, 2018 at 09:33:52PM +0200, Marek Vasut wrote:
> 
>>>>>> Try feeding it the following values, The codec I use is PCM1808.
>>>>>
>>>>>> [1] fsl_esai_set_dai_sysclk[227] clk_id=0 freq=24576000 dir=1
>>>>>> [2] fsl_esai_divisor_cal[131] tx=0 ratio=2 usefp=0 fp=0
>>>>>> [3] fsl_esai_set_bclk[322] tx=0 freq=3072000
>>>>>> [4] fsl_esai_divisor_cal[131] tx=0 ratio=8 usefp=1 fp=8
>>>>
>>>> Sorry for confusing you, clk_id in [1] should be 3 .
>>>
>>> No worries, it doesn't change the program flow. But it makes
>>> sense now as the input is 48MHz.
>>
>> Except with this configuration , the audio doesn't work without this
>> patch. I'm happy to receive any feedback on why or what is the problem.
> 
> I sent a patch to you and in the maillist for review. Would
> you please test it and sent a Tested-by to the patch? Thanks

Done, thanks for your help! :)

-- 
Best regards,
Marek Vasut