[PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Viresh Kumar]

The SCMI specification is fairly independent of the transport protocol,
which can be a simple mailbox (already implemented) or anything else.
The current Linux implementation however is very much dependent of the
mailbox transport layer.

This patch makes the SCMI core code (driver.c) independent of the
mailbox transport layer and moves all mailbox related code to a new
file: mailbox.c.

We can now implement more transport protocols to transport SCMI
messages, some of the transport protocols getting discussed currently
are SMC/HVC, SPCI (built on top of SMC/HVC), OPTEE based mailbox
(similar to SPCI), and vitio based transport as alternative to mailbox.

The transport protocols just need to provide struct scmi_desc, which
also implements the struct scmi_transport_ops.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2:
- Dropped __iomem from payload data.
- Moved transport ops to scmi_desc, and that has a per transport
  instance now which is differentiated using the compatible string.
- Converted IS_ERR_OR_NULL to IS_ERR.

 drivers/firmware/arm_scmi/Makefile  |   3 +-
 drivers/firmware/arm_scmi/common.h  |  55 ++++++++++
 drivers/firmware/arm_scmi/driver.c  | 151 ++++++----------------------
 drivers/firmware/arm_scmi/mailbox.c | 144 ++++++++++++++++++++++++++
 4 files changed, 233 insertions(+), 120 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/mailbox.c

diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index 5f298f00a82e..df2c05a545d8 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-y	= scmi-bus.o scmi-driver.o scmi-protocols.o
+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
 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 5237c2ff79fe..365368f8e6d1 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -111,3 +111,58 @@ void scmi_setup_protocol_implemented(const struct scmi_handle *handle,
 				     u8 *prot_imp);
 
 int scmi_base_protocol_init(struct scmi_handle *h);
+
+/* SCMI Transport */
+
+/**
+ * struct scmi_chan_info - Structure representing a SCMI channel information
+ *
+ * @payload: Transmit/Receive payload area
+ * @dev: Reference to device in the SCMI hierarchy corresponding to this
+ *	 channel
+ * @handle: Pointer to SCMI entity handle
+ * @transport_info: Transport layer related information
+ */
+struct scmi_chan_info {
+	void *payload;
+	struct device *dev;
+	struct scmi_handle *handle;
+	void *transport_info;
+};
+
+/**
+ * struct scmi_transport_ops - Structure representing a SCMI transport ops
+ *
+ * @send_message: Callback to send a message
+ * @mark_txdone: Callback to mark tx as done
+ * @chan_setup: Callback to allocate and setup a channel
+ * @chan_free: Callback to free a channel
+ */
+struct scmi_transport_ops {
+	bool (*chan_available)(struct device *dev, int idx);
+	int (*chan_setup)(struct scmi_chan_info *cinfo, 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);
+};
+
+/**
+ * struct scmi_desc - Description of SoC integration
+ *
+ * @max_rx_timeout_ms: Timeout for communication with SoC (in Milliseconds)
+ * @max_msg: Maximum number of messages that can be pending
+ *	simultaneously in the system
+ * @max_msg_size: Maximum size of data per message that can be handled.
+ */
+struct scmi_desc {
+	struct scmi_transport_ops *ops;
+	int max_rx_timeout_ms;
+	int max_msg;
+	int max_msg_size;
+};
+
+extern const struct scmi_desc scmi_mailbox_desc;
+
+void scmi_tx_prepare(struct scmi_chan_info *cinfo, struct scmi_xfer *t);
+void scmi_rx_callback(struct scmi_chan_info *cinfo, struct scmi_xfer *t);
+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 3eb0382491ce..e67fcbe27472 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -19,7 +19,6 @@
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/ktime.h>
-#include <linux/mailbox_client.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
@@ -77,38 +76,6 @@ struct scmi_xfers_info {
 	spinlock_t xfer_lock;
 };
 
-/**
- * struct scmi_desc - Description of SoC integration
- *
- * @max_rx_timeout_ms: Timeout for communication with SoC (in Milliseconds)
- * @max_msg: Maximum number of messages that can be pending
- *	simultaneously in the system
- * @max_msg_size: Maximum size of data per message that can be handled.
- */
-struct scmi_desc {
-	int max_rx_timeout_ms;
-	int max_msg;
-	int max_msg_size;
-};
-
-/**
- * struct scmi_chan_info - Structure representing a SCMI channel information
- *
- * @cl: Mailbox Client
- * @chan: Transmit/Receive mailbox channel
- * @payload: Transmit/Receive mailbox channel payload area
- * @dev: Reference to device in the SCMI hierarchy corresponding to this
- *	 channel
- * @handle: Pointer to SCMI entity handle
- */
-struct scmi_chan_info {
-	struct mbox_client cl;
-	struct mbox_chan *chan;
-	void __iomem *payload;
-	struct device *dev;
-	struct scmi_handle *handle;
-};
-
 /**
  * struct scmi_info - Structure representing a SCMI instance
  *
@@ -138,7 +105,6 @@ struct scmi_info {
 	int users;
 };
 
-#define client_to_scmi_chan_info(c) container_of(c, struct scmi_chan_info, cl)
 #define handle_to_scmi_info(h)	container_of(h, struct scmi_info, handle)
 
 /*
@@ -195,7 +161,7 @@ static inline void scmi_dump_header_dbg(struct device *dev,
 }
 
 static void scmi_fetch_response(struct scmi_xfer *xfer,
-				struct scmi_shared_mem __iomem *mem)
+				struct scmi_shared_mem *mem)
 {
 	xfer->hdr.status = ioread32(mem->msg_payload);
 	/* Skip the length of header and status in payload area i.e 8 bytes */
@@ -233,19 +199,17 @@ static inline void unpack_scmi_header(u32 msg_hdr, struct scmi_msg_hdr *hdr)
 }
 
 /**
- * scmi_tx_prepare() - mailbox client callback to prepare for the transfer
+ * scmi_tx_prepare() - callback to prepare for the transfer
  *
- * @cl: client pointer
- * @m: mailbox message
+ * @cinfo: SCMI channel info
+ * @t: transfer message
  *
  * This function prepares the shared memory which contains the header and the
  * payload.
  */
