diff mbox series

[08/10] cmd: add scmi command for SCMI firmware

Message ID 20230628004841.21774-9-takahiro.akashi@linaro.org
State New
Headers show
Series firmware: scmi: add SCMI base protocol support | expand

Commit Message

AKASHI Takahiro June 28, 2023, 12:48 a.m. UTC
This command, "scmi", provides a command line interface to various SCMI
protocols. It supports at least initially SCMI base protocol with the sub
command, "base", and is intended mainly for debug purpose.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/Kconfig  |   8 ++
 cmd/Makefile |   1 +
 cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 382 insertions(+)
 create mode 100644 cmd/scmi.c

Comments

Simon Glass June 29, 2023, 7:10 p.m. UTC | #1
Hi AKASHI,

On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> This command, "scmi", provides a command line interface to various SCMI
> protocols. It supports at least initially SCMI base protocol with the sub
> command, "base", and is intended mainly for debug purpose.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/Kconfig  |   8 ++
>  cmd/Makefile |   1 +
>  cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 382 insertions(+)
>  create mode 100644 cmd/scmi.c
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 02e54f1e50fe..065470a76295 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -2504,6 +2504,14 @@ config CMD_CROS_EC
>           a number of sub-commands for performing EC tasks such as
>           updating its flash, accessing a small saved context area
>           and talking to the I2C bus behind the EC (if there is one).
> +
> +config CMD_SCMI
> +       bool "Enable scmi command"
> +       depends on SCMI_FIRMWARE
> +       default n
> +       help
> +         This command provides user interfaces to several SCMI protocols,
> +         including Base protocol.

Please mention what is SCMI

>  endmenu
>
>  menu "Filesystem commands"
> diff --git a/cmd/Makefile b/cmd/Makefile
> index 6c37521b4e2b..826e0b74b587 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -156,6 +156,7 @@ obj-$(CONFIG_CMD_SATA) += sata.o
>  obj-$(CONFIG_CMD_NVME) += nvme.o
>  obj-$(CONFIG_SANDBOX) += sb.o
>  obj-$(CONFIG_CMD_SF) += sf.o
> +obj-$(CONFIG_CMD_SCMI) += scmi.o
>  obj-$(CONFIG_CMD_SCSI) += scsi.o disk.o
>  obj-$(CONFIG_CMD_SHA1SUM) += sha1sum.o
>  obj-$(CONFIG_CMD_SEAMA) += seama.o
> diff --git a/cmd/scmi.c b/cmd/scmi.c
> new file mode 100644
> index 000000000000..c97f77e97d95
> --- /dev/null
> +++ b/cmd/scmi.c
> @@ -0,0 +1,373 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  SCMI utility command
> + *
> + *  Copyright (c) 2023 Linaro Limited
> + *             Author: AKASHI Takahiro
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <dm/device.h>
> +#include <dm/uclass.h> /* uclass_get_device */

Goes before linux/bitfield.h

> +#include <exports.h>
> +#include <scmi_agent.h>
> +#include <scmi_agent-uclass.h>
> +#include <asm/types.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +
> +static struct udevice *agent;
> +static struct udevice *base_proto;
> +static const struct scmi_base_ops *ops;
> +
> +struct {
> +       enum scmi_std_protocol id;
> +       const char *name;
> +} protocol_name[] = {
> +       {SCMI_PROTOCOL_ID_BASE, "Base"},
> +       {SCMI_PROTOCOL_ID_POWER_DOMAIN, "Power domain management"},
> +       {SCMI_PROTOCOL_ID_SYSTEM, "System power management"},
> +       {SCMI_PROTOCOL_ID_PERF, "Performance domain management"},
> +       {SCMI_PROTOCOL_ID_CLOCK, "Clock management"},
> +       {SCMI_PROTOCOL_ID_SENSOR, "Sensor management"},
> +       {SCMI_PROTOCOL_ID_RESET_DOMAIN, "Reset domain management"},
> +       {SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN, "Voltage domain management"},
> +};
> +
> +/**
> + * scmi_convert_id_to_string() - get the name of SCMI protocol
> + *
> + * @id:                Protocol ID
> + *
> + * Get the printable name of the protocol, @id
> + *
> + * Return:     Name string on success, NULL on failure
> + */
> +static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)

scmi_lookup_id?

> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(protocol_name); i++)
> +               if (id == protocol_name[i].id)
> +                       return protocol_name[i].name;
> +
> +       return NULL;
> +}
> +
> +/**
> + * do_scmi_base_info() - get the information of SCMI services
> + *
> + * @cmdtp:     Command table
> + * @flag:      Command flag
> + * @argc:      Number of arguments
> + * @argv:      Argument array
> + *
> + * Get the information of SCMI services using various interfaces
> + * provided by the Base protocol.
> + *
> + * Return:     CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
> + */
> +static int do_scmi_base_info(struct cmd_tbl *cmdtp, int flag, int argc,
> +                            char * const argv[])
> +{
> +       u32 agent_id, num_protocols;
> +       u8 agent_name[SCMI_BASE_NAME_LENGTH_MAX], *protocols;
> +       int i, ret;
> +
> +       if (argc != 1)
> +               return CMD_RET_USAGE;
> +
> +       printf("SCMI device: %s\n", agent->name);
> +       printf("  protocol version: 0x%x\n", scmi_version(agent));
> +       printf("  # of agents: %d\n", scmi_num_agents(agent));
> +       for (i = 0; i < scmi_num_agents(agent); i++) {
> +               ret = (*ops->base_discover_agent)(base_proto, i, &agent_id,
> +                                                 agent_name);
> +               if (ret) {
> +                       if (ret != -EOPNOTSUPP)
> +                               printf("base_discover_agent() failed for id: %d (%d)\n",
> +                                      i, ret);
> +                       break;
> +               }
> +               printf("    %c%2d: %s\n", i == scmi_agent_id(agent) ? '>' : ' ',
> +                      i, agent_name);
> +       }
> +       printf("  # of protocols: %d\n", scmi_num_protocols(agent));
> +       num_protocols = scmi_num_protocols(agent);
> +       protocols = scmi_protocols(agent);
> +       if (protocols)
> +               for (i = 0; i < num_protocols; i++)
> +                       printf("      %s\n",
> +                              scmi_convert_id_to_string(protocols[i]));
> +       printf("  vendor: %s\n", scmi_vendor(agent));
> +       printf("  sub vendor: %s\n", scmi_sub_vendor(agent));
> +       printf("  impl version: 0x%x\n", scmi_impl_version(agent));
> +
> +       return CMD_RET_SUCCESS;
> +}
> +
> +/**
> + * do_scmi_base_set_dev() - set access permission to device
> + *
> + * @cmdtp:     Command table
> + * @flag:      Command flag
> + * @argc:      Number of arguments
> + * @argv:      Argument array
> + *
> + * Set access permission to device with SCMI_BASE_SET_DEVICE_PERMISSIONS
> + *
> + * Return:     CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
> + */
> +static int do_scmi_base_set_dev(struct cmd_tbl *cmdtp, int flag, int argc,
> +                               char * const argv[])
> +{
> +       u32 agent_id, device_id, flags, attributes;
> +       char *end;
> +       int ret;
> +
> +       if (argc != 4)
> +               return CMD_RET_USAGE;
> +
> +       agent_id = simple_strtoul(argv[1], &end, 16);
> +       if (*end != '\0')
> +               return CMD_RET_USAGE;
> +
> +       device_id = simple_strtoul(argv[2], &end, 16);
> +       if (*end != '\0')
> +               return CMD_RET_USAGE;
> +
> +       flags = simple_strtoul(argv[3], &end, 16);
> +       if (*end != '\0')
> +               return CMD_RET_USAGE;
> +
> +       ret = (*ops->protocol_message_attrs)(base_proto,
> +                                            SCMI_BASE_SET_DEVICE_PERMISSIONS,
> +                                            &attributes);
> +       if (ret) {
> +               printf("This operation is not supported\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       ret = (*ops->base_set_device_permissions)(base_proto, agent_id,
> +                                                 device_id, flags);

Please use helpers rather than using ops directly.

Regards,
Simon
AKASHI Takahiro July 3, 2023, 12:55 a.m. UTC | #2
On Thu, Jun 29, 2023 at 08:10:00PM +0100, Simon Glass wrote:
> Hi AKASHI,
> 
> On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > This command, "scmi", provides a command line interface to various SCMI
> > protocols. It supports at least initially SCMI base protocol with the sub
> > command, "base", and is intended mainly for debug purpose.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/Kconfig  |   8 ++
> >  cmd/Makefile |   1 +
> >  cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 382 insertions(+)
> >  create mode 100644 cmd/scmi.c
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 02e54f1e50fe..065470a76295 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -2504,6 +2504,14 @@ config CMD_CROS_EC
> >           a number of sub-commands for performing EC tasks such as
> >           updating its flash, accessing a small saved context area
> >           and talking to the I2C bus behind the EC (if there is one).
> > +
> > +config CMD_SCMI
> > +       bool "Enable scmi command"
> > +       depends on SCMI_FIRMWARE
> > +       default n
> > +       help
> > +         This command provides user interfaces to several SCMI protocols,
> > +         including Base protocol.
> 
> Please mention what is SCMI

I will give a full spelling.

> >  endmenu
> >
> >  menu "Filesystem commands"
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index 6c37521b4e2b..826e0b74b587 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -156,6 +156,7 @@ obj-$(CONFIG_CMD_SATA) += sata.o
> >  obj-$(CONFIG_CMD_NVME) += nvme.o
> >  obj-$(CONFIG_SANDBOX) += sb.o
> >  obj-$(CONFIG_CMD_SF) += sf.o
> > +obj-$(CONFIG_CMD_SCMI) += scmi.o
> >  obj-$(CONFIG_CMD_SCSI) += scsi.o disk.o
> >  obj-$(CONFIG_CMD_SHA1SUM) += sha1sum.o
> >  obj-$(CONFIG_CMD_SEAMA) += seama.o
> > diff --git a/cmd/scmi.c b/cmd/scmi.c
> > new file mode 100644
> > index 000000000000..c97f77e97d95
> > --- /dev/null
> > +++ b/cmd/scmi.c
> > @@ -0,0 +1,373 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  SCMI utility command
> > + *
> > + *  Copyright (c) 2023 Linaro Limited
> > + *             Author: AKASHI Takahiro
> > + */
> > +
> > +#include <common.h>
> > +#include <command.h>
> > +#include <dm/device.h>
> > +#include <dm/uclass.h> /* uclass_get_device */
> 
> Goes before linux/bitfield.h

Can you tell me why?

> > +#include <exports.h>
> > +#include <scmi_agent.h>
> > +#include <scmi_agent-uclass.h>
> > +#include <asm/types.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +
> > +static struct udevice *agent;
> > +static struct udevice *base_proto;
> > +static const struct scmi_base_ops *ops;
> > +
> > +struct {
> > +       enum scmi_std_protocol id;
> > +       const char *name;
> > +} protocol_name[] = {
> > +       {SCMI_PROTOCOL_ID_BASE, "Base"},
> > +       {SCMI_PROTOCOL_ID_POWER_DOMAIN, "Power domain management"},
> > +       {SCMI_PROTOCOL_ID_SYSTEM, "System power management"},
> > +       {SCMI_PROTOCOL_ID_PERF, "Performance domain management"},
> > +       {SCMI_PROTOCOL_ID_CLOCK, "Clock management"},
> > +       {SCMI_PROTOCOL_ID_SENSOR, "Sensor management"},
> > +       {SCMI_PROTOCOL_ID_RESET_DOMAIN, "Reset domain management"},
> > +       {SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN, "Voltage domain management"},
> > +};
> > +
> > +/**
> > + * scmi_convert_id_to_string() - get the name of SCMI protocol
> > + *
> > + * @id:                Protocol ID
> > + *
> > + * Get the printable name of the protocol, @id
> > + *
> > + * Return:     Name string on success, NULL on failure
> > + */
> > +static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)
> 
> scmi_lookup_id?

Not sure your intention.
The name above gives us exactly what this function does for pretty printing
instead of a number.
I think that 'lookup' implies we try to determine if the id exists or not.

> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(protocol_name); i++)
> > +               if (id == protocol_name[i].id)
> > +                       return protocol_name[i].name;
> > +
> > +       return NULL;
> > +}
> > +
> > +/**
> > + * do_scmi_base_info() - get the information of SCMI services
> > + *
> > + * @cmdtp:     Command table
> > + * @flag:      Command flag
> > + * @argc:      Number of arguments
> > + * @argv:      Argument array
> > + *
> > + * Get the information of SCMI services using various interfaces
> > + * provided by the Base protocol.
> > + *
> > + * Return:     CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
> > + */
> > +static int do_scmi_base_info(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                            char * const argv[])
> > +{
> > +       u32 agent_id, num_protocols;
> > +       u8 agent_name[SCMI_BASE_NAME_LENGTH_MAX], *protocols;
> > +       int i, ret;
> > +
> > +       if (argc != 1)
> > +               return CMD_RET_USAGE;
> > +
> > +       printf("SCMI device: %s\n", agent->name);
> > +       printf("  protocol version: 0x%x\n", scmi_version(agent));
> > +       printf("  # of agents: %d\n", scmi_num_agents(agent));
> > +       for (i = 0; i < scmi_num_agents(agent); i++) {
> > +               ret = (*ops->base_discover_agent)(base_proto, i, &agent_id,
> > +                                                 agent_name);
> > +               if (ret) {
> > +                       if (ret != -EOPNOTSUPP)
> > +                               printf("base_discover_agent() failed for id: %d (%d)\n",
> > +                                      i, ret);
> > +                       break;
> > +               }
> > +               printf("    %c%2d: %s\n", i == scmi_agent_id(agent) ? '>' : ' ',
> > +                      i, agent_name);
> > +       }
> > +       printf("  # of protocols: %d\n", scmi_num_protocols(agent));
> > +       num_protocols = scmi_num_protocols(agent);
> > +       protocols = scmi_protocols(agent);
> > +       if (protocols)
> > +               for (i = 0; i < num_protocols; i++)
> > +                       printf("      %s\n",
> > +                              scmi_convert_id_to_string(protocols[i]));
> > +       printf("  vendor: %s\n", scmi_vendor(agent));
> > +       printf("  sub vendor: %s\n", scmi_sub_vendor(agent));
> > +       printf("  impl version: 0x%x\n", scmi_impl_version(agent));
> > +
> > +       return CMD_RET_SUCCESS;
> > +}
> > +
> > +/**
> > + * do_scmi_base_set_dev() - set access permission to device
> > + *
> > + * @cmdtp:     Command table
> > + * @flag:      Command flag
> > + * @argc:      Number of arguments
> > + * @argv:      Argument array
> > + *
> > + * Set access permission to device with SCMI_BASE_SET_DEVICE_PERMISSIONS
> > + *
> > + * Return:     CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
> > + */
> > +static int do_scmi_base_set_dev(struct cmd_tbl *cmdtp, int flag, int argc,
> > +                               char * const argv[])
> > +{
> > +       u32 agent_id, device_id, flags, attributes;
> > +       char *end;
> > +       int ret;
> > +
> > +       if (argc != 4)
> > +               return CMD_RET_USAGE;
> > +
> > +       agent_id = simple_strtoul(argv[1], &end, 16);
> > +       if (*end != '\0')
> > +               return CMD_RET_USAGE;
> > +
> > +       device_id = simple_strtoul(argv[2], &end, 16);
> > +       if (*end != '\0')
> > +               return CMD_RET_USAGE;
> > +
> > +       flags = simple_strtoul(argv[3], &end, 16);
> > +       if (*end != '\0')
> > +               return CMD_RET_USAGE;
> > +
> > +       ret = (*ops->protocol_message_attrs)(base_proto,
> > +                                            SCMI_BASE_SET_DEVICE_PERMISSIONS,
> > +                                            &attributes);
> > +       if (ret) {
> > +               printf("This operation is not supported\n");
> > +               return CMD_RET_FAILURE;
> > +       }
> > +
> > +       ret = (*ops->base_set_device_permissions)(base_proto, agent_id,
> > +                                                 device_id, flags);
> 
> Please use helpers rather than using ops directly.

Okay.

-Takahiro Akashi

> Regards,
> Simon
Simon Glass July 3, 2023, 1:30 p.m. UTC | #3
Hi,

On Mon, 3 Jul 2023 at 01:55, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> On Thu, Jun 29, 2023 at 08:10:00PM +0100, Simon Glass wrote:
> > Hi AKASHI,
> >
> > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > This command, "scmi", provides a command line interface to various SCMI
> > > protocols. It supports at least initially SCMI base protocol with the sub
> > > command, "base", and is intended mainly for debug purpose.
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > ---
> > >  cmd/Kconfig  |   8 ++
> > >  cmd/Makefile |   1 +
> > >  cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 382 insertions(+)
> > >  create mode 100644 cmd/scmi.c
> > >
> > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > index 02e54f1e50fe..065470a76295 100644
> > > --- a/cmd/Kconfig
> > > +++ b/cmd/Kconfig
> > > @@ -2504,6 +2504,14 @@ config CMD_CROS_EC
> > >           a number of sub-commands for performing EC tasks such as
> > >           updating its flash, accessing a small saved context area
> > >           and talking to the I2C bus behind the EC (if there is one).
> > > +
> > > +config CMD_SCMI
> > > +       bool "Enable scmi command"
> > > +       depends on SCMI_FIRMWARE
> > > +       default n
> > > +       help
> > > +         This command provides user interfaces to several SCMI protocols,
> > > +         including Base protocol.
> >
> > Please mention what is SCMI
>
> I will give a full spelling.
>
> > >  endmenu
> > >
> > >  menu "Filesystem commands"
> > > diff --git a/cmd/Makefile b/cmd/Makefile
> > > index 6c37521b4e2b..826e0b74b587 100644
> > > --- a/cmd/Makefile
> > > +++ b/cmd/Makefile
> > > @@ -156,6 +156,7 @@ obj-$(CONFIG_CMD_SATA) += sata.o
> > >  obj-$(CONFIG_CMD_NVME) += nvme.o
> > >  obj-$(CONFIG_SANDBOX) += sb.o
> > >  obj-$(CONFIG_CMD_SF) += sf.o
> > > +obj-$(CONFIG_CMD_SCMI) += scmi.o
> > >  obj-$(CONFIG_CMD_SCSI) += scsi.o disk.o
> > >  obj-$(CONFIG_CMD_SHA1SUM) += sha1sum.o
> > >  obj-$(CONFIG_CMD_SEAMA) += seama.o
> > > diff --git a/cmd/scmi.c b/cmd/scmi.c
> > > new file mode 100644
> > > index 000000000000..c97f77e97d95
> > > --- /dev/null
> > > +++ b/cmd/scmi.c
> > > @@ -0,0 +1,373 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + *  SCMI utility command
> > > + *
> > > + *  Copyright (c) 2023 Linaro Limited
> > > + *             Author: AKASHI Takahiro
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <command.h>
> > > +#include <dm/device.h>
> > > +#include <dm/uclass.h> /* uclass_get_device */
> >
> > Goes before linux/bitfield.h
>
> Can you tell me why?

It is just a convention:
https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files

>
> > > +#include <exports.h>
> > > +#include <scmi_agent.h>
> > > +#include <scmi_agent-uclass.h>
> > > +#include <asm/types.h>
> > > +#include <linux/bitfield.h>
> > > +#include <linux/bitops.h>
> > > +
> > > +static struct udevice *agent;
> > > +static struct udevice *base_proto;
> > > +static const struct scmi_base_ops *ops;
> > > +
> > > +struct {
> > > +       enum scmi_std_protocol id;
> > > +       const char *name;
> > > +} protocol_name[] = {
> > > +       {SCMI_PROTOCOL_ID_BASE, "Base"},
> > > +       {SCMI_PROTOCOL_ID_POWER_DOMAIN, "Power domain management"},
> > > +       {SCMI_PROTOCOL_ID_SYSTEM, "System power management"},
> > > +       {SCMI_PROTOCOL_ID_PERF, "Performance domain management"},
> > > +       {SCMI_PROTOCOL_ID_CLOCK, "Clock management"},
> > > +       {SCMI_PROTOCOL_ID_SENSOR, "Sensor management"},
> > > +       {SCMI_PROTOCOL_ID_RESET_DOMAIN, "Reset domain management"},
> > > +       {SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN, "Voltage domain management"},
> > > +};
> > > +
> > > +/**
> > > + * scmi_convert_id_to_string() - get the name of SCMI protocol
> > > + *
> > > + * @id:                Protocol ID
> > > + *
> > > + * Get the printable name of the protocol, @id
> > > + *
> > > + * Return:     Name string on success, NULL on failure
> > > + */
> > > +static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)
> >
> > scmi_lookup_id?
>
> Not sure your intention.
> The name above gives us exactly what this function does for pretty printing
> instead of a number.
> I think that 'lookup' implies we try to determine if the id exists or not.

