[PATCH v1 0/6] rtio_rpmsg: make rpmsg channel configurable

[PATCH v1 0/6] rtio_rpmsg: make rpmsg channel configurable

[Loic Pallardy]

This goal of this series is to offer more flexibility for virtio_rpmsg
communication link configuration.

It should be possible to tune rpmsg buffer size per Rpmsg link, to provide
selected configuration to coprocessor, to take into account coprocessor
specificities like memory region.

This series introduces an optional virtio rpmsg configuration structure, which
will be managed as an extension of struct fw_rsc_vdev present in coprocessor firmware
resource table.
Structure access is possible thanks to virtio get and set API.
This structure contains the different information to tune the link between the
host and the coprocessor.

Patch "rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a valid
kernel address" has the same goal as Wendy's RFC [1]. I don't have tested
Wendy's series with level driver buffer allocation.

Last patch "rpmsg: virtio_rpmsg: set buffer configuration to virtio" has a
dependency with remoteproc subdevice boot sequence as coprocessor resource
table must be updated before coprocessor start. See [2]

[1] http://www.spinics.net/lists/linux-remoteproc/msg00852.html
[2] http://www.spinics.net/lists/linux-remoteproc/msg00826.html

Loic Pallardy (6):
  rpmsg: virtio_rpmsg: set rpmsg_buf_size customizable
  rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a valid
    kernel address
  include: virtio_rpmsg: add virtio rpmsg configuration structure
  rpmsg: virtio_rpmsg: get buffer configuration from virtio
  rpmsg: virtio_rpmsg: don't allocate buffer if provided by low level
    driver
  rpmsg: virtio_rpmsg: set buffer configuration to virtio

 drivers/rpmsg/virtio_rpmsg_bus.c   | 176 ++++++++++++++++++++++++++++++-------
 include/linux/rpmsg/virtio_rpmsg.h |  32 +++++++
 2 files changed, 177 insertions(+), 31 deletions(-)
 create mode 100644 include/linux/rpmsg/virtio_rpmsg.h

-- 
1.9.1

[PATCH v1 1/6] rpmsg: virtio_rpmsg: set rpmsg_buf_size customizable

[Loic Pallardy]

Rpmsg buffer size is currently fixed to 512 bytes.
This patch introduces a new capability in struct virtproc_info
to tune shared buffer size between host and coprocessor
according to the needs.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 3090b0d..1f6dfc6 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -45,6 +45,7 @@
  * @rbufs:	kernel address of rx buffers
  * @sbufs:	kernel address of tx buffers
  * @num_bufs:	total number of buffers for rx and tx
+ * @buf_size:   size of one rx or tx buffer
  * @last_sbuf:	index of last tx buffer used
  * @bufs_dma:	dma base addr of the buffers
  * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent senders.
@@ -65,6 +66,7 @@ struct virtproc_info {
 	struct virtqueue *rvq, *svq;
 	void *rbufs, *sbufs;
 	unsigned int num_bufs;
+	unsigned int buf_size;
 	int last_sbuf;
 	dma_addr_t bufs_dma;
 	struct mutex tx_lock;
@@ -158,7 +160,7 @@ struct virtio_rpmsg_channel {
  * processor.
  */
 #define MAX_RPMSG_NUM_BUFS	(512)
-#define RPMSG_BUF_SIZE		(512)
+#define MAX_RPMSG_BUF_SIZE	(512)
 
 /*
  * Local addresses are dynamically allocated on-demand.
@@ -429,7 +431,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
 	 * (half of our buffers are used for sending messages)
 	 */
 	if (vrp->last_sbuf < vrp->num_bufs / 2)
-		ret = vrp->sbufs + RPMSG_BUF_SIZE * vrp->last_sbuf++;
+		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
 	/* or recycle a used one */
 	else
 		ret = virtqueue_get_buf(vrp->svq, &len);
@@ -555,7 +557,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
 	 * messaging), or to improve the buffer allocator, to support
 	 * variable-length buffer sizes.
 	 */
-	if (len > RPMSG_BUF_SIZE - sizeof(struct rpmsg_hdr)) {
+	if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
 		dev_err(dev, "message is too big (%d)\n", len);
 		return -EMSGSIZE;
 	}
@@ -696,8 +698,8 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 	 * We currently use fixed-sized buffers, so trivially sanitize
 	 * the reported payload length.
 	 */
-	if (len > RPMSG_BUF_SIZE ||
-	    msg->len > (len - sizeof(struct rpmsg_hdr))) {
+	if (len > vrp->buf_size ||
+		msg->len > (len - sizeof(struct rpmsg_hdr))) {
 		dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
 		return -EINVAL;
 	}
@@ -729,7 +731,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 		dev_warn(dev, "msg received with no recipient\n");
 
 	/* publish the real size of the buffer */
-	sg_init_one(&sg, msg, RPMSG_BUF_SIZE);
+	sg_init_one(&sg, msg, vrp->buf_size);
 
 	/* add the buffer back to the remote processor's virtqueue */
 	err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
@@ -886,7 +888,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	else
 		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
 
-	total_buf_space = vrp->num_bufs * RPMSG_BUF_SIZE;
+	total_buf_space = vrp->num_bufs * vrp->buf_size;
 
 	/* allocate coherent memory for the buffers */
 	bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
@@ -909,9 +911,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	/* set up the receive buffers */
 	for (i = 0; i < vrp->num_bufs / 2; i++) {
 		struct scatterlist sg;
-		void *cpu_addr = vrp->rbufs + i * RPMSG_BUF_SIZE;
+		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
 
-		sg_init_one(&sg, cpu_addr, RPMSG_BUF_SIZE);
+		sg_init_one(&sg, cpu_addr, vrp->buf_size);
 
 		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
 					  GFP_KERNEL);
@@ -976,7 +978,7 @@ static int rpmsg_remove_device(struct device *dev, void *data)
 static void rpmsg_remove(struct virtio_device *vdev)
 {
 	struct virtproc_info *vrp = vdev->priv;
-	size_t total_buf_space = vrp->num_bufs * RPMSG_BUF_SIZE;
+	size_t total_buf_space = vrp->num_bufs * vrp->buf_size;
 	int ret;
 
 	vdev->config->reset(vdev);
-- 
1.9.1

[PATCH v1 2/6] rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a valid kernel address

[Loic Pallardy]

To specify memory for remoteproc, we declare (dma_declare_coherent_memory())
an area which is ioremap'ed to the vmalloc area.  However, this address is
not a kernel address so virt_addr_valid(buf) fails.

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 1f6dfc6..0810d1f 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -195,6 +195,29 @@ static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
 };
 
 /**
+ * rpmsg_sg_init - initialize scatterlist according to cpu address location
+ * @sg: scatterlist to fill
+ * @cpu_addr: virtual address of the buffer
+ * @len: buffer length
+ *
+ * An internal function filling scatterlist according to virtual address
+ * location (in vmalloc or in kernel).
+ */
+static void
+rpmsg_sg_init(struct scatterlist *sg, void *cpu_addr, unsigned int len)
+{
+	if (is_vmalloc_addr(cpu_addr)) {
+		sg_init_table(sg, 1);
+		sg_set_page(sg, vmalloc_to_page(cpu_addr), len,
+			    offset_in_page(cpu_addr));
+	} else {
+		WARN_ON(!virt_addr_valid(cpu_addr));
+		sg_init_one(sg, cpu_addr, len);
+	}
+}
+
+
+/**
  * __ept_release() - deallocate an rpmsg endpoint
  * @kref: the ept's reference count
  *
@@ -606,7 +629,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
 			 msg, sizeof(*msg) + msg->len, true);
 #endif
 
-	sg_init_one(&sg, msg, sizeof(*msg) + len);
+	rpmsg_sg_init(&sg, msg, sizeof(*msg) + len);
 
 	mutex_lock(&vrp->tx_lock);
 
@@ -731,7 +754,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 		dev_warn(dev, "msg received with no recipient\n");
 
 	/* publish the real size of the buffer */
-	sg_init_one(&sg, msg, vrp->buf_size);
+	rpmsg_sg_init(&sg, msg, vrp->buf_size);
 
 	/* add the buffer back to the remote processor's virtqueue */
 	err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
@@ -913,7 +936,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 		struct scatterlist sg;
 		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
 
-		sg_init_one(&sg, cpu_addr, vrp->buf_size);
+		rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
 
 		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
 					  GFP_KERNEL);
-- 
1.9.1

[PATCH v1 3/6] include: virtio_rpmsg: add virtio rpmsg configuration structure

[Loic Pallardy]

Rpmsg channel configuration should be identical on host and
coprocessor side.
This patch proposes a new structure named struct virtio_rpmsg_cfg
to gather all configuration information to characterize the
communication link and.
This structure will be exchanged with coprocessor via the resource
table, expanding struct fw_rsc_vdev. It will guarantee that host
and coprocessor will share the same information.
virtio_rpmsg will access it thanks to virtio get and set features.
Presence of struct virtio_rpmsg_cfg is optional.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 include/linux/rpmsg/virtio_rpmsg.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 include/linux/rpmsg/virtio_rpmsg.h

diff --git a/include/linux/rpmsg/virtio_rpmsg.h b/include/linux/rpmsg/virtio_rpmsg.h
new file mode 100644
index 0000000..5f3f0d0
--- /dev/null
+++ b/include/linux/rpmsg/virtio_rpmsg.h
@@ -0,0 +1,32 @@
+
+#ifndef _LINUX_VIRTIO_RPMSG_H
+#define _LINUX_VIRTIO_RPMSG_H
+
+/* Offset in struct fw_rsc_vdev */
+#define RPMSG_CONFIG_OFFSET	0
+
+/**
+ * struct virtio_rpmsg_cfg - optional configuration field for virtio rpmsg
+ * provided at probe time by virtio (get/set)
+ * @id: virtio cfg id (as in virtio_ids.h)
+ * @version: virtio_rpmsg_cfg structure version number
+ * @va: virtual address (used when buffer allocated by low level driver)
+ * @da: device address
+ * @pa: physical address
+ * @len: length (in bytes)
+ * @buf_size: size of rpmsg buffer size (defined by firmware else default value
+ *	      used)
+ * @reserved: reserved (must be zero)
+ */
+struct virtio_rpmsg_cfg {
+	u32 id;
+	u32 version;
+	u64 va;
+	u32 da;
+	u32 pa;
+	u32 len;
+	u32 buf_size;
+	u32 reserved;
+} __packed;
+
+#endif
-- 
1.9.1

[PATCH v1 4/6] rpmsg: virtio_rpmsg: get buffer configuration from virtio

[Loic Pallardy]

Some coprocessors have memory mapping constraints which require
predefined buffer location or specific buffer size different from
default definition.
Coprocessor resources are defined in associated firmware resource table.
Remoteproc offers access to firmware resource table via virtio get
interface.

This patch modifies rpmsg_probe sequence to:
- retrieve rpmsg buffer configuration (if any)
- verify and apply configuration
- allocate buffer according to requested configuration

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 52 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 3 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 0810d1f..1a97af8 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -32,6 +32,7 @@
 #include <linux/sched.h>
 #include <linux/wait.h>
 #include <linux/rpmsg.h>
+#include <linux/rpmsg/virtio_rpmsg.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
 
@@ -871,6 +872,45 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
 	return 0;
 }
 
