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

[Chuan Liu <chuan.liu@amlogic.com>]

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