| Message ID | 20220823033414.198525-1-seanga2@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [RESEND] clk: ls1c: Fix PLL rate calculation | expand |
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/20220418204519.19991-1-dhu@hodcarrier.org/T/#u > > Fixes: b4626a7f4892 ("CLK: Add Loongson1C clock support") > Signed-off-by: Sean Anderson <seanga2@gmail.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 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/20220418204519.19991-1-dhu@hodcarrier.org/T/#u > > Fixes: b4626a7f4892 ("CLK: Add Loongson1C clock support") > Signed-off-by: Sean Anderson <seanga2@gmail.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 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.
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; }
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/20220418204519.19991-1-dhu@hodcarrier.org/T/#u Fixes: b4626a7f4892 ("CLK: Add Loongson1C clock support") Signed-off-by: Sean Anderson <seanga2@gmail.com> --- drivers/clk/loongson1/clk-loongson1c.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)