[U-Boot] [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs

[U-Boot] [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

A proper stack is required to safely use C code in psci_arch_cpu_entry.

Fixes: 486daaa618e1 ("arm: psci: add a weak function psci_arch_cpu_entry")
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 arch/arm/cpu/armv7/psci.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/cpu/armv7/psci.S b/arch/arm/cpu/armv7/psci.S
index 08b5088675..983cd90442 100644
--- a/arch/arm/cpu/armv7/psci.S
+++ b/arch/arm/cpu/armv7/psci.S
@@ -331,6 +331,8 @@ ENTRY(psci_cpu_entry)
 
 	bl	_nonsec_init
 
+	bl	psci_stack_setup
+
 	bl	psci_arch_cpu_entry
 
 	bl	psci_get_cpu_id			@ CPU ID => r0
-- 
2.17.1

[U-Boot] [PATCH v1 2/5] imx: mx7: psci: use C code exclusively

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

There is no need for assembly in the platform specific part of
the PSCI implementation.

Note that this does not make it a complete PSCI 1.0 implementation
yet but aids to do so in upcoming patches.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 arch/arm/mach-imx/mx7/Makefile   |  5 +--
 arch/arm/mach-imx/mx7/psci-mx7.c | 29 +++++++++++----
 arch/arm/mach-imx/mx7/psci.S     | 60 --------------------------------
 3 files changed, 24 insertions(+), 70 deletions(-)
 delete mode 100644 arch/arm/mach-imx/mx7/psci.S

diff --git a/arch/arm/mach-imx/mx7/Makefile b/arch/arm/mach-imx/mx7/Makefile
index 7a1ee7a944..94a0e6464f 100644
--- a/arch/arm/mach-imx/mx7/Makefile
+++ b/arch/arm/mach-imx/mx7/Makefile
@@ -4,7 +4,4 @@
 #
 
 obj-y	:= soc.o clock.o clock_slice.o ddr.o snvs.o
-
-ifdef CONFIG_ARMV7_PSCI
-obj-y  += psci-mx7.o psci.o
-endif
+obj-$(CONFIG_ARMV7_PSCI)  += psci-mx7.o
diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c b/arch/arm/mach-imx/mx7/psci-mx7.c
index 7dc49bd444..95f8cb4def 100644
--- a/arch/arm/mach-imx/mx7/psci-mx7.c
+++ b/arch/arm/mach-imx/mx7/psci-mx7.c
@@ -67,23 +67,34 @@ __secure void imx_enable_cpu_ca7(int cpu, bool enable)
 	writel(val, SRC_BASE_ADDR + SRC_A7RCR1);
 }
 
-__secure int imx_cpu_on(int fn, int cpu, int pc)
+__secure s32 psci_cpu_on(u32 __always_unused function_id, u32 mpidr, u32 ep,
+			 u32 context_id)
 {
-	writel(pc, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D);
+	u32 cpu = (mpidr & 0x1);
+
+	psci_save(cpu, ep, context_id);
+
+	writel((u32)psci_cpu_entry, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D);
 	imx_gpcv2_set_core1_power(true);
 	imx_enable_cpu_ca7(cpu, true);
 	return 0;
 }
 
-__secure int imx_cpu_off(int cpu)
+__secure s32 psci_cpu_off(void)
 {
+	int cpu;
+
+	psci_cpu_off_common();
+	cpu = psci_get_cpu_id();
 	imx_enable_cpu_ca7(cpu, false);
 	imx_gpcv2_set_core1_power(false);
 	writel(0, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D + 4);
-	return 0;
+
+	while (1)
+		wfi();
 }
 
-__secure void imx_system_reset(void)
+__secure void psci_system_reset(void)
 {
 	struct wdog_regs *wdog = (struct wdog_regs *)WDOG1_BASE_ADDR;
 
@@ -91,9 +102,12 @@ __secure void imx_system_reset(void)
 	writel(0x1 << 28, CCM_BASE_ADDR + CCM_ROOT_WDOG);
 	writel(0x3, CCM_BASE_ADDR + CCM_CCGR_WDOG1);
 	writew(WCR_WDE, &wdog->wcr);
+
+	while (1)
+		wfi();
 }
 
-__secure void imx_system_off(void)
+__secure void psci_system_off(void)
 {
 	u32 val;
 
@@ -103,4 +117,7 @@ __secure void imx_system_off(void)
 	val = readl(SNVS_BASE_ADDR + SNVS_LPCR);
 	val |= BP_SNVS_LPCR_DP_EN | BP_SNVS_LPCR_TOP;
 	writel(val, SNVS_BASE_ADDR + SNVS_LPCR);
+
+	while (1)
+		wfi();
 }
diff --git a/arch/arm/mach-imx/mx7/psci.S b/arch/arm/mach-imx/mx7/psci.S
deleted file mode 100644
index 89dcf880e8..0000000000
--- a/arch/arm/mach-imx/mx7/psci.S
+++ /dev/null
@@ -1,60 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0+ */
-/*
- * Copyright (C) 2015-2016 Freescale Semiconductor, Inc.
- * Copyright 2017 NXP
- */
-
-#include <config.h>
-#include <linux/linkage.h>
-
-#include <asm/armv7.h>
-#include <asm/arch-armv7/generictimer.h>
-#include <asm/psci.h>
-
-	.pushsection ._secure.text, "ax"
-
-	.arch_extension sec
-
-.globl psci_cpu_on
-psci_cpu_on:
-	push	{r4, r5, lr}
-
-	mov	r4, r0
-	mov	r5, r1
-	mov	r0, r1
-	mov	r1, r2
-	mov	r2, r3
-	bl	psci_save
-
-	mov	r0, r4
-	mov	r1, r5
-	ldr	r2, =psci_cpu_entry
-	bl	imx_cpu_on
-
-	pop	{r4, r5, pc}
-
-.globl psci_cpu_off
-psci_cpu_off:
-
-	bl	psci_cpu_off_common
-	bl	psci_get_cpu_id
-	bl	imx_cpu_off
-
-1: 	wfi
-	b 1b
-
-.globl psci_system_reset
-psci_system_reset:
-	bl	imx_system_reset
-
-2: 	wfi
-	b 2b
-
-.globl psci_system_off
-psci_system_off:
-	bl	imx_system_off
-
-3: 	wfi
-	b 3b
-
-	.popsection
-- 
2.17.1

[U-Boot] [PATCH v1 3/5] imx: mx7: psci: provide complete PSCI 1.0 implementation

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

PSCI 1.0 require PSCI_VERSION, PSCI_FEATURES, AFFINITY_INFO and
CPU_SUSPEND to be implemented. Commit 0ec3d98f7692 ("mx7_common:
use psci 1.0 instead of 0.1") marked the i.MX 7 implementation to
be PSCI 1.0 compliant but failed to implement those functions.
Especially the missing PSCI version callback was noticeable when
booting Linux:

  [    0.000000] psci: probing for conduit method from DT.
  [    0.000000] psci: PSCIv65535.65535 detected in firmware.
  [    0.000000] psci: Using standard PSCI v0.2 function IDs
  [    0.000000] psci: MIGRATE_INFO_TYPE not supported.
  [    0.000000] psci: SMC Calling Convention v1.0

This patch provides a minimal implementation thereof. With this
patch applied Linux detects PSCI 1.0:

  [    0.000000] psci: probing for conduit method from DT.
  [    0.000000] psci: PSCIv1.0 detected in firmware.
  [    0.000000] psci: Using standard PSCI v0.2 function IDs
  [    0.000000] psci: MIGRATE_INFO_TYPE not supported.
  [    0.000000] psci: SMC Calling Convention v1.0

Fixes: 0ec3d98f7692 ("mx7_common: use psci 1.0 instead of 0.1")
Suggested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 arch/arm/mach-imx/mx7/psci-mx7.c | 96 +++++++++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c b/arch/arm/mach-imx/mx7/psci-mx7.c
index 95f8cb4def..8b839d37a9 100644
--- a/arch/arm/mach-imx/mx7/psci-mx7.c
+++ b/arch/arm/mach-imx/mx7/psci-mx7.c
@@ -8,6 +8,7 @@
 #include <asm/psci.h>
 #include <asm/secure.h>
 #include <asm/arch/imx-regs.h>
+#include <linux/bitops.h>
 #include <common.h>
 #include <fsl_wdog.h>
 
@@ -34,6 +35,24 @@
 #define CCM_ROOT_WDOG		0xbb80
 #define CCM_CCGR_WDOG1		0x49c0
 
+#define MPIDR_AFF0		GENMASK(7, 0)
+
+#define IMX7D_PSCI_NR_CPUS	2
+#if IMX7D_PSCI_NR_CPUS > CONFIG_ARMV7_PSCI_NR_CPUS
+#error "invalid value for CONFIG_ARMV7_PSCI_NR_CPUS"
+#endif
+
+u8 psci_state[IMX7D_PSCI_NR_CPUS] __secure_data = {
+	 PSCI_AFFINITY_LEVEL_ON,
+	 PSCI_AFFINITY_LEVEL_OFF};
+
+static inline void psci_set_state(int cpu, u8 state)
+{
+	psci_state[cpu] = state;
+	dsb();
+	isb();
+}
+
 static inline void imx_gpcv2_set_m_core_pgc(bool enable, u32 offset)
 {
 	writel(enable, GPC_IPS_BASE_ADDR + offset);
@@ -67,25 +86,51 @@ __secure void imx_enable_cpu_ca7(int cpu, bool enable)
 	writel(val, SRC_BASE_ADDR + SRC_A7RCR1);
 }
 
+__secure void psci_arch_cpu_entry(void)
+{
+	u32 cpu = psci_get_cpu_id();
+
+	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON);
+}
+
 __secure s32 psci_cpu_on(u32 __always_unused function_id, u32 mpidr, u32 ep,
 			 u32 context_id)
 {
-	u32 cpu = (mpidr & 0x1);
+	u32 cpu = mpidr & MPIDR_AFF0;
+
+	if (mpidr & ~MPIDR_AFF0)
+		return ARM_PSCI_RET_INVAL;
+
+	if (cpu >= IMX7D_PSCI_NR_CPUS)
+		return ARM_PSCI_RET_INVAL;
+
+	if (psci_state[cpu] == PSCI_AFFINITY_LEVEL_ON)
+		return ARM_PSCI_RET_ALREADY_ON;
+
+	if (psci_state[cpu] == PSCI_AFFINITY_LEVEL_ON_PENDING)
+		return ARM_PSCI_RET_ON_PENDING;
 
 	psci_save(cpu, ep, context_id);
 
 	writel((u32)psci_cpu_entry, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D);
+
+	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON_PENDING);
+
 	imx_gpcv2_set_core1_power(true);
 	imx_enable_cpu_ca7(cpu, true);
-	return 0;
+
+	return ARM_PSCI_RET_SUCCESS;
 }
 
 __secure s32 psci_cpu_off(void)
 {
 	int cpu;
 
-	psci_cpu_off_common();
 	cpu = psci_get_cpu_id();
+
+	psci_cpu_off_common();
+	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_OFF);
+
 	imx_enable_cpu_ca7(cpu, false);
 	imx_gpcv2_set_core1_power(false);
 	writel(0, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D + 4);