+static int virtio_rpmsg_get_config(struct virtio_device *vdev)
+{
+	struct virtio_rpmsg_cfg virtio_cfg;
+	struct virtproc_info *vrp = vdev->priv;
+	size_t total_buf_space;
+	int ret = 0;
+
+	memset(&virtio_cfg, 0, sizeof(virtio_cfg));
+	vdev->config->get(vdev, RPMSG_CONFIG_OFFSET, &virtio_cfg,
+			  sizeof(virtio_cfg));
+
+	if (virtio_cfg.id == VIRTIO_ID_RPMSG && virtio_cfg.version == 1 &&
+	    virtio_cfg.reserved == 0) {
+		if (virtio_cfg.buf_size <= MAX_RPMSG_BUF_SIZE) {
+			vrp->buf_size = virtio_cfg.buf_size;
+		} else {
+			WARN_ON(1);
+			dev_warn(&vdev->dev, "Requested RPMsg buffer size too big: %d\n",
+				 vrp->buf_size);
+			ret = -EINVAL;
+			goto out;
+		}
+		vrp->bufs_dma = virtio_cfg.pa;
+
+		/* Check rpmsg buffer length */
+		total_buf_space = vrp->num_bufs * vrp->buf_size;
+		if ((virtio_cfg.len != -1) && (total_buf_space > virtio_cfg.len)) {
+			dev_warn(&vdev->dev, "Not enough memory for buffers: %d\n",
+					total_buf_space);
+			ret = -ENOMEM;
+			goto out;
+		}
+		return !ret;
+	}
+out:
+	return ret;
+
+}
+
 static int rpmsg_probe(struct virtio_device *vdev)
 {
 	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
@@ -901,6 +941,8 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	vrp->rvq = vqs[0];
 	vrp->svq = vqs[1];
 
+	vdev->priv = vrp;
+
 	/* we expect symmetric tx/rx vrings */
 	WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
 		virtqueue_get_vring_size(vrp->svq));
@@ -911,9 +953,15 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	else
 		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
 
+	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
+
+	/* Try to get rpmsg configuration if any */
+	err = virtio_rpmsg_get_config(vdev);
+	if (err < 0)
+		goto free_vrp;
+
 	total_buf_space = vrp->num_bufs * vrp->buf_size;
 
-	/* allocate coherent memory for the buffers */
 	bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
 				     total_buf_space, &vrp->bufs_dma,
 				     GFP_KERNEL);
@@ -946,8 +994,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	/* suppress "tx-complete" interrupts */
 	virtqueue_disable_cb(vrp->svq);
 
-	vdev->priv = vrp;
-
 	/* if supported by the remote processor, enable the name service */
 	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
 		/* a dedicated endpoint handles the name service msgs */
-- 
1.9.1

[PATCH v1 5/6] rpmsg: virtio_rpmsg: don't allocate buffer if provided by low level driver

[Loic Pallardy]

Low level platform specific driver has the knowledge of the different
communication link constraints like fixed or secured memory region to
use for buffer allocation.

If virtual address is defined in virtio_rpmsg_cfg structure, a dedicated
memory pool buffer fitting platform requirements is available.
Rpmsg virtio layer should rely on it if its size is compliant with link
characteristics.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 54 ++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 18 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 1a97af8..b12fe2a 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -57,6 +57,7 @@
  * @sendq:	wait queue of sending contexts waiting for a tx buffers
  * @sleepers:	number of senders that are waiting for a tx buffer
  * @ns_ept:	the bus's name service endpoint
+ * @ext_buffer: buffer allocated by low level driver
  *
  * This structure stores the rpmsg state of a given virtio remote processor
  * device (there might be several virtio proc devices for each physical
@@ -76,6 +77,7 @@ struct virtproc_info {
 	wait_queue_head_t sendq;
 	atomic_t sleepers;
 	struct rpmsg_endpoint *ns_ept;
+	bool ext_buffer;
 };
 
 /* The feature bitmap for virtio rpmsg */
@@ -904,6 +906,17 @@ static int virtio_rpmsg_get_config(struct virtio_device *vdev)
 			ret = -ENOMEM;
 			goto out;
 		}
+
+		/* Level platform specific buffer driver ? */
+		if (virtio_cfg.va != -1) {
+			vrp->ext_buffer = true;
+			/* half of the buffers is dedicated for RX */
+			vrp->rbufs = (void *)(uintptr_t)virtio_cfg.va;
+
+			/* and half is dedicated for TX */
+			vrp->sbufs = (void *)(uintptr_t) virtio_cfg.va + total_buf_space / 2;
+		}
+
 		return !ret;
 	}
 out:
@@ -960,24 +973,27 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	if (err < 0)
 		goto free_vrp;
 
-	total_buf_space = vrp->num_bufs * vrp->buf_size;
+	/* Allocate buffer if none provided by low level platform driver */
+	if (!vrp->ext_buffer) {
+		total_buf_space = vrp->num_bufs * vrp->buf_size;
 
-	bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
-				     total_buf_space, &vrp->bufs_dma,
-				     GFP_KERNEL);
-	if (!bufs_va) {
-		err = -ENOMEM;
-		goto vqs_del;
-	}
+		bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
+				total_buf_space, &vrp->bufs_dma,
+				GFP_KERNEL);
+		if (!bufs_va) {
+			err = -ENOMEM;
+			goto vqs_del;
+		}
 
-	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
-		bufs_va, &vrp->bufs_dma);
+		dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
+				bufs_va, &vrp->bufs_dma);
 
-	/* half of the buffers is dedicated for RX */
-	vrp->rbufs = bufs_va;
+		/* half of the buffers is dedicated for RX */
+		vrp->rbufs = bufs_va;
 
-	/* and half is dedicated for TX */
-	vrp->sbufs = bufs_va + total_buf_space / 2;
+		/* and half is dedicated for TX */
+		vrp->sbufs = bufs_va + total_buf_space / 2;
+	}
 
 	/* set up the receive buffers */
 	for (i = 0; i < vrp->num_bufs / 2; i++) {
@@ -1028,8 +1044,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	return 0;
 
 free_coherent:
-	dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
-			  bufs_va, vrp->bufs_dma);
+	if (!vrp->ext_buffer)
+		dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
+				  bufs_va, vrp->bufs_dma);
 vqs_del:
 	vdev->config->del_vqs(vrp->vdev);
 free_vrp:
@@ -1063,8 +1080,9 @@ static void rpmsg_remove(struct virtio_device *vdev)
 
 	vdev->config->del_vqs(vrp->vdev);
 
-	dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
-			  vrp->rbufs, vrp->bufs_dma);
+	if (!vrp->ext_buffer)
+		dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
+				  vrp->rbufs, vrp->bufs_dma);
 
 	kfree(vrp);
 }
-- 
1.9.1

[PATCH v1 6/6] rpmsg: virtio_rpmsg: set buffer configuration to virtio

[Loic Pallardy]

Rpmsg is allocating buffer one dedicated communication link
with some specificity like number of buffers, size of one buffer...
These characteristics should be shared with remote coprocessor to
guarantee communication link coherency.

Proposal is to update rpmsg configuration fields in coprocessor firmware
resource table if it exists.

This is possible thanks to virtio set interface which allows to update
cfg fields of struct fw_rsc_vdev.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index b12fe2a..5527b65 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -924,6 +924,23 @@ static int virtio_rpmsg_get_config(struct virtio_device *vdev)
 
 }
 
+static void virtio_rpmsg_set_config(struct virtio_device *vdev)
+{
+	struct virtio_rpmsg_cfg virtio_cfg;
+	struct virtproc_info *vrp = vdev->priv;
+
+	/* fill virtio_cfg struct */
+	memset(&virtio_cfg, 0, sizeof(virtio_cfg));
+	virtio_cfg.id = VIRTIO_ID_RPMSG;
+	virtio_cfg.da = vrp->bufs_dma;
+	virtio_cfg.pa =	vrp->bufs_dma;
+	virtio_cfg.len = vrp->num_bufs * vrp->buf_size;
+	virtio_cfg.buf_size = vrp->buf_size;
+
+	vdev->config->set(vdev, RPMSG_CONFIG_OFFSET, &virtio_cfg,
+			  sizeof(virtio_cfg));
+}
+
 static int rpmsg_probe(struct virtio_device *vdev)
 {
 	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
@@ -934,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	int err = 0, i;
 	size_t total_buf_space;
 	bool notify;
+	bool has_cfg = false;
 
 	vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
 	if (!vrp)
@@ -973,6 +991,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	if (err < 0)
 		goto free_vrp;
 
+	if (err)
+		has_cfg = true;
+
 	/* Allocate buffer if none provided by low level platform driver */
 	if (!vrp->ext_buffer) {
 		total_buf_space = vrp->num_bufs * vrp->buf_size;
@@ -993,6 +1014,10 @@ static int rpmsg_probe(struct virtio_device *vdev)
 
 		/* and half is dedicated for TX */
 		vrp->sbufs = bufs_va + total_buf_space / 2;
+
+		/* Notify configuration to coprocessor */
+		if (has_cfg)
+			virtio_rpmsg_set_config(vdev);
 	}
 
 	/* set up the receive buffers */
-- 
1.9.1

Re: [PATCH v1 3/6] include: virtio_rpmsg: add virtio rpmsg configuration structure

[Wendy Liang]

HI Loic,

As this introduce a new virtio config. How about to add a new header
file for virtio_rpmsg in the include/uapi/linux directory just like
other virtio device driver, such as virtio_blk?

Thanks,
Wendy

On Wed, Dec 7, 2016 at 12:35 PM, Loic Pallardy <loic.pallardy@st.com> wrote:
> Rpmsg channel configuration should be identical on host and
> coprocessor side.
> This patch proposes a new structure named struct virtio_rpmsg_cfg
> to gather all configuration information to characterize the
> communication link and.
> This structure will be exchanged with coprocessor via the resource
> table, expanding struct fw_rsc_vdev. It will guarantee that host
> and coprocessor will share the same information.
> virtio_rpmsg will access it thanks to virtio get and set features.
> Presence of struct virtio_rpmsg_cfg is optional.
>
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  include/linux/rpmsg/virtio_rpmsg.h | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
>
> diff --git a/include/linux/rpmsg/virtio_rpmsg.h b/include/linux/rpmsg/virtio_rpmsg.h
> new file mode 100644
> index 0000000..5f3f0d0
> --- /dev/null
> +++ b/include/linux/rpmsg/virtio_rpmsg.h
> @@ -0,0 +1,32 @@
> +
> +#ifndef _LINUX_VIRTIO_RPMSG_H
> +#define _LINUX_VIRTIO_RPMSG_H
> +
> +/* Offset in struct fw_rsc_vdev */
> +#define RPMSG_CONFIG_OFFSET    0
> +
> +/**
> + * struct virtio_rpmsg_cfg - optional configuration field for virtio rpmsg
> + * provided at probe time by virtio (get/set)
> + * @id: virtio cfg id (as in virtio_ids.h)
> + * @version: virtio_rpmsg_cfg structure version number
> + * @va: virtual address (used when buffer allocated by low level driver)
> + * @da: device address
> + * @pa: physical address
> + * @len: length (in bytes)
> + * @buf_size: size of rpmsg buffer size (defined by firmware else default value
> + *           used)
> + * @reserved: reserved (must be zero)
> + */
> +struct virtio_rpmsg_cfg {
> +       u32 id;
> +       u32 version;
> +       u64 va;
> +       u32 da;
> +       u32 pa;
> +       u32 len;
> +       u32 buf_size;
> +       u32 reserved;
> +} __packed;
> +
> +#endif
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v1 3/6] include: virtio_rpmsg: add virtio rpmsg configuration structure

