[PATCH v2 0/3] add support for power off check in suspend

[PATCH v2 0/3] add support for power off check in suspend

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Hi,

AT91 platforms support a power saving mode where SoC's power is cut off (we call
it backup mode). The resume is done with the help of bootloaders. To be able to
suspend/resume Linux to/from this mode all the drivers suspend/resume callbacks
should save/restore the content of all the active registers. We have 2 problems
we are trying to solve:
- some of these drivers are shared with other non Microchip SoCs (e.g. macb
  driver) and we don't want to disturbe other users of corresponding IPs with
  all the register save/restore operations;
- the suspend/resume time for the rest of the power saving mode we are using
  could be improved if we would know in drivers the suspend mode the platform
  is switched to.

A solution that would have been solve our problems was proposed in [1] but in
the end it wasn't accepted. It ended up with the introduction of
pm_suspend_target_state variable that could be used along with the changes in
this series.

While the discussion of [1] progressed it has been proposed (in [2]) to
implement a function that would tell if the platform's power would be cut off
at the end of the suspend procedure.

The patches in this series does as follows:
1/3 - add a new member to platform_suspend_ops that will tell if platform's
      power will be cut off at the end of the suspend procedure; drivers could
      use it via platform_off_in_suspend()
2/3 - add a new function to regulator's core that could be used to check if a
      proper regulator has been configured (via DT) to be powered off in
      suspend. This is used on this series to check the CPU's regulator is
      properly configured in DT to be turned off in suspend.
3/3 - fill .off_in_suspend member of at91_pm_ops; the functionality in patch 2/3
      is used to double check CPU's regulator would be turned off in suspend.

Thank you,
Claudiu Beznea

[1] https://lkml.org/lkml/2017/6/22/938
[2] https://lkml.org/lkml/2017/7/16/457

Changes in v2:
- remove !! instruction in regulator_is_disabled_in_suspend() and
  at91_pm_state_allowed()
- s/weather/wether in patch 1/1

Claudiu Beznea (3):
  PM / Suspend: Add support to check if platform's power is off in
    suspend
  regulator: core: add helper to check if regulator is disabled in
    suspend
  ARM: at91: pm: add support for .off_in_suspend

 arch/arm/mach-at91/pm.c            | 61 +++++++++++++++++++++++++++++++++++---
 drivers/regulator/core.c           | 17 +++++++++++
 include/linux/regulator/consumer.h |  7 +++++
 include/linux/suspend.h            |  6 ++++
 kernel/power/suspend.c             | 13 ++++++++
 5 files changed, 100 insertions(+), 4 deletions(-)

-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/3] PM / Suspend: Add support to check if platform's power is off in suspend

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Add support to check if platform's power will be cut off in suspend.
This will help drivers shared by multiple platforms to take only the
necessary actions while suspending/resuming (some platform may not need
to save/restore all the registers if platforms remains powered while
suspended). In this way suspend/resume time could be improved.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 include/linux/suspend.h |  6 ++++++
 kernel/power/suspend.c  | 13 +++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 3f529ad9a9d2..21f19b167fe2 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -173,6 +173,9 @@ 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.
