[PATCH 00/10] ARM: OMAP: DMTIMER fixes and clean-up

[PATCH 00/10] ARM: OMAP: DMTIMER fixes and clean-up

[Jon Hunter]

This series includes several fixes for the OMAP DMTIMER driver and a few
clean-ups to simplify some of the code. This series is based upon 3.6-rc4.

Tested on OMAP5912 OSK, OMAP2420 H4, OMAP3430 Beagle and OMAP4430 Panda.
Testing includes ...
1. Booting kernel on above boards
2. Set date and ensuring time of day is correct after 24 hours
3. Checking the timer counter is incrementing when configuring and starting
   a timer
4. Checking the timer overflow interrupt when timer expires.
5. Using different clock sources to operate the timer with.
6. Running a loop test overnight that continually runs test #3 and #4 for
   each available timer

Jon Hunter (10):
  ARM: OMAP3+: Implement timer workaround for errata i103 and i767
  ARM: OMAP: Fix timer posted mode support
  ARM: OMAP3: Correct HWMOD DMTIMER SYSC register declarations
  ARM: OMAP2/3: Define HWMOD software reset status for DMTIMERs
  ARM: OMAP2+: Don't use __omap_dm_timer_reset()
  ARM: OMAP: Fix dmtimer reset for timer1
  ARM: OMAP: Clean-up dmtimer reset code
  ARM: OMAP: Clean-up timer posted mode support
  ARM: OMAP: Add dmtimer interrupt disable function
  ARM: OMAP: Remove unnecessary call to clk_get()

 arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c |   15 ++-
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c         |   41 ++++----
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c         |    4 +
 arch/arm/mach-omap2/timer.c                        |   27 +++--
 arch/arm/plat-omap/dmtimer.c                       |  109 +++++++++++++-------
 arch/arm/plat-omap/include/plat/dmtimer.h          |   95 ++++++++++-------
 6 files changed, 184 insertions(+), 107 deletions(-)

-- 
1.7.9.5

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

[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-hunter@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				0x20000000
 #define OMAP_TIMER_NEEDS_RESET				0x10000000
 
+/*
+ * 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_I767			0x80000000
+
 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->io_base + OMAP_TIMER_OCP_CFG_OFFSET);
 
+	if (timer->errata & OMAP_TIMER_ERRATA_I103_I767)
+		return;
+
 	/* Match hardware reset default of posted mode */
 	__omap_dm_timer_write(timer, OMAP_TIMER_IF_CTRL_REG,
 					OMAP_TIMER_CTRL_POSTED, 0);
 }
 
