Skip to contentSkip to conversation

tegra: Implement oscillator frequency detection

PATCH New Thierry Reding 2 participants 5 messages May 24, 2012 – May 25, 2012

Subsystems (2)
Arm (32-bit) Tegra architecture support
lore
Share & tools
lore

Thierry Reding

author
#0 lore raw

PATCH 1/1 tegra: Implement oscillator frequency detection

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 <[email protected]>
---
 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(+)
arch/arm/cpu/armv7/tegra2/ap20.c +2 -0
index 698bfd0..150c713 100644
@@ -351,6 +351,8 @@ void tegra2_start(void)
 		/* not reached */
 	}
 
+	clock_detect_osc_freq();
+
 	/* Init PMC scratch memory */
 	init_pmc_scratch();
arch/arm/cpu/armv7/tegra2/clock.c +42 -0

48 lines not on this page · raw message

arch/arm/include/asm/arch-tegra2/clk_rst.h +9 -0
index 8c3be91..66ca3ff 100644
@@ -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
arch/arm/include/asm/arch-tegra2/clock.h +3 -0
index 1d3ae38..a86db42 100644
@@ -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);
Signature · 1 line
1.7.10.2
#1 lore raw

PATCH 1/1 tegra: Implement oscillator frequency detection

Quoted 4 lines · Thierry Reding

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.

arch/arm/cpu/armv7/tegra2/ap20.c void tegra2_start(void) · line 354

Quoted 7 lines
arch/arm/cpu/armv7/tegra2/ap20.c +1 -0
@@ -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:-)

Quoted 3 lines
arch/arm/cpu/armv7/tegra2/clock.c +1 -0
 
+void clock_detect_osc_freq(void)

...

arch/arm/cpu/armv7/tegra2/clock.c static s8 periph_id_to_internal_id[PERIPH_ID_COUNT] = { · line 427

Quoted 2 lines
arch/arm/cpu/armv7/tegra2/clock.c +2 -0
+	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.

arch/arm/cpu/armv7/tegra2/clock.c static s8 periph_id_to_internal_id[PERIPH_ID_COUNT] = { · line 439

Quoted 11 lines
arch/arm/cpu/armv7/tegra2/clock.c +11 -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);
+	}
+}

The above is quite different from what the kernel does, which is the following:

Quoted 17 lines

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;
Quoted 8 lines
		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?

arch/arm/include/asm/arch-tegra2/clk_rst.h struct clk_rst_ctlr { · line 132

Quoted 4 lines
arch/arm/include/asm/arch-tegra2/clk_rst.h +2 -0
 
+/* 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.

author
#2 lore raw

PATCH 1/1 tegra: Implement oscillator frequency detection

Quoted 6 lines · Stephen Warren
Quoted 4 lines · Thierry Reding

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.

Quoted 8 lines

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.

Quoted 7 lines
Quoted 7 lines
arch/arm/cpu/armv7/tegra2/ap20.c +0 -0
@@ -351,6 +351,8 @@ void tegra2_start(void)
 		/* not reached */
 	}
Quoted 6 lines

arch/arm/cpu/armv7/tegra2/ap20.c void tegra2_start(void) · line 354

Quoted 1 line
arch/arm/cpu/armv7/tegra2/ap20.c +1 -0
+	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.

Quoted 50 lines
Quoted 3 lines
arch/arm/cpu/armv7/tegra2/clock.c +1 -0
 
+void clock_detect_osc_freq(void)

...

arch/arm/cpu/armv7/tegra2/clock.c static s8 periph_id_to_internal_id[PERIPH_ID_COUNT] = { · line 427

Quoted 2 lines
arch/arm/cpu/armv7/tegra2/clock.c +2 -0
+	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.

arch/arm/cpu/armv7/tegra2/clock.c static s8 periph_id_to_internal_id[PERIPH_ID_COUNT] = { · line 439

Quoted 11 lines
arch/arm/cpu/armv7/tegra2/clock.c +11 -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);
+	}
+}

The above is quite different from what the kernel does, which is the following:

Quoted 17 lines

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;
Quoted 8 lines
		break;
	default:
		pr_err("%s: Unexpected clock rate %ld", __func__, c->rate);
		BUG();
	}
	clk_writel(auto_clock_control, OSC_CTRL);
	return c->rate;
}
Quoted 1 line

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.

Quoted 7 lines

arch/arm/include/asm/arch-tegra2/clk_rst.h struct clk_rst_ctlr { · line 132

Quoted 4 lines
arch/arm/include/asm/arch-tegra2/clk_rst.h +2 -0
 
+/* 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>

#3 lore raw

PATCH 1/1 tegra: Implement oscillator frequency detection

Quoted 23 lines · Thierry Reding
Quoted 10 lines · Stephen Warren
Quoted 4 lines · Thierry Reding

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

Quoted 6 lines

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

Quoted 6 lines

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?

Quoted 25 lines
Quoted 25 lines
Quoted 7 lines
(quoted diff) +1 -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); + } +}

The above is quite different from what the kernel does, which is the following:

Quoted 13 lines

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; }

Quoted 5 lines
Quoted 2 lines

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:

Quoted 1 line
Quoted 1 line

arch/arm/cpu/armv7/tegra2/clock.c static s8 periph_id_to_internal_id[PERIPH_ID_COUNT] = { · line 433

Quoted 1 line
arch/arm/cpu/armv7/tegra2/clock.c +1 -0
+	if (frequency >= CLOCK_OSC_FREQ_COUNT) {

should be:

Quoted 1 line
Quoted 1 line

Review

Quoted 1 line
(quoted diff) +1 -0
+	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.

author
#4 lore raw

PATCH 1/1 tegra: Implement oscillator frequency detection

Quoted 10 lines · Stephen Warren
Quoted 9 lines · Thierry Reding

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.

Quoted 7 lines
Quoted 2 lines

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.

Quoted 35 lines
Quoted 25 lines
Quoted 25 lines
Quoted 7 lines
(quoted diff) +1 -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); + } +}

The above is quite different from what the kernel does, which is the following:

Quoted 13 lines

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; }

Quoted 5 lines
Quoted 2 lines

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.

Quoted 8 lines

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:

Quoted 1 line
Quoted 1 line

arch/arm/cpu/armv7/tegra2/clock.c static s8 periph_id_to_internal_id[PERIPH_ID_COUNT] = { · line 433

Quoted 1 line
arch/arm/cpu/armv7/tegra2/clock.c +1 -0
+	if (frequency >= CLOCK_OSC_FREQ_COUNT) {

should be:

Quoted 1 line
Quoted 1 line

Review

Quoted 1 line
(quoted diff) +1 -0
+	if (frequency < CLOCK_OSC_FREQ_COUNT) {

Yes, correct.

Quoted 6 lines

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
-------------- 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/20120525/81466890/attachment.pgp>