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

[PATCH V3] firmware: arm_scmi: Make scmi core independent of the 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 on 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.

The transport protocols just need to provide struct scmi_transport_ops,
with its version of the callbacks to enable exchange of SCMI messages.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
@Sudeep: Can you please help me getting this tested?

V2->V3:
- Added more ops to the structure to read/write/memcpy data
- Payload is moved to mailbox.c and is handled in transport specific way
  now. This resulted in lots of changes.

V1->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  |  88 +++++++++++
 drivers/firmware/arm_scmi/driver.c  | 223 +++++++---------------------
 drivers/firmware/arm_scmi/mailbox.c | 202 +++++++++++++++++++++++++
 4 files changed, 343 insertions(+), 173 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..fc3e427eb2cc 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -13,6 +13,7 @@
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/scmi_protocol.h>
+#include <linux/stddef.h>
 #include <linux/types.h>
 
 #include <asm/unaligned.h>
@@ -111,3 +112,90 @@ void scmi_setup_protocol_implemented(const struct scmi_handle *handle,
 				     u8 *prot_imp);
 
 int scmi_base_protocol_init(struct scmi_handle *h);
+
+/* SCMI Transport */
+
+/*
+ * SCMI specification requires all parameters, message headers, return
+ * arguments or any protocol data to be expressed in little endian
+ * format only.
+ */
+struct scmi_shared_mem {
+	__le32 reserved;
+	__le32 channel_status;
+#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR	BIT(1)
+#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE	BIT(0)
+	__le32 reserved1[2];
+	__le32 flags;
+#define SCMI_SHMEM_FLAG_INTR_ENABLED	BIT(0)
+	__le32 length;
+	__le32 msg_header;
+	u8 msg_payload[0];
+};
+
+/* Offset of fields within the above structure */
+#define SHMEM_CHANNEL_STATUS		offsetof(struct scmi_shared_mem, channel_status)
+#define SHMEM_FLAGS			offsetof(struct scmi_shared_mem, flags)
+#define SHMEM_LENGTH			offsetof(struct scmi_shared_mem, length)
+#define SHMEM_MSG_HEADER		offsetof(struct scmi_shared_mem, msg_header)
+#define SHMEM_MSG_PAYLOAD		offsetof(struct scmi_shared_mem, msg_payload)
+
+struct scmi_info;
+
+/**
+ * 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 {
+	struct scmi_info *info;
+	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, struct device *dev, 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);
+	u32 (*read32)(struct scmi_chan_info *cinfo, unsigned int offset);
+	void (*write32)(struct scmi_chan_info *cinfo, u32 val, unsigned int offset);
+	void (*memcpy_from)(struct scmi_chan_info *cinfo, void *to, unsigned int offset, long len);
+	void (*memcpy_to)(struct scmi_chan_info *cinfo, unsigned int offset, void *from, long len);
+
+};
+
+/**
+ * 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..c34b283685c5 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,27 +105,8 @@ 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)
 
-/*
- * SCMI specification requires all parameters, message headers, return
- * arguments or any protocol data to be expressed in little endian
- * format only.
- */
-struct scmi_shared_mem {
-	__le32 reserved;
-	__le32 channel_status;
-#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR	BIT(1)
-#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE	BIT(0)
-	__le32 reserved1[2];
-	__le32 flags;
-#define SCMI_SHMEM_FLAG_INTR_ENABLED	BIT(0)
-	__le32 length;
-	__le32 msg_header;
-	u8 msg_payload[0];
-};
-
 static const int scmi_linux_errmap[] = {
 	/* better than switch case as long as return value is continuous */
 	0,			/* SCMI_SUCCESS */
@@ -194,15 +142,17 @@ static inline void scmi_dump_header_dbg(struct device *dev,
 		hdr->id, hdr->seq, hdr->protocol_id);
 }
 
-static void scmi_fetch_response(struct scmi_xfer *xfer,
-				struct scmi_shared_mem __iomem *mem)
+static void scmi_fetch_response(struct scmi_chan_info *cinfo,
+				struct scmi_xfer *xfer)
 {
-	xfer->hdr.status = ioread32(mem->msg_payload);
+	struct scmi_transport_ops *ops = cinfo->info->desc->ops;
+
+	xfer->hdr.status = ops->read32(cinfo, SHMEM_MSG_PAYLOAD);
 	/* Skip the length of header and status in payload area i.e 8 bytes */
-	xfer->rx.len = min_t(size_t, xfer->rx.len, ioread32(&mem->length) - 8);
+	xfer->rx.len = min_t(size_t, xfer->rx.len, ops->read32(cinfo, SHMEM_LENGTH) - 8);
 
 	/* Take a copy to the rx buffer.. */
-	memcpy_fromio(xfer->rx.buf, mem->msg_payload + 4, xfer->rx.len);
+	ops->memcpy_from(cinfo, xfer->rx.buf, SHMEM_MSG_PAYLOAD + 4, xfer->rx.len);
 }
 
 /**
@@ -233,19 +183,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_transport_ops *ops = cinfo->info->desc->ops;
 
 	/*
 	 * Ideally channel must be free by now unless OS timeout last
@@ -253,16 +201,17 @@ static void scmi_tx_prepare(struct mbox_client *cl, void *m)
 	 * until it releases the shared memory, otherwise we may endup
 	 * overwriting its response with new message payload or vice-versa
 	 */
