Re: [PATCH 2/2] ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata i856.

[Lokesh Vutla <lokeshvutla@ti.com>]

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_STATUS                0x134
 #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 <lsorense@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 0x00000300
>> +		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 12000000:
>> -		num = 64;
>> -		den = 125;
>> -		break;
>> -	case 13000000:
>> -		num = 768;
>> -		den = 1625;
>> -		break;
>> -	case 19200000:
>> -		num = 8;
>> -		den = 25;
>> -		break;
>> -	case 20000000:
>> -		num = 192;
>> -		den = 625;
>> -		break;
>> -	case 26000000:
>> -		num = 384;
>> -		den = 1625;
>> -		break;
>> -	case 27000000:
>> -		num = 256;
>> -		den = 1125;
>> -		break;
>> -	case 38400000:
>> -	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;
>> +		den = 244;
>> +		arch_timer_freq = (rate * num) / den;
>> +	} else {
>> +		/* Numerator/denumerator values refer TRM Realtime Counter section */
>> +		switch (rate) {
>> +		case 12000000:
>> +			num = 64;
>> +			den = 125;
>> +			break;
>> +		case 13000000:
>> +			num = 768;
>> +			den = 1625;
>> +			break;
>> +		case 19200000:
>> +			num = 8;
>> +			den = 25;
>> +			break;
>> +		case 20000000:
>> +			num = 192;
>> +			den = 625;
>> +			break;
>> +		case 26000000:
>> +			num = 384;
>> +			den = 1625;
>> +			break;
>> +		case 27000000:
>> +			num = 256;
>> +			den = 1125;
>> +			break;
>> +		case 38400000:
>> +		default:
>> +			/* Program it for 38.4 MHz */
>> +			num = 4;
>> +			den = 25;
>> +			break;
>> +		}
>> +		/* This divides cleanly and fits in 32 bits */
>> +		arch_timer_freq = (rate / den) * num;
>>  	}
>>  
>>  	/* Program numerator and denumerator registers */
>> @@ -556,7 +611,6 @@ static void __init realtime_counter_init(void)
>>  	reg |= den;
>>  	writel_relaxed(reg, base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET);
>>  
>> -	arch_timer_freq = (rate / den) * num;
>>  	set_cntfreq();
>>  
>>  	iounmap(base);
>> -- 
>> 1.7.10.4
> 
> Stupid typos.  Will make sure to include in next revision, and maybe
> I can manage to reword the description a bit to make it flow better or
> be clearer.
>