2022-04-19 16:42:08

by Sean Anderson

[permalink] [raw]
Subject: [RFT PATCH] 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 * (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/[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 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


2022-04-21 09:01:47

by Du Huanpeng

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

On Tue, Apr 19, 2022 at 01:11:14AM -0400, 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:
Hi, the calculate method is based on Loongson's manual(龙芯 1C300
处理器用户手册 1.4)in page 35.
| 注: PLL 的分频系数 N 固定为 4, PLL 的频率计算公式如下:
| Freq_PLL = XIN *(M_PLL + FRAC_N)/4

I aslo made a tool to set pll rate and generate asm code at the same
time, I also put the formulae from the manual in code:

the tool:
[1]. https://github.com/hodcarrier/ls1c300_bsp/blob/master/clk-ls1c300.xlsx

lowlevel_init.S:
[2]. https://github.com/hodcarrier/u-boot/blob/lsmips/ls1c300b/arch/mips/mach-lsmips/ls1c300/lowlevel_init.S#L48
|/* Document:
| * Freq_PLL = XIN *(M_PLL + FRAC_N)/4
| */

The my v1 patch was using magic number for initialize pll, because I
use this tool to generate the code.

Set FRAC_N to 0, the pll can be adjust by step 6MHz. I noticed this
issues, you can see I always set the FRAC_N to 0 in the tool[1].
this will lost some pricise, but avoid to do the adventure...

>
> 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/[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 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
>

2022-08-03 23:31:43

by Sean Anderson

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

On 4/19/22 1:11 AM, 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 * (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/[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 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;
> }
>

Since there have been no objections, can we apply this?

--Sean


2022-08-09 19:05:43

by Stephen Boyd

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

Quoting Sean Anderson (2022-08-03 16:28:41)
> On 4/19/22 1:11 AM, Sean Anderson wrote:
> >
> > 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;
> > }
> >
>
> Since there have been no objections, can we apply this?
>

Can you resend it? I can apply it after the merge window closes next
week.

2022-08-18 03:53:53

by Sean Anderson

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

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 <[email protected]> 于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/[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 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
>>
>
>


2022-08-18 04:02:21

by Keguang Zhang

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

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 <[email protected]> 于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/[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 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

2022-11-09 05:17:03

by Du Huanpeng

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

On Thu, Aug 18, 2022 at 11:36:02AM +0800, Kelvin Cheung wrote:
Hi,
> 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?
the upstream kernel works for my board(1C300B v3.42) with diferent config,
1. base on the loongson1c_defconfig
$ make loongson1c_defconfig

2. change some options
$ make menuconfig
disable:
# CONFIG_RTC_DRV_LOONGSON1 is not set
enable:
CONFIG_BLK_DEV_INITRD=y
CONFIG_INITRAMFS_SOURCE="rootfs.cpio"
(or try this: https://github.com/hodcarrier/linux/blob/loongson-ls1c300b/arch/mips/configs/ls1c300_defconfig)

3. prepare rootfs.cpio and place it to
$ cp rootfs.cpio linux/

4. load the kernel image by pmon from TFTP server
PMON> set al "tftp://192.168.1.253/vmlinuz"
PMON> set append "earlycon=uart,0x1fe48000,ttyS2,115200 console=ttyS2,115200 root=/dev/ram0 rw mem=128M init=linuxrc"

or try my homebrew buildroot:
$ git clone https://github.com/hodcarrier/buildroot.git
$ cd buildroot
$ make loongson_ls1c300_defconfig
$ make
$ ls output/images/vmlinuz

load the vmlinuz image like above steps[4].

>
> Thanks!
>
> Sean Anderson <[email protected]> 于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/[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 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