Hi,
* Lokesh Vutla <lokeshvutla@xxxxxx> [200228 09:55]:
> 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.
>
> Instead of using get_context_loss_count call back, implement cpu_pm
> notifier with context save and restore support. And delete the
> get_context_loss_count callback all together.
Thanks for getting this going.
I noticed system timers are not working properly now though. Not
sure what might cause that, but I spotted few issues below.
> --- a/drivers/clocksource/timer-ti-dm.c
> +++ b/drivers/clocksource/timer-ti-dm.c
...
> +static void omap_timer_save_context(struct omap_dm_timer *timer)
> +{
> + pm_runtime_get_sync(&timer->pdev->dev);
> + timer->context.tclr =
> + omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
> + timer->context.twer =
> + omap_dm_timer_read_reg(timer, OMAP_TIMER_WAKEUP_EN_REG);
> + timer->context.tldr =
> + omap_dm_timer_read_reg(timer, OMAP_TIMER_LOAD_REG);
> + timer->context.tmar =
> + omap_dm_timer_read_reg(timer, OMAP_TIMER_MATCH_REG);
> + timer->context.tier = readl_relaxed(timer->irq_ena);
> + timer->context.tsicr =
> + omap_dm_timer_read_reg(timer, OMAP_TIMER_IF_CTRL_REG);
> + pm_runtime_put_sync(&timer->pdev->dev);
> +}
We must not use pm_runtime functions here, these notifiers run
at a point when runtime PM is out of the picture already. And
we really don't want to tag any modules with pm_runtime_irq_safe()
as it takes a permanent use count on the parent device.
Instead, just add atomic_t awake that runtime_resume sets at the end,
and runtime_suspend clears first thing. Then you can check for awake
here, and there's nothing to do here if !awake.
And then runtime_suspend should save the context too and
runtime_resume restore it :)
> @@ -827,6 +830,8 @@ static int omap_dm_timer_remove(struct platform_device *pdev)
> list_for_each_entry(timer, &omap_timer_list, node)
> if (!strcmp(dev_name(&timer->pdev->dev),
> dev_name(&pdev->dev))) {
> + if (!(timer->capability & OMAP_TIMER_ALWON))
> + cpu_pm_unregister_notifier(&timer->nb);
> list_del(&timer->node);
> ret = 0;
> break;
For the OMAP_TIMER_ALWON checks, I believe am335x and am437x have
OMAP_TIMER_ALWON set for timers but will still have context lost
in deeper idle states as only the PMIC is enabled.
For those cases, at least runtime_suspend and resume functions
need to save and restore context based on setting some flag
maybe based on of_machine_is_compatible() or soc_device_match().
I guess with recent cpuidle patches, this needs to be also done
during runtime for am335x and am437x. Maybe Dave or Keerthy have
more comments on that part?
Regards,
Tony
