[PATCH v2 0/7] ARM: EXYNOS: cpuidle: fix AFTR mode on boards with secure firmware enabled
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(-)
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, ...) *
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
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
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)
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
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; }
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
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)
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
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
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)
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];
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
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;
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
* 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; }
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
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
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; }
linaro-kernel@lists.linaro.org
-
Bartlomiej Zolnierkiewicz -
Daniel Lezcano -
Tomasz Figa