| Message ID | 1554284649-26764-1-git-send-email-zhangqing@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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 :-) ] 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. Heiko > Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com> > --- > 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), > + 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)); > + > /* wait for the pll to lock */ > ret = rockchip_pll_wait_lock(pll); > if (ret) { > @@ -685,6 +694,11 @@ static int rockchip_rk3399_pll_set_params(struct rockchip_clk_pll *pll, > rate_change_remuxed = 1; > } > > + /* set pll power down */ > + writel(HIWORD_UPDATE(RK3399_PLLCON3_PWRDOWN, > + RK3399_PLLCON3_PWRDOWN, 0), > + pll->reg_base + RK3399_PLLCON(3)); > + > /* update pll values */ > writel_relaxed(HIWORD_UPDATE(rate->fbdiv, RK3399_PLLCON0_FBDIV_MASK, > RK3399_PLLCON0_FBDIV_SHIFT), > @@ -708,6 +722,11 @@ static int rockchip_rk3399_pll_set_params(struct rockchip_clk_pll *pll, > RK3399_PLLCON3_DSMPD_SHIFT), > pll->reg_base + RK3399_PLLCON(3)); > > + /* set pll power up */ > + writel(HIWORD_UPDATE(0, > + RK3399_PLLCON3_PWRDOWN, 0), > + pll->reg_base + RK3399_PLLCON(3)); > + > /* wait for the pll to lock */ > ret = rockchip_rk3399_pll_wait_lock(pll); > if (ret) { >
Hi, On Fri, Apr 12, 2019 at 5:16 AM Heiko Stübner <heiko@sntech.de> 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@rock-chips.com> > > --- > > 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.
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), + 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)); + /* wait for the pll to lock */ ret = rockchip_pll_wait_lock(pll); if (ret) { @@ -685,6 +694,11 @@ static int rockchip_rk3399_pll_set_params(struct rockchip_clk_pll *pll, rate_change_remuxed = 1; } + /* set pll power down */ + writel(HIWORD_UPDATE(RK3399_PLLCON3_PWRDOWN, + RK3399_PLLCON3_PWRDOWN, 0), + pll->reg_base + RK3399_PLLCON(3)); + /* update pll values */ writel_relaxed(HIWORD_UPDATE(rate->fbdiv, RK3399_PLLCON0_FBDIV_MASK, RK3399_PLLCON0_FBDIV_SHIFT), @@ -708,6 +722,11 @@ static int rockchip_rk3399_pll_set_params(struct rockchip_clk_pll *pll, RK3399_PLLCON3_DSMPD_SHIFT), pll->reg_base + RK3399_PLLCON(3)); + /* set pll power up */ + writel(HIWORD_UPDATE(0, + RK3399_PLLCON3_PWRDOWN, 0), + pll->reg_base + RK3399_PLLCON(3)); + /* wait for the pll to lock */ ret = rockchip_rk3399_pll_wait_lock(pll); if (ret) {
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 Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com> --- drivers/clk/rockchip/clk-pll.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)