ARM: dts: exynos: Add proper regulator states for suspend-to-mem for odroid-u3
diff mbox series

Message ID 20181204194025.2719-1-linux.amoon@gmail.com
State New
Headers show
Series
  • ARM: dts: exynos: Add proper regulator states for suspend-to-mem for odroid-u3
Related show

Commit Message

Anand Moon Dec. 4, 2018, 7:40 p.m. UTC
Add suspend-to-mem node to regulator core to be enabled or disabled
during system suspend and also support changing the regulator operating
mode during runtime and when the system enter sleep mode.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
Tested on Odroid U3+
---
 .../boot/dts/exynos4412-odroid-common.dtsi    | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)

Comments

Krzysztof Kozlowski Dec. 5, 2018, 2:06 p.m. UTC | #1
On Tue, 4 Dec 2018 at 20:40, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Add suspend-to-mem node to regulator core to be enabled or disabled
> during system suspend and also support changing the regulator operating
> mode during runtime and when the system enter sleep mode.
>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> Tested on Odroid U3+
> ---
>  .../boot/dts/exynos4412-odroid-common.dtsi    | 72 +++++++++++++++++++
>  1 file changed, 72 insertions(+)
>
> diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> index 2caa3132f34e..837713a2ec3b 100644
> --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> @@ -285,6 +285,9 @@
>                                 regulator-min-microvolt = <1800000>;
>                                 regulator-max-microvolt = <1800000>;
>                                 regulator-always-on;
> +                               regulator-state-mem {
> +                                       regulator-on-in-suspend;
> +                               };

Driver does not support suspend_enable so this will be noop. We could
add this for DT-correctness (and full description of HW) but then I
need explanation why this regulator has to stay on during suspend.

>                         };
>
>                         ldo3_reg: LDO3 {
> @@ -292,6 +295,9 @@
>                                 regulator-min-microvolt = <1800000>;
>                                 regulator-max-microvolt = <1800000>;
>                                 regulator-always-on;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };

The same but with suspend_disable.

I am not saying that these are wrong but I would be happy to see
explanations why you chosen these particular properties.

