linux-amlogic.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] clk: meson-g12a: fix bit range for fixed sys and hifi pll
@ 2025-08-22  0:22 Da Xue
  2025-08-25  8:36 ` Jerome Brunet
  2025-08-26  7:59 ` Chuan Liu
  0 siblings, 2 replies; 5+ messages in thread
From: Da Xue @ 2025-08-22  0:22 UTC (permalink / raw)
  To: Neil Armstrong, Jerome Brunet, Michael Turquette, Stephen Boyd,
	Kevin Hilman, Martin Blumenstingl, Jian Hu
  Cc: Da Xue, linux-amlogic, linux-clk, linux-arm-kernel, linux-kernel

The bit range 17:0 does not match the datasheet for A311D / S905D3.
Change the bit range to 18:0 for FIX and HIFI PLLs to match datasheet.

The frac field is missing for sys pll so add that as well.

Patched:

+ sudo cat /sys/devices/system/cpu/cpufreq/policy0/cpuinfo_cur_freq \
/sys/devices/system/cpu/cpufreq/policy2/cpuinfo_cur_freq
996999
500000
+ sudo cat /sys/kernel/debug/meson-clk-msr/measure_summary
+ grep -i '\(sys_\|hifi_\|fixed_\)pll'
 hifi_pll                      0    +/-1562Hz
 sys_pll_div16                 0    +/-1562Hz
 sys_pll_cpub_div16            0    +/-1562Hz
+ sudo cat /sys/kernel/debug/clk/clk_summary
+ grep -i '\(sys_\|hifi_\|fixed_\)pll'
    hifi_pll_dco                     0       0        0        0
       hifi_pll                      0       0        0        0
    sys_pll_dco                      1       1        0        3999999985
       sys_pll                       0       0        0        499999999
          sys_pll_div16_en           0       0        0        499999999
             sys_pll_div16           0       0        0        31249999
    fixed_pll_dco                    1       1        1        3987999985
       fixed_pll                     3       3        1        1993999993

Unpatch:

+ sudo cat /sys/devices/system/cpu/cpufreq/policy0/cpuinfo_cur_freq \
/sys/devices/system/cpu/cpufreq/policy2/cpuinfo_cur_freq
1000000
500000
+ sudo cat /sys/kernel/debug/meson-clk-msr/measure_summary
+ grep -i '\(sys_\|hifi_\|fixed_\)pll'
 hifi_pll                      0    +/-1562Hz
 sys_pll_div16                 0    +/-1562Hz
 sys_pll_cpub_div16            0    +/-1562Hz
+ sudo cat /sys/kernel/debug/clk/clk_summary
+ grep -i '\(sys_\|hifi_\|fixed_\)pll'
    hifi_pll_dco                     0       0        0        0
       hifi_pll                      0       0        0        0
    sys_pll_dco                      1       1        0        4800000000
       sys_pll                       0       0        0        1200000000
          sys_pll_div16_en           0       0        0        1200000000
             sys_pll_div16           0       0        0        75000000
    fixed_pll_dco                    1       1        1        3999999939
       fixed_pll                     3       3        1        1999999970

Fixes: 085a4ea93d54 ("clk: meson: g12a: add peripheral clock controller")
Signed-off-by: Da Xue <da@libre.computer>
---
 drivers/clk/meson/g12a.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
