| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJKOXPcor0PC-H73nTyVnPYSk+1raL5Q0WSdH-MLvzVz2+u+pA@mail.gmail.com>
Date: Thu, 14 Feb 2019 13:59:30 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Anand Moon <linux.amoon@...il.com>
Cc: devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
"linux-samsung-soc@...r.kernel.org"
<linux-samsung-soc@...r.kernel.org>, linux-kernel@...r.kernel.org,
Rob Herring <robh+dt@...nel.org>,
Kukjin Kim <kgene@...nel.org>,
Marek Szyprowski <m.szyprowski@...sung.com>,
Tomasz Figa <tomasz.figa@...il.com>,
Chanwoo Choi <cw00.choi@...sung.com>,
Pankaj Dubey <pankaj.dubey@...sung.com>
Subject: Re: [RFC 2/2] soc: samsung: pmu: Add the PMU data of exynos4412 to
support low-power state
On Wed, 13 Feb 2019 at 22:41, Anand Moon <linux.amoon@...il.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?
> Cc: Marek Szyprowski <m.szyprowski@...sung.com>
> Cc: Krzysztof Kozlowski <krzk@...nel.org>
> Cc: Chanwoo Choi <cw00.choi@...sung.com>
> Signed-off-by: Anand Moon <linux.amoon@...il.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?
> +{
> + 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.
> + 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.
> +
> + /* 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.
> + /* 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?
> + }
> +
> + exynos4412_pmu_central_seq(false);
> +
> + pr_info("EXYNOS4x12 PMU Initialize\n");
> +}
> +
> +static void exynos4412_powerdown_conf(enum sys_powerdown mode)
> +{
> + exynos4412_pmu_central_seq(true);
> +}
> +
> const struct exynos_pmu_data exynos4210_pmu_data = {
> .pmu_config = exynos4210_pmu_config,
> };
>
> const struct exynos_pmu_data exynos4412_pmu_data = {
> .pmu_config = exynos4412_pmu_config,
> + .pmu_init = exynos4412_pmu_init,
> + .powerdown_conf = exynos4412_powerdown_conf,
> };
> diff --git a/include/linux/soc/samsung/exynos-regs-pmu.h b/include/linux/soc/samsung/exynos-regs-pmu.h
> index 5addaf5ccbce..6beed3e669d2 100644
> --- a/include/linux/soc/samsung/exynos-regs-pmu.h
> +++ b/include/linux/soc/samsung/exynos-regs-pmu.h
> @@ -16,6 +16,8 @@
>
> #define S5P_CENTRAL_SEQ_CONFIGURATION 0x0200
>
> +#define S5P_CENTRAL_SEQ_CONFIGURATION_COREBLK 0x0240
> +
> #define S5P_CENTRAL_LOWPWR_CFG (1 << 16)
>
> #define S5P_CENTRAL_SEQ_OPTION 0x0208
> @@ -347,6 +349,25 @@
> #define EXYNOS3_OPTION_USE_SC_FEEDBACK (1 << 1)
> #define EXYNOS3_OPTION_SKIP_DEACTIVATE_ACEACP_IN_PWDN (1 << 7)
>
> +/* EXYNOS4 */
> +#define EXYNOS_USE_SC_FEEDBACK BIT(1)
> +#define EXYNOS_USE_SC_COUNTER BIT(0)
Instead of adding third set of these defines, I would prefer to
cleanup existing and squash all into one (exynos3 and exynos5).
> +
> +#define EXYNOS4_ARM_CORE0_OPTION 0x2008
> +#define EXYNOS4_ARM_CORE1_OPTION 0x2088
> +#define EXYNOS4_ARM_CORE2_OPTION 0x2108
> +#define EXYNOS4_ARM_CORE3_OPTION 0x2188
The same.
> +#define EXYNOS4_ARM_COMMON_OPTION 0x2408
> +#define EXYNOS4_CAM_OPTION 0x3C08
> +#define EXYNOS4_TV_OPTION 0x3C28
> +#define EXYNOS4_MFC_OPTION 0x3C48
> +#define EXYNOS4_G3D_OPTION 0x3C68
> +#define EXYNOS4_LCD0_OPTION 0x3C88
> +#define EXYNOS4_ISP_OPTION 0x3CA8
> +#define EXYNOS4_MAUDIO_OPTION 0x3CC8
> +#define EXYNOS4_GPS_OPTION 0x3CE8
> +#define EXYNOS4_GPS_ALIVE_OPTION 0x3D08
Do you need them? I think the are already defined and used by Exynos3.
Best regards,
Krzysztof
> +
> /* For EXYNOS5 */
>
> #define EXYNOS5_AUTO_WDTRESET_DISABLE 0x0408
> --
> 2.20.1
>
Powered by blists - more mailing lists