I am just trying to avoid the code turning into Java :-)

How about scmi_get_name() ?

Regards,
Simon
AKASHI Takahiro July 4, 2023, 1:26 a.m. UTC | #4
On Mon, Jul 03, 2023 at 02:30:54PM +0100, Simon Glass wrote:
> Hi,
> 
> On Mon, 3 Jul 2023 at 01:55, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > On Thu, Jun 29, 2023 at 08:10:00PM +0100, Simon Glass wrote:
> > > Hi AKASHI,
> > >
> > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > This command, "scmi", provides a command line interface to various SCMI
> > > > protocols. It supports at least initially SCMI base protocol with the sub
> > > > command, "base", and is intended mainly for debug purpose.
> > > >
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > ---
> > > >  cmd/Kconfig  |   8 ++
> > > >  cmd/Makefile |   1 +
> > > >  cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 382 insertions(+)
> > > >  create mode 100644 cmd/scmi.c
> > > >
> > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > index 02e54f1e50fe..065470a76295 100644
> > > > --- a/cmd/Kconfig
> > > > +++ b/cmd/Kconfig
> > > > @@ -2504,6 +2504,14 @@ config CMD_CROS_EC
> > > >           a number of sub-commands for performing EC tasks such as
> > > >           updating its flash, accessing a small saved context area
> > > >           and talking to the I2C bus behind the EC (if there is one).
> > > > +
> > > > +config CMD_SCMI
> > > > +       bool "Enable scmi command"
> > > > +       depends on SCMI_FIRMWARE
> > > > +       default n
> > > > +       help
> > > > +         This command provides user interfaces to several SCMI protocols,
> > > > +         including Base protocol.
> > >
> > > Please mention what is SCMI
> >
> > I will give a full spelling.
> >
> > > >  endmenu
> > > >
> > > >  menu "Filesystem commands"
> > > > diff --git a/cmd/Makefile b/cmd/Makefile
> > > > index 6c37521b4e2b..826e0b74b587 100644
> > > > --- a/cmd/Makefile
> > > > +++ b/cmd/Makefile
> > > > @@ -156,6 +156,7 @@ obj-$(CONFIG_CMD_SATA) += sata.o
> > > >  obj-$(CONFIG_CMD_NVME) += nvme.o
> > > >  obj-$(CONFIG_SANDBOX) += sb.o
> > > >  obj-$(CONFIG_CMD_SF) += sf.o
> > > > +obj-$(CONFIG_CMD_SCMI) += scmi.o
> > > >  obj-$(CONFIG_CMD_SCSI) += scsi.o disk.o
> > > >  obj-$(CONFIG_CMD_SHA1SUM) += sha1sum.o
> > > >  obj-$(CONFIG_CMD_SEAMA) += seama.o
> > > > diff --git a/cmd/scmi.c b/cmd/scmi.c
> > > > new file mode 100644
> > > > index 000000000000..c97f77e97d95
> > > > --- /dev/null
> > > > +++ b/cmd/scmi.c
> > > > @@ -0,0 +1,373 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + *  SCMI utility command
> > > > + *
> > > > + *  Copyright (c) 2023 Linaro Limited
> > > > + *             Author: AKASHI Takahiro
> > > > + */
> > > > +
> > > > +#include <common.h>
> > > > +#include <command.h>
> > > > +#include <dm/device.h>
> > > > +#include <dm/uclass.h> /* uclass_get_device */
> > >
> > > Goes before linux/bitfield.h
> >
> > Can you tell me why?
> 
> It is just a convention:
> https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files

