[PATCH V2 0/2] firmware: arm_scmi: add smc/hvc transports support

[PATCH V2 0/2] firmware: arm_scmi: add smc/hvc transports support

[peng.fan]

From: Peng Fan <peng.fan@nxp.com>


V2:
 patch 1/2: only add smc-id property
 patch 2/2: Parse smc/hvc from psci node
	    Use prot_id as 2nd arg when issue smc/hvc
	    Differentiate tranports using mboxes or smc-id property

This is to add smc/hvc transports support, based on Viresh's v6.
SCMI firmware could be implemented in EL3, S-EL1, NS-EL2 or other
A core exception level. Then smc/hvc could be used. And for vendor
specific firmware, a wrapper layer could added in EL3, S-EL1,
NS-EL2 and etc to translate SCMI calls to vendor specific firmware calls.

A new compatible string arm,scmi-smc is added. arm,scmi is still for
mailbox transports.

All protocol share same smc/hvc id, the protocol id will be take as
2nd arg when issue smc/hvc.
Each protocol could use its own shmem or share the same shmem
Per smc/hvc, only Tx supported.

Peng Fan (2):
  dt-bindings: arm: arm,scmi: add smc/hvc transports
  firmware: arm_scmi: add smc/hvc transports

 Documentation/devicetree/bindings/arm/arm,scmi.txt |   1 +
 drivers/firmware/arm_scmi/Makefile                 |   2 +-
 drivers/firmware/arm_scmi/common.h                 |   4 +-
 drivers/firmware/arm_scmi/driver.c                 |  11 +-
 drivers/firmware/arm_scmi/mailbox.c                |   2 +-
 drivers/firmware/arm_scmi/smc.c                    | 222 +++++++++++++++++++++
 6 files changed, 235 insertions(+), 7 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/smc.c

-- 
2.16.4

[PATCH V2 1/2] dt-bindings: arm: arm,scmi: add smc/hvc transports

[peng.fan]

From: Peng Fan <peng.fan@nxp.com>

SCMI could use SMC/HVC as tranports. Since there is no
standardized id, we need to use vendor specific id. So
add into devicetree binding doc.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 Documentation/devicetree/bindings/arm/arm,scmi.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt
index f493d69e6194..dacc62dc248b 100644
--- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
@@ -25,6 +25,7 @@ The scmi node with the following properties shall be under the /firmware/ node.
 	  protocol identifier for a given sub-node.
 - #size-cells : should be '0' as 'reg' property doesn't have any size
 	  associated with it.
+- smc-id : SMC id required when using smc or hvc transports
 
 Optional properties:
 
-- 
2.16.4

Re: [PATCH V2 1/2] dt-bindings: arm: arm,scmi: add smc/hvc transports

[Sudeep Holla]

On Thu, Feb 13, 2020 at 11:58:49AM +0800, peng.fan@nxp.com wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> SCMI could use SMC/HVC as tranports. Since there is no
> standardized id, we need to use vendor specific id. So
> add into devicetree binding doc.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  Documentation/devicetree/bindings/arm/arm,scmi.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt
> index f493d69e6194..dacc62dc248b 100644
> --- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
> +++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
> @@ -25,6 +25,7 @@ The scmi node with the following properties shall be under the /firmware/ node.
>  	  protocol identifier for a given sub-node.
>  - #size-cells : should be '0' as 'reg' property doesn't have any size
>  	  associated with it.
> +- smc-id : SMC id required when using smc or hvc transports

IIUC, "arm,smc-id" is preferred more.

Why did you drop "arm,scmi-smc" ?

--
Regards,
Sudeep

RE: [PATCH V2 1/2] dt-bindings: arm: arm,scmi: add smc/hvc transports

[Peng Fan]

Hi Sudeep,

