Re: [PATCH] OMAP: add pwm driver using dmtimers.

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 5005 bytes --]

On Thu, 13 Dec 2012 11:07:25 -0600 Jon Hunter <jon-hunter@ti.com> wrote:

> 
> On 12/12/2012 09:06 PM, NeilBrown wrote:
> > 

> >>> +
> >>> +#if CONFIG_PM
> >>> +static int omap_pwm_suspend(struct platform_device *pdev, pm_message_t state)
> >>> +{
> >>> +	struct omap_chip *omap = platform_get_drvdata(pdev);
> >>> +	/* No one preserve these values during suspend so reset them
> >>> +	 * Otherwise driver leaves PWM unconfigured if same values
> >>> +	 * passed to pwm_config
> >>> +	 */
> >>> +	omap->period_ns = 0;
> >>> +	omap->duty_ns = 0;
> >>
> >>
> >> Hmmm, looks like you are trying to force a reconfiguration after suspend
> >> if the same values are used. Is there an underlying problem here that
> >> you are trying to workaround?
> > 
> > I copied that from pwm-samsung.c.
> > 
> > The key question is: does a dmtimer preserve all register values over suspend.
> > If so, then I guess we don't need this.
> > If not, we do (because omap_pwm_config short circuits if it thinks the config
> > hasn't changed).
> 
> I gave it a quick test on omap3/4 when just operating the timer as a
> counter (not driving a pwm output) and suspend/resume works fine.
> However, it does not work if I enable off mode (via the debugfs). This
> is not enabled by default and may be I should put that on my to-do list
> as well.

I've been playing with off-mode and discovered that the first attempt to set
the PWM after resume didn't work, but subsequent ones did.
I did some digging and came up with the following patch.  
With this in place, the omap_pwm_suspend() above is definitely pointless (was
wasn't really useful even without it).

NeilBrown


From: NeilBrown <neilb@suse.de>
Date: Mon, 7 Jan 2013 07:53:03 +1100
Subject: [PATCH] OMAP dmtimer - simplify context-loss handling.

The context loss handling in dmtimer appears to assume that
   omap_dm_timer_set_load_start() or omap_dm_timer_start()
and
   omap_dm_timer_stop()

bracket all interactions.  Only the first two restore the context and
the last updates the context loss counter.
However omap_dm_timer_set_load() or omap_dm_timer_set_match() can
reasonably be called outside this bracketing, and the fact that they
call omap_dm_timer_enable() / omap_dm_timer_disable() suggest that
is expected.

So if, after a transition into and out of off-mode which would cause
the dm timer to loose all state, omap_dm_timer_set_match() is called
before omap_dm_timer_start(), the value read from OMAP_TIMER_CTRL_REG
will be 'wrong' and this wrong value will be stored context.tclr so
a subsequent omap_dm_timer_start() can fail (As the control register
is wrong).

Simplify this be doing the restore-from-context in
omap_dm_timer_enable() so that whenever the timer is enabled, the
context is correct.
Also update the ctx_loss_count at the same time as we notice it is
wrong - these is no value in delaying this until the
omap_dm_timer_disable() as it cannot change while the timer is
enabled.

Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 938b50a..c216696 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -253,6 +253,15 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
 void omap_dm_timer_enable(struct omap_dm_timer *timer)
 {
 	pm_runtime_get_sync(&timer->pdev->dev);
+
+	if (!(timer->capability & OMAP_TIMER_ALWON)) {
+		int loss_count =
+			omap_pm_get_dev_context_loss_count(&timer->pdev->dev);
+		if (loss_count != timer->ctx_loss_count) {
+			omap_timer_restore_context(timer);
+			timer->ctx_loss_count = loss_count;
+		}
+	}
 }
 EXPORT_SYMBOL_GPL(omap_dm_timer_enable);
 
@@ -347,12 +356,6 @@ int omap_dm_timer_start(struct omap_dm_timer *timer)
 
 	omap_dm_timer_enable(timer);
 
-	if (!(timer->capability & OMAP_TIMER_ALWON)) {
-		if (omap_pm_get_dev_context_loss_count(&timer->pdev->dev) !=
-				timer->ctx_loss_count)
-			omap_timer_restore_context(timer);
-	}
-
 	l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
 	if (!(l & OMAP_TIMER_CTRL_ST)) {
 		l |= OMAP_TIMER_CTRL_ST;
@@ -377,10 +380,6 @@ int omap_dm_timer_stop(struct omap_dm_timer *timer)
 
 	__omap_dm_timer_stop(timer, timer->posted, rate);
 
-	if (!(timer->capability & OMAP_TIMER_ALWON))
-		timer->ctx_loss_count =
-			omap_pm_get_dev_context_loss_count(&timer->pdev->dev);
-
 	/*
 	 * Since the register values are computed and written within
 	 * __omap_dm_timer_stop, we need to use read to retrieve the
@@ -494,12 +493,6 @@ int omap_dm_timer_set_load_start(struct omap_dm_timer *timer, int autoreload,
 
 	omap_dm_timer_enable(timer);
 
-	if (!(timer->capability & OMAP_TIMER_ALWON)) {
-		if (omap_pm_get_dev_context_loss_count(&timer->pdev->dev) !=
-				timer->ctx_loss_count)
-			omap_timer_restore_context(timer);
-	}
-
 	l = omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
 	if (autoreload) {
 		l |= OMAP_TIMER_CTRL_AR;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]