diff mbox series

[PATCHv4,10/14] cmd: cpu: add release subcommand

Message ID 20240730073003.28892-11-Zhiqiang.Hou@nxp.com
State Superseded
Delegated to: Fabio Estevam
Headers show
Series Add a subcommand 'release' to cmd/cpu.c | expand

Commit Message

Z.Q. Hou July 30, 2024, 7:29 a.m. UTC 
From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>

Add a new subcommand 'release' to bring up a core to run baremetal
and RTOS applications.

For example on i.MX8M Plus EVK, release the LAST core to run a RTOS
application, passing the sequence number of the CPU core to release,
here it is 3:
    u-boot=> cpu list
      0: cpu@0      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
      1: cpu@1      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 30C
      2: cpu@2      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
      3: cpu@3      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C

    u-boot=> load mmc 1:2 c0000000 /hello_world.bin
    66008 bytes read in 5 ms (12.6 MiB/s)
    u-boot=> dcache flush; icache flush
    u-boot=> cpu release 3 c0000000
    Released CPU core (mpidr: 0x3) to address 0xc0000000

Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
V4:
 - Updated the change logs.

 cmd/cpu.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

Comments

Simon Glass July 31, 2024, 2:38 p.m. UTC | #1 
Hi Zhiqiang,

On Tue, 30 Jul 2024 at 01:09, Zhiqiang Hou <Zhiqiang.Hou@nxp.com> wrote:
>
> From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
>
> Add a new subcommand 'release' to bring up a core to run baremetal
> and RTOS applications.
>
> For example on i.MX8M Plus EVK, release the LAST core to run a RTOS
> application, passing the sequence number of the CPU core to release,
> here it is 3:
>     u-boot=> cpu list
>       0: cpu@0      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
>       1: cpu@1      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 30C
>       2: cpu@2      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
>       3: cpu@3      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
>
>     u-boot=> load mmc 1:2 c0000000 /hello_world.bin
>     66008 bytes read in 5 ms (12.6 MiB/s)
>     u-boot=> dcache flush; icache flush
>     u-boot=> cpu release 3 c0000000
>     Released CPU core (mpidr: 0x3) to address 0xc0000000
>
> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> V4:
>  - Updated the change logs.
>
>  cmd/cpu.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
>
> diff --git a/cmd/cpu.c b/cmd/cpu.c
> index 9e323069b9..2755250756 100644
> --- a/cmd/cpu.c
> +++ b/cmd/cpu.c
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2015 Google, Inc
>   * Written by Simon Glass <sjg@chromium.org>
>   * Copyright (c) 2017 Álvaro Fernández Rojas <noltari@gmail.com>
> + * Copyright 2024 NXP
>   */
>
>  #include <command.h>
> @@ -18,6 +19,19 @@ static const char *cpu_feature_name[CPU_FEAT_COUNT] = {
>         "Device ID",
>  };
>
> +static struct udevice *cpu_find_device(unsigned long cpu_id)
> +{
> +       struct udevice *dev;
> +
> +       for (uclass_first_device(UCLASS_CPU, &dev); dev;
> +            uclass_next_device(&dev)) {
> +               if (cpu_id == dev_seq(dev))
> +                       return dev;
> +       }
> +
> +       return NULL;
> +}
> +
>  static int print_cpu_list(bool detail)
>  {
>         struct udevice *dev;
> @@ -82,10 +96,36 @@ static int do_cpu_detail(struct cmd_tbl *cmdtp, int flag, int argc,
>         return 0;
>  }
>
> +static int do_cpu_release(struct cmd_tbl *cmdtp, int flag, int argc,
> +                         char *const argv[])
> +{
> +       struct udevice *dev;
> +       unsigned long cpu_id;
> +       unsigned long long boot_addr;
> +
> +       if (argc != 3)
> +               return CMD_RET_USAGE;
> +
> +       cpu_id = dectoul(argv[1], NULL);

I just noticed this...normally we use hex for parameters.

> +       dev = cpu_find_device(cpu_id);
> +       if (!dev)
> +               return CMD_RET_FAILURE;
> +
> +       boot_addr = simple_strtoull(argv[2], NULL, 16);
> +
> +       if (cpu_release_core(dev, boot_addr))
> +               return CMD_RET_FAILURE;
> +
> +       return 0;
> +}
> +
>  U_BOOT_LONGHELP(cpu,
>         "list   - list available CPUs\n"
> -       "cpu detail     - show CPU detail");
> +       "cpu detail     - show CPU detail\n"
> +       "cpu release <core ID> <addr>   - Release CPU <core ID> at <addr>\n"
> +       "            <core ID>: the sequence number in list subcommand outputs");
>
>  U_BOOT_CMD_WITH_SUBCMDS(cpu, "display information about CPUs", cpu_help_text,
>         U_BOOT_SUBCMD_MKENT(list, 1, 1, do_cpu_list),
> -       U_BOOT_SUBCMD_MKENT(detail, 1, 0, do_cpu_detail));
> +       U_BOOT_SUBCMD_MKENT(detail, 1, 0, do_cpu_detail),
> +       U_BOOT_SUBCMD_MKENT(release, 3, 0, do_cpu_release));
> --
> 2.17.1
>

