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

login
register
mail settings
Submitter Thierry Reding
Date May 24, 2012, 7:03 a.m.
Message ID <1337842993-8222-1-git-send-email-thierry.reding@avionic-design.de>
Download mbox | patch
Permalink /patch/161067/
State Not Applicable
Delegated to: Tom Warren
Headers show

Comments

Thierry Reding - May 24, 2012, 7:03 a.m.
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.

The Tegra clock and reset controller provides a means to detect the
input frequency by counting the number of oscillator periods within a
given number of 32 kHz periods. This commit introduces a function,
clock_detect_osc_freq(), which performs this calibration and programs
the CRC_OSC_CTRL accordingly.

This code is loosely based on the Linux kernel Tegra2 clock code.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 arch/arm/cpu/armv7/tegra2/ap20.c           |    2 ++
 arch/arm/cpu/armv7/tegra2/clock.c          |   42 ++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-tegra2/clk_rst.h |    9 ++++++
 arch/arm/include/asm/arch-tegra2/clock.h   |    3 ++
 4 files changed, 56 insertions(+)
Stephen Warren - May 24, 2012, 3:40 p.m.
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.
Thierry Reding - May 24, 2012, 9:03 p.m.
* 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
Stephen Warren - May 24, 2012, 10:12 p.m.
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.
Thierry Reding - May 25, 2012, 4:54 a.m.
* Stephen Warren wrote:
> On 05/24/2012 03:03 PM, Thierry Reding wrote:
> > 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?

The BCT was actually the first thing I looked at, but none of the values
seemed suspicious.

> >>> +	/* +	 * 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) {

Yes, correct.

> 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.

This also has me totally confused now. The code as it is now shouldn't write
to the CRC_OSC_CTRL register in any case and therefore, as you said shouldn't
make any difference. I'll have to double-check that I have the correct patch
applied.

Thierry

Patch

diff --git a/arch/arm/cpu/armv7/tegra2/ap20.c b/arch/arm/cpu/armv7/tegra2/ap20.c
index 698bfd0..150c713 100644
--- 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();
+
 	/* Init PMC scratch memory */
 	init_pmc_scratch();
 
diff --git a/arch/arm/cpu/armv7/tegra2/clock.c b/arch/arm/cpu/armv7/tegra2/clock.c
index ccad351..77aefbc 100644
--- a/arch/arm/cpu/armv7/tegra2/clock.c
+++ b/arch/arm/cpu/armv7/tegra2/clock.c
@@ -396,6 +396,48 @@  static s8 periph_id_to_internal_id[PERIPH_ID_COUNT] = {
 	NONE(CRAM2),
 };
 
+void clock_detect_osc_freq(void)
+{
+	struct clk_rst_ctlr *clkrst =
+			(struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE;
+	enum clock_osc_freq frequency = CLOCK_OSC_FREQ_COUNT;
+	unsigned int periods;
+	u32 value;
+
+	/* start OSC frequency detection */
+	value = OSC_FREQ_DET_TRIGGER | REF_CLK_WIN_CFG(1);
+	writel(value, &clkrst->crc_osc_freq_det);
+
+	/* wait for detection to complete */
+	do {
+		value = readl(&clkrst->crc_osc_freq_det_stat);
+		if (!(value & OSC_FREQ_DET_BUSY))
+			break;
+	} while (1);
+
+	periods = (value & OSC_FREQ_DET_CNT_MASK) >> OSC_FREQ_DET_CNT_SHIFT;
+
+	if (periods >= 732 - 3 && periods <= 732 + 3)
+		frequency = CLOCK_OSC_FREQ_12_0;
+	else if (periods >= 794 - 3 && periods <= 794 + 3)
+		frequency = CLOCK_OSC_FREQ_13_0;
+	else if (periods >= 1172 - 3 && periods <= 1172 + 3)
+		frequency = CLOCK_OSC_FREQ_19_2;
+	else if (periods >= 1587 - 3 && periods <= 1587 + 3)
+		frequency = CLOCK_OSC_FREQ_26_0;
+
+	/*
+	 * 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);
+	}
+}
+
 /*
  * Get the oscillator frequency, from the corresponding hardware configuration
  * field.
diff --git a/arch/arm/include/asm/arch-tegra2/clk_rst.h b/arch/arm/include/asm/arch-tegra2/clk_rst.h
index 8c3be91..66ca3ff 100644
--- a/arch/arm/include/asm/arch-tegra2/clk_rst.h
+++ b/arch/arm/include/asm/arch-tegra2/clk_rst.h
@@ -128,6 +128,15 @@  struct clk_rst_ctlr {
 #define OSC_XOBP_SHIFT		1
 #define OSC_XOBP_MASK		(1U << OSC_XOBP_SHIFT)
 
+/* CLK_RST_CONTROLLER_OSC_FREQ_DET_0 */
+#define OSC_FREQ_DET_TRIGGER	(1 << 31)
+#define REF_CLK_WIN_CFG(x)	((x) & 0xf)
+
+/* CLK_RST_CONTROLLER_OSC_FREQ_DET_STATUS_0 */
+#define OSC_FREQ_DET_BUSY	(1 << 31)
+#define OSC_FREQ_DET_CNT_SHIFT	0
+#define OSC_FREQ_DET_CNT_MASK	(0xffff << OSC_FREQ_DET_CNT_SHIFT)
+
 /*
  * CLK_RST_CONTROLLER_CLK_SOURCE_x_OUT_0 - the mask here is normally 8 bits
  * but can be 16. We could use knowledge we have to restrict the mask in
diff --git a/arch/arm/include/asm/arch-tegra2/clock.h b/arch/arm/include/asm/arch-tegra2/clock.h
index 1d3ae38..a86db42 100644
--- a/arch/arm/include/asm/arch-tegra2/clock.h
+++ b/arch/arm/include/asm/arch-tegra2/clock.h
@@ -192,6 +192,9 @@  enum periph_id {
 /* PLL stabilization delay in usec */
 #define CLOCK_PLL_STABLE_DELAY_US 300
 
+/* detects the oscillator clock frequency */
+void clock_detect_osc_freq(void);
+
 /* return the current oscillator clock frequency */
 enum clock_osc_freq clock_get_osc_freq(void);