[U-Boot] [Patch v2 3/5] armv8/fsl-lsch3: Release secondary cores from boot hold off with Boot Page

[Mark Rutland <mark.rutland@arm.com>]

Hi Bhupesh,

[...]

> > >> 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

Hi. As Arnab replied, it's only the CPU nodes themselves (e.g.
/cpus/cpu at 0) that have device_type = "cpu". The /cpus node does not have
a device_type at all.

The /cpus node can always be found by path however, as the name is
special.

[...]

> > >> --- 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.
> 
> 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.

Ok. I was only confused by the fact the label didn't seem to be used
anywhere, and it sounds like it can be dropped for now?

[...]

> > >> -       /*
> > >> -        * 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.

As I mentioned elsewhere, the existing DTS aren't good examples. The
fact that isn't clear is a problem on the Linux side, and I'm sorry that
they have misled you.

Using unique addresses is preferred. Sharing a single address should be
discouraged.

[...]

> > >> 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.
> 
> 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?

That sounds ideal to me.

In general I'd hope any bootloader would hand-off to the OS (Linux, Xen,
whatever) at the highest-privileged non-secure mode. So EL2 if present,
otherwise EL1N.

We don't have any legacy to worry about with old kernels that cannot
boot at EL1N, so I don't see a good reason to drop to EL1N if EL2 is
available.

If the option of booting at EL1 becomes necessary for some piece of
software, I'd rather that were a run-time option and that piece of
software were treated as the special case.

Thanks,
Mark.