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

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

[peng.fan]

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

V5:
 Add Rob's R-b tag for patch 1
 Add mutex for patch 2

V4:
 Drop prot_id in scmi_chan_info, since not used for now.

V3:
 Add back arm,scmi-smc compatible string
 Change smc-id to arm,smc-id
 Directly use arm_smccc_1_1_invoke
 Add prot_id in scmi_chan_info for per protocol shmem usage.

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
https://lore.kernel.org/patchwork/cover/1193435/

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.

Per smc/hvc, only Tx supported.

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

 Documentation/devicetree/bindings/arm/arm,scmi.txt |   3 +-
 drivers/firmware/arm_scmi/Makefile                 |   2 +-
 drivers/firmware/arm_scmi/common.h                 |   1 +
 drivers/firmware/arm_scmi/driver.c                 |   1 +
 drivers/firmware/arm_scmi/smc.c                    | 152 +++++++++++++++++++++
 5 files changed, 157 insertions(+), 2 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/smc.c

-- 
2.16.4

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

[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.

Also add arm,scmi-smc compatible string for smc/hvc transport

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 Documentation/devicetree/bindings/arm/arm,scmi.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt
index f493d69e6194..4ce57b88f84d 100644
--- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
@@ -14,7 +14,7 @@ Required properties:
 
 The scmi node with the following properties shall be under the /firmware/ node.
 
-- compatible : shall be "arm,scmi"
+- compatible : shall be "arm,scmi" or "arm,scmi-smc" for smc/hvc transports
 - mboxes: List of phandle and mailbox channel specifiers. It should contain
 	  exactly one or two mailboxes, one for transmitting messages("tx")
 	  and another optional for receiving the notifications("rx") if
@@ -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.
+- arm,smc-id : SMC id required when using smc or hvc transports
 
 Optional properties:
 
-- 
2.16.4

[PATCH V5 2/2] firmware: arm_scmi: add smc/hvc transport

[peng.fan]

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

Take arm,smc-id as the 1st arg, leave the other args as zero for now.
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 |   1 +
 drivers/firmware/arm_scmi/driver.c |   1 +
 drivers/firmware/arm_scmi/smc.c    | 152 +++++++++++++++++++++++++++++++++++++
 4 files changed, 155 insertions(+), 1 deletion(-)
 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 5ac06469b01c..94fc2b2df940 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -210,6 +210,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..e76a3fab1074 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -827,6 +827,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-smc", .data = &scmi_smc_desc},
 	{ /* Sentinel */ },
 };
 
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
new file mode 100644
index 000000000000..336168e40f49
--- /dev/null
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -0,0 +1,152 @@
+// 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/mutex.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
+ */
+
+struct scmi_smc {
+	struct scmi_chan_info *cinfo;
+	struct scmi_shared_mem __iomem *shmem;
+	u32 func_id;
+};
+
+static DEFINE_MUTEX(smc_mutex);
+
+static bool smc_chan_available(struct device *dev, int idx)
+{
+	return true;
+}
+
+static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
+			  bool tx)
+{
+	struct device *cdev = cinfo->dev;
+	struct scmi_smc *scmi_info;
+	resource_size_t size;
+	struct resource res;
+	struct device_node *np;
+	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, "arm,smc-id", &func_id);
+	if (ret < 0)
+		return ret;
+
+	scmi_info->func_id = func_id;
+	scmi_info->cinfo = cinfo;
+	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;
+	struct arm_smccc_res res;
+
+	mutex_lock(&smc_mutex);
+
+	shmem_tx_prepare(scmi_info->shmem, xfer);
+
+	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
+	scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
+
+	mutex_unlock(&smc_mutex);
+
+	return res.a0;
+}
+
+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 V5 1/2] dt-bindings: arm: arm,scmi: add smc/hvc transport

[Florian Fainelli]

On 3/8/2020 6:24 AM, 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.
> 
> Also add arm,scmi-smc compatible string for smc/hvc transport
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

