Hi Mark
On 08/22/2014 12:20 AM, Sharma Bhupesh-B45370 wrote:
Hi Mark,
-----Original Message-----
From: u-boot-boun...@lists.denx.de [mailto:u-boot-boun...@lists.denx.de]
On Behalf Of York Sun
Sent: Friday, August 22, 2014 12:03 AM
To: Mark Rutland; Basu Arnab-B45036
Cc: tr...@ti.com; u-boot@lists.denx.de; Wood Scott-B07421
Subject: Re: [U-Boot] [Patch v2 3/5] armv8/fsl-lsch3: Release secondary
cores from boot hold off with Boot Page
On 08/21/2014 06:47 AM, Mark Rutland wrote:
Hi York,
I have mostly minor comments this time; this is looking pretty good.
Thanks for taking the time to review this.
On Tue, Aug 19, 2014 at 09:28:00PM +0100, York Sun wrote:
Secondary cores need to be released from holdoff by boot release
registers. With GPP bootrom, they can boot from main memory directly.
Individual spin table is used for each core. If a single release
address is needed, defining macro CONFIG_FSL_SMP_RELEASE_ALL will use
the CPU_RELEASE_ADDR. Spin table and the boot page is reserved in
device tree so OS won't overwrite.
Signed-off-by: York Sun <york...@freescale.com>
Signed-off-by: Arnab Basu <arnab.b...@freescale.com>
---
v2: Removed copying boot page. Use u-boot image as is in memory.
Added dealing with different size of addr_cell in device tree.
Added dealing with big- and little-endian.
Added flushing spin table after cpu_release().
arch/arm/cpu/armv8/fsl-lsch3/Makefile | 2 +
arch/arm/cpu/armv8/fsl-lsch3/cpu.c | 13 ++
arch/arm/cpu/armv8/fsl-lsch3/cpu.h | 1 +
arch/arm/cpu/armv8/fsl-lsch3/fdt.c | 56 +++++++
arch/arm/cpu/armv8/fsl-lsch3/lowlevel.S | 128 ++++++++++++-
--
arch/arm/cpu/armv8/fsl-lsch3/mp.c | 172
+++++++++++++++++++++
arch/arm/cpu/armv8/fsl-lsch3/mp.h | 36 +++++
arch/arm/cpu/armv8/transition.S | 63 +-------
arch/arm/include/asm/arch-fsl-lsch3/config.h | 3 +-
arch/arm/include/asm/arch-fsl-lsch3/immap_lsch3.h | 35 +++++
arch/arm/include/asm/macro.h | 92 +++++++++++
arch/arm/lib/gic_64.S | 10 +-
common/board_f.c | 2 +-
13 files changed, 525 insertions(+), 88 deletions(-) create mode
100644 arch/arm/cpu/armv8/fsl-lsch3/fdt.c
create mode 100644 arch/arm/cpu/armv8/fsl-lsch3/mp.c create mode
100644 arch/arm/cpu/armv8/fsl-lsch3/mp.h
diff --git a/arch/arm/cpu/armv8/fsl-lsch3/Makefile
b/arch/arm/cpu/armv8/fsl-lsch3/Makefile
index 9249537..f920eeb 100644
--- a/arch/arm/cpu/armv8/fsl-lsch3/Makefile
+++ b/arch/arm/cpu/armv8/fsl-lsch3/Makefile
@@ -7,3 +7,5 @@
obj-y += cpu.o
obj-y += lowlevel.o
obj-y += speed.o
+obj-$(CONFIG_MP) += mp.o
+obj-$(CONFIG_OF_LIBFDT) += fdt.o
diff --git a/arch/arm/cpu/armv8/fsl-lsch3/cpu.c
b/arch/arm/cpu/armv8/fsl-lsch3/cpu.c
index c129d03..47b947f 100644
--- a/arch/arm/cpu/armv8/fsl-lsch3/cpu.c
+++ b/arch/arm/cpu/armv8/fsl-lsch3/cpu.c
@@ -11,6 +11,7 @@
#include <asm/io.h>
#include <asm/arch-fsl-lsch3/immap_lsch3.h>
#include "cpu.h"
+#include "mp.h"
#include "speed.h"
#include <fsl_mc.h>
@@ -434,3 +435,15 @@ int cpu_eth_init(bd_t *bis) #endif
return error;
}
+
+
+int arch_early_init_r(void)
+{
+ int rv;
+ rv = fsl_lsch3_wake_seconday_cores();
+
+ if (rv)
+ printf("Did not wake secondary cores\n");
+
+ return 0;
+}
diff --git a/arch/arm/cpu/armv8/fsl-lsch3/cpu.h
b/arch/arm/cpu/armv8/fsl-lsch3/cpu.h
index 28544d7..2e3312b 100644
--- a/arch/arm/cpu/armv8/fsl-lsch3/cpu.h
+++ b/arch/arm/cpu/armv8/fsl-lsch3/cpu.h
@@ -5,3 +5,4 @@
*/
int fsl_qoriq_core_to_cluster(unsigned int core);
+u32 cpu_mask(void);
diff --git a/arch/arm/cpu/armv8/fsl-lsch3/fdt.c
b/arch/arm/cpu/armv8/fsl-lsch3/fdt.c
new file mode 100644
index 0000000..2dbcdcb
--- /dev/null
+++ b/arch/arm/cpu/armv8/fsl-lsch3/fdt.c
@@ -0,0 +1,56 @@
+/*
+ * Copyright 2014 Freescale Semiconductor, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <common.h>
+#include <libfdt.h>
+#include <fdt_support.h>
+#include "mp.h"
+
+#ifdef CONFIG_MP
+void ft_fixup_cpu(void *blob)
+{
+ int off;
+ __maybe_unused u64 spin_tbl_addr = (u64)get_spin_tbl_addr();
+ fdt32_t *reg;
+ int addr_cells;
+ u64 val;
+ size_t *boot_code_size = &(__secondary_boot_code_size);
+
+ off = fdt_node_offset_by_prop_value(blob, -1, "device_type",
+ "cpus", 4);
I didn't think /cpus had device_type = "cpus". I can't see any
instances in any DTs I have to hand. Can we not find /cpus by path?
I will let Arnab to comment on this. He is coordinating with Linux device
tree.
Since I contribute to the DTS for FSL ARMv8 SoC, here is my rationale behind
the same.
I have used the standard ARM cpu device-tree binding documentation as a
reference (see [1])
which defined the device_type which it mentions should be set to cpu.
Please let me know if I am missing something.
[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/cpus.txt
I see the confusion here, "device_type" is required for the "cpu" node
but not its container the "cpus" node.
I will change this.
+ of_bus_default_count_cells(blob, off, &addr_cells, NULL);
+
+ off = fdt_node_offset_by_prop_value(blob, -1, "device_type",
"cpu", 4);
+ while (off != -FDT_ERR_NOTFOUND) {
+ reg = (fdt32_t *)fdt_getprop(blob, off, "reg", 0);
+ if (reg) {
+ val = spin_tbl_addr; #ifndef
+CONFIG_FSL_SMP_RELEASE_ALL
+ val += id_to_core(of_read_number(reg,
addr_cells))
+ * SPIN_TABLE_ELEM_SIZE; #endif
+ val = cpu_to_fdt64(val);
+ fdt_setprop_string(blob, off, "enable-method",
+ "spin-table");
+ fdt_setprop(blob, off, "cpu-release-addr",
+ &val, sizeof(val));
+ } else {
+ puts("cpu NULL\n");
Could we change that to "Warning: found cpu node without reg property"
or something like that which makes the problem clear?
Yes, we can.
+ }
+ off = fdt_node_offset_by_prop_value(blob, off,
"device_type",
+ "cpu", 4);
+ }
+
+ fdt_add_mem_rsv(blob, (uintptr_t)&secondary_boot_code,
+ *boot_code_size); } #endif
+
+void ft_cpu_setup(void *blob, bd_t *bd) { #ifdef CONFIG_MP
+ ft_fixup_cpu(blob);
+#endif
+}
diff --git a/arch/arm/cpu/armv8/fsl-lsch3/lowlevel.S
b/arch/arm/cpu/armv8/fsl-lsch3/lowlevel.S
index ad32b6c..629e6d4 100644
--- a/arch/arm/cpu/armv8/fsl-lsch3/lowlevel.S
+++ b/arch/arm/cpu/armv8/fsl-lsch3/lowlevel.S
@@ -8,7 +8,9 @@
#include <config.h>
#include <linux/linkage.h>
+#include <asm/gic.h>
#include <asm/macro.h>
+#include "mp.h"
ENTRY(lowlevel_init)
mov x29, lr /* Save LR */
@@ -37,29 +39,119 @@ ENTRY(lowlevel_init)
branch_if_master x0, x1, 1f
+ ldr x0, =secondary_boot_func
+ blr x0
+
+1:
+2:
Isn't the '2' label redundant?
We have some internal code dealing with trust zone between the 1 and 2. It
is not likely to be used in long term since we are trying to move them
into security monitor. I can drop label 2 here.
Yes, now that I look at it the labels in this file could do with some
cleanup.
Will do.
U-boot can still be booted in EL3, as it can be well booted w/o a ATF or EL3
capable
s/w running before the same. That's why we have CONFIG_EL3 and CONFIG_EL2 code
legs
in the u-boot ARMv8 code.
+ mov lr, x29 /* Restore LR */
+ ret
+ENDPROC(lowlevel_init)
+
+ /* Keep literals not used by the secondary boot code outside it
*/
+ .ltorg
+
+ /* Using 64 bit alignment since the spin table is accessed as
data */
+ .align 4
+ .global secondary_boot_code
+ /* Secondary Boot Code starts here */
+secondary_boot_code:
+ .global __spin_table
+__spin_table:
+ .space CONFIG_MAX_CPUS*SPIN_TABLE_ELEM_SIZE
+
+ .align 2
+ENTRY(secondary_boot_func)
/*
- * Slave should wait for master clearing spin table.
- * This sync prevent salves observing incorrect
- * value of spin table and jumping to wrong place.
+ * MPIDR_EL1 Fields:
+ * MPIDR[1:0] = AFF0_CPUID <- Core ID (0,1)
+ * MPIDR[7:2] = AFF0_RES
+ * MPIDR[15:8] = AFF1_CLUSTERID <- Cluster ID (0,1,2,3)
+ * MPIDR[23:16] = AFF2_CLUSTERID
+ * MPIDR[24] = MT
+ * MPIDR[29:25] = RES0
+ * MPIDR[30] = U
+ * MPIDR[31] = ME
+ * MPIDR[39:32] = AFF3
+ *
+ * Linear Processor ID (LPID) calculation from MPIDR_EL1:
+ * (We only use AFF0_CPUID and AFF1_CLUSTERID for now
+ * until AFF2_CLUSTERID and AFF3 have non-zero values)
+ *
+ * LPID = MPIDR[15:8] | MPIDR[1:0]
*/
-#if defined(CONFIG_GICV2) || defined(CONFIG_GICV3) -#ifdef
CONFIG_GICV2
- ldr x0, =GICC_BASE
+ mrs x0, mpidr_el1
+ ubfm x1, x0, #8, #15
+ ubfm x2, x0, #0, #1
+ orr x10, x2, x1, lsl #2 /* x10 has LPID */
+ ubfm x9, x0, #0, #15 /* x9 contains MPIDR[15:0] */
+ /*
+ * offset of the spin table element for this core from start of
spin
+ * table (each elem is padded to 64 bytes)
+ */
+ lsl x1, x10, #6
+ ldr x0, =__spin_table
+ /* physical address of this cpus spin table element */
+ add x11, x1, x0
+
+ str x9, [x11, #16] /* LPID */
+ mov x4, #1
+ str x4, [x11, #8] /* STATUS */
+ dsb sy
+#if defined(CONFIG_GICV3)
+ gic_wait_for_interrupt_m x0
#endif
- bl gic_wait_for_interrupt
Why do we only need to wait for GICv3? We previously did so for GICv2.
I think this is leftover from the attempt to boot all cores
simultaneously. I will let Arnab to comment.
There is a "#elif" that I'm sure used to be here but seems to have got
lost somehow. For CONFIG_GICV2 "gic_wait_for_interrupt_m" takes 2
arguments. We should be waiting in both cases.
Will fix.
+
+ bl secondary_switch_to_el2
+#ifdef CONFIG_ARMV8_SWITCH_TO_EL1
+ secondary_switch_to_el1
#endif
Missing bl?
Looks so.
Is there any reason to have the SWITCH_TO_EL1 option other than for
debugging?
Good question. I will let Arnab to comment here.
EL2 is the preferred EL to boot at for Linux and Xen (it gives far
more flexibility), and if dropping to EL1 is necessary I think it
would make more sense as a run-time option than a compile-time option.
I don't think we plan to boot Linux in EL1. This is primarily here to
maintain uniformity with "arch/arm/lib/bootm.c". If I remove it from
here and it was ever defined in the config, then the boot core would
enter Linux in EL1 while the secondaries entered Linux in EL2. I don't
know if that breaks anything...
The run-time option seems interesting and it would definitely work for
the primary core which could access the u-boot env variables and such
but the secondaries are executing assembly and the communication between
cores is fairly primitive (sgi's and sev's etc) so this might require a
little bit of work.
If you have any thoughts on how we can go about it, I would be glad to
do some research, but that seems to be the topic for a separate patchset
I guess.
- /*
- * All processors will enter EL2 and optionally EL1.
+slave_cpu:
+ wfe
+#ifdef CONFIG_FSL_SMP_RELEASE_ALL
+ /* All cores are released from the address in the 1st spin
table
+ * element
*/
- bl armv8_switch_to_el2
-#ifdef CONFIG_ARMV8_SWITCH_TO_EL1
- bl armv8_switch_to_el1
+ ldr x1, =__spin_table
+ ldr x0, [x1]
+#else
+ ldr x0, [x11]
+#endif
+ cbz x0, slave_cpu
Similarly is there any reason to have the option of a single release
addr if we can support unique addresses?
I think it was used by Linux for some ARM parts. I personally not a fun of
using single release. But if it makes everyone happy, I can keep it.
We followed the standard ARMv8 foundation model DTS initially which along with
others
supported a single release address for all the cores. So, we wanted to comply
to the same.
Yes this is left over code which should (and will) be cleaned up.
[...]
diff --git a/arch/arm/cpu/armv8/fsl-lsch3/mp.c
b/arch/arm/cpu/armv8/fsl-lsch3/mp.c
new file mode 100644
index 0000000..b29eda9
--- /dev/null
+++ b/arch/arm/cpu/armv8/fsl-lsch3/mp.c
@@ -0,0 +1,172 @@
+/*
+ * Copyright 2014 Freescale Semiconductor, Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <common.h>
+#include <asm/io.h>
+#include <asm/system.h>
+#include <asm/io.h>
+#include <asm/arch-fsl-lsch3/immap_lsch3.h>
+#include "mp.h"
+
+DECLARE_GLOBAL_DATA_PTR;
+
+void *get_spin_tbl_addr(void)
+{
+ void *ptr = (void *)&__spin_table;
+
+ return ptr;
+}
I don't think the cast is necessary here. As far as I can see this
could be just:
void *get_spin_tbl_addr(void)
{
return &__spin_table;
}
Let me try that.
[...]
diff --git a/arch/arm/include/asm/macro.h
b/arch/arm/include/asm/macro.h index f77e4b8..0009c28 100644
--- a/arch/arm/include/asm/macro.h
+++ b/arch/arm/include/asm/macro.h
@@ -105,6 +105,98 @@ lr .req x30
cbz \xreg1, \master_label
.endm
+.macro armv8_switch_to_el2_m, xreg1
+ mov \xreg1, #0x5b1 /* Non-secure EL0/EL1 | HVC | 64bit EL2
*/
+ msr scr_el3, \xreg1
When dropping to EL1 from EL2 we disable HVC via HCR_EL2; presumably
due to lack of a handler. Would it make sense to do similarly here and
disable SMC here until we have a user (e.g. PSCI)?
I will let Arnab to comment here.
SMC's are disabled (we are setting bit 7, the SMD bit). The comment does
not capture this. I'll fix it.
Thanks
Arnab
In my view u-boot should be similar to UEFI and both should drop only to EL2
before booting either Linux or Hypervisor. UEFI code doesn't switch to EL1
before invoking Linux. Thoughts?
York
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
