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

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

[David Brandt]

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>
+
+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);
+
+	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);
+	if (ret)
+		return 1;
+
+	return 0;
+}
+
+
+BAREBOX_CMD_START(bootaux)
+	.cmd		= do_bootaux,
+	BAREBOX_CMD_DESC("boot auxiliary core")
+	BAREBOX_CMD_OPTS("ADDR [CORE]")
+	BAREBOX_CMD_GROUP(CMD_GRP_BOOT)
+BAREBOX_CMD_END
-- 
2.17.1


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

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

[Roland Hieber]

Hi David,

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

I think there is no need to #ifdef this away and create niches for code
to hide from getting caught in compiler warnings while refactoring. :)
The ARM Makefile already sets

	CPPFLAGS += -fdata-sections -ffunction-sections
	LDFLAGS_barebox += --gc-sections

so all unneeded functions will get stripped from the final binary at
link time.

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

 - Roland

> +
>  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>
> +
> +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);
> +
> +	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);
> +	if (ret)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +
> +BAREBOX_CMD_START(bootaux)
> +	.cmd		= do_bootaux,
> +	BAREBOX_CMD_DESC("boot auxiliary core")
> +	BAREBOX_CMD_OPTS("ADDR [CORE]")
> +	BAREBOX_CMD_GROUP(CMD_GRP_BOOT)
> +BAREBOX_CMD_END
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Roland Hieber                     | r.hieber@pengutronix.de     |
Pengutronix e.K.                  | https://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim | Phone: +49-5121-206917-5086 |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |

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

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

[Andrey Smirnov]

On Mon, Mar 11, 2019 at 6:41 AM David Brandt <d.brandt@phytec.de> 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.
>

Not saying that there's anything wrong with this patch, but I am
guessing that upstream Linux will eventually implement this
functionality using remoteproc framework. Maybe now would be a good
time to add a very basic version of it to Barebox? Another option to
make this a little bit more flexible would be to implement this using
firmwaremgr framework similar to
drivers/frimware/{socfpga.c,altera_serial.c}. Anyway, just a couple of
suggestions that I had.

Thanks,
Andrey Smirnov

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

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

[Oleksij Rempel]

Hi,

On 12.03.19 02:37, Andrey Smirnov wrote:
> On Mon, Mar 11, 2019 at 6:41 AM David Brandt <d.brandt@phytec.de> 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.
>>
> 
> Not saying that there's anything wrong with this patch, but I am
> guessing that upstream Linux will eventually implement this
> functionality using remoteproc framework. Maybe now would be a good
> time to add a very basic version of it to Barebox? Another option to
> make this a little bit more flexible would be to implement this using
> firmwaremgr framework similar to
> drivers/frimware/{socfpga.c,altera_serial.c}. Anyway, just a couple of
> suggestions that I had.

I agree with remoteproc here.

The task of remoteproc on linux is:
- parse specially formated ELF file:
- Register needed resources extracted from ELF
- prepare clocks
- do address translation
- kick on the CPU

The main issue of linux part is to dunamically patch devicetree and tell master linux 
instance about IP Cores which was migrated for master CPU to the slave CPU.

With barebox this issues can be solved. First of all, barebox already has ELF parser. So 
it can be reused. The drivers/remoteproc/imx_rproc.c from linux can probably be ported as 
is. As soon as first step done, we would be able to patch devicetree in barebox and gave 
it over to linux. So, we will be able to tell linux to let some clocks on and do not touch 
reserved memory areas.

Kind regards,
Oleksij Rempel

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

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

[Sascha Hauer]

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

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

[David Brandt]

Hi Sascha,

Am 13.03.19 um 09:32 schrieb Sascha Hauer:
> 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.
Will do in v2.
>
>> +
>> +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.
I already thought about using getopt, if that fits the barebox style
better I can implement it in v2.

Just followed the syntax from u-boot from NXP.

>
>> +
>> +	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
>
For me it was clear that the first data in a loaded binary is the interrupt
table with the top of stack and reset handler in the start, but actually
this does not have to be true.
You are right in that it should be clarified that this points to the interrupt
table and that the reset handler is started, I will add help with that.
Since there always will be an interrupt table, I don't think there must be
an option to pass the entry point or am I overlooking a use case?

