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

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

[peng.fan]

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

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                    | 145 +++++++++++++++++++++
 5 files changed, 150 insertions(+), 2 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/smc.c

-- 
2.16.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V4 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

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

On Tue,  3 Mar 2020 10:06:58 +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.
> 
> Also add arm,scmi-smc compatible string for smc/hvc transport
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  Documentation/devicetree/bindings/arm/arm,scmi.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH V4 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    | 145 +++++++++++++++++++++++++++++++++++++
 4 files changed, 148 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..88f91b68f297
--- /dev/null
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -0,0 +1,145 @@
+// 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
+ */
+
+struct scmi_smc {
+	struct scmi_chan_info *cinfo;
+	struct scmi_shared_mem __iomem *shmem;
+	u32 func_id;
+};
+
+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;
+
+	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));
+
+	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Tue, Mar 03, 2020 at 10:06:59AM +0800, peng.fan@nxp.com wrote:
> 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>

[...]

> +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;
> +
> +	shmem_tx_prepare(scmi_info->shmem, xfer);

How do we protect another thread/process on another CPU going and
modifying the same shmem with another request ? We may need notion
of channel with associated shmem and it is protected with some lock.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

Hi Sudeep,

> Subject: Re: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
> 
> On Tue, Mar 03, 2020 at 10:06:59AM +0800, peng.fan@nxp.com wrote:
> > 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>
> 
> [...]
> 
> > +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;
> > +
> > +	shmem_tx_prepare(scmi_info->shmem, xfer);
> 
> How do we protect another thread/process on another CPU going and
> modifying the same shmem with another request ? We may need notion of
> channel with associated shmem and it is protected with some lock.

This is valid concern. But I think if shmem is shared bwteen protocols,
the access to shmem should be protected in 
drivers/firmware/arm_scmi/driver.c: scmi_do_xfer,
because send_message and fetch_response both touches shmem

The mailbox transport also has the issue you mentioned, I think.

Thanks,
Peng.
> 
> --
> Regards,
> Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

> Subject: RE: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
> 
> Hi Sudeep,
> 
> > Subject: Re: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
> >
> > On Tue, Mar 03, 2020 at 10:06:59AM +0800, peng.fan@nxp.com wrote:
> > > 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>
> >
> > [...]
> >
> > > +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;
> > > +
> > > +	shmem_tx_prepare(scmi_info->shmem, xfer);
> >
> > How do we protect another thread/process on another CPU going and
> > modifying the same shmem with another request ? We may need notion of
> > channel with associated shmem and it is protected with some lock.
> 
> This is valid concern. But I think if shmem is shared bwteen protocols, the
> access to shmem should be protected in
> drivers/firmware/arm_scmi/driver.c: scmi_do_xfer, because send_message
> and fetch_response both touches shmem
> 
> The mailbox transport also has the issue you mentioned, I think.

Ignore my upper comments. How do think the following diff based on current patch?

If ok, I'll squash it with current patch and send out v5.

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 88f91b68f297..7d770112f339 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -29,6 +29,8 @@ struct scmi_smc {
        u32 func_id;
 };

+static DEFINE_MUTEX(smc_mutex);
+
 static bool smc_chan_available(struct device *dev, int idx)
 {
        return true;
@@ -99,11 +101,15 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
        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;
 }

Thanks,
Peng.

> 
> Thanks,
> Peng.
> >
> > --
> > Regards,
> > Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

Hi Peng,

On Wed, Mar 04, 2020 at 02:16:00PM +0000, Peng Fan wrote:
> > Subject: RE: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
> >
> > Hi Sudeep,
> >
> > > Subject: Re: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
> > >
> > > On Tue, Mar 03, 2020 at 10:06:59AM +0800, peng.fan@nxp.com wrote:
> > > > 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>
> > >
> > > [...]
> > >
> > > > +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;
> > > > +
> > > > +	shmem_tx_prepare(scmi_info->shmem, xfer);
> > >
> > > How do we protect another thread/process on another CPU going and
> > > modifying the same shmem with another request ? We may need notion of
> > > channel with associated shmem and it is protected with some lock.
> >
> > This is valid concern. But I think if shmem is shared bwteen protocols, the
> > access to shmem should be protected in
> > drivers/firmware/arm_scmi/driver.c: scmi_do_xfer, because send_message
> > and fetch_response both touches shmem
> >
> > The mailbox transport also has the issue you mentioned, I think.

