Hi Tomasz, On Sat, Dec 21, 2013 at 3:07 AM, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote: > Hi Abhilash, > > Please see my comments inline. > > On Monday 16 of December 2013 17:31:10 Abhilash Kesavan wrote: >> Add PMU configuration table for various low power modes - AFTR/LPA/SLEEP. >> Also, add core s2r support for Exynos5420. >> >> Signed-off-by: Abhilash Kesavan <a.kesavan@xxxxxxxxxxx> >> --- >> This patch depends on "ARM: EXYNOS5: Add PMU settings for exynos5420" >> http://www.spinics.net/lists/linux-samsung-soc/msg24902.html >> >> arch/arm/mach-exynos/include/mach/regs-clock.h | 15 +++ >> arch/arm/mach-exynos/include/mach/regs-pmu.h | 84 ++++++++++++ >> arch/arm/mach-exynos/pm.c | 151 +++++++++++++++++++--- >> arch/arm/mach-exynos/pmu.c | 165 ++++++++++++++++++++++++ >> 4 files changed, 396 insertions(+), 19 deletions(-) >> >> diff --git a/arch/arm/mach-exynos/include/mach/regs-clock.h b/arch/arm/mach-exynos/include/mach/regs-clock.h >> index d36ad76..20008f6 100644 >> --- a/arch/arm/mach-exynos/include/mach/regs-clock.h >> +++ b/arch/arm/mach-exynos/include/mach/regs-clock.h >> @@ -363,6 +363,21 @@ >> #define PWR_CTRL2_CORE2_UP_RATIO (1 << 4) >> #define PWR_CTRL2_CORE1_UP_RATIO (1 << 0) >> >> +/* For EXYNOS5420 */ >> +#define EXYNOS5420_CLKSRC_MASK_CPERI EXYNOS_CLKREG(0x04300) >> +#define EXYNOS5420_CLKSRC_MASK_TOP0 EXYNOS_CLKREG(0x10300) >> +#define EXYNOS5420_CLKSRC_MASK_TOP1 EXYNOS_CLKREG(0x10304) >> +#define EXYNOS5420_CLKSRC_MASK_TOP2 EXYNOS_CLKREG(0x10308) >> +#define EXYNOS5420_CLKSRC_MASK_TOP7 EXYNOS_CLKREG(0x1031C) >> +#define EXYNOS5420_CLKSRC_MASK_DISP10 EXYNOS_CLKREG(0x1032C) >> +#define EXYNOS5420_CLKSRC_MASK_MAU EXYNOS_CLKREG(0x10334) >> +#define EXYNOS5420_CLKSRC_MASK_FSYS EXYNOS_CLKREG(0x10340) >> +#define EXYNOS5420_CLKSRC_MASK_PERIC0 EXYNOS_CLKREG(0x10350) >> +#define EXYNOS5420_CLKSRC_MASK_PERIC1 EXYNOS_CLKREG(0x10354) >> +#define EXYNOS5420_CLKSRC_MASK_ISP EXYNOS_CLKREG(0x10370) >> +#define EXYNOS5420_CLKGATE_BUS_DISP1 EXYNOS_CLKREG(0x10728) >> +#define EXYNOS5420_CLKGATE_IP_PERIC EXYNOS_CLKREG(0x10950) > > This looks suspicious. Let me see below if this is really needed for what > I think it is. > >> + >> /* Compatibility defines and inclusion */ >> >> #include <mach/regs-pmu.h> >> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h b/arch/arm/mach-exynos/include/mach/regs-pmu.h >> index d5d5386..ad316f3 100644 >> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h >> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h >> @@ -229,6 +229,7 @@ >> >> /* For EXYNOS5 */ >> >> +#define EXYNOS5_SYS_DISP1_BLK_CFG S5P_SYSREG(0x0214) > > Is it really a definition common for all Exynos5 SoCs? I do see the register in both 5250 and 5420 even though the bits differ. > >> #define EXYNOS5_SYS_I2C_CFG S5P_SYSREG(0x0234) >> >> #define EXYNOS5_AUTO_WDTRESET_DISABLE S5P_PMUREG(0x0408) >> @@ -360,6 +361,7 @@ >> #define EXYNOS5_USE_SC_COUNTER (1 << 0) >> >> #define EXYNOS5_MANUAL_L2RSTDISABLE_CONTROL (1 << 2) >> +#define EXYNOS5_L2RSTDISABLE_VALUE (1 << 3) > > Ditto. No, I will make this 5420 specific. > >> #define EXYNOS5_SKIP_DEACTIVATE_ACEACP_IN_PWDN (1 << 7) >> >> #define EXYNOS5_OPTION_USE_STANDBYWFE (1 << 24) > [snip] >> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c >> index 7fb0f13..3ad103f 100644 >> --- a/arch/arm/mach-exynos/pm.c >> +++ b/arch/arm/mach-exynos/pm.c >> @@ -52,6 +52,22 @@ static const struct sleep_save exynos4210_set_clksrc[] = { >> { .reg = EXYNOS4210_CLKSRC_MASK_LCD1 , .val = 0x00001111, }, >> }; >> >> +static struct sleep_save exynos5420_set_clksrc[] = { >> + { .reg = EXYNOS5420_CLKSRC_MASK_CPERI, .val = 0xffffffff, }, >> + { .reg = EXYNOS5420_CLKSRC_MASK_TOP0, .val = 0x11111111, }, >> + { .reg = EXYNOS5420_CLKSRC_MASK_TOP1, .val = 0x11101111, }, >> + { .reg = EXYNOS5420_CLKSRC_MASK_TOP2, .val = 0x11111110, }, >> + { .reg = EXYNOS5420_CLKSRC_MASK_TOP7, .val = 0x00111100, }, >> + { .reg = EXYNOS5420_CLKSRC_MASK_DISP10, .val = 0x11111110, }, >> + { .reg = EXYNOS5420_CLKSRC_MASK_MAU, .val = 0x10000000, }, >> + { .reg = EXYNOS5420_CLKSRC_MASK_FSYS, .val = 0x11111110, }, >> + { .reg = EXYNOS5420_CLKSRC_MASK_PERIC0, .val = 0x11111110, }, >> + { .reg = EXYNOS5420_CLKSRC_MASK_PERIC1, .val = 0x11111100, }, >> + { .reg = EXYNOS5420_CLKSRC_MASK_ISP, .val = 0x11111000, }, >> + { .reg = EXYNOS5420_CLKGATE_BUS_DISP1, .val = 0xffffffff, }, >> + { .reg = EXYNOS5420_CLKGATE_IP_PERIC, .val = 0xffffffff, }, >> +}; >> + > > OK, just as I thought... > > NAK. Clock registers should be accessed only inside clock driver. Please > see my patch implementing similar thing for Exynos 4: > > http://thread.gmane.org/gmane.linux.kernel.samsung-soc/24078/focus=24087 I had not seen these changes, will re-base it on your changes. > > >> static struct sleep_save exynos4_epll_save[] = { >> SAVE_ITEM(EXYNOS4_EPLL_CON0), >> SAVE_ITEM(EXYNOS4_EPLL_CON1), >> @@ -66,6 +82,14 @@ static struct sleep_save exynos5_sys_save[] = { >> SAVE_ITEM(EXYNOS5_SYS_I2C_CFG), >> }; >> >> +static struct sleep_save exynos5420_sys_save[] = { >> + SAVE_ITEM(EXYNOS5_SYS_DISP1_BLK_CFG), >> +}; >> + >> +static struct sleep_save exynos5420_cpustate_save[] = { >> + SAVE_ITEM(S5P_VA_SYSRAM + 0x28), >> +}; >> + >> static struct sleep_save exynos_core_save[] = { >> /* SROM side */ >> SAVE_ITEM(S5P_SROM_BW), >> @@ -84,8 +108,15 @@ static int exynos_cpu_suspend(unsigned long arg) >> #ifdef CONFIG_CACHE_L2X0 >> outer_flush_all(); >> #endif >> + /* >> + * Clear the IRAM register that holds a low power flag. The presence >> + * of this flag decides if the primary cpu starts executing the low >> + * power function at wake-up or not. >> + */ >> + if (soc_is_exynos5420()) >> + __raw_writel(0x0, S5P_VA_SYSRAM + 0x28); >> >> - if (soc_is_exynos5250()) >> + if (soc_is_exynos5250() || soc_is_exynos5420()) >> flush_cache_all(); >> >> /* issue the standby signal into the pm unit. */ >> @@ -101,9 +132,14 @@ static void exynos_pm_prepare(void) >> >> s3c_pm_do_save(exynos_core_save, ARRAY_SIZE(exynos_core_save)); >> >> - if (!soc_is_exynos5250()) { >> + if (!(soc_is_exynos5250() || soc_is_exynos5420())) { >> s3c_pm_do_save(exynos4_epll_save, ARRAY_SIZE(exynos4_epll_save)); >> s3c_pm_do_save(exynos4_vpll_save, ARRAY_SIZE(exynos4_vpll_save)); >> + } else if (soc_is_exynos5420()) { >> + s3c_pm_do_save(exynos5420_sys_save, >> + ARRAY_SIZE(exynos5420_sys_save)); >> + s3c_pm_do_save(exynos5420_cpustate_save, >> + ARRAY_SIZE(exynos5420_cpustate_save)); >> } else { >> s3c_pm_do_save(exynos5_sys_save, ARRAY_SIZE(exynos5_sys_save)); >> /* Disable USE_RETENTION of JPEG_MEM_OPTION */ > > What about rewriting this code as follows: > > if (soc_is_exynos5420()) { > // exynos5420 specific code > } else if (soc_is_exynos5250()) { > // exynos5250 specific code > } else { > // exynos4 specific code > } > > Still, I'd prefer this to be based on top of my series I mentioned above > which removes any clock handling from this file and so makes the exynos4 > branch above disappear. OK..will re-base on your patches. > >> @@ -123,12 +159,32 @@ static void exynos_pm_prepare(void) >> >> /* Before enter central sequence mode, clock src register have to set */ >> >> - if (!soc_is_exynos5250()) >> + if (!(soc_is_exynos5250() || soc_is_exynos5420())) >> s3c_pm_do_restore_core(exynos4_set_clksrc, ARRAY_SIZE(exynos4_set_clksrc)); >> >> if (soc_is_exynos4210()) >> s3c_pm_do_restore_core(exynos4210_set_clksrc, ARRAY_SIZE(exynos4210_set_clksrc)); >> >> + if (soc_is_exynos5420()) { >> + s3c_pm_do_restore_core(exynos5420_set_clksrc, >> + ARRAY_SIZE(exynos5420_set_clksrc)); >> + >> + tmp = __raw_readl(EXYNOS5420_SFR_AXI_CGDIS1); >> + tmp |= EXYNOS5420_UFS; >> + __raw_writel(tmp, EXYNOS5420_SFR_AXI_CGDIS1); >> + >> + tmp = __raw_readl(EXYNOS5420_ARM_COMMON_OPTION); >> + tmp &= ~EXYNOS5_L2RSTDISABLE_VALUE; >> + __raw_writel(tmp, EXYNOS5420_ARM_COMMON_OPTION); >> + >> + tmp = __raw_readl(EXYNOS5420_FSYS2_OPTION); >> + tmp |= EXYNOS5420_EMULATION; >> + __raw_writel(tmp, EXYNOS5420_FSYS2_OPTION); >> + >> + tmp = __raw_readl(EXYNOS5420_PSGEN_OPTION); >> + tmp |= EXYNOS5420_EMULATION; >> + __raw_writel(tmp, EXYNOS5420_PSGEN_OPTION); >> + } >> } >> >> static int exynos_pm_add(struct device *dev, struct subsys_interface *sif) >> @@ -224,11 +280,17 @@ static __init int exynos_pm_drvinit(void) >> >> /* All wakeup disable */ >> >> - tmp = __raw_readl(S5P_WAKEUP_MASK); >> - tmp |= ((0xFF << 8) | (0x1F << 1)); >> - __raw_writel(tmp, S5P_WAKEUP_MASK); >> + if (soc_is_exynos5420()) { >> + tmp = __raw_readl(S5P_WAKEUP_MASK); >> + tmp |= ((0x7F << 7) | (0x1F << 1)); >> + __raw_writel(tmp, S5P_WAKEUP_MASK); >> + } else { >> + tmp = __raw_readl(S5P_WAKEUP_MASK); >> + tmp |= ((0xFF << 8) | (0x1F << 1)); >> + __raw_writel(tmp, S5P_WAKEUP_MASK); >> + } >> >> - if (!soc_is_exynos5250()) { >> + if (!(soc_is_exynos5250() || soc_is_exynos5420())) { >> pll_base = clk_get(NULL, "xtal"); > > This code is also removed by my series. Ok > >> >> if (!IS_ERR(pll_base)) { >> @@ -244,6 +306,7 @@ arch_initcall(exynos_pm_drvinit); >> static int exynos_pm_suspend(void) >> { >> unsigned long tmp; >> + unsigned int cluster_id; >> >> /* Setting Central Sequence Register for power down mode */ >> >> @@ -253,10 +316,20 @@ static int exynos_pm_suspend(void) >> >> /* Setting SEQ_OPTION register */ >> >> - tmp = (S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0); >> - __raw_writel(tmp, S5P_CENTRAL_SEQ_OPTION); >> + if (soc_is_exynos5420()) { >> + cluster_id = (read_cpuid(CPUID_MPIDR) >> 8) & 0xf; >> + if (!cluster_id) >> + __raw_writel(EXYNOS5420_ARM_USE_STANDBY_WFI0, >> + S5P_CENTRAL_SEQ_OPTION); >> + else >> + __raw_writel(EXYNOS5420_KFC_USE_STANDBY_WFI0, >> + S5P_CENTRAL_SEQ_OPTION); >> + } else if (soc_is_exynos5250()) { > > This code should be also called on other Exynos SoCs, not just 5250. Yes, Bartlomiej caught this in an earlier review. > >> + tmp = (S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0); >> + __raw_writel(tmp, S5P_CENTRAL_SEQ_OPTION); >> + } >> >> - if (!soc_is_exynos5250()) { >> + if (!(soc_is_exynos5250() || soc_is_exynos5420())) { >> /* Save Power control register */ >> asm ("mrc p15, 0, %0, c15, c0, 0" >> : "=r" (tmp) : : "cc"); >> @@ -275,6 +348,15 @@ static void exynos_pm_resume(void) >> { >> unsigned long tmp; >> >> + if (soc_is_exynos5420()) { >> + /* Restore the low power flag */ >> + s3c_pm_do_restore(exynos5420_cpustate_save, >> + ARRAY_SIZE(exynos5420_cpustate_save)); >> + >> + __raw_writel(EXYNOS5420_USE_STANDBY_WFI_ALL, >> + S5P_CENTRAL_SEQ_OPTION); >> + } >> + >> /* >> * If PMU failed while entering sleep mode, WFI will be >> * ignored by PMU and then exiting cpu_do_idle(). >> @@ -290,7 +372,7 @@ static void exynos_pm_resume(void) >> /* No need to perform below restore code */ >> goto early_wakeup; >> } >> - if (!soc_is_exynos5250()) { >> + if (!(soc_is_exynos5250() || soc_is_exynos5420())) { >> /* Restore Power control register */ >> tmp = save_arm_register[0]; >> asm volatile ("mcr p15, 0, %0, c15, c0, 0" >> @@ -306,21 +388,40 @@ static void exynos_pm_resume(void) >> >> /* For release retention */ >> >> - __raw_writel((1 << 28), S5P_PAD_RET_MAUDIO_OPTION); >> - __raw_writel((1 << 28), S5P_PAD_RET_GPIO_OPTION); >> - __raw_writel((1 << 28), S5P_PAD_RET_UART_OPTION); >> - __raw_writel((1 << 28), S5P_PAD_RET_MMCA_OPTION); >> - __raw_writel((1 << 28), S5P_PAD_RET_MMCB_OPTION); >> - __raw_writel((1 << 28), S5P_PAD_RET_EBIA_OPTION); >> - __raw_writel((1 << 28), S5P_PAD_RET_EBIB_OPTION); >> + if (soc_is_exynos5420()) { > > Indentation issue. > >> + __raw_writel((1 << 28), EXYNOS5420_PAD_RET_DRAM_OPTION); >> + __raw_writel((1 << 28), EXYNOS5420_PAD_RET_MAUDIO_OPTION); >> + __raw_writel((1 << 28), EXYNOS5420_PAD_RET_JTAG_OPTION); >> + __raw_writel((1 << 28), EXYNOS5420_PAD_RET_GPIO_OPTION); >> + __raw_writel((1 << 28), EXYNOS5420_PAD_RET_UART_OPTION); >> + __raw_writel((1 << 28), EXYNOS5420_PAD_RET_MMCA_OPTION); >> + __raw_writel((1 << 28), EXYNOS5420_PAD_RET_MMCB_OPTION); >> + __raw_writel((1 << 28), EXYNOS5420_PAD_RET_MMCC_OPTION); >> + __raw_writel((1 << 28), EXYNOS5420_PAD_RET_HSI_OPTION); >> + __raw_writel((1 << 28), EXYNOS5420_PAD_RET_EBIA_OPTION); >> + __raw_writel((1 << 28), EXYNOS5420_PAD_RET_EBIB_OPTION); >> + __raw_writel((1 << 28), EXYNOS5420_PAD_RET_SPI_OPTION); >> + __raw_writel((1 << 28), EXYNOS5420_PAD_RET_DRAM_COREBLK_OPTION); >> + } else { >> + __raw_writel((1 << 28), S5P_PAD_RET_MAUDIO_OPTION); >> + __raw_writel((1 << 28), S5P_PAD_RET_GPIO_OPTION); >> + __raw_writel((1 << 28), S5P_PAD_RET_UART_OPTION); >> + __raw_writel((1 << 28), S5P_PAD_RET_MMCA_OPTION); >> + __raw_writel((1 << 28), S5P_PAD_RET_MMCB_OPTION); >> + __raw_writel((1 << 28), S5P_PAD_RET_EBIA_OPTION); >> + __raw_writel((1 << 28), S5P_PAD_RET_EBIB_OPTION); >> + } >> >> if (soc_is_exynos5250()) >> s3c_pm_do_restore(exynos5_sys_save, >> ARRAY_SIZE(exynos5_sys_save)); >> + else if (soc_is_exynos5420()) >> + s3c_pm_do_restore(exynos5420_sys_save, >> + ARRAY_SIZE(exynos5420_sys_save)); >> >> s3c_pm_do_restore_core(exynos_core_save, ARRAY_SIZE(exynos_core_save)); >> >> - if (!soc_is_exynos5250()) { >> + if (!(soc_is_exynos5250() || soc_is_exynos5420())) { >> exynos4_restore_pll(); >> >> #ifdef CONFIG_SMP >> @@ -330,6 +431,18 @@ static void exynos_pm_resume(void) >> >> early_wakeup: >> >> + if (soc_is_exynos5420()) { >> + tmp = __raw_readl(EXYNOS5420_SFR_AXI_CGDIS1); >> + tmp &= ~EXYNOS5420_UFS; >> + __raw_writel(tmp, EXYNOS5420_SFR_AXI_CGDIS1); >> + tmp = __raw_readl(EXYNOS5420_FSYS2_OPTION); >> + tmp &= ~EXYNOS5420_EMULATION; >> + __raw_writel(tmp, EXYNOS5420_FSYS2_OPTION); >> + tmp = __raw_readl(EXYNOS5420_PSGEN_OPTION); >> + tmp &= ~EXYNOS5420_EMULATION; >> + __raw_writel(tmp, EXYNOS5420_PSGEN_OPTION); >> + } >> + >> /* Clear SLEEP mode set in INFORM1 */ >> __raw_writel(0x0, S5P_INFORM1); >> >> diff --git a/arch/arm/mach-exynos/pmu.c b/arch/arm/mach-exynos/pmu.c >> index e39cc75..1449404 100644 >> --- a/arch/arm/mach-exynos/pmu.c >> +++ b/arch/arm/mach-exynos/pmu.c >> @@ -16,6 +16,8 @@ >> #include <mach/regs-clock.h> >> #include <mach/regs-pmu.h> >> >> +#include <asm/cputype.h> >> + >> #include "common.h" >> >> static const struct exynos_pmu_conf *exynos_pmu_config; >> @@ -318,6 +320,151 @@ static const struct exynos_pmu_conf exynos5250_pmu_config[] = { >> { PMU_TABLE_END,}, >> }; >> >> +static struct exynos_pmu_conf exynos5420_pmu_config[] = { > > static const? Will fix. > > Best regards, > Tomasz Abhilash > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |