[U-Boot] [Patch v2 3/5] armv8/fsl-lsch3: Release secondary cores from boot hold off with Boot Page
York Sun
yorksun at freescale.com
Thu Aug 21 20:32:53 CEST 2014
On 08/21/2014 06:47 AM, Mark Rutland wrote:
> Hi York,
>
> I have mostly minor comments this time; this is looking pretty good.
>
> 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 <yorksun at freescale.com>
>> Signed-off-by: Arnab Basu <arnab.basu at 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.
>
>> + 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.
>
>> + 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.
>
>> +
>> + 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.
>
>>
>> - /*
>> - * 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.
>
> [...]
>
>> 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.
York
More information about the U-Boot
mailing list