+/**
+ * __omap_dm_timer_populate_errata - populate errata flags for a timer
+ * @timer:      pointer to timer handle
+ * @override:   errata flags to be ignored
+ *
+ * For a given timer, populate the timer errata flags that are specific to the
+ * OMAP device being used. An override argument is provided so that a specific
+ * erratum could be ignored for a timer if the timer is used in such a way the
+ * erratum has no impact.
+ */
+static inline void
+__omap_dm_timer_populate_errata(struct omap_dm_timer *timer, u32 override)
+{
+	timer->errata = 0;
+
+	if (cpu_class_is_omap1() || cpu_is_omap24xx())
+		return;
+
+	if (override & OMAP_TIMER_ERRATA_I103_I767)
+		return;
+
+	timer->errata = OMAP_TIMER_ERRATA_I103_I767;
+}
+
 static inline int __omap_dm_timer_set_source(struct clk *timer_fck,
 						struct clk *parent)
 {
-- 
1.7.9.5

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

[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-hunter@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				0x20000000
>  #define OMAP_TIMER_NEEDS_RESET				0x10000000
>  
> +/*
> + * 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_I767			0x80000000
> +
>  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->io_base + OMAP_TIMER_OCP_CFG_OFFSET);
>  
> +	if (timer->errata & OMAP_TIMER_ERRATA_I103_I767)
> +		return;
> +
>  	/* Match hardware reset default of posted mode */
>  	__omap_dm_timer_write(timer, OMAP_TIMER_IF_CTRL_REG,
>  					OMAP_TIMER_CTRL_POSTED, 0);
>  }
>  
> +/**
> + * __omap_dm_timer_populate_errata - populate errata flags for a timer
> + * @timer:      pointer to timer handle
> + * @override:   errata flags to be ignored
> + *
> + * For a given timer, populate the timer errata flags that are specific to the
> + * OMAP device being used. An override argument is provided so that a specific
> + * erratum could be ignored for a timer if the timer is used in such a way the
> + * erratum has no impact.
> + */
> +static inline void
> +__omap_dm_timer_populate_errata(struct omap_dm_timer *timer, u32 override)
> +{
> +	timer->errata = 0;
> +
> +	if (cpu_class_is_omap1() || cpu_is_omap24xx())
> +		return;
> +
> +	if (override & OMAP_TIMER_ERRATA_I103_I767)
> +		return;
> +
> +	timer->errata = OMAP_TIMER_ERRATA_I103_I767;
> +}
> +
>  static inline int __omap_dm_timer_set_source(struct clk *timer_fck,
>  						struct clk *parent)
>  {
> 

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

[Jon Hunter]

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-hunter@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.

> 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 Benoit and add some general errata helpers to HWMOD.

Cheers
Jon

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

[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-hunter@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

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

[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-hunter@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;

Cheers
Jon

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

[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-hunter@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;
>  
> +       __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;
> 
> Cheers
> Jon
> 

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

[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-hunter@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 done 
indirect implementation of errata. So I still feel, we should leverage hwmod 
info for this, and anyway going forward DT will replace it.

Thanks,
Vaibhav

> 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 Benoit and add some general errata helpers to HWMOD.
> 
> Cheers
> Jon
> 

[PATCH 02/10] ARM: OMAP: Fix timer posted mode support

[Jon Hunter]

Currently the dmtimer posted mode is being enabled when the function
__omap_dm_timer_reset() is called. This function is only being called for
OMAP1 timers and OMAP2+ timers that are being used as system timers. Hence,
for OMAP2+ timers that are NOT being used as a system timer, posted mode is
not enabled but the "timer->posted" variable is still set (incorrectly) in
the omap_dm_timer_prepare() function.

This is a regression introduced by commit 3392cdd3 (ARM: OMAP: dmtimer:
switch-over to platform device driver) which changed the code to only call
omap_dm_timer_reset() for OMAP1 devices. Although this is a regression from
the original code it only impacts performance and so is not needed for stable.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/timer.c               |    3 +--
 arch/arm/plat-omap/dmtimer.c              |   14 +++++---------
 arch/arm/plat-omap/include/plat/dmtimer.h |    9 ++++++++-
 3 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index 5471706..e24ee0f 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -194,10 +194,9 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 	}
 	__omap_dm_timer_init_regs(timer);
 	__omap_dm_timer_reset(timer, 1, 1);
-	timer->posted = 1;
+	__omap_dm_timer_enable_posted(timer);
 
 	timer->rate = clk_get_rate(timer->fclk);
-
 	timer->reserved = 1;
 
 	return res;
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index c34f55b..22790ea 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -122,21 +122,15 @@ static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
 
 static void omap_dm_timer_reset(struct omap_dm_timer *timer)
 {
-	omap_dm_timer_enable(timer);
 	if (timer->pdev->id != 1) {
 		omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG, 0x06);
 		omap_dm_timer_wait_for_reset(timer);
 	}
-
 	__omap_dm_timer_reset(timer, 0, 0);
-	omap_dm_timer_disable(timer);
-	timer->posted = 1;
 }
 
 int omap_dm_timer_prepare(struct omap_dm_timer *timer)
 {
-	int ret;
-
 	/*
 	 * FIXME: OMAP1 devices do not use the clock framework for dmtimers so
 	 * do not call clk_get() for these devices.
@@ -150,13 +144,15 @@ int omap_dm_timer_prepare(struct omap_dm_timer *timer)
 		}
 	}
 
+	omap_dm_timer_enable(timer);
+
 	if (timer->capability & OMAP_TIMER_NEEDS_RESET)
 		omap_dm_timer_reset(timer);
 
-	ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
+	__omap_dm_timer_enable_posted(timer);
+	omap_dm_timer_disable(timer);
 
-	timer->posted = 1;
-	return ret;
+	return omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
 }
 
 static inline u32 omap_dm_timer_reserved_systimer(int id)
diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h b/arch/arm/plat-omap/include/plat/dmtimer.h
index 5ce2f00..fa9d04b 100644
--- a/arch/arm/plat-omap/include/plat/dmtimer.h
+++ b/arch/arm/plat-omap/include/plat/dmtimer.h
@@ -348,13 +348,20 @@ static inline void __omap_dm_timer_reset(struct omap_dm_timer *timer,
 		l |= 1 << 2;
 
 	__raw_writel(l, timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET);
+}
+
+static inline void __omap_dm_timer_enable_posted(struct omap_dm_timer *timer)
+{
+	if (timer->posted)
+		return;
 
 	if (timer->errata & OMAP_TIMER_ERRATA_I103_I767)
 		return;
 
-	/* Match hardware reset default of posted mode */
 	__omap_dm_timer_write(timer, OMAP_TIMER_IF_CTRL_REG,
 					OMAP_TIMER_CTRL_POSTED, 0);
+	timer->context.tsicr = OMAP_TIMER_CTRL_POSTED;
+	timer->posted = 1;
 }
 
 /**
-- 
1.7.9.5

[PATCH 02/10] ARM: OMAP: Fix timer posted mode support

[Vaibhav Hiremath]

On 9/6/2012 12:34 AM, Jon Hunter wrote:
> Currently the dmtimer posted mode is being enabled when the function
> __omap_dm_timer_reset() is called. This function is only being called for
> OMAP1 timers and OMAP2+ timers that are being used as system timers. Hence,
> for OMAP2+ timers that are NOT being used as a system timer, posted mode is
> not enabled but the "timer->posted" variable is still set (incorrectly) in
> the omap_dm_timer_prepare() function.
> 
> This is a regression introduced by commit 3392cdd3 (ARM: OMAP: dmtimer:
> switch-over to platform device driver) which changed the code to only call
> omap_dm_timer_reset() for OMAP1 devices. Although this is a regression from
> the original code it only impacts performance and so is not needed for stable.
> 
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> ---
>  arch/arm/mach-omap2/timer.c               |    3 +--
>  arch/arm/plat-omap/dmtimer.c              |   14 +++++---------
>  arch/arm/plat-omap/include/plat/dmtimer.h |    9 ++++++++-
>  3 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index 5471706..e24ee0f 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -194,10 +194,9 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
>  	}
>  	__omap_dm_timer_init_regs(timer);
>  	__omap_dm_timer_reset(timer, 1, 1);
> -	timer->posted = 1;
> +	__omap_dm_timer_enable_posted(timer);
>  
>  	timer->rate = clk_get_rate(timer->fclk);
> -
>  	timer->reserved = 1;
>  
>  	return res;
> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> index c34f55b..22790ea 100644
> --- a/arch/arm/plat-omap/dmtimer.c
> +++ b/arch/arm/plat-omap/dmtimer.c
> @@ -122,21 +122,15 @@ static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
>  
>  static void omap_dm_timer_reset(struct omap_dm_timer *timer)
>  {
> -	omap_dm_timer_enable(timer);
>  	if (timer->pdev->id != 1) {
>  		omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG, 0x06);
>  		omap_dm_timer_wait_for_reset(timer);
>  	}
> -
>  	__omap_dm_timer_reset(timer, 0, 0);
> -	omap_dm_timer_disable(timer);
> -	timer->posted = 1;
>  }
>  
>  int omap_dm_timer_prepare(struct omap_dm_timer *timer)
>  {
> -	int ret;
> -
>  	/*
>  	 * FIXME: OMAP1 devices do not use the clock framework for dmtimers so
>  	 * do not call clk_get() for these devices.
> @@ -150,13 +144,15 @@ int omap_dm_timer_prepare(struct omap_dm_timer *timer)
>  		}
>  	}
>  
> +	omap_dm_timer_enable(timer);
> +
>  	if (timer->capability & OMAP_TIMER_NEEDS_RESET)
>  		omap_dm_timer_reset(timer);
>  
> -	ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
> +	__omap_dm_timer_enable_posted(timer);
> +	omap_dm_timer_disable(timer);
>  
> -	timer->posted = 1;
> -	return ret;
> +	return omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);

May be I am speculating here and I know this is tested and supposed to
work, but Isn't it safe to set parent keeping module enables.

I would still recommend you to move is before omap_dm_timer_disable().
There could be devices or hw bugs/issues, may be related to standby/idle
protocol happening underneath module enable/disable.

Thanks,
Vaibhav
>  }
>  
>  static inline u32 omap_dm_timer_reserved_systimer(int id)
> diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h b/arch/arm/plat-omap/include/plat/dmtimer.h
> index 5ce2f00..fa9d04b 100644
> --- a/arch/arm/plat-omap/include/plat/dmtimer.h
> +++ b/arch/arm/plat-omap/include/plat/dmtimer.h
> @@ -348,13 +348,20 @@ static inline void __omap_dm_timer_reset(struct omap_dm_timer *timer,
>  		l |= 1 << 2;
>  
>  	__raw_writel(l, timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET);
> +}
> +
> +static inline void __omap_dm_timer_enable_posted(struct omap_dm_timer *timer)
> +{
> +	if (timer->posted)
> +		return;
>  
>  	if (timer->errata & OMAP_TIMER_ERRATA_I103_I767)
>  		return;
>  
> -	/* Match hardware reset default of posted mode */
>  	__omap_dm_timer_write(timer, OMAP_TIMER_IF_CTRL_REG,
>  					OMAP_TIMER_CTRL_POSTED, 0);
> +	timer->context.tsicr = OMAP_TIMER_CTRL_POSTED;
> +	timer->posted = 1;
>  }
>  
>  /**
> 

[PATCH 02/10] ARM: OMAP: Fix timer posted mode support

[Jon Hunter]

On 09/06/2012 07:57 AM, Vaibhav Hiremath wrote:
> 
> 
> On 9/6/2012 12:34 AM, Jon Hunter wrote:
>> Currently the dmtimer posted mode is being enabled when the function
>> __omap_dm_timer_reset() is called. This function is only being called for
>> OMAP1 timers and OMAP2+ timers that are being used as system timers. Hence,
>> for OMAP2+ timers that are NOT being used as a system timer, posted mode is
>> not enabled but the "timer->posted" variable is still set (incorrectly) in
>> the omap_dm_timer_prepare() function.
>>
>> This is a regression introduced by commit 3392cdd3 (ARM: OMAP: dmtimer:
>> switch-over to platform device driver) which changed the code to only call
>> omap_dm_timer_reset() for OMAP1 devices. Although this is a regression from
>> the original code it only impacts performance and so is not needed for stable.
>>
>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
>> ---
>>  arch/arm/mach-omap2/timer.c               |    3 +--
>>  arch/arm/plat-omap/dmtimer.c              |   14 +++++---------
>>  arch/arm/plat-omap/include/plat/dmtimer.h |    9 ++++++++-
>>  3 files changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
>> index 5471706..e24ee0f 100644
>> --- a/arch/arm/mach-omap2/timer.c
>> +++ b/arch/arm/mach-omap2/timer.c
>> @@ -194,10 +194,9 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
>>  	}
>>  	__omap_dm_timer_init_regs(timer);
>>  	__omap_dm_timer_reset(timer, 1, 1);
>> -	timer->posted = 1;
>> +	__omap_dm_timer_enable_posted(timer);
>>  
>>  	timer->rate = clk_get_rate(timer->fclk);
>> -
>>  	timer->reserved = 1;
>>  
>>  	return res;
>> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
>> index c34f55b..22790ea 100644
>> --- a/arch/arm/plat-omap/dmtimer.c
>> +++ b/arch/arm/plat-omap/dmtimer.c
>> @@ -122,21 +122,15 @@ static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
>>  
>>  static void omap_dm_timer_reset(struct omap_dm_timer *timer)
>>  {
>> -	omap_dm_timer_enable(timer);
>>  	if (timer->pdev->id != 1) {
>>  		omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG, 0x06);
>>  		omap_dm_timer_wait_for_reset(timer);
>>  	}
>> -
>>  	__omap_dm_timer_reset(timer, 0, 0);
>> -	omap_dm_timer_disable(timer);
>> -	timer->posted = 1;
>>  }
>>  
>>  int omap_dm_timer_prepare(struct omap_dm_timer *timer)
>>  {
>> -	int ret;
>> -
>>  	/*
>>  	 * FIXME: OMAP1 devices do not use the clock framework for dmtimers so
>>  	 * do not call clk_get() for these devices.
>> @@ -150,13 +144,15 @@ int omap_dm_timer_prepare(struct omap_dm_timer *timer)
>>  		}
>>  	}
>>  
>> +	omap_dm_timer_enable(timer);
>> +
>>  	if (timer->capability & OMAP_TIMER_NEEDS_RESET)
>>  		omap_dm_timer_reset(timer);
>>  
>> -	ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
>> +	__omap_dm_timer_enable_posted(timer);
>> +	omap_dm_timer_disable(timer);
>>  
>> -	timer->posted = 1;
>> -	return ret;
>> +	return omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
> 
> May be I am speculating here and I know this is tested and supposed to
> work, but Isn't it safe to set parent keeping module enables.

So you would rather change the functional clock while the timer is
enabled/active? So although that we are doing this today, that does not
sound like a good idea to me :-)

> I would still recommend you to move is before omap_dm_timer_disable().
> There could be devices or hw bugs/issues, may be related to standby/idle
> protocol happening underneath module enable/disable.

The register for setting the timer parent clock is located in the
Configuration Module (OMAP1) and PRCM (OMAP2+) and so there is no
requirement to keep the timer active while doing this.

Furthermore, if you look at the omap_dm_timer_set_source() function that
can be called from any driver using the timer, we never enable the timer
when setting the timer clock source. So this way is more consistent with
how the set_source function is implemented.

Cheers
Jon

[PATCH 02/10] ARM: OMAP: Fix timer posted mode support

[Jon Hunter]

On 09/06/2012 09:20 AM, Jon Hunter wrote:
> 
> On 09/06/2012 07:57 AM, Vaibhav Hiremath wrote:
>>
>>
>> On 9/6/2012 12:34 AM, Jon Hunter wrote:
>>> Currently the dmtimer posted mode is being enabled when the function
>>> __omap_dm_timer_reset() is called. This function is only being called for
>>> OMAP1 timers and OMAP2+ timers that are being used as system timers. Hence,
>>> for OMAP2+ timers that are NOT being used as a system timer, posted mode is
>>> not enabled but the "timer->posted" variable is still set (incorrectly) in
>>> the omap_dm_timer_prepare() function.
>>>
>>> This is a regression introduced by commit 3392cdd3 (ARM: OMAP: dmtimer:
>>> switch-over to platform device driver) which changed the code to only call
>>> omap_dm_timer_reset() for OMAP1 devices. Although this is a regression from
>>> the original code it only impacts performance and so is not needed for stable.
>>>
>>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
>>> ---
>>>  arch/arm/mach-omap2/timer.c               |    3 +--
>>>  arch/arm/plat-omap/dmtimer.c              |   14 +++++---------
>>>  arch/arm/plat-omap/include/plat/dmtimer.h |    9 ++++++++-
>>>  3 files changed, 14 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
>>> index 5471706..e24ee0f 100644
>>> --- a/arch/arm/mach-omap2/timer.c
>>> +++ b/arch/arm/mach-omap2/timer.c
>>> @@ -194,10 +194,9 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
>>>  	}
>>>  	__omap_dm_timer_init_regs(timer);
>>>  	__omap_dm_timer_reset(timer, 1, 1);
>>> -	timer->posted = 1;
>>> +	__omap_dm_timer_enable_posted(timer);
>>>  
>>>  	timer->rate = clk_get_rate(timer->fclk);
>>> -
>>>  	timer->reserved = 1;
>>>  
>>>  	return res;
>>> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
>>> index c34f55b..22790ea 100644
>>> --- a/arch/arm/plat-omap/dmtimer.c
>>> +++ b/arch/arm/plat-omap/dmtimer.c
>>> @@ -122,21 +122,15 @@ static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
>>>  
>>>  static void omap_dm_timer_reset(struct omap_dm_timer *timer)
>>>  {
>>> -	omap_dm_timer_enable(timer);
>>>  	if (timer->pdev->id != 1) {
>>>  		omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG, 0x06);
>>>  		omap_dm_timer_wait_for_reset(timer);
>>>  	}
>>> -
>>>  	__omap_dm_timer_reset(timer, 0, 0);
>>> -	omap_dm_timer_disable(timer);
>>> -	timer->posted = 1;
>>>  }
>>>  
>>>  int omap_dm_timer_prepare(struct omap_dm_timer *timer)
>>>  {
>>> -	int ret;
>>> -
>>>  	/*
>>>  	 * FIXME: OMAP1 devices do not use the clock framework for dmtimers so
>>>  	 * do not call clk_get() for these devices.
>>> @@ -150,13 +144,15 @@ int omap_dm_timer_prepare(struct omap_dm_timer *timer)
>>>  		}
>>>  	}
>>>  
>>> +	omap_dm_timer_enable(timer);
>>> +
>>>  	if (timer->capability & OMAP_TIMER_NEEDS_RESET)
>>>  		omap_dm_timer_reset(timer);
>>>  
>>> -	ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
>>> +	__omap_dm_timer_enable_posted(timer);
>>> +	omap_dm_timer_disable(timer);
>>>  
>>> -	timer->posted = 1;
>>> -	return ret;
>>> +	return omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
>>
>> May be I am speculating here and I know this is tested and supposed to
>> work, but Isn't it safe to set parent keeping module enables.
> 
> So you would rather change the functional clock while the timer is
> enabled/active? So although that we are doing this today, that does not
> sound like a good idea to me :-)

Actually, we are not doing this today. If you look at the current code
we are only enabling the timer while doing the soft-reset for omap1
devices. Hence, even in the current code we set the parent while the
timer is not enabled. So there is no actual change here in the sequence.

Cheers
Jon

[PATCH 02/10] ARM: OMAP: Fix timer posted mode support

[Hiremath, Vaibhav]

On Thu, Sep 06, 2012 at 21:31:21, Hunter, Jon wrote:
> 
> On 09/06/2012 09:20 AM, Jon Hunter wrote:
> > 
> > On 09/06/2012 07:57 AM, Vaibhav Hiremath wrote:
> >>
> >>
> >> On 9/6/2012 12:34 AM, Jon Hunter wrote:
> >>> Currently the dmtimer posted mode is being enabled when the function
> >>> __omap_dm_timer_reset() is called. This function is only being called for
> >>> OMAP1 timers and OMAP2+ timers that are being used as system timers. Hence,
> >>> for OMAP2+ timers that are NOT being used as a system timer, posted mode is
> >>> not enabled but the "timer->posted" variable is still set (incorrectly) in
> >>> the omap_dm_timer_prepare() function.
> >>>
> >>> This is a regression introduced by commit 3392cdd3 (ARM: OMAP: dmtimer:
> >>> switch-over to platform device driver) which changed the code to only call
> >>> omap_dm_timer_reset() for OMAP1 devices. Although this is a regression from
> >>> the original code it only impacts performance and so is not needed for stable.
> >>>
> >>> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> >>> ---
> >>>  arch/arm/mach-omap2/timer.c               |    3 +--
> >>>  arch/arm/plat-omap/dmtimer.c              |   14 +++++---------
> >>>  arch/arm/plat-omap/include/plat/dmtimer.h |    9 ++++++++-
> >>>  3 files changed, 14 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> >>> index 5471706..e24ee0f 100644
> >>> --- a/arch/arm/mach-omap2/timer.c
> >>> +++ b/arch/arm/mach-omap2/timer.c
> >>> @@ -194,10 +194,9 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
> >>>  	}
> >>>  	__omap_dm_timer_init_regs(timer);
> >>>  	__omap_dm_timer_reset(timer, 1, 1);
> >>> -	timer->posted = 1;
> >>> +	__omap_dm_timer_enable_posted(timer);
> >>>  
> >>>  	timer->rate = clk_get_rate(timer->fclk);
> >>> -
> >>>  	timer->reserved = 1;
> >>>  
> >>>  	return res;
> >>> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> >>> index c34f55b..22790ea 100644
> >>> --- a/arch/arm/plat-omap/dmtimer.c
> >>> +++ b/arch/arm/plat-omap/dmtimer.c
> >>> @@ -122,21 +122,15 @@ static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
> >>>  
> >>>  static void omap_dm_timer_reset(struct omap_dm_timer *timer)
> >>>  {
> >>> -	omap_dm_timer_enable(timer);
> >>>  	if (timer->pdev->id != 1) {
> >>>  		omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG, 0x06);
> >>>  		omap_dm_timer_wait_for_reset(timer);
> >>>  	}
> >>> -
> >>>  	__omap_dm_timer_reset(timer, 0, 0);
> >>> -	omap_dm_timer_disable(timer);
> >>> -	timer->posted = 1;
> >>>  }
> >>>  
> >>>  int omap_dm_timer_prepare(struct omap_dm_timer *timer)
> >>>  {
> >>> -	int ret;
> >>> -
> >>>  	/*
> >>>  	 * FIXME: OMAP1 devices do not use the clock framework for dmtimers so
> >>>  	 * do not call clk_get() for these devices.
> >>> @@ -150,13 +144,15 @@ int omap_dm_timer_prepare(struct omap_dm_timer *timer)
> >>>  		}
> >>>  	}
> >>>  
> >>> +	omap_dm_timer_enable(timer);
> >>> +
> >>>  	if (timer->capability & OMAP_TIMER_NEEDS_RESET)
> >>>  		omap_dm_timer_reset(timer);
> >>>  
> >>> -	ret = omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
> >>> +	__omap_dm_timer_enable_posted(timer);
> >>> +	omap_dm_timer_disable(timer);
> >>>  
> >>> -	timer->posted = 1;
> >>> -	return ret;
> >>> +	return omap_dm_timer_set_source(timer, OMAP_TIMER_SRC_32_KHZ);
> >>
> >> May be I am speculating here and I know this is tested and supposed to
> >> work, but Isn't it safe to set parent keeping module enables.
> > 
> > So you would rather change the functional clock while the timer is
> > enabled/active? So although that we are doing this today, that does not
> > sound like a good idea to me :-)
> 
> Actually, we are not doing this today. If you look at the current code
> we are only enabling the timer while doing the soft-reset for omap1
> devices. Hence, even in the current code we set the parent while the
> timer is not enabled. So there is no actual change here in the sequence.
> 

Yes, you are absolutely right here. As such there is no change in the 
sequence.

Thanks,
Vaibhav 

[PATCH 03/10] ARM: OMAP3: Correct HWMOD DMTIMER SYSC register declarations

[Jon Hunter]

Currently, the OMAP3 HWMOD data defines two TIOCP_CFG register structures
(referred to as the SYSC register in the HWMOD data) where timers 1, 2 and 10
use one of the defintions and the other timers use the other definition. For
OMAP3 devices the structure of the DMTIMER TIOCP_CFG register is the same for
all 12 instances of the DMTIMER. Please note that this is a difference between
OMAP3 and OMAP4 and could be the source of the confusion.

For OMAP3 devices, the DMTIMER TIOCP_CFG register has the fields,
clock-activity, emufree, idlemode, enwakeup, softreset and autoidle for all
12 timers. Therefore, remove one of the SYSC register definitions for the
DMTIMERs and ensure the appropriate register fields are defined for all
DMTIMERs.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |   27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index c9e3820..659f7c4 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -115,28 +115,13 @@ static struct omap_hwmod omap3xxx_iva_hwmod = {
 };
 
 /* timer class */
-static struct omap_hwmod_class_sysconfig omap3xxx_timer_1ms_sysc = {
-	.rev_offs	= 0x0000,
-	.sysc_offs	= 0x0010,
-	.syss_offs	= 0x0014,
-	.sysc_flags	= (SYSC_HAS_SIDLEMODE | SYSC_HAS_CLOCKACTIVITY |
-				SYSC_HAS_ENAWAKEUP | SYSC_HAS_SOFTRESET |
-				SYSC_HAS_EMUFREE | SYSC_HAS_AUTOIDLE),
-	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
-	.sysc_fields	= &omap_hwmod_sysc_type1,
-};
-
-static struct omap_hwmod_class omap3xxx_timer_1ms_hwmod_class = {
-	.name = "timer",
-	.sysc = &omap3xxx_timer_1ms_sysc,
-};
-
 static struct omap_hwmod_class_sysconfig omap3xxx_timer_sysc = {
 	.rev_offs	= 0x0000,
 	.sysc_offs	= 0x0010,
 	.syss_offs	= 0x0014,
-	.sysc_flags	= (SYSC_HAS_SIDLEMODE | SYSC_HAS_ENAWAKEUP |
-			   SYSC_HAS_SOFTRESET | SYSC_HAS_AUTOIDLE),
+	.sysc_flags	= (SYSC_HAS_SIDLEMODE | SYSC_HAS_CLOCKACTIVITY |
+			   SYSC_HAS_ENAWAKEUP | SYSC_HAS_SOFTRESET |
+			   SYSC_HAS_EMUFREE | SYSC_HAS_AUTOIDLE),
 	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
 	.sysc_fields	= &omap_hwmod_sysc_type1,
 };
@@ -176,7 +161,7 @@ static struct omap_hwmod omap3xxx_timer1_hwmod = {
 		},
 	},
 	.dev_attr	= &capability_alwon_dev_attr,
-	.class		= &omap3xxx_timer_1ms_hwmod_class,
+	.class		= &omap3xxx_timer_hwmod_class,
 };
 
 /* timer2 */
@@ -193,7 +178,7 @@ static struct omap_hwmod omap3xxx_timer2_hwmod = {
 			.idlest_idle_bit = OMAP3430_ST_GPT2_SHIFT,
 		},
 	},