+ *
+ * @off_in_suspend: Returns wether the platform's power will be cut off at
+ *	the end of suspend procedure or not.
  */
 struct platform_suspend_ops {
 	int (*valid)(suspend_state_t state);
@@ -185,6 +188,7 @@ struct platform_suspend_ops {
 	bool (*suspend_again)(void);
 	void (*end)(void);
 	void (*recover)(void);
+	bool (*off_in_suspend)(suspend_state_t state);
 };
 
 struct platform_s2idle_ops {
@@ -275,6 +279,7 @@ extern void arch_suspend_disable_irqs(void);
 extern void arch_suspend_enable_irqs(void);
 
 extern int pm_suspend(suspend_state_t state);
+extern bool platform_off_in_suspend(suspend_state_t state);
 #else /* !CONFIG_SUSPEND */
 #define suspend_valid_only_mem	NULL
 
@@ -287,6 +292,7 @@ static inline bool pm_suspend_via_s2idle(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 bool platform_off_in_suspend(suspend_state_t state) { return false; }
 static inline bool idle_should_enter_s2idle(void) { return false; }
 static inline void __init pm_states_init(void) {}
 static inline void s2idle_set_ops(const struct platform_s2idle_ops *ops) {}
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 0bd595a0b610..e1f60c8a17b9 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -319,6 +319,19 @@ static bool platform_suspend_again(suspend_state_t state)
 		suspend_ops->suspend_again() : false;
 }
 
+/**
+ * platform_off_in_suspend() - specifies if SoC's power will pe cut off at the
+ * end of suspend procedure.
+ */
+bool platform_off_in_suspend(suspend_state_t state)
+{
+	if (!suspend_ops || !suspend_ops->off_in_suspend)
+		return false;
+
+	return suspend_ops->off_in_suspend(state);
+}
+EXPORT_SYMBOL_GPL(platform_off_in_suspend);
+
 #ifdef CONFIG_PM_DEBUG
 static unsigned int pm_test_delay = 5;
 module_param(pm_test_delay, uint, 0644);
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] PM / Suspend: Add support to check if platform's power is off in suspend

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 1632 bytes --]

Hi!

> Add support to check if platform's power will be cut off in suspend.
> This will help drivers shared by multiple platforms to take only the
> necessary actions while suspending/resuming (some platform may not need
> to save/restore all the registers if platforms remains powered while
> suspended). In this way suspend/resume time could be improved.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  include/linux/suspend.h |  6 ++++++
>  kernel/power/suspend.c  | 13 +++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index 3f529ad9a9d2..21f19b167fe2 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -173,6 +173,9 @@ 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.
> + *
> + * @off_in_suspend: Returns wether the platform's power will be cut off at

wether -- spelling?

> @@ -185,6 +188,7 @@ struct platform_suspend_ops {
>  	bool (*suspend_again)(void);
>  	void (*end)(void);
>  	void (*recover)(void);
> +	bool (*off_in_suspend)(suspend_state_t state);
>  };

Dunno, should it be per-regulator? SoCs commonly have more than one
power supply, with some of them off during suspend...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] PM / Suspend: Add support to check if platform's power is off in suspend

[Claudiu.Beznea]

On 09.01.2019 16:14, Pavel Machek wrote:
> Hi!
> 
>> Add support to check if platform's power will be cut off in suspend.
>> This will help drivers shared by multiple platforms to take only the
>> necessary actions while suspending/resuming (some platform may not need
>> to save/restore all the registers if platforms remains powered while
>> suspended). In this way suspend/resume time could be improved.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  include/linux/suspend.h |  6 ++++++
>>  kernel/power/suspend.c  | 13 +++++++++++++
>>  2 files changed, 19 insertions(+)
>>
>> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
>> index 3f529ad9a9d2..21f19b167fe2 100644
>> --- a/include/linux/suspend.h
>> +++ b/include/linux/suspend.h
>> @@ -173,6 +173,9 @@ 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.
>> + *
>> + * @off_in_suspend: Returns wether the platform's power will be cut off at
> 
> wether -- spelling?

Yep, I did it again :), sorry

> 
>> @@ -185,6 +188,7 @@ struct platform_suspend_ops {
>>  	bool (*suspend_again)(void);
>>  	void (*end)(void);
>>  	void (*recover)(void);
>> +	bool (*off_in_suspend)(suspend_state_t state);
>>  };
> 
> Dunno, should it be per-regulator? SoCs commonly have more than one
> power supply, with some of them off during suspend...

Every regulator has a suspend state which could be described via device
tree. In our case, besides other regulators, we have core regulator, which
is turned off in suspend at the end of the suspend procedure, when PMIC
detects a level transition on one of its pins, transition which is
triggered the moment the CPU is shut down (we are doing this from software
at the end of suspend procedure).

One aspect is that we map our proprietary power saving modes to Linux power
saving modes (suspend-to-ram and standby) at the system initialization. So,
we could have our backup mode mapped to any of suspend-to-ram or standby so
that when:
echo mem > /sys/power/state or
echo standby > /sys/power/state
the final power saving mode could be or not this backup mode (depending on
the mapping - which is done at system initialization based on kernel
parameters).

