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

[Lokesh Vutla]

On 04/03/20 11:47 PM, Tony Lindgren wrote:
> * 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.


Tested your changes on my BBB, timer and pwm are still working fine. Will repost
the series along with your changes.

Thanks and regards,
Lokesh

> 
>>> 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< ------------------------
> From tony Mon Sep 17 00:00:00 2001
> From: Lokesh Vutla <lokeshvutla@xxxxxx>
> Date: Fri, 28 Feb 2020 15:23:43 +0530
> Subject: [PATCH] clocksource: timer-ti-dm: Implement cpu_pm notifier for
>  context save and restore
> 
> 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.
> 
> Suggested-by: Tony Lindgren <tony@xxxxxxxxxxx>
> Signed-off-by: Lokesh Vutla <lokeshvutla@xxxxxx>
> [tony@xxxxxxxxxxx: removed pm_runtime calls from cpuidle calls]
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> ---
>  drivers/clocksource/timer-ti-dm.c | 99 ++++++++++++++++++-------------
>  include/clocksource/timer-ti-dm.h |  3 +-
>  2 files changed, 60 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-ti-dm.c b/drivers/clocksource/timer-ti-dm.c
> --- a/drivers/clocksource/timer-ti-dm.c
> +++ b/drivers/clocksource/timer-ti-dm.c
> @@ -20,6 +20,7 @@
>  
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
> +#include <linux/cpu_pm.h>
>  #include <linux/module.h>
>  #include <linux/io.h>
>  #include <linux/device.h>
> @@ -92,6 +93,47 @@ static void omap_timer_restore_context(struct omap_dm_timer *timer)
>  				timer->context.tclr);
>  }
>  
> +static void omap_timer_save_context(struct omap_dm_timer *timer)
> +{
> +	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);
> +}
> +
> +static int omap_timer_context_notifier(struct notifier_block *nb,
> +				       unsigned long cmd, void *v)
> +{
> +	struct omap_dm_timer *timer;
> +
> +	timer = container_of(nb, struct omap_dm_timer, nb);
> +
> +	switch (cmd) {
> +	case CPU_CLUSTER_PM_ENTER:
> +		if ((timer->capability & OMAP_TIMER_ALWON) ||
> +		    !atomic_read(&timer->enabled))
> +			break;
> +		omap_timer_save_context(timer);
> +		break;
> +	case CPU_CLUSTER_PM_ENTER_FAILED:
> +	case CPU_CLUSTER_PM_EXIT:
> +		if ((timer->capability & OMAP_TIMER_ALWON) ||
> +		    !atomic_read(&timer->enabled))
> +			break;
> +		omap_timer_restore_context(timer);
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
>  static int omap_dm_timer_reset(struct omap_dm_timer *timer)
>  {
>  	u32 l, timeout = 100000;
> @@ -208,21 +250,7 @@ static int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
>  
>  static 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;
> -			}
> -		} else {
> -			omap_timer_restore_context(timer);
> -		}
> -	}
>  }
>  
>  static void omap_dm_timer_disable(struct omap_dm_timer *timer)
> @@ -515,8 +543,6 @@ static int omap_dm_timer_start(struct omap_dm_timer *timer)
>  		omap_dm_timer_write_reg(timer, OMAP_TIMER_CTRL_REG, l);
>  	}
>  
> -	/* Save the context */
> -	timer->context.tclr = l;
>  	return 0;
>  }
>  
> @@ -532,13 +558,6 @@ static int omap_dm_timer_stop(struct omap_dm_timer *timer)
>  
>  	__omap_dm_timer_stop(timer, timer->posted, rate);
>  
> -	/*
> -	 * Since the register values are computed and written within
> -	 * __omap_dm_timer_stop, we need to use read to retrieve the
> -	 * context.
> -	 */
> -	timer->context.tclr =
> -			omap_dm_timer_read_reg(timer, OMAP_TIMER_CTRL_REG);
>  	omap_dm_timer_disable(timer);
>  	return 0;
>  }
> @@ -561,9 +580,6 @@ static int omap_dm_timer_set_load(struct omap_dm_timer *timer, int autoreload,
>  	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;
>  	omap_dm_timer_disable(timer);
>  	return 0;
>  }
> @@ -585,9 +601,6 @@ static int omap_dm_timer_set_match(struct omap_dm_timer *timer, int enable,
>  	omap_dm_timer_write_reg(timer, OMAP_TIMER_MATCH_REG, match);
>  	omap_dm_timer_write_reg(timer, OMAP_TIMER_CTRL_REG, l);
>  
> -	/* Save the context */
> -	timer->context.tclr = l;
> -	timer->context.tmar = match;
>  	omap_dm_timer_disable(timer);
>  	return 0;
>  }
> @@ -611,8 +624,6 @@ static int omap_dm_timer_set_pwm(struct omap_dm_timer *timer, int def_on,
>  	l |= trigger << 10;
>  	omap_dm_timer_write_reg(timer, OMAP_TIMER_CTRL_REG, l);
>  
> -	/* Save the context */
> -	timer->context.tclr = l;
>  	omap_dm_timer_disable(timer);
>  	return 0;
>  }
> @@ -634,8 +645,6 @@ static int omap_dm_timer_set_prescaler(struct omap_dm_timer *timer,
>  	}
>  	omap_dm_timer_write_reg(timer, OMAP_TIMER_CTRL_REG, l);
>  
> -	/* Save the context */
> -	timer->context.tclr = l;
>  	omap_dm_timer_disable(timer);
>  	return 0;
>  }
> @@ -649,9 +658,6 @@ static int omap_dm_timer_set_int_enable(struct omap_dm_timer *timer,
>  	omap_dm_timer_enable(timer);
>  	__omap_dm_timer_int_enable(timer, value);
>  
> -	/* Save the context */
> -	timer->context.tier = value;
> -	timer->context.twer = value;
>  	omap_dm_timer_disable(timer);
>  	return 0;
>  }
> @@ -679,9 +685,6 @@ static int omap_dm_timer_set_int_disable(struct omap_dm_timer *timer, u32 mask)
>  	l = omap_dm_timer_read_reg(timer, OMAP_TIMER_WAKEUP_EN_REG) & ~mask;
>  	omap_dm_timer_write_reg(timer, OMAP_TIMER_WAKEUP_EN_REG, l);
>  
> -	/* Save the context */
> -	timer->context.tier &= ~mask;
> -	timer->context.twer &= ~mask;
>  	omap_dm_timer_disable(timer);
>  	return 0;
>  }
> @@ -756,6 +759,12 @@ static int __maybe_unused omap_dm_timer_runtime_suspend(struct device *dev)
>  
>  	atomic_set(&timer->enabled, 0);
>  
> +	if (timer->capability & OMAP_TIMER_ALWON ||
> +	    !timer->func_base)
> +	    return 0;
> +
> +	omap_timer_save_context(timer);
> +
>  	return 0;
>  }
>  
> @@ -763,6 +772,10 @@ static int __maybe_unused omap_dm_timer_runtime_resume(struct device *dev)
>  {
>  	struct omap_dm_timer *timer = dev_get_drvdata(dev);
>  
> +	if (!(timer->capability & OMAP_TIMER_ALWON) &&
> +	    timer->func_base)
> +		omap_timer_restore_context(timer);
> +
>  	atomic_set(&timer->enabled, 1);
>  
>  	return 0;
> @@ -838,7 +851,11 @@ static int omap_dm_timer_probe(struct platform_device *pdev)
>  		timer->id = pdev->id;
>  		timer->capability = pdata->timer_capability;
>  		timer->reserved = omap_dm_timer_reserved_systimer(timer->id);
> -		timer->get_context_loss_count = pdata->get_context_loss_count;
> +	}
> +
> +	if (!(timer->capability & OMAP_TIMER_ALWON)) {
> +		timer->nb.notifier_call = omap_timer_context_notifier;
> +		cpu_pm_register_notifier(&timer->nb);
>  	}
>  
>  	if (pdata)
> @@ -893,6 +910,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;
> diff --git a/include/clocksource/timer-ti-dm.h b/include/clocksource/timer-ti-dm.h
> --- a/include/clocksource/timer-ti-dm.h
> +++ b/include/clocksource/timer-ti-dm.h
> @@ -110,13 +110,12 @@ struct omap_dm_timer {
>  	unsigned reserved:1;
>  	unsigned posted:1;
>  	struct timer_regs context;
> -	int (*get_context_loss_count)(struct device *);
> -	int ctx_loss_count;
>  	int revision;
>  	u32 capability;
>  	u32 errata;
>  	struct platform_device *pdev;
>  	struct list_head node;
> +	struct notifier_block nb;
>  };
>  
>  int omap_dm_timer_reserve_systimer(int id);
>