> Subject: Re: [PATCH V2 1/2] dt-bindings: arm: arm,scmi: add smc/hvc
> transports
> 
> On Thu, Feb 13, 2020 at 11:58:49AM +0800, peng.fan@nxp.com wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > SCMI could use SMC/HVC as tranports. Since there is no standardized
> > id, we need to use vendor specific id. So add into devicetree binding
> > doc.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  Documentation/devicetree/bindings/arm/arm,scmi.txt | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt
> > b/Documentation/devicetree/bindings/arm/arm,scmi.txt
> > index f493d69e6194..dacc62dc248b 100644
> > --- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
> > +++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
> > @@ -25,6 +25,7 @@ The scmi node with the following properties shall be
> under the /firmware/ node.
> >  	  protocol identifier for a given sub-node.
> >  - #size-cells : should be '0' as 'reg' property doesn't have any size
> >  	  associated with it.
> > +- smc-id : SMC id required when using smc or hvc transports
> 
> IIUC, "arm,smc-id" is preferred more.

ok. Fix in v3.

> 
> Why did you drop "arm,scmi-smc" ?

Per our discuss in v1 patchset, mailbox/smc-id could be used
to differentiate mailbox and smc transports.

https://lkml.org/lkml/2020/2/11/226

So I still use "arm,scmi" for smc tranports. 


Thanks,
Peng.

> 
> --
> Regards,
> Sudeep

[PATCH V2 2/2] firmware: arm_scmi: add smc/hvc transports

[peng.fan]

From: Peng Fan <peng.fan@nxp.com>

Add SCMI smc/hvc transports support.

Take smc-id as the 2nd arg, and protocol id as the 2nd arg when
invokding SMC/HVC. Since we need protocol id, so add this parrameter
to chan_setup, then smc transport driver could directly use it.
There is no Rx, only Tx because of smc/hvc not support Rx.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/arm_scmi/Makefile  |   2 +-
 drivers/firmware/arm_scmi/common.h  |   4 +-
 drivers/firmware/arm_scmi/driver.c  |  11 +-
 drivers/firmware/arm_scmi/mailbox.c |   2 +-
 drivers/firmware/arm_scmi/smc.c     | 222 ++++++++++++++++++++++++++++++++++++
 5 files changed, 234 insertions(+), 7 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/smc.c

diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index 6694d0d908d6..6b1b0d6c6d0e 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -2,6 +2,6 @@
 obj-y	= scmi-bus.o scmi-driver.o scmi-protocols.o scmi-transport.o
 scmi-bus-y = bus.o
 scmi-driver-y = driver.o
-scmi-transport-y = mailbox.o shmem.o
+scmi-transport-y = mailbox.o shmem.o smc.o
 scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
 obj-$(CONFIG_ARM_SCMI_POWER_DOMAIN) += scmi_pm_domain.o
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index fd091a4ccbff..b8cd33be8272 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -182,7 +182,8 @@ struct scmi_chan_info {
  */
 struct scmi_transport_ops {
 	bool (*chan_available)(struct device *dev, int idx);
-	int (*chan_setup)(struct scmi_chan_info *cinfo, struct device *dev, bool tx);
+	int (*chan_setup)(struct scmi_chan_info *cinfo, struct device *dev,
+			  int prot_id, bool tx);
 	int (*chan_free)(int id, void *p, void *data);
 	int (*send_message)(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer);
 	void (*mark_txdone)(struct scmi_chan_info *cinfo, int ret);
@@ -207,6 +208,7 @@ struct scmi_desc {
 };
 
 extern const struct scmi_desc scmi_mailbox_desc;
+extern const struct scmi_desc scmi_smc_desc;
 
 void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr);
 void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr, int id);
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index dbec767222e9..65c56328e6d8 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -595,7 +595,7 @@ static int scmi_chan_setup(struct scmi_info *info, struct device *dev,
 
 	cinfo->dev = dev;
 
-	ret = info->desc->ops->chan_setup(cinfo, info->dev, tx);
+	ret = info->desc->ops->chan_setup(cinfo, info->dev, prot_id, tx);
 	if (ret)
 		return ret;
 
@@ -687,8 +687,11 @@ static int scmi_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *child, *np = dev->of_node;
 
-	desc = of_device_get_match_data(dev);
-	if (!desc)
+	if (of_get_property(np, "mboxes", NULL))
+		desc = &scmi_mailbox_desc;
+	else if (of_get_property(np, "smc-id", NULL))
+		desc = &scmi_smc_desc;
+	else
 		return -EINVAL;
 
 	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
@@ -826,7 +829,7 @@ ATTRIBUTE_GROUPS(versions);
 
 /* Each compatible listed below must have descriptor associated with it */
 static const struct of_device_id scmi_of_match[] = {
-	{ .compatible = "arm,scmi", .data = &scmi_mailbox_desc },
+	{ .compatible = "arm,scmi",  },
 	{ /* Sentinel */ },
 };
 
diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
index 73077bbc4ad9..b06d5d30fe90 100644
--- a/drivers/firmware/arm_scmi/mailbox.c
+++ b/drivers/firmware/arm_scmi/mailbox.c
@@ -53,7 +53,7 @@ static bool mailbox_chan_available(struct device *dev, int idx)
 }
 
 static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
