* Stephen Warren wrote:
On 05/24/2012 01:03 AM, Thierry Reding wrote:
Upon reset, the CRC_OSC_CTRL register defaults to a 13 MHz oscillator input frequency. With Lucas' recent commit b8cb519 ("tegra2: trivially enable 13 mhz crystal frequency) applied, this breaks on hardware that provides a different frequency.
Can you expand upon "breaks"? Do you mean "detects the wrong value", or "causes U-Boot to fail to execute successfully", or...
For reference, I have this commit in my local branch, and have run U-Boot on at least a couple of our boards without any apparent issue.
But, I agree there is a problem that should be fixed; I'm just not sure what the current impact is.
On Tamonten, U-Boot doesn't execute properly. Or at least I can't tell because it may just be that there is no output whatsoever on the serial port (perhaps due to the peripheral clock being configured wrongly?).
Strange thing is that if I don't do the frequency detection and without Lucas' patch things still work, even though CRC_OSC_CTRL contains the value for a 13 MHz clock.
Have you tested on Harmony? I believe that has a 12 MHz oscillator as well, so it should have the same problem than Tamonten.
diff --git a/arch/arm/cpu/armv7/tegra2/ap20.c b/arch/arm/cpu/armv7/tegra2/ap20.c
@@ -351,6 +351,8 @@ void tegra2_start(void) /* not reached */ }
- clock_detect_osc_freq();
Would this be better called from clock_early_init() in clock.c? That's called only very marginally later than tegra2_start(), and would keep all the clock-related code together. The patch would also edit fewer files:-)
On a second look that should be possible. I thought it was being used by the warmboot code, which is initialized in init_pmc_scratch(). But that's clock_get_osc_bypass(). I'll move the call to clock_early_init() and recheck if it works for me.
diff --git a/arch/arm/cpu/armv7/tegra2/clock.c b/arch/arm/cpu/armv7/tegra2/clock.c
+void clock_detect_osc_freq(void)
...
- else if (periods >= 1587 - 3 && periods <= 1587 + 3)
frequency = CLOCK_OSC_FREQ_26_0;Everything up to here looks good, and does indeed match the kernel.
- /*
* Configure oscillator frequency. If the measured frequency isn't* among those supported, keep the default and hope for the best.*/- if (frequency >= CLOCK_OSC_FREQ_COUNT) {
value = readl(&clkrst->crc_osc_ctrl);value &= ~OSC_FREQ_MASK;value |= frequency << OSC_FREQ_SHIFT;writel(value, &clkrst->crc_osc_ctrl);- }
+}
The above is quite different from what the kernel does, which is the following:
static unsigned long tegra2_clk_m_autodetect_rate(struct clk *c) { u32 auto_clock_control = clk_readl(OSC_CTRL) & ~OSC_CTRL_OSC_FREQ_MASK;
c->rate = clk_measure_input_freq(); switch (c->rate) { case 12000000: auto_clock_control |= OSC_CTRL_OSC_FREQ_12MHZ; break; case 13000000: auto_clock_control |= OSC_CTRL_OSC_FREQ_13MHZ; break; case 19200000: auto_clock_control |= OSC_CTRL_OSC_FREQ_19_2MHZ; break; case 26000000: auto_clock_control |= OSC_CTRL_OSC_FREQ_26MHZ; break; default: pr_err("%s: Unexpected clock rate %ld", __func__, c->rate); BUG(); } clk_writel(auto_clock_control, OSC_CTRL); return c->rate; }
Is there a specific reason for U-Boot not to do the same thing here?
I can't see any difference between the two. Except that the U-Boot code doesn't BUG(), but instead continues hoping for the best.
diff --git a/arch/arm/include/asm/arch-tegra2/clk_rst.h b/arch/arm/include/asm/arch-tegra2/clk_rst.h
+/* CLK_RST_CONTROLLER_OSC_FREQ_DET_0 */ +#define OSC_FREQ_DET_TRIGGER (1 << 31)
Nitpicky I know, but this is "TRIG" not "TRIGGER" in the TRM. It's probably a good idea to stay consistent where possible.
Right, I'll fix that up.
Thierry