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

[Thierry Reding <thierry.reding@avionic-design.de>]

* 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120524/302b800f/attachment.pgp>