[PATCH v2 0/2] ARM: EXYNOS: Call regulator suspend prepare/finish

[PATCH v2 0/2] ARM: EXYNOS: Call regulator suspend prepare/finish

[Javier Martinez Canillas]

Hello,

This series add support for Exynos platforms to prepare regulators for
system suspend. The regulator core has a set of helpers functions to be
used when the system is entering and leaving from a suspend state but
currently there is only one user in mainline.

This user is drivers/mfd/sec-core.c but it calls regulator_suspend_prepare()
from within the driver power-management suspend function. This does not
seems to be correct since the regulator suspend prepare function affects all
regulators in the system and not only the ones managed by this device.

So patch #1 in this series revert the commit that introduced that change and
patch #2 calls the regulator framework suspend/finish functions from the
Exynos platform power-management code. The first patch should be queued through
the mfd tree and the second through the linux-samsung tree.

Changes since v1:
 - Remove the call to regulator_suspend_prepare() from drivers/mfd/sec-core.c
   as suggested by Doug Anderson.
 - Call regulator_suspend_prepare() before s3c_pm_check_prepare() as suggested
   by Doug Anderson.
 - Added Lee Jones to cc list since there is a change for the mfd framework.

Javier Martinez Canillas (2):
  Revert "mfd: sec-core: Prepare regulators for suspend state to reduce
    power-consumption"
  ARM: EXYNOS: Call regulator core suspend prepare and finish functions

 arch/arm/mach-exynos/suspend.c | 18 ++++++++++++++++++
 drivers/mfd/Kconfig            |  1 -
 drivers/mfd/sec-core.c         | 10 ----------
 3 files changed, 18 insertions(+), 11 deletions(-)

NOTES:

* The drivers/mfd/sec-core.c only called regulator_suspend_prepare() on
  suspend but did not call regulator_suspend_finish() on resume so this
  series changes the assumption the driver did that the regulators .enable
  functions won't be called on resume and the regulators should be recovered
  automatically from suspend. So it may cause issues on this device since
  the semantics are changed. But I think not calling suspend finish was not
  correct in the first place.

* This could also be made from the suspend core (kernel/power/suspend.c) to
  avoid each platform support having the same code but I was not sure about
  this so I decided to it at the platform level.
  Another option is that the regulator core register a pm_notifier that could
  handle the PM_SUSPEND_PREPARE and PM_POST_SUSPEND events.

Best regards,
Javier

[PATCH v2 1/2] Revert "mfd: sec-core: Prepare regulators for suspend state to reduce power-consumption"

[Javier Martinez Canillas]

This reverts commit b7cde7078d2344073c310aa65fc2b0a845d2cb5b
("mfd: sec-core: Prepare regulators for suspend state to reduce power-consumption")

Commit b7cde7078d23 called regulator_suspend_prepare() to prepare the
regulators for a suspend state. But did it from the device pm suspend
handler but regulator_suspend_prepare() iterates over all regulators
and not only the one managed by this device so it doesn't seems to be
correct to call it from a device driver.

It is better to call the regulator suspend prepare/finish functions
from platform code instead so this patch reverts the mentioned commit.
---
 drivers/mfd/Kconfig    |  1 -
 drivers/mfd/sec-core.c | 10 ----------
 2 files changed, 11 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 1456ea7..fd8cc4c 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -655,7 +655,6 @@ config MFD_SEC_CORE
 	select MFD_CORE
 	select REGMAP_I2C
 	select REGMAP_IRQ
-	select REGULATOR
 	help
 	 Support for the Samsung Electronics MFD series.
 	 This driver provides common support for accessing the device,
diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
index dba7e2b..5993608 100644
--- a/drivers/mfd/sec-core.c
+++ b/drivers/mfd/sec-core.c
@@ -31,7 +31,6 @@
 #include <linux/mfd/samsung/s2mpu02.h>
 #include <linux/mfd/samsung/s5m8763.h>
 #include <linux/mfd/samsung/s5m8767.h>
-#include <linux/regulator/machine.h>
 #include <linux/regmap.h>
 
 static const struct mfd_cell s5m8751_devs[] = {
@@ -432,15 +431,6 @@ static int sec_pmic_suspend(struct device *dev)
 	 */
 	disable_irq(sec_pmic->irq);
 
-	switch (sec_pmic->device_type) {
-	case S2MPS14X:
-	case S2MPU02:
-		regulator_suspend_prepare(PM_SUSPEND_MEM);
-		break;
-	default:
-		break;
-	}
-
 	return 0;
 }
 
-- 
2.1.0

[PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions

[Javier Martinez Canillas]

The regulator framework has a set of helpers functions to be used when
the system is entering and leaving from suspend but these are not called
on Exynos platforms. This means that the .set_suspend_* function handlers
defined by regulator drivers are not called when the system is suspended.

Suggested-by: Doug Anderson <dianders@chromium.org>
Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
---
 arch/arm/mach-exynos/suspend.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
index cc8d237..ee9a8e0 100644
--- a/arch/arm/mach-exynos/suspend.c
+++ b/arch/arm/mach-exynos/suspend.c
@@ -20,6 +20,7 @@
 #include <linux/io.h>
 #include <linux/irqchip/arm-gic.h>
 #include <linux/err.h>
+#include <linux/regulator/machine.h>
 
 #include <asm/cacheflush.h>
 #include <asm/hardware/cache-l2x0.h>
@@ -443,6 +444,22 @@ static int exynos_suspend_enter(suspend_state_t state)
 
 static int exynos_suspend_prepare(void)
 {
+	int ret;
+
+	/*
+	 * REVISIT: It would be better if struct platform_suspend_ops
+	 * .prepare handler get the suspend_state_t as a parameter to
+	 * avoid hard-coding the suspend to mem state. It's safe to do
+	 * it now only because the suspend_valid_only_mem function is
+	 * used as the .valid callback used to check if a given state
+	 * is supported by the platform anyways.
+	 */
+	ret = regulator_suspend_prepare(PM_SUSPEND_MEM);
+	if (ret) {
+		pr_err("Failed to prepare regulators for system suspend\n");
+		return ret;
+	}
+
 	s3c_pm_check_prepare();
 
 	return 0;
@@ -451,6 +468,7 @@ static int exynos_suspend_prepare(void)
 static void exynos_suspend_finish(void)
 {
 	s3c_pm_check_cleanup();
+	regulator_suspend_finish();
 }
 
 static const struct platform_suspend_ops exynos_suspend_ops = {
-- 
2.1.0

[PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions

[Chanwoo Choi]

Hi Javier,

On 10/16/2014 07:13 PM, Javier Martinez Canillas wrote:
> The regulator framework has a set of helpers functions to be used when
> the system is entering and leaving from suspend but these are not called
> on Exynos platforms. This means that the .set_suspend_* function handlers
> defined by regulator drivers are not called when the system is suspended.
> 
> Suggested-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  arch/arm/mach-exynos/suspend.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
> index cc8d237..ee9a8e0 100644
> --- a/arch/arm/mach-exynos/suspend.c
> +++ b/arch/arm/mach-exynos/suspend.c
> @@ -20,6 +20,7 @@
>  #include <linux/io.h>
>  #include <linux/irqchip/arm-gic.h>
>  #include <linux/err.h>
> +#include <linux/regulator/machine.h>
>  
>  #include <asm/cacheflush.h>
>  #include <asm/hardware/cache-l2x0.h>
> @@ -443,6 +444,22 @@ static int exynos_suspend_enter(suspend_state_t state)
>  
>  static int exynos_suspend_prepare(void)
>  {
> +	int ret;
> +
> +	/*
> +	 * REVISIT: It would be better if struct platform_suspend_ops
> +	 * .prepare handler get the suspend_state_t as a parameter to
> +	 * avoid hard-coding the suspend to mem state. It's safe to do
> +	 * it now only because the suspend_valid_only_mem function is
> +	 * used as the .valid callback used to check if a given state
> +	 * is supported by the platform anyways.
> +	 */
> +	ret = regulator_suspend_prepare(PM_SUSPEND_MEM);
> +	if (ret) {
> +		pr_err("Failed to prepare regulators for system suspend\n");
> +		return ret;
> +	}
> +
>  	s3c_pm_check_prepare();
>  
>  	return 0;
> @@ -451,6 +468,7 @@ static int exynos_suspend_prepare(void)
>  static void exynos_suspend_finish(void)
>  {
>  	s3c_pm_check_cleanup();
> +	regulator_suspend_finish();
>  }
>  
>  static const struct platform_suspend_ops exynos_suspend_ops = {
> 

Looks good to me.

Reviewed-by: Chanwoo Choi<cw00.choi@samsung.com>

Thanks,
Chanwoo Choi

[PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions

[Doug Anderson]

Javier,

On Thu, Oct 16, 2014 at 3:13 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
> The regulator framework has a set of helpers functions to be used when
> the system is entering and leaving from suspend but these are not called
> on Exynos platforms. This means that the .set_suspend_* function handlers
> defined by regulator drivers are not called when the system is suspended.
>
> Suggested-by: Doug Anderson <dianders@chromium.org>
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  arch/arm/mach-exynos/suspend.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
> index cc8d237..ee9a8e0 100644
> --- a/arch/arm/mach-exynos/suspend.c
> +++ b/arch/arm/mach-exynos/suspend.c
> @@ -20,6 +20,7 @@
>  #include <linux/io.h>
>  #include <linux/irqchip/arm-gic.h>
>  #include <linux/err.h>
> +#include <linux/regulator/machine.h>
>
>  #include <asm/cacheflush.h>
>  #include <asm/hardware/cache-l2x0.h>
> @@ -443,6 +444,22 @@ static int exynos_suspend_enter(suspend_state_t state)
>
>  static int exynos_suspend_prepare(void)
>  {
> +       int ret;
> +
> +       /*
> +        * REVISIT: It would be better if struct platform_suspend_ops
> +        * .prepare handler get the suspend_state_t as a parameter to
> +        * avoid hard-coding the suspend to mem state. It's safe to do
> +        * it now only because the suspend_valid_only_mem function is
> +        * used as the .valid callback used to check if a given state
> +        * is supported by the platform anyways.
> +        */
> +       ret = regulator_suspend_prepare(PM_SUSPEND_MEM);
> +       if (ret) {
> +               pr_err("Failed to prepare regulators for system suspend\n");
> +               return ret;
> +       }
> +
>         s3c_pm_check_prepare();
>
>         return 0;
> @@ -451,6 +468,7 @@ static int exynos_suspend_prepare(void)
>  static void exynos_suspend_finish(void)
>  {
>         s3c_pm_check_cleanup();
> +       regulator_suspend_finish();

It turns out that regulator_suspend_finish() actually returns an error
code.  Could you print a warning if you see it?

Other than that, feel free to add my Reviewed-by.  Thanks!

-Doug

[PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions

[Javier Martinez Canillas]

[adding Chris Zong as cc who posted a similar patch for Rockchip]

Hello Doug,

On 10/20/2014 06:26 PM, Doug Anderson wrote:
> Javier,
> 
> On Thu, Oct 16, 2014 at 3:13 AM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>> The regulator framework has a set of helpers functions to be used when
>> the system is entering and leaving from suspend but these are not called
>> on Exynos platforms. This means that the .set_suspend_* function handlers
>> defined by regulator drivers are not called when the system is suspended.
>>
>> Suggested-by: Doug Anderson <dianders@chromium.org>
>> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
>> ---
>>  arch/arm/mach-exynos/suspend.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c
>> index cc8d237..ee9a8e0 100644
>> --- a/arch/arm/mach-exynos/suspend.c
>> +++ b/arch/arm/mach-exynos/suspend.c
>> @@ -20,6 +20,7 @@
>>  #include <linux/io.h>
>>  #include <linux/irqchip/arm-gic.h>
>>  #include <linux/err.h>
>> +#include <linux/regulator/machine.h>
>>
>>  #include <asm/cacheflush.h>
>>  #include <asm/hardware/cache-l2x0.h>
>> @@ -443,6 +444,22 @@ static int exynos_suspend_enter(suspend_state_t state)
>>
>>  static int exynos_suspend_prepare(void)
>>  {
>> +       int ret;
>> +
>> +       /*
>> +        * REVISIT: It would be better if struct platform_suspend_ops
>> +        * .prepare handler get the suspend_state_t as a parameter to
>> +        * avoid hard-coding the suspend to mem state. It's safe to do
>> +        * it now only because the suspend_valid_only_mem function is
>> +        * used as the .valid callback used to check if a given state
>> +        * is supported by the platform anyways.
>> +        */
>> +       ret = regulator_suspend_prepare(PM_SUSPEND_MEM);
>> +       if (ret) {
>> +               pr_err("Failed to prepare regulators for system suspend\n");
>> +               return ret;
>> +       }
>> +
>>         s3c_pm_check_prepare();
>>
>>         return 0;
>> @@ -451,6 +468,7 @@ static int exynos_suspend_prepare(void)
>>  static void exynos_suspend_finish(void)
>>  {
>>         s3c_pm_check_cleanup();
>> +       regulator_suspend_finish();
> 
> It turns out that regulator_suspend_finish() actually returns an error
> code.  Could you print a warning if you see it?
> 

Yes, I noticed this when looking at Chris patch for Rockchip but didn't re-spin
because I'm not sure anymore if this is the right solution. I mean, if is
correct to add the same calls on every platform or if the regulator suspend
prepare and finish functions should be called from the suspend core instead.

For example calling regulator_suspend_prepare() from platform_suspend_prepare()
[0] will have the advantage of passing the correct suspend_state_t state instead
of hard-coding PM_SUSPEND_MEM and will make the regulator suspend states to work
on all platforms.

> Other than that, feel free to add my Reviewed-by.  Thanks!
> 
> -Doug
> 

Best regards,
Javier

[0]: http://lxr.free-electrons.com/source/kernel/power/suspend.c#L141

[PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions

[Doug Anderson]

Javier,

On Mon, Oct 20, 2014 at 9:58 AM, Javier Martinez Canillas
<javier.martinez@collabora.co.uk> wrote:
>> It turns out that regulator_suspend_finish() actually returns an error
>> code.  Could you print a warning if you see it?
>>
>
> Yes, I noticed this when looking at Chris patch for Rockchip but didn't re-spin
> because I'm not sure anymore if this is the right solution. I mean, if is
> correct to add the same calls on every platform or if the regulator suspend
> prepare and finish functions should be called from the suspend core instead.
>
> For example calling regulator_suspend_prepare() from platform_suspend_prepare()
> [0] will have the advantage of passing the correct suspend_state_t state instead
> of hard-coding PM_SUSPEND_MEM and will make the regulator suspend states to work
> on all platforms.

Yes.  If we can get this added to the core that would be better.

I guess I was just trying to follow the suggestion that was in the
regulator code:
http://lxr.free-electrons.com/source/drivers/regulator/core.c#L3699
that says "This will usually be called by machine suspend code prior
to supending."

-Doug

[PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions

[Javier Martinez Canillas]

[adding Rafael Wysocki to cc as Suspend-to-RAM maintainer]

On 10/20/2014 07:36 PM, Doug Anderson wrote:
> Javier,
> 
> On Mon, Oct 20, 2014 at 9:58 AM, Javier Martinez Canillas
> <javier.martinez@collabora.co.uk> wrote:
>>> It turns out that regulator_suspend_finish() actually returns an error
>>> code.  Could you print a warning if you see it?
>>>
>>
>> Yes, I noticed this when looking at Chris patch for Rockchip but didn't re-spin
>> because I'm not sure anymore if this is the right solution. I mean, if is
>> correct to add the same calls on every platform or if the regulator suspend
>> prepare and finish functions should be called from the suspend core instead.
>>
>> For example calling regulator_suspend_prepare() from platform_suspend_prepare()
>> [0] will have the advantage of passing the correct suspend_state_t state instead
>> of hard-coding PM_SUSPEND_MEM and will make the regulator suspend states to work
>> on all platforms.
> 
> Yes.  If we can get this added to the core that would be better.
> 

Agreed, let's see what Rafael says about it.

> I guess I was just trying to follow the suggestion that was in the
> regulator code:
> http://lxr.free-electrons.com/source/drivers/regulator/core.c#L3699
> that says "This will usually be called by machine suspend code prior
> to supending."
> 
> -Doug
> 

I see, but still I feel as if it may be a lot of duplication since most
platforms will likely want to call the regulator core suspend prepare
and finish functions. Maybe it can be added as a Kconfig option so each
platform can choose at the config level if they want those to be called?

Best regard,
Javier

[PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions

[Mark Brown]

On Mon, Oct 20, 2014 at 09:50:57PM +0200, Javier Martinez Canillas wrote:
> On 10/20/2014 07:36 PM, Doug Anderson wrote:

> > I guess I was just trying to follow the suggestion that was in the
> > regulator code:
> > http://lxr.free-electrons.com/source/drivers/regulator/core.c#L3699
> > that says "This will usually be called by machine suspend code prior
> > to supending."

> I see, but still I feel as if it may be a lot of duplication since most
> platforms will likely want to call the regulator core suspend prepare

Note that architectures are an example of a platform.  It really depends
what's responsible for final poweroff, we want this called as late as we
possibly can.

> and finish functions. Maybe it can be added as a Kconfig option so each
> platform can choose at the config level if they want those to be called?

No, that's obviously not going to do anything useful for multiplatform.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141020/c97771a0/attachment.sig>

[PATCH v2 2/2] ARM: EXYNOS: Call regulator core suspend prepare and finish functions

[Javier Martinez Canillas]

Hello Mark,

On 10/20/2014 09:56 PM, Mark Brown wrote:
> On Mon, Oct 20, 2014 at 09:50:57PM +0200, Javier Martinez Canillas wrote:
>> On 10/20/2014 07:36 PM, Doug Anderson wrote:
> 
>> > I guess I was just trying to follow the suggestion that was in the
>> > regulator code:
>> > http://lxr.free-electrons.com/source/drivers/regulator/core.c#L3699
>> > that says "This will usually be called by machine suspend code prior
>> > to supending."
> 
>> I see, but still I feel as if it may be a lot of duplication since most
>> platforms will likely want to call the regulator core suspend prepare
> 
> Note that architectures are an example of a platform.  It really depends
> what's responsible for final poweroff, we want this called as late as we
> possibly can.
> 

Got it. Thanks for the explanation.

>> and finish functions. Maybe it can be added as a Kconfig option so each
>> platform can choose at the config level if they want those to be called?
> 
> No, that's obviously not going to do anything useful for multiplatform.
> 

Ok, then let's keep to do it per-platform as is proposed on $subject for
Exynos and what Chris proposed for Rockchip in [0] since it seems that's
the place where these calls belong.

Best regards,
Javier

[0]: http://www.spinics.net/lists/devicetree/msg53640.html