Re: [PATCH] commands: bootaux: New command bootaux for iMX

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Mar 11, 2019 at 02:41:25PM +0100, David Brandt wrote:
> This patch adds the bootaux command which starts
> auxiliary cores of iMX processors, currently only
> for iMX8MQ. This is based on the u-boot-imx
> command and shares the behaviour.
> 
> The currently unnecessary parameter for the core
> to start is necessary for the iMX8QM and other
> processors having multiple auxiliary cores.
> 
> Signed-off-by: David Brandt <d.brandt@phytec.de>
> ---
>  arch/arm/mach-imx/imx8mq.c               | 37 +++++++++++++++
>  arch/arm/mach-imx/include/mach/generic.h |  4 ++
>  commands/Kconfig                         | 14 ++++++
>  commands/Makefile                        |  1 +
>  commands/bootaux.c                       | 58 ++++++++++++++++++++++++
>  5 files changed, 114 insertions(+)
>  create mode 100644 commands/bootaux.c
> 
> diff --git a/arch/arm/mach-imx/imx8mq.c b/arch/arm/mach-imx/imx8mq.c
> index 3f6b433a5..9e60a8bd7 100644
> --- a/arch/arm/mach-imx/imx8mq.c
> +++ b/arch/arm/mach-imx/imx8mq.c
> @@ -123,4 +123,41 @@ static int imx8mq_report_hdmi_firmware(void)
>  
>  	return 0;
>  }
> +
> +#ifdef CONFIG_CMD_BOOTAUX
> +
> +#define FSL_SIP_SRC             0xC2000005
> +#define FSL_SIP_SRC_M4_START    0x00
> +#define FSL_SIP_SRC_M4_STARTED  0x01
> +int imx_auxiliary_core_up(u32 core_id, ulong boot_private_data)
> +{
> +	u32 stack, pc;
> +	struct arm_smccc_res res;
> +
> +	if (!boot_private_data)
> +		return -EINVAL;
> +
> +	stack = *(u32 *)boot_private_data;
> +	pc = *(u32 *)(boot_private_data + 4);
> +
> +	/* Set the stack and pc to M4 bootROM */
> +	writel(stack, MX8MQ_M4_BOOTROM_BASE_ADDR);
> +	writel(pc, MX8MQ_M4_BOOTROM_BASE_ADDR + 4);
> +
> +	/* Enable M4 */
> +	arm_smccc_smc(FSL_SIP_SRC, FSL_SIP_SRC_M4_START,
> +			0, 0, 0, 0, 0, 0, &res);
> +
> +	return res.a0;
> +}
> +
> +int imx_auxiliary_core_check_up(u32 core_id)
> +{
> +	struct arm_smccc_res res;
> +	arm_smccc_smc(FSL_SIP_SRC, FSL_SIP_SRC_M4_STARTED,
> +			0, 0, 0, 0, 0, 0, &res);
> +	return res.a0;
> +}
> +#endif
> +
>  console_initcall(imx8mq_report_hdmi_firmware);
> diff --git a/arch/arm/mach-imx/include/mach/generic.h b/arch/arm/mach-imx/include/mach/generic.h
> index be58da4da..59fcbc767 100644
> --- a/arch/arm/mach-imx/include/mach/generic.h
> +++ b/arch/arm/mach-imx/include/mach/generic.h
> @@ -59,6 +59,10 @@ void imx6ul_cpu_lowlevel_init(void);
>  void imx7_cpu_lowlevel_init(void);
>  void vf610_cpu_lowlevel_init(void);
>  
> +/* processor specific functions to boot auxiliary core */
> +int imx_auxiliary_core_up(u32 core_id, ulong boot_private_data);
> +int imx_auxiliary_core_check_up(u32 core_id);
> +
>  /* There's a off-by-one betweem the gpio bank number and the gpiochip */
>  /* range e.g. GPIO_1_5 is gpio 5 under linux */
>  #define IMX_GPIO_NR(bank, nr)		(((bank) - 1) * 32 + (nr))
> diff --git a/commands/Kconfig b/commands/Kconfig
> index 4f5d84ac1..c39a84f23 100644
> --- a/commands/Kconfig
> +++ b/commands/Kconfig
> @@ -270,6 +270,20 @@ config CMD_AT91_BOOT_TEST
>  	  -j ADDR  jump address
>  	  -s SRAM  SRAM device (default /dev/sram0)
>  
> +config CMD_BOOTAUX
> +	tristate
> +	default y
> +	depends on ARCH_IMX8MQ
> +	prompt "bootaux"
> +	help
> +	  Boot auxiliary core on NXP iMXP Processors.
> +
> +	  Usage: bootaux ADDRESS [CORE]
> +
> +	  Options:
> +	  ADDRESS  load address
> +	  CORE  auxiliary core to start, defaults to 0
> +
>  config CMD_BOOT_ORDER
>  	tristate
>  	depends on ARCH_OMAP4
> diff --git a/commands/Makefile b/commands/Makefile
> index 358671bb5..c72b52c27 100644
> --- a/commands/Makefile
> +++ b/commands/Makefile
> @@ -1,6 +1,7 @@
>  obj-$(CONFIG_STDDEV)		+= stddev.o
>  obj-$(CONFIG_CMD_DIGEST)	+= digest.o
>  obj-$(CONFIG_COMPILE_HASH)	+= hashsum.o
> +obj-$(CONFIG_CMD_BOOTAUX)	+= bootaux.o
>  obj-$(CONFIG_CMD_BOOTM)		+= bootm.o
>  obj-$(CONFIG_CMD_UIMAGE)	+= uimage.o
>  obj-$(CONFIG_CMD_LINUX16)	+= linux16.o
> diff --git a/commands/bootaux.c b/commands/bootaux.c
> new file mode 100644
> index 000000000..62209bf3e
> --- /dev/null
> +++ b/commands/bootaux.c
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * bootaux - boot auxiliary core
> + *
> + * Copyright (C) 2019 David Brandt <d.brandt@phytec.de>
> + *
> + * Based on code from u-boot-imx from Freescale Semiconductor, Inc.
> + *
> + */
> +
> +#include <command.h>
> +#include <getopt.h>
> +#include <memory.h>
> +#include <malloc.h>
> +#include <common.h>
> +#include <errno.h>
> +#include <memtest.h>
> +#include <mmu.h>
> +#include <mach/generic.h>

Please clean up the include list. Nothing from malloc.h, memtest.h and
mmu.h seems to be used here.

> +
> +static int do_bootaux(int argc, char *argv[])
> +{
> +	ulong addr;
> +	int ret, up;
> +	u32 core = 0;
> +
> +	if (argc < 2) {
> +		printf("no load address given\n");
> +		return 1;
> +	}
> +
> +	if (argc > 2)
> +		core = strict_strtoul(argv[2], NULL, 10);

We do not have strict_strtoul().

Please do not specify the base. Normally we provide numbers as decimal
and addresses as hexadecimal, but there's no reason to force this. If I
specify a number without '0x' prefix I expect it to be treated as
decimal.

Consider using getopt() here for the core parameter. Commands with multiple
positional and sometimes even optional arguments tend to get very creepy
over time.

> +
> +	up = imx_auxiliary_core_check_up(core);
> +	if (up) {
> +		printf("Auxiliary core %d is already up\n", core);
> +		return 0;
> +	}
> +
> +	addr = strict_strtoul(argv[1], NULL, 16);
> +
> +	printf("Starting auxiliary core %d at 0x%08lX ...\n", core, addr);
> +
> +	ret = imx_auxiliary_core_up(core, addr);

How is this command supposed to be used? If I read the code correctly
'addr' is not the address the aux core will start from, but is instead
treated as a pointer to a struct of type:

struct aux {
	void *stack;
	void *start_address;
};

Who sets this struct up? Wouldn't it be better to specify start_address
and stack directly to the command? At least I would expect some
information at which address the core is actually started and where its
stack pointer is.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox