| Message ID | 6053586.L6Ix8HgKpA@diego (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Heiko Stübner (2014-11-14 10:06:47) > Hi Mike, > > Am Donnerstag, 13. November 2014, 17:41:02 schrieb Mike Turquette: > > Quoting Doug Anderson (2014-11-13 15:27:32) > > [...] > > > All of the above is to say that perhaps the solution to this problem > > belongs in the driver. In the end we're talking about details for > > correctly programming hardware, which sounds an awful lot like what > > drivers are supposed to do. > > > > Let me know if the ->init() callback holds any promise for you. If not > > we can figure something out. > > From my theoretical musings, ->init() sounds like a nice idea - but of > course it comes with a "but". > > I guess the general idea would be to have the pll clk-type simply reset > to the same rate but forcing it to use the parameters from its parameter > table - when the rate params differ [0]. > > The only problem would be the apll supplying the cpu cores. After all clocks > are registered, our armclk makes sure that the core clock gets reparented > before changing the underlying apll [dpll is safe, as it is read-only currently]. > > At the moment the order would be > clk_register(apll) > apll->init() > clk_register(armclk); Sorry, but I don't understand the problem. The at registration-time, apll is re-programmed to a correct value for its current rate. Then armclk is registered which might change apll's rate. Any change to the apll which is issued from armclk should insure that apll is programmed correctly. Regards, Mike > > I'm currently unsure if simply exchanging the register-order of armclk and > the plls would help, but as the orphan handling is done before the ->init > call I guess it might help. > > > Heiko > > > [0] (compile-tested only) > diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c > index a3e886a..7f59579 100644 > --- a/drivers/clk/rockchip/clk-pll.c > +++ b/drivers/clk/rockchip/clk-pll.c > @@ -257,6 +257,40 @@ static int rockchip_rk3066_pll_is_enabled(struct clk_hw *hw) > return !(pllcon & RK3066_PLLCON3_PWRDOWN); > } > > +static void rockchip_rk3066_pll_init(struct clk_hw *hw) > +{ > + struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw); > + const struct rockchip_pll_rate_table *rate; > + unsigned int nf, nr, no, bwadj; > + unsigned long drate; > + u32 pllcon; > + > + drate = __clk_get_rate(hw->clk); > + rate = rockchip_get_pll_settings(pll, drate); > + > + /* when no rate setting for the current rate, rely on clk_set_rate */ > + if (!rate) > + return; > + > + pllcon = readl_relaxed(pll->reg_base + RK3066_PLLCON(0)); > + nr = ((pllcon >> RK3066_PLLCON0_NR_SHIFT) & RK3066_PLLCON0_NR_MASK) + 1; > + no = ((pllcon >> RK3066_PLLCON0_OD_SHIFT) & RK3066_PLLCON0_OD_MASK) + 1; > + > + pllcon = readl_relaxed(pll->reg_base + RK3066_PLLCON(1)); > + nf = ((pllcon >> RK3066_PLLCON1_NF_SHIFT) & RK3066_PLLCON1_NF_MASK) + 1; > + > + pllcon = readl_relaxed(pll->reg_base + RK3066_PLLCON(2)); > + bwadj = (pllcon >> RK3066_PLLCON2_BWADJ_SHIFT) & RK3066_PLLCON2_BWADJ_MASK; > + > + if (rate->nr != nr || rate->no != no || rate->nf != nf > + || rate->bwadj != bwadj) { > + struct clk *parent = __clk_get_parent(hw->clk); > + unsigned long prate = __clk_get_rate(parent); > + > + rockchip_rk3066_pll_set_rate(hw, drate, prate); > + } > +} > + > static const struct clk_ops rockchip_rk3066_pll_clk_norate_ops = { > .recalc_rate = rockchip_rk3066_pll_recalc_rate, > .enable = rockchip_rk3066_pll_enable, > @@ -271,6 +305,7 @@ static const struct clk_ops rockchip_rk3066_pll_clk_ops = { > .enable = rockchip_rk3066_pll_enable, > .disable = rockchip_rk3066_pll_disable, > .is_enabled = rockchip_rk3066_pll_is_enabled, > + .init = rockchip_rk3066_pll_init, > }; > > /* >
Hi, On Mon, Nov 17, 2014 at 1:14 PM, Mike Turquette <mturquette@linaro.org> wrote: > Quoting Heiko Stübner (2014-11-14 10:06:47) >> Hi Mike, >> >> Am Donnerstag, 13. November 2014, 17:41:02 schrieb Mike Turquette: >> > Quoting Doug Anderson (2014-11-13 15:27:32) >> >> [...] >> >> > All of the above is to say that perhaps the solution to this problem >> > belongs in the driver. In the end we're talking about details for >> > correctly programming hardware, which sounds an awful lot like what >> > drivers are supposed to do. >> > >> > Let me know if the ->init() callback holds any promise for you. If not >> > we can figure something out. >> >> From my theoretical musings, ->init() sounds like a nice idea - but of >> course it comes with a "but". >> >> I guess the general idea would be to have the pll clk-type simply reset >> to the same rate but forcing it to use the parameters from its parameter >> table - when the rate params differ [0]. >> >> The only problem would be the apll supplying the cpu cores. After all clocks >> are registered, our armclk makes sure that the core clock gets reparented >> before changing the underlying apll [dpll is safe, as it is read-only currently]. DPLL probably won't be read only forever... >> At the moment the order would be >> clk_register(apll) >> apll->init() >> clk_register(armclk); > > Sorry, but I don't understand the problem. The at registration-time, > apll is re-programmed to a correct value for its current rate. Then > armclk is registered which might change apll's rate. Any change to the > apll which is issued from armclk should insure that apll is programmed > correctly. I think Heiko is worried that until the "armclk" is registered that nobody is there to reparent the ARM core to GPLL while APLL changes (that's armclk's job). This is potentially unsafe. NOTE: it actually might not be unsafe, just slow. I think we'll actually swap the PLL into "slow" mode before changing it (24MHz) so we won't die we'll just run at 24MHz at boot time while we wait for APLL to re-lock. One option would be to just add yet another per-pll parameter. We'll only cleanup CPLL, GPLL, and NPLL. If APLL (ARM clock) and DPLL (memory clock) are set differently by firmware then we're just SOL. Of course if firmware boots us on GPLL then I guess we're back to square one (would firmware really be that malicious?) -Doug
Am Dienstag, 18. November 2014, 09:59:56 schrieb Doug Anderson: > Hi, > > On Mon, Nov 17, 2014 at 1:14 PM, Mike Turquette <mturquette@linaro.org> wrote: > > Quoting Heiko Stübner (2014-11-14 10:06:47) > > > >> Hi Mike, > >> > >> Am Donnerstag, 13. November 2014, 17:41:02 schrieb Mike Turquette: > >> > Quoting Doug Anderson (2014-11-13 15:27:32) > >> > >> [...] > >> > >> > All of the above is to say that perhaps the solution to this problem > >> > belongs in the driver. In the end we're talking about details for > >> > correctly programming hardware, which sounds an awful lot like what > >> > drivers are supposed to do. > >> > > >> > Let me know if the ->init() callback holds any promise for you. If not > >> > we can figure something out. > >> > >> From my theoretical musings, ->init() sounds like a nice idea - but of > >> course it comes with a "but". > >> > >> I guess the general idea would be to have the pll clk-type simply reset > >> to the same rate but forcing it to use the parameters from its parameter > >> table - when the rate params differ [0]. > >> > >> The only problem would be the apll supplying the cpu cores. After all > >> clocks are registered, our armclk makes sure that the core clock gets > >> reparented before changing the underlying apll [dpll is safe, as it is > >> read-only currently]. > DPLL probably won't be read only forever... > > >> At the moment the order would be > >> clk_register(apll) > >> apll->init() > >> clk_register(armclk); > > > > Sorry, but I don't understand the problem. The at registration-time, > > apll is re-programmed to a correct value for its current rate. Then > > armclk is registered which might change apll's rate. Any change to the > > apll which is issued from armclk should insure that apll is programmed > > correctly. > > I think Heiko is worried that until the "armclk" is registered that > nobody is there to reparent the ARM core to GPLL while APLL changes > (that's armclk's job). This is potentially unsafe. > > NOTE: it actually might not be unsafe, just slow. I think we'll > actually swap the PLL into "slow" mode before changing it (24MHz) so > we won't die we'll just run at 24MHz at boot time while we wait for > APLL to re-lock. that is correct, we switch the pll to slow-mode (which simply passes through the xin24m) before touching its params, which is also the behaviour described in the manual for this. > One option would be to just add yet another per-pll parameter. We'll > only cleanup CPLL, GPLL, and NPLL. If APLL (ARM clock) and DPLL > (memory clock) are set differently by firmware then we're just SOL. > Of course if firmware boots us on GPLL then I guess we're back to > square one (would firmware really be that malicious?) I was talking with Kever about the same thing today. My best idea would be to give our plls a flag property - like all the other clocks have (divider_flags, mux_flags, etc) - because who knows if later pll designs will need other smaller adjustments. Then adding a ROCKCHIP_PLL_FLAG_SYNC_RATE triggering the init function to check the rate params. Heiko
diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c index a3e886a..7f59579 100644 --- a/drivers/clk/rockchip/clk-pll.c +++ b/drivers/clk/rockchip/clk-pll.c @@ -257,6 +257,40 @@ static int rockchip_rk3066_pll_is_enabled(struct clk_hw *hw) return !(pllcon & RK3066_PLLCON3_PWRDOWN); } +static void rockchip_rk3066_pll_init(struct clk_hw *hw) +{ + struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw); + const struct rockchip_pll_rate_table *rate; + unsigned int nf, nr, no, bwadj; + unsigned long drate; + u32 pllcon; + + drate = __clk_get_rate(hw->clk); + rate = rockchip_get_pll_settings(pll, drate); + + /* when no rate setting for the current rate, rely on clk_set_rate */ + if (!rate) + return; + + pllcon = readl_relaxed(pll->reg_base + RK3066_PLLCON(0)); + nr = ((pllcon >> RK3066_PLLCON0_NR_SHIFT) & RK3066_PLLCON0_NR_MASK) + 1; + no = ((pllcon >> RK3066_PLLCON0_OD_SHIFT) & RK3066_PLLCON0_OD_MASK) + 1; + + pllcon = readl_relaxed(pll->reg_base + RK3066_PLLCON(1)); + nf = ((pllcon >> RK3066_PLLCON1_NF_SHIFT) & RK3066_PLLCON1_NF_MASK) + 1; + + pllcon = readl_relaxed(pll->reg_base + RK3066_PLLCON(2)); + bwadj = (pllcon >> RK3066_PLLCON2_BWADJ_SHIFT) & RK3066_PLLCON2_BWADJ_MASK; + + if (rate->nr != nr || rate->no != no || rate->nf != nf + || rate->bwadj != bwadj) { + struct clk *parent = __clk_get_parent(hw->clk); + unsigned long prate = __clk_get_rate(parent); + + rockchip_rk3066_pll_set_rate(hw, drate, prate); + } +} + static const struct clk_ops rockchip_rk3066_pll_clk_norate_ops = { .recalc_rate = rockchip_rk3066_pll_recalc_rate, .enable = rockchip_rk3066_pll_enable, @@ -271,6 +305,7 @@ static const struct clk_ops rockchip_rk3066_pll_clk_ops = { .enable = rockchip_rk3066_pll_enable, .disable = rockchip_rk3066_pll_disable, .is_enabled = rockchip_rk3066_pll_is_enabled, + .init = rockchip_rk3066_pll_init, }; /*