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

Sean Anderson
2022-08-18T03:48:34Z

Hi Kelvin,

On 8/17/22 11:36 PM, Kelvin Cheung wrote:
> 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?

Unfortunately, I do not have an ls1c300 to test with. This is why I
marked the patch as RFT when I submitted it.

--Sean

>
> Sean Anderson <seanga2@gmail.com> 于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@hodcarrier.org/T/#u
>>
>> Fixes: b4626a7f4892 ("CLK: Add Loongson1C clock support")
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> 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
>>
>
>