No, it doesn't. I hope you realised that now based on your statement below.

>
> Ignore my upper comments. How do think the following diff based on current patch?
>
> If ok, I'll squash it with current patch and send out v5.
>
> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> index 88f91b68f297..7d770112f339 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -29,6 +29,8 @@ struct scmi_smc {
>         u32 func_id;
>  };
>
> +static DEFINE_MUTEX(smc_mutex);
> +
>  static bool smc_chan_available(struct device *dev, int idx)
>  {
>         return true;
> @@ -99,11 +101,15 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
>         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;
>  }
>

Yes, this may fix the issue. However I would like to know if we need to
support multiple channels/shared memory simultaneously. It is fair
requirement and may need some work which should be fine. I just want to
make sure we don't need anything more from DT or if we need to add more
to DT bindings, we need to ensure it won't break single channel. I will
think about that, but I would like to hear from other users of this SMC
for SCMI.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

> Subject: Re: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
> 
> Hi Peng,
> 
> On Wed, Mar 04, 2020 at 02:16:00PM +0000, Peng Fan wrote:
> > > Subject: RE: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc
> > > transport
> > >
> > > Hi Sudeep,
> > >
> > > > Subject: Re: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc
> > > > transport
> > > >
> > > > On Tue, Mar 03, 2020 at 10:06:59AM +0800, peng.fan@nxp.com wrote:
> > > > > 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>
> > > >
> > > > [...]
> > > >
> > > > > +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;
> > > > > +
> > > > > +	shmem_tx_prepare(scmi_info->shmem, xfer);
> > > >
> > > > How do we protect another thread/process on another CPU going and
> > > > modifying the same shmem with another request ? We may need notion
> > > > of channel with associated shmem and it is protected with some lock.
> > >
> > > This is valid concern. But I think if shmem is shared bwteen
> > > protocols, the access to shmem should be protected in
> > > drivers/firmware/arm_scmi/driver.c: scmi_do_xfer, because
> > > send_message and fetch_response both touches shmem
> > >
> > > The mailbox transport also has the issue you mentioned, I think.
> 
> No, it doesn't. I hope you realised that now based on your statement below.
> 
> >
> > Ignore my upper comments. How do think the following diff based on
> current patch?
> >
> > If ok, I'll squash it with current patch and send out v5.
> >
> > diff --git a/drivers/firmware/arm_scmi/smc.c
> > b/drivers/firmware/arm_scmi/smc.c index 88f91b68f297..7d770112f339
> > 100644
> > --- a/drivers/firmware/arm_scmi/smc.c
> > +++ b/drivers/firmware/arm_scmi/smc.c
> > @@ -29,6 +29,8 @@ struct scmi_smc {
> >         u32 func_id;
> >  };
> >
> > +static DEFINE_MUTEX(smc_mutex);
> > +
> >  static bool smc_chan_available(struct device *dev, int idx)  {
> >         return true;
> > @@ -99,11 +101,15 @@ static int smc_send_message(struct
> scmi_chan_info *cinfo,
> >         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;
> >  }
> >
> 
> Yes, this may fix the issue. However I would like to know if we need to support
> multiple channels/shared memory simultaneously. It is fair requirement and
> may need some work which should be fine. 

Do you have any suggestions? Currently I have not worked out an good
solution.

Thanks,
Peng.

I just want to make sure we don't
> need anything more from DT or if we need to add more to DT bindings, we
> need to ensure it won't break single channel. I will think about that, but I
> would like to hear from other users of this SMC for SCMI.
> 
> --
> Regards,
> Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Thu, Mar 05, 2020 at 11:25:35AM +0000, Peng Fan wrote:

[...]

> >
> > Yes, this may fix the issue. However I would like to know if we need to support
> > multiple channels/shared memory simultaneously. It is fair requirement and
> > may need some work which should be fine.
>
> Do you have any suggestions? Currently I have not worked out an good
> solution.
>

TBH, I haven't given it a much thought. I would like to know if people
are happy with just one SMC channel for SCMI or do they need more ?
If they need it, we can try to solve it. Otherwise, what you have will
suffice IMO.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Florian Fainelli]