-	spin_until_cond(ioread32(&mem->channel_status) &
+	spin_until_cond(ops->read32(cinfo, SHMEM_CHANNEL_STATUS) &
 			SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE);
 	/* Mark channel busy + clear error */
-	iowrite32(0x0, &mem->channel_status);
-	iowrite32(t->hdr.poll_completion ? 0 : SCMI_SHMEM_FLAG_INTR_ENABLED,
-		  &mem->flags);
-	iowrite32(sizeof(mem->msg_header) + t->tx.len, &mem->length);
-	iowrite32(pack_scmi_header(&t->hdr), &mem->msg_header);
+	ops->write32(cinfo, 0x0, SHMEM_CHANNEL_STATUS);
+	ops->write32(cinfo,
+		     t->hdr.poll_completion ? 0 : SCMI_SHMEM_FLAG_INTR_ENABLED,
+		     SHMEM_FLAGS);
+	ops->write32(cinfo, sizeof(((struct scmi_shared_mem *)0)->msg_header) + t->tx.len, SHMEM_LENGTH);
+	ops->write32(cinfo, pack_scmi_header(&t->hdr), SHMEM_MSG_HEADER);
 	if (t->tx.buf)
-		memcpy_toio(mem->msg_payload, t->tx.buf, t->tx.len);
+		ops->memcpy_to(cinfo, SHMEM_MSG_PAYLOAD, t->tx.buf, t->tx.len);
 }
 
 /**
@@ -332,10 +281,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 receiving 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,19 +292,17 @@ __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;
 
-	msg_hdr = ioread32(&mem->msg_header);
+	msg_hdr = cinfo->info->desc->ops->read32(cinfo, SHMEM_MSG_HEADER);
 	msg_type = MSG_XTRACT_TYPE(msg_hdr);
 	xfer_id = MSG_XTRACT_TOKEN(msg_hdr);
 
@@ -372,7 +319,7 @@ static void scmi_rx_callback(struct mbox_client *cl, void *m)
 
 	scmi_dump_header_dbg(dev, &xfer->hdr);
 
-	scmi_fetch_response(xfer, mem);
+	scmi_fetch_response(cinfo, xfer);
 
 	if (msg_type == MSG_TYPE_DELAYED_RESP)
 		complete(xfer->async_done);
@@ -394,22 +341,22 @@ 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)
+scmi_xfer_poll_done(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
 {
-	struct scmi_shared_mem __iomem *mem = cinfo->payload;
-	u16 xfer_id = MSG_XTRACT_TOKEN(ioread32(&mem->msg_header));
+	struct scmi_transport_ops *ops = cinfo->info->desc->ops;
+	u16 xfer_id = MSG_XTRACT_TOKEN(ops->read32(cinfo, SHMEM_MSG_HEADER));
 
 	if (xfer->hdr.seq != xfer_id)
 		return false;
 
-	return ioread32(&mem->channel_status) &
+	return ops->read32(cinfo, SHMEM_CHANNEL_STATUS) &
 		(SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR |
 		SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE);
 }
 
 #define SCMI_MAX_POLL_TO_NS	(100 * NSEC_PER_USEC)
 
-static bool scmi_xfer_done_no_timeout(const struct scmi_chan_info *cinfo,
+static bool scmi_xfer_done_no_timeout(struct scmi_chan_info *cinfo,
 				      struct scmi_xfer *xfer, ktime_t stop)
 {
 	ktime_t __cur = ktime_get();
@@ -439,29 +386,26 @@ 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);
 
 		spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer, stop));
 
 		if (ktime_before(ktime_get(), stop))
-			scmi_fetch_response(xfer, cinfo->payload);
+			scmi_fetch_response(cinfo, xfer);
 		else
 			ret = -ETIMEDOUT;
 	} else {
 		/* 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 +414,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,29 +651,18 @@ 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 scmi_chan_info *cinfo;
-	struct mbox_client *cl;
 	struct idr *idr;
-	const char *desc = tx ? "Tx" : "Rx";
 
 	/* Transmit channel is first entry i.e. index 0 */
 	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;
@@ -747,37 +674,11 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
 		return -ENOMEM;
 
 	cinfo->dev = dev;
+	cinfo->info = info;
 
-	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);
-	ret = of_address_to_resource(shmem, 0, &res);
-	of_node_put(shmem);
-	if (ret) {
-		dev_err(dev, "failed to get SCMI %s payload memory\n", desc);
-		return ret;
-	}
-
-	size = resource_size(&res);
-	cinfo->payload = devm_ioremap(info->dev, res.start, size);
-	if (!cinfo->payload) {
-		dev_err(dev, "failed to ioremap SCMI %s payload\n", desc);
-		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, info->dev, tx);
+	if (ret)
 		return ret;
-	}
 
 idr_alloc:
 	ret = idr_alloc(idr, cinfo, prot_id, prot_id + 1, GFP_KERNEL);
