Re: [PATCH v6 02/14] firmware: scmi: use a protocol's own channel if assigned

Hi,

On 10/11/23 12:06 PM, AKASHI Takahiro wrote:

SCMI specification allows any protocol to have its own channel for the transport. While the current SCMI driver may assign its channel from a device tree, the core function, devm_scmi_process_msg(), doesn't use a protocol's channel, but always use an agent's channel.

With this commit, devm_scmi_process_msg() tries to find and use a protocol's channel. If it doesn't exist, use an agent's.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Etienne Carriere etienne.carriere@foss.st.com

---

v5

• new commit (fixing a potential bug)

---

drivers/firmware/scmi/mailbox_agent.c | 5 +++-- drivers/firmware/scmi/optee_agent.c | 5 +++-- drivers/firmware/scmi/scmi_agent-uclass.c | 7 ++++--- drivers/firmware/scmi/smccc_agent.c | 5 +++-- include/scmi_agent-uclass.h | 8 +++++--- 5 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/scmi/mailbox_agent.c b/drivers/firmware/scmi/mailbox_agent.c index 8277c1860606..7ad3e8da9f08 100644 --- a/drivers/firmware/scmi/mailbox_agent.c +++ b/drivers/firmware/scmi/mailbox_agent.c @@ -94,13 +94,14 @@ static int setup_channel(struct udevice *dev, struct scmi_mbox_channel *chan) }

static int scmi_mbox_get_channel(struct udevice *dev,

• 		 struct udevice *protocol,
  	 struct scmi_channel **channel)
  

  { struct scmi_mbox_channel *base_chan = dev_get_plat(dev); struct scmi_mbox_channel *chan; int ret;

• if (!dev_read_prop(dev, "shmem", NULL)) {

• if (!dev_read_prop(protocol, "shmem", NULL)) { /* Uses agent base channel */ *channel = container_of(base_chan, struct scmi_channel, ref);

@@ -112,7 +113,7 @@ static int scmi_mbox_get_channel(struct udevice *dev, return -ENOMEM;

/* Setup a dedicated channel for the protocol */

• ret = setup_channel(dev, chan);

• ret = setup_channel(protocol, chan); if (ret) { free(chan); return ret;

diff --git a/drivers/firmware/scmi/optee_agent.c b/drivers/firmware/scmi/optee_agent.c index db927fb21405..e3e462774045 100644 --- a/drivers/firmware/scmi/optee_agent.c +++ b/drivers/firmware/scmi/optee_agent.c @@ -324,6 +324,7 @@ static int setup_channel(struct udevice *dev, struct scmi_optee_channel *chan) }

static int scmi_optee_get_channel(struct udevice *dev,

• 		  struct udevice *protocol,
  	  struct scmi_channel **channel)
  

  { struct scmi_optee_channel *base_chan = dev_get_plat(dev);

@@ -331,7 +332,7 @@ static int scmi_optee_get_channel(struct udevice *dev, u32 channel_id; int ret;

• if (dev_read_u32(dev, "linaro,optee-channel-id", &channel_id)) {

• if (dev_read_u32(protocol, "linaro,optee-channel-id", &channel_id)) { /* Uses agent base channel */ *channel = container_of(base_chan, struct scmi_channel, ref);

@@ -343,7 +344,7 @@ static int scmi_optee_get_channel(struct udevice *dev, if (!chan) return -ENOMEM;

• ret = setup_channel(dev, chan);

• ret = setup_channel(protocol, chan); if (ret) { free(chan); return ret;

diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c b/drivers/firmware/scmi/scmi_agent-uclass.c index ec58ccd2bc5d..a28692f39f4d 100644 --- a/drivers/firmware/scmi/scmi_agent-uclass.c +++ b/drivers/firmware/scmi/scmi_agent-uclass.c @@ -144,13 +144,14 @@ static const struct scmi_agent_ops *transport_dev_ops(struct udevice *dev)

• On return, @channel will be set.
• Return 0 on success and a negative errno on failure

*/ -static int scmi_of_get_channel(struct udevice *dev, struct scmi_channel **channel) +static int scmi_of_get_channel(struct udevice *dev, struct udevice *protocol,

• 	       struct scmi_channel **channel)
  

  { const struct scmi_agent_ops *ops;

  ops = transport_dev_ops(dev); if (ops->of_get_channel)

• return ops->of_get_channel(dev, channel);
  

• return ops->of_get_channel(dev, protocol, channel);
  

  else return -EPROTONOSUPPORT; }

@@ -166,7 +167,7 @@ int devm_scmi_of_get_channel(struct udevice *dev) return -ENODEV;

priv = dev_get_parent_priv(protocol);

• ret = scmi_of_get_channel(protocol->parent, &priv->channel);

• ret = scmi_of_get_channel(protocol->parent, protocol, &priv->channel); if (ret == -EPROTONOSUPPORT) { /* Drivers without a get_channel operator don't need a channel ref */ priv->channel = NULL;

diff --git a/drivers/firmware/scmi/smccc_agent.c b/drivers/firmware/scmi/smccc_agent.c index 6a52cd75d67b..972c6addde21 100644 --- a/drivers/firmware/scmi/smccc_agent.c +++ b/drivers/firmware/scmi/smccc_agent.c @@ -81,6 +81,7 @@ static int setup_channel(struct udevice *dev, struct scmi_smccc_channel *chan) }

static int scmi_smccc_get_channel(struct udevice *dev,

• 		  struct udevice *protocol,
  	  struct scmi_channel **channel)
  

  { struct scmi_smccc_channel *base_chan = dev_get_plat(dev);

@@ -88,7 +89,7 @@ static int scmi_smccc_get_channel(struct udevice *dev, u32 func_id; int ret;

• if (dev_read_u32(dev, "arm,smc-id", &func_id)) {

• if (dev_read_u32(protocol, "arm,smc-id", &func_id)) {

This seems to trigger the assert in https://elixir.bootlin.com/u-boot/latest/source/drivers/core/ofnode.c#L393 when building with DM_DEBUG=y on my RK3588 board, making it fail to boot into U-Boot CLI:

""" - found match at driver 'scmi-over-smccc' for 'arm,scmi-smc' ofnode_read_u32_index: arm,smc-id: 0x82000010 (2181038096) drivers/core/ofnode.c:393: ofnode_read_u32_index: Assertion `ofnode_valid(node)' failed. resetting ... """

Not sure what we should be doing here?

Cheers, Quentin