[Loic PALLARDY]

> -----Original Message-----
> From: Wendy Liang [mailto:sunnyliangjy@gmail.com]
> Sent: Thursday, December 08, 2016 11:00 PM
> To: Loic PALLARDY <loic.pallardy@st.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>; Ohad Ben-Cohen
> <ohad@wizery.com>; lee.jones@linaro.org; Patrice CHOTARD
> <patrice.chotard@st.com>; linux-remoteproc@vger.kernel.org;
> kernel@stlinux.com
> Subject: Re: [PATCH v1 3/6] include: virtio_rpmsg: add virtio rpmsg
> configuration structure
> 
> HI Loic,
> 
Hi Wendy,

> As this introduce a new virtio config. How about to add a new header file for
> virtio_rpmsg in the include/uapi/linux directory just like other virtio device
> driver, such as virtio_blk?

I put this new file in include/linux/rpmsg directory for two reasons (from my pov):
- because there is no API/struct shared with user space (only a struct shared between drivers)
- because virtio_rpmsg.c driver is in drivers/rpmsg directory and not in virtio tree. Goal was to be coherent with c file.

Bjorn, what's your view on this point?

Regards,
Loic
> 
> Thanks,
> Wendy
> 
> On Wed, Dec 7, 2016 at 12:35 PM, Loic Pallardy <loic.pallardy@st.com> wrote:
> > Rpmsg channel configuration should be identical on host and
> > coprocessor side.
> > This patch proposes a new structure named struct virtio_rpmsg_cfg to
> > gather all configuration information to characterize the communication
> > link and.
> > This structure will be exchanged with coprocessor via the resource
> > table, expanding struct fw_rsc_vdev. It will guarantee that host and
> > coprocessor will share the same information.
> > virtio_rpmsg will access it thanks to virtio get and set features.
> > Presence of struct virtio_rpmsg_cfg is optional.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  include/linux/rpmsg/virtio_rpmsg.h | 32
> > ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> >  create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
> >
> > diff --git a/include/linux/rpmsg/virtio_rpmsg.h
> > b/include/linux/rpmsg/virtio_rpmsg.h
> > new file mode 100644
> > index 0000000..5f3f0d0
> > --- /dev/null
> > +++ b/include/linux/rpmsg/virtio_rpmsg.h
> > @@ -0,0 +1,32 @@
> > +
> > +#ifndef _LINUX_VIRTIO_RPMSG_H
> > +#define _LINUX_VIRTIO_RPMSG_H
> > +
> > +/* Offset in struct fw_rsc_vdev */
> > +#define RPMSG_CONFIG_OFFSET    0
> > +
> > +/**
> > + * struct virtio_rpmsg_cfg - optional configuration field for virtio
> > +rpmsg
> > + * provided at probe time by virtio (get/set)
> > + * @id: virtio cfg id (as in virtio_ids.h)
> > + * @version: virtio_rpmsg_cfg structure version number
> > + * @va: virtual address (used when buffer allocated by low level
> > +driver)
> > + * @da: device address
> > + * @pa: physical address
> > + * @len: length (in bytes)
> > + * @buf_size: size of rpmsg buffer size (defined by firmware else default
> value
> > + *           used)
> > + * @reserved: reserved (must be zero)  */ struct virtio_rpmsg_cfg {
> > +       u32 id;
> > +       u32 version;
> > +       u64 va;
> > +       u32 da;
> > +       u32 pa;
> > +       u32 len;
> > +       u32 buf_size;
> > +       u32 reserved;
> > +} __packed;
> > +
> > +#endif
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-remoteproc" in the body of a message to
> > majordomo@vger.kernel.org More majordomo info at
> > http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 0/6] rtio_rpmsg: make rpmsg channel configurable

[loic pallardy]

On 12/07/2016 09:35 PM, Loic Pallardy wrote:
> This goal of this series is to offer more flexibility for virtio_rpmsg
> communication link configuration.
>
> It should be possible to tune rpmsg buffer size per Rpmsg link, to provide
> selected configuration to coprocessor, to take into account coprocessor
> specificities like memory region.
>
> This series introduces an optional virtio rpmsg configuration structure, which
> will be managed as an extension of struct fw_rsc_vdev present in coprocessor firmware
> resource table.
> Structure access is possible thanks to virtio get and set API.
> This structure contains the different information to tune the link between the
> host and the coprocessor.
>
> Patch "rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a valid
> kernel address" has the same goal as Wendy's RFC [1]. I don't have tested
> Wendy's series with level driver buffer allocation.
>
> Last patch "rpmsg: virtio_rpmsg: set buffer configuration to virtio" has a
> dependency with remoteproc subdevice boot sequence as coprocessor resource
> table must be updated before coprocessor start. See [2]
>
> [1] http://www.spinics.net/lists/linux-remoteproc/msg00852.html
> [2] http://www.spinics.net/lists/linux-remoteproc/msg00826.html
>

Hi Bjorn,

Gentle reminder. This series is present on ML since one month without 
update or objection. I hope to see it in your next pull request for v4.11.

Best regards,
Loic

> Loic Pallardy (6):
>   rpmsg: virtio_rpmsg: set rpmsg_buf_size customizable
>   rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a valid
>     kernel address
>   include: virtio_rpmsg: add virtio rpmsg configuration structure
>   rpmsg: virtio_rpmsg: get buffer configuration from virtio
>   rpmsg: virtio_rpmsg: don't allocate buffer if provided by low level
>     driver
>   rpmsg: virtio_rpmsg: set buffer configuration to virtio
>
>  drivers/rpmsg/virtio_rpmsg_bus.c   | 176 ++++++++++++++++++++++++++++++-------
>  include/linux/rpmsg/virtio_rpmsg.h |  32 +++++++
>  2 files changed, 177 insertions(+), 31 deletions(-)
>  create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
>

Re: [PATCH v1 0/6] rtio_rpmsg: make rpmsg channel configurable

[Suman Anna]

Hi Loic,

> 
> On 12/07/2016 09:35 PM, Loic Pallardy wrote:
>> This goal of this series is to offer more flexibility for virtio_rpmsg
>> communication link configuration.
>>
>> It should be possible to tune rpmsg buffer size per Rpmsg link, to
>> provide
>> selected configuration to coprocessor, to take into account coprocessor
>> specificities like memory region.
>>
>> This series introduces an optional virtio rpmsg configuration
>> structure, which
>> will be managed as an extension of struct fw_rsc_vdev present in
>> coprocessor firmware
>> resource table.
>> Structure access is possible thanks to virtio get and set API.
>> This structure contains the different information to tune the link
>> between the
>> host and the coprocessor.
>>
>> Patch "rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a valid
>> kernel address" has the same goal as Wendy's RFC [1]. I don't have tested
>> Wendy's series with level driver buffer allocation.
>>
>> Last patch "rpmsg: virtio_rpmsg: set buffer configuration to virtio"
>> has a
>> dependency with remoteproc subdevice boot sequence as coprocessor
>> resource
>> table must be updated before coprocessor start. See [2]
>>
>> [1] http://www.spinics.net/lists/linux-remoteproc/msg00852.html
>> [2] http://www.spinics.net/lists/linux-remoteproc/msg00826.html
>>
> 
> Hi Bjorn,
> 
> Gentle reminder. This series is present on ML since one month without
> update or objection. I hope to see it in your next pull request for v4.11.

I like the overall objective of this series. I will need some time to
test the series, but have some preliminary comments. You would mostly
need to send a v2.

regards
Suman

> 
> Best regards,
> Loic
> 
>> Loic Pallardy (6):
>>   rpmsg: virtio_rpmsg: set rpmsg_buf_size customizable
>>   rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a valid
>>     kernel address
>>   include: virtio_rpmsg: add virtio rpmsg configuration structure
>>   rpmsg: virtio_rpmsg: get buffer configuration from virtio
>>   rpmsg: virtio_rpmsg: don't allocate buffer if provided by low level
>>     driver
>>   rpmsg: virtio_rpmsg: set buffer configuration to virtio
>>
>>  drivers/rpmsg/virtio_rpmsg_bus.c   | 176
>> ++++++++++++++++++++++++++++++-------
>>  include/linux/rpmsg/virtio_rpmsg.h |  32 +++++++
>>  2 files changed, 177 insertions(+), 31 deletions(-)
>>  create mode 100644 include/linux/rpmsg/virtio_rpmsg.h
>>
> -- 
> To unsubscribe from this list: send the line "unsubscribe
> linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/6] rpmsg: virtio_rpmsg: set rpmsg_buf_size customizable

[Suman Anna]

On 12/07/2016 02:35 PM, Loic Pallardy wrote:
> Rpmsg buffer size is currently fixed to 512 bytes.
> This patch introduces a new capability in struct virtproc_info
> to tune shared buffer size between host and coprocessor
> according to the needs.

This patch breaks rpmsg functionality w.r.t bisectability. The new
buf_size variable is not initialized (is 0) in this patch while you
replace the RPMSG_BUF_SIZE with vrp->buf_size in most call sites. You
introduce the initialization in Patch 4, that should be moved up. The
ops to override can stay in Patch 4.

regards
Suman

> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 3090b0d..1f6dfc6 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -45,6 +45,7 @@
>   * @rbufs:	kernel address of rx buffers
>   * @sbufs:	kernel address of tx buffers
>   * @num_bufs:	total number of buffers for rx and tx
> + * @buf_size:   size of one rx or tx buffer
>   * @last_sbuf:	index of last tx buffer used
>   * @bufs_dma:	dma base addr of the buffers
>   * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent senders.
> @@ -65,6 +66,7 @@ struct virtproc_info {
>  	struct virtqueue *rvq, *svq;
>  	void *rbufs, *sbufs;
>  	unsigned int num_bufs;
> +	unsigned int buf_size;
>  	int last_sbuf;
>  	dma_addr_t bufs_dma;
>  	struct mutex tx_lock;
> @@ -158,7 +160,7 @@ struct virtio_rpmsg_channel {
>   * processor.
>   */
>  #define MAX_RPMSG_NUM_BUFS	(512)
> -#define RPMSG_BUF_SIZE		(512)
> +#define MAX_RPMSG_BUF_SIZE	(512)
>  
>  /*
>   * Local addresses are dynamically allocated on-demand.
> @@ -429,7 +431,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>  	 * (half of our buffers are used for sending messages)
>  	 */
>  	if (vrp->last_sbuf < vrp->num_bufs / 2)
> -		ret = vrp->sbufs + RPMSG_BUF_SIZE * vrp->last_sbuf++;
> +		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
>  	/* or recycle a used one */
>  	else
>  		ret = virtqueue_get_buf(vrp->svq, &len);
> @@ -555,7 +557,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
>  	 * messaging), or to improve the buffer allocator, to support
>  	 * variable-length buffer sizes.
>  	 */
> -	if (len > RPMSG_BUF_SIZE - sizeof(struct rpmsg_hdr)) {
> +	if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
>  		dev_err(dev, "message is too big (%d)\n", len);
>  		return -EMSGSIZE;
>  	}
> @@ -696,8 +698,8 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  	 * We currently use fixed-sized buffers, so trivially sanitize
>  	 * the reported payload length.
>  	 */
> -	if (len > RPMSG_BUF_SIZE ||
> -	    msg->len > (len - sizeof(struct rpmsg_hdr))) {
> +	if (len > vrp->buf_size ||
> +		msg->len > (len - sizeof(struct rpmsg_hdr))) {
>  		dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
>  		return -EINVAL;
>  	}
> @@ -729,7 +731,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  		dev_warn(dev, "msg received with no recipient\n");
>  
>  	/* publish the real size of the buffer */
> -	sg_init_one(&sg, msg, RPMSG_BUF_SIZE);
> +	sg_init_one(&sg, msg, vrp->buf_size);
>  
>  	/* add the buffer back to the remote processor's virtqueue */
>  	err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> @@ -886,7 +888,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	else
>  		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
>  
> -	total_buf_space = vrp->num_bufs * RPMSG_BUF_SIZE;
> +	total_buf_space = vrp->num_bufs * vrp->buf_size;
>  
>  	/* allocate coherent memory for the buffers */
>  	bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
> @@ -909,9 +911,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	/* set up the receive buffers */
>  	for (i = 0; i < vrp->num_bufs / 2; i++) {
>  		struct scatterlist sg;
> -		void *cpu_addr = vrp->rbufs + i * RPMSG_BUF_SIZE;
> +		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>  
> -		sg_init_one(&sg, cpu_addr, RPMSG_BUF_SIZE);
> +		sg_init_one(&sg, cpu_addr, vrp->buf_size);
>  
>  		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>  					  GFP_KERNEL);
> @@ -976,7 +978,7 @@ static int rpmsg_remove_device(struct device *dev, void *data)
>  static void rpmsg_remove(struct virtio_device *vdev)
>  {
>  	struct virtproc_info *vrp = vdev->priv;
> -	size_t total_buf_space = vrp->num_bufs * RPMSG_BUF_SIZE;
> +	size_t total_buf_space = vrp->num_bufs * vrp->buf_size;
>  	int ret;
>  
>  	vdev->config->reset(vdev);
> 

Re: [PATCH v1 2/6] rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a valid kernel address

[Suman Anna]

Hi Loic,

On 12/07/2016 02:35 PM, Loic Pallardy wrote:
> To specify memory for remoteproc, we declare (dma_declare_coherent_memory())
> an area which is ioremap'ed to the vmalloc area.  However, this address is
> not a kernel address so virt_addr_valid(buf) fails.
> 
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>

I like this patch compared to the old RFC series from Edgar (from the
same discussion pointed out in Wendy's RFC). With vrp->buf_size
initialized properly, I have been able to use this patch to have rpmsg
functional both with CMA pools in HIGHMEM as well as with using a
device-specific carveout.

Tested-by: Suman Anna <s-anna@ti.com>

regards
Suman

> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 1f6dfc6..0810d1f 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -195,6 +195,29 @@ static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
>  };
>  
>  /**
> + * rpmsg_sg_init - initialize scatterlist according to cpu address location
> + * @sg: scatterlist to fill
> + * @cpu_addr: virtual address of the buffer
> + * @len: buffer length
> + *
> + * An internal function filling scatterlist according to virtual address
> + * location (in vmalloc or in kernel).
> + */
> +static void
> +rpmsg_sg_init(struct scatterlist *sg, void *cpu_addr, unsigned int len)
> +{
> +	if (is_vmalloc_addr(cpu_addr)) {
> +		sg_init_table(sg, 1);
> +		sg_set_page(sg, vmalloc_to_page(cpu_addr), len,
> +			    offset_in_page(cpu_addr));
> +	} else {
> +		WARN_ON(!virt_addr_valid(cpu_addr));
> +		sg_init_one(sg, cpu_addr, len);
> +	}
> +}
> +
> +
> +/**
>   * __ept_release() - deallocate an rpmsg endpoint
>   * @kref: the ept's reference count
>   *
> @@ -606,7 +629,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
>  			 msg, sizeof(*msg) + msg->len, true);
>  #endif
>  
> -	sg_init_one(&sg, msg, sizeof(*msg) + len);
> +	rpmsg_sg_init(&sg, msg, sizeof(*msg) + len);
>  
>  	mutex_lock(&vrp->tx_lock);
>  
> @@ -731,7 +754,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  		dev_warn(dev, "msg received with no recipient\n");
>  
>  	/* publish the real size of the buffer */
> -	sg_init_one(&sg, msg, vrp->buf_size);
> +	rpmsg_sg_init(&sg, msg, vrp->buf_size);
>  
>  	/* add the buffer back to the remote processor's virtqueue */
>  	err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> @@ -913,7 +936,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  		struct scatterlist sg;
>  		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>  
> -		sg_init_one(&sg, cpu_addr, vrp->buf_size);
> +		rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
>  
>  		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>  					  GFP_KERNEL);
> 

Re: [PATCH v1 4/6] rpmsg: virtio_rpmsg: get buffer configuration from virtio

[Suman Anna]

Hi Loic,

On 12/07/2016 02:35 PM, Loic Pallardy wrote:
> Some coprocessors have memory mapping constraints which require
> predefined buffer location or specific buffer size different from
> default definition.
> Coprocessor resources are defined in associated firmware resource table.
> Remoteproc offers access to firmware resource table via virtio get
> interface.
> 
> This patch modifies rpmsg_probe sequence to:
> - retrieve rpmsg buffer configuration (if any)
> - verify and apply configuration
> - allocate buffer according to requested configuration
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 52 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 0810d1f..1a97af8 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -32,6 +32,7 @@
>  #include <linux/sched.h>
>  #include <linux/wait.h>
>  #include <linux/rpmsg.h>
> +#include <linux/rpmsg/virtio_rpmsg.h>
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
>  
> @@ -871,6 +872,45 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,
>  	return 0;
>  }
>  
> +static int virtio_rpmsg_get_config(struct virtio_device *vdev)
> +{
> +	struct virtio_rpmsg_cfg virtio_cfg;
> +	struct virtproc_info *vrp = vdev->priv;
> +	size_t total_buf_space;
> +	int ret = 0;
> +
> +	memset(&virtio_cfg, 0, sizeof(virtio_cfg));
> +	vdev->config->get(vdev, RPMSG_CONFIG_OFFSET, &virtio_cfg,
> +			  sizeof(virtio_cfg));
> +
> +	if (virtio_cfg.id == VIRTIO_ID_RPMSG && virtio_cfg.version == 1 &&
> +	    virtio_cfg.reserved == 0) {
> +		if (virtio_cfg.buf_size <= MAX_RPMSG_BUF_SIZE) {
> +			vrp->buf_size = virtio_cfg.buf_size;
> +		} else {
> +			WARN_ON(1);
> +			dev_warn(&vdev->dev, "Requested RPMsg buffer size too big: %d\n",
> +				 vrp->buf_size);
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		vrp->bufs_dma = virtio_cfg.pa;

I believe this line belongs to the next patch. It gets overwritten in
this patch anyway since you go through the normal dma allocation.

> +
> +		/* Check rpmsg buffer length */
> +		total_buf_space = vrp->num_bufs * vrp->buf_size;
> +		if ((virtio_cfg.len != -1) && (total_buf_space > virtio_cfg.len)) {
> +			dev_warn(&vdev->dev, "Not enough memory for buffers: %d\n",
> +					total_buf_space);
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +		return !ret;
> +	}
> +out:
> +	return ret;
> +
> +}
> +
>  static int rpmsg_probe(struct virtio_device *vdev)
>  {
>  	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
> @@ -901,6 +941,8 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	vrp->rvq = vqs[0];
>  	vrp->svq = vqs[1];
>  
> +	vdev->priv = vrp;
> +
>  	/* we expect symmetric tx/rx vrings */
>  	WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
>  		virtqueue_get_vring_size(vrp->svq));
> @@ -911,9 +953,15 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	else
>  		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
>  
> +	vrp->buf_size = MAX_RPMSG_BUF_SIZE;

As pointed previously, this assignment should be moved into Patch 1.

> +
> +	/* Try to get rpmsg configuration if any */
> +	err = virtio_rpmsg_get_config(vdev);
> +	if (err < 0)
> +		goto free_vrp;
> +
>  	total_buf_space = vrp->num_bufs * vrp->buf_size;
>  
> -	/* allocate coherent memory for the buffers */

don't delete the comment here, has nothing to do with the change you are
doing.

regards
Suman

>  	bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
>  				     total_buf_space, &vrp->bufs_dma,
>  				     GFP_KERNEL);
> @@ -946,8 +994,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	/* suppress "tx-complete" interrupts */
>  	virtqueue_disable_cb(vrp->svq);
>  
> -	vdev->priv = vrp;
> -
>  	/* if supported by the remote processor, enable the name service */
>  	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
>  		/* a dedicated endpoint handles the name service msgs */
> 

Re: [PATCH v1 5/6] rpmsg: virtio_rpmsg: don't allocate buffer if provided by low level driver

[Suman Anna]

On 12/07/2016 02:35 PM, Loic Pallardy wrote:
> Low level platform specific driver has the knowledge of the different
> communication link constraints like fixed or secured memory region to
> use for buffer allocation.
> 
> If virtual address is defined in virtio_rpmsg_cfg structure, a dedicated
> memory pool buffer fitting platform requirements is available.
> Rpmsg virtio layer should rely on it if its size is compliant with link
> characteristics.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 54 ++++++++++++++++++++++++++--------------
>  1 file changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 1a97af8..b12fe2a 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -57,6 +57,7 @@
>   * @sendq:	wait queue of sending contexts waiting for a tx buffers
>   * @sleepers:	number of senders that are waiting for a tx buffer
>   * @ns_ept:	the bus's name service endpoint
> + * @ext_buffer: buffer allocated by low level driver
>   *
>   * This structure stores the rpmsg state of a given virtio remote processor
>   * device (there might be several virtio proc devices for each physical
> @@ -76,6 +77,7 @@ struct virtproc_info {
>  	wait_queue_head_t sendq;
>  	atomic_t sleepers;
>  	struct rpmsg_endpoint *ns_ept;
> +	bool ext_buffer;
>  };
>  
>  /* The feature bitmap for virtio rpmsg */
> @@ -904,6 +906,17 @@ static int virtio_rpmsg_get_config(struct virtio_device *vdev)
>  			ret = -ENOMEM;
>  			goto out;
>  		}
> +
> +		/* Level platform specific buffer driver ? */
> +		if (virtio_cfg.va != -1) {
> +			vrp->ext_buffer = true;
> +			/* half of the buffers is dedicated for RX */
> +			vrp->rbufs = (void *)(uintptr_t)virtio_cfg.va;
> +
> +			/* and half is dedicated for TX */
> +			vrp->sbufs = (void *)(uintptr_t) virtio_cfg.va + total_buf_space / 2;
> +		}
> +
>  		return !ret;
>  	}
>  out:
> @@ -960,24 +973,27 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	if (err < 0)
>  		goto free_vrp;
>  
> -	total_buf_space = vrp->num_bufs * vrp->buf_size;
> +	/* Allocate buffer if none provided by low level platform driver */
> +	if (!vrp->ext_buffer) {
> +		total_buf_space = vrp->num_bufs * vrp->buf_size;
>  
> -	bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
> -				     total_buf_space, &vrp->bufs_dma,
> -				     GFP_KERNEL);
> -	if (!bufs_va) {
> -		err = -ENOMEM;
> -		goto vqs_del;
> -	}
> +		bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
> +				total_buf_space, &vrp->bufs_dma,
> +				GFP_KERNEL);

