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
------------------------------

1) Added initial PMU support for exynos5260

2) 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.

3) 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 <redacted>
Signed-off-by: Vikas Sajjan <redacted>
---
 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]

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.

Do you really need all the static addresses above?

[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.

[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.

Ditto.

[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's_Guide_to_the_Galaxy#Answer_to_the_Ultimate_Question_of_Life.2C_the_Universe.2C_and_Everything_.2842.29

+	}
+
+	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;

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