Re: [PATCH v1 5/6] clk: rockchip: add pll up and down when change pll freq

[Doug Anderson]

Hi,

On Fri, Apr 12, 2019 at 5:16 AM Heiko StÃbner <heiko@xxxxxxxxx> wrote:
>
> Hi Elaine,
>
> Am Mittwoch, 3. April 2019, 11:44:09 CEST schrieb Elaine Zhang:
> > set pll sequence:
> >       ->set pll to slow mode or other plls
> >       ->set pll down
> >       ->set pll params
> >       ->set pll up
> >       ->wait pll lock status
> >       ->set pll to normal mode
> >
> > To slove the system error:
> > wait_pll_lock: timeout waiting for pll to lock
> > pll_set_params: pll update unsucessful,
> >               trying to restore old params
>
> Can you tell me on what soc this was experienced?
>
> The patch includes rk3399, but I don't think the CrOS kernel
> does powerdown the pll when changing the cpu-frequency
> [added Doug and Brian for clarification and possible testing :-) ]

As far as I can tell you're right.  We don't seem to have it and I'm
not aware of problems.


> But I did find that the M0 code in ATF does actually power-down the
> PLL and follow your outline from above. So essentially I'd just like
> a thumbs up from chromeos people if they have the time.

It does seem like it should be fine in general to do it.  It's one
extra step but presumably it should be fine.

In general the Rockchip PLL programming guidelines have always been a
bit funny.  Looking at the version of the doc I have, I see phrases
like "The PLL programming support changed on-the-fly and the PLL will
simply slew to the new frequency" which makes me feel like you're
supposed to be able to change the PLL frequency without powering down.
This is repeated in another part of the manual which talks about the
glitches that can happen when changing the PLL on the fly: it doesn't
say not to do it, it just says to expect glitches (which can be
avoided by changing the parent first).

...but then in another section of the doc it talks about asserting PD
before doing a frequency change!  :-P

Though in that same section it says: "Release PD after no less than
1us from the time it was asserted."  Even though probably 1 us has
passed, I'd still expect a udelay(1) to be explicit here.


One other thing that concerns me a little about this patch is that I
wonder if it is legal to call rockchip_rk3399_pll_set_params() while
the PLL is off.  AKA is it OK to change the rate of a PLL while it is
not enabled?  I'm not saying that this would have worked before
(actually, you might end up hitting the exact error "timeout waiting
for pll to lock"), but now it seems even worse because we'll
implicitly turning on the PLL.  ...a part of me wonders if this is the
root cause of the problem Elaine's patch is trying to solve: that some
code was trying to set the rate of a PLL before enabling it.


So, tl; dr:
* I doubt this patch is needed on rk3399, but it probably won't hurt.
* If you're going to do the power down, you should add the udelay()
* There's a bug on 3036.  See below.
* You should change your patch so it doesn't enable the PLL if it
wasn't already enabled.


> > Signed-off-by: Elaine Zhang <zhangqing@xxxxxxxxxxxxxx>
> > ---
> >  drivers/clk/rockchip/clk-pll.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
> > index dd0433d4753e..9fe1227e77e9 100644
> > --- a/drivers/clk/rockchip/clk-pll.c
> > +++ b/drivers/clk/rockchip/clk-pll.c
> > @@ -208,6 +208,11 @@ static int rockchip_rk3036_pll_set_params(struct rockchip_clk_pll *pll,
> >               rate_change_remuxed = 1;
> >       }
> >
> > +     /* set pll power down */
> > +     writel(HIWORD_UPDATE(1,
> > +                          RK3036_PLLCON1_PWRDOWN, 13),

This does not do what you think it does.  It should be:

HIWORD_UPDATE(RK3036_PLLCON1_PWRDOWN,
     RK3036_PLLCON1_PWRDOWN, 0)

...without that my compiler yells at me:

signed shift result (0x40000000000) requires 44 bits to represent

...and the compiler is, indeed, correct.


> > +            pll->reg_base + RK3036_PLLCON(1));
> > +
> >       /* update pll values */
> >       writel_relaxed(HIWORD_UPDATE(rate->fbdiv, RK3036_PLLCON0_FBDIV_MASK,
> >                                         RK3036_PLLCON0_FBDIV_SHIFT) |
> > @@ -229,6 +234,10 @@ static int rockchip_rk3036_pll_set_params(struct rockchip_clk_pll *pll,
> >       pllcon |= rate->frac << RK3036_PLLCON2_FRAC_SHIFT;
> >       writel_relaxed(pllcon, pll->reg_base + RK3036_PLLCON(2));
> >
> > +     /* set pll power up */
> > +     writel(HIWORD_UPDATE(0, RK3036_PLLCON1_PWRDOWN, 13),
> > +            pll->reg_base + RK3036_PLLCON(1));

In a similar vein, the above should be:

writel(HIWORD_UPDATE(0, RK3036_PLLCON1_PWRDOWN, 0),

...since RK3036_PLLCON1_PWRDOWN already has the shift.