[U-Boot] [PATCH] Tegra114: spl: Set system clock to clk_m

[Tom Warren]

Jimmy - please answer Stephen's questions below, and/or correct my answers.


On Tue, Oct 8, 2013 at 2:18 PM, Stephen Warren <swarren at wwwdotorg.org>wrote:

> On 10/07/2013 03:17 PM, Tom Warren wrote:
> > From: Jimmy Zhang <jimmzhang at nvidia.com>
> >
> > Based on Tegra114 TRM, system clock can run up to 275MHz. On power on,
>
> Which exact clock is "system clock"?
>

In the T1x4 TRM, SCLK is the system clock that drives the AVP/COP. It
appears to have 8 possible sources (PLLP, CLK_M, etc.). See the settings in
CLK_RST_CONTROLLER_SCLK_BURST_POLICY_0 (offset 0x28 in the CAR block).


>
> > the default sytem clock source is set to pllp_out0. In function
> > clock_early_init(), pllp_out0 will be set to 480MHz which is beyond
> system
> > clock's upper limit.
> >
> > The fix is to set the system clock to CLK_M before initializing PLLP.
> > Tested on A02 dalmore board.
>
> > diff --git a/arch/arm/cpu/arm720t/tegra-common/spl.c
> b/arch/arm/cpu/arm720t/tegra-common/spl.c
>
> > @@ -24,6 +28,9 @@ void spl_board_init(void)
> >       /* enable JTAG */
> >       writel(0xC0, &pmt->pmt_cfg_ctl);
> >
> > +     /* use clk_m as avp clock source */
> > +     set_avp_clock_to_clkm();
>
> Doesn't clk_m run at the crystal frequency, so this is going to make the
> AVP run pretty slow, at least for a while? Is that a good thing?
>

Another clock source that's closer to the 'limit' or 275MHz could probably
have been chosen, true. Jimmy?


>
> > diff --git a/arch/arm/cpu/arm720t/tegra114/cpu.c
> b/arch/arm/cpu/arm720t/tegra114/cpu.c
>
> > @@ -127,8 +127,8 @@ void t114_init_clocks(void)
> ...
> >       /*
> > -      * Switch system clock to PLLP_OUT4 (108 MHz), AVP will now run
> > -      * at 108 MHz. This is glitch free as only the source is changed,
> no
> > +      * Switch system clock to PLLP_OUT4 (204 MHz), AVP will now run
> > +      * at 204 MHz. This is glitch free as only the source is changed,
> no
> >        * special precaution needed.
> >        */
>
> ... OK, so hopefully the AVP only runs very slowly for a very short
> time. How much code runs between spl_board_init() and
> t114_init_clocks()? Should spl_board_init() do all of the AVP clock
> source setup to avoid too long a time running at crystal frequency?
>

Not sure if PLLP is set up with all of it's frequencies (OUT0-4) that early
in init. Jimmy?


>
> > @@ -152,18 +152,11 @@ void t114_init_clocks(void)
> >       clock_set_enable(PERIPH_ID_CACHE2, 1);
> >       clock_set_enable(PERIPH_ID_GPIO, 1);
> >       clock_set_enable(PERIPH_ID_TMR, 1);
> > -     clock_set_enable(PERIPH_ID_RTC, 1);
>
> This seems entirely unrelated to the intended purpose of this patch. Why
> remove all these clock_set_enable() calls? At the very least, this
> should be explained in the commit description. Since it seems like a
> different logical change, I would expect it to be a separate patch.
>

True, looks like I combined another change from the internal repo when
creating these patches. I'll move it to the correct patch in V2.


>
> > @@ -220,7 +212,7 @@ static int is_clamp_enabled(u32 mask)
> >       u32 reg;
> >
> >       /* Get clamp status. TODO: Add pmc_clamp_status alias to pmc.h */
> > -     reg = readl(&pmc->pmc_pwrgate_timer_on);
> > +     reg = readl(&pmc->pmc_clamp_status);
>
> Again, unrelated?
>

This register is called pwrgate_timer_on in one TRM and clamp_status in
another. I'll do a cleaner version of this update in V2.


>
> > +/*
> > + * On poweron, AVP clock source (also called system clock) is set to
> PLLP_out0
> > + * with frequency set at 1MHz. Before initializing PLLP, we need to
> move the
> > + * system clock's source to CLK_M temporarily. And then switch it to
> PLLP_out4
> > + * (204MHz) at a later time.
> > + */
> > +void set_avp_clock_to_clkm(void)
>
> That comment would be better located in spl_board_init() in order to
> explain why it calls this function. Any comment for this function would
> be better explaining the function itself more than why other code might
> call it.
>

I'll look at moving it to the actual set_avp_clock_to_clkm() call.

Thanks,

Tom