-	.class		= &omap3xxx_timer_1ms_hwmod_class,
+	.class		= &omap3xxx_timer_hwmod_class,
 };
 
 /* timer3 */
@@ -332,7 +317,7 @@ static struct omap_hwmod omap3xxx_timer10_hwmod = {
 		},
 	},
 	.dev_attr	= &capability_pwm_dev_attr,
-	.class		= &omap3xxx_timer_1ms_hwmod_class,
+	.class		= &omap3xxx_timer_hwmod_class,
 };
 
 /* timer11 */
-- 
1.7.9.5

[PATCH 04/10] ARM: OMAP2/3: Define HWMOD software reset status for DMTIMERs

[Jon Hunter]

For OMAP2/3 devices, the HWMOD data does not define a software reset status
field for the DMTIMERs. Therefore, when HWMOD performs a soft-reset of the
DMTIMER we don't check and wait for the reset to complete. For OMAP2/3 devices,
the software reset status for a DMTIMER can be read from bit 0 of the DMTIMER
TISTAT register (referred to as the SYSS register in HWMOD). Add the
appropriate HWMOD definitions so that HWMOD will check the software reset
status when performing a software reset of the DMTIMER.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c |    2 +-
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c         |    3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c b/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c
index afad69c..d094285 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c
@@ -60,7 +60,7 @@ static struct omap_hwmod_class_sysconfig omap2xxx_timer_sysc = {
 	.syss_offs	= 0x0014,
 	.sysc_flags	= (SYSC_HAS_SIDLEMODE | SYSC_HAS_CLOCKACTIVITY |
 			   SYSC_HAS_ENAWAKEUP | SYSC_HAS_SOFTRESET |
-			   SYSC_HAS_AUTOIDLE),
+			   SYSC_HAS_AUTOIDLE | SYSS_HAS_RESET_STATUS),
 	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
 	.sysc_fields	= &omap_hwmod_sysc_type1,
 };
diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index 659f7c4..5f0b2ee 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -121,7 +121,8 @@ static struct omap_hwmod_class_sysconfig omap3xxx_timer_sysc = {
 	.syss_offs	= 0x0014,
 	.sysc_flags	= (SYSC_HAS_SIDLEMODE | SYSC_HAS_CLOCKACTIVITY |
 			   SYSC_HAS_ENAWAKEUP | SYSC_HAS_SOFTRESET |
-			   SYSC_HAS_EMUFREE | SYSC_HAS_AUTOIDLE),
+			   SYSC_HAS_EMUFREE | SYSC_HAS_AUTOIDLE |
+			   SYSS_HAS_RESET_STATUS),
 	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
 	.sysc_fields	= &omap_hwmod_sysc_type1,
 };
-- 
1.7.9.5

[PATCH 05/10] ARM: OMAP2+: Don't use __omap_dm_timer_reset()

[Jon Hunter]

Currently OMAP2+ devices are using the function __omap_dm_timer_reset() to
configure the clock-activity, idle, wakeup-enable and auto-idle fields in the
timer OCP_CFG register. The name of the function is mis-leading because this
function does not actually perform a reset of the timer.

For OMAP2+ devices, HWMOD is responsible for reseting and configuring the
timer OCP_CFG register. Therefore, do not use __omap_dm_timer_reset() for
OMAP2+ devices and rely on HWMOD. Furthermore, some timer instances do not
have the fields clock-activity, wakeup-enable and auto-idle and so this
function could configure the OCP_CFG register incorrectly.

Currently HWMOD is not configuring the clock-activity field in the OCP_CFG
register for timers that have this field. Commit 0f0d080 (ARM: OMAP: DMTimer:
Use posted mode) configures the clock-activity field to keep the f-clk enabled
so that the wake-up capability is enabled. Therefore, add the appropriate flags
to the timer HWMOD structures to configure this field in the same way.

For OMAP2/3 devices all dmtimers have the clock-activity field, where as for
OMAP4 devices, only dmtimer 1, 2 and 10 have the clock-activity field.

Verified on OMAP2420 H4, OMAP3430 Beagle and OMAP4430 Panda that HWMOD is
configuring the dmtimer OCP_CFG register as expected for clock-events timer.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c |   13 +++++++++++++
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c         |   13 +++++++++++++
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c         |    4 ++++
 arch/arm/mach-omap2/timer.c                        |    1 -
 4 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c b/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c
