[PATCH 0/2] clocksource: timer-ti-dm: Misc fixes for omap dm timer

[PATCH 0/2] clocksource: timer-ti-dm: Misc fixes for omap dm timer

[Lokesh Vutla]

This series fixes timer enabling sequence and pwm period updating
sequence.

Lokesh Vutla (2):
  clocksource: timer-ti-dm: Do not restore context on every timer enable
  clocksource: timer-ti-dm: Do not update counter on updating the period

 drivers/clocksource/timer-ti-dm.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

-- 
2.23.0

[PATCH 1/2] clocksource: timer-ti-dm: Do not restore context on every timer enable

[Lokesh Vutla]

omap_dm_timer_enable() restores the entire context(including counter)
based on 2 conditions:
- If get_context_loss_count is populated and context is lost.
- If get_context_loss_count is not populated update unconditionally.

Case2 has a side effect of updating the counter register even though
context is not lost. When timer is configured in pwm mode, this is
causing undesired behaviour in the pwm period. So restore context only
if get_context_loss_count is populated and context is actually lost.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/clocksource/timer-ti-dm.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/timer-ti-dm.c b/drivers/clocksource/timer-ti-dm.c
index 269a994d6a99..40742715ed21 100644
--- a/drivers/clocksource/timer-ti-dm.c
+++ b/drivers/clocksource/timer-ti-dm.c
@@ -229,15 +229,12 @@ static void omap_dm_timer_enable(struct omap_dm_timer *timer)
 
 	pm_runtime_get_sync(&timer->pdev->dev);
 