@@ -121,3 +166,48 @@ __secure void psci_system_off(void)
 	while (1)
 		wfi();
 }
+
+__secure u32 psci_version(void)
+{
+	return ARM_PSCI_VER_1_0;
+}
+
+__secure s32 psci_cpu_suspend(u32 __always_unused function_id, u32 power_state,
+			      u32 entry_point_address,
+			      u32 context_id)
+{
+	return ARM_PSCI_RET_INVAL;
+}
+
+__secure s32 psci_affinity_info(u32 __always_unused function_id,
+				u32 target_affinity,
+				u32 lowest_affinity_level)
+{
+	u32 cpu = target_affinity & MPIDR_AFF0;
+
+	if (lowest_affinity_level > 0)
+		return ARM_PSCI_RET_INVAL;
+
+	if (target_affinity & ~MPIDR_AFF0)
+		return ARM_PSCI_RET_INVAL;
+
+	if (cpu >= IMX7D_PSCI_NR_CPUS)
+		return ARM_PSCI_RET_INVAL;
+
+	return psci_state[cpu];
+}
+
+__secure s32 psci_features(u32 __always_unused function_id, u32 psci_fid)
+{
+	switch (psci_fid) {
+	case ARM_PSCI_0_2_FN_PSCI_VERSION:
+	case ARM_PSCI_0_2_FN_CPU_OFF:
+	case ARM_PSCI_0_2_FN_CPU_ON:
+	case ARM_PSCI_0_2_FN_AFFINITY_INFO:
+	case ARM_PSCI_0_2_FN_SYSTEM_OFF:
+	case ARM_PSCI_0_2_FN_SYSTEM_RESET:
+	case ARM_PSCI_1_0_FN_PSCI_FEATURES:
+		return 0x0;
+	}
+	return ARM_PSCI_RET_NI;
+}
-- 
2.17.1