index 66f0e817e416..f78cca619ca5 100644
--- a/drivers/clk/meson/g12a.c
+++ b/drivers/clk/meson/g12a.c
@@ -157,7 +157,7 @@ static struct clk_regmap g12a_fixed_pll_dco = {
 		.frac = {
 			.reg_off = HHI_FIX_PLL_CNTL1,
 			.shift   = 0,
-			.width   = 17,
+			.width   = 19,
 		},
 		.l = {
 			.reg_off = HHI_FIX_PLL_CNTL0,
@@ -223,6 +223,11 @@ static struct clk_regmap g12a_sys_pll_dco = {
 			.shift   = 10,
 			.width   = 5,
 		},
+		.frac = {
+			.reg_off = HHI_SYS_PLL_CNTL1,
+			.shift   = 0,
+			.width   = 19,
+		},
 		.l = {
 			.reg_off = HHI_SYS_PLL_CNTL0,
 			.shift   = 31,
@@ -1901,7 +1906,7 @@ static struct clk_regmap g12a_hifi_pll_dco = {
 		.frac = {
 			.reg_off = HHI_HIFI_PLL_CNTL1,
 			.shift   = 0,
-			.width   = 17,
+			.width   = 19,
 		},
 		.l = {
 			.reg_off = HHI_HIFI_PLL_CNTL0,
-- 
2.47.2


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] clk: meson-g12a: fix bit range for fixed sys and hifi pll
  2025-08-22  0:22 [PATCH v2] clk: meson-g12a: fix bit range for fixed sys and hifi pll Da Xue
@ 2025-08-25  8:36 ` Jerome Brunet
  2025-08-26  7:59 ` Chuan Liu
  1 sibling, 0 replies; 5+ messages in thread
From: Jerome Brunet @ 2025-08-25  8:36 UTC (permalink / raw)
  To: Da Xue
  Cc: Neil Armstrong, Michael Turquette, Stephen Boyd, Kevin Hilman,
	Martin Blumenstingl, Jian Hu, linux-amlogic, linux-clk,
	linux-arm-kernel, linux-kernel

On Thu 21 Aug 2025 at 20:22, Da Xue <da@libre.computer> wrote:

> The bit range 17:0 does not match the datasheet for A311D / S905D3.
> Change the bit range to 18:0 for FIX and HIFI PLLs to match datasheet.
>
> The frac field is missing for sys pll so add that as well.
>
> Patched:
>
> + sudo cat /sys/devices/system/cpu/cpufreq/policy0/cpuinfo_cur_freq \
> /sys/devices/system/cpu/cpufreq/policy2/cpuinfo_cur_freq
> 996999
> 500000
> + sudo cat /sys/kernel/debug/meson-clk-msr/measure_summary
> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>  hifi_pll                      0    +/-1562Hz
>  sys_pll_div16                 0    +/-1562Hz
>  sys_pll_cpub_div16            0    +/-1562Hz
> + sudo cat /sys/kernel/debug/clk/clk_summary
> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>     hifi_pll_dco                     0       0        0        0
>        hifi_pll                      0       0        0        0
>     sys_pll_dco                      1       1        0        3999999985
>        sys_pll                       0       0        0        499999999
>           sys_pll_div16_en           0       0        0        499999999
>              sys_pll_div16           0       0        0        31249999
>     fixed_pll_dco                    1       1        1        3987999985
>        fixed_pll                     3       3        1        1993999993
>
> Unpatch:
>
> + sudo cat /sys/devices/system/cpu/cpufreq/policy0/cpuinfo_cur_freq \
> /sys/devices/system/cpu/cpufreq/policy2/cpuinfo_cur_freq
> 1000000
> 500000
> + sudo cat /sys/kernel/debug/meson-clk-msr/measure_summary
> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>  hifi_pll                      0    +/-1562Hz
>  sys_pll_div16                 0    +/-1562Hz
>  sys_pll_cpub_div16            0    +/-1562Hz
> + sudo cat /sys/kernel/debug/clk/clk_summary
> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>     hifi_pll_dco                     0       0        0        0
>        hifi_pll                      0       0        0        0
>     sys_pll_dco                      1       1        0        4800000000
>        sys_pll                       0       0        0        1200000000
>           sys_pll_div16_en           0       0        0        1200000000
>              sys_pll_div16           0       0        0        75000000
>     fixed_pll_dco                    1       1        1        3999999939
>        fixed_pll                     3       3        1        1999999970
>
> Fixes: 085a4ea93d54 ("clk: meson: g12a: add peripheral clock controller")
> Signed-off-by: Da Xue <da@libre.computer>
> ---
>  drivers/clk/meson/g12a.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
> index 66f0e817e416..f78cca619ca5 100644
> --- a/drivers/clk/meson/g12a.c
> +++ b/drivers/clk/meson/g12a.c
> @@ -157,7 +157,7 @@ static struct clk_regmap g12a_fixed_pll_dco = {
>  		.frac = {
>  			.reg_off = HHI_FIX_PLL_CNTL1,
>  			.shift   = 0,
> -			.width   = 17,
> +			.width   = 19,
>  		},
>  		.l = {
>  			.reg_off = HHI_FIX_PLL_CNTL0,
> @@ -223,6 +223,11 @@ static struct clk_regmap g12a_sys_pll_dco = {
>  			.shift   = 10,
>  			.width   = 5,
>  		},
> +		.frac = {
> +			.reg_off = HHI_SYS_PLL_CNTL1,
> +			.shift   = 0,
> +			.width   = 19,
> +		},
>  		.l = {
>  			.reg_off = HHI_SYS_PLL_CNTL0,
>  			.shift   = 31,
> @@ -1901,7 +1906,7 @@ static struct clk_regmap g12a_hifi_pll_dco = {
>  		.frac = {
>  			.reg_off = HHI_HIFI_PLL_CNTL1,
>  			.shift   = 0,
> -			.width   = 17,
> +			.width   = 19,
>  		},
>  		.l = {
>  			.reg_off = HHI_HIFI_PLL_CNTL0,

Thanks for this.

While this is indeed the same topic, this is doing 2 things at once.
Could you please split this, fixup in one change, adding the missing
frac in another ?

It is much easier to handle in case something needs to be reverted.

-- 
Jerome

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] clk: meson-g12a: fix bit range for fixed sys and hifi pll
  2025-08-22  0:22 [PATCH v2] clk: meson-g12a: fix bit range for fixed sys and hifi pll Da Xue
  2025-08-25  8:36 ` Jerome Brunet
@ 2025-08-26  7:59 ` Chuan Liu
  2025-08-26  9:11   ` Jerome Brunet
  1 sibling, 1 reply; 5+ messages in thread
From: Chuan Liu @ 2025-08-26  7:59 UTC (permalink / raw)
  To: Da Xue, Neil Armstrong, Jerome Brunet, Michael Turquette,
	Stephen Boyd, Kevin Hilman, Martin Blumenstingl, Jian Hu
  Cc: linux-amlogic, linux-clk, linux-arm-kernel, linux-kernel

Hi Da:

     Thanks for your feedback. but this patch is wrong.


On 8/22/2025 8:22 AM, Da Xue wrote:
> [ EXTERNAL EMAIL ]
>
> The bit range 17:0 does not match the datasheet for A311D / S905D3.
> Change the bit range to 18:0 for FIX and HIFI PLLs to match datasheet.


The upper 2 bits (bit18, bit17) of the frac were deprecated long ago.
The actual effective bit field for frac is bit[16:0]. However, the
corresponding datasheet has not been updated. I will provide feedback
and update the datasheet accordingly.


>
> The frac field is missing for sys pll so add that as well.


PLLs with frac support are used in scenarios requiring a wide range
of output frequencies (e.g., audio/video applications).

Since sys_pll is dedicated to clocking the CPU and does not require
such frequency versatility, it does not support fractional frequency
multiplication.


>
> Patched:
>
> + sudo cat /sys/devices/system/cpu/cpufreq/policy0/cpuinfo_cur_freq \
> /sys/devices/system/cpu/cpufreq/policy2/cpuinfo_cur_freq
> 996999
> 500000
> + sudo cat /sys/kernel/debug/meson-clk-msr/measure_summary
> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>   hifi_pll                      0    +/-1562Hz
>   sys_pll_div16                 0    +/-1562Hz
>   sys_pll_cpub_div16            0    +/-1562Hz
> + sudo cat /sys/kernel/debug/clk/clk_summary
> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>      hifi_pll_dco                     0       0        0        0
>         hifi_pll                      0       0        0        0
>      sys_pll_dco                      1       1        0        3999999985
>         sys_pll                       0       0        0        499999999
>            sys_pll_div16_en           0       0        0        499999999
>               sys_pll_div16           0       0        0        31249999
>      fixed_pll_dco                    1       1        1        3987999985
>         fixed_pll                     3       3        1        1993999993
>
> Unpatch:
>
> + sudo cat /sys/devices/system/cpu/cpufreq/policy0/cpuinfo_cur_freq \
> /sys/devices/system/cpu/cpufreq/policy2/cpuinfo_cur_freq
> 1000000
> 500000
> + sudo cat /sys/kernel/debug/meson-clk-msr/measure_summary
> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>   hifi_pll                      0    +/-1562Hz
>   sys_pll_div16                 0    +/-1562Hz
>   sys_pll_cpub_div16            0    +/-1562Hz
> + sudo cat /sys/kernel/debug/clk/clk_summary
> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>      hifi_pll_dco                     0       0        0        0
>         hifi_pll                      0       0        0        0
>      sys_pll_dco                      1       1        0        4800000000
>         sys_pll                       0       0        0        1200000000
>            sys_pll_div16_en           0       0        0        1200000000
>               sys_pll_div16           0       0        0        75000000
>      fixed_pll_dco                    1       1        1        3999999939
>         fixed_pll                     3       3        1        1999999970
>
> Fixes: 085a4ea93d54 ("clk: meson: g12a: add peripheral clock controller")
> Signed-off-by: Da Xue <da@libre.computer>
> ---
>   drivers/clk/meson/g12a.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
> index 66f0e817e416..f78cca619ca5 100644
> --- a/drivers/clk/meson/g12a.c
> +++ b/drivers/clk/meson/g12a.c
> @@ -157,7 +157,7 @@ static struct clk_regmap g12a_fixed_pll_dco = {
>                  .frac = {
>                          .reg_off = HHI_FIX_PLL_CNTL1,
>                          .shift   = 0,
> -                       .width   = 17,
> +                       .width   = 19,
>                  },
>                  .l = {
>                          .reg_off = HHI_FIX_PLL_CNTL0,
> @@ -223,6 +223,11 @@ static struct clk_regmap g12a_sys_pll_dco = {
>                          .shift   = 10,
>                          .width   = 5,
>                  },
> +               .frac = {
> +                       .reg_off = HHI_SYS_PLL_CNTL1,
> +                       .shift   = 0,
> +                       .width   = 19,
> +               },
>                  .l = {
>                          .reg_off = HHI_SYS_PLL_CNTL0,
>                          .shift   = 31,
> @@ -1901,7 +1906,7 @@ static struct clk_regmap g12a_hifi_pll_dco = {
>                  .frac = {
>                          .reg_off = HHI_HIFI_PLL_CNTL1,
>                          .shift   = 0,
> -                       .width   = 17,
> +                       .width   = 19,
>                  },
>                  .l = {
>                          .reg_off = HHI_HIFI_PLL_CNTL0,
> --
> 2.47.2
>
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] clk: meson-g12a: fix bit range for fixed sys and hifi pll
  2025-08-26  7:59 ` Chuan Liu
@ 2025-08-26  9:11   ` Jerome Brunet
  2025-08-27  3:29     ` Chuan Liu
  0 siblings, 1 reply; 5+ messages in thread
From: Jerome Brunet @ 2025-08-26  9:11 UTC (permalink / raw)
  To: Chuan Liu
  Cc: Da Xue, Neil Armstrong, Michael Turquette, Stephen Boyd,
	Kevin Hilman, Martin Blumenstingl, Jian Hu, linux-amlogic,
	linux-clk, linux-arm-kernel, linux-kernel

On Tue 26 Aug 2025 at 15:59, Chuan Liu <chuan.liu@amlogic.com> wrote:

> Hi Da:
>
>     Thanks for your feedback. but this patch is wrong.
>
>
> On 8/22/2025 8:22 AM, Da Xue wrote:
>> [ EXTERNAL EMAIL ]
>>
>> The bit range 17:0 does not match the datasheet for A311D / S905D3.
>> Change the bit range to 18:0 for FIX and HIFI PLLs to match datasheet.
>
>
> The upper 2 bits (bit18, bit17) of the frac were deprecated long ago.

deprecated ? that is really confusing

That seems to imply that it does have an effect but you are choosing not
to use it. Please clarify.

> The actual effective bit field for frac is bit[16:0]. However, the
> corresponding datasheet has not been updated. I will provide feedback
> and update the datasheet accordingly.
>

What about bit 17 and 18 then ? does it have any effect at all ?

>
>>
>> The frac field is missing for sys pll so add that as well.
>
>
> PLLs with frac support are used in scenarios requiring a wide range
> of output frequencies (e.g., audio/video applications).
>
> Since sys_pll is dedicated to clocking the CPU and does not require
> such frequency versatility, it does not support fractional frequency
> multiplication.

You are mixing "HW support" and "usage choice" here.

What I read is :
* Da says the SYS PLL does have HW support for the frac parameter
* Amlogic does not see the point of using it since the CPU does not
  require fine tuning of the rate.

Is that correct ? or is the HW just no present ?

>
>
>>
>> Patched:
>>
>> + sudo cat /sys/devices/system/cpu/cpufreq/policy0/cpuinfo_cur_freq \
>> /sys/devices/system/cpu/cpufreq/policy2/cpuinfo_cur_freq
>> 996999
>> 500000
>> + sudo cat /sys/kernel/debug/meson-clk-msr/measure_summary
>> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>>   hifi_pll                      0    +/-1562Hz
>>   sys_pll_div16                 0    +/-1562Hz
>>   sys_pll_cpub_div16            0    +/-1562Hz
>> + sudo cat /sys/kernel/debug/clk/clk_summary
>> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>>      hifi_pll_dco                     0       0        0        0
>>         hifi_pll                      0       0        0        0
>>      sys_pll_dco                      1       1        0        3999999985
>>         sys_pll                       0       0        0        499999999
>>            sys_pll_div16_en           0       0        0        499999999
>>               sys_pll_div16           0       0        0        31249999
>>      fixed_pll_dco                    1       1        1        3987999985
>>         fixed_pll                     3       3        1        1993999993
>>
>> Unpatch:
>>
>> + sudo cat /sys/devices/system/cpu/cpufreq/policy0/cpuinfo_cur_freq \
>> /sys/devices/system/cpu/cpufreq/policy2/cpuinfo_cur_freq
>> 1000000
>> 500000
>> + sudo cat /sys/kernel/debug/meson-clk-msr/measure_summary
>> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>>   hifi_pll                      0    +/-1562Hz
>>   sys_pll_div16                 0    +/-1562Hz
>>   sys_pll_cpub_div16            0    +/-1562Hz
>> + sudo cat /sys/kernel/debug/clk/clk_summary
>> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>>      hifi_pll_dco                     0       0        0        0
>>         hifi_pll                      0       0        0        0
>>      sys_pll_dco                      1       1        0        4800000000
>>         sys_pll                       0       0        0        1200000000
>>            sys_pll_div16_en           0       0        0        1200000000
>>               sys_pll_div16           0       0        0        75000000
>>      fixed_pll_dco                    1       1        1        3999999939
>>         fixed_pll                     3       3        1        1999999970
>>
>> Fixes: 085a4ea93d54 ("clk: meson: g12a: add peripheral clock controller")
>> Signed-off-by: Da Xue <da@libre.computer>
>> ---
>>   drivers/clk/meson/g12a.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
>> index 66f0e817e416..f78cca619ca5 100644
>> --- a/drivers/clk/meson/g12a.c
>> +++ b/drivers/clk/meson/g12a.c
>> @@ -157,7 +157,7 @@ static struct clk_regmap g12a_fixed_pll_dco = {
>>                  .frac = {
>>                          .reg_off = HHI_FIX_PLL_CNTL1,
>>                          .shift   = 0,
>> -                       .width   = 17,
>> +                       .width   = 19,
>>                  },
>>                  .l = {
>>                          .reg_off = HHI_FIX_PLL_CNTL0,
>> @@ -223,6 +223,11 @@ static struct clk_regmap g12a_sys_pll_dco = {
>>                          .shift   = 10,
>>                          .width   = 5,
>>                  },
>> +               .frac = {
>> +                       .reg_off = HHI_SYS_PLL_CNTL1,
>> +                       .shift   = 0,
>> +                       .width   = 19,
>> +               },
>>                  .l = {
>>                          .reg_off = HHI_SYS_PLL_CNTL0,
>>                          .shift   = 31,
>> @@ -1901,7 +1906,7 @@ static struct clk_regmap g12a_hifi_pll_dco = {
>>                  .frac = {
>>                          .reg_off = HHI_HIFI_PLL_CNTL1,
>>                          .shift   = 0,
>> -                       .width   = 17,
>> +                       .width   = 19,
>>                  },
>>                  .l = {
>>                          .reg_off = HHI_HIFI_PLL_CNTL0,
>> --
>> 2.47.2
>>
>>
>> _______________________________________________
>> linux-amlogic mailing list
>> linux-amlogic@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-amlogic

-- 
Jerome

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] clk: meson-g12a: fix bit range for fixed sys and hifi pll
  2025-08-26  9:11   ` Jerome Brunet
@ 2025-08-27  3:29     ` Chuan Liu
  0 siblings, 0 replies; 5+ messages in thread
From: Chuan Liu @ 2025-08-27  3:29 UTC (permalink / raw)
  To: Jerome Brunet
  Cc: Da Xue, Neil Armstrong, Michael Turquette, Stephen Boyd,
	Kevin Hilman, Martin Blumenstingl, Jian Hu, linux-amlogic,
	linux-clk, linux-arm-kernel, linux-kernel

Hi Jerome:


On 8/26/2025 5:11 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Tue 26 Aug 2025 at 15:59, Chuan Liu<chuan.liu@amlogic.com> wrote:
>
>> Hi Da:
>>
>>      Thanks for your feedback. but this patch is wrong.
>>
>>
>> On 8/22/2025 8:22 AM, Da Xue wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> The bit range 17:0 does not match the datasheet for A311D / S905D3.
>>> Change the bit range to 18:0 for FIX and HIFI PLLs to match datasheet.
>> The upper 2 bits (bit18, bit17) of the frac were deprecated long ago.
> deprecated ? that is really confusing
>
> That seems to imply that it does have an effect but you are choosing not
> to use it. Please clarify.


After investigating the historical context of this issue, the original
design intent for frac[18:0] was as follows:

Bit [18]: Configure the sign for fractional multiplication:
0 → The fractional parameter configured in bits [17:0] is positive:
     fout = 24M * (m + frac / frac_max) / n
1 → The fractional parameter configured in bits [17:0] is negative:
     fout = 24M * (m - frac / frac_max) / n

Bits [17:0]: Fractional multiplier parameter, where:
bit[17] - bit[0] = 1/2^0 + 1/2^1 + ... + 1/2^17

However, the functionality of both bit [18] and bit [17] could be
replaced by simply adjusting the integer ‘m’ value. Moreover, actual
testing revealed that these bits did not perform as expected.
Consequently, this feature was eventually deprecated.


>> The actual effective bit field for frac is bit[16:0]. However, the
>> corresponding datasheet has not been updated. I will provide feedback
>> and update the datasheet accordingly.
>>
> What about bit 17 and 18 then ? does it have any effect at all ?


The functionality of these two bits is deprecated, please ignore it.


>>> The frac field is missing for sys pll so add that as well.
>> PLLs with frac support are used in scenarios requiring a wide range
>> of output frequencies (e.g., audio/video applications).
>>
>> Since sys_pll is dedicated to clocking the CPU and does not require
>> such frequency versatility, it does not support fractional frequency
>> multiplication.
> You are mixing "HW support" and "usage choice" here.


Alright, the previous explanation may have caused some misunderstanding.
The actual intent was to explain why the SYS PLL does not support frac.


>
> What I read is :
> * Da says the SYS PLL does have HW support for the frac parameter
> * Amlogic does not see the point of using it since the CPU does not
>    require fine tuning of the rate.
>
> Is that correct ? or is the HW just no present ?


The SYS PLL HW does not present frac functionality.


>>> Patched:
>>>
>>> + sudo cat /sys/devices/system/cpu/cpufreq/policy0/cpuinfo_cur_freq \
>>> /sys/devices/system/cpu/cpufreq/policy2/cpuinfo_cur_freq
>>> 996999
>>> 500000
>>> + sudo cat /sys/kernel/debug/meson-clk-msr/measure_summary
>>> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>>>    hifi_pll                      0    +/-1562Hz
>>>    sys_pll_div16                 0    +/-1562Hz
>>>    sys_pll_cpub_div16            0    +/-1562Hz
>>> + sudo cat /sys/kernel/debug/clk/clk_summary
>>> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>>>       hifi_pll_dco                     0       0        0        0
>>>          hifi_pll                      0       0        0        0
>>>       sys_pll_dco                      1       1        0        3999999985
>>>          sys_pll                       0       0        0        499999999
>>>             sys_pll_div16_en           0       0        0        499999999
>>>                sys_pll_div16           0       0        0        31249999
>>>       fixed_pll_dco                    1       1        1        3987999985
>>>          fixed_pll                     3       3        1        1993999993
>>>
>>> Unpatch:
>>>
>>> + sudo cat /sys/devices/system/cpu/cpufreq/policy0/cpuinfo_cur_freq \
>>> /sys/devices/system/cpu/cpufreq/policy2/cpuinfo_cur_freq
>>> 1000000
>>> 500000
>>> + sudo cat /sys/kernel/debug/meson-clk-msr/measure_summary
>>> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>>>    hifi_pll                      0    +/-1562Hz
>>>    sys_pll_div16                 0    +/-1562Hz
>>>    sys_pll_cpub_div16            0    +/-1562Hz
>>> + sudo cat /sys/kernel/debug/clk/clk_summary
>>> + grep -i '\(sys_\|hifi_\|fixed_\)pll'
>>>       hifi_pll_dco                     0       0        0        0
>>>          hifi_pll                      0       0        0        0
>>>       sys_pll_dco                      1       1        0        4800000000
>>>          sys_pll                       0       0        0        1200000000
>>>             sys_pll_div16_en           0       0        0        1200000000
>>>                sys_pll_div16           0       0        0        75000000
>>>       fixed_pll_dco                    1       1        1        3999999939
>>>          fixed_pll                     3       3        1        1999999970
>>>
>>> Fixes: 085a4ea93d54 ("clk: meson: g12a: add peripheral clock controller")
>>> Signed-off-by: Da Xue<da@libre.computer>
>>> ---
>>>    drivers/clk/meson/g12a.c | 9 +++++++--
>>>    1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
>>> index 66f0e817e416..f78cca619ca5 100644
>>> --- a/drivers/clk/meson/g12a.c
>>> +++ b/drivers/clk/meson/g12a.c
>>> @@ -157,7 +157,7 @@ static struct clk_regmap g12a_fixed_pll_dco = {
>>>                   .frac = {
>>>                           .reg_off = HHI_FIX_PLL_CNTL1,
>>>                           .shift   = 0,
>>> -                       .width   = 17,
>>> +                       .width   = 19,
>>>                   },
>>>                   .l = {
>>>                           .reg_off = HHI_FIX_PLL_CNTL0,
>>> @@ -223,6 +223,11 @@ static struct clk_regmap g12a_sys_pll_dco = {
>>>                           .shift   = 10,
>>>                           .width   = 5,
>>>                   },
>>> +               .frac = {
>>> +                       .reg_off = HHI_SYS_PLL_CNTL1,
>>> +                       .shift   = 0,
>>> +                       .width   = 19,
>>> +               },
>>>                   .l = {
>>>                           .reg_off = HHI_SYS_PLL_CNTL0,
>>>                           .shift   = 31,
>>> @@ -1901,7 +1906,7 @@ static struct clk_regmap g12a_hifi_pll_dco = {
>>>                   .frac = {
>>>                           .reg_off = HHI_HIFI_PLL_CNTL1,
>>>                           .shift   = 0,
>>> -                       .width   = 17,
>>> +                       .width   = 19,
>>>                   },
>>>                   .l = {
>>>                           .reg_off = HHI_HIFI_PLL_CNTL0,
>>> --
>>> 2.47.2
>>>
>>>
>>> _______________________________________________
>>> linux-amlogic mailing list
>>> linux-amlogic@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-amlogic
> --
> Jerome

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-08-27  3:31 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-22  0:22 [PATCH v2] clk: meson-g12a: fix bit range for fixed sys and hifi pll Da Xue
2025-08-25  8:36 ` Jerome Brunet
2025-08-26  7:59 ` Chuan Liu
2025-08-26  9:11   ` Jerome Brunet
2025-08-27  3:29     ` Chuan Liu

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