index d094285..e64df71 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c
@@ -62,6 +62,7 @@ static struct omap_hwmod_class_sysconfig omap2xxx_timer_sysc = {
 			   SYSC_HAS_ENAWAKEUP | SYSC_HAS_SOFTRESET |
 			   SYSC_HAS_AUTOIDLE | SYSS_HAS_RESET_STATUS),
 	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+	.clockact       = CLOCKACT_TEST_ICLK,
 	.sysc_fields	= &omap_hwmod_sysc_type1,
 };
 
@@ -239,6 +240,7 @@ struct omap_hwmod omap2xxx_timer1_hwmod = {
 	},
 	.dev_attr	= &capability_alwon_dev_attr,
 	.class		= &omap2xxx_timer_hwmod_class,
+	.flags          = HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer2 */
@@ -257,6 +259,7 @@ struct omap_hwmod omap2xxx_timer2_hwmod = {
 		},
 	},
 	.class		= &omap2xxx_timer_hwmod_class,
+	.flags          = HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer3 */
@@ -275,6 +278,7 @@ struct omap_hwmod omap2xxx_timer3_hwmod = {
 		},
 	},
 	.class		= &omap2xxx_timer_hwmod_class,
+	.flags          = HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer4 */
@@ -293,6 +297,7 @@ struct omap_hwmod omap2xxx_timer4_hwmod = {
 		},
 	},
 	.class		= &omap2xxx_timer_hwmod_class,
+	.flags          = HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer5 */
@@ -311,6 +316,7 @@ struct omap_hwmod omap2xxx_timer5_hwmod = {
 		},
 	},
 	.class		= &omap2xxx_timer_hwmod_class,
+	.flags          = HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer6 */
@@ -329,6 +335,7 @@ struct omap_hwmod omap2xxx_timer6_hwmod = {
 		},
 	},
 	.class		= &omap2xxx_timer_hwmod_class,
+	.flags          = HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer7 */
@@ -347,6 +354,7 @@ struct omap_hwmod omap2xxx_timer7_hwmod = {
 		},
 	},
 	.class		= &omap2xxx_timer_hwmod_class,
+	.flags          = HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer8 */
@@ -365,6 +373,7 @@ struct omap_hwmod omap2xxx_timer8_hwmod = {
 		},
 	},
 	.class		= &omap2xxx_timer_hwmod_class,
+	.flags          = HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer9 */
@@ -384,6 +393,7 @@ struct omap_hwmod omap2xxx_timer9_hwmod = {
 	},
 	.dev_attr	= &capability_pwm_dev_attr,
 	.class		= &omap2xxx_timer_hwmod_class,
+	.flags          = HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer10 */
@@ -403,6 +413,7 @@ struct omap_hwmod omap2xxx_timer10_hwmod = {
 	},
 	.dev_attr	= &capability_pwm_dev_attr,
 	.class		= &omap2xxx_timer_hwmod_class,
+	.flags          = HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer11 */
@@ -422,6 +433,7 @@ struct omap_hwmod omap2xxx_timer11_hwmod = {
 	},
 	.dev_attr	= &capability_pwm_dev_attr,
 	.class		= &omap2xxx_timer_hwmod_class,
+	.flags          = HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer12 */
@@ -441,6 +453,7 @@ struct omap_hwmod omap2xxx_timer12_hwmod = {
 	},
 	.dev_attr	= &capability_pwm_dev_attr,
 	.class		= &omap2xxx_timer_hwmod_class,
+	.flags          = HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* wd_timer2 */
diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index 5f0b2ee..4b02440 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -124,6 +124,7 @@ static struct omap_hwmod_class_sysconfig omap3xxx_timer_sysc = {
 			   SYSC_HAS_EMUFREE | SYSC_HAS_AUTOIDLE |
 			   SYSS_HAS_RESET_STATUS),
 	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+	.clockact	= CLOCKACT_TEST_ICLK,
 	.sysc_fields	= &omap_hwmod_sysc_type1,
 };
 
