[PATCH 0/4] clk: meson: Fix an issue with inaccurate hifi_pll frequency

[PATCH 0/4] clk: meson: Fix an issue with inaccurate hifi_pll frequency

[Chuan Liu via B4 Relay]

Some PLLs with fractional multipliers have fractional denominators that
are fixed to "100000" instead of the previous "(1 << pll->frac.width)".

The hifi_pll for both C3 and S4 supports a fractional multiplier and has
a fixed fractional denominator of "100000".

Here are the results of the C3-based command tests (already defined
CLOCK_ALLOW_WRITE_DEBUGFS):
* echo 491520000 > /sys/kernel/debug/clk/hifi_pll/clk_rate
* cat /sys/kernel/debug/clk/hifi_pll/clk_rate
491520000
* echo 1 > /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable
* cat /sys/kernel/debug/meson-clk-msr/clks/hifi_pll_clk
491515625       +/-15625Hz
* devmem 0xfe008100 32
0xD00304A3
* devmem 0xfe008104 32
0x00014820

Based on the register information read above, it can be obtained:
m = 0xA3 = 0d163;
n = 0x1 = 0d1
frac = 0x14820 = 0d84000
od = 0x3 = 0d3

hifi_pll calculates the output frequency:
calc_rate = xtal_rate / n * (m + (frac / frac_max)) >> od;
calc_rate = 24000000 / 1 * (163 + (84000 / 100000)) >> 3;
calc_rate = 491520000

clk_rate, msr_rate, and calc_rate all match.

The test and calculation results of S4 are consistent with those of C3,
which will not be repeated here.

Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
Chuan Liu (4):
      clk: meson: Support PLL with fixed fractional denominators
      clk: meson: c3: pll: hifi_pll frequency is not accurate
      clk: meson: s4: pll: hifi_pll support fractional multiplier
      clk: meson: s4: pll: hifi_pll frequency is not accurate

 drivers/clk/meson/c3-pll.c  |  1 +
 drivers/clk/meson/clk-pll.c | 22 +++++++++++++++++++---
 drivers/clk/meson/clk-pll.h |  1 +
 drivers/clk/meson/s4-pll.c  |  9 +++++++--
 4 files changed, 28 insertions(+), 5 deletions(-)
---
base-commit: adac147c6a32e2919cb04555387e12e738991a19
change-id: 20240904-fix_clk-668f7a1a2b16

Best regards,
-- 
Chuan Liu <chuan.liu@amlogic.com>

[PATCH 1/4] clk: meson: Support PLL with fixed fractional denominators

[Chuan Liu via B4 Relay]

From: Chuan Liu <chuan.liu@amlogic.com>

Some PLLs with fractional multipliers have fractional denominators that
are fixed to "100000" instead of the previous "(1 << pll->frac.width)".

Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
 drivers/clk/meson/clk-pll.c | 22 +++++++++++++++++++---
 drivers/clk/meson/clk-pll.h |  1 +
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index bc570a2ff3a3..f0009c174564 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -36,6 +36,12 @@
 #include "clk-regmap.h"
 #include "clk-pll.h"
 
