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.
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:-)
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?
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.