[PATCH 0/2] OMAP3+: PM: SR fixes

[PATCH 0/2] OMAP3+: PM: SR fixes

[Nishanth Menon]

Hi Kevin,
Please find attached a couple of patches collected as part of SR stabilization
around idle and a slight improvement for suspend path. The second patch
could probably help at a later date as part of moving the SmartReflex driver
off as a seperate independent driver of it's own.

These are based on Linus master v3.0-rc7

Colin Cross (1):
  OMAP3+: PM: SR: use put_sync_suspend for disabling

Nishanth Menon (1):
  OMAP2+: PM: SR: add suspend/resume handlers

 arch/arm/mach-omap2/smartreflex.c |   89 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 88 insertions(+), 1 deletions(-)

-- 
Regards,
Nishanth Menon

[PATCH 1/2] OMAP3+: PM: SR: use put_sync_suspend for disabling

[Nishanth Menon]

From: Colin Cross <ccross@google.com>

omap_sr_disable_reset_volt is called with irqs off in omapx_enter_sleep,
as part of idle sequence, this eventually calls sr_disable and
pm_runtime_put_sync. pm_runtime_put_sync calls rpm_idle, which will
enable interrupts in order to call the callback. In this short interval
when interrupts are enabled, scenarios such as the following can occur:
while interrupts are enabled, the timer interrupt that is supposed to
wake the device out of idle occurs and is acked, so when the CPU finally
goes to off, the timer is already gone, missing a wakeup event.

Further, as the documentation for runtime states:"
 However, subsystems can use the pm_runtime_irq_safe() helper function
 to tell the PM core that a device's ->runtime_suspend() and ->runtime_resume()
 callbacks should be invoked in atomic context with interrupts disabled
 (->runtime_idle() is still invoked the default way)."

Hence, replace pm_runtime_put_sync with pm_runtime_put_sync_suspend
to invoke the suspend handler and shut off the fclk for SmartReflex
module instead of using the idle handler in interrupt disabled context.

Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Colin Cross <ccross@google.com>
---
 arch/arm/mach-omap2/smartreflex.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
index fb7dc52..33a027f 100644
--- a/arch/arm/mach-omap2/smartreflex.c
+++ b/arch/arm/mach-omap2/smartreflex.c
@@ -622,7 +622,7 @@ void sr_disable(struct voltagedomain *voltdm)
 			sr_v2_disable(sr);
 	}
 
-	pm_runtime_put_sync(&sr->pdev->dev);
+	pm_runtime_put_sync_suspend(&sr->pdev->dev);
 }
 
 /**
-- 
1.7.4.1

Re: [PATCH 1/2] OMAP3+: PM: SR: use put_sync_suspend for disabling

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> From: Colin Cross <ccross@google.com>
>
> omap_sr_disable_reset_volt is called with irqs off in omapx_enter_sleep,
> as part of idle sequence, this eventually calls sr_disable and
> pm_runtime_put_sync. pm_runtime_put_sync calls rpm_idle, which will
> enable interrupts in order to call the callback. In this short interval
> when interrupts are enabled, scenarios such as the following can occur:
> while interrupts are enabled, the timer interrupt that is supposed to
> wake the device out of idle occurs and is acked, so when the CPU finally
> goes to off, the timer is already gone, missing a wakeup event.
>
> Further, as the documentation for runtime states:"
>  However, subsystems can use the pm_runtime_irq_safe() helper function
>  to tell the PM core that a device's ->runtime_suspend() and ->runtime_resume()
>  callbacks should be invoked in atomic context with interrupts disabled
>  (->runtime_idle() is still invoked the default way)."
>
> Hence, replace pm_runtime_put_sync with pm_runtime_put_sync_suspend
> to invoke the suspend handler and shut off the fclk for SmartReflex
> module instead of using the idle handler in interrupt disabled context.
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Colin Cross <ccross@google.com>

Great catch!

Looking through Documentation/power/runtime_pm.txt, I see (now) that it
is well documented that _put_sync_suspend() is safe to use from
interrupts-disabled context, but _put_sync() is not on that list.

Queuing this as a fix for v3.1.

Kevin

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

[Nishanth Menon]

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;
+	}
+
+
 	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

Re: [PATCH 2/2] OMAP2+: PM: SR: add suspend/resume handlers

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]

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 ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 2/2] OMAP2+: PM: SR: add suspend/resume handlers

[Menon, Nishanth]

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

Re: [PATCH 2/2] OMAP2+: PM: SR: add suspend/resume handlers

[Kevin Hilman]

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