From: Anand Moon <linux.amoon@gmail.com>
To: Krzysztof Kozlowski <krzk@kernel.org>
Cc: devicetree <devicetree@vger.kernel.org>,
"linux-samsung-soc@vger.kernel.org"
<linux-samsung-soc@vger.kernel.org>,
Pankaj Dubey <pankaj.dubey@samsung.com>,
Linux Kernel <linux-kernel@vger.kernel.org>,
Tomasz Figa <tomasz.figa@gmail.com>,
Chanwoo Choi <cw00.choi@samsung.com>,
Rob Herring <robh+dt@kernel.org>, Kukjin Kim <kgene@kernel.org>,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
Marek Szyprowski <m.szyprowski@samsung.com>
Subject: Re: [RFC 1/2] ARM: dts: exynos: Add proper regulator states for suspend-to-mem for odroid-u3
Date: Fri, 15 Feb 2019 14:13:55 +0530 [thread overview]
Message-ID: <CANAwSgQv6O281dJ_8nz8dvfuf5sYijZTn9XBEx1RDwN6ikgjQA@mail.gmail.com> (raw)
In-Reply-To: <CAJKOXPcrkVEgsaTFrQbCvn=gx3h6av7zvaXcfroNMYk6qgs8mQ@mail.gmail.com>
Hi Krzysztof,
On Fri, 15 Feb 2019 at 13:01, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Thu, 14 Feb 2019 at 19:35, Anand Moon <linux.amoon@gmail.com> wrote:
> >
> > hi Krzysztof,
> >
> > Thanks fro your review comments.
> >
> > On Thu, 14 Feb 2019 at 18:11, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > Hi Anand,
> > >
> > > Thanks for the patch. See comments below.
> > >
> > > On Wed, 13 Feb 2019 at 22:41, 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.
> > > >
> > > > 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>
> > > > ---
> > > >
> > > > Changes from previos patch
> > > > [0] https://patchwork.kernel.org/patch/10712549/
> > > >
> > > > Set all the WAKEUP source regulator in suspend-on state.
> > > > LD04, LD012, LD015, LD020, LD022
> > > > 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
> > > >
> > > > Tested on microSD card and it resumes correcly after suspend.
> > > > eMMC is not able to resume after entering into suspend state,
> > > > which need to be investigated and how to debug more.
> > > > ---
> > > > .../boot/dts/exynos4412-odroid-common.dtsi | 63 +++++++++++++++++++
> > > > arch/arm/boot/dts/exynos4412-odroidu3.dts | 3 +
> > > > 2 files changed, 66 insertions(+)
> > > >
> > > > diff --git a/arch/arm/boot/dts/exynos4412-odroid-common.dtsi b/arch/arm/boot/dts/exynos4412-odroid-common.dtsi
> > > > index 08d3a0a7b4eb..e984461c37d9 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;
> > > > + };
> > >
> > > I see my comment from previous patch was not addressed.
> >
> > I left this unchanged since this regulator is not active linked and used.
> > Ok I will change this to regulator-on-in-suspend,
>
> Why?
>
> Previous patch was setting this to "on". I said that this is noop and
> if you want to add it, I need explanation why this regulator has to
> stay on during suspend.
>
> So you changed to "off"... which is still noop... and you did not
> provide explanation. Now you replied that you will change to "on"...
> so this will be circle. Still without explanation.
>
I will re check with my previous patch and the driver code which
support suspend state.
Ok check the regulator driver max77686-regulator and enable the logs their.
> >
> > From documentation device tree binding regulator.txt
> > "- regulator-always-on: boolean, regulator should never be disabled"
> >
> > But some regulator switches to a low power mode under light current loads
> > and the device automatically switches back to a fast response mode as the
> > output load increases.
> >
> > >
> > > > };
> > > >
> > > > ldo3_reg: LDO3 {
> > > > @@ -295,6 +298,9 @@
> > > > regulator-min-microvolt = <1800000>;
> > > > regulator-max-microvolt = <1800000>;
> > > > regulator-always-on;
> > > > + regulator-state-mem {
> > > > + regulator-off-in-suspend;
> > > > + };
> > >
> > > The same...
> >
> > Ok I will change this to regulator-on-in-suspend,
>
> The same - why changing this to on or off? I need explanation why this
> should be in first place.
I will check in the driver code.
>
> > >
> > > > };
> > > >
> > > > ldo4_reg: LDO4 {
> > > > @@ -302,6 +308,9 @@
> > > > regulator-min-microvolt = <2800000>;
> > > > regulator-max-microvolt = <2800000>;
> > > > regulator-boot-on;
> > > > + regulator-state-mem {
> > > > + regulator-on-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > ldo5_reg: LDO5 {
> > > > @@ -310,6 +319,9 @@
> > > > regulator-max-microvolt = <1800000>;
> > > > regulator-always-on;
> > > > regulator-boot-on;
> > > > + regulator-state-mem {
> > > > + regulator-off-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > ldo6_reg: LDO6 {
> > > > @@ -317,6 +329,9 @@
> > > > regulator-min-microvolt = <1000000>;
> > > > regulator-max-microvolt = <1000000>;
> > > > regulator-always-on;
> > > > + regulator-state-mem {
> > > > + regulator-off-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > ldo7_reg: LDO7 {
> > > > @@ -324,18 +339,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-on-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > ldo10_reg: LDO10 {
> > > > regulator-name = "VDDQ_MIPIHSI_1.8V";
> > > > regulator-min-microvolt = <1800000>;
> > > > regulator-max-microvolt = <1800000>;
> > > > + regulator-state-mem {
> > > > + regulator-on-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > ldo11_reg: LDO11 {
> > > > @@ -343,6 +367,9 @@
> > > > regulator-min-microvolt = <1800000>;
> > > > regulator-max-microvolt = <1800000>;
> > > > regulator-always-on;
> > > > + regulator-state-mem {
> > > > + regulator-off-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > ldo12_reg: LDO12 {
> > > > @@ -351,6 +378,9 @@
> > > > regulator-max-microvolt = <3300000>;
> > > > regulator-always-on;
> > > > regulator-boot-on;
> > > > + regulator-state-mem {
> > > > + regulator-off-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > ldo13_reg: LDO13 {
> > > > @@ -359,6 +389,9 @@
> > > > regulator-max-microvolt = <1800000>;
> > > > regulator-always-on;
> > > > regulator-boot-on;
> > > > + regulator-state-mem {
> > > > + regulator-off-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > ldo14_reg: LDO14 {
> > > > @@ -367,6 +400,9 @@
> > > > regulator-max-microvolt = <1800000>;
> > > > regulator-always-on;
> > > > regulator-boot-on;
> > > > + regulator-state-mem {
> > > > + regulator-off-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > ldo15_reg: LDO15 {
> > > > @@ -375,6 +411,9 @@
> > > > regulator-max-microvolt = <1000000>;
> > > > regulator-always-on;
> > > > regulator-boot-on;
> > > > + regulator-state-mem {
> > > > + regulator-off-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > ldo16_reg: LDO16 {
> > > > @@ -383,6 +422,9 @@
> > > > regulator-max-microvolt = <1800000>;
> > > > regulator-always-on;
> > > > regulator-boot-on;
> > > > + regulator-state-mem {
> > > > + regulator-off-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > ldo20_reg: LDO20 {
> > > > @@ -396,6 +438,9 @@
> > > > regulator-min-microvolt = <2800000>;
> > > > regulator-max-microvolt = <2800000>;
> > > > regulator-boot-on;
> > > > + regulator-state-mem {
> > > > + regulator-on-in-suspend;
> > > > + };
> > >
> > >
> > > The same... any comments?
> >
> > I left this on for mshc_0 (emmc) regulator,
> > since I could not get suspend resume working on eMMc
>
> In general eMMC can be turned off during suspend. However eMMC layer
> may be handling this (manually turning off/on). So after checking and
> confirming it, please document it.
I will check in the driver code.
>
> > >
> > > > };
> > > >
> > > > ldo22_reg: LDO22 {
> > > > @@ -405,6 +450,9 @@
> > > > */
> > > > regulator-name = "LDO22";
> > > > regulator-boot-on;
> > > > + regulator-state-mem {
> > > > + regulator-on-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > ldo25_reg: LDO25 {
> > > > @@ -413,6 +461,9 @@
> > > > regulator-max-microvolt = <1800000>;
> > > > regulator-always-on;
> > > > regulator-boot-on;
> > > > + regulator-state-mem {
> > > > + regulator-off-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > buck1_reg: BUCK1 {
> > > > @@ -421,6 +472,9 @@
> > > > regulator-max-microvolt = <1100000>;
> > > > regulator-always-on;
> > > > regulator-boot-on;
> > > > + regulator-state-mem {
> > > > + regulator-off-in-suspend;
> > >
> > > Again, you did not address my comments.
> > >
> >
> > Buck1 support entering into LPM setting,
> > But I will set this to regulator-on-in-suspend; in next patch.
>
> Why?
I will check in the driver code.
>
> > > > + };
> > > > };
> > > >
> > > > buck2_reg: BUCK2 {
> > > > @@ -429,6 +483,9 @@
> > > > regulator-max-microvolt = <1350000>;
> > > > regulator-always-on;
> > > > regulator-boot-on;
> > > > + regulator-state-mem {
> > > > + regulator-on-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > buck3_reg: BUCK3 {
> > > > @@ -437,6 +494,9 @@
> > > > regulator-max-microvolt = <1050000>;
> > > > regulator-always-on;
> > > > regulator-boot-on;
> > > > + regulator-state-mem {
> > > > + regulator-off-in-suspend;
> > > > + };
> > >
> > > The same...
> >
> > Buck3 support entering into LPM setting,
> > But I will set this to regulator-on-in-suspend; in next patch.
>
> Again - why this should be on or off?
>
> > >
> > > > };
> > > >
> > > > buck4_reg: BUCK4 {
> > > > @@ -444,6 +504,9 @@
> > > > regulator-min-microvolt = <900000>;
> > > > regulator-max-microvolt = <1100000>;
> > > > regulator-microvolt-offset = <50000>;
> > > > + regulator-state-mem {
> > > > + regulator-off-in-suspend;
> > > > + };
> > > > };
> > > >
> > > > buck5_reg: BUCK5 {
> > > > diff --git a/arch/arm/boot/dts/exynos4412-odroidu3.dts b/arch/arm/boot/dts/exynos4412-odroidu3.dts
> > > > index 2bdf899df436..4ebde09fc51d 100644
> > > > --- a/arch/arm/boot/dts/exynos4412-odroidu3.dts
> > > > +++ b/arch/arm/boot/dts/exynos4412-odroidu3.dts
> > > > @@ -82,6 +82,9 @@
> > > > regulator-name = "LDO22_VDDQ_MMC4_2.8V";
> > > > regulator-min-microvolt = <2800000>;
> > > > regulator-max-microvolt = <2800000>;
> > > > + regulator-state-mem {
> > > > + regulator-on-in-suspend;
> > >
> > > Why?
> > >
> >
> > I chose not to disable mshc_0 (emmc) regulator,
> > since I could not get suspend resume working on eMMC.
>
> Having "regulator-on-in-suspend" is not the same as not disabling
> regulator during suspend. This property means you will explicitly
> enable it during suspend. To me it looks wrong so I would be happy to
> see explanations.
>
Ok I will disable this and check this help me fix the issue I am observing.
> Best regards,
> Krzysztof
*Sorry for wasting your time*
I know I have the improve a lot of things.
Best regards
-Anand
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-02-15 8:44 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-13 21:40 [RFC 0/2] Add support for Suspend and Resume for Exynos4412 on Anand Moon
2019-02-13 21:40 ` [RFC 1/2] ARM: dts: exynos: Add proper regulator states for suspend-to-mem for odroid-u3 Anand Moon
2019-02-14 12:41 ` Krzysztof Kozlowski
2019-02-14 18:34 ` Anand Moon
2019-02-15 7:30 ` Krzysztof Kozlowski
2019-02-15 8:43 ` Anand Moon [this message]
2019-02-15 13:58 ` Anand Moon
2019-02-13 21:40 ` [RFC 2/2] soc: samsung: pmu: Add the PMU data of exynos4412 to support low-power state Anand Moon
2019-02-14 12:59 ` Krzysztof Kozlowski
2019-02-14 18:37 ` Anand Moon
2019-02-15 7:33 ` Krzysztof Kozlowski
2019-02-15 8:43 ` Anand Moon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CANAwSgQv6O281dJ_8nz8dvfuf5sYijZTn9XBEx1RDwN6ikgjQA@mail.gmail.com \
--to=linux.amoon@gmail.com \
--cc=cw00.choi@samsung.com \
--cc=devicetree@vger.kernel.org \
--cc=kgene@kernel.org \
--cc=krzk@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=pankaj.dubey@samsung.com \
--cc=robh+dt@kernel.org \
--cc=tomasz.figa@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).