| Message ID | 1311314153-23531-3-git-send-email-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Fri, Jul 22, 2011 at 12:55:53AM -0500, Nishanth Menon wrote: > Suspend and Resume paths are safe enough to do it in > the standard LDM suspend/resume handlers where one can > sleep. Add suspend/resume handlers for SmartReflex. > > Signed-off-by: Nishanth Menon <nm@ti.com> > --- > arch/arm/mach-omap2/smartreflex.c | 87 +++++++++++++++++++++++++++++++++++++ > 1 files changed, 87 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c > index 33a027f..fb90bd2 100644 > --- a/arch/arm/mach-omap2/smartreflex.c > +++ b/arch/arm/mach-omap2/smartreflex.c > @@ -39,6 +39,7 @@ struct omap_sr { > int ip_type; > int nvalue_count; > bool autocomp_active; > + bool is_suspended; > u32 clk_length; > u32 err_weight; > u32 err_minlimit; > @@ -684,6 +685,12 @@ void omap_sr_enable(struct voltagedomain *voltdm) > if (!sr->autocomp_active) > return; > > + if (sr->is_suspended) { > + dev_dbg(&sr->pdev->dev, "%s: in suspended state\n", __func__); > + return; > + } I wonder why you get these called if you're in suspend state. If this is called by some other driver, then shouldn't you pm_runtime_get_sync(); do_whatever_you_need_to_do(); and pm_runtime_put(); rather then just returning ?
On Fri, Jul 22, 2011 at 04:13, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Fri, Jul 22, 2011 at 12:55:53AM -0500, Nishanth Menon wrote: >> Suspend and Resume paths are safe enough to do it in >> the standard LDM suspend/resume handlers where one can >> sleep. Add suspend/resume handlers for SmartReflex. >> >> Signed-off-by: Nishanth Menon <nm@ti.com> >> --- >> arch/arm/mach-omap2/smartreflex.c | 87 +++++++++++++++++++++++++++++++++++++ >> 1 files changed, 87 insertions(+), 0 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c >> index 33a027f..fb90bd2 100644 >> --- a/arch/arm/mach-omap2/smartreflex.c >> +++ b/arch/arm/mach-omap2/smartreflex.c >> @@ -39,6 +39,7 @@ struct omap_sr { >> int ip_type; >> int nvalue_count; >> bool autocomp_active; >> + bool is_suspended; >> u32 clk_length; >> u32 err_weight; >> u32 err_minlimit; >> @@ -684,6 +685,12 @@ void omap_sr_enable(struct voltagedomain *voltdm) >> if (!sr->autocomp_active) >> return; >> >> + if (sr->is_suspended) { >> + dev_dbg(&sr->pdev->dev, "%s: in suspended state\n", __func__); >> + return; >> + } > > I wonder why you get these called if you're in suspend state. If this is > called by some other driver, then shouldn't you pm_runtime_get_sync(); > do_whatever_you_need_to_do(); and pm_runtime_put(); rather then just > returning ? because we have two state machines running in parallel - cpu idle and dvfs(cpufreq, and other dependent domain voltage scaling) and SR driver is just one - it does it's own get_sync and put_sync_suspend. we cannot guarentee that the SR enable/disable calls can be properly sequenced unless we use suspend lockouts. SmartReflex driver is an independent entity of it's own - yeah we can do the same as we have done historically in the omap[34]_idle paths(which is disable SR after we have disabled interrupts), but in case of suspend(unlike in idle), we do *know* that SR will have to be disabled - hence doing it earlier as part of standard LDM suspend sequences is more opportunistic. Regards, Nishanth Menon
Nishanth Menon <nm@ti.com> writes: > Suspend and Resume paths are safe enough to do it in What is 'it' ? > the standard LDM suspend/resume handlers where one can > sleep. Add suspend/resume handlers for SmartReflex. Minor comments on the code below, but this changelog doesn't read well, or at least I can't make any sense of it. [...] > @@ -684,6 +685,12 @@ void omap_sr_enable(struct voltagedomain *voltdm) > if (!sr->autocomp_active) > return; > > + if (sr->is_suspended) { > + dev_dbg(&sr->pdev->dev, "%s: in suspended state\n", __func__); > + return; > + } > + > + extra blank line > if (!sr_class || !(sr_class->enable) || !(sr_class->configure)) { > dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not" > "registered\n", __func__); [...] > static struct platform_driver smartreflex_driver = { > .remove = omap_sr_remove, > + .suspend = omap_sr_suspend, > + .resume = omap_sr_resume, You're using the legacy methods here, please use dev_pm_ops. That means you'll need to create a struct dev_pm_ops and fill in these mehods there (and note the dev_pm_ops methods don't have a 'state' argument. Also, when implementing suspend/resume, you need to make sure the hibernate callbacks are populated also. They should be populated with the same callbacks, so the best way to do this is to use SIMPLE_DEV_PM_OPS() (see <linux/pm.h>). That macro also takes care of the !CONFIG_PM case as well. IOW, the result would look someting like this (not even compile tested): static SIMPLE_DEV_PM_OPS(omap_sr_pm_ops, omap_sr_suspend, omap_sr_resume) static struct platform_driver smartreflex_driver = { .remove = omap_sr_remove, .driver = { .name = "smartreflex", .pm = &omap_sr_pm_ops, }, }; Kevin
diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c index 33a027f..fb90bd2 100644 --- a/arch/arm/mach-omap2/smartreflex.c +++ b/arch/arm/mach-omap2/smartreflex.c @@ -39,6 +39,7 @@ struct omap_sr { int ip_type; int nvalue_count; bool autocomp_active; + bool is_suspended; u32 clk_length; u32 err_weight; u32 err_minlimit; @@ -684,6 +685,12 @@ void omap_sr_enable(struct voltagedomain *voltdm) if (!sr->autocomp_active) return; + if (sr->is_suspended) { + dev_dbg(&sr->pdev->dev, "%s: in suspended state\n", __func__); + return; + } + + if (!sr_class || !(sr_class->enable) || !(sr_class->configure)) { dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not" "registered\n", __func__); @@ -717,6 +724,11 @@ void omap_sr_disable(struct voltagedomain *voltdm) if (!sr->autocomp_active) return; + if (sr->is_suspended) { + dev_dbg(&sr->pdev->dev, "%s: in suspended state\n", __func__); + return; + } + if (!sr_class || !(sr_class->disable)) { dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not" "registered\n", __func__); @@ -750,6 +762,11 @@ void omap_sr_disable_reset_volt(struct voltagedomain *voltdm) if (!sr->autocomp_active) return; + if (sr->is_suspended) { + dev_dbg(&sr->pdev->dev, "%s: in suspended state\n", __func__); + return; + } + if (!sr_class || !(sr_class->disable)) { dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not" "registered\n", __func__); @@ -808,6 +825,11 @@ static int omap_sr_autocomp_store(void *data, u64 val) return -EINVAL; } + if (sr_info->is_suspended) { + pr_warning("%s: in suspended state\n", __func__); + return -EBUSY; + } + if (!val) sr_stop_vddautocomp(sr_info); else @@ -998,8 +1020,73 @@ static int __devexit omap_sr_remove(struct platform_device *pdev) return 0; } +static int omap_sr_suspend(struct platform_device *pdev, pm_message_t state) +{ + struct omap_sr_data *pdata = pdev->dev.platform_data; + struct omap_sr *sr_info; + + if (!pdata) { + dev_err(&pdev->dev, "%s: platform data missing\n", __func__); + return -EINVAL; + } + + sr_info = _sr_lookup(pdata->voltdm); + if (IS_ERR(sr_info)) { + dev_warn(&pdev->dev, "%s: omap_sr struct not found\n", + __func__); + return -EINVAL; + } + + if (!sr_info->autocomp_active) + return 0; + + if (sr_info->is_suspended) + return 0; + + omap_sr_disable_reset_volt(pdata->voltdm); + sr_info->is_suspended = true; + /* Flag the same info to the other CPUs */ + smp_wmb(); + + return 0; +} + +static int omap_sr_resume(struct platform_device *pdev) +{ + struct omap_sr_data *pdata = pdev->dev.platform_data; + struct omap_sr *sr_info; + + if (!pdata) { + dev_err(&pdev->dev, "%s: platform data missing\n", __func__); + return -EINVAL; + } + + sr_info = _sr_lookup(pdata->voltdm); + if (IS_ERR(sr_info)) { + dev_warn(&pdev->dev, "%s: omap_sr struct not found\n", + __func__); + return -EINVAL; + } + + if (!sr_info->autocomp_active) + return 0; + + if (!sr_info->is_suspended) + return 0; + + sr_info->is_suspended = false; + /* Flag the same info to the other CPUs */ + smp_wmb(); + omap_sr_enable(pdata->voltdm); + + return 0; +} + + static struct platform_driver smartreflex_driver = { .remove = omap_sr_remove, + .suspend = omap_sr_suspend, + .resume = omap_sr_resume, .driver = { .name = "smartreflex", },
Suspend and Resume paths are safe enough to do it in the standard LDM suspend/resume handlers where one can sleep. Add suspend/resume handlers for SmartReflex. Signed-off-by: Nishanth Menon <nm@ti.com> --- arch/arm/mach-omap2/smartreflex.c | 87 +++++++++++++++++++++++++++++++++++++ 1 files changed, 87 insertions(+), 0 deletions(-)