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

[Quentin Schulz]

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 at linaro.org>
> Reviewed-by: Etienne Carriere <etienne.carriere at 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