And so, depending on the mapped mode (which we currently know only from
arch/arm/mach-at91/) the real core's regulator could be on or off. We need
both these information to know if core's regulator would be off in suspend.

I chose to have it here, in platform_suspend_ops thinking that it is more
related to suspend procedure and that every platform could take use of it
to do whatever the platform specific things are done in suspend.
Also, I had in mind that regulator framework doesn't care (as far as I
know) what every regulator feeds.
It looked to me more proper to have it per platform since in our case we
have the mapping b/w Linux power saving modes (suspend-to-ram, standby) and
SoC specific power saving modes done only in arch/arm/mach-at91/.

I also though that doing so, and having the platform_off_in_suspend()
wrapper, in drivers we could only include linux/suspend.h and make use of
this wrapper and in case we would have had it per regulator we should have
been taken care in drivers or regulator framework which regulator deals
with feeding the core.

Thank you,
Claudiu Beznea

> 									Pavel
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/3] regulator: core: add helper to check if regulator is disabled in suspend

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Add helper to check if regulator will be disabled in suspend.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/regulator/core.c           | 17 +++++++++++++++++
 include/linux/regulator/consumer.h |  7 +++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b9d7b45c7295..3b761969732a 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3786,6 +3786,23 @@ int regulator_suspend_disable(struct regulator_dev *rdev,
 }
 EXPORT_SYMBOL_GPL(regulator_suspend_disable);
 
+bool regulator_is_disabled_in_suspend(struct regulator *regulator,
+				      suspend_state_t state)
+{
+	struct regulator_state *rstate;
+
+	if (!regulator->rdev->constraints)
+		return false;
+
+	rstate = regulator_get_suspend_state(regulator->rdev, state);
+	if (!rstate)
+		return false;
+
+	return regulator->rdev->desc->ops->set_suspend_disable &&
+	       rstate->enabled == DISABLE_IN_SUSPEND;
+}
+EXPORT_SYMBOL_GPL(regulator_is_disabled_in_suspend);
+
 static int _regulator_set_suspend_voltage(struct regulator *regulator,
 					  int min_uV, int max_uV,
 					  suspend_state_t state)
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index f3f76051e8b0..cf5e71dc63ce 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -36,6 +36,7 @@
 #define __LINUX_REGULATOR_CONSUMER_H_
 
 #include <linux/err.h>
+#include <linux/suspend.h>
 
 struct device;
 struct notifier_block;