@@ -791,12 +692,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 +715,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 +734,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 +758,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 +793,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 +815,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..7509e7eb262a
--- /dev/null
+++ b/drivers/firmware/arm_scmi/mailbox.c
@@ -0,0 +1,202 @@
+// 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/of_address.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;
+	void __iomem *payload;
+};
+
+#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, struct device *dev,
+			      bool tx)
+{
+	const char *desc = tx ? "Tx" : "Rx";
+	struct device *cdev = cinfo->dev;
+	struct scmi_mailbox *smbox;
+	struct device_node *shmem;
+	int ret, idx = tx ? 0 : 1;
+	struct mbox_client *cl;
+	resource_size_t size;
+	struct resource res;
+
+	smbox = devm_kzalloc(cdev, sizeof(*smbox), GFP_KERNEL);
+	if (!smbox)
+		return -ENOMEM;
+
+	shmem = of_parse_phandle(cdev->of_node, "shmem", idx);
+	ret = of_address_to_resource(shmem, 0, &res);
+	of_node_put(shmem);
+	if (ret) {
+		dev_err(cdev, "failed to get SCMI %s payload memory\n", desc);
+		return ret;
+	}
+
+	size = resource_size(&res);
+	smbox->payload = devm_ioremap(dev, res.start, size);
+	if (!smbox->payload) {
+		dev_err(dev, "failed to ioremap SCMI %s payload\n", desc);
+		return -EADDRNOTAVAIL;
+	}
+
+	cl = &smbox->cl;
+	cl->dev = cdev;
+	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(cdev, "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 u32 mailbox_read32(struct scmi_chan_info *cinfo, unsigned int offset)
+{
+	struct scmi_mailbox *smbox = cinfo->transport_info;
+
+	return ioread32(smbox->payload + offset);
+}
+
+static void mailbox_write32(struct scmi_chan_info *cinfo, u32 val,
+			    unsigned int offset)
+{
+	struct scmi_mailbox *smbox = cinfo->transport_info;
+
+	iowrite32(val, smbox->payload + offset);
+}
+
+static void mailbox_memcpy_from(struct scmi_chan_info *cinfo, void *to,
+				unsigned int offset, long len)
+{
+	struct scmi_mailbox *smbox = cinfo->transport_info;
+
+	memcpy_fromio(to, smbox->payload + offset, len);
+}
+
+static void mailbox_memcpy_to(struct scmi_chan_info *cinfo, unsigned int offset,
+			      void *from, long len)
+{
+	struct scmi_mailbox *smbox = cinfo->transport_info;
+
+	memcpy_toio(smbox->payload + offset, from, len);
+}
+
+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,
+	.read32 = mailbox_read32,
+	.write32 = mailbox_write32,
+	.memcpy_from = mailbox_memcpy_from,
+	.memcpy_to = mailbox_memcpy_to,
+};
+
+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

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

[Arnd Bergmann]

On Tue, Jan 21, 2020 at 9:27 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 on 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.
>
> The transport protocols just need to provide struct scmi_transport_ops,
> with its version of the callbacks to enable exchange of SCMI messages.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> @Sudeep: Can you please help me getting this tested?
>
> V2->V3:
> - Added more ops to the structure to read/write/memcpy data
> - Payload is moved to mailbox.c and is handled in transport specific way
>   now. This resulted in lots of changes.

This addresses the comments I had about the implementation.

It's still hard for me to judge whether this is a good abstraction as
long as there is only one backend in the framework, but I see nothing
immediately wrong with it either.

       Arnd

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

[Sudeep Holla]

On Tue, Jan 21, 2020 at 04:11:11PM +0100, Arnd Bergmann wrote:
> On Tue, Jan 21, 2020 at 9:27 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 on 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.
> >
> > The transport protocols just need to provide struct scmi_transport_ops,
> > with its version of the callbacks to enable exchange of SCMI messages.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > @Sudeep: Can you please help me getting this tested?
> >

Sure(I may need to rebase on top of -next to test on top of what's queued
for v5.6)

> > V2->V3:
> > - Added more ops to the structure to read/write/memcpy data
> > - Payload is moved to mailbox.c and is handled in transport specific way
> >   now. This resulted in lots of changes.
>
> This addresses the comments I had about the implementation.
>

Thanks for review and all the suggestions Arnd.

> It's still hard for me to judge whether this is a good abstraction as
> long as there is only one backend in the framework, but I see nothing
> immediately wrong with it either.
>

Peter and Peng(both in cc) is trying out virtio and smc/hvc based transport
respectively. Hopefully they will raise concerns(if any) with the abstraction.

--
Regards,
Sudeep

[PATCH V4] firmware: arm_scmi: Make scmi core independent of the 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 on 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.

The transport protocols just need to provide struct scmi_transport_ops,
with its version of the callbacks to enable exchange of SCMI messages.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
@Sudeep: It was my mistake that I didn't check if there is something
already queued for next release and you never told me as well earlier :)

Can you please help me getting this tested, now that I have rebased it
as well :) ?

V3->V4:
- Rebased on top of linux-next.

V2->V3:
- Added more ops to the structure to read/write/memcpy data
- Payload is moved to mailbox.c and is handled in transport specific way
  now. This resulted in lots of changes.

V1->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  |  88 +++++++++++
 drivers/firmware/arm_scmi/driver.c  | 223 +++++++---------------------
 drivers/firmware/arm_scmi/mailbox.c | 202 +++++++++++++++++++++++++
 4 files changed, 343 insertions(+), 173 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 df35358ff324..6d130c53d266 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -13,6 +13,7 @@
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/scmi_protocol.h>
+#include <linux/stddef.h>
 #include <linux/types.h>
 
 #include <asm/unaligned.h>
@@ -113,3 +114,90 @@ void scmi_setup_protocol_implemented(const struct scmi_handle *handle,
 				     u8 *prot_imp);
 
 int scmi_base_protocol_init(struct scmi_handle *h);
+
+/* SCMI Transport */
+
+/*
+ * SCMI specification requires all parameters, message headers, return
+ * arguments or any protocol data to be expressed in little endian
+ * format only.
+ */
+struct scmi_shared_mem {
+	__le32 reserved;
+	__le32 channel_status;
+#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR	BIT(1)
+#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE	BIT(0)
+	__le32 reserved1[2];
+	__le32 flags;
+#define SCMI_SHMEM_FLAG_INTR_ENABLED	BIT(0)
+	__le32 length;
+	__le32 msg_header;
+	u8 msg_payload[0];
+};
+
+/* Offset of fields within the above structure */
+#define SHMEM_CHANNEL_STATUS		offsetof(struct scmi_shared_mem, channel_status)
+#define SHMEM_FLAGS			offsetof(struct scmi_shared_mem, flags)
+#define SHMEM_LENGTH			offsetof(struct scmi_shared_mem, length)
+#define SHMEM_MSG_HEADER		offsetof(struct scmi_shared_mem, msg_header)
+#define SHMEM_MSG_PAYLOAD		offsetof(struct scmi_shared_mem, msg_payload)
+
+struct scmi_info;
+
+/**
+ * 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 {
+	struct scmi_info *info;
+	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, struct device *dev, 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);
+	u32 (*read32)(struct scmi_chan_info *cinfo, unsigned int offset);
+	void (*write32)(struct scmi_chan_info *cinfo, u32 val, unsigned int offset);
+	void (*memcpy_from)(struct scmi_chan_info *cinfo, void *to, unsigned int offset, long len);
+	void (*memcpy_to)(struct scmi_chan_info *cinfo, unsigned int offset, void *from, long len);
+
+};
+
+/**
+ * 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 2c96f6b5a7d8..289340ce014c 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>
@@ -82,38 +81,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
  *
@@ -143,27 +110,8 @@ 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)
 
-/*
- * SCMI specification requires all parameters, message headers, return
- * arguments or any protocol data to be expressed in little endian
- * format only.
- */
-struct scmi_shared_mem {
-	__le32 reserved;
-	__le32 channel_status;
-#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR	BIT(1)
-#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE	BIT(0)
-	__le32 reserved1[2];
-	__le32 flags;
-#define SCMI_SHMEM_FLAG_INTR_ENABLED	BIT(0)
-	__le32 length;
-	__le32 msg_header;
-	u8 msg_payload[0];
-};
-
 static const int scmi_linux_errmap[] = {
 	/* better than switch case as long as return value is continuous */
 	0,			/* SCMI_SUCCESS */
@@ -199,15 +147,17 @@ static inline void scmi_dump_header_dbg(struct device *dev,
 		hdr->id, hdr->seq, hdr->protocol_id);
 }
 