-static void scmi_tx_prepare(struct mbox_client *cl, void *m)
+void scmi_tx_prepare(struct scmi_chan_info *cinfo, struct scmi_xfer *t)
 {
-	struct scmi_xfer *t = m;
-	struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
-	struct scmi_shared_mem __iomem *mem = cinfo->payload;
+	struct scmi_shared_mem *mem = cinfo->payload;
 
 	/*
 	 * Ideally channel must be free by now unless OS timeout last
@@ -332,10 +296,10 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo, struct scmi_xfer *xfer)
 }
 
 /**
- * scmi_rx_callback() - mailbox client callback for receive messages
+ * scmi_rx_callback() - callback for receive messages
  *
- * @cl: client pointer
- * @m: mailbox message
+ * @cinfo: SCMI channel info
+ * @t: transfer message
  *
  * Processes one received message to appropriate transfer information and
  * signals completion of the transfer.
@@ -343,17 +307,16 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo, struct scmi_xfer *xfer)
  * NOTE: This function will be invoked in IRQ context, hence should be
  * as optimal as possible.
  */
-static void scmi_rx_callback(struct mbox_client *cl, void *m)
+void scmi_rx_callback(struct scmi_chan_info *cinfo, struct scmi_xfer *t)
 {
 	u8 msg_type;
 	u32 msg_hdr;
 	u16 xfer_id;
 	struct scmi_xfer *xfer;
-	struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
 	struct device *dev = cinfo->dev;
 	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
 	struct scmi_xfers_info *minfo = &info->tx_minfo;
-	struct scmi_shared_mem __iomem *mem = cinfo->payload;
+	struct scmi_shared_mem *mem = cinfo->payload;
 
 	msg_hdr = ioread32(&mem->msg_header);
 	msg_type = MSG_XTRACT_TYPE(msg_hdr);
@@ -396,7 +359,7 @@ void scmi_xfer_put(const struct scmi_handle *handle, struct scmi_xfer *xfer)
 static bool
 scmi_xfer_poll_done(const struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
 {
-	struct scmi_shared_mem __iomem *mem = cinfo->payload;
+	struct scmi_shared_mem *mem = cinfo->payload;
 	u16 xfer_id = MSG_XTRACT_TOKEN(ioread32(&mem->msg_header));
 
 	if (xfer->hdr.seq != xfer_id)
@@ -439,15 +402,12 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
 	if (unlikely(!cinfo))
 		return -EINVAL;
 
-	ret = mbox_send_message(cinfo->chan, xfer);
+	ret = info->desc->ops->send_message(cinfo, xfer);
 	if (ret < 0) {
-		dev_dbg(dev, "mbox send fail %d\n", ret);
+		dev_dbg(dev, "Failed to send message %d\n", ret);
 		return ret;
 	}
 
-	/* mbox_send_message returns non-negative value on success, so reset */
-	ret = 0;
-
 	if (xfer->hdr.poll_completion) {
 		ktime_t stop = ktime_add_ns(ktime_get(), SCMI_MAX_POLL_TO_NS);
 
@@ -461,7 +421,7 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
 		/* And we wait for the response. */
 		timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
 		if (!wait_for_completion_timeout(&xfer->done, timeout)) {
-			dev_err(dev, "mbox timed out in resp(caller: %pS)\n",
+			dev_err(dev, "timed out in resp(caller: %pS)\n",
 				(void *)_RET_IP_);
 			ret = -ETIMEDOUT;
 		}
@@ -470,13 +430,7 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
 	if (!ret && xfer->hdr.status)
 		ret = scmi_to_linux_errno(xfer->hdr.status);
 
-	/*
-	 * NOTE: we might prefer not to need the mailbox ticker to manage the
-	 * transfer queueing since the protocol layer queues things by itself.
-	 * Unfortunately, we have to kick the mailbox framework after we have
-	 * received our message.
-	 */
-	mbox_client_txdone(cinfo->chan, ret);
+	info->desc->ops->mark_txdone(cinfo, ret);
 
 	return ret;
 }
@@ -713,21 +667,14 @@ static int scmi_xfer_info_init(struct scmi_info *sinfo)
 	return 0;
 }
 
-static int scmi_mailbox_check(struct device_node *np, int idx)
-{
-	return of_parse_phandle_with_args(np, "mboxes", "#mbox-cells",
-					  idx, NULL);
-}
-
-static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
-				int prot_id, bool tx)
+static int scmi_chan_setup(struct scmi_info *info, struct device *dev,
+			   int prot_id, bool tx)
 {
 	int ret, idx;
 	struct resource res;
 	resource_size_t size;
-	struct device_node *shmem, *np = dev->of_node;
+	struct device_node *shmem;
 	struct scmi_chan_info *cinfo;
-	struct mbox_client *cl;
 	struct idr *idr;
 	const char *desc = tx ? "Tx" : "Rx";
 
@@ -735,7 +682,7 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
 	idx = tx ? 0 : 1;
 	idr = tx ? &info->tx_idr : &info->rx_idr;
 
-	if (scmi_mailbox_check(np, idx)) {
+	if (!info->desc->ops->chan_available(dev, idx)) {
 		cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
 		if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
 			return -EINVAL;
@@ -748,14 +695,7 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
 
 	cinfo->dev = dev;
 
-	cl = &cinfo->cl;
-	cl->dev = dev;
-	cl->rx_callback = scmi_rx_callback;
-	cl->tx_prepare = tx ? scmi_tx_prepare : NULL;
-	cl->tx_block = false;
-	cl->knows_txdone = tx;
-
-	shmem = of_parse_phandle(np, "shmem", idx);
+	shmem = of_parse_phandle(dev->of_node, "shmem", idx);
 	ret = of_address_to_resource(shmem, 0, &res);
 	of_node_put(shmem);
 	if (ret) {
@@ -770,14 +710,9 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
 		return -EADDRNOTAVAIL;
 	}
 
-	cinfo->chan = mbox_request_channel(cl, idx);
-	if (IS_ERR(cinfo->chan)) {
-		ret = PTR_ERR(cinfo->chan);
-		if (ret != -EPROBE_DEFER)
-			dev_err(dev, "failed to request SCMI %s mailbox\n",
-				desc);
+	ret = info->desc->ops->chan_setup(cinfo, tx);
+	if (ret)
 		return ret;
-	}
 
 idr_alloc:
 	ret = idr_alloc(idr, cinfo, prot_id, prot_id + 1, GFP_KERNEL);
@@ -791,12 +726,12 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
 }
 
 static inline int
-scmi_mbox_txrx_setup(struct scmi_info *info, struct device *dev, int prot_id)
+scmi_txrx_setup(struct scmi_info *info, struct device *dev, int prot_id)
 {
-	int ret = scmi_mbox_chan_setup(info, dev, prot_id, true);
+	int ret = scmi_chan_setup(info, dev, prot_id, true);
 
 	if (!ret) /* Rx is optional, hence no error check */
-		scmi_mbox_chan_setup(info, dev, prot_id, false);
+		scmi_chan_setup(info, dev, prot_id, false);
 
 	return ret;
 }
@@ -814,7 +749,7 @@ scmi_create_protocol_device(struct device_node *np, struct scmi_info *info,
 		return;
 	}
 
-	if (scmi_mbox_txrx_setup(info, &sdev->dev, prot_id)) {
+	if (scmi_txrx_setup(info, &sdev->dev, prot_id)) {
 		dev_err(&sdev->dev, "failed to setup transport\n");
 		scmi_device_destroy(sdev);
 		return;
@@ -833,12 +768,6 @@ static int scmi_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *child, *np = dev->of_node;
 
-	/* Only mailbox method supported, check for the presence of one */
-	if (scmi_mailbox_check(np, 0)) {
-		dev_err(dev, "no mailbox found in %pOF\n", np);
-		return -EINVAL;
-	}
-
 	desc = of_device_get_match_data(dev);
 	if (!desc)
 		return -EINVAL;
@@ -863,7 +792,7 @@ static int scmi_probe(struct platform_device *pdev)
 	handle->dev = info->dev;
 	handle->version = &info->version;
 
-	ret = scmi_mbox_txrx_setup(info, dev, SCMI_PROTOCOL_BASE);
+	ret = scmi_txrx_setup(info, dev, SCMI_PROTOCOL_BASE);
 	if (ret)
 		return ret;
 
@@ -898,19 +827,9 @@ static int scmi_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int scmi_mbox_free_channel(int id, void *p, void *data)
+void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr, int id)
 {
-	struct scmi_chan_info *cinfo = p;
-	struct idr *idr = data;
-
-	if (!IS_ERR_OR_NULL(cinfo->chan)) {
-		mbox_free_channel(cinfo->chan);
-		cinfo->chan = NULL;
-	}
-
 	idr_remove(idr, id);
-
-	return 0;
 }
 
 static int scmi_remove(struct platform_device *pdev)
@@ -930,25 +849,19 @@ static int scmi_remove(struct platform_device *pdev)
 		return ret;
 
 	/* Safe to free channels since no more users */
-	ret = idr_for_each(idr, scmi_mbox_free_channel, idr);
+	ret = idr_for_each(idr, info->desc->ops->chan_free, idr);
 	idr_destroy(&info->tx_idr);
 
 	idr = &info->rx_idr;
-	ret = idr_for_each(idr, scmi_mbox_free_channel, idr);
+	ret = idr_for_each(idr, info->desc->ops->chan_free, idr);
 	idr_destroy(&info->rx_idr);
 
 	return ret;
 }
 
-static const struct scmi_desc scmi_generic_desc = {
-	.max_rx_timeout_ms = 30,	/* We may increase this if required */
-	.max_msg = 20,		/* Limited by MBOX_TX_QUEUE_LEN */
-	.max_msg_size = 128,
-};
-
 /* Each compatible listed below must have descriptor associated with it */
 static const struct of_device_id scmi_of_match[] = {
-	{ .compatible = "arm,scmi", .data = &scmi_generic_desc },
+	{ .compatible = "arm,scmi", .data = &scmi_mailbox_desc },
 	{ /* Sentinel */ },
 };
 
diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
new file mode 100644
index 000000000000..2d1f7c8be293
--- /dev/null
+++ b/drivers/firmware/arm_scmi/mailbox.c
@@ -0,0 +1,144 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Message Mailbox Transport driver
+ *
+ * Copyright (C) 2019 ARM Ltd.
+ */
+
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/mailbox_client.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+
+#include "common.h"
+
+/**
+ * struct scmi_mailbox - Structure representing a SCMI mailbox transport
+ *
+ * @cl: Mailbox Client
+ * @chan: Transmit/Receive mailbox channel
+ * @cinfo: SCMI channel info
+ */
+struct scmi_mailbox {
+	struct mbox_client cl;
+	struct mbox_chan *chan;
+	struct scmi_chan_info *cinfo;
+};
+
+#define client_to_scmi_mailbox(c) container_of(c, struct scmi_mailbox, cl)
+
+static bool mailbox_chan_available(struct device *dev, int idx)
+{
+	return !of_parse_phandle_with_args(dev->of_node, "mboxes",
+					   "#mbox-cells", idx, NULL);
+}
+
+static void mailbox_tx_prepare(struct mbox_client *cl, void *m)
+{
+	struct scmi_mailbox *smbox = client_to_scmi_mailbox(cl);
+	struct scmi_chan_info *cinfo = smbox->cinfo;
+
+	scmi_tx_prepare(cinfo, m);
+}
+
+static void mailbox_rx_callback(struct mbox_client *cl, void *m)
+{
+	struct scmi_mailbox *smbox = client_to_scmi_mailbox(cl);
+	struct scmi_chan_info *cinfo = smbox->cinfo;
+
+	scmi_rx_callback(cinfo, m);
+}
+
+static int mailbox_chan_setup(struct scmi_chan_info *cinfo, bool tx)
+{
+	struct device *dev = cinfo->dev;
+	struct scmi_mailbox *smbox;
+	struct mbox_client *cl;
+	int ret;
+
+	smbox = devm_kzalloc(dev, sizeof(*smbox), GFP_KERNEL);
+	if (!smbox)
+		return -ENOMEM;
+
+	cl = &smbox->cl;
+	cl->dev = dev;
+	cl->tx_prepare = tx ? mailbox_tx_prepare : NULL;
+	cl->rx_callback = mailbox_rx_callback;
+	cl->tx_block = false;
+	cl->knows_txdone = tx;
+
+	smbox->chan = mbox_request_channel(cl, tx ? 0 : 1);
+	if (IS_ERR(smbox->chan)) {
+		ret = PTR_ERR(smbox->chan);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to request SCMI %s mailbox\n",
+				tx ? "Tx" : "Rx");
+		return ret;
+	}
+
+	cinfo->transport_info = smbox;
+	smbox->cinfo = cinfo;
+
+	return 0;
+}
+
+static int mailbox_chan_free(int id, void *p, void *data)
+{
+	struct scmi_chan_info *cinfo = p;
+	struct scmi_mailbox *smbox = cinfo->transport_info;
+
+	if (!IS_ERR(smbox->chan)) {
+		mbox_free_channel(smbox->chan);
+		cinfo->transport_info = NULL;
+		smbox->chan = NULL;
+		smbox->cinfo = NULL;
+	}
+
+	scmi_free_channel(cinfo, data, id);
+
+	return 0;
+}
+
+static int mailbox_send_message(struct scmi_chan_info *cinfo,
+			struct scmi_xfer *xfer)
+{
+	struct scmi_mailbox *smbox = cinfo->transport_info;
+	int ret;
+
+	ret = mbox_send_message(smbox->chan, xfer);
+
+	/* mbox_send_message returns non-negative value on success, so reset */
+	if (ret > 0)
+		ret = 0;
+
+	return ret;
+}
+
+static void mailbox_mark_txdone(struct scmi_chan_info *cinfo, int ret)
+{
+	struct scmi_mailbox *smbox = cinfo->transport_info;
+
+	/*
+	 * NOTE: we might prefer not to need the mailbox ticker to manage the
+	 * transfer queueing since the protocol layer queues things by itself.
+	 * Unfortunately, we have to kick the mailbox framework after we have
+	 * received our message.
+	 */
+	mbox_client_txdone(smbox->chan, ret);
+}
+
+static struct scmi_transport_ops scmi_mailbox_ops = {
+	.chan_available = mailbox_chan_available,
+	.chan_setup = mailbox_chan_setup,
+	.chan_free = mailbox_chan_free,
+	.send_message = mailbox_send_message,
+	.mark_txdone = mailbox_mark_txdone,
+};
+
+const struct scmi_desc scmi_mailbox_desc = {
+	.ops = &scmi_mailbox_ops,
+	.max_rx_timeout_ms = 30, /* We may increase this if required */
+	.max_msg = 20, /* Limited by MBOX_TX_QUEUE_LEN */
+	.max_msg_size = 128,
+};
-- 
2.21.0.rc0.269.g1a574e7a288b


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

Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Arnd Bergmann]

On Fri, Jan 10, 2020 at 10:43 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> The SCMI specification is fairly independent of the transport protocol,
> which can be a simple mailbox (already implemented) or anything else.
> The current Linux implementation however is very much dependent of the
> mailbox transport layer.
>
> This patch makes the SCMI core code (driver.c) independent of the
> mailbox transport layer and moves all mailbox related code to a new
> file: mailbox.c.
>
> We can now implement more transport protocols to transport SCMI
> messages, some of the transport protocols getting discussed currently
> are SMC/HVC, SPCI (built on top of SMC/HVC), OPTEE based mailbox
> (similar to SPCI), and vitio based transport as alternative to mailbox.
>
> The transport protocols just need to provide struct scmi_desc, which
> also implements the struct scmi_transport_ops.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2:
> - Dropped __iomem from payload data.

Simply dropping the __iomem isn't much better, now you get other
type mismatches.

> - Moved transport ops to scmi_desc, and that has a per transport
>   instance now which is differentiated using the compatible string.
> - Converted IS_ERR_OR_NULL to IS_ERR.

These look good to me.

> + * @payload: Transmit/Receive payload area
> + * @dev: Reference to device in the SCMI hierarchy corresponding to this
> + *      channel
> + * @handle: Pointer to SCMI entity handle
> + * @transport_info: Transport layer related information
> + */
> +struct scmi_chan_info {
> +       void *payload;
> +       struct device *dev;
> +       struct scmi_handle *handle;
> +       void *transport_info;
> +};

Maybe you can wrap the scmi_chan_info inside of another
structure that contains  the payload pointer, and use container_of
to convert between them?

It's not obvious which parts of the structure should be shared and
which are transport specific.

> -static void scmi_rx_callback(struct mbox_client *cl, void *m)
> +void scmi_rx_callback(struct scmi_chan_info *cinfo, struct scmi_xfer *t)
>  {
>         u8 msg_type;
>         u32 msg_hdr;
>         u16 xfer_id;
>         struct scmi_xfer *xfer;
> -       struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
>         struct device *dev = cinfo->dev;
>         struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
>         struct scmi_xfers_info *minfo = &info->tx_minfo;
> -       struct scmi_shared_mem __iomem *mem = cinfo->payload;
> +       struct scmi_shared_mem *mem = cinfo->payload;
>
>         msg_hdr = ioread32(&mem->msg_header);

This is where it goes wrong: you cannot pass a kernel pointer
without __iomem into ioread32(). Building the driver with sparse
(using "make C=1") should show you this and possibly other
related conversion bugs.

       Arnd

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

Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Viresh Kumar]

On 10-01-20, 12:15, Arnd Bergmann wrote:
> On Fri, Jan 10, 2020 at 10:43 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > The SCMI specification is fairly independent of the transport protocol,
> > which can be a simple mailbox (already implemented) or anything else.
> > The current Linux implementation however is very much dependent of the
> > mailbox transport layer.
> >
> > This patch makes the SCMI core code (driver.c) independent of the
> > mailbox transport layer and moves all mailbox related code to a new
> > file: mailbox.c.
> >
> > We can now implement more transport protocols to transport SCMI
> > messages, some of the transport protocols getting discussed currently
> > are SMC/HVC, SPCI (built on top of SMC/HVC), OPTEE based mailbox
> > (similar to SPCI), and vitio based transport as alternative to mailbox.
> >
> > The transport protocols just need to provide struct scmi_desc, which
> > also implements the struct scmi_transport_ops.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > V2:
> > - Dropped __iomem from payload data.
> 
> Simply dropping the __iomem isn't much better, now you get other
> type mismatches.

Right. So what exactly do you suggest I should do now? Drop __iomem
from the structure's payload field but keep all local variables and
function arguments with __iomem ?

> > - Moved transport ops to scmi_desc, and that has a per transport
> >   instance now which is differentiated using the compatible string.
> > - Converted IS_ERR_OR_NULL to IS_ERR.
> 
> These look good to me.
> 
> > + * @payload: Transmit/Receive payload area
> > + * @dev: Reference to device in the SCMI hierarchy corresponding to this
> > + *      channel
> > + * @handle: Pointer to SCMI entity handle
> > + * @transport_info: Transport layer related information
> > + */
> > +struct scmi_chan_info {
> > +       void *payload;
> > +       struct device *dev;
> > +       struct scmi_handle *handle;
> > +       void *transport_info;
> > +};
> 
> Maybe you can wrap the scmi_chan_info inside of another
> structure that contains  the payload pointer, and use container_of
> to convert between them?

We don't need to convert between the two of them, isn't it ? Are you
referring some other field here ?

> It's not obvious which parts of the structure should be shared and
> which are transport specific.

All transport specific information is kept in the transport specific
structure which is saved here in the transport_info field. Is there
something else that isn't clear ?

-- 
viresh

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

Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Arnd Bergmann]

On Mon, Jan 13, 2020 at 7:42 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 10-01-20, 12:15, Arnd Bergmann wrote:
> > On Fri, Jan 10, 2020 at 10:43 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > Simply dropping the __iomem isn't much better, now you get other
> > type mismatches.
>
> Right. So what exactly do you suggest I should do now? Drop __iomem
> from the structure's payload field but keep all local variables and
> function arguments with __iomem ?

> > > +struct scmi_chan_info {
> > > +       void *payload;
> > > +       struct device *dev;
> > > +       struct scmi_handle *handle;
> > > +       void *transport_info;
> > > +};
> >
> > Maybe you can wrap the scmi_chan_info inside of another
> > structure that contains  the payload pointer, and use container_of
> > to convert between them?
>
> We don't need to convert between the two of them, isn't it ? Are you
> referring some other field here ?

> > It's not obvious which parts of the structure should be shared and
> > which are transport specific.
>
> All transport specific information is kept in the transport specific
> structure which is saved here in the transport_info field. Is there
> something else that isn't clear ?

To answer all three, what I meant is that the payload pointer appears
to be transport specific and should not be part of the common
structure if there is generic way to access it.

      Arnd

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

Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Viresh Kumar]

On 13-01-20, 12:36, Arnd Bergmann wrote:
> On Mon, Jan 13, 2020 at 7:42 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 10-01-20, 12:15, Arnd Bergmann wrote:
> > > On Fri, Jan 10, 2020 at 10:43 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > Simply dropping the __iomem isn't much better, now you get other
> > > type mismatches.
> >
> > Right. So what exactly do you suggest I should do now? Drop __iomem
> > from the structure's payload field but keep all local variables and
> > function arguments with __iomem ?
> 
> > > > +struct scmi_chan_info {
> > > > +       void *payload;
> > > > +       struct device *dev;
> > > > +       struct scmi_handle *handle;
> > > > +       void *transport_info;
> > > > +};
> > >
> > > Maybe you can wrap the scmi_chan_info inside of another
> > > structure that contains  the payload pointer, and use container_of
> > > to convert between them?
> >
> > We don't need to convert between the two of them, isn't it ? Are you
> > referring some other field here ?
> 
> > > It's not obvious which parts of the structure should be shared and
> > > which are transport specific.
> >
> > All transport specific information is kept in the transport specific
> > structure which is saved here in the transport_info field. Is there
> > something else that isn't clear ?
> 
> To answer all three, what I meant is that the payload pointer appears
> to be transport specific and

I am not sure if I understood the below statement properly. Is there
something missing from it ?

> should not be part of the common
> structure if there is generic way to access it.

The scmi protocol requires a block of shared memory which is
represented by struct scmi_shared_mem, and payload is this memory
block itself. This block of memory is accessed throughout driver.c
file using ioread/write commands. If payload is transport specific, so
will be those accesses, isn't it ? Are you suggesting to move all this
to mailbox.c (the transport specific file) instead ? I am sorry, but I
am not able to understand how exactly you want me to reorder code here
:(

@Sudeep: I had a question for you though. Looks like we are doing
ioremap() of this payload for every channel's tx/rx, why ? Why is the
same memory area mapped that way ? Can we just map the area once for
scmi block ?

-- 
viresh

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

Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Arnd Bergmann]

On Tue, Jan 14, 2020 at 10:26 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 13-01-20, 12:36, Arnd Bergmann wrote:
> > On Mon, Jan 13, 2020 at 7:42 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > To answer all three, what I meant is that the payload pointer appears
> > to be transport specific and
>
> I am not sure if I understood the below statement properly. Is there
> something missing from it ?
>
> > should not be part of the common
> > structure if there is generic way to access it.
>
> The scmi protocol requires a block of shared memory which is
> represented by struct scmi_shared_mem, and payload is this memory
> block itself. This block of memory is accessed throughout driver.c
> file using ioread/write commands. If payload is transport specific, so
> will be those accesses, isn't it ? Are you suggesting to move all this
> to mailbox.c (the transport specific file) instead ? I am sorry, but I
> am not able to understand how exactly you want me to reorder code here
> :(

My point was that you cannot mix __iomem accesses with pointer
accesses. As I understood it, the current version uses a pointer to a
hardware mailbox with structured data, so you have to use ioremap()
to get a token you can pass into ioread(), but (some of) the new
transport types would just be backed by regular RAM, on which this
is not a well-defined operation and you have to use memremap()
and memcpy() instead.

     Arnd

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

Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Viresh Kumar]

On 14-01-20, 10:56, Arnd Bergmann wrote:
> My point was that you cannot mix __iomem accesses with pointer
> accesses. As I understood it, the current version uses a pointer to a

The current version is stupid as I misunderstood the whole __iomem
thing and just dropped it :)

