[alsa-devel] [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support
From: "Chen.Liu" chen.liu.opensource@gmail.com
Issue: MCLK=24MHZ,SYSCLOCK=12.288MHZ. When the 'wm8960_set_pll' function is called,the driver will prompted "WM8960 PLL: unsupported N = 4" error message. However,the value of PLLN should be 8 based on the table45( PLL Frequency Examples) of the wm8960 chip manuanl.
Reason: The intergrated PLL can be used to generate SYSCLK for the wm8960. The pll_factors function in the wm8960.c file is mainly responsible for setting the divider factor of the PLL divider with reference to MCLK clock frequency to obtain the required SYSCLK clock frequency. The configure diagram for this function can be seen below.
MCLK-->[f/2]-->[R=f2/f1]-->[f/4]-->[f/N](1)-->SYSCLOCK
The configuration of the PLL divider in above diagram is applicable to MCLK clock frequency is less than or equal to 14.4MHZ. examples: (1)MCLK=12MHz, required clock = 12.288MHz. R should be chosen to ensure 5 < PLLN < 13. There is a fixed divide by 4 in the PLL and a selectable divide by N after the PLL which should be set to divide by 1 to meet this requirement. Enabling the divide by 1 sets the required f2 = 4 x 1 x 12.288MHz = 49.152MHz.
R = 49.152 / (12 / 2) = 8.192 PLLN = int R = 8
(2)MCLK=24MHz, required clock = 12.288MHz. Enabling the divide by 1 sets the required f2 = 4 x 1 x 12.288MHZ = 49.152MHZ.
R = 49.152 / (24 / 2) = 4.096 PLLN = int R = 4
And if the [f/N] reg is set to 2,then the required f2 = 4x2x12.288MHZ = 98.304MHZ.
R = 98.304 / (24 / 2) = 8.192 PLLN = int R = 8
Because the [f/N] reg is fixed divide by 1,that's why driver prompted error message.
Solution: The purpose of this patch is when the conditions(5 < PLLN < 13) is false, set the register[f/N] to be 2, and the recalculate the divide factor of the PLL divider.
MCLK-->[f/2]-->[R=f2/f1]-->[f/4]-->[f/N](2)-->SYSCLOCK
Signed-off-by: Chen.Liu chen.liu.opensource@gmail.com --- Changes in v2: - The calculation of the Ndiv is encapsulated in a loop. - Use 'unsupported' instead of 'unsupport'. --- sound/soc/codecs/wm8960.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c index 997c446..83dd746 100644 --- a/sound/soc/codecs/wm8960.c +++ b/sound/soc/codecs/wm8960.c @@ -1036,28 +1036,38 @@ static bool is_pll_freq_available(unsigned int source, unsigned int target) * to allow rounding later */ #define FIXED_PLL_SIZE ((1 << 24) * 10)
-static int pll_factors(unsigned int source, unsigned int target, +static int pll_factors(struct snd_soc_codec *codec, + unsigned int source, unsigned int target, struct _pll_div *pll_div) { unsigned long long Kpart; unsigned int K, Ndiv, Nmod; + int unsupported = 0;
pr_debug("WM8960 PLL: setting %dHz->%dHz\n", source, target);
/* Scale up target to PLL operating frequency */ target *= 4;
- Ndiv = target / source; - if (Ndiv < 6) { - source >>= 1; - pll_div->pre_div = 1; + while (1) { Ndiv = target / source; - } else - pll_div->pre_div = 0; + if (Ndiv < 6) { + source >>= 1; + pll_div->pre_div = 1; + Ndiv = target / source; + } else + pll_div->pre_div = 0; + + if ((Ndiv < 6) || (Ndiv > 12)) { + if (unsupported == 1) { + pr_err("WM8960 PLL: Unsupported N=%d\n", Ndiv); + return -EINVAL; + } + } else + break;
- if ((Ndiv < 6) || (Ndiv > 12)) { - pr_err("WM8960 PLL: Unsupported N=%d\n", Ndiv); - return -EINVAL; + target *= 2; + unsupported += 1; }
pll_div->n = Ndiv; @@ -1077,6 +1087,10 @@ static int pll_factors(unsigned int source, unsigned int target,
pll_div->k = K;
+ if (unsupported) + snd_soc_update_bits(codec, WM8960_CLOCK1, 0x6, + WM8960_SYSCLK_DIV_2); + pr_debug("WM8960 PLL: N=%x K=%x pre_div=%d\n", pll_div->n, pll_div->k, pll_div->pre_div);
@@ -1091,7 +1105,7 @@ static int wm8960_set_pll(struct snd_soc_codec *codec, int ret;
if (freq_in && freq_out) { - ret = pll_factors(freq_in, freq_out, &pll_div); + ret = pll_factors(codec, freq_in, freq_out, &pll_div); if (ret != 0) return ret; }
On Wed, Dec 13, 2017 at 08:37:30PM +0800, Chen.Liu wrote:
From: "Chen.Liu" chen.liu.opensource@gmail.com
Issue: MCLK=24MHZ,SYSCLOCK=12.288MHZ. When the 'wm8960_set_pll' function is called,the driver will prompted "WM8960 PLL: unsupported N = 4" error message. However,the value of PLLN should be 8 based on the table45( PLL Frequency Examples) of the wm8960 chip manuanl.
Reason: The intergrated PLL can be used to generate SYSCLK for the wm8960. The pll_factors function in the wm8960.c file is mainly responsible for setting the divider factor of the PLL divider with reference to MCLK clock frequency to obtain the required SYSCLK clock frequency. The configure diagram for this function can be seen below.
MCLK-->[f/2]-->[R=f2/f1]-->[f/4]-->[f/N](1)-->SYSCLOCK
The configuration of the PLL divider in above diagram is applicable to MCLK clock frequency is less than or equal to 14.4MHZ. examples: (1)MCLK=12MHz, required clock = 12.288MHz. R should be chosen to ensure 5 < PLLN < 13. There is a fixed divide by 4 in the PLL and a selectable divide by N after the PLL which should be set to divide by 1 to meet this requirement. Enabling the divide by 1 sets the required f2 = 4 x 1 x 12.288MHz = 49.152MHz.
R = 49.152 / (12 / 2) = 8.192 PLLN = int R = 8
(2)MCLK=24MHz, required clock = 12.288MHz. Enabling the divide by 1 sets the required f2 = 4 x 1 x 12.288MHZ = 49.152MHZ.
R = 49.152 / (24 / 2) = 4.096 PLLN = int R = 4
And if the [f/N] reg is set to 2,then the required f2 = 4x2x12.288MHZ = 98.304MHZ.
R = 98.304 / (24 / 2) = 8.192 PLLN = int R = 8
Because the [f/N] reg is fixed divide by 1,that's why driver prompted error message.
Solution: The purpose of this patch is when the conditions(5 < PLLN < 13) is false, set the register[f/N] to be 2, and the recalculate the divide factor of the PLL divider.
MCLK-->[f/2]-->[R=f2/f1]-->[f/4]-->[f/N](2)-->SYSCLOCK
Signed-off-by: Chen.Liu chen.liu.opensource@gmail.com
+ Daniel Baluta, as he has done quite a bit of work on the clocking in this driver and I would like to at least hear this doesn't cause issues in his systems.
Changes in v2:
- The calculation of the Ndiv is encapsulated in a loop.
- Use 'unsupported' instead of 'unsupport'.
sound/soc/codecs/wm8960.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c index 997c446..83dd746 100644 --- a/sound/soc/codecs/wm8960.c +++ b/sound/soc/codecs/wm8960.c @@ -1036,28 +1036,38 @@ static bool is_pll_freq_available(unsigned int source, unsigned int target)
- to allow rounding later */
#define FIXED_PLL_SIZE ((1 << 24) * 10)
-static int pll_factors(unsigned int source, unsigned int target, +static int pll_factors(struct snd_soc_codec *codec,
unsigned int source, unsigned int target, struct _pll_div *pll_div){ unsigned long long Kpart; unsigned int K, Ndiv, Nmod;
int unsupported = 0;
pr_debug("WM8960 PLL: setting %dHz->%dHz\n", source, target);
/* Scale up target to PLL operating frequency */ target *= 4;
- Ndiv = target / source;
- if (Ndiv < 6) {
source >>= 1;pll_div->pre_div = 1;
- while (1) { Ndiv = target / source;
- } else
pll_div->pre_div = 0;
if (Ndiv < 6) {source >>= 1;pll_div->pre_div = 1;Ndiv = target / source;} elsepll_div->pre_div = 0;if ((Ndiv < 6) || (Ndiv > 12)) {if (unsupported == 1) {pr_err("WM8960 PLL: Unsupported N=%d\n", Ndiv);return -EINVAL;}} elsebreak;
- if ((Ndiv < 6) || (Ndiv > 12)) {
pr_err("WM8960 PLL: Unsupported N=%d\n", Ndiv);return -EINVAL;
target *= 2;unsupported += 1;}
pll_div->n = Ndiv;
@@ -1077,6 +1087,10 @@ static int pll_factors(unsigned int source, unsigned int target,
pll_div->k = K;
- if (unsupported)
snd_soc_update_bits(codec, WM8960_CLOCK1, 0x6,WM8960_SYSCLK_DIV_2);
Looking at this a bit more I do have some reservations. Firstly this divider can be set through wm8960_set_dai_clkdiv, and secondly it is also set at the bottom of wm8960_configure_clocking. Adding a third path that also controls this bit is starting to seem very complex. Do you have any thoughts on how this interacts with the other two paths? And should it perhaps take this field into account but not be controlling it directly?
Thanks, Charles
pr_debug("WM8960 PLL: N=%x K=%x pre_div=%d\n", pll_div->n, pll_div->k, pll_div->pre_div);
@@ -1091,7 +1105,7 @@ static int wm8960_set_pll(struct snd_soc_codec *codec, int ret;
if (freq_in && freq_out) {
ret = pll_factors(freq_in, freq_out, &pll_div);
ret = pll_factors(codec, freq_in, freq_out, &pll_div);-- 1.8.3.1
On Thu, Dec 14, 2017 at 11:56:48AM +0000, Charles Keepax wrote:
On Wed, Dec 13, 2017 at 08:37:30PM +0800, Chen.Liu wrote:
- if (unsupported)
snd_soc_update_bits(codec, WM8960_CLOCK1, 0x6,WM8960_SYSCLK_DIV_2);
Looking at this a bit more I do have some reservations. Firstly this divider can be set through wm8960_set_dai_clkdiv, and secondly it is also set at the bottom of
Removing set_clkdiv() would be a better thing if there were conflicts here, in general we're trying to avoid uses of it.
On Thu, Dec 14, 2017 at 03:31:39PM +0000, Mark Brown wrote:
On Thu, Dec 14, 2017 at 11:56:48AM +0000, Charles Keepax wrote:
On Wed, Dec 13, 2017 at 08:37:30PM +0800, Chen.Liu wrote:
- if (unsupported)
snd_soc_update_bits(codec, WM8960_CLOCK1, 0x6,WM8960_SYSCLK_DIV_2);Looking at this a bit more I do have some reservations. Firstly this divider can be set through wm8960_set_dai_clkdiv, and secondly it is also set at the bottom of
Removing set_clkdiv() would be a better thing if there were conflicts here, in general we're trying to avoid uses of it.
Ok that can probably be done as a separate patch I will review again with that in mind.
Thanks, Charles
On Thu, Dec 14, 2017 at 03:51:51PM +0000, Charles Keepax wrote:
On Thu, Dec 14, 2017 at 03:31:39PM +0000, Mark Brown wrote:
Removing set_clkdiv() would be a better thing if there were conflicts here, in general we're trying to avoid uses of it.
Ok that can probably be done as a separate patch I will review again with that in mind.
Yeah. It does depend a bit if something's using it but I don't *think* so.
On Wed, Dec 13, 2017 at 08:37:30PM +0800, Chen.Liu wrote:
From: "Chen.Liu" chen.liu.opensource@gmail.com diff --git a/sound/soc/codecs/wm8960.c b/sound/soc/codecs/wm8960.c index 997c446..83dd746 100644 --- a/sound/soc/codecs/wm8960.c +++ b/sound/soc/codecs/wm8960.c @@ -1036,28 +1036,38 @@ static bool is_pll_freq_available(unsigned int source, unsigned int target)
- to allow rounding later */
#define FIXED_PLL_SIZE ((1 << 24) * 10)
-static int pll_factors(unsigned int source, unsigned int target, +static int pll_factors(struct snd_soc_codec *codec,
unsigned int source, unsigned int target, struct _pll_div *pll_div){ unsigned long long Kpart; unsigned int K, Ndiv, Nmod;
int unsupported = 0;
pr_debug("WM8960 PLL: setting %dHz->%dHz\n", source, target);
/* Scale up target to PLL operating frequency */ target *= 4;
- Ndiv = target / source;
- if (Ndiv < 6) {
source >>= 1;pll_div->pre_div = 1;
- while (1) { Ndiv = target / source;
- } else
pll_div->pre_div = 0;
if (Ndiv < 6) {source >>= 1;pll_div->pre_div = 1;Ndiv = target / source;} elsepll_div->pre_div = 0;if ((Ndiv < 6) || (Ndiv > 12)) {if (unsupported == 1) {pr_err("WM8960 PLL: Unsupported N=%d\n", Ndiv);return -EINVAL;}} elsebreak;
- if ((Ndiv < 6) || (Ndiv > 12)) {
pr_err("WM8960 PLL: Unsupported N=%d\n", Ndiv);return -EINVAL;
target *= 2;unsupported += 1;}
pll_div->n = Ndiv;
@@ -1077,6 +1087,10 @@ static int pll_factors(unsigned int source, unsigned int target,
pll_div->k = K;
- if (unsupported)
snd_soc_update_bits(codec, WM8960_CLOCK1, 0x6,WM8960_SYSCLK_DIV_2);
OK so looking over this some more with Mark's comments in mind the thing that troubles me is this. Currently there are two ways you can configure the clocking manually or automatically.
The manual way using the set_dai_pll, set_dai_clkdiv and set_dai_sysclk calls.
Automatically by calling set_dai_sysclk and set_dai_pll with WM8960_SYSCLK_AUTO and the driver will work it out for you. This option already supports setting the SYSCLK_DIV.
I guess my first question would be can you get the result you desire by just using the automatic option?
If not, it seems like what we are trying to do here is move the set_dai_clkdiv for SYSCLK_DIV to be automatic in both cases. The problem is that I suspect this handling interfers somewhat with the handling of these bits that is done in wm8960_configure_clocking through wm8960_configure_pll when we are doing things automatically. In which case I think you need to look at how to align these two clocking methods.
Thanks, Charles
2017-12-15 0:19 GMT+08:00 Charles Keepax ckeepax@opensource.cirrus.com:
On Wed, Dec 13, 2017 at 08:37:30PM +0800, Chen.Liu wrote:
OK so looking over this some more with Mark's comments in mind the thing that troubles me is this. Currently there are two ways you can configure the clocking manually or automatically.
The manual way using the set_dai_pll, set_dai_clkdiv and set_dai_sysclk calls.
Automatically by calling set_dai_sysclk and set_dai_pll with WM8960_SYSCLK_AUTO and the driver will work it out for you. This option already supports setting the SYSCLK_DIV.
I guess my first question would be can you get the result you desire by just using the automatic option?
If not, it seems like what we are trying to do here is move the set_dai_clkdiv for SYSCLK_DIV to be automatic in both cases. The problem is that I suspect this handling interfers somewhat with the handling of these bits that is done in wm8960_configure_clocking through wm8960_configure_pll when we are doing things automatically. In which case I think you need to look at how to align these two clocking methods.
Hi Charles,
When the MCLK clock frequency cannot meet the SYSCLK clock frequency of the wm8960 codec,we must use the PLL divider to generate the clock frequency.
For what you are talking about setting the SYSCLK clock frequency automatically and manually,the difference is that one uses the PLL divider to generate the SYSCLK clock frequency and the other one provides the SYSCLK clock frequency directly through the MCLK.
Although the driver already supports setting the SYSCLK_DIV register, it can only be set to 1,thus causing the codec to not support multiple MCLK clock frequency.(For more information, please see the wm8960 codec manual)
The important purpose of this patch is to support multiple MCLK clock frequency.
Thanks again, Chen.
On Fri, Dec 15, 2017 at 09:07:15PM +0800, chen liu wrote:
2017-12-15 0:19 GMT+08:00 Charles Keepax ckeepax@opensource.cirrus.com:
On Wed, Dec 13, 2017 at 08:37:30PM +0800, Chen.Liu wrote:
When the MCLK clock frequency cannot meet the SYSCLK clock frequency of the wm8960 codec,we must use the PLL divider to generate the clock frequency.
For what you are talking about setting the SYSCLK clock frequency automatically and manually,the difference is that one uses the PLL divider to generate the SYSCLK clock frequency and the other one provides the SYSCLK clock frequency directly through the MCLK.
Although the driver already supports setting the SYSCLK_DIV register, it can only be set to 1,thus causing the codec to not support multiple MCLK clock frequency.(For more information, please see the wm8960 codec manual)
The important purpose of this patch is to support multiple MCLK clock frequency.
Sorry I think I am not following something here, could you be a bit more specific, especially about why the automatic approach doesn't meet your needs.
As far as I can see this will let you set the SYSCLKDIV using the manual method and it can be easily set to either 1 or 2:
static int wm8960_set_dai_clkdiv(struct snd_soc_dai *codec_dai, ... switch (div_id) { case WM8960_SYSCLKDIV: reg = snd_soc_read(codec, WM8960_CLOCK1) & 0x1f9; snd_soc_write(codec, WM8960_CLOCK1, reg | div); break; ...
And as for the automatic method:
static int wm8960_configure_pll(struct snd_soc_codec *codec, int freq_in, ... for (i = 0; i < ARRAY_SIZE(sysclk_divs); ++i) { if (sysclk_divs[i] == -1) continue; ...
This will iterate through all the sysclk_dividers and pick one that is appropriate either 1 or 2. So in what way does the automatic method not support a SYSCLK_DIV of 2?
Thanks, Charles
2017-12-18 17:31 GMT+08:00 Charles Keepax ckeepax@opensource.cirrus.com:
On Fri, Dec 15, 2017 at 09:07:15PM +0800, chen liu wrote:
2017-12-15 0:19 GMT+08:00 Charles Keepax <ckeepax@opensource.cirrus.com :
On Wed, Dec 13, 2017 at 08:37:30PM +0800, Chen.Liu wrote:
When the MCLK clock frequency cannot meet the SYSCLK clock frequency of the wm8960 codec,we must use the PLL divider to generate the clock frequency.
For what you are talking about setting the SYSCLK clock frequency automatically and manually,the difference is that one uses the PLL divider to generate the SYSCLK clock frequency and the other one provides the SYSCLK clock frequency directly through the MCLK.
Although the driver already supports setting the SYSCLK_DIV register, it can only be set to 1,thus causing the codec to not support multiple MCLK clock frequency.(For more information, please see the wm8960 codec manual)
The important purpose of this patch is to support multiple MCLK clock frequency.
Sorry I think I am not following something here, could you be a bit more specific, especially about why the automatic approach doesn't meet your needs.
As far as I can see this will let you set the SYSCLKDIV using the manual method and it can be easily set to either 1 or 2:
static int wm8960_set_dai_clkdiv(struct snd_soc_dai *codec_dai, ... switch (div_id) { case WM8960_SYSCLKDIV: reg = snd_soc_read(codec, WM8960_CLOCK1) & 0x1f9; snd_soc_write(codec, WM8960_CLOCK1, reg | div); break; ...
And as for the automatic method:
static int wm8960_configure_pll(struct snd_soc_codec *codec, int freq_in, ... for (i = 0; i < ARRAY_SIZE(sysclk_divs); ++i) { if (sysclk_divs[i] == -1) continue; ...
This will iterate through all the sysclk_dividers and pick one that is appropriate either 1 or 2. So in what way does the automatic method not support a SYSCLK_DIV of 2?
Hi Charles,
Thanks for your quickly reply.
According to your detailed description above, I understand what you mean. For the 'wm8960_configure_pll' function,it deduces a reasonable PLL output clock frequency based on the 'freq_in' frequency,the sample rate,and the bit clock.
static int wm8960_configure_clocking(struct snd_soc_codec*codec) ... freq_out = wm8960_configure_pll(codec, freq_in, &i, &j, &k); if (freq_out < 0) { dev_err(codec->dev, "failed to configure clock via PLL\n"); return freq_out; } wm8960_set_pll(codec, freq_in, freq_out); ...
In the 'wm8960_configure_clocking' function, it sets the PLL divider by calling the 'wm8960_set_pll' function after calling the 'wm8960_configure_pll'. However,there is no support for SYSCLK_DIV = 2 in the 'wm8960_set_pll' function.
Looking forward to your reply.
Thanks, Chen.
On Mon, Dec 18, 2017 at 07:32:41PM +0800, chen liu wrote:
2017-12-18 17:31 GMT+08:00 Charles Keepax ckeepax@opensource.cirrus.com:
On Fri, Dec 15, 2017 at 09:07:15PM +0800, chen liu wrote:
2017-12-15 0:19 GMT+08:00 Charles Keepax <ckeepax@opensource.cirrus.com :
On Wed, Dec 13, 2017 at 08:37:30PM +0800, Chen.Liu wrote:
According to your detailed description above, I understand what you mean. For the 'wm8960_configure_pll' function,it deduces a reasonable PLL output clock frequency based on the 'freq_in' frequency,the sample rate,and the bit clock.
static int wm8960_configure_clocking(struct snd_soc_codec*codec) ... freq_out = wm8960_configure_pll(codec, freq_in, &i, &j, &k); if (freq_out < 0) { dev_err(codec->dev, "failed to configure clock via PLL\n"); return freq_out; } wm8960_set_pll(codec, freq_in, freq_out); ...
In the 'wm8960_configure_clocking' function, it sets the PLL divider by calling the 'wm8960_set_pll' function after calling the 'wm8960_configure_pll'. However,there is no support for SYSCLK_DIV = 2 in the 'wm8960_set_pll' function.
Looking forward to your reply.
Indeed yes, as it looks like the intention was you would set the SYSCLKDIV manually if setting the PLL manually. But why not just call wm8960_set_pll will WM8960_SYSCLK_AUTO, and then your code will use the configure_pll stuff?
I would like to understand what about that approach isn't working for you as that seems like the easiest solution.
Thanks, Charles
2017-12-18 19:55 GMT+08:00 Charles Keepax ckeepax@opensource.cirrus.com:
On Mon, Dec 18, 2017 at 07:32:41PM +0800, chen liu wrote:
2017-12-18 17:31 GMT+08:00 Charles Keepax <ckeepax@opensource.cirrus.com :
On Fri, Dec 15, 2017 at 09:07:15PM +0800, chen liu wrote:
2017-12-15 0:19 GMT+08:00 Charles Keepax <
ckeepax@opensource.cirrus.com
:
On Wed, Dec 13, 2017 at 08:37:30PM +0800, Chen.Liu wrote:
According to your detailed description above, I understand what you mean. For the 'wm8960_configure_pll' function,it deduces a reasonable PLL
output
clock frequency based on the 'freq_in' frequency,the sample rate,and the
bit
clock.
static int wm8960_configure_clocking(struct snd_soc_codec*codec) ... freq_out = wm8960_configure_pll(codec, freq_in, &i, &j, &k); if (freq_out < 0) { dev_err(codec->dev, "failed to configure clock via
PLL\n");
return freq_out; } wm8960_set_pll(codec, freq_in, freq_out);...
In the 'wm8960_configure_clocking' function, it sets the PLL divider by calling the 'wm8960_set_pll' function after calling the 'wm8960_configure_pll'. However,there is no support for SYSCLK_DIV = 2 in the 'wm8960_set_pll' function.
Looking forward to your reply.
Indeed yes, as it looks like the intention was you would set the SYSCLKDIV manually if setting the PLL manually. But why not just call wm8960_set_pll will WM8960_SYSCLK_AUTO, and then your code will use the configure_pll stuff?
I would like to understand what about that approach isn't working for you as that seems like the easiest solution.
Hi Charles,
Thanks for your reply.
For your question,i will explain in detail below.
First at all,we can not call the 'wm8960_set_pll' function directly in the machine driver,but we can call the 'wm8960_set_dai_pll' function with 'WM8960_SYSCLK_AUTO' as a parameter.
example: sample rate = 44100HZ, MCLK = 24MHZ, channel = 2;
In the Machine driver, we call 'snd_soc_dai_set_pll(codec_dai, WM8960_SYSCLK_AUTO, 0, 24000000, 0);' function to automatically configure the clock frequency.
When we playback the audio file,the ALSA middle layer will call the 'wm8960_hw_params' function to configure the hardware parameters and the clock frequency.At the bottom of this function it will call the 'wm8960_configure_clocking' function to configure the clock frequency.
static int wm8960_configure_clocking(struct snd_soc_codec *codec) ... freq_in = wm8960->freq_in; //Should be 24MHZ ... if (wm8960->clk_id == WM8960_SYSCLK_AUTO) { /* disable the PLL and using MCLK to provide sysclk */ wm8960_set_pll(codec, 0, 0); freq_out = freq_in; //should be 24MHZ } else if (wm8960->sysclk) { freq_out = wm8960->sysclk; } else { dev_err(codec->dev, "No SYSCLK configured\n"); return -EINVAL; } ... if (wm8960->clk_id != WM8960_SYSCLK_PLL) { // If the freq_out is 24MHZ,ret will be less than zero. ret = wm8960_configure_sysclk(wm8960, freq_out, &i, &j, &k); if (ret >= 0) { goto configure_clock; } else if (wm8960->clk_id != WM8960_SYSCLK_AUTO) { dev_err(codec->dev, "failed to configure clock\n"); return -EINVAL; } } // Then this branch will be executed. // If the sample rate is 44100HZ, the value of freq_out will be 11.2896MHZ freq_out = wm8960_configure_pll(codec, freq_in, &i, &j, &k); if (freq_out < 0) { dev_err(codec->dev, "failed to configure clock via PLL\n"); return freq_out; } // The 'wm8960_set_pll' function will be executed,this function is very important. // But now, the freq_in is 24MHZ and the freq_out is 11.2896MHZ. wm8960_set_pll(codec, freq_in, freq_out); ...
Because the 'wm8960_configure_pll' function has been pre-scaled by 2 for freq_out, but the value of freq_out is not multiplied by 2 in the 'pll_factors' function, it prints the "WM8960 PLL: Unsupported N =" error message.
Looking forward to your reply.
Thanks, Chen.
On Tue, Dec 19, 2017 at 02:19:48PM +0800, chen liu wrote:
2017-12-18 19:55 GMT+08:00 Charles Keepax ckeepax@opensource.cirrus.com:
On Mon, Dec 18, 2017 at 07:32:41PM +0800, chen liu wrote:
2017-12-18 17:31 GMT+08:00 Charles Keepax <ckeepax@opensource.cirrus.com
On Fri, Dec 15, 2017 at 09:07:15PM +0800, chen liu wrote:
2017-12-15 0:19 GMT+08:00 Charles Keepax <
ckeepax@opensource.cirrus.com
On Wed, Dec 13, 2017 at 08:37:30PM +0800, Chen.Liu wrote:
// Then this branch will be executed. // If the sample rate is 44100HZ, the value of freq_out will be11.2896MHZ freq_out = wm8960_configure_pll(codec, freq_in, &i, &j, &k); if (freq_out < 0) { dev_err(codec->dev, "failed to configure clock via PLL\n"); return freq_out; } // The 'wm8960_set_pll' function will be executed,this function is very important. // But now, the freq_in is 24MHZ and the freq_out is 11.2896MHZ. wm8960_set_pll(codec, freq_in, freq_out); ...
Because the 'wm8960_configure_pll' function has been pre-scaled by 2 for freq_out, but the value of freq_out is not multiplied by 2 in the 'pll_factors' function, it prints the "WM8960 PLL: Unsupported N =" error message.
Ah ok so I hadn't realised you were using the SYSCLK_AUTO stuff and finding an issue with it. Thank you very much for the explanation.
One thing I don't understand though is it looks like the freq_out returned from wm8960_configure_pll should already be taking the SYSCLK_DIV into account:
for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) { sysclk = lrclk * dac_divs[j]; freq_out = sysclk * sysclk_divs[i];
So if lrclk=44100, j=0, i=2 which I believe is the case in question we should get:
sysclk = 44100 * 256 = 11.2896MHz freq_out = 11.2896MHz * 2 = 22.5792MHz
So when wm8960_configure_pll returns i=2 then it should also be setting freq_out as 22.5792MHz. So you final call there to wm8960_set_pll should be:
wm8960_set_pll(codec, 24MHz, 22.5792MHz);
Can you see why that isn't happening?
Thanks, Charles
2017-12-22 0:48 GMT+08:00 Charles Keepax ckeepax@opensource.cirrus.com:
On Tue, Dec 19, 2017 at 02:19:48PM +0800, chen liu wrote:
2017-12-18 19:55 GMT+08:00 Charles Keepax ckeepax@opensource.cirrus.com:
On Mon, Dec 18, 2017 at 07:32:41PM +0800, chen liu wrote:
2017-12-18 17:31 GMT+08:00 Charles Keepax <ckeepax@opensource.cirrus.com
On Fri, Dec 15, 2017 at 09:07:15PM +0800, chen liu wrote:
2017-12-15 0:19 GMT+08:00 Charles Keepax <
ckeepax@opensource.cirrus.com
> On Wed, Dec 13, 2017 at 08:37:30PM +0800, Chen.Liu wrote:
One thing I don't understand though is it looks like the freq_out returned from wm8960_configure_pll should already be taking the SYSCLK_DIV into account:
for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) { sysclk = lrclk * dac_divs[j]; freq_out = sysclk * sysclk_divs[i];
So if lrclk=44100, j=0, i=2 which I believe is the case in question we should get:
sysclk = 44100 * 256 = 11.2896MHz freq_out = 11.2896MHz * 2 = 22.5792MHz
So when wm8960_configure_pll returns i=2 then it should also be setting freq_out as 22.5792MHz. So you final call there to wm8960_set_pll should be:
Hi Charles,
You are right.
This problem seems to affect only the clock frequency configured in manual mode.
Because i'm directly calling the 'snd_soc_dai_set_pll' function in the machine driver,the driver will prompts ''WM8960 PLL: Unsupported N" error message. The following code is part of how i configured the clock frequency in the machine driver: ====================================================== + if (params_rate(params) == 44100) + pll_out = 11289600; + else + pll_out = 12288000; + + ret = snd_soc_dai_set_pll(codec_dai, WM8960_SYSCLK_PLL, 0, + data->clk_frequency, pll_out);
======================================================
On the other hand,i think if the codec support these MCLK clock frequency in the manual,so no matter use manual mode and automatic mode to configure the clock frequency,the driver prompt errors is very bad.
Thanks again, Chen.
On Fri, Dec 22, 2017 at 06:40:04PM +0800, chen liu wrote:
2017-12-22 0:48 GMT+08:00 Charles Keepax ckeepax@opensource.cirrus.com:
On Tue, Dec 19, 2017 at 02:19:48PM +0800, chen liu wrote:
2017-12-18 19:55 GMT+08:00 Charles Keepax ckeepax@opensource.cirrus.com:
On Mon, Dec 18, 2017 at 07:32:41PM +0800, chen liu wrote:
2017-12-18 17:31 GMT+08:00 Charles Keepax <ckeepax@opensource.cirrus.com
On Fri, Dec 15, 2017 at 09:07:15PM +0800, chen liu wrote: > 2017-12-15 0:19 GMT+08:00 Charles Keepax <
ckeepax@opensource.cirrus.com
> > On Wed, Dec 13, 2017 at 08:37:30PM +0800, Chen.Liu wrote:
One thing I don't understand though is it looks like the freq_out returned from wm8960_configure_pll should already be taking the SYSCLK_DIV into account:
for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) { sysclk = lrclk * dac_divs[j]; freq_out = sysclk * sysclk_divs[i];
So if lrclk=44100, j=0, i=2 which I believe is the case in question we should get:
sysclk = 44100 * 256 = 11.2896MHz freq_out = 11.2896MHz * 2 = 22.5792MHz
So when wm8960_configure_pll returns i=2 then it should also be setting freq_out as 22.5792MHz. So you final call there to wm8960_set_pll should be:
This problem seems to affect only the clock frequency configured in manual mode.
Ok cool, so you are using manual mode :-)
Because i'm directly calling the 'snd_soc_dai_set_pll' function in the machine driver,the driver will prompts ''WM8960 PLL: Unsupported N" error message. The following code is part of how i configured the clock frequency in the machine driver: ======================================================
if (params_rate(params) == 44100)pll_out = 11289600;elsepll_out = 12288000;ret = snd_soc_dai_set_pll(codec_dai, WM8960_SYSCLK_PLL, 0,data->clk_frequency, pll_out);
======================================================
On the other hand,i think if the codec support these MCLK clock frequency in the manual,so no matter use manual mode and automatic mode to configure the clock frequency,the driver prompt errors is very bad.
The original intention using the manual path was that you would set the clock divider manually using snd_soc_dai_set_clkdiv:
snd_soc_dai_set_clkdiv(codec_dai, WM8960_SYSCLKDIV, 2);
And then pass an adjusted frequency to your call to snd_soc_dai_set_pll:
ret = snd_soc_dai_set_pll(codec_dai, WM8960_SYSCLK_PLL, 0, data->clk_frequency, pll_out / 2);
So you could do that and that should give you everything working using the manual mode.
Now Mark does point out that we probably should be looking to replace set_clkdiv since its use is discouraged. Now if you want to proceed down that path your patch will need some work.
The problem is you are duplicating code that basically already exists in wm8960_configure_pll and both sets of code are running when using the automatic path. I am not very keen on that, really you need to look at starting to combine the code for both paths a little more. Such that there is one place in the code that is calculating and setting the sysclk div.
Thanks, Charles
participants (4)
-
Charles Keepax -
chen liu -
Chen.Liu
-
Mark Brown