Re: [RFT PATCH] clk: ls1c: Fix PLL rate calculation

[Kelvin Cheung]

Sean, Du,
I saw you are discussing the PLL rate calculation issue.
My question is whether the upstream kernel works on your ls1c300?
For me, it never works, even the earliest version which LS1C support was merged.
After the kernel is loaded by PMON, there is no console output at all.
I also confirm this issue with Yang.
BTW, my board is 1C300B.
Are your board is different from me? Or your bootloader?

Thanks!

Sean Anderson <seanga2@xxxxxxxxx> 于2022年4月19日周二 13:11写道：
>
> While reviewing Dhu's patch adding ls1c300 clock support to U-Boot [1], I
> noticed the following calculation, which is copied from
> drivers/clk/loongson1/clk-loongson1c.c:
>
> ulong ls1c300_pll_get_rate(struct clk *clk)
> {
>         unsigned int mult;
>         long long parent_rate;
>         void *base;
>         unsigned int val;
>
>         parent_rate = clk_get_parent_rate(clk);
>         base = (void *)clk->data;
>
>         val = readl(base + START_FREQ);
>         mult = FIELD_GET(FRAC_N, val) + FIELD_GET(M_PLL, val);
>         return (mult * parent_rate) / 4;
> }
>
> I would like to examine the use of M_PLL and FRAC_N to calculate the multiplier
> for the PLL. The datasheet has the following to say:
>
> START_FREQ 位    缺省值      描述
> ========== ===== =========== ====================================
> FRAC_N     23:16 0           PLL 倍频系数的小数部分
>
>                  由          PLL 倍频系数的整数部分
> M_PLL      15:8  NAND_D[3:0] (理论可以达到 255，建议不要超过 100)
>                  配置
>
> which according to google translate means
>
> START_FREQ Bits  Default       Description
> ========== ===== ============= ================================================
> FRAC_N     23:16 0             Fractional part of the PLL multiplication factor
>
>                  Depends on    Integer part of PLL multiplication factor
> M_PLL      15:8  NAND_D[3:0]   (Theoretically it can reach 255, [but] it is
>                  configuration  recommended not to exceed 100)
>
> So just based on this description, I would expect that the formula to be
> something like
>
>         rate = parent * (255 * M_PLL + FRAC_N) / 255 / 4
>
> However, the datasheet also gives the following formula:
>
>         rate = parent * (M_PLL + FRAC_N) / 4
>
> which is what the Linux driver has implemented. I find this very unusual.
> First, the datasheet specifically says that these fields are the integer and
> fractional parts of the multiplier. Second, I think such a construct does not
> easily map to traditional PLL building blocks. Implementing this formula in
> hardware would likely require an adder, just to then set the threshold of a
> clock divider.
>
> I think it is much more likely that the first formula is correct. The author of
> the datasheet may think of a multiplier of (say) 3.14 as
>
>         M_PLL = 3
>         FRAC_N = 0.14
>
> which together sum to the correct multiplier, even though the actual value
> stored in FRAC_N would be 36.
>
> I suspect that this has slipped by unnoticed because when FRAC_N is 0, there is
> no difference in the formulae. The following patch is untested, but I suspect
> it will fix this issue. I would appreciate if anyone with access to the
> hardware could measure the output of the PLL (or one of its derived clocks) and
> determine the correct formula.
>
> [1] https://lore.kernel.org/u-boot/20220418204519.19991-1-dhu@xxxxxxxxxxxxxx/T/#u
>
> Fixes: b4626a7f4892 ("CLK: Add Loongson1C clock support")
> Signed-off-by: Sean Anderson <seanga2@xxxxxxxxx>
> ---
>
>  drivers/clk/loongson1/clk-loongson1c.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/loongson1/clk-loongson1c.c b/drivers/clk/loongson1/clk-loongson1c.c
> index 703f87622cf5..2b98a116c1ea 100644
> --- a/drivers/clk/loongson1/clk-loongson1c.c
> +++ b/drivers/clk/loongson1/clk-loongson1c.c
> @@ -21,9 +21,9 @@ static unsigned long ls1x_pll_recalc_rate(struct clk_hw *hw,
>         u32 pll, rate;
>
>         pll = __raw_readl(LS1X_CLK_PLL_FREQ);
> -       rate = ((pll >> 8) & 0xff) + ((pll >> 16) & 0xff);
> +       rate = (pll & 0xff00) + ((pll >> 16) & 0xff);
>         rate *= OSC;
> -       rate >>= 2;
> +       rate >>= 10;
>
>         return rate;
>  }
> --
> 2.35.1
>


-- 
Best regards,

Kelvin Cheung