Re: [PATCH v2 2/5] clocksource: timer-ti-dm: Implement cpu_pm notifier for context save and restore

[Tony Lindgren]

* Lokesh Vutla <lokeshvutla@xxxxxx> [200304 04:45]:
> Hi Tony,
> 
> On 03/03/20 10:02 PM, Tony Lindgren wrote:
> > 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
> 
> Can you provide me details on how you are testing and on which SoC?

Hmm I guess with any SoC with gp timer as a clockevent as long as the
system is idle enough for interrupts. I was seeing this at least on
omap4, it may not show up with NFSroot because interrupts.

> But context should be saved when awake is enabled. In this case how to make sure
> the registers are accessible? Driver heavily uses pm_runtime calls for most
> register access. When timer is running the register are made accessible but I am
> worried about the case when timer is not running and trying to save context.
> 
> Also in CLUSTER_PM_EXIT case,  how to guarantee that registers are accessible?

Well below is a version of your patch updated along the lines I described
above. That's on top of the preparatory 1.5/2 patch I just sent for
"clocksource: timer-ti-dm: Prepare for using cpuidle".

It now seems to work for me bsed on quick testing. I have also not
tested yet with the rest of the series. And we still need to run timer
tests on some idle SoC.

> > 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().
> 
> hmm..then it is better to not mark as alwon in case of am335x and am43xx no? I
> don't see the flag being used for anything else other that context save and restore.

Yeah or change the flag during runtime. But let's assume we have
the flag properly set for this series.

Regards,

Tony

8< ------------------------