-static void scmi_fetch_response(struct scmi_xfer *xfer,
-				struct scmi_shared_mem __iomem *mem)
+static void scmi_fetch_response(struct scmi_chan_info *cinfo,
+				struct scmi_xfer *xfer)
 {
-	xfer->hdr.status = ioread32(mem->msg_payload);
+	struct scmi_transport_ops *ops = cinfo->info->desc->ops;
+
+	xfer->hdr.status = ops->read32(cinfo, SHMEM_MSG_PAYLOAD);
 	/* Skip the length of header and status in payload area i.e 8 bytes */
-	xfer->rx.len = min_t(size_t, xfer->rx.len, ioread32(&mem->length) - 8);
+	xfer->rx.len = min_t(size_t, xfer->rx.len, ops->read32(cinfo, SHMEM_LENGTH) - 8);
 
 	/* Take a copy to the rx buffer.. */
-	memcpy_fromio(xfer->rx.buf, mem->msg_payload + 4, xfer->rx.len);
+	ops->memcpy_from(cinfo, xfer->rx.buf, SHMEM_MSG_PAYLOAD + 4, xfer->rx.len);
 }
 
 /**
@@ -238,19 +188,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_transport_ops *ops = cinfo->info->desc->ops;
 
 	/*
 	 * Ideally channel must be free by now unless OS timeout last
@@ -258,16 +206,17 @@ static void scmi_tx_prepare(struct mbox_client *cl, void *m)
 	 * until it releases the shared memory, otherwise we may endup
 	 * overwriting its response with new message payload or vice-versa
 	 */
-	spin_until_cond(ioread32(&mem->channel_status) &
+	spin_until_cond(ops->read32(cinfo, SHMEM_CHANNEL_STATUS) &
 			SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE);
 	/* Mark channel busy + clear error */
-	iowrite32(0x0, &mem->channel_status);
-	iowrite32(t->hdr.poll_completion ? 0 : SCMI_SHMEM_FLAG_INTR_ENABLED,
-		  &mem->flags);
-	iowrite32(sizeof(mem->msg_header) + t->tx.len, &mem->length);
-	iowrite32(pack_scmi_header(&t->hdr), &mem->msg_header);
+	ops->write32(cinfo, 0x0, SHMEM_CHANNEL_STATUS);
+	ops->write32(cinfo,
+		     t->hdr.poll_completion ? 0 : SCMI_SHMEM_FLAG_INTR_ENABLED,
+		     SHMEM_FLAGS);
+	ops->write32(cinfo, sizeof(((struct scmi_shared_mem *)0)->msg_header) + t->tx.len, SHMEM_LENGTH);
+	ops->write32(cinfo, pack_scmi_header(&t->hdr), SHMEM_MSG_HEADER);
 	if (t->tx.buf)
-		memcpy_toio(mem->msg_payload, t->tx.buf, t->tx.len);
+		ops->memcpy_to(cinfo, SHMEM_MSG_PAYLOAD, t->tx.buf, t->tx.len);
 }
 
 /**
@@ -338,10 +287,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 receiving 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.
@@ -349,19 +298,17 @@ __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;
 
-	msg_hdr = ioread32(&mem->msg_header);
+	msg_hdr = cinfo->info->desc->ops->read32(cinfo, SHMEM_MSG_HEADER);
 	msg_type = MSG_XTRACT_TYPE(msg_hdr);
 	xfer_id = MSG_XTRACT_TOKEN(msg_hdr);
 
@@ -378,7 +325,7 @@ static void scmi_rx_callback(struct mbox_client *cl, void *m)
 
 	scmi_dump_header_dbg(dev, &xfer->hdr);
 
-	scmi_fetch_response(xfer, mem);
+	scmi_fetch_response(cinfo, xfer);
 
 	trace_scmi_rx_done(xfer->transfer_id, xfer->hdr.id,
 			   xfer->hdr.protocol_id, xfer->hdr.seq,
@@ -404,22 +351,22 @@ 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)
+scmi_xfer_poll_done(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
 {
-	struct scmi_shared_mem __iomem *mem = cinfo->payload;
-	u16 xfer_id = MSG_XTRACT_TOKEN(ioread32(&mem->msg_header));
+	struct scmi_transport_ops *ops = cinfo->info->desc->ops;
+	u16 xfer_id = MSG_XTRACT_TOKEN(ops->read32(cinfo, SHMEM_MSG_HEADER));
 
 	if (xfer->hdr.seq != xfer_id)
 		return false;
 
-	return ioread32(&mem->channel_status) &
+	return ops->read32(cinfo, SHMEM_CHANNEL_STATUS) &
 		(SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR |
 		SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE);
 }
 
 #define SCMI_MAX_POLL_TO_NS	(100 * NSEC_PER_USEC)
 
-static bool scmi_xfer_done_no_timeout(const struct scmi_chan_info *cinfo,
+static bool scmi_xfer_done_no_timeout(struct scmi_chan_info *cinfo,
 				      struct scmi_xfer *xfer, ktime_t stop)
 {
 	ktime_t __cur = ktime_get();
@@ -453,29 +400,26 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
 			      xfer->hdr.protocol_id, xfer->hdr.seq,
 			      xfer->hdr.poll_completion);
 
-	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);
 
 		spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer, stop));
 
 		if (ktime_before(ktime_get(), stop))
-			scmi_fetch_response(xfer, cinfo->payload);
+			scmi_fetch_response(cinfo, xfer);
 		else
 			ret = -ETIMEDOUT;
 	} else {
 		/* 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;
 		}
@@ -484,13 +428,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);
 
 	trace_scmi_xfer_end(xfer->transfer_id, xfer->hdr.id,
 			    xfer->hdr.protocol_id, xfer->hdr.seq,
@@ -731,23 +669,12 @@ 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 scmi_chan_info *cinfo;
-	struct mbox_client *cl;
 	struct idr *idr;
-	const char *desc = tx ? "Tx" : "Rx";
 
 	/* Transmit channel is first entry i.e. index 0 */
 	idx = tx ? 0 : 1;
@@ -758,7 +685,7 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
 	if (cinfo)
 		return 0;
 
-	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;
@@ -770,37 +697,11 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
 		return -ENOMEM;
 
 	cinfo->dev = dev;
+	cinfo->info = info;
 
-	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);
-	ret = of_address_to_resource(shmem, 0, &res);
-	of_node_put(shmem);
-	if (ret) {
-		dev_err(dev, "failed to get SCMI %s payload memory\n", desc);
-		return ret;
-	}
-
-	size = resource_size(&res);
-	cinfo->payload = devm_ioremap(info->dev, res.start, size);
-	if (!cinfo->payload) {
-		dev_err(dev, "failed to ioremap SCMI %s payload\n", desc);
-		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, info->dev, tx);
+	if (ret)
 		return ret;
-	}
 
 idr_alloc:
 	ret = idr_alloc(idr, cinfo, prot_id, prot_id + 1, GFP_KERNEL);
@@ -814,12 +715,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;
 }
@@ -837,7 +738,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;
@@ -890,12 +791,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;
@@ -920,7 +815,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;
 
@@ -955,19 +850,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)
@@ -987,11 +872,11 @@ 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;
@@ -1043,15 +928,9 @@ static struct attribute *versions_attrs[] = {
 };
 ATTRIBUTE_GROUPS(versions);
 
-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..7509e7eb262a
--- /dev/null
+++ b/drivers/firmware/arm_scmi/mailbox.c
@@ -0,0 +1,202 @@
+// 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/of_address.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;
+	void __iomem *payload;
+};
+
+#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, struct device *dev,
+			      bool tx)
+{
+	const char *desc = tx ? "Tx" : "Rx";
+	struct device *cdev = cinfo->dev;
+	struct scmi_mailbox *smbox;
+	struct device_node *shmem;
+	int ret, idx = tx ? 0 : 1;
+	struct mbox_client *cl;
+	resource_size_t size;
+	struct resource res;
+
+	smbox = devm_kzalloc(cdev, sizeof(*smbox), GFP_KERNEL);
+	if (!smbox)
+		return -ENOMEM;
+
+	shmem = of_parse_phandle(cdev->of_node, "shmem", idx);
+	ret = of_address_to_resource(shmem, 0, &res);
+	of_node_put(shmem);
+	if (ret) {
+		dev_err(cdev, "failed to get SCMI %s payload memory\n", desc);
+		return ret;
+	}
+
+	size = resource_size(&res);
+	smbox->payload = devm_ioremap(dev, res.start, size);
+	if (!smbox->payload) {
+		dev_err(dev, "failed to ioremap SCMI %s payload\n", desc);
+		return -EADDRNOTAVAIL;
+	}
+
+	cl = &smbox->cl;
+	cl->dev = cdev;
+	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(cdev, "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 u32 mailbox_read32(struct scmi_chan_info *cinfo, unsigned int offset)
+{
+	struct scmi_mailbox *smbox = cinfo->transport_info;
+
+	return ioread32(smbox->payload + offset);
+}
+
+static void mailbox_write32(struct scmi_chan_info *cinfo, u32 val,
+			    unsigned int offset)
+{
+	struct scmi_mailbox *smbox = cinfo->transport_info;
+
+	iowrite32(val, smbox->payload + offset);
+}
+
+static void mailbox_memcpy_from(struct scmi_chan_info *cinfo, void *to,
+				unsigned int offset, long len)
+{
+	struct scmi_mailbox *smbox = cinfo->transport_info;
+
+	memcpy_fromio(to, smbox->payload + offset, len);
+}
+
+static void mailbox_memcpy_to(struct scmi_chan_info *cinfo, unsigned int offset,
+			      void *from, long len)
+{
+	struct scmi_mailbox *smbox = cinfo->transport_info;
+
+	memcpy_toio(smbox->payload + offset, from, len);
+}
+
+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,
+	.read32 = mailbox_read32,
+	.write32 = mailbox_write32,
+	.memcpy_from = mailbox_memcpy_from,
+	.memcpy_to = mailbox_memcpy_to,
+};
+
+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

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

[Sudeep Holla]

On Wed, Jan 22, 2020 at 08:06:23AM +0530, Viresh Kumar 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 on 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.
>
> The transport protocols just need to provide struct scmi_transport_ops,
> with its version of the callbacks to enable exchange of SCMI messages.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> @Sudeep: It was my mistake that I didn't check if there is something
> already queued for next release and you never told me as well earlier :)
>

No I didn't want to distract the discussion with that silly thing.

> Can you please help me getting this tested, now that I have rebased it
> as well :) ?
>

Sure, I will give it a go on my Juno. Thanks for the rebase, makes it
simpler.

--
Regards,
Sudeep

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

[Cristian Marussi]

Hi

On 21/01/2020 08:27, Viresh Kumar 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 on 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.
> 
> The transport protocols just need to provide struct scmi_transport_ops,
> with its version of the callbacks to enable exchange of SCMI messages.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

[snip]

> +/* Offset of fields within the above structure */
> +#define SHMEM_CHANNEL_STATUS		offsetof(struct scmi_shared_mem, channel_status)
> +#define SHMEM_FLAGS			offsetof(struct scmi_shared_mem, flags)
> +#define SHMEM_LENGTH			offsetof(struct scmi_shared_mem, length)
> +#define SHMEM_MSG_HEADER		offsetof(struct scmi_shared_mem, msg_header)
> +#define SHMEM_MSG_PAYLOAD		offsetof(struct scmi_shared_mem, msg_payload)
> +
> +struct scmi_info;
> +
> +/**
> + * 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
> + */