@@ -163,6 +164,7 @@ static struct omap_hwmod omap3xxx_timer1_hwmod = {
 	},
 	.dev_attr	= &capability_alwon_dev_attr,
 	.class		= &omap3xxx_timer_hwmod_class,
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer2 */
@@ -180,6 +182,7 @@ static struct omap_hwmod omap3xxx_timer2_hwmod = {
 		},
 	},
 	.class		= &omap3xxx_timer_hwmod_class,
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer3 */
@@ -197,6 +200,7 @@ static struct omap_hwmod omap3xxx_timer3_hwmod = {
 		},
 	},
 	.class		= &omap3xxx_timer_hwmod_class,
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer4 */
@@ -214,6 +218,7 @@ static struct omap_hwmod omap3xxx_timer4_hwmod = {
 		},
 	},
 	.class		= &omap3xxx_timer_hwmod_class,
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer5 */
@@ -231,6 +236,7 @@ static struct omap_hwmod omap3xxx_timer5_hwmod = {
 		},
 	},
 	.class		= &omap3xxx_timer_hwmod_class,
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer6 */
@@ -248,6 +254,7 @@ static struct omap_hwmod omap3xxx_timer6_hwmod = {
 		},
 	},
 	.class		= &omap3xxx_timer_hwmod_class,
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer7 */
@@ -265,6 +272,7 @@ static struct omap_hwmod omap3xxx_timer7_hwmod = {
 		},
 	},
 	.class		= &omap3xxx_timer_hwmod_class,
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer8 */
@@ -283,6 +291,7 @@ static struct omap_hwmod omap3xxx_timer8_hwmod = {
 	},
 	.dev_attr	= &capability_pwm_dev_attr,
 	.class		= &omap3xxx_timer_hwmod_class,
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer9 */
@@ -301,6 +310,7 @@ static struct omap_hwmod omap3xxx_timer9_hwmod = {
 	},
 	.dev_attr	= &capability_pwm_dev_attr,
 	.class		= &omap3xxx_timer_hwmod_class,
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer10 */
@@ -319,6 +329,7 @@ static struct omap_hwmod omap3xxx_timer10_hwmod = {
 	},
 	.dev_attr	= &capability_pwm_dev_attr,
 	.class		= &omap3xxx_timer_hwmod_class,
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer11 */
@@ -337,6 +348,7 @@ static struct omap_hwmod omap3xxx_timer11_hwmod = {
 	},
 	.dev_attr	= &capability_pwm_dev_attr,
 	.class		= &omap3xxx_timer_hwmod_class,
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /* timer12 */
@@ -360,6 +372,7 @@ static struct omap_hwmod omap3xxx_timer12_hwmod = {
 	},
 	.dev_attr	= &capability_secure_dev_attr,
 	.class		= &omap3xxx_timer_hwmod_class,
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 };
 
 /*
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 242aee4..e486ebd 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -2869,6 +2869,7 @@ static struct omap_hwmod_class_sysconfig omap44xx_timer_1ms_sysc = {
 			   SYSC_HAS_SIDLEMODE | SYSC_HAS_SOFTRESET |
 			   SYSS_HAS_RESET_STATUS),
 	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+	.clockact	= CLOCKACT_TEST_ICLK,
 	.sysc_fields	= &omap_hwmod_sysc_type1,
 };
 
@@ -2912,6 +2913,7 @@ static struct omap_hwmod omap44xx_timer1_hwmod = {
 	.name		= "timer1",
 	.class		= &omap44xx_timer_1ms_hwmod_class,
 	.clkdm_name	= "l4_wkup_clkdm",
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 	.mpu_irqs	= omap44xx_timer1_irqs,
 	.main_clk	= "timer1_fck",
 	.prcm = {
@@ -2934,6 +2936,7 @@ static struct omap_hwmod omap44xx_timer2_hwmod = {
 	.name		= "timer2",
 	.class		= &omap44xx_timer_1ms_hwmod_class,
 	.clkdm_name	= "l4_per_clkdm",
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 	.mpu_irqs	= omap44xx_timer2_irqs,
 	.main_clk	= "timer2_fck",
 	.prcm = {
@@ -3105,6 +3108,7 @@ static struct omap_hwmod omap44xx_timer10_hwmod = {
 	.name		= "timer10",
 	.class		= &omap44xx_timer_1ms_hwmod_class,
 	.clkdm_name	= "l4_per_clkdm",
+	.flags		= HWMOD_SET_DEFAULT_CLOCKACT,
 	.mpu_irqs	= omap44xx_timer10_irqs,
 	.main_clk	= "timer10_fck",
 	.prcm = {
diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index e24ee0f..1ee67a3 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -193,7 +193,6 @@ static int __init omap_dm_timer_init_one(struct omap_dm_timer *timer,
 		}
 	}
 	__omap_dm_timer_init_regs(timer);
-	__omap_dm_timer_reset(timer, 1, 1);
 	__omap_dm_timer_enable_posted(timer);
 
 	timer->rate = clk_get_rate(timer->fclk);
-- 
1.7.9.5

[PATCH 06/10] ARM: OMAP: Fix dmtimer reset for timer1

[Jon Hunter]

In commit e32f7ec2 (ARM: OMAP: Fix 32 kHz timer and modify GP timer to use GPT1)
a fix was added to prevent timer1 being reset in the function
omap_dm_timer_reset() because timer1 was being used as the system timer for
OMAP2 devices. Although timer1 is still used by most OMAP2+ devices as a system
timer, the function omap_dm_timer_reset() is now only being called for OMAP1
devices and OMAP1 does not use timer1 as a system timer. Therefore, remove the
check in omap_dm_timer_reset() so that timer1 is reset for OMAP1 devices.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/plat-omap/dmtimer.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 22790ea..623c1f3 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -122,10 +122,8 @@ static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
 
 static void omap_dm_timer_reset(struct omap_dm_timer *timer)
 {
-	if (timer->pdev->id != 1) {
-		omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG, 0x06);
-		omap_dm_timer_wait_for_reset(timer);
-	}
+	omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG, 0x06);
+	omap_dm_timer_wait_for_reset(timer);
 	__omap_dm_timer_reset(timer, 0, 0);
 }
 
-- 
1.7.9.5

[PATCH 07/10] ARM: OMAP: Clean-up dmtimer reset code

[Jon Hunter]

Only OMAP1 devices use the omap_dm_timer_reset() and so require the
omap_dm_timer_wait_for_reset() and __omap_dm_timer_reset() functions.
Therefore combine these into a single function called omap_dm_timer_reset()
and simplify the code.

Please note that for OMAP1 devices, the TIOCP_CFG register does not have the
clock-activity field and so when we reset the timer for an OMAP1 device we
only need to configure the idle-mode field in the TIOCP_CFG register.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/plat-omap/dmtimer.c              |   40 +++++++++++++++++------------
 arch/arm/plat-omap/include/plat/dmtimer.h |   19 --------------
 2 files changed, 24 insertions(+), 35 deletions(-)

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 623c1f3..1eb7353 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -43,6 +43,7 @@
 
 #include <plat/dmtimer.h>
 #include <plat/omap-pm.h>
+#include <plat/omap_hwmod.h>
 
 #include <mach/hardware.h>
 
@@ -103,32 +104,34 @@ static void omap_timer_restore_context(struct omap_dm_timer *timer)
 				timer->context.tclr);
 }
 
-static void omap_dm_timer_wait_for_reset(struct omap_dm_timer *timer)
+static int omap_dm_timer_reset(struct omap_dm_timer *timer)
 {
-	int c;
+	u32 l, timeout = 0;
 
 	if (!timer->sys_stat)
-		return;
+		return -EINVAL;
+
+	omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG, 0x06);
 
-	c = 0;
 	while (!(__raw_readl(timer->sys_stat) & 1)) {
-		c++;
-		if (c > 100000) {
-			printk(KERN_ERR "Timer failed to reset\n");
-			return;
+		if (timeout++ > 100000) {
+			dev_err(&timer->pdev->dev, "Timer failed to reset\n");
+			return -ETIMEDOUT;
 		}
 	}
-}
 
-static void omap_dm_timer_reset(struct omap_dm_timer *timer)
-{
-	omap_dm_timer_write_reg(timer, OMAP_TIMER_IF_CTRL_REG, 0x06);
-	omap_dm_timer_wait_for_reset(timer);
-	__omap_dm_timer_reset(timer, 0, 0);
+	/* Configure timer for smart-idle mode */
+	l = __raw_readl(timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET);
+	l |= __ffs(HWMOD_IDLEMODE_SMART) << SYSC_TYPE1_SIDLEMODE_SHIFT;
+	__raw_writel(l, timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET);
+
+	return 0;
 }
 
 int omap_dm_timer_prepare(struct omap_dm_timer *timer)
 {
+	int rc;
+
 	/*
 	 * FIXME: OMAP1 devices do not use the clock framework for dmtimers so
 	 * do not call clk_get() for these devices.
@@ -144,8 +147,13 @@ int omap_dm_timer_prepare(struct omap_dm_timer *timer)
 
 	omap_dm_timer_enable(timer);
 
-	if (timer->capability & OMAP_TIMER_NEEDS_RESET)
-		omap_dm_timer_reset(timer);
+	if (timer->capability & OMAP_TIMER_NEEDS_RESET) {
+		rc = omap_dm_timer_reset(timer);
+		if (rc) {
+			omap_dm_timer_disable(timer);
+			return rc;
+		}
+	}
 
 	__omap_dm_timer_enable_posted(timer);
 	omap_dm_timer_disable(timer);
diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h b/arch/arm/plat-omap/include/plat/dmtimer.h
index fa9d04b..6488a19 100644
--- a/arch/arm/plat-omap/include/plat/dmtimer.h
+++ b/arch/arm/plat-omap/include/plat/dmtimer.h
@@ -331,25 +331,6 @@ static inline void __omap_dm_timer_init_regs(struct omap_dm_timer *timer)
 	}
 }
 
-/* Assumes the source clock has been set by caller */
-static inline void __omap_dm_timer_reset(struct omap_dm_timer *timer,
-					int autoidle, int wakeup)
-{
-	u32 l;
-
-	l = __raw_readl(timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET);
-	l |= 0x02 << 3;  /* Set to smart-idle mode */
-	l |= 0x2 << 8;   /* Set clock activity to perserve f-clock on idle */
-
-	if (autoidle)
-		l |= 0x1 << 0;
-
-	if (wakeup)
-		l |= 1 << 2;
-
-	__raw_writel(l, timer->io_base + OMAP_TIMER_OCP_CFG_OFFSET);
-}
-
 static inline void __omap_dm_timer_enable_posted(struct omap_dm_timer *timer)
 {
 	if (timer->posted)
-- 
1.7.9.5

[PATCH 08/10] ARM: OMAP: Clean-up timer posted mode support

[Jon Hunter]

The dmtimer functions to read and write the dmtimer registers are currently
defined as follows ...

static inline u32 __omap_dm_timer_read(struct omap_dm_timer *timer, u32 reg,
						int posted);
static inline void __omap_dm_timer_write(struct omap_dm_timer *timer,
						u32 reg, u32 val, int posted);

The posted variable indicates if the timer is configured to use the posted mode
when performing register accesses. The posted mode configuration of the dmtimer
is stored in the omap_dm_timer structure that is also being passed to the above
functions and therefore we do not need to pass the posted variable separately.
Therefore, simplify the above functions by removing the posted variable as an
argument as this is not necessary.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/timer.c               |   14 +++++------
 arch/arm/plat-omap/dmtimer.c              |   10 ++++----
 arch/arm/plat-omap/include/plat/dmtimer.h |   37 +++++++++++++----------------
 3 files changed, 29 insertions(+), 32 deletions(-)

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index 1ee67a3..43da595 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -94,7 +94,7 @@ static int omap2_gp_timer_set_next_event(unsigned long cycles,
 					 struct clock_event_device *evt)
 {
 	__omap_dm_timer_load_start(&clkev, OMAP_TIMER_CTRL_ST,
-						0xffffffff - cycles, 1);
+						0xffffffff - cycles);
 
 	return 0;
 }
@@ -104,7 +104,7 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
 {
 	u32 period;
 
-	__omap_dm_timer_stop(&clkev, 1, clkev.rate);
+	__omap_dm_timer_stop(&clkev, clkev.rate);
 
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
@@ -112,10 +112,10 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode mode,
 		period -= 1;
 		/* Looks like we need to first set the load value separately */
 		__omap_dm_timer_write(&clkev, OMAP_TIMER_LOAD_REG,
-					0xffffffff - period, 1);
+					0xffffffff - period);
 		__omap_dm_timer_load_start(&clkev,
 					OMAP_TIMER_CTRL_AR | OMAP_TIMER_CTRL_ST,
-						0xffffffff - period, 1);
+						0xffffffff - period);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 		break;
@@ -246,7 +246,7 @@ static bool use_gptimer_clksrc;
  */
 static cycle_t clocksource_read_cycles(struct clocksource *cs)
 {
-	return (cycle_t)__omap_dm_timer_read_counter(&clksrc, 1);
+	return (cycle_t)__omap_dm_timer_read_counter(&clksrc);
 }
 
 static struct clocksource clocksource_gpt = {
@@ -260,7 +260,7 @@ static struct clocksource clocksource_gpt = {
 static u32 notrace dmtimer_read_sched_clock(void)
 {
 	if (clksrc.reserved)
-		return __omap_dm_timer_read_counter(&clksrc, 1);
+		return __omap_dm_timer_read_counter(&clksrc);
 
 	return 0;
 }
@@ -316,7 +316,7 @@ static void __init omap2_gptimer_clocksource_init(int gptimer_id,
 	BUG_ON(res);
 
 	__omap_dm_timer_load_start(&clksrc,
-			OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
+			OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0);
 	setup_sched_clock(dmtimer_read_sched_clock, 32, clksrc.rate);
 
 	if (clocksource_register_hz(&clocksource_gpt, clksrc.rate))
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 1eb7353..541adbb 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -63,7 +63,7 @@ static DEFINE_SPINLOCK(dm_timer_lock);
 static inline u32 omap_dm_timer_read_reg(struct omap_dm_timer *timer, u32 reg)
 {
 	WARN_ON((reg & 0xff) < _OMAP_TIMER_WAKEUP_EN_OFFSET);
-	return __omap_dm_timer_read(timer, reg, timer->posted);
+	return __omap_dm_timer_read(timer, reg);
 }
 
 /**
@@ -80,7 +80,7 @@ static void omap_dm_timer_write_reg(struct omap_dm_timer *timer, u32 reg,
 						u32 value)
 {
 	WARN_ON((reg & 0xff) < _OMAP_TIMER_WAKEUP_EN_OFFSET);
-	__omap_dm_timer_write(timer, reg, value, timer->posted);
+	__omap_dm_timer_write(timer, reg, value);
 }
 
 static void omap_timer_restore_context(struct omap_dm_timer *timer)
@@ -377,7 +377,7 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer)
 	if (!(timer->capability & OMAP_TIMER_NEEDS_RESET))
 		rate = clk_get_rate(timer->fclk);
 
-	__omap_dm_timer_stop(timer, timer->posted, rate);
+	__omap_dm_timer_stop(timer, rate);
 
 	if (!(timer->capability & OMAP_TIMER_ALWON))
 		timer->ctx_loss_count =
@@ -511,7 +511,7 @@ int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload,
 	}
 	l |= OMAP_TIMER_CTRL_ST;
 
-	__omap_dm_timer_load_start(timer, l, load, timer->posted);
+	__omap_dm_timer_load_start(timer, l, load);
 
 	/* Save the context */
 	timer->context.tclr = l;
@@ -646,7 +646,7 @@ unsigned int omap_dm_timer_read_counter(struct omap_dm_timer *timer)
 		return 0;
 	}
 
-	return __omap_dm_timer_read_counter(timer, timer->posted);
+	return __omap_dm_timer_read_counter(timer);
 }
 EXPORT_SYMBOL_GPL(omap_dm_timer_read_counter);
 
diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h b/arch/arm/plat-omap/include/plat/dmtimer.h
index 6488a19..b3150a3 100644
--- a/arch/arm/plat-omap/include/plat/dmtimer.h
+++ b/arch/arm/plat-omap/include/plat/dmtimer.h
@@ -283,20 +283,19 @@ struct omap_dm_timer {
 
 int omap_dm_timer_prepare(struct omap_dm_timer *timer);
 
-static inline u32 __omap_dm_timer_read(struct omap_dm_timer *timer, u32 reg,
-						int posted)
+static inline u32 __omap_dm_timer_read(struct omap_dm_timer *timer, u32 reg)
 {
-	if (posted)
+	if (timer->posted)
 		while (__raw_readl(timer->pend) & (reg >> WPSHIFT))
 			cpu_relax();
 
 	return __raw_readl(timer->func_base + (reg & 0xff));
 }
 
-static inline void __omap_dm_timer_write(struct omap_dm_timer *timer,
-					u32 reg, u32 val, int posted)
+static inline void
+__omap_dm_timer_write(struct omap_dm_timer *timer, u32 reg, u32 val)
 {
-	if (posted)
+	if (timer->posted)
 		while (__raw_readl(timer->pend) & (reg >> WPSHIFT))
 			cpu_relax();
 
@@ -340,7 +339,7 @@ static inline void __omap_dm_timer_enable_posted(struct omap_dm_timer *timer)
 		return;
 
 	__omap_dm_timer_write(timer, OMAP_TIMER_IF_CTRL_REG,
-					OMAP_TIMER_CTRL_POSTED, 0);
+					OMAP_TIMER_CTRL_POSTED);
 	timer->context.tsicr = OMAP_TIMER_CTRL_POSTED;
 	timer->posted = 1;
 }
@@ -387,18 +386,18 @@ static inline int __omap_dm_timer_set_source(struct clk *timer_fck,
 	return ret;
 }
 
-static inline void __omap_dm_timer_stop(struct omap_dm_timer *timer,
-					int posted, unsigned long rate)
+static inline void
+__omap_dm_timer_stop(struct omap_dm_timer *timer, unsigned long rate)
 {
 	u32 l;
 
-	l = __omap_dm_timer_read(timer, OMAP_TIMER_CTRL_REG, posted);
+	l = __omap_dm_timer_read(timer, OMAP_TIMER_CTRL_REG);
 	if (l & OMAP_TIMER_CTRL_ST) {
 		l &= ~0x1;
-		__omap_dm_timer_write(timer, OMAP_TIMER_CTRL_REG, l, posted);
+		__omap_dm_timer_write(timer, OMAP_TIMER_CTRL_REG, l);
 #ifdef CONFIG_ARCH_OMAP2PLUS
 		/* Readback to make sure write has completed */
-		__omap_dm_timer_read(timer, OMAP_TIMER_CTRL_REG, posted);
+		__omap_dm_timer_read(timer, OMAP_TIMER_CTRL_REG);
 		/*
 		 * Wait for functional clock period x 3.5 to make sure that
 		 * timer is stopped
@@ -412,24 +411,22 @@ static inline void __omap_dm_timer_stop(struct omap_dm_timer *timer,
 }
 
 static inline void __omap_dm_timer_load_start(struct omap_dm_timer *timer,
-						u32 ctrl, unsigned int load,
-						int posted)
+						u32 ctrl, unsigned int load)
 {
-	__omap_dm_timer_write(timer, OMAP_TIMER_COUNTER_REG, load, posted);
-	__omap_dm_timer_write(timer, OMAP_TIMER_CTRL_REG, ctrl, posted);
+	__omap_dm_timer_write(timer, OMAP_TIMER_COUNTER_REG, load);
+	__omap_dm_timer_write(timer, OMAP_TIMER_CTRL_REG, ctrl);
 }
 
 static inline void __omap_dm_timer_int_enable(struct omap_dm_timer *timer,
 						unsigned int value)
 {
 	__raw_writel(value, timer->irq_ena);
-	__omap_dm_timer_write(timer, OMAP_TIMER_WAKEUP_EN_REG, value, 0);
+	__omap_dm_timer_write(timer, OMAP_TIMER_WAKEUP_EN_REG, value);
 }
 
-static inline unsigned int
-__omap_dm_timer_read_counter(struct omap_dm_timer *timer, int posted)
+static inline u32 __omap_dm_timer_read_counter(struct omap_dm_timer *timer)
 {
-	return __omap_dm_timer_read(timer, OMAP_TIMER_COUNTER_REG, posted);
+	return __omap_dm_timer_read(timer, OMAP_TIMER_COUNTER_REG);
 }
 
 static inline void __omap_dm_timer_write_status(struct omap_dm_timer *timer,
-- 
1.7.9.5

[PATCH 08/10] ARM: OMAP: Clean-up timer posted mode support

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [120905 12:05]:
> The dmtimer functions to read and write the dmtimer registers are currently
> defined as follows ...
> 
> static inline u32 __omap_dm_timer_read(struct omap_dm_timer *timer, u32 reg,
> 						int posted);
> static inline void __omap_dm_timer_write(struct omap_dm_timer *timer,
> 						u32 reg, u32 val, int posted);
> 
> The posted variable indicates if the timer is configured to use the posted mode
> when performing register accesses. The posted mode configuration of the dmtimer
> is stored in the omap_dm_timer structure that is also being passed to the above
> functions and therefore we do not need to pass the posted variable separately.
> Therefore, simplify the above functions by removing the posted variable as an
> argument as this is not necessary.

I believe the reason for passing the posted flag was to optimize out some
functions from the timer code as that's being run all the time.

Care to check the assembly before and after this patch for the timer
functions with objdump -d to make sure it does not add tons of bloat
there?

Thanks,

Tony

[PATCH 08/10] ARM: OMAP: Clean-up timer posted mode support

[Jon Hunter]

On 09/07/2012 05:22 PM, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [120905 12:05]:
>> The dmtimer functions to read and write the dmtimer registers are currently
>> defined as follows ...
>>
>> static inline u32 __omap_dm_timer_read(struct omap_dm_timer *timer, u32 reg,
>> 						int posted);
>> static inline void __omap_dm_timer_write(struct omap_dm_timer *timer,
>> 						u32 reg, u32 val, int posted);
>>
>> The posted variable indicates if the timer is configured to use the posted mode
>> when performing register accesses. The posted mode configuration of the dmtimer
>> is stored in the omap_dm_timer structure that is also being passed to the above
>> functions and therefore we do not need to pass the posted variable separately.
>> Therefore, simplify the above functions by removing the posted variable as an
>> argument as this is not necessary.
> 
> I believe the reason for passing the posted flag was to optimize out some
> functions from the timer code as that's being run all the time.
> 
> Care to check the assembly before and after this patch for the timer
> functions with objdump -d to make sure it does not add tons of bloat
> there?

Hi Tony,

Thanks for the details here. I see that makes sense and that the
compiler could take advantage of this as the functions are inlined.

I have taken a look at the disassembled output using objdump as you
mentioned. What I see is ...

1. For dmtimer.c the impact appears negligible, the total number of
   lines outputted by objdump only changed by 8 with (1215 lines) and
   without (1207 lines) the patch applied.

2. For timer.c the impact is greater. I see that
   omap2_gp_timer_set_next_event() increased by 6 instructions from 29
   to 35. clocksource_read_cycles() increased by 2 instructions 15 to
   17 instructions. dmtimer_read_sched_clock() increased by 2
   instructions from 17 to 19. omap2_gp_timer_set_mode() increased by
   21 instructions from 102 to 123.

I imagine that we are mainly concerned about
omap2_gp_timer_set_next_event(), clocksource_read_cycles() and
dmtimer_read_sched_clock() as these will be called often. Therefore, I
am not sure if you wish to drop this patch.

By the way, if we do drop this patch, I would then need to fix the
setting of the posted variable in mach-omap2/timer.c for clock-source in
the case where a dmtimer is used. Today the code assumes that for
clock-source and clock-events posted mode is always used. However, with
the errata i103/i767 we will disable posted mode for clock-source on
omap2/3/4/5/am33xx devices.

Cheers
Jon

[PATCH 08/10] ARM: OMAP: Clean-up timer posted mode support

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [120910 15:00]:
> 
> On 09/07/2012 05:22 PM, Tony Lindgren wrote:
> > * Jon Hunter <jon-hunter@ti.com> [120905 12:05]:
> >> The dmtimer functions to read and write the dmtimer registers are currently
> >> defined as follows ...
> >>
> >> static inline u32 __omap_dm_timer_read(struct omap_dm_timer *timer, u32 reg,
> >> 						int posted);
> >> static inline void __omap_dm_timer_write(struct omap_dm_timer *timer,
> >> 						u32 reg, u32 val, int posted);
> >>
> >> The posted variable indicates if the timer is configured to use the posted mode
> >> when performing register accesses. The posted mode configuration of the dmtimer
> >> is stored in the omap_dm_timer structure that is also being passed to the above
> >> functions and therefore we do not need to pass the posted variable separately.
> >> Therefore, simplify the above functions by removing the posted variable as an
> >> argument as this is not necessary.
> > 
> > I believe the reason for passing the posted flag was to optimize out some
> > functions from the timer code as that's being run all the time.
> > 
> > Care to check the assembly before and after this patch for the timer
> > functions with objdump -d to make sure it does not add tons of bloat
> > there?
> 
> Hi Tony,
> 
> Thanks for the details here. I see that makes sense and that the
> compiler could take advantage of this as the functions are inlined.
> 
> I have taken a look at the disassembled output using objdump as you
> mentioned. What I see is ...
> 
> 1. For dmtimer.c the impact appears negligible, the total number of
>    lines outputted by objdump only changed by 8 with (1215 lines) and
>    without (1207 lines) the patch applied.
> 
> 2. For timer.c the impact is greater. I see that
>    omap2_gp_timer_set_next_event() increased by 6 instructions from 29
>    to 35. clocksource_read_cycles() increased by 2 instructions 15 to
>    17 instructions. dmtimer_read_sched_clock() increased by 2
>    instructions from 17 to 19. omap2_gp_timer_set_mode() increased by
>    21 instructions from 102 to 123.
> 
> I imagine that we are mainly concerned about
> omap2_gp_timer_set_next_event(), clocksource_read_cycles() and
> dmtimer_read_sched_clock() as these will be called often. Therefore, I
> am not sure if you wish to drop this patch.

Well does it at lots of new ldr to the critical code?
 
> By the way, if we do drop this patch, I would then need to fix the
> setting of the posted variable in mach-omap2/timer.c for clock-source in
> the case where a dmtimer is used. Today the code assumes that for
> clock-source and clock-events posted mode is always used. However, with
> the errata i103/i767 we will disable posted mode for clock-source on
> omap2/3/4/5/am33xx devices.

Yes I see, I guess that means just adding a new systimer entry?

Regards,

Tony

[PATCH 08/10] ARM: OMAP: Clean-up timer posted mode support

[Jon Hunter]

On 09/10/2012 07:58 PM, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [120910 15:00]:
>>
>> On 09/07/2012 05:22 PM, Tony Lindgren wrote:
>>> * Jon Hunter <jon-hunter@ti.com> [120905 12:05]:
>>>> The dmtimer functions to read and write the dmtimer registers are currently
>>>> defined as follows ...
>>>>
>>>> static inline u32 __omap_dm_timer_read(struct omap_dm_timer *timer, u32 reg,
>>>> 						int posted);
>>>> static inline void __omap_dm_timer_write(struct omap_dm_timer *timer,
>>>> 						u32 reg, u32 val, int posted);
>>>>
>>>> The posted variable indicates if the timer is configured to use the posted mode
>>>> when performing register accesses. The posted mode configuration of the dmtimer
>>>> is stored in the omap_dm_timer structure that is also being passed to the above
>>>> functions and therefore we do not need to pass the posted variable separately.
>>>> Therefore, simplify the above functions by removing the posted variable as an
>>>> argument as this is not necessary.
>>>
>>> I believe the reason for passing the posted flag was to optimize out some
>>> functions from the timer code as that's being run all the time.
>>>
>>> Care to check the assembly before and after this patch for the timer
>>> functions with objdump -d to make sure it does not add tons of bloat
>>> there?
>>
>> Hi Tony,
>>
>> Thanks for the details here. I see that makes sense and that the
>> compiler could take advantage of this as the functions are inlined.
>>
>> I have taken a look at the disassembled output using objdump as you
>> mentioned. What I see is ...
>>
>> 1. For dmtimer.c the impact appears negligible, the total number of
>>    lines outputted by objdump only changed by 8 with (1215 lines) and
>>    without (1207 lines) the patch applied.
>>
>> 2. For timer.c the impact is greater. I see that
>>    omap2_gp_timer_set_next_event() increased by 6 instructions from 29
>>    to 35. clocksource_read_cycles() increased by 2 instructions 15 to
>>    17 instructions. dmtimer_read_sched_clock() increased by 2
>>    instructions from 17 to 19. omap2_gp_timer_set_mode() increased by
>>    21 instructions from 102 to 123.
>>
>> I imagine that we are mainly concerned about
>> omap2_gp_timer_set_next_event(), clocksource_read_cycles() and
>> dmtimer_read_sched_clock() as these will be called often. Therefore, I
>> am not sure if you wish to drop this patch.
> 
> Well does it at lots of new ldr to the critical code?

For the omap2_gp_timer_set_next_event() function it adds 3 load
instructions, increasing the number of possible loads from 11 to 14.

For the clocksource_read_cycles() function it adds 1 load instruction,
increasing the number of possible loads from 7 to 8.

For the dmtimer_read_sched_clock() function, I don't see any additional
loads, but instructions added are a tst and beq instruction.

>> By the way, if we do drop this patch, I would then need to fix the
>> setting of the posted variable in mach-omap2/timer.c for clock-source in
>> the case where a dmtimer is used. Today the code assumes that for
>> clock-source and clock-events posted mode is always used. However, with
>> the errata i103/i767 we will disable posted mode for clock-source on
>> omap2/3/4/5/am33xx devices.
> 
> Yes I see, I guess that means just adding a new systimer entry?

Actually, I think we can avoid that by not using posted mode for
clock-source timers at all. Posted mode only benefits the clock-events
timers that are configured often.

The benefit of not using posted mode for clock-source timers and setting
the "posted" parameter to 0, will really allow the compiler to optimise
the clocksource_read_cycles() and dmtimer_read_sched_clock() quite a
bit. So this could be a nice optimisation.

Cheers
Jon

[PATCH 08/10] ARM: OMAP: Clean-up timer posted mode support

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [120911 09:26]:
> 
> On 09/10/2012 07:58 PM, Tony Lindgren wrote:
> > * Jon Hunter <jon-hunter@ti.com> [120910 15:00]:
> >>
> >> On 09/07/2012 05:22 PM, Tony Lindgren wrote:
> >>> * Jon Hunter <jon-hunter@ti.com> [120905 12:05]:
> >>>> The dmtimer functions to read and write the dmtimer registers are currently
> >>>> defined as follows ...
> >>>>
> >>>> static inline u32 __omap_dm_timer_read(struct omap_dm_timer *timer, u32 reg,
> >>>> 						int posted);
> >>>> static inline void __omap_dm_timer_write(struct omap_dm_timer *timer,
> >>>> 						u32 reg, u32 val, int posted);
> >>>>
> >>>> The posted variable indicates if the timer is configured to use the posted mode
> >>>> when performing register accesses. The posted mode configuration of the dmtimer
> >>>> is stored in the omap_dm_timer structure that is also being passed to the above
> >>>> functions and therefore we do not need to pass the posted variable separately.
> >>>> Therefore, simplify the above functions by removing the posted variable as an
> >>>> argument as this is not necessary.
> >>>
> >>> I believe the reason for passing the posted flag was to optimize out some
> >>> functions from the timer code as that's being run all the time.
> >>>
> >>> Care to check the assembly before and after this patch for the timer
> >>> functions with objdump -d to make sure it does not add tons of bloat
> >>> there?
> >>
> >> Hi Tony,
> >>
> >> Thanks for the details here. I see that makes sense and that the
> >> compiler could take advantage of this as the functions are inlined.
> >>
> >> I have taken a look at the disassembled output using objdump as you
> >> mentioned. What I see is ...
> >>
> >> 1. For dmtimer.c the impact appears negligible, the total number of
> >>    lines outputted by objdump only changed by 8 with (1215 lines) and
> >>    without (1207 lines) the patch applied.
> >>
> >> 2. For timer.c the impact is greater. I see that
> >>    omap2_gp_timer_set_next_event() increased by 6 instructions from 29
> >>    to 35. clocksource_read_cycles() increased by 2 instructions 15 to
> >>    17 instructions. dmtimer_read_sched_clock() increased by 2
> >>    instructions from 17 to 19. omap2_gp_timer_set_mode() increased by
> >>    21 instructions from 102 to 123.
> >>
> >> I imagine that we are mainly concerned about
> >> omap2_gp_timer_set_next_event(), clocksource_read_cycles() and
> >> dmtimer_read_sched_clock() as these will be called often. Therefore, I
> >> am not sure if you wish to drop this patch.
> > 
> > Well does it at lots of new ldr to the critical code?
> 
> For the omap2_gp_timer_set_next_event() function it adds 3 load
> instructions, increasing the number of possible loads from 11 to 14.
> 
> For the clocksource_read_cycles() function it adds 1 load instruction,
> increasing the number of possible loads from 7 to 8.
> 
> For the dmtimer_read_sched_clock() function, I don't see any additional
> loads, but instructions added are a tst and beq instruction.
> 
> >> By the way, if we do drop this patch, I would then need to fix the
> >> setting of the posted variable in mach-omap2/timer.c for clock-source in
> >> the case where a dmtimer is used. Today the code assumes that for
> >> clock-source and clock-events posted mode is always used. However, with
> >> the errata i103/i767 we will disable posted mode for clock-source on
> >> omap2/3/4/5/am33xx devices.
> > 
> > Yes I see, I guess that means just adding a new systimer entry?
> 
> Actually, I think we can avoid that by not using posted mode for
> clock-source timers at all. Posted mode only benefits the clock-events
> timers that are configured often.
> 
> The benefit of not using posted mode for clock-source timers and setting
> the "posted" parameter to 0, will really allow the compiler to optimise
> the clocksource_read_cycles() and dmtimer_read_sched_clock() quite a
> bit. So this could be a nice optimisation.

OK up to you, but maybe run some benchmarks first to figure out what
makes most sense? Updating the timer used to be a bottleneck earlier.

Regards,

Tony

[PATCH 08/10] ARM: OMAP: Clean-up timer posted mode support

[Jon Hunter]

On 09/11/2012 11:34 AM, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [120911 09:26]:
>>
>> On 09/10/2012 07:58 PM, Tony Lindgren wrote:
>>> * Jon Hunter <jon-hunter@ti.com> [120910 15:00]:
>>>>
>>>> On 09/07/2012 05:22 PM, Tony Lindgren wrote:
>>>>> * Jon Hunter <jon-hunter@ti.com> [120905 12:05]:
>>>>>> The dmtimer functions to read and write the dmtimer registers are currently
>>>>>> defined as follows ...
>>>>>>
>>>>>> static inline u32 __omap_dm_timer_read(struct omap_dm_timer *timer, u32 reg,
>>>>>> 						int posted);
>>>>>> static inline void __omap_dm_timer_write(struct omap_dm_timer *timer,
>>>>>> 						u32 reg, u32 val, int posted);
>>>>>>
>>>>>> The posted variable indicates if the timer is configured to use the posted mode
>>>>>> when performing register accesses. The posted mode configuration of the dmtimer
>>>>>> is stored in the omap_dm_timer structure that is also being passed to the above
>>>>>> functions and therefore we do not need to pass the posted variable separately.
>>>>>> Therefore, simplify the above functions by removing the posted variable as an
>>>>>> argument as this is not necessary.
>>>>>
>>>>> I believe the reason for passing the posted flag was to optimize out some
>>>>> functions from the timer code as that's being run all the time.
>>>>>
>>>>> Care to check the assembly before and after this patch for the timer
>>>>> functions with objdump -d to make sure it does not add tons of bloat
>>>>> there?
>>>>
>>>> Hi Tony,
>>>>
>>>> Thanks for the details here. I see that makes sense and that the
>>>> compiler could take advantage of this as the functions are inlined.
>>>>
>>>> I have taken a look at the disassembled output using objdump as you
>>>> mentioned. What I see is ...
>>>>
>>>> 1. For dmtimer.c the impact appears negligible, the total number of
>>>>    lines outputted by objdump only changed by 8 with (1215 lines) and
>>>>    without (1207 lines) the patch applied.
>>>>
>>>> 2. For timer.c the impact is greater. I see that
>>>>    omap2_gp_timer_set_next_event() increased by 6 instructions from 29
>>>>    to 35. clocksource_read_cycles() increased by 2 instructions 15 to
>>>>    17 instructions. dmtimer_read_sched_clock() increased by 2
>>>>    instructions from 17 to 19. omap2_gp_timer_set_mode() increased by
>>>>    21 instructions from 102 to 123.
>>>>
>>>> I imagine that we are mainly concerned about
>>>> omap2_gp_timer_set_next_event(), clocksource_read_cycles() and
>>>> dmtimer_read_sched_clock() as these will be called often. Therefore, I
>>>> am not sure if you wish to drop this patch.
>>>
>>> Well does it at lots of new ldr to the critical code?
>>
>> For the omap2_gp_timer_set_next_event() function it adds 3 load
>> instructions, increasing the number of possible loads from 11 to 14.
>>
>> For the clocksource_read_cycles() function it adds 1 load instruction,
>> increasing the number of possible loads from 7 to 8.
>>
>> For the dmtimer_read_sched_clock() function, I don't see any additional
>> loads, but instructions added are a tst and beq instruction.
>>
>>>> By the way, if we do drop this patch, I would then need to fix the
>>>> setting of the posted variable in mach-omap2/timer.c for clock-source in
>>>> the case where a dmtimer is used. Today the code assumes that for
>>>> clock-source and clock-events posted mode is always used. However, with
>>>> the errata i103/i767 we will disable posted mode for clock-source on
>>>> omap2/3/4/5/am33xx devices.
>>>
>>> Yes I see, I guess that means just adding a new systimer entry?
>>
>> Actually, I think we can avoid that by not using posted mode for
>> clock-source timers at all. Posted mode only benefits the clock-events
>> timers that are configured often.
>>
>> The benefit of not using posted mode for clock-source timers and setting
>> the "posted" parameter to 0, will really allow the compiler to optimise
>> the clocksource_read_cycles() and dmtimer_read_sched_clock() quite a
>> bit. So this could be a nice optimisation.
> 
> OK up to you, but maybe run some benchmarks first to figure out what
> makes most sense? Updating the timer used to be a bottleneck earlier.

Ok, let me re-work this. Thanks for the inputs.

Cheers
Jon

[PATCH 09/10] ARM: OMAP: Add dmtimer interrupt disable function

[Jon Hunter]

The OMAP dmtimer driver does not currently have a function to disable the
timer interrupts. For some timer instances the timer interrupt enable
function can be used to disable the interrupts because the same interrupt
enable register is used to disable interrupts. However, some timer instances
have separate interrupt enable/disable registers and so this will not work.
Therefore, add a dedicated function to disable interrupts.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/plat-omap/dmtimer.c              |   31 +++++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/dmtimer.h |    3 ++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 541adbb..3b548dc 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -612,6 +612,37 @@ int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer,
 }
 EXPORT_SYMBOL_GPL(omap_dm_timer_set_int_enable);
 
+/**
+ * omap_dm_timer_set_int_disable - disable timer interrupts
+ * @timer:	pointer to timer handle
+ * @mask:	bit mask of interrupts to be disabled
+ *
+ * Disables the specified timer interrupts for a timer.
+ */
+int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask)
+{
+	u32 l = mask;
+
+	if (unlikely(!timer))
+		return -EINVAL;
+
+	omap_dm_timer_enable(timer);
+
+	if (timer->revision == 1)
+		l = __raw_readl(timer->irq_ena) & ~mask;
+
+	__raw_writel(l, timer->irq_dis);
+	l = __omap_dm_timer_read(timer, OMAP_TIMER_WAKEUP_EN_REG) & ~mask;
+	__omap_dm_timer_write(timer, OMAP_TIMER_WAKEUP_EN_REG, l);
+
+	/* Save the context */
+	timer->context.tier &= ~mask;
+	timer->context.twer &= ~mask;
+	omap_dm_timer_disable(timer);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(omap_dm_timer_set_int_disable);
+
 unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer)
 {
 	unsigned int l;
diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h b/arch/arm/plat-omap/include/plat/dmtimer.h
index b3150a3..820f0ce 100644
--- a/arch/arm/plat-omap/include/plat/dmtimer.h
+++ b/arch/arm/plat-omap/include/plat/dmtimer.h
@@ -130,6 +130,7 @@ int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on, int toggle, i
 int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler);
 
 int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer, unsigned int value);
+int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask);
 
 unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer);
 int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value);