Okay.

> >
> > > > +#include <exports.h>
> > > > +#include <scmi_agent.h>
> > > > +#include <scmi_agent-uclass.h>
> > > > +#include <asm/types.h>
> > > > +#include <linux/bitfield.h>
> > > > +#include <linux/bitops.h>
> > > > +
> > > > +static struct udevice *agent;
> > > > +static struct udevice *base_proto;
> > > > +static const struct scmi_base_ops *ops;
> > > > +
> > > > +struct {
> > > > +       enum scmi_std_protocol id;
> > > > +       const char *name;
> > > > +} protocol_name[] = {
> > > > +       {SCMI_PROTOCOL_ID_BASE, "Base"},
> > > > +       {SCMI_PROTOCOL_ID_POWER_DOMAIN, "Power domain management"},
> > > > +       {SCMI_PROTOCOL_ID_SYSTEM, "System power management"},
> > > > +       {SCMI_PROTOCOL_ID_PERF, "Performance domain management"},
> > > > +       {SCMI_PROTOCOL_ID_CLOCK, "Clock management"},
> > > > +       {SCMI_PROTOCOL_ID_SENSOR, "Sensor management"},
> > > > +       {SCMI_PROTOCOL_ID_RESET_DOMAIN, "Reset domain management"},
> > > > +       {SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN, "Voltage domain management"},
> > > > +};
> > > > +
> > > > +/**
> > > > + * scmi_convert_id_to_string() - get the name of SCMI protocol
> > > > + *
> > > > + * @id:                Protocol ID
> > > > + *
> > > > + * Get the printable name of the protocol, @id
> > > > + *
> > > > + * Return:     Name string on success, NULL on failure
> > > > + */
> > > > +static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)
> > >
> > > scmi_lookup_id?
> >
> > Not sure your intention.
> > The name above gives us exactly what this function does for pretty printing
> > instead of a number.
> > I think that 'lookup' implies we try to determine if the id exists or not.
> 
> I am just trying to avoid the code turning into Java :-)