commment is obsolete
> +struct scmi_chan_info {
> +	struct scmi_info *info;
> +	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
> + */
commment is obsolete but I would also ask: are all of these operations supposed to be mandatory supported
on any possible foreseeable transport ? (beside the obviously needed like send_message)

I'm asking because they are all called straight away from the driver core without any NULL check
so that if a new transport should not need one of them it will be forced to anyway implement a dummy one
to comply, which it will be needlessly invoked every time.

> +struct scmi_transport_ops {
> +	bool (*chan_available)(struct device *dev, int idx);
> +	int (*chan_setup)(struct scmi_chan_info *cinfo, struct device *dev, bool tx);
> +	int (*chan_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);
> +	u32 (*read32)(struct scmi_chan_info *cinfo, unsigned int offset);
> +	void (*write32)(struct scmi_chan_info *cinfo, u32 val, unsigned int offset);
> +	void (*memcpy_from)(struct scmi_chan_info *cinfo, void *to, unsigned int offset, long len);
> +	void (*memcpy_to)(struct scmi_chan_info *cinfo, unsigned int offset, void *from, long len);
> +
> +};
> +
> +/**
> + * 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.
> + */
comment is obsolete
> +struct scmi_desc {
> +	struct scmi_transport_ops *ops;
> +	int max_rx_timeout_ms;
> +	int max_msg;
> +	int max_msg_size;
> +};
> +

[big snip]

>  
> -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 */ },
>  };

minor thing: shouldn't the chosen transport being configurable at compile time with some
option like CONFIG_SCMI_TRANSPORT_MBOX ? or via DT ?
(minor thing in fact since as of now we have only one transport...)

>  
> diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
> new file mode 100644
> index 000000000000..7509e7eb262a
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/mailbox.c
> @@ -0,0 +1,202 @@
> +// 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/of_address.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
> + */
comment is obsolete
> +struct scmi_mailbox {
> +	struct mbox_client cl;
> +	struct mbox_chan *chan;
> +	struct scmi_chan_info *cinfo;
> +	void __iomem *payload;
> +};
> +

[snip]

> +static void mailbox_memcpy_from(struct scmi_chan_info *cinfo, void *to,
> +				unsigned int offset, long len)
> +{
> +	struct scmi_mailbox *smbox = cinfo->transport_info;
> +
> +	memcpy_fromio(to, smbox->payload + offset, len);
> +}
> +
> +static void mailbox_memcpy_to(struct scmi_chan_info *cinfo, unsigned int offset,
> +			      void *from, long len)
> +{
> +	struct scmi_mailbox *smbox = cinfo->transport_info;
> +
> +	memcpy_toio(smbox->payload + offset, from, len);
> +}
> +
> +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,
> +	.read32 = mailbox_read32,
> +	.write32 = mailbox_write32,
> +	.memcpy_from = mailbox_memcpy_from,
> +	.memcpy_to = mailbox_memcpy_to,
> +};
> +
> +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,
> +};
> 

Regards

Cristian

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

[Viresh Kumar]

On 22-01-20, 12:44, Cristian Marussi wrote:
> On 21/01/2020 08:27, Viresh Kumar wrote:
> commment is obsolete

Right, they need to be checked everywhere again. Sorry for missing
that earlier.

> > +struct scmi_chan_info {
> > +	struct scmi_info *info;
> > +	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
> > + */
> commment is obsolete but I would also ask: are all of these operations supposed to be mandatory supported
> on any possible foreseeable transport ? (beside the obviously needed like send_message)
> 
> I'm asking because they are all called straight away from the driver core without any NULL check
> so that if a new transport should not need one of them it will be forced to anyway implement a dummy one
> to comply, which it will be needlessly invoked every time.

They are kept as mandatory for now as we don't really know how it
will look for other transport types. Lets make them optional only when
someone don't need to define them. It would be a simple change anyway.

> >  /* 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 */ },
> >  };
> 
> minor thing: shouldn't the chosen transport being configurable at compile time with some
> option like CONFIG_SCMI_TRANSPORT_MBOX ? or via DT ?

It is configurable via DT. The compatible should look different in
that case, something like: "arm,scmi-<newtranport>".

-- 
viresh

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

[Sudeep Holla]

On Wed, Jan 22, 2020 at 12:15:38PM +0000, Sudeep Holla wrote:
> On Wed, Jan 22, 2020 at 08:06:23AM +0530, Viresh Kumar wrote:
>

[...]

> > Can you please help me getting this tested, now that I have rebased it
> > as well :) ?
> >
>
> Sure, I will give it a go on my Juno. Thanks for the rebase, makes it
> simpler.
>

Sorry for the delay. I gave this a spin on my Juno. I am seeing below
warning once on boot but it continues and everything seem to work fine.
Also the warning is not related to this change I believe and this patch
is just helping to hit some corner case with deferred probe and devres.
I need to spend some time to debug it.

Regards,
Sudeep

--->8

WARNING: CPU: 1 PID: 187 at drivers/base/dd.c:519 really_probe+0x11c/0x418
Modules linked in:
CPU: 1 PID: 187 Comm: kworker/1:2 Not tainted 5.5.0-rc7-00026-gf7231cd3108d-dirty #20
Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Jan 16 2020
Workqueue: events deferred_probe_work_func
pstate: 80000005 (Nzcv daif -PAN -UAO)
pc : really_probe+0x11c/0x418
lr : really_probe+0x10c/0x418
Call trace:
 really_probe+0x11c/0x418
 driver_probe_device+0xe4/0x138
 __device_attach_driver+0x90/0x110
 bus_for_each_drv+0x80/0xd0
 __device_attach+0xdc/0x160
 device_initial_probe+0x18/0x20
 bus_probe_device+0x98/0xa0
 deferred_probe_work_func+0x90/0xe0
 process_one_work+0x1ec/0x4a8
 worker_thread+0x210/0x490
 kthread+0x110/0x118
 ret_from_fork+0x10/0x18
---[ end trace 06f96d55ce6093a8 ]---

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

[Cristian Marussi]

On 23/01/2020 02:39, Viresh Kumar wrote:
> On 22-01-20, 12:44, Cristian Marussi wrote:
>> On 21/01/2020 08:27, Viresh Kumar wrote:
>> commment is obsolete
> 
> Right, they need to be checked everywhere again. Sorry for missing
> that earlier.
> 
>>> +struct scmi_chan_info {
>>> +	struct scmi_info *info;
>>> +	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
>>> + */
>> commment is obsolete but I would also ask: are all of these operations supposed to be mandatory supported
>> on any possible foreseeable transport ? (beside the obviously needed like send_message)
>>
>> I'm asking because they are all called straight away from the driver core without any NULL check
>> so that if a new transport should not need one of them it will be forced to anyway implement a dummy one
>> to comply, which it will be needlessly invoked every time.
> 
> They are kept as mandatory for now as we don't really know how it
> will look for other transport types. Lets make them optional only when
> someone don't need to define them. It would be a simple change anyway.

Ok, fine.
> 
>>>  /* 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 */ },
>>>  };
>>
>> minor thing: shouldn't the chosen transport being configurable at compile time with some
>> option like CONFIG_SCMI_TRANSPORT_MBOX ? or via DT ?
> 
> It is configurable via DT. The compatible should look different in
> that case, something like: "arm,scmi-<newtranport>".
> 