[U-Boot] [PATCH v1 4/5] imx: mx7: psci: support CPU0 on/off

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

So far psci_cpu_(on|off) only worked for CPU1. Allow to control
CPU0 too. This allows to run the Linux PSCI checker successfully:
  [    2.213447] psci_checker: PSCI checker started using 2 CPUs
  [    2.219107] psci_checker: Starting hotplug tests
  [    2.223859] psci_checker: Trying to turn off and on again all CPUs
  [    2.267191] IRQ21 no longer affine to CPU0
  [    2.293266] Retrying again to check for CPU kill
  [    2.302269] CPU0 killed.
  [    2.311648] psci_checker: Trying to turn off and on again group 0 (CPUs 0-1)
  [    2.354354] IRQ21 no longer affine to CPU0
  [    2.383222] Retrying again to check for CPU kill
  [    2.392148] CPU0 killed.
  [    2.398063] psci_checker: Hotplug tests passed OK
  [    2.402910] psci_checker: Starting suspend tests (10 cycles per state)
  [    2.410019] psci_checker: cpuidle not available on CPU 0, ignoring
  [    2.416452] psci_checker: cpuidle not available on CPU 1, ignoring
  [    2.422757] psci_checker: Could not start suspend tests on any CPU
  [    2.429370] psci_checker: PSCI checker completed

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 arch/arm/mach-imx/mx7/psci-mx7.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c b/arch/arm/mach-imx/mx7/psci-mx7.c
index 8b839d37a9..cd72449d9b 100644
--- a/arch/arm/mach-imx/mx7/psci-mx7.c
+++ b/arch/arm/mach-imx/mx7/psci-mx7.c
@@ -14,8 +14,10 @@
 
 #define GPC_CPU_PGC_SW_PDN_REQ	0xfc
 #define GPC_CPU_PGC_SW_PUP_REQ	0xf0
