[U-Boot] [PATCH 06/11] imx: imx-common: introduce boot auxiliary core

[Peng Fan <van.freenix@gmail.com>]

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.fan@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.li@nxp.com>
>>>> Signed-off-by: Peng Fan <peng.fan@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.
>> 
>