RE: [PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767

2012-09-13 Thread Hiremath, Vaibhav
On Thu, Sep 06, 2012 at 19:36:08, Hunter, Jon wrote:
 
 On 09/06/2012 12:07 AM, Vaibhav Hiremath wrote:
  
  
  On 9/6/2012 12:34 AM, Jon Hunter wrote:
  Errata Titles:
  i103: Delay needed to read some GP timer, WD timer and sync timer registers
after wakeup (OMAP3/4)
  i767: Delay needed to read some GP timer registers after wakeup (OMAP5)
 
  Description (i103/i767):
  If a General Purpose Timer (GPTimer) is in posted mode (TSICR 
  [2].POSTED=1),
  due to internal resynchronizations, values read in TCRR, TCAR1 and TCAR2
  registers right after the timer interface clock (L4) goes from stopped to
  active may not return the expected values. The most common event leading to
  this situation occurs upon wake up from idle.
 
  GPTimer non-posted synchronization mode is not impacted by this limitation.
 
  Workarounds:
  1). Disable posted mode
  2). Use static dependency between timer clock domain and MPUSS clock domain
  3). Use no-idle mode when the timer is active
 
  Workarounds #2 and #3 are not pratical from a power standpoint and so
  workaround #1 has been implemented. Disabling posted mode adds some CPU 
  overhead
  for configuring the timers as the CPU has to wait for the write to 
  complete.
  However, disabling posted mode guarantees correct operation.
 
  Please note that it is safe to use posted mode for timers if the counter 
  (TCRR)
  and capture (TCARx) registers will never be read. An example of this is the
  clock-event system timer. This is used by the kernel to schedule events 
  however,
  the timers counter is never read and capture registers are not used. Given 
  that
  the kernel configures this timer often yet never reads the counter 
  register it
  is safe to enable posted mode in this case. Hence, for the timer used for 
  kernel
  clock-events, posted mode is enabled by overriding the errata for devices 
  that
  are impacted by this defect.
 
  Although both dmtimers and watchdogs are impacted by this defect this 
  patch only
  implements the workaround for the dmtimer. Currently the watchdog driver 
  does
  not read the counter register and so no workaround is necessary.
 
  Confirmed with Vaibhav Hiremath that this bug also impacts AM33xx devices.
 
  
  Thanks for pinging me on this and getting it confirmed.
  
  Signed-off-by: Jon Hunter jon-hun...@ti.com
  ---
   arch/arm/mach-omap2/timer.c   |9 +++
   arch/arm/plat-omap/dmtimer.c  |2 ++
   arch/arm/plat-omap/include/plat/dmtimer.h |   39 
  +
   3 files changed, 50 insertions(+)
 
  diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
  index 2ff6d41..5471706 100644
  --- a/arch/arm/mach-omap2/timer.c
  +++ b/arch/arm/mach-omap2/timer.c
  @@ -208,6 +208,13 @@ static void __init omap2_gp_clockevent_init(int 
  gptimer_id,
   {
 int res;
   
  +  /*
  +   * For clock-event timers we never read the timer counter and
  +   * so we are not impacted by errata i103 and i767. Therefore,
  +   * we can safely ignore this errata for clock-event timers.
  +   */
  +  __omap_dm_timer_populate_errata(clkev, OMAP_TIMER_ERRATA_I103_I767);
  +
  
  Couple of points,
  
  1. It is confusing to me, as you are passing the errata flag so i expect
  api should set it. Why can't we do reverse way, you pass 0 here, since
  you don't want to set and pass this flag every other places where you
  want to enable this errata.
 
 Per the design of the __omap_dm_timer_populate_errata function, the 2nd
 argument is called override to allow us to override an errata. I am not
 a huge fan of this, but I wanted to be explicit in the code that we are
 intentionally allowing posted mode for the clock-events timer.
 
 I did not wish to pass the flags we want to set because if there more
 flags added in the future then we will have to keep changing the calls
 to the populate_errata function to add these.
 

Isn't that would self-explain himself which flag is going to set without 
looking at the implementation?
I do not have any reservations here, it just doesn't seem easily readable to 
me.
You can make a call here.


  2. Why can't we enable for all timers? Even though clock-event is anyway
  not reading it, but still is is applicable to it, right?
 
 Yes it is still applicable but we never read it so it is ok to override.
 If you see Richard W's original patch for enabling posted mode it is to
 reduce overhead of programming timers, specifically the clock-events
 timer which is program very often.
 
 For other timers, we do not know how they will be used and so by default
 we disable posted mode as this is safe.
 
  3. Why can't we just simply Add this flag to hwmod_data file and read it
  back in omap_timer_init() and omap_dm_timer_init_one(). Wouldn't that be
  a good approach to handle it?
 
 It could be done in this case, but typically I have not seen errata
 flags added to HWMOD. 

We are already using it, look at I2C, MMC. And in some cases, we have 

RE: [PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767

2012-09-13 Thread Hiremath, Vaibhav
On Thu, Sep 06, 2012 at 20:50:27, Hunter, Jon wrote:
 
 On 09/06/2012 09:42 AM, Jon Hunter wrote:
  
  On 09/06/2012 09:06 AM, Jon Hunter wrote:
 
  On 09/06/2012 12:07 AM, Vaibhav Hiremath wrote:
 
 
  On 9/6/2012 12:34 AM, Jon Hunter wrote:
  Errata Titles:
  i103: Delay needed to read some GP timer, WD timer and sync timer 
  registers
after wakeup (OMAP3/4)
  i767: Delay needed to read some GP timer registers after wakeup (OMAP5)
 
  Description (i103/i767):
  If a General Purpose Timer (GPTimer) is in posted mode (TSICR 
  [2].POSTED=1),
  due to internal resynchronizations, values read in TCRR, TCAR1 and TCAR2
  registers right after the timer interface clock (L4) goes from stopped to
  active may not return the expected values. The most common event leading 
  to
  this situation occurs upon wake up from idle.
 
  GPTimer non-posted synchronization mode is not impacted by this 
  limitation.
 
  Workarounds:
  1). Disable posted mode
  2). Use static dependency between timer clock domain and MPUSS clock 
  domain
  3). Use no-idle mode when the timer is active
 
  Workarounds #2 and #3 are not pratical from a power standpoint and so
  workaround #1 has been implemented. Disabling posted mode adds some CPU 
  overhead
  for configuring the timers as the CPU has to wait for the write to 
  complete.
  However, disabling posted mode guarantees correct operation.
 
  Please note that it is safe to use posted mode for timers if the counter 
  (TCRR)
  and capture (TCARx) registers will never be read. An example of this is 
  the
  clock-event system timer. This is used by the kernel to schedule events 
  however,
  the timers counter is never read and capture registers are not used. 
  Given that
  the kernel configures this timer often yet never reads the counter 
  register it
  is safe to enable posted mode in this case. Hence, for the timer used 
  for kernel
  clock-events, posted mode is enabled by overriding the errata for 
  devices that
  are impacted by this defect.
 
  Although both dmtimers and watchdogs are impacted by this defect this 
  patch only
  implements the workaround for the dmtimer. Currently the watchdog driver 
  does
  not read the counter register and so no workaround is necessary.
 
  Confirmed with Vaibhav Hiremath that this bug also impacts AM33xx 
  devices.
 
 
  Thanks for pinging me on this and getting it confirmed.
 
  Signed-off-by: Jon Hunter jon-hun...@ti.com
  ---
   arch/arm/mach-omap2/timer.c   |9 +++
   arch/arm/plat-omap/dmtimer.c  |2 ++
   arch/arm/plat-omap/include/plat/dmtimer.h |   39 
  +
   3 files changed, 50 insertions(+)
 
  diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
  index 2ff6d41..5471706 100644
  --- a/arch/arm/mach-omap2/timer.c
  +++ b/arch/arm/mach-omap2/timer.c
  @@ -208,6 +208,13 @@ static void __init omap2_gp_clockevent_init(int 
  gptimer_id,
   {
   int res;
   
  +/*
  + * For clock-event timers we never read the timer counter and
  + * so we are not impacted by errata i103 and i767. Therefore,
  + * we can safely ignore this errata for clock-event timers.
  + */
  +__omap_dm_timer_populate_errata(clkev, 
  OMAP_TIMER_ERRATA_I103_I767);
  +
 
  Couple of points,
 
  1. It is confusing to me, as you are passing the errata flag so i expect
  api should set it. Why can't we do reverse way, you pass 0 here, since
  you don't want to set and pass this flag every other places where you
  want to enable this errata.
 
  Per the design of the __omap_dm_timer_populate_errata function, the 2nd
  argument is called override to allow us to override an errata. I am not
  a huge fan of this, but I wanted to be explicit in the code that we are
  intentionally allowing posted mode for the clock-events timer.
 
  I did not wish to pass the flags we want to set because if there more
  flags added in the future then we will have to keep changing the calls
  to the populate_errata function to add these.
  
  By the way, your proposal could work nicely if we could pass errata
  flags from HWMOD. However, I am not sure if Paul or Benoit would go for
  this as they want HWMOD data to be auto-generated as much as possible
  and so I am not sure how that would work for errata which are not
  expected by design ;-)
 
 Another alternative would be to drop the override argument altogether
 and just do something like the following for the clock-events timer ...
 

I would still vote for adding this info to hwmod data, I think that is the 
right place for all such hw related information.

Thanks,
Viabhav
 diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
 index 43da595..f59e797 100644
 --- a/arch/arm/mach-omap2/timer.c
 +++ b/arch/arm/mach-omap2/timer.c
 @@ -206,12 +206,14 @@ static void __init omap2_gp_clockevent_init(int 
 gptimer_id,
  {
 int res;
  
 +   

Re: [PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767

2012-09-06 Thread Jon Hunter

On 09/06/2012 09:06 AM, Jon Hunter wrote:
 
 On 09/06/2012 12:07 AM, Vaibhav Hiremath wrote:


 On 9/6/2012 12:34 AM, Jon Hunter wrote:
 Errata Titles:
 i103: Delay needed to read some GP timer, WD timer and sync timer registers
   after wakeup (OMAP3/4)
 i767: Delay needed to read some GP timer registers after wakeup (OMAP5)

 Description (i103/i767):
 If a General Purpose Timer (GPTimer) is in posted mode (TSICR [2].POSTED=1),
 due to internal resynchronizations, values read in TCRR, TCAR1 and TCAR2
 registers right after the timer interface clock (L4) goes from stopped to
 active may not return the expected values. The most common event leading to
 this situation occurs upon wake up from idle.

 GPTimer non-posted synchronization mode is not impacted by this limitation.

 Workarounds:
 1). Disable posted mode
 2). Use static dependency between timer clock domain and MPUSS clock domain
 3). Use no-idle mode when the timer is active

 Workarounds #2 and #3 are not pratical from a power standpoint and so
 workaround #1 has been implemented. Disabling posted mode adds some CPU 
 overhead
 for configuring the timers as the CPU has to wait for the write to complete.
 However, disabling posted mode guarantees correct operation.

 Please note that it is safe to use posted mode for timers if the counter 
 (TCRR)
 and capture (TCARx) registers will never be read. An example of this is the
 clock-event system timer. This is used by the kernel to schedule events 
 however,
 the timers counter is never read and capture registers are not used. Given 
 that
 the kernel configures this timer often yet never reads the counter register 
 it
 is safe to enable posted mode in this case. Hence, for the timer used for 
 kernel
 clock-events, posted mode is enabled by overriding the errata for devices 
 that
 are impacted by this defect.

 Although both dmtimers and watchdogs are impacted by this defect this patch 
 only
 implements the workaround for the dmtimer. Currently the watchdog driver 
 does
 not read the counter register and so no workaround is necessary.

 Confirmed with Vaibhav Hiremath that this bug also impacts AM33xx devices.


 Thanks for pinging me on this and getting it confirmed.

 Signed-off-by: Jon Hunter jon-hun...@ti.com
 ---
  arch/arm/mach-omap2/timer.c   |9 +++
  arch/arm/plat-omap/dmtimer.c  |2 ++
  arch/arm/plat-omap/include/plat/dmtimer.h |   39 
 +
  3 files changed, 50 insertions(+)

 diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
 index 2ff6d41..5471706 100644
 --- a/arch/arm/mach-omap2/timer.c
 +++ b/arch/arm/mach-omap2/timer.c
 @@ -208,6 +208,13 @@ static void __init omap2_gp_clockevent_init(int 
 gptimer_id,
  {
 int res;
  
 +   /*
 +* For clock-event timers we never read the timer counter and
 +* so we are not impacted by errata i103 and i767. Therefore,
 +* we can safely ignore this errata for clock-event timers.
 +*/
 +   __omap_dm_timer_populate_errata(clkev, OMAP_TIMER_ERRATA_I103_I767);
 +

 Couple of points,

 1. It is confusing to me, as you are passing the errata flag so i expect
 api should set it. Why can't we do reverse way, you pass 0 here, since
 you don't want to set and pass this flag every other places where you
 want to enable this errata.
 
 Per the design of the __omap_dm_timer_populate_errata function, the 2nd
 argument is called override to allow us to override an errata. I am not
 a huge fan of this, but I wanted to be explicit in the code that we are
 intentionally allowing posted mode for the clock-events timer.
 
 I did not wish to pass the flags we want to set because if there more
 flags added in the future then we will have to keep changing the calls
 to the populate_errata function to add these.

By the way, your proposal could work nicely if we could pass errata
flags from HWMOD. However, I am not sure if Paul or Benoit would go for
this as they want HWMOD data to be auto-generated as much as possible
and so I am not sure how that would work for errata which are not
expected by design ;-)

Cheers
Jon
--
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 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767

2012-09-06 Thread Jon Hunter

On 09/06/2012 09:42 AM, Jon Hunter wrote:
 
 On 09/06/2012 09:06 AM, Jon Hunter wrote:

 On 09/06/2012 12:07 AM, Vaibhav Hiremath wrote:


 On 9/6/2012 12:34 AM, Jon Hunter wrote:
 Errata Titles:
 i103: Delay needed to read some GP timer, WD timer and sync timer registers
   after wakeup (OMAP3/4)
 i767: Delay needed to read some GP timer registers after wakeup (OMAP5)

 Description (i103/i767):
 If a General Purpose Timer (GPTimer) is in posted mode (TSICR 
 [2].POSTED=1),
 due to internal resynchronizations, values read in TCRR, TCAR1 and TCAR2
 registers right after the timer interface clock (L4) goes from stopped to
 active may not return the expected values. The most common event leading to
 this situation occurs upon wake up from idle.

 GPTimer non-posted synchronization mode is not impacted by this limitation.

 Workarounds:
 1). Disable posted mode
 2). Use static dependency between timer clock domain and MPUSS clock domain
 3). Use no-idle mode when the timer is active

 Workarounds #2 and #3 are not pratical from a power standpoint and so
 workaround #1 has been implemented. Disabling posted mode adds some CPU 
 overhead
 for configuring the timers as the CPU has to wait for the write to 
 complete.
 However, disabling posted mode guarantees correct operation.

 Please note that it is safe to use posted mode for timers if the counter 
 (TCRR)
 and capture (TCARx) registers will never be read. An example of this is the
 clock-event system timer. This is used by the kernel to schedule events 
 however,
 the timers counter is never read and capture registers are not used. Given 
 that
 the kernel configures this timer often yet never reads the counter 
 register it
 is safe to enable posted mode in this case. Hence, for the timer used for 
 kernel
 clock-events, posted mode is enabled by overriding the errata for devices 
 that
 are impacted by this defect.

 Although both dmtimers and watchdogs are impacted by this defect this 
 patch only
 implements the workaround for the dmtimer. Currently the watchdog driver 
 does
 not read the counter register and so no workaround is necessary.

 Confirmed with Vaibhav Hiremath that this bug also impacts AM33xx devices.


 Thanks for pinging me on this and getting it confirmed.

 Signed-off-by: Jon Hunter jon-hun...@ti.com
 ---
  arch/arm/mach-omap2/timer.c   |9 +++
  arch/arm/plat-omap/dmtimer.c  |2 ++
  arch/arm/plat-omap/include/plat/dmtimer.h |   39 
 +
  3 files changed, 50 insertions(+)

 diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
 index 2ff6d41..5471706 100644
 --- a/arch/arm/mach-omap2/timer.c
 +++ b/arch/arm/mach-omap2/timer.c
 @@ -208,6 +208,13 @@ static void __init omap2_gp_clockevent_init(int 
 gptimer_id,
  {
int res;
  
 +  /*
 +   * For clock-event timers we never read the timer counter and
 +   * so we are not impacted by errata i103 and i767. Therefore,
 +   * we can safely ignore this errata for clock-event timers.
 +   */
 +  __omap_dm_timer_populate_errata(clkev, OMAP_TIMER_ERRATA_I103_I767);
 +

 Couple of points,

 1. It is confusing to me, as you are passing the errata flag so i expect
 api should set it. Why can't we do reverse way, you pass 0 here, since
 you don't want to set and pass this flag every other places where you
 want to enable this errata.

 Per the design of the __omap_dm_timer_populate_errata function, the 2nd
 argument is called override to allow us to override an errata. I am not
 a huge fan of this, but I wanted to be explicit in the code that we are
 intentionally allowing posted mode for the clock-events timer.

 I did not wish to pass the flags we want to set because if there more
 flags added in the future then we will have to keep changing the calls
 to the populate_errata function to add these.
 
 By the way, your proposal could work nicely if we could pass errata
 flags from HWMOD. However, I am not sure if Paul or Benoit would go for
 this as they want HWMOD data to be auto-generated as much as possible
 and so I am not sure how that would work for errata which are not
 expected by design ;-)

Another alternative would be to drop the override argument altogether
and just do something like the following for the clock-events timer ...

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index 43da595..f59e797 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -206,12 +206,14 @@ static void __init omap2_gp_clockevent_init(int 
gptimer_id,
 {
int res;
 
+   __omap_dm_timer_populate_errata(clkev);
+
/*
 * For clock-event timers we never read the timer counter and
 * so we are not impacted by errata i103 and i767. Therefore,
 * we can safely ignore this errata for clock-event timers.
 */
-   __omap_dm_timer_populate_errata(clkev, OMAP_TIMER_ERRATA_I103_I767);
+   clkev.errata = ~OMAP_TIMER_ERRATA_I103_I767;


[PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767

2012-09-05 Thread Jon Hunter
Errata Titles:
i103: Delay needed to read some GP timer, WD timer and sync timer registers
  after wakeup (OMAP3/4)
i767: Delay needed to read some GP timer registers after wakeup (OMAP5)

Description (i103/i767):
If a General Purpose Timer (GPTimer) is in posted mode (TSICR [2].POSTED=1),
due to internal resynchronizations, values read in TCRR, TCAR1 and TCAR2
registers right after the timer interface clock (L4) goes from stopped to
active may not return the expected values. The most common event leading to
this situation occurs upon wake up from idle.

GPTimer non-posted synchronization mode is not impacted by this limitation.

Workarounds:
1). Disable posted mode
2). Use static dependency between timer clock domain and MPUSS clock domain
3). Use no-idle mode when the timer is active

Workarounds #2 and #3 are not pratical from a power standpoint and so
workaround #1 has been implemented. Disabling posted mode adds some CPU overhead
for configuring the timers as the CPU has to wait for the write to complete.
However, disabling posted mode guarantees correct operation.

Please note that it is safe to use posted mode for timers if the counter (TCRR)
and capture (TCARx) registers will never be read. An example of this is the
clock-event system timer. This is used by the kernel to schedule events however,
the timers counter is never read and capture registers are not used. Given that
the kernel configures this timer often yet never reads the counter register it
is safe to enable posted mode in this case. Hence, for the timer used for kernel
clock-events, posted mode is enabled by overriding the errata for devices that
are impacted by this defect.

Although both dmtimers and watchdogs are impacted by this defect this patch only
implements the workaround for the dmtimer. Currently the watchdog driver does
not read the counter register and so no workaround is necessary.

Confirmed with Vaibhav Hiremath that this bug also impacts AM33xx devices.

Signed-off-by: Jon Hunter jon-hun...@ti.com
---
 arch/arm/mach-omap2/timer.c   |9 +++
 arch/arm/plat-omap/dmtimer.c  |2 ++
 arch/arm/plat-omap/include/plat/dmtimer.h |   39 +
 3 files changed, 50 insertions(+)

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index 2ff6d41..5471706 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -208,6 +208,13 @@ static void __init omap2_gp_clockevent_init(int gptimer_id,
 {
int res;
 
+   /*
+* For clock-event timers we never read the timer counter and
+* so we are not impacted by errata i103 and i767. Therefore,
+* we can safely ignore this errata for clock-event timers.
+*/
+   __omap_dm_timer_populate_errata(clkev, OMAP_TIMER_ERRATA_I103_I767);
+
res = omap_dm_timer_init_one(clkev, gptimer_id, fck_source);
BUG_ON(res);
 
@@ -305,6 +312,8 @@ static void __init omap2_gptimer_clocksource_init(int 
gptimer_id,
 {
int res;
 
+   __omap_dm_timer_populate_errata(clksrc, 0);
+
res = omap_dm_timer_init_one(clksrc, gptimer_id, fck_source);
BUG_ON(res);
 
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 938b50a..c34f55b 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -730,6 +730,8 @@ static int __devinit omap_dm_timer_probe(struct 
platform_device *pdev)
timer-pdev = pdev;
timer-capability = pdata-timer_capability;
 
+   __omap_dm_timer_populate_errata(timer, 0);
+
/* Skip pm_runtime_enable for OMAP1 */
if (!(timer-capability  OMAP_TIMER_NEEDS_RESET)) {
pm_runtime_enable(dev);
diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h 
b/arch/arm/plat-omap/include/plat/dmtimer.h
index 19e7fa5..5ce2f00 100644
--- a/arch/arm/plat-omap/include/plat/dmtimer.h
+++ b/arch/arm/plat-omap/include/plat/dmtimer.h
@@ -36,6 +36,7 @@
 #include linux/delay.h
 #include linux/io.h
 #include linux/platform_device.h
+#include plat/cpu.h
 
 #ifndef __ASM_ARCH_DMTIMER_H
 #define __ASM_ARCH_DMTIMER_H
@@ -61,6 +62,16 @@
 #define OMAP_TIMER_HAS_PWM 0x2000
 #define OMAP_TIMER_NEEDS_RESET 0x1000
 
+/*
+ * timer errata flags
+ *
+ * Errata i103/i767 impacts all OMAP3/4/5 devices including AM33xx. This
+ * errata prevents us from using posted mode on these devices, unless the
+ * timer counter register is never read. For more details please refer to
+ * the OMAP3/4/5 errata documents.
+ */
+#define OMAP_TIMER_ERRATA_I103_I7670x8000
+
 struct omap_timer_capability_dev_attr {
u32 timer_capability;
 };
@@ -265,6 +276,7 @@ struct omap_dm_timer {
int ctx_loss_count;
int revision;
u32 capability;
+   u32 errata;
struct platform_device *pdev;
struct list_head node;
 };
@@ -337,11 +349,38 @@ static inline void 

Re: [PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767

2012-09-05 Thread Vaibhav Hiremath


On 9/6/2012 12:34 AM, Jon Hunter wrote:
 Errata Titles:
 i103: Delay needed to read some GP timer, WD timer and sync timer registers
   after wakeup (OMAP3/4)
 i767: Delay needed to read some GP timer registers after wakeup (OMAP5)
 
 Description (i103/i767):
 If a General Purpose Timer (GPTimer) is in posted mode (TSICR [2].POSTED=1),
 due to internal resynchronizations, values read in TCRR, TCAR1 and TCAR2
 registers right after the timer interface clock (L4) goes from stopped to
 active may not return the expected values. The most common event leading to
 this situation occurs upon wake up from idle.
 
 GPTimer non-posted synchronization mode is not impacted by this limitation.
 
 Workarounds:
 1). Disable posted mode
 2). Use static dependency between timer clock domain and MPUSS clock domain
 3). Use no-idle mode when the timer is active
 
 Workarounds #2 and #3 are not pratical from a power standpoint and so
 workaround #1 has been implemented. Disabling posted mode adds some CPU 
 overhead
 for configuring the timers as the CPU has to wait for the write to complete.
 However, disabling posted mode guarantees correct operation.
 
 Please note that it is safe to use posted mode for timers if the counter 
 (TCRR)
 and capture (TCARx) registers will never be read. An example of this is the
 clock-event system timer. This is used by the kernel to schedule events 
 however,
 the timers counter is never read and capture registers are not used. Given 
 that
 the kernel configures this timer often yet never reads the counter register it
 is safe to enable posted mode in this case. Hence, for the timer used for 
 kernel
 clock-events, posted mode is enabled by overriding the errata for devices that
 are impacted by this defect.
 
 Although both dmtimers and watchdogs are impacted by this defect this patch 
 only
 implements the workaround for the dmtimer. Currently the watchdog driver does
 not read the counter register and so no workaround is necessary.
 
 Confirmed with Vaibhav Hiremath that this bug also impacts AM33xx devices.
 

Thanks for pinging me on this and getting it confirmed.

 Signed-off-by: Jon Hunter jon-hun...@ti.com
 ---
  arch/arm/mach-omap2/timer.c   |9 +++
  arch/arm/plat-omap/dmtimer.c  |2 ++
  arch/arm/plat-omap/include/plat/dmtimer.h |   39 
 +
  3 files changed, 50 insertions(+)
 
 diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
 index 2ff6d41..5471706 100644
 --- a/arch/arm/mach-omap2/timer.c
 +++ b/arch/arm/mach-omap2/timer.c
 @@ -208,6 +208,13 @@ static void __init omap2_gp_clockevent_init(int 
 gptimer_id,
  {
   int res;
  
 + /*
 +  * For clock-event timers we never read the timer counter and
 +  * so we are not impacted by errata i103 and i767. Therefore,
 +  * we can safely ignore this errata for clock-event timers.
 +  */
 + __omap_dm_timer_populate_errata(clkev, OMAP_TIMER_ERRATA_I103_I767);
 +

Couple of points,

1. It is confusing to me, as you are passing the errata flag so i expect
api should set it. Why can't we do reverse way, you pass 0 here, since
you don't want to set and pass this flag every other places where you
want to enable this errata.

2. Why can't we enable for all timers? Even though clock-event is anyway
not reading it, but still is is applicable to it, right?

3. Why can't we just simply Add this flag to hwmod_data file and read it
back in omap_timer_init() and omap_dm_timer_init_one(). Wouldn't that be
a good approach to handle it?

Thanks,
Vaibhav
   res = omap_dm_timer_init_one(clkev, gptimer_id, fck_source);
   BUG_ON(res);
  
 @@ -305,6 +312,8 @@ static void __init omap2_gptimer_clocksource_init(int 
 gptimer_id,
  {
   int res;
  
 + __omap_dm_timer_populate_errata(clksrc, 0);
 +
   res = omap_dm_timer_init_one(clksrc, gptimer_id, fck_source);
   BUG_ON(res);
  
 diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
 index 938b50a..c34f55b 100644
 --- a/arch/arm/plat-omap/dmtimer.c
 +++ b/arch/arm/plat-omap/dmtimer.c
 @@ -730,6 +730,8 @@ static int __devinit omap_dm_timer_probe(struct 
 platform_device *pdev)
   timer-pdev = pdev;
   timer-capability = pdata-timer_capability;
  
 + __omap_dm_timer_populate_errata(timer, 0);
 +
   /* Skip pm_runtime_enable for OMAP1 */
   if (!(timer-capability  OMAP_TIMER_NEEDS_RESET)) {
   pm_runtime_enable(dev);
 diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h 
 b/arch/arm/plat-omap/include/plat/dmtimer.h
 index 19e7fa5..5ce2f00 100644
 --- a/arch/arm/plat-omap/include/plat/dmtimer.h
 +++ b/arch/arm/plat-omap/include/plat/dmtimer.h
 @@ -36,6 +36,7 @@
  #include linux/delay.h
  #include linux/io.h
  #include linux/platform_device.h
 +#include plat/cpu.h
  
  #ifndef __ASM_ARCH_DMTIMER_H
  #define __ASM_ARCH_DMTIMER_H
 @@ -61,6 +62,16 @@
  #define OMAP_TIMER_HAS_PWM   0x2000
  #define