> hardware mailbox with structured data, so you have to use ioremap()
> to get a token you can pass into ioread(), but (some of) the new
> transport types would just be backed by regular RAM, on which this
> is not a well-defined operation and you have to use memremap()
> and memcpy() instead.

Okay, I think I understand that a bit now. So here are the things
which I may need to do now:

- Maybe move payload to struct scmi_mailbox structure, as that is the
  transport dependent structure..

- Do ioremap, etc in mailbox.c only instead of driver.c

- Provide more ops in struct scmi_transport_ops to provide read/write
  helpers to the payload and implement the ones based on
  ioread/iowrite in mailbox.c ..

Am I thinking in the right direction now ?

-- 
viresh

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

Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Arnd Bergmann]

On Tue, Jan 14, 2020 at 12:11 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 14-01-20, 10:56, Arnd Bergmann wrote:
> > My point was that you cannot mix __iomem accesses with pointer
> > accesses. As I understood it, the current version uses a pointer to a
>
> The current version is stupid as I misunderstood the whole __iomem
> thing and just dropped it :)
>
> > hardware mailbox with structured data, so you have to use ioremap()
> > to get a token you can pass into ioread(), but (some of) the new
> > transport types would just be backed by regular RAM, on which this
> > is not a well-defined operation and you have to use memremap()
> > and memcpy() instead.
>
> Okay, I think I understand that a bit now. So here are the things
> which I may need to do now:
>
> - Maybe move payload to struct scmi_mailbox structure, as that is the
>   transport dependent structure..
>
> - Do ioremap, etc in mailbox.c only instead of driver.c
>
> - Provide more ops in struct scmi_transport_ops to provide read/write
>   helpers to the payload and implement the ones based on
>   ioread/iowrite in mailbox.c ..
>
> Am I thinking in the right direction now ?

