[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

[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;
}

[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 <***@chromium.org>
Signed-off-by: Javier Martinez Canillas <***@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 = {

[Chanwoo Choi]

Hi Javier,

Post by 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.
---
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<***@samsung.com>

Thanks,
Chanwoo Choi

[Doug Anderson]

Javier,

On Thu, Oct 16, 2014 at 3:13 AM, Javier Martinez Canillas

Post by 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.
---
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

[Javier Martinez Canillas]

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

Hello Doug,

Post by Chanwoo Choi
Javier,
On Thu, Oct 16, 2014 at 3:13 AM, Javier Martinez Canillas

Post by 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.
---
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.

Post by Chanwoo Choi
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

[Doug Anderson]

Javier,

On Mon, Oct 20, 2014 at 9:58 AM, Javier Martinez Canillas

Post by Javier Martinez Canillas

Post by Doug Anderson
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