| Message ID | 1337842993-8222-1-git-send-email-thierry.reding@avionic-design.de |
|---|---|
| State | Not Applicable |
| Delegated to: | Tom Warren |
| Headers | show |
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.
* 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
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.
* 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
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);
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(+)