>                         };
>
>                         ldo4_reg: LDO4 {
> @@ -299,6 +305,9 @@
>                                 regulator-min-microvolt = <2800000>;
>                                 regulator-max-microvolt = <2800000>;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-on-in-suspend;
> +                               };
>                         };
>
>                         ldo5_reg: LDO5 {
> @@ -307,6 +316,9 @@
>                                 regulator-max-microvolt = <1800000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-on-in-suspend;
> +                               };
>                         };
>
>                         ldo6_reg: LDO6 {
> @@ -314,6 +326,9 @@
>                                 regulator-min-microvolt = <1000000>;
>                                 regulator-max-microvolt = <1000000>;
>                                 regulator-always-on;
> +                               regulator-state-mem {
> +                                       regulator-on-in-suspend;
> +                               };
>                         };
>
>                         ldo7_reg: LDO7 {
> @@ -321,18 +336,27 @@
>                                 regulator-min-microvolt = <1000000>;
>                                 regulator-max-microvolt = <1000000>;
>                                 regulator-always-on;
> +                               regulator-state-mem {
> +                                       regulator-on-in-suspend;
> +                               };
>                         };
>
>                         ldo8_reg: LDO8 {
>                                 regulator-name = "VDD10_HDMI_1.0V";
>                                 regulator-min-microvolt = <1000000>;
>                                 regulator-max-microvolt = <1000000>;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         ldo10_reg: LDO10 {
>                                 regulator-name = "VDDQ_MIPIHSI_1.8V";
>                                 regulator-min-microvolt = <1800000>;
>                                 regulator-max-microvolt = <1800000>;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         ldo11_reg: LDO11 {
> @@ -340,6 +364,9 @@
>                                 regulator-min-microvolt = <1800000>;
>                                 regulator-max-microvolt = <1800000>;
>                                 regulator-always-on;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         ldo12_reg: LDO12 {
> @@ -348,6 +375,9 @@
>                                 regulator-max-microvolt = <3300000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         ldo13_reg: LDO13 {
> @@ -356,6 +386,9 @@
>                                 regulator-max-microvolt = <1800000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         ldo14_reg: LDO14 {
> @@ -364,6 +397,9 @@
>                                 regulator-max-microvolt = <1800000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         ldo15_reg: LDO15 {
> @@ -372,6 +408,9 @@
>                                 regulator-max-microvolt = <1000000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         ldo16_reg: LDO16 {
> @@ -380,6 +419,9 @@
>                                 regulator-max-microvolt = <1800000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-on-in-suspend;
> +                               };
>                         };
>
>                         ldo20_reg: LDO20 {
> @@ -387,6 +429,9 @@
>                                 regulator-min-microvolt = <1800000>;
>                                 regulator-max-microvolt = <1800000>;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         ldo21_reg: LDO21 {
> @@ -394,6 +439,9 @@
>                                 regulator-min-microvolt = <2800000>;
>                                 regulator-max-microvolt = <2800000>;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-on-in-suspend;

That's unusual setting... enabled in suspend but not necessarily
during work (no always-on).

> +                               };
>                         };
>
>                         ldo22_reg: LDO22 {
> @@ -411,6 +459,9 @@
>                                 regulator-max-microvolt = <1800000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         buck1_reg: BUCK1 {
> @@ -419,6 +470,9 @@
>                                 regulator-max-microvolt = <1100000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };

This is seriously wrong so I have doubts whether you tested the
changes or you know what is happening with them. If you turn off
memory bus regulator off, how the memory will work in
Suspend-to-Memory mode?

>                         };
>
>                         buck2_reg: BUCK2 {
> @@ -427,6 +481,9 @@
>                                 regulator-max-microvolt = <1350000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-on-in-suspend;
> +                               };
>                         };
>
>                         buck3_reg: BUCK3 {
> @@ -435,6 +492,9 @@
>                                 regulator-max-microvolt = <1050000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;

Looks suspicious as well.

Best regards,
Krzysztof

> +                               };
>                         };
>
>                         buck4_reg: BUCK4 {
> @@ -442,6 +502,9 @@
>                                 regulator-min-microvolt = <900000>;
>                                 regulator-max-microvolt = <1100000>;
>                                 regulator-microvolt-offset = <50000>;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         buck5_reg: BUCK5 {
> @@ -450,6 +513,9 @@
>                                 regulator-max-microvolt = <1200000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         buck6_reg: BUCK6 {
> @@ -458,6 +524,9 @@
>                                 regulator-max-microvolt = <1350000>;
>                                 regulator-always-on;
>                                 regulator-boot-on;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         buck7_reg: BUCK7 {
> @@ -465,6 +534,9 @@
>                                 regulator-min-microvolt = <2000000>;
>                                 regulator-max-microvolt = <2000000>;
>                                 regulator-always-on;
> +                               regulator-state-mem {
> +                                       regulator-off-in-suspend;
> +                               };
>                         };
>
>                         buck8_reg: BUCK8 {
> --
> 2.19.2
>
Anand Moon Dec. 5, 2018, 4:11 p.m. UTC | #2
Hi Krzysztof,

Thanks for your review.
.
On Wed, 5 Dec 2018 at 19:36, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Tue, 4 Dec 2018 at 20:40, Anand Moon <linux.amoon@gmail.com> wrote:
> >
> > Add suspend-to-mem node to regulator core to be enabled or disabled
> > during system suspend and also support changing the regulator operating
> > mode during runtime and when the system enter sleep mode.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > Tested on Odroid U3+
> > ---
> >  .../boot/dts/exynos4412-odroid-common.dtsi    | 72 +++++++++++++++++++
> >  1 file changed, 72 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > index 2caa3132f34e..837713a2ec3b 100644
> > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > @@ -285,6 +285,9 @@
> >                                 regulator-min-microvolt = <1800000>;
> >                                 regulator-max-microvolt = <1800000>;
> >                                 regulator-always-on;
> > +                               regulator-state-mem {
> > +                                       regulator-on-in-suspend;
> > +                               };
>
> Driver does not support suspend_enable so this will be noop. We could
> add this for DT-correctness (and full description of HW) but then I
> need explanation why this regulator has to stay on during suspend.
>

Well I tried to study the suspend/resume feature from
*arch/arm/boot/dts/exynos4412-midas.dtsi*
and them I tried to apply the same with this on Odroid-U3 boards and
test these changes.

> >                         };
> >
> >                         ldo3_reg: LDO3 {
> > @@ -292,6 +295,9 @@
> >                                 regulator-min-microvolt = <1800000>;
> >                                 regulator-max-microvolt = <1800000>;
> >                                 regulator-always-on;
> > +                               regulator-state-mem {
> > +                                       regulator-off-in-suspend;
> > +                               };
>
> The same but with suspend_disable.
>
> I am not saying that these are wrong but I would be happy to see
> explanations why you chosen these particular properties.
>

Well I was not aware on the regulator-always-on cannot be set to off
in suspend mode.
Will drop this in the future patch where regulator-always-on; is set.

> >                         };
> >
> >                         ldo4_reg: LDO4 {
> > @@ -299,6 +305,9 @@
> >                                 regulator-min-microvolt = <2800000>;
> >                                 regulator-max-microvolt = <2800000>;
> >                                 regulator-boot-on;
> > +                               regulator-state-mem {
> > +                                       regulator-on-in-suspend;
> > +                               };
> >                         };
> >
> >                         ldo5_reg: LDO5 {
> > @@ -307,6 +316,9 @@
> >                                 regulator-max-microvolt = <1800000>;
> >                                 regulator-always-on;
> >                                 regulator-boot-on;
> > +                               regulator-state-mem {
> > +                                       regulator-on-in-suspend;
> > +                               };
> >                         };
> >
> >                         ldo6_reg: LDO6 {
> > @@ -314,6 +326,9 @@
> >                                 regulator-min-microvolt = <1000000>;
> >                                 regulator-max-microvolt = <1000000>;
> >                                 regulator-always-on;
> > +                               regulator-state-mem {
> > +                                       regulator-on-in-suspend;
> > +                               };
> >                         };
> >
> >                         ldo7_reg: LDO7 {
> > @@ -321,18 +336,27 @@
> >                                 regulator-min-microvolt = <1000000>;
> >                                 regulator-max-microvolt = <1000000>;
> >                                 regulator-always-on;
> > +                               regulator-state-mem {
> > +                                       regulator-on-in-suspend;
> > +                               };
> >                         };
> >
> >                         ldo8_reg: LDO8 {
> >                                 regulator-name = "VDD10_HDMI_1.0V";
> >                                 regulator-min-microvolt = <1000000>;
> >                                 regulator-max-microvolt = <1000000>;
> > +                               regulator-state-mem {
> > +                                       regulator-off-in-suspend;
> > +                               };
> >                         };
> >
> >                         ldo10_reg: LDO10 {
> >                                 regulator-name = "VDDQ_MIPIHSI_1.8V";
> >                                 regulator-min-microvolt = <1800000>;
> >                                 regulator-max-microvolt = <1800000>;
> > +                               regulator-state-mem {
> > +                                       regulator-off-in-suspend;
> > +                               };
> >                         };
> >
> >                         ldo11_reg: LDO11 {
> > @@ -340,6 +364,9 @@
> >                                 regulator-min-microvolt = <1800000>;
> >                                 regulator-max-microvolt = <1800000>;
> >                                 regulator-always-on;
> > +                               regulator-state-mem {
> > +                                       regulator-off-in-suspend;
> > +                               };
> >                         };
> >
> >                         ldo12_reg: LDO12 {
> > @@ -348,6 +375,9 @@
> >                                 regulator-max-microvolt = <3300000>;
> >                                 regulator-always-on;
> >                                 regulator-boot-on;
> > +                               regulator-state-mem {
> > +                                       regulator-off-in-suspend;
> > +                               };
> >                         };
> >
> >                         ldo13_reg: LDO13 {
> > @@ -356,6 +386,9 @@
> >                                 regulator-max-microvolt = <1800000>;
> >                                 regulator-always-on;
> >                                 regulator-boot-on;
> > +                               regulator-state-mem {
> > +                                       regulator-off-in-suspend;
> > +                               };
> >                         };
> >
> >                         ldo14_reg: LDO14 {
> > @@ -364,6 +397,9 @@
> >                                 regulator-max-microvolt = <1800000>;
> >                                 regulator-always-on;
> >                                 regulator-boot-on;
> > +                               regulator-state-mem {
> > +                                       regulator-off-in-suspend;
> > +                               };
> >                         };
> >
> >                         ldo15_reg: LDO15 {
> > @@ -372,6 +408,9 @@
> >                                 regulator-max-microvolt = <1000000>;
> >                                 regulator-always-on;
> >                                 regulator-boot-on;
> > +                               regulator-state-mem {
> > +                                       regulator-off-in-suspend;
> > +                               };
> >                         };
> >
> >                         ldo16_reg: LDO16 {
> > @@ -380,6 +419,9 @@
> >                                 regulator-max-microvolt = <1800000>;
> >                                 regulator-always-on;
> >                                 regulator-boot-on;
> > +                               regulator-state-mem {
> > +                                       regulator-on-in-suspend;
> > +                               };
> >                         };
> >
> >                         ldo20_reg: LDO20 {
> > @@ -387,6 +429,9 @@
> >                                 regulator-min-microvolt = <1800000>;
> >                                 regulator-max-microvolt = <1800000>;
> >                                 regulator-boot-on;
> > +                               regulator-state-mem {
> > +                                       regulator-off-in-suspend;
> > +                               };
> >                         };
> >
> >                         ldo21_reg: LDO21 {
> > @@ -394,6 +439,9 @@
> >                                 regulator-min-microvolt = <2800000>;
> >                                 regulator-max-microvolt = <2800000>;
> >                                 regulator-boot-on;
> > +                               regulator-state-mem {
> > +                                       regulator-on-in-suspend;
>
> That's unusual setting... enabled in suspend but not necessarily
> during work (no always-on).
>

I kept the regulator required for emmc/sd card in
regulator-on-in-suspend in suspend mode.

> > +                               };
> >                         };
> >
> >                         ldo22_reg: LDO22 {
> > @@ -411,6 +459,9 @@
> >                                 regulator-max-microvolt = <1800000>;
> >                                 regulator-always-on;
> >                                 regulator-boot-on;
> > +                               regulator-state-mem {
> > +                                       regulator-off-in-suspend;
> > +                               };
> >                         };
> >
> >                         buck1_reg: BUCK1 {
> > @@ -419,6 +470,9 @@
> >                                 regulator-max-microvolt = <1100000>;
> >                                 regulator-always-on;
> >                                 regulator-boot-on;
> > +                               regulator-state-mem {
> > +                                       regulator-off-in-suspend;
> > +                               };
>
> This is seriously wrong so I have doubts whether you tested the
> changes or you know what is happening with them. If you turn off
> memory bus regulator off, how the memory will work in
> Suspend-to-Memory mode?
>

Well I did not find any issue in my testing but I might be wrong.
Ok I will drop this changes in the next version of the patch where
regulator-always-on is set.

> >                         };
> >
> >                         buck2_reg: BUCK2 {
> > @@ -427,6 +481,9 @@
> >                                 regulator-max-microvolt = <1350000>;
> >                                 regulator-always-on;
> >                                 regulator-boot-on;
> > +                               regulator-state-mem {
> > +                                       regulator-on-in-suspend;
> > +                               };
> >                         };
> >
> >                         buck3_reg: BUCK3 {
> > @@ -435,6 +492,9 @@
> >                                 regulator-max-microvolt = <1050000>;
> >                                 regulator-always-on;
> >                                 regulator-boot-on;
> > +                               regulator-state-mem {
> > +                                       regulator-off-in-suspend;
>
> Looks suspicious as well.

Ok I will drop this changes in the next version of the patch where
regulator-always-on is set.

>
> Best regards,
> Krzysztof
>

Well I have tested this patch as following
with only one issue, before enable suspend number of On-line cpu is 4
after resume number of On-line cpu is 1.

# lscpu
Architecture:        armv7l
Byte Order:          Little Endian
CPU(s):              4
On-line CPU(s) list: 0-3
Thread(s) per core:  1
Core(s) per socket:  4
Socket(s):           1
Vendor ID:           ARM
Model:               0
Model name:          Cortex-A9
Stepping:            r3p0
CPU max MHz:         1704.0000
CPU min MHz:         200.0000
BogoMIPS:            48.00
Flags:               half thumb fastmult vfp edsp neon vfpv3 tls vfpd32
#
# echo 1 > /sys/power/pm_debug_messages
# echo +30 > /sys/class/rtc/rtc0/wakealarm
#  echo -n mem > /sys/power/state
[   86.584147] PM: suspend entry (deep)
[   86.584684] PM: Syncing filesystems ... done.
[   86.735819] Freezing user space processes ... (elapsed 0.003 seconds) done.
[   86.742319] OOM killer disabled.
[   86.744018] Freezing remaining freezable tasks ... (elapsed 0.002
seconds) done.
[   86.751479] printk: Suspending console(s) (use no_console_suspend to debug)
[   86.792770] smsc95xx 1-2:1.0 eth0: entering SUSPEND2 mode
[   86.821751] dwc2 12480000.hsotg: suspending usb gadget g_ether
[   86.822560] dwc2 12480000.hsotg: new device is full-speed
[   86.822666] dwc2 12480000.hsotg: dwc2_hsotg_ep_disable: called for ep0
[   86.822682] dwc2 12480000.hsotg: dwc2_hsotg_ep_disable: called for ep0
[   86.824507] wake enabled for irq 119
[   86.824592] BUCK9: No configuration
[   86.824672] BUCK8_P3V3: No configuration
[   86.824706] BUCK7_2.0V: No configuration
[   86.824738] BUCK6_1.35V: No configuration
[   86.824770] VDDQ_CKEM1_2_1.2V: No configuration
[   86.826766] LDO26: No configuration
[   86.826802] VDDQ_LCD_1.8V: No configuration
[   86.826834] LDO24: No configuration
[   86.826866] LDO23: No configuration
[   86.826898] LDO22_VDDQ_MMC4_2.8V: No configuration
[   86.826930] TFLASH_2.8V: No configuration
[   86.826962] LDO20_1.8V: No configuration
[   86.826994] LDO19: No configuration
[   86.827026] LDO18: No configuration
[   86.827057] LDO17: No configuration
[   86.827516] VDDQ_C2C_W_1.8V: No configuration
[   86.828324] VDDQ_MIPIHSI_1.8V: No configuration
[   86.828359] LDO9: No configuration
[   86.828818] VDDQ_MMC1_3_1.8V: No configuration
[   86.828851] VDDQ_MMC2_2.8V: No configuration
[   86.828884] VDDQ_EXT_1.8V: No configuration
[   86.828935] VDD_ALIVE_1.0V: No configuration
[   86.839760] usb3503 0-0008: switched to STANDBY mode
[   86.840336] wake enabled for irq 123
[   86.855834] samsung-pinctrl 11000000.pinctrl: Setting external
wakeup interrupt mask: 0xfbfff7ff
[   86.871661] Disabling non-boot CPUs ...
[   87.016430] Enabling non-boot CPUs ...
[   88.009557] CPU1: failed to boot: -110
[   88.010324] Error taking CPU1 up: -110
[   89.009841] CPU2: failed to boot: -110
[   89.011290] Error taking CPU2 up: -110
[   90.009615] CPU3: failed to boot: -110
[   90.010258] Error taking CPU3 up: -110
[   90.012152] s3c-i2c 13860000.i2c: slave address 0x00
[   90.012174] s3c-i2c 13860000.i2c: bus frequency set to 390 KHz
[   90.012208] s3c-i2c 13870000.i2c: slave address 0x00
[   90.012225] s3c-i2c 13870000.i2c: bus frequency set to 97 KHz
[   90.012257] s3c-i2c 13880000.i2c: slave address 0x00
[   90.012274] s3c-i2c 13880000.i2c: bus frequency set to 97 KHz
[   90.012306] s3c-i2c 138e0000.i2c: slave address 0x00
[   90.012323] s3c-i2c 138e0000.i2c: bus frequency set to 97 KHz
[   90.028694] s3c-rtc 10070000.rtc: rtc disabled, re-enabling
[   90.029174] usb usb1: root hub lost power or was reset
[   90.032911] s3c2410-wdt 10060000.watchdog: watchdog disabled
[   90.033064] wake disabled for irq 123
[   90.041886] usb3503 0-0008: switched to HUB mode
[   90.127583] wake disabled for irq 119
[   90.127865] dwc2 12480000.hsotg: resuming usb gadget g_ether
[   90.429505] usb 1-2: reset high-speed USB device number 2 using exynos-ehci
[   90.781458] usb 1-3: reset high-speed USB device number 3 using exynos-ehci
[   91.366725] OOM killer enabled.
[   91.369869] Restarting tasks ... done.
[   91.419515] PM: suspend exit
# [   92.229649] smsc95xx 1-2:1.0 eth0: link up, 100Mbps, full-duplex,
lpa 0xC5E1
# lscpu
Architecture:         armv7l
Byte Order:           Little Endian
CPU(s):               4
On-line CPU(s) list:  0
Off-line CPU(s) list: 1-3
Thread(s) per core:   1
Core(s) per socket:   1
Socket(s):            1
Vendor ID:            ARM
Model:                0
Model name:           Cortex-A9
Stepping:             r3p0
CPU max MHz:          1704.0000
CPU min MHz:          200.0000
BogoMIPS:             24.00
Flags:                half thumb fastmult vfp edsp neon vfpv3 tls vfpd32

Best Regards
-Anand
Krzysztof Kozlowski Dec. 5, 2018, 4:19 p.m. UTC | #3
On Wed, 5 Dec 2018 at 17:11, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi Krzysztof,
>
> Thanks for your review.
> .
> On Wed, 5 Dec 2018 at 19:36, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On Tue, 4 Dec 2018 at 20:40, Anand Moon <linux.amoon@gmail.com> wrote:
> > >
> > > Add suspend-to-mem node to regulator core to be enabled or disabled
> > > during system suspend and also support changing the regulator operating
> > > mode during runtime and when the system enter sleep mode.
> > >
> > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > ---
> > > Tested on Odroid U3+
> > > ---
> > >  .../boot/dts/exynos4412-odroid-common.dtsi    | 72 +++++++++++++++++++
> > >  1 file changed, 72 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > index 2caa3132f34e..837713a2ec3b 100644
> > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > @@ -285,6 +285,9 @@
> > >                                 regulator-min-microvolt = <1800000>;
> > >                                 regulator-max-microvolt = <1800000>;
> > >                                 regulator-always-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-on-in-suspend;
> > > +                               };
> >
> > Driver does not support suspend_enable so this will be noop. We could
> > add this for DT-correctness (and full description of HW) but then I
> > need explanation why this regulator has to stay on during suspend.
> >
>
> Well I tried to study the suspend/resume feature from
> *arch/arm/boot/dts/exynos4412-midas.dtsi*
> and them I tried to apply the same with this on Odroid-U3 boards and
> test these changes.

Midas DTSI clearly has bugs then.

>
> > >                         };
> > >
> > >                         ldo3_reg: LDO3 {
> > > @@ -292,6 +295,9 @@
> > >                                 regulator-min-microvolt = <1800000>;
> > >                                 regulator-max-microvolt = <1800000>;
> > >                                 regulator-always-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-off-in-suspend;
> > > +                               };
> >
> > The same but with suspend_disable.
> >
> > I am not saying that these are wrong but I would be happy to see
> > explanations why you chosen these particular properties.
> >
>
> Well I was not aware on the regulator-always-on cannot be set to off
> in suspend mode.
> Will drop this in the future patch where regulator-always-on; is set.
>
> > >                         };
> > >
> > >                         ldo4_reg: LDO4 {
> > > @@ -299,6 +305,9 @@
> > >                                 regulator-min-microvolt = <2800000>;
> > >                                 regulator-max-microvolt = <2800000>;
> > >                                 regulator-boot-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-on-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         ldo5_reg: LDO5 {
> > > @@ -307,6 +316,9 @@
> > >                                 regulator-max-microvolt = <1800000>;
> > >                                 regulator-always-on;
> > >                                 regulator-boot-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-on-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         ldo6_reg: LDO6 {
> > > @@ -314,6 +326,9 @@
> > >                                 regulator-min-microvolt = <1000000>;
> > >                                 regulator-max-microvolt = <1000000>;
> > >                                 regulator-always-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-on-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         ldo7_reg: LDO7 {
> > > @@ -321,18 +336,27 @@
> > >                                 regulator-min-microvolt = <1000000>;
> > >                                 regulator-max-microvolt = <1000000>;
> > >                                 regulator-always-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-on-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         ldo8_reg: LDO8 {
> > >                                 regulator-name = "VDD10_HDMI_1.0V";
> > >                                 regulator-min-microvolt = <1000000>;
> > >                                 regulator-max-microvolt = <1000000>;
> > > +                               regulator-state-mem {
> > > +                                       regulator-off-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         ldo10_reg: LDO10 {
> > >                                 regulator-name = "VDDQ_MIPIHSI_1.8V";
> > >                                 regulator-min-microvolt = <1800000>;
> > >                                 regulator-max-microvolt = <1800000>;
> > > +                               regulator-state-mem {
> > > +                                       regulator-off-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         ldo11_reg: LDO11 {
> > > @@ -340,6 +364,9 @@
> > >                                 regulator-min-microvolt = <1800000>;
> > >                                 regulator-max-microvolt = <1800000>;
> > >                                 regulator-always-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-off-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         ldo12_reg: LDO12 {
> > > @@ -348,6 +375,9 @@
> > >                                 regulator-max-microvolt = <3300000>;
> > >                                 regulator-always-on;
> > >                                 regulator-boot-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-off-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         ldo13_reg: LDO13 {
> > > @@ -356,6 +386,9 @@
> > >                                 regulator-max-microvolt = <1800000>;
> > >                                 regulator-always-on;
> > >                                 regulator-boot-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-off-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         ldo14_reg: LDO14 {
> > > @@ -364,6 +397,9 @@
> > >                                 regulator-max-microvolt = <1800000>;
> > >                                 regulator-always-on;
> > >                                 regulator-boot-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-off-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         ldo15_reg: LDO15 {
> > > @@ -372,6 +408,9 @@
> > >                                 regulator-max-microvolt = <1000000>;
> > >                                 regulator-always-on;
> > >                                 regulator-boot-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-off-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         ldo16_reg: LDO16 {
> > > @@ -380,6 +419,9 @@
> > >                                 regulator-max-microvolt = <1800000>;
> > >                                 regulator-always-on;
> > >                                 regulator-boot-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-on-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         ldo20_reg: LDO20 {
> > > @@ -387,6 +429,9 @@
> > >                                 regulator-min-microvolt = <1800000>;
> > >                                 regulator-max-microvolt = <1800000>;
> > >                                 regulator-boot-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-off-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         ldo21_reg: LDO21 {
> > > @@ -394,6 +439,9 @@
> > >                                 regulator-min-microvolt = <2800000>;
> > >                                 regulator-max-microvolt = <2800000>;
> > >                                 regulator-boot-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-on-in-suspend;
> >
> > That's unusual setting... enabled in suspend but not necessarily
> > during work (no always-on).
> >
>
> I kept the regulator required for emmc/sd card in
> regulator-on-in-suspend in suspend mode.
>
> > > +                               };
> > >                         };
> > >
> > >                         ldo22_reg: LDO22 {
> > > @@ -411,6 +459,9 @@
> > >                                 regulator-max-microvolt = <1800000>;
> > >                                 regulator-always-on;
> > >                                 regulator-boot-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-off-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         buck1_reg: BUCK1 {
> > > @@ -419,6 +470,9 @@
> > >                                 regulator-max-microvolt = <1100000>;
> > >                                 regulator-always-on;
> > >                                 regulator-boot-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-off-in-suspend;
> > > +                               };
> >
> > This is seriously wrong so I have doubts whether you tested the
> > changes or you know what is happening with them. If you turn off
> > memory bus regulator off, how the memory will work in
> > Suspend-to-Memory mode?
> >
>
> Well I did not find any issue in my testing but I might be wrong.
> Ok I will drop this changes in the next version of the patch where
> regulator-always-on is set.

It worked fine because regulator-off-in-suspend is noop for buck1 but
it is clearly wrong from logical point of view. Do you think that
memory should be off in Suspend to RAM?

> > >                         };
> > >
> > >                         buck2_reg: BUCK2 {
> > > @@ -427,6 +481,9 @@
> > >                                 regulator-max-microvolt = <1350000>;
> > >                                 regulator-always-on;
> > >                                 regulator-boot-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-on-in-suspend;
> > > +                               };
> > >                         };
> > >
> > >                         buck3_reg: BUCK3 {
> > > @@ -435,6 +492,9 @@
> > >                                 regulator-max-microvolt = <1050000>;
> > >                                 regulator-always-on;
> > >                                 regulator-boot-on;
> > > +                               regulator-state-mem {
> > > +                                       regulator-off-in-suspend;
> >
> > Looks suspicious as well.
>
> Ok I will drop this changes in the next version of the patch where
> regulator-always-on is set.
>
> >
> > Best regards,
> > Krzysztof
> >
>
> Well I have tested this patch as following
> with only one issue, before enable suspend number of On-line cpu is 4
> after resume number of On-line cpu is 1.

If before your change number of resumed CPUs was 4, then you
apparently break some things.

Best regards,
Krzysztof
Anand Moon Dec. 6, 2018, 3:12 a.m. UTC | #4
Hi Krzysztof,

On Wed, 5 Dec 2018 at 21:49, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Wed, 5 Dec 2018 at 17:11, Anand Moon <linux.amoon@gmail.com> wrote:
> >
> > Hi Krzysztof,
> >
> > Thanks for your review.
> > .
> > On Wed, 5 Dec 2018 at 19:36, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > On Tue, 4 Dec 2018 at 20:40, Anand Moon <linux.amoon@gmail.com> wrote:
> > > >
> > > > Add suspend-to-mem node to regulator core to be enabled or disabled
> > > > during system suspend and also support changing the regulator operating
> > > > mode during runtime and when the system enter sleep mode.
> > > >
> > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > > ---
> > > > Tested on Odroid U3+
> > > > ---
> > > >  .../boot/dts/exynos4412-odroid-common.dtsi    | 72 +++++++++++++++++++
> > > >  1 file changed, 72 insertions(+)
> > > >
> > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > index 2caa3132f34e..837713a2ec3b 100644
> > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > @@ -285,6 +285,9 @@
> > > >                                 regulator-min-microvolt = <1800000>;
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > >
> > > Driver does not support suspend_enable so this will be noop. We could
> > > add this for DT-correctness (and full description of HW) but then I
> > > need explanation why this regulator has to stay on during suspend.
> > >
> >
> > Well I tried to study the suspend/resume feature from
> > *arch/arm/boot/dts/exynos4412-midas.dtsi*
> > and them I tried to apply the same with this on Odroid-U3 boards and
> > test these changes.
>
> Midas DTSI clearly has bugs then.
>
> >
> > > >                         };
> > > >
> > > >                         ldo3_reg: LDO3 {
> > > > @@ -292,6 +295,9 @@
> > > >                                 regulator-min-microvolt = <1800000>;
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > >
> > > The same but with suspend_disable.
> > >
> > > I am not saying that these are wrong but I would be happy to see
> > > explanations why you chosen these particular properties.
> > >
> >
> > Well I was not aware on the regulator-always-on cannot be set to off
> > in suspend mode.
> > Will drop this in the future patch where regulator-always-on; is set.
> >
> > > >                         };
> > > >
> > > >                         ldo4_reg: LDO4 {
> > > > @@ -299,6 +305,9 @@
> > > >                                 regulator-min-microvolt = <2800000>;
> > > >                                 regulator-max-microvolt = <2800000>;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo5_reg: LDO5 {
> > > > @@ -307,6 +316,9 @@
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo6_reg: LDO6 {
> > > > @@ -314,6 +326,9 @@
> > > >                                 regulator-min-microvolt = <1000000>;
> > > >                                 regulator-max-microvolt = <1000000>;
> > > >                                 regulator-always-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo7_reg: LDO7 {
> > > > @@ -321,18 +336,27 @@
> > > >                                 regulator-min-microvolt = <1000000>;
> > > >                                 regulator-max-microvolt = <1000000>;
> > > >                                 regulator-always-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo8_reg: LDO8 {
> > > >                                 regulator-name = "VDD10_HDMI_1.0V";
> > > >                                 regulator-min-microvolt = <1000000>;
> > > >                                 regulator-max-microvolt = <1000000>;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo10_reg: LDO10 {
> > > >                                 regulator-name = "VDDQ_MIPIHSI_1.8V";
> > > >                                 regulator-min-microvolt = <1800000>;
> > > >                                 regulator-max-microvolt = <1800000>;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo11_reg: LDO11 {
> > > > @@ -340,6 +364,9 @@
> > > >                                 regulator-min-microvolt = <1800000>;
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo12_reg: LDO12 {
> > > > @@ -348,6 +375,9 @@
> > > >                                 regulator-max-microvolt = <3300000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo13_reg: LDO13 {
> > > > @@ -356,6 +386,9 @@
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo14_reg: LDO14 {
> > > > @@ -364,6 +397,9 @@
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo15_reg: LDO15 {
> > > > @@ -372,6 +408,9 @@
> > > >                                 regulator-max-microvolt = <1000000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo16_reg: LDO16 {
> > > > @@ -380,6 +419,9 @@
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo20_reg: LDO20 {
> > > > @@ -387,6 +429,9 @@
> > > >                                 regulator-min-microvolt = <1800000>;
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo21_reg: LDO21 {
> > > > @@ -394,6 +439,9 @@
> > > >                                 regulator-min-microvolt = <2800000>;
> > > >                                 regulator-max-microvolt = <2800000>;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > >
> > > That's unusual setting... enabled in suspend but not necessarily
> > > during work (no always-on).
> > >
> >
> > I kept the regulator required for emmc/sd card in
> > regulator-on-in-suspend in suspend mode.
> >
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo22_reg: LDO22 {
> > > > @@ -411,6 +459,9 @@
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         buck1_reg: BUCK1 {
> > > > @@ -419,6 +470,9 @@
> > > >                                 regulator-max-microvolt = <1100000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > >
> > > This is seriously wrong so I have doubts whether you tested the
> > > changes or you know what is happening with them. If you turn off
> > > memory bus regulator off, how the memory will work in
> > > Suspend-to-Memory mode?
> > >
> >
> > Well I did not find any issue in my testing but I might be wrong.
> > Ok I will drop this changes in the next version of the patch where
> > regulator-always-on is set.
>
> It worked fine because regulator-off-in-suspend is noop for buck1 but
> it is clearly wrong from logical point of view. Do you think that
> memory should be off in Suspend to RAM?
>
My bad setting this for buck setting I will drop this in the future patch.
> > > >                         };
> > > >
> > > >                         buck2_reg: BUCK2 {
> > > > @@ -427,6 +481,9 @@
> > > >                                 regulator-max-microvolt = <1350000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         buck3_reg: BUCK3 {
> > > > @@ -435,6 +492,9 @@
> > > >                                 regulator-max-microvolt = <1050000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > >
> > > Looks suspicious as well.
> >
> > Ok I will drop this changes in the next version of the patch where
> > regulator-always-on is set.
> >
> > >
> > > Best regards,
> > > Krzysztof
> > >
> >
> > Well I have tested this patch as following
> > with only one issue, before enable suspend number of On-line cpu is 4
> > after resume number of On-line cpu is 1.
>
> If before your change number of resumed CPUs was 4, then you
> apparently break some things.
>
> Best regards,
> Krzysztof

Ok I will double check that I do not break any feature.

Best Regards
-Anand
Anand Moon Dec. 6, 2018, 7:52 a.m. UTC | #5
Hi Krzysztof,

On Wed, 5 Dec 2018 at 21:49, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Wed, 5 Dec 2018 at 17:11, Anand Moon <linux.amoon@gmail.com> wrote:
> >
> > Hi Krzysztof,
> >
> > Thanks for your review.
> > .
> > On Wed, 5 Dec 2018 at 19:36, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > On Tue, 4 Dec 2018 at 20:40, Anand Moon <linux.amoon@gmail.com> wrote:
> > > >
> > > > Add suspend-to-mem node to regulator core to be enabled or disabled
> > > > during system suspend and also support changing the regulator operating
> > > > mode during runtime and when the system enter sleep mode.
> > > >
> > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > > ---
> > > > Tested on Odroid U3+
> > > > ---
> > > >  .../boot/dts/exynos4412-odroid-common.dtsi    | 72 +++++++++++++++++++
> > > >  1 file changed, 72 insertions(+)
> > > >
> > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > index 2caa3132f34e..837713a2ec3b 100644
> > > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > @@ -285,6 +285,9 @@
> > > >                                 regulator-min-microvolt = <1800000>;
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > >
> > > Driver does not support suspend_enable so this will be noop. We could
> > > add this for DT-correctness (and full description of HW) but then I
> > > need explanation why this regulator has to stay on during suspend.
> > >
> >
> > Well I tried to study the suspend/resume feature from
> > *arch/arm/boot/dts/exynos4412-midas.dtsi*
> > and them I tried to apply the same with this on Odroid-U3 boards and
> > test these changes.
>
> Midas DTSI clearly has bugs then.
>
> >
> > > >                         };
> > > >
> > > >                         ldo3_reg: LDO3 {
> > > > @@ -292,6 +295,9 @@
> > > >                                 regulator-min-microvolt = <1800000>;
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > >
> > > The same but with suspend_disable.
> > >
> > > I am not saying that these are wrong but I would be happy to see
> > > explanations why you chosen these particular properties.
> > >
> >
> > Well I was not aware on the regulator-always-on cannot be set to off
> > in suspend mode.
> > Will drop this in the future patch where regulator-always-on; is set.
> >
> > > >                         };
> > > >
> > > >                         ldo4_reg: LDO4 {
> > > > @@ -299,6 +305,9 @@
> > > >                                 regulator-min-microvolt = <2800000>;
> > > >                                 regulator-max-microvolt = <2800000>;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo5_reg: LDO5 {
> > > > @@ -307,6 +316,9 @@
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo6_reg: LDO6 {
> > > > @@ -314,6 +326,9 @@
> > > >                                 regulator-min-microvolt = <1000000>;
> > > >                                 regulator-max-microvolt = <1000000>;
> > > >                                 regulator-always-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo7_reg: LDO7 {
> > > > @@ -321,18 +336,27 @@
> > > >                                 regulator-min-microvolt = <1000000>;
> > > >                                 regulator-max-microvolt = <1000000>;
> > > >                                 regulator-always-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo8_reg: LDO8 {
> > > >                                 regulator-name = "VDD10_HDMI_1.0V";
> > > >                                 regulator-min-microvolt = <1000000>;
> > > >                                 regulator-max-microvolt = <1000000>;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo10_reg: LDO10 {
> > > >                                 regulator-name = "VDDQ_MIPIHSI_1.8V";
> > > >                                 regulator-min-microvolt = <1800000>;
> > > >                                 regulator-max-microvolt = <1800000>;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo11_reg: LDO11 {
> > > > @@ -340,6 +364,9 @@
> > > >                                 regulator-min-microvolt = <1800000>;
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo12_reg: LDO12 {
> > > > @@ -348,6 +375,9 @@
> > > >                                 regulator-max-microvolt = <3300000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo13_reg: LDO13 {
> > > > @@ -356,6 +386,9 @@
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo14_reg: LDO14 {
> > > > @@ -364,6 +397,9 @@
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo15_reg: LDO15 {
> > > > @@ -372,6 +408,9 @@
> > > >                                 regulator-max-microvolt = <1000000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo16_reg: LDO16 {
> > > > @@ -380,6 +419,9 @@
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo20_reg: LDO20 {
> > > > @@ -387,6 +429,9 @@
> > > >                                 regulator-min-microvolt = <1800000>;
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo21_reg: LDO21 {
> > > > @@ -394,6 +439,9 @@
> > > >                                 regulator-min-microvolt = <2800000>;
> > > >                                 regulator-max-microvolt = <2800000>;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > >
> > > That's unusual setting... enabled in suspend but not necessarily
> > > during work (no always-on).
> > >
> >
> > I kept the regulator required for emmc/sd card in
> > regulator-on-in-suspend in suspend mode.
> >
> > > > +                               };
> > > >                         };
> > > >
> > > >                         ldo22_reg: LDO22 {
> > > > @@ -411,6 +459,9 @@
> > > >                                 regulator-max-microvolt = <1800000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         buck1_reg: BUCK1 {
> > > > @@ -419,6 +470,9 @@
> > > >                                 regulator-max-microvolt = <1100000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > > > +                               };
> > >
> > > This is seriously wrong so I have doubts whether you tested the
> > > changes or you know what is happening with them. If you turn off
> > > memory bus regulator off, how the memory will work in
> > > Suspend-to-Memory mode?
> > >
> >
> > Well I did not find any issue in my testing but I might be wrong.
> > Ok I will drop this changes in the next version of the patch where
> > regulator-always-on is set.
>
> It worked fine because regulator-off-in-suspend is noop for buck1 but
> it is clearly wrong from logical point of view. Do you think that
> memory should be off in Suspend to RAM?
>
> > > >                         };
> > > >
> > > >                         buck2_reg: BUCK2 {
> > > > @@ -427,6 +481,9 @@
> > > >                                 regulator-max-microvolt = <1350000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-on-in-suspend;
> > > > +                               };
> > > >                         };
> > > >
> > > >                         buck3_reg: BUCK3 {
> > > > @@ -435,6 +492,9 @@
> > > >                                 regulator-max-microvolt = <1050000>;
> > > >                                 regulator-always-on;
> > > >                                 regulator-boot-on;
> > > > +                               regulator-state-mem {
> > > > +                                       regulator-off-in-suspend;
> > >
> > > Looks suspicious as well.
> >
> > Ok I will drop this changes in the next version of the patch where
> > regulator-always-on is set.
> >
> > >
> > > Best regards,
> > > Krzysztof
> > >
> >
> > Well I have tested this patch as following
> > with only one issue, before enable suspend number of On-line cpu is 4
> > after resume number of On-line cpu is 1.
>
> If before your change number of resumed CPUs was 4, then you
> apparently break some things.
>

No bring of secondary cpu's is broken. I have check this without my dts changes.

[root@archlinux-u3 alarm]# echo 1 > /sys/power/pm_debug_messages
[root@archlinux-u3 alarm]# echo +30 > /sys/class/rtc/rtc0/wakealarm
[root@archlinux-u3 alarm]# echo -n mem > /sys/power/state
[   86.594308] PM: suspend entry (deep)
[   86.594762] PM: Syncing filesystems ... done.
[   86.899480] Freezing user space processes ... (elapsed 0.009 seconds) done.
[   86.911830] OOM killer disabled.
[   86.914121] Freezing remaining freezable tasks ... (elapsed 0.002
seconds) done.
[   86.921412] printk: Suspending console(s) (use no_console_suspend to debug)
[   86.971761] smsc95xx 1-2:1.0 eth0: entering SUSPEND2 mode
[   87.003552] dwc2 12480000.hsotg: suspending usb gadget g_ether
[   87.004504] dwc2 12480000.hsotg: dwc2_hsotg_ep_disable: called for ep0
[   87.004521] dwc2 12480000.hsotg: dwc2_hsotg_ep_disable: called for ep0
[   87.004594] dwc2 12480000.hsotg: new device is full-speed
[   87.006402] wake enabled for irq 119
[   87.006503] BUCK9: No configuration
[   87.006592] BUCK8_P3V3: No configuration
[   87.006631] BUCK7_2.0V: No configuration
[   87.006667] BUCK6_1.35V: No configuration
[   87.006703] VDDQ_CKEM1_2_1.2V: No configuration
[   87.006830] LDO26: No configuration
[   87.006868] VDDQ_LCD_1.8V: No configuration
[   87.006905] LDO24: No configuration
[   87.006939] LDO23: No configuration
[   87.006977] LDO22_VDDQ_MMC4_2.8V: No configuration
[   87.007013] TFLASH_2.8V: No configuration
[   87.007048] LDO20_1.8V: No configuration
[   87.007083] LDO19: No configuration
[   87.007119] LDO18: No configuration
[   87.007154] LDO17: No configuration
[   87.007190] VDD18_HSIC_1.8V: No configuration
[   87.007226] VDD10_HSIC_1.0V: No configuration
[   87.007262] VDD18_ABB0_2_1.8V: No configuration
[   87.007298] VDDQ_C2C_W_1.8V: No configuration
[   87.007333] VDD33_USB_3.3V: No configuration
[   87.007369] VDD18_ABB1_1.8V: No configuration
[   87.007405] VDDQ_MIPIHSI_1.8V: No configuration
[   87.007441] LDO9: No configuration
[   87.007477] VDD10_HDMI_1.0V: No configuration
[   87.007512] VDD10_XPLL_1.0V: No configuration
[   87.007548] VDD10_MPLL_1.0V: No configuration
[   87.007583] VDDQ_MMC1_3_1.8V: No configuration
[   87.007619] VDDQ_MMC2_2.8V: No configuration
[   87.007655] VDDQ_EXT_1.8V: No configuration
[   87.007691] VDDQ_M1_2_1.8V: No configuration
[   87.007727] VDD_ALIVE_1.0V: No configuration
[   87.014494] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[   87.024071] usb3503 0-0008: switched to STANDBY mode
[   87.024739] wake enabled for irq 123
[   87.066437] samsung-pinctrl 11000000.pinctrl: Setting external
wakeup interrupt mask: 0xfbfff7ff
[   87.086663] Disabling non-boot CPUs ...
[   87.230744] Enabling non-boot CPUs ...
[   88.229226] CPU1: failed to boot: -110
[   88.231113] Error taking CPU1 up: -110
[   88.234223] ------------[ cut here ]------------
[   88.234267] WARNING: CPU: 2 PID: 0 at
arch/arm/include/asm/proc-fns.h:126 secondary_start_kernel+0x214/0x270
[   88.234273] Modules linked in:
[   89.229219] CPU2: failed to boot: -110
[   89.230012] Error taking CPU2 up: -110
[   90.229071] CPU3: failed to boot: -110
[   90.229823] Error taking CPU3 up: -110
[   90.234887] s3c-i2c 13860000.i2c: slave address 0x00
[   90.234917] s3c-i2c 13860000.i2c: bus frequency set to 390 KHz
[   90.234955] s3c-i2c 13870000.i2c: slave address 0x00
[   90.234975] s3c-i2c 13870000.i2c: bus frequency set to 97 KHz
[   90.235012] s3c-i2c 13880000.i2c: slave address 0x00
[   90.235031] s3c-i2c 13880000.i2c: bus frequency set to 97 KHz
[   90.235067] s3c-i2c 138e0000.i2c: slave address 0x00
[   90.235086] s3c-i2c 138e0000.i2c: bus frequency set to 97 KHz
[   90.255430] s3c-rtc 10070000.rtc: rtc disabled, re-enabling
[   90.255985] usb usb1: root hub lost power or was reset
[   90.261948] s3c2410-wdt 10060000.watchdog: watchdog disabled
[   90.262139] wake disabled for irq 123
[   90.273868] usb3503 0-0008: switched to HUB mode
[   90.364530] wake disabled for irq 119
[   90.364688] dwc2 12480000.hsotg: resuming usb gadget g_ether
[   90.649045] usb 1-2: reset high-speed USB device number 2 using exynos-ehci
[   91.001232] usb 1-3: reset high-speed USB device number 3 using exynos-ehci
[   91.539004] usb 1-3.3: reset high-speed USB device number 4 using exynos-ehci
[   92.027260] OOM killer enabled.
[   92.030413] Restarting tasks ... done.
[   92.078154] PM: suspend exit
[root@archlinux-u3 alarm]# [   92.451624] smsc95xx 1-2:1.0 eth0: link
up, 100Mbps, full-duplex, lpa 0xC5E1

Here is the summary of the regulator to be turned off during suspend
regulator-off-in-suspend
LDO1, LDO2, LDO3, LDO6, LDO7, LDO8, LDO10, LDO11, LDO12, LDO13, LDO14,
LDO15, LDO16, LDO20, LDO21, LDO22, LDO25,BUCK4, BUCK5, BUCK6, BUCK7,
regulator-on-in-suspend
LDO4, LDO5, LD021, BUCK1, BUCK2, BUCK3.

Please let me know if this would be fine with you, so that I could
send patch v2.

Best Regards
-Anand
Marek Szyprowski Dec. 6, 2018, 8:25 a.m. UTC | #6
Hi All,

On 2018-12-05 17:11, Anand Moon wrote:
> Hi Krzysztof,
>
> Thanks for your review.
> .
> On Wed, 5 Dec 2018 at 19:36, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> On Tue, 4 Dec 2018 at 20:40, Anand Moon <linux.amoon@gmail.com> wrote:
>>> Add suspend-to-mem node to regulator core to be enabled or disabled
>>> during system suspend and also support changing the regulator operating
>>> mode during runtime and when the system enter sleep mode.
>>>
>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>> ---
>>> Tested on Odroid U3+


>>> ...


> Well I have tested this patch as following
> with only one issue, before enable suspend number of On-line cpu is 4
> after resume number of On-line cpu is 1.

This seems to be a regression in v4.20-rc1, not related to dts changes
at all. I'm investigating this now...

> ...

Best regards
Marek Szyprowski Dec. 6, 2018, 9:18 a.m. UTC | #7
Hi All,

On 2018-12-06 09:25, Marek Szyprowski wrote:
> On 2018-12-05 17:11, Anand Moon wrote:
>> On Wed, 5 Dec 2018 at 19:36, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>> On Tue, 4 Dec 2018 at 20:40, Anand Moon <linux.amoon@gmail.com> wrote:
>>>> Add suspend-to-mem node to regulator core to be enabled or disabled
>>>> during system suspend and also support changing the regulator operating
>>>> mode during runtime and when the system enter sleep mode.
>>>>
>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>> ---
>>>> Tested on Odroid U3+
>>>>
>>>> ...
>> Well I have tested this patch as following
>> with only one issue, before enable suspend number of On-line cpu is 4
>> after resume number of On-line cpu is 1.
> This seems to be a regression in v4.20-rc1, not related to dts changes
> at all. I'm investigating this now...


Okay, I mixed kernel versions a bit. To be precise, this regression is
between v4.20-rc2 and v4.20-rc3. Bisecting pointed commit 383fb3ee8024
("ARM: spectre-v2: per-CPU vtables to work around big.Little systems").
I will post a bug report about the regression.


Best regards

Patch
diff mbox series

diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
index 2caa3132f34e..837713a2ec3b 100644
--- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
+++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
@@ -285,6 +285,9 @@ 
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
 			};
 
 			ldo3_reg: LDO3 {
@@ -292,6 +295,9 @@ 
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			ldo4_reg: LDO4 {
@@ -299,6 +305,9 @@ 
 				regulator-min-microvolt = <2800000>;
 				regulator-max-microvolt = <2800000>;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
 			};
 
 			ldo5_reg: LDO5 {
@@ -307,6 +316,9 @@ 
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
 			};
 
 			ldo6_reg: LDO6 {
@@ -314,6 +326,9 @@ 
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <1000000>;
 				regulator-always-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
 			};
 
 			ldo7_reg: LDO7 {
@@ -321,18 +336,27 @@ 
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <1000000>;
 				regulator-always-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
 			};
 
 			ldo8_reg: LDO8 {
 				regulator-name = "VDD10_HDMI_1.0V";
 				regulator-min-microvolt = <1000000>;
 				regulator-max-microvolt = <1000000>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			ldo10_reg: LDO10 {
 				regulator-name = "VDDQ_MIPIHSI_1.8V";
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			ldo11_reg: LDO11 {
@@ -340,6 +364,9 @@ 
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			ldo12_reg: LDO12 {
@@ -348,6 +375,9 @@ 
 				regulator-max-microvolt = <3300000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			ldo13_reg: LDO13 {
@@ -356,6 +386,9 @@ 
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			ldo14_reg: LDO14 {
@@ -364,6 +397,9 @@ 
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			ldo15_reg: LDO15 {
@@ -372,6 +408,9 @@ 
 				regulator-max-microvolt = <1000000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			ldo16_reg: LDO16 {
@@ -380,6 +419,9 @@ 
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
 			};
 
 			ldo20_reg: LDO20 {
@@ -387,6 +429,9 @@ 
 				regulator-min-microvolt = <1800000>;
 				regulator-max-microvolt = <1800000>;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			ldo21_reg: LDO21 {
@@ -394,6 +439,9 @@ 
 				regulator-min-microvolt = <2800000>;
 				regulator-max-microvolt = <2800000>;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
 			};
 
 			ldo22_reg: LDO22 {
@@ -411,6 +459,9 @@ 
 				regulator-max-microvolt = <1800000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			buck1_reg: BUCK1 {
@@ -419,6 +470,9 @@ 
 				regulator-max-microvolt = <1100000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			buck2_reg: BUCK2 {
@@ -427,6 +481,9 @@ 
 				regulator-max-microvolt = <1350000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-on-in-suspend;
+				};
 			};
 
 			buck3_reg: BUCK3 {
@@ -435,6 +492,9 @@ 
 				regulator-max-microvolt = <1050000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			buck4_reg: BUCK4 {
@@ -442,6 +502,9 @@ 
 				regulator-min-microvolt = <900000>;
 				regulator-max-microvolt = <1100000>;
 				regulator-microvolt-offset = <50000>;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			buck5_reg: BUCK5 {
@@ -450,6 +513,9 @@ 
 				regulator-max-microvolt = <1200000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			buck6_reg: BUCK6 {
@@ -458,6 +524,9 @@ 
 				regulator-max-microvolt = <1350000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			buck7_reg: BUCK7 {
@@ -465,6 +534,9 @@ 
 				regulator-min-microvolt = <2000000>;
 				regulator-max-microvolt = <2000000>;
 				regulator-always-on;
+				regulator-state-mem {
+					regulator-off-in-suspend;
+				};
 			};
 
 			buck8_reg: BUCK8 {