[Peng Fan]

Hi Sudeep,

> Subject: [PATCH V5 0/2] firmware: arm_scmi: add smc/hvc transports support

Are you fine with this patchset? Or You expect multi channel support?

We have some features depending on this for long time, so hope smc transports
could be finalized sooner.

Thanks,
Peng.

> 
> From: Peng Fan <peng.fan@nxp.com>
> 
> V5:
>  Add Rob's R-b tag for patch 1
>  Add mutex for patch 2
> 
> V4:
>  Drop prot_id in scmi_chan_info, since not used for now.
> 
> V3:
>  Add back arm,scmi-smc compatible string  Change smc-id to arm,smc-id
> Directly use arm_smccc_1_1_invoke  Add prot_id in scmi_chan_info for per
> protocol shmem usage.
> 
> 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
> https://lore.kernel.org/patchwork/cover/1193435/
> 
> 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.
> 
> Per smc/hvc, only Tx supported.
> 
> Peng Fan (2):
>   dt-bindings: arm: arm,scmi: add smc/hvc transport
>   firmware: arm_scmi: add smc/hvc transport
> 
>  Documentation/devicetree/bindings/arm/arm,scmi.txt |   3 +-
>  drivers/firmware/arm_scmi/Makefile                 |   2 +-
>  drivers/firmware/arm_scmi/common.h                 |   1 +
>  drivers/firmware/arm_scmi/driver.c                 |   1 +
>  drivers/firmware/arm_scmi/smc.c                    | 152
> +++++++++++++++++++++
>  5 files changed, 157 insertions(+), 2 deletions(-)  create mode 100644
> drivers/firmware/arm_scmi/smc.c
> 
> --
> 2.16.4

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

[Sudeep Holla]

On Fri, Mar 20, 2020 at 08:27:47AM +0000, Peng Fan wrote:
> Hi Sudeep,
>
> > Subject: [PATCH V5 0/2] firmware: arm_scmi: add smc/hvc transports support
>
> Are you fine with this patchset? Or You expect multi channel support?
>
> We have some features depending on this for long time, so hope smc transports
> could be finalized sooner.
>

I have applied these patches as is, yet to push it out. I am waiting for
my v5.7 scmi PR to reach arm-soc next before I update my scmi branch. I will
let you know replying to this thread once that happens.

--
Regards,
Sudeep

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

[Peng Fan]

> Subject: Re: [PATCH V5 0/2] firmware: arm_scmi: add smc/hvc transports
> support
> 
> On Fri, Mar 20, 2020 at 08:27:47AM +0000, Peng Fan wrote:
> > Hi Sudeep,
> >
> > > Subject: [PATCH V5 0/2] firmware: arm_scmi: add smc/hvc transports
> > > support
> >
> > Are you fine with this patchset? Or You expect multi channel support?
> >
> > We have some features depending on this for long time, so hope smc
> > transports could be finalized sooner.
> >
> 
> I have applied these patches as is, yet to push it out. I am waiting for my v5.7
> scmi PR to reach arm-soc next before I update my scmi branch. I will let you
> know replying to this thread once that happens.

Appreciate.

Thanks,
Peng.

> 
> --
> Regards,
> Sudeep

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

[Sudeep Holla]

On Fri, Mar 20, 2020 at 08:27:47AM +0000, Peng Fan wrote:
> Hi Sudeep,
>
> > Subject: [PATCH V5 0/2] firmware: arm_scmi: add smc/hvc transports support
>
> Are you fine with this patchset? Or You expect multi channel support?
>

I applied these patches[1]. I also looked at multi channel support and
I think it should be simple. I have made changes and will post soon.
I would like you to review and if possible test. I don't want to break
the existing single channel, so please do test in your setup for single
channel itself.

--
Regards,
Sudeep

[1] git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git for-next/scmi

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

[Sudeep Holla]