+#define GPC_PGC_C0		0x800
 #define GPC_PGC_C1		0x840
 
+#define BM_CPU_PGC_SW_PDN_PUP_REQ_CORE0_A7	0x1
 #define BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7	0x2
 
 /* below is for i.MX7D */
@@ -58,22 +60,24 @@ static inline void imx_gpcv2_set_m_core_pgc(bool enable, u32 offset)
 	writel(enable, GPC_IPS_BASE_ADDR + offset);
 }
 
-__secure void imx_gpcv2_set_core1_power(bool pdn)
+__secure void imx_gpcv2_set_core_power(int cpu, bool pdn)
 {
 	u32 reg = pdn ? GPC_CPU_PGC_SW_PUP_REQ : GPC_CPU_PGC_SW_PDN_REQ;
+	u32 pgc = cpu ? GPC_PGC_C1 : GPC_PGC_C0;
+	u32 pdn_pup_req = cpu ? BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7 :
+				BM_CPU_PGC_SW_PDN_PUP_REQ_CORE0_A7;
 	u32 val;
 
-	imx_gpcv2_set_m_core_pgc(true, GPC_PGC_C1);
+	imx_gpcv2_set_m_core_pgc(true, pgc);
 
 	val = readl(GPC_IPS_BASE_ADDR + reg);
-	val |= BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7;
+	val |= pdn_pup_req;
 	writel(val, GPC_IPS_BASE_ADDR + reg);
 
-	while ((readl(GPC_IPS_BASE_ADDR + reg) &
-	       BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7) != 0)
+	while ((readl(GPC_IPS_BASE_ADDR + reg) & pdn_pup_req) != 0)
 		;
 
-	imx_gpcv2_set_m_core_pgc(false, GPC_PGC_C1);
+	imx_gpcv2_set_m_core_pgc(false, pgc);
 }
 
 __secure void imx_enable_cpu_ca7(int cpu, bool enable)
