ARM: OMAP: DMTIMER fixes and clean-up

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 <[email protected]>

---
 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(+)

arch/arm/mach-omap2/timer.c +0 -0

index 2ff6d41..5471706 100644

arch/arm/mach-omap2/timer.c +2 -0

 	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);
 

arch/arm/plat-omap/dmtimer.c +2 -0

index 938b50a..c34f55b 100644
@@ -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);

arch/arm/plat-omap/include/plat/dmtimer.h +39 -0

index 19e7fa5..5ce2f00 100644
@@ -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)
 {

Quoted 33 lines · 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.

Quoted 6 lines

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.

Quoted 12 lines

Signed-off-by Jon Hunter <[email protected]>

---
 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(+)

arch/arm/mach-omap2/timer.c +0 -0

index 2ff6d41..5471706 100644

arch/arm/mach-omap2/timer.c static void __init omap2_gp_clockevent_init(int gptimer_id, · line 217

Quoted 10 lines

arch/arm/mach-omap2/timer.c +7 -0

@@ -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?

Quoted 70 lines · Vaibhav Hiremath

Quoted 33 lines · 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.

Quoted 6 lines

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.

Quoted 12 lines

Signed-off-by Jon Hunter <[email protected]>

---
 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(+)

arch/arm/mach-omap2/timer.c +0 -0

index 2ff6d41..5471706 100644

arch/arm/mach-omap2/timer.c static void __init omap2_gp_clockevent_init(int gptimer_id, · line 217

Quoted 10 lines

arch/arm/mach-omap2/timer.c +7 -0

@@ -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,

Quoted 4 lines

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.

Quoted 77 lines · Jon Hunter

Quoted 70 lines · Vaibhav Hiremath

Quoted 33 lines · 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.

Quoted 6 lines

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.

Quoted 12 lines

Signed-off-by Jon Hunter <[email protected]>

---
 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(+)

arch/arm/mach-omap2/timer.c +0 -0

index 2ff6d41..5471706 100644

arch/arm/mach-omap2/timer.c static void __init omap2_gp_clockevent_init(int gptimer_id, · line 217

Quoted 10 lines

arch/arm/mach-omap2/timer.c +7 -0

@@ -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,

Quoted 4 lines

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.

Quoted 8 lines

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 ;-)

Quoted 88 lines · Jon Hunter

Quoted 85 lines · Jon Hunter

Quoted 70 lines · Vaibhav Hiremath

Quoted 33 lines · 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.

Quoted 6 lines

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.

Quoted 12 lines

Signed-off-by Jon Hunter <[email protected]>

---
 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(+)

arch/arm/mach-omap2/timer.c +0 -0

index 2ff6d41..5471706 100644

arch/arm/mach-omap2/timer.c static void __init omap2_gp_clockevent_init(int gptimer_id, · line 217

Quoted 10 lines

arch/arm/mach-omap2/timer.c +7 -0

@@ -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,

Quoted 4 lines

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.

Quoted 5 lines

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

arch/arm/mach-omap2/timer.c +3 -1

index 43da595..f59e797 100644
@@ -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

Quoted 70 lines · Vaibhav Hiremath

Quoted 33 lines · 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.

Quoted 6 lines

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.

Quoted 12 lines

Signed-off-by Jon Hunter <[email protected]>

---
 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(+)

arch/arm/mach-omap2/timer.c +0 -0

index 2ff6d41..5471706 100644

arch/arm/mach-omap2/timer.c static void __init omap2_gp_clockevent_init(int gptimer_id, · line 217

Quoted 10 lines

arch/arm/mach-omap2/timer.c +7 -0

@@ -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,

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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

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 <[email protected]>

---
 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(-)

arch/arm/mach-omap2/timer.c +1 -2

index 5471706..e24ee0f 100644
@@ -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;

arch/arm/plat-omap/dmtimer.c +2 -6

index c34f55b..22790ea 100644
@@ -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);
+

arch/arm/plat-omap/dmtimer.c +0 -0

 }
 
 static inline u32 omap_dm_timer_reserved_systimer(int id)

arch/arm/plat-omap/include/plat/dmtimer.h +8 -1

index 5ce2f00..fa9d04b 100644
@@ -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;
 }
 
 /**

Quoted 74 lines · 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 <[email protected]>

---
 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(-)

arch/arm/mach-omap2/timer.c +1 -2

index 5471706..e24ee0f 100644
@@ -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;

arch/arm/plat-omap/dmtimer.c +4 -7

index c34f55b..22790ea 100644
@@ -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);
 

arch/arm/plat-omap/dmtimer.c int omap_dm_timer_prepare(struct omap_dm_timer *timer) · line 155

Quoted 3 lines

arch/arm/plat-omap/dmtimer.c +1 -2

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

Quoted 77 lines · Vaibhav Hiremath

Quoted 74 lines · 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 <[email protected]>

---
 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(-)

arch/arm/mach-omap2/timer.c +1 -2

index 5471706..e24ee0f 100644
@@ -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;

arch/arm/plat-omap/dmtimer.c +4 -7

index c34f55b..22790ea 100644
@@ -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);
 

Quoted 6 lines

arch/arm/plat-omap/dmtimer.c int omap_dm_timer_prepare(struct omap_dm_timer *timer) · line 155

Quoted 3 lines

arch/arm/plat-omap/dmtimer.c +1 -2

-	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 :-)

Quoted 86 lines · Jon Hunter

Quoted 83 lines · Vaibhav Hiremath

Quoted 67 lines · 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 <[email protected]>

---
 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(-)

arch/arm/mach-omap2/timer.c +1 -2

index 5471706..e24ee0f 100644
@@ -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;

arch/arm/plat-omap/dmtimer.c +2 -6

index c34f55b..22790ea 100644
@@ -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);
+

arch/arm/plat-omap/dmtimer.c int omap_dm_timer_prepare(struct omap_dm_timer *timer) · line 155

Quoted 10 lines

arch/arm/plat-omap/dmtimer.c +3 -3

 	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.

Quoted 3 lines

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.

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.

Quoted 8 lines · 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);

Quoted 6 lines

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?

Quoted 16 lines · Tony Lindgren

Quoted 8 lines · 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);

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 44 lines · Jon Hunter

Quoted 16 lines · Tony Lindgren

Quoted 8 lines · 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);

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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?

Quoted 6 lines

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?

Quoted 45 lines · Tony Lindgren

Quoted 44 lines · Jon Hunter

Quoted 16 lines · Tony Lindgren

Quoted 8 lines · 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);

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 6 lines

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.

Quoted 76 lines · Jon Hunter

Quoted 45 lines · Tony Lindgren

Quoted 44 lines · Jon Hunter

Quoted 16 lines · Tony Lindgren

Quoted 8 lines · 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);

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 8 lines

Quoted 6 lines

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.

Quoted 4 lines

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.

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 <[email protected]>

---
 arch/arm/plat-omap/dmtimer.c              |   31 +++++++++++++++++++++++++++++
 arch/arm/plat-omap/include/plat/dmtimer.h |    3 ++-
 2 files changed, 33 insertions(+), 1 deletion(-)

arch/arm/plat-omap/dmtimer.c +31 -0

index 541adbb..3b548dc 100644
@@ -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;

arch/arm/plat-omap/include/plat/dmtimer.h +2 -1

index b3150a3..820f0ce 100644
@@ -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 {

Quoted 7 lines · 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.

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

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(-)

Quoted 14 lines · 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

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,

arch/arm/mach-omap2/clock33xx_data.c +2 -0

index a4006b2..daa36a5 100644
@@ -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)