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

[Menon, Nishanth]

On Mon, Jul 25, 2011 at 03:42, Felipe Balbi <balbi at 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