@@ -285,6 +286,9 @@ void devm_regulator_unregister_notifier(struct regulator *regulator,
 void *regulator_get_drvdata(struct regulator *regulator);
 void regulator_set_drvdata(struct regulator *regulator, void *data);
 
+/* regulator suspend/resume state */
+bool regulator_is_disabled_in_suspend(struct regulator *regulator,
+				      suspend_state_t state);
 #else
 
 /*
@@ -579,6 +583,9 @@ static inline int regulator_list_voltage(struct regulator *regulator, unsigned s
 	return -EINVAL;
 }
 
+bool regulator_is_disabled_in_suspend(struct regulator *regulator,
+				      suspend_state_t state);
+
 #endif
 
 static inline int regulator_set_voltage_triplet(struct regulator *regulator,
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] regulator: core: add helper to check if regulator is disabled in suspend

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 824 bytes --]

On Tue, Jan 08, 2019 at 10:56:32AM +0000, Claudiu.Beznea@microchip.com wrote:
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> Add helper to check if regulator will be disabled in suspend.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>

This feels like it's the wrong way round - if this is configurable I'd
expect something to configure the suspend mode and then for that to
arrange to configure the regulator appropriately (along with anything
else that needs doing) rather than to infer the configuration from the
regulator state which feels fragile.  But based on the cover letter
that's kind of like what the initial proposal about target states was so
perhaps this is the way we end up going...  this certainly looks a lot
less impactful that the target state stuff though.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] regulator: core: add helper to check if regulator is disabled in suspend

[Claudiu.Beznea]

On 09.01.2019 18:57, Mark Brown wrote:
> On Tue, Jan 08, 2019 at 10:56:32AM +0000, Claudiu.Beznea@microchip.com wrote:
>> From: Claudiu Beznea <claudiu.beznea@microchip.com>
>>
>> Add helper to check if regulator will be disabled in suspend.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> This feels like it's the wrong way round - if this is configurable I'd
> expect something to configure the suspend mode and then for that to
> arrange to configure the regulator appropriately (along with anything
> else that needs doing) rather than to infer the configuration from the
> regulator state which feels fragile.  But based on the cover letter
> that's kind of like what the initial proposal about target states was so
> perhaps this is the way we end up going... 

Are you talking about [1] ?

this certainly looks a lot
> less impactful that the target state stuff though.
> 

For the moment, the patches which describes the regulators states in
suspend for SAMA5D2 Xplained board (which we are trying to address here)
are in pending [2] (they were introduced with patches for act8945a
suspend/resume stuff). Probably they will be introduced after one more
Linux version.

I can get rid of this patch, take advantage of [3] and [4] and introduce
also the regulator standby states. In this case, no matter the mapping b/w
Linux power saving modes and AT91 SoC's power saving modes, we will be
covered on misconfiguration (at least on SAMA5D2 Xplained board).

And in patch 3/3 I could get rid of regulator checks and rely on DT (bad
thing would be that in case of no input for regulator's state in
mem/standby the board could not properly suspended/resumed), if any.

What do you think about this?

Thank you,
Claudiu Beznea

[1] https://patchwork.kernel.org/patch/9458445/
[2]
https://lore.kernel.org/lkml/1544543768-2066-6-git-send-email-claudiu.beznea@microchip.com/
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git/commit/?h=for-next&id=f2b4076988a9c229dab373470b4b108ef0e106c8
[4]
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git/commit/?h=for-next&id=5279e96ff8033500b6008be5925ae2d20f42c434

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] regulator: core: add helper to check if regulator is disabled in suspend

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1303 bytes --]

On Thu, Jan 10, 2019 at 10:24:26AM +0000, Claudiu.Beznea@microchip.com wrote:
> On 09.01.2019 18:57, Mark Brown wrote:

> > regulator state which feels fragile.  But based on the cover letter
> > that's kind of like what the initial proposal about target states was so
> > perhaps this is the way we end up going... 

> Are you talking about [1] ?

I can't follow that link right now, I'm working offline.

> I can get rid of this patch, take advantage of [3] and [4] and introduce
> also the regulator standby states. In this case, no matter the mapping b/w
> Linux power saving modes and AT91 SoC's power saving modes, we will be
> covered on misconfiguration (at least on SAMA5D2 Xplained board).

> And in patch 3/3 I could get rid of regulator checks and rely on DT (bad
> thing would be that in case of no input for regulator's state in
> mem/standby the board could not properly suspended/resumed), if any.

> What do you think about this?

Like I say I'm working offline so I can't check the links but it sounds
like you're saying that the existing suspend mode configuration features
are enough for your systems?  If so that's great - certainly what you're
saying above sounds sensible to me but it's possible I misunderstood
something based on not having the links.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] regulator: core: add helper to check if regulator is disabled in suspend

[Claudiu.Beznea]

On 11.01.2019 14:39, Mark Brown wrote:
> On Thu, Jan 10, 2019 at 10:24:26AM +0000, Claudiu.Beznea@microchip.com wrote:
>> On 09.01.2019 18:57, Mark Brown wrote:
> 
>>> regulator state which feels fragile.  But based on the cover letter
>>> that's kind of like what the initial proposal about target states was so
>>> perhaps this is the way we end up going... 
> 
>> Are you talking about [1] ?
> 
> I can't follow that link right now, I'm working offline.
> 
>> I can get rid of this patch, take advantage of [3] and [4] and introduce
>> also the regulator standby states. In this case, no matter the mapping b/w
>> Linux power saving modes and AT91 SoC's power saving modes, we will be
>> covered on misconfiguration (at least on SAMA5D2 Xplained board).
> 
>> And in patch 3/3 I could get rid of regulator checks and rely on DT (bad
>> thing would be that in case of no input for regulator's state in
>> mem/standby the board could not properly suspended/resumed), if any.
> 
>> What do you think about this?
> 
> Like I say I'm working offline so I can't check the links but it sounds
> like you're saying that the existing suspend mode configuration features
> are enough for your systems? 

Yes, if we rely on the fact that core's regulator device tree bindings for
suspend-to-mem/suspend-to-standby were filled correctly.

The function I added here was to double check that core's regulator will be
off in suspend/standby based on what was parsed from DT.

Thank you,
Claudiu Beznea

> so that's great - certainly what you're
> saying above sounds sensible to me but it's possible I misunderstood
> something based on not having the links.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] regulator: core: add helper to check if regulator is disabled in suspend

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 680 bytes --]

On Fri, Jan 11, 2019 at 02:08:19PM +0000, Claudiu.Beznea@microchip.com wrote:
> On 11.01.2019 14:39, Mark Brown wrote:

> > Like I say I'm working offline so I can't check the links but it sounds
> > like you're saying that the existing suspend mode configuration features
> > are enough for your systems? 

> Yes, if we rely on the fact that core's regulator device tree bindings for
> suspend-to-mem/suspend-to-standby were filled correctly.

> The function I added here was to double check that core's regulator will be
> off in suspend/standby based on what was parsed from DT.

Ah, so it's being used as a consistency check?  OK, that does make sense
to me.  

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] regulator: core: add helper to check if regulator is disabled in suspend

[Claudiu.Beznea]

On 15.01.2019 00:53, Mark Brown wrote:
> On Fri, Jan 11, 2019 at 02:08:19PM +0000, Claudiu.Beznea@microchip.com wrote:
>> On 11.01.2019 14:39, Mark Brown wrote:
> 
>>> Like I say I'm working offline so I can't check the links but it sounds
>>> like you're saying that the existing suspend mode configuration features
>>> are enough for your systems? 
> 
>> Yes, if we rely on the fact that core's regulator device tree bindings for
>> suspend-to-mem/suspend-to-standby were filled correctly.
> 
>> The function I added here was to double check that core's regulator will be
>> off in suspend/standby based on what was parsed from DT.
> 
> Ah, so it's being used as a consistency check? 

Yes, that was the idea.

Thank you,
Claudiu Beznea

> OK, that does make sense
> to me.  
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/3] ARM: at91: pm: add support for .off_in_suspend

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Add support to check if platform is off in suspend. The check is based on
pm_data.mode and CPU's regulator which will be turn off in suspend.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 arch/arm/mach-at91/pm.c | 61 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 57 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-at91/pm.c b/arch/arm/mach-at91/pm.c
index 51e808adb00c..be9bd482772b 100644
--- a/arch/arm/mach-at91/pm.c
+++ b/arch/arm/mach-at91/pm.c
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/parser.h>
+#include <linux/regulator/consumer.h>
 #include <linux/suspend.h>
 
 #include <linux/clk/at91_pmc.h>
@@ -164,6 +165,47 @@ static int at91_pm_config_ws(unsigned int pm_mode, bool set)
 	return mode ? 0 : -EPERM;
 }
 
+static const struct of_device_id sama5d2_cpu_reg_ids[] = {
+	{ .compatible = "active-semi,act8945a",		.data = "VDD_1V2" },
+	{ /* sentinel */ }
+};
+
+static struct regulator *at91_pm_cpu_regulator_get(void)
+{
+	struct device *cpu_dev = get_cpu_device(0);
+	static struct regulator *cpu_reg;
+	const struct of_device_id *match;
+	struct platform_device *pdev;
+	struct device_node *np;
+
+	if (!cpu_dev)
+		return cpu_reg;
+
+	if (!cpu_reg) {
+		for_each_matching_node_and_match(np, sama5d2_cpu_reg_ids,
+						 &match) {
+			pdev = of_find_device_by_node(np);
+			if (!pdev)
+				continue;
+
+			cpu_reg = regulator_get(cpu_dev, match->data);
+			put_device(&pdev->dev);
+			break;
+		}
+	}
+
+	return cpu_reg;
+}
+
+static bool at91_pm_state_allowed(suspend_state_t state)
+{
+	struct regulator *reg;
+
+	reg = at91_pm_cpu_regulator_get();
+
+	return reg && regulator_is_disabled_in_suspend(reg, state);
+}
+
 /*
  * Called after processes are frozen, but before we shutdown devices.
  */
