| Message ID | 20200228095346.32177-3-lokeshvutla@ti.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | clocksource: timer-ti-dm: Prepare for dynamic pwm period updates | expand |
Hi, * Lokesh Vutla <lokeshvutla@ti.com> [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
Hi Tony, On 03/03/20 10:02 PM, Tony Lindgren wrote: > Hi, > > * Lokesh Vutla <lokeshvutla@ti.com> [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? > 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. 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? > > 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(). 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. Thanks and regards, Lokesh > > 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 >
* Lokesh Vutla <lokeshvutla@ti.com> [200304 04:45]: > Hi Tony, > > On 03/03/20 10:02 PM, Tony Lindgren wrote: > > Hi, > > > > * Lokesh Vutla <lokeshvutla@ti.com> [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< ------------------------ From tony Mon Sep 17 00:00:00 2001 From: Lokesh Vutla <lokeshvutla@ti.com> 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@atomide.com> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> [tony@atomide.com: removed pm_runtime calls from cpuidle calls] Signed-off-by: Tony Lindgren <tony@atomide.com> --- 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);
On 04/03/20 11:47 PM, Tony Lindgren wrote: > * Lokesh Vutla <lokeshvutla@ti.com> [200304 04:45]: >> Hi Tony, >> >> On 03/03/20 10:02 PM, Tony Lindgren wrote: >>> Hi, >>> >>> * Lokesh Vutla <lokeshvutla@ti.com> [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@ti.com> > 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@atomide.com> > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > [tony@atomide.com: removed pm_runtime calls from cpuidle calls] > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > 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); >
diff --git a/drivers/clocksource/timer-ti-dm.c b/drivers/clocksource/timer-ti-dm.c index bf132d321ecc..9606103b92e0 100644 --- 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,43 @@ 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) +{ + 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); +} + +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: + omap_timer_save_context(timer); + break; + case CPU_CLUSTER_PM_ENTER_FAILED: + case CPU_CLUSTER_PM_EXIT: + omap_timer_restore_context(timer); + break; + } + + return NOTIFY_OK; +} + static int omap_dm_timer_reset(struct omap_dm_timer *timer) { u32 l, timeout = 100000; @@ -179,21 +217,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) @@ -484,8 +508,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; } @@ -501,13 +523,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; } @@ -530,9 +545,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; } @@ -554,9 +566,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; } @@ -580,8 +589,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; } @@ -603,8 +610,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; } @@ -618,9 +623,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; } @@ -648,9 +650,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; } @@ -773,7 +772,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) @@ -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; diff --git a/include/clocksource/timer-ti-dm.h b/include/clocksource/timer-ti-dm.h index 7d9598dc578d..c2b172b28652 100644 --- a/include/clocksource/timer-ti-dm.h +++ b/include/clocksource/timer-ti-dm.h @@ -109,13 +109,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);
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@atomide.com> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> --- drivers/clocksource/timer-ti-dm.c | 85 ++++++++++++++++--------------- include/clocksource/timer-ti-dm.h | 3 +- 2 files changed, 46 insertions(+), 42 deletions(-)