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.
The real speed is then SYSCLK1 / 610 * 375 / 2 or SYSCLK1 * 375 / 1220.
This is unfortunately not integer math so the actual arch_timer_freq
needs to be precalculated.
Also the SYSCLK1 frequencies that have never been used by an omap
evaluation board were all missing a 0.
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index 4f61148..2e81511 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -513,11 +513,11 @@ static void __init realtime_counter_init(void)
rate = clk_get_rate(sys_clk);
/* Numerator/denumerator values refer TRM Realtime Counter section */
switch (rate) {
- case 1200000:
+ case 12000000:
num = 64;
den = 125;
break;
- case 1300000:
+ case 13000000:
num = 768;
den = 1625;
break;
@@ -529,11 +529,11 @@ static void __init realtime_counter_init(void)
num = 192;
den = 625;
break;
- case 2600000:
+ case 26000000:
num = 384;
den = 1625;
break;
- case 2700000:
+ case 27000000:
num = 256;
den = 1125;
break;
@@ -557,6 +557,73 @@ static void __init realtime_counter_init(void)
writel_relaxed(reg, base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET);
arch_timer_freq = (rate / den) * num;
+
+ 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 what it
+ * was expected to be. 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
+ * 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.
+ *
+ * Precalculate the arch_timer_freq, since
+ * rate/610 isn't integer math and accuracy is
+ * desirable here.
+ */
+ if (reg) {
+ switch (rate) {
+ case 19200000:
+ num = 375;
+ den = 1220;
+ arch_timer_freq = 5901639;
+ break;
+ case 27000000:
+ num = 375;
+ den = 1220;
+ arch_timer_freq = 8299180;
+ break;
+ case 20000000:
+ default:
+ num = 375;
+ den = 1220;
+ arch_timer_freq = 6147541;
+ break;
+ }
+ reg = readl_relaxed(base + INCREMENTER_NUMERATOR_OFFSET) &
+ NUMERATOR_DENUMERATOR_MASK;
+ reg |= num;
+ writel_relaxed(reg, base + INCREMENTER_NUMERATOR_OFFSET);
+
+ reg = readl_relaxed(base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET) &
+ NUMERATOR_DENUMERATOR_MASK;
+ reg |= den;
+ writel_relaxed(reg, base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET);
+
+ printk(KERN_WARNING "DRA7xx CP15 compensated for sloppy internal 32KHz clock.\n");
+ }
+ }
+ }
+
set_cntfreq();
iounmap(base);
--
Len Sorensen
On Thu, Dec 11, 2014 at 03:41:16PM -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.
> The real speed is then SYSCLK1 / 610 * 375 / 2 or SYSCLK1 * 375 / 1220.
> This is unfortunately not integer math so the actual arch_timer_freq
> needs to be precalculated.
>
> Also the SYSCLK1 frequencies that have never been used by an omap
> evaluation board were all missing a 0.
>
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index 4f61148..2e81511 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -513,11 +513,11 @@ static void __init realtime_counter_init(void)
> rate = clk_get_rate(sys_clk);
> /* Numerator/denumerator values refer TRM Realtime Counter section */
> switch (rate) {
> - case 1200000:
> + case 12000000:
> num = 64;
> den = 125;
> break;
> - case 1300000:
> + case 13000000:
> num = 768;
> den = 1625;
> break;
> @@ -529,11 +529,11 @@ static void __init realtime_counter_init(void)
> num = 192;
> den = 625;
> break;
> - case 2600000:
> + case 26000000:
> num = 384;
> den = 1625;
> break;
> - case 2700000:
> + case 27000000:
> num = 256;
> den = 1125;
> break;
> @@ -557,6 +557,73 @@ static void __init realtime_counter_init(void)
> writel_relaxed(reg, base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET);
>
> arch_timer_freq = (rate / den) * num;
> +
> + 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 what it
> + * was expected to be. 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
> + * 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.
> + *
> + * Precalculate the arch_timer_freq, since
> + * rate/610 isn't integer math and accuracy is
> + * desirable here.
> + */
> + if (reg) {
> + switch (rate) {
> + case 19200000:
> + num = 375;
> + den = 1220;
> + arch_timer_freq = 5901639;
> + break;
> + case 27000000:
> + num = 375;
> + den = 1220;
> + arch_timer_freq = 8299180;
> + break;
> + case 20000000:
> + default:
> + num = 375;
> + den = 1220;
> + arch_timer_freq = 6147541;
> + break;
> + }
> + reg = readl_relaxed(base + INCREMENTER_NUMERATOR_OFFSET) &
> + NUMERATOR_DENUMERATOR_MASK;
> + reg |= num;
> + writel_relaxed(reg, base + INCREMENTER_NUMERATOR_OFFSET);
> +
> + reg = readl_relaxed(base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET) &
> + NUMERATOR_DENUMERATOR_MASK;
> + reg |= den;
> + writel_relaxed(reg, base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET);
> +
> + printk(KERN_WARNING "DRA7xx CP15 compensated for sloppy internal 32KHz clock.\n");
> + }
> + }
> + }
> +
> set_cntfreq();
>
> iounmap(base);
I am perfectly willing to split this into two patches it prefered (one
for the counter frequency fix and one for the rate typos).
Other cleanups needed would be great too, since I am still not too
pleased with how this looks.
With this fix however many boards have run ntp with drift less than 3ppm,
so it works very well.
--
Len Sorensen
-sricharan, as the email ID is defunct.
On 12/11/2014 02:43 PM, Lennart Sorensen wrote:
> On Thu, Dec 11, 2014 at 03:41:16PM -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.
Why not just change the clock parent to something that is more
accurate like the 32k clk?
>>
>> 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
Without digging into docs, This is just the boot configuration, right?
are we not able to reconfigure?
>> by known that the real counter frequency can be determined and used.
>> The real speed is then SYSCLK1 / 610 * 375 / 2 or SYSCLK1 * 375 / 1220.
>> This is unfortunately not integer math so the actual arch_timer_freq
>> needs to be precalculated.
>>
>> Also the SYSCLK1 frequencies that have never been used by an omap
>> evaluation board were all missing a 0.
>>
Please sign-off on you patch. use git format-patch -M -s to generate
patches. and when posting a series, use --cover-letter. Will also be
good to provide tests that show that this is indeed an issue on OMAP5
and DRA7 (considering the $subject here).
>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
>> index 4f61148..2e81511 100644
>> --- a/arch/arm/mach-omap2/timer.c
>> +++ b/arch/arm/mach-omap2/timer.c
>> @@ -513,11 +513,11 @@ static void __init realtime_counter_init(void)
>> rate = clk_get_rate(sys_clk);
>> /* Numerator/denumerator values refer TRM Realtime Counter section */
>> switch (rate) {
>> - case 1200000:
>> + case 12000000:
>> num = 64;
>> den = 125;
>> break;
>> - case 1300000:
>> + case 13000000:
>> num = 768;
>> den = 1625;
>> break;
>> @@ -529,11 +529,11 @@ static void __init realtime_counter_init(void)
>> num = 192;
>> den = 625;
>> break;
>> - case 2600000:
>> + case 26000000:
>> num = 384;
>> den = 1625;
>> break;
>> - case 2700000:
>> + case 27000000:
>> num = 256;
>> den = 1125;
>> break;
These should probably fall in as a separate patch.
>> @@ -557,6 +557,73 @@ static void __init realtime_counter_init(void)
>> writel_relaxed(reg, base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET);
>>
>> arch_timer_freq = (rate / den) * num;
>> +
>> + if (soc_is_dra7xx()) {
>> + #define CTRL_CORE_BOOTSTRAP 0x4A0026C4
>> + #define SPEEDSELECT_MASK 0x00000300
we dont usually define it like this.
>> + void __iomem *corebase;
>> + corebase = ioremap(CTRL_CORE_BOOTSTRAP, SZ_4);
>> + if (!corebase)
>> + pr_err("%s: ioremap failed\n", __func__);
>> + else {
also run ./scripts/checkpatch.pl --strict on your patch prior to
posting. try to ensure there are 0 warnings or checks.
>> + 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 what it
>> + * was expected to be. 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
>> + * 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.
>> + *
>> + * Precalculate the arch_timer_freq, since
>> + * rate/610 isn't integer math and accuracy is
>> + * desirable here.
>> + */
>> + if (reg) {
>> + switch (rate) {
>> + case 19200000:
>> + num = 375;
>> + den = 1220;
>> + arch_timer_freq = 5901639;
>> + break;
>> + case 27000000:
>> + num = 375;
>> + den = 1220;
>> + arch_timer_freq = 8299180;
>> + break;
>> + case 20000000:
>> + default:
>> + num = 375;
>> + den = 1220;
>> + arch_timer_freq = 6147541;
>> + break;
>> + }
>> + reg = readl_relaxed(base + INCREMENTER_NUMERATOR_OFFSET) &
>> + NUMERATOR_DENUMERATOR_MASK;
>> + reg |= num;
>> + writel_relaxed(reg, base + INCREMENTER_NUMERATOR_OFFSET);
>> +
>> + reg = readl_relaxed(base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET) &
>> + NUMERATOR_DENUMERATOR_MASK;
>> + reg |= den;
>> + writel_relaxed(reg, base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET);
>> +
>> + printk(KERN_WARNING "DRA7xx CP15 compensated for sloppy internal 32KHz clock.\n");
we would want to reuse the configuration code previously done, not
repeat the logic.
in general we want the flow to be common - so the point in logic where
num, den is decided is also the place you want this logic to fit in..
>> + }
>> + }
>> + }
>> +
>> set_cntfreq();
>>
>> iounmap(base);
>
> I am perfectly willing to split this into two patches it prefered (one
> for the counter frequency fix and one for the rate typos).
>
> Other cleanups needed would be great too, since I am still not too
> pleased with how this looks.
>
> With this fix however many boards have run ntp with drift less than 3ppm,
> so it works very well.
>
--
Regards,
Nishanth Menon
On Fri, Dec 12, 2014 at 11:37:41AM -0600, Nishanth Menon wrote:
> -sricharan, as the email ID is defunct.
So I noticed.
> On 12/11/2014 02:43 PM, Lennart Sorensen wrote:
> > On Thu, Dec 11, 2014 at 03:41:16PM -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.
>
> Why not just change the clock parent to something that is more
> accurate like the 32k clk?
According to the documentation, it can not be changed. The course counter
runs from the external 32 crystal (which isn't working as per the errata)
and the only other choice is the emulated one. I suspect it is limited
because it has to run in low power mode, so it tries to stay in a small
part of the CPU. The same errata also means the internal RTC isn't
running, although why anyone would want a RTC that wasn't battery backed
I can't imagine, and I suspect every design wiill have a seperate external
RTC instead.
> Without digging into docs, This is just the boot configuration, right?
> are we not able to reconfigure?
Not according to anything I have seen in the documentation, and the
errata doesn't offer any options, and no one from TI has come back with
any options for changing it. They have agreed that my patch will make
it run correctly however.
> Please sign-off on you patch. use git format-patch -M -s to generate
> patches. and when posting a series, use --cover-letter. Will also be
> good to provide tests that show that this is indeed an issue on OMAP5
> and DRA7 (considering the $subject here).
Oh yeah I missed that line.
As for a test, running ntp will pretty quickly simply declare that the
system clock is more than 500ppm out and give up. Just using 'date'
you will see that it is off by over 10 seconds within 8 hours.
> >> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> >> index 4f61148..2e81511 100644
> >> --- a/arch/arm/mach-omap2/timer.c
> >> +++ b/arch/arm/mach-omap2/timer.c
> >> @@ -513,11 +513,11 @@ static void __init realtime_counter_init(void)
> >> rate = clk_get_rate(sys_clk);
> >> /* Numerator/denumerator values refer TRM Realtime Counter section */
> >> switch (rate) {
> >> - case 1200000:
> >> + case 12000000:
> >> num = 64;
> >> den = 125;
> >> break;
> >> - case 1300000:
> >> + case 13000000:
> >> num = 768;
> >> den = 1625;
> >> break;
> >> @@ -529,11 +529,11 @@ static void __init realtime_counter_init(void)
> >> num = 192;
> >> den = 625;
> >> break;
> >> - case 2600000:
> >> + case 26000000:
> >> num = 384;
> >> den = 1625;
> >> break;
> >> - case 2700000:
> >> + case 27000000:
> >> num = 256;
> >> den = 1125;
> >> break;
>
> These should probably fall in as a separate patch.
OK, I will do that.
> >> @@ -557,6 +557,73 @@ static void __init realtime_counter_init(void)
> >> writel_relaxed(reg, base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET);
> >>
> >> arch_timer_freq = (rate / den) * num;
> >> +
> >> + if (soc_is_dra7xx()) {
> >> + #define CTRL_CORE_BOOTSTRAP 0x4A0026C4
> >> + #define SPEEDSELECT_MASK 0x00000300
>
> we dont usually define it like this.
>
> >> + void __iomem *corebase;
> >> + corebase = ioremap(CTRL_CORE_BOOTSTRAP, SZ_4);
> >> + if (!corebase)
> >> + pr_err("%s: ioremap failed\n", __func__);
> >> + else {
>
> also run ./scripts/checkpatch.pl --strict on your patch prior to
> posting. try to ensure there are 0 warnings or checks.
>
> >> + 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 what it
> >> + * was expected to be. 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
> >> + * 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.
> >> + *
> >> + * Precalculate the arch_timer_freq, since
> >> + * rate/610 isn't integer math and accuracy is
> >> + * desirable here.
> >> + */
> >> + if (reg) {
> >> + switch (rate) {
> >> + case 19200000:
> >> + num = 375;
> >> + den = 1220;
> >> + arch_timer_freq = 5901639;
> >> + break;
> >> + case 27000000:
> >> + num = 375;
> >> + den = 1220;
> >> + arch_timer_freq = 8299180;
> >> + break;
> >> + case 20000000:
>
> >> + default:
> >> + num = 375;
> >> + den = 1220;
> >> + arch_timer_freq = 6147541;
> >> + break;
> >> + }
> >> + reg = readl_relaxed(base + INCREMENTER_NUMERATOR_OFFSET) &
> >> + NUMERATOR_DENUMERATOR_MASK;
> >> + reg |= num;
> >> + writel_relaxed(reg, base + INCREMENTER_NUMERATOR_OFFSET);
> >> +
> >> + reg = readl_relaxed(base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET) &
> >> + NUMERATOR_DENUMERATOR_MASK;
> >> + reg |= den;
> >> + writel_relaxed(reg, base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET);
> >> +
> >> + printk(KERN_WARNING "DRA7xx CP15 compensated for sloppy internal 32KHz clock.\n");
>
> we would want to reuse the configuration code previously done, not
> repeat the logic.
So which part do you suggest reusing?
> in general we want the flow to be common - so the point in logic where
> num, den is decided is also the place you want this logic to fit in..
I was trying to avoid making the code mroe complicated for the other
CPUs using this code, but I suppose since it runs only at boot once,
that probably isn't really a great concern.
I will try changing the flow and send an updated version.
--
Len Sorensen
On 14:23-20141212, Lennart Sorensen wrote:
> I was trying to avoid making the code mroe complicated for the other
> CPUs using this code, but I suppose since it runs only at boot once,
> that probably isn't really a great concern.
we try and avoid soc_is or cpu_is as much as possible and depend usually
on compatible to mark the change..
>
> I will try changing the flow and send an updated version.
k, thanks. lets go through your next rev to see if we can improve on the
same. btw, nice catch :) - thanks for proposing a fix.
--
Regards,
Nishanth Menon
On Fri, Dec 12, 2014 at 01:40:01PM -0600, Nishanth Menon wrote:
> we try and avoid soc_is or cpu_is as much as possible and depend usually
> on compatible to mark the change..
Well you can't use the dtb really, since it depends on the chip revision
and how it started at power on. After all if you purposely don't connect
a 32KHz crystal to save board space and cost, then you will also need this
"fix" to make time work properly, which is in fact what we will now be doing.
> k, thanks. lets go through your next rev to see if we can improve on the
> same. btw, nice catch :) - thanks for proposing a fix.
Well we have been trying to get the system time to run well and have ntp
happy for a few months now and it took the die designers quite a while
to track down the power on problem with the 32KHz crystal. Unfortunately
the emulation using 20000000/610 is close enough that you don't notice
it unless you are looking at time keeping issues. Of course if you try
any of the other SYSCLK1 options for the CPU then time goes totally wrong.
With a 19.2MHz crystal it was running almost 5% too slow which was
very noticeable.
--
Len Sorensen