Hello Akashi-san,
Some minor comments.
> From: AKASHI Takahiro <takahiro.aka...@linaro.org>
> Sent: Friday, September 8, 2023 04:51
>
> 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.aka...@linaro.org>
> Reviewed-by: Simon Glass <s...@chromium.org>
> Reviewed-by: Etienne Carriere <etienne.carri...@foss.st.com>
> ---
> v3
> * typo fix: add '@' for argument name in function description
> * eliminate dev_get_uclass_plat()'s repeated in inline
> * modify the code for dynamically allocated sub-vendor/agent names
> v2
> * use helper functions, removing direct uses of ops
> ---
> drivers/firmware/scmi/scmi_agent-uclass.c | 116 ++++++++++++++++++++++
> include/scmi_agent-uclass.h | 66 ++++++++++++
> 2 files changed, 182 insertions(+)
>
> diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c
> b/drivers/firmware/scmi/scmi_agent-uclass.c
> index e823d105a3eb..87a667d60124 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;
> @@ -237,6 +243,83 @@ int devm_scmi_process_msg(struct udevice *dev, struct
> scmi_msg *msg)
> return scmi_process_msg(protocol->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;
> + }
> + 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 */
I think this line should move in below instruction block, next to
priv->agent_name = "NA", for consistency,
> + 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 = "NA";
> + }
> +
> + 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;
Value of ret here is not really relevant. I suggest to force to -EPROTO.
> + }
> +
> + return 0;
> +}
> +
> /*
> * SCMI agent devices binds devices of various uclasses depeding on
> * the FDT description. scmi_bind_protocol() is a generic bind sequence
> @@ -251,6 +334,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);
> + 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 3358c2b2d804..6fd6d54d6a5a 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
nitpicking: s/agent_id/@agent_id/
> + * @base_dev: SCMI base protocol device
> * @clock_dev: SCMI clock 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;
> + u8 *sub_vendor;
> + u8 *agent_name;
> + 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;
> +}
> +
> /**
> * struct scmi_transport_ops - The functions that a SCMI transport layer
> must implement.
> */
> --
> 2.34.1
>