On Fri, Mar 27, 2020 at 04:32:53PM +0000, Sudeep Holla wrote:
> On Fri, Mar 20, 2020 at 08:27:47AM +0000, Peng Fan wrote:
> > Hi Sudeep,
> >
> > > Subject: [PATCH V5 0/2] firmware: arm_scmi: add smc/hvc transports support
> >
> > Are you fine with this patchset? Or You expect multi channel support?
> >
> 
> I applied these patches[1]. I also looked at multi channel support and
> I think it should be simple. I have made changes and will post soon.
> I would like you to review and if possible test. I don't want to break
> the existing single channel, so please do test in your setup for single
> channel itself.
> 

This caused build break when !CONFIG_HAVE_ARM_SMCCC, I have added the
below fixup. Posting the same for sake of tracking.

Regards,
Sudeep

--
diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index 6b1b0d6c6d0e..11b238f81923 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -2,6 +2,8 @@
 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 smc.o
+scmi-transport-y = shmem.o
+scmi-transport-$(CONFIG_MAILBOX) += mailbox.o
+scmi-transport-$(CONFIG_HAVE_ARM_SMCCC) += 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 94fc2b2df940..34bfadca14cc 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -210,7 +210,9 @@ struct scmi_desc {
 };

 extern const struct scmi_desc scmi_mailbox_desc;
+#ifdef CONFIG_HAVE_ARM_SMCCC
 extern const struct scmi_desc scmi_smc_desc;
+#endif

 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 e76a3fab1074..6ef61e52eef7 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -827,7 +827,9 @@ 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 },
+#ifdef CONFIG_HAVE_ARM_SMCCC
 	{ .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
+#endif
 	{ /* Sentinel */ },
 };

[PATCH V5 2/2] firmware: arm_scmi: add smc/hvc transport

[Etienne Carriere]

Hello Peng,

I  have 2 comments on this change. The main is about using
arm_smccc_1_1_invoke(). Below some details and I added comments
inside you patch. The second of on SMC return value, see my
comment in your patch below.


About arm_smccc_1_1_invoke(), this functon currently relies on PSCI
driver to define a conduit method but SCMI agent driver does not
mandate CONFIG_PSCI to be enable.

Could you add an optional "method" property for "arm,scmi-smc" for platforms
willing to not rely on PSCI Linux driver? If no property "method" is
defined in the FDT, invocation relies on arm_smccc_1_1_invoke().

"method" naming mimics what is done in the OP-TEE driver (drivers/tee/optee/).
Here is a proposal for the documenting property "method" in 
Documentation/arm,scmi.txt:

- method : "smc" or "hvc"
            Optional property defining the conduit method for to be used
	    for invoking the SCMI server in secure world.
	    "smc" states instruction SMC #0 is used whereas "hvc" states
	    instruction HVC #0 is used.



Regards,
Etienne


> From: Peng Fan <peng.fan@nxp.commm>
> 
> Take arm,smc-id as the 1st arg, leave the other args as zero for now.
> There is no Rx, only Tx because of smc/hvc not support Rx.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>
> (...)
>
> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> new file mode 100644
> index 000000000000..336168e40f49
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -0,0 +1,152 @@
> +// 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/mutex.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
> + */
> +
> +struct scmi_smc {
> +	struct scmi_chan_info *cinfo;
> +	struct scmi_shared_mem __iomem *shmem;
> +	u32 func_id;
> +};

Add here a field for the secure world invocation function handler:

	scmi_arm_smccc_invoke_fn *invoke_fn;

With function proto type defined:

  typedef void (scmi_arm_smccc_invoke_fn)(unsigned long, struct arm_smccc_res *);

And materials to set the invocation hanlder:

/* Simple wrapper functions to be able to use a function pointer */
static void _smccc_smc(unsigned long func_id, struct arm_smccc_res *res)
{
	arm_smccc_smc(func_id, 0, 0, 0, 0, 0, 0, 0, res);
}

static void _smccc_hvc(unsigned long func_id, struct arm_smccc_res *res)
{
        arm_smccc_hvc(func_id, 0, 0, 0, 0, 0, 0, 0, res);
}

