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(-)
diff --git a/drivers/clk/loongson1/clk-loongson1c.c b/drivers/clk/loongson1/clk-loongson1c.c
index 1ebf740380ef..2aa839b05d6b 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.37.1
On 8/22/22 11:34 PM, Sean Anderson wrote:
> 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(-)
>
> diff --git a/drivers/clk/loongson1/clk-loongson1c.c b/drivers/clk/loongson1/clk-loongson1c.c
> index 1ebf740380ef..2aa839b05d6b 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;
> }
>
+CC Stephen Boyd
On Mon, Aug 22, 2022 at 11:34:14PM -0400, Sean Anderson wrote:
Dear Sean,
> 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
> 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(-)
>
> diff --git a/drivers/clk/loongson1/clk-loongson1c.c b/drivers/clk/loongson1/clk-loongson1c.c
> index 1ebf740380ef..2aa839b05d6b 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.37.1
>
Quoting Sean Anderson (2022-08-22 20:34:14)
> 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.