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

[Du Huanpeng <dhu@hodcarrier.org>]

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 <seanga2@gmail.com> 于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/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 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