On 3/5/20 8:06 AM, Sudeep Holla wrote:
> On Thu, Mar 05, 2020 at 11:25:35AM +0000, Peng Fan wrote:
> 
> [...]
> 
>>>
>>> Yes, this may fix the issue. However I would like to know if we need to support
>>> multiple channels/shared memory simultaneously. It is fair requirement and
>>> may need some work which should be fine.
>>
>> Do you have any suggestions? Currently I have not worked out an good
>> solution.
>>
> 
> TBH, I haven't given it a much thought. I would like to know if people
> are happy with just one SMC channel for SCMI or do they need more ?
> If they need it, we can try to solve it. Otherwise, what you have will
> suffice IMO.

On our platforms we have one channel/shared memory area/mailbox instance
for all standard SCMI protocols, and we have a separate channel/shared
memory area/mailbox driver instance for a proprietary one. They happen
to have difference throughput requirements, hence the split.

If I read Peng's submission correctly, it seems to me that the usage
model described before is still fine.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Peng Fan]

> Subject: Re: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
> 
> On 3/5/20 8:06 AM, Sudeep Holla wrote:
> > On Thu, Mar 05, 2020 at 11:25:35AM +0000, Peng Fan wrote:
> >
> > [...]
> >
> >>>
> >>> Yes, this may fix the issue. However I would like to know if we need
> >>> to support multiple channels/shared memory simultaneously. It is
> >>> fair requirement and may need some work which should be fine.
> >>
> >> Do you have any suggestions? Currently I have not worked out an good
> >> solution.
> >>
> >
> > TBH, I haven't given it a much thought. I would like to know if people
> > are happy with just one SMC channel for SCMI or do they need more ?
> > If they need it, we can try to solve it. Otherwise, what you have will
> > suffice IMO.
> 
> On our platforms we have one channel/shared memory area/mailbox
> instance for all standard SCMI protocols, and we have a separate
> channel/shared memory area/mailbox driver instance for a proprietary one.
> They happen to have difference throughput requirements, hence the split.
> 
> If I read Peng's submission correctly, it seems to me that the usage model
> described before is still fine.

Thanks. 

Sudeep,

Then should I repost with the global mutex added?

Thanks,
Peng.


> --
> Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Sudeep Holla]

On Fri, Mar 06, 2020 at 08:07:19AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
> >
> > On 3/5/20 8:06 AM, Sudeep Holla wrote:
> > > On Thu, Mar 05, 2020 at 11:25:35AM +0000, Peng Fan wrote:
> > >
> > > [...]
> > >
> > >>>
> > >>> Yes, this may fix the issue. However I would like to know if we need
> > >>> to support multiple channels/shared memory simultaneously. It is
> > >>> fair requirement and may need some work which should be fine.
> > >>
> > >> Do you have any suggestions? Currently I have not worked out an good
> > >> solution.
> > >>
> > >
> > > TBH, I haven't given it a much thought. I would like to know if people
> > > are happy with just one SMC channel for SCMI or do they need more ?
> > > If they need it, we can try to solve it. Otherwise, what you have will
> > > suffice IMO.
> >
> > On our platforms we have one channel/shared memory area/mailbox
> > instance for all standard SCMI protocols, and we have a separate
> > channel/shared memory area/mailbox driver instance for a proprietary one.
> > They happen to have difference throughput requirements, hence the split.
> >

OK, when you refer proprietary protocol, do you mean outside the scope of
SCMI ? The reason I ask is SCMI allows vendor specific protocols and if
you are using other channel for that, it still make sense to add
multi-channel support here.

> > If I read Peng's submission correctly, it seems to me that the usage model
> > described before is still fine.
>
> Thanks.
>
> Sudeep,
>
> Then should I repost with the global mutex added?
>

Sure, you can send the updated. I will think about adding support for more
than one channel and send a patch on top of it if I get around it.

Note that I sent PR for v5.7 last earlier this week, so this will be for v5.8

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Florian Fainelli]

