[PATCH v2 0/7] ARM: EXYNOS: cpuidle: fix AFTR mode on boards with secure firmware enabled

[PATCH v2 0/7] ARM: EXYNOS: cpuidle: fix AFTR mode on boards with secure firmware enabled

[Bartlomiej Zolnierkiewicz]

Hi,

This patch series fixes support for AFTR idle mode on boards with
secure firmware enabled (it also includes fix for register setup on
EXYNOS4x12 SoCs).  It has been tested on Trats2 target but should
also work on (EXYNOS4412 based) Insignal Origen board.

v2:
- synced against next-20140602
- added missing Acked-by-s

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics


Bartlomiej Zolnierkiewicz (5):
  ARM: EXYNOS: add AFTR mode support to firmware do_idle method
  ARM: EXYNOS: PM: replace EXYNOS_BOOT_VECTOR_* macros by static inlines
  ARM: EXYNOS: PM: use c15resume firmware method if secure firmware is
    enabled
  ARM: EXYNOS: PM: fix register setup on EXYNOS4x12 for AFTR mode code
  ARM: EXYNOS: cpuidle: add secure firmware support to AFTR mode code

Kyungmin Park (1):
  arm: firmware: Check firmware is running or not

Tomasz Figa (1):
  ARM: EXYNOS: add support for firmware-assisted c15resume

 arch/arm/common/firmware.c       |  5 +++++
 arch/arm/include/asm/firmware.h  | 14 +++++++++++++-
 arch/arm/mach-exynos/firmware.c  | 18 ++++++++++++++++--
 arch/arm/mach-exynos/pm.c        | 40 ++++++++++++++++++++++++++++++++--------
 drivers/cpuidle/cpuidle-exynos.c |  7 ++++++-
 5 files changed, 72 insertions(+), 12 deletions(-)

-- 
1.8.2.3

[PATCH v2 1/7] arm: firmware: Check firmware is running or not

[Bartlomiej Zolnierkiewicz]

From: Kyungmin Park <kyungmin.park@samsung.com>

To support multi-platform, it needs to know it's running under secure
OS or not.  Sometimes it needs to access physical address by SMC calls.

e.g.,
        if (firmware_run()) {
                addr = physical address;
        } else {
                addr = virtual address;
        }

        call_firmware_ops(read_address, addr, &value);

Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 arch/arm/common/firmware.c      | 5 +++++
 arch/arm/include/asm/firmware.h | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/arch/arm/common/firmware.c b/arch/arm/common/firmware.c
index 27ddccb..e9d9ee5 100644
--- a/arch/arm/common/firmware.c
+++ b/arch/arm/common/firmware.c
@@ -16,3 +16,8 @@
 static const struct firmware_ops default_firmware_ops;
 
 const struct firmware_ops *firmware_ops = &default_firmware_ops;
+
+int firmware_run(void)
+{
+	return firmware_ops != &default_firmware_ops;
+}
diff --git a/arch/arm/include/asm/firmware.h b/arch/arm/include/asm/firmware.h
index 2c9f10d..c72ec47 100644
--- a/arch/arm/include/asm/firmware.h
+++ b/arch/arm/include/asm/firmware.h
@@ -46,6 +46,9 @@ struct firmware_ops {
 /* Global pointer for current firmware_ops structure, can't be NULL. */
 extern const struct firmware_ops *firmware_ops;
 
+/* Check firmware is running */
+extern int firmware_run(void);
+
 /*
  * call_firmware_op(op, ...)
  *
-- 
1.8.2.3

[PATCH v2 2/7] ARM: EXYNOS: add support for firmware-assisted c15resume

[Bartlomiej Zolnierkiewicz]

From: Tomasz Figa <t.figa@samsung.com>

This change is a preparation for adding secure firmware support to
EXYNOS cpuidle driver.

This patch shouldn't cause any functionality changes.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
[bzolnier: splitted out from bigger patch]
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 arch/arm/include/asm/firmware.h | 4 ++++
 arch/arm/mach-exynos/firmware.c | 8 ++++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/arm/include/asm/firmware.h b/arch/arm/include/asm/firmware.h
index c72ec47..70883c7 100644
--- a/arch/arm/include/asm/firmware.h
+++ b/arch/arm/include/asm/firmware.h
@@ -41,6 +41,10 @@ struct firmware_ops {
 	 * Initializes L2 cache
 	 */
 	int (*l2x0_init)(void);
+	/*
+	 * Restores coprocessor 15 registers
+	 */
+	int (*c15resume)(u32 *regs);
 };
 
 /* Global pointer for current firmware_ops structure, can't be NULL. */
diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c
index eb91d23..195b65c 100644
--- a/arch/arm/mach-exynos/firmware.c
+++ b/arch/arm/mach-exynos/firmware.c
@@ -64,10 +64,18 @@ static int exynos_set_cpu_boot_addr(int cpu, unsigned long boot_addr)
 	return 0;
 }
 
+static int exynos_c15resume(u32 *regs)
+{
+	exynos_smc(SMC_CMD_C15RESUME, regs[0], regs[1], 0);
+
+	return 0;
+}
+
 static const struct firmware_ops exynos_firmware_ops = {
 	.do_idle		= exynos_do_idle,
 	.set_cpu_boot_addr	= exynos_set_cpu_boot_addr,
 	.cpu_boot		= exynos_cpu_boot,
+	.c15resume	= exynos_c15resume,
 };
 
 void __init exynos_firmware_init(void)
-- 
1.8.2.3

[PATCH v2 3/7] ARM: EXYNOS: add AFTR mode support to firmware do_idle method

[Bartlomiej Zolnierkiewicz]

On some platforms (i.e. EXYNOS ones) more than one idle mode is
available and we need to distinguish them in firmware do_idle method.

Add mode parameter to do_idle firmware method and AFTR mode support
to EXYNOS do_idle implementation.

This change is a preparation for adding secure firmware support to
EXYNOS cpuidle driver.

This patch shouldn't cause any functionality changes (please note
that do_idle firmware method is unused currently).

Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/include/asm/firmware.h |  7 ++++++-
 arch/arm/mach-exynos/firmware.c | 10 ++++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/firmware.h b/arch/arm/include/asm/firmware.h
index 70883c7..63989c3 100644
--- a/arch/arm/include/asm/firmware.h
+++ b/arch/arm/include/asm/firmware.h
@@ -28,7 +28,7 @@ struct firmware_ops {
 	/*
 	 * Enters CPU idle mode
 	 */
-	int (*do_idle)(void);
+	int (*do_idle)(int mode);
 	/*
 	 * Sets boot address of specified physical CPU
 	 */
@@ -47,6 +47,11 @@ struct firmware_ops {
 	int (*c15resume)(u32 *regs);
 };
 
+enum {
+	FW_DO_IDLE_NORMAL,
+	FW_DO_IDLE_AFTR,
+};
+
 /* Global pointer for current firmware_ops structure, can't be NULL. */
 extern const struct firmware_ops *firmware_ops;
 
diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c
index 195b65c..3a34132 100644
--- a/arch/arm/mach-exynos/firmware.c
+++ b/arch/arm/mach-exynos/firmware.c
@@ -21,9 +21,15 @@
 #include "common.h"
 #include "smc.h"
 
-static int exynos_do_idle(void)
+static int exynos_do_idle(int mode)
 {
-	exynos_smc(SMC_CMD_SLEEP, 0, 0, 0);
+	switch (mode) {
+	case FW_DO_IDLE_AFTR:
+		exynos_smc(SMC_CMD_CPU0AFTR, 0, 0, 0);
+		break;
+	case FW_DO_IDLE_NORMAL:
+		exynos_smc(SMC_CMD_SLEEP, 0, 0, 0);
+	}
 	return 0;
 }
 
-- 
1.8.2.3

[PATCH v2 4/7] ARM: EXYNOS: PM: replace EXYNOS_BOOT_VECTOR_* macros by static inlines

[Bartlomiej Zolnierkiewicz]

Replace EXYNOS_BOOT_VECTOR_ADDR and EXYNOS_BOOT_VECTOR_FLAG macros
by exynos_boot_vector_addr() and exynos_boot_vector_flag() static
inlines.

This patch shouldn't cause any functionality changes.

Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-exynos/pm.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index 87c0d34..cf09383 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -166,12 +166,23 @@ int exynos_cluster_power_state(int cluster)
 			S5P_CORE_LOCAL_PWR_EN);
 }
 
-#define EXYNOS_BOOT_VECTOR_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
-			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
-			(sysram_base_addr + 0x24) : S5P_INFORM0))
-#define EXYNOS_BOOT_VECTOR_FLAG	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
-			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
-			(sysram_base_addr + 0x20) : S5P_INFORM1))
+static inline void __iomem *exynos_boot_vector_addr(void)
+{
+	if (samsung_rev() == EXYNOS4210_REV_1_1)
+		return S5P_INFORM7;
+	else if (samsung_rev() == EXYNOS4210_REV_1_0)
+		return sysram_base_addr + 0x24;
+	return S5P_INFORM0;
+}
+
+static inline void __iomem *exynos_boot_vector_flag(void)
+{
+	if (samsung_rev() == EXYNOS4210_REV_1_1)
+		return S5P_INFORM6;
+	else if (samsung_rev() == EXYNOS4210_REV_1_0)
+		return sysram_base_addr + 0x20;
+	return S5P_INFORM1;
+}
 
 #define S5P_CHECK_AFTR  0xFCBA0D10
 #define S5P_CHECK_SLEEP 0x00000BAD
@@ -184,8 +195,9 @@ static void exynos_set_wakeupmask(long mask)
 
 static void exynos_cpu_set_boot_vector(long flags)
 {
-	__raw_writel(virt_to_phys(exynos_cpu_resume), EXYNOS_BOOT_VECTOR_ADDR);
-	__raw_writel(flags, EXYNOS_BOOT_VECTOR_FLAG);
+	__raw_writel(virt_to_phys(exynos_cpu_resume),
+		     exynos_boot_vector_addr());
+	__raw_writel(flags, exynos_boot_vector_flag());
 }
 
 void exynos_enter_aftr(void)
-- 
1.8.2.3

[PATCH v2 5/7] ARM: EXYNOS: PM: use c15resume firmware method if secure firmware is enabled

[Bartlomiej Zolnierkiewicz]

Use c15resume firmware method instead of accessing the registers
directly in exynos_cpu_restore_register() if secure firmware is
enabled.  This affects both PM resume method and cpuidle AFTR mode.

This patch shouldn't cause any functionality changes on boards that
don't use secure firmware.

Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-exynos/pm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index cf09383..aeff99e 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -26,6 +26,7 @@
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/smp_scu.h>
 #include <asm/suspend.h>
+#include <asm/firmware.h>
 
 #include <plat/pm-common.h>
 #include <plat/pll.h>
