* [U-Boot] [PATCH] tegra: Implement oscillator frequency detection
@ 2012-05-24 7:03 Thierry Reding
2012-05-24 15:40 ` Stephen Warren
0 siblings, 1 reply; 5+ messages in thread
From: Thierry Reding @ 2012-05-24 7:03 UTC (permalink / raw)
To: u-boot
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(+)
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);
--
1.7.10.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] tegra: Implement oscillator frequency detection
2012-05-24 7:03 [U-Boot] [PATCH] tegra: Implement oscillator frequency detection Thierry Reding
@ 2012-05-24 15:40 ` Stephen Warren
2012-05-24 21:03 ` Thierry Reding
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Warren @ 2012-05-24 15:40 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] tegra: Implement oscillator frequency detection
2012-05-24 15:40 ` Stephen Warren
@ 2012-05-24 21:03 ` Thierry Reding
2012-05-24 22:12 ` Stephen Warren
0 siblings, 1 reply; 5+ messages in thread
From: Thierry Reding @ 2012-05-24 21:03 UTC (permalink / raw)
To: u-boot
* 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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] tegra: Implement oscillator frequency detection
2012-05-24 21:03 ` Thierry Reding
@ 2012-05-24 22:12 ` Stephen Warren
2012-05-25 4:54 ` Thierry Reding
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Warren @ 2012-05-24 22:12 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] tegra: Implement oscillator frequency detection
2012-05-24 22:12 ` Stephen Warren
@ 2012-05-25 4:54 ` Thierry Reding
0 siblings, 0 replies; 5+ messages in thread
From: Thierry Reding @ 2012-05-25 4:54 UTC (permalink / raw)
To: u-boot
* 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
-------------- 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>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-05-25 4:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-24 7:03 [U-Boot] [PATCH] tegra: Implement oscillator frequency detection Thierry Reding
2012-05-24 15:40 ` Stephen Warren
2012-05-24 21:03 ` Thierry Reding
2012-05-24 22:12 ` Stephen Warren
2012-05-25 4:54 ` Thierry Reding
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.