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

[Jon Hunter <jon-hunter@ti.com>]

On 01/06/2013 03:12 PM, NeilBrown wrote:

[snip]

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

Thanks for sending. I have given this patch a try on omap3 and I am still
some some failures with my timer read test. I need to dig into that further,
but I am guessing not related to your patch as there were problems there
before :-(

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

Nit, subject should formatted "ARM: OMAP: blah blah blah"

Also, may be worth calling this "fix context-loss" as this is really
fixing something that is broken.
 
> 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;
> +		}
> +	}
>  }

Can you rebase on v3.8-rc2? We no longer call 
omap_pm_get_dev_context_loss_count() directly and so this
does not apply. Should be something like ...

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index d51b75b..2c48182 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -315,7 +315,19 @@ EXPORT_SYMBOL_GPL(omap_dm_timer_free);
 
 void omap_dm_timer_enable(struct omap_dm_timer *timer)
 {
+       int c;
+
        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;
+                       }
+               }
+       }

Cheers
Jon