Re: [PATCH v2] clk: meson-g12a: fix bit range for fixed sys and hifi pll

[Jerome Brunet <jbrunet@baylibre.com>]

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