-			      bool tx)
+			      int prot_id, bool tx)
 {
 	const char *desc = tx ? "Tx" : "Rx";
 	struct device *cdev = cinfo->dev;
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
new file mode 100644
index 000000000000..67bbf33efb1d
--- /dev/null
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -0,0 +1,222 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Message SMC/HVC
+ * Transport driver
+ *
+ * Copyright 2020 NXP
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+
+#include "common.h"
+
+/**
+ * struct scmi_smc - Structure representing a SCMI smc transport
+ *
+ * @cinfo: SCMI channel info
+ * @shmem: Transmit/Receive shared memory area
+ * @func_id: smc/hvc call function id
+ * @prot_id: The protocol id
+ */
+
+struct scmi_smc {
+	struct scmi_chan_info *cinfo;
+	struct scmi_shared_mem __iomem *shmem;
+	u32 func_id;
+	int prot_id;
+};
+
+typedef unsigned long (scmi_smc_fn)(unsigned long, unsigned long,
+				    unsigned long, unsigned long,
+				    unsigned long, unsigned long,
+				    unsigned long, unsigned long);
+static scmi_smc_fn *invoke_scmi_smc_fn;
+
+#define client_to_scmi_smc(c) container_of(c, struct scmi_smc, cl)
+
+static bool smc_chan_available(struct device *dev, int idx)
+{
+	return true;
+}
+
+static unsigned long
+__invoke_scmi_fn_hvc(unsigned long function_id, unsigned long arg0,
+		     unsigned long arg1, unsigned long arg2,
+		     unsigned long arg3, unsigned long arg4,
+		     unsigned long arg5, unsigned long arg6)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4, arg5,
+		      arg6, &res);
+
+	return res.a0;
+}
+
+static unsigned long
+__invoke_scmi_fn_smc(unsigned long function_id, unsigned long arg0,
+		     unsigned long arg1, unsigned long arg2,
+		     unsigned long arg3, unsigned long arg4,
+		     unsigned long arg5, unsigned long arg6)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4, arg5,
+		      arg6, &res);
+
+	return res.a0;
+}
+
+static int scmi_smc_conduit_method(struct device_node *np)
+{
+	const char *method;
+
+	if (invoke_scmi_smc_fn)
+		return 0;
+
+	if (of_property_read_string(np, "method", &method))
+		return -ENXIO;
+
+	if (!strcmp("hvc", method)) {
+		invoke_scmi_smc_fn = __invoke_scmi_fn_hvc;
+	} else if (!strcmp("smc", method)) {
+		invoke_scmi_smc_fn = __invoke_scmi_fn_smc;
+	} else {
+		pr_warn("invalid \"method\" property: %s\n", method);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
+			  int prot_id, bool tx)
+{
+	struct device *cdev = cinfo->dev;
+	struct scmi_smc *scmi_info;
+	struct device_node *np;
+	resource_size_t size;
+	struct resource res;
+	u32 func_id;
+	int ret;
+
+	if (!tx)
+		return -ENODEV;
+
+	scmi_info = devm_kzalloc(dev, sizeof(*scmi_info), GFP_KERNEL);
+	if (!scmi_info)
+		return -ENOMEM;
+
+	np = of_parse_phandle(cdev->of_node, "shmem", 0);
+	if (!np)
+		np = of_parse_phandle(dev->of_node, "shmem", 0);
+	ret = of_address_to_resource(np, 0, &res);
+	of_node_put(np);
+	if (ret) {
+		dev_err(cdev, "failed to get SCMI Tx shared memory\n");
+		return ret;
+	}
+
+	size = resource_size(&res);
+	scmi_info->shmem = devm_ioremap(dev, res.start, size);
+	if (!scmi_info->shmem) {
+		dev_err(dev, "failed to ioremap SCMI Tx shared memory\n");
+		return -EADDRNOTAVAIL;
+	}
+
+	ret = of_property_read_u32(dev->of_node, "smc-id", &func_id);
+	if (ret < 0)
+		return ret;
+
+	np = of_find_node_by_path("/psci");
+	if (!np) {
+		dev_err(dev, "Not able to find /psci node\n");
+		return -ENODEV;
+	}
+
+	ret = scmi_smc_conduit_method(np);
+	if (ret)
+		return ret;
+
+	of_node_put(np);
+
+	scmi_info->func_id = func_id;
+	scmi_info->cinfo = cinfo;
+	scmi_info->prot_id = prot_id;
+	cinfo->transport_info = scmi_info;
+
+	return 0;
+}
+
+static int smc_chan_free(int id, void *p, void *data)
+{
+	struct scmi_chan_info *cinfo = p;
+	struct scmi_smc *scmi_info = cinfo->transport_info;
+
+	cinfo->transport_info = NULL;
+	scmi_info->cinfo = NULL;
+
+	scmi_free_channel(cinfo, data, id);
+
+	return 0;
+}
+
+static int smc_send_message(struct scmi_chan_info *cinfo,
+			    struct scmi_xfer *xfer)
+{
+	struct scmi_smc *scmi_info = cinfo->transport_info;
+	int ret;
+
+	shmem_tx_prepare(scmi_info->shmem, xfer);
+
+	ret = invoke_scmi_smc_fn(scmi_info->func_id, scmi_info->prot_id,
+				 0, 0, 0, 0, 0, 0);
+	if (ret > 0)
+		ret = 0;
+
+	scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
+
+	return ret;
+}
+
+static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret)
+{
+}
+
+static void smc_fetch_response(struct scmi_chan_info *cinfo,
+			       struct scmi_xfer *xfer)
+{
+	struct scmi_smc *scmi_info = cinfo->transport_info;
+
+	shmem_fetch_response(scmi_info->shmem, xfer);
+}
+
+static bool
+smc_poll_done(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
+{
+	struct scmi_smc *scmi_info = cinfo->transport_info;
+
+	return shmem_poll_done(scmi_info->shmem, xfer);
+}
+
+static struct scmi_transport_ops scmi_smc_ops = {
+	.chan_available = smc_chan_available,
+	.chan_setup = smc_chan_setup,
+	.chan_free = smc_chan_free,
+	.send_message = smc_send_message,
+	.mark_txdone = smc_mark_txdone,
+	.fetch_response = smc_fetch_response,
+	.poll_done = smc_poll_done,
+};
+
+const struct scmi_desc scmi_smc_desc = {
+	.ops = &scmi_smc_ops,
+	.max_rx_timeout_ms = 30,
+	.max_msg = 1,
+	.max_msg_size = 128,
+};
-- 
2.16.4

Re: [PATCH V2 2/2] firmware: arm_scmi: add smc/hvc transports

[Sudeep Holla]

On Thu, Feb 13, 2020 at 11:58:50AM +0800, peng.fan@nxp.com wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Add SCMI smc/hvc transports support.
> 
> Take smc-id as the 2nd arg, and protocol id as the 2nd arg when
> invokding SMC/HVC. Since we need protocol id, so add this parrameter
> to chan_setup, then smc transport driver could directly use it.
> There is no Rx, only Tx because of smc/hvc not support Rx.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/arm_scmi/Makefile  |   2 +-
>  drivers/firmware/arm_scmi/common.h  |   4 +-
>  drivers/firmware/arm_scmi/driver.c  |  11 +-
>  drivers/firmware/arm_scmi/mailbox.c |   2 +-
>  drivers/firmware/arm_scmi/smc.c     | 222 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 234 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/firmware/arm_scmi/smc.c

[...]

>
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index dbec767222e9..65c56328e6d8 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -595,7 +595,7 @@ static int scmi_chan_setup(struct scmi_info *info, struct device *dev,
>  
>  	cinfo->dev = dev;
>  
> -	ret = info->desc->ops->chan_setup(cinfo, info->dev, tx);
> +	ret = info->desc->ops->chan_setup(cinfo, info->dev, prot_id, tx);

Why do you need this ?

>  	if (ret)
>  		return ret;
>  
> @@ -826,7 +829,7 @@ ATTRIBUTE_GROUPS(versions);
>  
>  /* Each compatible listed below must have descriptor associated with it */
>  static const struct of_device_id scmi_of_match[] = {
> -	{ .compatible = "arm,scmi", .data = &scmi_mailbox_desc },
> +	{ .compatible = "arm,scmi",  },

Don't do this, get "arm,scmi-smc"

>  	{ /* Sentinel */ },
>  };
>  
[...]


> +static unsigned long
> +__invoke_scmi_fn_hvc(unsigned long function_id, unsigned long arg0,
> +		     unsigned long arg1, unsigned long arg2,
> +		     unsigned long arg3, unsigned long arg4,
> +		     unsigned long arg5, unsigned long arg6)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4, arg5,
> +		      arg6, &res);
> +
> +	return res.a0;
> +}
> +
> +static unsigned long
> +__invoke_scmi_fn_smc(unsigned long function_id, unsigned long arg0,
> +		     unsigned long arg1, unsigned long arg2,
> +		     unsigned long arg3, unsigned long arg4,
> +		     unsigned long arg5, unsigned long arg6)
> +{
> +	struct arm_smccc_res res;
> +
> +	arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4, arg5,
> +		      arg6, &res);
> +
> +	return res.a0;
> +}
> +
> +static int scmi_smc_conduit_method(struct device_node *np)
> +{
> +	const char *method;
> +
> +	if (invoke_scmi_smc_fn)
> +		return 0;
> +
> +	if (of_property_read_string(np, "method", &method))
> +		return -ENXIO;
> +
> +	if (!strcmp("hvc", method)) {
> +		invoke_scmi_smc_fn = __invoke_scmi_fn_hvc;
> +	} else if (!strcmp("smc", method)) {
> +		invoke_scmi_smc_fn = __invoke_scmi_fn_smc;
> +	} else {
> +		pr_warn("invalid \"method\" property: %s\n", method);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +

You don't the above functions

[...]

> +
> +	np = of_find_node_by_path("/psci");
> +	if (!np) {
> +		dev_err(dev, "Not able to find /psci node\n");
> +		return -ENODEV;
> +	}

No need for this as mentioned below.

> +
> +	ret = scmi_smc_conduit_method(np);

Just use arm_smccc_1_1_get_conduit if you want to get the conduit which
I don't think you need. You can just use arm_smccc_1_1_invoke() directly.

--
Regards,
Sudeep

RE: [PATCH V2 2/2] firmware: arm_scmi: add smc/hvc transports

[Peng Fan]

Hi Sudeep,

> Subject: Re: [PATCH V2 2/2] firmware: arm_scmi: add smc/hvc transports
> 
> On Thu, Feb 13, 2020 at 11:58:50AM +0800, peng.fan@nxp.com wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add SCMI smc/hvc transports support.
> >
> > Take smc-id as the 2nd arg, and protocol id as the 2nd arg when
> > invokding SMC/HVC. Since we need protocol id, so add this parrameter
> > to chan_setup, then smc transport driver could directly use it.
> > There is no Rx, only Tx because of smc/hvc not support Rx.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/firmware/arm_scmi/Makefile  |   2 +-
> >  drivers/firmware/arm_scmi/common.h  |   4 +-
> >  drivers/firmware/arm_scmi/driver.c  |  11 +-
> >  drivers/firmware/arm_scmi/mailbox.c |   2 +-
> >  drivers/firmware/arm_scmi/smc.c     | 222
> ++++++++++++++++++++++++++++++++++++
> >  5 files changed, 234 insertions(+), 7 deletions(-)  create mode
> > 100644 drivers/firmware/arm_scmi/smc.c
> 
> [...]
> 
> >
> > diff --git a/drivers/firmware/arm_scmi/driver.c
> > b/drivers/firmware/arm_scmi/driver.c
> > index dbec767222e9..65c56328e6d8 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -595,7 +595,7 @@ static int scmi_chan_setup(struct scmi_info *info,
> > struct device *dev,
> >
> >  	cinfo->dev = dev;
> >
> > -	ret = info->desc->ops->chan_setup(cinfo, info->dev, tx);
> > +	ret = info->desc->ops->chan_setup(cinfo, info->dev, prot_id, tx);
> 
> Why do you need this ?

For smc tranports, all protocols share same smd-id, but if protocols
not share the same shmem, we need let firmare know which protocol
issues the smc call. So I take prot_id as an arguments of smc call.

> 
> >  	if (ret)
> >  		return ret;
> >
> > @@ -826,7 +829,7 @@ ATTRIBUTE_GROUPS(versions);
> >
> >  /* Each compatible listed below must have descriptor associated with
> > it */  static const struct of_device_id scmi_of_match[] = {
> > -	{ .compatible = "arm,scmi", .data = &scmi_mailbox_desc },
> > +	{ .compatible = "arm,scmi",  },
> 
> Don't do this, get "arm,scmi-smc"

You mean code as below?
/* Each compatible listed below must have descriptor associated with it */
static const struct of_device_id scmi_of_match[] = {
         { .compatible = "arm,scmi", .data = &scmi_mailbox_desc },
         { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc },
         { /* Sentinel */ },
};

But since we could use mboxes and smc-id to know the tranports type,
do we really need arm,scmi-smc?

> 
> >  	{ /* Sentinel */ },
> >  };
> >
> [...]
> 
> 
> > +static unsigned long
> > +__invoke_scmi_fn_hvc(unsigned long function_id, unsigned long arg0,
> > +		     unsigned long arg1, unsigned long arg2,
> > +		     unsigned long arg3, unsigned long arg4,
> > +		     unsigned long arg5, unsigned long arg6) {
> > +	struct arm_smccc_res res;
> > +
> > +	arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4, arg5,
> > +		      arg6, &res);
> > +
> > +	return res.a0;
> > +}
> > +
> > +static unsigned long
> > +__invoke_scmi_fn_smc(unsigned long function_id, unsigned long arg0,
> > +		     unsigned long arg1, unsigned long arg2,
> > +		     unsigned long arg3, unsigned long arg4,
> > +		     unsigned long arg5, unsigned long arg6) {
> > +	struct arm_smccc_res res;
> > +
> > +	arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4, arg5,
> > +		      arg6, &res);
> > +
> > +	return res.a0;
> > +}
> > +
> > +static int scmi_smc_conduit_method(struct device_node *np) {
> > +	const char *method;
> > +
> > +	if (invoke_scmi_smc_fn)
> > +		return 0;
> > +
> > +	if (of_property_read_string(np, "method", &method))
> > +		return -ENXIO;
> > +
> > +	if (!strcmp("hvc", method)) {
> > +		invoke_scmi_smc_fn = __invoke_scmi_fn_hvc;
> > +	} else if (!strcmp("smc", method)) {
> > +		invoke_scmi_smc_fn = __invoke_scmi_fn_smc;
> > +	} else {
> > +		pr_warn("invalid \"method\" property: %s\n", method);
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> You don't the above functions

ok

> 
> [...]
> 
> > +
> > +	np = of_find_node_by_path("/psci");
> > +	if (!np) {
> > +		dev_err(dev, "Not able to find /psci node\n");
> > +		return -ENODEV;
> > +	}
> 
> No need for this as mentioned below.

ok

> 
> > +
> > +	ret = scmi_smc_conduit_method(np);
> 
> Just use arm_smccc_1_1_get_conduit if you want to get the conduit which I
> don't think you need. You can just use arm_smccc_1_1_invoke() directly.

Fix in v3.

I'll post v3 after we have an agreement on whether we need a new compatible
string arm,scmi-smc and the prot_id introduced in chan_setup.

Thanks,
Peng.

> 
> --
> Regards,
> Sudeep