| Message ID | 20170623010837.11199-2-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote: > Add an optional platform_suspend_ops callback: target_state, and a > helper function globally visible to get this called: > platform_suspend_target_state(). > > This is useful for platform specific drivers that may need to take a > slightly different suspend/resume path based on the system's > suspend/resume state being entered. > > Although this callback is optional and documented as such, it requires > a platform_suspend_ops::begin callback to be implemented in order to > provide an accurate suspend/resume state within the driver that > implements this platform_suspend_ops. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > include/linux/suspend.h | 12 ++++++++++++ > kernel/power/suspend.c | 15 +++++++++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h > index d9718378a8be..d998a04a90a2 100644 > --- a/include/linux/suspend.h > +++ b/include/linux/suspend.h > @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step) > * Called by the PM core if the suspending of devices fails. > * This callback is optional and should only be implemented by platforms > * which require special recovery actions in that situation. > + * > + * @target_state: Returns the suspend state the suspend_ops will be entering. > + * Called by device drivers that need to know the platform specific suspend > + * state the system is about to enter. > + * This callback is optional and should only be implemented by platforms > + * which require special handling of power management states within > + * drivers. It does require @begin to be implemented to provide the suspend > + * state. Return value is platform_suspend_ops specific, and may be a 1:1 > + * mapping to suspend_state_t when relevant. > */ > struct platform_suspend_ops { > int (*valid)(suspend_state_t state); > @@ -184,6 +193,7 @@ struct platform_suspend_ops { > bool (*suspend_again)(void); > void (*end)(void); > void (*recover)(void); > + int (*target_state)(void); I would use unsigned int (the sign should not matter). > }; That's almost what I was thinking about except that the values returned by ->target_state should be unique, so it would be good to do something to ensure that. The concern is as follows. Say you have a driver develped for platform X where ->target_state returns A for "mem" and B for "standby". Then, the same IP is re-used on platform Y returning B for "mem" and C for "standby" and now the driver cannot distinguish between them. Moreover, even if they both returned A for "mem" there might be differences in how "mem" was defined by each of them and therefore in what the driver was expected to do to handle "mem" on X and Y. Thanks, Rafael
On Sun 2017-07-16 01:29:57, Rafael J. Wysocki wrote: > On Saturday, July 15, 2017 06:46:26 PM Pavel Machek wrote: > > Hi! > > > > > > > I had an idea of using an enum type encompassing all of the power states > > > > > defined for various platforms and serving both as a registry (to ensure the > > > > > uniqueness of the values assigned to the states) and a common ground > > > > > between platforms and drivers. > > > > > > > > > > Something like: > > > > > > > > > > enum platform_target_state { > > > > > PLATFORM_STATE_UNKNOWN = -1, > > > > > PLATFORM_STATE_WORKING = 0, > > > > > PLATFORM_STATE_ACPI_S1, > > > > > PLATFORM_STATE_ACPI_S2, > > > > > PLATFORM_STATE_ACPI_S3, > > > > > PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS, > > > > > PLATFORM_STATE_MY_BOARD_1_GATE_POWER, > > > > > PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF, > > > > > ... > > > > > }; > > > > > > > > > > and define ->target_state to return a value of this type. > > > > > > > > > > Then, if a driver sees one of these and recognizes that value, it should > > > > > know exactly what to do. > > > > > > > > Remind me why this is good idea? > > > > > > Because there are drivers that need to do specific things during > > > suspend on a specific board when it goes into a specific state as a > > > whole. > > > > We have seen driver that cares about voltage to his device being > > lost. That's reasonable. > > > > Inquiring what the platform target state is... is not. > > So why exactly isn't it reasonable? > > Please use technical arguments. Saying that something is wrong without > explaining the problem you see with it isn't particulatly useful in technical > discussions. Deep in your heart, you should know that having enum listing all the platforms linux runs on is a very bad idea. Anyway, there are better solutions, regulator framework already knows if given rail will be powered off or not, and their driver already knows if they are going suspend/standby. They just need to use existing interfaces. Pavel
On Sat 2017-07-15 20:33:58, Alexandre Belloni wrote: > On 15/07/2017 at 10:20:27 -0700, Florian Fainelli wrote: > > > We already have > > > > > > struct regulator_state { > > > int uV; /* suspend voltage */ > > > unsigned int mode; /* suspend regulator operating mode */ > > > int enabled; /* is regulator enabled in this suspend state */ > > > int disabled; /* is the regulator disabled in this suspend state */ > > > }; > > > > > > * struct regulation_constraints - regulator operating constraints. > > > * @state_disk: State for regulator when system is suspended in disk > > > * mode. > > > * @state_mem: State for regulator when system is suspended in mem > > > * mode. > > > * @state_standby: State for regulator when system is suspended in > > > * standby > > > * mode. > > > > > > . So it seems that maybe we should tell the drivers if we are entering > > > "state_mem" or "state_standby" (something I may have opposed, sorry), > > > then the driver can get neccessary information from regulator > > > framework. > > > > OK, so what would be the mechanism to tell these drivers about the > > system wide suspend state they are entering if it is not via > > platform_suspend_target_state()? > > > > Keep in mind that regulators might be one aspect of what could be > > causing the platform to behave specifically in one suspend state vs. > > another, but there could be pieces of HW within the SoC that can't be > > described with power domains, voltage islands etc. that would still have > > inherent suspend states properties (like memory retention, pin/pad > > controls etc. etc). We still need some mechanism, possibly centralized > > > > I concur, the regulator stuff is one aspect of one of our suspend state > (cutting VDDcore). But we have another state where the main clock (going > to the IPs) is going from a few hundred MHz to 32kHz. This is currently > handled by calling at91_suspend_entering_slow_clock(). I think it is > important to take that into account so we can remove this hack from the > kernel. Cure should not be worse then the disease... and it is in this case. For clocks, take a look at clock framework, perhaps it already has "clock_will_be_suspended" as regulator framework had. If not, implement it. Same with memory retention, pin/pad controls. Pavel
On 06/29/2017 04:00 PM, Rafael J. Wysocki wrote: > On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote: >> Add an optional platform_suspend_ops callback: target_state, and a >> helper function globally visible to get this called: >> platform_suspend_target_state(). >> >> This is useful for platform specific drivers that may need to take a >> slightly different suspend/resume path based on the system's >> suspend/resume state being entered. >> >> Although this callback is optional and documented as such, it requires >> a platform_suspend_ops::begin callback to be implemented in order to >> provide an accurate suspend/resume state within the driver that >> implements this platform_suspend_ops. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> include/linux/suspend.h | 12 ++++++++++++ >> kernel/power/suspend.c | 15 +++++++++++++++ >> 2 files changed, 27 insertions(+) >> >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h >> index d9718378a8be..d998a04a90a2 100644 >> --- a/include/linux/suspend.h >> +++ b/include/linux/suspend.h >> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step) >> * Called by the PM core if the suspending of devices fails. >> * This callback is optional and should only be implemented by platforms >> * which require special recovery actions in that situation. >> + * >> + * @target_state: Returns the suspend state the suspend_ops will be entering. >> + * Called by device drivers that need to know the platform specific suspend >> + * state the system is about to enter. >> + * This callback is optional and should only be implemented by platforms >> + * which require special handling of power management states within >> + * drivers. It does require @begin to be implemented to provide the suspend >> + * state. Return value is platform_suspend_ops specific, and may be a 1:1 >> + * mapping to suspend_state_t when relevant. >> */ >> struct platform_suspend_ops { >> int (*valid)(suspend_state_t state); >> @@ -184,6 +193,7 @@ struct platform_suspend_ops { >> bool (*suspend_again)(void); >> void (*end)(void); >> void (*recover)(void); >> + int (*target_state)(void); > > I would use unsigned int (the sign should not matter). > >> }; > > That's almost what I was thinking about except that the values returned by > ->target_state should be unique, so it would be good to do something to > ensure that. > > The concern is as follows. > > Say you have a driver develped for platform X where ->target_state returns > A for "mem" and B for "standby". Then, the same IP is re-used on platform Y > returning B for "mem" and C for "standby" and now the driver cannot > distinguish between them. > > Moreover, even if they both returned A for "mem" there might be differences > in how "mem" was defined by each of them and therefore in what the driver was > expected to do to handle "mem" on X and Y. That makes sense, would you need the core implementation in platform_suspend_target_state() to range check what suspend_ops->target_state() returns against a set of reserved value say, checking from 0 up to ACPI_S_STATE_COUNT or is there another range you would like to see being used? Thanks!
On Wednesday, July 12, 2017 11:08:19 AM Florian Fainelli wrote: > On 06/29/2017 04:00 PM, Rafael J. Wysocki wrote: > > On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote: > >> Add an optional platform_suspend_ops callback: target_state, and a > >> helper function globally visible to get this called: > >> platform_suspend_target_state(). > >> > >> This is useful for platform specific drivers that may need to take a > >> slightly different suspend/resume path based on the system's > >> suspend/resume state being entered. > >> > >> Although this callback is optional and documented as such, it requires > >> a platform_suspend_ops::begin callback to be implemented in order to > >> provide an accurate suspend/resume state within the driver that > >> implements this platform_suspend_ops. > >> > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> --- > >> include/linux/suspend.h | 12 ++++++++++++ > >> kernel/power/suspend.c | 15 +++++++++++++++ > >> 2 files changed, 27 insertions(+) > >> > >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h > >> index d9718378a8be..d998a04a90a2 100644 > >> --- a/include/linux/suspend.h > >> +++ b/include/linux/suspend.h > >> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step) > >> * Called by the PM core if the suspending of devices fails. > >> * This callback is optional and should only be implemented by platforms > >> * which require special recovery actions in that situation. > >> + * > >> + * @target_state: Returns the suspend state the suspend_ops will be entering. > >> + * Called by device drivers that need to know the platform specific suspend > >> + * state the system is about to enter. > >> + * This callback is optional and should only be implemented by platforms > >> + * which require special handling of power management states within > >> + * drivers. It does require @begin to be implemented to provide the suspend > >> + * state. Return value is platform_suspend_ops specific, and may be a 1:1 > >> + * mapping to suspend_state_t when relevant. > >> */ > >> struct platform_suspend_ops { > >> int (*valid)(suspend_state_t state); > >> @@ -184,6 +193,7 @@ struct platform_suspend_ops { > >> bool (*suspend_again)(void); > >> void (*end)(void); > >> void (*recover)(void); > >> + int (*target_state)(void); > > > > I would use unsigned int (the sign should not matter). > > > >> }; > > > > That's almost what I was thinking about except that the values returned by > > ->target_state should be unique, so it would be good to do something to > > ensure that. > > > > The concern is as follows. > > > > Say you have a driver develped for platform X where ->target_state returns > > A for "mem" and B for "standby". Then, the same IP is re-used on platform Y > > returning B for "mem" and C for "standby" and now the driver cannot > > distinguish between them. > > > > Moreover, even if they both returned A for "mem" there might be differences > > in how "mem" was defined by each of them and therefore in what the driver was > > expected to do to handle "mem" on X and Y. > > That makes sense, would you need the core implementation in > platform_suspend_target_state() to range check what > suspend_ops->target_state() returns against a set of reserved value say, > checking from 0 up to ACPI_S_STATE_COUNT or is there another range you > would like to see being used? I had an idea of using an enum type encompassing all of the power states defined for various platforms and serving both as a registry (to ensure the uniqueness of the values assigned to the states) and a common ground between platforms and drivers. Something like: enum platform_target_state { PLATFORM_STATE_UNKNOWN = -1, PLATFORM_STATE_WORKING = 0, PLATFORM_STATE_ACPI_S1, PLATFORM_STATE_ACPI_S2, PLATFORM_STATE_ACPI_S3, PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS, PLATFORM_STATE_MY_BOARD_1_GATE_POWER, PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF, ... }; and define ->target_state to return a value of this type. Then, if a driver sees one of these and recognizes that value, it should know exactly what to do. Thanks, Rafael
On Sat 2017-07-15 00:16:16, Rafael J. Wysocki wrote: > On Wednesday, July 12, 2017 11:08:19 AM Florian Fainelli wrote: > > On 06/29/2017 04:00 PM, Rafael J. Wysocki wrote: > > > On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote: > > >> Add an optional platform_suspend_ops callback: target_state, and a > > >> helper function globally visible to get this called: > > >> platform_suspend_target_state(). > > >> > > >> This is useful for platform specific drivers that may need to take a > > >> slightly different suspend/resume path based on the system's > > >> suspend/resume state being entered. > > >> > > >> Although this callback is optional and documented as such, it requires > > >> a platform_suspend_ops::begin callback to be implemented in order to > > >> provide an accurate suspend/resume state within the driver that > > >> implements this platform_suspend_ops. > > >> > > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > >> --- > > >> include/linux/suspend.h | 12 ++++++++++++ > > >> kernel/power/suspend.c | 15 +++++++++++++++ > > >> 2 files changed, 27 insertions(+) > > >> > > >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h > > >> index d9718378a8be..d998a04a90a2 100644 > > >> --- a/include/linux/suspend.h > > >> +++ b/include/linux/suspend.h > > >> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step) > > >> * Called by the PM core if the suspending of devices fails. > > >> * This callback is optional and should only be implemented by platforms > > >> * which require special recovery actions in that situation. > > >> + * > > >> + * @target_state: Returns the suspend state the suspend_ops will be entering. > > >> + * Called by device drivers that need to know the platform specific suspend > > >> + * state the system is about to enter. > > >> + * This callback is optional and should only be implemented by platforms > > >> + * which require special handling of power management states within > > >> + * drivers. It does require @begin to be implemented to provide the suspend > > >> + * state. Return value is platform_suspend_ops specific, and may be a 1:1 > > >> + * mapping to suspend_state_t when relevant. > > >> */ > > >> struct platform_suspend_ops { > > >> int (*valid)(suspend_state_t state); > > >> @@ -184,6 +193,7 @@ struct platform_suspend_ops { > > >> bool (*suspend_again)(void); > > >> void (*end)(void); > > >> void (*recover)(void); > > >> + int (*target_state)(void); > > > > > > I would use unsigned int (the sign should not matter). > > > > > >> }; > > > > > > That's almost what I was thinking about except that the values returned by > > > ->target_state should be unique, so it would be good to do something to > > > ensure that. > > > > > > The concern is as follows. > > > > > > Say you have a driver develped for platform X where ->target_state returns > > > A for "mem" and B for "standby". Then, the same IP is re-used on platform Y > > > returning B for "mem" and C for "standby" and now the driver cannot > > > distinguish between them. > > > > > > Moreover, even if they both returned A for "mem" there might be differences > > > in how "mem" was defined by each of them and therefore in what the driver was > > > expected to do to handle "mem" on X and Y. > > > > That makes sense, would you need the core implementation in > > platform_suspend_target_state() to range check what > > suspend_ops->target_state() returns against a set of reserved value say, > > checking from 0 up to ACPI_S_STATE_COUNT or is there another range you > > would like to see being used? > > I had an idea of using an enum type encompassing all of the power states > defined for various platforms and serving both as a registry (to ensure the > uniqueness of the values assigned to the states) and a common ground > between platforms and drivers. > > Something like: > > enum platform_target_state { > PLATFORM_STATE_UNKNOWN = -1, > PLATFORM_STATE_WORKING = 0, > PLATFORM_STATE_ACPI_S1, > PLATFORM_STATE_ACPI_S2, > PLATFORM_STATE_ACPI_S3, > PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS, > PLATFORM_STATE_MY_BOARD_1_GATE_POWER, > PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF, > ... > }; > > and define ->target_state to return a value of this type. > > Then, if a driver sees one of these and recognizes that value, it should > know exactly what to do. Remind me why this is good idea? We currently have 1364+ boards in tree. That will be rather large enum. If board wants to know if certain regulator stays online during suspend, it should invent an API for _that_. Pavel
On Saturday, July 15, 2017 08:28:38 AM Pavel Machek wrote: > On Sat 2017-07-15 00:16:16, Rafael J. Wysocki wrote: > > On Wednesday, July 12, 2017 11:08:19 AM Florian Fainelli wrote: > > > On 06/29/2017 04:00 PM, Rafael J. Wysocki wrote: > > > > On Thursday, June 22, 2017 06:08:36 PM Florian Fainelli wrote: > > > >> Add an optional platform_suspend_ops callback: target_state, and a > > > >> helper function globally visible to get this called: > > > >> platform_suspend_target_state(). > > > >> > > > >> This is useful for platform specific drivers that may need to take a > > > >> slightly different suspend/resume path based on the system's > > > >> suspend/resume state being entered. > > > >> > > > >> Although this callback is optional and documented as such, it requires > > > >> a platform_suspend_ops::begin callback to be implemented in order to > > > >> provide an accurate suspend/resume state within the driver that > > > >> implements this platform_suspend_ops. > > > >> > > > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > > >> --- > > > >> include/linux/suspend.h | 12 ++++++++++++ > > > >> kernel/power/suspend.c | 15 +++++++++++++++ > > > >> 2 files changed, 27 insertions(+) > > > >> > > > >> diff --git a/include/linux/suspend.h b/include/linux/suspend.h > > > >> index d9718378a8be..d998a04a90a2 100644 > > > >> --- a/include/linux/suspend.h > > > >> +++ b/include/linux/suspend.h > > > >> @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step) > > > >> * Called by the PM core if the suspending of devices fails. > > > >> * This callback is optional and should only be implemented by platforms > > > >> * which require special recovery actions in that situation. > > > >> + * > > > >> + * @target_state: Returns the suspend state the suspend_ops will be entering. > > > >> + * Called by device drivers that need to know the platform specific suspend > > > >> + * state the system is about to enter. > > > >> + * This callback is optional and should only be implemented by platforms > > > >> + * which require special handling of power management states within > > > >> + * drivers. It does require @begin to be implemented to provide the suspend > > > >> + * state. Return value is platform_suspend_ops specific, and may be a 1:1 > > > >> + * mapping to suspend_state_t when relevant. > > > >> */ > > > >> struct platform_suspend_ops { > > > >> int (*valid)(suspend_state_t state); > > > >> @@ -184,6 +193,7 @@ struct platform_suspend_ops { > > > >> bool (*suspend_again)(void); > > > >> void (*end)(void); > > > >> void (*recover)(void); > > > >> + int (*target_state)(void); > > > > > > > > I would use unsigned int (the sign should not matter). > > > > > > > >> }; > > > > > > > > That's almost what I was thinking about except that the values returned by > > > > ->target_state should be unique, so it would be good to do something to > > > > ensure that. > > > > > > > > The concern is as follows. > > > > > > > > Say you have a driver develped for platform X where ->target_state returns > > > > A for "mem" and B for "standby". Then, the same IP is re-used on platform Y > > > > returning B for "mem" and C for "standby" and now the driver cannot > > > > distinguish between them. > > > > > > > > Moreover, even if they both returned A for "mem" there might be differences > > > > in how "mem" was defined by each of them and therefore in what the driver was > > > > expected to do to handle "mem" on X and Y. > > > > > > That makes sense, would you need the core implementation in > > > platform_suspend_target_state() to range check what > > > suspend_ops->target_state() returns against a set of reserved value say, > > > checking from 0 up to ACPI_S_STATE_COUNT or is there another range you > > > would like to see being used? > > > > I had an idea of using an enum type encompassing all of the power states > > defined for various platforms and serving both as a registry (to ensure the > > uniqueness of the values assigned to the states) and a common ground > > between platforms and drivers. > > > > Something like: > > > > enum platform_target_state { > > PLATFORM_STATE_UNKNOWN = -1, > > PLATFORM_STATE_WORKING = 0, > > PLATFORM_STATE_ACPI_S1, > > PLATFORM_STATE_ACPI_S2, > > PLATFORM_STATE_ACPI_S3, > > PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS, > > PLATFORM_STATE_MY_BOARD_1_GATE_POWER, > > PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF, > > ... > > }; > > > > and define ->target_state to return a value of this type. > > > > Then, if a driver sees one of these and recognizes that value, it should > > know exactly what to do. > > Remind me why this is good idea? Because there are drivers that need to do specific things during suspend on a specific board when it goes into a specific state as a whole. > We currently have 1364+ boards in tree. That will be rather large > enum. Fortunately enough, only some of the platform states need to be listed here, the ones that drivers need to find out about. The vast majority of drivers do the same thing regardless of the target state of the platform. > If board wants to know if certain regulator stays online during > suspend, it should invent an API for _that_. Ideally, yes. However, that may be problematic for multiplatform kernels, because they would need to have all of those APIs built in and the driver code to figure out which API to use would be rather nasty. Thanks, Rafael
Hi! > > > I had an idea of using an enum type encompassing all of the power states > > > defined for various platforms and serving both as a registry (to ensure the > > > uniqueness of the values assigned to the states) and a common ground > > > between platforms and drivers. > > > > > > Something like: > > > > > > enum platform_target_state { > > > PLATFORM_STATE_UNKNOWN = -1, > > > PLATFORM_STATE_WORKING = 0, > > > PLATFORM_STATE_ACPI_S1, > > > PLATFORM_STATE_ACPI_S2, > > > PLATFORM_STATE_ACPI_S3, > > > PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS, > > > PLATFORM_STATE_MY_BOARD_1_GATE_POWER, > > > PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF, > > > ... > > > }; > > > > > > and define ->target_state to return a value of this type. > > > > > > Then, if a driver sees one of these and recognizes that value, it should > > > know exactly what to do. > > > > Remind me why this is good idea? > > Because there are drivers that need to do specific things during > suspend on a specific board when it goes into a specific state as a > whole. We have seen driver that cares about voltage to his device being lost. That's reasonable. Inquiring what the platform target state is... is not. > > If board wants to know if certain regulator stays online during > > suspend, it should invent an API for _that_. > > Ideally, yes. However, that may be problematic for multiplatform kernels, > because they would need to have all of those APIs built in and the driver > code to figure out which API to use would be rather nasty. Lets do it the right way. Big enum is wrong. We already have struct regulator_state { int uV; /* suspend voltage */ unsigned int mode; /* suspend regulator operating mode */ int enabled; /* is regulator enabled in this suspend state */ int disabled; /* is the regulator disabled in this suspend state */ }; * struct regulation_constraints - regulator operating constraints. * @state_disk: State for regulator when system is suspended in disk * mode. * @state_mem: State for regulator when system is suspended in mem * mode. * @state_standby: State for regulator when system is suspended in * standby * mode. . So it seems that maybe we should tell the drivers if we are entering "state_mem" or "state_standby" (something I may have opposed, sorry), then the driver can get neccessary information from regulator framework. I don't think it should cause problems with multiplatform kernels. Best regards, Pavel
On 07/15/2017 09:46 AM, Pavel Machek wrote: > Hi! > >>>> I had an idea of using an enum type encompassing all of the power states >>>> defined for various platforms and serving both as a registry (to ensure the >>>> uniqueness of the values assigned to the states) and a common ground >>>> between platforms and drivers. >>>> >>>> Something like: >>>> >>>> enum platform_target_state { >>>> PLATFORM_STATE_UNKNOWN = -1, >>>> PLATFORM_STATE_WORKING = 0, >>>> PLATFORM_STATE_ACPI_S1, >>>> PLATFORM_STATE_ACPI_S2, >>>> PLATFORM_STATE_ACPI_S3, >>>> PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS, >>>> PLATFORM_STATE_MY_BOARD_1_GATE_POWER, >>>> PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF, >>>> ... >>>> }; >>>> >>>> and define ->target_state to return a value of this type. >>>> >>>> Then, if a driver sees one of these and recognizes that value, it should >>>> know exactly what to do. >>> >>> Remind me why this is good idea? >> >> Because there are drivers that need to do specific things during >> suspend on a specific board when it goes into a specific state as a >> whole. > > We have seen driver that cares about voltage to his device being > lost. That's reasonable. > > Inquiring what the platform target state is... is not. > >>> If board wants to know if certain regulator stays online during >>> suspend, it should invent an API for _that_. >> >> Ideally, yes. However, that may be problematic for multiplatform kernels, >> because they would need to have all of those APIs built in and the driver >> code to figure out which API to use would be rather nasty. > > Lets do it the right way. Big enum is wrong. The enum offers the advantage of centralizing how many different states exist for all the platforms we know about in the kernel, it's easy to define common values for platforms that have the same semantics, just like it's simple to add new values for platform specific details. Is the concern that we could overflow the enum size, or that there is not a common set of values to describe states? > > We already have > > struct regulator_state { > int uV; /* suspend voltage */ > unsigned int mode; /* suspend regulator operating mode */ > int enabled; /* is regulator enabled in this suspend state */ > int disabled; /* is the regulator disabled in this suspend state */ > }; > > * struct regulation_constraints - regulator operating constraints. > * @state_disk: State for regulator when system is suspended in disk > * mode. > * @state_mem: State for regulator when system is suspended in mem > * mode. > * @state_standby: State for regulator when system is suspended in > * standby > * mode. > > . So it seems that maybe we should tell the drivers if we are entering > "state_mem" or "state_standby" (something I may have opposed, sorry), > then the driver can get neccessary information from regulator > framework. OK, so what would be the mechanism to tell these drivers about the system wide suspend state they are entering if it is not via platform_suspend_target_state()? Keep in mind that regulators might be one aspect of what could be causing the platform to behave specifically in one suspend state vs. another, but there could be pieces of HW within the SoC that can't be described with power domains, voltage islands etc. that would still have inherent suspend states properties (like memory retention, pin/pad controls etc. etc). We still need some mechanism, possibly centralized > > I don't think it should cause problems with multiplatform kernels. Just like platform_suspend_target_state() with an enum is not creating problems either. suspend_ops is already a singleton for a given kernel image so a given kernel running on a given platform will get to see a subset of the enum values defined. In any case, just agree and I will be happy to follow-up with patches.
On 15/07/2017 at 10:20:27 -0700, Florian Fainelli wrote: > > We already have > > > > struct regulator_state { > > int uV; /* suspend voltage */ > > unsigned int mode; /* suspend regulator operating mode */ > > int enabled; /* is regulator enabled in this suspend state */ > > int disabled; /* is the regulator disabled in this suspend state */ > > }; > > > > * struct regulation_constraints - regulator operating constraints. > > * @state_disk: State for regulator when system is suspended in disk > > * mode. > > * @state_mem: State for regulator when system is suspended in mem > > * mode. > > * @state_standby: State for regulator when system is suspended in > > * standby > > * mode. > > > > . So it seems that maybe we should tell the drivers if we are entering > > "state_mem" or "state_standby" (something I may have opposed, sorry), > > then the driver can get neccessary information from regulator > > framework. > > OK, so what would be the mechanism to tell these drivers about the > system wide suspend state they are entering if it is not via > platform_suspend_target_state()? > > Keep in mind that regulators might be one aspect of what could be > causing the platform to behave specifically in one suspend state vs. > another, but there could be pieces of HW within the SoC that can't be > described with power domains, voltage islands etc. that would still have > inherent suspend states properties (like memory retention, pin/pad > controls etc. etc). We still need some mechanism, possibly centralized > I concur, the regulator stuff is one aspect of one of our suspend state (cutting VDDcore). But we have another state where the main clock (going to the IPs) is going from a few hundred MHz to 32kHz. This is currently handled by calling at91_suspend_entering_slow_clock(). I think it is important to take that into account so we can remove this hack from the kernel.
On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote: > > On 07/15/2017 09:46 AM, Pavel Machek wrote: > > Hi! > > > >>>> I had an idea of using an enum type encompassing all of the power states > >>>> defined for various platforms and serving both as a registry (to ensure the > >>>> uniqueness of the values assigned to the states) and a common ground > >>>> between platforms and drivers. > >>>> > >>>> Something like: > >>>> > >>>> enum platform_target_state { > >>>> PLATFORM_STATE_UNKNOWN = -1, > >>>> PLATFORM_STATE_WORKING = 0, > >>>> PLATFORM_STATE_ACPI_S1, > >>>> PLATFORM_STATE_ACPI_S2, > >>>> PLATFORM_STATE_ACPI_S3, > >>>> PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS, > >>>> PLATFORM_STATE_MY_BOARD_1_GATE_POWER, > >>>> PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF, > >>>> ... > >>>> }; > >>>> > >>>> and define ->target_state to return a value of this type. > >>>> > >>>> Then, if a driver sees one of these and recognizes that value, it should > >>>> know exactly what to do. > >>> > >>> Remind me why this is good idea? > >> > >> Because there are drivers that need to do specific things during > >> suspend on a specific board when it goes into a specific state as a > >> whole. > > > > We have seen driver that cares about voltage to his device being > > lost. That's reasonable. > > > > Inquiring what the platform target state is... is not. > > > >>> If board wants to know if certain regulator stays online during > >>> suspend, it should invent an API for _that_. > >> > >> Ideally, yes. However, that may be problematic for multiplatform kernels, > >> because they would need to have all of those APIs built in and the driver > >> code to figure out which API to use would be rather nasty. > > > > Lets do it the right way. Big enum is wrong. > > The enum offers the advantage of centralizing how many different states > exist for all the platforms we know about in the kernel, it's easy to > define common values for platforms that have the same semantics, just > like it's simple to add new values for platform specific details. Well, you seem to be liking this, so why don't you just implement it? Thanks, Rafael
On Saturday, July 15, 2017 06:46:26 PM Pavel Machek wrote: > Hi! > > > > > I had an idea of using an enum type encompassing all of the power states > > > > defined for various platforms and serving both as a registry (to ensure the > > > > uniqueness of the values assigned to the states) and a common ground > > > > between platforms and drivers. > > > > > > > > Something like: > > > > > > > > enum platform_target_state { > > > > PLATFORM_STATE_UNKNOWN = -1, > > > > PLATFORM_STATE_WORKING = 0, > > > > PLATFORM_STATE_ACPI_S1, > > > > PLATFORM_STATE_ACPI_S2, > > > > PLATFORM_STATE_ACPI_S3, > > > > PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS, > > > > PLATFORM_STATE_MY_BOARD_1_GATE_POWER, > > > > PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF, > > > > ... > > > > }; > > > > > > > > and define ->target_state to return a value of this type. > > > > > > > > Then, if a driver sees one of these and recognizes that value, it should > > > > know exactly what to do. > > > > > > Remind me why this is good idea? > > > > Because there are drivers that need to do specific things during > > suspend on a specific board when it goes into a specific state as a > > whole. > > We have seen driver that cares about voltage to his device being > lost. That's reasonable. > > Inquiring what the platform target state is... is not. So why exactly isn't it reasonable? Please use technical arguments. Saying that something is wrong without explaining the problem you see with it isn't particulatly useful in technical discussions. Thanks, Rafael
On 16/07/2017 01:24, Rafael J. Wysocki wrote: > On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote: > >> The enum offers the advantage of centralizing how many different states >> exist for all the platforms we know about in the kernel, it's easy to >> define common values for platforms that have the same semantics, just >> like it's simple to add new values for platform specific details. > > Well, you seem to be liking this, so why don't you just implement it? At the end of his message, Florian wrote: > In any case, just agree and I will be happy to follow-up with patches. Regards.
On Sunday, July 16, 2017 01:34:53 AM Mason wrote: > On 16/07/2017 01:24, Rafael J. Wysocki wrote: > > > On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote: > > > >> The enum offers the advantage of centralizing how many different states > >> exist for all the platforms we know about in the kernel, it's easy to > >> define common values for platforms that have the same semantics, just > >> like it's simple to add new values for platform specific details. > > > > Well, you seem to be liking this, so why don't you just implement it? > > At the end of his message, Florian wrote: > > > In any case, just agree and I will be happy to follow-up with patches. But it may be hard to convince everybody without posting code changes and often enough showing a patch makes a good argument. Thanks, Rafael
On 07/15/2017 04:38 PM, Rafael J. Wysocki wrote: > On Sunday, July 16, 2017 01:34:53 AM Mason wrote: >> On 16/07/2017 01:24, Rafael J. Wysocki wrote: >> >>> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote: >>> >>>> The enum offers the advantage of centralizing how many different states >>>> exist for all the platforms we know about in the kernel, it's easy to >>>> define common values for platforms that have the same semantics, just >>>> like it's simple to add new values for platform specific details. >>> >>> Well, you seem to be liking this, so why don't you just implement it? >> >> At the end of his message, Florian wrote: >> >>> In any case, just agree and I will be happy to follow-up with patches. > > But it may be hard to convince everybody without posting code changes > and often enough showing a patch makes a good argument. I had the patches ready last night, saw the emails this morning and decided to go mountain bike for a bit to think about it some more. You will find my follow-up patches that hopefully implement your recommendation. Thanks
On Saturday, July 15, 2017 07:36:21 PM Florian Fainelli wrote: > > On 07/15/2017 04:38 PM, Rafael J. Wysocki wrote: > > On Sunday, July 16, 2017 01:34:53 AM Mason wrote: > >> On 16/07/2017 01:24, Rafael J. Wysocki wrote: > >> > >>> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote: > >>> > >>>> The enum offers the advantage of centralizing how many different states > >>>> exist for all the platforms we know about in the kernel, it's easy to > >>>> define common values for platforms that have the same semantics, just > >>>> like it's simple to add new values for platform specific details. > >>> > >>> Well, you seem to be liking this, so why don't you just implement it? > >> > >> At the end of his message, Florian wrote: > >> > >>> In any case, just agree and I will be happy to follow-up with patches. > > > > But it may be hard to convince everybody without posting code changes > > and often enough showing a patch makes a good argument. > > I had the patches ready last night, saw the emails this morning and > decided to go mountain bike for a bit to think about it some more. You > will find my follow-up patches that hopefully implement your recommendation. OK, thanks! There is one problem with this I missed before, though, sorry about that. Drivers need to be able to distinguish between suspend-to-idle and the platform states too, so we need to store the argument passed to suspend_devices_and_enter() somewhere too, either in the core or in the platform code. And if we need to store it anyway, let's just store it in the core in a global var (say pm_suspend_target_state), export that and be done. There still will be a concern regarding drivers that care about differences between PM_SUSPEND_MEM and PM_SUSPEND_STANDBY, because those differences are platform-dependent, but let's defer addressing this until we have a driver that needs to run on different platforms with different definitions for those things. That should address the Pavel's objections too I guess. Thanks, Rafael
On Thursday, July 06, 2017 05:17:50 AM Pavel Machek wrote: > On Sun 2017-07-16 01:29:57, Rafael J. Wysocki wrote: > > On Saturday, July 15, 2017 06:46:26 PM Pavel Machek wrote: > > > Hi! > > > > > > > > > I had an idea of using an enum type encompassing all of the power states > > > > > > defined for various platforms and serving both as a registry (to ensure the > > > > > > uniqueness of the values assigned to the states) and a common ground > > > > > > between platforms and drivers. > > > > > > > > > > > > Something like: > > > > > > > > > > > > enum platform_target_state { > > > > > > PLATFORM_STATE_UNKNOWN = -1, > > > > > > PLATFORM_STATE_WORKING = 0, > > > > > > PLATFORM_STATE_ACPI_S1, > > > > > > PLATFORM_STATE_ACPI_S2, > > > > > > PLATFORM_STATE_ACPI_S3, > > > > > > PLATFORM_STATE_MY_BOARD_1_GATE_CLOCKS, > > > > > > PLATFORM_STATE_MY_BOARD_1_GATE_POWER, > > > > > > PLATFORM_STATE_ANOTHER_BOARD_DO_CRAZY_STUFF, > > > > > > ... > > > > > > }; > > > > > > > > > > > > and define ->target_state to return a value of this type. > > > > > > > > > > > > Then, if a driver sees one of these and recognizes that value, it should > > > > > > know exactly what to do. > > > > > > > > > > Remind me why this is good idea? > > > > > > > > Because there are drivers that need to do specific things during > > > > suspend on a specific board when it goes into a specific state as a > > > > whole. > > > > > > We have seen driver that cares about voltage to his device being > > > lost. That's reasonable. > > > > > > Inquiring what the platform target state is... is not. > > > > So why exactly isn't it reasonable? > > > > Please use technical arguments. Saying that something is wrong without > > explaining the problem you see with it isn't particulatly useful in technical > > discussions. > > Deep in your heart, you should know that having enum listing all the platforms linux > runs on is a very bad idea. Even so, if I'm unable to explain to people why this is a bad idea in technical terms, that doesn't mean too much. I actually noticed an issue with the approach that I missed before, see my last reply to Florian. > Anyway, there are better solutions, regulator framework already knows if given rail > will be powered off or not, and their driver already knows if they are going > suspend/standby. They just need to use existing interfaces. So they need to know what has been passed to suspend_devices_and_enter() anyway and currently there's no interface for that. That actually is the source of the whole issue. Thanks, Rafael
On 16/07/2017 at 12:22:08 +0200, Rafael J. Wysocki wrote: > On Saturday, July 15, 2017 07:36:21 PM Florian Fainelli wrote: > > > > On 07/15/2017 04:38 PM, Rafael J. Wysocki wrote: > > > On Sunday, July 16, 2017 01:34:53 AM Mason wrote: > > >> On 16/07/2017 01:24, Rafael J. Wysocki wrote: > > >> > > >>> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote: > > >>> > > >>>> The enum offers the advantage of centralizing how many different states > > >>>> exist for all the platforms we know about in the kernel, it's easy to > > >>>> define common values for platforms that have the same semantics, just > > >>>> like it's simple to add new values for platform specific details. > > >>> > > >>> Well, you seem to be liking this, so why don't you just implement it? > > >> > > >> At the end of his message, Florian wrote: > > >> > > >>> In any case, just agree and I will be happy to follow-up with patches. > > > > > > But it may be hard to convince everybody without posting code changes > > > and often enough showing a patch makes a good argument. > > > > I had the patches ready last night, saw the emails this morning and > > decided to go mountain bike for a bit to think about it some more. You > > will find my follow-up patches that hopefully implement your recommendation. > > OK, thanks! > > There is one problem with this I missed before, though, sorry about that. > > Drivers need to be able to distinguish between suspend-to-idle and the platform > states too, so we need to store the argument passed to suspend_devices_and_enter() > somewhere too, either in the core or in the platform code. > > And if we need to store it anyway, let's just store it in the core in a global var > (say pm_suspend_target_state), export that and be done. > > There still will be a concern regarding drivers that care about differences between > PM_SUSPEND_MEM and PM_SUSPEND_STANDBY, because those differences are > platform-dependent, but let's defer addressing this until we have a driver > that needs to run on different platforms with different definitions for those > things. > We already have the case for drivers/net/ethernet/cadence/ and drivers/net/can/m_can/ and many of the at91 drivers. Depending on the specific SoC they run on, PM_SUSPEND_MEM may or may not cut VDDcore or may or may not change the peripheral clock.
On 06/07/2017 at 05:18:19 +0200, Pavel Machek wrote: > On Sat 2017-07-15 20:33:58, Alexandre Belloni wrote: > > On 15/07/2017 at 10:20:27 -0700, Florian Fainelli wrote: > > > > We already have > > > > > > > > struct regulator_state { > > > > int uV; /* suspend voltage */ > > > > unsigned int mode; /* suspend regulator operating mode */ > > > > int enabled; /* is regulator enabled in this suspend state */ > > > > int disabled; /* is the regulator disabled in this suspend state */ > > > > }; > > > > > > > > * struct regulation_constraints - regulator operating constraints. > > > > * @state_disk: State for regulator when system is suspended in disk > > > > * mode. > > > > * @state_mem: State for regulator when system is suspended in mem > > > > * mode. > > > > * @state_standby: State for regulator when system is suspended in > > > > * standby > > > > * mode. > > > > > > > > . So it seems that maybe we should tell the drivers if we are entering > > > > "state_mem" or "state_standby" (something I may have opposed, sorry), > > > > then the driver can get neccessary information from regulator > > > > framework. > > > > > > OK, so what would be the mechanism to tell these drivers about the > > > system wide suspend state they are entering if it is not via > > > platform_suspend_target_state()? > > > > > > Keep in mind that regulators might be one aspect of what could be > > > causing the platform to behave specifically in one suspend state vs. > > > another, but there could be pieces of HW within the SoC that can't be > > > described with power domains, voltage islands etc. that would still have > > > inherent suspend states properties (like memory retention, pin/pad > > > controls etc. etc). We still need some mechanism, possibly centralized > > > > > > > I concur, the regulator stuff is one aspect of one of our suspend state > > (cutting VDDcore). But we have another state where the main clock (going > > to the IPs) is going from a few hundred MHz to 32kHz. This is currently > > handled by calling at91_suspend_entering_slow_clock(). I think it is > > important to take that into account so we can remove this hack from the > > kernel. > > Cure should not be worse then the disease... and it is in this case. > > For clocks, take a look at clock framework, perhaps it already has "clock_will_be_suspended" > as regulator framework had. If not, implement it. > See Rafael's comment, currently, the clock framework can't say whether the clock will change because it doesn't know anything about the suspend target. > Same with memory retention, pin/pad controls. > Same here.
On 07/16/2017 06:41 AM, Alexandre Belloni wrote: > On 06/07/2017 at 05:18:19 +0200, Pavel Machek wrote: >> On Sat 2017-07-15 20:33:58, Alexandre Belloni wrote: >>> On 15/07/2017 at 10:20:27 -0700, Florian Fainelli wrote: >>>>> We already have >>>>> >>>>> struct regulator_state { >>>>> int uV; /* suspend voltage */ >>>>> unsigned int mode; /* suspend regulator operating mode */ >>>>> int enabled; /* is regulator enabled in this suspend state */ >>>>> int disabled; /* is the regulator disabled in this suspend state */ >>>>> }; >>>>> >>>>> * struct regulation_constraints - regulator operating constraints. >>>>> * @state_disk: State for regulator when system is suspended in disk >>>>> * mode. >>>>> * @state_mem: State for regulator when system is suspended in mem >>>>> * mode. >>>>> * @state_standby: State for regulator when system is suspended in >>>>> * standby >>>>> * mode. >>>>> >>>>> . So it seems that maybe we should tell the drivers if we are entering >>>>> "state_mem" or "state_standby" (something I may have opposed, sorry), >>>>> then the driver can get neccessary information from regulator >>>>> framework. >>>> >>>> OK, so what would be the mechanism to tell these drivers about the >>>> system wide suspend state they are entering if it is not via >>>> platform_suspend_target_state()? >>>> >>>> Keep in mind that regulators might be one aspect of what could be >>>> causing the platform to behave specifically in one suspend state vs. >>>> another, but there could be pieces of HW within the SoC that can't be >>>> described with power domains, voltage islands etc. that would still have >>>> inherent suspend states properties (like memory retention, pin/pad >>>> controls etc. etc). We still need some mechanism, possibly centralized >>>> >>> >>> I concur, the regulator stuff is one aspect of one of our suspend state >>> (cutting VDDcore). But we have another state where the main clock (going >>> to the IPs) is going from a few hundred MHz to 32kHz. This is currently >>> handled by calling at91_suspend_entering_slow_clock(). I think it is >>> important to take that into account so we can remove this hack from the >>> kernel. >> >> Cure should not be worse then the disease... and it is in this case. >> >> For clocks, take a look at clock framework, perhaps it already has "clock_will_be_suspended" >> as regulator framework had. If not, implement it. >> > > See Rafael's comment, currently, the clock framework can't say whether > the clock will change because it doesn't know anything about the suspend > target. > >> Same with memory retention, pin/pad controls. >> > > Same here. Exactly, here is another side effect of not knowing the platform suspend/state that I came across on our platforms: https://patchwork.kernel.org/patch/9561575/ (we later discussed this in details with Linus and this is why this very patch set is being introduced now)
On 07/16/2017 03:22 AM, Rafael J. Wysocki wrote: > On Saturday, July 15, 2017 07:36:21 PM Florian Fainelli wrote: >> >> On 07/15/2017 04:38 PM, Rafael J. Wysocki wrote: >>> On Sunday, July 16, 2017 01:34:53 AM Mason wrote: >>>> On 16/07/2017 01:24, Rafael J. Wysocki wrote: >>>> >>>>> On Saturday, July 15, 2017 10:20:27 AM Florian Fainelli wrote: >>>>> >>>>>> The enum offers the advantage of centralizing how many different states >>>>>> exist for all the platforms we know about in the kernel, it's easy to >>>>>> define common values for platforms that have the same semantics, just >>>>>> like it's simple to add new values for platform specific details. >>>>> >>>>> Well, you seem to be liking this, so why don't you just implement it? >>>> >>>> At the end of his message, Florian wrote: >>>> >>>>> In any case, just agree and I will be happy to follow-up with patches. >>> >>> But it may be hard to convince everybody without posting code changes >>> and often enough showing a patch makes a good argument. >> >> I had the patches ready last night, saw the emails this morning and >> decided to go mountain bike for a bit to think about it some more. You >> will find my follow-up patches that hopefully implement your recommendation. > > OK, thanks! > > There is one problem with this I missed before, though, sorry about that. > > Drivers need to be able to distinguish between suspend-to-idle and the platform > states too, so we need to store the argument passed to suspend_devices_and_enter() > somewhere too, either in the core or in the platform code. > > And if we need to store it anyway, let's just store it in the core in a global var > (say pm_suspend_target_state), export that and be done. I was not sure this would be acceptable which was why I opted for making suspend_ops::begin store the state passed from suspend_ops::enter, I will change that. > > There still will be a concern regarding drivers that care about differences between > PM_SUSPEND_MEM and PM_SUSPEND_STANDBY, because those differences are > platform-dependent, but let's defer addressing this until we have a driver > that needs to run on different platforms with different definitions for those > things. Makes sense, thanks! > > That should address the Pavel's objections too I guess. > > Thanks, > Rafael >
Hi! > > > So why exactly isn't it reasonable? > > > > > > Please use technical arguments. Saying that something is wrong without > > > explaining the problem you see with it isn't particulatly useful in technical > > > discussions. > > > > Deep in your heart, you should know that having enum listing all the platforms linux > > runs on is a very bad idea. > > Even so, if I'm unable to explain to people why this is a bad idea in technical > terms, that doesn't mean too much. I could say something O(#drivers * #platforms) vs. O(#drivers + #platforms) lines of code -- but I thought it was obvious...? > > Anyway, there are better solutions, regulator framework already knows if given rail > > will be powered off or not, and their driver already knows if they are going > > suspend/standby. They just need to use existing interfaces. > > So they need to know what has been passed to suspend_devices_and_enter() > anyway and currently there's no interface for that. That actually is the source > of the whole issue. Yep, I don't like that, but I guess we should give drivers enough information to ask regulator framework. Best regards, Pavel
Hi! > > There still will be a concern regarding drivers that care about differences between > > PM_SUSPEND_MEM and PM_SUSPEND_STANDBY, because those differences are > > platform-dependent, but let's defer addressing this until we have a driver > > that needs to run on different platforms with different definitions for those > > things. > > > > We already have the case for drivers/net/ethernet/cadence/ and > drivers/net/can/m_can/ and many of the at91 drivers. Depending on the > specific SoC they run on, PM_SUSPEND_MEM may or may not cut VDDcore or > may or may not change the peripheral clock. Please please introduce will_vddcore_be_cut_down() or similar helper, so that we have one place to fix.. Thanks, Pavel
diff --git a/include/linux/suspend.h b/include/linux/suspend.h index d9718378a8be..d998a04a90a2 100644 --- a/include/linux/suspend.h +++ b/include/linux/suspend.h @@ -172,6 +172,15 @@ static inline void dpm_save_failed_step(enum suspend_stat_step step) * Called by the PM core if the suspending of devices fails. * This callback is optional and should only be implemented by platforms * which require special recovery actions in that situation. + * + * @target_state: Returns the suspend state the suspend_ops will be entering. + * Called by device drivers that need to know the platform specific suspend + * state the system is about to enter. + * This callback is optional and should only be implemented by platforms + * which require special handling of power management states within + * drivers. It does require @begin to be implemented to provide the suspend + * state. Return value is platform_suspend_ops specific, and may be a 1:1 + * mapping to suspend_state_t when relevant. */ struct platform_suspend_ops { int (*valid)(suspend_state_t state); @@ -184,6 +193,7 @@ struct platform_suspend_ops { bool (*suspend_again)(void); void (*end)(void); void (*recover)(void); + int (*target_state)(void); }; struct platform_freeze_ops { @@ -200,6 +210,7 @@ struct platform_freeze_ops { */ extern void suspend_set_ops(const struct platform_suspend_ops *ops); extern int suspend_valid_only_mem(suspend_state_t state); +extern int platform_suspend_target_state(void); extern unsigned int pm_suspend_global_flags; @@ -279,6 +290,7 @@ static inline bool pm_resume_via_firmware(void) { return false; } static inline void suspend_set_ops(const struct platform_suspend_ops *ops) {} static inline int pm_suspend(suspend_state_t state) { return -ENOSYS; } +static inline int platform_suspend_target_state(void) { return -ENOSYS; } static inline bool idle_should_freeze(void) { return false; } static inline void __init pm_states_init(void) {} static inline void freeze_set_ops(const struct platform_freeze_ops *ops) {} diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c index 15e6baef5c73..d31fe7a08f4a 100644 --- a/kernel/power/suspend.c +++ b/kernel/power/suspend.c @@ -177,6 +177,21 @@ void suspend_set_ops(const struct platform_suspend_ops *ops) EXPORT_SYMBOL_GPL(suspend_set_ops); /** + * platform_suspend_target_state - Return the platform specific suspend state. + * a begin() callback is necessary in order to fill this information correctly + * for callers. + */ +int platform_suspend_target_state(void) +{ + if (!suspend_ops || !suspend_ops->target_state || + (suspend_ops->target_state && !suspend_ops->begin)) + return -ENOTSUPP; + + return suspend_ops->target_state(); +} +EXPORT_SYMBOL_GPL(platform_suspend_target_state); + +/** * suspend_valid_only_mem - Generic memory-only valid callback. * * Platform drivers that implement mem suspend only and only need to check for
Add an optional platform_suspend_ops callback: target_state, and a helper function globally visible to get this called: platform_suspend_target_state(). This is useful for platform specific drivers that may need to take a slightly different suspend/resume path based on the system's suspend/resume state being entered. Although this callback is optional and documented as such, it requires a platform_suspend_ops::begin callback to be implemented in order to provide an accurate suspend/resume state within the driver that implements this platform_suspend_ops. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- include/linux/suspend.h | 12 ++++++++++++ kernel/power/suspend.c | 15 +++++++++++++++ 2 files changed, 27 insertions(+)