@@ -116,7 +120,7 @@ __secure s32 psci_cpu_on(u32 __always_unused function_id, u32 mpidr, u32 ep,
 
 	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON_PENDING);
 
-	imx_gpcv2_set_core1_power(true);
+	imx_gpcv2_set_core_power(cpu, true);
 	imx_enable_cpu_ca7(cpu, true);
 
 	return ARM_PSCI_RET_SUCCESS;
@@ -132,7 +136,7 @@ __secure s32 psci_cpu_off(void)
 	psci_set_state(cpu, PSCI_AFFINITY_LEVEL_OFF);
 
 	imx_enable_cpu_ca7(cpu, false);
-	imx_gpcv2_set_core1_power(false);
+	imx_gpcv2_set_core_power(cpu, false);
 	writel(0, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D + 4);
 
 	while (1)
-- 
2.17.1

[U-Boot] [PATCH v1 5/5] imx: mx7: psci: implement MIGRATE_INFO_TYPE

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

Implement MIGRATE_INFO_TYPE. This informs Linux that no migration
for the trusted operating system is necessary:
  [    0.000000] psci: Trusted OS migration not required

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 arch/arm/mach-imx/mx7/psci-mx7.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c b/arch/arm/mach-imx/mx7/psci-mx7.c
index cd72449d9b..aae96c553f 100644
--- a/arch/arm/mach-imx/mx7/psci-mx7.c
+++ b/arch/arm/mach-imx/mx7/psci-mx7.c
@@ -201,6 +201,12 @@ __secure s32 psci_affinity_info(u32 __always_unused function_id,
 	return psci_state[cpu];
 }
 
