Quoting Paul Walmsley (2013-10-07 01:21:16) > On Tue, 10 Sep 2013, Tero Kristo wrote: > > > In theory, DPLLs can also be used in their bypass mode to feed customer nodes > > clocks. I just think the check in the clkoutx2_recalc is wrong, and should be > > enhanced to actually check what is the target mode for the clock once it is > > enabled. Maybe something like this would work properly: > > > > diff --git a/arch/arm/mach-omap2/dpll3xxx.c b/arch/arm/mach-omap2/dpll3xxx.c > > index 3a0296c..ba218fb 100644 > > --- a/arch/arm/mach-omap2/dpll3xxx.c > > +++ b/arch/arm/mach-omap2/dpll3xxx.c > > @@ -658,14 +658,12 @@ unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw, > > > > dd = pclk->dpll_data; > > > > - WARN_ON(!dd->enable_mask); > > - > > - v = __raw_readl(dd->control_reg) & dd->enable_mask; > > - v >>= __ffs(dd->enable_mask); > > - if ((v != OMAP3XXX_EN_DPLL_LOCKED) || (dd->flags & DPLL_J_TYPE)) > > + if ((dd->flags & DPLL_J_TYPE) || > > + __clk_get_rate(dd->clk_bypass) == __clk_get_rate(pclk)) > > rate = parent_rate; > > else > > rate = parent_rate * 2; > > + > > return rate; > > } > > > > [...] > > > Getting comment from someone like Paul would probably help here. > > Looks like this is a regression from the CCF port. > > Seems to me that the code above assumes that when the DPLL's rate is set > to the same rate as the bypass clock's rate, we can assume that the DPLL > is in bypass. I wonder if that's valid in a case where a previous OS or > bootloader may have programmed the DPLL. Well it used to be that calling clk_set_rate(dpll, bypass_rate) would put it in bypass, I don't know if that is still the case. But you are right that having the implicit assumption that 'bypass rate' == 'DPLL in bypass' is not safe since a bootloader could lock this PLL to the same rate as it's bypass rate. I hope that the bypass rate stuff does not get swept away in the changes since it is an interesting way to save a little power. Regards, Mike > > Sounds to me like the best way to fix it would be to test whether this > code is intended to return the "target rate" (when the struct clk > representing the DPLL is disabled), versus the "current rate" (when the > struct clk representing the DPLL is enabled). If it's the target rate, > then there's no point checking the current state of the hardware. A check > similar to the above would probably be fine. Otherwise, seems best to use > the existing code that does test the PLL state. > > > > - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
