Jimmy - please answer Stephen's questions below, and/or correct my answers.
On Tue, Oct 8, 2013 at 2:18 PM, Stephen Warren swarren@wwwdotorg.orgwrote:
On 10/07/2013 03:17 PM, Tom Warren wrote:
From: Jimmy Zhang jimmzhang@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