While trying to deal with errata i856 on the dra7xx I encountered some
obvious typos in the frequencies checked in timer.c, so those are being
fixed in case anyone ever tries to use one of them.
Also implement a fix for errata i856. Without the fix the time on the
system will get ahead by 43 seconds per day if SYSCLK1 is 20MHz, which it
is on the EVM boards as well as the other boards I am currently aware of.
ntp declares that the system time drifts by over 500ppm (the maximum
ntp will tolerate).
To fix it, check the status register to determine if the 32.768KHz clock
source is real (driven by an external crystal) or emulated (SYSCLK1 /
610), and if it is emulated use the appropriate numerator and divisor
to make the fine master counter match the coarse master counter (which
is driven directly from the 32KHz clock (real or emulated) with a fixed
multiplier of 375 / 2). Making the fine counter run at a frequency
different from the coarse counter is not an option, as the value in the
fine counter is updated to match the coarse counter if they ever get
out of sync.
With 19.2MHz installed on the board, the clock runs almost 5% slow.
With the change in place, ntp runs with a drift of around 3ppm on all
boards I have tried which is well within the spec of the crystals used
for SYSCLK1.
Even if the errata is fixed in future revisions of the chip, there is
still the option of someone purposely not connecting a 32.768KHz crystal
to save cost or board space, and relying on the emulated clock instead,
in which case this correction will still be necessary.
Len Sorensen (2):
ARM: omap5/dra7xx: Fix frequency typos.
ARM: omap5/dra7xx: Fix counter frequency drift for AM572x errata
i856.
arch/arm/mach-omap2/timer.c | 120 +++++++++++++++++++++++++++++++------------
1 file changed, 87 insertions(+), 33 deletions(-)
--
1.7.10.4
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 * 75 / 244.
Signed-off-by: Len Sorensen <[email protected]>
---
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
+ * 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
The switch statement of the possible list of SYSCLK1 frequencies is
missing a 0 in 4 out of the 7 frequencies.
Signed-off-by: Len Sorensen <[email protected]>
---
arch/arm/mach-omap2/timer.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index 4f61148..fb0cb2b 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;
--
1.7.10.4
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 <[email protected]>
> ---
> 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.
--
Len Sorensen
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 <[email protected]>
>> ---
>> 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.
>
Hi Len,
On Saturday 13 December 2014 03:38 AM, Lennart Sorensen wrote:
> The switch statement of the possible list of SYSCLK1 frequencies is
> missing a 0 in 4 out of the 7 frequencies.
Indeed a good catch !!
Reviewed-by: Lokesh Vutla <[email protected]>
Thanks and regards,
Lokesh
>
> Signed-off-by: Len Sorensen <[email protected]>
> ---
> arch/arm/mach-omap2/timer.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index 4f61148..fb0cb2b 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;
>
On 12/12/2014 04:08 PM, Lennart Sorensen wrote:
> The switch statement of the possible list of SYSCLK1 frequencies is
> missing a 0 in 4 out of the 7 frequencies.
>
> Signed-off-by: Len Sorensen <[email protected]>
> ---
> arch/arm/mach-omap2/timer.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index 4f61148..fb0cb2b 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;
>
Also please add:
Fixes: fa6d79d27614 ("ARM: OMAP: Add initialisation for the real-time
counter")
With that,
Acked-by: Nishanth Menon <[email protected]>
--
Regards,
Nishanth Menon
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_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 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 1200000:
@@ -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
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:
20000000 / 75 * 244 = 6147525
where as
20000000 * 244 / 75 = 6147540
best value would be 6147541 with proper rounding.
In the normal case the worst case is:
26000000 * 384 = 9984000000
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
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_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.
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 12000000:
@@ -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;
set_cntfreq();
iounmap(base);
--
Len Sorensen
On Tue, Dec 16, 2014 at 08:06:34AM -0600, Nishanth Menon wrote:
> On 12/12/2014 04:08 PM, Lennart Sorensen wrote:
> > The switch statement of the possible list of SYSCLK1 frequencies is
> > missing a 0 in 4 out of the 7 frequencies.
> >
> > Signed-off-by: Len Sorensen <[email protected]>
> > ---
> > arch/arm/mach-omap2/timer.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> > index 4f61148..fb0cb2b 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;
> >
>
>
> Also please add:
> Fixes: fa6d79d27614 ("ARM: OMAP: Add initialisation for the real-time
> counter")
>
> With that,
>
> Acked-by: Nishanth Menon <[email protected]>
Should I send a new version with that added, or will someone else handle
doing that bit?
--
Len Sorensen
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_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.
>
> 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 12000000:
> @@ -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
> 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
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
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
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
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_12000000_ck: virt_12000000_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_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 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 1200000:
> @@ -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;
>
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_12000000_ck: virt_12000000_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
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_12000000_ck: virt_12000000_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
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
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
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
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
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