[RFC 0/2] PM / suspend: Add platform_suspend_target_state()

[RFC 0/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

This patch series implements the idea discussed in this thread:

https://www.spinics.net/lists/arm-kernel/msg590068.html

The last patch is relative to the pending submission of the Broadcom STB
S2/S3/S5 suspend/resume code that can be found below, and is provided
as an example of how this can be useful.

https://lkml.org/lkml/2017/6/16/737

Florian Fainelli (2):
  PM / suspend: Add platform_suspend_target_state()
  soc: bcm: brcmstb: PM: Implement target_state callback

 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 15 +++++++++++++++
 include/linux/suspend.h             | 12 ++++++++++++
 kernel/power/suspend.c              | 15 +++++++++++++++
 3 files changed, 42 insertions(+)

-- 
2.9.3

[RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

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);
 };
 
 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
-- 
2.9.3

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

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

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

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!
-- 
Florian

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

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

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

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

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

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

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

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

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

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

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

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.
-- 
Florian

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Alexandre Belloni]

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.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

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

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Alexandre Belloni]

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.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

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)
-- 
Florian

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

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

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Mason]

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.

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

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

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

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
-- 
Florian

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

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

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Alexandre Belloni]

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.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

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

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

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
> 

-- 
Florian

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

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

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

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

Re: [RFC 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

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

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[RFC 2/2] soc: bcm: brcmstb: PM: Implement target_state callback

[Florian Fainelli]

Provide a target_state callback implementation which just returns the
suspend_state_t the system is about to enter. Broadcom STB drivers can
utilize platform_suspend_target_state() to retrieve that and take
appropriate actions (e.g: full vs. partial re-initialization).

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
index 4b7e6c297b23..7d4695734093 100644
--- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
+++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
@@ -104,6 +104,7 @@ struct brcmstb_pm_control {
 	u32 phy_b_standby_ctrl_offs;
 	bool needs_ddr_pad;
 	struct platform_device *pdev;
+	suspend_state_t pm_state;
 };
 
 enum bsp_initiate_command {
@@ -547,9 +548,23 @@ static int brcmstb_pm_valid(suspend_state_t state)
 	}
 }
 
