[U-Boot] [PATCH] tegra: Implement oscillator frequency detection

[Stephen Warren <swarren@wwwdotorg.org>]

On 05/24/2012 03:03 PM, Thierry Reding wrote:
> * 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.

Odd. Yes, I have tested on Harmony. I think all/most of the boards I
have use a 12MHz clock.

I wonder if this is due to some incorrect setting in your BCT?

>>> +	/* +	 * 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.

The kernel code supports 4 explicit rates, and if the measured clock
is any of those rates, it writes the appropriate enum to the OSC_CTRL
register.

However, the U-Boot code above only writes to OSC_CTRL in the case
where no known match was found. Perhaps it's just that:

>>> +	if (frequency >= CLOCK_OSC_FREQ_COUNT) {

should be:

>>> +	if (frequency < CLOCK_OSC_FREQ_COUNT) {

Given that though, I'm confused why this patch makes any difference,
unless I'm just totally misreading it?

I think when I first read your patch, I thought there were other
differences between kernel and U-Boot, but upon further inspection I
think not.