| Message ID | 1311526357-7578-1-git-send-email-nm@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Sun, Jul 24, 2011 at 11:52:37AM -0500, Nishanth Menon wrote: > SmartReflex should be disabled while entering low power mode due to > the following reasons: > a) SmartReflex values are not defined for retention voltage. > b) With SmartReflex enabled, if the CPU enters low power state, FSM > will try to bump the voltage to current OPP's voltage for which > it has entered low power state, causing power loss and potential > unknown states for the SoC. > Since we are sure to attempt entering the lowest possible power state > during suspend operation, SmartReflex needs to be disabled for the > voltage domains in suspend path before achieving auto retention voltage > on the device. > > Traditionally, this has been done with interrupts disabled as part of > the common code which handles the idle sequence. Instead, by using the > fact that we have to disable SmartReflex for sure during suspend > operations, we can opportunistically disable SmartReflex in device > standard pm ops, instead of disabling it as part of the code which > executes with interrupts disabled and slave CPU{s} shutdown. This > allows the system to do other parallel activities(such as suspending > other devices in the system using slave CPU{s}) and save the time > required to achieve suspend and resume from suspended state as a > sequential activity. > > However, by being opportunistic as described above, we also increase > the likelihood of SmartReflex library access functions being invoked in > parallel contexts *after* SmartReflex driver's suspend handler (during > suspend operation) or *before* resume handler (during resume operation) > have been invoked (Example: DVFS for dependent devices, cpufreq for > MPU etc.). We prevent this by using a flag to reject the callers in > the duration where SmartReflex has been disabled. while at that, SR's IRQ is never freed on exit path, could fix it while you're already there ? > @@ -998,10 +1020,75 @@ static int __devexit omap_sr_remove(struct platform_device *pdev) > return 0; > } > > +static int omap_sr_suspend(struct device *dev) > +{ > + struct omap_sr_data *pdata; > + struct omap_sr *sr_info; > + > + pdata = dev_get_platdata(dev); I'm not sure you need to use platform data here... > + if (!pdata) { > + dev_err(dev, "%s: platform data missing\n", __func__); > + return -EINVAL; > + } > + > + sr_info = _sr_lookup(pdata->voltdm); this field is held on struct omap_sr. Can't you: struct omap_sr *info = dev_get_drvdata(dev); ?? I see that a platform_set_drvdata() is missing from the driver, but maybe you should add that instead of accessing platform_data. > static struct platform_driver smartreflex_driver = { > .remove = omap_sr_remove, > .driver = { > .name = "smartreflex", > + .pm = &omap_sr_dev_pm_ops, this should not be valid in case CONFIG_PM isn't enabled.
On Mon, Jul 25, 2011 at 03:42, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Sun, Jul 24, 2011 at 11:52:37AM -0500, Nishanth Menon wrote: >> SmartReflex should be disabled while entering low power mode due to >> the following reasons: >> a) SmartReflex values are not defined for retention voltage. >> b) With SmartReflex enabled, if the CPU enters low power state, FSM >> will try to bump the voltage to current OPP's voltage for which >> it has entered low power state, causing power loss and potential >> unknown states for the SoC. >> Since we are sure to attempt entering the lowest possible power state >> during suspend operation, SmartReflex needs to be disabled for the >> voltage domains in suspend path before achieving auto retention voltage >> on the device. >> >> Traditionally, this has been done with interrupts disabled as part of >> the common code which handles the idle sequence. Instead, by using the >> fact that we have to disable SmartReflex for sure during suspend >> operations, we can opportunistically disable SmartReflex in device >> standard pm ops, instead of disabling it as part of the code which >> executes with interrupts disabled and slave CPU{s} shutdown. This >> allows the system to do other parallel activities(such as suspending >> other devices in the system using slave CPU{s}) and save the time >> required to achieve suspend and resume from suspended state as a >> sequential activity. >> >> However, by being opportunistic as described above, we also increase >> the likelihood of SmartReflex library access functions being invoked in >> parallel contexts *after* SmartReflex driver's suspend handler (during >> suspend operation) or *before* resume handler (during resume operation) >> have been invoked (Example: DVFS for dependent devices, cpufreq for >> MPU etc.). We prevent this by using a flag to reject the callers in >> the duration where SmartReflex has been disabled. > > while at that, SR's IRQ is never freed on exit path, could fix it while > you're already there ? This is not really related to this patch is it? IMHO IRQ handling is broken badly. Current support is for SmartReflex class3 - which does not use the IRQ, Class2 and Class1.5 use it, but the current code requires major fixes which I dont intend to support in this series. > >> @@ -998,10 +1020,75 @@ static int __devexit omap_sr_remove(struct platform_device *pdev) >> return 0; >> } >> >> +static int omap_sr_suspend(struct device *dev) >> +{ >> + struct omap_sr_data *pdata; >> + struct omap_sr *sr_info; >> + >> + pdata = dev_get_platdata(dev); > > I'm not sure you need to use platform data here... see below > >> + if (!pdata) { >> + dev_err(dev, "%s: platform data missing\n", __func__); >> + return -EINVAL; >> + } >> + >> + sr_info = _sr_lookup(pdata->voltdm); > > this field is held on struct omap_sr. Can't you: > > struct omap_sr *info = dev_get_drvdata(dev); > > ?? I see that a platform_set_drvdata() is missing from the driver, but > maybe you should add that instead of accessing platform_data. omap_sr_data is added in arch/arm/mach-omap2/sr_device.c With the current handling - it needs sr_data from which sr_info is pulled out. in the current implementation, sr_data contains the voltdm pointer from which sr_info is pulled out. > >> static struct platform_driver smartreflex_driver = { >> .remove = omap_sr_remove, >> .driver = { >> .name = "smartreflex", >> + .pm = &omap_sr_dev_pm_ops, > > this should not be valid in case CONFIG_PM isn't enabled. a) arch/arm/plat-omap/Kconfig CONFIG_OMAP_SMARTREFLEX depends on PM. b) SmartReflex is PM feature I dont see a need to add redundant code here. Regards, Nishanth Menon
Hi, On Mon, Jul 25, 2011 at 11:59:10AM -0500, Menon, Nishanth wrote: > > On Sun, Jul 24, 2011 at 11:52:37AM -0500, Nishanth Menon wrote: > >> SmartReflex should be disabled while entering low power mode due to > >> the following reasons: > >> a) SmartReflex values are not defined for retention voltage. > >> b) With SmartReflex enabled, if the CPU enters low power state, FSM > >> will try to bump the voltage to current OPP's voltage for which > >> it has entered low power state, causing power loss and potential > >> unknown states for the SoC. > >> Since we are sure to attempt entering the lowest possible power state > >> during suspend operation, SmartReflex needs to be disabled for the > >> voltage domains in suspend path before achieving auto retention voltage > >> on the device. > >> > >> Traditionally, this has been done with interrupts disabled as part of > >> the common code which handles the idle sequence. Instead, by using the > >> fact that we have to disable SmartReflex for sure during suspend > >> operations, we can opportunistically disable SmartReflex in device > >> standard pm ops, instead of disabling it as part of the code which > >> executes with interrupts disabled and slave CPU{s} shutdown. This > >> allows the system to do other parallel activities(such as suspending > >> other devices in the system using slave CPU{s}) and save the time > >> required to achieve suspend and resume from suspended state as a > >> sequential activity. > >> > >> However, by being opportunistic as described above, we also increase > >> the likelihood of SmartReflex library access functions being invoked in > >> parallel contexts *after* SmartReflex driver's suspend handler (during > >> suspend operation) or *before* resume handler (during resume operation) > >> have been invoked (Example: DVFS for dependent devices, cpufreq for > >> MPU etc.). We prevent this by using a flag to reject the callers in > >> the duration where SmartReflex has been disabled. > > > > while at that, SR's IRQ is never freed on exit path, could fix it while > > you're already there ? > > This is not really related to this patch is it? IMHO IRQ handling is I didn't say to put it on the same patch ;-) I meant that while at that, you could add that simple fix before this patch ;-) > broken badly. Current support is for SmartReflex class3 - which does > not use the IRQ, Class2 and Class1.5 use it, but the current code > requires major fixes which I dont intend to support in this series. And that's exactly what I mean. IMHO it's far better to fix the mess before adding more stuff, otherwise it just becomes an even bigger mess, even more difficult to fix in the long run. We've seen that with GPIO and sDMA drivers _at_least_ ;-( > >> @@ -998,10 +1020,75 @@ static int __devexit omap_sr_remove(struct platform_device *pdev) > >> return 0; > >> } > >> > >> +static int omap_sr_suspend(struct device *dev) > >> +{ > >> + struct omap_sr_data *pdata; > >> + struct omap_sr *sr_info; > >> + > >> + pdata = dev_get_platdata(dev); > > > > I'm not sure you need to use platform data here... > see below > > > >> + if (!pdata) { > >> + dev_err(dev, "%s: platform data missing\n", __func__); > >> + return -EINVAL; > >> + } > >> + > >> + sr_info = _sr_lookup(pdata->voltdm); > > > > this field is held on struct omap_sr. Can't you: > > > > struct omap_sr *info = dev_get_drvdata(dev); > > > > ?? I see that a platform_set_drvdata() is missing from the driver, but > > maybe you should add that instead of accessing platform_data. > omap_sr_data is added in arch/arm/mach-omap2/sr_device.c > > With the current handling - it needs sr_data from which sr_info is > pulled out. in the current implementation, sr_data contains the voltdm > pointer from which sr_info is pulled out. but sr_info is allocated on probe() isn't it ? if you add platform_set_drvdata(pdev, sr_info) on probe, you won't need sr_data to fetch sr_info, all you need is to use dev_get_drvdata(dev). Am I missing something ? > >> static struct platform_driver smartreflex_driver = { > >> .remove = omap_sr_remove, > >> .driver = { > >> .name = "smartreflex", > >> + .pm = &omap_sr_dev_pm_ops, > > > > this should not be valid in case CONFIG_PM isn't enabled. > a) arch/arm/plat-omap/Kconfig > CONFIG_OMAP_SMARTREFLEX depends on PM. > b) SmartReflex is PM feature > > I dont see a need to add redundant code here. fair enough if this will never build without CONFIG_PM ;-)
On Mon, Jul 25, 2011 at 12:13, Felipe Balbi <balbi@ti.com> wrote: [..] >> > while at that, SR's IRQ is never freed on exit path, could fix it while >> > you're already there ? >> >> This is not really related to this patch is it? IMHO IRQ handling is > > I didn't say to put it on the same patch ;-) I meant that while at that, > you could add that simple fix before this patch ;-) Not really that simple - it is just one part of the equation, my point being - if we are cleaning up, we better cleanup completely on that thread at least. > >> broken badly. Current support is for SmartReflex class3 - which does >> not use the IRQ, Class2 and Class1.5 use it, but the current code >> requires major fixes which I dont intend to support in this series. > > And that's exactly what I mean. IMHO it's far better to fix the mess > before adding more stuff, otherwise it just becomes an even bigger mess, > even more difficult to fix in the long run. We've seen that with GPIO > and sDMA drivers _at_least_ ;-( I tried pushing the cleanups in my series http://marc.info/?l=linux-omap&m=129933897910785&w=2 few of them did go through and I have since personally lost interest and depending on my next free slot (not forthcoming for next few months), I might want to retry it, but I guess there is more interest in turning things into regulators than add new code. I am ok if folks want to drop this patch - like previously, things tend to get forgotten.. > >> >> @@ -998,10 +1020,75 @@ static int __devexit omap_sr_remove(struct platform_device *pdev) >> >> return 0; >> >> } >> >> >> >> +static int omap_sr_suspend(struct device *dev) >> >> +{ >> >> + struct omap_sr_data *pdata; >> >> + struct omap_sr *sr_info; >> >> + >> >> + pdata = dev_get_platdata(dev); >> > >> > I'm not sure you need to use platform data here... >> see below >> > >> >> + if (!pdata) { >> >> + dev_err(dev, "%s: platform data missing\n", __func__); >> >> + return -EINVAL; >> >> + } >> >> + >> >> + sr_info = _sr_lookup(pdata->voltdm); >> > >> > this field is held on struct omap_sr. Can't you: >> > >> > struct omap_sr *info = dev_get_drvdata(dev); >> > >> > ?? I see that a platform_set_drvdata() is missing from the driver, but >> > maybe you should add that instead of accessing platform_data. >> omap_sr_data is added in arch/arm/mach-omap2/sr_device.c >> >> With the current handling - it needs sr_data from which sr_info is >> pulled out. in the current implementation, sr_data contains the voltdm >> pointer from which sr_info is pulled out. > > but sr_info is allocated on probe() isn't it ? if you add > platform_set_drvdata(pdev, sr_info) on probe, you won't need sr_data to > fetch sr_info, all you need is to use dev_get_drvdata(dev). Am I missing > something ? sr_info - I am not debating that this is not possible - I am just explaining how it is being done now. I just dont have the bandwidth to kill all evils in smartreflex driver. :( Regards, Nishanth Menon
Hi, On Mon, Jul 25, 2011 at 12:55:39PM -0500, Menon, Nishanth wrote: > On Mon, Jul 25, 2011 at 12:13, Felipe Balbi <balbi@ti.com> wrote: > [..] > >> > while at that, SR's IRQ is never freed on exit path, could fix it while > >> > you're already there ? > >> > >> This is not really related to this patch is it? IMHO IRQ handling is > > > > I didn't say to put it on the same patch ;-) I meant that while at that, > > you could add that simple fix before this patch ;-) > Not really that simple - it is just one part of the equation, my point > being - if we are cleaning up, we better cleanup completely on that > thread at least. fair enough :-) > >> broken badly. Current support is for SmartReflex class3 - which does > >> not use the IRQ, Class2 and Class1.5 use it, but the current code > >> requires major fixes which I dont intend to support in this series. > > > > And that's exactly what I mean. IMHO it's far better to fix the mess > > before adding more stuff, otherwise it just becomes an even bigger mess, > > even more difficult to fix in the long run. We've seen that with GPIO > > and sDMA drivers _at_least_ ;-( > > I tried pushing the cleanups in my series > http://marc.info/?l=linux-omap&m=129933897910785&w=2 > > few of them did go through and I have since personally lost interest > and depending on my next free slot (not forthcoming for next few > months), I might want to retry it, but I guess there is more interest > in turning things into regulators than add new code. > > I am ok if folks want to drop this patch - like previously, things > tend to get forgotten.. I didn't really say that, but ok ;-) > >> >> @@ -998,10 +1020,75 @@ static int __devexit omap_sr_remove(struct platform_device *pdev) > >> >> return 0; > >> >> } > >> >> > >> >> +static int omap_sr_suspend(struct device *dev) > >> >> +{ > >> >> + struct omap_sr_data *pdata; > >> >> + struct omap_sr *sr_info; > >> >> + > >> >> + pdata = dev_get_platdata(dev); > >> > > >> > I'm not sure you need to use platform data here... > >> see below > >> > > >> >> + if (!pdata) { > >> >> + dev_err(dev, "%s: platform data missing\n", __func__); > >> >> + return -EINVAL; > >> >> + } > >> >> + > >> >> + sr_info = _sr_lookup(pdata->voltdm); > >> > > >> > this field is held on struct omap_sr. Can't you: > >> > > >> > struct omap_sr *info = dev_get_drvdata(dev); > >> > > >> > ?? I see that a platform_set_drvdata() is missing from the driver, but > >> > maybe you should add that instead of accessing platform_data. > >> omap_sr_data is added in arch/arm/mach-omap2/sr_device.c > >> > >> With the current handling - it needs sr_data from which sr_info is > >> pulled out. in the current implementation, sr_data contains the voltdm > >> pointer from which sr_info is pulled out. > > > > but sr_info is allocated on probe() isn't it ? if you add > > platform_set_drvdata(pdev, sr_info) on probe, you won't need sr_data to > > fetch sr_info, all you need is to use dev_get_drvdata(dev). Am I missing > > something ? > sr_info - I am not debating that this is not possible - I am just > explaining how it is being done now. I just dont have the bandwidth to > kill all evils in smartreflex driver. :( I see... ok then, I'll see if I find some time to play with that, should be kinda fun - not. Is this the only patch pending for that driver ? Should I base off of v3.0 and everything should be ok ?
Nishanth Menon <nm@ti.com> writes: > SmartReflex should be disabled while entering low power mode due to > the following reasons: > a) SmartReflex values are not defined for retention voltage. > b) With SmartReflex enabled, if the CPU enters low power state, FSM > will try to bump the voltage to current OPP's voltage for which > it has entered low power state, causing power loss and potential > unknown states for the SoC. > Since we are sure to attempt entering the lowest possible power state > during suspend operation, SmartReflex needs to be disabled for the > voltage domains in suspend path before achieving auto retention voltage > on the device. > > Traditionally, this has been done with interrupts disabled as part of > the common code which handles the idle sequence. Instead, by using the > fact that we have to disable SmartReflex for sure during suspend > operations, we can opportunistically disable SmartReflex in device > standard pm ops, instead of disabling it as part of the code which > executes with interrupts disabled and slave CPU{s} shutdown. This > allows the system to do other parallel activities(such as suspending > other devices in the system using slave CPU{s}) and save the time > required to achieve suspend and resume from suspended state as a > sequential activity. > > However, by being opportunistic as described above, we also increase > the likelihood of SmartReflex library access functions being invoked in > parallel contexts *after* SmartReflex driver's suspend handler (during > suspend operation) or *before* resume handler (during resume operation) > have been invoked (Example: DVFS for dependent devices, cpufreq for > MPU etc.). We prevent this by using a flag to reject the callers in > the duration where SmartReflex has been disabled. > > Signed-off-by: Nishanth Menon <nm@ti.com> > --- > V2: more verbose changelog :) and SIMPLE_DEV_PM_OPS > V1: https://patchwork.kernel.org/patch/998312/ > > 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..8699682 100644 > --- a/arch/arm/mach-omap2/smartreflex.c > +++ b/arch/arm/mach-omap2/smartreflex.c > @@ -23,6 +23,7 @@ > #include <linux/debugfs.h> > #include <linux/delay.h> > #include <linux/slab.h> > +#include <linux/pm.h> > #include <linux/pm_runtime.h> > > #include <plat/common.h> > @@ -39,6 +40,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 +686,11 @@ 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,10 +1020,75 @@ static int __devexit omap_sr_remove(struct platform_device *pdev) > return 0; > } > > +static int omap_sr_suspend(struct device *dev) > +{ > + struct omap_sr_data *pdata; > + struct omap_sr *sr_info; > + > + pdata = dev_get_platdata(dev); > + if (!pdata) { > + dev_err(dev, "%s: platform data missing\n", __func__); > + return -EINVAL; > + } > + > + sr_info = _sr_lookup(pdata->voltdm); > + if (IS_ERR(sr_info)) { > + dev_warn(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); To be safest, I think the actual disable should be after setting the is_suspended flag. Otherwise, there's still a small window (right here) where SR is physically disabled, but is_suspended is false. > + sr_info->is_suspended = true; > + /* Flag the same info to the other CPUs */ > + smp_wmb(); also, the comment around the wmb isn't necesary. > + > + return 0; > +} > + > +static int omap_sr_resume(struct device *dev) > +{ > + struct omap_sr_data *pdata; > + struct omap_sr *sr_info; > + > + pdata = dev_get_platdata(dev); > + if (!pdata) { > + dev_err(dev, "%s: platform data missing\n", __func__); > + return -EINVAL; > + } > + > + sr_info = _sr_lookup(pdata->voltdm); > + if (IS_ERR(sr_info)) { > + dev_warn(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; Similarily here, you should probably not clear the flag until SR is actually fully enabled. Otherwise, there is still a small window for a race. > + /* Flag the same info to the other CPUs */ > + smp_wmb(); > + omap_sr_enable(pdata->voltdm); > + > + return 0; > +} > + > +static SIMPLE_DEV_PM_OPS(omap_sr_dev_pm_ops, omap_sr_suspend, omap_sr_resume); > + > static struct platform_driver smartreflex_driver = { > .remove = omap_sr_remove, > .driver = { > .name = "smartreflex", > + .pm = &omap_sr_dev_pm_ops, > }, > }; Felipe, Re: your comments about the !CONFIG_PM case. See the definition of SIMPLE_DEV_PM_OPS. That macro has the right #ifdef in it, so the !CONFIG_PM case is already taken care of by the macro. Kevin
diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c index 33a027f..8699682 100644 --- a/arch/arm/mach-omap2/smartreflex.c +++ b/arch/arm/mach-omap2/smartreflex.c @@ -23,6 +23,7 @@ #include <linux/debugfs.h> #include <linux/delay.h> #include <linux/slab.h> +#include <linux/pm.h> #include <linux/pm_runtime.h> #include <plat/common.h> @@ -39,6 +40,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 +686,11 @@ 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,10 +1020,75 @@ static int __devexit omap_sr_remove(struct platform_device *pdev) return 0; } +static int omap_sr_suspend(struct device *dev) +{ + struct omap_sr_data *pdata; + struct omap_sr *sr_info; + + pdata = dev_get_platdata(dev); + if (!pdata) { + dev_err(dev, "%s: platform data missing\n", __func__); + return -EINVAL; + } + + sr_info = _sr_lookup(pdata->voltdm); + if (IS_ERR(sr_info)) { + dev_warn(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 device *dev) +{ + struct omap_sr_data *pdata; + struct omap_sr *sr_info; + + pdata = dev_get_platdata(dev); + if (!pdata) { + dev_err(dev, "%s: platform data missing\n", __func__); + return -EINVAL; + } + + sr_info = _sr_lookup(pdata->voltdm); + if (IS_ERR(sr_info)) { + dev_warn(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 SIMPLE_DEV_PM_OPS(omap_sr_dev_pm_ops, omap_sr_suspend, omap_sr_resume); + static struct platform_driver smartreflex_driver = { .remove = omap_sr_remove, .driver = { .name = "smartreflex", + .pm = &omap_sr_dev_pm_ops, }, };
SmartReflex should be disabled while entering low power mode due to the following reasons: a) SmartReflex values are not defined for retention voltage. b) With SmartReflex enabled, if the CPU enters low power state, FSM will try to bump the voltage to current OPP's voltage for which it has entered low power state, causing power loss and potential unknown states for the SoC. Since we are sure to attempt entering the lowest possible power state during suspend operation, SmartReflex needs to be disabled for the voltage domains in suspend path before achieving auto retention voltage on the device. Traditionally, this has been done with interrupts disabled as part of the common code which handles the idle sequence. Instead, by using the fact that we have to disable SmartReflex for sure during suspend operations, we can opportunistically disable SmartReflex in device standard pm ops, instead of disabling it as part of the code which executes with interrupts disabled and slave CPU{s} shutdown. This allows the system to do other parallel activities(such as suspending other devices in the system using slave CPU{s}) and save the time required to achieve suspend and resume from suspended state as a sequential activity. However, by being opportunistic as described above, we also increase the likelihood of SmartReflex library access functions being invoked in parallel contexts *after* SmartReflex driver's suspend handler (during suspend operation) or *before* resume handler (during resume operation) have been invoked (Example: DVFS for dependent devices, cpufreq for MPU etc.). We prevent this by using a flag to reject the callers in the duration where SmartReflex has been disabled. Signed-off-by: Nishanth Menon <nm@ti.com> --- V2: more verbose changelog :) and SIMPLE_DEV_PM_OPS V1: https://patchwork.kernel.org/patch/998312/ arch/arm/mach-omap2/smartreflex.c | 87 +++++++++++++++++++++++++++++++++++++ 1 files changed, 87 insertions(+), 0 deletions(-)