* [PATCH v1] ARM: dts: exynos: Add proper regulator states for suspend-to-mem for odroid-u3
@ 2019-03-24 8:32 Anand Moon
2019-03-25 12:46 ` Krzysztof Kozlowski
0 siblings, 1 reply; 6+ messages in thread
From: Anand Moon @ 2019-03-24 8:32 UTC (permalink / raw)
To: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel
Cc: Rob Herring, Kukjin Kim, Krzysztof Kozlowski, Marek Szyprowski,
Chanwoo Choi
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 (stand by mode).
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
Current patch:
Note: Both microSD and eMMC suspend resume works this changes at my end.
regulator-off-in-suspend:
set the regulator node into suspend state i.e. standby mode during suspend
operation.
Current changes are based on
[0] https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/max77686.txt
Regulators which can be turned off during system suspend:
-LDOn : 2, 6-8, 10-12, 14-16,
-BUCKn : 1-4.
Use standard regulator bindings for it ('regulator-off-in-suspend').
drop the suspend off binding which are not supported by the driver.
RFC version
[1] https://patchwork.kernel.org/patch/10810909/
These changes had some problem with eMMC not entering into suspend mode.
with some miss configuration in regulator-off-in-suspend mode.
Changes from previos patch.
[2] https://patchwork.kernel.org/patch/10712549/
Set all the non used regulator in suspend-odd state
LD02, LD03, LD05, LD06, LD07, LD011, LD013, LDO14, LD016
BUCK5, BUCK6, BUCK7 and not confirable as per driver max77686-regulator
---
.../boot/dts/exynos4412-odroid-common.dtsi | 39 +++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
index 08d3a0a7b4eb..375156ad5454 100644
--- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
+++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
@@ -288,6 +288,9 @@
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <1800000>;
regulator-always-on;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
};
ldo3_reg: LDO3 {
@@ -317,6 +320,9 @@
regulator-min-microvolt = <1000000>;
regulator-max-microvolt = <1000000>;
regulator-always-on;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
};
ldo7_reg: LDO7 {
@@ -324,18 +330,27 @@
regulator-min-microvolt = <1000000>;
regulator-max-microvolt = <1000000>;
regulator-always-on;
+ regulator-state-mem {
+ regulator-off-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 {
@@ -343,6 +358,9 @@
regulator-min-microvolt = <1800000>;
regulator-max-microvolt = <1800000>;
regulator-always-on;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
};
ldo12_reg: LDO12 {
@@ -351,6 +369,9 @@
regulator-max-microvolt = <3300000>;
regulator-always-on;
regulator-boot-on;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
};
ldo13_reg: LDO13 {
@@ -367,6 +388,9 @@
regulator-max-microvolt = <1800000>;
regulator-always-on;
regulator-boot-on;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
};
ldo15_reg: LDO15 {
@@ -375,6 +399,9 @@
regulator-max-microvolt = <1000000>;
regulator-always-on;
regulator-boot-on;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
};
ldo16_reg: LDO16 {
@@ -383,6 +410,9 @@
regulator-max-microvolt = <1800000>;
regulator-always-on;
regulator-boot-on;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
};
ldo20_reg: LDO20 {
@@ -421,6 +451,9 @@
regulator-max-microvolt = <1100000>;
regulator-always-on;
regulator-boot-on;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
};
buck2_reg: BUCK2 {
@@ -437,6 +470,9 @@
regulator-max-microvolt = <1050000>;
regulator-always-on;
regulator-boot-on;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
};
buck4_reg: BUCK4 {
@@ -444,6 +480,9 @@
regulator-min-microvolt = <900000>;
regulator-max-microvolt = <1100000>;
regulator-microvolt-offset = <50000>;
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
};
buck5_reg: BUCK5 {
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v1] ARM: dts: exynos: Add proper regulator states for suspend-to-mem for odroid-u3 2019-03-24 8:32 [PATCH v1] ARM: dts: exynos: Add proper regulator states for suspend-to-mem for odroid-u3 Anand Moon @ 2019-03-25 12:46 ` Krzysztof Kozlowski 2019-03-25 12:56 ` Krzysztof Kozlowski 2019-03-26 10:35 ` Anand Moon 0 siblings, 2 replies; 6+ messages in thread From: Krzysztof Kozlowski @ 2019-03-25 12:46 UTC (permalink / raw) To: Anand Moon Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, Rob Herring, Kukjin Kim, Marek Szyprowski, Chanwoo Choi On Sun, 24 Mar 2019 at 09:33, 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 (stand by mode). > > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > Cc: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Chanwoo Choi <cw00.choi@samsung.com> > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > --- > Current patch: > > Note: Both microSD and eMMC suspend resume works this changes at my end. > > regulator-off-in-suspend: > set the regulator node into suspend state i.e. standby mode during suspend > operation. > > Current changes are based on > [0] https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/max77686.txt > > Regulators which can be turned off during system suspend: > -LDOn : 2, 6-8, 10-12, 14-16, > -BUCKn : 1-4. > Use standard regulator bindings for it ('regulator-off-in-suspend'). > > drop the suspend off binding which are not supported by the driver. > > RFC version > [1] https://patchwork.kernel.org/patch/10810909/ > These changes had some problem with eMMC not entering into suspend mode. > with some miss configuration in regulator-off-in-suspend mode. > > Changes from previos patch. > [2] https://patchwork.kernel.org/patch/10712549/ > > Set all the non used regulator in suspend-odd state > LD02, LD03, LD05, LD06, LD07, LD011, LD013, LDO14, LD016 > > BUCK5, BUCK6, BUCK7 and not confirable as per driver max77686-regulator > --- > .../boot/dts/exynos4412-odroid-common.dtsi | 39 +++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > index 08d3a0a7b4eb..375156ad5454 100644 > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > @@ -288,6 +288,9 @@ > regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <1800000>; > regulator-always-on; > + regulator-state-mem { > + regulator-off-in-suspend; Please maintain proper versioning of patches. First patch sent to lists should be either RFC or v1. Second release/submission is then always v2. Third v3. This is third or fourth submission but you marked it as v1. This makes it very difficult to discuss and reference previous versions. The commit message did not change since beginning (first version). I asked twice that you need to explain exactly why you put the the regulator to off or on state in suspend. Why? Because: 1. This change looks without justification - once you put on, then you put off, now again on, 2. Anyone reading the code later must know the rationale why this was done, 3. I am not quite sure whether this is good setting so I would be happy to be convinced. How to provide such explanation? The best in commit message. Sometimes in the comment in the code, depends. How such explanation could look like? For example like this: f8f3b7fc21b1 ("ARM: dts: exynos: Fix regulators configuration on Peach Pi/Pit Chromebooks") Marek clearly explained why he put the regulators "always-on", even tough we do not know everything about this. More over, he mentions that this fixes specific issue. Summarizing, please answer: 1. Why this is made off-in-suspend? 2. Why this can be made off-in-suspend? > + }; > }; > > ldo3_reg: LDO3 { > @@ -317,6 +320,9 @@ > regulator-min-microvolt = <1000000>; > regulator-max-microvolt = <1000000>; > regulator-always-on; > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > }; > > ldo7_reg: LDO7 { > @@ -324,18 +330,27 @@ > regulator-min-microvolt = <1000000>; > regulator-max-microvolt = <1000000>; > regulator-always-on; > + regulator-state-mem { > + regulator-off-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 { > @@ -343,6 +358,9 @@ > regulator-min-microvolt = <1800000>; > regulator-max-microvolt = <1800000>; > regulator-always-on; > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > }; > > ldo12_reg: LDO12 { > @@ -351,6 +369,9 @@ > regulator-max-microvolt = <3300000>; > regulator-always-on; > regulator-boot-on; > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > }; > > ldo13_reg: LDO13 { > @@ -367,6 +388,9 @@ > regulator-max-microvolt = <1800000>; > regulator-always-on; > regulator-boot-on; > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > }; > > ldo15_reg: LDO15 { > @@ -375,6 +399,9 @@ > regulator-max-microvolt = <1000000>; > regulator-always-on; > regulator-boot-on; > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > }; > > ldo16_reg: LDO16 { > @@ -383,6 +410,9 @@ > regulator-max-microvolt = <1800000>; > regulator-always-on; > regulator-boot-on; > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > }; > > ldo20_reg: LDO20 { > @@ -421,6 +451,9 @@ > regulator-max-microvolt = <1100000>; > regulator-always-on; > regulator-boot-on; > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > }; I questioned this change two times. You did not answer to my question... If you turn memory bus regulator off, how the memory will work in Suspend-to-Memory mode? I might be missing here something but it just looks suspicious. Maybe the regulator does not supply the memory itself (so refresh works even when it is down), just the interface? I don't know, it just looks suspicious. I need to see proper explanation. I am sorry but I will not check other hunks in this patch. Please provide the answers for all my questions here first. Best regards, Krzysztof > > buck2_reg: BUCK2 { > @@ -437,6 +470,9 @@ > regulator-max-microvolt = <1050000>; > regulator-always-on; > regulator-boot-on; > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > }; > > buck4_reg: BUCK4 { > @@ -444,6 +480,9 @@ > regulator-min-microvolt = <900000>; > regulator-max-microvolt = <1100000>; > regulator-microvolt-offset = <50000>; > + regulator-state-mem { > + regulator-off-in-suspend; > + }; > }; > > buck5_reg: BUCK5 { > -- > 2.21.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] ARM: dts: exynos: Add proper regulator states for suspend-to-mem for odroid-u3 2019-03-25 12:46 ` Krzysztof Kozlowski @ 2019-03-25 12:56 ` Krzysztof Kozlowski 2019-03-26 10:35 ` Anand Moon 1 sibling, 0 replies; 6+ messages in thread From: Krzysztof Kozlowski @ 2019-03-25 12:56 UTC (permalink / raw) To: Anand Moon Cc: devicetree, linux-arm-kernel, linux-samsung-soc, linux-kernel, Rob Herring, Kukjin Kim, Marek Szyprowski, Chanwoo Choi On Mon, 25 Mar 2019 at 13:46, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Sun, 24 Mar 2019 at 09:33, 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 (stand by mode). > > > > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > > Cc: Krzysztof Kozlowski <krzk@kernel.org> > > Cc: Chanwoo Choi <cw00.choi@samsung.com> > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > > --- > > Current patch: > > > > Note: Both microSD and eMMC suspend resume works this changes at my end. > > > > regulator-off-in-suspend: > > set the regulator node into suspend state i.e. standby mode during suspend > > operation. > > > > Current changes are based on > > [0] https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/max77686.txt > > > > Regulators which can be turned off during system suspend: > > -LDOn : 2, 6-8, 10-12, 14-16, > > -BUCKn : 1-4. > > Use standard regulator bindings for it ('regulator-off-in-suspend'). > > > > drop the suspend off binding which are not supported by the driver. > > > > RFC version > > [1] https://patchwork.kernel.org/patch/10810909/ > > These changes had some problem with eMMC not entering into suspend mode. > > with some miss configuration in regulator-off-in-suspend mode. > > > > Changes from previos patch. > > [2] https://patchwork.kernel.org/patch/10712549/ > > > > Set all the non used regulator in suspend-odd state > > LD02, LD03, LD05, LD06, LD07, LD011, LD013, LDO14, LD016 > > > > BUCK5, BUCK6, BUCK7 and not confirable as per driver max77686-regulator > > --- > > .../boot/dts/exynos4412-odroid-common.dtsi | 39 +++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > index 08d3a0a7b4eb..375156ad5454 100644 > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > @@ -288,6 +288,9 @@ > > regulator-min-microvolt = <1800000>; > > regulator-max-microvolt = <1800000>; > > regulator-always-on; > > + regulator-state-mem { > > + regulator-off-in-suspend; > > Please maintain proper versioning of patches. > First patch sent to lists should be either RFC or v1. Second > release/submission is then always v2. Third v3. > > This is third or fourth submission but you marked it as v1. This makes > it very difficult to discuss and reference previous versions. > > The commit message did not change since beginning (first version). I > asked twice that you need to explain exactly why you put the the > regulator to off or on state in suspend. Why? > Because: > 1. This change looks without justification - once you put on, then you > put off, now again on, > 2. Anyone reading the code later must know the rationale why this was done, > 3. I am not quite sure whether this is good setting so I would be > happy to be convinced. > > How to provide such explanation? The best in commit message. Sometimes > in the comment in the code, depends. > How such explanation could look like? For example like this: > f8f3b7fc21b1 ("ARM: dts: exynos: Fix regulators configuration on Peach > Pi/Pit Chromebooks") > Marek clearly explained why he put the regulators "always-on", even > tough we do not know everything about this. More over, he mentions > that this fixes specific issue. > > Summarizing, please answer: > 1. Why this is made off-in-suspend? > 2. Why this can be made off-in-suspend? > > > + }; > > }; > > > > ldo3_reg: LDO3 { > > @@ -317,6 +320,9 @@ > > regulator-min-microvolt = <1000000>; > > regulator-max-microvolt = <1000000>; > > regulator-always-on; > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + }; > > }; > > > > ldo7_reg: LDO7 { > > @@ -324,18 +330,27 @@ > > regulator-min-microvolt = <1000000>; > > regulator-max-microvolt = <1000000>; > > regulator-always-on; > > + regulator-state-mem { > > + regulator-off-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 { > > @@ -343,6 +358,9 @@ > > regulator-min-microvolt = <1800000>; > > regulator-max-microvolt = <1800000>; > > regulator-always-on; > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + }; > > }; > > > > ldo12_reg: LDO12 { > > @@ -351,6 +369,9 @@ > > regulator-max-microvolt = <3300000>; > > regulator-always-on; > > regulator-boot-on; > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + }; > > }; > > > > ldo13_reg: LDO13 { > > @@ -367,6 +388,9 @@ > > regulator-max-microvolt = <1800000>; > > regulator-always-on; > > regulator-boot-on; > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + }; > > }; > > > > ldo15_reg: LDO15 { > > @@ -375,6 +399,9 @@ > > regulator-max-microvolt = <1000000>; > > regulator-always-on; > > regulator-boot-on; > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + }; > > }; > > > > ldo16_reg: LDO16 { > > @@ -383,6 +410,9 @@ > > regulator-max-microvolt = <1800000>; > > regulator-always-on; > > regulator-boot-on; > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + }; > > }; > > > > ldo20_reg: LDO20 { > > @@ -421,6 +451,9 @@ > > regulator-max-microvolt = <1100000>; > > regulator-always-on; > > regulator-boot-on; > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + }; > > }; > > I questioned this change two times. You did not answer to my > question... If you turn memory bus regulator off, how the memory will > work in Suspend-to-Memory mode? I might be missing here something but > it just looks suspicious. Maybe the regulator does not supply the > memory itself (so refresh works even when it is down), just the > interface? I don't know, it just looks suspicious. I need to see > proper explanation. > > I am sorry but I will not check other hunks in this patch. Please > provide the answers for all my questions here first. I started digging around this and datasheet describes the regulator values in sleep mode. The MIF can be off if C2C is not used. It seems that the memory refresh is provided by CKEM regulators (which you wanted to turn off in first version of the patch). I am not happy that I ask about such information and cannot get it. It is the job of submitter to provide rationale. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] ARM: dts: exynos: Add proper regulator states for suspend-to-mem for odroid-u3 2019-03-25 12:46 ` Krzysztof Kozlowski 2019-03-25 12:56 ` Krzysztof Kozlowski @ 2019-03-26 10:35 ` Anand Moon 2019-03-26 10:58 ` Krzysztof Kozlowski 1 sibling, 1 reply; 6+ messages in thread From: Anand Moon @ 2019-03-26 10:35 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: devicetree, linux-arm-kernel, linux-samsung-soc, Linux Kernel, Rob Herring, Kukjin Kim, Marek Szyprowski, Chanwoo Choi [-- Attachment #1: Type: text/plain, Size: 12060 bytes --] Hi Krzysztof, Thanks your for your valuable comments. I will try to answer your queries to best of my knowledge. On Mon, 25 Mar 2019 at 18:16, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Sun, 24 Mar 2019 at 09:33, 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 (stand by mode). > > > > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > > Cc: Krzysztof Kozlowski <krzk@kernel.org> > > Cc: Chanwoo Choi <cw00.choi@samsung.com> > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > > --- > > Current patch: > > > > Note: Both microSD and eMMC suspend resume works this changes at my end. > > > > regulator-off-in-suspend: > > set the regulator node into suspend state i.e. standby mode during suspend > > operation. > > > > Current changes are based on > > [0] https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/max77686.txt > > > > Regulators which can be turned off during system suspend: > > -LDOn : 2, 6-8, 10-12, 14-16, > > -BUCKn : 1-4. > > Use standard regulator bindings for it ('regulator-off-in-suspend'). > > > > drop the suspend off binding which are not supported by the driver. > > > > RFC version > > [1] https://patchwork.kernel.org/patch/10810909/ > > These changes had some problem with eMMC not entering into suspend mode. > > with some miss configuration in regulator-off-in-suspend mode. > > > > Changes from previos patch. > > [2] https://patchwork.kernel.org/patch/10712549/ > > > > Set all the non used regulator in suspend-odd state > > LD02, LD03, LD05, LD06, LD07, LD011, LD013, LDO14, LD016 > > > > BUCK5, BUCK6, BUCK7 and not confirable as per driver max77686-regulator > > --- > > .../boot/dts/exynos4412-odroid-common.dtsi | 39 +++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > index 08d3a0a7b4eb..375156ad5454 100644 > > --- a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > +++ b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi > > @@ -288,6 +288,9 @@ > > regulator-min-microvolt = <1800000>; > > regulator-max-microvolt = <1800000>; > > regulator-always-on; > > + regulator-state-mem { > > + regulator-off-in-suspend; > > Please maintain proper versioning of patches. > First patch sent to lists should be either RFC or v1. Second > release/submission is then always v2. Third v3. I tried to mixed up some changes that's the reason. Ok if needed next version will be Patch V3. > > This is third or fourth submission but you marked it as v1. This makes > it very difficult to discuss and reference previous versions. > > The commit message did not change since beginning (first version). I > asked twice that you need to explain exactly why you put the the > regulator to off or on state in suspend. Why? > Because: > 1. This change looks without justification - once you put on, then you > put off, now again on, > 2. Anyone reading the code later must know the rationale why this was done, > 3. I am not quite sure whether this is good setting so I would be > happy to be convinced. > Like I mention in the patch summary that this. Current changes are based on [0] https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/max77686.txt Regulators which can be turned off during system suspend: -LDOn : 2, 6-8, 10-12, 14-16, -BUCKn : 1-4. Use standard regulator bindings for it ('regulator-off-in-suspend'). > How to provide such explanation? The best in commit message. Sometimes > in the comment in the code, depends. Ok I have been testing with following regulator debug prints to catch error. [0] max77686-regulator.patch below is the console logs during suspend and resume. ------------------------------------------------------------------------------- Last login: Sat Mar 23 18:22:46 on ttySAC1 [root@archl-u3e ~]# echo no > /sys/module/printk/parameters/console_suspend [root@archl-u3e ~]# rtcwake -d /dev/rtc0 -m mem -s 10 rtcwake: wakeup from "mem" using /dev/rtc0 at Sat Mar 23 19:56:17 2019 [ 38.595854] PM: suspend entry (deep) [ 38.596603] PM: Syncing filesystems ... done. [ 38.629351] Freezing user space processes ... (elapsed 0.002 seconds) done. [ 38.633192] OOM killer disabled. [ 38.636035] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 38.675059] smsc95xx 1-2:1.0 eth0: entering SUSPEND2 mode [ 38.753120] dwc2 12480000.hsotg: suspending usb gadget g_ether [ 38.754007] dwc2 12480000.hsotg: new device is full-speed [ 38.758960] dwc2 12480000.hsotg: dwc2_hsotg_ep_disable: called for ep0 [ 38.765507] dwc2 12480000.hsotg: dwc2_hsotg_ep_disable: called for ep0 [ 38.774050] wake enabled for irq 119 [ 38.775761] BUCK9: No configuration [ 38.779191] BUCK8_P3V3: No configuration [ 38.782852] BUCK7_2.0V: No configuration [ 38.786851] BUCK6_1.35V: No configuration [ 38.790752] VDDQ_CKEM1_2_1.2V: No configuration [ 38.796220] BUCK4: regulator suspend disable supported [ 38.800769] BUCK3: regulator suspend disable supported [ 38.806002] BUCK1: regulator suspend disable supported [ 38.810644] LDO26: No configuration [ 38.814169] VDDQ_LCD_1.8V: No configuration [ 38.818267] LDO24: No configuration [ 38.821732] LDO23: No configuration [ 38.825262] LDO22_VDDQ_MMC4_2.8V: No configuration [ 38.829992] TFLASH_2.8V: No configuration [ 38.834040] LDO20_1.8V: No configuration [ 38.837883] LDO19: No configuration [ 38.841349] LDO18: No configuration [ 38.844878] LDO17: No configuration [ 38.848667] LDO16: regulator suspend disable supported [ 38.853889] LDO15: regulator suspend disable supported [ 38.858931] LDO14: regulator suspend disable supported [ 38.863771] VDDQ_C2C_W_1.8V: No configuration [ 38.868378] LDO12: regulator suspend disable supported [ 38.873508] LDO11: regulator suspend disable supported [ 38.878545] LDO10: regulator suspend disable supported [ 38.883384] LDO9: No configuration [ 38.887190] LDO8: regulator suspend disable supported [ 38.892168] LDO7: regulator suspend disable supported [ 38.897279] LDO6: regulator suspend disable supported [ 38.901872] VDDQ_MMC1_3_1.8V: No configuration [ 38.906363] VDDQ_MMC2_2.8V: No configuration [ 38.910541] VDDQ_EXT_1.8V: No configuration [ 38.915134] LDO2: regulator suspend disable supported [ 38.919753] VDD_ALIVE_1.0V: No configuration [ 38.935229] usb3503 0-0008: switched to STANDBY mode [ 38.935981] wake enabled for irq 123 [ 38.955192] samsung-pinctrl 11000000.pinctrl: Setting external wakeup interrupt mask: 0xfbfff7ff [ 38.975448] Disabling non-boot CPUs ... [ 39.029279] s3c2410-wdt 10060000.watchdog: watchdog disabled [ 39.029576] wake disabled for irq 123 [ 39.044319] usb3503 0-0008: switched to HUB mode [ 39.144089] wake disabled for irq 119 [ 39.144812] dwc2 12480000.hsotg: resuming usb gadget g_ether [ 39.422626] usb 1-2: reset high-speed USB device number 2 using exynos-ehci [ 39.774632] usb 1-3: reset high-speed USB device number 3 using exynos-ehci [ 40.106478] OOM killer enabled. [ 40.106609] Restarting tasks ... done. [ 40.111100] PM: suspend exit [ 40.124058] mmc_host mmc1: Bus speed (slot 0) = 50000000Hz (slot req 400000Hz, actual 396825HZ div = 63) [root@archl-u3e ~]# [ 40.364705] mmc_host mmc1: Bus speed (slot 0) = 50000000Hz (slot req 52000000Hz, actual 50000000HZ div = 0) [ 41.220200] smsc95xx 1-2:1.0 eth0: link up, 100Mbps, full-duplex, lpa 0xC5E1 ------------------------------------------------------------------------------- > How such explanation could look like? For example like this: > f8f3b7fc21b1 ("ARM: dts: exynos: Fix regulators configuration on Peach > Pi/Pit Chromebooks") > Marek clearly explained why he put the regulators "always-on", even > tough we do not know everything about this. More over, he mentions > that this fixes specific issue. > Thanks but I am not the expert here in general with regulator on/off. I will add comment if needed explain in more detail. [snip] > Summarizing, please answer: > 1. Why this is made off-in-suspend? > 2. Why this can be made off-in-suspend? > On MAX77686A PMIC. (some quote from the datasheet). Power ON/OFF by PWRREQ in PMIC=ON sequence is only effective when ON/OFF on all regulators above are assigned by PWRREQ=H/L. All programming must be done before the AP enters the sleep mode by pulling PWRREQ low since the AP does not have programming capability in (deep) sleep mode. *Regulator are not really turned off but set into IDLE / Standby state too be restored to Normal state* Power summary in image : [1] https://imgur.com/gallery/l74kliX Power Summary for MAX77686A of the regulator support PWRREQ Power Default ON | ON/OFF -------------------------------------- BUCK1 | ON | PWRREQ or I2C BUCK2 | ON | PWRREQ or I2C BUCK3 | ON | PWRREQ or I2C BUCK4 | ON | PWREWQ or I2C BUCK5 | ON | I2C BUCK6 | ON | I2C BUCK7 | ON | I2C BUCK8 | OFF | I2C or ENB8 The external enable/disable pin, ENB8 BUCK9 | OFF | I2C or ENB9 The external enable/disable pin, ENB9 LDO1 | ON | I2C LDO2 | ON | PWRREQ or I2C LDO3 | ON | I2C LDO4 | ON | I2C LDO5 | ON | I2C LDO6 | ON | PWRREQ or I2C LDO7 | ON | PWRREQ or I2C LDO8 | ON | PWRREQ or I2C LDO9 | OFF | I2C LDO10 | ON | PWRREQ or I2C LDO11 | ON | PWRREQ or I2C LDO12 | ON | PWRREQ or I2C LDO13 | ON | I2C LDO14 | ON | PWRREQ or I2C LDO15 | ON | PWRREQ or I2C LDO16 | ON | PWRREQ or I2C LDO17 | OFF | I2C LDO18 | OFF | I2C LDO19 | OFF | I2C LDO20 | OFF | I2C OR ENL20 ENL20, external enable/disable pin LDO21 | OFF | I2C OR ENL21 ENL21, external enable/disable pin LDO22 | OFF | I2C OR ENL22 ENL22, external enable/disable pin LDO23 | OFF | I2C LDO24 | OFF | I2C LDO25 | OFF | I2C LDO26 | OFF | I2C > > ldo20_reg: LDO20 { > > @@ -421,6 +451,9 @@ > > regulator-max-microvolt = <1100000>; > > regulator-always-on; > > regulator-boot-on; > > + regulator-state-mem { > > + regulator-off-in-suspend; > > + }; > > }; > > I questioned this change two times. You did not answer to my > question... If you turn memory bus regulator off, how the memory will > work in Suspend-to-Memory mode? I might be missing here something but > it just looks suspicious. Maybe the regulator does not supply the > memory itself (so refresh works even when it is down), just the > interface? I don't know, it just looks suspicious. I need to see > proper explanation. > > I am sorry but I will not check other hunks in this patch. Please > provide the answers for all my questions here first. > As per above table of MAX77686 PWRREQ capabilities regulator nodes can be turned off during suspend reset of them remain on during suspend. > Best regards, > Krzysztof > We could monitor the regulator states via sys filesystem and also using below tool https://git.linaro.org/power/powerdebug.git I have tried to summaries the feature required for this patch. some of which I have overlooked earlier before sending the patch. I am really poor in english for transform / interpreter the technical details required at for your queries. But I will try to improve my self in the future. Best Regards -Anand [-- Attachment #2: max77686-regulator.patch --] [-- Type: application/octet-stream, Size: 2076 bytes --] diff --git a/drivers/regulator/max77686-regulator.c b/drivers/regulator/max77686-regulator.c index 8020eb57374a..fcd1ce23e824 100644 --- a/drivers/regulator/max77686-regulator.c +++ b/drivers/regulator/max77686-regulator.c @@ -131,6 +131,8 @@ static int max77686_set_suspend_disable(struct regulator_dev *rdev) if (ret) return ret; + pr_info("%s: regulator suspend disable supported\n", + rdev->desc->name); max77686->opmode[id] = val; return 0; } @@ -149,13 +151,17 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev, switch (mode) { case REGULATOR_MODE_IDLE: /* ON in LP Mode */ + pr_info("%s: regulator_idle_mode : 0x%x supported\n", + rdev->desc->name, mode); val = MAX77686_LDO_LOWPOWER_PWRREQ; break; case REGULATOR_MODE_NORMAL: /* ON in Normal Mode */ + pr_info("%s: regulator_normal_mode : 0x%x supported\n", + rdev->desc->name, mode); val = max77686_map_normal_mode(max77686, id); break; default: - pr_warn("%s: regulator_suspend_mode : 0x%x not supported\n", + pr_err("%s: regulator_suspend_mode : 0x%x not supported\n", rdev->desc->name, mode); return -EINVAL; } @@ -166,6 +172,8 @@ static int max77686_set_suspend_mode(struct regulator_dev *rdev, if (ret) return ret; + pr_info("%s: regulator suspend disable supported\n", + rdev->desc->name); max77686->opmode[id] = val; return 0; } @@ -180,12 +188,18 @@ static int max77686_ldo_set_suspend_mode(struct regulator_dev *rdev, switch (mode) { case REGULATOR_MODE_STANDBY: /* switch off */ + pr_info("%s: regulator_standby mode : 0x%x supported\n", + rdev->desc->name, mode); val = MAX77686_OFF_PWRREQ; break; case REGULATOR_MODE_IDLE: /* ON in LP Mode */ + pr_info("%s: regulator_idle mode : 0x%x supported\n", + rdev->desc->name, mode); val = MAX77686_LDO_LOWPOWER_PWRREQ; break; case REGULATOR_MODE_NORMAL: /* ON in Normal Mode */ + pr_info("%s: regulator_normal mode : 0x%x supported\n", + rdev->desc->name, mode); val = max77686_map_normal_mode(max77686, id); break; default: ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1] ARM: dts: exynos: Add proper regulator states for suspend-to-mem for odroid-u3 2019-03-26 10:35 ` Anand Moon @ 2019-03-26 10:58 ` Krzysztof Kozlowski 2019-04-04 4:01 ` Anand Moon 0 siblings, 1 reply; 6+ messages in thread From: Krzysztof Kozlowski @ 2019-03-26 10:58 UTC (permalink / raw) To: Anand Moon Cc: devicetree, linux-arm-kernel, linux-samsung-soc, Linux Kernel, Rob Herring, Kukjin Kim, Marek Szyprowski, Chanwoo Choi On Tue, 26 Mar 2019 at 11:35, Anand Moon <linux.amoon@gmail.com> wrote: (...) > > This is third or fourth submission but you marked it as v1. This makes > > it very difficult to discuss and reference previous versions. > > > > The commit message did not change since beginning (first version). I > > asked twice that you need to explain exactly why you put the the > > regulator to off or on state in suspend. Why? > > Because: > > 1. This change looks without justification - once you put on, then you > > put off, now again on, > > 2. Anyone reading the code later must know the rationale why this was done, > > 3. I am not quite sure whether this is good setting so I would be > > happy to be convinced. > > > > Like I mention in the patch summary that this. > > Current changes are based on > [0] https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/max77686.txt > > Regulators which can be turned off during system suspend: > -LDOn : 2, 6-8, 10-12, 14-16, > -BUCKn : 1-4. > Use standard regulator bindings for it ('regulator-off-in-suspend'). I do not see how this is related to my questions. > > How to provide such explanation? The best in commit message. Sometimes > > in the comment in the code, depends. > > Ok I have been testing with following regulator debug prints to catch error. > [0] max77686-regulator.patch > > below is the console logs during suspend and resume. > ------------------------------------------------------------------------------- > Last login: Sat Mar 23 18:22:46 on ttySAC1 > [root@archl-u3e ~]# echo no > /sys/module/printk/parameters/console_suspend > [root@archl-u3e ~]# rtcwake -d /dev/rtc0 -m mem -s 10 > rtcwake: wakeup from "mem" using /dev/rtc0 at Sat Mar 23 19:56:17 2019 > [ 38.595854] PM: suspend entry (deep) > [ 38.596603] PM: Syncing filesystems ... done. > [ 38.629351] Freezing user space processes ... (elapsed 0.002 seconds) done. > [ 38.633192] OOM killer disabled. > [ 38.636035] Freezing remaining freezable tasks ... (elapsed 0.001 > seconds) done. > [ 38.675059] smsc95xx 1-2:1.0 eth0: entering SUSPEND2 mode > [ 38.753120] dwc2 12480000.hsotg: suspending usb gadget g_ether > [ 38.754007] dwc2 12480000.hsotg: new device is full-speed > [ 38.758960] dwc2 12480000.hsotg: dwc2_hsotg_ep_disable: called for ep0 > [ 38.765507] dwc2 12480000.hsotg: dwc2_hsotg_ep_disable: called for ep0 > [ 38.774050] wake enabled for irq 119 > [ 38.775761] BUCK9: No configuration > [ 38.779191] BUCK8_P3V3: No configuration > [ 38.782852] BUCK7_2.0V: No configuration > [ 38.786851] BUCK6_1.35V: No configuration > [ 38.790752] VDDQ_CKEM1_2_1.2V: No configuration > [ 38.796220] BUCK4: regulator suspend disable supported > [ 38.800769] BUCK3: regulator suspend disable supported > [ 38.806002] BUCK1: regulator suspend disable supported > [ 38.810644] LDO26: No configuration > [ 38.814169] VDDQ_LCD_1.8V: No configuration > [ 38.818267] LDO24: No configuration > [ 38.821732] LDO23: No configuration > [ 38.825262] LDO22_VDDQ_MMC4_2.8V: No configuration > [ 38.829992] TFLASH_2.8V: No configuration > [ 38.834040] LDO20_1.8V: No configuration > [ 38.837883] LDO19: No configuration > [ 38.841349] LDO18: No configuration > [ 38.844878] LDO17: No configuration > [ 38.848667] LDO16: regulator suspend disable supported > [ 38.853889] LDO15: regulator suspend disable supported > [ 38.858931] LDO14: regulator suspend disable supported > [ 38.863771] VDDQ_C2C_W_1.8V: No configuration > [ 38.868378] LDO12: regulator suspend disable supported > [ 38.873508] LDO11: regulator suspend disable supported > [ 38.878545] LDO10: regulator suspend disable supported > [ 38.883384] LDO9: No configuration > [ 38.887190] LDO8: regulator suspend disable supported > [ 38.892168] LDO7: regulator suspend disable supported > [ 38.897279] LDO6: regulator suspend disable supported > [ 38.901872] VDDQ_MMC1_3_1.8V: No configuration > [ 38.906363] VDDQ_MMC2_2.8V: No configuration > [ 38.910541] VDDQ_EXT_1.8V: No configuration > [ 38.915134] LDO2: regulator suspend disable supported > [ 38.919753] VDD_ALIVE_1.0V: No configuration > [ 38.935229] usb3503 0-0008: switched to STANDBY mode > [ 38.935981] wake enabled for irq 123 > [ 38.955192] samsung-pinctrl 11000000.pinctrl: Setting external > wakeup interrupt mask: 0xfbfff7ff > [ 38.975448] Disabling non-boot CPUs ... > [ 39.029279] s3c2410-wdt 10060000.watchdog: watchdog disabled > [ 39.029576] wake disabled for irq 123 > [ 39.044319] usb3503 0-0008: switched to HUB mode > [ 39.144089] wake disabled for irq 119 > [ 39.144812] dwc2 12480000.hsotg: resuming usb gadget g_ether > [ 39.422626] usb 1-2: reset high-speed USB device number 2 using exynos-ehci > [ 39.774632] usb 1-3: reset high-speed USB device number 3 using exynos-ehci > [ 40.106478] OOM killer enabled. > [ 40.106609] Restarting tasks ... done. > [ 40.111100] PM: suspend exit > [ 40.124058] mmc_host mmc1: Bus speed (slot 0) = 50000000Hz (slot > req 400000Hz, actual 396825HZ div = 63) > [root@archl-u3e ~]# [ 40.364705] mmc_host mmc1: Bus speed (slot 0) = > 50000000Hz (slot req 52000000Hz, actual 50000000HZ div = 0) > [ 41.220200] smsc95xx 1-2:1.0 eth0: link up, 100Mbps, full-duplex, lpa 0xC5E1 > ------------------------------------------------------------------------------- > > How such explanation could look like? For example like this: > > f8f3b7fc21b1 ("ARM: dts: exynos: Fix regulators configuration on Peach > > Pi/Pit Chromebooks") > > Marek clearly explained why he put the regulators "always-on", even > > tough we do not know everything about this. More over, he mentions > > that this fixes specific issue. > > > > Thanks but I am not the expert here in general with regulator on/off. > I will add comment if needed explain in more detail. This kernel log does not prove whether these DTS properties make sense or whether are proper at all. They prove that kernel uses some configuration but I did not ask for this. > > [snip] > > > Summarizing, please answer: > > 1. Why this is made off-in-suspend? > > 2. Why this can be made off-in-suspend? > > > > On MAX77686A PMIC. (some quote from the datasheet). > > Power ON/OFF by PWRREQ in PMIC=ON sequence is only effective when ON/OFF > on all regulators above are assigned by PWRREQ=H/L. > > All programming must be done before the AP enters the sleep mode by > pulling PWRREQ low since > the AP does not have programming capability in (deep) sleep mode. > > *Regulator are not really turned off but set into IDLE / Standby > state too be restored to Normal state* > > Power summary in image : [1] https://imgur.com/gallery/l74kliX > > Power Summary for MAX77686A of the regulator support PWRREQ > Power Default ON | ON/OFF > -------------------------------------- > BUCK1 | ON | PWRREQ or I2C > BUCK2 | ON | PWRREQ or I2C > BUCK3 | ON | PWRREQ or I2C > BUCK4 | ON | PWREWQ or I2C > BUCK5 | ON | I2C > BUCK6 | ON | I2C > BUCK7 | ON | I2C > BUCK8 | OFF | I2C or ENB8 The external enable/disable pin, ENB8 > BUCK9 | OFF | I2C or ENB9 The external enable/disable pin, ENB9 > LDO1 | ON | I2C > LDO2 | ON | PWRREQ or I2C > LDO3 | ON | I2C > LDO4 | ON | I2C > LDO5 | ON | I2C > LDO6 | ON | PWRREQ or I2C > LDO7 | ON | PWRREQ or I2C > LDO8 | ON | PWRREQ or I2C > LDO9 | OFF | I2C > LDO10 | ON | PWRREQ or I2C > LDO11 | ON | PWRREQ or I2C > LDO12 | ON | PWRREQ or I2C > LDO13 | ON | I2C > LDO14 | ON | PWRREQ or I2C > LDO15 | ON | PWRREQ or I2C > LDO16 | ON | PWRREQ or I2C > LDO17 | OFF | I2C > LDO18 | OFF | I2C > LDO19 | OFF | I2C > LDO20 | OFF | I2C OR ENL20 ENL20, external enable/disable pin > LDO21 | OFF | I2C OR ENL21 ENL21, external enable/disable pin > LDO22 | OFF | I2C OR ENL22 ENL22, external enable/disable pin > LDO23 | OFF | I2C > LDO24 | OFF | I2C > LDO25 | OFF | I2C > LDO26 | OFF | I2C You quoted the datasheet which describes PMIC behavior. This does not answer to my two questions at all. > > > > ldo20_reg: LDO20 { > > > @@ -421,6 +451,9 @@ > > > regulator-max-microvolt = <1100000>; > > > regulator-always-on; > > > regulator-boot-on; > > > + regulator-state-mem { > > > + regulator-off-in-suspend; > > > + }; > > > }; > > > > I questioned this change two times. You did not answer to my > > question... If you turn memory bus regulator off, how the memory will > > work in Suspend-to-Memory mode? I might be missing here something but > > it just looks suspicious. Maybe the regulator does not supply the > > memory itself (so refresh works even when it is down), just the > > interface? I don't know, it just looks suspicious. I need to see > > proper explanation. > > > > I am sorry but I will not check other hunks in this patch. Please > > provide the answers for all my questions here first. > > > > As per above table of MAX77686 PWRREQ capabilities regulator nodes > can be turned off during suspend reset of them remain on during suspend. Just because something can be turned off does not mean that it should. Apparently you made this assumption everywhere here and for my questions "why?" you reply "datasheet of PMIC says it can be done". This is wrong assumption and wrong justification for the patch. I cannot accept this. Please answer my questions and provide proper rationale for this patch. > > > Best regards, > > Krzysztof > > > > We could monitor the regulator states via sys filesystem and also > using below tool > https://git.linaro.org/power/powerdebug.git > > I have tried to summaries the feature required for this patch. > some of which I have overlooked earlier before sending the patch. > > I am really poor in english for transform / interpreter the technical details > required at for your queries. But I will try to improve my self in the future. Again this is observing of kernel behavior after applying patch. It has nothing to do whether change should be implemented and how it affects real hardware. This is not even a measurement... Best regards, Krzysztof ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1] ARM: dts: exynos: Add proper regulator states for suspend-to-mem for odroid-u3 2019-03-26 10:58 ` Krzysztof Kozlowski @ 2019-04-04 4:01 ` Anand Moon 0 siblings, 0 replies; 6+ messages in thread From: Anand Moon @ 2019-04-04 4:01 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: devicetree, linux-arm-kernel, linux-samsung-soc, Linux Kernel, Rob Herring, Kukjin Kim, Marek Szyprowski, Chanwoo Choi hi Krzysztof, On Tue, 26 Mar 2019 at 16:28, Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Tue, 26 Mar 2019 at 11:35, Anand Moon <linux.amoon@gmail.com> wrote: > > (...) > > > > This is third or fourth submission but you marked it as v1. This makes > > > it very difficult to discuss and reference previous versions. > > > > > > The commit message did not change since beginning (first version). I > > > asked twice that you need to explain exactly why you put the the > > > regulator to off or on state in suspend. Why? > > > Because: > > > 1. This change looks without justification - once you put on, then you > > > put off, now again on, > > > 2. Anyone reading the code later must know the rationale why this was done, > > > 3. I am not quite sure whether this is good setting so I would be > > > happy to be convinced. > > > > > > > Like I mention in the patch summary that this. > > > > Current changes are based on > > [0] https://www.kernel.org/doc/Documentation/devicetree/bindings/regulator/max77686.txt > > > > Regulators which can be turned off during system suspend: > > -LDOn : 2, 6-8, 10-12, 14-16, > > -BUCKn : 1-4. > > Use standard regulator bindings for it ('regulator-off-in-suspend'). > > I do not see how this is related to my questions. > > > > How to provide such explanation? The best in commit message. Sometimes > > > in the comment in the code, depends. > > > > Ok I have been testing with following regulator debug prints to catch error. > > [0] max77686-regulator.patch > > > > below is the console logs during suspend and resume. > > ------------------------------------------------------------------------------- > > Last login: Sat Mar 23 18:22:46 on ttySAC1 > > [root@archl-u3e ~]# echo no > /sys/module/printk/parameters/console_suspend > > [root@archl-u3e ~]# rtcwake -d /dev/rtc0 -m mem -s 10 > > rtcwake: wakeup from "mem" using /dev/rtc0 at Sat Mar 23 19:56:17 2019 > > [ 38.595854] PM: suspend entry (deep) > > [ 38.596603] PM: Syncing filesystems ... done. > > [ 38.629351] Freezing user space processes ... (elapsed 0.002 seconds) done. > > [ 38.633192] OOM killer disabled. > > [ 38.636035] Freezing remaining freezable tasks ... (elapsed 0.001 > > seconds) done. > > [ 38.675059] smsc95xx 1-2:1.0 eth0: entering SUSPEND2 mode > > [ 38.753120] dwc2 12480000.hsotg: suspending usb gadget g_ether > > [ 38.754007] dwc2 12480000.hsotg: new device is full-speed > > [ 38.758960] dwc2 12480000.hsotg: dwc2_hsotg_ep_disable: called for ep0 > > [ 38.765507] dwc2 12480000.hsotg: dwc2_hsotg_ep_disable: called for ep0 > > [ 38.774050] wake enabled for irq 119 > > [ 38.775761] BUCK9: No configuration > > [ 38.779191] BUCK8_P3V3: No configuration > > [ 38.782852] BUCK7_2.0V: No configuration > > [ 38.786851] BUCK6_1.35V: No configuration > > [ 38.790752] VDDQ_CKEM1_2_1.2V: No configuration > > [ 38.796220] BUCK4: regulator suspend disable supported > > [ 38.800769] BUCK3: regulator suspend disable supported > > [ 38.806002] BUCK1: regulator suspend disable supported > > [ 38.810644] LDO26: No configuration > > [ 38.814169] VDDQ_LCD_1.8V: No configuration > > [ 38.818267] LDO24: No configuration > > [ 38.821732] LDO23: No configuration > > [ 38.825262] LDO22_VDDQ_MMC4_2.8V: No configuration > > [ 38.829992] TFLASH_2.8V: No configuration > > [ 38.834040] LDO20_1.8V: No configuration > > [ 38.837883] LDO19: No configuration > > [ 38.841349] LDO18: No configuration > > [ 38.844878] LDO17: No configuration > > [ 38.848667] LDO16: regulator suspend disable supported > > [ 38.853889] LDO15: regulator suspend disable supported > > [ 38.858931] LDO14: regulator suspend disable supported > > [ 38.863771] VDDQ_C2C_W_1.8V: No configuration > > [ 38.868378] LDO12: regulator suspend disable supported > > [ 38.873508] LDO11: regulator suspend disable supported > > [ 38.878545] LDO10: regulator suspend disable supported > > [ 38.883384] LDO9: No configuration > > [ 38.887190] LDO8: regulator suspend disable supported > > [ 38.892168] LDO7: regulator suspend disable supported > > [ 38.897279] LDO6: regulator suspend disable supported > > [ 38.901872] VDDQ_MMC1_3_1.8V: No configuration > > [ 38.906363] VDDQ_MMC2_2.8V: No configuration > > [ 38.910541] VDDQ_EXT_1.8V: No configuration > > [ 38.915134] LDO2: regulator suspend disable supported > > [ 38.919753] VDD_ALIVE_1.0V: No configuration > > [ 38.935229] usb3503 0-0008: switched to STANDBY mode > > [ 38.935981] wake enabled for irq 123 > > [ 38.955192] samsung-pinctrl 11000000.pinctrl: Setting external > > wakeup interrupt mask: 0xfbfff7ff > > [ 38.975448] Disabling non-boot CPUs ... > > [ 39.029279] s3c2410-wdt 10060000.watchdog: watchdog disabled > > [ 39.029576] wake disabled for irq 123 > > [ 39.044319] usb3503 0-0008: switched to HUB mode > > [ 39.144089] wake disabled for irq 119 > > [ 39.144812] dwc2 12480000.hsotg: resuming usb gadget g_ether > > [ 39.422626] usb 1-2: reset high-speed USB device number 2 using exynos-ehci > > [ 39.774632] usb 1-3: reset high-speed USB device number 3 using exynos-ehci > > [ 40.106478] OOM killer enabled. > > [ 40.106609] Restarting tasks ... done. > > [ 40.111100] PM: suspend exit > > [ 40.124058] mmc_host mmc1: Bus speed (slot 0) = 50000000Hz (slot > > req 400000Hz, actual 396825HZ div = 63) > > [root@archl-u3e ~]# [ 40.364705] mmc_host mmc1: Bus speed (slot 0) = > > 50000000Hz (slot req 52000000Hz, actual 50000000HZ div = 0) > > [ 41.220200] smsc95xx 1-2:1.0 eth0: link up, 100Mbps, full-duplex, lpa 0xC5E1 > > ------------------------------------------------------------------------------- > > > How such explanation could look like? For example like this: > > > f8f3b7fc21b1 ("ARM: dts: exynos: Fix regulators configuration on Peach > > > Pi/Pit Chromebooks") > > > Marek clearly explained why he put the regulators "always-on", even > > > tough we do not know everything about this. More over, he mentions > > > that this fixes specific issue. > > > > > > > Thanks but I am not the expert here in general with regulator on/off. > > I will add comment if needed explain in more detail. > > This kernel log does not prove whether these DTS properties make sense > or whether are proper at all. They prove that kernel uses some > configuration but I did not ask for this. > > > > > [snip] > > > > > Summarizing, please answer: > > > 1. Why this is made off-in-suspend? > > > 2. Why this can be made off-in-suspend? > > > > > > > On MAX77686A PMIC. (some quote from the datasheet). > > > > Power ON/OFF by PWRREQ in PMIC=ON sequence is only effective when ON/OFF > > on all regulators above are assigned by PWRREQ=H/L. > > > > All programming must be done before the AP enters the sleep mode by > > pulling PWRREQ low since > > the AP does not have programming capability in (deep) sleep mode. > > > > *Regulator are not really turned off but set into IDLE / Standby > > state too be restored to Normal state* > > > > Power summary in image : [1] https://imgur.com/gallery/l74kliX > > > > Power Summary for MAX77686A of the regulator support PWRREQ > > Power Default ON | ON/OFF > > -------------------------------------- > > BUCK1 | ON | PWRREQ or I2C > > BUCK2 | ON | PWRREQ or I2C > > BUCK3 | ON | PWRREQ or I2C > > BUCK4 | ON | PWREWQ or I2C > > BUCK5 | ON | I2C > > BUCK6 | ON | I2C > > BUCK7 | ON | I2C > > BUCK8 | OFF | I2C or ENB8 The external enable/disable pin, ENB8 > > BUCK9 | OFF | I2C or ENB9 The external enable/disable pin, ENB9 > > LDO1 | ON | I2C > > LDO2 | ON | PWRREQ or I2C > > LDO3 | ON | I2C > > LDO4 | ON | I2C > > LDO5 | ON | I2C > > LDO6 | ON | PWRREQ or I2C > > LDO7 | ON | PWRREQ or I2C > > LDO8 | ON | PWRREQ or I2C > > LDO9 | OFF | I2C > > LDO10 | ON | PWRREQ or I2C > > LDO11 | ON | PWRREQ or I2C > > LDO12 | ON | PWRREQ or I2C > > LDO13 | ON | I2C > > LDO14 | ON | PWRREQ or I2C > > LDO15 | ON | PWRREQ or I2C > > LDO16 | ON | PWRREQ or I2C > > LDO17 | OFF | I2C > > LDO18 | OFF | I2C > > LDO19 | OFF | I2C > > LDO20 | OFF | I2C OR ENL20 ENL20, external enable/disable pin > > LDO21 | OFF | I2C OR ENL21 ENL21, external enable/disable pin > > LDO22 | OFF | I2C OR ENL22 ENL22, external enable/disable pin > > LDO23 | OFF | I2C > > LDO24 | OFF | I2C > > LDO25 | OFF | I2C > > LDO26 | OFF | I2C > > You quoted the datasheet which describes PMIC behavior. This does not > answer to my two questions at all. > > > > > > > ldo20_reg: LDO20 { > > > > @@ -421,6 +451,9 @@ > > > > regulator-max-microvolt = <1100000>; > > > > regulator-always-on; > > > > regulator-boot-on; > > > > + regulator-state-mem { > > > > + regulator-off-in-suspend; > > > > + }; > > > > }; > > > > > > I questioned this change two times. You did not answer to my > > > question... If you turn memory bus regulator off, how the memory will > > > work in Suspend-to-Memory mode? I might be missing here something but > > > it just looks suspicious. Maybe the regulator does not supply the > > > memory itself (so refresh works even when it is down), just the > > > interface? I don't know, it just looks suspicious. I need to see > > > proper explanation. > > > > > > I am sorry but I will not check other hunks in this patch. Please > > > provide the answers for all my questions here first. > > > > > > > As per above table of MAX77686 PWRREQ capabilities regulator nodes > > can be turned off during suspend reset of them remain on during suspend. > > Just because something can be turned off does not mean that it should. > Apparently you made this assumption everywhere here and for my > questions "why?" you reply "datasheet of PMIC says it can be done". > This is wrong assumption and wrong justification for the patch. > > I cannot accept this. Please answer my questions and provide proper > rationale for this patch. > > > > > > Best regards, > > > Krzysztof > > > > > > > We could monitor the regulator states via sys filesystem and also > > using below tool > > https://git.linaro.org/power/powerdebug.git > > > > I have tried to summaries the feature required for this patch. > > some of which I have overlooked earlier before sending the patch. > > > > I am really poor in english for transform / interpreter the technical details > > required at for your queries. But I will try to improve my self in the future. > > Again this is observing of kernel behavior after applying patch. It > has nothing to do whether change should be implemented and how it > affects real hardware. This is not even a measurement... > > Best regards, > Krzysztof Lets discard this patch. sorry for the trouble in review. Best Regards -Anand ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-04 4:01 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-24 8:32 [PATCH v1] ARM: dts: exynos: Add proper regulator states for suspend-to-mem for odroid-u3 Anand Moon 2019-03-25 12:46 ` Krzysztof Kozlowski 2019-03-25 12:56 ` Krzysztof Kozlowski 2019-03-26 10:35 ` Anand Moon 2019-03-26 10:58 ` Krzysztof Kozlowski 2019-04-04 4:01 ` Anand Moon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).