Regards,
Simon
Z.Q. Hou Aug. 1, 2024, 2:17 a.m. UTC | #2 
Hi Simon,

> -----Original Message-----
> From: Simon Glass <sjg@chromium.org>
> Sent: Wednesday, July 31, 2024 10:39 PM
> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
> Cc: u-boot@lists.denx.de; trini@konsulko.com; Peng Fan
> <peng.fan@nxp.com>; festevam@gmail.com; marex@denx.de;
> lukma@denx.de; seanga2@gmail.com; xypron.glpk@gmx.de
> Subject: Re: [PATCHv4 10/14] cmd: cpu: add release subcommand
> 
> Hi Zhiqiang,
> 
> On Tue, 30 Jul 2024 at 01:09, Zhiqiang Hou <Zhiqiang.Hou@nxp.com> wrote:
> >
> > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> >
> > Add a new subcommand 'release' to bring up a core to run baremetal and
> > RTOS applications.
> >
> > For example on i.MX8M Plus EVK, release the LAST core to run a RTOS
> > application, passing the sequence number of the CPU core to release,
> > here it is 3:
> >     u-boot=> cpu list
> >       0: cpu@0      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
> >       1: cpu@1      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 30C
> >       2: cpu@2      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
> >       3: cpu@3      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
> >
> >     u-boot=> load mmc 1:2 c0000000 /hello_world.bin
> >     66008 bytes read in 5 ms (12.6 MiB/s)
> >     u-boot=> dcache flush; icache flush
> >     u-boot=> cpu release 3 c0000000
> >     Released CPU core (mpidr: 0x3) to address 0xc0000000
> >
> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > ---
> > V4:
> >  - Updated the change logs.
> >
> >  cmd/cpu.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 42 insertions(+), 2 deletions(-)
> >
> > diff --git a/cmd/cpu.c b/cmd/cpu.c
> > index 9e323069b9..2755250756 100644
> > --- a/cmd/cpu.c
> > +++ b/cmd/cpu.c
> > @@ -3,6 +3,7 @@
> >   * Copyright (c) 2015 Google, Inc
> >   * Written by Simon Glass <sjg@chromium.org>
> >   * Copyright (c) 2017 Álvaro Fernández Rojas <noltari@gmail.com>
> > + * Copyright 2024 NXP
> >   */
> >
> >  #include <command.h>
> > @@ -18,6 +19,19 @@ static const char
> *cpu_feature_name[CPU_FEAT_COUNT] = {
> >         "Device ID",
> >  };
> >
> > +static struct udevice *cpu_find_device(unsigned long cpu_id) {
> > +       struct udevice *dev;
> > +
> > +       for (uclass_first_device(UCLASS_CPU, &dev); dev;
> > +            uclass_next_device(&dev)) {
> > +               if (cpu_id == dev_seq(dev))
> > +                       return dev;
> > +       }
> > +
> > +       return NULL;
> > +}
> > +
> >  static int print_cpu_list(bool detail)  {
> >         struct udevice *dev;
> > @@ -82,10 +96,36 @@ static int do_cpu_detail(struct cmd_tbl *cmdtp, int
> flag, int argc,
> >         return 0;
> >  }
> >
> > +static int do_cpu_release(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                         char *const argv[]) {
> > +       struct udevice *dev;
> > +       unsigned long cpu_id;
> > +       unsigned long long boot_addr;
> > +
> > +       if (argc != 3)
> > +               return CMD_RET_USAGE;
> > +
> > +       cpu_id = dectoul(argv[1], NULL);
> 
> I just noticed this...normally we use hex for parameters.

Yes, but the <core ID> is from the CPU device sequence number listed by the 'cpu list', which is printed in dec. So I think use dec parameter here is reasonable.

Thanks,
Zhiqiang
Simon Glass Aug. 1, 2024, 2:42 p.m. UTC | #3 
Hi Zhiqiang,

On Wed, 31 Jul 2024 at 20:37, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:
>
> Hi Simon,
>
> > -----Original Message-----
> > From: Simon Glass <sjg@chromium.org>
> > Sent: Wednesday, July 31, 2024 10:39 PM
> > To: Z.Q. Hou <zhiqiang.hou@nxp.com>
> > Cc: u-boot@lists.denx.de; trini@konsulko.com; Peng Fan
> > <peng.fan@nxp.com>; festevam@gmail.com; marex@denx.de;
> > lukma@denx.de; seanga2@gmail.com; xypron.glpk@gmx.de
> > Subject: Re: [PATCHv4 10/14] cmd: cpu: add release subcommand
> >
> > Hi Zhiqiang,
> >
> > On Tue, 30 Jul 2024 at 01:09, Zhiqiang Hou <Zhiqiang.Hou@nxp.com> wrote:
> > >
> > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > >
> > > Add a new subcommand 'release' to bring up a core to run baremetal and
> > > RTOS applications.
> > >
> > > For example on i.MX8M Plus EVK, release the LAST core to run a RTOS
> > > application, passing the sequence number of the CPU core to release,
> > > here it is 3:
> > >     u-boot=> cpu list
> > >       0: cpu@0      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
> > >       1: cpu@1      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 30C
> > >       2: cpu@2      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
> > >       3: cpu@3      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
> > >
> > >     u-boot=> load mmc 1:2 c0000000 /hello_world.bin
> > >     66008 bytes read in 5 ms (12.6 MiB/s)
> > >     u-boot=> dcache flush; icache flush
> > >     u-boot=> cpu release 3 c0000000
> > >     Released CPU core (mpidr: 0x3) to address 0xc0000000
> > >
> > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > ---
> > > V4:
> > >  - Updated the change logs.
> > >
> > >  cmd/cpu.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 42 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/cmd/cpu.c b/cmd/cpu.c
> > > index 9e323069b9..2755250756 100644
> > > --- a/cmd/cpu.c
> > > +++ b/cmd/cpu.c
> > > @@ -3,6 +3,7 @@
> > >   * Copyright (c) 2015 Google, Inc
> > >   * Written by Simon Glass <sjg@chromium.org>
> > >   * Copyright (c) 2017 Álvaro Fernández Rojas <noltari@gmail.com>
> > > + * Copyright 2024 NXP
> > >   */
> > >
> > >  #include <command.h>
> > > @@ -18,6 +19,19 @@ static const char
> > *cpu_feature_name[CPU_FEAT_COUNT] = {
> > >         "Device ID",
> > >  };
> > >
> > > +static struct udevice *cpu_find_device(unsigned long cpu_id) {
> > > +       struct udevice *dev;
> > > +
> > > +       for (uclass_first_device(UCLASS_CPU, &dev); dev;
> > > +            uclass_next_device(&dev)) {
> > > +               if (cpu_id == dev_seq(dev))
> > > +                       return dev;
> > > +       }
> > > +
> > > +       return NULL;
> > > +}
> > > +
> > >  static int print_cpu_list(bool detail)  {
> > >         struct udevice *dev;
> > > @@ -82,10 +96,36 @@ static int do_cpu_detail(struct cmd_tbl *cmdtp, int
> > flag, int argc,
> > >         return 0;
> > >  }
> > >
> > > +static int do_cpu_release(struct cmd_tbl *cmdtp, int flag, int argc,
> > > +                         char *const argv[]) {
> > > +       struct udevice *dev;
> > > +       unsigned long cpu_id;
> > > +       unsigned long long boot_addr;
> > > +
> > > +       if (argc != 3)
> > > +               return CMD_RET_USAGE;
> > > +
> > > +       cpu_id = dectoul(argv[1], NULL);
> >
> > I just noticed this...normally we use hex for parameters.
>
> Yes, but the <core ID> is from the CPU device sequence number listed by the 'cpu list', which is printed in dec. So I think use dec parameter here is reasonable.

We will have devices with hundreds of CPUs so decimal is probably
going to bite us. U-Boot uses hex for most values.

Anyway, let's worry about it later. What you have is fine.

Regards,
Simon
Z.Q. Hou Aug. 2, 2024, 4:11 a.m. UTC | #4 
Hi Simon,

> -----Original Message-----
> From: Simon Glass <sjg@chromium.org>
> Sent: Thursday, August 1, 2024 10:42 PM
> To: Z.Q. Hou <zhiqiang.hou@nxp.com>
> Cc: u-boot@lists.denx.de; trini@konsulko.com; Peng Fan
> <peng.fan@nxp.com>; festevam@gmail.com; marex@denx.de;
> lukma@denx.de; seanga2@gmail.com; xypron.glpk@gmx.de
> Subject: Re: [PATCHv4 10/14] cmd: cpu: add release subcommand
> 
> Hi Zhiqiang,
> 
> On Wed, 31 Jul 2024 at 20:37, Z.Q. Hou <zhiqiang.hou@nxp.com> wrote:
> >
> > Hi Simon,
> >
> > > -----Original Message-----
> > > From: Simon Glass <sjg@chromium.org>
> > > Sent: Wednesday, July 31, 2024 10:39 PM
> > > To: Z.Q. Hou <zhiqiang.hou@nxp.com>
> > > Cc: u-boot@lists.denx.de; trini@konsulko.com; Peng Fan
> > > <peng.fan@nxp.com>; festevam@gmail.com; marex@denx.de;
> > > lukma@denx.de; seanga2@gmail.com; xypron.glpk@gmx.de
> > > Subject: Re: [PATCHv4 10/14] cmd: cpu: add release subcommand
> > >
> > > Hi Zhiqiang,
> > >
> > > On Tue, 30 Jul 2024 at 01:09, Zhiqiang Hou <Zhiqiang.Hou@nxp.com>
> wrote:
> > > >
> > > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > > >
> > > > Add a new subcommand 'release' to bring up a core to run baremetal
> > > > and RTOS applications.
> > > >
> > > > For example on i.MX8M Plus EVK, release the LAST core to run a
> > > > RTOS application, passing the sequence number of the CPU core to
> > > > release, here it is 3:
> > > >     u-boot=> cpu list
> > > >       0: cpu@0      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
> > > >       1: cpu@1      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 30C
> > > >       2: cpu@2      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
> > > >       3: cpu@3      NXP i.MX8MP Rev1.1 A53 at 1200 MHz at 31C
> > > >
> > > >     u-boot=> load mmc 1:2 c0000000 /hello_world.bin
> > > >     66008 bytes read in 5 ms (12.6 MiB/s)
> > > >     u-boot=> dcache flush; icache flush
> > > >     u-boot=> cpu release 3 c0000000
> > > >     Released CPU core (mpidr: 0x3) to address 0xc0000000
> > > >
> > > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com>
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > ---
> > > > V4:
> > > >  - Updated the change logs.
> > > >
> > > >  cmd/cpu.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 42 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/cmd/cpu.c b/cmd/cpu.c index 9e323069b9..2755250756
> > > > 100644
> > > > --- a/cmd/cpu.c
> > > > +++ b/cmd/cpu.c
> > > > @@ -3,6 +3,7 @@
> > > >   * Copyright (c) 2015 Google, Inc
> > > >   * Written by Simon Glass <sjg@chromium.org>
> > > >   * Copyright (c) 2017 Álvaro Fernández Rojas <noltari@gmail.com>
> > > > + * Copyright 2024 NXP
> > > >   */
> > > >
> > > >  #include <command.h>
> > > > @@ -18,6 +19,19 @@ static const char
> > > *cpu_feature_name[CPU_FEAT_COUNT] = {
> > > >         "Device ID",
> > > >  };
> > > >
> > > > +static struct udevice *cpu_find_device(unsigned long cpu_id) {
> > > > +       struct udevice *dev;
> > > > +
> > > > +       for (uclass_first_device(UCLASS_CPU, &dev); dev;
> > > > +            uclass_next_device(&dev)) {
> > > > +               if (cpu_id == dev_seq(dev))
> > > > +                       return dev;
> > > > +       }
> > > > +
> > > > +       return NULL;
> > > > +}
> > > > +
> > > >  static int print_cpu_list(bool detail)  {
> > > >         struct udevice *dev;
> > > > @@ -82,10 +96,36 @@ static int do_cpu_detail(struct cmd_tbl
> > > > *cmdtp, int
> > > flag, int argc,
> > > >         return 0;
> > > >  }
> > > >
> > > > +static int do_cpu_release(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > +                         char *const argv[]) {
> > > > +       struct udevice *dev;
> > > > +       unsigned long cpu_id;
> > > > +       unsigned long long boot_addr;
> > > > +
> > > > +       if (argc != 3)
> > > > +               return CMD_RET_USAGE;
> > > > +
> > > > +       cpu_id = dectoul(argv[1], NULL);
> > >
> > > I just noticed this...normally we use hex for parameters.
> >
> > Yes, but the <core ID> is from the CPU device sequence number listed by the
> 'cpu list', which is printed in dec. So I think use dec parameter here is
> reasonable.
> 
> We will have devices with hundreds of CPUs so decimal is probably going to
> bite us. U-Boot uses hex for most values.
> 
> Anyway, let's worry about it later. What you have is fine.

Okay, I think the key point is to keep the CPU core sequence number in 'list' subcommand and the input <core ID> using the same format.

Thanks,
Zhiqiang
diff mbox series

Patch

diff --git a/cmd/cpu.c b/cmd/cpu.c
index 9e323069b9..2755250756 100644
--- a/cmd/cpu.c
+++ b/cmd/cpu.c
@@ -3,6 +3,7 @@ 
  * Copyright (c) 2015 Google, Inc
  * Written by Simon Glass <sjg@chromium.org>
  * Copyright (c) 2017 Álvaro Fernández Rojas <noltari@gmail.com>
+ * Copyright 2024 NXP
  */
 
 #include <command.h>
@@ -18,6 +19,19 @@  static const char *cpu_feature_name[CPU_FEAT_COUNT] = {
 	"Device ID",
 };
 
+static struct udevice *cpu_find_device(unsigned long cpu_id)
+{
+	struct udevice *dev;
+
+	for (uclass_first_device(UCLASS_CPU, &dev); dev;
+	     uclass_next_device(&dev)) {
+		if (cpu_id == dev_seq(dev))
+			return dev;
+	}
+
+	return NULL;
+}
+
 static int print_cpu_list(bool detail)
 {
 	struct udevice *dev;
@@ -82,10 +96,36 @@  static int do_cpu_detail(struct cmd_tbl *cmdtp, int flag, int argc,
 	return 0;
 }
 
+static int do_cpu_release(struct cmd_tbl *cmdtp, int flag, int argc,
+			  char *const argv[])
+{
+	struct udevice *dev;
+	unsigned long cpu_id;
+	unsigned long long boot_addr;
+
+	if (argc != 3)
+		return CMD_RET_USAGE;
+
+	cpu_id = dectoul(argv[1], NULL);
+	dev = cpu_find_device(cpu_id);
+	if (!dev)
+		return CMD_RET_FAILURE;
+
+	boot_addr = simple_strtoull(argv[2], NULL, 16);
+
+	if (cpu_release_core(dev, boot_addr))
+		return CMD_RET_FAILURE;
+
+	return 0;
+}
+
 U_BOOT_LONGHELP(cpu,
 	"list	- list available CPUs\n"
-	"cpu detail	- show CPU detail");
+	"cpu detail	- show CPU detail\n"
+	"cpu release <core ID> <addr>	- Release CPU <core ID> at <addr>\n"
+	"            <core ID>: the sequence number in list subcommand outputs");
 
 U_BOOT_CMD_WITH_SUBCMDS(cpu, "display information about CPUs", cpu_help_text,
 	U_BOOT_SUBCMD_MKENT(list, 1, 1, do_cpu_list),
-	U_BOOT_SUBCMD_MKENT(detail, 1, 0, do_cpu_detail));
+	U_BOOT_SUBCMD_MKENT(detail, 1, 0, do_cpu_detail),
+	U_BOOT_SUBCMD_MKENT(release, 3, 0, do_cpu_release));