Can you run checkpatch with --strict when you resubmit the series, I
believe a few have been introduced that weren't there before.

regards
Suman

> +		if (!bufs_va) {
> +			err = -ENOMEM;
> +			goto vqs_del;
> +		}
>  
> -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
> -		bufs_va, &vrp->bufs_dma);
> +		dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
> +				bufs_va, &vrp->bufs_dma);
>  
> -	/* half of the buffers is dedicated for RX */
> -	vrp->rbufs = bufs_va;
> +		/* half of the buffers is dedicated for RX */
> +		vrp->rbufs = bufs_va;
>  
> -	/* and half is dedicated for TX */
> -	vrp->sbufs = bufs_va + total_buf_space / 2;
> +		/* and half is dedicated for TX */
> +		vrp->sbufs = bufs_va + total_buf_space / 2;
> +	}
>  
>  	/* set up the receive buffers */
>  	for (i = 0; i < vrp->num_bufs / 2; i++) {
> @@ -1028,8 +1044,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	return 0;
>  
>  free_coherent:
> -	dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
> -			  bufs_va, vrp->bufs_dma);
> +	if (!vrp->ext_buffer)
> +		dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
> +				  bufs_va, vrp->bufs_dma);
>  vqs_del:
>  	vdev->config->del_vqs(vrp->vdev);
>  free_vrp:
> @@ -1063,8 +1080,9 @@ static void rpmsg_remove(struct virtio_device *vdev)
>  
>  	vdev->config->del_vqs(vrp->vdev);
>  
> -	dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
> -			  vrp->rbufs, vrp->bufs_dma);
> +	if (!vrp->ext_buffer)
> +		dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
> +				  vrp->rbufs, vrp->bufs_dma);
>  
>  	kfree(vrp);
>  }
> 

Re: [PATCH v1 6/6] rpmsg: virtio_rpmsg: set buffer configuration to virtio

[Suman Anna]

On 12/07/2016 02:35 PM, Loic Pallardy wrote:
> Rpmsg is allocating buffer one dedicated communication link
> with some specificity like number of buffers, size of one buffer...
> These characteristics should be shared with remote coprocessor to
> guarantee communication link coherency.
> 
> Proposal is to update rpmsg configuration fields in coprocessor firmware
> resource table if it exists.
> 
> This is possible thanks to virtio set interface which allows to update
> cfg fields of struct fw_rsc_vdev.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index b12fe2a..5527b65 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -924,6 +924,23 @@ static int virtio_rpmsg_get_config(struct virtio_device *vdev)
>  
>  }
>  
> +static void virtio_rpmsg_set_config(struct virtio_device *vdev)
> +{
> +	struct virtio_rpmsg_cfg virtio_cfg;
> +	struct virtproc_info *vrp = vdev->priv;
> +
> +	/* fill virtio_cfg struct */
> +	memset(&virtio_cfg, 0, sizeof(virtio_cfg));
> +	virtio_cfg.id = VIRTIO_ID_RPMSG;
> +	virtio_cfg.da = vrp->bufs_dma;

Hmm, I would expect this to fail if the device was behind an IOMMU.

regards
Suman

> +	virtio_cfg.pa =	vrp->bufs_dma;
> +	virtio_cfg.len = vrp->num_bufs * vrp->buf_size;
> +	virtio_cfg.buf_size = vrp->buf_size;
> +
> +	vdev->config->set(vdev, RPMSG_CONFIG_OFFSET, &virtio_cfg,
> +			  sizeof(virtio_cfg));
> +}
> +
>  static int rpmsg_probe(struct virtio_device *vdev)
>  {
>  	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
> @@ -934,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	int err = 0, i;
>  	size_t total_buf_space;
>  	bool notify;
> +	bool has_cfg = false;
>  
>  	vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
>  	if (!vrp)
> @@ -973,6 +991,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	if (err < 0)
>  		goto free_vrp;
>  
> +	if (err)
> +		has_cfg = true;
> +
>  	/* Allocate buffer if none provided by low level platform driver */
>  	if (!vrp->ext_buffer) {
>  		total_buf_space = vrp->num_bufs * vrp->buf_size;
> @@ -993,6 +1014,10 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  
>  		/* and half is dedicated for TX */
>  		vrp->sbufs = bufs_va + total_buf_space / 2;
> +
> +		/* Notify configuration to coprocessor */
> +		if (has_cfg)
> +			virtio_rpmsg_set_config(vdev);
>  	}
>  
>  	/* set up the receive buffers */
> 

RE: [PATCH v1 0/6] rtio_rpmsg: make rpmsg channel configurable

[Loic PALLARDY]

> -----Original Message-----
> From: Suman Anna [mailto:s-anna@ti.com]
> Sent: Saturday, January 14, 2017 2:36 AM
> To: Loic PALLARDY <loic.pallardy@st.com>; bjorn.andersson@linaro.org;
> ohad@wizery.com; lee.jones@linaro.org; Patrice CHOTARD
> <patrice.chotard@st.com>
> Cc: linux-remoteproc@vger.kernel.org; kernel@stlinux.com
> Subject: Re: [PATCH v1 0/6] rtio_rpmsg: make rpmsg channel configurable
> 
> Hi Loic,
> 
> >
> > On 12/07/2016 09:35 PM, Loic Pallardy wrote:
> >> This goal of this series is to offer more flexibility for
> >> virtio_rpmsg communication link configuration.
> >>
> >> It should be possible to tune rpmsg buffer size per Rpmsg link, to
> >> provide selected configuration to coprocessor, to take into account
> >> coprocessor specificities like memory region.
> >>
> >> This series introduces an optional virtio rpmsg configuration
> >> structure, which will be managed as an extension of struct
> >> fw_rsc_vdev present in coprocessor firmware resource table.
> >> Structure access is possible thanks to virtio get and set API.
> >> This structure contains the different information to tune the link
> >> between the host and the coprocessor.
> >>
> >> Patch "rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a
> >> valid kernel address" has the same goal as Wendy's RFC [1]. I don't
> >> have tested Wendy's series with level driver buffer allocation.
> >>
> >> Last patch "rpmsg: virtio_rpmsg: set buffer configuration to virtio"
> >> has a
> >> dependency with remoteproc subdevice boot sequence as coprocessor
> >> resource table must be updated before coprocessor start. See [2]
> >>
> >> [1] http://www.spinics.net/lists/linux-remoteproc/msg00852.html
> >> [2] http://www.spinics.net/lists/linux-remoteproc/msg00826.html
> >>
> >
> > Hi Bjorn,
> >
> > Gentle reminder. This series is present on ML since one month without
> > update or objection. I hope to see it in your next pull request for v4.11.
> 
> I like the overall objective of this series. I will need some time to test the
> series, but have some preliminary comments. You would mostly need to
> send a v2.
> 
Hi Suman,

It will be with pleasure. Thanks for your comments. I'll update the series and send a v2.
Regards,
Loic

> regards
> Suman
> 
> >
> > Best regards,
> > Loic
> >
> >> Loic Pallardy (6):
> >>   rpmsg: virtio_rpmsg: set rpmsg_buf_size customizable
> >>   rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a valid
> >>     kernel address
> >>   include: virtio_rpmsg: add virtio rpmsg configuration structure
> >>   rpmsg: virtio_rpmsg: get buffer configuration from virtio
> >>   rpmsg: virtio_rpmsg: don't allocate buffer if provided by low level
> >>     driver
> >>   rpmsg: virtio_rpmsg: set buffer configuration to virtio
> >>
> >>  drivers/rpmsg/virtio_rpmsg_bus.c   | 176
> >> ++++++++++++++++++++++++++++++-------
> >>  include/linux/rpmsg/virtio_rpmsg.h |  32 +++++++
> >>  2 files changed, 177 insertions(+), 31 deletions(-)  create mode
> >> 100644 include/linux/rpmsg/virtio_rpmsg.h
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-remoteproc" in the body of a message to
> > majordomo@vger.kernel.org More majordomo info at
> > http://vger.kernel.org/majordomo-info.html

RE: [PATCH v1 1/6] rpmsg: virtio_rpmsg: set rpmsg_buf_size customizable

[Loic PALLARDY]

> -----Original Message-----
> From: Suman Anna [mailto:s-anna@ti.com]
> Sent: Saturday, January 14, 2017 2:40 AM
> To: Loic PALLARDY <loic.pallardy@st.com>; bjorn.andersson@linaro.org;
> ohad@wizery.com; lee.jones@linaro.org; Patrice CHOTARD
> <patrice.chotard@st.com>
> Cc: linux-remoteproc@vger.kernel.org; kernel@stlinux.com
> Subject: Re: [PATCH v1 1/6] rpmsg: virtio_rpmsg: set rpmsg_buf_size
> customizable
> 
> On 12/07/2016 02:35 PM, Loic Pallardy wrote:
> > Rpmsg buffer size is currently fixed to 512 bytes.
> > This patch introduces a new capability in struct virtproc_info to tune
> > shared buffer size between host and coprocessor according to the
> > needs.
> 
> This patch breaks rpmsg functionality w.r.t bisectability. The new buf_size
> variable is not initialized (is 0) in this patch while you replace the
> RPMSG_BUF_SIZE with vrp->buf_size in most call sites. You introduce the
> initialization in Patch 4, that should be moved up. The ops to override can
> stay in Patch 4.

Exact vrp->buf_size should be forced to MAX_RPMSG_BUF_SIZE (512) to preserve virtio_rpmsg behavior.
Thanks,
Loic
> 
> regards
> Suman
> 
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
> > b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 3090b0d..1f6dfc6 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -45,6 +45,7 @@
> >   * @rbufs:	kernel address of rx buffers
> >   * @sbufs:	kernel address of tx buffers
> >   * @num_bufs:	total number of buffers for rx and tx
> > + * @buf_size:   size of one rx or tx buffer
> >   * @last_sbuf:	index of last tx buffer used
> >   * @bufs_dma:	dma base addr of the buffers
> >   * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent
> senders.
> > @@ -65,6 +66,7 @@ struct virtproc_info {
> >  	struct virtqueue *rvq, *svq;
> >  	void *rbufs, *sbufs;
> >  	unsigned int num_bufs;
> > +	unsigned int buf_size;
> >  	int last_sbuf;
> >  	dma_addr_t bufs_dma;
> >  	struct mutex tx_lock;
> > @@ -158,7 +160,7 @@ struct virtio_rpmsg_channel {
> >   * processor.
> >   */
> >  #define MAX_RPMSG_NUM_BUFS	(512)
> > -#define RPMSG_BUF_SIZE		(512)
> > +#define MAX_RPMSG_BUF_SIZE	(512)
> >
> >  /*
> >   * Local addresses are dynamically allocated on-demand.
> > @@ -429,7 +431,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
> >  	 * (half of our buffers are used for sending messages)
> >  	 */
> >  	if (vrp->last_sbuf < vrp->num_bufs / 2)
> > -		ret = vrp->sbufs + RPMSG_BUF_SIZE * vrp->last_sbuf++;
> > +		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
> >  	/* or recycle a used one */
> >  	else
> >  		ret = virtqueue_get_buf(vrp->svq, &len); @@ -555,7 +557,7
> @@ static
> > int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
> >  	 * messaging), or to improve the buffer allocator, to support
> >  	 * variable-length buffer sizes.
> >  	 */
> > -	if (len > RPMSG_BUF_SIZE - sizeof(struct rpmsg_hdr)) {
> > +	if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
> >  		dev_err(dev, "message is too big (%d)\n", len);
> >  		return -EMSGSIZE;
> >  	}
> > @@ -696,8 +698,8 @@ static int rpmsg_recv_single(struct virtproc_info
> *vrp, struct device *dev,
> >  	 * We currently use fixed-sized buffers, so trivially sanitize
> >  	 * the reported payload length.
> >  	 */
> > -	if (len > RPMSG_BUF_SIZE ||
> > -	    msg->len > (len - sizeof(struct rpmsg_hdr))) {
> > +	if (len > vrp->buf_size ||
> > +		msg->len > (len - sizeof(struct rpmsg_hdr))) {
> >  		dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg-
> >len);
> >  		return -EINVAL;
> >  	}
> > @@ -729,7 +731,7 @@ static int rpmsg_recv_single(struct virtproc_info
> *vrp, struct device *dev,
> >  		dev_warn(dev, "msg received with no recipient\n");
> >
> >  	/* publish the real size of the buffer */
> > -	sg_init_one(&sg, msg, RPMSG_BUF_SIZE);
> > +	sg_init_one(&sg, msg, vrp->buf_size);
> >
> >  	/* add the buffer back to the remote processor's virtqueue */
> >  	err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL); @@
> > -886,7 +888,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >  	else
> >  		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
> >
> > -	total_buf_space = vrp->num_bufs * RPMSG_BUF_SIZE;
> > +	total_buf_space = vrp->num_bufs * vrp->buf_size;
> >
> >  	/* allocate coherent memory for the buffers */
> >  	bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
> > @@ -909,9 +911,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >  	/* set up the receive buffers */
> >  	for (i = 0; i < vrp->num_bufs / 2; i++) {
> >  		struct scatterlist sg;
> > -		void *cpu_addr = vrp->rbufs + i * RPMSG_BUF_SIZE;
> > +		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> >
> > -		sg_init_one(&sg, cpu_addr, RPMSG_BUF_SIZE);
> > +		sg_init_one(&sg, cpu_addr, vrp->buf_size);
> >
> >  		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
> >  					  GFP_KERNEL);
> > @@ -976,7 +978,7 @@ static int rpmsg_remove_device(struct device *dev,
> > void *data)  static void rpmsg_remove(struct virtio_device *vdev)  {
> >  	struct virtproc_info *vrp = vdev->priv;
> > -	size_t total_buf_space = vrp->num_bufs * RPMSG_BUF_SIZE;
> > +	size_t total_buf_space = vrp->num_bufs * vrp->buf_size;
> >  	int ret;
> >
> >  	vdev->config->reset(vdev);
> >

RE: [PATCH v1 5/6] rpmsg: virtio_rpmsg: don't allocate buffer if provided by low level driver

[Loic PALLARDY]

> -----Original Message-----
> From: Suman Anna [mailto:s-anna@ti.com]
> Sent: Saturday, January 14, 2017 3:38 AM
> To: Loic PALLARDY <loic.pallardy@st.com>; bjorn.andersson@linaro.org;
> ohad@wizery.com; lee.jones@linaro.org; Patrice CHOTARD
> <patrice.chotard@st.com>
> Cc: linux-remoteproc@vger.kernel.org; kernel@stlinux.com
> Subject: Re: [PATCH v1 5/6] rpmsg: virtio_rpmsg: don't allocate buffer if
> provided by low level driver
> 
> On 12/07/2016 02:35 PM, Loic Pallardy wrote:
> > Low level platform specific driver has the knowledge of the different
> > communication link constraints like fixed or secured memory region to
> > use for buffer allocation.
> >
> > If virtual address is defined in virtio_rpmsg_cfg structure, a
> > dedicated memory pool buffer fitting platform requirements is available.
> > Rpmsg virtio layer should rely on it if its size is compliant with
> > link characteristics.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 54
> > ++++++++++++++++++++++++++--------------
> >  1 file changed, 36 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
> > b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 1a97af8..b12fe2a 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -57,6 +57,7 @@
> >   * @sendq:	wait queue of sending contexts waiting for a tx buffers
> >   * @sleepers:	number of senders that are waiting for a tx buffer
> >   * @ns_ept:	the bus's name service endpoint
> > + * @ext_buffer: buffer allocated by low level driver
> >   *
> >   * This structure stores the rpmsg state of a given virtio remote processor
> >   * device (there might be several virtio proc devices for each
> > physical @@ -76,6 +77,7 @@ struct virtproc_info {
> >  	wait_queue_head_t sendq;
> >  	atomic_t sleepers;
> >  	struct rpmsg_endpoint *ns_ept;
> > +	bool ext_buffer;
> >  };
> >
> >  /* The feature bitmap for virtio rpmsg */ @@ -904,6 +906,17 @@ static
> > int virtio_rpmsg_get_config(struct virtio_device *vdev)
> >  			ret = -ENOMEM;
> >  			goto out;
> >  		}
> > +
> > +		/* Level platform specific buffer driver ? */
> > +		if (virtio_cfg.va != -1) {
> > +			vrp->ext_buffer = true;
> > +			/* half of the buffers is dedicated for RX */
> > +			vrp->rbufs = (void *)(uintptr_t)virtio_cfg.va;
> > +
> > +			/* and half is dedicated for TX */
> > +			vrp->sbufs = (void *)(uintptr_t) virtio_cfg.va +
> total_buf_space / 2;
> > +		}
> > +
> >  		return !ret;
> >  	}
> >  out:
> > @@ -960,24 +973,27 @@ static int rpmsg_probe(struct virtio_device
> *vdev)
> >  	if (err < 0)
> >  		goto free_vrp;
> >
> > -	total_buf_space = vrp->num_bufs * vrp->buf_size;
> > +	/* Allocate buffer if none provided by low level platform driver */
> > +	if (!vrp->ext_buffer) {
> > +		total_buf_space = vrp->num_bufs * vrp->buf_size;
> >
> > -	bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
> > -				     total_buf_space, &vrp->bufs_dma,
> > -				     GFP_KERNEL);
> > -	if (!bufs_va) {
> > -		err = -ENOMEM;
> > -		goto vqs_del;
> > -	}
> > +		bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
> > +				total_buf_space, &vrp->bufs_dma,
> > +				GFP_KERNEL);
> 
> Can you run checkpatch with --strict when you resubmit the series, I believe
> a few have been introduced that weren't there before.

Sure I will.
Regards,
Loic
> 
> regards
> Suman
> 
> > +		if (!bufs_va) {
> > +			err = -ENOMEM;
> > +			goto vqs_del;
> > +		}
> >
> > -	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
> > -		bufs_va, &vrp->bufs_dma);
> > +		dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
> > +				bufs_va, &vrp->bufs_dma);
> >
> > -	/* half of the buffers is dedicated for RX */
> > -	vrp->rbufs = bufs_va;
> > +		/* half of the buffers is dedicated for RX */
> > +		vrp->rbufs = bufs_va;
> >
> > -	/* and half is dedicated for TX */
> > -	vrp->sbufs = bufs_va + total_buf_space / 2;
> > +		/* and half is dedicated for TX */
> > +		vrp->sbufs = bufs_va + total_buf_space / 2;
> > +	}
> >
> >  	/* set up the receive buffers */
> >  	for (i = 0; i < vrp->num_bufs / 2; i++) { @@ -1028,8 +1044,9 @@
> > static int rpmsg_probe(struct virtio_device *vdev)
> >  	return 0;
> >
> >  free_coherent:
> > -	dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
> > -			  bufs_va, vrp->bufs_dma);
> > +	if (!vrp->ext_buffer)
> > +		dma_free_coherent(vdev->dev.parent->parent,
> total_buf_space,
> > +				  bufs_va, vrp->bufs_dma);
> >  vqs_del:
> >  	vdev->config->del_vqs(vrp->vdev);
> >  free_vrp:
> > @@ -1063,8 +1080,9 @@ static void rpmsg_remove(struct virtio_device
> > *vdev)
> >
> >  	vdev->config->del_vqs(vrp->vdev);
> >
> > -	dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
> > -			  vrp->rbufs, vrp->bufs_dma);
> > +	if (!vrp->ext_buffer)
> > +		dma_free_coherent(vdev->dev.parent->parent,
> total_buf_space,
> > +				  vrp->rbufs, vrp->bufs_dma);
> >
> >  	kfree(vrp);
> >  }
> >

RE: [PATCH v1 4/6] rpmsg: virtio_rpmsg: get buffer configuration from virtio

[Loic PALLARDY]

