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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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