On 3/6/20 6:23 AM, Sudeep Holla wrote:
> On Fri, Mar 06, 2020 at 08:07:19AM +0000, Peng Fan wrote:
>>> Subject: Re: [PATCH V4 2/2] firmware: arm_scmi: add smc/hvc transport
>>>
>>> On 3/5/20 8:06 AM, Sudeep Holla wrote:
>>>> On Thu, Mar 05, 2020 at 11:25:35AM +0000, Peng Fan wrote:
>>>>
>>>> [...]
>>>>
>>>>>>
>>>>>> Yes, this may fix the issue. However I would like to know if we need
>>>>>> to support multiple channels/shared memory simultaneously. It is
>>>>>> fair requirement and may need some work which should be fine.
>>>>>
>>>>> Do you have any suggestions? Currently I have not worked out an good
>>>>> solution.
>>>>>
>>>>
>>>> TBH, I haven't given it a much thought. I would like to know if people
>>>> are happy with just one SMC channel for SCMI or do they need more ?
>>>> If they need it, we can try to solve it. Otherwise, what you have will
>>>> suffice IMO.
>>>
>>> On our platforms we have one channel/shared memory area/mailbox
>>> instance for all standard SCMI protocols, and we have a separate
>>> channel/shared memory area/mailbox driver instance for a proprietary one.
>>> They happen to have difference throughput requirements, hence the split.
>>>
> 
> OK, when you refer proprietary protocol, do you mean outside the scope of
> SCMI ? The reason I ask is SCMI allows vendor specific protocols and if
> you are using other channel for that, it still make sense to add
> multi-channel support here.

Sorry this was not clear, I meant a protocol ID which is in the 0x80 -
0xFF range. We are using one pair of channels (rx and tx) plus shared
memory area for the standard SCMI protocol numbers, and we have another
pair of rx/tx channels and shared memory area for this vendor specific
protocol.

Maybe providing the Device Tree entries would be clearer, so this is
what it looks like (this is the output from the bootloader generated
Device Tree):


/       brcm_scmi_mailbox@0 {
                #mbox-cells = <0x1>;
                compatible = "brcm,brcmstb-mbox";
                status = "disabled";
                linux,phandle = <0xe>;
                phandle = <0xe>;
        };

        brcm_scmi@0 {
                compatible = "arm,scmi";
                mboxes = <0xe 0x0 0xe 0x1>;
                mbox-names = "tx", "rx";
                shmem = <0xf>;
                status = "disabled";
                #address-cells = <0x1>;
                #size-cells = <0x0>;

                protocol@13 {
                        reg = <0x13>;
                };

                protocol@14 {
                        reg = <0x14>;
                        #clock-cells = <0x1>;
                        linux,phandle = <0x3>;
                        phandle = <0x3>;
                };

                protocol@15 {
                        reg = <0x15>;
                        #sensor-cells = <0x1>;
                        #thermal-sensor-cells = <0x1>;
                        linux,phandle = <0x12>;
                        phandle = <0x12>;
                };

                protocol@80 {
                        reg = <0x80>;
                };
        };

        brcm_scmi_mailbox@1 {
                #mbox-cells = <0x1>;
                compatible = "brcm,brcmstb-mbox";
                status = "disabled";
                linux,phandle = <0x10>;
                phandle = <0x10>;
        };

        brcm_scmi@1 {
                compatible = "arm,scmi";
                mboxes = <0x10 0x0 0x10 0x1>;
                mbox-names = "tx", "rx";
                shmem = <0x11>;
                status = "disabled";
                #address-cells = <0x1>;
                #size-cells = <0x0>;

                protocol@82 {
                        reg = <0x82>;
                };
        };


> 
>>> If I read Peng's submission correctly, it seems to me that the usage model
>>> described before is still fine.
>>
>> Thanks.
>>
>> Sudeep,
>>
>> Then should I repost with the global mutex added?
>>
> 
> Sure, you can send the updated. I will think about adding support for more
> than one channel and send a patch on top of it if I get around it.
> 
> Note that I sent PR for v5.7 last earlier this week, so this will be for v5.8
> 
> --
> Regards,
> Sudeep
> 


-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel