[PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Chen.Liu]

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

[PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Chen.Liu]

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Mark Brown]

[-- 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 --]

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Mark Brown]

[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Mark Brown]

[-- 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 --]

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Mark Brown]

[-- Attachment #1.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[chen liu]

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.

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[chen liu]

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.

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[chen liu]

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.

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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 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.

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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 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.

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[chen liu]

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.

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[chen liu]

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.

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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;
> +       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.

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

Re: [PATCH v2] ASOC: wm8960: Add multiple MCLK frequency support

[Charles Keepax]

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;
> +       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.

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