@@ -182,6 +224,9 @@ static int at91_pm_begin(suspend_state_t state)
 		pm_data.mode = -1;
 	}
 
+	if (pm_data.mode == AT91_PM_BACKUP && !at91_pm_state_allowed(state))
+		return -EPERM;
+
 	return at91_pm_config_ws(pm_data.mode, true);
 }
 
@@ -321,12 +366,20 @@ static void at91_pm_end(void)
 	at91_pm_config_ws(pm_data.mode, false);
 }
 
+static bool at91_pm_off_in_suspend(suspend_state_t state)
+{
+	if (pm_data.mode != AT91_PM_BACKUP)
+		return false;
+
+	return at91_pm_state_allowed(state);
+}
 
 static const struct platform_suspend_ops at91_pm_ops = {
-	.valid	= at91_pm_valid_state,
-	.begin	= at91_pm_begin,
-	.enter	= at91_pm_enter,
-	.end	= at91_pm_end,
+	.valid		= at91_pm_valid_state,
+	.begin		= at91_pm_begin,
+	.enter		= at91_pm_enter,
+	.end		= at91_pm_end,
+	.off_in_suspend	= at91_pm_off_in_suspend,
 };
 
 static struct platform_device at91_cpuidle_device = {
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/3] add support for power off check in suspend

