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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