+/*
+ * Some PLLs with fractional multipliers have fractional denominators that
+ * are fixed to "100000" instead of the previous "(1 << pll->frac.width)".
+ */
+#define FIXED_FRAC_MAX			100000
+
 static inline struct meson_clk_pll_data *
 meson_clk_pll_data(struct clk_regmap *clk)
 {
@@ -57,12 +63,17 @@ static unsigned long __pll_params_to_rate(unsigned long parent_rate,
 					  struct meson_clk_pll_data *pll)
 {
 	u64 rate = (u64)parent_rate * m;
+	unsigned int frac_max;
 
 	if (frac && MESON_PARM_APPLICABLE(&pll->frac)) {
 		u64 frac_rate = (u64)parent_rate * frac;
 
-		rate += DIV_ROUND_UP_ULL(frac_rate,
-					 (1 << pll->frac.width));
+		if (pll->flags & CLK_MESON_PLL_FIXED_FRAC_MAX)
+			frac_max = FIXED_FRAC_MAX;
+		else
+			frac_max = (1 << pll->frac.width);
+
+		rate += DIV_ROUND_UP_ULL(frac_rate, frac_max);
 	}
 
 	return DIV_ROUND_UP_ULL(rate, n);
@@ -100,13 +111,18 @@ static unsigned int __pll_params_with_frac(unsigned long rate,
 					   unsigned int n,
 					   struct meson_clk_pll_data *pll)
 {
-	unsigned int frac_max = (1 << pll->frac.width);
+	unsigned int frac_max;
 	u64 val = (u64)rate * n;
 
 	/* Bail out if we are already over the requested rate */
 	if (rate < parent_rate * m / n)
 		return 0;
 
+	if (pll->flags & CLK_MESON_PLL_FIXED_FRAC_MAX)
+		frac_max = FIXED_FRAC_MAX;
+	else
+		frac_max = (1 << pll->frac.width);
+
 	if (pll->flags & CLK_MESON_PLL_ROUND_CLOSEST)
 		val = DIV_ROUND_CLOSEST_ULL(val * frac_max, parent_rate);
 	else
diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
index 7b6b87274073..e996d3727eb1 100644
--- a/drivers/clk/meson/clk-pll.h
+++ b/drivers/clk/meson/clk-pll.h
@@ -29,6 +29,7 @@ struct pll_mult_range {
 
 #define CLK_MESON_PLL_ROUND_CLOSEST	BIT(0)
 #define CLK_MESON_PLL_NOINIT_ENABLED	BIT(1)
+#define CLK_MESON_PLL_FIXED_FRAC_MAX	BIT(2)
 
 struct meson_clk_pll_data {
 	struct parm en;

-- 
2.42.0

Re: [PATCH 1/4] clk: meson: Support PLL with fixed fractional denominators

[Jerome Brunet]

On Fri 06 Sep 2024 at 13:52, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:

> From: Chuan Liu <chuan.liu@amlogic.com>
>
> Some PLLs with fractional multipliers have fractional denominators that
> are fixed to "100000" instead of the previous "(1 << pll->frac.width)".
>
> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
> ---
>  drivers/clk/meson/clk-pll.c | 22 +++++++++++++++++++---
>  drivers/clk/meson/clk-pll.h |  1 +
>  2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index bc570a2ff3a3..f0009c174564 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -36,6 +36,12 @@
>  #include "clk-regmap.h"
>  #include "clk-pll.h"
>  
> +/*
> + * Some PLLs with fractional multipliers have fractional denominators that
> + * are fixed to "100000" instead of the previous "(1 << pll->frac.width)".
> + */
> +#define FIXED_FRAC_MAX			100000

When the next arbitrary limit comes around, this will get very ugly.
Instead, please add frac_max to the pll parameter

> +
>  static inline struct meson_clk_pll_data *
>  meson_clk_pll_data(struct clk_regmap *clk)
>  {
> @@ -57,12 +63,17 @@ static unsigned long __pll_params_to_rate(unsigned long parent_rate,
>  					  struct meson_clk_pll_data *pll)
>  {
>  	u64 rate = (u64)parent_rate * m;
> +	unsigned int frac_max;
>  
>  	if (frac && MESON_PARM_APPLICABLE(&pll->frac)) {
>  		u64 frac_rate = (u64)parent_rate * frac;
>  
> -		rate += DIV_ROUND_UP_ULL(frac_rate,
> -					 (1 << pll->frac.width));
> +		if (pll->flags & CLK_MESON_PLL_FIXED_FRAC_MAX)
> +			frac_max = FIXED_FRAC_MAX;
> +		else
> +			frac_max = (1 << pll->frac.width);
> +
> +		rate += DIV_ROUND_UP_ULL(frac_rate, frac_max);
>  	}
>  
>  	return DIV_ROUND_UP_ULL(rate, n);
> @@ -100,13 +111,18 @@ static unsigned int __pll_params_with_frac(unsigned long rate,
>  					   unsigned int n,
>  					   struct meson_clk_pll_data *pll)
>  {
> -	unsigned int frac_max = (1 << pll->frac.width);
> +	unsigned int frac_max;
>  	u64 val = (u64)rate * n;
>  
>  	/* Bail out if we are already over the requested rate */
>  	if (rate < parent_rate * m / n)
>  		return 0;
>  
> +	if (pll->flags & CLK_MESON_PLL_FIXED_FRAC_MAX)

Certainly don't need a flag for that. Use a parameter and default to (1
<< pll->frac.width) if unset.

> +		frac_max = FIXED_FRAC_MAX;
> +	else
> +		frac_max = (1 << pll->frac.width);
> +
>  	if (pll->flags & CLK_MESON_PLL_ROUND_CLOSEST)
>  		val = DIV_ROUND_CLOSEST_ULL(val * frac_max, parent_rate);
>  	else
> diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
> index 7b6b87274073..e996d3727eb1 100644
> --- a/drivers/clk/meson/clk-pll.h
> +++ b/drivers/clk/meson/clk-pll.h
> @@ -29,6 +29,7 @@ struct pll_mult_range {
>  
>  #define CLK_MESON_PLL_ROUND_CLOSEST	BIT(0)
>  #define CLK_MESON_PLL_NOINIT_ENABLED	BIT(1)
> +#define CLK_MESON_PLL_FIXED_FRAC_MAX	BIT(2)

Remove this.

>  
>  struct meson_clk_pll_data {
>  	struct parm en;

-- 
Jerome

Re: [PATCH 1/4] clk: meson: Support PLL with fixed fractional denominators

[Chuan Liu]

On 2024/9/6 14:51, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 06 Sep 2024 at 13:52, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
>
>> From: Chuan Liu <chuan.liu@amlogic.com>
>>
>> Some PLLs with fractional multipliers have fractional denominators that
>> are fixed to "100000" instead of the previous "(1 << pll->frac.width)".
>>
>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>> ---
>>   drivers/clk/meson/clk-pll.c | 22 +++++++++++++++++++---
>>   drivers/clk/meson/clk-pll.h |  1 +
>>   2 files changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
>> index bc570a2ff3a3..f0009c174564 100644
>> --- a/drivers/clk/meson/clk-pll.c
>> +++ b/drivers/clk/meson/clk-pll.c
>> @@ -36,6 +36,12 @@
>>   #include "clk-regmap.h"
>>   #include "clk-pll.h"
>>
>> +/*
>> + * Some PLLs with fractional multipliers have fractional denominators that
>> + * are fixed to "100000" instead of the previous "(1 << pll->frac.width)".
>> + */
>> +#define FIXED_FRAC_MAX                       100000
> When the next arbitrary limit comes around, this will get very ugly.
> Instead, please add frac_max to the pll parameter


I also had this consideration before, and after confirmation, the
hifi_pll of the subsequent chip design will continue to be this value,
and the hifi_pll of the chip in recent years is also this value. So
let's define it here.

In the next version I replaced it with a member inside the structure
member of meson_clk_pll_data.


>
>> +
>>   static inline struct meson_clk_pll_data *
>>   meson_clk_pll_data(struct clk_regmap *clk)
>>   {
>> @@ -57,12 +63,17 @@ static unsigned long __pll_params_to_rate(unsigned long parent_rate,
>>                                          struct meson_clk_pll_data *pll)
>>   {
>>        u64 rate = (u64)parent_rate * m;
>> +     unsigned int frac_max;
>>
>>        if (frac && MESON_PARM_APPLICABLE(&pll->frac)) {
>>                u64 frac_rate = (u64)parent_rate * frac;
>>
>> -             rate += DIV_ROUND_UP_ULL(frac_rate,
>> -                                      (1 << pll->frac.width));
>> +             if (pll->flags & CLK_MESON_PLL_FIXED_FRAC_MAX)
>> +                     frac_max = FIXED_FRAC_MAX;
>> +             else
>> +                     frac_max = (1 << pll->frac.width);
>> +
>> +             rate += DIV_ROUND_UP_ULL(frac_rate, frac_max);
>>        }
>>
>>        return DIV_ROUND_UP_ULL(rate, n);
>> @@ -100,13 +111,18 @@ static unsigned int __pll_params_with_frac(unsigned long rate,
>>                                           unsigned int n,
>>                                           struct meson_clk_pll_data *pll)
>>   {
>> -     unsigned int frac_max = (1 << pll->frac.width);
>> +     unsigned int frac_max;
>>        u64 val = (u64)rate * n;
>>
>>        /* Bail out if we are already over the requested rate */
>>        if (rate < parent_rate * m / n)
>>                return 0;
>>
>> +     if (pll->flags & CLK_MESON_PLL_FIXED_FRAC_MAX)
> Certainly don't need a flag for that. Use a parameter and default to (1
> << pll->frac.width) if unset.
Okay
>
>> +             frac_max = FIXED_FRAC_MAX;
>> +     else
>> +             frac_max = (1 << pll->frac.width);
>> +
>>        if (pll->flags & CLK_MESON_PLL_ROUND_CLOSEST)
>>                val = DIV_ROUND_CLOSEST_ULL(val * frac_max, parent_rate);
>>        else
>> diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
>> index 7b6b87274073..e996d3727eb1 100644
>> --- a/drivers/clk/meson/clk-pll.h
>> +++ b/drivers/clk/meson/clk-pll.h
>> @@ -29,6 +29,7 @@ struct pll_mult_range {
>>
>>   #define CLK_MESON_PLL_ROUND_CLOSEST  BIT(0)
>>   #define CLK_MESON_PLL_NOINIT_ENABLED BIT(1)
>> +#define CLK_MESON_PLL_FIXED_FRAC_MAX BIT(2)
> Remove this.
>
>>   struct meson_clk_pll_data {
>>        struct parm en;
> --
> Jerome

[PATCH 2/4] clk: meson: c3: pll: hifi_pll frequency is not accurate

[Chuan Liu via B4 Relay]

From: Chuan Liu <chuan.liu@amlogic.com>

The fractional denominator of C3's hifi_pll fractional multiplier is
fixed to 100000, so flag CLK_MESON_PLL_FIXED_FRAC_MAX is added.

Fixes: 8a9a129dc565 ("clk: meson: c3: add support for the C3 SoC PLL clock")
Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
 drivers/clk/meson/c3-pll.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/meson/c3-pll.c b/drivers/clk/meson/c3-pll.c
index 32bd2ed9d304..a350173efe90 100644
--- a/drivers/clk/meson/c3-pll.c
+++ b/drivers/clk/meson/c3-pll.c
@@ -361,6 +361,7 @@ static struct clk_regmap hifi_pll_dco = {
 		.range = &c3_gp0_pll_mult_range,
 		.init_regs = c3_hifi_init_regs,
 		.init_count = ARRAY_SIZE(c3_hifi_init_regs),
+		.flags = CLK_MESON_PLL_FIXED_FRAC_MAX,
 	},
 	.hw.init = &(struct clk_init_data) {
 		.name = "hifi_pll_dco",

-- 
2.42.0

Re: [PATCH 2/4] clk: meson: c3: pll: hifi_pll frequency is not accurate

[Jerome Brunet]

On Fri 06 Sep 2024 at 13:52, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:

> From: Chuan Liu <chuan.liu@amlogic.com>

The patch title is not good. the clock innacurate ... Ok, but you are doing
something about it, right ? Plus just saying that is a bit vague.

How about something like "fix frac maximum value" ? This what you are
doing, right ?

>
> The fractional denominator of C3's hifi_pll fractional multiplier is
> fixed to 100000, so flag CLK_MESON_PLL_FIXED_FRAC_MAX is added.
>
> Fixes: 8a9a129dc565 ("clk: meson: c3: add support for the C3 SoC PLL clock")
> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
> ---
>  drivers/clk/meson/c3-pll.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clk/meson/c3-pll.c b/drivers/clk/meson/c3-pll.c
> index 32bd2ed9d304..a350173efe90 100644
> --- a/drivers/clk/meson/c3-pll.c
> +++ b/drivers/clk/meson/c3-pll.c
> @@ -361,6 +361,7 @@ static struct clk_regmap hifi_pll_dco = {
>  		.range = &c3_gp0_pll_mult_range,
>  		.init_regs = c3_hifi_init_regs,
>  		.init_count = ARRAY_SIZE(c3_hifi_init_regs),
> +		.flags = CLK_MESON_PLL_FIXED_FRAC_MAX,
>  	},
>  	.hw.init = &(struct clk_init_data) {
>  		.name = "hifi_pll_dco",

-- 
Jerome

Re: [PATCH 2/4] clk: meson: c3: pll: hifi_pll frequency is not accurate

[Chuan Liu]

On 2024/9/6 14:55, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 06 Sep 2024 at 13:52, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
>
>> From: Chuan Liu <chuan.liu@amlogic.com>
> The patch title is not good. the clock innacurate ... Ok, but you are doing
> something about it, right ? Plus just saying that is a bit vague.
>
> How about something like "fix frac maximum value" ? This what you are
> doing, right ?


Right, Fix in next version.


>> The fractional denominator of C3's hifi_pll fractional multiplier is
>> fixed to 100000, so flag CLK_MESON_PLL_FIXED_FRAC_MAX is added.
>>
>> Fixes: 8a9a129dc565 ("clk: meson: c3: add support for the C3 SoC PLL clock")
>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>> ---
>>   drivers/clk/meson/c3-pll.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/clk/meson/c3-pll.c b/drivers/clk/meson/c3-pll.c
>> index 32bd2ed9d304..a350173efe90 100644
>> --- a/drivers/clk/meson/c3-pll.c
>> +++ b/drivers/clk/meson/c3-pll.c
>> @@ -361,6 +361,7 @@ static struct clk_regmap hifi_pll_dco = {
>>                .range = &c3_gp0_pll_mult_range,
>>                .init_regs = c3_hifi_init_regs,
>>                .init_count = ARRAY_SIZE(c3_hifi_init_regs),
>> +             .flags = CLK_MESON_PLL_FIXED_FRAC_MAX,
>>        },
>>        .hw.init = &(struct clk_init_data) {
>>                .name = "hifi_pll_dco",
> --
> Jerome

[PATCH 3/4] clk: meson: s4: pll: hifi_pll support fractional multiplier

[Chuan Liu via B4 Relay]

From: Chuan Liu <chuan.liu@amlogic.com>

The s4's hifi_pll supports a fractional frequency multiplier, but frac
parameters are not configured in the driver.

Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
 drivers/clk/meson/s4-pll.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/meson/s4-pll.c b/drivers/clk/meson/s4-pll.c
index b0258933fb9d..a97e19057b05 100644
--- a/drivers/clk/meson/s4-pll.c
+++ b/drivers/clk/meson/s4-pll.c
@@ -329,7 +329,6 @@ static struct clk_regmap s4_gp0_pll = {
  * Internal hifi pll emulation configuration parameters
  */
 static const struct reg_sequence s4_hifi_init_regs[] = {
-	{ .reg = ANACTRL_HIFIPLL_CTRL1,	.def = 0x00010e56 },
 	{ .reg = ANACTRL_HIFIPLL_CTRL2,	.def = 0x00000000 },
 	{ .reg = ANACTRL_HIFIPLL_CTRL3,	.def = 0x6a285c00 },
 	{ .reg = ANACTRL_HIFIPLL_CTRL4,	.def = 0x65771290 },
@@ -354,6 +353,11 @@ static struct clk_regmap s4_hifi_pll_dco = {
 			.shift   = 10,
 			.width   = 5,
 		},
+		.frac = {
+			.reg_off = ANACTRL_HIFIPLL_CTRL1,
+			.shift   = 0,
+			.width   = 17,
+		},
 		.l = {
 			.reg_off = ANACTRL_HIFIPLL_CTRL0,
 			.shift   = 31,

-- 
2.42.0

Re: [PATCH 3/4] clk: meson: s4: pll: hifi_pll support fractional multiplier

[Jerome Brunet]

On Fri 06 Sep 2024 at 13:52, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:

> From: Chuan Liu <chuan.liu@amlogic.com>
>
> The s4's hifi_pll supports a fractional frequency multiplier, but frac
> parameters are not configured in the driver.

That should probably have been sent separately.
It's about frac, yes, but it has nothing to do with the issue described
in the cover letter.

>
> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
> ---
>  drivers/clk/meson/s4-pll.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/meson/s4-pll.c b/drivers/clk/meson/s4-pll.c
> index b0258933fb9d..a97e19057b05 100644
> --- a/drivers/clk/meson/s4-pll.c
> +++ b/drivers/clk/meson/s4-pll.c
> @@ -329,7 +329,6 @@ static struct clk_regmap s4_gp0_pll = {
>   * Internal hifi pll emulation configuration parameters
>   */
>  static const struct reg_sequence s4_hifi_init_regs[] = {
> -	{ .reg = ANACTRL_HIFIPLL_CTRL1,	.def = 0x00010e56 },
>  	{ .reg = ANACTRL_HIFIPLL_CTRL2,	.def = 0x00000000 },
>  	{ .reg = ANACTRL_HIFIPLL_CTRL3,	.def = 0x6a285c00 },
>  	{ .reg = ANACTRL_HIFIPLL_CTRL4,	.def = 0x65771290 },
> @@ -354,6 +353,11 @@ static struct clk_regmap s4_hifi_pll_dco = {
>  			.shift   = 10,
>  			.width   = 5,
>  		},
> +		.frac = {
> +			.reg_off = ANACTRL_HIFIPLL_CTRL1,
> +			.shift   = 0,
> +			.width   = 17,
> +		},
>  		.l = {
>  			.reg_off = ANACTRL_HIFIPLL_CTRL0,
>  			.shift   = 31,

-- 
Jerome

[PATCH 4/4] clk: meson: s4: pll: hifi_pll frequency is not accurate

[Chuan Liu via B4 Relay]

From: Chuan Liu <chuan.liu@amlogic.com>

The fractional denominator of S4's hifi_pll fractional multiplier is
fixed to 100000, so flag CLK_MESON_PLL_FIXED_FRAC_MAX is added.

Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
---
 drivers/clk/meson/s4-pll.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/meson/s4-pll.c b/drivers/clk/meson/s4-pll.c
index a97e19057b05..a793ac2e8cc7 100644
--- a/drivers/clk/meson/s4-pll.c
+++ b/drivers/clk/meson/s4-pll.c
@@ -371,7 +371,8 @@ static struct clk_regmap s4_hifi_pll_dco = {
 		.range = &s4_gp0_pll_mult_range,
 		.init_regs = s4_hifi_init_regs,
 		.init_count = ARRAY_SIZE(s4_hifi_init_regs),
-		.flags = CLK_MESON_PLL_ROUND_CLOSEST,
+		.flags = CLK_MESON_PLL_ROUND_CLOSEST |
+			 CLK_MESON_PLL_FIXED_FRAC_MAX,
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "hifi_pll_dco",

-- 
2.42.0

Re: [PATCH 0/4] clk: meson: Fix an issue with inaccurate hifi_pll frequency

[Jerome Brunet]

On Fri 06 Sep 2024 at 13:52, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:

> Some PLLs with fractional multipliers have fractional denominators that
> are fixed to "100000" instead of the previous "(1 << pll->frac.width)".
>
> The hifi_pll for both C3 and S4 supports a fractional multiplier and has
> a fixed fractional denominator of "100000".
>
> Here are the results of the C3-based command tests (already defined
> CLOCK_ALLOW_WRITE_DEBUGFS):
> * echo 491520000 > /sys/kernel/debug/clk/hifi_pll/clk_rate
> * cat /sys/kernel/debug/clk/hifi_pll/clk_rate
> 491520000
> * echo 1 > /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable
> * cat /sys/kernel/debug/meson-clk-msr/clks/hifi_pll_clk
> 491515625       +/-15625Hz
> * devmem 0xfe008100 32
> 0xD00304A3
> * devmem 0xfe008104 32
> 0x00014820
>
> Based on the register information read above, it can be obtained:
> m = 0xA3 = 0d163;
> n = 0x1 = 0d1
> frac = 0x14820 = 0d84000
> od = 0x3 = 0d3
>
> hifi_pll calculates the output frequency:
> calc_rate = xtal_rate / n * (m + (frac / frac_max)) >> od;
> calc_rate = 24000000 / 1 * (163 + (84000 / 100000)) >> 3;
> calc_rate = 491520000
>
> clk_rate, msr_rate, and calc_rate all match.

Thanks for the detailed description.

Is there a possibility this applies to the g12/sm1 as well ? HiFi PLL
has had trouble on these SoCs since support has been added. It sometimes
takes a long time to report a lock. So long we consider it a failure.

There was no such issue on AXG. If you check DT, it is the reason why
AXG use the HiFi PLL for the sound card and G12/SM1 does not.

>
> The test and calculation results of S4 are consistent with those of C3,
> which will not be repeated here.
>
> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
> ---
> Chuan Liu (4):
>       clk: meson: Support PLL with fixed fractional denominators
>       clk: meson: c3: pll: hifi_pll frequency is not accurate
>       clk: meson: s4: pll: hifi_pll support fractional multiplier
>       clk: meson: s4: pll: hifi_pll frequency is not accurate
>
>  drivers/clk/meson/c3-pll.c  |  1 +
>  drivers/clk/meson/clk-pll.c | 22 +++++++++++++++++++---
>  drivers/clk/meson/clk-pll.h |  1 +
>  drivers/clk/meson/s4-pll.c  |  9 +++++++--
>  4 files changed, 28 insertions(+), 5 deletions(-)
> ---
> base-commit: adac147c6a32e2919cb04555387e12e738991a19
> change-id: 20240904-fix_clk-668f7a1a2b16
>
> Best regards,

-- 
Jerome

Re: [PATCH 0/4] clk: meson: Fix an issue with inaccurate hifi_pll frequency

[Chuan Liu]

On 2024/9/6 15:04, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 06 Sep 2024 at 13:52, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
>
>> Some PLLs with fractional multipliers have fractional denominators that
>> are fixed to "100000" instead of the previous "(1 << pll->frac.width)".
>>
>> The hifi_pll for both C3 and S4 supports a fractional multiplier and has
>> a fixed fractional denominator of "100000".
>>
>> Here are the results of the C3-based command tests (already defined
>> CLOCK_ALLOW_WRITE_DEBUGFS):
>> * echo 491520000 > /sys/kernel/debug/clk/hifi_pll/clk_rate
>> * cat /sys/kernel/debug/clk/hifi_pll/clk_rate
>> 491520000
>> * echo 1 > /sys/kernel/debug/clk/hifi_pll/clk_prepare_enable
>> * cat /sys/kernel/debug/meson-clk-msr/clks/hifi_pll_clk
>> 491515625       +/-15625Hz
>> * devmem 0xfe008100 32
>> 0xD00304A3
>> * devmem 0xfe008104 32
>> 0x00014820
>>
>> Based on the register information read above, it can be obtained:
>> m = 0xA3 = 0d163;
>> n = 0x1 = 0d1
>> frac = 0x14820 = 0d84000
>> od = 0x3 = 0d3
>>
>> hifi_pll calculates the output frequency:
>> calc_rate = xtal_rate / n * (m + (frac / frac_max)) >> od;
>> calc_rate = 24000000 / 1 * (163 + (84000 / 100000)) >> 3;
>> calc_rate = 491520000
>>
>> clk_rate, msr_rate, and calc_rate all match.
> Thanks for the detailed description.
>
> Is there a possibility this applies to the g12/sm1 as well ? HiFi PLL
> has had trouble on these SoCs since support has been added. It sometimes
> takes a long time to report a lock. So long we consider it a failure.
>
> There was no such issue on AXG. If you check DT, it is the reason why
> AXG use the HiFi PLL for the sound card and G12/SM1 does not.


I have confirmed that only the hifi_pll of the chip in recent years has 
this feature,

and g12a/sm1/axg is not like this.


I confirm that our sm1 uses hifi_pll, and I have not encountered the
situation you said. There was a probability of lock failure in pll
before, which was solved by adding 50us delay in meson_clk_pll_enable:
@@ -378,6 +378,8 @@ static int meson_clk_pll_enable(struct clk_hw *hw)
         /* Enable the pll */
         meson_parm_write(clk->map, &pll->en, 1);

+       udelay(50);
+
         /* Take the pll out reset */
         if (MESON_PARM_APPLICABLE(&pll->rst))
                 meson_parm_write(clk->map, &pll->rst, 0);

This patch is also prepare to push to upstream later.


Another detail is that the larger the frac value, the longer the lock
time, but the time is at the us level, so that the timeout in the pll
driver is not triggered.


>
>> The test and calculation results of S4 are consistent with those of C3,
>> which will not be repeated here.
>>
>> Signed-off-by: Chuan Liu <chuan.liu@amlogic.com>
>> ---
>> Chuan Liu (4):
>>        clk: meson: Support PLL with fixed fractional denominators
>>        clk: meson: c3: pll: hifi_pll frequency is not accurate
>>        clk: meson: s4: pll: hifi_pll support fractional multiplier
>>        clk: meson: s4: pll: hifi_pll frequency is not accurate
>>
>>   drivers/clk/meson/c3-pll.c  |  1 +
>>   drivers/clk/meson/clk-pll.c | 22 +++++++++++++++++++---
>>   drivers/clk/meson/clk-pll.h |  1 +
>>   drivers/clk/meson/s4-pll.c  |  9 +++++++--
>>   4 files changed, 28 insertions(+), 5 deletions(-)
>> ---
>> base-commit: adac147c6a32e2919cb04555387e12e738991a19
>> change-id: 20240904-fix_clk-668f7a1a2b16
>>
>> Best regards,
> --
> Jerome

Re: (subset) [PATCH 0/4] clk: meson: Fix an issue with inaccurate hifi_pll frequency

[Jerome Brunet]

Applied to clk-meson (clk-meson-next), thanks!

[3/4] clk: meson: s4: pll: hifi_pll support fractional multiplier
      https://github.com/BayLibre/clk-meson/commit/80344f4c1a1e

Best regards,
--
Jerome