Hi Vikas, Pankaj,
On Wednesday 11 of December 2013 16:25:15 Vikas Sajjan wrote:
Adds initial PMU support for Exynos5260
Following are the changes done
Added initial PMU support for exynos5260
Added exynos5260_iodesc for mapping 5260 specific SFRs. We modified
exynos5_map_io so that in case of exynos5260 only exynos5260_iodesc can be initialized.
- Added new macros for WAKEUP MASK for 5260, and modified exynos_pm_drvinit
accrodingly.
Change-Id: Ie585d47a499c17813cfe0e5a668072ca7b13ffb5
This line should be removed.
Signed-off-by: Pankaj Dubey pankaj.dubey@samsung.com Signed-off-by: Vikas Sajjan vikas.sajjan@samsung.com
arch/arm/mach-exynos/common.c | 89 ++++++++++- arch/arm/mach-exynos/common.h | 5 + arch/arm/mach-exynos/include/mach/map.h | 17 +++ arch/arm/mach-exynos/include/mach/regs-pmu.h | 221 +++++++++++++++++++++++++++ arch/arm/mach-exynos/pm.c | 33 +++- arch/arm/mach-exynos/pmu.c | 140 +++++++++++++++++ arch/arm/plat-samsung/include/plat/map-s5p.h | 14 ++ 7 files changed, 508 insertions(+), 11 deletions(-)
[snip]
- }, {
.virtual = (unsigned long)S5P_VA_SROMC,.pfn = __phys_to_pfn(EXYNOS5260_PA_SROMC),.length = SZ_4K,.type = MT_DEVICE,- }, {
.virtual = (unsigned long)S5P_VA_SYSRAM,.pfn = __phys_to_pfn(EXYNOS5_PA_SYSRAM),.length = SZ_4K,.type = MT_DEVICE,- }, { .virtual = (unsigned long)S5P_VA_SYSRAM_NS, .pfn = __phys_to_pfn(EXYNOS5260_PA_SYSRAM_NS), .length = SZ_4K, .type = MT_DEVICE,
- }, {
.virtual = (unsigned long)S5P_VA_PMU,.pfn = __phys_to_pfn(EXYNOS5260_PA_PMU),.length = SZ_64K,.type = MT_DEVICE,
Do you really need all these static mappings above? Why can't you create a mapping dynamically?
};
[snip]
struct bus_type exynos_subsys = { diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h index ff9b6a9..e2cabfe 100644 --- a/arch/arm/mach-exynos/common.h +++ b/arch/arm/mach-exynos/common.h @@ -47,6 +47,11 @@ enum sys_powerdown { NUM_SYS_POWERDOWN, }; +enum running_cpu {
- KFC,
- ARM,
+};
This isn't too meaningful. You should write a comment saying how this enum is used and describing its values.
Also the name is too generic. It should be prefixed with exynos5260_ probably.
extern unsigned long l2x0_regs_phys; struct exynos_pmu_conf { void __iomem *reg; diff --git a/arch/arm/mach-exynos/include/mach/map.h b/arch/arm/mach-exynos/include/mach/map.h index bd6fa02..cc190b9 100644 --- a/arch/arm/mach-exynos/include/mach/map.h +++ b/arch/arm/mach-exynos/include/mach/map.h @@ -31,6 +31,23 @@ #define EXYNOS5250_PA_SYSRAM_NS 0x0204F000 #define EXYNOS5260_PA_SYSRAM_NS 0x02073000 +#define EXYNOS5260_PA_PMU 0x10D50000 +#define EXYNOS5260_PA_SROMC 0x12180000 +#define EXYNOS5260_PA_PWM 0x12D90000
+#define EXYNOS5260_PA_SYS_PERI 0x10220000 +#define EXYNOS5260_PA_SYS_MIF 0x10D00000 +#define EXYNOS5260_PA_SYS_MFC 0x110A0000 +#define EXYNOS5260_PA_SYS_KFC 0x10710000 +#define EXYNOS5260_PA_SYS_ISP 0x133E0000 +#define EXYNOS5260_PA_SYS_GSCL 0x13F20000 +#define EXYNOS5260_PA_SYS_G3D 0x11850000 +#define EXYNOS5260_PA_SYS_G2D 0x10A20000 +#define EXYNOS5260_PA_SYS_FSYS 0x122F0000 +#define EXYNOS5260_PA_SYS_EGL 0x10610000 +#define EXYNOS5260_PA_SYS_DISP 0x14540000 +#define EXYNOS5260_PA_SYS_AUD 0x128F0000
Do you really need all the static addresses above?
#define EXYNOS_PA_CHIPID 0x10000000 #define EXYNOS4_PA_SYSCON 0x10010000 diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 09ae29a..ac53f85 100644 --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h @@ -125,6 +125,25 @@ #define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084) #define S5P_ARM_CORE1_OPTION S5P_PMUREG(0x2088)
[snip]
+#define EXYNOS5260_CORE_LOCAL_PWR_EN 0xf +#define EXYNOS5260_CPUS_PER_CLUSTER 4 +#define EXYNOS5260_USE_DELAYED_RESET_ASSERTION (1 << 12)
+#define EXYNOS5260_WAKEUP_STAT2 S5P_PMUREG(0x0604) +#define EXYNOS5260_WAKEUP_STAT3 S5P_PMUREG(0x0608) +#define EXYNOS5260_EINT_WAKEUP_MASK S5P_PMUREG(0x060C) +#define EXYNOS5260_WAKEUP_MASK S5P_PMUREG(0x0610) +#define EXYNOS5260_WAKEUP_MASK2 S5P_PMUREG(0x0614) +#define EXYNOS5260_WAKEUP_MASK3 S5P_PMUREG(0x0618)
+/* Exynos5260 specific PMU SYS_PWR_REGs */ +#define EXYNOS5260_A15_EGL0_SYS_PWR_REG S5P_PMUREG(0x1000)
CodingStyle: There should be just one space between #define and definiton name. The same for a lot of definitons below.
+#define EXYNOS5260_DIS_IRQ_A15_EGL0_LOCAL_SYS_PWR_REG S5P_PMUREG(0x1004) +#define EXYNOS5260_DIS_IRQ_A15_EGL0_CNTRL_SYS_PWR_REG S5P_PMUREG(0x1008) +#define EXYNOS5260_DIS_IRQ_A15_EGL0_EGLSEQ_SYS_PWR_REG S5P_PMUREG(0x100C) +#define EXYNOS5260_A15_EGL1_SYS_PWR_REG S5P_PMUREG(0x1010)
[snip]
+/* CENTRAL_SEQ_OPTION */ +#define EXYNOS5260_ARM_USE_STANDBY_WFI0 (1 << 16) +#define EXYNOS5260_ARM_USE_STANDBY_WFI1 (1 << 17) +#define EXYNOS5260_KFC_USE_STANDBY_WFI0 (1 << 20) +#define EXYNOS5260_KFC_USE_STANDBY_WFI1 (1 << 21) +#define EXYNOS5260_KFC_USE_STANDBY_WFI2 (1 << 22) +#define EXYNOS5260_KFC_USE_STANDBY_WFI3 (1 << 23) +#define EXYNOS5260_ARM_USE_STANDBY_WFE0 (1 << 24) +#define EXYNOS5260_ARM_USE_STANDBY_WFE1 (1 << 25) +#define EXYNOS5260_KFC_USE_STANDBY_WFE0 (1 << 28) +#define EXYNOS5260_KFC_USE_STANDBY_WFE1 (1 << 29) +#define EXYNOS5260_KFC_USE_STANDBY_WFE2 (1 << 30) +#define EXYNOS5260_KFC_USE_STANDBY_WFE3 (1 << 31)
+#define EXYNOS5260_USE_STANDBY_WFI_ALL (EXYNOS5260_ARM_USE_STANDBY_WFI0 \
| EXYNOS5260_ARM_USE_STANDBY_WFI1 \| EXYNOS5260_KFC_USE_STANDBY_WFI0 \| EXYNOS5260_KFC_USE_STANDBY_WFI1 \| EXYNOS5260_KFC_USE_STANDBY_WFI2 \| EXYNOS5260_KFC_USE_STANDBY_WFI3)
This is not a definition of registers, so should be present in the file using it as a convenience macro.
#endif /* __ASM_ARCH_REGS_PMU_H */ diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c index 78a22bf..c6def953 100644 --- a/arch/arm/mach-exynos/pm.c +++ b/arch/arm/mach-exynos/pm.c @@ -58,7 +58,7 @@ static int exynos_cpu_suspend(unsigned long arg) outer_flush_all(); #endif
[snip]
/* Setting SEQ_OPTION register */
- if (soc_is_exynos5250()) {
tmp = (S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0);__raw_writel(tmp, S5P_CENTRAL_SEQ_OPTION);- } else if (soc_is_exynos5260()) {
cluster_id = (read_cpuid(CPUID_MPIDR) >> 8) & 0xf;if (!cluster_id)__raw_writel(EXYNOS5260_ARM_USE_STANDBY_WFI0,S5P_CENTRAL_SEQ_OPTION);else__raw_writel(EXYNOS5260_KFC_USE_STANDBY_WFI0,S5P_CENTRAL_SEQ_OPTION);
Hmm, this seems strange. Shouldn't just a single particular CPU (CPU0 most likely) be allowed to enter standby such as the code for other Exynos SoCs is doing?
- 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_exynos5260()) {
Is this condition really correct? It doesn't make much sense.
/* Save Power control register */ asm ("mrc p15, 0, %0, c15, c0, 0" : "=r" (tmp) : : "cc");@@ -174,7 +191,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_exynos5260()) {
Ditto.
/* Restore Power control register */ tmp = save_arm_register[0]; asm volatile ("mcr p15, 0, %0, c15, c0, 0"diff --git a/arch/arm/mach-exynos/pmu.c b/arch/arm/mach-exynos/pmu.c index 97d6885..c828d07 100644 --- a/arch/arm/mach-exynos/pmu.c +++ b/arch/arm/mach-exynos/pmu.c @@ -317,6 +317,99 @@ static struct exynos_pmu_conf exynos5250_pmu_config[] = { { PMU_TABLE_END,}, };
[snip]
+static void exynos5260_reset_assert_ctrl(bool on, enum running_cpu cluster) +{
- unsigned int i;
- unsigned int option;
- unsigned int cpu_s, cpu_f;
- if (cluster == KFC) {
cpu_s = EXYNOS5260_CPUS_PER_CLUSTER;cpu_f = cpu_s + EXYNOS5260_CPUS_PER_CLUSTER;- } else {
cpu_s = 0;cpu_f = 2;
Hmm, a magic number. I wonder what it means. It doesn't seem to be the Answer to the Ultimate Question of Life, The Universe, and Everything, though. ;)
[1] http://en.wikipedia.org/wiki/Phrases_from_The_Hitchhiker%27s_Guide_to_the_Ga...
- }
- for (i = cpu_s; i < cpu_f; i++) {
option = __raw_readl(EXYNOS_ARM_CORE_OPTION(i));option = on ?(option | EXYNOS5260_USE_DELAYED_RESET_ASSERTION) :(option & ~EXYNOS5260_USE_DELAYED_RESET_ASSERTION);
Readability of this is quite poor. I'd recommend rewriting this to a simple
if (on) option |= EXYNOS5260_USE_DELAYED_RESET_ASSERTION; else option &= ~EXYNOS5260_USE_DELAYED_RESET_ASSERTION;
__raw_writel(option, EXYNOS_ARM_CORE_OPTION(i));- }
+}
static int __init exynos_pmu_init(void) { unsigned int value; @@ -415,6 +532,29 @@ static int __init exynos_pmu_init(void) exynos_pmu_config = exynos5250_pmu_config; pr_info("EXYNOS5250 PMU Initialize\n");
- } else if (soc_is_exynos5260()) {
/* Enable USE_STANDBY_WFI for all CORE */__raw_writel(EXYNOS5260_USE_STANDBY_WFI_ALL,S5P_CENTRAL_SEQ_OPTION);/** Set PSHOLD port for output high*/value = __raw_readl(EXYNOS_PS_HOLD_CONTROL);value |= EXYNOS_PS_HOLD_OUTPUT_HIGH;__raw_writel(value, EXYNOS_PS_HOLD_CONTROL);/** Enable signal for PSHOLD port*/value = __raw_readl(EXYNOS_PS_HOLD_CONTROL);value |= EXYNOS_PS_HOLD_EN;__raw_writel(value, EXYNOS_PS_HOLD_CONTROL);
Hmm, do you really need this? What about boards with different polarity of power hold signal in used PMIC? I believe this should be set correctly by the bootloader and never changed later, except in power off callback.
Best regards, Tomasz