+__secure s32 psci_migrate_info_type(u32 function_id)
+{
+	/* Trusted OS is either not present or does not require migration */
+	return 2;
+}
+
 __secure s32 psci_features(u32 __always_unused function_id, u32 psci_fid)
 {
 	switch (psci_fid) {
@@ -208,6 +214,7 @@ __secure s32 psci_features(u32 __always_unused function_id, u32 psci_fid)
 	case ARM_PSCI_0_2_FN_CPU_OFF:
 	case ARM_PSCI_0_2_FN_CPU_ON:
 	case ARM_PSCI_0_2_FN_AFFINITY_INFO:
+	case ARM_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
 	case ARM_PSCI_0_2_FN_SYSTEM_OFF:
 	case ARM_PSCI_0_2_FN_SYSTEM_RESET:
 	case ARM_PSCI_1_0_FN_PSCI_FEATURES:
-- 
2.17.1

[U-Boot] [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs

[Stefan Agner]

On 24.06.2018 21:09, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> A proper stack is required to safely use C code in psci_arch_cpu_entry.

Patrick, I prefer to have your ack on this since you introduced
psci_arch_cpu_entry.

As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The
same function in i.MX 7's PSCI implementation the compiler actually
pushed stuff on the (uninitialized) stack, which caused the newly
brought up CPU to immediately crash.

Not sure if in your case the stack pointer is already setup by some
other means or your compiler does not use the stack.

In any case, I think it is better to just setup the stack properly as
done in this patch...


Stefano, I think we really want patch 2/3 applied before the release
since it fixes i.MX 7 PSCI. Right now the implementation is really
broken and not PSCI 1.0 conformant. But patch 2/3 require this patch to
be applied... Not sure how we should handle this.

--
Stefan

> 
> Fixes: 486daaa618e1 ("arm: psci: add a weak function psci_arch_cpu_entry")
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---
> 
>  arch/arm/cpu/armv7/psci.S | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv7/psci.S b/arch/arm/cpu/armv7/psci.S
> index 08b5088675..983cd90442 100644
> --- a/arch/arm/cpu/armv7/psci.S
> +++ b/arch/arm/cpu/armv7/psci.S
> @@ -331,6 +331,8 @@ ENTRY(psci_cpu_entry)
>  
>  	bl	_nonsec_init
>  
> +	bl	psci_stack_setup
> +
>  	bl	psci_arch_cpu_entry
>  
>  	bl	psci_get_cpu_id			@ CPU ID => r0

[U-Boot] [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs

[Patrick DELAUNAY]

Hi Stefan

> From: Stefan Agner [mailto:stefan at agner.ch]
> Sent: mercredi 27 juin 2018 10:36
> Subject: Re: [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
> 
> On 24.06.2018 21:09, Stefan Agner wrote:
> > From: Stefan Agner <stefan.agner@toradex.com>
> >
> > A proper stack is required to safely use C code in psci_arch_cpu_entry.
> 
> Patrick, I prefer to have your ack on this since you introduced
> psci_arch_cpu_entry.
> 
> As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The same
> function in i.MX 7's PSCI implementation the compiler actually pushed stuff on
> the (uninitialized) stack, which caused the newly brought up CPU to immediately
> crash.
> 
> Not sure if in your case the stack pointer is already setup by some other means
> or your compiler does not use the stack.
> 
> In any case, I think it is better to just setup the stack properly as done in this
> patch...

I expected that the secure stack is initialized by bootROM, but after check on 2018.07-rc2,
I got a crash also with the stm32mp1 platform.

After code review, my behavior  is clearly not safe: I don't sure that the initial BootROM stack
is not overlapping the installed PSCI monitor code or data.
So I agree:  it is needed to initialize the stack in psci_cpu_entry.

Moreover after your patch the crash is solved for my platform stm32mp1.

> 
> Stefano, I think we really want patch 2/3 applied before the release since it fixes
> i.MX 7 PSCI. Right now the implementation is really broken and not PSCI 1.0
> conformant. But patch 2/3 require this patch to be applied... Not sure how we
> should handle this.
> 
> --
> Stefan

Acked-by: Patrick DELAUNAY <Patrick.delaunay@st.com>
Tested-by: Patrick DELAUNAY <Patrick.delaunay@st.com>

Test done on stm32mp1 platform

Thanks!
Patrick 

[U-Boot] [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs

[Stefan Agner]

Hi Tom, Hi Stefano,

On 02.07.2018 11:08, Patrick DELAUNAY wrote:
> Hi Stefan
> 
>> From: Stefan Agner [mailto:stefan at agner.ch]
>> Sent: mercredi 27 juin 2018 10:36
>> Subject: Re: [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
>>
>> On 24.06.2018 21:09, Stefan Agner wrote:
>> > From: Stefan Agner <stefan.agner@toradex.com>
>> >
>> > A proper stack is required to safely use C code in psci_arch_cpu_entry.
>>
>> Patrick, I prefer to have your ack on this since you introduced
>> psci_arch_cpu_entry.
>>
>> As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The same
>> function in i.MX 7's PSCI implementation the compiler actually pushed stuff on
>> the (uninitialized) stack, which caused the newly brought up CPU to immediately
>> crash.
>>
>> Not sure if in your case the stack pointer is already setup by some other means
>> or your compiler does not use the stack.
>>
>> In any case, I think it is better to just setup the stack properly as done in this
>> patch...
> 
> I expected that the secure stack is initialized by bootROM, but after
> check on 2018.07-rc2,
> I got a crash also with the stm32mp1 platform.
> 
> After code review, my behavior  is clearly not safe: I don't sure that
> the initial BootROM stack
> is not overlapping the installed PSCI monitor code or data.
> So I agree:  it is needed to initialize the stack in psci_cpu_entry.
> 
> Moreover after your patch the crash is solved for my platform stm32mp1.

Given that this fixes two platforms I guess it definitely should go into
v2018.07.

Technically, for i.MX 7 only patch 1, 2 and 3 are necessary, but 4 and 5
are fairly straight forward and seem to work fine here.

Patch 1 is generic, so not sure through which trees the patchset should
go...

--
Stefan

> 
>>
>> Stefano, I think we really want patch 2/3 applied before the release since it fixes
>> i.MX 7 PSCI. Right now the implementation is really broken and not PSCI 1.0
>> conformant. But patch 2/3 require this patch to be applied... Not sure how we
>> should handle this.
>>
>> --
>> Stefan
> 
> Acked-by: Patrick DELAUNAY <Patrick.delaunay@st.com>
> Tested-by: Patrick DELAUNAY <Patrick.delaunay@st.com>
> 
> Test done on stm32mp1 platform
> 
> Thanks!
> Patrick

[U-Boot] [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs

[Tom Rini]

On Tue, Jul 03, 2018 at 02:32:28PM +0200, Stefan Agner wrote:
> Hi Tom, Hi Stefano,
> 
> On 02.07.2018 11:08, Patrick DELAUNAY wrote:
> > Hi Stefan
> > 
> >> From: Stefan Agner [mailto:stefan at agner.ch]
> >> Sent: mercredi 27 juin 2018 10:36
> >> Subject: Re: [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
> >>
> >> On 24.06.2018 21:09, Stefan Agner wrote:
> >> > From: Stefan Agner <stefan.agner@toradex.com>
> >> >
> >> > A proper stack is required to safely use C code in psci_arch_cpu_entry.
> >>
> >> Patrick, I prefer to have your ack on this since you introduced
> >> psci_arch_cpu_entry.
> >>
> >> As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The same
> >> function in i.MX 7's PSCI implementation the compiler actually pushed stuff on
> >> the (uninitialized) stack, which caused the newly brought up CPU to immediately
> >> crash.
> >>
> >> Not sure if in your case the stack pointer is already setup by some other means
> >> or your compiler does not use the stack.
> >>
> >> In any case, I think it is better to just setup the stack properly as done in this
> >> patch...
> > 
> > I expected that the secure stack is initialized by bootROM, but after
> > check on 2018.07-rc2,
> > I got a crash also with the stm32mp1 platform.
> > 
> > After code review, my behavior  is clearly not safe: I don't sure that
> > the initial BootROM stack
> > is not overlapping the installed PSCI monitor code or data.
> > So I agree:  it is needed to initialize the stack in psci_cpu_entry.
> > 
> > Moreover after your patch the crash is solved for my platform stm32mp1.
> 
> Given that this fixes two platforms I guess it definitely should go into
> v2018.07.
> 
> Technically, for i.MX 7 only patch 1, 2 and 3 are necessary, but 4 and 5
> are fairly straight forward and seem to work fine here.
> 
> Patch 1 is generic, so not sure through which trees the patchset should
> go...

I'm OK with the series coming in if Stefano is.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180703/85cd7b76/attachment.sig>

[U-Boot] [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs

[Stefano Babic]

On 03/07/2018 14:32, Stefan Agner wrote:
> Hi Tom, Hi Stefano,
> 
> On 02.07.2018 11:08, Patrick DELAUNAY wrote:
>> Hi Stefan
>>
>>> From: Stefan Agner [mailto:stefan at agner.ch]
>>> Sent: mercredi 27 juin 2018 10:36
>>> Subject: Re: [PATCH v1 1/5] ARM: PSCI: initialize stack pointer on secondary CPUs
>>>
>>> On 24.06.2018 21:09, Stefan Agner wrote:
>>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>>
>>>> A proper stack is required to safely use C code in psci_arch_cpu_entry.
>>>
>>> Patrick, I prefer to have your ack on this since you introduced
>>> psci_arch_cpu_entry.
>>>
>>> As far as I can tell STM32MP1 uses C code in psci_arch_cpu_entry. The same
>>> function in i.MX 7's PSCI implementation the compiler actually pushed stuff on
>>> the (uninitialized) stack, which caused the newly brought up CPU to immediately
>>> crash.
>>>
>>> Not sure if in your case the stack pointer is already setup by some other means
>>> or your compiler does not use the stack.
>>>
>>> In any case, I think it is better to just setup the stack properly as done in this
>>> patch...
>>
>> I expected that the secure stack is initialized by bootROM, but after
>> check on 2018.07-rc2,
>> I got a crash also with the stm32mp1 platform.
>>
>> After code review, my behavior  is clearly not safe: I don't sure that
>> the initial BootROM stack
>> is not overlapping the installed PSCI monitor code or data.
>> So I agree:  it is needed to initialize the stack in psci_cpu_entry.
>>
>> Moreover after your patch the crash is solved for my platform stm32mp1.
> 
> Given that this fixes two platforms I guess it definitely should go into
> v2018.07.
> 
> Technically, for i.MX 7 only patch 1, 2 and 3 are necessary, but 4 and 5
> are fairly straight forward and seem to work fine here.
> 
> Patch 1 is generic, so not sure through which trees the patchset should
> go...


Sorry for late answer, but benner late as never...;-)

I have applied the whole patchset to u-boot-imx. I will push it in a
couple of hours on the server.

Regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================