Java?

> How about scmi_get_name() ?

Well, more specifically scmi_get_protocol_name()?

-Takahiro Akashi

> Regards,
> Simon
Simon Glass July 7, 2023, 5:35 p.m. UTC | #5
Hi,

On Tue, 4 Jul 2023 at 02:26, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> On Mon, Jul 03, 2023 at 02:30:54PM +0100, Simon Glass wrote:
> > Hi,
> >
> > On Mon, 3 Jul 2023 at 01:55, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Thu, Jun 29, 2023 at 08:10:00PM +0100, Simon Glass wrote:
> > > > Hi AKASHI,
> > > >
> > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > > <takahiro.akashi@linaro.org> wrote:
> > > > >
> > > > > This command, "scmi", provides a command line interface to various SCMI
> > > > > protocols. It supports at least initially SCMI base protocol with the sub
> > > > > command, "base", and is intended mainly for debug purpose.
> > > > >
> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > ---
> > > > >  cmd/Kconfig  |   8 ++
> > > > >  cmd/Makefile |   1 +
> > > > >  cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 382 insertions(+)
> > > > >  create mode 100644 cmd/scmi.c
> > > > >
> > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > > index 02e54f1e50fe..065470a76295 100644
> > > > > --- a/cmd/Kconfig
> > > > > +++ b/cmd/Kconfig
> > > > > @@ -2504,6 +2504,14 @@ config CMD_CROS_EC
> > > > >           a number of sub-commands for performing EC tasks such as
> > > > >           updating its flash, accessing a small saved context area
> > > > >           and talking to the I2C bus behind the EC (if there is one).
> > > > > +
> > > > > +config CMD_SCMI
> > > > > +       bool "Enable scmi command"
> > > > > +       depends on SCMI_FIRMWARE
> > > > > +       default n
> > > > > +       help
> > > > > +         This command provides user interfaces to several SCMI protocols,
> > > > > +         including Base protocol.
> > > >
> > > > Please mention what is SCMI
> > >
> > > I will give a full spelling.
> > >
> > > > >  endmenu
> > > > >
> > > > >  menu "Filesystem commands"
> > > > > diff --git a/cmd/Makefile b/cmd/Makefile
> > > > > index 6c37521b4e2b..826e0b74b587 100644
> > > > > --- a/cmd/Makefile
> > > > > +++ b/cmd/Makefile
> > > > > @@ -156,6 +156,7 @@ obj-$(CONFIG_CMD_SATA) += sata.o
> > > > >  obj-$(CONFIG_CMD_NVME) += nvme.o
> > > > >  obj-$(CONFIG_SANDBOX) += sb.o
> > > > >  obj-$(CONFIG_CMD_SF) += sf.o
> > > > > +obj-$(CONFIG_CMD_SCMI) += scmi.o
> > > > >  obj-$(CONFIG_CMD_SCSI) += scsi.o disk.o
> > > > >  obj-$(CONFIG_CMD_SHA1SUM) += sha1sum.o
> > > > >  obj-$(CONFIG_CMD_SEAMA) += seama.o
> > > > > diff --git a/cmd/scmi.c b/cmd/scmi.c
> > > > > new file mode 100644
> > > > > index 000000000000..c97f77e97d95
> > > > > --- /dev/null
> > > > > +++ b/cmd/scmi.c
> > > > > @@ -0,0 +1,373 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > +/*
> > > > > + *  SCMI utility command
> > > > > + *
> > > > > + *  Copyright (c) 2023 Linaro Limited
> > > > > + *             Author: AKASHI Takahiro
> > > > > + */
> > > > > +
> > > > > +#include <common.h>
> > > > > +#include <command.h>
> > > > > +#include <dm/device.h>
> > > > > +#include <dm/uclass.h> /* uclass_get_device */
> > > >
> > > > Goes before linux/bitfield.h
> > >
> > > Can you tell me why?
> >
> > It is just a convention:
> > https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files
>
> Okay.
>
> > >
> > > > > +#include <exports.h>
> > > > > +#include <scmi_agent.h>
> > > > > +#include <scmi_agent-uclass.h>
> > > > > +#include <asm/types.h>
> > > > > +#include <linux/bitfield.h>
> > > > > +#include <linux/bitops.h>
> > > > > +
> > > > > +static struct udevice *agent;
> > > > > +static struct udevice *base_proto;
> > > > > +static const struct scmi_base_ops *ops;
> > > > > +
> > > > > +struct {
> > > > > +       enum scmi_std_protocol id;
> > > > > +       const char *name;
> > > > > +} protocol_name[] = {
> > > > > +       {SCMI_PROTOCOL_ID_BASE, "Base"},
> > > > > +       {SCMI_PROTOCOL_ID_POWER_DOMAIN, "Power domain management"},
> > > > > +       {SCMI_PROTOCOL_ID_SYSTEM, "System power management"},
> > > > > +       {SCMI_PROTOCOL_ID_PERF, "Performance domain management"},
> > > > > +       {SCMI_PROTOCOL_ID_CLOCK, "Clock management"},
> > > > > +       {SCMI_PROTOCOL_ID_SENSOR, "Sensor management"},
> > > > > +       {SCMI_PROTOCOL_ID_RESET_DOMAIN, "Reset domain management"},
> > > > > +       {SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN, "Voltage domain management"},
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * scmi_convert_id_to_string() - get the name of SCMI protocol
> > > > > + *
> > > > > + * @id:                Protocol ID
> > > > > + *
> > > > > + * Get the printable name of the protocol, @id
> > > > > + *
> > > > > + * Return:     Name string on success, NULL on failure
> > > > > + */
> > > > > +static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)
> > > >
> > > > scmi_lookup_id?
> > >
> > > Not sure your intention.
> > > The name above gives us exactly what this function does for pretty printing
> > > instead of a number.
> > > I think that 'lookup' implies we try to determine if the id exists or not.
> >
> > I am just trying to avoid the code turning into Java :-)
>
> Java?

Long names like stories

>
> > How about scmi_get_name() ?
>
> Well, more specifically scmi_get_protocol_name()?

Perhaps get_prot_name() since it is static? Anyway, please make it short-ish.

We really need some limits on identifier names.