That sounds about right. What I'm still not sure about is whether the
current kernel code is actually correct and should use iomeap()
in the first place. Can you confirm that all current hardware
implementations actually use MMIO type registers here rather than
just rely on a buffer in RAM?

      Arnd

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

Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Sudeep Holla]

On Tue, Jan 14, 2020 at 12:17:28PM +0100, Arnd Bergmann wrote:
> On Tue, Jan 14, 2020 at 12:11 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >

> >
> > Okay, I think I understand that a bit now. So here are the things
> > which I may need to do now:
> >
> > - Maybe move payload to struct scmi_mailbox structure, as that is the
> >   transport dependent structure..
> >
> > - Do ioremap, etc in mailbox.c only instead of driver.c
> >
> > - Provide more ops in struct scmi_transport_ops to provide read/write
> >   helpers to the payload and implement the ones based on
> >   ioread/iowrite in mailbox.c ..
> >
> > Am I thinking in the right direction now ?
>
> That sounds about right. What I'm still not sure about is whether the
> current kernel code is actually correct and should use iomeap()
> in the first place. Can you confirm that all current hardware
> implementations actually use MMIO type registers here rather than
> just rely on a buffer in RAM?
>

I remember we had this discussion in the past and was trying to dig up
the archive. I found it [1]. At that time and even today, I don't have
knowledge of any upstream platform using memory other than SRAM and
hence I found it safe to retain ioremap as is.

But I agree in general, this abstraction will allow us to add shmem of
other memory types like RAM. However, it's difficult to have understanding
and representation of the memory type used by the platform firmware say
in DT or even ACPI.

--
Regards,
Sudeep

[1] https://www.spinics.net/lists/arm-kernel/msg647292.html

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

RE: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Peng Fan]

> Subject: Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of
> transport type
> 
> On Tue, Jan 14, 2020 at 12:11 PM Viresh Kumar <viresh.kumar@linaro.org>
> wrote:
> >
> > On 14-01-20, 10:56, Arnd Bergmann wrote:
> > > My point was that you cannot mix __iomem accesses with pointer
> > > accesses. As I understood it, the current version uses a pointer to
> > > a
> >
> > The current version is stupid as I misunderstood the whole __iomem
> > thing and just dropped it :)
> >
> > > hardware mailbox with structured data, so you have to use ioremap()
> > > to get a token you can pass into ioread(), but (some of) the new
> > > transport types would just be backed by regular RAM, on which this
> > > is not a well-defined operation and you have to use memremap() and
> > > memcpy() instead.
> >
> > Okay, I think I understand that a bit now. So here are the things
> > which I may need to do now:
> >
> > - Maybe move payload to struct scmi_mailbox structure, as that is the
> >   transport dependent structure..
> >
> > - Do ioremap, etc in mailbox.c only instead of driver.c
> >
> > - Provide more ops in struct scmi_transport_ops to provide read/write
> >   helpers to the payload and implement the ones based on
> >   ioread/iowrite in mailbox.c ..
> >
> > Am I thinking in the right direction now ?
> 
> That sounds about right. What I'm still not sure about is whether the current
> kernel code is actually correct and should use iomeap() in the first place. Can
> you confirm that all current hardware implementations actually use MMIO
> type registers here rather than just rely on a buffer in RAM?

i.MX8(alought not use SCMI) use MU(message unit) to transfer data between
Acore and Mcore firmware, not using shared memory.

Thanks,
Peng.

> 
>       Arnd
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infr
> adead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&amp;data=02%7C01
> %7Cpeng.fan%40nxp.com%7C1b3e0ec89bde469abfd008d798e36c89%7C686
> ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637145974848395212&a
> mp;sdata=PKDDHvz0M45%2B31vSfWCxwxiDEEY8tQxkmL9AzfzRoSM%3D&a
> mp;reserved=0

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

Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[peter.hilber]

On 14.01.20 12:11, Viresh Kumar wrote:
> Okay, I think I understand that a bit now. So here are the things
> which I may need to do now:
>
> - Maybe move payload to struct scmi_mailbox structure, as that is the
>   transport dependent structure..
>
> - Do ioremap, etc in mailbox.c only instead of driver.c
>
> - Provide more ops in struct scmi_transport_ops to provide read/write
>   helpers to the payload and implement the ones based on
>   ioread/iowrite in mailbox.c ..
>
> Am I thinking in the right direction now ?
>

Another related issue IMHO:

The "shmem" DT property, which is mapped to payload, will not be used
for the planned SCMI virtio transport. But currently "shmem" is still
required by scmi_chan_setup().

Best regards,

Peter

Please mind our privacy notice<https://www.opensynergy.com/datenschutzerklaerung/privacy-notice-for-business-partners-pursuant-to-article-13-of-the-general-data-protection-regulation-gdpr/> pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13 DSGVO finden Sie hier.<https://www.opensynergy.com/de/datenschutzerklaerung/datenschutzhinweise-fuer-geschaeftspartner-gem-art-13-dsgvo/>

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

Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Sudeep Holla]

On Tue, Jan 14, 2020 at 02:56:15PM +0530, Viresh Kumar wrote:

[...]

> The scmi protocol requires a block of shared memory which is
> represented by struct scmi_shared_mem, and payload is this memory
> block itself. This block of memory is accessed throughout driver.c
> file using ioread/write commands. If payload is transport specific, so
> will be those accesses, isn't it ? Are you suggesting to move all this
> to mailbox.c (the transport specific file) instead ? I am sorry, but I
> am not able to understand how exactly you want me to reorder code here
> :(
>

I don't have complete understanding of the requirements yet, but we may
have to make scmi_shared_mem transport specific vaguely based on virtio
requirements. I must have more info once I have discussion with them
soon. I will reply to this thread after that. More likely by end of this
week. Thanks for your patience, we are still trying to make sure we
have gathered all the requirements.

> @Sudeep: I had a question for you though. Looks like we are doing
> ioremap() of this payload for every channel's tx/rx, why ? Why is the
> same memory area mapped that way ? Can we just map the area once for
> scmi block ?
>

Though it may be part of same block of memory on most of the platforms,
it is not mandatory. It can be scattered. VirtIO based transport might
have something like that.

--
Regards,
Sudeep

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

RE: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Peng Fan]

> Subject: [PATCH V2] firmware: arm_scmi: Make scmi core independent of
> transport type
> 
> The SCMI specification is fairly independent of the transport protocol, which
> can be a simple mailbox (already implemented) or anything else.
> The current Linux implementation however is very much dependent of the
> mailbox transport layer.
> 
> This patch makes the SCMI core code (driver.c) independent of the mailbox
> transport layer and moves all mailbox related code to a new
> file: mailbox.c.
> 
> We can now implement more transport protocols to transport SCMI messages,
> some of the transport protocols getting discussed currently are SMC/HVC,
> SPCI (built on top of SMC/HVC), OPTEE based mailbox (similar to SPCI), and
> vitio based transport as alternative to mailbox.
> 
> The transport protocols just need to provide struct scmi_desc, which also
> implements the struct scmi_transport_ops.

I need put shmem for each protocol, is this expected?
Sudeep,
I am able to use smc to directly transport data,
with adding a new file, just named smc.c including a scmi_smc_desc,
But I not find a good way to pass smc id to smc transport file.

+       sram@910000 {
+               compatible = "mmio-sram";
+               reg = <0x0 0x93f000 0x0 0x1000>;
+
+               #address-cells = <1>;
+               #size-cells = <1>;
+               ranges = <0 0x0 0x93f000 0x1000>;
+
+               cpu_scp_lpri: scp-shmem@0 {
+                       compatible = "arm,scmi-shmem";
+                       reg = <0x0 0x200>;
+               };
+
+               cpu_scp_hpri: scp-shmem@200 {
+                       compatible = "arm,scmi-shmem";
+                       reg = <0x200 0x200>;
+               };
+       };
+
+       firmware {
+               scmi {
+                       compatible = "arm,scmi";
+                       shmem = <&cpu_scp_lpri>;
+                       transport = <1>;
+                       arm,func-id = <0xc20000fe>;
+                       #address-cells = <1>;
+                       #size-cells = <0>;
+
+                       scmi_devpd: protocol@11 {
+                               reg = <0x11>;
+                               shmem = <&cpu_scp_lpri>;
+                               #power-domain-cells = <1>;
+                       };
+
+                       scmi_clk: protocol@14 {
+                               reg = <0x14>;
+                               shmem = <&cpu_scp_lpri>;
+                               #clock-cells = <1>;
+                               clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
+                                        <&clk_ext3>, <&clk_ext4>;
+                               clock-names = "osc_32k", "osc_24m", "clk_ext1", "clk_ext2",
+                                             "clk_ext3", "clk_ext4";
+                       };
+               };
+       };