> -----Original Message-----
> From: Suman Anna [mailto:s-anna@ti.com]
> Sent: Saturday, January 14, 2017 3:27 AM
> To: Loic PALLARDY <loic.pallardy@st.com>; bjorn.andersson@linaro.org;
> ohad@wizery.com; lee.jones@linaro.org; Patrice CHOTARD
> <patrice.chotard@st.com>
> Cc: linux-remoteproc@vger.kernel.org; kernel@stlinux.com
> Subject: Re: [PATCH v1 4/6] rpmsg: virtio_rpmsg: get buffer configuration
> from virtio
> 
> Hi Loic,
> 
> On 12/07/2016 02:35 PM, Loic Pallardy wrote:
> > Some coprocessors have memory mapping constraints which require
> > predefined buffer location or specific buffer size different from
> > default definition.
> > Coprocessor resources are defined in associated firmware resource table.
> > Remoteproc offers access to firmware resource table via virtio get
> > interface.
> >
> > This patch modifies rpmsg_probe sequence to:
> > - retrieve rpmsg buffer configuration (if any)
> > - verify and apply configuration
> > - allocate buffer according to requested configuration
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 52
> > +++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 49 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
> > b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 0810d1f..1a97af8 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/wait.h>
> >  #include <linux/rpmsg.h>
> > +#include <linux/rpmsg/virtio_rpmsg.h>
> >  #include <linux/mutex.h>
> >  #include <linux/of_device.h>
> >
> > @@ -871,6 +872,45 @@ static int rpmsg_ns_cb(struct rpmsg_device
> *rpdev, void *data, int len,
> >  	return 0;
> >  }
> >
> > +static int virtio_rpmsg_get_config(struct virtio_device *vdev) {
> > +	struct virtio_rpmsg_cfg virtio_cfg;
> > +	struct virtproc_info *vrp = vdev->priv;
> > +	size_t total_buf_space;
> > +	int ret = 0;
> > +
> > +	memset(&virtio_cfg, 0, sizeof(virtio_cfg));
> > +	vdev->config->get(vdev, RPMSG_CONFIG_OFFSET, &virtio_cfg,
> > +			  sizeof(virtio_cfg));
> > +
> > +	if (virtio_cfg.id == VIRTIO_ID_RPMSG && virtio_cfg.version == 1 &&
> > +	    virtio_cfg.reserved == 0) {
> > +		if (virtio_cfg.buf_size <= MAX_RPMSG_BUF_SIZE) {
> > +			vrp->buf_size = virtio_cfg.buf_size;
> > +		} else {
> > +			WARN_ON(1);
> > +			dev_warn(&vdev->dev, "Requested RPMsg buffer
> size too big: %d\n",
> > +				 vrp->buf_size);
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +		vrp->bufs_dma = virtio_cfg.pa;
> 
> I believe this line belongs to the next patch. It gets overwritten in this patch
> anyway since you go through the normal dma allocation.
OK to move it in next patch

> 
> > +
> > +		/* Check rpmsg buffer length */
> > +		total_buf_space = vrp->num_bufs * vrp->buf_size;
> > +		if ((virtio_cfg.len != -1) && (total_buf_space > virtio_cfg.len))
> {
> > +			dev_warn(&vdev->dev, "Not enough memory for
> buffers: %d\n",
> > +					total_buf_space);
> > +			ret = -ENOMEM;
> > +			goto out;
> > +		}
> > +		return !ret;
> > +	}
> > +out:
> > +	return ret;
> > +
> > +}
> > +
> >  static int rpmsg_probe(struct virtio_device *vdev)  {
> >  	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
> @@
> > -901,6 +941,8 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >  	vrp->rvq = vqs[0];
> >  	vrp->svq = vqs[1];
> >
> > +	vdev->priv = vrp;
> > +
> >  	/* we expect symmetric tx/rx vrings */
> >  	WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
> >  		virtqueue_get_vring_size(vrp->svq));
> > @@ -911,9 +953,15 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >  	else
> >  		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
> >
> > +	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> 
> As pointed previously, this assignment should be moved into Patch 1.
OK

> 
> > +
> > +	/* Try to get rpmsg configuration if any */
> > +	err = virtio_rpmsg_get_config(vdev);
> > +	if (err < 0)
> > +		goto free_vrp;
> > +
> >  	total_buf_space = vrp->num_bufs * vrp->buf_size;
> >
> > -	/* allocate coherent memory for the buffers */
> 
> don't delete the comment here, has nothing to do with the change you are
> doing.
Right

Regards,
Loic
> 
> regards
> Suman
> 
> >  	bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
> >  				     total_buf_space, &vrp->bufs_dma,
> >  				     GFP_KERNEL);
> > @@ -946,8 +994,6 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >  	/* suppress "tx-complete" interrupts */
> >  	virtqueue_disable_cb(vrp->svq);
> >
> > -	vdev->priv = vrp;
> > -
> >  	/* if supported by the remote processor, enable the name service */
> >  	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
> >  		/* a dedicated endpoint handles the name service msgs */
> >

RE: [PATCH v1 6/6] rpmsg: virtio_rpmsg: set buffer configuration to virtio

[Loic PALLARDY]

> -----Original Message-----
> From: Suman Anna [mailto:s-anna@ti.com]
> Sent: Saturday, January 14, 2017 3:41 AM
> To: Loic PALLARDY <loic.pallardy@st.com>; bjorn.andersson@linaro.org;
> ohad@wizery.com; lee.jones@linaro.org; Patrice CHOTARD
> <patrice.chotard@st.com>
> Cc: linux-remoteproc@vger.kernel.org; kernel@stlinux.com
> Subject: Re: [PATCH v1 6/6] rpmsg: virtio_rpmsg: set buffer configuration to
> virtio
> 
> On 12/07/2016 02:35 PM, Loic Pallardy wrote:
> > Rpmsg is allocating buffer one dedicated communication link with some
> > specificity like number of buffers, size of one buffer...
> > These characteristics should be shared with remote coprocessor to
> > guarantee communication link coherency.
> >
> > Proposal is to update rpmsg configuration fields in coprocessor
> > firmware resource table if it exists.
> >
> > This is possible thanks to virtio set interface which allows to update
> > cfg fields of struct fw_rsc_vdev.
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
> > b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index b12fe2a..5527b65 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -924,6 +924,23 @@ static int virtio_rpmsg_get_config(struct
> > virtio_device *vdev)
> >
> >  }
> >
> > +static void virtio_rpmsg_set_config(struct virtio_device *vdev) {
> > +	struct virtio_rpmsg_cfg virtio_cfg;
> > +	struct virtproc_info *vrp = vdev->priv;
> > +
> > +	/* fill virtio_cfg struct */
> > +	memset(&virtio_cfg, 0, sizeof(virtio_cfg));
> > +	virtio_cfg.id = VIRTIO_ID_RPMSG;
> > +	virtio_cfg.da = vrp->bufs_dma;
> 
> Hmm, I would expect this to fail if the device was behind an IOMMU.
I think so. I think I did the same vring. I need to check.
Perhaps better to set virtio_cfg.da to ANY_ADDRESS (-1) and add a comment about adding IOMMU support.
What do you think?

Regards,
Loic
> 
> regards
> Suman
> 
> > +	virtio_cfg.pa =	vrp->bufs_dma;
> > +	virtio_cfg.len = vrp->num_bufs * vrp->buf_size;
> > +	virtio_cfg.buf_size = vrp->buf_size;
> > +
> > +	vdev->config->set(vdev, RPMSG_CONFIG_OFFSET, &virtio_cfg,
> > +			  sizeof(virtio_cfg));
> > +}
> > +
> >  static int rpmsg_probe(struct virtio_device *vdev)  {
> >  	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
> @@
> > -934,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >  	int err = 0, i;
> >  	size_t total_buf_space;
> >  	bool notify;
> > +	bool has_cfg = false;
> >
> >  	vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> >  	if (!vrp)
> > @@ -973,6 +991,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >  	if (err < 0)
> >  		goto free_vrp;
> >
> > +	if (err)
> > +		has_cfg = true;
> > +
> >  	/* Allocate buffer if none provided by low level platform driver */
> >  	if (!vrp->ext_buffer) {
> >  		total_buf_space = vrp->num_bufs * vrp->buf_size; @@ -
> 993,6 +1014,10
> > @@ static int rpmsg_probe(struct virtio_device *vdev)
> >
> >  		/* and half is dedicated for TX */
> >  		vrp->sbufs = bufs_va + total_buf_space / 2;
> > +
> > +		/* Notify configuration to coprocessor */
> > +		if (has_cfg)
> > +			virtio_rpmsg_set_config(vdev);
> >  	}
> >
> >  	/* set up the receive buffers */
> >

Re: [PATCH v1 6/6] rpmsg: virtio_rpmsg: set buffer configuration to virtio

[Suman Anna]

On 01/14/2017 01:36 PM, Loic PALLARDY wrote:
> 
> 
>> -----Original Message-----
>> From: Suman Anna [mailto:s-anna@ti.com]
>> Sent: Saturday, January 14, 2017 3:41 AM
>> To: Loic PALLARDY <loic.pallardy@st.com>; bjorn.andersson@linaro.org;
>> ohad@wizery.com; lee.jones@linaro.org; Patrice CHOTARD
>> <patrice.chotard@st.com>
>> Cc: linux-remoteproc@vger.kernel.org; kernel@stlinux.com
>> Subject: Re: [PATCH v1 6/6] rpmsg: virtio_rpmsg: set buffer configuration to
>> virtio
>>
>> On 12/07/2016 02:35 PM, Loic Pallardy wrote:
>>> Rpmsg is allocating buffer one dedicated communication link with some
>>> specificity like number of buffers, size of one buffer...
>>> These characteristics should be shared with remote coprocessor to
>>> guarantee communication link coherency.
>>>
>>> Proposal is to update rpmsg configuration fields in coprocessor
>>> firmware resource table if it exists.
>>>
>>> This is possible thanks to virtio set interface which allows to update
>>> cfg fields of struct fw_rsc_vdev.
>>>
>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>> ---
>>>  drivers/rpmsg/virtio_rpmsg_bus.c | 25 +++++++++++++++++++++++++
>>>  1 file changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> index b12fe2a..5527b65 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -924,6 +924,23 @@ static int virtio_rpmsg_get_config(struct
>>> virtio_device *vdev)
>>>
>>>  }
>>>
>>> +static void virtio_rpmsg_set_config(struct virtio_device *vdev) {
>>> +	struct virtio_rpmsg_cfg virtio_cfg;
>>> +	struct virtproc_info *vrp = vdev->priv;
>>> +
>>> +	/* fill virtio_cfg struct */
>>> +	memset(&virtio_cfg, 0, sizeof(virtio_cfg));
>>> +	virtio_cfg.id = VIRTIO_ID_RPMSG;
>>> +	virtio_cfg.da = vrp->bufs_dma;
>>
>> Hmm, I would expect this to fail if the device was behind an IOMMU.
> I think so. I think I did the same vring. I need to check.
> Perhaps better to set virtio_cfg.da to ANY_ADDRESS (-1) and add a comment about adding IOMMU support.
> What do you think?

Yeah, that is a better interim change.

regards
Suman

> 
> Regards,
> Loic
>>
>> regards
>> Suman
>>
>>> +	virtio_cfg.pa =	vrp->bufs_dma;
>>> +	virtio_cfg.len = vrp->num_bufs * vrp->buf_size;
>>> +	virtio_cfg.buf_size = vrp->buf_size;
>>> +
>>> +	vdev->config->set(vdev, RPMSG_CONFIG_OFFSET, &virtio_cfg,
>>> +			  sizeof(virtio_cfg));
>>> +}
>>> +
>>>  static int rpmsg_probe(struct virtio_device *vdev)  {
>>>  	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
>> @@
>>> -934,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>  	int err = 0, i;
>>>  	size_t total_buf_space;
>>>  	bool notify;
>>> +	bool has_cfg = false;
>>>
>>>  	vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
>>>  	if (!vrp)
>>> @@ -973,6 +991,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>  	if (err < 0)
>>>  		goto free_vrp;
>>>
>>> +	if (err)
>>> +		has_cfg = true;
>>> +
>>>  	/* Allocate buffer if none provided by low level platform driver */
>>>  	if (!vrp->ext_buffer) {
>>>  		total_buf_space = vrp->num_bufs * vrp->buf_size; @@ -
>> 993,6 +1014,10
>>> @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>
>>>  		/* and half is dedicated for TX */
>>>  		vrp->sbufs = bufs_va + total_buf_space / 2;
>>> +
>>> +		/* Notify configuration to coprocessor */
>>> +		if (has_cfg)
>>> +			virtio_rpmsg_set_config(vdev);
>>>  	}
>>>
>>>  	/* set up the receive buffers */
>>>
> 

Re: [PATCH v1 0/6] rtio_rpmsg: make rpmsg channel configurable

[Suman Anna]

On 01/14/2017 01:20 PM, Loic PALLARDY wrote:
> 
> 
>> -----Original Message-----
>> From: Suman Anna [mailto:s-anna@ti.com]
>> Sent: Saturday, January 14, 2017 2:36 AM
>> To: Loic PALLARDY <loic.pallardy@st.com>; bjorn.andersson@linaro.org;
>> ohad@wizery.com; lee.jones@linaro.org; Patrice CHOTARD
>> <patrice.chotard@st.com>
>> Cc: linux-remoteproc@vger.kernel.org; kernel@stlinux.com
>> Subject: Re: [PATCH v1 0/6] rtio_rpmsg: make rpmsg channel configurable
>>
>> Hi Loic,
>>
>>>
>>> On 12/07/2016 09:35 PM, Loic Pallardy wrote:
>>>> This goal of this series is to offer more flexibility for
>>>> virtio_rpmsg communication link configuration.
>>>>
>>>> It should be possible to tune rpmsg buffer size per Rpmsg link, to
>>>> provide selected configuration to coprocessor, to take into account
>>>> coprocessor specificities like memory region.
>>>>
>>>> This series introduces an optional virtio rpmsg configuration
>>>> structure, which will be managed as an extension of struct
>>>> fw_rsc_vdev present in coprocessor firmware resource table.
>>>> Structure access is possible thanks to virtio get and set API.
>>>> This structure contains the different information to tune the link
>>>> between the host and the coprocessor.
>>>>
>>>> Patch "rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a
>>>> valid kernel address" has the same goal as Wendy's RFC [1]. I don't
>>>> have tested Wendy's series with level driver buffer allocation.
>>>>
>>>> Last patch "rpmsg: virtio_rpmsg: set buffer configuration to virtio"
>>>> has a
>>>> dependency with remoteproc subdevice boot sequence as coprocessor
>>>> resource table must be updated before coprocessor start. See [2]
>>>>
>>>> [1] http://www.spinics.net/lists/linux-remoteproc/msg00852.html
>>>> [2] http://www.spinics.net/lists/linux-remoteproc/msg00826.html
>>>>
>>>
>>> Hi Bjorn,
>>>
>>> Gentle reminder. This series is present on ML since one month without
>>> update or objection. I hope to see it in your next pull request for v4.11.
>>
>> I like the overall objective of this series. I will need some time to test the
>> series, but have some preliminary comments. You would mostly need to
>> send a v2.
>>
> Hi Suman,
> 
> It will be with pleasure. Thanks for your comments. I'll update the series and send a v2.
> Regards,
> Loic

Hi Loic,

I have given this series a test spin, and so far it works fine on TI
platforms which do not have have this config (so backward compatible). I
am interested in seeing where the plumbing is done for updating the va
for this, is there any patch on the ST remoteproc that I can look at for
this.

regards
Suman

> 
>> regards
>> Suman
>>
>>>
>>> Best regards,
>>> Loic
>>>
>>>> Loic Pallardy (6):
>>>>   rpmsg: virtio_rpmsg: set rpmsg_buf_size customizable
>>>>   rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a valid
>>>>     kernel address
>>>>   include: virtio_rpmsg: add virtio rpmsg configuration structure
>>>>   rpmsg: virtio_rpmsg: get buffer configuration from virtio
>>>>   rpmsg: virtio_rpmsg: don't allocate buffer if provided by low level
>>>>     driver
>>>>   rpmsg: virtio_rpmsg: set buffer configuration to virtio
>>>>
>>>>  drivers/rpmsg/virtio_rpmsg_bus.c   | 176
>>>> ++++++++++++++++++++++++++++++-------
>>>>  include/linux/rpmsg/virtio_rpmsg.h |  32 +++++++
>>>>  2 files changed, 177 insertions(+), 31 deletions(-)  create mode
>>>> 100644 include/linux/rpmsg/virtio_rpmsg.h
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe
>>> linux-remoteproc" in the body of a message to
>>> majordomo@vger.kernel.org More majordomo info at
>>> http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH v1 0/6] rtio_rpmsg: make rpmsg channel configurable

