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