clk: ls1c: Fix PLL rate calculation

PATCH RESEND In review Sean Anderson 3 participants 4 messages Aug 23, 2022 – Oct 17, 2022

Subsystems (2)

MIPS Common clk

lore

Share & tools

lore

Sean Anderson

author

2022-08-23 03:34 UTC #0 lore raw

PATCH RESEND 1/1 clk: ls1c: Fix PLL rate calculation

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 * (256 * M_PLL + FRAC_N) / 256 / 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/[email protected]/T/#u

Fixes b4626a7f4892 ("CLK: Add Loongson1C clock support")

Signed-off-by Sean Anderson <[email protected]>

---

 drivers/clk/loongson1/clk-loongson1c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/clk/loongson1/clk-loongson1c.c +2 -2

index 1ebf740380ef..2aa839b05d6b 100644
@@ -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;
 }

Signature · 1 line

2.37.1

Sean Anderson

author

2022-08-23 03:35 UTC #1 lore raw

Quoted 87 lines · Sean Anderson

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 * (256 * M_PLL + FRAC_N) / 256 / 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/[email protected]/T/#u
Fixes b4626a7f4892 ("CLK: Add Loongson1C clock support")
Signed-off-by Sean Anderson <[email protected]>
---
  drivers/clk/loongson1/clk-loongson1c.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
drivers/clk/loongson1/clk-loongson1c.c +0 -0
index 1ebf740380ef..2aa839b05d6b 100644
@@ -21,9 +21,9 @@ static unsigned long ls1x_pll_recalc_rate(struct clk_hw *hw,
  	u32 pll, rate;

drivers/clk/loongson1/clk-loongson1c.c static unsigned long ls1x_pll_recalc_rate(struct clk_hw *hw, · line 29

Quoted 10 lines

drivers/clk/loongson1/clk-loongson1c.c +2 -2

 
  	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;
  }

+CC Stephen Boyd

Du Huanpeng

2022-08-24 01:30 UTC #2 lore raw

In reply to Sean Anderson

On Mon, Aug 22, 2022 at 11:34:14PM -0400, Sean Anderson wrote:
Dear Sean,

Quoted 2 lines

While reviewing Dhu's patch adding ls1c300 clock support to U-Boot [1], I noticed the following calculation, which is copied from

I didn't copy it from this driver, I read the document and ``try'' to understand it.
I also write a excel [1] file to calculate values for clock nodes.

[1] https://github.com/hodcarrier/ls1c300_bsp

Quoted 97 lines

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 * (256 * M_PLL + FRAC_N) / 256 / 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/[email protected]/T/#u
Fixes b4626a7f4892 ("CLK: Add Loongson1C clock support")
Signed-off-by Sean Anderson <[email protected]>
---
 drivers/clk/loongson1/clk-loongson1c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/clk/loongson1/clk-loongson1c.c +2 -2
index 1ebf740380ef..2aa839b05d6b 100644
@@ -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;
 }
Signature · 1 line
2.37.1

Stephen Boyd

maintainer

2022-10-17 21:09 UTC #3 lore raw

In reply to Sean Anderson

Quoting Sean Anderson (2022-08-22 20:34:14)

Quoted 4 lines

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:

Nobody has provided a review for this patch. If it is still important, please resend. Thanks.