Ah ok, we're assuming mailbox transport as the default, being the only one existing as of now.
Fine for me, thanks for the explanation.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Regards

Cristian

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

[Viresh Kumar]

On 23-01-20, 10:30, Sudeep Holla wrote:
> On Wed, Jan 22, 2020 at 12:15:38PM +0000, Sudeep Holla wrote:
> > On Wed, Jan 22, 2020 at 08:06:23AM +0530, Viresh Kumar wrote:
> >
> 
> [...]
> 
> > > Can you please help me getting this tested, now that I have rebased it
> > > as well :) ?
> > >
> >
> > Sure, I will give it a go on my Juno. Thanks for the rebase, makes it
> > simpler.
> >
> 
> Sorry for the delay. I gave this a spin on my Juno. I am seeing below
> warning once on boot but it continues and everything seem to work fine.
> Also the warning is not related to this change I believe and this patch
> is just helping to hit some corner case with deferred probe and devres.
> I need to spend some time to debug it.
> 
> Regards,
> Sudeep
> 
> --->8
> 
> WARNING: CPU: 1 PID: 187 at drivers/base/dd.c:519 really_probe+0x11c/0x418
> Modules linked in:
> CPU: 1 PID: 187 Comm: kworker/1:2 Not tainted 5.5.0-rc7-00026-gf7231cd3108d-dirty #20
> Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Jan 16 2020
> Workqueue: events deferred_probe_work_func
> pstate: 80000005 (Nzcv daif -PAN -UAO)
> pc : really_probe+0x11c/0x418
> lr : really_probe+0x10c/0x418
> Call trace:
>  really_probe+0x11c/0x418
>  driver_probe_device+0xe4/0x138
>  __device_attach_driver+0x90/0x110
>  bus_for_each_drv+0x80/0xd0
>  __device_attach+0xdc/0x160
>  device_initial_probe+0x18/0x20
>  bus_probe_device+0x98/0xa0
>  deferred_probe_work_func+0x90/0xe0
>  process_one_work+0x1ec/0x4a8
>  worker_thread+0x210/0x490
>  kthread+0x110/0x118
>  ret_from_fork+0x10/0x18
> ---[ end trace 06f96d55ce6093a8 ]---

Still it looks strange that the warning comes only after my patch :)

Should I send V5 (fixed few comments after reviews) now ?

-- 
viresh

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

[Cristian Marussi]

On 23/01/2020 11:27, Viresh Kumar wrote:
> On 23-01-20, 10:30, Sudeep Holla wrote:
>> On Wed, Jan 22, 2020 at 12:15:38PM +0000, Sudeep Holla wrote:
>>> On Wed, Jan 22, 2020 at 08:06:23AM +0530, Viresh Kumar wrote:
>>>
>>
>> [...]
>>
>>>> Can you please help me getting this tested, now that I have rebased it
>>>> as well :) ?
>>>>
>>>
>>> Sure, I will give it a go on my Juno. Thanks for the rebase, makes it
>>> simpler.
>>>
>>
>> Sorry for the delay. I gave this a spin on my Juno. I am seeing below
>> warning once on boot but it continues and everything seem to work fine.
>> Also the warning is not related to this change I believe and this patch
>> is just helping to hit some corner case with deferred probe and devres.
>> I need to spend some time to debug it.
>>
>> Regards,
>> Sudeep
>>
>> --->8
>>
>> WARNING: CPU: 1 PID: 187 at drivers/base/dd.c:519 really_probe+0x11c/0x418
>> Modules linked in:
>> CPU: 1 PID: 187 Comm: kworker/1:2 Not tainted 5.5.0-rc7-00026-gf7231cd3108d-dirty #20
>> Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Jan 16 2020
>> Workqueue: events deferred_probe_work_func
>> pstate: 80000005 (Nzcv daif -PAN -UAO)
>> pc : really_probe+0x11c/0x418
>> lr : really_probe+0x10c/0x418
>> Call trace:
>>  really_probe+0x11c/0x418
>>  driver_probe_device+0xe4/0x138
>>  __device_attach_driver+0x90/0x110
>>  bus_for_each_drv+0x80/0xd0
>>  __device_attach+0xdc/0x160
>>  device_initial_probe+0x18/0x20
>>  bus_probe_device+0x98/0xa0
>>  deferred_probe_work_func+0x90/0xe0
>>  process_one_work+0x1ec/0x4a8
>>  worker_thread+0x210/0x490
>>  kthread+0x110/0x118
>>  ret_from_fork+0x10/0x18
>> ---[ end trace 06f96d55ce6093a8 ]---
> 
> Still it looks strange that the warning comes only after my patch :)

In fact, got the same warning while testing your patch on JUNO at top of SCMI for-next.
But then everything worked fine as Sudeep said.

Thanks

Cristian

> 
> Should I send V5 (fixed few comments after reviews) now ?
> 

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

[Sudeep Holla]

On Thu, Jan 23, 2020 at 04:57:11PM +0530, Viresh Kumar wrote:

[...]
>
> Still it looks strange that the warning comes only after my patch :)
>

Yes indeed, I spent some time last evening and hopefully close, will
continue later today.

> Should I send V5 (fixed few comments after reviews) now ?

May be wait until tomorrow ? I will try to review as I try to debug the
warning.

--
Regards,
Sudeep

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

[Viresh Kumar]