@@ -232,6 +233,9 @@ static void exynos_cpu_restore_register(void)
 {
 	unsigned long tmp;
 
+	if (call_firmware_op(c15resume, save_arm_register) == 0)
+		return;
+
 	/* Restore Power control register */
 	tmp = save_arm_register[0];
 
-- 
1.8.2.3

[PATCH v2 6/7] ARM: EXYNOS: PM: fix register setup on EXYNOS4x12 for AFTR mode code

[Bartlomiej Zolnierkiewicz]

Add S5P_CENTRAL_SEQ_OPTION register setup for EXYNOS4x12 to AFTR
mode code.  Without this setup AFTR mode doesn't show any benefit
over WFI one.  When this setup is applied AFTR mode reduces power
consumption by ~12% (as measured on Trats2 board).

This change is a preparation for adding secure firmware support to
EXYNOS cpuidle driver.

Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-exynos/pm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index aeff99e..0fb9a5a 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -456,6 +456,10 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
 	case CPU_PM_ENTER:
 		if (cpu == 0) {
 			exynos_pm_central_suspend();
+			if (soc_is_exynos4212() || soc_is_exynos4412())
+				__raw_writel(S5P_USE_STANDBY_WFI0 |
+					     S5P_USE_STANDBY_WFE0,
+					     S5P_CENTRAL_SEQ_OPTION);
 			exynos_cpu_save_register();
 		}
 		break;
-- 
1.8.2.3

[PATCH v2 7/7] ARM: EXYNOS: cpuidle: add secure firmware support to AFTR mode code

[Bartlomiej Zolnierkiewicz]

* Use do_idle firmware method instead of cpu_do_idle() on boards with
  secure firmware enabled.

* Use sysram_ns_base_addr + 0x24 address for exynos_boot_vector_addr()
  and sysram_ns_base_addr + 0x20 one for exynos_boot_vector_flag() on
  boards with secure firmware enabled.

This patch fixes hang on an attempt to enter AFTR mode for TRATS2
board (which uses EXYNOS4412 SoC with secure firmware enabled).

This patch shouldn't cause any functionality changes on boards that
don't use secure firmware.

Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-exynos/pm.c        | 8 ++++++--
 drivers/cpuidle/cpuidle-exynos.c | 7 ++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index 0fb9a5a..62a0a5e 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -169,7 +169,9 @@ int exynos_cluster_power_state(int cluster)
 
 static inline void __iomem *exynos_boot_vector_addr(void)
 {
-	if (samsung_rev() == EXYNOS4210_REV_1_1)
+	if (firmware_run())
+		return sysram_ns_base_addr + 0x24;
+	else if (samsung_rev() == EXYNOS4210_REV_1_1)
 		return S5P_INFORM7;
 	else if (samsung_rev() == EXYNOS4210_REV_1_0)
 		return sysram_base_addr + 0x24;
@@ -178,7 +180,9 @@ static inline void __iomem *exynos_boot_vector_addr(void)
 
 static inline void __iomem *exynos_boot_vector_flag(void)
 {
-	if (samsung_rev() == EXYNOS4210_REV_1_1)
+	if (firmware_run())
+		return sysram_ns_base_addr + 0x20;
+	else if (samsung_rev() == EXYNOS4210_REV_1_1)
 		return S5P_INFORM6;
 	else if (samsung_rev() == EXYNOS4210_REV_1_0)
 		return sysram_base_addr + 0x20;
diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
index 7c01512..f90a4a0 100644
--- a/drivers/cpuidle/cpuidle-exynos.c
+++ b/drivers/cpuidle/cpuidle-exynos.c
@@ -17,13 +17,18 @@
 #include <asm/proc-fns.h>
 #include <asm/suspend.h>
 #include <asm/cpuidle.h>
+#include <asm/firmware.h>
 
 static void (*exynos_enter_aftr)(void);
 
 static int idle_finisher(unsigned long flags)
 {
 	exynos_enter_aftr();
-	cpu_do_idle();
+	if (firmware_run())
+		/* no need to check the return value on EXYNOS SoCs */
+		call_firmware_op(do_idle, FW_DO_IDLE_AFTR);
+	else
+		cpu_do_idle();
 
 	return 1;
 }
-- 
1.8.2.3

Re: [PATCH v2 1/7] arm: firmware: Check firmware is running or not

[Tomasz Figa]

Hi,

On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> From: Kyungmin Park <kyungmin.park@samsung.com>
> 
> To support multi-platform, it needs to know it's running under secure
> OS or not.  Sometimes it needs to access physical address by SMC calls.
> 
> e.g.,
>         if (firmware_run()) {
>                 addr = physical address;
>         } else {
>                 addr = virtual address;
>         }
> 
>         call_firmware_ops(read_address, addr, &value);

Hmm, I don't understand the code above. It first asks whether the
firmware is available and then calls a firmware operation anyway
(assuming that firmware is available regardless of the check above)...

I don't like the idea of this function, because we have designed the
firmware API to not require this kind of checks. Instead, you just call
whatever firmware operation you need and if it returns -ENOSYS you need
to fallback to legacy (firmware-less) way of doing it.

Could you provide your use case for which this doesn't work?

Best regards,
Tomasz

Re: [PATCH v2 2/7] ARM: EXYNOS: add support for firmware-assisted c15resume

[Tomasz Figa]

Hi,

On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> From: Tomasz Figa <t.figa@samsung.com>
> 
> This change is a preparation for adding secure firmware support to
> EXYNOS cpuidle driver.
> 
> This patch shouldn't cause any functionality changes.
> 
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> [bzolnier: splitted out from bigger patch]
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
>  arch/arm/include/asm/firmware.h | 4 ++++
>  arch/arm/mach-exynos/firmware.c | 8 ++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/arch/arm/include/asm/firmware.h b/arch/arm/include/asm/firmware.h
> index c72ec47..70883c7 100644
> --- a/arch/arm/include/asm/firmware.h
> +++ b/arch/arm/include/asm/firmware.h
> @@ -41,6 +41,10 @@ struct firmware_ops {
>  	 * Initializes L2 cache
>  	 */
>  	int (*l2x0_init)(void);
> +	/*
> +	 * Restores coprocessor 15 registers
> +	 */
> +	int (*c15resume)(u32 *regs);

Well, this was just a kind of internal hack, so I'm not sure this is
suitable as a generic firmware operation in mainline.

I'd rather implement this as a part of a wider suspend and resume
firmware operations, where suspend would save values of cp15 registers
and resume would restore them.

Best regards,
Tomasz

Re: [PATCH v2 3/7] ARM: EXYNOS: add AFTR mode support to firmware do_idle method

[Tomasz Figa]

Hi,

On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> On some platforms (i.e. EXYNOS ones) more than one idle mode is
> available and we need to distinguish them in firmware do_idle method.
> 
> Add mode parameter to do_idle firmware method and AFTR mode support
> to EXYNOS do_idle implementation.
> 
> This change is a preparation for adding secure firmware support to
> EXYNOS cpuidle driver.
> 
> This patch shouldn't cause any functionality changes (please note
> that do_idle firmware method is unused currently).
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/include/asm/firmware.h |  7 ++++++-
>  arch/arm/mach-exynos/firmware.c | 10 ++++++++--
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/firmware.h b/arch/arm/include/asm/firmware.h
> index 70883c7..63989c3 100644
> --- a/arch/arm/include/asm/firmware.h
> +++ b/arch/arm/include/asm/firmware.h
> @@ -28,7 +28,7 @@ struct firmware_ops {
>  	/*
>  	 * Enters CPU idle mode
>  	 */
> -	int (*do_idle)(void);
> +	int (*do_idle)(int mode);
>  	/*
>  	 * Sets boot address of specified physical CPU
>  	 */
> @@ -47,6 +47,11 @@ struct firmware_ops {
>  	int (*c15resume)(u32 *regs);
>  };
>  
> +enum {

What about making this enum named and using it instead of int for mode
argument of do_idle operation?

> +	FW_DO_IDLE_NORMAL,

IMHO this should be called FW_DO_IDLE_SLEEP, as this is how it is used
on Exynos4x12.

OR (which is probably a better idea)

This is probably too Exynos-specific, so mode argument could be kept int
or maybe unsigned long to make it more universal and this enum defined
in an Exynos-specific header instead.

Best regards,
Tomasz

Re: [PATCH v2 4/7] ARM: EXYNOS: PM: replace EXYNOS_BOOT_VECTOR_* macros by static inlines

[Tomasz Figa]

Hi,

On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> Replace EXYNOS_BOOT_VECTOR_ADDR and EXYNOS_BOOT_VECTOR_FLAG macros
> by exynos_boot_vector_addr() and exynos_boot_vector_flag() static
> inlines.
> 
> This patch shouldn't cause any functionality changes.
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/mach-exynos/pm.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)

> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index 87c0d34..cf09383 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -166,12 +166,23 @@ int exynos_cluster_power_state(int cluster)
>  			S5P_CORE_LOCAL_PWR_EN);
>  }
>  
> -#define EXYNOS_BOOT_VECTOR_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> -			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> -			(sysram_base_addr + 0x24) : S5P_INFORM0))
> -#define EXYNOS_BOOT_VECTOR_FLAG	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> -			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> -			(sysram_base_addr + 0x20) : S5P_INFORM1))
> +static inline void __iomem *exynos_boot_vector_addr(void)
> +{
> +	if (samsung_rev() == EXYNOS4210_REV_1_1)
> +		return S5P_INFORM7;
> +	else if (samsung_rev() == EXYNOS4210_REV_1_0)
> +		return sysram_base_addr + 0x24;
> +	return S5P_INFORM0;

I know this is not strictly related to this patch, but isn't a check
whether the SoC is Exynos4210 also needed, before comparing the revision
with Exynos4210-specific values?

Otherwise looks good.

Best regards,
Tomasz

Re: [PATCH v2 1/7] arm: firmware: Check firmware is running or not

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday, June 02, 2014 02:51:14 PM Tomasz Figa wrote:
> Hi,
> 
> On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> > From: Kyungmin Park <kyungmin.park@samsung.com>
> > 
> > To support multi-platform, it needs to know it's running under secure
> > OS or not.  Sometimes it needs to access physical address by SMC calls.
> > 
> > e.g.,
> >         if (firmware_run()) {
> >                 addr = physical address;
> >         } else {
> >                 addr = virtual address;
> >         }
> > 
> >         call_firmware_ops(read_address, addr, &value);
> 
> Hmm, I don't understand the code above. It first asks whether the
> firmware is available and then calls a firmware operation anyway
> (assuming that firmware is available regardless of the check above)...
> 
> I don't like the idea of this function, because we have designed the
> firmware API to not require this kind of checks. Instead, you just call
> whatever firmware operation you need and if it returns -ENOSYS you need
> to fallback to legacy (firmware-less) way of doing it.
> 
> Could you provide your use case for which this doesn't work?

Please take a look at patch #7.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH v2 5/7] ARM: EXYNOS: PM: use c15resume firmware method if secure firmware is enabled

[Tomasz Figa]

Hi,

On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> Use c15resume firmware method instead of accessing the registers
> directly in exynos_cpu_restore_register() if secure firmware is
> enabled.  This affects both PM resume method and cpuidle AFTR mode.
> 
> This patch shouldn't cause any functionality changes on boards that
> don't use secure firmware.
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/mach-exynos/pm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index cf09383..aeff99e 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -26,6 +26,7 @@
>  #include <asm/hardware/cache-l2x0.h>
>  #include <asm/smp_scu.h>
>  #include <asm/suspend.h>
> +#include <asm/firmware.h>
>  
>  #include <plat/pm-common.h>
>  #include <plat/pll.h>
> @@ -232,6 +233,9 @@ static void exynos_cpu_restore_register(void)
>  {
>  	unsigned long tmp;
>  
> +	if (call_firmware_op(c15resume, save_arm_register) == 0)
> +		return;
> +

As I mentioned in my comments to patch 2/7, instead of introducing
heavily SoC-specific operations, I'd rather add more general suspend and
resume firmware operations which would take care of both saving and
restoring those registers.

Best regards,
Tomasz

Re: [PATCH v2 6/7] ARM: EXYNOS: PM: fix register setup on EXYNOS4x12 for AFTR mode code

[Tomasz Figa]

Hi,

On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> Add S5P_CENTRAL_SEQ_OPTION register setup for EXYNOS4x12 to AFTR
> mode code.  Without this setup AFTR mode doesn't show any benefit
> over WFI one.  When this setup is applied AFTR mode reduces power
> consumption by ~12% (as measured on Trats2 board).
> 
> This change is a preparation for adding secure firmware support to
> EXYNOS cpuidle driver.
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/mach-exynos/pm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index aeff99e..0fb9a5a 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -456,6 +456,10 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
>  	case CPU_PM_ENTER:
>  		if (cpu == 0) {
>  			exynos_pm_central_suspend();
> +			if (soc_is_exynos4212() || soc_is_exynos4412())
> +				__raw_writel(S5P_USE_STANDBY_WFI0 |
> +					     S5P_USE_STANDBY_WFE0,
> +					     S5P_CENTRAL_SEQ_OPTION);

I wonder whether this isn't required on any Exynos SoC in general, as
this mask decides which STANDBY_WFI/WFE signals are considered before
entering the lower power state.

Also you should check the behavior with Krzysztof's patch adding support
for delayed reset assertion, which should cause WFI/WFE signals of CPUs
powered down to be kept asserted.

Best regards,
Tomasz

Re: [PATCH v2 7/7] ARM: EXYNOS: cpuidle: add secure firmware support to AFTR mode code

[Tomasz Figa]

Hi,

On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> * Use do_idle firmware method instead of cpu_do_idle() on boards with
>   secure firmware enabled.
> 
> * Use sysram_ns_base_addr + 0x24 address for exynos_boot_vector_addr()
>   and sysram_ns_base_addr + 0x20 one for exynos_boot_vector_flag() on
>   boards with secure firmware enabled.
> 
> This patch fixes hang on an attempt to enter AFTR mode for TRATS2
> board (which uses EXYNOS4412 SoC with secure firmware enabled).
> 
> This patch shouldn't cause any functionality changes on boards that
> don't use secure firmware.
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/mach-exynos/pm.c        | 8 ++++++--
>  drivers/cpuidle/cpuidle-exynos.c | 7 ++++++-
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index 0fb9a5a..62a0a5e 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -169,7 +169,9 @@ int exynos_cluster_power_state(int cluster)
>  
>  static inline void __iomem *exynos_boot_vector_addr(void)
>  {
> -	if (samsung_rev() == EXYNOS4210_REV_1_1)
> +	if (firmware_run())
> +		return sysram_ns_base_addr + 0x24;
> +	else if (samsung_rev() == EXYNOS4210_REV_1_1)

Aha, so this is the use case for the function added by patch 1/7.

Well, I don't see the need to do it this way and complicate the API. As
I mentioned in my comments to patches 2/7 and 5/7, more general firmware
operations should be taking care of setting those registers to
appropriate values and so there shouldn't be any need to use them
directly outside the implementation of firmware ops.

[snip]

>  static int idle_finisher(unsigned long flags)
>  {
>  	exynos_enter_aftr();
> -	cpu_do_idle();
> +	if (firmware_run())
> +		/* no need to check the return value on EXYNOS SoCs */
> +		call_firmware_op(do_idle, FW_DO_IDLE_AFTR);
> +	else
> +		cpu_do_idle();

This could be done just by

	if (call_firmware_op(do_idle, FW_DO_IDLE_AFTR) == -ENOSYS)
		cpu_do_idle();

which is 3 lines less than with a function that is suppose to simplify
the code.

Best regards,
Tomasz

Re: [PATCH v2 4/7] ARM: EXYNOS: PM: replace EXYNOS_BOOT_VECTOR_* macros by static inlines

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday, June 02, 2014 03:05:40 PM Tomasz Figa wrote:
> Hi,
> 
> On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> > Replace EXYNOS_BOOT_VECTOR_ADDR and EXYNOS_BOOT_VECTOR_FLAG macros
> > by exynos_boot_vector_addr() and exynos_boot_vector_flag() static
> > inlines.
> > 
> > This patch shouldn't cause any functionality changes.
> > 
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  arch/arm/mach-exynos/pm.c | 28 ++++++++++++++++++++--------
> >  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> > diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> > index 87c0d34..cf09383 100644
> > --- a/arch/arm/mach-exynos/pm.c
> > +++ b/arch/arm/mach-exynos/pm.c
> > @@ -166,12 +166,23 @@ int exynos_cluster_power_state(int cluster)
> >  			S5P_CORE_LOCAL_PWR_EN);
> >  }
> >  
> > -#define EXYNOS_BOOT_VECTOR_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> > -			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> > -			(sysram_base_addr + 0x24) : S5P_INFORM0))
> > -#define EXYNOS_BOOT_VECTOR_FLAG	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> > -			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> > -			(sysram_base_addr + 0x20) : S5P_INFORM1))
> > +static inline void __iomem *exynos_boot_vector_addr(void)
> > +{
> > +	if (samsung_rev() == EXYNOS4210_REV_1_1)
> > +		return S5P_INFORM7;
> > +	else if (samsung_rev() == EXYNOS4210_REV_1_0)
> > +		return sysram_base_addr + 0x24;
> > +	return S5P_INFORM0;
> 
> I know this is not strictly related to this patch, but isn't a check
> whether the SoC is Exynos4210 also needed, before comparing the revision
> with Exynos4210-specific values?

Yes, it is needed but other SoCs need to be verified that they do not
rely on a buggy code (to not introduce regressions).  This is of course
outside a scope of the current patchset.

> Otherwise looks good.
> 
> Best regards,
> Tomasz

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH v2 7/7] ARM: EXYNOS: cpuidle: add secure firmware support to AFTR mode code

[Bartlomiej Zolnierkiewicz]

On Monday, June 02, 2014 03:15:07 PM Tomasz Figa wrote:
> Hi,
> 
> On 02.06.2014 14:35, Bartlomiej Zolnierkiewicz wrote:
> > * Use do_idle firmware method instead of cpu_do_idle() on boards with
> >   secure firmware enabled.
> > 
> > * Use sysram_ns_base_addr + 0x24 address for exynos_boot_vector_addr()
> >   and sysram_ns_base_addr + 0x20 one for exynos_boot_vector_flag() on
> >   boards with secure firmware enabled.
> > 
> > This patch fixes hang on an attempt to enter AFTR mode for TRATS2
> > board (which uses EXYNOS4412 SoC with secure firmware enabled).
> > 
> > This patch shouldn't cause any functionality changes on boards that
> > don't use secure firmware.
> > 
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  arch/arm/mach-exynos/pm.c        | 8 ++++++--
> >  drivers/cpuidle/cpuidle-exynos.c | 7 ++++++-
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> > index 0fb9a5a..62a0a5e 100644
> > --- a/arch/arm/mach-exynos/pm.c
> > +++ b/arch/arm/mach-exynos/pm.c
> > @@ -169,7 +169,9 @@ int exynos_cluster_power_state(int cluster)
> >  
> >  static inline void __iomem *exynos_boot_vector_addr(void)
> >  {
> > -	if (samsung_rev() == EXYNOS4210_REV_1_1)
> > +	if (firmware_run())
> > +		return sysram_ns_base_addr + 0x24;
> > +	else if (samsung_rev() == EXYNOS4210_REV_1_1)
> 
> Aha, so this is the use case for the function added by patch 1/7.
> 
> Well, I don't see the need to do it this way and complicate the API. As
> I mentioned in my comments to patches 2/7 and 5/7, more general firmware
> operations should be taking care of setting those registers to
> appropriate values and so there shouldn't be any need to use them
> directly outside the implementation of firmware ops.

More general firmware operations would handle the secure firmware case
fine but how would you like to handle a fallback case given that you
cannot use samsung_rev() etc. in drivers/cpuidle/cpuidle-exynos.c?

> [snip]
> 
> >  static int idle_finisher(unsigned long flags)
> >  {
> >  	exynos_enter_aftr();
> > -	cpu_do_idle();
> > +	if (firmware_run())
> > +		/* no need to check the return value on EXYNOS SoCs */
> > +		call_firmware_op(do_idle, FW_DO_IDLE_AFTR);
> > +	else
> > +		cpu_do_idle();
> 
> This could be done just by
> 
> 	if (call_firmware_op(do_idle, FW_DO_IDLE_AFTR) == -ENOSYS)
> 		cpu_do_idle();
> 
> which is 3 lines less than with a function that is suppose to simplify
> the code.

OK.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH v2 4/7] ARM: EXYNOS: PM: replace EXYNOS_BOOT_VECTOR_* macros by static inlines

[Daniel Lezcano]

On 06/02/2014 02:35 PM, Bartlomiej Zolnierkiewicz wrote:
> Replace EXYNOS_BOOT_VECTOR_ADDR and EXYNOS_BOOT_VECTOR_FLAG macros
> by exynos_boot_vector_addr() and exynos_boot_vector_flag() static
> inlines.
>
> This patch shouldn't cause any functionality changes.
>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---
>   arch/arm/mach-exynos/pm.c | 28 ++++++++++++++++++++--------
>   1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index 87c0d34..cf09383 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -166,12 +166,23 @@ int exynos_cluster_power_state(int cluster)
>   			S5P_CORE_LOCAL_PWR_EN);
>   }
>
> -#define EXYNOS_BOOT_VECTOR_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> -			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> -			(sysram_base_addr + 0x24) : S5P_INFORM0))
> -#define EXYNOS_BOOT_VECTOR_FLAG	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> -			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> -			(sysram_base_addr + 0x20) : S5P_INFORM1))
> +static inline void __iomem *exynos_boot_vector_addr(void)
> +{
> +	if (samsung_rev() == EXYNOS4210_REV_1_1)
> +		return S5P_INFORM7;
> +	else if (samsung_rev() == EXYNOS4210_REV_1_0)
> +		return sysram_base_addr + 0x24;
> +	return S5P_INFORM0;
> +}
> +
> +static inline void __iomem *exynos_boot_vector_flag(void)
> +{
> +	if (samsung_rev() == EXYNOS4210_REV_1_1)
> +		return S5P_INFORM6;
> +	else if (samsung_rev() == EXYNOS4210_REV_1_0)
> +		return sysram_base_addr + 0x20;
> +	return S5P_INFORM1;
> +}
>
>   #define S5P_CHECK_AFTR  0xFCBA0D10
>   #define S5P_CHECK_SLEEP 0x00000BAD
> @@ -184,8 +195,9 @@ static void exynos_set_wakeupmask(long mask)
>
>   static void exynos_cpu_set_boot_vector(long flags)
>   {
> -	__raw_writel(virt_to_phys(exynos_cpu_resume), EXYNOS_BOOT_VECTOR_ADDR);
> -	__raw_writel(flags, EXYNOS_BOOT_VECTOR_FLAG);
> +	__raw_writel(virt_to_phys(exynos_cpu_resume),
> +		     exynos_boot_vector_addr());
> +	__raw_writel(flags, exynos_boot_vector_flag());
>   }
>
>   void exynos_enter_aftr(void)
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 7/7] ARM: EXYNOS: cpuidle: add secure firmware support to AFTR mode code

[Daniel Lezcano]

On 06/02/2014 02:35 PM, Bartlomiej Zolnierkiewicz wrote:
> * Use do_idle firmware method instead of cpu_do_idle() on boards with
>    secure firmware enabled.
>
> * Use sysram_ns_base_addr + 0x24 address for exynos_boot_vector_addr()
>    and sysram_ns_base_addr + 0x20 one for exynos_boot_vector_flag() on
>    boards with secure firmware enabled.
>
> This patch fixes hang on an attempt to enter AFTR mode for TRATS2
> board (which uses EXYNOS4412 SoC with secure firmware enabled).
>
> This patch shouldn't cause any functionality changes on boards that
> don't use secure firmware.
>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>   arch/arm/mach-exynos/pm.c        | 8 ++++++--
>   drivers/cpuidle/cpuidle-exynos.c | 7 ++++++-
>   2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index 0fb9a5a..62a0a5e 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -169,7 +169,9 @@ int exynos_cluster_power_state(int cluster)
>
>   static inline void __iomem *exynos_boot_vector_addr(void)
>   {
> -	if (samsung_rev() == EXYNOS4210_REV_1_1)
> +	if (firmware_run())
> +		return sysram_ns_base_addr + 0x24;
> +	else if (samsung_rev() == EXYNOS4210_REV_1_1)
>   		return S5P_INFORM7;
>   	else if (samsung_rev() == EXYNOS4210_REV_1_0)
>   		return sysram_base_addr + 0x24;
> @@ -178,7 +180,9 @@ static inline void __iomem *exynos_boot_vector_addr(void)
>
>   static inline void __iomem *exynos_boot_vector_flag(void)
>   {
> -	if (samsung_rev() == EXYNOS4210_REV_1_1)
> +	if (firmware_run())
> +		return sysram_ns_base_addr + 0x20;
> +	else if (samsung_rev() == EXYNOS4210_REV_1_1)
>   		return S5P_INFORM6;
>   	else if (samsung_rev() == EXYNOS4210_REV_1_0)
>   		return sysram_base_addr + 0x20;
> diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
> index 7c01512..f90a4a0 100644
> --- a/drivers/cpuidle/cpuidle-exynos.c
> +++ b/drivers/cpuidle/cpuidle-exynos.c
> @@ -17,13 +17,18 @@
>   #include <asm/proc-fns.h>
>   #include <asm/suspend.h>
>   #include <asm/cpuidle.h>
> +#include <asm/firmware.h>
>
>   static void (*exynos_enter_aftr)(void);
>
>   static int idle_finisher(unsigned long flags)
>   {
>   	exynos_enter_aftr();
> -	cpu_do_idle();
> +	if (firmware_run())
> +		/* no need to check the return value on EXYNOS SoCs */
> +		call_firmware_op(do_idle, FW_DO_IDLE_AFTR);
> +	else
> +		cpu_do_idle();

Why not move this code into the exynos_enter_aftr() function, so 
preventing more dependency ?


>   	return 1;
>   }
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog