Re: [PATCH 2/2] OMAP2+: PM: SR: add suspend/resume handlers
Nishanth Menon 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 ). 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] OMAP2+: PM: SR: add suspend/resume handlers
On Fri, Jul 22, 2011 at 04:13, Felipe Balbi 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
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] OMAP2+: PM: SR: add suspend/resume handlers
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
> ---
> 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;
> boolautocomp_active;
> + boolis_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 ?
--
balbi
signature.asc
Description: Digital signature
[PATCH 2/2] OMAP2+: PM: SR: add suspend/resume handlers
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
---
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;
boolautocomp_active;
+ boolis_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",
},
--
1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
