Re: [PATCH v2 05/12] firmware: scmi: install base protocol to SCMI agent

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

Hi Simon,

Thank you for your extensive review.

On Wed, Jul 26, 2023 at 06:50:23PM -0600, Simon Glass wrote:
> Hi,
> 
> On Wed, 26 Jul 2023 at 02:39, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > SCMI base protocol is mandatory, and once SCMI node is found in a device
> > tree, the protocol handle (udevice) is unconditionally installed to
> > the agent. Then basic information will be retrieved from SCMI server via
> > the protocol and saved into the agent instance's local storage.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> > v2
> > * use helper functions, removing direct uses of ops
> > ---
> >  drivers/firmware/scmi/scmi_agent-uclass.c | 116 ++++++++++++++++++++++
> >  include/scmi_agent-uclass.h               |  70 ++++++++++++-
> >  2 files changed, 184 insertions(+), 2 deletions(-)
> >
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> > diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c b/drivers/firmware/scmi/scmi_agent-uclass.c
> > index 2244fcf487e8..279c2c218913 100644
> > --- a/drivers/firmware/scmi/scmi_agent-uclass.c
> > +++ b/drivers/firmware/scmi/scmi_agent-uclass.c
> > @@ -57,6 +57,9 @@ struct udevice *scmi_get_protocol(struct udevice *dev,
> >         }
> >
> >         switch (id) {
> > +       case SCMI_PROTOCOL_ID_BASE:
> > +               proto = priv->base_dev;
> > +               break;
> >         case SCMI_PROTOCOL_ID_CLOCK:
> >                 proto = priv->clock_dev;
> >                 break;
> > @@ -101,6 +104,9 @@ static int scmi_add_protocol(struct udevice *dev,
> >         }
> >
> >         switch (proto_id) {
> > +       case SCMI_PROTOCOL_ID_BASE:
> > +               priv->base_dev = proto;
> > +               break;
> >         case SCMI_PROTOCOL_ID_CLOCK:
> >                 priv->clock_dev = proto;
> >                 break;
> > @@ -229,6 +235,83 @@ int devm_scmi_process_msg(struct udevice *dev, struct scmi_msg *msg)
> >         return scmi_process_msg(parent, priv->channel, msg);
> >  }
> >
> > +/**
> > + * scmi_fill_base_info - get base information about SCMI server
> > + * @agent:     SCMI agent device
> > + * @dev:       SCMI protocol device
> > + *
> > + * By using Base protocol commands, collect the base information
> > + * about SCMI server.
> > + *
> > + * Return: 0 on success, error code otherwise
> > + */
> > +static int scmi_fill_base_info(struct udevice *agent, struct udevice *dev)
> > +{
> > +       struct scmi_agent_priv *priv = dev_get_uclass_plat(agent);
> > +       int ret;
> > +
> > +       ret = scmi_base_protocol_version(dev, &priv->version);
> > +       if (ret) {
> > +               dev_err(dev, "protocol_version() failed (%d)\n", ret);
> > +               return ret;
> > +       }
> > +       /* check for required version */
> > +       if (priv->version < SCMI_BASE_PROTOCOL_VERSION) {
> > +               dev_err(dev, "base protocol version (%d) lower than expected\n",
> > +                       priv->version);
> > +               return -EPROTO;
> > +       }
> > +
> > +       ret = scmi_base_protocol_attrs(dev, &priv->num_agents,
> > +                                      &priv->num_protocols);
> > +       if (ret) {
> > +               dev_err(dev, "protocol_attrs() failed (%d)\n", ret);
> > +               return ret;
> > +       }
> > +       ret = scmi_base_discover_vendor(dev, priv->vendor);
> > +       if (ret) {
> > +               dev_err(dev, "base_discover_vendor() failed (%d)\n", ret);
> > +               return ret;
> > +       }
> > +       ret = scmi_base_discover_sub_vendor(dev, priv->sub_vendor);
> > +       if (ret) {
> > +               if (ret != -EOPNOTSUPP) {
> > +                       dev_err(dev, "base_discover_sub_vendor() failed (%d)\n",
> > +                               ret);
> > +                       return ret;
> > +               }
> > +               strcpy(priv->sub_vendor, "NA");
> > +       }
> > +       ret = scmi_base_discover_impl_version(dev, &priv->impl_version);
> > +       if (ret) {
> > +               dev_err(dev, "base_discover_impl_version() failed (%d)\n",
> > +                       ret);
> > +               return ret;
> > +       }
> > +
> > +       priv->agent_id = 0xffffffff; /* to avoid false claim */
> > +       ret = scmi_base_discover_agent(dev, 0xffffffff,
> > +                                      &priv->agent_id, priv->agent_name);
> > +       if (ret) {
> > +               if (ret != -EOPNOTSUPP) {
> > +                       dev_err(dev,
> > +                               "base_discover_agent() failed for myself (%d)\n",
> > +                               ret);
> > +                       return ret;
> > +               }
> > +               priv->agent_name[0] = '\0';
> > +       }
> > +
> > +       ret = scmi_base_discover_list_protocols(dev, &priv->protocols);
> > +       if (ret != priv->num_protocols) {
> > +               dev_err(dev, "base_discover_list_protocols() failed (%d)\n",
> > +                       ret);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  /*
> >   * SCMI agent devices binds devices of various uclasses depeding on
> >   * the FDT description. scmi_bind_protocol() is a generic bind sequence
> > @@ -243,6 +326,39 @@ static int scmi_bind_protocols(struct udevice *dev)
> >         struct driver *drv;
> >         struct udevice *proto;
> >
> > +       if (!uclass_get_device(UCLASS_SCMI_AGENT, 1, &agent)) {
> > +               /* This is a second SCMI agent */
> > +               dev_err(dev, "Cannot have more than one SCMI agent\n");
> > +               return -EEXIST;
> > +       }
> > +
> > +       /* initialize the device from device tree */
> > +       drv = DM_DRIVER_GET(scmi_base_drv);
> > +       name = "scmi-base.0";
> > +       ret = device_bind(dev, drv, name, NULL, ofnode_null(), &proto);
> 
> Why is this not in the devicetree too?

I don't know the history of discussions, but it is
because no binding for the base protocol is necessary, I guess,
as it is a mandatory protocol without any customisable parameters.
Please see kernel's Documentation/devicetree/bindings/firmware/arm,scmi.yaml.

> > +       if (ret) {
> > +               dev_err(dev, "failed to bind base protocol\n");
> > +               return ret;
> > +       }
> > +       ret = scmi_add_protocol(dev, SCMI_PROTOCOL_ID_BASE, proto);
> > +       if (ret) {
> > +               dev_err(dev, "failed to add protocol: %s, ret: %d\n",
> > +                       proto->name, ret);
> > +               return ret;
> > +       }
> > +
> > +       ret = device_probe(proto);
> > +       if (ret) {
> > +               dev_err(dev, "failed to probe base protocol\n");
> > +               return ret;
> > +       }
> > +
> > +       ret = scmi_fill_base_info(dev, proto);
> > +       if (ret) {
> > +               dev_err(dev, "failed to get base information\n");
> > +               return ret;
> > +       }
> > +
> >         dev_for_each_subnode(node, dev) {
> >                 u32 protocol_id;
> >
> > diff --git a/include/scmi_agent-uclass.h b/include/scmi_agent-uclass.h
> > index f9e7d3f51de1..0043a93c8d90 100644
> > --- a/include/scmi_agent-uclass.h
> > +++ b/include/scmi_agent-uclass.h
> > @@ -5,6 +5,7 @@
> >  #ifndef _SCMI_AGENT_UCLASS_H
> >  #define _SCMI_AGENT_UCLASS_H
> >
> > +#include <scmi_protocols.h>
> >  #include <dm/device.h>
> >
> >  struct scmi_msg;
> > @@ -12,16 +13,81 @@ struct scmi_channel;
> >
> >  /**
> >   * struct scmi_agent_priv - private data maintained by agent instance
> > + * @version:           Version
> > + * @num_agents:                Number of agents
> > + * @num_protocols:     Number of protocols
> > + * @impl_version:      Implementation version
> > + * @protocols:         Array of protocol IDs
> > + * @vendor:            Vendor name
> > + * @sub_vendor:                Sub-vendor name
> > + * @agent_name:                Agent name
> > + * agent_id:           Identifier of agent
> > + * @base_dev:          SCMI base protocol device
> >   * @clock_dev:         SCMI clock protocol device
> > - * @clock_dev:         SCMI reset domain protocol device
> > - * @clock_dev:         SCMI voltage domain protocol device
> > + * @resetdom_dev:      SCMI reset domain protocol device
> > + * @voltagedom_dev:    SCMI voltage domain protocol device
> >   */
> >  struct scmi_agent_priv {
> > +       u32 version;
> > +       u32 num_agents;
> > +       u32 num_protocols;
> > +       u32 impl_version;
> > +       u8 *protocols;
> > +       u8 vendor[SCMI_BASE_NAME_LENGTH_MAX];
> > +       u8 sub_vendor[SCMI_BASE_NAME_LENGTH_MAX];
> > +       u8 agent_name[SCMI_BASE_NAME_LENGTH_MAX];
> > +       u32 agent_id;
> > +       struct udevice *base_dev;
> >         struct udevice *clock_dev;
> >         struct udevice *resetdom_dev;
> >         struct udevice *voltagedom_dev;
> >  };
> >
> > +static inline u32 scmi_version(struct udevice *dev)
> > +{
> > +       return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->version;
> > +}
> > +
> > +static inline u32 scmi_num_agents(struct udevice *dev)
> > +{
> > +       return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->num_agents;
> > +}
> > +
> > +static inline u32 scmi_num_protocols(struct udevice *dev)
> > +{
> > +       return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->num_protocols;
> > +}
> > +
> > +static inline u32 scmi_impl_version(struct udevice *dev)
> > +{
> > +       return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->impl_version;
> > +}
> > +
> > +static inline u8 *scmi_protocols(struct udevice *dev)
> > +{
> > +       return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->protocols;
> > +}
> > +
> > +static inline u8 *scmi_vendor(struct udevice *dev)
> > +{
> > +       return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->vendor;
> > +}
> > +
> > +static inline u8 *scmi_sub_vendor(struct udevice *dev)
> > +{
> > +       return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->sub_vendor;
> > +}
> > +
> > +static inline u8 *scmi_agent_name(struct udevice *dev)
> > +{
> > +       return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->agent_name;
> > +}
> > +
> > +static inline u32 scmi_agent_id(struct udevice *dev)
> > +{
> > +       return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->agent_id;
> > +}
> 
> Why these helper functions? It seems better to avoid inlining access
> to dev_get_uclass_plat(). Are you worried about exposing the priv
> struct to clients?

Well, simply wanted to avoid repeating
"((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->)" in the code.

Thanks,
-Takahiro Akashi

> > +
> >  /**
> >   * struct scmi_transport_ops - The functions that a SCMI transport layer must implement.
> >   */
> > --
> > 2.41.0
> >
> 
> Regards,
> Simon