On 23-01-20, 10:30, Sudeep Holla wrote:
> WARNING: CPU: 1 PID: 187 at drivers/base/dd.c:519 really_probe+0x11c/0x418
> Modules linked in:
> CPU: 1 PID: 187 Comm: kworker/1:2 Not tainted 5.5.0-rc7-00026-gf7231cd3108d-dirty #20
> Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Jan 16 2020
> Workqueue: events deferred_probe_work_func
> pstate: 80000005 (Nzcv daif -PAN -UAO)
> pc : really_probe+0x11c/0x418
> lr : really_probe+0x10c/0x418
> Call trace:
>  really_probe+0x11c/0x418
>  driver_probe_device+0xe4/0x138
>  __device_attach_driver+0x90/0x110
>  bus_for_each_drv+0x80/0xd0
>  __device_attach+0xdc/0x160
>  device_initial_probe+0x18/0x20
>  bus_probe_device+0x98/0xa0
>  deferred_probe_work_func+0x90/0xe0
>  process_one_work+0x1ec/0x4a8
>  worker_thread+0x210/0x490
>  kthread+0x110/0x118
>  ret_from_fork+0x10/0x18
> ---[ end trace 06f96d55ce6093a8 ]---

linux-next didn't had a WARN at line 519 and so I looked at the
difference between your and my branch and reached to this patch:

commit 7c35e699c88bd60734277b26962783c60e04b494
Author: Geert Uytterhoeven <geert+renesas@glider.be>
Date:   Fri Dec 6 14:22:19 2019 +0100

    driver core: Print device when resources present in really_probe()
    
    If a device already has devres items attached before probing, a warning
    backtrace is printed.  However, this backtrace does not reveal the
    offending device, leaving the user uninformed.  Furthermore, using
    WARN_ON() causes systems with panic-on-warn to reboot.
    
    Fix this by replacing the WARN_ON() by a dev_crit() message.
    Abort probing the device, to prevent doing more damage to the device's
    resources.
    
    Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
    Link: https://lore.kernel.org/r/20191206132219.28908-1-geert+renesas@glider.be
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/dd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index d811e60610d3..b25bcab2a26b 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -516,7 +516,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
        atomic_inc(&probe_count);
        pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
                 drv->bus->name, __func__, drv->name, dev_name(dev));
-       WARN_ON(!list_empty(&dev->devres_head));
+       if (!list_empty(&dev->devres_head)) {
+               dev_crit(dev, "Resources present before probing\n");
+               return -EBUSY;
+       }
 
 re_probe:
        dev->driver = drv;


-------------------------8<-------------------------

I think this defines the problem somewhat, though I wasn't able to
reproduce the problem on my platform :)

-- 
viresh

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

[Sudeep Holla]

On Fri, Jan 24, 2020 at 08:32:12AM +0530, Viresh Kumar wrote:

[...]

>
> I think this defines the problem somewhat, though I wasn't able to
> reproduce the problem on my platform :)

No worries, I found the issue. It's devm_kzalloc in mailbox_chan_setup.
You need to pin the allocation with dev the main scmi platform device and
not individual protocol specific scmi_device. Basically, change cdev to
dev in there. It fixed the issue. You have it correct later for payload
devm_ioremap. I couldn't review the change in detail yet, allow me today.
You can post next version next week, anyways too late for v5.6 ;).

Peng, Peter,

Any comments on this ? I hope you are happy with this for SMC and virtio
based transport.

--
Regards,
Sudeep

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

[Peter Hilber]

On 22.01.20 03:36, Viresh Kumar 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 on 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.
>
> The transport protocols just need to provide struct scmi_transport_ops,
> with its version of the callbacks to enable exchange of SCMI messages.

Sorry for being a bit late with my feedback.

Accessing struct scmi_shared_mem members from driver.c forces any
transport to also use the struct scmi_shared_mem layout (or at least
pretend to do so). IMHO this does not work very well for transports
which are not using a fixed memory region to transmit and receive. But I
think the current approach will still work out.

virtio transfers each message in a separate buffer, and always uses
different parts of the buffer for transmit data and receive data, which
is contrary to the assumptions of the struct scmi_shared_mem.

The virtio transport will probably have no use for the struct
scmi_shared_mem.channel_status and .flags. The check for
SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE in scmi_tx_prepare() is not required
for the virtio transport.

I would have preferred (to have an option) to use as data passing
interface to the transport just the struct scmi_xfer. A transport using
this option would not implement ops (read|write)32 and memcpy_(from|to).
The transport would also not call scmi_tx_prepare(), but instead take
data from struct scmi_xfer directly. The transport would use a modified
scmi_rx_callback() to notify that it updated the struct scmi_xfer. A
helper to derive the struct scmi_xfer * from the message header would be
extracted from scmi_rx_callback(). The scmi_xfer_poll_done() would
become an (optional) transport op.

Other remarks:

If staying with this approach, it would be more elegant to add an
abstraction through which the transport can set the
SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE bit in the struct scmi_shared_mem.

The SCMI spec allows both interrupt-based and polling-based completion
notification. The transport should be able to indicate which
notification methods it supports. The virtio transport would not want to
support polling.

> -static void scmi_rx_callback(struct mbox_client *cl, void *m)
> +void scmi_rx_callback(struct scmi_chan_info *cinfo, struct scmi_xfer *t)

scmi_rx_callback() doesn't need the struct scmi_xfer * parameter any
more ATM (and the transport might also not know about it).

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/>


[tech_days_munchen]

OpenSynergy TechDay München

am 11. Februar 2020, ab 12:00Uhr, im Studio Balan, Moosacherstr. 86.

Anmeldung bitte hier<mailto:sabine.mutumba@opensynergy.com>

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

[Jassi Brar]

On Fri, Jan 24, 2020 at 6:15 AM Peter Hilber
<peter.hilber@opensynergy.com> wrote:
....
>
> I would have preferred (to have an option) to use as data passing
> interface to the transport just the struct scmi_xfer. A transport using
> this option would not implement ops (read|write)32 and memcpy_(from|to).
> The transport would also not call scmi_tx_prepare(), but instead take
> data from struct scmi_xfer directly. The transport would use a modified
> scmi_rx_callback() to notify that it updated the struct scmi_xfer. A
> helper to derive the struct scmi_xfer * from the message header would be
> extracted from scmi_rx_callback(). The scmi_xfer_poll_done() would
> become an (optional) transport op.
>
+1
I have pointed out many times the SCMI needs to realize not every
transport layer can conform to its expectations, the scmi_xfer must
have some transport specific element to it. Or there would be
emulation/pretend modes implemented in controller drivers.