On Tue, 8 Sep 2009, Paul Walmsley wrote:
>
> Hi Anand,
>
> looks like a good approach, just a few minor comments.
>
> On Fri, 4 Sep 2009, Anand Gadiyar wrote:
>
> > From: Rajendra Nayak <rnayak@xxxxxx>
> >
> > OMAP3: Enable and autoidle DPLL5 at boot
> >
> > Enable DPLL5 at bootup and put it into auto-idle.
> > This is required for the USBHOST 120MHz f-clock and USBTLL f-clock
> > to work.
> >
> > Signed-off-by: Rajendra Nayak <rnayak@xxxxxx>
> > Signed-off-by: Anand Gadiyar <gadiyar@xxxxxx>
> > ---
> > arch/arm/mach-omap2/clock34xx.c | 25 +++++++++++++++++++++++++
> > 1 files changed, 25 insertions(+)
> >
> > Index: linux-omap-2.6/arch/arm/mach-omap2/clock34xx.c
> > ===================================================================
> > --- linux-omap-2.6.orig/arch/arm/mach-omap2/clock34xx.c
> > +++ linux-omap-2.6/arch/arm/mach-omap2/clock34xx.c
> > @@ -1056,6 +1056,25 @@ void omap2_clk_prepare_for_reboot(void)
> > #endif
> > }
> >
> > +static void omap3_lock_dpll5(void)
>
> Please put "clk" in this function name, like "omap3_clk_lock_dpll5" or
> something similar...
>
Okay.
> > +{
> > + struct clk *dpll5_clk;
> > + struct clk *dpll5_m2_clk;
> > +
> > + dpll5_clk = clk_get(NULL, "dpll5_ck");
> > + clk_set_rate(dpll5_clk, 120000000);
>
> Please move this constant up to a macro at the top of the file and add a
> brief descriptive comment answering the question: "why 120000000" -- e.g.,
> needed by USB, etc.
Okay.
>
> > + clk_enable(dpll5_clk);
> > +
> > + /* Enable autoidle to allow it to enter low power bypass */
> > + omap3_dpll_allow_idle(dpll5_clk);
> > +
> > + /* Program dpll5_m2_clk divider for no division */
> > + dpll5_m2_clk = clk_get(NULL, "dpll5_m2_ck");
> > + clk_enable(dpll5_m2_clk);
> > + clk_set_rate(dpll5_m2_clk, 120000000);
>
> Shouldn't we clk_disable() dpll5_clk and dpll5_m2_clk here? The rationale
> is that we should keep these disabled until some driver signals that it
> needs them with a clk_enable().
The idea behind enabling these upfront was:
- If done when the USB controller (the only user of this DPLL) needs the
USB clocks, there would be an additional delay required to lock this DPLL.
- Enabling and placing these DPLLs in bypass at init leaves no additional
power drain in the OMAP, and avoids the above delay at first clock-init.
What do you think?
- Anand
--
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
