[v2] armv8/vexpress64: make multientry conditional

Message ID 1422485701-17505-1-git-send-email-linus.walleij@linaro.org
State New
Headers show

Commit Message

Linus Walleij Jan. 28, 2015, 10:55 p.m.
While the Freescale ARMv8 board LS2085A will enter U-Boot both
on a master and a secondary (slave) CPU, this is not the common
behaviour on ARMv8 platforms. The norm is that U-Boot is entered
from the master CPU only, while the other CPUs are kept in
WFI (wait for interrupt) state.

The code determining which CPU we are running on is using the
MPIDR register, but the definition of that register varies with
platform to some extent, and handling multi-cluster platforms
(such as the Juno) will become cumbersome. It is better to only
enable the multiple entry code on machines that actually need
it and disable it by default.

Make the single entry default and add a special
ARMV8_MULTIENTRY KConfig option to be used by the
platforms that need multientry and set it for the LS2085A.
Delete all use of the CPU_RELEASE_ADDR from the Vexpress64
boards as it is just totally unused and misleading, and
make it conditional in the generic start.S code.

This makes the Juno platform start U-Boot properly.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Move configuration of ARMV8_MULTIENTRY over to Kconfig
  as requested by Tom Rini.

This patch applied on top of the other patch series send,
ending with
[PATCH 4/4] vexpress64: support the Juno Development Platform
Please apply it on top of these if the patch seems OK.
---
 arch/arm/Kconfig                     | 4 ++++
 arch/arm/cpu/armv8/Kconfig           | 6 ++++++
 arch/arm/cpu/armv8/start.S           | 8 ++++----
 arch/arm/include/asm/macro.h         | 8 ++++++++
 board/armltd/vexpress64/vexpress64.c | 6 ------
 include/configs/vexpress_aemv8a.h    | 8 --------
 6 files changed, 22 insertions(+), 18 deletions(-)
 create mode 100644 arch/arm/cpu/armv8/Kconfig

Comments

Linus Walleij Feb. 3, 2015, 12:31 p.m. | #1
On Fri, Jan 30, 2015 at 3:27 PM, FengHua <fenghua@phytium.com.cn> wrote:

>> While the Freescale ARMv8 board LS2085A will enter U-Boot both
>> on a master and a secondary (slave) CPU, this is not the common
>> behaviour on ARMv8 platforms. The norm is that U-Boot is entered
>> from the master CPU only, while the other CPUs are kept in
>> WFI (wait for interrupt) state.
>
> According to fsl-lsch3 source code,
> Freescale LS2085A master cpu enter u-boot first, then it kick slaves up.
> That's right?

I don't know, I don't have access to this system.

However it doesn't matter to the code.

> There's ATF in Juno, so all slaves are held up.

Yep. Also in FVP and the base model.

But generally I don't see why this multientry exists at all,
it seems more natural for me to keep all CPUs in WFI and
let the OS start them.

>> Make the single entry default and add a special
>> ARMV8_MULTIENTRY KConfig option to be used by the
>> platforms that need multientry and set it for the LS2085A.
>> Delete all use of the CPU_RELEASE_ADDR from the Vexpress64
>> boards as it is just totally unused and misleading, and
>> make it conditional in the generic start.S code.
>
> It's better to retain CPU_RELEASE_ADDR usage with
> CONFIG_ARMV8_MULTIENTRY.
> It still useful when ATF or other PSCI service do not exist.

Yes and this is exactly what this patch does.

CPU_RELEASE_ADDR is still used on the fsl machine.

Yours,
Linus Walleij
Linus Walleij Feb. 6, 2015, 10:56 a.m. | #2
On Wed, Feb 4, 2015 at 3:38 PM, FengHua <fenghua@phytium.com.cn> wrote:

>>  config TARGET_LS2085A_SIMU
>>       bool "Support ls2085a_simu"
>>       select ARM64
>> +     select ARMV8_MULTIENTRY
>>
> VEXPRESS_AEMV8A and VEXPRESS_AEMV8A_SEMI are
> defaultly single entry?