Thanks,
Peng.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2:
> - Dropped __iomem from payload data.
> - Moved transport ops to scmi_desc, and that has a per transport
>   instance now which is differentiated using the compatible string.
> - Converted IS_ERR_OR_NULL to IS_ERR.
> 
>  drivers/firmware/arm_scmi/Makefile  |   3 +-
>  drivers/firmware/arm_scmi/common.h  |  55 ++++++++++
> drivers/firmware/arm_scmi/driver.c  | 151 ++++++----------------------
> drivers/firmware/arm_scmi/mailbox.c | 144 ++++++++++++++++++++++++++
>  4 files changed, 233 insertions(+), 120 deletions(-)  create mode 100644
> drivers/firmware/arm_scmi/mailbox.c
> 
> diff --git a/drivers/firmware/arm_scmi/Makefile
> b/drivers/firmware/arm_scmi/Makefile
> index 5f298f00a82e..df2c05a545d8 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -obj-y	= scmi-bus.o scmi-driver.o scmi-protocols.o
> +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
>  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 5237c2ff79fe..365368f8e6d1 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -111,3 +111,58 @@ void scmi_setup_protocol_implemented(const
> struct scmi_handle *handle,
>  				     u8 *prot_imp);
> 
>  int scmi_base_protocol_init(struct scmi_handle *h);
> +
> +/* SCMI Transport */
> +
> +/**
> + * struct scmi_chan_info - Structure representing a SCMI channel
> +information
> + *
> + * @payload: Transmit/Receive payload area
> + * @dev: Reference to device in the SCMI hierarchy corresponding to this
> + *	 channel
> + * @handle: Pointer to SCMI entity handle
> + * @transport_info: Transport layer related information  */ struct
> +scmi_chan_info {
> +	void *payload;
> +	struct device *dev;
> +	struct scmi_handle *handle;
> +	void *transport_info;
> +};
> +
> +/**
> + * struct scmi_transport_ops - Structure representing a SCMI transport
> +ops
> + *
> + * @send_message: Callback to send a message
> + * @mark_txdone: Callback to mark tx as done
> + * @chan_setup: Callback to allocate and setup a channel
> + * @chan_free: Callback to free a channel  */ struct scmi_transport_ops
> +{
> +	bool (*chan_available)(struct device *dev, int idx);
> +	int (*chan_setup)(struct scmi_chan_info *cinfo, 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); };
> +
> +/**
> + * struct scmi_desc - Description of SoC integration
> + *
> + * @max_rx_timeout_ms: Timeout for communication with SoC (in
> +Milliseconds)
> + * @max_msg: Maximum number of messages that can be pending
> + *	simultaneously in the system
> + * @max_msg_size: Maximum size of data per message that can be handled.
> + */
> +struct scmi_desc {
> +	struct scmi_transport_ops *ops;
> +	int max_rx_timeout_ms;
> +	int max_msg;
> +	int max_msg_size;
> +};
> +
> +extern const struct scmi_desc scmi_mailbox_desc;
> +
> +void scmi_tx_prepare(struct scmi_chan_info *cinfo, struct scmi_xfer
> +*t); void scmi_rx_callback(struct scmi_chan_info *cinfo, struct
> +scmi_xfer *t); 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 3eb0382491ce..e67fcbe27472 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -19,7 +19,6 @@
>  #include <linux/io.h>
>  #include <linux/kernel.h>
>  #include <linux/ktime.h>
> -#include <linux/mailbox_client.h>
>  #include <linux/module.h>
>  #include <linux/of_address.h>
>  #include <linux/of_device.h>
> @@ -77,38 +76,6 @@ struct scmi_xfers_info {
>  	spinlock_t xfer_lock;
>  };
> 
> -/**
> - * struct scmi_desc - Description of SoC integration
> - *
> - * @max_rx_timeout_ms: Timeout for communication with SoC (in
> Milliseconds)
> - * @max_msg: Maximum number of messages that can be pending
> - *	simultaneously in the system
> - * @max_msg_size: Maximum size of data per message that can be handled.
> - */
> -struct scmi_desc {
> -	int max_rx_timeout_ms;
> -	int max_msg;
> -	int max_msg_size;
> -};
> -
> -/**
> - * struct scmi_chan_info - Structure representing a SCMI channel information
> - *
> - * @cl: Mailbox Client
> - * @chan: Transmit/Receive mailbox channel
> - * @payload: Transmit/Receive mailbox channel payload area
> - * @dev: Reference to device in the SCMI hierarchy corresponding to this
> - *	 channel
> - * @handle: Pointer to SCMI entity handle
> - */
> -struct scmi_chan_info {
> -	struct mbox_client cl;
> -	struct mbox_chan *chan;
> -	void __iomem *payload;
> -	struct device *dev;
> -	struct scmi_handle *handle;
> -};
> -
>  /**
>   * struct scmi_info - Structure representing a SCMI instance
>   *
> @@ -138,7 +105,6 @@ struct scmi_info {
>  	int users;
>  };
> 
> -#define client_to_scmi_chan_info(c) container_of(c, struct scmi_chan_info,
> cl)
>  #define handle_to_scmi_info(h)	container_of(h, struct scmi_info, handle)
> 
>  /*
> @@ -195,7 +161,7 @@ static inline void scmi_dump_header_dbg(struct
> device *dev,  }
> 
>  static void scmi_fetch_response(struct scmi_xfer *xfer,
> -				struct scmi_shared_mem __iomem *mem)
> +				struct scmi_shared_mem *mem)
>  {
>  	xfer->hdr.status = ioread32(mem->msg_payload);
>  	/* Skip the length of header and status in payload area i.e 8 bytes */ @@
> -233,19 +199,17 @@ static inline void unpack_scmi_header(u32 msg_hdr,
> struct scmi_msg_hdr *hdr)  }
> 
>  /**
> - * scmi_tx_prepare() - mailbox client callback to prepare for the transfer
> + * scmi_tx_prepare() - callback to prepare for the transfer
>   *
> - * @cl: client pointer
> - * @m: mailbox message
> + * @cinfo: SCMI channel info
> + * @t: transfer message
>   *
>   * This function prepares the shared memory which contains the header and
> the
>   * payload.
>   */
> -static void scmi_tx_prepare(struct mbox_client *cl, void *m)
> +void scmi_tx_prepare(struct scmi_chan_info *cinfo, struct scmi_xfer *t)
>  {
> -	struct scmi_xfer *t = m;
> -	struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
> -	struct scmi_shared_mem __iomem *mem = cinfo->payload;
> +	struct scmi_shared_mem *mem = cinfo->payload;
> 
>  	/*
>  	 * Ideally channel must be free by now unless OS timeout last @@
> -332,10 +296,10 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo, struct
> scmi_xfer *xfer)  }
> 
>  /**
> - * scmi_rx_callback() - mailbox client callback for receive messages
> + * scmi_rx_callback() - callback for receive messages
>   *
> - * @cl: client pointer
> - * @m: mailbox message
> + * @cinfo: SCMI channel info
> + * @t: transfer message
>   *
>   * Processes one received message to appropriate transfer information and
>   * signals completion of the transfer.
> @@ -343,17 +307,16 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo,
> struct scmi_xfer *xfer)
>   * NOTE: This function will be invoked in IRQ context, hence should be
>   * as optimal as possible.
>   */
> -static void scmi_rx_callback(struct mbox_client *cl, void *m)
> +void scmi_rx_callback(struct scmi_chan_info *cinfo, struct scmi_xfer
> +*t)
>  {
>  	u8 msg_type;
>  	u32 msg_hdr;
>  	u16 xfer_id;
>  	struct scmi_xfer *xfer;
> -	struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
>  	struct device *dev = cinfo->dev;
>  	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
>  	struct scmi_xfers_info *minfo = &info->tx_minfo;
> -	struct scmi_shared_mem __iomem *mem = cinfo->payload;
> +	struct scmi_shared_mem *mem = cinfo->payload;
> 
>  	msg_hdr = ioread32(&mem->msg_header);
>  	msg_type = MSG_XTRACT_TYPE(msg_hdr);
> @@ -396,7 +359,7 @@ void scmi_xfer_put(const struct scmi_handle *handle,
> struct scmi_xfer *xfer)  static bool  scmi_xfer_poll_done(const struct
> scmi_chan_info *cinfo, struct scmi_xfer *xfer)  {
> -	struct scmi_shared_mem __iomem *mem = cinfo->payload;
> +	struct scmi_shared_mem *mem = cinfo->payload;
>  	u16 xfer_id = MSG_XTRACT_TOKEN(ioread32(&mem->msg_header));
> 
>  	if (xfer->hdr.seq != xfer_id)
> @@ -439,15 +402,12 @@ int scmi_do_xfer(const struct scmi_handle *handle,
> struct scmi_xfer *xfer)
>  	if (unlikely(!cinfo))
>  		return -EINVAL;
> 
> -	ret = mbox_send_message(cinfo->chan, xfer);
> +	ret = info->desc->ops->send_message(cinfo, xfer);
>  	if (ret < 0) {
> -		dev_dbg(dev, "mbox send fail %d\n", ret);
> +		dev_dbg(dev, "Failed to send message %d\n", ret);
>  		return ret;
>  	}
> 
> -	/* mbox_send_message returns non-negative value on success, so reset
> */
> -	ret = 0;
> -
>  	if (xfer->hdr.poll_completion) {
>  		ktime_t stop = ktime_add_ns(ktime_get(),
> SCMI_MAX_POLL_TO_NS);
> 
> @@ -461,7 +421,7 @@ int scmi_do_xfer(const struct scmi_handle *handle,
> struct scmi_xfer *xfer)
>  		/* And we wait for the response. */
>  		timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
>  		if (!wait_for_completion_timeout(&xfer->done, timeout)) {
> -			dev_err(dev, "mbox timed out in resp(caller: %pS)\n",
> +			dev_err(dev, "timed out in resp(caller: %pS)\n",
>  				(void *)_RET_IP_);
>  			ret = -ETIMEDOUT;
>  		}
> @@ -470,13 +430,7 @@ int scmi_do_xfer(const struct scmi_handle *handle,
> struct scmi_xfer *xfer)
>  	if (!ret && xfer->hdr.status)
>  		ret = scmi_to_linux_errno(xfer->hdr.status);
> 
> -	/*
> -	 * NOTE: we might prefer not to need the mailbox ticker to manage the
> -	 * transfer queueing since the protocol layer queues things by itself.
> -	 * Unfortunately, we have to kick the mailbox framework after we have
> -	 * received our message.
> -	 */
> -	mbox_client_txdone(cinfo->chan, ret);
> +	info->desc->ops->mark_txdone(cinfo, ret);
> 
>  	return ret;
>  }
> @@ -713,21 +667,14 @@ static int scmi_xfer_info_init(struct scmi_info
> *sinfo)
>  	return 0;
>  }
> 
> -static int scmi_mailbox_check(struct device_node *np, int idx) -{
> -	return of_parse_phandle_with_args(np, "mboxes", "#mbox-cells",
> -					  idx, NULL);
> -}
> -
> -static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
> -				int prot_id, bool tx)
> +static int scmi_chan_setup(struct scmi_info *info, struct device *dev,
> +			   int prot_id, bool tx)
>  {
>  	int ret, idx;
>  	struct resource res;
>  	resource_size_t size;
> -	struct device_node *shmem, *np = dev->of_node;
> +	struct device_node *shmem;
>  	struct scmi_chan_info *cinfo;
> -	struct mbox_client *cl;
>  	struct idr *idr;
>  	const char *desc = tx ? "Tx" : "Rx";
> 
> @@ -735,7 +682,7 @@ static int scmi_mbox_chan_setup(struct scmi_info
> *info, struct device *dev,
>  	idx = tx ? 0 : 1;
>  	idr = tx ? &info->tx_idr : &info->rx_idr;
> 
> -	if (scmi_mailbox_check(np, idx)) {
> +	if (!info->desc->ops->chan_available(dev, idx)) {
>  		cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
>  		if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
>  			return -EINVAL;
> @@ -748,14 +695,7 @@ static int scmi_mbox_chan_setup(struct scmi_info
> *info, struct device *dev,
> 
>  	cinfo->dev = dev;
> 
> -	cl = &cinfo->cl;
> -	cl->dev = dev;
> -	cl->rx_callback = scmi_rx_callback;
> -	cl->tx_prepare = tx ? scmi_tx_prepare : NULL;
> -	cl->tx_block = false;
> -	cl->knows_txdone = tx;
> -
> -	shmem = of_parse_phandle(np, "shmem", idx);
> +	shmem = of_parse_phandle(dev->of_node, "shmem", idx);
>  	ret = of_address_to_resource(shmem, 0, &res);
>  	of_node_put(shmem);
>  	if (ret) {
> @@ -770,14 +710,9 @@ static int scmi_mbox_chan_setup(struct scmi_info
> *info, struct device *dev,
>  		return -EADDRNOTAVAIL;
>  	}
> 
> -	cinfo->chan = mbox_request_channel(cl, idx);
> -	if (IS_ERR(cinfo->chan)) {
> -		ret = PTR_ERR(cinfo->chan);
> -		if (ret != -EPROBE_DEFER)
> -			dev_err(dev, "failed to request SCMI %s mailbox\n",
> -				desc);
> +	ret = info->desc->ops->chan_setup(cinfo, tx);
> +	if (ret)
>  		return ret;
> -	}
> 
>  idr_alloc:
>  	ret = idr_alloc(idr, cinfo, prot_id, prot_id + 1, GFP_KERNEL); @@ -791,12
> +726,12 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct
> device *dev,  }
> 
>  static inline int
> -scmi_mbox_txrx_setup(struct scmi_info *info, struct device *dev, int prot_id)
> +scmi_txrx_setup(struct scmi_info *info, struct device *dev, int
> +prot_id)
>  {
> -	int ret = scmi_mbox_chan_setup(info, dev, prot_id, true);
> +	int ret = scmi_chan_setup(info, dev, prot_id, true);
> 
>  	if (!ret) /* Rx is optional, hence no error check */
> -		scmi_mbox_chan_setup(info, dev, prot_id, false);
> +		scmi_chan_setup(info, dev, prot_id, false);
> 
>  	return ret;
>  }
> @@ -814,7 +749,7 @@ scmi_create_protocol_device(struct device_node *np,
> struct scmi_info *info,
>  		return;
>  	}
> 
> -	if (scmi_mbox_txrx_setup(info, &sdev->dev, prot_id)) {
> +	if (scmi_txrx_setup(info, &sdev->dev, prot_id)) {
>  		dev_err(&sdev->dev, "failed to setup transport\n");
>  		scmi_device_destroy(sdev);
>  		return;
> @@ -833,12 +768,6 @@ static int scmi_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *child, *np = dev->of_node;
> 
> -	/* Only mailbox method supported, check for the presence of one */
> -	if (scmi_mailbox_check(np, 0)) {
> -		dev_err(dev, "no mailbox found in %pOF\n", np);
> -		return -EINVAL;
> -	}
> -
>  	desc = of_device_get_match_data(dev);
>  	if (!desc)
>  		return -EINVAL;
> @@ -863,7 +792,7 @@ static int scmi_probe(struct platform_device *pdev)
>  	handle->dev = info->dev;
>  	handle->version = &info->version;
> 
> -	ret = scmi_mbox_txrx_setup(info, dev, SCMI_PROTOCOL_BASE);
> +	ret = scmi_txrx_setup(info, dev, SCMI_PROTOCOL_BASE);
>  	if (ret)
>  		return ret;
> 
> @@ -898,19 +827,9 @@ static int scmi_probe(struct platform_device *pdev)
>  	return 0;
>  }
> 
> -static int scmi_mbox_free_channel(int id, void *p, void *data)
> +void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr,
> +int id)
>  {
> -	struct scmi_chan_info *cinfo = p;
> -	struct idr *idr = data;
> -
> -	if (!IS_ERR_OR_NULL(cinfo->chan)) {
> -		mbox_free_channel(cinfo->chan);
> -		cinfo->chan = NULL;
> -	}
> -
>  	idr_remove(idr, id);
> -
> -	return 0;
>  }
> 
>  static int scmi_remove(struct platform_device *pdev) @@ -930,25 +849,19
> @@ static int scmi_remove(struct platform_device *pdev)
>  		return ret;
> 
>  	/* Safe to free channels since no more users */
> -	ret = idr_for_each(idr, scmi_mbox_free_channel, idr);
> +	ret = idr_for_each(idr, info->desc->ops->chan_free, idr);
>  	idr_destroy(&info->tx_idr);
> 
>  	idr = &info->rx_idr;
> -	ret = idr_for_each(idr, scmi_mbox_free_channel, idr);
> +	ret = idr_for_each(idr, info->desc->ops->chan_free, idr);
>  	idr_destroy(&info->rx_idr);
> 
>  	return ret;
>  }
> 
> -static const struct scmi_desc scmi_generic_desc = {
> -	.max_rx_timeout_ms = 30,	/* We may increase this if required */
> -	.max_msg = 20,		/* Limited by MBOX_TX_QUEUE_LEN */
> -	.max_msg_size = 128,
> -};
> -
>  /* Each compatible listed below must have descriptor associated with it */
> static const struct of_device_id scmi_of_match[] = {
> -	{ .compatible = "arm,scmi", .data = &scmi_generic_desc },
> +	{ .compatible = "arm,scmi", .data = &scmi_mailbox_desc },
>  	{ /* Sentinel */ },
>  };
> 
> diff --git a/drivers/firmware/arm_scmi/mailbox.c
> b/drivers/firmware/arm_scmi/mailbox.c
> new file mode 100644
> index 000000000000..2d1f7c8be293
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/mailbox.c
> @@ -0,0 +1,144 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Message Mailbox
> +Transport driver
> + *
> + * Copyright (C) 2019 ARM Ltd.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/device.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +
> +#include "common.h"
> +
> +/**
> + * struct scmi_mailbox - Structure representing a SCMI mailbox
> +transport
> + *
> + * @cl: Mailbox Client
> + * @chan: Transmit/Receive mailbox channel
> + * @cinfo: SCMI channel info
> + */
> +struct scmi_mailbox {
> +	struct mbox_client cl;
> +	struct mbox_chan *chan;
> +	struct scmi_chan_info *cinfo;
> +};
> +
> +#define client_to_scmi_mailbox(c) container_of(c, struct scmi_mailbox,
> +cl)
> +
> +static bool mailbox_chan_available(struct device *dev, int idx) {
> +	return !of_parse_phandle_with_args(dev->of_node, "mboxes",
> +					   "#mbox-cells", idx, NULL);
> +}
> +
> +static void mailbox_tx_prepare(struct mbox_client *cl, void *m) {
> +	struct scmi_mailbox *smbox = client_to_scmi_mailbox(cl);
> +	struct scmi_chan_info *cinfo = smbox->cinfo;
> +
> +	scmi_tx_prepare(cinfo, m);
> +}
> +
> +static void mailbox_rx_callback(struct mbox_client *cl, void *m) {
> +	struct scmi_mailbox *smbox = client_to_scmi_mailbox(cl);
> +	struct scmi_chan_info *cinfo = smbox->cinfo;
> +
> +	scmi_rx_callback(cinfo, m);
> +}
> +
> +static int mailbox_chan_setup(struct scmi_chan_info *cinfo, bool tx) {
> +	struct device *dev = cinfo->dev;
> +	struct scmi_mailbox *smbox;
> +	struct mbox_client *cl;
> +	int ret;
> +
> +	smbox = devm_kzalloc(dev, sizeof(*smbox), GFP_KERNEL);
> +	if (!smbox)
> +		return -ENOMEM;
> +
> +	cl = &smbox->cl;
> +	cl->dev = dev;
> +	cl->tx_prepare = tx ? mailbox_tx_prepare : NULL;
> +	cl->rx_callback = mailbox_rx_callback;
> +	cl->tx_block = false;
> +	cl->knows_txdone = tx;
> +
> +	smbox->chan = mbox_request_channel(cl, tx ? 0 : 1);
> +	if (IS_ERR(smbox->chan)) {
> +		ret = PTR_ERR(smbox->chan);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to request SCMI %s mailbox\n",
> +				tx ? "Tx" : "Rx");
> +		return ret;
> +	}
> +
> +	cinfo->transport_info = smbox;
> +	smbox->cinfo = cinfo;
> +
> +	return 0;
> +}
> +
> +static int mailbox_chan_free(int id, void *p, void *data) {
> +	struct scmi_chan_info *cinfo = p;
> +	struct scmi_mailbox *smbox = cinfo->transport_info;
> +
> +	if (!IS_ERR(smbox->chan)) {
> +		mbox_free_channel(smbox->chan);
> +		cinfo->transport_info = NULL;
> +		smbox->chan = NULL;
> +		smbox->cinfo = NULL;
> +	}
> +
> +	scmi_free_channel(cinfo, data, id);
> +
> +	return 0;
> +}
> +
> +static int mailbox_send_message(struct scmi_chan_info *cinfo,
> +			struct scmi_xfer *xfer)
> +{
> +	struct scmi_mailbox *smbox = cinfo->transport_info;
> +	int ret;
> +
> +	ret = mbox_send_message(smbox->chan, xfer);
> +
> +	/* mbox_send_message returns non-negative value on success, so reset
> */
> +	if (ret > 0)
> +		ret = 0;
> +
> +	return ret;
> +}
> +
> +static void mailbox_mark_txdone(struct scmi_chan_info *cinfo, int ret)
> +{
> +	struct scmi_mailbox *smbox = cinfo->transport_info;
> +
> +	/*
> +	 * NOTE: we might prefer not to need the mailbox ticker to manage the
> +	 * transfer queueing since the protocol layer queues things by itself.
> +	 * Unfortunately, we have to kick the mailbox framework after we have
> +	 * received our message.
> +	 */
> +	mbox_client_txdone(smbox->chan, ret);
> +}
> +
> +static struct scmi_transport_ops scmi_mailbox_ops = {
> +	.chan_available = mailbox_chan_available,
> +	.chan_setup = mailbox_chan_setup,
> +	.chan_free = mailbox_chan_free,
> +	.send_message = mailbox_send_message,
> +	.mark_txdone = mailbox_mark_txdone,
> +};
> +
> +const struct scmi_desc scmi_mailbox_desc = {
> +	.ops = &scmi_mailbox_ops,
> +	.max_rx_timeout_ms = 30, /* We may increase this if required */
> +	.max_msg = 20, /* Limited by MBOX_TX_QUEUE_LEN */
> +	.max_msg_size = 128,
> +};
> --
> 2.21.0.rc0.269.g1a574e7a288b


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

Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Sudeep Holla]

On Wed, Jan 15, 2020 at 08:53:51AM +0000, Peng Fan wrote:
>
> > Subject: [PATCH V2] firmware: arm_scmi: Make scmi core independent of
> > transport type
> >
> > The SCMI specification is fairly independent of the transport protocol, which
> > can be a simple mailbox (already implemented) or anything else.
> > The current Linux implementation however is very much dependent of the
> > mailbox transport layer.
> >
> > This patch makes the SCMI core code (driver.c) independent of the mailbox
> > transport layer and moves all mailbox related code to a new
> > file: mailbox.c.
> >
> > We can now implement more transport protocols to transport SCMI messages,
> > some of the transport protocols getting discussed currently are SMC/HVC,
> > SPCI (built on top of SMC/HVC), OPTEE based mailbox (similar to SPCI), and
> > vitio based transport as alternative to mailbox.
> >
> > The transport protocols just need to provide struct scmi_desc, which also
> > implements the struct scmi_transport_ops.
>
> I need put shmem for each protocol, is this expected?

No, it's optional. If some/all protocols need dedicated channel for whatever
reasons(like DVFS/Perf for polling based transfers), they can specify.
Absence of dedicated channel infers all protocols share the channel(s).

> Sudeep,
> I am able to use smc to directly transport data,
> with adding a new file, just named smc.c including a scmi_smc_desc,

Good.

> But I not find a good way to pass smc id to smc transport file.
>

IMO, we have to deal this in transport specific init. I am thinking of
chan_setup in context of this patch. Does that make sense ?

[...]

> +
> +    scmi_clk: protocol@14 {
> +            reg = <0x14>;
> +            shmem = <&cpu_scp_lpri>;
> +            #clock-cells = <1>;
> +            clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
> +                     <&clk_ext3>, <&clk_ext4>;
> +            clock-names = "osc_32k", "osc_24m", "clk_ext1", "clk_ext2",
> +                          "clk_ext3", "clk_ext4";

This caught my attention, why do we need these clocks phandle list and
clock names above ? Ideally just need scmi_clk phandle and the index to
refer and names need to be provided by the firmware.

--
Regards,
Sudeep

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

RE: [PATCH V2] firmware: arm_scmi: Make scmi core independent of transport type

[Peng Fan]

> Subject: Re: [PATCH V2] firmware: arm_scmi: Make scmi core independent of
> transport type
> 
> On Wed, Jan 15, 2020 at 08:53:51AM +0000, Peng Fan wrote:
> >
> > > Subject: [PATCH V2] firmware: arm_scmi: Make scmi core independent
> > > of transport type
> > >
> > > The SCMI specification is fairly independent of the transport
> > > protocol, which can be a simple mailbox (already implemented) or
> anything else.
> > > The current Linux implementation however is very much dependent of
> > > the mailbox transport layer.
> > >
> > > This patch makes the SCMI core code (driver.c) independent of the
> > > mailbox transport layer and moves all mailbox related code to a new
> > > file: mailbox.c.
> > >
> > > We can now implement more transport protocols to transport SCMI
> > > messages, some of the transport protocols getting discussed
> > > currently are SMC/HVC, SPCI (built on top of SMC/HVC), OPTEE based
> > > mailbox (similar to SPCI), and vitio based transport as alternative to
> mailbox.
> > >
> > > The transport protocols just need to provide struct scmi_desc, which
> > > also implements the struct scmi_transport_ops.
> >
> > I need put shmem for each protocol, is this expected?
> 
> No, it's optional. If some/all protocols need dedicated channel for whatever
> reasons(like DVFS/Perf for polling based transfers), they can specify.
> Absence of dedicated channel infers all protocols share the channel(s).
> 
> > Sudeep,
> > I am able to use smc to directly transport data, with adding a new
> > file, just named smc.c including a scmi_smc_desc,
> 
> Good.
> 
> > But I not find a good way to pass smc id to smc transport file.
> >
> 
> IMO, we have to deal this in transport specific init. I am thinking of
> chan_setup in context of this patch. Does that make sense ?

Yes, will you implement that?

> 
> [...]
> 
> > +
> > +    scmi_clk: protocol@14 {
> > +            reg = <0x14>;
> > +            shmem = <&cpu_scp_lpri>;
> > +            #clock-cells = <1>;
> > +            clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>,
> <&clk_ext2>,
> > +                     <&clk_ext3>, <&clk_ext4>;
> > +            clock-names = "osc_32k", "osc_24m", "clk_ext1",
> "clk_ext2",
> > +                          "clk_ext3", "clk_ext4";
> 
> This caught my attention, why do we need these clocks phandle list and clock
> names above ? Ideally just need scmi_clk phandle and the index to refer and
> names need to be provided by the firmware.

No need, I forgot the remove them.

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 V2] firmware: arm_scmi: Make scmi core independent of transport type

[kbuild test robot]

Hi Viresh,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.5-rc6]
[cannot apply to arm-soc/for-next next-20200115]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Viresh-Kumar/firmware-arm_scmi-Make-scmi-core-independent-of-transport-type/20200111-034851
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 1522d9da40bdfe502c91163e6d769332897201fa
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-130-g1a803e7a-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/firmware/arm_scmi/driver.c:166:37: sparse: sparse: incorrect type in argument 1 (different address spaces)
>> drivers/firmware/arm_scmi/driver.c:166:37: sparse:    expected void [noderef] <asn:2> *
>> drivers/firmware/arm_scmi/driver.c:166:37: sparse:    got unsigned char *
   drivers/firmware/arm_scmi/driver.c:168:24: sparse: sparse: incorrect type in argument 1 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:168:24: sparse:    expected void [noderef] <asn:2> *
>> drivers/firmware/arm_scmi/driver.c:168:24: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:168:24: sparse: sparse: incorrect type in argument 1 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:168:24: sparse:    expected void [noderef] <asn:2> *
>> drivers/firmware/arm_scmi/driver.c:168:24: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:168:24: sparse: sparse: incorrect type in argument 1 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:168:24: sparse:    expected void [noderef] <asn:2> *
>> drivers/firmware/arm_scmi/driver.c:168:24: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:168:24: sparse: sparse: incorrect type in argument 1 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:168:24: sparse:    expected void [noderef] <asn:2> *
>> drivers/firmware/arm_scmi/driver.c:168:24: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:168:24: sparse: sparse: incorrect type in argument 1 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:168:24: sparse:    expected void [noderef] <asn:2> *
>> drivers/firmware/arm_scmi/driver.c:168:24: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:168:24: sparse: sparse: incorrect type in argument 1 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:168:24: sparse:    expected void [noderef] <asn:2> *
>> drivers/firmware/arm_scmi/driver.c:168:24: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:171:54: sparse: sparse: incorrect type in argument 2 (different address spaces)
>> drivers/firmware/arm_scmi/driver.c:171:54: sparse:    expected void const volatile [noderef] <asn:2> *
   drivers/firmware/arm_scmi/driver.c:171:54: sparse:    got unsigned char *
   drivers/firmware/arm_scmi/driver.c:220:9: sparse: sparse: incorrect type in argument 1 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:220:9: sparse:    expected void [noderef] <asn:2> *
   drivers/firmware/arm_scmi/driver.c:220:9: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:220:9: sparse: sparse: incorrect type in argument 1 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:220:9: sparse:    expected void [noderef] <asn:2> *
   drivers/firmware/arm_scmi/driver.c:220:9: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:223:25: sparse: sparse: incorrect type in argument 2 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:223:25: sparse:    expected void [noderef] <asn:2> *
   drivers/firmware/arm_scmi/driver.c:223:25: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:225:20: sparse: sparse: incorrect type in argument 2 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:225:20: sparse:    expected void [noderef] <asn:2> *
   drivers/firmware/arm_scmi/driver.c:225:20: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:226:57: sparse: sparse: incorrect type in argument 2 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:226:57: sparse:    expected void [noderef] <asn:2> *
   drivers/firmware/arm_scmi/driver.c:226:57: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:227:47: sparse: sparse: incorrect type in argument 2 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:227:47: sparse:    expected void [noderef] <asn:2> *
   drivers/firmware/arm_scmi/driver.c:227:47: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:229:29: sparse: sparse: incorrect type in argument 1 (different address spaces)
>> drivers/firmware/arm_scmi/driver.c:229:29: sparse:    expected void volatile [noderef] <asn:2> *
   drivers/firmware/arm_scmi/driver.c:229:29: sparse:    got unsigned char *
   drivers/firmware/arm_scmi/driver.c:321:29: sparse: sparse: incorrect type in argument 1 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:321:29: sparse:    expected void [noderef] <asn:2> *
   drivers/firmware/arm_scmi/driver.c:321:29: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:363:23: sparse: sparse: incorrect type in argument 1 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:363:23: sparse:    expected void [noderef] <asn:2> *
   drivers/firmware/arm_scmi/driver.c:363:23: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:363:23: sparse: sparse: incorrect type in argument 1 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:363:23: sparse:    expected void [noderef] <asn:2> *
   drivers/firmware/arm_scmi/driver.c:363:23: sparse:    got restricted __le32 *
   drivers/firmware/arm_scmi/driver.c:368:26: sparse: sparse: incorrect type in argument 1 (different address spaces)
   drivers/firmware/arm_scmi/driver.c:368:26: sparse:    expected void [noderef] <asn:2> *
   drivers/firmware/arm_scmi/driver.c:368:26: sparse:    got restricted __le32 *
>> drivers/firmware/arm_scmi/driver.c:707:24: sparse: sparse: incorrect type in assignment (different address spaces)
>> drivers/firmware/arm_scmi/driver.c:707:24: sparse:    expected void *payload
>> drivers/firmware/arm_scmi/driver.c:707:24: sparse:    got void [noderef] <asn:2> *

vim +166 drivers/firmware/arm_scmi/driver.c

aa4f886f3893f8 Sudeep Holla 2017-03-28  162  
aa4f886f3893f8 Sudeep Holla 2017-03-28  163  static void scmi_fetch_response(struct scmi_xfer *xfer,
800abc7f2fa85c Viresh Kumar 2020-01-10  164  				struct scmi_shared_mem *mem)
aa4f886f3893f8 Sudeep Holla 2017-03-28  165  {
aa4f886f3893f8 Sudeep Holla 2017-03-28 @166  	xfer->hdr.status = ioread32(mem->msg_payload);
c29a628976b39e Sudeep Holla 2019-07-08  167  	/* Skip the length of header and status in payload area i.e 8 bytes */
aa4f886f3893f8 Sudeep Holla 2017-03-28 @168  	xfer->rx.len = min_t(size_t, xfer->rx.len, ioread32(&mem->length) - 8);
aa4f886f3893f8 Sudeep Holla 2017-03-28  169  
aa4f886f3893f8 Sudeep Holla 2017-03-28  170  	/* Take a copy to the rx buffer.. */
aa4f886f3893f8 Sudeep Holla 2017-03-28 @171  	memcpy_fromio(xfer->rx.buf, mem->msg_payload + 4, xfer->rx.len);
aa4f886f3893f8 Sudeep Holla 2017-03-28  172  }
aa4f886f3893f8 Sudeep Holla 2017-03-28  173  
aa4f886f3893f8 Sudeep Holla 2017-03-28  174  /**
aa4f886f3893f8 Sudeep Holla 2017-03-28  175   * pack_scmi_header() - packs and returns 32-bit header
aa4f886f3893f8 Sudeep Holla 2017-03-28  176   *
aa4f886f3893f8 Sudeep Holla 2017-03-28  177   * @hdr: pointer to header containing all the information on message id,
aa4f886f3893f8 Sudeep Holla 2017-03-28  178   *	protocol id and sequence id.
1baf47c2e5c946 Sudeep Holla 2018-05-09  179   *
5b65af8f60f580 Sudeep Holla 2019-07-08  180   * Return: 32-bit packed message header to be sent to the platform.
aa4f886f3893f8 Sudeep Holla 2017-03-28  181   */
aa4f886f3893f8 Sudeep Holla 2017-03-28  182  static inline u32 pack_scmi_header(struct scmi_msg_hdr *hdr)
aa4f886f3893f8 Sudeep Holla 2017-03-28  183  {
354b2e36d7dea9 Sudeep Holla 2018-05-09  184  	return FIELD_PREP(MSG_ID_MASK, hdr->id) |
354b2e36d7dea9 Sudeep Holla 2018-05-09  185  		FIELD_PREP(MSG_TOKEN_ID_MASK, hdr->seq) |
354b2e36d7dea9 Sudeep Holla 2018-05-09  186  		FIELD_PREP(MSG_PROTOCOL_ID_MASK, hdr->protocol_id);
aa4f886f3893f8 Sudeep Holla 2017-03-28  187  }
aa4f886f3893f8 Sudeep Holla 2017-03-28  188  
22d1f76109f74b Sudeep Holla 2019-07-08  189  /**
22d1f76109f74b Sudeep Holla 2019-07-08  190   * unpack_scmi_header() - unpacks and records message and protocol id
22d1f76109f74b Sudeep Holla 2019-07-08  191   *
22d1f76109f74b Sudeep Holla 2019-07-08  192   * @msg_hdr: 32-bit packed message header sent from the platform
22d1f76109f74b Sudeep Holla 2019-07-08  193   * @hdr: pointer to header to fetch message and protocol id.
22d1f76109f74b Sudeep Holla 2019-07-08  194   */
22d1f76109f74b Sudeep Holla 2019-07-08  195  static inline void unpack_scmi_header(u32 msg_hdr, struct scmi_msg_hdr *hdr)
22d1f76109f74b Sudeep Holla 2019-07-08  196  {
22d1f76109f74b Sudeep Holla 2019-07-08  197  	hdr->id = MSG_XTRACT_ID(msg_hdr);
22d1f76109f74b Sudeep Holla 2019-07-08  198  	hdr->protocol_id = MSG_XTRACT_PROT_ID(msg_hdr);
22d1f76109f74b Sudeep Holla 2019-07-08  199  }
22d1f76109f74b Sudeep Holla 2019-07-08  200  
aa4f886f3893f8 Sudeep Holla 2017-03-28  201  /**
800abc7f2fa85c Viresh Kumar 2020-01-10  202   * scmi_tx_prepare() - callback to prepare for the transfer
aa4f886f3893f8 Sudeep Holla 2017-03-28  203   *
800abc7f2fa85c Viresh Kumar 2020-01-10  204   * @cinfo: SCMI channel info
800abc7f2fa85c Viresh Kumar 2020-01-10  205   * @t: transfer message
aa4f886f3893f8 Sudeep Holla 2017-03-28  206   *
aa4f886f3893f8 Sudeep Holla 2017-03-28  207   * This function prepares the shared memory which contains the header and the
aa4f886f3893f8 Sudeep Holla 2017-03-28  208   * payload.
aa4f886f3893f8 Sudeep Holla 2017-03-28  209   */
800abc7f2fa85c Viresh Kumar 2020-01-10  210  void scmi_tx_prepare(struct scmi_chan_info *cinfo, struct scmi_xfer *t)
aa4f886f3893f8 Sudeep Holla 2017-03-28  211  {
800abc7f2fa85c Viresh Kumar 2020-01-10  212  	struct scmi_shared_mem *mem = cinfo->payload;
aa4f886f3893f8 Sudeep Holla 2017-03-28  213  
9dc34d635c67e5 Sudeep Holla 2019-07-08  214  	/*
9dc34d635c67e5 Sudeep Holla 2019-07-08  215  	 * Ideally channel must be free by now unless OS timeout last
9dc34d635c67e5 Sudeep Holla 2019-07-08  216  	 * request and platform continued to process the same, wait
9dc34d635c67e5 Sudeep Holla 2019-07-08  217  	 * until it releases the shared memory, otherwise we may endup
9dc34d635c67e5 Sudeep Holla 2019-07-08  218  	 * overwriting its response with new message payload or vice-versa
9dc34d635c67e5 Sudeep Holla 2019-07-08  219  	 */
9dc34d635c67e5 Sudeep Holla 2019-07-08  220  	spin_until_cond(ioread32(&mem->channel_status) &
9dc34d635c67e5 Sudeep Holla 2019-07-08  221  			SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE);
aa4f886f3893f8 Sudeep Holla 2017-03-28  222  	/* Mark channel busy + clear error */
aa4f886f3893f8 Sudeep Holla 2017-03-28  223  	iowrite32(0x0, &mem->channel_status);
aa4f886f3893f8 Sudeep Holla 2017-03-28  224  	iowrite32(t->hdr.poll_completion ? 0 : SCMI_SHMEM_FLAG_INTR_ENABLED,
aa4f886f3893f8 Sudeep Holla 2017-03-28  225  		  &mem->flags);
aa4f886f3893f8 Sudeep Holla 2017-03-28  226  	iowrite32(sizeof(mem->msg_header) + t->tx.len, &mem->length);
aa4f886f3893f8 Sudeep Holla 2017-03-28  227  	iowrite32(pack_scmi_header(&t->hdr), &mem->msg_header);
aa4f886f3893f8 Sudeep Holla 2017-03-28  228  	if (t->tx.buf)
aa4f886f3893f8 Sudeep Holla 2017-03-28 @229  		memcpy_toio(mem->msg_payload, t->tx.buf, t->tx.len);
aa4f886f3893f8 Sudeep Holla 2017-03-28  230  }
aa4f886f3893f8 Sudeep Holla 2017-03-28  231  

:::::: The code at line 166 was first introduced by commit
:::::: aa4f886f3893f88146e8e02fd1e9c5c9e43cbcc1 firmware: arm_scmi: add basic driver infrastructure for SCMI

:::::: TO: Sudeep Holla <sudeep.holla@arm.com>
:::::: CC: Sudeep Holla <sudeep.holla@arm.com>

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

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