Regards,
Simon
AKASHI Takahiro July 10, 2023, 1:46 a.m. UTC | #6
On Fri, Jul 07, 2023 at 11:35:52AM -0600, Simon Glass wrote:
> Hi,
> 
> On Tue, 4 Jul 2023 at 02:26, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > On Mon, Jul 03, 2023 at 02:30:54PM +0100, Simon Glass wrote:
> > > Hi,
> > >
> > > On Mon, 3 Jul 2023 at 01:55, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Thu, Jun 29, 2023 at 08:10:00PM +0100, Simon Glass wrote:
> > > > > Hi AKASHI,
> > > > >
> > > > > On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
> > > > > <takahiro.akashi@linaro.org> wrote:
> > > > > >
> > > > > > This command, "scmi", provides a command line interface to various SCMI
> > > > > > protocols. It supports at least initially SCMI base protocol with the sub
> > > > > > command, "base", and is intended mainly for debug purpose.
> > > > > >
> > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > > ---
> > > > > >  cmd/Kconfig  |   8 ++
> > > > > >  cmd/Makefile |   1 +
> > > > > >  cmd/scmi.c   | 373 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 382 insertions(+)
> > > > > >  create mode 100644 cmd/scmi.c
> > > > > >
> > > > > > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > > > > > index 02e54f1e50fe..065470a76295 100644
> > > > > > --- a/cmd/Kconfig
> > > > > > +++ b/cmd/Kconfig
> > > > > > @@ -2504,6 +2504,14 @@ config CMD_CROS_EC
> > > > > >           a number of sub-commands for performing EC tasks such as
> > > > > >           updating its flash, accessing a small saved context area
> > > > > >           and talking to the I2C bus behind the EC (if there is one).
> > > > > > +
> > > > > > +config CMD_SCMI
> > > > > > +       bool "Enable scmi command"
> > > > > > +       depends on SCMI_FIRMWARE
> > > > > > +       default n
> > > > > > +       help
> > > > > > +         This command provides user interfaces to several SCMI protocols,
> > > > > > +         including Base protocol.
> > > > >
> > > > > Please mention what is SCMI
> > > >
> > > > I will give a full spelling.
> > > >
> > > > > >  endmenu
> > > > > >
> > > > > >  menu "Filesystem commands"
> > > > > > diff --git a/cmd/Makefile b/cmd/Makefile
> > > > > > index 6c37521b4e2b..826e0b74b587 100644
> > > > > > --- a/cmd/Makefile
> > > > > > +++ b/cmd/Makefile
> > > > > > @@ -156,6 +156,7 @@ obj-$(CONFIG_CMD_SATA) += sata.o
> > > > > >  obj-$(CONFIG_CMD_NVME) += nvme.o
> > > > > >  obj-$(CONFIG_SANDBOX) += sb.o
> > > > > >  obj-$(CONFIG_CMD_SF) += sf.o
> > > > > > +obj-$(CONFIG_CMD_SCMI) += scmi.o
> > > > > >  obj-$(CONFIG_CMD_SCSI) += scsi.o disk.o
> > > > > >  obj-$(CONFIG_CMD_SHA1SUM) += sha1sum.o
> > > > > >  obj-$(CONFIG_CMD_SEAMA) += seama.o
> > > > > > diff --git a/cmd/scmi.c b/cmd/scmi.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..c97f77e97d95
> > > > > > --- /dev/null
> > > > > > +++ b/cmd/scmi.c
> > > > > > @@ -0,0 +1,373 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > +/*
> > > > > > + *  SCMI utility command
> > > > > > + *
> > > > > > + *  Copyright (c) 2023 Linaro Limited
> > > > > > + *             Author: AKASHI Takahiro
> > > > > > + */
> > > > > > +
> > > > > > +#include <common.h>
> > > > > > +#include <command.h>
> > > > > > +#include <dm/device.h>
> > > > > > +#include <dm/uclass.h> /* uclass_get_device */
> > > > >
> > > > > Goes before linux/bitfield.h
> > > >
> > > > Can you tell me why?
> > >
> > > It is just a convention:
> > > https://u-boot.readthedocs.io/en/latest/develop/codingstyle.html#include-files
> >
> > Okay.
> >
> > > >
> > > > > > +#include <exports.h>
> > > > > > +#include <scmi_agent.h>
> > > > > > +#include <scmi_agent-uclass.h>
> > > > > > +#include <asm/types.h>
> > > > > > +#include <linux/bitfield.h>
> > > > > > +#include <linux/bitops.h>
> > > > > > +
> > > > > > +static struct udevice *agent;
> > > > > > +static struct udevice *base_proto;
> > > > > > +static const struct scmi_base_ops *ops;
> > > > > > +
> > > > > > +struct {
> > > > > > +       enum scmi_std_protocol id;
> > > > > > +       const char *name;
> > > > > > +} protocol_name[] = {
> > > > > > +       {SCMI_PROTOCOL_ID_BASE, "Base"},
> > > > > > +       {SCMI_PROTOCOL_ID_POWER_DOMAIN, "Power domain management"},
> > > > > > +       {SCMI_PROTOCOL_ID_SYSTEM, "System power management"},
> > > > > > +       {SCMI_PROTOCOL_ID_PERF, "Performance domain management"},
> > > > > > +       {SCMI_PROTOCOL_ID_CLOCK, "Clock management"},
> > > > > > +       {SCMI_PROTOCOL_ID_SENSOR, "Sensor management"},
> > > > > > +       {SCMI_PROTOCOL_ID_RESET_DOMAIN, "Reset domain management"},
> > > > > > +       {SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN, "Voltage domain management"},
> > > > > > +};
> > > > > > +
> > > > > > +/**
> > > > > > + * scmi_convert_id_to_string() - get the name of SCMI protocol
> > > > > > + *
> > > > > > + * @id:                Protocol ID
> > > > > > + *
> > > > > > + * Get the printable name of the protocol, @id
> > > > > > + *
> > > > > > + * Return:     Name string on success, NULL on failure
> > > > > > + */
> > > > > > +static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)
> > > > >
> > > > > scmi_lookup_id?
> > > >
> > > > Not sure your intention.
> > > > The name above gives us exactly what this function does for pretty printing
> > > > instead of a number.
> > > > I think that 'lookup' implies we try to determine if the id exists or not.
> > >
> > > I am just trying to avoid the code turning into Java :-)
> >
> > Java?
> 
> Long names like stories

Well, recent software specification likes long names anyway.
(i.e. UEFI)

> >
> > > How about scmi_get_name() ?
> >
> > Well, more specifically scmi_get_protocol_name()?
> 
> Perhaps get_prot_name() since it is static? Anyway, please make it short-ish.

Okay as far as this function is contained in this file.

