[2/2] OMAP2+: PM: SR: add suspend/resume handlers
diff mbox

Message ID 1311314153-23531-3-git-send-email-nm@ti.com
State New, archived
Headers show

Commit Message

Nishanth Menon July 22, 2011, 5:55 a.m. UTC
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(-)

Comments

Felipe Balbi July 22, 2011, 9:13 a.m. UTC | #1
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 ?
Nishanth Menon July 22, 2011, 1:53 p.m. UTC | #2
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
Kevin Hilman July 22, 2011, 8:10 p.m. UTC | #3
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

Patch
diff mbox

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",
 	},