David


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

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

[David Brandt]

This patch add the bootaux command which starts
auxiliary cores of iMX processors, currently only
iMX8MQ. This is based on the u-boot-imx command.

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>
---
Changes v1->v2:
- removed ifdef around imx_auxiliary functions
  in arch/arm/mach-imx/imx8mq.c
- cleaned up includes in commands/bootaux.c
- clarified what ADDR is and what is done
- core is now specified with -c option
- added some checks and message to hint what
  could be wrong
- use kstrtouint/kstrtoul
---
 arch/arm/mach-imx/imx8mq.c               | 37 ++++++++++++
 arch/arm/mach-imx/include/mach/generic.h |  4 ++
 commands/Kconfig                         | 16 ++++++
 commands/Makefile                        |  1 +
 commands/bootaux.c                       | 73 ++++++++++++++++++++++++
 5 files changed, 131 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..98c62b58d 100644
--- a/arch/arm/mach-imx/imx8mq.c
+++ b/arch/arm/mach-imx/imx8mq.c
@@ -124,3 +124,40 @@ static int imx8mq_report_hdmi_firmware(void)
 	return 0;
 }
 console_initcall(imx8mq_report_hdmi_firmware);
+
+#define FSL_SIP_SRC             0xC2000005
+#define FSL_SIP_SRC_M4_START    0x00
+#define FSL_SIP_SRC_M4_STARTED  0x01
+int imx_auxiliary_core_up(unsigned int core_id, ulong boot_private_data)
+{
+	u32 stack, pc;
+	struct arm_smccc_res res;
+
+	if (core_id || !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(unsigned int core_id)
+{
+	struct arm_smccc_res res;
+
+	if (core_id)
+		return -EINVAL;
+
+	arm_smccc_smc(FSL_SIP_SRC, FSL_SIP_SRC_M4_STARTED,
+			0, 0, 0, 0, 0, 0, &res);
+	return res.a0;
+}
diff --git a/arch/arm/mach-imx/include/mach/generic.h b/arch/arm/mach-imx/include/mach/generic.h
index be58da4da..c5929dcab 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(unsigned int core_id, ulong boot_private_data);
+int imx_auxiliary_core_check_up(unsigned int 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..fe960c029 100644
--- a/commands/Kconfig
+++ b/commands/Kconfig
@@ -270,6 +270,22 @@ 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.
+
+	  Usage: bootaux [-c CORE] ADDRESS
+
+	  ADDRESS points to an isr table from which the
+	  reset handler will be started.
+
+	  Options:
+	  -c 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..517f49ff3
--- /dev/null
+++ b/commands/bootaux.c
@@ -0,0 +1,73 @@
+/* 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 <common.h>
+#include <mach/generic.h>
+
+static int do_bootaux(int argc, char *argv[])
+{
+	ulong addr;
+	unsigned int core = 0;
+	int ret, up, opt;
+
+	while ((opt = getopt(argc, argv, "c:")) > 0) {
+		switch (opt) {
+		case 'c':
+			if (kstrtouint(optarg, 0, &core))
+				return COMMAND_ERROR_USAGE;
+			break;
+		default:
+			return COMMAND_ERROR_USAGE;
+		}
+	}
+
+	if (optind != argc - 1 ||
+	    kstrtoul(argv[optind], 0, &addr))
+		return COMMAND_ERROR_USAGE;
+
+	up = imx_auxiliary_core_check_up(core);
+	if (up == -EINVAL) {
+		printf("Unknown auxiliary core %u\n", core);
+		return 1;
+	} else if (up) {
+		printf("Auxiliary core %u is already up\n", core);
+		return 0;
+	}
+
+	printf("Starting auxiliary core %u at 0x%08lX ...\n", core, addr);
+
+	ret = imx_auxiliary_core_up(core, addr);
+	if (ret) {
+		printf("Error starting auxiliary core %u\n", core);
+		return 1;
+	}
+
+	return 0;
+}
+
+BAREBOX_CMD_HELP_START(bootaux)
+BAREBOX_CMD_HELP_TEXT("Boots the auxiliary core with the isr table at")
+BAREBOX_CMD_HELP_TEXT("ADDR, setting the PC to the reset handler entry.")
+BAREBOX_CMD_HELP_TEXT("Passing the load address of the M4 code works,")
+BAREBOX_CMD_HELP_TEXT("as long as the isr table is the first section.")
+BAREBOX_CMD_HELP_TEXT("")
+BAREBOX_CMD_HELP_TEXT("Options:")
+BAREBOX_CMD_HELP_OPT("-c CORE", "select core to start (default 0)")
+BAREBOX_CMD_HELP_END
+
+BAREBOX_CMD_START(bootaux)
+	.cmd		= do_bootaux,
+	BAREBOX_CMD_DESC("boot auxiliary core with isr table at ADDR")
+	BAREBOX_CMD_OPTS("[-c CORE] ADDR")
+	BAREBOX_CMD_GROUP(CMD_GRP_BOOT)
+	BAREBOX_CMD_HELP(cmd_bootaux_help)
+BAREBOX_CMD_END
-- 
2.17.1


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

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

[Sascha Hauer]

Hi David,

On Wed, Mar 13, 2019 at 03:41:09PM +0100, David Brandt wrote:
> This patch add the bootaux command which starts
> auxiliary cores of iMX processors, currently only
> iMX8MQ. This is based on the u-boot-imx command.
> 
> 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>
> ---
> Changes v1->v2:
> - removed ifdef around imx_auxiliary functions
>   in arch/arm/mach-imx/imx8mq.c
> - cleaned up includes in commands/bootaux.c
> - clarified what ADDR is and what is done
> - core is now specified with -c option
> - added some checks and message to hint what
>   could be wrong
> - use kstrtouint/kstrtoul
> ---
>  arch/arm/mach-imx/imx8mq.c               | 37 ++++++++++++
>  arch/arm/mach-imx/include/mach/generic.h |  4 ++
>  commands/Kconfig                         | 16 ++++++
>  commands/Makefile                        |  1 +
>  commands/bootaux.c                       | 73 ++++++++++++++++++++++++
>  5 files changed, 131 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..98c62b58d 100644
> --- a/arch/arm/mach-imx/imx8mq.c
> +++ b/arch/arm/mach-imx/imx8mq.c
> @@ -124,3 +124,40 @@ static int imx8mq_report_hdmi_firmware(void)
>  	return 0;
>  }
>  console_initcall(imx8mq_report_hdmi_firmware);
> +
> +#define FSL_SIP_SRC             0xC2000005
> +#define FSL_SIP_SRC_M4_START    0x00
> +#define FSL_SIP_SRC_M4_STARTED  0x01
> +int imx_auxiliary_core_up(unsigned int core_id, ulong boot_private_data)
> +{
> +	u32 stack, pc;
> +	struct arm_smccc_res res;
> +
> +	if (core_id || !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;
> +}

I think this needs more work. First of all we have
imx_auxiliary_core_up(), but this function is i.MX8 specific, i.MX6/7
need other methods. Similar to this "bootaux" is a generic command name,
but here i.MX specific functions are called. The typical U-Boot way
would be to add #ifdefs to it until it works for other known cases and
until the code is completely unreadable. Fortunately we don't do U-Boot
here, so we'll need some kind of cpu core registration function and a
generic command could show a list of registered cores and start a
particular one.
That would fit well with the already existing devicetree binding for the
auxiliary i.MX cores, see bindings/remoteproc/imx-rproc.txt, compatible
"fsl,imx7d-cm4" and "fsl,imx6sx-cm4". The code for the aux cores could
then be simply written as regular drivers.

Also we should make sure that the memory area used for the aux core
isn't passed to Linux. I would expect some convenience here for the user
so that he doesn't have to adjust the devicetrees manually.

I agree with Oleksij here that remoteproc is the right approach for
doing this.

Overall I think these aux cores are a hot topic and we should do it
right rather than fast. Merging a trivial approach for it would just
give the impression that a SoC specific quick hack is the way to go
for others, but I'm quite sure we can do better.

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