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

[Stephen Warren]

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.