[Rafael J. Wysocki]

On Tue, Jan 8, 2019 at 11:56 AM <Claudiu.Beznea@microchip.com> wrote:
>
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
>
> Hi,
>
> AT91 platforms support a power saving mode where SoC's power is cut off (we call
> it backup mode). The resume is done with the help of bootloaders.

But still the contents of RAM are preserved?

That would require at least the memory controller to be under power AFAICS.

> To be able to
> suspend/resume Linux to/from this mode all the drivers suspend/resume callbacks
> should save/restore the content of all the active registers. We have 2 problems
> we are trying to solve:
> - some of these drivers are shared with other non Microchip SoCs (e.g. macb
>   driver) and we don't want to disturbe other users of corresponding IPs with
>   all the register save/restore operations;
> - the suspend/resume time for the rest of the power saving mode we are using
>   could be improved if we would know in drivers the suspend mode the platform
>   is switched to.
>
> A solution that would have been solve our problems was proposed in [1] but in
> the end it wasn't accepted. It ended up with the introduction of
> pm_suspend_target_state variable that could be used along with the changes in
> this series.
>
> While the discussion of [1] progressed it has been proposed (in [2]) to
> implement a function that would tell if the platform's power would be cut off
> at the end of the suspend procedure.
>
> The patches in this series does as follows:
> 1/3 - add a new member to platform_suspend_ops that will tell if platform's
>       power will be cut off at the end of the suspend procedure; drivers could
>       use it via platform_off_in_suspend()

I would rather avoid doing this if possible.

Thanks,
Rafael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/3] add support for power off check in suspend

[Claudiu.Beznea]

On 08.01.2019 13:46, Rafael J. Wysocki wrote:
> On Tue, Jan 8, 2019 at 11:56 AM <Claudiu.Beznea@microchip.com> wrote:
>>
>> From: Claudiu Beznea <claudiu.beznea@microchip.com>
>>
>> Hi,
>>
>> AT91 platforms support a power saving mode where SoC's power is cut off (we call
>> it backup mode). The resume is done with the help of bootloaders.
> 
> But still the contents of RAM are preserved?

Yes, the RAM is kept alive, in self-refresh (forgot to mention).
Besides this, there is the so called backup area which contains RTC
shutdown controller, RC oscillator which remains powered. The rest is off.

> 
> That would require at least the memory controller to be under power AFAICS.

Yes, this is powered by a different regulator.

