* [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support
@ 2017-12-13 12:37 Chen.Liu
2017-12-14 11:56 ` Charles Keepax
2017-12-14 16:19 ` Charles Keepax
0 siblings, 2 replies; 11+ messages in thread
From: Chen.Liu @ 2017-12-13 12:37 UTC (permalink / raw)
To: ckeepax, lgirdwood, broonie, perex, tiwai, shengjiu.wang,
patches, alsa-devel
Cc: linux-kernel
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;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support
2017-12-13 12:37 [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support Chen.Liu
@ 2017-12-14 11:56 ` Charles Keepax
2017-12-14 15:31 ` Mark Brown
2017-12-14 16:19 ` Charles Keepax
1 sibling, 1 reply; 11+ messages in thread
From: Charles Keepax @ 2017-12-14 11:56 UTC (permalink / raw)
To: Chen.Liu
Cc: lgirdwood, broonie, perex, tiwai, shengjiu.wang, patches,
alsa-devel, linux-kernel, daniel.baluta
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;
> + } 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);
> +
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);
> if (ret != 0)
> return ret;
> }
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support
2017-12-14 11:56 ` Charles Keepax
@ 2017-12-14 15:31 ` Mark Brown
2017-12-14 15:51 ` Charles Keepax
0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2017-12-14 15:31 UTC (permalink / raw)
To: Charles Keepax
Cc: Chen.Liu, lgirdwood, perex, tiwai, shengjiu.wang, patches,
alsa-devel, linux-kernel, daniel.baluta
[-- Attachment #1: Type: text/plain, Size: 532 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support
2017-12-14 15:31 ` Mark Brown
@ 2017-12-14 15:51 ` Charles Keepax
2017-12-14 16:45 ` Mark Brown
0 siblings, 1 reply; 11+ messages in thread
From: Charles Keepax @ 2017-12-14 15:51 UTC (permalink / raw)
To: Mark Brown
Cc: Chen.Liu, lgirdwood, perex, tiwai, shengjiu.wang, patches,
alsa-devel, linux-kernel, daniel.baluta
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support
2017-12-14 15:51 ` Charles Keepax
@ 2017-12-14 16:45 ` Mark Brown
0 siblings, 0 replies; 11+ messages in thread
From: Mark Brown @ 2017-12-14 16:45 UTC (permalink / raw)
To: Charles Keepax
Cc: Chen.Liu, lgirdwood, perex, tiwai, shengjiu.wang, patches,
alsa-devel, linux-kernel, daniel.baluta
[-- Attachment #1: Type: text/plain, Size: 426 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support
2017-12-13 12:37 [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support Chen.Liu
2017-12-14 11:56 ` Charles Keepax
@ 2017-12-14 16:19 ` Charles Keepax
[not found] ` <CACJSzi0Kuknj6xFpambi87a1cXecGdfNGQ7PpgywwZV=9_U1yA@mail.gmail.com>
1 sibling, 1 reply; 11+ messages in thread
From: Charles Keepax @ 2017-12-14 16:19 UTC (permalink / raw)
To: Chen.Liu
Cc: lgirdwood, broonie, perex, tiwai, shengjiu.wang, patches,
alsa-devel, linux-kernel, daniel.baluta
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;
> + } 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);
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-12-29 11:06 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-13 12:37 [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support Chen.Liu
2017-12-14 11:56 ` Charles Keepax
2017-12-14 15:31 ` Mark Brown
2017-12-14 15:51 ` Charles Keepax
2017-12-14 16:45 ` Mark Brown
2017-12-14 16:19 ` Charles Keepax
[not found] ` <CACJSzi0Kuknj6xFpambi87a1cXecGdfNGQ7PpgywwZV=9_U1yA@mail.gmail.com>
2017-12-18 9:31 ` Charles Keepax
[not found] ` <CACJSzi13ZdOjvz1A2YFbE3z0nUPjeC7i5Dh+ZTnsQH55ON=YMA@mail.gmail.com>
2017-12-18 11:55 ` Charles Keepax
[not found] ` <CACJSzi0hbv2p_6dXkp2AFwyYVzmU6_H56LgJxY70_ZKn2y=aUw@mail.gmail.com>
2017-12-21 16:48 ` Charles Keepax
2017-12-22 10:40 ` chen liu
2017-12-29 11:05 ` Charles Keepax
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).