| Message ID | 20230726083808.140780-6-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: scmi: add SCMI base protocol support | expand |
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? > + 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? > + > /** > * struct scmi_transport_ops - The functions that a SCMI transport layer must implement. > */ > -- > 2.41.0 > Regards, Simon
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
Hi, On Wed, 26 Jul 2023 at 19:07, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Hi Simon, > > Thank you for your extensive review. Cheers. > > 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> > > [..] > > > +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. That's fine, so long as it would not be better for the caller to obtain the priv pointer and just access the fields in the struct, Regards, Simon
> From: AKASHI Takahiro <takahiro.akashi@linaro.org> > Sent: Thursday, July 27, 2023 03:07 > > 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> Reviewed-by: Etienne Carriere <etienne.carriere@foss.st.com> with below reported typo preferrably addressed. > > > > > 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 typo: s/agent_id/@agent_id/ > > > + * @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
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); + 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; +} + /** * struct scmi_transport_ops - The functions that a SCMI transport layer must implement. */
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(-)