@@ -314,7 +315,7 @@ static inline void __omap_dm_timer_init_regs(struct omap_dm_timer *timer)
 				OMAP_TIMER_V1_SYS_STAT_OFFSET;
 		timer->irq_stat = timer->io_base + OMAP_TIMER_V1_STAT_OFFSET;
 		timer->irq_ena = timer->io_base + OMAP_TIMER_V1_INT_EN_OFFSET;
-		timer->irq_dis = NULL;
+		timer->irq_dis = timer->io_base + OMAP_TIMER_V1_INT_EN_OFFSET;
 		timer->pend = timer->io_base + _OMAP_TIMER_WRITE_PEND_OFFSET;
 		timer->func_base = timer->io_base;
 	} else {
-- 
1.7.9.5

[PATCH 09/10] ARM: OMAP: Add dmtimer interrupt disable function

[Vaibhav Hiremath]

On 9/6/2012 12:34 AM, Jon Hunter wrote:
> The OMAP dmtimer driver does not currently have a function to disable the
> timer interrupts. For some timer instances the timer interrupt enable
> function can be used to disable the interrupts because the same interrupt
> enable register is used to disable interrupts. However, some timer instances
> have separate interrupt enable/disable registers and so this will not work.
> Therefore, add a dedicated function to disable interrupts.
> 

I think you should clearly specify which timer and which device you are
referring to.

Thanks,
Vaibhav
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> ---
>  arch/arm/plat-omap/dmtimer.c              |   31 +++++++++++++++++++++++++++++
>  arch/arm/plat-omap/include/plat/dmtimer.h |    3 ++-
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> index 541adbb..3b548dc 100644
> --- a/arch/arm/plat-omap/dmtimer.c
> +++ b/arch/arm/plat-omap/dmtimer.c
> @@ -612,6 +612,37 @@ int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer,
>  }
>  EXPORT_SYMBOL_GPL(omap_dm_timer_set_int_enable);
>  
> +/**
> + * omap_dm_timer_set_int_disable - disable timer interrupts
> + * @timer:	pointer to timer handle
> + * @mask:	bit mask of interrupts to be disabled
> + *
> + * Disables the specified timer interrupts for a timer.
> + */
> +int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask)
> +{
> +	u32 l = mask;
> +
> +	if (unlikely(!timer))
> +		return -EINVAL;
> +
> +	omap_dm_timer_enable(timer);
> +
> +	if (timer->revision == 1)
> +		l = __raw_readl(timer->irq_ena) & ~mask;
> +
> +	__raw_writel(l, timer->irq_dis);
> +	l = __omap_dm_timer_read(timer, OMAP_TIMER_WAKEUP_EN_REG) & ~mask;
> +	__omap_dm_timer_write(timer, OMAP_TIMER_WAKEUP_EN_REG, l);
> +
> +	/* Save the context */
> +	timer->context.tier &= ~mask;
> +	timer->context.twer &= ~mask;
> +	omap_dm_timer_disable(timer);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(omap_dm_timer_set_int_disable);
> +
>  unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer)
>  {
>  	unsigned int l;
> diff --git a/arch/arm/plat-omap/include/plat/dmtimer.h b/arch/arm/plat-omap/include/plat/dmtimer.h
> index b3150a3..820f0ce 100644
> --- a/arch/arm/plat-omap/include/plat/dmtimer.h
> +++ b/arch/arm/plat-omap/include/plat/dmtimer.h
> @@ -130,6 +130,7 @@ int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on, int toggle, i
>  int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer, int prescaler);
>  
>  int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer, unsigned int value);
> +int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask);
>  
>  unsigned int omap_dm_timer_read_status(struct omap_dm_timer *timer);
>  int omap_dm_timer_write_status(struct omap_dm_timer *timer, unsigned int value);
> @@ -314,7 +315,7 @@ static inline void __omap_dm_timer_init_regs(struct omap_dm_timer *timer)
>  				OMAP_TIMER_V1_SYS_STAT_OFFSET;
>  		timer->irq_stat = timer->io_base + OMAP_TIMER_V1_STAT_OFFSET;
>  		timer->irq_ena = timer->io_base + OMAP_TIMER_V1_INT_EN_OFFSET;
> -		timer->irq_dis = NULL;
> +		timer->irq_dis = timer->io_base + OMAP_TIMER_V1_INT_EN_OFFSET;
>  		timer->pend = timer->io_base + _OMAP_TIMER_WRITE_PEND_OFFSET;
>  		timer->func_base = timer->io_base;
>  	} else {
> 

[PATCH 09/10] ARM: OMAP: Add dmtimer interrupt disable function

[Jon Hunter]

On 09/06/2012 07:58 AM, Vaibhav Hiremath wrote:
> 
> 
> On 9/6/2012 12:34 AM, Jon Hunter wrote:
>> The OMAP dmtimer driver does not currently have a function to disable the
>> timer interrupts. For some timer instances the timer interrupt enable
>> function can be used to disable the interrupts because the same interrupt
>> enable register is used to disable interrupts. However, some timer instances
>> have separate interrupt enable/disable registers and so this will not work.
>> Therefore, add a dedicated function to disable interrupts.
>>
> 
> I think you should clearly specify which timer and which device you are
> referring to.

OMAP4+ devices. For OMAP4 all timers apart from 1, 2 and 10. For OMAP5
all timers.

Jon

[PATCH 10/10] ARM: OMAP: Remove unnecessary call to clk_get()

[Jon Hunter]

Whenever we call the function omap_dm_timer_set_source() to set the clock
source of a dmtimer we look-up the dmtimer functional clock source by
calling clk_get(). This is not necessary because on requesting a dmtimer
we look-up the functional clock source and store it in the omap_dm_timer
structure. So instead of looking up the clock again used the clock handle
that stored in the omap_dm_timer structure.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/plat-omap/dmtimer.c |   14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 3b548dc..a4cc122 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -400,7 +400,7 @@ int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
 {
 	int ret;
 	char *parent_name = NULL;
-	struct clk *fclk, *parent;
+	struct clk *parent;
 	struct dmtimer_platform_data *pdata;
 
 	if (unlikely(!timer))
@@ -419,11 +419,8 @@ int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
 	if (pdata->set_timer_src)
 		return pdata->set_timer_src(timer->pdev, source);
 
-	fclk = clk_get(&timer->pdev->dev, "fck");
-	if (IS_ERR_OR_NULL(fclk)) {
-		pr_err("%s: fck not found\n", __func__);
+	if (!timer->fclk)
 		return -EINVAL;
-	}
 
 	switch (source) {
 	case OMAP_TIMER_SRC_SYS_CLK:
@@ -442,18 +439,15 @@ int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
 	parent = clk_get(&timer->pdev->dev, parent_name);
 	if (IS_ERR_OR_NULL(parent)) {
 		pr_err("%s: %s not found\n", __func__, parent_name);
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
-	ret = clk_set_parent(fclk, parent);
+	ret = clk_set_parent(timer->fclk, parent);
 	if (IS_ERR_VALUE(ret))
 		pr_err("%s: failed to set %s as parent\n", __func__,
 			parent_name);
 
 	clk_put(parent);
-out:
-	clk_put(fclk);
 
 	return ret;
 }
-- 
1.7.9.5

[PATCH 00/10] ARM: OMAP: DMTIMER fixes and clean-up

[Vaibhav Hiremath]

On 9/6/2012 12:34 AM, Jon Hunter wrote:
> This series includes several fixes for the OMAP DMTIMER driver and a few
> clean-ups to simplify some of the code. This series is based upon 3.6-rc4.
> 
> Tested on OMAP5912 OSK, OMAP2420 H4, OMAP3430 Beagle and OMAP4430 Panda.
> Testing includes ...
> 1. Booting kernel on above boards
> 2. Set date and ensuring time of day is correct after 24 hours
> 3. Checking the timer counter is incrementing when configuring and starting
>    a timer
> 4. Checking the timer overflow interrupt when timer expires.
> 5. Using different clock sources to operate the timer with.
> 6. Running a loop test overnight that continually runs test #3 and #4 for
>    each available timer
> 

On the other hand I have validated this patch-series on both AM37xEVM
and BeagleBone platform. I used the test code which you shared with me
to validate it.

AM33XX Clock tree needs minor change here,

diff --git a/arch/arm/mach-omap2/clock33xx_data.c
b/arch/arm/mach-omap2/clock33xx_data.c
index a4006b2..daa36a5 100644
--- a/arch/arm/mach-omap2/clock33xx_data.c
+++ b/arch/arm/mach-omap2/clock33xx_data.c
@@ -1070,6 +1070,8 @@ static struct omap_clk am33xx_clks[] = {
        CLK(NULL,       "gfx_fck_div_ck",       &gfx_fck_div_ck,
CK_AM33XX),
        CLK(NULL,       "sysclkout_pre_ck",     &sysclkout_pre_ck,
CK_AM33XX),
        CLK(NULL,       "clkout2_ck",           &clkout2_ck,    CK_AM33XX),
+       CLK(NULL,       "timer_32k_ck",         &clkdiv32k_ick, CK_AM33XX),
+       CLK(NULL,       "timer_sys_ck",         &sys_clkin_ck,  CK_AM33XX),
 };

 int __init am33xx_clk_init(void)


Thanks,
Vaibhav

> Jon Hunter (10):
>   ARM: OMAP3+: Implement timer workaround for errata i103 and i767
>   ARM: OMAP: Fix timer posted mode support
>   ARM: OMAP3: Correct HWMOD DMTIMER SYSC register declarations
>   ARM: OMAP2/3: Define HWMOD software reset status for DMTIMERs
>   ARM: OMAP2+: Don't use __omap_dm_timer_reset()
>   ARM: OMAP: Fix dmtimer reset for timer1
>   ARM: OMAP: Clean-up dmtimer reset code
>   ARM: OMAP: Clean-up timer posted mode support
>   ARM: OMAP: Add dmtimer interrupt disable function
>   ARM: OMAP: Remove unnecessary call to clk_get()
> 
>  arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c |   15 ++-
>  arch/arm/mach-omap2/omap_hwmod_3xxx_data.c         |   41 ++++----
>  arch/arm/mach-omap2/omap_hwmod_44xx_data.c         |    4 +
>  arch/arm/mach-omap2/timer.c                        |   27 +++--
>  arch/arm/plat-omap/dmtimer.c                       |  109 +++++++++++++-------
>  arch/arm/plat-omap/include/plat/dmtimer.h          |   95 ++++++++++-------
>  6 files changed, 184 insertions(+), 107 deletions(-)
> 

[PATCH 00/10] ARM: OMAP: DMTIMER fixes and clean-up

[Jon Hunter]

On 09/06/2012 07:58 AM, Vaibhav Hiremath wrote:
> 
> 
> On 9/6/2012 12:34 AM, Jon Hunter wrote:
>> This series includes several fixes for the OMAP DMTIMER driver and a few
>> clean-ups to simplify some of the code. This series is based upon 3.6-rc4.
>>
>> Tested on OMAP5912 OSK, OMAP2420 H4, OMAP3430 Beagle and OMAP4430 Panda.
>> Testing includes ...
>> 1. Booting kernel on above boards
>> 2. Set date and ensuring time of day is correct after 24 hours
>> 3. Checking the timer counter is incrementing when configuring and starting
>>    a timer
>> 4. Checking the timer overflow interrupt when timer expires.
>> 5. Using different clock sources to operate the timer with.
>> 6. Running a loop test overnight that continually runs test #3 and #4 for
>>    each available timer
>>
> 
> On the other hand I have validated this patch-series on both AM37xEVM
> and BeagleBone platform. I used the test code which you shared with me
> to validate it.

Thanks!

> AM33XX Clock tree needs minor change here,
> 
> diff --git a/arch/arm/mach-omap2/clock33xx_data.c
> b/arch/arm/mach-omap2/clock33xx_data.c
> index a4006b2..daa36a5 100644
> --- a/arch/arm/mach-omap2/clock33xx_data.c
> +++ b/arch/arm/mach-omap2/clock33xx_data.c
> @@ -1070,6 +1070,8 @@ static struct omap_clk am33xx_clks[] = {
>         CLK(NULL,       "gfx_fck_div_ck",       &gfx_fck_div_ck,
> CK_AM33XX),
>         CLK(NULL,       "sysclkout_pre_ck",     &sysclkout_pre_ck,
> CK_AM33XX),
>         CLK(NULL,       "clkout2_ck",           &clkout2_ck,    CK_AM33XX),
> +       CLK(NULL,       "timer_32k_ck",         &clkdiv32k_ick, CK_AM33XX),
> +       CLK(NULL,       "timer_sys_ck",         &sys_clkin_ck,  CK_AM33XX),
>  };
> 
>  int __init am33xx_clk_init(void)

Ok, do you plan to submit this? If I need to do a V2 of this series, I
can include this one too.

Cheers
Jon