Re: [RFT PATCH] clk: ls1c: Fix PLL rate calculation
From: Sean Anderson
Date: Wed Aug 17 2022 - 23:48:34 EST
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@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