Message-ID: <20220420233034.GA5694@black>
Date:   Thu, 21 Apr 2022 07:30:34 +0800
From:   Du Huanpeng <dhu@...carrier.org>
To:     Sean Anderson <seanga2@...il.com>
Cc:     linux-mips@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Keguang Zhang <keguang.zhang@...il.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Yang Ling <gnaygnil@...il.com>
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/20220418204519.19991-1-dhu@hodcarrier.org/T/#u
> 
> Fixes: b4626a7f4892 ("CLK: Add Loongson1C clock support")
> Signed-off-by: Sean Anderson <seanga2@...il.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
> 

Powered by blists - more mailing lists