Yes.

> That means we always has ATF exist.

Not necessarily, just that something keeps the secondary cores
in WFI. If that is ATF or something else (like the ROM, or just
them being kept in power-on state) not doesn't matter.

But the more important question is: who is using this on vexpress64?

If I can reproduce the usecase with some sane setup, I'll be happy
to handle the multientry case for the vexpress64's.

>>  master_cpu:
>> +     /* On the master CPU */
>> +#endif /* CONFIG_ARMV8_MULTIENTRY */
>> +
>>       bl      _main
>>
> How about put the slave part of lowlevel_init in CONFIG_ARMV8_MULTIENTRY?
> although it still works with modified branch_if_master macro.

Yes it may save a few bytes, added some more #ifdefs.

>>  .macro armv8_switch_to_el2_m, xreg1
>> diff --git a/board/armltd/vexpress64/vexpress64.c b/board/armltd/vexpress64/vexpress64.c
>> index 58973185ecda..7ab000cca77b 100644
>> --- a/board/armltd/vexpress64/vexpress64.c
>> +++ b/board/armltd/vexpress64/vexpress64.c
>> @@ -22,12 +22,6 @@ int board_init(void)
>>
>>  int dram_init(void)
>>  {
>> -     /*
>> -      * Clear spin table so that secondary processors
>> -      * observe the correct value after waken up from wfe.
>> -      */
>> -     *(unsigned long *)CPU_RELEASE_ADDR = 0;
>> -
>
> Put them in CONFIG_ARMV8_MULTIENTRY instead of removing, so as to
> it works without ATF.

Do you have a system that uses this?

How can I reproduce a boot on one of the ARM vexpress64
simulators that will come up on multiple CPUs?

>> -
>> -/* SMP Spin Table Definitions */
>> -#ifdef CONFIG_TARGET_VEXPRESS64_BASE_FVP
>> -#define CPU_RELEASE_ADDR             (CONFIG_SYS_SDRAM_BASE + 0x03f00000)
>
> Are you sure Base FVP did not need MultiEntry?

Noone I know of uses it, if they exist the should speak up.

> why is CPU_RELEASE_ADDR defined here with different value?

I think somebody just put it in there because the #define was
needed to compile the U-boot. Noone has given any explanation
to these magic numbers...

Yours,
Linus Walleij

Patch

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 986b4c5d81db..75dd9bb60d6b 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -743,10 +743,12 @@  config TARGET_VEXPRESS64_JUNO
 config TARGET_LS2085A_EMU
 	bool "Support ls2085a_emu"
 	select ARM64
+	select ARMV8_MULTIENTRY
 
 config TARGET_LS2085A_SIMU
 	bool "Support ls2085a_simu"
 	select ARM64
+	select ARMV8_MULTIENTRY
 
 config TARGET_LS1021AQDS
 	bool "Support ls1021aqds"
@@ -855,6 +857,8 @@  source "arch/arm/cpu/armv7/zynq/Kconfig"
 
 source "arch/arm/cpu/armv7/Kconfig"
 
+source "arch/arm/cpu/armv8/Kconfig"
+
 source "board/aristainetos/Kconfig"
 source "board/BuR/kwb/Kconfig"
 source "board/BuR/tseries/Kconfig"
diff --git a/arch/arm/cpu/armv8/Kconfig b/arch/arm/cpu/armv8/Kconfig
new file mode 100644
index 000000000000..4cd84b031114
--- /dev/null
+++ b/arch/arm/cpu/armv8/Kconfig
@@ -0,0 +1,6 @@ 
+if ARM64
+
+config ARMV8_MULTIENTRY
+        boolean "Enable multiple CPUs to enter into U-boot"
+
+endif
diff --git a/arch/arm/cpu/armv8/start.S b/arch/arm/cpu/armv8/start.S
index 4b11aa4f2227..9b439f30b779 100644
--- a/arch/arm/cpu/armv8/start.S
+++ b/arch/arm/cpu/armv8/start.S
@@ -77,6 +77,7 @@  reset:
 	/* Processor specific initialization */
 	bl	lowlevel_init
 
+#ifdef CONFIG_ARMV8_MULTIENTRY
 	branch_if_master x0, x1, master_cpu
 
 	/*
@@ -88,11 +89,10 @@  slave_cpu:
 	ldr	x0, [x1]
 	cbz	x0, slave_cpu
 	br	x0			/* branch to the given address */
-
-	/*
-	 * Master CPU
-	 */
 master_cpu:
+	/* On the master CPU */
+#endif /* CONFIG_ARMV8_MULTIENTRY */
+
 	bl	_main
 
 /*-----------------------------------------------------------------------*/
diff --git a/arch/arm/include/asm/macro.h b/arch/arm/include/asm/macro.h
index 1c8c4251ee0c..3b3146ab2239 100644
--- a/arch/arm/include/asm/macro.h
+++ b/arch/arm/include/asm/macro.h
@@ -78,6 +78,8 @@  lr	.req	x30
  * choose processor with all zero affinity value as the master.
  */
 .macro	branch_if_slave, xreg, slave_label
+#ifdef CONFIG_ARMV8_MULTIENTRY
+	/* NOTE: MPIDR handling will be erroneous on multi-cluster machines */
 	mrs	\xreg, mpidr_el1
 	tst	\xreg, #0xff		/* Test Affinity 0 */
 	b.ne	\slave_label
@@ -90,6 +92,7 @@  lr	.req	x30
 	lsr	\xreg, \xreg, #16
 	tst	\xreg, #0xff		/* Test Affinity 3 */
 	b.ne	\slave_label
+#endif
 .endm
 
 /*
@@ -97,12 +100,17 @@  lr	.req	x30
  * choose processor with all zero affinity value as the master.
  */
 .macro	branch_if_master, xreg1, xreg2, master_label
+#ifdef CONFIG_ARMV8_MULTIENTRY
+	/* NOTE: MPIDR handling will be erroneous on multi-cluster machines */
 	mrs	\xreg1, mpidr_el1
 	lsr	\xreg2, \xreg1, #32
 	lsl	\xreg1, \xreg1, #40
 	lsr	\xreg1, \xreg1, #40
 	orr	\xreg1, \xreg1, \xreg2
 	cbz	\xreg1, \master_label
+#else
+	b 	\master_label
+#endif
 .endm
 
 .macro armv8_switch_to_el2_m, xreg1
diff --git a/board/armltd/vexpress64/vexpress64.c b/board/armltd/vexpress64/vexpress64.c
index 58973185ecda..7ab000cca77b 100644
--- a/board/armltd/vexpress64/vexpress64.c
+++ b/board/armltd/vexpress64/vexpress64.c
@@ -22,12 +22,6 @@  int board_init(void)
 
 int dram_init(void)
 {
-	/*
-	 * Clear spin table so that secondary processors
-	 * observe the correct value after waken up from wfe.
-	 */
-	*(unsigned long *)CPU_RELEASE_ADDR = 0;
-
 	gd->ram_size = PHYS_SDRAM_1_SIZE;
 	return 0;
 }
diff --git a/include/configs/vexpress_aemv8a.h b/include/configs/vexpress_aemv8a.h
index 7fb28a54ba17..e276fff7e442 100644
--- a/include/configs/vexpress_aemv8a.h
+++ b/include/configs/vexpress_aemv8a.h
@@ -56,14 +56,6 @@ 
 /* Flat Device Tree Definitions */
 #define CONFIG_OF_LIBFDT
 
-
-/* SMP Spin Table Definitions */
-#ifdef CONFIG_TARGET_VEXPRESS64_BASE_FVP
-#define CPU_RELEASE_ADDR		(CONFIG_SYS_SDRAM_BASE + 0x03f00000)
-#else
-#define CPU_RELEASE_ADDR		(CONFIG_SYS_SDRAM_BASE + 0x7fff0)
-#endif
-
 /* CS register bases for the original memory map. */
 #define V2M_PA_CS0			0x00000000
 #define V2M_PA_CS1			0x14000000