[Loic PALLARDY]

> -----Original Message-----
> From: Suman Anna [mailto:s-anna@ti.com]
> Sent: Monday, January 16, 2017 5:03 PM
> To: Loic PALLARDY <loic.pallardy@st.com>; bjorn.andersson@linaro.org;
> ohad@wizery.com; lee.jones@linaro.org; Patrice CHOTARD
> <patrice.chotard@st.com>
> Cc: linux-remoteproc@vger.kernel.org; kernel@stlinux.com
> Subject: Re: [PATCH v1 0/6] rtio_rpmsg: make rpmsg channel configurable
> 
> On 01/14/2017 01:20 PM, Loic PALLARDY wrote:
> >
> >
> >> -----Original Message-----
> >> From: Suman Anna [mailto:s-anna@ti.com]
> >> Sent: Saturday, January 14, 2017 2:36 AM
> >> To: Loic PALLARDY <loic.pallardy@st.com>; bjorn.andersson@linaro.org;
> >> ohad@wizery.com; lee.jones@linaro.org; Patrice CHOTARD
> >> <patrice.chotard@st.com>
> >> Cc: linux-remoteproc@vger.kernel.org; kernel@stlinux.com
> >> Subject: Re: [PATCH v1 0/6] rtio_rpmsg: make rpmsg channel
> >> configurable
> >>
> >> Hi Loic,
> >>
> >>>
> >>> On 12/07/2016 09:35 PM, Loic Pallardy wrote:
> >>>> This goal of this series is to offer more flexibility for
> >>>> virtio_rpmsg communication link configuration.
> >>>>
> >>>> It should be possible to tune rpmsg buffer size per Rpmsg link, to
> >>>> provide selected configuration to coprocessor, to take into account
> >>>> coprocessor specificities like memory region.
> >>>>
> >>>> This series introduces an optional virtio rpmsg configuration
> >>>> structure, which will be managed as an extension of struct
> >>>> fw_rsc_vdev present in coprocessor firmware resource table.
> >>>> Structure access is possible thanks to virtio get and set API.
> >>>> This structure contains the different information to tune the link
> >>>> between the host and the coprocessor.
> >>>>
> >>>> Patch "rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a
> >>>> valid kernel address" has the same goal as Wendy's RFC [1]. I don't
> >>>> have tested Wendy's series with level driver buffer allocation.
> >>>>
> >>>> Last patch "rpmsg: virtio_rpmsg: set buffer configuration to virtio"
> >>>> has a
> >>>> dependency with remoteproc subdevice boot sequence as
> coprocessor
> >>>> resource table must be updated before coprocessor start. See [2]
> >>>>
> >>>> [1] http://www.spinics.net/lists/linux-remoteproc/msg00852.html
> >>>> [2] http://www.spinics.net/lists/linux-remoteproc/msg00826.html
> >>>>
> >>>
> >>> Hi Bjorn,
> >>>
> >>> Gentle reminder. This series is present on ML since one month
> >>> without update or objection. I hope to see it in your next pull request for
> v4.11.
> >>
> >> I like the overall objective of this series. I will need some time to
> >> test the series, but have some preliminary comments. You would mostly
> >> need to send a v2.
> >>
> > Hi Suman,
> >
> > It will be with pleasure. Thanks for your comments. I'll update the series
> and send a v2.
> > Regards,
> > Loic
> 
> Hi Loic,
> 
> I have given this series a test spin, and so far it works fine on TI platforms
> which do not have have this config (so backward compatible). I am interested
> in seeing where the plumbing is done for updating the va for this, is there
> any patch on the ST remoteproc that I can look at for this.
>
Hi Suman,

Sorry for my late answer. Very busy since beginning of this year.
All the plumbing is in st_remoteproc, in probe function.
All memory regions are declared as reserved memory. Probe function will parse DT reserved memory nodes to get information about fixed memory regions associated to coprocessor (memory regions are accessed by name).
Then regions accesses are granted either thanks to memremap or thanks to dma_alloc_coherent.
Next step is to update firmware resource table with memory region information (pa, da, len and va (only for rpmsg buffer))
This is done using series [1] which allows to verify and modify any part of one resource table.

st_rproc_probe sequence becomes:
rproc_alloc --> alloc "struct rproc"
          |
          V
st_rproc_parse_dt --> parse DT and detected associated memory region (code, vrings, buffer)
          |
          V
rproc_add --> (without autoboot feature) to create new remoteproc device
          |
          V
rproc_request_resource --> overload vdev resource in firmware resource table (applied by rproc_boot)
          |
          V
rproc_boot --> coprocessor with modified resource table

I need to clean up my internal code before sharing with you.

Regards,
Loic

[1] https://lkml.org/lkml/2016/10/12/466
 
> regards
> Suman
> 
> >
> >> regards
> >> Suman
> >>
> >>>
> >>> Best regards,
> >>> Loic
> >>>
> >>>> Loic Pallardy (6):
> >>>>   rpmsg: virtio_rpmsg: set rpmsg_buf_size customizable
> >>>>   rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is not a valid
> >>>>     kernel address
> >>>>   include: virtio_rpmsg: add virtio rpmsg configuration structure
> >>>>   rpmsg: virtio_rpmsg: get buffer configuration from virtio
> >>>>   rpmsg: virtio_rpmsg: don't allocate buffer if provided by low level
> >>>>     driver
> >>>>   rpmsg: virtio_rpmsg: set buffer configuration to virtio
> >>>>
> >>>>  drivers/rpmsg/virtio_rpmsg_bus.c   | 176
> >>>> ++++++++++++++++++++++++++++++-------
> >>>>  include/linux/rpmsg/virtio_rpmsg.h |  32 +++++++
> >>>>  2 files changed, 177 insertions(+), 31 deletions(-)  create mode
> >>>> 100644 include/linux/rpmsg/virtio_rpmsg.h
> >>>>
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe
> >>> linux-remoteproc" in the body of a message to
> >>> majordomo@vger.kernel.org More majordomo info at
> >>> http://vger.kernel.org/majordomo-info.html
> >