[PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767
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
---
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
#include
#include
+#include
#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 __omap_dm_timer_reset(struct
omap_dm_timer *timer,
__raw_writel(l, timer
Re: [PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767
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
> ---
> 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
> #include
> #include
> +#include
>
> #ifndef __ASM_ARCH_DMTIMER_H
> #define __ASM_ARCH_DMTIMER_H
> @@ -61,6 +62,16 @@
> #define OMAP_TIMER_HAS_PWM
Re: [PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767
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
>> ---
>> 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.
> 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. One limitation you would have with HWMOD is if an
erratum is only specific to a certain revision of the device. In this
case it is not and so it could work.
In general I think that having errata described by HWMOD would be a good
thing, but if think that should be a longer term goal with agreement
from Ben
Re: [PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767
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
>>> ---
>>> 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
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
---
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);
+
/*
RE: [PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767
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
> >> ---
> >> 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_
RE: [PATCH 01/10] ARM: OMAP3+: Implement timer workaround for errata i103 and i767
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
> ---
> 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 followin
