Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220419051114.1569291-1-seanga2@gmail.com>
In-Reply-To: <20220419051114.1569291-1-seanga2@gmail.com>
From:   Kelvin Cheung <keguang.zhang@gmail.com>
Date:   Thu, 18 Aug 2022 11:36:02 +0800
Message-ID: <CAJhJPsUM=LrgrKcoA8xT=4JWt8uxjn6yDxP9vjuZmvb4WvjPZQ@mail.gmail.com>
Subject: Re: [RFT PATCH] clk: ls1c: Fix PLL rate calculation
To:     Sean Anderson <seanga2@gmail.com>
Cc:     linux-mips@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, Du Huanpeng <dhu@hodcarrier.org>,
        Stephen Boyd <sboyd@codeaurora.org>,
        Yang Ling <gnaygnil@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 me=
rged.
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 <seanga2@gmail.com> =E4=BA=8E2022=E5=B9=B44=E6=9C=8819=E6=97=
=A5=E5=91=A8=E4=BA=8C 13:11=E5=86=99=E9=81=93=EF=BC=9A
>
> 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 =3D clk_get_parent_rate(clk);
>         base =3D (void *)clk->data;
>
>         val =3D readl(base + START_FREQ);
>         mult =3D 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 mult=
iplier
> for the PLL. The datasheet has the following to say:
>
> START_FREQ =E4=BD=8D    =E7=BC=BA=E7=9C=81=E5=80=BC      =E6=8F=8F=E8=BF=
=B0
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> FRAC_N     23:16 0           PLL =E5=80=8D=E9=A2=91=E7=B3=BB=E6=95=B0=E7=
=9A=84=E5=B0=8F=E6=95=B0=E9=83=A8=E5=88=86
>
>                  =E7=94=B1          PLL =E5=80=8D=E9=A2=91=E7=B3=BB=E6=95=
=B0=E7=9A=84=E6=95=B4=E6=95=B0=E9=83=A8=E5=88=86
> M_PLL      15:8  NAND_D[3:0] (=E7=90=86=E8=AE=BA=E5=8F=AF=E4=BB=A5=E8=BE=
=BE=E5=88=B0 255=EF=BC=8C=E5=BB=BA=E8=AE=AE=E4=B8=8D=E8=A6=81=E8=B6=85=E8=
=BF=87 100)
>                  =E9=85=8D=E7=BD=AE
>
> which according to google translate means
>
> START_FREQ Bits  Default       Description
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D
> 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 =3D parent * (255 * M_PLL + FRAC_N) / 255 / 4
>
> However, the datasheet also gives the following formula:
>
>         rate =3D 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 aut=
hor of
> the datasheet may think of a multiplier of (say) 3.14 as
>
>         M_PLL =3D 3
>         FRAC_N =3D 0.14
>
> which together sum to the correct multiplier, even though the actual valu=
e
> stored in FRAC_N would be 36.
>
> I suspect that this has slipped by unnoticed because when FRAC_N is 0, th=
ere is
> no difference in the formulae. The following patch is untested, but I sus=
pect
> 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 clock=
s) 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/loongso=
n1/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 =3D __raw_readl(LS1X_CLK_PLL_FREQ);
> -       rate =3D ((pll >> 8) & 0xff) + ((pll >> 16) & 0xff);
> +       rate =3D (pll & 0xff00) + ((pll >> 16) & 0xff);
>         rate *=3D OSC;
> -       rate >>=3D 2;
> +       rate >>=3D 10;
>
>         return rate;
>  }
> --
> 2.35.1
>


--=20
Best regards,

Kelvin Cheung