-Takahiro Akashi


> We really need some limits on identifier names.
> 
> Regards,
> Simon
diff mbox series

Patch

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 02e54f1e50fe..065470a76295 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -2504,6 +2504,14 @@  config CMD_CROS_EC
 	  a number of sub-commands for performing EC tasks such as
 	  updating its flash, accessing a small saved context area
 	  and talking to the I2C bus behind the EC (if there is one).
+
+config CMD_SCMI
+	bool "Enable scmi command"
+	depends on SCMI_FIRMWARE
+	default n
+	help
+	  This command provides user interfaces to several SCMI protocols,
+	  including Base protocol.
 endmenu
 
 menu "Filesystem commands"
diff --git a/cmd/Makefile b/cmd/Makefile
index 6c37521b4e2b..826e0b74b587 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -156,6 +156,7 @@  obj-$(CONFIG_CMD_SATA) += sata.o
 obj-$(CONFIG_CMD_NVME) += nvme.o
 obj-$(CONFIG_SANDBOX) += sb.o
 obj-$(CONFIG_CMD_SF) += sf.o
+obj-$(CONFIG_CMD_SCMI) += scmi.o
 obj-$(CONFIG_CMD_SCSI) += scsi.o disk.o
 obj-$(CONFIG_CMD_SHA1SUM) += sha1sum.o
 obj-$(CONFIG_CMD_SEAMA) += seama.o
