Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On 12/16/2014 04:58 PM, Nishanth Menon wrote: On 17:05-20141216, Lokesh Vutla wrote: [...] @@ -545,6 +546,16 @@ static void __init realtime_counter_init(void) break; } + if (soc_is_dra7xx()) { + reg = omap_ctrl_readl(DRA7_CTRL_CORE_BOOTSTRAP); + reg = reg DRA7_SPEEDSELECT_MASK; + + if (reg) { + num = 75; + den = 244; + } + } + /* Program numerator and denumerator registers */ reg = readl_relaxed(base + INCREMENTER_NUMERATOR_OFFSET) NUMERATOR_DENUMERATOR_MASK; A) So, it does look like the 32k node is actually wrong then - Tero: any comments? should'nt this now be modeled based on CTRL_CORE_BOOTSTRAP::SPEEDSELECT considering that clock nodes do have clk mux options based on the 32k.. sys_32k_ck: sys_32k_ck { #clock-cells = 0; compatible = fixed-clock; clock-frequency = 32768; }; Yea I think the 32k clock node should be fixed based on this. Something like this: --- a/arch/arm/boot/dts/dra7xx-clocks.dtsi +++ b/arch/arm/boot/dts/dra7xx-clocks.dtsi @@ -100,8 +100,10 @@ sys_32k_ck: sys_32k_ck { #clock-cells = 0; - compatible = fixed-clock; - clock-frequency = 32768; + compatible = fixed-factor-clock; + clocks = sys_clkin1; + clock-mult = 1; + clock-div = 610; }; virt_1200_ck: virt_1200_ck { It might be better then just query the actual clock rate from the timer code. -Tero B) Since rate switch is no longer needed, how about something like the following: diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h index a3c0133..315d6d1 100644 --- a/arch/arm/mach-omap2/control.h +++ b/arch/arm/mach-omap2/control.h @@ -286,6 +286,11 @@ #define OMAP5XXX_CONTROL_STATUS0x134 #define OMAP5_DEVICETYPE_MASK (0x7 6) + +/* DRA7XX CONTROL CORE BOOTSTRAP */ +#define DRA7_CTRL_CORE_BOOTSTRAP 0x6c4 +#define DRA7_SPEEDSELECT_MASK (0x3 8) + /* * REVISIT: This list of registers is not comprehensive - there are more * that should be added. diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index 4f61148..783d3c3 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -54,6 +54,7 @@ #include soc.h #include common.h +#include control.h #include powerdomain.h #include omap-secure.h @@ -511,6 +512,35 @@ static void __init realtime_counter_init(void) } rate = clk_get_rate(sys_clk); + + if (soc_is_dra7xx()) { + /* +* Errata i856 says the 32.768KHz crystal does not start at +* power on, so the CPU falls back in an emulated 32KHz clock +* based on sysclk / 610 instead. This causes the master counter +* frequency to not be 6.144MHz but at sysclk / 610 * 375 / 2 +* (OR sysclk * 75 / 244) +* +* This affects at least the DRA7/AM572x 1.0, 1.1 revisions. +* Of course any board built without a populated 32.768KHz +* crystal would also need this fix even if the CPU is fixed +* later. +* +* Either case can be detected by using the two speedselect bits +* If they are not 0, then the 32.768KHz clock driving the +* course counter that corrects the fine counter every time it +* ticks is actually rate/610 rather than 32.768KHz and we +* should compensate to avoid the 570ppm (At 20MHz, much worse +* at other rates) too fast system time. +*/ + reg = omap_ctrl_readl(DRA7_CTRL_CORE_BOOTSTRAP); + if (reg DRA7_SPEEDSELECT_MASK) { + num = 75; + den = 244; + goto sysclk_based; + } + } + /* Numerator/denumerator values refer TRM Realtime Counter section */ switch (rate) { case 120: @@ -545,6 +575,7 @@ static void __init realtime_counter_init(void) break; } +sysclk_based: /* Program numerator and denumerator registers */ reg = readl_relaxed(base + INCREMENTER_NUMERATOR_OFFSET) NUMERATOR_DENUMERATOR_MASK; -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On Wed, Dec 17, 2014 at 03:21:27PM +0200, Tero Kristo wrote: Yea I think the 32k clock node should be fixed based on this. Something like this: --- a/arch/arm/boot/dts/dra7xx-clocks.dtsi +++ b/arch/arm/boot/dts/dra7xx-clocks.dtsi @@ -100,8 +100,10 @@ sys_32k_ck: sys_32k_ck { #clock-cells = 0; - compatible = fixed-clock; - clock-frequency = 32768; + compatible = fixed-factor-clock; + clocks = sys_clkin1; + clock-mult = 1; + clock-div = 610; }; virt_1200_ck: virt_1200_ck { It might be better then just query the actual clock rate from the timer code. But it is only SYSCLK1 / 610 if the DRA7_CTRL_CORE_BOOTSTRAP register says it is. Otherwise is is in fact 32768Hz. I certainly would expect that if another revision of the chip is made (and even for the single core AM571x chips when they are done) will have this fixed and will work with the external 32.768KHz crystal, if it is present. So how does one make the dtb reflect what the state of the CPU actually is? The errata even says that if SYSCLK1 is 26MHz (not a supported option in the manual), then the 32.768KHz crystal does work and the counter frequency will in fact be 6.144MHz like it should be. So just changing the dtb is not an option. -- Len Sorensen -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On 09:55-20141217, Lennart Sorensen wrote: On Wed, Dec 17, 2014 at 03:21:27PM +0200, Tero Kristo wrote: Yea I think the 32k clock node should be fixed based on this. Something like this: --- a/arch/arm/boot/dts/dra7xx-clocks.dtsi +++ b/arch/arm/boot/dts/dra7xx-clocks.dtsi @@ -100,8 +100,10 @@ sys_32k_ck: sys_32k_ck { #clock-cells = 0; - compatible = fixed-clock; - clock-frequency = 32768; + compatible = fixed-factor-clock; + clocks = sys_clkin1; + clock-mult = 1; + clock-div = 610; }; virt_1200_ck: virt_1200_ck { It might be better then just query the actual clock rate from the timer code. But it is only SYSCLK1 / 610 if the DRA7_CTRL_CORE_BOOTSTRAP register says it is. Otherwise is is in fact 32768Hz. I certainly would expect that if another revision of the chip is made (and even for the single core AM571x chips when they are done) will have this fixed and will work with the external 32.768KHz crystal, if it is present. So how does one make the dtb reflect what the state of the CPU actually is? The errata even says that if SYSCLK1 is 26MHz (not a supported option in the manual), then the 32.768KHz crystal does work and the counter frequency will in fact be 6.144MHz like it should be. So just changing the dtb is not an option. A clock mux might do the job? value 1, 2 , 3 will imply sysclk1 / 610 value of 0 implies fixed 32768 soemthing like sys_clk32_crystal { compatible = fixed-clock; clock-frequency = 32768; } sys_clk32_pseudo { compatible = fixed-clock; compatible = fixed-factor-clock; clocks = sys_clkin1; clock-mult = 1; clock-div = 610; } sys_32k_ck: sys_32k_ck { compatible = ti,mux-clock; clocks = sys_clk32_crystal, sys_clk32_pseudo, sys_clk32_pseudo, sys_clk32_pseudo; }; I think... The only issue is that the BOOTSTRAP register is not around the usual CM1,2 address region... -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On Wed, Dec 17, 2014 at 09:22:25AM -0600, Nishanth Menon wrote: A clock mux might do the job? value 1, 2 , 3 will imply sysclk1 / 610 value of 0 implies fixed 32768 soemthing like sys_clk32_crystal { compatible = fixed-clock; clock-frequency = 32768; } sys_clk32_pseudo { compatible = fixed-clock; compatible = fixed-factor-clock; clocks = sys_clkin1; clock-mult = 1; clock-div = 610; } sys_32k_ck: sys_32k_ck { compatible = ti,mux-clock; clocks = sys_clk32_crystal, sys_clk32_pseudo, sys_clk32_pseudo, sys_clk32_pseudo; }; I think... The only issue is that the BOOTSTRAP register is not around the usual CM1,2 address region... OK, I like where that idea is headed. That looks promising. -- Len Sorensen -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On 12/17/2014 05:27 PM, Lennart Sorensen wrote: On Wed, Dec 17, 2014 at 09:22:25AM -0600, Nishanth Menon wrote: A clock mux might do the job? value 1, 2 , 3 will imply sysclk1 / 610 value of 0 implies fixed 32768 soemthing like sys_clk32_crystal { compatible = fixed-clock; clock-frequency = 32768; } sys_clk32_pseudo { compatible = fixed-clock; compatible = fixed-factor-clock; clocks = sys_clkin1; clock-mult = 1; clock-div = 610; } sys_32k_ck: sys_32k_ck { compatible = ti,mux-clock; clocks = sys_clk32_crystal, sys_clk32_pseudo, sys_clk32_pseudo, sys_clk32_pseudo; }; I think... The only issue is that the BOOTSTRAP register is not around the usual CM1,2 address region... OK, I like where that idea is headed. That looks promising. Yea clock mux can be used. However, we don't have support for DRA7 control module clocks in the DT yet. I have posted patches with support towards this a couple of weeks back, but they need some revising. Thus, we maybe need to apply the timer patches as is for now, and fix the clock tree a bit later. -Tero -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On Wed, Dec 17, 2014 at 05:45:05PM +0200, Tero Kristo wrote: Yea clock mux can be used. However, we don't have support for DRA7 control module clocks in the DT yet. I have posted patches with support towards this a couple of weeks back, but they need some revising. Thus, we maybe need to apply the timer patches as is for now, and fix the clock tree a bit later. Certainly the patches so far fix the system time issue. I don't think anything else in the clock tree using this clock is that picky about the clock being of by 0.05% (when using 20MHz SYSCLK1, which I believe all boards so far do, other than the one board on my desk with a 19.2MHz clock because we wanted to see what happened). Certainly if someone does build a baord using either 19.2 or 27MHz as supported in the documentation, then it becomes more relevant potentially. But I certainly don't consider fixing the clock tree urgent. I think we can start shipping products with the clock tree in the current state (but we couldn't with the system time being that bad, which the patches so far fix). -- Len Sorensen -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On 17:45-20141217, Tero Kristo wrote: On 12/17/2014 05:27 PM, Lennart Sorensen wrote: On Wed, Dec 17, 2014 at 09:22:25AM -0600, Nishanth Menon wrote: A clock mux might do the job? value 1, 2 , 3 will imply sysclk1 / 610 value of 0 implies fixed 32768 soemthing like sys_clk32_crystal { compatible = fixed-clock; clock-frequency = 32768; } sys_clk32_pseudo { compatible = fixed-clock; compatible = fixed-factor-clock; clocks = sys_clkin1; clock-mult = 1; clock-div = 610; } sys_32k_ck: sys_32k_ck { compatible = ti,mux-clock; clocks = sys_clk32_crystal, sys_clk32_pseudo, sys_clk32_pseudo, sys_clk32_pseudo; }; I think... The only issue is that the BOOTSTRAP register is not around the usual CM1,2 address region... OK, I like where that idea is headed. That looks promising. Yea clock mux can be used. However, we don't have support for DRA7 control module clocks in the DT yet. I have posted patches with support towards this a couple of weeks back, but they need some revising. Thus, we maybe need to apply the timer patches as is for now, and fix the clock tree a bit later. Sounds good to me, could you propose a patch in list? -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On 12/17/2014 05:53 PM, Nishanth Menon wrote: On 17:45-20141217, Tero Kristo wrote: On 12/17/2014 05:27 PM, Lennart Sorensen wrote: On Wed, Dec 17, 2014 at 09:22:25AM -0600, Nishanth Menon wrote: A clock mux might do the job? value 1, 2 , 3 will imply sysclk1 / 610 value of 0 implies fixed 32768 soemthing like sys_clk32_crystal { compatible = fixed-clock; clock-frequency = 32768; } sys_clk32_pseudo { compatible = fixed-clock; compatible = fixed-factor-clock; clocks = sys_clkin1; clock-mult = 1; clock-div = 610; } sys_32k_ck: sys_32k_ck { compatible = ti,mux-clock; clocks = sys_clk32_crystal, sys_clk32_pseudo, sys_clk32_pseudo, sys_clk32_pseudo; }; I think... The only issue is that the BOOTSTRAP register is not around the usual CM1,2 address region... OK, I like where that idea is headed. That looks promising. Yea clock mux can be used. However, we don't have support for DRA7 control module clocks in the DT yet. I have posted patches with support towards this a couple of weeks back, but they need some revising. Thus, we maybe need to apply the timer patches as is for now, and fix the clock tree a bit later. Sounds good to me, could you propose a patch in list? Yea, I can add a patch for this to the PRCM cleanup set once I get to update that. -Tero -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
Hi Lennart, On Sunday 14 December 2014 10:15 AM, Lennart Sorensen wrote: On Fri, Dec 12, 2014 at 05:08:56PM -0500, Lennart Sorensen wrote: Errata i856 for the AM572x (DRA7xx) points out that the 32.768KHz external crystal is not enabled at power up. Instead the CPU falls back to using an emulation for the 32KHz clock which is SYSCLK1/610. SYSCLK1 is usually 20MHz on boards so far (which gives an emulated frequency of 32.786KHz), but can also be 19.2 or 27MHz which result in much larger drift. Since this is used to drive the master counter at 32.768KHz * 375 / 2 = 6.144MHz, the emulated speed for 20MHz is of by 570ppm, or about 43 seconds per day, and more than the 500ppm NTP is able to tolerate. Checking the CTRL_CORE_BOOTSTRAP register can determine if the CPU is using the real 32.768KHz crystal or the emulated SYSCLK1/610, and by known that the real counter frequency can be determined and used. s/known/knowing/ and a comma after that. The real speed is then SYSCLK1 / 610 * 375 / 2 or SYSCLK1 * 75 / 244. Is this applicable for OMAP5 also? If not can you drop omap5 from $subject? In order to make the code more simpler, can you use the following logic: and add documentation accordingly. diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h index a3c0133..a80ac2d 100644 --- a/arch/arm/mach-omap2/control.h +++ b/arch/arm/mach-omap2/control.h @@ -286,6 +286,10 @@ #define OMAP5XXX_CONTROL_STATUS0x134 #define OMAP5_DEVICETYPE_MASK (0x7 6) +/* DRA7XX CONTROL CORE BOOTSTRAP */ +#define DRA7_CTRL_CORE_BOOTSTRAP 0x6c4 +#define DRA7_SPEEDSELECT_MASK (0x3 8) + /* * REVISIT: This list of registers is not comprehensive - there are more * that should be added. diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index fb0cb2b..84aadae 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -54,6 +54,7 @@ #include soc.h #include common.h +#include control.h #include powerdomain.h #include omap-secure.h @@ -545,6 +546,16 @@ static void __init realtime_counter_init(void) break; } + if (soc_is_dra7xx()) { + reg = omap_ctrl_readl(DRA7_CTRL_CORE_BOOTSTRAP); + reg = reg DRA7_SPEEDSELECT_MASK; + + if (reg) { + num = 75; + den = 244; + } + } + /* Program numerator and denumerator registers */ reg = readl_relaxed(base + INCREMENTER_NUMERATOR_OFFSET) NUMERATOR_DENUMERATOR_MASK; -- 1.9.1 Thanks and regards, Lokesh Signed-off-by: Len Sorensen lsore...@csclub.uwaterloo.ca --- arch/arm/mach-omap2/timer.c | 120 +++ 1 file changed, 87 insertions(+), 33 deletions(-) diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index fb0cb2b..f00e4b4 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -497,6 +497,7 @@ static void __init realtime_counter_init(void) static struct clk *sys_clk; unsigned long rate; unsigned int reg, num, den; +bool errata_i856_workaround = false; base = ioremap(REALTIME_COUNTER_BASE, SZ_32); if (!base) { @@ -510,39 +511,93 @@ static void __init realtime_counter_init(void) return; } +if (soc_is_dra7xx()) { +#define CTRL_CORE_BOOTSTRAP 0x4A0026C4 +#define SPEEDSELECT_MASK 0x0300 +void __iomem *corebase; +corebase = ioremap(CTRL_CORE_BOOTSTRAP, SZ_4); +if (!corebase) +pr_err(%s: ioremap failed\n, __func__); +else { +reg = readl_relaxed(corebase) SPEEDSELECT_MASK; +iounmap(corebase); +/* + * Errata i856 says the 32.768KHz crystal does + * not start at power on, so the CPU falls back in + * an emulated 32KHz clock instead. This causes + * the master counter frequency to not be 6.144MHz + * This affects at least the AM572x 1.0 and + * 1.1 revisions. + * + * Of course any board built without a populated + * 32.768KHz crystal would also need this fix + * even if the CPU is fixed later. + * + * If the two speedselect bits are not 0, then the + * 32.768KHz clock driving the course counter that s/course/coarse/ + * corrects the fine counter every time it ticks is + * actually rate/610 rather than 32.768KHz and we + * should compensate to avoid the 570ppm (At 20MHz, + * much worse at other rates) too fast system time. +
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On 17:05-20141216, Lokesh Vutla wrote: [...] @@ -545,6 +546,16 @@ static void __init realtime_counter_init(void) break; } + if (soc_is_dra7xx()) { + reg = omap_ctrl_readl(DRA7_CTRL_CORE_BOOTSTRAP); + reg = reg DRA7_SPEEDSELECT_MASK; + + if (reg) { + num = 75; + den = 244; + } + } + /* Program numerator and denumerator registers */ reg = readl_relaxed(base + INCREMENTER_NUMERATOR_OFFSET) NUMERATOR_DENUMERATOR_MASK; A) So, it does look like the 32k node is actually wrong then - Tero: any comments? should'nt this now be modeled based on CTRL_CORE_BOOTSTRAP::SPEEDSELECT considering that clock nodes do have clk mux options based on the 32k.. sys_32k_ck: sys_32k_ck { #clock-cells = 0; compatible = fixed-clock; clock-frequency = 32768; }; B) Since rate switch is no longer needed, how about something like the following: diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h index a3c0133..315d6d1 100644 --- a/arch/arm/mach-omap2/control.h +++ b/arch/arm/mach-omap2/control.h @@ -286,6 +286,11 @@ #define OMAP5XXX_CONTROL_STATUS0x134 #define OMAP5_DEVICETYPE_MASK (0x7 6) + +/* DRA7XX CONTROL CORE BOOTSTRAP */ +#define DRA7_CTRL_CORE_BOOTSTRAP 0x6c4 +#define DRA7_SPEEDSELECT_MASK (0x3 8) + /* * REVISIT: This list of registers is not comprehensive - there are more * that should be added. diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index 4f61148..783d3c3 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -54,6 +54,7 @@ #include soc.h #include common.h +#include control.h #include powerdomain.h #include omap-secure.h @@ -511,6 +512,35 @@ static void __init realtime_counter_init(void) } rate = clk_get_rate(sys_clk); + + if (soc_is_dra7xx()) { + /* +* Errata i856 says the 32.768KHz crystal does not start at +* power on, so the CPU falls back in an emulated 32KHz clock +* based on sysclk / 610 instead. This causes the master counter +* frequency to not be 6.144MHz but at sysclk / 610 * 375 / 2 +* (OR sysclk * 75 / 244) +* +* This affects at least the DRA7/AM572x 1.0, 1.1 revisions. +* Of course any board built without a populated 32.768KHz +* crystal would also need this fix even if the CPU is fixed +* later. +* +* Either case can be detected by using the two speedselect bits +* If they are not 0, then the 32.768KHz clock driving the +* course counter that corrects the fine counter every time it +* ticks is actually rate/610 rather than 32.768KHz and we +* should compensate to avoid the 570ppm (At 20MHz, much worse +* at other rates) too fast system time. +*/ + reg = omap_ctrl_readl(DRA7_CTRL_CORE_BOOTSTRAP); + if (reg DRA7_SPEEDSELECT_MASK) { + num = 75; + den = 244; + goto sysclk_based; + } + } + /* Numerator/denumerator values refer TRM Realtime Counter section */ switch (rate) { case 120: @@ -545,6 +575,7 @@ static void __init realtime_counter_init(void) break; } +sysclk_based: /* Program numerator and denumerator registers */ reg = readl_relaxed(base + INCREMENTER_NUMERATOR_OFFSET) NUMERATOR_DENUMERATOR_MASK; -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On Tue, Dec 16, 2014 at 05:05:08PM +0530, Lokesh Vutla wrote: Is this applicable for OMAP5 also? If not can you drop omap5 from $subject? DRA7xx = OMAP57xx, which to me is an omap5. Isn't it? And I haven't been able to get a manual for the omap54xx to confirm it, although it seems it does not apply to the omap54xx from what I have been able to gather indirectly. arch_timer_freq = (rate / den) * num; If I do this with the workaround I get: 2000 / 75 * 244 = 6147525 where as 2000 * 244 / 75 = 6147540 best value would be 6147541 with proper rounding. In the normal case the worst case is: 2600 * 384 = 998400 That is too big for 32 bits. Now what could be done is prescale by 4 to make the worst case still fit in 32 bits while doing the multiplication before the division, so like this: arch_timer_freq = ((rate / 4) * num / den ) * 4; That gives the same result in all cases including the errata case for the dra7xx at 20MHz and 27MHz. It is off by 3 in the 19.2MHz case though which isn't so nice. Would that be more acceptable? I think having the arch_timer_freq calculated for the errata case seperately from the normal case is cleaner looking compared to that mess and gives a better result for the 19.2MHz case. -- Len Sorensen -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On Tue, Dec 16, 2014 at 08:58:56AM -0600, Nishanth Menon wrote: On 17:05-20141216, Lokesh Vutla wrote: [...] @@ -545,6 +546,16 @@ static void __init realtime_counter_init(void) break; } + if (soc_is_dra7xx()) { + reg = omap_ctrl_readl(DRA7_CTRL_CORE_BOOTSTRAP); + reg = reg DRA7_SPEEDSELECT_MASK; + + if (reg) { + num = 75; + den = 244; + } + } + /* Program numerator and denumerator registers */ reg = readl_relaxed(base + INCREMENTER_NUMERATOR_OFFSET) NUMERATOR_DENUMERATOR_MASK; A) So, it does look like the 32k node is actually wrong then - Tero: any comments? should'nt this now be modeled based on CTRL_CORE_BOOTSTRAP::SPEEDSELECT considering that clock nodes do have clk mux options based on the 32k.. sys_32k_ck: sys_32k_ck { #clock-cells = 0; compatible = fixed-clock; clock-frequency = 32768; }; Hmm, I hadn't considered other things that might care. Certainly if you were to use a SYSCLK1 other than 20MHz, it certainly won't be 32KHz anymore. B) Since rate switch is no longer needed, how about something like the following: diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h index a3c0133..315d6d1 100644 --- a/arch/arm/mach-omap2/control.h +++ b/arch/arm/mach-omap2/control.h @@ -286,6 +286,11 @@ #define OMAP5XXX_CONTROL_STATUS0x134 #define OMAP5_DEVICETYPE_MASK (0x7 6) + +/* DRA7XX CONTROL CORE BOOTSTRAP */ +#define DRA7_CTRL_CORE_BOOTSTRAP 0x6c4 +#define DRA7_SPEEDSELECT_MASK(0x3 8) + /* * REVISIT: This list of registers is not comprehensive - there are more * that should be added. I like that as a place for those. diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index 4f61148..783d3c3 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -54,6 +54,7 @@ #include soc.h #include common.h +#include control.h #include powerdomain.h #include omap-secure.h How about this version for the rest of the file. It handles that for the errata case we would like to do rate * num / den, which given how small the num is will fit in 32bit and gives the best accuracy for the calculation, while the non errata case the existing calculation works well and fits in 32 bit for all other cases. It just has to be moved up so that the goto can skip it. I changed sysclk to sysclk1 since on the dra7xx there is in fact a sysclk1 and a sysclk2 and it is probably worth keeping it clear which one this is referring to. diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index fb0cb2b..be254bf 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -511,6 +511,36 @@ static void __init realtime_counter_init(void) } rate = clk_get_rate(sys_clk); + + if (soc_is_dra7xx()) { + /* +* Errata i856 says the 32.768KHz crystal does not start at +* power on, so the CPU falls back to an emulated 32KHz clock +* based on sysclk1 / 610 instead. This causes the master counter +* frequency to not be 6.144MHz but at sysclk1 / 610 * 375 / 2 +* (OR sysclk1 * 75 / 244) +* +* This affects at least the DRA7/AM572x 1.0, 1.1 revisions. +* Of course any board built without a populated 32.768KHz +* crystal would also need this fix even if the CPU is fixed +* later. +* +* Either case can be detected by using the two speedselect bits +* If they are not 0, then the 32.768KHz clock driving the +* coarse counter that corrects the fine counter every time it +* ticks is actually rate/610 rather than 32.768KHz and we +* should compensate to avoid the 570ppm (at 20MHz, much worse +* at other rates) too fast system time. +*/ + reg = omap_ctrl_readl(DRA7_CTRL_CORE_BOOTSTRAP); + if (reg DRA7_SPEEDSELECT_MASK) { + num = 75; + den = 244; + arch_timer_freq = (rate * num) / den; + goto sysclk1_based; + } + } + /* Numerator/denumerator values refer TRM Realtime Counter section */ switch (rate) { case 1200: @@ -544,7 +574,9 @@ static void __init realtime_counter_init(void) den = 25; break; } + arch_timer_freq = (rate / den) * num; +sysclk1_based: /* Program numerator and denumerator registers */ reg = readl_relaxed(base + INCREMENTER_NUMERATOR_OFFSET) NUMERATOR_DENUMERATOR_MASK; @@ -556,7 +588,6 @@ static void
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On 12/16/2014 10:36 AM, Lennart Sorensen wrote: [...] B) Since rate switch is no longer needed, how about something like the following: diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h index a3c0133..315d6d1 100644 --- a/arch/arm/mach-omap2/control.h +++ b/arch/arm/mach-omap2/control.h @@ -286,6 +286,11 @@ #define OMAP5XXX_CONTROL_STATUS0x134 #define OMAP5_DEVICETYPE_MASK (0x7 6) + +/* DRA7XX CONTROL CORE BOOTSTRAP */ +#define DRA7_CTRL_CORE_BOOTSTRAP0x6c4 +#define DRA7_SPEEDSELECT_MASK (0x3 8) + /* * REVISIT: This list of registers is not comprehensive - there are more * that should be added. I like that as a place for those. diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index 4f61148..783d3c3 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -54,6 +54,7 @@ #include soc.h #include common.h +#include control.h #include powerdomain.h #include omap-secure.h How about this version for the rest of the file. It handles that for the errata case we would like to do rate * num / den, which given how small the num is will fit in 32bit and gives the best accuracy for the calculation, while the non errata case the existing calculation works well and fits in 32 bit for all other cases. It just has to be moved up so that the goto can skip it. I changed sysclk to sysclk1 since on the dra7xx there is in fact a sysclk1 and a sysclk2 and it is probably worth keeping it clear which one this is referring to. diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index fb0cb2b..be254bf 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -511,6 +511,36 @@ static void __init realtime_counter_init(void) } rate = clk_get_rate(sys_clk); + + if (soc_is_dra7xx()) { + /* + * Errata i856 says the 32.768KHz crystal does not start at + * power on, so the CPU falls back to an emulated 32KHz clock + * based on sysclk1 / 610 instead. This causes the master counter + * frequency to not be 6.144MHz but at sysclk1 / 610 * 375 / 2 + * (OR sysclk1 * 75 / 244) + * + * This affects at least the DRA7/AM572x 1.0, 1.1 revisions. + * Of course any board built without a populated 32.768KHz + * crystal would also need this fix even if the CPU is fixed + * later. + * + * Either case can be detected by using the two speedselect bits + * If they are not 0, then the 32.768KHz clock driving the + * coarse counter that corrects the fine counter every time it + * ticks is actually rate/610 rather than 32.768KHz and we + * should compensate to avoid the 570ppm (at 20MHz, much worse + * at other rates) too fast system time. + */ + reg = omap_ctrl_readl(DRA7_CTRL_CORE_BOOTSTRAP); + if (reg DRA7_SPEEDSELECT_MASK) { + num = 75; + den = 244; + arch_timer_freq = (rate * num) / den; + goto sysclk1_based; + } + } + /* Numerator/denumerator values refer TRM Realtime Counter section */ switch (rate) { case 1200: @@ -544,7 +574,9 @@ static void __init realtime_counter_init(void) den = 25; break; } + arch_timer_freq = (rate / den) * num; +sysclk1_based: /* Program numerator and denumerator registers */ reg = readl_relaxed(base + INCREMENTER_NUMERATOR_OFFSET) NUMERATOR_DENUMERATOR_MASK; @@ -556,7 +588,6 @@ static void __init realtime_counter_init(void) reg |= den; writel_relaxed(reg, base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET); - arch_timer_freq = (rate / den) * num; I see why arch_timer_freq might skip the rounding error of 39, 15 and 55 Vs existing logic which is possibly at a truncation error risk (without errata for sysclk 13, 26 and 27MHz). all you'd probably need to do is cast rate, num and den to unsigned long and have a common computation logic. if you'd really want to handle truncation error, it must be a separate patch of it's own - I would not mix it with the errata fix. -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
I see why arch_timer_freq might skip the rounding error of 39, 15 and 55 Vs existing logic which is possibly at a truncation error risk (without errata for sysclk 13, 26 and 27MHz). all you'd probably need to do is cast rate, num and den to unsigned long and have a common computation logic. If that is acceptable, then sure I can do that. I liked avoiding casts in general though. if you'd really want to handle truncation error, it must be a separate patch of it's own - I would not mix it with the errata fix. Well there is no error in the existing code because the rate / den is always a clean integer division. The problem is introduced by the SYSCLK1 / 610 used by the emulated clock which is not a clean division. So for the existing logic, the calculation was perfect. It is only for the errata case that it is a problem. So I think leaving the existing calculation but moved up works well, and then having the alternate order calculation only in the errata case seemed cleaner and avoids casts and 64bit math which I thought was overall desirable. -- Len Sorensen -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On 12/16/2014 01:27 PM, Lennart Sorensen wrote: I see why arch_timer_freq might skip the rounding error of 39, 15 and 55 Vs existing logic which is possibly at a truncation error risk (without errata for sysclk 13, 26 and 27MHz). all you'd probably need to do is cast rate, num and den to unsigned long and have a common computation logic. If that is acceptable, then sure I can do that. I liked avoiding casts in general though. if you'd really want to handle truncation error, it must be a separate patch of it's own - I would not mix it with the errata fix. Well there is no error in the existing code because the rate / den is always a clean integer division. The problem is introduced by the key is there is no error in existing code for existing value :) - the same code for new values will fail. and introducing (rate * num) / den without cast will fail for old values. SYSCLK1 / 610 used by the emulated clock which is not a clean division. So for the existing logic, the calculation was perfect. It is only for the errata case that it is a problem. So I think leaving the existing calculation but moved up works well, and then having the alternate order calculation only in the errata case seemed cleaner and avoids casts and 64bit math which I thought was overall desirable. In general using DIV_ROUND_UP and family of macros(in kernel.h) is the right way of doing division in similar cases in Linux kernel. And for the same functionality, we want a common equation - if it does not fit well for all values (even if we introduce new values), then we must come to a better equation that will work for all values. What we do not do is to have two equations meant for doing the same thing. -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On 12/16/2014 10:16 AM, Lennart Sorensen wrote: On Tue, Dec 16, 2014 at 05:05:08PM +0530, Lokesh Vutla wrote: Is this applicable for OMAP5 also? If not can you drop omap5 from $subject? DRA7xx = OMAP57xx, which to me is an omap5. Isn't it? *AM*57xx is not *OMAP*57xx - there is no OMAP57xx. AM57xx and DRA7xx are in the same generation of processors and is derivative technology (not the same) as OMAP5432/OMAP5430. We have been using DRA7 as a generic terminology to indicate AM57xx/DRA7 family of processors. -- Regards, Nishanth Menon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On Tue, Dec 16, 2014 at 01:56:16PM -0600, Nishanth Menon wrote: *AM*57xx is not *OMAP*57xx - there is no OMAP57xx. Oh OK. Thanks for clearing that up. AM57xx and DRA7xx are in the same generation of processors and is derivative technology (not the same) as OMAP5432/OMAP5430. We have been using DRA7 as a generic terminology to indicate AM57xx/DRA7 family of processors. I will drop omap5 from the subject for the next version then since it seems clear that the omap5 does not have this problem. -- Len Sorensen -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.
On Fri, Dec 12, 2014 at 05:08:56PM -0500, Lennart Sorensen wrote: Errata i856 for the AM572x (DRA7xx) points out that the 32.768KHz external crystal is not enabled at power up. Instead the CPU falls back to using an emulation for the 32KHz clock which is SYSCLK1/610. SYSCLK1 is usually 20MHz on boards so far (which gives an emulated frequency of 32.786KHz), but can also be 19.2 or 27MHz which result in much larger drift. Since this is used to drive the master counter at 32.768KHz * 375 / 2 = 6.144MHz, the emulated speed for 20MHz is of by 570ppm, or about 43 seconds per day, and more than the 500ppm NTP is able to tolerate. Checking the CTRL_CORE_BOOTSTRAP register can determine if the CPU is using the real 32.768KHz crystal or the emulated SYSCLK1/610, and by known that the real counter frequency can be determined and used. s/known/knowing/ and a comma after that. The real speed is then SYSCLK1 / 610 * 375 / 2 or SYSCLK1 * 75 / 244. Signed-off-by: Len Sorensen lsore...@csclub.uwaterloo.ca --- arch/arm/mach-omap2/timer.c | 120 +++ 1 file changed, 87 insertions(+), 33 deletions(-) diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c index fb0cb2b..f00e4b4 100644 --- a/arch/arm/mach-omap2/timer.c +++ b/arch/arm/mach-omap2/timer.c @@ -497,6 +497,7 @@ static void __init realtime_counter_init(void) static struct clk *sys_clk; unsigned long rate; unsigned int reg, num, den; + bool errata_i856_workaround = false; base = ioremap(REALTIME_COUNTER_BASE, SZ_32); if (!base) { @@ -510,39 +511,93 @@ static void __init realtime_counter_init(void) return; } + if (soc_is_dra7xx()) { + #define CTRL_CORE_BOOTSTRAP 0x4A0026C4 + #define SPEEDSELECT_MASK 0x0300 + void __iomem *corebase; + corebase = ioremap(CTRL_CORE_BOOTSTRAP, SZ_4); + if (!corebase) + pr_err(%s: ioremap failed\n, __func__); + else { + reg = readl_relaxed(corebase) SPEEDSELECT_MASK; + iounmap(corebase); + /* + * Errata i856 says the 32.768KHz crystal does + * not start at power on, so the CPU falls back in + * an emulated 32KHz clock instead. This causes + * the master counter frequency to not be 6.144MHz + * This affects at least the AM572x 1.0 and + * 1.1 revisions. + * + * Of course any board built without a populated + * 32.768KHz crystal would also need this fix + * even if the CPU is fixed later. + * + * If the two speedselect bits are not 0, then the + * 32.768KHz clock driving the course counter that s/course/coarse/ + * corrects the fine counter every time it ticks is + * actually rate/610 rather than 32.768KHz and we + * should compensate to avoid the 570ppm (At 20MHz, + * much worse at other rates) too fast system time. + */ + if (reg) { + errata_i856_workaround = true; + } + } + } + rate = clk_get_rate(sys_clk); - /* Numerator/denumerator values refer TRM Realtime Counter section */ - switch (rate) { - case 1200: - num = 64; - den = 125; - break; - case 1300: - num = 768; - den = 1625; - break; - case 1920: - num = 8; - den = 25; - break; - case 2000: - num = 192; - den = 625; - break; - case 2600: - num = 384; - den = 1625; - break; - case 2700: - num = 256; - den = 1125; - break; - case 3840: - default: - /* Program it for 38.4 MHz */ - num = 4; - den = 25; - break; + if (errata_i856_workaround) { + /* + * Realtime Counter frequency is not based on a real + * 32.768KHz time source, so calculate the real resulting + * frequency instead. It is not 6.144MHz in this case. + * + * The frequency is always rate / 610 + 375 / 2 which + * is rate * 244 / 75 and will fit in 32 bit for all rates. + * + * The multiplication has to be first to keep accuracy + * with integer math as high as possible. + */ + num = 75; +