static void _smccc_1_1(unsigned long func_id, struct arm_smccc_res *res)
{
	arm_smccc_1_1_invoke(func_id, 0, 0, 0, 0, 0, 0, 0, res);
}

static scmi_arm_smccc_invoke_fn *get_invoke_function(struct device *dev)
{
        const char *method;

        if (device_property_read_string(dev, "method", &method))
		return _smccc_1_1;

        if (!strcmp("hvc", method))
                return _smccc_hvc;

        if (!strcmp("smc", method))
                return _smccc_smc;

        dev_err(dev, "Invalid \"method\" property: %s\n", method);
        return ERR_PTR(-EINVAL);
}
 
> +
> +static DEFINE_MUTEX(smc_mutex);
> +
> +static bool smc_chan_available(struct device *dev, int idx)
> +{
> +	return true;
> +}
> +
> +static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> +			  bool tx)
> +{
> +	struct device *cdev = cinfo->dev;
> +	struct scmi_smc *scmi_info;
> +	resource_size_t size;
> +	struct resource res;
> +	struct device_node *np;
> +	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, "arm,smc-id", &func_id);
> +	if (ret < 0)
> +		return ret;

Here to get the handler for the invocation method:

	scmi_info->invoke_fn = get_invoke_function(dev);
	if (IS_ERR(scmi_info->invoke_fn))
		return PTR_ERR(scmi_info->invoke_fn);
 
> +
> +	scmi_info->func_id = func_id;
> +	scmi_info->cinfo = cinfo;
> +	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;
> +	struct arm_smccc_res res;
> +
> +	mutex_lock(&smc_mutex);
> +
> +	shmem_tx_prepare(scmi_info->shmem, xfer);
> +
> +	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);

Last, here would rahter call the registered handler instead:

	scmi_info->invoke_fn(scmi_info->func_id, &res);


> +	scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
> +
> +	mutex_unlock(&smc_mutex);
> +
> +	return res.a0;


The SCMI server is likely not to return a errno compliant value.

SMCCC specification states that unsupported function IDs should return signed
extended -1. I suggest to change the return above with:

	return res.a0 == ~0 ? -EINVAL : 0;


Regards,
Etienne

> +}
> +
> +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
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V5 2/2] firmware: arm_scmi: add smc/hvc transport

[Sudeep Holla]

On Wed, Apr 15, 2020 at 12:58:58PM +0200, Etienne Carriere wrote:
> Hello Peng,
>
> I  have 2 comments on this change. The main is about using
> arm_smccc_1_1_invoke(). Below some details and I added comments
> inside you patch. The second of on SMC return value, see my
> comment in your patch below.
>
> About arm_smccc_1_1_invoke(), this functon currently relies on PSCI
> driver to define a conduit method but SCMI agent driver does not
> mandate CONFIG_PSCI to be enable.
>

Yes this was discussed and it is done so deliberately. I have added the
build dependency when I merged the patch. There's no dependency on
CONFIG_PSCI.

> Could you add an optional "method" property for "arm,scmi-smc" for platforms
> willing to not rely on PSCI Linux driver? If no property "method" is
> defined in the FDT, invocation relies on arm_smccc_1_1_invoke().
>

Nope, we don't want mixture here. Why is the system not using PSCI/SMCCC ?

> "method" naming mimics what is done in the OP-TEE driver (drivers/tee/optee/).
> Here is a proposal for the documenting property "method" in
> Documentation/arm,scmi.txt:
>
> - method : "smc" or "hvc"
>             Optional property defining the conduit method for to be used
> 	    for invoking the SCMI server in secure world.
> 	    "smc" states instruction SMC #0 is used whereas "hvc" states
> 	    instruction HVC #0 is used.
>
>

It was rejected, you can try your luck with OPTEE :)
We will just use the system conduit here with SCMI for SMC/HVC transport.
Details in previous version of the patch.