+static int brcmstb_pm_begin(suspend_state_t state)
+{
+	ctrl.pm_state = state;
+
+	return 0;
+}
+
+static int brcmstb_target_state(void)
+{
+	return ctrl.pm_state;
+}
+
 static const struct platform_suspend_ops brcmstb_pm_ops = {
+	.begin		= brcmstb_pm_begin,
 	.enter		= brcmstb_pm_enter,
 	.valid		= brcmstb_pm_valid,
+	.target_state	= brcmstb_target_state,
 };
 
 static const struct of_device_id aon_ctrl_dt_ids[] = {
-- 
2.9.3

Re: [RFC 2/2] soc: bcm: brcmstb: PM: Implement target_state callback

[Rafael J. Wysocki]

On Thursday, June 22, 2017 06:08:37 PM Florian Fainelli wrote:
> Provide a target_state callback implementation which just returns the
> suspend_state_t the system is about to enter. Broadcom STB drivers can
> utilize platform_suspend_target_state() to retrieve that and take
> appropriate actions (e.g: full vs. partial re-initialization).
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/soc/bcm/brcmstb/pm/pm-arm.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
> index 4b7e6c297b23..7d4695734093 100644
> --- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
> +++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
> @@ -104,6 +104,7 @@ struct brcmstb_pm_control {
>  	u32 phy_b_standby_ctrl_offs;
>  	bool needs_ddr_pad;
>  	struct platform_device *pdev;
> +	suspend_state_t pm_state;

I wouldn't use suspend_state_t here, because the mapping between those
things and real platform power states is somewhat arbitrary and totally
platform-specific.

It's better to define symbols representing platform power states for your
platform (or use an enum) and then use those symbols in the drivers IMO.

Thanks,
Rafael

[PATCH 0/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

This patch series implements the idea discussed in this thread:

https://www.spinics.net/lists/arm-kernel/msg590068.html

The last patch is relative to the pending submission of the Broadcom STB
S2/S3/S5 suspend/resume code that can be found below, and is provided
as an example of how this can be useful.

https://lkml.org/lkml/2017/6/16/737

Changes from RFC:

- make platform_target_state an enum that platforms can modify to
  include their own states
- updated brcmstb PM code to translate internal states to externally
  visible platform_target_state

Florian Fainelli (2):
  PM / suspend: Add platform_suspend_target_state()
  soc: bcm: brcmstb: PM: Implement target_state callback

 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 22 ++++++++++++++++++++++
 include/linux/suspend.h             | 26 ++++++++++++++++++++++++++
 kernel/power/suspend.c              | 15 +++++++++++++++
 3 files changed, 63 insertions(+)

-- 
2.9.3

[PATCH 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

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.

An enumeration: platform_target_state is defined which currently defines
the standard ACPI_S[1-4] states and can be extended with platform
specific suspend states.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/linux/suspend.h | 25 +++++++++++++++++++++++++
 kernel/power/suspend.c  | 15 +++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0b1cf32edfd7..6e6cc0778816 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -50,6 +50,16 @@ enum suspend_stat_step {
 	SUSPEND_RESUME
 };
 
+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_ACPI_S4,
+	/* Add platform specific states here */
+};
+
 struct suspend_stats {
 	int	success;
 	int	fail;
@@ -172,6 +182,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 +203,7 @@ struct platform_suspend_ops {
 	bool (*suspend_again)(void);
 	void (*end)(void);
 	void (*recover)(void);
+	enum platform_target_state (*target_state)(void);
 };
 
 struct platform_freeze_ops {
@@ -202,6 +222,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 enum platform_target_state platform_suspend_target_state(void);
 
 extern unsigned int pm_suspend_global_flags;
 
@@ -281,6 +302,10 @@ 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 PLATFORM_STATE_UNKNOWN;
+}
 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 3ecf275d7e44..cd1b62f23b0e 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -202,6 +202,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.
+ */
+enum platform_target_state 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
-- 
2.9.3

Re: [PATCH 1/2] PM / suspend: Add platform_suspend_target_state()

[Pavel Machek]

Hi!

> 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.
> 
> An enumeration: platform_target_state is defined which currently defines
> the standard ACPI_S[1-4] states and can be extended with platform
> specific suspend states.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  include/linux/suspend.h | 25 +++++++++++++++++++++++++
>  kernel/power/suspend.c  | 15 +++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 0b1cf32edfd7..6e6cc0778816 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -50,6 +50,16 @@ enum suspend_stat_step {
>  	SUSPEND_RESUME
>  };
>  
> +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_ACPI_S4,
> +	/* Add platform specific states here */
> +};
> +

As I tried to explain in the email thread, having list with all the possible platform
states is no-go. We have about 1000 platforms supported...

NAK.
									Pavel

Re: [PATCH 1/2] PM / suspend: Add platform_suspend_target_state()

[Florian Fainelli]

On 07/05/2017 08:18 PM, Pavel Machek wrote:
> Hi!
> 
>> 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.
>>
>> An enumeration: platform_target_state is defined which currently defines
>> the standard ACPI_S[1-4] states and can be extended with platform
>> specific suspend states.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  include/linux/suspend.h | 25 +++++++++++++++++++++++++
>>  kernel/power/suspend.c  | 15 +++++++++++++++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>> index 0b1cf32edfd7..6e6cc0778816 100644
>> --- a/include/linux/suspend.h
>> +++ b/include/linux/suspend.h
>> @@ -50,6 +50,16 @@ enum suspend_stat_step {
>>  	SUSPEND_RESUME
>>  };
>>  
>> +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_ACPI_S4,
>> +	/* Add platform specific states here */
>> +};
>> +
> 
> As I tried to explain in the email thread, having list with all the possible platform
> states is no-go. We have about 1000 platforms supported...

FYI, the recent (relatively recent) CPU hotplug conversion from
notifiers to a state machine has a similar pattern whereby pieces of
code needing to hook into the CPU hotplug state machine add their own
enum values as they need. So far it's been working for them, and there
were tons of CPU hotplug notifiers in the kernel.

Anyhow, let me implement Rafael's suggestions and we can see how we move
from there.
-- 
Florian

Re: [PATCH 1/2] PM / suspend: Add platform_suspend_target_state()

[Rafael J. Wysocki]

On Saturday, July 15, 2017 07:36:09 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.
> 
> An enumeration: platform_target_state is defined which currently defines
> the standard ACPI_S[1-4] states and can be extended with platform
> specific suspend states.

This has a couple of problems, but I'm not sure if it is worth to go too much
into details here.

Let's just take a different approach as I said in the other thread.

Thanks,
Rafael

[PATCH 2/2] soc: bcm: brcmstb: PM: Implement target_state callback

[Florian Fainelli]

Provide a target_state callback implementation which just returns the
suspend_state_t the system is about to enter. Broadcom STB drivers can
utilize platform_suspend_target_state() to retrieve that and take
appropriate actions (e.g: full vs. partial re-initialization). Two
states are implemented: PLATFORM_STATE_WORKING and
PLATFORM_STATE_BRCMSTB_S3MEM to that end.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/soc/bcm/brcmstb/pm/pm-arm.c | 22 ++++++++++++++++++++++
 include/linux/suspend.h             |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/soc/bcm/brcmstb/pm/pm-arm.c b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
index dcf8c8065508..750a74b3fc9d 100644
--- a/drivers/soc/bcm/brcmstb/pm/pm-arm.c
+++ b/drivers/soc/bcm/brcmstb/pm/pm-arm.c
@@ -104,6 +104,7 @@ struct brcmstb_pm_control {
 	u32 phy_b_standby_ctrl_offs;
 	bool needs_ddr_pad;
 	struct platform_device *pdev;
+	suspend_state_t pm_state;
 };
 
 enum bsp_initiate_command {
@@ -533,9 +534,30 @@ static int brcmstb_pm_valid(suspend_state_t state)
 	}
 }
 
+static int brcmstb_pm_begin(suspend_state_t state)
+{
+	ctrl.pm_state = state;
+
+	return 0;
+}
+
+static enum platform_target_state brcmstb_target_state(void)
+{
+	switch (ctrl.pm_state) {
+	case PM_SUSPEND_STANDBY:
+		return PLATFORM_STATE_WORKING;
+	case PM_SUSPEND_MEM:
+		return PLATFORM_STATE_BRCMSTB_S3MEM;
+	default:
+		return PLATFORM_STATE_UNKNOWN;
+	}
+}
+
 static const struct platform_suspend_ops brcmstb_pm_ops = {
+	.begin		= brcmstb_pm_begin,
 	.enter		= brcmstb_pm_enter,
 	.valid		= brcmstb_pm_valid,
+	.target_state	= brcmstb_target_state,
 };
 
 static const struct of_device_id aon_ctrl_dt_ids[] = {
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 6e6cc0778816..15d21d76e0bf 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -58,6 +58,7 @@ enum platform_target_state {
 	PLATFORM_STATE_ACPI_S3,
 	PLATFORM_STATE_ACPI_S4,
 	/* Add platform specific states here */
+	PLATFORM_STATE_BRCMSTB_S3MEM,
 };
 
 struct suspend_stats {
-- 
2.9.3

[PATCH v2] PM / suspend: Add suspend_target_state()

[Florian Fainelli]

Have the core suspend/resume framework store the system-wide suspend
state (suspend_state_t) we are about to enter, and expose it to drivers
via suspend_target_state() in order to retrieve that. The state is
assigned in suspend_devices_and_enter().

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.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:

- rename platform_suspend_target_state() -> suspend_target_state()
- directly export the suspend_state_t value and assign it in
  suspend_devices_and_enter()

 include/linux/suspend.h |  2 ++
 kernel/power/suspend.c  | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0b1cf32edfd7..7b70e7d6a006 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -202,6 +202,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 suspend_state_t suspend_target_state(void);
 
 extern unsigned int pm_suspend_global_flags;
 
@@ -281,6 +282,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 suspend_state_t 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 3ecf275d7e44..a296d6e25d52 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -47,6 +47,7 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
 
 suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
 static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
+static suspend_state_t pm_suspend_target_state;
 
 unsigned int pm_suspend_global_flags;
 EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
@@ -202,6 +203,18 @@ void suspend_set_ops(const struct platform_suspend_ops *ops)
 EXPORT_SYMBOL_GPL(suspend_set_ops);
 
 /**
+ * suspend_target_state - Return the system wide suspend state.
+ *
+ * The pm_suspend_target_state becomes valid during
+ * suspend_devices_and_enter().
+ */
+suspend_state_t suspend_target_state(void)
+{
+	return pm_suspend_target_state;
+}
+EXPORT_SYMBOL_GPL(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
@@ -456,6 +469,8 @@ int suspend_devices_and_enter(suspend_state_t state)
 	if (!sleep_state_supported(state))
 		return -ENOSYS;
 
+	pm_suspend_target_state = state;
+
 	error = platform_suspend_begin(state);
 	if (error)
 		goto Close;
-- 
2.9.3

Re: [PATCH v2] PM / suspend: Add suspend_target_state()

[Pavel Machek]

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

Hi!

> Have the core suspend/resume framework store the system-wide suspend
> state (suspend_state_t) we are about to enter, and expose it to drivers
> via suspend_target_state() in order to retrieve that. The state is
> assigned in suspend_devices_and_enter().

Do we really want to have variable + inline functions that just read
that variable?

> +static inline suspend_state_t suspend_target_state(void) { return  -ENOSYS; }

I'm pretty sure -ENOSYS is not compatible with suspend_state_t ...

> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 3ecf275d7e44..a296d6e25d52 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -47,6 +47,7 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
>  
>  suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
>  static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
> +static suspend_state_t pm_suspend_target_state;

Is there disadvantage of just having this variable non-static?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

Re: [PATCH v2] PM / suspend: Add suspend_target_state()

[Rafael J. Wysocki]

On Mon, Jul 17, 2017 at 10:16 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> Have the core suspend/resume framework store the system-wide suspend
>> state (suspend_state_t) we are about to enter, and expose it to drivers
>> via suspend_target_state() in order to retrieve that. The state is
>> assigned in suspend_devices_and_enter().
>
> Do we really want to have variable + inline functions that just read
> that variable?

Florian, Pavel is right, you can simply export the variable.

Anything accessing it should go under CONFIG_PM_SLEEP anyway.

Thanks,
Rafael

Re: [PATCH v2] PM / suspend: Add suspend_target_state()

[Florian Fainelli]

On 07/17/2017 02:03 PM, Rafael J. Wysocki wrote:
> On Mon, Jul 17, 2017 at 10:16 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> Hi!
>>
>>> Have the core suspend/resume framework store the system-wide suspend
>>> state (suspend_state_t) we are about to enter, and expose it to drivers
>>> via suspend_target_state() in order to retrieve that. The state is
>>> assigned in suspend_devices_and_enter().
>>
>> Do we really want to have variable + inline functions that just read
>> that variable?
> 
> Florian, Pavel is right, you can simply export the variable.
> 
> Anything accessing it should go under CONFIG_PM_SLEEP anyway.

Alright then, I will just export it. Stay tuned.
-- 
Florian

Re: [PATCH v2] PM / suspend: Add suspend_target_state()

[Pavel Machek]

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

On Mon 2017-07-17 13:06:28, Florian Fainelli wrote:
> Have the core suspend/resume framework store the system-wide suspend
> state (suspend_state_t) we are about to enter, and expose it to drivers
> via suspend_target_state() in order to retrieve that. The state is
> assigned in suspend_devices_and_enter().
> 
> 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.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[PATCH v3] PM / suspend: Export pm_suspend_target_state

[Florian Fainelli]

Have the core suspend/resume framework store the system-wide suspend
state (suspend_state_t) we are about to enter, and expose it to drivers
via pm_suspend_target_state in order to retrieve that. The state is
assigned in suspend_devices_and_enter().

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.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v3:

- just export pm_suspend_target_state without a helper function

Changes in v2:

- rename platform_suspend_target_state() -> suspend_target_state()
- directly export the suspend_state_t value and assign it in
  suspend_devices_and_enter()

 include/linux/suspend.h | 1 +
 kernel/power/suspend.c  | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0b1cf32edfd7..2159f6841768 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -427,6 +427,7 @@ extern int unregister_pm_notifier(struct notifier_block *nb);
 /* drivers/base/power/wakeup.c */
 extern bool events_check_enabled;
 extern unsigned int pm_wakeup_irq;
+extern suspend_state_t pm_suspend_target_state;
 
 extern bool pm_wakeup_pending(void);
 extern void pm_system_wakeup(void);
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 3ecf275d7e44..1aecdaf22ab5 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -47,6 +47,8 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
 
 suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
 static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
+suspend_state_t pm_suspend_target_state;
+EXPORT_SYMBOL_GPL(pm_suspend_target_state);
 
 unsigned int pm_suspend_global_flags;
 EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
@@ -456,6 +458,8 @@ int suspend_devices_and_enter(suspend_state_t state)
 	if (!sleep_state_supported(state))
 		return -ENOSYS;
 
+	pm_suspend_target_state = state;
+
 	error = platform_suspend_begin(state);
 	if (error)
 		goto Close;
-- 
2.9.3

Re: [PATCH v3] PM / suspend: Export pm_suspend_target_state

[Rafael J. Wysocki]

On Monday, July 17, 2017 03:10:59 PM Florian Fainelli wrote:
> Have the core suspend/resume framework store the system-wide suspend
> state (suspend_state_t) we are about to enter, and expose it to drivers
> via pm_suspend_target_state in order to retrieve that. The state is
> assigned in suspend_devices_and_enter().
> 
> 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.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v3:
> 
> - just export pm_suspend_target_state without a helper function
> 
> Changes in v2:
> 
> - rename platform_suspend_target_state() -> suspend_target_state()
> - directly export the suspend_state_t value and assign it in
>   suspend_devices_and_enter()
> 
>  include/linux/suspend.h | 1 +
>  kernel/power/suspend.c  | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 0b1cf32edfd7..2159f6841768 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -427,6 +427,7 @@ extern int unregister_pm_notifier(struct notifier_block *nb);
>  /* drivers/base/power/wakeup.c */
>  extern bool events_check_enabled;
>  extern unsigned int pm_wakeup_irq;
> +extern suspend_state_t pm_suspend_target_state;
>  
>  extern bool pm_wakeup_pending(void);
>  extern void pm_system_wakeup(void);
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 3ecf275d7e44..1aecdaf22ab5 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -47,6 +47,8 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
>  
>  suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
>  static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
> +suspend_state_t pm_suspend_target_state;
> +EXPORT_SYMBOL_GPL(pm_suspend_target_state);
>  
>  unsigned int pm_suspend_global_flags;
>  EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
> @@ -456,6 +458,8 @@ int suspend_devices_and_enter(suspend_state_t state)
>  	if (!sleep_state_supported(state))
>  		return -ENOSYS;
>  
> +	pm_suspend_target_state = state;
> +
>  	error = platform_suspend_begin(state);
>  	if (error)
>  		goto Close;
> 

And please clear pm_suspend_target_state before returning from
suspend_devices_and_enter().

Thanks,
Rafael

[PATCH v4] PM / suspend: Export pm_suspend_target_state

[Florian Fainelli]

Have the core suspend/resume framework store the system-wide suspend
state (suspend_state_t) we are about to enter, and expose it to drivers
via pm_suspend_target_state in order to retrieve that. The state is
assigned in suspend_devices_and_enter().

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.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v4:
- clear pm_suspend_target_state in Close label

Changes in v3:

- just export pm_suspend_target_state without a helper function

Changes in v2:

- rename platform_suspend_target_state() -> suspend_target_state()
- directly export the suspend_state_t value and assign it in
  suspend_devices_and_enter()


 include/linux/suspend.h | 1 +
 kernel/power/suspend.c  | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 0b1cf32edfd7..2159f6841768 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -427,6 +427,7 @@ extern int unregister_pm_notifier(struct notifier_block *nb);
 /* drivers/base/power/wakeup.c */
 extern bool events_check_enabled;
 extern unsigned int pm_wakeup_irq;
+extern suspend_state_t pm_suspend_target_state;
 
 extern bool pm_wakeup_pending(void);
 extern void pm_system_wakeup(void);
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 3ecf275d7e44..d0c0b96c2383 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -47,6 +47,8 @@ const char *mem_sleep_states[PM_SUSPEND_MAX];
 
 suspend_state_t mem_sleep_current = PM_SUSPEND_FREEZE;
 static suspend_state_t mem_sleep_default = PM_SUSPEND_MEM;
+suspend_state_t pm_suspend_target_state;
+EXPORT_SYMBOL_GPL(pm_suspend_target_state);
 
 unsigned int pm_suspend_global_flags;
 EXPORT_SYMBOL_GPL(pm_suspend_global_flags);
@@ -456,6 +458,8 @@ int suspend_devices_and_enter(suspend_state_t state)
 	if (!sleep_state_supported(state))
 		return -ENOSYS;
 
+	pm_suspend_target_state = state;
+
 	error = platform_suspend_begin(state);
 	if (error)
 		goto Close;
@@ -485,6 +489,7 @@ int suspend_devices_and_enter(suspend_state_t state)
 
  Close:
 	platform_resume_end(state);
+	pm_suspend_target_state = PM_SUSPEND_ON;
 	return error;
 
  Recover_platform:
-- 
2.9.3

Re: [PATCH v4] PM / suspend: Export pm_suspend_target_state

[Rafael J. Wysocki]

On Monday, July 17, 2017 05:19:25 PM Florian Fainelli wrote:
> Have the core suspend/resume framework store the system-wide suspend
> state (suspend_state_t) we are about to enter, and expose it to drivers
> via pm_suspend_target_state in order to retrieve that. The state is
> assigned in suspend_devices_and_enter().
> 
> 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.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v4:
> - clear pm_suspend_target_state in Close label
> 
> Changes in v3:
> 
> - just export pm_suspend_target_state without a helper function
> 
> Changes in v2:
> 
> - rename platform_suspend_target_state() -> suspend_target_state()
> - directly export the suspend_state_t value and assign it in
>   suspend_devices_and_enter()

Applied, thanks!