2020-02-13 04:04:57

by Peng Fan

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

From: Peng Fan <[email protected]>


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


2020-02-13 04:05:04

by Peng Fan

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

From: Peng Fan <[email protected]>

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 <[email protected]>
---
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

2020-02-13 04:05:23

by Peng Fan

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

From: Peng Fan <[email protected]>

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 <[email protected]>
---
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

2020-02-13 10:54:43

by Sudeep Holla

[permalink] [raw]
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, [email protected] wrote:
> From: Peng Fan <[email protected]>
>
> 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 <[email protected]>
> ---
> 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

2020-02-13 11:06:12

by Sudeep Holla

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

On Thu, Feb 13, 2020 at 11:58:50AM +0800, [email protected] wrote:
> From: Peng Fan <[email protected]>
>
> 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 <[email protected]>
> ---
> 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

2020-02-14 00:59:34

by Peng Fan

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

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, [email protected] wrote:
> > From: Peng Fan <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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

2020-02-14 01:05:19

by Peng Fan

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

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, [email protected] wrote:
> > From: Peng Fan <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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