[...]

> > +struct scmi_smc {
> > +	struct scmi_chan_info *cinfo;
> > +	struct scmi_shared_mem __iomem *shmem;
> > +	u32 func_id;
> > +};
>
> Add here a field for the secure world invocation function handler:
>
> 	scmi_arm_smccc_invoke_fn *invoke_fn;
>

As stated not needed if we use  arm_smccc_1_1_invoke()

[...]

>
> The SCMI server is likely not to return a errno compliant value.
>
> SMCCC specification states that unsupported function IDs should return signed
> extended -1. I suggest to change the return above with:
>
> 	return res.a0 == ~0 ? -EINVAL : 0;
>

I need to check that.

--
Regards,
Sudeep

Re: [PATCH V5 2/2] firmware: arm_scmi: add smc/hvc transport

[Etienne Carriere]

Hello Sudeep,

On Wed, 15 Apr 2020 at 15:16, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Apr 15, 2020 at 12:58:58PM +0200, Etienne Carriere wrote:
> > Hello Peng,
> >
> > I  have 2 comments on this change. The main is about using
> > arm_smccc_1_1_invoke(). Below some details and I added comments
> > inside you patch. The second of on SMC return value, see my
> > comment in your patch below.
> >
> > About arm_smccc_1_1_invoke(), this functon currently relies on PSCI
> > driver to define a conduit method but SCMI agent driver does not
> > mandate CONFIG_PSCI to be enable.
> >
>
> Yes this was discussed and it is done so deliberately. I have added the
> build dependency when I merged the patch. There's no dependency on
> CONFIG_PSCI.

Ok, I understand your point.
Yet it seems to me there is still a dependency on CONFIG_ARM_PSCI_FW
and also a dependency on a PSCI node in the DT.

However, I must admit I don't know yet a platform that enables
CONFIG_ARM_SCMI_PROTOCOL but not CONFIG_ARM_PSCI_FW, hence
so far, so good.


About dependencies, it think the dependency on MAILBOX in
firmware/Kconfig should be updated:

 config ARM_SCMI_PROTOCOL
         bool "ARM System Control and Management Interface (SCMI)
Message Protocol"
         depends on ARM || ARM64 || COMPILE_TEST
-        depends on MAILBOX
+        depends on MAILBOX | HAVE_ARM_SMCCC
         help

>
> > Could you add an optional "method" property for "arm,scmi-smc" for platforms
> > willing to not rely on PSCI Linux driver? If no property "method" is
> > defined in the FDT, invocation relies on arm_smccc_1_1_invoke().
> >
>
> Nope, we don't want mixture here. Why is the system not using PSCI/SMCCC ?

Ok, as I staed above, I don't know any platform that enables
CONFIG_ARM_SCMI_PROTOCOL but not CONFIG_ARM_PSCI_FW.

>
> > "method" naming mimics what is done in the OP-TEE driver (drivers/tee/optee/).
> > Here is a proposal for the documenting property "method" in
> > Documentation/arm,scmi.txt:
> >
> > - method : "smc" or "hvc"
> >             Optional property defining the conduit method for to be used
> >           for invoking the SCMI server in secure world.
> >           "smc" states instruction SMC #0 is used whereas "hvc" states
> >           instruction HVC #0 is used.
> >
> >
>
> It was rejected, you can try your luck with OPTEE :)
> We will just use the system conduit here with SCMI for SMC/HVC transport.
> Details in previous version of the patch.
>
> [...]
>
> > > +struct scmi_smc {
> > > +   struct scmi_chan_info *cinfo;
> > > +   struct scmi_shared_mem __iomem *shmem;
> > > +   u32 func_id;
> > > +};
> >
> > Add here a field for the secure world invocation function handler:
> >
> >       scmi_arm_smccc_invoke_fn *invoke_fn;
> >
>
> As stated not needed if we use  arm_smccc_1_1_invoke()
>
> [...]
>

Regards,
Etienne