| Message ID | 96495050.e1u1tibeG9@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Fri, Oct 19, 2012 at 08:08:38PM +0200, Rafael J. Wysocki wrote: > On Friday 19 of October 2012 01:39:25 Rafael J. Wysocki wrote: > > On Friday 12 of October 2012 11:12:41 Aaron Lu wrote: > > > In sdio bus level runtime callback function, after call the driver's > > > runtime suspend callback, we will check if the device supports a > > > platform level power management, and if so, a proper power state is > > > chosen by the corresponding platform callback and then set. > > > > > > Platform level runtime wakeup is also set, if device is enabled for > > > runtime wakeup by its driver, it will be armed the ability to generate > > > a wakeup event by the platform. > > > > > > Signed-off-by: Aaron Lu <aaron.lu@intel.com> > > > --- > > > drivers/mmc/core/sdio_bus.c | 49 +++++++++++++++++++++++++++++++++++++++++++-- > > > 1 file changed, 47 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c > > > index aaec9e2..d83dea8 100644 > > > --- a/drivers/mmc/core/sdio_bus.c > > > +++ b/drivers/mmc/core/sdio_bus.c > > > @@ -23,6 +23,7 @@ > > > > > > #include "sdio_cis.h" > > > #include "sdio_bus.h" > > > +#include "sdio.h" > > > #include "sdio_acpi.h" > > > > > > /* show configuration fields */ > > > @@ -194,10 +195,54 @@ static int sdio_bus_remove(struct device *dev) > > > } > > > > > > #ifdef CONFIG_PM > > > + > > > +static int sdio_bus_runtime_suspend(struct device *dev) > > > +{ > > > + int ret; > > > + sdio_power_t state; > > > + > > > + ret = pm_generic_runtime_suspend(dev); > > > + if (ret) > > > + goto out; > > > + > > > + if (!platform_sdio_power_manageable(dev)) > > > + goto out; > > > + > > > + platform_sdio_run_wake(dev, true); > > > + > > > + state = platform_sdio_choose_power_state(dev); > > > + if (state == SDIO_POWER_ERROR) { > > > + ret = -EIO; > > > + goto out; > > > + } > > > + > > > + ret = platform_sdio_set_power_state(dev, state); > > > + > > > +out: > > > + return ret; > > > +} > > > + > > > +static int sdio_bus_runtime_resume(struct device *dev) > > > +{ > > > + int ret; > > > + > > > + if (platform_sdio_power_manageable(dev)) { > > > + platform_sdio_run_wake(dev, false); > > > + ret = platform_sdio_set_power_state(dev, SDIO_D0); > > > + if (ret) > > > + goto out; > > > + } > > > + > > > + ret = pm_generic_runtime_resume(dev); > > > + > > > +out: > > > + return ret; > > > +} > > > + > > > > Most likely we will need to make analogous changes for other bus types that > > don't support power management natively, like platform, SPI, I2C etc. In all > > of them the _runtime_suspend() and _runtime_resume() routine will look > > almost exactly the same except for the platform_sdio_ prefix. > > > > For this reason, I think it would be better to simply define two functions > > acpi_pm_runtime_suspend() and acpi_pm_runtime_resume() that will do all of > > the ACPI-specific operations related to runtime suspend/resume. Then, we > > will be able to use these functions for all of the bus types in question > > in the same way (we may also need to add analogous functions for system > > suspend/resume handling). > > Something like in the (totally untested) patch below. Looks good to me. I'll test the code and put it into v2 of the patchset with your sign-off, is it OK? Thanks, Aaron > > With this code the SDIO bus type should be able to use > acpi_subsys_runtime_suspend() and acpi_subsys_runtime_resume() as its > bus-type-level runtime suspend/resume callback routines, respectively. > > Thanks, > Rafael > > > Prototype, no sign-off. > --- > drivers/acpi/dev_pm.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/acpi.h | 12 +++++++++++ > 2 files changed, 66 insertions(+) > > Index: linux-pm/drivers/acpi/dev_pm.c > =================================================================== > --- /dev/null > +++ linux-pm/drivers/acpi/dev_pm.c > @@ -0,0 +1,54 @@ > +#include <linux/acpi.h> > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/pm_qos.h> > +#include <linux/pm_runtime.h> > + > +#include <acpi/acpi_bus.h> > + > +int acpi_dev_runtime_suspend(struct device *dev) > +{ > + acpi_handle handle = DEVICE_ACPI_HANDLE(dev); > + bool enable_wakeup; > + int power_state; > + > + if (!handle || !acpi_bus_power_manageable(handle)) > + return 0; > + > + enable_wakeup = dev_pm_qos_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP)) > + != PM_QOS_FLAGS_NONE; > + acpi_pm_device_run_wake(dev, enable_wakeup); > + > + power_state = acpi_pm_device_sleep_state(dev, NULL, ACPI_STATE_D3); > + if (power_state < ACPI_STATE_D0 || power_state > ACPI_STATE_D3) > + return -EIO; > + > + return acpi_bus_set_power(handle, power_state); > +} > +EXPORT_SYMBOL_GPL(acpi_dev_runtime_suspend); > + > +int acpi_dev_runtime_resume(struct device *dev) > +{ > + acpi_handle handle = DEVICE_ACPI_HANDLE(dev); > + > + if (!handle || !acpi_bus_power_manageable(handle)) > + return 0; > + > + acpi_pm_device_run_wake(dev, false); > + return acpi_bus_set_power(handle, ACPI_STATE_D0); > +} > +EXPORT_SYMBOL_GPL(acpi_dev_runtime_resume); > + > +int acpi_subsys_runtime_suspend(struct device *dev) > +{ > + int ret = pm_generic_runtime_suspend(dev); > + return ret ? ret : acpi_dev_runtime_suspend(dev); > +} > +EXPORT_SYMBOL_GPL(acpi_subsys_runtime_suspend); > + > +int acpi_subsys_runtime_resume(struct device *dev) > +{ > + int ret = acpi_dev_runtime_resume(dev); > + return ret ? ret : pm_generic_runtime_resume(dev); > +} > +EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume); > Index: linux-pm/include/linux/acpi.h > =================================================================== > --- linux-pm.orig/include/linux/acpi.h > +++ linux-pm/include/linux/acpi.h > @@ -434,4 +434,16 @@ acpi_status acpi_os_prepare_sleep(u8 sle > #define acpi_os_set_prepare_sleep(func, pm1a_ctrl, pm1b_ctrl) do { } while (0) > #endif > > +#if defined(CONFIG_ACPI) && defined (CONFIG_PM_RUNTIME) > +int acpi_dev_runtime_suspend(struct device *dev); > +int acpi_dev_runtime_resume(struct device *dev); > +int acpi_subsys_runtime_suspend(struct device *dev); > +int acpi_subsys_runtime_resume(struct device *dev); > +#else > +static inline acpi_dev_runtime_suspend(struct device *dev) { return 0; } > +static inline acpi_dev_runtime_resume(struct device *dev) { return 0; } > +static inline acpi_subsys_runtime_suspend(struct device *dev) { return 0; } > +static inline acpi_subsys_runtime_resume(struct device *dev) { return 0; } > +#endif > + > #endif /*_LINUX_ACPI_H*/ > > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 20 of October 2012 15:15:41 Aaron Lu wrote: > On Fri, Oct 19, 2012 at 08:08:38PM +0200, Rafael J. Wysocki wrote: > > On Friday 19 of October 2012 01:39:25 Rafael J. Wysocki wrote: > > > On Friday 12 of October 2012 11:12:41 Aaron Lu wrote: > > > > In sdio bus level runtime callback function, after call the driver's > > > > runtime suspend callback, we will check if the device supports a > > > > platform level power management, and if so, a proper power state is > > > > chosen by the corresponding platform callback and then set. > > > > > > > > Platform level runtime wakeup is also set, if device is enabled for > > > > runtime wakeup by its driver, it will be armed the ability to generate > > > > a wakeup event by the platform. > > > > > > > > Signed-off-by: Aaron Lu <aaron.lu@intel.com> > > > > --- > > > > drivers/mmc/core/sdio_bus.c | 49 +++++++++++++++++++++++++++++++++++++++++++-- > > > > 1 file changed, 47 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c > > > > index aaec9e2..d83dea8 100644 > > > > --- a/drivers/mmc/core/sdio_bus.c > > > > +++ b/drivers/mmc/core/sdio_bus.c > > > > @@ -23,6 +23,7 @@ > > > > > > > > #include "sdio_cis.h" > > > > #include "sdio_bus.h" > > > > +#include "sdio.h" > > > > #include "sdio_acpi.h" > > > > > > > > /* show configuration fields */ > > > > @@ -194,10 +195,54 @@ static int sdio_bus_remove(struct device *dev) > > > > } > > > > > > > > #ifdef CONFIG_PM > > > > + > > > > +static int sdio_bus_runtime_suspend(struct device *dev) > > > > +{ > > > > + int ret; > > > > + sdio_power_t state; > > > > + > > > > + ret = pm_generic_runtime_suspend(dev); > > > > + if (ret) > > > > + goto out; > > > > + > > > > + if (!platform_sdio_power_manageable(dev)) > > > > + goto out; > > > > + > > > > + platform_sdio_run_wake(dev, true); > > > > + > > > > + state = platform_sdio_choose_power_state(dev); > > > > + if (state == SDIO_POWER_ERROR) { > > > > + ret = -EIO; > > > > + goto out; > > > > + } > > > > + > > > > + ret = platform_sdio_set_power_state(dev, state); > > > > + > > > > +out: > > > > + return ret; > > > > +} > > > > + > > > > +static int sdio_bus_runtime_resume(struct device *dev) > > > > +{ > > > > + int ret; > > > > + > > > > + if (platform_sdio_power_manageable(dev)) { > > > > + platform_sdio_run_wake(dev, false); > > > > + ret = platform_sdio_set_power_state(dev, SDIO_D0); > > > > + if (ret) > > > > + goto out; > > > > + } > > > > + > > > > + ret = pm_generic_runtime_resume(dev); > > > > + > > > > +out: > > > > + return ret; > > > > +} > > > > + > > > > > > Most likely we will need to make analogous changes for other bus types that > > > don't support power management natively, like platform, SPI, I2C etc. In all > > > of them the _runtime_suspend() and _runtime_resume() routine will look > > > almost exactly the same except for the platform_sdio_ prefix. > > > > > > For this reason, I think it would be better to simply define two functions > > > acpi_pm_runtime_suspend() and acpi_pm_runtime_resume() that will do all of > > > the ACPI-specific operations related to runtime suspend/resume. Then, we > > > will be able to use these functions for all of the bus types in question > > > in the same way (we may also need to add analogous functions for system > > > suspend/resume handling). > > > > Something like in the (totally untested) patch below. > > Looks good to me. > I'll test the code and put it into v2 of the patchset with your > sign-off, is it OK? I'd rather do it a bit differently in the signed-off version (I'm working on these patches, they should be ready around Tuesday), but if you can test it in its current form, that'd be useful too. Thanks, Rafael
On 10/22/2012 03:57 AM, Rafael J. Wysocki wrote: > On Saturday 20 of October 2012 15:15:41 Aaron Lu wrote: >> On Fri, Oct 19, 2012 at 08:08:38PM +0200, Rafael J. Wysocki wrote: >>> On Friday 19 of October 2012 01:39:25 Rafael J. Wysocki wrote: >>>> On Friday 12 of October 2012 11:12:41 Aaron Lu wrote: >>>>> In sdio bus level runtime callback function, after call the driver's >>>>> runtime suspend callback, we will check if the device supports a >>>>> platform level power management, and if so, a proper power state is >>>>> chosen by the corresponding platform callback and then set. >>>>> >>>>> Platform level runtime wakeup is also set, if device is enabled for >>>>> runtime wakeup by its driver, it will be armed the ability to generate >>>>> a wakeup event by the platform. >>>>> >>>>> Signed-off-by: Aaron Lu <aaron.lu@intel.com> >>>>> --- >>>>> drivers/mmc/core/sdio_bus.c | 49 +++++++++++++++++++++++++++++++++++++++++++-- >>>>> 1 file changed, 47 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c >>>>> index aaec9e2..d83dea8 100644 >>>>> --- a/drivers/mmc/core/sdio_bus.c >>>>> +++ b/drivers/mmc/core/sdio_bus.c >>>>> @@ -23,6 +23,7 @@ >>>>> >>>>> #include "sdio_cis.h" >>>>> #include "sdio_bus.h" >>>>> +#include "sdio.h" >>>>> #include "sdio_acpi.h" >>>>> >>>>> /* show configuration fields */ >>>>> @@ -194,10 +195,54 @@ static int sdio_bus_remove(struct device *dev) >>>>> } >>>>> >>>>> #ifdef CONFIG_PM >>>>> + >>>>> +static int sdio_bus_runtime_suspend(struct device *dev) >>>>> +{ >>>>> + int ret; >>>>> + sdio_power_t state; >>>>> + >>>>> + ret = pm_generic_runtime_suspend(dev); >>>>> + if (ret) >>>>> + goto out; >>>>> + >>>>> + if (!platform_sdio_power_manageable(dev)) >>>>> + goto out; >>>>> + >>>>> + platform_sdio_run_wake(dev, true); >>>>> + >>>>> + state = platform_sdio_choose_power_state(dev); >>>>> + if (state == SDIO_POWER_ERROR) { >>>>> + ret = -EIO; >>>>> + goto out; >>>>> + } >>>>> + >>>>> + ret = platform_sdio_set_power_state(dev, state); >>>>> + >>>>> +out: >>>>> + return ret; >>>>> +} >>>>> + >>>>> +static int sdio_bus_runtime_resume(struct device *dev) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + if (platform_sdio_power_manageable(dev)) { >>>>> + platform_sdio_run_wake(dev, false); >>>>> + ret = platform_sdio_set_power_state(dev, SDIO_D0); >>>>> + if (ret) >>>>> + goto out; >>>>> + } >>>>> + >>>>> + ret = pm_generic_runtime_resume(dev); >>>>> + >>>>> +out: >>>>> + return ret; >>>>> +} >>>>> + >>>> >>>> Most likely we will need to make analogous changes for other bus types that >>>> don't support power management natively, like platform, SPI, I2C etc. In all >>>> of them the _runtime_suspend() and _runtime_resume() routine will look >>>> almost exactly the same except for the platform_sdio_ prefix. >>>> >>>> For this reason, I think it would be better to simply define two functions >>>> acpi_pm_runtime_suspend() and acpi_pm_runtime_resume() that will do all of >>>> the ACPI-specific operations related to runtime suspend/resume. Then, we >>>> will be able to use these functions for all of the bus types in question >>>> in the same way (we may also need to add analogous functions for system >>>> suspend/resume handling). >>> >>> Something like in the (totally untested) patch below. >> >> Looks good to me. >> I'll test the code and put it into v2 of the patchset with your >> sign-off, is it OK? > > I'd rather do it a bit differently in the signed-off version (I'm working > on these patches, they should be ready around Tuesday), but if you can test OK, thanks. > it in its current form, that'd be useful too. I was planning to test it some time later, so looks like I can directly test your signed-off version :-) Thanks, Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-pm/drivers/acpi/dev_pm.c =================================================================== --- /dev/null +++ linux-pm/drivers/acpi/dev_pm.c @@ -0,0 +1,54 @@ +#include <linux/acpi.h> +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/pm_qos.h> +#include <linux/pm_runtime.h> + +#include <acpi/acpi_bus.h> + +int acpi_dev_runtime_suspend(struct device *dev) +{ + acpi_handle handle = DEVICE_ACPI_HANDLE(dev); + bool enable_wakeup; + int power_state; + + if (!handle || !acpi_bus_power_manageable(handle)) + return 0; + + enable_wakeup = dev_pm_qos_flags(dev, PM_QOS_FLAG_REMOTE_WAKEUP)) + != PM_QOS_FLAGS_NONE; + acpi_pm_device_run_wake(dev, enable_wakeup); + + power_state = acpi_pm_device_sleep_state(dev, NULL, ACPI_STATE_D3); + if (power_state < ACPI_STATE_D0 || power_state > ACPI_STATE_D3) + return -EIO; + + return acpi_bus_set_power(handle, power_state); +} +EXPORT_SYMBOL_GPL(acpi_dev_runtime_suspend); + +int acpi_dev_runtime_resume(struct device *dev) +{ + acpi_handle handle = DEVICE_ACPI_HANDLE(dev); + + if (!handle || !acpi_bus_power_manageable(handle)) + return 0; + + acpi_pm_device_run_wake(dev, false); + return acpi_bus_set_power(handle, ACPI_STATE_D0); +} +EXPORT_SYMBOL_GPL(acpi_dev_runtime_resume); + +int acpi_subsys_runtime_suspend(struct device *dev) +{ + int ret = pm_generic_runtime_suspend(dev); + return ret ? ret : acpi_dev_runtime_suspend(dev); +} +EXPORT_SYMBOL_GPL(acpi_subsys_runtime_suspend); + +int acpi_subsys_runtime_resume(struct device *dev) +{ + int ret = acpi_dev_runtime_resume(dev); + return ret ? ret : pm_generic_runtime_resume(dev); +} +EXPORT_SYMBOL_GPL(acpi_subsys_runtime_resume); Index: linux-pm/include/linux/acpi.h =================================================================== --- linux-pm.orig/include/linux/acpi.h +++ linux-pm/include/linux/acpi.h @@ -434,4 +434,16 @@ acpi_status acpi_os_prepare_sleep(u8 sle #define acpi_os_set_prepare_sleep(func, pm1a_ctrl, pm1b_ctrl) do { } while (0) #endif +#if defined(CONFIG_ACPI) && defined (CONFIG_PM_RUNTIME) +int acpi_dev_runtime_suspend(struct device *dev); +int acpi_dev_runtime_resume(struct device *dev); +int acpi_subsys_runtime_suspend(struct device *dev); +int acpi_subsys_runtime_resume(struct device *dev); +#else +static inline acpi_dev_runtime_suspend(struct device *dev) { return 0; } +static inline acpi_dev_runtime_resume(struct device *dev) { return 0; } +static inline acpi_subsys_runtime_suspend(struct device *dev) { return 0; } +static inline acpi_subsys_runtime_resume(struct device *dev) { return 0; } +#endif + #endif /*_LINUX_ACPI_H*/