diff --git a/cmd/scmi.c b/cmd/scmi.c
new file mode 100644
index 000000000000..c97f77e97d95
--- /dev/null
+++ b/cmd/scmi.c
@@ -0,0 +1,373 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  SCMI utility command
+ *
+ *  Copyright (c) 2023 Linaro Limited
+ *		Author: AKASHI Takahiro
+ */
+
+#include <common.h>
+#include <command.h>
+#include <dm/device.h>
+#include <dm/uclass.h> /* uclass_get_device */
+#include <exports.h>
+#include <scmi_agent.h>
+#include <scmi_agent-uclass.h>
+#include <asm/types.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+
+static struct udevice *agent;
+static struct udevice *base_proto;
+static const struct scmi_base_ops *ops;
+
+struct {
+	enum scmi_std_protocol id;
+	const char *name;
+} protocol_name[] = {
+	{SCMI_PROTOCOL_ID_BASE, "Base"},
+	{SCMI_PROTOCOL_ID_POWER_DOMAIN, "Power domain management"},
+	{SCMI_PROTOCOL_ID_SYSTEM, "System power management"},
+	{SCMI_PROTOCOL_ID_PERF, "Performance domain management"},
+	{SCMI_PROTOCOL_ID_CLOCK, "Clock management"},
+	{SCMI_PROTOCOL_ID_SENSOR, "Sensor management"},
+	{SCMI_PROTOCOL_ID_RESET_DOMAIN, "Reset domain management"},
+	{SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN, "Voltage domain management"},
+};
+
+/**
+ * scmi_convert_id_to_string() - get the name of SCMI protocol
+ *
+ * @id:		Protocol ID
+ *
+ * Get the printable name of the protocol, @id
+ *
+ * Return:	Name string on success, NULL on failure
+ */
+static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(protocol_name); i++)
+		if (id == protocol_name[i].id)
+			return protocol_name[i].name;
+
+	return NULL;
+}
+
+/**
+ * do_scmi_base_info() - get the information of SCMI services
+ *
+ * @cmdtp:	Command table
+ * @flag:	Command flag
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ *
+ * Get the information of SCMI services using various interfaces
+ * provided by the Base protocol.
+ *
+ * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
+ */
+static int do_scmi_base_info(struct cmd_tbl *cmdtp, int flag, int argc,
+			     char * const argv[])
+{
+	u32 agent_id, num_protocols;
+	u8 agent_name[SCMI_BASE_NAME_LENGTH_MAX], *protocols;
+	int i, ret;
+
+	if (argc != 1)
+		return CMD_RET_USAGE;
+
+	printf("SCMI device: %s\n", agent->name);
+	printf("  protocol version: 0x%x\n", scmi_version(agent));
+	printf("  # of agents: %d\n", scmi_num_agents(agent));
+	for (i = 0; i < scmi_num_agents(agent); i++) {
+		ret = (*ops->base_discover_agent)(base_proto, i, &agent_id,
+						  agent_name);
+		if (ret) {
+			if (ret != -EOPNOTSUPP)
+				printf("base_discover_agent() failed for id: %d (%d)\n",
+				       i, ret);
+			break;
+		}
+		printf("    %c%2d: %s\n", i == scmi_agent_id(agent) ? '>' : ' ',
+		       i, agent_name);
+	}
+	printf("  # of protocols: %d\n", scmi_num_protocols(agent));
+	num_protocols = scmi_num_protocols(agent);
+	protocols = scmi_protocols(agent);
+	if (protocols)
+		for (i = 0; i < num_protocols; i++)
+			printf("      %s\n",
+			       scmi_convert_id_to_string(protocols[i]));
+	printf("  vendor: %s\n", scmi_vendor(agent));
+	printf("  sub vendor: %s\n", scmi_sub_vendor(agent));
+	printf("  impl version: 0x%x\n", scmi_impl_version(agent));
+
+	return CMD_RET_SUCCESS;
+}
+
+/**
+ * do_scmi_base_set_dev() - set access permission to device
+ *
+ * @cmdtp:	Command table
+ * @flag:	Command flag
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ *
+ * Set access permission to device with SCMI_BASE_SET_DEVICE_PERMISSIONS
+ *
+ * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
+ */
+static int do_scmi_base_set_dev(struct cmd_tbl *cmdtp, int flag, int argc,
+				char * const argv[])
+{
+	u32 agent_id, device_id, flags, attributes;
+	char *end;
+	int ret;
+
+	if (argc != 4)
+		return CMD_RET_USAGE;
+
+	agent_id = simple_strtoul(argv[1], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	device_id = simple_strtoul(argv[2], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	flags = simple_strtoul(argv[3], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	ret = (*ops->protocol_message_attrs)(base_proto,
+					     SCMI_BASE_SET_DEVICE_PERMISSIONS,
+					     &attributes);
+	if (ret) {
+		printf("This operation is not supported\n");
+		return CMD_RET_FAILURE;
+	}
+
+	ret = (*ops->base_set_device_permissions)(base_proto, agent_id,
+						  device_id, flags);
+	if (ret) {
+		printf("%s access to device:%u failed (%d)\n",
+		       flags ? "Allowing" : "Denying", device_id, ret);
+		return CMD_RET_FAILURE;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+/**
+ * do_scmi_base_set_proto() - set protocol permission to device
+ *
+ * @cmdtp:	Command table
+ * @flag:	Command flag
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ *
+ * Set protocol permission to device with SCMI_BASE_SET_PROTOCOL_PERMISSIONS
+ *
+ * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
+ */
+static int do_scmi_base_set_proto(struct cmd_tbl *cmdtp, int flag, int argc,
+				  char * const argv[])
+{
+	u32 agent_id, device_id, command_id, flags, attributes;
+	char *end;
+	int ret;
+
+	if (argc != 5)
+		return CMD_RET_USAGE;
+
+	agent_id = simple_strtoul(argv[1], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	device_id = simple_strtoul(argv[2], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	command_id = simple_strtoul(argv[3], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	flags = simple_strtoul(argv[4], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	ret = (*ops->protocol_message_attrs)(base_proto,
+					     SCMI_BASE_SET_PROTOCOL_PERMISSIONS,
+					     &attributes);
+	if (ret) {
+		printf("This operation is not supported\n");
+		return CMD_RET_FAILURE;
+	}
+
+	ret = (*ops->base_set_protocol_permissions)(base_proto, agent_id,
+						    device_id, command_id,
+						    flags);
+	if (ret) {
+		printf("%s access to protocol:0x%x on device:%u failed (%d)\n",
+		       flags ? "Allowing" : "Denying", command_id, device_id,
+		       ret);
+		return CMD_RET_FAILURE;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+/**
+ * do_scmi_base_reset() - reset platform resource settings
+ *
+ * @cmdtp:	Command table
+ * @flag:	Command flag
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ *
+ * Reset platform resource settings with BASE_RESET_AGENT_CONFIGURATION
+ *
+ * Return:	CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
+ */
+static int do_scmi_base_reset(struct cmd_tbl *cmdtp, int flag, int argc,
+			      char * const argv[])
+{
+	u32 agent_id, flags, attributes;
+	char *end;
+	int ret;
+
+	if (argc != 3)
+		return CMD_RET_USAGE;
+
+	agent_id = simple_strtoul(argv[1], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	flags = simple_strtoul(argv[2], &end, 16);
+	if (*end != '\0')
+		return CMD_RET_USAGE;
+
+	ret = (*ops->protocol_message_attrs)(base_proto,
+					     SCMI_BASE_RESET_AGENT_CONFIGURATION,
+					     &attributes);
+	if (ret) {
+		printf("Reset is not supported\n");
+		return CMD_RET_FAILURE;
+	}
+
+	ret = (*ops->base_reset_agent_configuration)(base_proto, agent_id,
+						     flags);
+	if (ret) {
+		printf("Reset failed (%d)\n", ret);
+		return CMD_RET_FAILURE;
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
+static struct cmd_tbl cmd_scmi_base_sub[] = {
+	U_BOOT_CMD_MKENT(info, CONFIG_SYS_MAXARGS, 1,
+			 do_scmi_base_info, "", ""),
+	U_BOOT_CMD_MKENT(perm_dev, CONFIG_SYS_MAXARGS, 1,
+			 do_scmi_base_set_dev, "", ""),
+	U_BOOT_CMD_MKENT(perm_proto, CONFIG_SYS_MAXARGS, 1,
+			 do_scmi_base_set_proto, "", ""),
+	U_BOOT_CMD_MKENT(reset, CONFIG_SYS_MAXARGS, 1,
+			 do_scmi_base_reset, "", ""),
+};
+
+/**
+ * do_scmi_base() - handle SCMI base protocol
+ *
+ * @cmdtp:	Command table
+ * @flag:	Command flag
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ *
+ * Provide user interfaces to SCMI base protocol.
+ *
+ * Return:	CMD_RET_SUCCESS on success,
+ *		CMD_RET_USAGE or CMD_RET_RET_FAILURE on failure
+ */
+static int do_scmi_base(struct cmd_tbl *cmdtp, int flag,
+			int argc, char *const argv[])
+{
+	struct cmd_tbl *cp;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	argc--; argv++;
+
+	if (!base_proto)
+		base_proto = scmi_get_protocol(agent, SCMI_PROTOCOL_ID_BASE);
+	if (!base_proto) {
+		printf("SCMI base protocol not found\n");
+		return CMD_RET_FAILURE;
+	}
+	ops = dev_get_driver_ops(base_proto);
+
+	cp = find_cmd_tbl(argv[0], cmd_scmi_base_sub,
+			  ARRAY_SIZE(cmd_scmi_base_sub));
+	if (!cp)
+		return CMD_RET_USAGE;
+
+	return cp->cmd(cmdtp, flag, argc, argv);
+}
+
+static struct cmd_tbl cmd_scmi_sub[] = {
+	U_BOOT_CMD_MKENT(base, CONFIG_SYS_MAXARGS, 1, do_scmi_base, "", ""),
+};
+
+/**
+ * do_scmi() - SCMI utility
+ *
+ * @cmdtp:	Command table
+ * @flag:	Command flag
+ * @argc:	Number of arguments
+ * @argv:	Argument array
+ *
+ * Provide user interfaces to SCMI protocols.
+ *
+ * Return:	CMD_RET_SUCCESS on success,
+ *		CMD_RET_USAGE or CMD_RET_RET_FAILURE on failure
+ */
+static int do_scmi(struct cmd_tbl *cmdtp, int flag,
+		   int argc, char *const argv[])
+{
+	struct cmd_tbl *cp;
+
+	if (argc < 2)
+		return CMD_RET_USAGE;
+
+	argc--; argv++;
+
+	if (!agent) {
+		if (uclass_get_device(UCLASS_SCMI_AGENT, 0, &agent)) {
+			printf("Cannot find any SCMI agent\n");
+			return CMD_RET_FAILURE;
+		}
+	}
+
+	cp = find_cmd_tbl(argv[0], cmd_scmi_sub, ARRAY_SIZE(cmd_scmi_sub));
+	if (!cp)
+		return CMD_RET_USAGE;
+
+	return cp->cmd(cmdtp, flag, argc, argv);
+}
+
+static char scmi_help_text[] =
+	" - SCMI utility\n"
+	" base info - get the info of SCMI services\n"
+	" base perm_dev <agent id> <device id> <flags>\n"
+	"   - set access permission to device\n"
+	" base perm_proto <agent id> <device id> <command id> <flags>\n"
+	"   - set protocol permission to device\n"
+	" base reset <agent id> <flags>\n"
+	"   - reset platform resource settings\n"
+	"";
+
+U_BOOT_CMD(scmi, CONFIG_SYS_MAXARGS, 0, do_scmi, "SCMI utility",
+	   scmi_help_text);