From: Krzysztof Kozlowski <krzk@kernel.org>
To: Anand Moon <linux.amoon@gmail.com>
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 2/2] soc: samsung: pmu: Add the PMU data of exynos4412 to support low-power state
Date: Fri, 15 Feb 2019 08:33:17 +0100 [thread overview]
Message-ID: <CAJKOXPdo8cVwL8kb4NzvvDUDaVgAgDf3OM=aJqVdkyPminPG2A@mail.gmail.com> (raw)
In-Reply-To: <CANAwSgQfarF9VAJwPuGgaM3T5WFPqSdxDU8OP_OLMb_GWCB=XQ@mail.gmail.com>
On Thu, 14 Feb 2019 at 19:37, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi Krzysztof,
>
> Thanks for your review comments.
>
> On Thu, 14 Feb 2019 at 18:29, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On Wed, 13 Feb 2019 at 22:41, Anand Moon <linux.amoon@gmail.com> wrote:
> > >
> > > This patch adds configration for PMU (Power Management Unit) state
> > > tuning for exynos4412 SoC in order to enter low-power mode during
> > > suspend power modes and help resume from suspend state.
> >
> > The U3 and Trats2 already enter STOP/S2R so please describe what
> > exactly you change.
> >
> > > Fixes: bfce552d0b1 ("drivers: soc: Add support for Exynos PMU driver")
> >
> > How it fixes it? What was broken in that commit?
>
> * I was not aware on their is common framework for suspend and resume
> other than setting this here.I only look in to some the other exynos
> pmu architecture
> and referring 3.10.x kernel to model my changes.*
Suspend in general should be working already so adding this code just
because is not a valid reason. Please specify what is not working
first.
> >
> > > 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 previous patch.
> > > New patch to this series to support suspend and resume state
> > >
> > > Changes have been tested on microSD card but fails to resume on cMMC.
> > > It need to be investigated and more debuging
> > > ---
> > > drivers/soc/samsung/exynos4-pmu.c | 83 +++++++++++++++++++++
> > > include/linux/soc/samsung/exynos-regs-pmu.h | 21 ++++++
> > > 2 files changed, 104 insertions(+)
> > >
> > > diff --git a/drivers/soc/samsung/exynos4-pmu.c b/drivers/soc/samsung/exynos4-pmu.c
> > > index a7cdbf1aac0c..d261a0d2371e 100644
> > > --- a/drivers/soc/samsung/exynos4-pmu.c
> > > +++ b/drivers/soc/samsung/exynos4-pmu.c
> > > @@ -200,10 +200,93 @@ static const struct exynos_pmu_conf exynos4412_pmu_config[] = {
> > > { PMU_TABLE_END,},
> > > };
> > >
> > > +static unsigned int const exynos4412_list_feed[] = {
> > > + EXYNOS4_ARM_CORE0_OPTION,
> > > + EXYNOS4_ARM_CORE1_OPTION,
> > > + EXYNOS4_ARM_CORE2_OPTION,
> > > + EXYNOS4_ARM_CORE3_OPTION,
> > > + EXYNOS4_ARM_COMMON_OPTION,
> > > + EXYNOS4_CAM_OPTION,
> > > + EXYNOS4_TV_OPTION,
> > > + EXYNOS4_MFC_OPTION,
> > > + EXYNOS4_G3D_OPTION,
> > > + EXYNOS4_LCD0_OPTION,
> > > + EXYNOS4_ISP_OPTION,
> > > + EXYNOS4_MAUDIO_OPTION,
> > > + EXYNOS4_GPS_OPTION,
> > > + EXYNOS4_GPS_ALIVE_OPTION,
> > > +};
> > > +
> > > +static void exynos4412_pmu_central_seq(bool enable)
> >
> > You name the argument as "enable" but during initialization and
> > system running you pass here false. It confuses me. What do you enable
> > here?
> >
>
> Yep your are correct need to drop this function as already done in
> common frame work.
>
> > > +{
> > > + unsigned int value;
> > > +
> > > + value = pmu_raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
> > > + if (enable)
> > > + value &= ~S5P_CENTRAL_LOWPWR_CFG;
> > > + else
> > > + value |= S5P_CENTRAL_LOWPWR_CFG;
> > > + pmu_raw_writel(value, S5P_CENTRAL_SEQ_CONFIGURATION);
> >
> > You duplicate exynos_pm_central_suspend() without removing the original code.
> >
> > > +
> > > + value = pmu_raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION_COREBLK);
> > > + if (enable)
> > > + value &= ~S5P_CENTRAL_LOWPWR_CFG;
> > > + else
> > > + value |= S5P_CENTRAL_LOWPWR_CFG;
> >
> > As manual says - set this register only if you disable C2C. Our entire
> > low power configuration for STOP mode is for C2C enabled case so you
> > add inconsistent configuration.
>
> Ok Sorry I overlook this code change.
>
> Enable system power down. Set only CENTRAL_SEQ_CONFIGURATION register
> if you disable C2C.
> Set both CENTRAL_SEQ_CONFIGURATION and
> CENTRAL_SEQ_CONFIGURATION_COREBLK registers if you enable C2C.
>
> > > + pmu_raw_writel(value, S5P_CENTRAL_SEQ_CONFIGURATION_COREBLK);
> > > +}
> > > +
> > > +static void exynos4412_pmu_init(void)
> > > +{
> > > + unsigned int value;
> > > + int i;
> > > +
> > > + /* Enable USE_STANDBY_WFI for all CORE */
> > > + pmu_raw_writel(S5P_USE_STANDBY_WFI_ALL, S5P_CENTRAL_SEQ_OPTION);
> >
> > This does not look related to improving suspend... it looks unrelated.
> >
>
> Ok yes your are correct all ready done in comment frame work.
>
> "Execute WFI/WFE for all CPU cores. As soon as all the CPU cores in
> Exynos 4412 SCP enter STANDBY mode"
> > > +
> > > + /* Decides whether to use retention capability */
> > > + value = pmu_raw_readl(S5P_ARM_L2_0_OPTION);
> > > + value &= ~EXYNOS_L2_USE_RETENTION;
> > > + pmu_raw_writel(value, S5P_ARM_L2_0_OPTION);
> > > +
> > > + value = pmu_raw_readl(S5P_ARM_L2_1_OPTION);
> > > + value &= ~EXYNOS_L2_USE_RETENTION;
> > > + pmu_raw_writel(value, S5P_ARM_L2_1_OPTION);
> > > +
> > > + /* Set PSHOLD port for output high */
> > > + value = pmu_raw_readl(S5P_PS_HOLD_CONTROL);
> > > + value |= S5P_PS_HOLD_OUTPUT_HIGH;
> > > + pmu_raw_writel(value, S5P_PS_HOLD_CONTROL);
> > > +
> > > + /* Enable signal for PSHOLD port */
> > > + value = pmu_raw_readl(S5P_PS_HOLD_CONTROL);
> > > + value |= S5P_PS_HOLD_EN;
> > > + pmu_raw_writel(value, S5P_PS_HOLD_CONTROL);
> > > +
> >
> > The same - not related.
>
> PS_HOLD_CONTROL need to be set under following set
> Hardware reset Reset
> Watchdog timer reset Keep its value
> Software reset Keep its value
> Warm reset Keep its value
> Wakeup reset Keep its value
Which is not related to suspend...
>
> >
> > > + /* Enable only SC_FEEDBACK */
> > > + for (i = 0; i < ARRAY_SIZE(exynos4412_list_feed); i++) {
> > > + value = pmu_raw_readl(exynos4412_list_feed[i]);
> > > + value &= ~(EXYNOS_USE_SC_COUNTER);
> > > + value |= EXYNOS_USE_SC_FEEDBACK;
> > > + pmu_raw_writel(value, exynos4412_list_feed[i]);
> >
> > Why do you prefer to use feedback instead of counter?
> >
>
> Their is a note below if we use this setting.
> NOTE: Either one of USE_SC_FEEDBACK and USE_SC_COUNTER should be activated.
This is not the answer to my question at all.
Best regards,
Krzysztof
_______________________________________________
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 7:34 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
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 [this message]
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='CAJKOXPdo8cVwL8kb4NzvvDUDaVgAgDf3OM=aJqVdkyPminPG2A@mail.gmail.com' \
--to=krzk@kernel.org \
--cc=cw00.choi@samsung.com \
--cc=devicetree@vger.kernel.org \
--cc=kgene@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux.amoon@gmail.com \
--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).