> 
>> To be able to
>> suspend/resume Linux to/from this mode all the drivers suspend/resume callbacks
>> should save/restore the content of all the active registers. We have 2 problems
>> we are trying to solve:
>> - some of these drivers are shared with other non Microchip SoCs (e.g. macb
>>   driver) and we don't want to disturbe other users of corresponding IPs with
>>   all the register save/restore operations;
>> - the suspend/resume time for the rest of the power saving mode we are using
>>   could be improved if we would know in drivers the suspend mode the platform
>>   is switched to.
>>
>> A solution that would have been solve our problems was proposed in [1] but in
>> the end it wasn't accepted. It ended up with the introduction of
>> pm_suspend_target_state variable that could be used along with the changes in
>> this series.
>>
>> While the discussion of [1] progressed it has been proposed (in [2]) to
>> implement a function that would tell if the platform's power would be cut off
>> at the end of the suspend procedure.
>>
>> The patches in this series does as follows:
>> 1/3 - add a new member to platform_suspend_ops that will tell if platform's
>>       power will be cut off at the end of the suspend procedure; drivers could
>>       use it via platform_off_in_suspend()
> 
> I would rather avoid doing this if possible.

Having this mechanism as part of platform_suspend_ops? Or this mechanism at
all?
Could you give me some hints on how would you prefer to do it, if any?

Thank you,
Claudiu Beznea

> 
> Thanks,
> Rafael
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/3] add support for power off check in suspend

[Claudiu.Beznea]

Hi Pavel, Rafael,

Do you have some other hints I could follow in order to solve the
scenario I tried to cover with these patches, if these are not
a proper fit for the current code?

Thank you,
Claudiu Beznea

On 08.01.2019 12:56, Claudiu Beznea - M18063 wrote:
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> Hi,
> 
> AT91 platforms support a power saving mode where SoC's power is cut off (we call
> it backup mode). The resume is done with the help of bootloaders. To be able to
> suspend/resume Linux to/from this mode all the drivers suspend/resume callbacks
> should save/restore the content of all the active registers. We have 2 problems
> we are trying to solve:
> - some of these drivers are shared with other non Microchip SoCs (e.g. macb
>   driver) and we don't want to disturbe other users of corresponding IPs with
>   all the register save/restore operations;
> - the suspend/resume time for the rest of the power saving mode we are using
>   could be improved if we would know in drivers the suspend mode the platform
>   is switched to.
> 
> A solution that would have been solve our problems was proposed in [1] but in
> the end it wasn't accepted. It ended up with the introduction of
> pm_suspend_target_state variable that could be used along with the changes in
> this series.
> 
> While the discussion of [1] progressed it has been proposed (in [2]) to
> implement a function that would tell if the platform's power would be cut off
> at the end of the suspend procedure.
> 
> The patches in this series does as follows:
> 1/3 - add a new member to platform_suspend_ops that will tell if platform's
>       power will be cut off at the end of the suspend procedure; drivers could
>       use it via platform_off_in_suspend()
> 2/3 - add a new function to regulator's core that could be used to check if a
>       proper regulator has been configured (via DT) to be powered off in
>       suspend. This is used on this series to check the CPU's regulator is
>       properly configured in DT to be turned off in suspend.
> 3/3 - fill .off_in_suspend member of at91_pm_ops; the functionality in patch 2/3
>       is used to double check CPU's regulator would be turned off in suspend.
> 
> Thank you,
> Claudiu Beznea
> 
> [1] https://lkml.org/lkml/2017/6/22/938
> [2] https://lkml.org/lkml/2017/7/16/457
> 
> Changes in v2:
> - remove !! instruction in regulator_is_disabled_in_suspend() and
>   at91_pm_state_allowed()
> - s/weather/wether in patch 1/1
> 
> Claudiu Beznea (3):
>   PM / Suspend: Add support to check if platform's power is off in
>     suspend
>   regulator: core: add helper to check if regulator is disabled in
>     suspend
>   ARM: at91: pm: add support for .off_in_suspend
> 
>  arch/arm/mach-at91/pm.c            | 61 +++++++++++++++++++++++++++++++++++---
>  drivers/regulator/core.c           | 17 +++++++++++
>  include/linux/regulator/consumer.h |  7 +++++
>  include/linux/suspend.h            |  6 ++++
>  kernel/power/suspend.c             | 13 ++++++++
>  5 files changed, 100 insertions(+), 4 deletions(-)
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel