| Message ID | 20180407130221.24770-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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
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
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
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 ?
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
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 >
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...
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! :)
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",
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(-)