Hi Stefan, Sorry for this late reply.
On Wed, Jan 13, 2016 at 12:45:05PM -0800, Stefan Agner wrote: >Hi, > >I would like to keep the discussion going and shed some light on the >image format introduced here, see below... > >On 2016-01-07 00:38, Peng Fan wrote: >> Hi Stefan, >> On Wed, Jan 06, 2016 at 10:59:17PM -0800, Stefan Agner wrote: >>>On 2016-01-04 21:56, Peng Fan wrote: >>>> From: Peng Fan <peng....@nxp.com> >>>> >>>> To boot a auxiliary core in asymmetric multicore system, introduce the >>>> new command "bootaux" to do it. Example of boot auxliary core from >>>> 0x70000000 where stores the boot head information that should be >>>> parsed by auxiliary core, "bootaux 0x70000000". >>> >>>This reminds me of a question which was nagging me lately: >>>Is the M4 core of SoloX/MX7 running a boot ROM? Or who/what is "parsing >>>the boot head information"? >> >> There is no bootrom for m4 core. The bootimage for M4 contains stack >> info and pc info. bootaux command will use the info extracted from bootimage. > > >So the image expected by bootaux is really a raw binary image, with the >only notion that the first two words need to be the stack pointer and >the reset handler (firmware entry point). Yeah. > >U-Boot has other commands which work with "raw" images, such as the >bootz command. The bootz command also expects a certain "raw" format, >hence I guess it is ok to introduce something like that also for the >auxiliary core. Here bootaux will not let uboot lose control. bootz will let os get the control to soc. But I agree we add a simliar command, I personally think boot[aux] is good :). > >Also, since every standard Cortex-M4 vector table begins with SP and >reset handler, it is very likely that a firmwares linker file put the >vector table also at the beginning of the image. Otherwise, one would >need to adopt the linker file to create a "compatible" image. Btw, as Ye >Li pointed out, the CPU needs those two words located at 0x0000000000, >this is what bootaux is actually doing, coping the information to the >right location for the M4 CPU. As an alternative, SCB_VTOR could be used >to point to the vector table. However that can only be controlled from >the M4 itself, hence is not an option for the bootaux command (as I did >it for the libopencm3 Cortex-M4 support for Vybrid, see >https://github.com/libopencm3/libopencm3/blob/master/lib/vf6xx/vector_chipset.c). > >However, IMHO, this patch should at least add this >information/restriction to a README and maybe even give a hint in the >help text what is expected here.... Add a README for bootaux command? or you mean the image format? > >The other restriction is that the binary needs to be loaded to the >location where the firmware has been linked to. Since the firmware is a >"raw" image, this information need to be carried along the binary (I >need to know that binary xyz.bin is linked to work at memory location >0x7F8000...). This is different to the bootz command: A raw Linux kernel >zImage has a position independent loader at the very beginning and hence >can be placed anywhere. Good point. How about let bootaux to handle libz compressed M4 image? > >I don't really like that this information need to be carried along >separately. > >In our downstream U-Boot for Vybrid, which has a M4 core too, I >introduced a m4boot command which makes use of the FIT image format to >carry this information, see: >http://git.toradex.com/cgit/u-boot-toradex.git/commit/?h=2015.04-toradex-next&id=d1d416f272e840e8139aec911f89a70fe5523eb2 > >It comes with the cost that each firmware need to be packed into a FIT >image using mkimage, but then one can just call m4boot <address of >image> and the command will make sure that the image is placed at the >right location in memory. > >Another possible option would be to introduce elf support. I like this >approach since the Linux framework remoteproc also supports loading elf >firmwares, hence this would align nicely with what has been done in the >Linux kernel. > >The elf format has all information required to load the firmware: >Sections and their location (in M4 terms) as well as the start address. >The platform data would only need to have a translation table for the >section addresses (0x1fff8000 <=> 0x7F8000) to be able to load the >firmware to the correct location. It comes with the cost of coping the >firmware to the right location, but it would be much easier to handle >elf firmware files. If we need to let uboot support more functions such as interact with M4 cores, remoteproc maybe a better choice. For simple usage only needs to boot M4, bootaux is enough, I think. Regards, Peng. > >Other opinions? > >One more thing below: > >> >> See the following code: >> " >> int arch_auxiliary_core_up(u32 core_id, u32 boot_private_data) >> { >> struct src *src_reg; >> u32 stack, pc; >> >> if (!boot_private_data) >> return 1; >> >> stack = *(u32 *)boot_private_data; >> pc = *(u32 *)(boot_private_data + 4); >> >> /* Set the stack and pc to M4 bootROM */ >> writel(stack, M4_BOOTROM_BASE_ADDR); >> writel(pc, M4_BOOTROM_BASE_ADDR + 4); >> >> /* Enable M4 */ >> src_reg = (struct src *)SRC_BASE_ADDR; >> setbits_le32(&src_reg->scr, 0x00400000); >> clrbits_le32(&src_reg->scr, 0x00000010); >> >> return 0; >> } >> >> " >> >>> >>> >>>> Introduce Kconfig option IMX_BOOTAUX. >>>> >>>> Signed-off-by: Ye.Li <ye...@nxp.com> >>>> Signed-off-by: Peng Fan <peng....@nxp.com> >>>> --- >>>> arch/arm/imx-common/Kconfig | 6 ++++ >>>> arch/arm/imx-common/Makefile | 1 + >>>> arch/arm/imx-common/imx_bootaux.c | 59 >>>> +++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 66 insertions(+) >>>> create mode 100644 arch/arm/imx-common/imx_bootaux.c >>>> >>>> diff --git a/arch/arm/imx-common/Kconfig b/arch/arm/imx-common/Kconfig >>>> index c4f48bb..1b7da5a 100644 >>>> --- a/arch/arm/imx-common/Kconfig >>>> +++ b/arch/arm/imx-common/Kconfig >>>> @@ -11,3 +11,9 @@ config IMX_RDC >>>> i.MX Resource domain controller is used to assign masters >>>> and peripherals to differet domains. This can be used to >>>> isolate resources. >>>> + >>>> +config IMX_BOOTAUX >>>> + bool "Support boot auxiliary core" >>>> + depends on ARCH_MX7 || ARCH_MX6 >>>> + help >>>> + bootaux [addr] to boot auxiliary core. >>>> diff --git a/arch/arm/imx-common/Makefile b/arch/arm/imx-common/Makefile >>>> index 568f41c..30e66ba 100644 >>>> --- a/arch/arm/imx-common/Makefile >>>> +++ b/arch/arm/imx-common/Makefile >>>> @@ -28,6 +28,7 @@ obj-y += cache.o init.o >>>> obj-$(CONFIG_CMD_SATA) += sata.o >>>> obj-$(CONFIG_IMX_VIDEO_SKIP) += video.o >>>> obj-$(CONFIG_IMX_RDC) += rdc-sema.o >>>> +obj-$(CONFIG_IMX_BOOTAUX) += imx_bootaux.o >>>> obj-$(CONFIG_SECURE_BOOT) += hab.o >>>> endif >>>> ifeq ($(SOC),$(filter $(SOC),vf610)) >>>> diff --git a/arch/arm/imx-common/imx_bootaux.c >>>> b/arch/arm/imx-common/imx_bootaux.c >>>> new file mode 100644 >>>> index 0000000..da424a7 >>>> --- /dev/null >>>> +++ b/arch/arm/imx-common/imx_bootaux.c >>>> @@ -0,0 +1,59 @@ >>>> +/* >>>> + * Copyright (C) 2016 Freescale Semiconductor, Inc. >>>> + * >>>> + * SPDX-License-Identifier: GPL-2.0+ >>>> + */ >>>> + >>>> +#include <common.h> >>>> +#include <command.h> >>>> + >>>> +/* Allow for arch specific config before we boot */ >>>> +static int __arch_auxiliary_core_up(u32 core_id, u32 boot_private_data) >>>> +{ >>>> + /* please define platform specific arch_auxiliary_core_up() */ >>>> + return CMD_RET_FAILURE; >>>> +} >>>> + >>>> +int arch_auxiliary_core_up(u32 core_id, u32 boot_private_data) >>>> + __attribute__((weak, alias("__arch_auxiliary_core_up"))); >>>> + >>>> +/* Allow for arch specific config before we boot */ >>>> +static int __arch_auxiliary_core_check_up(u32 core_id) >>>> +{ >>>> + /* please define platform specific arch_auxiliary_core_check_up() */ >>>> + return 0; >>>> +} >>>> + >>>> +int arch_auxiliary_core_check_up(u32 core_id) >>>> + __attribute__((weak, alias("__arch_auxiliary_core_check_up"))); >>>> + >>>> +int do_bootaux(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>>> +{ >>>> + ulong addr; >>>> + int ret, up; >>>> + >>>> + if (argc < 2) >>>> + return CMD_RET_USAGE; >>>> + >>>> + up = arch_auxiliary_core_check_up(0); >>>> + if (up) { >>>> + printf("## Auxiliary core is already up\n"); >>>> + return CMD_RET_SUCCESS; >>>> + } >>>> + >>>> + addr = simple_strtoul(argv[1], NULL, 16); >>>> + >>>> + printf("## Starting auxiliary core at 0x%08lX ...\n", addr); >>>> + > >A dcache flush is required after loading the firmware. This could be >also part of this command e.g. by using flush_dcache_all. > >If we would make use of the elf format, then the main CPU would copy the >data to the target memory address, and we could explicitly flush only >the range required by the auxiliary core. > >-- >Stefan > >>>> + ret = arch_auxiliary_core_up(0, addr); >>>> + if (ret) >>>> + return CMD_RET_FAILURE; >>>> + >>>> + return CMD_RET_SUCCESS; >>>> +} >>>> + >>>> +U_BOOT_CMD( >>>> + bootaux, CONFIG_SYS_MAXARGS, 1, do_bootaux, >>>> + "Start auxiliary core", >>> >>>What kind of image are supported by the bootaux command? Afaik, this is >>>some special/binary only format right? >> >> Yeah. I just got the image from our rtos team, I do not have detail >> info about the format. >> > > > >>> >>>Probably another discussion, but before polluting the command namespace: >>>Is that what we generally want? >> >> Hmm, bootaux invokes >> arch_auxiliary_core_check_up >> arch_auxiliary_core_u >> >> And arch code takes the responsibility to implement these two functions. >> >> We try to make this common, but not sure whether this is ok for other SoCs. >> So I move the command to arch/arm/imx-common >> >> Thanks, >> Peng. >> > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot