Quoting Tero Kristo (2013-09-10 06:17:45) > On 09/10/2013 03:40 PM, Tomi Valkeinen wrote: > > On 10/09/13 15:24, Tero Kristo wrote: > >> On 09/10/2013 03:19 PM, Tomi Valkeinen wrote: > >>> On 10/09/13 15:12, Tero Kristo wrote: > >>> > >>>> If it claims it is not locked, it means the DPLL itself is disabled. You > >>>> could try clk_enable for the clock before doing clk_set_rate. > >>> > >>> Hmm, so is it required to enable the clock before setting the rate? If > >>> so, I think I'm using the clocks wrong in all the places =). > >> > >> In generic case, it is not. But DPLLs behave strangely if they go to low > >> power stop mode. If there is any downstream clock enabled for a specific > >> DPLL it is enabled and things work okay. > >> > >> One could also argue that the API behavior in OMAP is wrong currently, > >> as the bypass rate is something you are most likely never actually going > >> to use for anything.... > >> > >> Just try the change and check the results. > > > > Ok, so as Stefan said, enabling the clock fixes the issue. > > > > How do you suggest we fix this? Changing omapdss to enable the clock > > before changing its rate is not very difficult, so it can be used as a > > quick fix. But it doesn't sound like a proper fix if this is not > > normally required. > > The quick fix sounds good to me. > > > And, maybe I'm missing something as I don't have good understanding of > > the PRCM's PLLs, but the current behavior sounds odd. So the DPLL is > > off, and in bypass mode. When we try to change the rate of the clock > > provided by the PLL, shouldn't it fail, as bypass mode's rate cannot be > > changed? Or better, change the non-bypass rate. > > 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)) Two quick asides here: 1) might be nice if the J_TYPE pll was it's own clock type 2) would be nice if the check for bypass used something like: if (clk_get_parent(hw->clk) == dd->bypass_clk) rate = parent_rate; Which implies that the DPLLs become proper mux clocks. Regards, Mike > rate = parent_rate; > else > rate = parent_rate * 2; > + > return rate; > } > > > > > > How is the DPLL4's clock rate 432000000 anyway in bypass mode. Isn't > > bypass mode usually plain sys-clock, or such? > > This again reflects the rate that the clock has once it is enabled, the > clkoutx2 does not. > > Getting comment from someone like Paul would probably help here. > > -Tero -- 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