-	if (!(timer->capability & OMAP_TIMER_ALWON)) {
-		if (timer->get_context_loss_count) {
-			c = timer->get_context_loss_count(&timer->pdev->dev);
-			if (c != timer->ctx_loss_count) {
-				omap_timer_restore_context(timer);
-				timer->ctx_loss_count = c;
-			}
-		} else {
+	if (!(timer->capability & OMAP_TIMER_ALWON) &&
+	    timer->get_context_loss_count) {
+		c = timer->get_context_loss_count(&timer->pdev->dev);
+		if (c != timer->ctx_loss_count) {
 			omap_timer_restore_context(timer);
+			timer->ctx_loss_count = c;
 		}
 	}
 }
-- 
2.23.0

[PATCH 2/2] clocksource: timer-ti-dm: Do not update counter on updating the period

[Lokesh Vutla]

Write to trigger register(OMAP_TIMER_TRIGGER_REG) will load the value
in Load register(OMAP_TIMER_LOAD_REG) into Counter register
(OMAP_TIMER_COUNTER_REG).

omap_dm_timer_set_load() writes into trigger register every time load
register is updated. When timer is configured in pwm mode, this causes
disruption in current pwm cycle, which is not expected especially when
pwm is used as PPS signal for synchronized PTP clocks. So do not write
into trigger register on updating the period.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
---
 drivers/clocksource/timer-ti-dm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/clocksource/timer-ti-dm.c b/drivers/clocksource/timer-ti-dm.c
index 40742715ed21..62b145ef3bb8 100644
--- a/drivers/clocksource/timer-ti-dm.c
+++ b/drivers/clocksource/timer-ti-dm.c
@@ -574,7 +574,6 @@ static int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload,
 	omap_dm_timer_write_reg(timer, OMAP_TIMER_CTRL_REG, l);
 	omap_dm_timer_write_reg(timer, OMAP_TIMER_LOAD_REG, load);
 
-	omap_dm_timer_write_reg(timer, OMAP_TIMER_TRIGGER_REG, 0);
 	/* Save the context */
 	timer->context.tclr = l;
 	timer->context.tldr = load;
-- 
2.23.0

Re: [PATCH 1/2] clocksource: timer-ti-dm: Do not restore context on every timer enable

[Tony Lindgren]

* Lokesh Vutla <lokeshvutla@ti.com> [200224 05:09]:
> omap_dm_timer_enable() restores the entire context(including counter)
> based on 2 conditions:
> - If get_context_loss_count is populated and context is lost.
> - If get_context_loss_count is not populated update unconditionally.
> 
> Case2 has a side effect of updating the counter register even though
> context is not lost. When timer is configured in pwm mode, this is
> causing undesired behaviour in the pwm period. So restore context only
> if get_context_loss_count is populated and context is actually lost.

Sounds like this will break things. We have get_context_loss_count()
only on omap4 and later, and even that was only partially implemented
from what I recall.

To mee it seems the right thing to do here is to save and restore
context on CPU_CLUSTER_PM_ENTER and CPU_CLUSTER_PM_EXIT. And I'd
just get rid of the partially implemented custom calls to
get_context_loss_count().

See for example a recent patch on removing the legacy SDMA code
for CPU_CLUSTER_PM_ENTER in commit d2f924879e19 ("thermal:
ti-soc-thermal:  Enable addition power management").

Then if we really need to add checks for context lost at some point,
that can be implemented via reset_control_status() returning -ENOLINK
or something similar.

Regards,

Tony

Re: [PATCH 2/2] clocksource: timer-ti-dm: Do not update counter on updating the period

[Tony Lindgren]

* Lokesh Vutla <lokeshvutla@ti.com> [200224 05:09]:
> Write to trigger register(OMAP_TIMER_TRIGGER_REG) will load the value
> in Load register(OMAP_TIMER_LOAD_REG) into Counter register
> (OMAP_TIMER_COUNTER_REG).
> 
> omap_dm_timer_set_load() writes into trigger register every time load
> register is updated. When timer is configured in pwm mode, this causes
> disruption in current pwm cycle, which is not expected especially when
> pwm is used as PPS signal for synchronized PTP clocks. So do not write
> into trigger register on updating the period.

This patch without patch 1/2 applied still works for me:

Tested-by: Tony Lindgren <tony@atomide.com>

[tip: timers/core] clocksource/drivers/timer-ti-dm: Do not update counter on updating the period

[tip-bot2 for Lokesh Vutla]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     6ce4fcb015a1a1290ffafcf3554901b40f9322df
Gitweb:        https://git.kernel.org/tip/6ce4fcb015a1a1290ffafcf3554901b40f9322df
Author:        Lokesh Vutla <lokeshvutla@ti.com>
AuthorDate:    Mon, 24 Feb 2020 10:37:53 +05:30
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Thu, 27 Feb 2020 10:26:23 +01:00

clocksource/drivers/timer-ti-dm: Do not update counter on updating the period

Write to trigger register(OMAP_TIMER_TRIGGER_REG) will load the value
in Load register(OMAP_TIMER_LOAD_REG) into Counter register
(OMAP_TIMER_COUNTER_REG).

omap_dm_timer_set_load() writes into trigger register every time load
register is updated. When timer is configured in pwm mode, this causes
disruption in current pwm cycle, which is not expected especially when
pwm is used as PPS signal for synchronized PTP clocks. So do not write
into trigger register on updating the period.

Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
Tested-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20200224050753.17784-3-lokeshvutla@ti.com
---
 drivers/clocksource/timer-ti-dm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/clocksource/timer-ti-dm.c b/drivers/clocksource/timer-ti-dm.c
index 269a994..acc9360 100644
--- a/drivers/clocksource/timer-ti-dm.c
+++ b/drivers/clocksource/timer-ti-dm.c
@@ -577,7 +577,6 @@ static int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload,
 	omap_dm_timer_write_reg(timer, OMAP_TIMER_CTRL_REG, l);
 	omap_dm_timer_write_reg(timer, OMAP_TIMER_LOAD_REG, load);
 
-	omap_dm_timer_write_reg(timer, OMAP_TIMER_TRIGGER_REG, 0);
 	/* Save the context */
 	timer->context.tclr = l;
 	timer->context.tldr = load;