[PATCH 0/7] remoteproc: additional virtio support

[PATCH 0/7] remoteproc: additional virtio support

[Ohad Ben-Cohen]

The patch set focuses on extending remoteproc's virtio support: we're
putting behind the single rpmsg virtio device limitation, and allowing
firmwares to publish any number of virtio devices and of any type.

This allows us to reuse the existing virtio drivers with remote processor
backends.

For example, by publishing a virtio console device and hooking it up to the
logging mechanism of OMAP's SYS/BIOS (the RTOS which runs on the M3
subsystem), we get a fancy console with log messages coming from the M3
without writing any additional driver:

root@omap4430-panda:~# modprobe virtio_console
root@omap4430-panda:~# cat /dev/hvc0 
M3 Core0 init...
Hello from SYS/BIOS
copyTask 50: Entered...:
registering rpmsg-client-sample service on 50 with HOST
copyTask 51: Entered...:
registering rpmsg-proto service on 51 with HOST
registering rpmsg-omx service on 60 with HOST
copyTask 1: Received data: hello world!, len:12
copyTask 2: Received data: hello world!, len:12
copyTask 3: Received data: hello world!, len:12
...

Note: at this point, whether you can start using vanilla virtio drivers
with your remote processor strongly depends on your platform. E.g., there
are additional changes required for this to work on OMAP4 (mainly to
satisfy the M3's iommu requirements), and that's not upstream yet.
Other non-iommu remote processors might be able to use vanilla virtio
drivers though (probably DaVinci, for example, but this wasn't tested yet).

Cc: Brian Swetland <swetland@google.com>
Cc: Iliyan Malchev <malchev@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Mark Grosen <mgrosen@ti.com>
Cc: John Williams <john.williams@petalogix.com>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Loic PALLARDY <loic.pallardy@stericsson.com>
Cc: Ludovic BARRE <ludovic.barre@stericsson.com>
Cc: Omar Ramirez Luna <omar.luna@linaro.org>
Cc: Guzman Lugo Fernando <fernando.lugo@ti.com>
Cc: Anna Suman <s-anna@ti.com>
Cc: Clark Rob <rob@ti.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Saravana Kannan <skannan@codeaurora.org>
Cc: David Brown <davidb@codeaurora.org>
Cc: Kieran Bingham <kieranbingham@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>

Ohad Ben-Cohen (7):
  remoteproc: resource table overhaul
  remoteproc: remoteproc_rpmsg -> remoteproc_virtio
  remoteproc: safer boot/shutdown order
  remoteproc: remove the single rpmsg vdev limitation
  remoteproc/omap: remove the mbox_callback limitation
  remoteproc: remove the hardcoded vring alignment
  remoteproc: cleanup resource table parsing paths

 Documentation/remoteproc.txt                       |  136 +++---
 drivers/remoteproc/Makefile                        |    2 +-
 drivers/remoteproc/omap_remoteproc.c               |   11 +-
 drivers/remoteproc/remoteproc_core.c               |  524 ++++++++++++--------
 drivers/remoteproc/remoteproc_internal.h           |    6 +-
 .../{remoteproc_rpmsg.c => remoteproc_virtio.c}    |  162 +++----
 include/linux/remoteproc.h                         |  339 ++++++++++---
 7 files changed, 747 insertions(+), 433 deletions(-)
 rename drivers/remoteproc/{remoteproc_rpmsg.c => remoteproc_virtio.c} (65%)

-- 
1.7.5.4

[PATCH 1/7] remoteproc: resource table overhaul

[Ohad Ben-Cohen]

The resource table is an array of 'struct fw_resource' members, where
each resource entry is expressed as a single member of that array.

This approach got us this far, but it has a few drawbacks:

1. Different resource entries end up overloading the same members of 'struct
   fw_resource' with different meanings. The resulting code is error prone
   and hard to read and maintain.

2. It's impossible to extend 'struct fw_resource' without breaking the
   existing firmware images (and we already want to: we can't introduce the
   new virito device resource entry with the current scheme).

3. It doesn't scale: 'struct fw_resource' must be as big as the largest
   resource entry type. As a result, smaller resource entries end up
   utilizing only small part of it.

This is fixed by defining a dedicated structure for every resource type,
and then converting the resource table to a list of type-value members.
Instead of a rigid array of homogeneous structs, the resource table
is turned into a collection of heterogeneous structures.

This way:
1. Resource entries consume exactly the amount of bytes they need.
2. It's easy to extend: just create a new resource entry structure, and assign
   it a new type.
3. The code is easier to read and maintain: the structures' members names are
   meaningful.

While we're at it, this patch has several other resource table changes:
1. The resource table gains a simple header which contains the
   number of entries in the table and their offsets within the table. This
   makes the parsing code simpler and easier to read.
2. A version member is added to the resource table. Should we change the
   format again, we'll bump up this version to prevent breakage with
   existing firmware images.
3. The VRING and VIRTIO_DEV resource entries are combined to a single
   VDEV entry. This paves the way to supporting multiple VDEV entries.
4. Since we don't really support 64-bit rprocs yet, convert two stray u64
   members to u32.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Brian Swetland <swetland@google.com>
Cc: Iliyan Malchev <malchev@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Mark Grosen <mgrosen@ti.com>
Cc: John Williams <john.williams@petalogix.com>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Loic PALLARDY <loic.pallardy@stericsson.com>
Cc: Ludovic BARRE <ludovic.barre@stericsson.com>
Cc: Omar Ramirez Luna <omar.luna@linaro.org>
Cc: Guzman Lugo Fernando <fernando.lugo@ti.com>
Cc: Anna Suman <s-anna@ti.com>
Cc: Clark Rob <rob@ti.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Saravana Kannan <skannan@codeaurora.org>
Cc: David Brown <davidb@codeaurora.org>
Cc: Kieran Bingham <kieranbingham@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
---
 Documentation/remoteproc.txt         |  127 +++++++--------
 drivers/remoteproc/remoteproc_core.c |  306 +++++++++++++++++++++++-----------
 include/linux/remoteproc.h           |  289 ++++++++++++++++++++++++++------
 3 files changed, 505 insertions(+), 217 deletions(-)

diff --git a/Documentation/remoteproc.txt b/Documentation/remoteproc.txt
index 23ff734..07057ca 100644
--- a/Documentation/remoteproc.txt
+++ b/Documentation/remoteproc.txt
@@ -221,43 +221,52 @@ resource entries that publish the existence of supported features
 or configurations by the remote processor, such as trace buffers and
 supported virtio devices (and their configurations).
 
-Currently the resource table is just an array of:
+The resource table begins with this header:
 
 /**
- * struct fw_resource - describes an entry from the resource section
+ * struct resource_table - firmware resource table header
+ * @ver: version number
+ * @num: number of resource entries
+ * @reserved: reserved (must be zero)
+ * @offset: array of offsets pointing at the various resource entries
+ *
+ * The header of the resource table, as expressed by this structure,
+ * contains a version number (should we need to change this format in the
+ * future), the number of available resource entries, and their offsets
+ * in the table.
+ */
+struct resource_table {
+	u32 ver;
+	u32 num;
+	u32 reserved[2];
+	u32 offset[0];
+} __packed;
+
+Immediately following this header are the resource entries themselves,
+each of which begins with the following resource entry header:
+
+/**
+ * struct fw_rsc_hdr - firmware resource entry header
  * @type: resource type
- * @id: index number of the resource
- * @da: device address of the resource
- * @pa: physical address of the resource
- * @len: size, in bytes, of the resource
- * @flags: properties of the resource, e.g. iommu protection required
- * @reserved: must be 0 atm
- * @name: name of resource
+ * @data: resource data
+ *
+ * Every resource entry begins with a 'struct fw_rsc_hdr' header providing
+ * its @type. The content of the entry itself will immediately follow
+ * this header, and it should be parsed according to the resource type.
  */
-struct fw_resource {
+struct fw_rsc_hdr {
 	u32 type;
-	u32 id;
-	u64 da;
-	u64 pa;
-	u32 len;
-	u32 flags;
-	u8 reserved[16];
-	u8 name[48];
+	u8 data[0];
 } __packed;
 
 Some resources entries are mere announcements, where the host is informed
 of specific remoteproc configuration. Other entries require the host to
-do something (e.g. reserve a requested resource) and possibly also reply
-by overwriting a member inside 'struct fw_resource' with info about the
-allocated resource.
-
-Different resource entries use different members of this struct,
-with different meanings. This is pretty limiting and error-prone,
-so the plan is to move to variable-length TLV-based resource entries,
-where each resource will begin with a type and length fields, followed by
-its own specific structure.
+do something (e.g. allocate a system resource). Sometimes a negotiation
+is expected, where the firmware requests a resource, and once allocated,
+the host should provide back its details (e.g. address of an allocated
+memory region).
 
-Here are the resource types that are currently being used:
+Here are the various resource types that are currently supported:
 
 /**
  * enum fw_resource_type - types of resource entries
@@ -266,59 +275,45 @@ Here are the resource types that are currently being used:
  *		    memory region.
  * @RSC_DEVMEM:     request to iommu_map a memory-based peripheral.
  * @RSC_TRACE:	    announces the availability of a trace buffer into which
- *		    the remote processor will be writing logs. In this case,
- *		    'da' indicates the device address where logs are written to,
- *		    and 'len' is the size of the trace buffer.
- * @RSC_VRING:	    request for allocation of a virtio vring (address should
- *		    be indicated in 'da', and 'len' should contain the number
- *		    of buffers supported by the vring).
- * @RSC_VIRTIO_DEV: announces support for a virtio device, and serves as
- *		    the virtio header. 'da' contains the virtio device
- *		    features, 'pa' holds the virtio guest features (host
- *		    will write them here after they're negotiated), 'len'
- *		    holds the virtio status, and 'flags' holds the virtio
- *		    device id (currently only VIRTIO_ID_RPMSG is supported).
+ *		    the remote processor will be writing logs.
+ * @RSC_VDEV:       declare support for a virtio device, and serve as its
+ *		    virtio header.
+ * @RSC_LAST:       just keep this one at the end
+ *
+ * Please note that these values are used as indices to the rproc_handle_rsc
+ * lookup table, so please keep them sane. Moreover, @RSC_LAST is used to
+ * check the validity of an index before the lookup table is accessed, so
+ * please update it as needed.
  */
 enum fw_resource_type {
 	RSC_CARVEOUT	= 0,
 	RSC_DEVMEM	= 1,
 	RSC_TRACE	= 2,
-	RSC_VRING	= 3,
-	RSC_VIRTIO_DEV	= 4,
-	RSC_VIRTIO_CFG	= 5,
+	RSC_VDEV	= 3,
+	RSC_LAST	= 4,
 };
 
-Most of the resource entries share the basic idea of address/length
-negotiation with the host: the firmware usually asks for memory
-of size 'len' bytes, and the host needs to allocate it and provide
-the device/physical address (when relevant) in 'da'/'pa' respectively.
-
-If the firmware is compiled with hard coded device addresses, and
-can't handle dynamically allocated 'da' values, then the 'da' field
-will contain the expected device addresses (today we actually only support
-this scheme, as there aren't yet any use cases for dynamically allocated
-device addresses).
+For more details regarding a specific resource type, please see its
+dedicated structure in include/linux/remoteproc.h.
 
 We also expect that platform-specific resource entries will show up
-at some point. When that happens, we could easily add a new RSC_PLAFORM
+at some point. When that happens, we could easily add a new RSC_PLATFORM
 type, and hand those resources to the platform-specific rproc driver to handle.
 
 7. Virtio and remoteproc
 
 The firmware should provide remoteproc information about virtio devices
-that it supports, and their configurations: a RSC_VIRTIO_DEV resource entry
-should specify the virtio device id, and subsequent RSC_VRING resource entries
-should indicate the vring size (i.e. how many buffers do they support) and
-where should they be mapped (i.e. which device address). Note: the alignment
-between the consumer and producer parts of the vring is assumed to be 4096.
-
-At this point we only support a single virtio rpmsg device per remote
-processor, but the plan is to remove this limitation. In addition, once we
-move to TLV-based resource table, the plan is to have a single RSC_VIRTIO
-entry per supported virtio device, which will include the virtio header,
-the vrings information and the virtio config space.
-
-Of course, RSC_VIRTIO resource entries are only good enough for static
+that it supports, and their configurations: a RSC_VDEV resource entry
+should specify the virtio device id (as in virtio_ids.h), virtio features,
+virtio config space, vrings information, etc.
+
+When a new remote processor is registered, the remoteproc framework
+will look for its resource table and will register the virtio devices
+it supports. A firmware may support any number of virtio devices, and
+of any type (a single remote processor can also easily support several
+rpmsg virtio devices this way, if desired).
+
+Of course, RSC_VDEV resource entries are only good enough for static
 allocation of virtio devices. Dynamic allocations will also be made possible
 using the rpmsg bus (similar to how we already do dynamic allocations of
 rpmsg channels; read more about it in rpmsg.txt).
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 8990c51..1034845 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -63,9 +63,8 @@ static void klist_rproc_put(struct klist_node *n);
 static DEFINE_KLIST(rprocs, klist_rproc_get, klist_rproc_put);
 
 typedef int (*rproc_handle_resources_t)(struct rproc *rproc,
-				struct fw_resource *rsc, int len);
-typedef int (*rproc_handle_resource_t)(struct rproc *rproc,
-				struct fw_resource *rsc);
+				struct resource_table *table, int len);
+typedef int (*rproc_handle_resource_t)(struct rproc *rproc, void *, int avail);
 
 /*
  * This is the IOMMU fault handler we register with the IOMMU API
@@ -281,9 +280,10 @@ rproc_load_segments(struct rproc *rproc, const u8 *elf_data, size_t len)
 }
 
 /**
- * rproc_handle_virtio_hdr() - handle a virtio header resource
+ * rproc_handle_early_vdev() - early handle a virtio header resource
  * @rproc: the remote processor
  * @rsc: the resource descriptor
+ * @avail: size of available data (for sanity checking the image)
  *
  * The existence of this virtio hdr resource entry means that the firmware
  * of this @rproc supports this virtio device.
@@ -291,37 +291,32 @@ rproc_load_segments(struct rproc *rproc, const u8 *elf_data, size_t len)
  * Currently we support only a single virtio device of type VIRTIO_ID_RPMSG,
  * but the plan is to remove this limitation and support any number
  * of virtio devices (and of any type). We'll also add support for dynamically
- * adding (and removing) virtio devices over the rpmsg bus, but small
+ * adding (and removing) virtio devices over the rpmsg bus, but simple
  * firmwares that doesn't want to get involved with rpmsg will be able
- * to simple use the resource table for this.
- *
- * At this point this virtio header entry is rather simple: it just
- * announces the virtio device id and the supported virtio device features.
- * The plan though is to extend this to include the vring information and
- * the virtio config space, too (but first, some resource table overhaul
- * is needed: move from fixed-sized to variable-length TLV entries).
- *
- * For now, the 'flags' member of the resource entry contains the virtio
- * device id, the 'da' member contains the device features, and 'pa' is
- * where we need to store the guest features once negotiation completes.
- * As usual, the 'id' member of this resource contains the index of this
- * resource type (i.e. is this the first virtio hdr entry, the 2nd, ...).
+ * to simply use the resource table for this.
  *
  * Returns 0 on success, or an appropriate error code otherwise
  */
-static int rproc_handle_virtio_hdr(struct rproc *rproc, struct fw_resource *rsc)
+static int rproc_handle_early_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
+								int avail)
 {
 	struct rproc_vdev *rvdev;
 
+	/* make sure resource isn't truncated */
+	if (sizeof(*rsc) > avail) {
+		dev_err(rproc->dev, "vdev rsc is truncated\n");
+		return -EINVAL;
+	}
+
 	/* we only support VIRTIO_ID_RPMSG devices for now */
-	if (rsc->flags != VIRTIO_ID_RPMSG) {
-		dev_warn(rproc->dev, "unsupported vdev: %d\n", rsc->flags);
+	if (rsc->id != VIRTIO_ID_RPMSG) {
+		dev_warn(rproc->dev, "unsupported vdev: %d\n", rsc->id);
 		return -EINVAL;
 	}
 
 	/* we only support a single vdev per rproc for now */
-	if (rsc->id || rproc->rvdev) {
-		dev_warn(rproc->dev, "redundant vdev entry: %s\n", rsc->name);
+	if (rproc->rvdev) {
+		dev_warn(rproc->dev, "redundant vdev entry\n");
 		return -EINVAL;
 	}
 
@@ -330,7 +325,7 @@ static int rproc_handle_virtio_hdr(struct rproc *rproc, struct fw_resource *rsc)
 		return -ENOMEM;
 
 	/* remember the device features */
-	rvdev->dfeatures = rsc->da;
+	rvdev->dfeatures = rsc->dfeatures;
 
 	rproc->rvdev = rvdev;
 	rvdev->rproc = rproc;
@@ -339,9 +334,10 @@ static int rproc_handle_virtio_hdr(struct rproc *rproc, struct fw_resource *rsc)
 }
 
 /**
- * rproc_handle_vring() - handle a vring fw resource
+ * rproc_handle_vdev() - handle a vdev fw resource
  * @rproc: the remote processor
  * @rsc: the vring resource descriptor
+ * @avail: size of available data (for sanity checking the image)
  *
  * This resource entry requires allocation of non-cacheable memory
  * for a virtio vring. Currently we only support two vrings per remote
@@ -360,57 +356,82 @@ static int rproc_handle_virtio_hdr(struct rproc *rproc, struct fw_resource *rsc)
  *
  * Returns 0 on success, or an appropriate error code otherwise
  */
-static int rproc_handle_vring(struct rproc *rproc, struct fw_resource *rsc)
+static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
+								int avail)
 {
 	struct device *dev = rproc->dev;
 	struct rproc_vdev *rvdev = rproc->rvdev;
-	dma_addr_t dma;
-	int size, id = rsc->id;
-	void *va;
+	int i;
 
-	/* no vdev is in place ? */
-	if (!rvdev) {
-		dev_err(dev, "vring requested without a virtio dev entry\n");
+	/* make sure resource isn't truncated */
+	if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct fw_rsc_vdev_vring)
+			+ rsc->config_len > avail) {
+		dev_err(rproc->dev, "vdev rsc is truncated\n");
 		return -EINVAL;
 	}
 
-	/* the firmware must provide the expected queue size */
-	if (!rsc->len) {
-		dev_err(dev, "missing expected queue size\n");
+	/* make sure reserved bytes are zeroes */
+	if (rsc->reserved[0] || rsc->reserved[1]) {
+		dev_err(dev, "vdev rsc has non zero reserved bytes\n");
 		return -EINVAL;
 	}
 
-	/* we currently support two vrings per rproc (for rx and tx) */
-	if (id >= ARRAY_SIZE(rvdev->vring)) {
-		dev_err(dev, "%s: invalid vring id %d\n", rsc->name, id);
+	dev_dbg(dev, "vdev rsc: id %d, dfeatures %x, cfg len %d, %d vrings\n",
+		rsc->id, rsc->dfeatures, rsc->config_len, rsc->num_of_vrings);
+
+	/* no vdev is in place ? */
+	if (!rvdev) {
+		dev_err(dev, "vring requested without a virtio dev entry\n");
 		return -EINVAL;
 	}
 
-	/* have we already allocated this vring id ? */
-	if (rvdev->vring[id].len) {
-		dev_err(dev, "%s: duplicated id %d\n", rsc->name, id);
+	/* we currently support two vrings per rproc (for rx and tx) */
+	if (rsc->num_of_vrings != ARRAY_SIZE(rvdev->vring)) {
+		dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
 		return -EINVAL;
 	}
 
-	/* actual size of vring (in bytes) */
-	size = PAGE_ALIGN(vring_size(rsc->len, AMP_VRING_ALIGN));
+	/* initialize the vrings */
+	for (i = 0; i < rsc->num_of_vrings; i++) {
+		struct fw_rsc_vdev_vring *vring = &rsc->vring[i];
+		dma_addr_t dma;
+		int size;
+		void *va;
+
+		/* make sure reserved bytes are zeroes */
+		if (vring->reserved) {
+			dev_err(dev, "vring rsc has non zero reserved bytes\n");
+			return -EINVAL;
+		}
 
-	/*
-	 * Allocate non-cacheable memory for the vring. In the future
-	 * this call will also configure the IOMMU for us
-	 */
-	va = dma_alloc_coherent(dev, size, &dma, GFP_KERNEL);
-	if (!va) {
-		dev_err(dev, "dma_alloc_coherent failed\n");
-		return -ENOMEM;
-	}
+		/* the firmware must provide the expected queue size */
+		if (!vring->num) {
+			dev_err(dev, "missing expected queue size\n");
+			/* potential cleanups are taken care of later on */
+			return -EINVAL;
+		}
 
-	dev_dbg(dev, "vring%d: va %p dma %x qsz %d ring size %x\n", id, va,
-					dma, rsc->len, size);
+		/* actual size of vring (in bytes) */
+		size = PAGE_ALIGN(vring_size(vring->num, AMP_VRING_ALIGN));
 
-	rvdev->vring[id].len = rsc->len;
-	rvdev->vring[id].va = va;
-	rvdev->vring[id].dma = dma;
+		/*
+		 * Allocate non-cacheable memory for the vring. In the future
+		 * this call will also configure the IOMMU for us
+		 */
+		va = dma_alloc_coherent(dev, size, &dma, GFP_KERNEL);
+		if (!va) {
+			dev_err(dev, "dma_alloc_coherent failed\n");
+			/* potential cleanups are taken care of later on */
+			return -EINVAL;
+		}
+
+		dev_dbg(dev, "vring%d: va %p dma %x qsz %d ring size %x\n", i,
+						va, dma, vring->num, size);
+
+		rvdev->vring[i].len = vring->num;
+		rvdev->vring[i].va = va;
+		rvdev->vring[i].dma = dma;
+	}
 
 	return 0;
 }
@@ -419,6 +440,7 @@ static int rproc_handle_vring(struct rproc *rproc, struct fw_resource *rsc)
  * rproc_handle_trace() - handle a shared trace buffer resource
  * @rproc: the remote processor
  * @rsc: the trace resource descriptor
+ * @avail: size of available data (for sanity checking the image)
  *
  * In case the remote processor dumps trace logs into memory,
  * export it via debugfs.
@@ -430,13 +452,25 @@ static int rproc_handle_vring(struct rproc *rproc, struct fw_resource *rsc)
  *
  * Returns 0 on success, or an appropriate error code otherwise
  */
-static int rproc_handle_trace(struct rproc *rproc, struct fw_resource *rsc)
+static int rproc_handle_trace(struct rproc *rproc, struct fw_rsc_trace *rsc,
+								int avail)
 {
 	struct rproc_mem_entry *trace;
 	struct device *dev = rproc->dev;
 	void *ptr;
 	char name[15];
 
+	if (sizeof(*rsc) > avail) {
+		dev_err(rproc->dev, "trace rsc is truncated\n");
+		return -EINVAL;
+	}
+
+	/* make sure reserved bytes are zeroes */
+	if (rsc->reserved) {
+		dev_err(dev, "trace rsc has non zero reserved bytes\n");
+		return -EINVAL;
+	}
+
 	/* what's the kernel address of this resource ? */
 	ptr = rproc_da_to_va(rproc, rsc->da, rsc->len);
 	if (!ptr) {
@@ -469,7 +503,7 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_resource *rsc)
 
 	rproc->num_traces++;
 
-	dev_dbg(dev, "%s added: va %p, da 0x%llx, len 0x%x\n", name, ptr,
+	dev_dbg(dev, "%s added: va %p, da 0x%x, len 0x%x\n", name, ptr,
 						rsc->da, rsc->len);
 
 	return 0;
@@ -479,6 +513,7 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_resource *rsc)
  * rproc_handle_devmem() - handle devmem resource entry
  * @rproc: remote processor handle
  * @rsc: the devmem resource entry
+ * @avail: size of available data (for sanity checking the image)
  *
  * Remote processors commonly need to access certain on-chip peripherals.
  *
@@ -499,7 +534,8 @@ static int rproc_handle_trace(struct rproc *rproc, struct fw_resource *rsc)
  * and not allow firmwares to request access to physical addresses that
  * are outside those ranges.
  */
-static int rproc_handle_devmem(struct rproc *rproc, struct fw_resource *rsc)
+static int rproc_handle_devmem(struct rproc *rproc, struct fw_rsc_devmem *rsc,
+								int avail)
 {
 	struct rproc_mem_entry *mapping;
 	int ret;
@@ -508,6 +544,17 @@ static int rproc_handle_devmem(struct rproc *rproc, struct fw_resource *rsc)
 	if (!rproc->domain)
 		return -EINVAL;
 
+	if (sizeof(*rsc) > avail) {
+		dev_err(rproc->dev, "devmem rsc is truncated\n");
+		return -EINVAL;
+	}
+
+	/* make sure reserved bytes are zeroes */
+	if (rsc->reserved) {
+		dev_err(rproc->dev, "devmem rsc has non zero reserved bytes\n");
+		return -EINVAL;
+	}
+
 	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
 	if (!mapping) {
 		dev_err(rproc->dev, "kzalloc mapping failed\n");
@@ -531,7 +578,7 @@ static int rproc_handle_devmem(struct rproc *rproc, struct fw_resource *rsc)
 	mapping->len = rsc->len;
 	list_add_tail(&mapping->node, &rproc->mappings);
 
-	dev_dbg(rproc->dev, "mapped devmem pa 0x%llx, da 0x%llx, len 0x%x\n",
+	dev_dbg(rproc->dev, "mapped devmem pa 0x%x, da 0x%x, len 0x%x\n",
 					rsc->pa, rsc->da, rsc->len);
 
 	return 0;
@@ -545,6 +592,7 @@ out:
  * rproc_handle_carveout() - handle phys contig memory allocation requests
  * @rproc: rproc handle
  * @rsc: the resource entry
+ * @avail: size of available data (for image validation)
  *
  * This function will handle firmware requests for allocation of physically
  * contiguous memory regions.
@@ -558,7 +606,8 @@ out:
  * needed to map it (in case @rproc is using an IOMMU). Reducing the TLB
  * pressure is important; it may have a substantial impact on performance.
  */
-static int rproc_handle_carveout(struct rproc *rproc, struct fw_resource *rsc)
+static int rproc_handle_carveout(struct rproc *rproc,
+				struct fw_rsc_carveout *rsc, int avail)
 {
 	struct rproc_mem_entry *carveout, *mapping;
 	struct device *dev = rproc->dev;
@@ -566,6 +615,20 @@ static int rproc_handle_carveout(struct rproc *rproc, struct fw_resource *rsc)
 	void *va;
 	int ret;
 
+	if (sizeof(*rsc) > avail) {
+		dev_err(rproc->dev, "carveout rsc is truncated\n");
+		return -EINVAL;
+	}
+
+	/* make sure reserved bytes are zeroes */
+	if (rsc->reserved) {
+		dev_err(dev, "carveout rsc has non zero reserved bytes\n");
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "carveout rsc: da %x, pa %x, len %x, flags %x\n",
+			rsc->da, rsc->pa, rsc->len, rsc->flags);
+
 	mapping = kzalloc(sizeof(*mapping), GFP_KERNEL);
 	if (!mapping) {
 		dev_err(dev, "kzalloc mapping failed\n");
@@ -624,7 +687,7 @@ static int rproc_handle_carveout(struct rproc *rproc, struct fw_resource *rsc)
 		mapping->len = rsc->len;
 		list_add_tail(&mapping->node, &rproc->mappings);
 
-		dev_dbg(dev, "carveout mapped 0x%llx to 0x%x\n", rsc->da, dma);
+		dev_dbg(dev, "carveout mapped 0x%x to 0x%x\n", rsc->da, dma);
 
 		/*
 		 * Some remote processors might need to know the pa
@@ -665,36 +728,44 @@ free_mapping:
  * enum fw_resource_type.
  */
 static rproc_handle_resource_t rproc_handle_rsc[] = {
-	[RSC_CARVEOUT] = rproc_handle_carveout,
-	[RSC_DEVMEM] = rproc_handle_devmem,
-	[RSC_TRACE] = rproc_handle_trace,
-	[RSC_VRING] = rproc_handle_vring,
-	[RSC_VIRTIO_DEV] = NULL, /* handled early upon registration */
+	[RSC_CARVEOUT] = (rproc_handle_resource_t)rproc_handle_carveout,
+	[RSC_DEVMEM] = (rproc_handle_resource_t)rproc_handle_devmem,
+	[RSC_TRACE] = (rproc_handle_resource_t)rproc_handle_trace,
+	[RSC_VDEV] = (rproc_handle_resource_t)rproc_handle_vdev,
 };
 
 /* handle firmware resource entries before booting the remote processor */
 static int
-rproc_handle_boot_rsc(struct rproc *rproc, struct fw_resource *rsc, int len)
+rproc_handle_boot_rsc(struct rproc *rproc, struct resource_table *table, int len)
 {
 	struct device *dev = rproc->dev;
 	rproc_handle_resource_t handler;
-	int ret = 0;
+	int ret = 0, i;
+
+	for (i = 0; i < table->num; i++) {
+		int offset = table->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table + offset;
+		int avail = len - offset - sizeof(*hdr);
+		void *rsc = (void *)hdr + sizeof(*hdr);
+
+		/* make sure table isn't truncated */
+		if (avail < 0) {
+			dev_err(dev, "rsc table is truncated\n");
+			return -EINVAL;
+		}
 
-	for (; len >= sizeof(*rsc); rsc++, len -= sizeof(*rsc)) {
-		dev_dbg(dev, "rsc: type %d, da 0x%llx, pa 0x%llx, len 0x%x, "
-			"id %d, name %s, flags %x\n", rsc->type, rsc->da,
-			rsc->pa, rsc->len, rsc->id, rsc->name, rsc->flags);
+		dev_dbg(dev, "rsc: type %d\n", hdr->type);
 
-		if (rsc->type >= RSC_LAST) {
-			dev_warn(dev, "unsupported resource %d\n", rsc->type);
+		if (hdr->type >= RSC_LAST) {
+			dev_warn(dev, "unsupported resource %d\n", hdr->type);
 			continue;
 		}
 
-		handler = rproc_handle_rsc[rsc->type];
+		handler = rproc_handle_rsc[hdr->type];
 		if (!handler)
 			continue;
 
-		ret = handler(rproc, rsc);
+		ret = handler(rproc, rsc, avail);
 		if (ret)
 			break;
 	}
@@ -704,18 +775,31 @@ rproc_handle_boot_rsc(struct rproc *rproc, struct fw_resource *rsc, int len)
 
 /* handle firmware resource entries while registering the remote processor */
 static int
-rproc_handle_virtio_rsc(struct rproc *rproc, struct fw_resource *rsc, int len)
+rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table *table, int len)
 {
 	struct device *dev = rproc->dev;
-	int ret = -ENODEV;
+	int ret = 0, i;
+
+	for (i = 0; i < table->num; i++) {
+		int offset = table->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table + offset;
+		int avail = len - offset - sizeof(*hdr);
 
-	for (; len >= sizeof(*rsc); rsc++, len -= sizeof(*rsc))
-		if (rsc->type == RSC_VIRTIO_DEV) {
-			dev_dbg(dev, "found vdev %d/%s features %llx\n",
-					rsc->flags, rsc->name, rsc->da);
-			ret = rproc_handle_virtio_hdr(rproc, rsc);
+		/* make sure table isn't truncated */
+		if (avail < 0) {
+			dev_err(dev, "rsc table is truncated\n");
+			return -EINVAL;
+		}
+
+		dev_dbg(dev, "%s: rsc type %d\n", __func__, hdr->type);
+
+		if (hdr->type == RSC_VDEV) {
+			struct fw_rsc_vdev *vrsc =
+					(struct fw_rsc_vdev *)hdr->data;
+			ret = rproc_handle_early_vdev(rproc, vrsc, avail);
 			break;
 		}
+	}
 
 	return ret;
 }
@@ -744,7 +828,9 @@ static int rproc_handle_resources(struct rproc *rproc, const u8 *elf_data,
 	struct elf32_hdr *ehdr;
 	struct elf32_shdr *shdr;
 	const char *name_table;
+	struct device *dev = rproc->dev;
 	int i, ret = -EINVAL;
+	struct resource_table *table;
 
 	ehdr = (struct elf32_hdr *)elf_data;
 	shdr = (struct elf32_shdr *)(elf_data + ehdr->e_shoff);
@@ -752,21 +838,47 @@ static int rproc_handle_resources(struct rproc *rproc, const u8 *elf_data,
 
 	/* look for the resource table and handle it */
 	for (i = 0; i < ehdr->e_shnum; i++, shdr++) {
-		if (!strcmp(name_table + shdr->sh_name, ".resource_table")) {
-			struct fw_resource *table = (struct fw_resource *)
-						(elf_data + shdr->sh_offset);
+		int size = shdr->sh_size;
+		int offset = shdr->sh_offset;
 
-			if (shdr->sh_offset + shdr->sh_size > len) {
-				dev_err(rproc->dev,
-					"truncated fw: need 0x%x avail 0x%x\n",
-					shdr->sh_offset + shdr->sh_size, len);
-				ret = -EINVAL;
-			}
+		if (strcmp(name_table + shdr->sh_name, ".resource_table"))
+			continue;
 
-			ret = handler(rproc, table, shdr->sh_size);
+		table = (struct resource_table *)(elf_data + offset);
 
-			break;
+		/* make sure we have the entire table */
+		if (offset + size > len) {
+			dev_err(dev, "resource table truncated\n");
+			return -EINVAL;
+		}
+
+		/* make sure table has at least the header */
+		if (sizeof(struct resource_table) > size) {
+			dev_err(dev, "header-less resource table\n");
+			return -EINVAL;
 		}
+
+		/* we don't support any version beyond the first */
+		if (table->ver != 1) {
+			dev_err(dev, "unsupported fw ver: %d\n", table->ver);
+			return -EINVAL;
+		}
+
+		/* make sure reserved bytes are zeroes */
+		if (table->reserved[0] || table->reserved[1]) {
+			dev_err(dev, "non zero reserved bytes\n");
+			return -EINVAL;
+		}
+
+		/* make sure the offsets array isn't truncated */
+		if (table->num * sizeof(table->offset[0]) +
+				sizeof(struct resource_table) > size) {
+			dev_err(dev, "resource table incomplete\n");
+			return -EINVAL;
+		}
+
+		ret = handler(rproc, table, shdr->sh_size);
+		break;
 	}
 
 	return ret;
@@ -980,7 +1092,7 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	if (rproc_fw_sanity_check(rproc, fw) < 0)
 		goto out;
 
-	/* does the fw supports any virtio devices ? */
+	/* does the fw support any virtio devices ? */
 	ret = rproc_handle_resources(rproc, fw->data, fw->size,
 						rproc_handle_virtio_rsc);
 	if (ret) {
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index ada4cb0..6040f83 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -50,39 +50,51 @@
 #define AMP_VRING_ALIGN	(4096)
 
 /**
- * struct fw_resource - describes an entry from the resource section
- * @type: resource type
- * @id: index number of the resource
- * @da: device address of the resource
- * @pa: physical address of the resource
- * @len: size, in bytes, of the resource
- * @flags: properties of the resource, e.g. iommu protection required
- * @reserved: must be 0 atm
- * @name: name of resource
+ * struct resource_table - firmware resource table header
+ * @ver: version number
+ * @num: number of resource entries
+ * @reserved: reserved (must be zero)
+ * @offset: array of offsets pointing at the various resource entries
  *
- * The remote processor firmware should contain a "resource table":
- * array of 'struct fw_resource' entries.
+ * A resource table is essentially a list of system resources required
+ * by the remote processor. It may also include configuration entries.
+ * If needed, the remote processor firmware should contain this table
+ * as a dedicated ".resource_table" ELF section.
  *
  * Some resources entries are mere announcements, where the host is informed
  * of specific remoteproc configuration. Other entries require the host to
- * do something (e.g. reserve a requested resource) and possibly also reply
- * by overwriting a member inside 'struct fw_resource' with info about the
- * allocated resource.
- *
- * Different resource entries use different members of this struct,
- * with different meanings. This is pretty limiting and error-prone,
- * so the plan is to move to variable-length TLV-based resource entries,
- * where each resource type will have its own structure.
+ * do something (e.g. allocate a system resource). Sometimes a negotiation
+ * is expected, where the firmware requests a resource, and once allocated,
+ * the host should provide back its details (e.g. address of an allocated
+ * memory region).
+ *
+ * The header of the resource table, as expressed by this structure,
+ * contains a version number (should we need to change this format in the
+ * future), the number of available resource entries, and their offsets
+ * in the table.
+ *
+ * Immediately following this header are the resource entries themselves,
+ * each of which begins with a resource entry header (as described below).
+ */
+struct resource_table {
+	u32 ver;
+	u32 num;
+	u32 reserved[2];
+	u32 offset[0];
+} __packed;
+
+/**
+ * struct fw_rsc_hdr - firmware resource entry header
+ * @type: resource type
+ * @data: resource data
+ *
+ * Every resource entry begins with a 'struct fw_rsc_hdr' header providing
+ * its @type. The content of the entry itself will immediately follow
+ * this header, and it should be parsed according to the resource type.
  */
-struct fw_resource {
+struct fw_rsc_hdr {
 	u32 type;
-	u32 id;
-	u64 da;
-	u64 pa;
-	u32 len;
-	u32 flags;
-	u8 reserved[16];
-	u8 name[48];
+	u8 data[0];
 } __packed;
 
 /**
@@ -92,30 +104,13 @@ struct fw_resource {
  *		    memory region.
  * @RSC_DEVMEM:     request to iommu_map a memory-based peripheral.
  * @RSC_TRACE:	    announces the availability of a trace buffer into which
- *		    the remote processor will be writing logs. In this case,
- *		    'da' indicates the device address where logs are written to,
- *		    and 'len' is the size of the trace buffer.
- * @RSC_VRING:	    request for allocation of a virtio vring (address should
- *		    be indicated in 'da', and 'len' should contain the number
- *		    of buffers supported by the vring).
- * @RSC_VIRTIO_DEV: this entry declares about support for a virtio device,
- *		    and serves as the virtio header. 'da' holds the
- *		    the virtio device features, 'pa' holds the virtio guest
- *		    features, 'len' holds the virtio status, and 'flags' holds
- *		    the virtio id (currently only VIRTIO_ID_RPMSG is supported).
+ *		    the remote processor will be writing logs.
+ * @RSC_VDEV:       declare support for a virtio device, and serve as its
+ *		    virtio header.
  * @RSC_LAST:       just keep this one at the end
  *
- * Most of the resource entries share the basic idea of address/length
- * negotiation with the host: the firmware usually asks (on behalf of the
- * remote processor that will soon be booted with it) for memory
- * of size 'len' bytes, and the host needs to allocate it and provide
- * the device/physical address (when relevant) in 'da'/'pa' respectively.
- *
- * If the firmware is compiled with hard coded device addresses, and
- * can't handle dynamically allocated 'da' values, then the 'da' field
- * will contain the expected device addresses (today we actually only support
- * this scheme, as there aren't yet any use cases for dynamically allocated
- * device addresses).
+ * For more details regarding a specific resource type, please see its
+ * dedicated structure below.
  *
  * Please note that these values are used as indices to the rproc_handle_rsc
  * lookup table, so please keep them sane. Moreover, @RSC_LAST is used to
@@ -126,11 +121,197 @@ enum fw_resource_type {
 	RSC_CARVEOUT	= 0,
 	RSC_DEVMEM	= 1,
 	RSC_TRACE	= 2,
-	RSC_VRING	= 3,
-	RSC_VIRTIO_DEV	= 4,
-	RSC_LAST	= 5,
+	RSC_VDEV	= 3,
+	RSC_LAST	= 4,
 };
 
+#define FW_RSC_ADDR_ANY (0xFFFFFFFFFFFFFFFF)
+
+/**
+ * struct fw_rsc_carveout - physically contiguous memory request
+ * @da: device address
+ * @pa: physical address
+ * @len: length (in bytes)
+ * @flags: iommu protection flags
+ * @reserved: reserved (must be zero)
+ * @name: human-readable name of the requested memory region
+ *
+ * This resource entry requests the host to allocate a physically contiguous
+ * memory region.
+ *
+ * These request entries should precede other firmware resource entries,
+ * as other entries might request placing other data objects inside
+ * these memory regions (e.g. data/code segments, trace resource entries, ...).
+ *
+ * Allocating memory this way helps utilizing the reserved physical memory
+ * (e.g. CMA) more efficiently, and also minimizes the number of TLB entries
+ * needed to map it (in case @rproc is using an IOMMU). Reducing the TLB
+ * pressure is important; it may have a substantial impact on performance.
+ *
+ * If the firmware is compiled with static addresses, then @da should specify
+ * the expected device address of this memory region. If @da is set to
+ * FW_RSC_ADDR_ANY, then the host will dynamically allocate it, and then
+ * overwrite @da with the dynamically allocated address.
+ *
+ * We will always use @da to negotiate the device addresses, even if it
+ * isn't using an iommu. In that case, though, it will obviously contain
+ * physical addresses.
+ *
+ * Some remote processors needs to know the allocated physical address
+ * even if they do use an iommu. This is needed, e.g., if they control
+ * hardware accelerators which access the physical memory directly (this
+ * is the case with OMAP4 for instance). In that case, the host will
+ * overwrite @pa with the dynamically allocated physical address.
+ * Generally we don't want to expose physical addresses if we don't have to
+ * (remote processors are generally _not_ trusted), so we might want to
+ * change this to happen _only_ when explicitly required by the hardware.
+ *
+ * @flags is used to provide IOMMU protection flags, and @name should
+ * (optionally) contain a human readable name of this carveout region
+ * (mainly for debugging purposes).
+ */
+struct fw_rsc_carveout {
+	u32 da;
+	u32 pa;
+	u32 len;
+	u32 flags;
+	u32 reserved;
+	u8 name[32];
+} __packed;
+
+/**
+ * struct fw_rsc_devmem - iommu mapping request
+ * @da: device address
+ * @pa: physical address
+ * @len: length (in bytes)
+ * @flags: iommu protection flags
+ * @reserved: reserved (must be zero)
+ * @name: human-readable name of the requested region to be mapped
+ *
+ * This resource entry requests the host to iommu map a physically contiguous
+ * memory region. This is needed in case the remote processor requires
+ * access to certain memory-based peripherals; _never_ use it to access
+ * regular memory.
+ *
+ * This is obviously only needed if the remote processor is accessing memory
+ * via an iommu.
+ *
+ * @da should specify the required device address, @pa should specify
+ * the physical address we want to map, @len should specify the size of
+ * the mapping and @flags is the IOMMU protection flags. As always, @name may
+ * (optionally) contain a human readable name of this mapping (mainly for
+ * debugging purposes).
+ *
+ * Note: at this point we just "trust" those devmem entries to contain valid
+ * physical addresses, but this isn't safe and will be changed: eventually we
+ * want remoteproc implementations to provide us ranges of physical addresses
+ * the firmware is allowed to request, and not allow firmwares to request
+ * access to physical addresses that are outside those ranges.
+ */
+struct fw_rsc_devmem {
+	u32 da;
+	u32 pa;
+	u32 len;
+	u32 flags;
+	u32 reserved;
+	u8 name[32];
+} __packed;
+
+/**
+ * struct fw_rsc_trace - trace buffer declaration
+ * @da: device address
+ * @len: length (in bytes)
+ * @reserved: reserved (must be zero)
+ * @name: human-readable name of the trace buffer
+ *
+ * This resource entry provides the host information about a trace buffer
+ * into which the remote processor will write log messages.
+ *
+ * @da specifies the device address of the buffer, @len specifies
+ * its size, and @name may contain a human readable name of the trace buffer.
+ *
+ * After booting the remote processor, the trace buffers are exposed to the
+ * user via debugfs entries (called trace0, trace1, etc..).
+ */
+struct fw_rsc_trace {
+	u32 da;
+	u32 len;
+	u32 reserved;
+	u8 name[32];
+} __packed;
+
+/**
+ * struct fw_rsc_vdev_vring - vring descriptor entry
+ * @da: device address
+ * @align: the alignment between the consumer and producer parts of the vring
+ * @num: num of buffers supported by this vring (must be power of two)
+ * @notifyid is a unique rproc-wide notify index for this vring. This notify
+ * index is used when kicking a remote processor, to let it know that this
+ * vring is triggered.
+ * @reserved: reserved (must be zero)
+ *
+ * This descriptor is not a resource entry by itself; it is part of the
+ * vdev resource type (see below).
+ *
+ * Note that @da should either contain the device address where
+ * the remote processor is expecting the vring, or indicate that
+ * dynamically allocation of the vring's device address is supported.
+ */
+struct fw_rsc_vdev_vring {
+	u32 da;
+	u32 align;
+	u32 num;
+	u32 notifyid;
+	u32 reserved;
+} __packed;
+
+/**
+ * struct fw_rsc_vdev - virtio device header
+ * @id: virtio device id (as in virtio_ids.h)
+ * @notifyid is a unique rproc-wide notify index for this vdev. This notify
+ * index is used when kicking a remote processor, to let it know that the
+ * status/features of this vdev have changes.
+ * @dfeatures specifies the virtio device features supported by the firmware
+ * @gfeatures is a place holder used by the host to write back the
+ * negotiated features that are supported by both sides.
+ * @config_len is the size of the virtio config space of this vdev. The config
+ * space lies in the resource table immediate after this vdev header.
+ * @status is a place holder where the host will indicate its virtio progress.
+ * @num_of_vrings indicates how many vrings are described in this vdev header
+ * @reserved: reserved (must be zero)
+ * @vring is an array of @num_of_vrings entries of 'struct fw_rsc_vdev_vring'.
+ *
+ * This resource is a virtio device header: it provides information about
+ * the vdev, and is then used by the host and its peer remote processors
+ * to negotiate and share certain virtio properties.
+ *
+ * By providing this resource entry, the firmware essentially asks remoteproc
+ * to statically allocate a vdev upon registration of the rproc (dynamic vdev
+ * allocation is not yet supported).
+ *
+ * Note: unlike virtualization systems, the term 'host' here means
+ * the Linux side which is running remoteproc to control the remote
+ * processors. We use the name 'gfeatures' to comply with virtio's terms,
+ * though there isn't really any virtualized guest OS here: it's the host
+ * which is responsible for negotiating the final features.
+ * Yeah, it's a bit confusing.
+ *
+ * Note: immediately following this structure is the virtio config space for
+ * this vdev (which is specific to the vdev; for more info, read the virtio
+ * spec). the size of the config space is specified by @config_len.
+ */
+struct fw_rsc_vdev {
+	u32 id;
+	u32 notifyid;
+	u32 dfeatures;
+	u32 gfeatures;
+	u32 config_len;
+	u8 status;
+	u8 num_of_vrings;
+	u8 reserved[2];
+	struct fw_rsc_vdev_vring vring[0];
+} __packed;
+
 /**
  * struct rproc_mem_entry - memory entry descriptor
  * @va:	virtual address
@@ -144,7 +325,7 @@ struct rproc_mem_entry {
 	void *va;
 	dma_addr_t dma;
 	int len;
-	u64 da;
+	u32 da;
 	void *priv;
 	struct list_head node;
 };
@@ -226,7 +407,7 @@ struct rproc {
 	struct list_head carveouts;
 	struct list_head mappings;
 	struct completion firmware_loading_complete;
-	u64 bootaddr;
+	u32 bootaddr;
 	struct rproc_vdev *rvdev;
 };
 
-- 
1.7.5.4

Re: [PATCH 1/7] remoteproc: resource table overhaul

[Michal Simek]

Hi Ohad,

Ohad Ben-Cohen wrote:
> The resource table is an array of 'struct fw_resource' members, where
> each resource entry is expressed as a single member of that array.
> 
> This approach got us this far, but it has a few drawbacks:
> 
> 1. Different resource entries end up overloading the same members of 'struct
>    fw_resource' with different meanings. The resulting code is error prone
>    and hard to read and maintain.
> 
> 2. It's impossible to extend 'struct fw_resource' without breaking the
>    existing firmware images (and we already want to: we can't introduce the
>    new virito device resource entry with the current scheme).
> 
> 3. It doesn't scale: 'struct fw_resource' must be as big as the largest
>    resource entry type. As a result, smaller resource entries end up
>    utilizing only small part of it.
> 
> This is fixed by defining a dedicated structure for every resource type,
> and then converting the resource table to a list of type-value members.
> Instead of a rigid array of homogeneous structs, the resource table
> is turned into a collection of heterogeneous structures.
> 
> This way:
> 1. Resource entries consume exactly the amount of bytes they need.
> 2. It's easy to extend: just create a new resource entry structure, and assign
>    it a new type.
> 3. The code is easier to read and maintain: the structures' members names are
>    meaningful.
> 
> While we're at it, this patch has several other resource table changes:
> 1. The resource table gains a simple header which contains the
>    number of entries in the table and their offsets within the table. This
>    makes the parsing code simpler and easier to read.
> 2. A version member is added to the resource table. Should we change the
>    format again, we'll bump up this version to prevent breakage with
>    existing firmware images.
> 3. The VRING and VIRTIO_DEV resource entries are combined to a single
>    VDEV entry. This paves the way to supporting multiple VDEV entries.
> 4. Since we don't really support 64-bit rprocs yet, convert two stray u64
>    members to u32.

I don't want to look for in this patch but there is one change which completely
break my remoteproc for dual core ARM.

I am using ohad-github/rpmsg_3.3_rc6_TLV branch and I have also changed resource
table to get it work.

Here is the part of log where you can see what happen.

rproc_handle_virtio_rsc: rsc type 0
rproc_handle_virtio_rsc: rsc type 3
vdev rsc: id 7, dfeatures 1, cfg len 0, 2 vrings
vdev rsc: vring0: da 100000, qsz 256, align 4096
vring0: va e1000000 dma 0 size 3000 idr 0
vdev rsc: vring1: da 104000, qsz 256, align 4096
vring1: va e1004000 dma 4000 size 3000 idr 1
zynq-rproc zynq-rproc.1: powering up cpu1_freertos
zynq-rproc zynq-rproc.1: Booting fw image test, size 211118
iommu not found
rsc: type 0
carveout rsc: da 0, pa 0, len 100000, flags 0
carveout va e1100000, dma 100000, len 0x100000
rsc: type 3
rsc: type 2
trace0 added: va e1140000, da 0x40000, len 0x8000
phdr: type 1 da 0x0 memsz 0x1ae60 filesz 0x14014

The problem which I have is that vdev entry and vrings are handled first
and in __rproc_handle_vring calls dma_alloc_coherent first which caused
that virtaddr points to memory start (remapped to virt addr 0xe1000000)
which should be used by code.

In previous version I had carverout first which means that code was copyied
to physical address 0x0 and then vrigns were allocated. Current code allocate
vring first and then RTOS is not able to run from zero address.

Thanks,
Michal




-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

Re: [PATCH 1/7] remoteproc: resource table overhaul

[Ohad Ben-Cohen]

Hi Michal,

On Fri, Mar 16, 2012 at 4:57 PM, Michal Simek <monstr@monstr.eu> wrote:
> In previous version I had carverout first which means that code was copyied
> to physical address 0x0 and then vrigns were allocated. Current code
> allocate vring first and then RTOS is not able to run from zero address.

In general, depending on specific allocation ordering is error prone
and you shouldn't do it.

For a given device, we treat memory allocated with
dma_alloc_coherent() as homogeneous. We care about its utilization and
alignments of allocations rather than their specific location in the
total region.

If that's not the case, and there's special memory you want assigned
with a specific purpose, than it's recommended to let the code make
sure it happens explicitly, and not rely on implicit order of
allocation.

Today there's no way to assign several memory regions with a specific
device, but a reasonable workaround that's being used is creating
several sub-devices with the special memory, and then looking them up
with stuff like device_for_each_child() or device_find_child().

About your specific issue: the current allocation scheme was chosen
for simplicity, but I do plan to change it back by postponing the
vring dma allocation until a remote processor is really powered on.
It's not a lot of memory, but this way we'd only allocate it when
really needed (so if it's CMA, others could use it).

IOW: you can probably just wait a bit until this patch is ready and
take it into your tree. It will most likely bring back the behavior you
need :)

Ohad.

Re: [PATCH 1/7] remoteproc: resource table overhaul

[Michal Simek]

Hi Ohad,

> On Fri, Mar 16, 2012 at 4:57 PM, Michal Simek <monstr@monstr.eu> wrote:
>> In previous version I had carverout first which means that code was copyied
>> to physical address 0x0 and then vrigns were allocated. Current code
>> allocate vring first and then RTOS is not able to run from zero address.
> 
> In general, depending on specific allocation ordering is error prone
> and you shouldn't do it.

Yes, I agree with you. It was in v1.


> For a given device, we treat memory allocated with
> dma_alloc_coherent() as homogeneous. We care about its utilization and
> alignments of allocations rather than their specific location in the
> total region.

For zynq platform is very important to use addresses from 0 for rtos.
The main reason why it is so important is ARM reset vectors.


> If that's not the case, and there's special memory you want assigned
> with a specific purpose, than it's recommended to let the code make
> sure it happens explicitly, and not rely on implicit order of
> allocation.

Sure.


> Today there's no way to assign several memory regions with a specific
> device, but a reasonable workaround that's being used is creating
> several sub-devices with the special memory, and then looking them up
> with stuff like device_for_each_child() or device_find_child().
> 
> About your specific issue: the current allocation scheme was chosen
> for simplicity, but I do plan to change it back by postponing the
> vring dma allocation until a remote processor is really powered on.
> It's not a lot of memory, but this way we'd only allocate it when
> really needed (so if it's CMA, others could use it).
> 
> IOW: you can probably just wait a bit until this patch is ready and
> take it into your tree. It will most likely bring back the behavior you
> need :)

OK. I will use v1 where allocation is based on order in resource table.
It is not ideal but it is working.

 From my point of view will be the best to call dma_alloc_coherent only once
which will allocate the whole chunk of memory which is required for "RTOS".

My resource table is easy:
#define TEXT_BASE 0
struct resource_table __resource resources = {
	1, /* we're the first version that implements this */
	3, /* number of entries in the table */
	0, 0, /* reserved, must be zero */
	/* offsets to entries */
	{
			offsetof(struct resource_table, text_cout),
			offsetof(struct resource_table, rpmsg_vdev),
			offsetof(struct resource_table, trace),
	},
	{
		TYPE_CARVEOUT, 0, 0, SZ_1M, 0, 0, "MEM_DATA",
	},
	{
		TYPE_VDEV, VIRTIO_ID_RPMSG, 0,
		RPMSG_IPU_C0_FEATURES, 0, 0, 0, 2, { 0, 0 },
		/* no config data */
	},

	/* the two vrings */
	{ TEXT_BASE + SZ_1M, 4096, 256, 1, 0 },
	{ TEXT_BASE + SZ_1M + 0x4000, 4096, 256, 2, 0 },

	{
		TYPE_TRACE, TEXT_BASE + 0x40000, 0x8000, 0, "trace",
	},

};

But as you see I need to keep that offset structure which is
that text/data starts at address 0, vrings0 on 0x100000, vrigns1 on 0x104000
and trace buffer 0x140000.

If dma_alloc_coherent is called just once with size taken from resource table.
(For my case for example 2MB=0x200000) and offset are driven by remoteproc then
it should just work.


Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

Re: [PATCH 1/7] remoteproc: resource table overhaul

[Ohad Ben-Cohen]

[-- Attachment #1: Type: text/plain, Size: 295 bytes --]

Hi Michal,

On Sat, Mar 17, 2012 at 8:39 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> IOW: you can probably just wait a bit until this patch is ready and
> take it into your tree. It will most likely bring back the behavior you
> need :)

Does something like the attached help ?

Thanks,
Ohad.

[-- Attachment #2: 0001-remoteproc-allocate-vrings-on-demand-free-when-not-n.patch --]
[-- Type: application/octet-stream, Size: 9560 bytes --]

From 638a75ebba8b7a1078606abf2e72a58f356f7d36 Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen <ohad@wizery.com>
Date: Thu, 17 May 2012 14:23:59 +0300
Subject: [PATCH] remoteproc: allocate vrings on demand, free when not needed

Dynamically allocate the vrings' DMA when the remote processor
is about to be powered on (i.e. when ->find_vqs() is invoked),
and release them as soon as it is powered off (i.e. when ->del_vqs()
is invoked).

The obvious and immediate benefit is better memory utilization, since
memory for the vrings is now only allocated when the relevant remote
processor is used.

Additionally, this approach also makes recovery of a (crashing)
remote processor easier: one just needs to remove the relevant
vdevs, and the entire vrings cleanup takes place automagically.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/remoteproc/remoteproc_core.c     |  108 +++++++++++++++---------------
 drivers/remoteproc/remoteproc_internal.h |    2 +
 drivers/remoteproc/remoteproc_virtio.c   |   13 +++-
 3 files changed, 66 insertions(+), 57 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index e756a0d..71d01dc 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -279,34 +279,17 @@ rproc_load_segments(struct rproc *rproc, const u8 *elf_data, size_t len)
 	return ret;
 }
 
-static int
-__rproc_handle_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i)
+int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 {
 	struct rproc *rproc = rvdev->rproc;
 	struct device *dev = rproc->dev;
-	struct fw_rsc_vdev_vring *vring = &rsc->vring[i];
+	struct rproc_vring *vring = &rvdev->vring[i];
 	dma_addr_t dma;
 	void *va;
 	int ret, size, notifyid;
 
-	dev_dbg(dev, "vdev rsc: vring%d: da %x, qsz %d, align %d\n",
-				i, vring->da, vring->num, vring->align);
-
-	/* make sure reserved bytes are zeroes */
-	if (vring->reserved) {
-		dev_err(dev, "vring rsc has non zero reserved bytes\n");
-		return -EINVAL;
-	}
-
-	/* verify queue size and vring alignment are sane */
-	if (!vring->num || !vring->align) {
-		dev_err(dev, "invalid qsz (%d) or alignment (%d)\n",
-						vring->num, vring->align);
-		return -EINVAL;
-	}
-
 	/* actual size of vring (in bytes) */
-	size = PAGE_ALIGN(vring_size(vring->num, vring->align));
+	size = PAGE_ALIGN(vring_size(vring->len, vring->align));
 
 	if (!idr_pre_get(&rproc->notifyids, GFP_KERNEL)) {
 		dev_err(dev, "idr_pre_get failed\n");
@@ -316,6 +299,7 @@ __rproc_handle_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i)
 	/*
 	 * Allocate non-cacheable memory for the vring. In the future
 	 * this call will also configure the IOMMU for us
+	 * TODO: let the rproc know the da of this vring
 	 */
 	va = dma_alloc_coherent(dev, size, &dma, GFP_KERNEL);
 	if (!va) {
@@ -323,44 +307,66 @@ __rproc_handle_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i)
 		return -EINVAL;
 	}
 
-	/* assign an rproc-wide unique index for this vring */
-	/* TODO: assign a notifyid for rvdev updates as well */
-	ret = idr_get_new(&rproc->notifyids, &rvdev->vring[i], &notifyid);
+	/*
+	 * Assign an rproc-wide unique index for this vring
+	 * TODO: assign a notifyid for rvdev updates as well
+	 * TODO: let the rproc know the notifyid of this vring
+	 * TODO: support predefined notifyids (via resource table)
+	 */
+	ret = idr_get_new(&rproc->notifyids, vring, &notifyid);
 	if (ret) {
 		dev_err(dev, "idr_get_new failed: %d\n", ret);
 		dma_free_coherent(dev, size, va, dma);
 		return ret;
 	}
 
-	/* let the rproc know the da and notifyid of this vring */
-	/* TODO: expose this to remote processor */
-	vring->da = dma;
-	vring->notifyid = notifyid;
-
 	dev_dbg(dev, "vring%d: va %p dma %x size %x idr %d\n", i, va,
 					dma, size, notifyid);
 
-	rvdev->vring[i].len = vring->num;
-	rvdev->vring[i].align = vring->align;
-	rvdev->vring[i].va = va;
-	rvdev->vring[i].dma = dma;
-	rvdev->vring[i].notifyid = notifyid;
-	rvdev->vring[i].rvdev = rvdev;
+	vring->va = va;
+	vring->dma = dma;
+	vring->notifyid = notifyid;
 
 	return 0;
 }
 
-static void __rproc_free_vrings(struct rproc_vdev *rvdev, int i)
+static int
+rproc_parse_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i)
 {
 	struct rproc *rproc = rvdev->rproc;
+	struct device *dev = rproc->dev;
+	struct fw_rsc_vdev_vring *vring = &rsc->vring[i];
+
+	dev_dbg(dev, "vdev rsc: vring%d: da %x, qsz %d, align %d\n",
+				i, vring->da, vring->num, vring->align);
 
-	for (i--; i >= 0; i--) {
-		struct rproc_vring *rvring = &rvdev->vring[i];
-		int size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
+	/* make sure reserved bytes are zeroes */
+	if (vring->reserved) {
+		dev_err(dev, "vring rsc has non zero reserved bytes\n");
+		return -EINVAL;
+	}
 
-		dma_free_coherent(rproc->dev, size, rvring->va, rvring->dma);
-		idr_remove(&rproc->notifyids, rvring->notifyid);
+	/* verify queue size and vring alignment are sane */
+	if (!vring->num || !vring->align) {
+		dev_err(dev, "invalid qsz (%d) or alignment (%d)\n",
+						vring->num, vring->align);
+		return -EINVAL;
 	}
+
+	rvdev->vring[i].len = vring->num;
+	rvdev->vring[i].align = vring->align;
+	rvdev->vring[i].rvdev = rvdev;
+
+	return 0;
+}
+
+void rproc_free_vring(struct rproc_vring *rvring)
+{
+	int size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
+	struct rproc *rproc = rvring->rvdev->rproc;
+
+	dma_free_coherent(rproc->dev, size, rvring->va, rvring->dma);
+	idr_remove(&rproc->notifyids, rvring->notifyid);
 }
 
 /**
@@ -425,11 +431,11 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 
 	rvdev->rproc = rproc;
 
-	/* allocate the vrings */
+	/* parse the vrings */
 	for (i = 0; i < rsc->num_of_vrings; i++) {
-		ret = __rproc_handle_vring(rvdev, rsc, i);
+		ret = rproc_parse_vring(rvdev, rsc, i);
 		if (ret)
-			goto free_vrings;
+			goto free_rvdev;
 	}
 
 	/* remember the device features */
@@ -440,12 +446,11 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 	/* it is now safe to add the virtio device */
 	ret = rproc_add_virtio_dev(rvdev, rsc->id);
 	if (ret)
-		goto free_vrings;
+		goto free_rvdev;
 
 	return 0;
 
-free_vrings:
-	__rproc_free_vrings(rvdev, i);
+free_rvdev:
 	kfree(rvdev);
 	return ret;
 }
@@ -1265,18 +1270,11 @@ EXPORT_SYMBOL(rproc_shutdown);
 void rproc_release(struct kref *kref)
 {
 	struct rproc *rproc = container_of(kref, struct rproc, refcount);
-	struct rproc_vdev *rvdev, *rvtmp;
 
 	dev_info(rproc->dev, "removing %s\n", rproc->name);
 
 	rproc_delete_debug_dir(rproc);
 
-	/* clean up remote vdev entries */
-	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node) {
-		__rproc_free_vrings(rvdev, RVDEV_NUM_VRINGS);
-		list_del(&rvdev->node);
-	}
-
 	/*
 	 * At this point no one holds a reference to rproc anymore,
 	 * so we can directly unroll rproc_alloc()
@@ -1547,7 +1545,7 @@ EXPORT_SYMBOL(rproc_free);
  */
 int rproc_unregister(struct rproc *rproc)
 {
-	struct rproc_vdev *rvdev;
+	struct rproc_vdev *rvdev, *tmp;
 
 	if (!rproc)
 		return -EINVAL;
@@ -1556,7 +1554,7 @@ int rproc_unregister(struct rproc *rproc)
 	wait_for_completion(&rproc->firmware_loading_complete);
 
 	/* clean up remote vdev entries */
-	list_for_each_entry(rvdev, &rproc->rvdevs, node)
+	list_for_each_entry_safe(rvdev, tmp, &rproc->rvdevs, node)
 		rproc_remove_virtio_dev(rvdev);
 
 	/* the rproc is downref'ed as soon as it's removed from the klist */
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 9f336d6..f4957cf 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -41,4 +41,6 @@ void rproc_create_debug_dir(struct rproc *rproc);
 void rproc_init_debugfs(void);
 void rproc_exit_debugfs(void);
 
+void rproc_free_vring(struct rproc_vring *rvring);
+int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
 #endif /* REMOTEPROC_INTERNAL_H */
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index ecf6121..26a7144 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -77,14 +77,17 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
 	struct rproc_vring *rvring;
 	struct virtqueue *vq;
 	void *addr;
-	int len, size;
+	int len, size, ret;
 
 	/* we're temporarily limited to two virtqueues per rvdev */
 	if (id >= ARRAY_SIZE(rvdev->vring))
 		return ERR_PTR(-EINVAL);
 
-	rvring = &rvdev->vring[id];
+	ret = rproc_alloc_vring(rvdev, id);
+	if (ret)
+		return ERR_PTR(ret);
 
+	rvring = &rvdev->vring[id];
 	addr = rvring->va;
 	len = rvring->len;
 
@@ -103,6 +106,7 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
 					rproc_virtio_notify, callback, name);
 	if (!vq) {
 		dev_err(rproc->dev, "vring_new_virtqueue %s failed\n", name);
+		rproc_free_vring(rvring);
 		return ERR_PTR(-ENOMEM);
 	}
 
@@ -125,6 +129,7 @@ static void rproc_virtio_del_vqs(struct virtio_device *vdev)
 		rvring = vq->priv;
 		rvring->vq = NULL;
 		vring_del_virtqueue(vq);
+		rproc_free_vring(rvring);
 	}
 }
 
@@ -228,8 +233,12 @@ static struct virtio_config_ops rproc_virtio_config_ops = {
 static void rproc_vdev_release(struct device *dev)
 {
 	struct virtio_device *vdev = dev_to_virtio(dev);
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
 	struct rproc *rproc = vdev_to_rproc(vdev);
 
+	list_del(&rvdev->node);
+	kfree(rvdev);
+
 	kref_put(&rproc->refcount, rproc_release);
 }
 
-- 
1.7.5.4

Re: [PATCH 1/7] remoteproc: resource table overhaul

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 3558 bytes --]

Hi Ohad,

2012/5/17 Ohad Ben-Cohen <ohad@wizery.com>

> Hi Michal,
>
> On Sat, Mar 17, 2012 at 8:39 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> > IOW: you can probably just wait a bit until this patch is ready and
> > take it into your tree. It will most likely bring back the behavior you
> > need :)
>
> Does something like the attached help ?
>

I have looked at it and tested on latest and greatest and the problem is
still there.  If you look at debug output below
you see that vrings are allocated before carveout area which is causing
that rtos text is not placed at 0x0 address.

I have looked at the code and path is
rproc_fw_config_virtio -> rproc_handliefirtion_rsc -> rproc_handle_vdev


~ # zynq_remoteproc 0.remoteproc-test: rproc_handle_virtio_rsc: rsc type 0
zynq_remoteproc 0.remoteproc-test: rproc_handle_virtio_rsc: rsc type 3
zynq_remoteproc 0.remoteproc-test: vdev rsc: id 7, dfeatures 1, cfg len 0,
2 vrings
zynq_remoteproc 0.remoteproc-test: vdev rsc: vring0: da 400000, qsz 256,
align 256
zynq_remoteproc 0.remoteproc-test: vdev rsc: vring1: da 404000, qsz 256,
align 256
zynq_remoteproc 0.remoteproc-test: vring0: va d1000000 dma 0 size 2000 idr 0
zynq_remoteproc 0.remoteproc-test: vring1: va d1002000 dma 2000 size 2000
idr 1
zynq_remoteproc 0.remoteproc-test: powering up 0.remoteproc-test
zynq_remoteproc 0.remoteproc-test: Booting fw image test, size 2353146
zynq_remoteproc 0.remoteproc-test: iommu not found
zynq_remoteproc 0.remoteproc-test: rsc: type 0
zynq_remoteproc 0.remoteproc-test: carveout rsc: da 0, pa 0, len 20ec20,
flags 0
zynq_remoteproc 0.remoteproc-test: carveout va d1400000, dma 400000, len
0x20ec20
zynq_remoteproc 0.remoteproc-test: rsc: type 3
zynq_remoteproc 0.remoteproc-test: rsc: type 2


For me it is necessary to ensure that dma_alloc_coherent in
rproc_alloc_vring is called after carveout allocation
just because dma_alloc_cohorent takes mermory from preallocated pool which
starts at 0x0. I also don't have
any other advance mechanism how to ensure mapping for the second arm core.

Here is log which I need to get. Look at attached patch which exactly show
you what I need to do which I have tested.
Maybe it is not big deal and I can use mainline remoteproc version and use
this patch to be in sync with mainline code.

zynq_remoteproc 0.remoteproc-test: rproc_handle_virtio_rsc: rsc type 0
zynq_remoteproc 0.remoteproc-test: carveout rsc: da 0, pa 0, len 20ec20,
flags 0
zynq_remoteproc 0.remoteproc-test: carveout va d1000000, dma 0, len 0x20ec20
zynq_remoteproc 0.remoteproc-test: rproc_handle_virtio_rsc: rsc type 3
zynq_remoteproc 0.remoteproc-test: rproc_handle_virtio_rsc: rsc type 2
zynq_remoteproc 0.remoteproc-test: rproc_handle_virtio_rsc: rsc type 0
zynq_remoteproc 0.remoteproc-test: rproc_handle_virtio_rsc: rsc type 3
zynq_remoteproc 0.remoteproc-test: vdev rsc: id 7, dfeatures 1, cfg len 0,
2 vrings
zynq_remoteproc 0.remoteproc-test: vdev rsc: vring0: da 400000, qsz 256,
align 4096
zynq_remoteproc 0.remoteproc-test: vdev rsc: vring1: da 404000, qsz 256,
align 4096
zynq_remoteproc 0.remoteproc-test: vring0: va d1400000 dma 400000 size 3000
idr 0
zynq_remoteproc 0.remoteproc-test: vring1: va d1404000 dma 404000 size 3000
idr 1
zynq_remoteproc 0.remoteproc-test: powering up 0.remoteproc-test
zynq_remoteproc 0.remoteproc-test: Booting fw image test, size 2353146
zynq_remoteproc 0.remoteproc-test: iommu not found
zynq_remoteproc 0.remoteproc-test: rsc: type 0
zynq_remoteproc 0.remoteproc-test: rsc: type 3
zynq_remoteproc 0.remoteproc-test: rsc: type 2

Thanks,
Michal

[-- Attachment #1.2: Type: text/html, Size: 4124 bytes --]

[-- Attachment #2: 0001-remoteproc-Allocate-carveout-area-before-vrings-allo.patch --]
[-- Type: application/octet-stream, Size: 2773 bytes --]

From 48250faed7aeae7e968c3779221fb413238e3c63 Mon Sep 17 00:00:00 2001
From: Michal Simek <monstr@monstr.eu>
Date: Mon, 21 May 2012 14:30:45 +0200
Subject: [PATCH] remoteproc: Allocate carveout area before vrings alloation

Zynq requires to carveout allocation before vrings
because firmware must be placed from address 0x0.
There is no any other advance mechanism to be able to remap
addresses for remoteproc. Zynq is using the second arm core.

Signed-off-by: Michal Simek <monstr@monstr.eu>
---
 drivers/remoteproc/remoteproc_core.c |   41 +++++++++++++++++++++++++++++++++-
 1 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 40e2b2d..9434861 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -748,7 +748,7 @@ free_mapping:
  * enum fw_resource_type.
  */
 static rproc_handle_resource_t rproc_handle_rsc[] = {
-	[RSC_CARVEOUT] = (rproc_handle_resource_t)rproc_handle_carveout,
+	[RSC_CARVEOUT] = NULL,
 	[RSC_DEVMEM] = (rproc_handle_resource_t)rproc_handle_devmem,
 	[RSC_TRACE] = (rproc_handle_resource_t)rproc_handle_trace,
 	[RSC_VDEV] = NULL, /* VDEVs were handled upon registrarion */
@@ -793,6 +793,40 @@ rproc_handle_boot_rsc(struct rproc *rproc, struct resource_table *table, int len
 	return ret;
 }
 
+/* handle carveout firmware resource entries while registering the remote processor */
+static int
+rproc_handle_carveout_rsc(struct rproc *rproc, struct resource_table *table, int len)
+{
+	struct device *dev = rproc->dev;
+	int ret = 0, i;
+
+	for (i = 0; i < table->num; i++) {
+		int offset = table->offset[i];
+		struct fw_rsc_hdr *hdr = (void *)table + offset;
+		int avail = len - offset - sizeof(*hdr);
+		struct fw_rsc_carveout *crsc;
+
+		/* make sure table isn't truncated */
+		if (avail < 0) {
+			dev_err(dev, "rsc table is truncated\n");
+			return -EINVAL;
+		}
+
+		dev_dbg(dev, "%s: rsc type %d\n", __func__, hdr->type);
+
+		if (hdr->type != RSC_CARVEOUT)
+			continue;
+
+		crsc = (struct fw_rsc_carveout *)hdr->data;
+
+		ret = rproc_handle_carveout(rproc, crsc, avail);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
 /* handle firmware resource entries while registering the remote processor */
 static int
 rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table *table, int len)
@@ -1105,6 +1139,11 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 	if (!table)
 		goto out;
 
+	/* look for carveout areas and register them first */
+	ret = rproc_handle_carveout_rsc(rproc, table, tablesz);
+	if (ret)
+		goto out;
+
 	/* look for virtio devices and register them */
 	ret = rproc_handle_virtio_rsc(rproc, table, tablesz);
 	if (ret)
-- 
1.7.0.4


[-- Attachment #3: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 1/7] remoteproc: resource table overhaul

[Ohad Ben-Cohen]

Hi Michal,

On Mon, May 21, 2012 at 4:02 PM, Michal Simek <monstr@monstr.eu> wrote:
> I have looked at it and tested on latest and greatest and the problem is
> still there.

Ok, I see why this is happening.

We're now allocating the vrings' DMA in ->find_vqs() just before we
boot the remote processor, and we release it on ->del_vqs(). These are
the virtio handlers for setting up and tearing down the vrings, so it
makes sense to do so, but as a result, we still don't get the early
carveout allocation you wanted.

> I have looked at the code and path is
> rproc_fw_config_virtio -> rproc_handliefirtion_rsc -> rproc_handle_vdev

Please note that rproc_handle_vdev doesn't allocate any DMA at this
point, and the allocation path is now quite different - it begins with
virtio_rpmsg_bus probing and then calling into the ->find_vqs()
handler.

> For me it is necessary to ensure that dma_alloc_coherent in
> rproc_alloc_vring is called after carveout allocation
> just because dma_alloc_cohorent takes mermory from preallocated pool which
> starts at 0x0.

ok.

> I also don't have
> any other advance mechanism how to ensure mapping for the second arm core.

Probably the best way to ensure this (without it breaking unexpectedly
again as remoteproc evolves) is to have separate sub-devices for
different kind of memories, which remoteproc could then look up using
something like device_find_child().

Ludovic is exploring this direction too, because STE also needs to
bind specific memory regions with predefined purposes, without
depending on the order of DMA allocations.

I think Ludovic already has a preliminary patch which he may be able
to share with you.

> Here is log which I need to get. Look at attached patch which exactly show
> you what I need to do which I have tested.

Yes, I see. Another temporary approach might be moving the vrings'
allocations to the rproc_handle_rsc table (in the RSC_VDEV slot), but
I'm not sure we want to merge this as well because this solution too
is only hiding the real issue and not solving it permanently.

Thanks,
Ohad.

Re: [PATCH 1/7] remoteproc: resource table overhaul

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 4357 bytes --]

Hi Ohad and Ludovic,

2012/5/21 Ohad Ben-Cohen <ohad@wizery.com>

> Hi Michal,
>
> On Mon, May 21, 2012 at 4:02 PM, Michal Simek <monstr@monstr.eu> wrote:
> > I have looked at it and tested on latest and greatest and the problem is
> > still there.
>
> Ok, I see why this is happening.
>
> We're now allocating the vrings' DMA in ->find_vqs() just before we
> boot the remote processor, and we release it on ->del_vqs(). These are
> the virtio handlers for setting up and tearing down the vrings, so it
> makes sense to do so, but as a result, we still don't get the early
> carveout allocation you wanted.
>
> > I have looked at the code and path is
> > rproc_fw_config_virtio -> rproc_handliefirtion_rsc -> rproc_handle_vdev
>
> Please note that rproc_handle_vdev doesn't allocate any DMA at this
> point, and the allocation path is now quite different - it begins with
> virtio_rpmsg_bus probing and then calling into the ->find_vqs()
> handler.
>

> > For me it is necessary to ensure that dma_alloc_coherent in
> > rproc_alloc_vring is called after carveout allocation
> > just because dma_alloc_cohorent takes mermory from preallocated pool
> which
> > starts at 0x0.
>
> ok.
>
> > I also don't have
> > any other advance mechanism how to ensure mapping for the second arm
> core.
>
> Probably the best way to ensure this (without it breaking unexpectedly
> again as remoteproc evolves) is to have separate sub-devices for
> different kind of memories, which remoteproc could then look up using
> something like device_find_child().
>
> Ludovic is exploring this direction too, because STE also needs to
> bind specific memory regions with predefined purposes, without
> depending on the order of DMA allocations.
>

It is good that someone else has similar requirements.


>
> I think Ludovic already has a preliminary patch which he may be able
> to share with you.
>

Ludovic: Can you share your patch with us? I would like to try it.


>
> > Here is log which I need to get. Look at attached patch which exactly
> show
> > you what I need to do which I have tested.
>
> Yes, I see. Another temporary approach might be moving the vrings'
> allocations to the rproc_handle_rsc table (in the RSC_VDEV slot), but
> I'm not sure we want to merge this as well because this solution too
> is only hiding the real issue and not solving it permanently.
>
>
Simple enabling RSC_VDEV in rproc_handle_rsc doesn't work.

BTW: I am using kernel modules and there is no dependency for
virtio_rpmsg_bus in remoteproc module
in sense if you load remoteproc module you also need to load
virtio_rpmsg_bus to get firmware to work.

I think that will be good to create that dependency automatically because
you need to load virtio_rpmsg_bus
when you problem remoteproc module.

~ # lsmod
~ # modprobe zynq_remoteproc
zynq_remoteproc: Unknown symbol rproc_register (err 0)
zynq_remoteproc: Unknown symbol rproc_alloc (err 0)
zynq_remoteproc: Unknown symbol rproc_vq_interrupt (err 0)
zynq_remoteproc: Unknown symbol rproc_free (err 0)
zynq_remoteproc: Unknown symbol rproc_unregister (err 0)
CPU1: shutdown
zynq_remoteproc 0.remoteproc-test: 0.remoteproc-test is available
zynq_remoteproc 0.remoteproc-test: Note: remoteproc is still under
development and considered experimental.
zynq_remoteproc 0.remoteproc-test: THE BINARY FORMAT IS NOT YET FINALIZED,
and backward compatibility isn't yet guaranteed.
~ # zynq_remoteproc 0.remoteproc-test: registered virtio0 (type 7)

~ # lsmod
zynq_remoteproc 4346 0 - Live 0xbf01f000
remoteproc 13831 1 zynq_remoteproc, Live 0xbf018000
virtio 2840 1 remoteproc, Live 0xbf008000
virtio_ring 4965 1 remoteproc, Live 0xbf013000
~ #
~ # modprobe virtio_rpmsg_bus
zynq_remoteproc 0.remoteproc-test: powering up 0.remoteproc-test
zynq_remoteproc 0.remoteproc-test: Booting fw image test, size 2351790
zynq_remoteproc 0.remoteproc-test: remote processor 0.remoteproc-test is
now up
virtio_rpmsg_bus virtio0: rpmsg host is online
virtio_rpmsg_bus virtio0: creating channel rpmsg-timer-statistic addr 0x50
~ #
~ # lsmod
virtio_rpmsg_bus 7575 0 - Live 0xbf024000
zynq_remoteproc 4346 1 - Live 0xbf01f000
remoteproc 13831 1 zynq_remoteproc, Live 0xbf018000
virtio 2840 2 virtio_rpmsg_bus,remoteproc, Live 0xbf008000
virtio_ring 4965 2 virtio_rpmsg_bus,remoteproc, Live 0xbf013000
~ #

Can you see the same behavior on omap?

Thanks,
Michal

[-- Attachment #1.2: Type: text/html, Size: 5648 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 1/7] remoteproc: resource table overhaul

[Ohad Ben-Cohen]

Hi Michal,

On Tue, May 22, 2012 at 8:51 AM, Michal Simek <monstr@monstr.eu> wrote:
> Simple enabling RSC_VDEV in rproc_handle_rsc doesn't work.

Sure - you'll need to actually plug the vrings allocation code there,
too (i.e. this requires some coding, it's not a 1-liner).

> BTW: I am using kernel modules and there is no dependency for
> virtio_rpmsg_bus in remoteproc module
> in sense if you load remoteproc module you also need to load
> virtio_rpmsg_bus to get firmware to work.

This is expected: remoteproc only provides means for booting and
controlling remote processors and it doesn't boot them upfront.

To actually boot a remote processor, and do something useful with it,
one needs to load a module which utilizes remoteproc's API (e.g.
virtio_rpmsg_bus, but YMMV).

> ~ # lsmod
> ~ # modprobe zynq_remoteproc
> zynq_remoteproc: Unknown symbol rproc_register (err 0)
> zynq_remoteproc: Unknown symbol rproc_alloc (err 0)
> zynq_remoteproc: Unknown symbol rproc_vq_interrupt (err 0)
> zynq_remoteproc: Unknown symbol rproc_free (err 0)
> zynq_remoteproc: Unknown symbol rproc_unregister (err 0)

We don't have these errors - have you tried looking into them ?

Btw do you plan on submitting the code for zynq_remoteproc ?

> Can you see the same behavior on omap?

Except the "Unknown symbol" errors, everything else looks good.

Thanks,
Ohad.

Re: [PATCH 1/7] remoteproc: resource table overhaul

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 1833 bytes --]

Hi Ohad,

2012/5/22 Ohad Ben-Cohen <ohad@wizery.com>

> Hi Michal,
>
> On Tue, May 22, 2012 at 8:51 AM, Michal Simek <monstr@monstr.eu> wrote:
> > Simple enabling RSC_VDEV in rproc_handle_rsc doesn't work.
>
> Sure - you'll need to actually plug the vrings allocation code there,
> too (i.e. this requires some coding, it's not a 1-liner).
>
> > BTW: I am using kernel modules and there is no dependency for
> > virtio_rpmsg_bus in remoteproc module
> > in sense if you load remoteproc module you also need to load
> > virtio_rpmsg_bus to get firmware to work.
>
> This is expected: remoteproc only provides means for booting and
> controlling remote processors and it doesn't boot them upfront.
>
> To actually boot a remote processor, and do something useful with it,
> one needs to load a module which utilizes remoteproc's API (e.g.
> virtio_rpmsg_bus, but YMMV).
>
> > ~ # lsmod
> > ~ # modprobe zynq_remoteproc
> > zynq_remoteproc: Unknown symbol rproc_register (err 0)
> > zynq_remoteproc: Unknown symbol rproc_alloc (err 0)
> > zynq_remoteproc: Unknown symbol rproc_vq_interrupt (err 0)
> > zynq_remoteproc: Unknown symbol rproc_free (err 0)
> > zynq_remoteproc: Unknown symbol rproc_unregister (err 0)
>
> We don't have these errors - have you tried looking into them ?
>

It is probably any problem with modules. If I load them in sequence
there is no error.


>
> Btw do you plan on submitting the code for zynq_remoteproc ?
>

It is still under developing and there are some core changes in ARM low
level code
which need to be applied to mainline before this driver. I haven't sent
these patches
to mainline for review but I expect they won't be accepted but maybe I am
wrong.


>
> > Can you see the same behavior on omap?
>
> Except the "Unknown symbol" errors, everything else looks good.
>

Ok. Good.

Thanks,
Michal

[-- Attachment #1.2: Type: text/html, Size: 2725 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH 1/7] remoteproc: resource table overhaul

[frq09524]

Hi Ohad and Michal

yes I can share my patch, remoteproc has evolved and my patch is not 
aligned on latest version of remote proc (especially since remoteproc: 
remove the single rpmsg vdev limitation).

Ohad, for alignment I can take the latest branch of kernel.org 
(remoteproc) branch for-next?

/BR
Ludovic Barré

On 05/22/2012 07:51 AM, Michal Simek wrote:
> Hi Ohad and Ludovic,
>
> 2012/5/21 Ohad Ben-Cohen <ohad@wizery.com <mailto:ohad@wizery.com>>
>
>     Hi Michal,
>
>     On Mon, May 21, 2012 at 4:02 PM, Michal Simek <monstr@monstr.eu
>     <mailto:monstr@monstr.eu>> wrote:
>      > I have looked at it and tested on latest and greatest and the
>     problem is
>      > still there.
>
>     Ok, I see why this is happening.
>
>     We're now allocating the vrings' DMA in ->find_vqs() just before we
>     boot the remote processor, and we release it on ->del_vqs(). These are
>     the virtio handlers for setting up and tearing down the vrings, so it
>     makes sense to do so, but as a result, we still don't get the early
>     carveout allocation you wanted.
>
>      > I have looked at the code and path is
>      > rproc_fw_config_virtio -> rproc_handliefirtion_rsc ->
>     rproc_handle_vdev
>
>     Please note that rproc_handle_vdev doesn't allocate any DMA at this
>     point, and the allocation path is now quite different - it begins with
>     virtio_rpmsg_bus probing and then calling into the ->find_vqs()
>     handler.
>
>
>      > For me it is necessary to ensure that dma_alloc_coherent in
>      > rproc_alloc_vring is called after carveout allocation
>      > just because dma_alloc_cohorent takes mermory from preallocated
>     pool which
>      > starts at 0x0.
>
>     ok.
>
>      > I also don't have
>      > any other advance mechanism how to ensure mapping for the second
>     arm core.
>
>     Probably the best way to ensure this (without it breaking unexpectedly
>     again as remoteproc evolves) is to have separate sub-devices for
>     different kind of memories, which remoteproc could then look up using
>     something like device_find_child().
>
>     Ludovic is exploring this direction too, because STE also needs to
>     bind specific memory regions with predefined purposes, without
>     depending on the order of DMA allocations.
>
>
> It is good that someone else has similar requirements.
>
>
>     I think Ludovic already has a preliminary patch which he may be able
>     to share with you.
>
>
> Ludovic: Can you share your patch with us? I would like to try it.
>
>
>      > Here is log which I need to get. Look at attached patch which
>     exactly show
>      > you what I need to do which I have tested.
>
>     Yes, I see. Another temporary approach might be moving the vrings'
>     allocations to the rproc_handle_rsc table (in the RSC_VDEV slot), but
>     I'm not sure we want to merge this as well because this solution too
>     is only hiding the real issue and not solving it permanently.
>
>
> Simple enabling RSC_VDEV in rproc_handle_rsc doesn't work.
>
> BTW: I am using kernel modules and there is no dependency for
> virtio_rpmsg_bus in remoteproc module
> in sense if you load remoteproc module you also need to load
> virtio_rpmsg_bus to get firmware to work.
>
> I think that will be good to create that dependency automatically
> because you need to load virtio_rpmsg_bus
> when you problem remoteproc module.
>
> ~ # lsmod
> ~ # modprobe zynq_remoteproc
> zynq_remoteproc: Unknown symbol rproc_register (err 0)
> zynq_remoteproc: Unknown symbol rproc_alloc (err 0)
> zynq_remoteproc: Unknown symbol rproc_vq_interrupt (err 0)
> zynq_remoteproc: Unknown symbol rproc_free (err 0)
> zynq_remoteproc: Unknown symbol rproc_unregister (err 0)
> CPU1: shutdown
> zynq_remoteproc 0.remoteproc-test: 0.remoteproc-test is available
> zynq_remoteproc 0.remoteproc-test: Note: remoteproc is still under
> development and considered experimental.
> zynq_remoteproc 0.remoteproc-test: THE BINARY FORMAT IS NOT YET
> FINALIZED, and backward compatibility isn't yet guaranteed.
> ~ # zynq_remoteproc 0.remoteproc-test: registered virtio0 (type 7)
>
> ~ # lsmod
> zynq_remoteproc 4346 0 - Live 0xbf01f000
> remoteproc 13831 1 zynq_remoteproc, Live 0xbf018000
> virtio 2840 1 remoteproc, Live 0xbf008000
> virtio_ring 4965 1 remoteproc, Live 0xbf013000
> ~ #
> ~ # modprobe virtio_rpmsg_bus
> zynq_remoteproc 0.remoteproc-test: powering up 0.remoteproc-test
> zynq_remoteproc 0.remoteproc-test: Booting fw image test, size 2351790
> zynq_remoteproc 0.remoteproc-test: remote processor 0.remoteproc-test is
> now up
> virtio_rpmsg_bus virtio0: rpmsg host is online
> virtio_rpmsg_bus virtio0: creating channel rpmsg-timer-statistic addr 0x50
> ~ #
> ~ # lsmod
> virtio_rpmsg_bus 7575 0 - Live 0xbf024000
> zynq_remoteproc 4346 1 - Live 0xbf01f000
> remoteproc 13831 1 zynq_remoteproc, Live 0xbf018000
> virtio 2840 2 virtio_rpmsg_bus,remoteproc, Live 0xbf008000
> virtio_ring 4965 2 virtio_rpmsg_bus,remoteproc, Live 0xbf013000
> ~ #
>
> Can you see the same behavior on omap?
>
> Thanks,
> Michal
>
>
>
>
>
>
>
>

Re: [PATCH 1/7] remoteproc: resource table overhaul

[Ohad Ben-Cohen]

Hi Ludovic,

On Tue, May 22, 2012 at 12:14 PM, frq09524 <ludovic.barre@stericsson.com> wrote:
> Ohad, for alignment I can take the latest branch of kernel.org (remoteproc)
> branch for-next?

Sure, it's pretty much updated sans a few simple changes.

Thanks,
Ohad.

Re: [PATCH 1/7] remoteproc: resource table overhaul

[frq09524]

hi Ohad

In my previous patch, to find the correct subdevice that match with 
physical memory, I used pa member of rproc_mem_entry.
but today in these 2 resources: fw_rsc_trace, fw_rsc_vdev_vring, pa 
menber has been removed.

for fw_rsc_trace it's not a problem, because use rproc_da_to_va and 
carveout feature.

but for fw_rsc_vdev_vring, I think use da member, like this.

dev = rproc_dma_find_dev(rproc, (phys_addr_t) vring->da);
va = dma_alloc_coherent(dev, size, &dma, GFP_KERNEL);
if (!va) {
	dev_err(dev, "dma_alloc_coherent failed\n");
	return -EINVAL;
}

are you OK ?

/BR
Ludovic Barré

On 05/22/2012 11:22 AM, Ohad Ben-Cohen wrote:
> Hi Ludovic,
>
> On Tue, May 22, 2012 at 12:14 PM, frq09524<ludovic.barre@stericsson.com>  wrote:
>> Ohad, for alignment I can take the latest branch of kernel.org (remoteproc)
>> branch for-next?
>
> Sure, it's pretty much updated sans a few simple changes.
>
> Thanks,
> Ohad.

Re: [PATCH 1/7] remoteproc: resource table overhaul

[Ohad Ben-Cohen]

Hi Ludovic,

On Tue, May 22, 2012 at 3:51 PM, frq09524 <ludovic.barre@stericsson.com> wrote:
> In my previous patch, to find the correct subdevice that match with physical memory, I used pa member of rproc_mem_entry.

I'm not sure we want to find these subdevices using the physical
memory of the expected region.

Doing so means more work when setting up those numbers initially, and
may also break if/when those number changes.

What do you think about looking up those subdevices by names?

E.g. before allocating the vrings, we may look for a subdevice called
"vrings-mem". If one exists, we'll use it when invoking the DMA API.
If not, we'll just use the default rproc device.

This way we don't need to change the firmware if/when we change the
memory regions allocated for each purpose.

Thanks,
Ohad.

[PATCH 2/7] remoteproc: remoteproc_rpmsg -> remoteproc_virtio

[Ohad Ben-Cohen]

At this point remoteproc can only register a single VIRTIO_ID_RPMSG virtio
device.

This limitation is going away soon: remoteproc is getting support for
registering any number of virtio devices and of any type (as
published by the firmware of the remote processor).

Rename remoteproc_rpmsg.c to remoteproc_virtio.c in preparation of
this generalization work.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Brian Swetland <swetland@google.com>
Cc: Iliyan Malchev <malchev@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Mark Grosen <mgrosen@ti.com>
Cc: John Williams <john.williams@petalogix.com>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Loic PALLARDY <loic.pallardy@stericsson.com>
Cc: Ludovic BARRE <ludovic.barre@stericsson.com>
Cc: Omar Ramirez Luna <omar.luna@linaro.org>
Cc: Guzman Lugo Fernando <fernando.lugo@ti.com>
Cc: Anna Suman <s-anna@ti.com>
Cc: Clark Rob <rob@ti.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Saravana Kannan <skannan@codeaurora.org>
Cc: David Brown <davidb@codeaurora.org>
Cc: Kieran Bingham <kieranbingham@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
---
 drivers/remoteproc/Makefile                        |    2 +-
 .../{remoteproc_rpmsg.c => remoteproc_virtio.c}    |    0
 2 files changed, 1 insertions(+), 1 deletions(-)
 rename drivers/remoteproc/{remoteproc_rpmsg.c => remoteproc_virtio.c} (100%)

diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index df0897f..5445d9b 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -5,5 +5,5 @@
 obj-$(CONFIG_REMOTEPROC)		+= remoteproc.o
 remoteproc-y				:= remoteproc_core.o
 remoteproc-y				+= remoteproc_debugfs.o
-remoteproc-y				+= remoteproc_rpmsg.o
+remoteproc-y				+= remoteproc_virtio.o
 obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
diff --git a/drivers/remoteproc/remoteproc_rpmsg.c b/drivers/remoteproc/remoteproc_virtio.c
similarity index 100%
rename from drivers/remoteproc/remoteproc_rpmsg.c
rename to drivers/remoteproc/remoteproc_virtio.c
-- 
1.7.5.4

[PATCH 3/7] remoteproc: safer boot/shutdown order

[Ohad Ben-Cohen]

Boot the remote processor only after setting up the virtqueues,
and shut it down before deleting them.

Remote processors should obey virtio status changes, and
therefore not manipulate/trigger the virtqueues while the virtio
driver isn't ready, but it's just safer not to rely on that
(plus a vq access might already be inflight while a vdev
status is changed).

We also don't have yet status change notifications, but that's
a temporary limitation.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Brian Swetland <swetland@google.com>
Cc: Iliyan Malchev <malchev@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Mark Grosen <mgrosen@ti.com>
Cc: John Williams <john.williams@petalogix.com>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Loic PALLARDY <loic.pallardy@stericsson.com>
Cc: Ludovic BARRE <ludovic.barre@stericsson.com>
Cc: Omar Ramirez Luna <omar.luna@linaro.org>
Cc: Guzman Lugo Fernando <fernando.lugo@ti.com>
Cc: Anna Suman <s-anna@ti.com>
Cc: Clark Rob <rob@ti.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Saravana Kannan <skannan@codeaurora.org>
Cc: David Brown <davidb@codeaurora.org>
Cc: Kieran Bingham <kieranbingham@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
---
 drivers/remoteproc/remoteproc_virtio.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 4f73e81..78d8527 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -123,14 +123,14 @@ static void rproc_virtio_del_vqs(struct virtio_device *vdev)
 	struct virtqueue *vq, *n;
 	struct rproc *rproc = vdev_to_rproc(vdev);
 
+	/* power down the remote processor before deleting vqs */
+	rproc_shutdown(rproc);
+
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
 		struct rproc_virtio_vq_info *rpvq = vq->priv;
 		vring_del_virtqueue(vq);
 		kfree(rpvq);
 	}
-
-	/* power down the remote processor */
-	rproc_shutdown(rproc);
 }
 
 static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
@@ -145,13 +145,6 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 	if (nvqs != 2)
 		return -EINVAL;
 
-	/* boot the remote processor */
-	ret = rproc_boot(rproc);
-	if (ret) {
-		dev_err(rproc->dev, "rproc_boot() failed %d\n", ret);
-		goto error;
-	}
-
 	for (i = 0; i < nvqs; ++i) {
 		vqs[i] = rp_find_vq(vdev, i, callbacks[i], names[i]);
 		if (IS_ERR(vqs[i])) {
@@ -160,6 +153,13 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		}
 	}
 
+	/* now that the vqs are all set, boot the remote processor */
+	ret = rproc_boot(rproc);
+	if (ret) {
+		dev_err(rproc->dev, "rproc_boot() failed %d\n", ret);
+		goto error;
+	}
+
 	return 0;
 
 error:
-- 
1.7.5.4

[PATCH 4/7] remoteproc: remove the single rpmsg vdev limitation

[Ohad Ben-Cohen]

Now that the resource table supports publishing a virtio device
in a single resource entry, firmware images can start supporting
more than a single vdev.

This patch removes the single vdev limitation of the remoteproc
framework so multi-vdev firmwares can be leveraged: VDEV resource
entries are parsed when the rproc is registered, and as a result
their vrings are set up and the virtio devices are registered
(and they go away when the rproc goes away).

Moreover, we no longer only support VIRTIO_ID_RPMSG vdevs; any
virtio device type goes now. As a result, there's no more any
rpmsg-specific APIs or code in remoteproc: it all becomes generic
virtio handling.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Brian Swetland <swetland@google.com>
Cc: Iliyan Malchev <malchev@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Mark Grosen <mgrosen@ti.com>
Cc: John Williams <john.williams@petalogix.com>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Loic PALLARDY <loic.pallardy@stericsson.com>
Cc: Ludovic BARRE <ludovic.barre@stericsson.com>
Cc: Omar Ramirez Luna <omar.luna@linaro.org>
Cc: Guzman Lugo Fernando <fernando.lugo@ti.com>
Cc: Anna Suman <s-anna@ti.com>
Cc: Clark Rob <rob@ti.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Saravana Kannan <skannan@codeaurora.org>
Cc: David Brown <davidb@codeaurora.org>
Cc: Kieran Bingham <kieranbingham@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
---
 Documentation/remoteproc.txt             |    9 +-
 drivers/remoteproc/remoteproc_core.c     |  292 +++++++++++++++---------------
 drivers/remoteproc/remoteproc_internal.h |    6 +-
 drivers/remoteproc/remoteproc_virtio.c   |  140 +++++++--------
 include/linux/remoteproc.h               |   41 ++++-
 5 files changed, 260 insertions(+), 228 deletions(-)

diff --git a/Documentation/remoteproc.txt b/Documentation/remoteproc.txt
index 07057ca..70a048c 100644
--- a/Documentation/remoteproc.txt
+++ b/Documentation/remoteproc.txt
@@ -20,6 +20,11 @@ platform-specific remoteproc drivers only need to provide a few low-level
 handlers, and then all rpmsg drivers will then just work
 (for more information about the virtio-based rpmsg bus and its drivers,
 please read Documentation/rpmsg.txt).
+Registration of other types of virtio devices is now also possible. Firmwares
+just need to publish what kind of virtio devices do they support, and then
+remoteproc will add those devices. This makes it possible to reuse the
+existing virtio drivers with remote processor backends at a minimal development
+cost.
 
 2. User API
 
@@ -136,8 +141,6 @@ int dummy_rproc_example(struct rproc *my_rproc)
       If found, those virtio devices will be created and added, so as a result
       of registering this remote processor, additional virtio drivers might get
       probed.
-      Currently, though, we only support a single RPMSG virtio vdev per remote
-      processor.
 
   int rproc_unregister(struct rproc *rproc)
     - Unregister a remote processor, and decrement its refcount.
@@ -174,7 +177,7 @@ struct rproc_ops {
 };
 
 Every remoteproc implementation should at least provide the ->start and ->stop
-handlers. If rpmsg functionality is also desired, then the ->kick handler
+handlers. If rpmsg/virtio functionality is also desired, then the ->kick handler
 should be provided as well.
 
 The ->start() handler takes an rproc handle and should then power on the
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 1034845..ca02f12 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -52,8 +52,8 @@ static void klist_rproc_put(struct klist_node *n);
  * We need this in order to support name-based lookups (needed by the
  * rproc_get_by_name()).
  *
- * That said, we don't use rproc_get_by_name() anymore within the rpmsg
- * framework. The use cases that do require its existence should be
+ * That said, we don't use rproc_get_by_name() at this point.
+ * The use cases that do require its existence should be
  * scrutinized, and hopefully migrated to rproc_boot() using device-based
  * binding.
  *
@@ -279,80 +279,112 @@ rproc_load_segments(struct rproc *rproc, const u8 *elf_data, size_t len)
 	return ret;
 }
 
-/**
- * rproc_handle_early_vdev() - early handle a virtio header resource
- * @rproc: the remote processor
- * @rsc: the resource descriptor
- * @avail: size of available data (for sanity checking the image)
- *
- * The existence of this virtio hdr resource entry means that the firmware
- * of this @rproc supports this virtio device.
- *
- * Currently we support only a single virtio device of type VIRTIO_ID_RPMSG,
- * but the plan is to remove this limitation and support any number
- * of virtio devices (and of any type). We'll also add support for dynamically
- * adding (and removing) virtio devices over the rpmsg bus, but simple
- * firmwares that doesn't want to get involved with rpmsg will be able
- * to simply use the resource table for this.
- *
- * Returns 0 on success, or an appropriate error code otherwise
- */
-static int rproc_handle_early_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
-								int avail)
+static int
+__rproc_handle_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i)
 {
-	struct rproc_vdev *rvdev;
+	struct rproc *rproc = rvdev->rproc;
+	struct device *dev = rproc->dev;
+	struct fw_rsc_vdev_vring *vring = &rsc->vring[i];
+	dma_addr_t dma;
+	void *va;
+	int ret, size, notifyid;
 
-	/* make sure resource isn't truncated */
-	if (sizeof(*rsc) > avail) {
-		dev_err(rproc->dev, "vdev rsc is truncated\n");
+	dev_dbg(dev, "vdev rsc: vring%d: da %x, qsz %d, align %d\n",
+				i, vring->da, vring->num, vring->align);
+
+	/* make sure reserved bytes are zeroes */
+	if (vring->reserved) {
+		dev_err(dev, "vring rsc has non zero reserved bytes\n");
 		return -EINVAL;
 	}
 
-	/* we only support VIRTIO_ID_RPMSG devices for now */
-	if (rsc->id != VIRTIO_ID_RPMSG) {
-		dev_warn(rproc->dev, "unsupported vdev: %d\n", rsc->id);
+	/* the firmware must provide the expected queue size */
+	if (!vring->num) {
+		dev_err(dev, "invalid qsz (%d)\n", vring->num);
 		return -EINVAL;
 	}
 
-	/* we only support a single vdev per rproc for now */
-	if (rproc->rvdev) {
-		dev_warn(rproc->dev, "redundant vdev entry\n");
+	/* actual size of vring (in bytes) */
+	size = PAGE_ALIGN(vring_size(vring->num, AMP_VRING_ALIGN));
+
+	if (!idr_pre_get(&rproc->notifyids, GFP_KERNEL)) {
+		dev_err(dev, "idr_pre_get failed\n");
+		return -ENOMEM;
+	}
+
+	/*
+	 * Allocate non-cacheable memory for the vring. In the future
+	 * this call will also configure the IOMMU for us
+	 */
+	va = dma_alloc_coherent(dev, size, &dma, GFP_KERNEL);
+	if (!va) {
+		dev_err(dev, "dma_alloc_coherent failed\n");
 		return -EINVAL;
 	}
 
-	rvdev = kzalloc(sizeof(struct rproc_vdev), GFP_KERNEL);
-	if (!rvdev)
-		return -ENOMEM;
+	/* assign an rproc-wide unique index for this vring */
+	/* TODO: assign a notifyid for rvdev updates as well */
+	ret = idr_get_new(&rproc->notifyids, &rvdev->vring[i], &notifyid);
+	if (ret) {
+		dev_err(dev, "idr_get_new failed: %d\n", ret);
+		dma_free_coherent(dev, size, va, dma);
+		return ret;
+	}
 
-	/* remember the device features */
-	rvdev->dfeatures = rsc->dfeatures;
+	/* let the rproc know the da and notifyid of this vring */
+	/* TODO: expose this to remote processor */
+	vring->da = dma;
+	vring->notifyid = notifyid;
 
-	rproc->rvdev = rvdev;
-	rvdev->rproc = rproc;
+	dev_dbg(dev, "vring%d: va %p dma %x size %x idr %d\n", i, va,
+					dma, size, notifyid);
+
+	rvdev->vring[i].len = vring->num;
+	rvdev->vring[i].va = va;
+	rvdev->vring[i].dma = dma;
+	rvdev->vring[i].notifyid = notifyid;
+	rvdev->vring[i].rvdev = rvdev;
 
 	return 0;
 }
 
+static void __rproc_free_vrings(struct rproc_vdev *rvdev, int i)
+{
+	struct rproc *rproc = rvdev->rproc;
+
+	for (i--; i > 0; i--) {
+		struct rproc_vring *rvring = &rvdev->vring[i];
+		int size = PAGE_ALIGN(vring_size(rvring->len, AMP_VRING_ALIGN));
+
+		dma_free_coherent(rproc->dev, size, rvring->va, rvring->dma);
+		idr_remove(&rproc->notifyids, rvring->notifyid);
+	}
+}
+
 /**
  * rproc_handle_vdev() - handle a vdev fw resource
  * @rproc: the remote processor
  * @rsc: the vring resource descriptor
  * @avail: size of available data (for sanity checking the image)
  *
- * This resource entry requires allocation of non-cacheable memory
- * for a virtio vring. Currently we only support two vrings per remote
- * processor, required for the virtio rpmsg device.
- *
- * The 'len' member of @rsc should contain the number of buffers this vring
- * support and 'da' should either contain the device address where
- * the remote processor is expecting the vring, or indicate that
- * dynamically allocation of the vring's device address is supported.
- *
- * Note: 'da' is currently not handled. This will be revised when the generic
- * iommu-based DMA API will arrive, or a dynanic & non-iommu use case show
- * up. Meanwhile, statically-addressed iommu-based images should use
- * RSC_DEVMEM resource entries to map their require 'da' to the physical
- * address of their base CMA region.
+ * This resource entry requests the host to statically register a virtio
+ * device (vdev), and setup everything needed to support it. It contains
+ * everything needed to make it possible: the virtio device id, virtio
+ * device features, vrings information, virtio config space, etc...
+ *
+ * Before registering the vdev, the vrings are allocated from non-cacheable
+ * physically contiguous memory. Currently we only support two vrings per
+ * remote processor (temporary limitation). We might also want to consider
+ * doing the vring allocation only later when ->find_vqs() is invoked, and
+ * then release them upon ->del_vqs().
+ *
+ * Note: @da is currently not really handled correctly: we dynamically
+ * allocate it using the DMA API, ignoring requested hard coded addresses,
+ * and we don't take care of any required IOMMU programming. This is all
+ * going to be taken care of when the generic iommu-based DMA API will be
+ * merged. Meanwhile, statically-addressed iommu-based firmware images should
+ * use RSC_DEVMEM resource entries to map their required @da to the physical
+ * address of their base CMA region (ouch, hacky!).
  *
  * Returns 0 on success, or an appropriate error code otherwise
  */
@@ -360,8 +392,8 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 								int avail)
 {
 	struct device *dev = rproc->dev;
-	struct rproc_vdev *rvdev = rproc->rvdev;
-	int i;
+	struct rproc_vdev *rvdev;
+	int i, ret;
 
 	/* make sure resource isn't truncated */
 	if (sizeof(*rsc) + rsc->num_of_vrings * sizeof(struct fw_rsc_vdev_vring)
@@ -379,61 +411,41 @@ static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 	dev_dbg(dev, "vdev rsc: id %d, dfeatures %x, cfg len %d, %d vrings\n",
 		rsc->id, rsc->dfeatures, rsc->config_len, rsc->num_of_vrings);
 
-	/* no vdev is in place ? */
-	if (!rvdev) {
-		dev_err(dev, "vring requested without a virtio dev entry\n");
-		return -EINVAL;
-	}
-
-	/* we currently support two vrings per rproc (for rx and tx) */
-	if (rsc->num_of_vrings != ARRAY_SIZE(rvdev->vring)) {
+	/* we currently support only two vrings per rvdev */
+	if (rsc->num_of_vrings > ARRAY_SIZE(rvdev->vring)) {
 		dev_err(dev, "too many vrings: %d\n", rsc->num_of_vrings);
 		return -EINVAL;
 	}
 
-	/* initialize the vrings */
-	for (i = 0; i < rsc->num_of_vrings; i++) {
-		struct fw_rsc_vdev_vring *vring = &rsc->vring[i];
-		dma_addr_t dma;
-		int size;
-		void *va;
-
-		/* make sure reserved bytes are zeroes */
-		if (vring->reserved) {
-			dev_err(dev, "vring rsc has non zero reserved bytes\n");
-			return -EINVAL;
-		}
+	rvdev = kzalloc(sizeof(struct rproc_vdev), GFP_KERNEL);
+	if (!rvdev)
+		return -ENOMEM;
 
-		/* the firmware must provide the expected queue size */
-		if (!vring->num) {
-			dev_err(dev, "missing expected queue size\n");
-			/* potential cleanups are taken care of later on */
-			return -EINVAL;
-		}
+	rvdev->rproc = rproc;
 
-		/* actual size of vring (in bytes) */
-		size = PAGE_ALIGN(vring_size(vring->num, AMP_VRING_ALIGN));
+	/* allocate the vrings */
+	for (i = 0; i < rsc->num_of_vrings; i++) {
+		ret = __rproc_handle_vring(rvdev, rsc, i);
+		if (ret)
+			goto free_vrings;
+	}
 
-		/*
-		 * Allocate non-cacheable memory for the vring. In the future
-		 * this call will also configure the IOMMU for us
-		 */
-		va = dma_alloc_coherent(dev, size, &dma, GFP_KERNEL);
-		if (!va) {
-			dev_err(dev, "dma_alloc_coherent failed\n");
-			/* potential cleanups are taken care of later on */
-			return -EINVAL;
-		}
+	/* remember the device features */
+	rvdev->dfeatures = rsc->dfeatures;
 
-		dev_dbg(dev, "vring%d: va %p dma %x qsz %d ring size %x\n", i,
-						va, dma, vring->num, size);
+	list_add_tail(&rvdev->node, &rproc->rvdevs);
 
-		rvdev->vring[i].len = vring->num;
-		rvdev->vring[i].va = va;
-		rvdev->vring[i].dma = dma;
-	}
+	/* it is now safe to add the virtio device */
+	ret = rproc_add_virtio_dev(rvdev, rsc->id);
+	if (ret)
+		goto free_vrings;
 
 	return 0;
+
+free_vrings:
+	__rproc_free_vrings(rvdev, i);
+	kfree(rvdev);
+	return ret;
 }
 
 /**
@@ -731,7 +743,7 @@ static rproc_handle_resource_t rproc_handle_rsc[] = {
 	[RSC_CARVEOUT] = (rproc_handle_resource_t)rproc_handle_carveout,
 	[RSC_DEVMEM] = (rproc_handle_resource_t)rproc_handle_devmem,
 	[RSC_TRACE] = (rproc_handle_resource_t)rproc_handle_trace,
-	[RSC_VDEV] = (rproc_handle_resource_t)rproc_handle_vdev,
+	[RSC_VDEV] = NULL, /* VDEVs were handled upon registrarion */
 };
 
 /* handle firmware resource entries before booting the remote processor */
@@ -784,6 +796,7 @@ rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table *table, int l
 		int offset = table->offset[i];
 		struct fw_rsc_hdr *hdr = (void *)table + offset;
 		int avail = len - offset - sizeof(*hdr);
+		struct fw_rsc_vdev *vrsc;
 
 		/* make sure table isn't truncated */
 		if (avail < 0) {
@@ -793,12 +806,14 @@ rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table *table, int l
 
 		dev_dbg(dev, "%s: rsc type %d\n", __func__, hdr->type);
 
-		if (hdr->type == RSC_VDEV) {
-			struct fw_rsc_vdev *vrsc =
-					(struct fw_rsc_vdev *)hdr->data;
-			ret = rproc_handle_early_vdev(rproc, vrsc, avail);
+		if (hdr->type != RSC_VDEV)
+			continue;
+
+		vrsc = (struct fw_rsc_vdev *)hdr->data;
+
+		ret = rproc_handle_vdev(rproc, vrsc, avail);
+		if (ret)
 			break;
-		}
 	}
 
 	return ret;
@@ -889,14 +904,12 @@ static int rproc_handle_resources(struct rproc *rproc, const u8 *elf_data,
  * @rproc: rproc handle
  *
  * This function will free all resources acquired for @rproc, and it
- * is called when @rproc shuts down, or just failed booting.
+ * is called whenever @rproc either shuts down or fails to boot.
  */
 static void rproc_resource_cleanup(struct rproc *rproc)
 {
 	struct rproc_mem_entry *entry, *tmp;
 	struct device *dev = rproc->dev;
-	struct rproc_vdev *rvdev = rproc->rvdev;
-	int i;
 
 	/* clean up debugfs trace entries */
 	list_for_each_entry_safe(entry, tmp, &rproc->traces, node) {
@@ -906,23 +919,6 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 		kfree(entry);
 	}
 
-	/* free the coherent memory allocated for the vrings */
-	for (i = 0; rvdev && i < ARRAY_SIZE(rvdev->vring); i++) {
-		int qsz = rvdev->vring[i].len;
-		void *va = rvdev->vring[i].va;
-		int dma = rvdev->vring[i].dma;
-
-		/* virtqueue size is expressed in number of buffers supported */
-		if (qsz) {
-			/* how many bytes does this vring really occupy ? */
-			int size = PAGE_ALIGN(vring_size(qsz, AMP_VRING_ALIGN));
-
-			dma_free_coherent(rproc->dev, size, va, dma);
-
-			rvdev->vring[i].len = 0;
-		}
-	}
-
 	/* clean up carveout allocations */
 	list_for_each_entry_safe(entry, tmp, &rproc->carveouts, node) {
 		dma_free_coherent(dev, entry->len, entry->va, entry->dma);
@@ -1100,11 +1096,6 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 		goto out;
 	}
 
-	/* add the virtio device (currently only rpmsg vdevs are supported) */
-	ret = rproc_add_rpmsg_vdev(rproc);
-	if (ret)
-		goto out;
-
 out:
 	if (fw)
 		release_firmware(fw);
@@ -1266,13 +1257,23 @@ EXPORT_SYMBOL(rproc_shutdown);
 void rproc_release(struct kref *kref)
 {
 	struct rproc *rproc = container_of(kref, struct rproc, refcount);
+	struct rproc_vdev *rvdev, *rvtmp;
 
 	dev_info(rproc->dev, "removing %s\n", rproc->name);
 
 	rproc_delete_debug_dir(rproc);
 
-	/* at this point no one holds a reference to rproc anymore */
-	kfree(rproc);
+	/* clean up remote vdev entries */
+	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node) {
+		__rproc_free_vrings(rvdev, RVDEV_NUM_VRINGS);
+		list_del(&rvdev->node);
+	}
+
+	/*
+	 * At this point no one holds a reference to rproc anymore,
+	 * so we can directly unroll rproc_alloc()
+	 */
+	rproc_free(rproc);
 }
 
 /* will be called when an rproc is added to the rprocs klist */
@@ -1316,7 +1317,7 @@ static struct rproc *next_rproc(struct klist_iter *i)
  * use rproc_put() to decrement it back once rproc isn't needed anymore.
  *
  * Note: currently this function (and its counterpart rproc_put()) are not
- * used anymore by the rpmsg subsystem. We need to scrutinize the use cases
+ * being used. We need to scrutinize the use cases
  * that still need them, and see if we can migrate them to use the non
  * name-based boot/shutdown interface.
  */
@@ -1391,11 +1392,8 @@ EXPORT_SYMBOL(rproc_put);
  * firmware.
  *
  * If found, those virtio devices will be created and added, so as a result
- * of registering this remote processor, additional virtio drivers will be
+ * of registering this remote processor, additional virtio drivers might be
  * probed.
- *
- * Currently, though, we only support a single RPMSG virtio vdev per remote
- * processor.
  */
 int rproc_register(struct rproc *rproc)
 {
@@ -1418,7 +1416,7 @@ int rproc_register(struct rproc *rproc)
 
 	/*
 	 * We must retrieve early virtio configuration info from
-	 * the firmware (e.g. whether to register a virtio rpmsg device,
+	 * the firmware (e.g. whether to register a virtio device,
 	 * what virtio features does it support, ...).
 	 *
 	 * We're initiating an asynchronous firmware loading, so we can
@@ -1487,9 +1485,12 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 
 	mutex_init(&rproc->lock);
 
+	idr_init(&rproc->notifyids);
+
 	INIT_LIST_HEAD(&rproc->carveouts);
 	INIT_LIST_HEAD(&rproc->mappings);
 	INIT_LIST_HEAD(&rproc->traces);
+	INIT_LIST_HEAD(&rproc->rvdevs);
 
 	rproc->state = RPROC_OFFLINE;
 
@@ -1509,6 +1510,9 @@ EXPORT_SYMBOL(rproc_alloc);
  */
 void rproc_free(struct rproc *rproc)
 {
+	idr_remove_all(&rproc->notifyids);
+	idr_destroy(&rproc->notifyids);
+
 	kfree(rproc);
 }
 EXPORT_SYMBOL(rproc_free);
@@ -1535,18 +1539,22 @@ EXPORT_SYMBOL(rproc_free);
  */
 int rproc_unregister(struct rproc *rproc)
 {
+	struct rproc_vdev *rvdev;
+
 	if (!rproc)
 		return -EINVAL;
 
 	/* if rproc is just being registered, wait */
 	wait_for_completion(&rproc->firmware_loading_complete);
 
-	/* was an rpmsg vdev created ? */
-	if (rproc->rvdev)
-		rproc_remove_rpmsg_vdev(rproc);
+	/* clean up remote vdev entries */
+	list_for_each_entry(rvdev, &rproc->rvdevs, node)
+		rproc_remove_virtio_dev(rvdev);
 
-	klist_remove(&rproc->node);
+	/* the rproc is downref'ed as soon as it's removed from the klist */
+	klist_del(&rproc->node);
 
+	/* the rproc will only be released after its refcount drops to zero */
 	kref_put(&rproc->refcount, rproc_release);
 
 	return 0;
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 8b2fc40..9f336d6 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -28,9 +28,9 @@ struct rproc;
 void rproc_release(struct kref *kref);
 irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
 
-/* from remoteproc_rpmsg.c */
-int rproc_add_rpmsg_vdev(struct rproc *);
-void rproc_remove_rpmsg_vdev(struct rproc *rproc);
+/* from remoteproc_virtio.c */
+int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id);
+void rproc_remove_virtio_dev(struct rproc_vdev *rvdev);
 
 /* from remoteproc_debugfs.c */
 void rproc_remove_trace_file(struct dentry *tfile);
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 78d8527..0700410 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -19,7 +19,6 @@
 
 #include <linux/export.h>
 #include <linux/remoteproc.h>
-#include <linux/rpmsg.h>
 #include <linux/virtio.h>
 #include <linux/virtio_config.h>
 #include <linux/virtio_ids.h>
@@ -30,45 +29,41 @@
 
 #include "remoteproc_internal.h"
 
-/**
- * struct rproc_virtio_vq_info - virtqueue state
- * @vq_id: a unique index of this virtqueue (unique for this @rproc)
- * @rproc: handle to the remote processor
- *
- * Such a struct will be maintained for every virtqueue we're
- * using to communicate with the remote processor
- */
-struct rproc_virtio_vq_info {
-	__u16 vq_id;
-	struct rproc *rproc;
-};
-
 /* kick the remote processor, and let it know which virtqueue to poke at */
 static void rproc_virtio_notify(struct virtqueue *vq)
 {
-	struct rproc_virtio_vq_info *rpvq = vq->priv;
-	struct rproc *rproc = rpvq->rproc;
+	struct rproc_vring *rvring = vq->priv;
+	struct rproc *rproc = rvring->rvdev->rproc;
+	int notifyid = rvring->notifyid;
 
-	dev_dbg(rproc->dev, "kicking vq id: %d\n", rpvq->vq_id);
+	dev_dbg(rproc->dev, "kicking vq index: %d\n", notifyid);
 
-	rproc->ops->kick(rproc, rpvq->vq_id);
+	rproc->ops->kick(rproc, notifyid);
 }
 
 /**
  * rproc_vq_interrupt() - tell remoteproc that a virtqueue is interrupted
  * @rproc: handle to the remote processor
- * @vq_id: index of the signalled virtqueue
+ * @notifyid: index of the signalled virtqueue (unique per this @rproc)
  *
  * This function should be called by the platform-specific rproc driver,
  * when the remote processor signals that a specific virtqueue has pending
  * messages available.
  *
- * Returns IRQ_NONE if no message was found in the @vq_id virtqueue,
+ * Returns IRQ_NONE if no message was found in the @notifyid virtqueue,
  * and otherwise returns IRQ_HANDLED.
  */
-irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
+irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int notifyid)
 {
-	return vring_interrupt(0, rproc->rvdev->vq[vq_id]);
+	struct rproc_vring *rvring;
+
+	dev_dbg(rproc->dev, "vq index %d is interrupted\n", notifyid);
+
+	rvring = idr_find(&rproc->notifyids, notifyid);
+	if (!rvring || !rvring->vq)
+		return IRQ_NONE;
+
+	return vring_interrupt(0, rvring->vq);
 }
 EXPORT_SYMBOL(rproc_vq_interrupt);
 
@@ -77,24 +72,28 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
 				    void (*callback)(struct virtqueue *vq),
 				    const char *name)
 {
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
 	struct rproc *rproc = vdev_to_rproc(vdev);
-	struct rproc_vdev *rvdev = rproc->rvdev;
-	struct rproc_virtio_vq_info *rpvq;
+	struct rproc_vring *rvring;
 	struct virtqueue *vq;
 	void *addr;
-	int ret, len;
+	int len, size;
 
-	rpvq = kmalloc(sizeof(*rpvq), GFP_KERNEL);
-	if (!rpvq)
-		return ERR_PTR(-ENOMEM);
+	/* we're temporarily limited to two virtqueues per rvdev */
+	if (id >= ARRAY_SIZE(rvdev->vring))
+		return ERR_PTR(-EINVAL);
+
+	rvring = &rvdev->vring[id];
 
-	rpvq->rproc = rproc;
-	rpvq->vq_id = id;
+	addr = rvring->va;
+	len = rvring->len;
 
-	addr = rvdev->vring[id].va;
-	len = rvdev->vring[id].len;
+	/* zero vring */
+	size = vring_size(len, rvring->align);
+	memset(addr, 0, size);
 
-	dev_dbg(rproc->dev, "vring%d: va %p qsz %d\n", id, addr, len);
+	dev_dbg(rproc->dev, "vring%d: va %p qsz %d notifyid %d\n",
+					id, addr, len, rvring->notifyid);
 
 	/*
 	 * Create the new vq, and tell virtio we're not interested in
@@ -104,32 +103,28 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
 					rproc_virtio_notify, callback, name);
 	if (!vq) {
 		dev_err(rproc->dev, "vring_new_virtqueue %s failed\n", name);
-		ret = -ENOMEM;
-		goto free_rpvq;
+		return ERR_PTR(-ENOMEM);
 	}
 
-	rvdev->vq[id] = vq;
-	vq->priv = rpvq;
+	rvring->vq = vq;
+	vq->priv = rvring;
 
 	return vq;
-
-free_rpvq:
-	kfree(rpvq);
-	return ERR_PTR(ret);
 }
 
 static void rproc_virtio_del_vqs(struct virtio_device *vdev)
 {
 	struct virtqueue *vq, *n;
 	struct rproc *rproc = vdev_to_rproc(vdev);
+	struct rproc_vring *rvring;
 
 	/* power down the remote processor before deleting vqs */
 	rproc_shutdown(rproc);
 
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
-		struct rproc_virtio_vq_info *rpvq = vq->priv;
+		rvring = vq->priv;
+		rvring->vq = NULL;
 		vring_del_virtqueue(vq);
-		kfree(rpvq);
 	}
 }
 
@@ -141,10 +136,6 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 	struct rproc *rproc = vdev_to_rproc(vdev);
 	int i, ret;
 
-	/* we maintain two virtqueues per remote processor (for RX and TX) */
-	if (nvqs != 2)
-		return -EINVAL;
-
 	for (i = 0; i < nvqs; ++i) {
 		vqs[i] = rp_find_vq(vdev, i, callbacks[i], names[i]);
 		if (IS_ERR(vqs[i])) {
@@ -170,7 +161,7 @@ error:
 /*
  * We don't support yet real virtio status semantics.
  *
- * The plan is to provide this via the VIRTIO HDR resource entry
+ * The plan is to provide this via the VDEV resource entry
  * which is part of the firmware: this way the remote processor
  * will be able to access the status values as set by us.
  */
@@ -181,7 +172,7 @@ static u8 rproc_virtio_get_status(struct virtio_device *vdev)
 
 static void rproc_virtio_set_status(struct virtio_device *vdev, u8 status)
 {
-	dev_dbg(&vdev->dev, "new status: %d\n", status);
+	dev_dbg(&vdev->dev, "status: %d\n", status);
 }
 
 static void rproc_virtio_reset(struct virtio_device *vdev)
@@ -192,15 +183,14 @@ static void rproc_virtio_reset(struct virtio_device *vdev)
 /* provide the vdev features as retrieved from the firmware */
 static u32 rproc_virtio_get_features(struct virtio_device *vdev)
 {
-	struct rproc *rproc = vdev_to_rproc(vdev);
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
 
-	/* we only support a single vdev device for now */
-	return rproc->rvdev->dfeatures;
+	return rvdev->dfeatures;
 }
 
 static void rproc_virtio_finalize_features(struct virtio_device *vdev)
 {
-	struct rproc *rproc = vdev_to_rproc(vdev);
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
 
 	/* Give virtio_ring a chance to accept features */
 	vring_transport_features(vdev);
@@ -214,7 +204,7 @@ static void rproc_virtio_finalize_features(struct virtio_device *vdev)
 	 * fixed as part of a small resource table overhaul and then an
 	 * extension of the virtio resource entries.
 	 */
-	rproc->rvdev->gfeatures = vdev->features[0];
+	rvdev->gfeatures = vdev->features[0];
 }
 
 static struct virtio_config_ops rproc_virtio_config_ops = {
@@ -244,26 +234,25 @@ static void rproc_vdev_release(struct device *dev)
 }
 
 /**
- * rproc_add_rpmsg_vdev() - create an rpmsg virtio device
- * @rproc: the rproc handle
+ * rproc_add_virtio_dev() - register an rproc-induced virtio device
+ * @rvdev: the remote vdev
  *
- * This function is called if virtio rpmsg support was found in the
- * firmware of the remote processor.
+ * This function registers a virtio device. This vdev's partent is
+ * the rproc device.
  *
- * Today we only support creating a single rpmsg vdev (virtio device),
- * but the plan is to remove this limitation. At that point this interface
- * will be revised/extended.
+ * Returns 0 on success or an appropriate error value otherwise.
  */
-int rproc_add_rpmsg_vdev(struct rproc *rproc)
+int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 {
+	struct rproc *rproc = rvdev->rproc;
 	struct device *dev = rproc->dev;
-	struct rproc_vdev *rvdev = rproc->rvdev;
+	struct virtio_device *vdev = &rvdev->vdev;
 	int ret;
 
-	rvdev->vdev.id.device	= VIRTIO_ID_RPMSG,
-	rvdev->vdev.config	= &rproc_virtio_config_ops,
-	rvdev->vdev.dev.parent	= dev;
-	rvdev->vdev.dev.release	= rproc_vdev_release;
+	vdev->id.device	= id,
+	vdev->config = &rproc_virtio_config_ops,
+	vdev->dev.parent = dev;
+	vdev->dev.release = rproc_vdev_release;
 
 	/*
 	 * We're indirectly making a non-temporary copy of the rproc pointer
@@ -275,25 +264,26 @@ int rproc_add_rpmsg_vdev(struct rproc *rproc)
 	 */
 	kref_get(&rproc->refcount);
 
-	ret = register_virtio_device(&rvdev->vdev);
+	ret = register_virtio_device(vdev);
 	if (ret) {
 		kref_put(&rproc->refcount, rproc_release);
 		dev_err(dev, "failed to register vdev: %d\n", ret);
+		goto out;
 	}
 
+	dev_info(dev, "registered %s (type %d)\n", dev_name(&vdev->dev), id);
+
+out:
 	return ret;
 }
 
 /**
- * rproc_remove_rpmsg_vdev() - remove an rpmsg vdev device
- * @rproc: the rproc handle
+ * rproc_remove_virtio_dev() - remove an rproc-induced virtio device
+ * @rvdev: the remote vdev
  *
- * This function is called whenever @rproc is removed _iff_ an rpmsg
- * vdev was created beforehand.
+ * This function unregisters an existing virtio device.
  */
-void rproc_remove_rpmsg_vdev(struct rproc *rproc)
+void rproc_remove_virtio_dev(struct rproc_vdev *rvdev)
 {
-	struct rproc_vdev *rvdev = rproc->rvdev;
-
 	unregister_virtio_device(&rvdev->vdev);
 }
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 6040f83..7750d8a 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -41,6 +41,7 @@
 #include <linux/mutex.h>
 #include <linux/virtio.h>
 #include <linux/completion.h>
+#include <linux/idr.h>
 
 /*
  * The alignment between the consumer and producer parts of the vring.
@@ -387,7 +388,8 @@ enum rproc_state {
  * @mappings: list of iommu mappings we initiated, needed on shutdown
  * @firmware_loading_complete: marks e/o asynchronous firmware loading
  * @bootaddr: address of first instruction to boot rproc with (optional)
- * @rvdev: virtio device (we only support a single rpmsg virtio device for now)
+ * @rvdevs: list of remote virtio devices
+ * @notifyids: idr for dynamically assigning rproc-wide unique notify ids
  */
 struct rproc {
 	struct klist_node node;
@@ -408,23 +410,47 @@ struct rproc {
 	struct list_head mappings;
 	struct completion firmware_loading_complete;
 	u32 bootaddr;
+	struct list_head rvdevs;
+	struct idr notifyids;
+};
+
+/* we currently support only two vrings per rvdev */
+#define RVDEV_NUM_VRINGS 2
+
+/**
+ * struct rproc_vring - remoteproc vring state
+ * @va:	virtual address
+ * @dma: dma address
+ * @len: length, in bytes
+ * @da: device address
+ * @notifyid: rproc-specific unique vring index
+ * @rvdev: remote vdev
+ * @vq: the virtqueue of this vring
+ */
+struct rproc_vring {
+	void *va;
+	dma_addr_t dma;
+	int len;
+	u32 da;
+	int notifyid;
 	struct rproc_vdev *rvdev;
+	struct virtqueue *vq;
 };
 
 /**
  * struct rproc_vdev - remoteproc state for a supported virtio device
+ * @node: list node
  * @rproc: the rproc handle
  * @vdev: the virio device
- * @vq: the virtqueues for this vdev
  * @vring: the vrings for this vdev
  * @dfeatures: virtio device features
  * @gfeatures: virtio guest features
  */
 struct rproc_vdev {
+	struct list_head node;
 	struct rproc *rproc;
 	struct virtio_device vdev;
-	struct virtqueue *vq[2];
-	struct rproc_mem_entry vring[2];
+	struct rproc_vring vring[RVDEV_NUM_VRINGS];
 	unsigned long dfeatures;
 	unsigned long gfeatures;
 };
@@ -442,9 +468,14 @@ int rproc_unregister(struct rproc *rproc);
 int rproc_boot(struct rproc *rproc);
 void rproc_shutdown(struct rproc *rproc);
 
+static inline struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
+{
+	return container_of(vdev, struct rproc_vdev, vdev);
+}
+
 static inline struct rproc *vdev_to_rproc(struct virtio_device *vdev)
 {
-	struct rproc_vdev *rvdev = container_of(vdev, struct rproc_vdev, vdev);
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
 
 	return rvdev->rproc;
 }
-- 
1.7.5.4

[PATCH 5/7] remoteproc/omap: remove the mbox_callback limitation

[Ohad Ben-Cohen]

Now that remoteproc supports any number of virtio devices,
the previous sanity check in omap_rproc_mbox_callback
doesn't make sense anymore.

Remove that so we can start supporting multiple vdevs.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Brian Swetland <swetland@google.com>
Cc: Iliyan Malchev <malchev@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Mark Grosen <mgrosen@ti.com>
Cc: John Williams <john.williams@petalogix.com>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Loic PALLARDY <loic.pallardy@stericsson.com>
Cc: Ludovic BARRE <ludovic.barre@stericsson.com>
Cc: Omar Ramirez Luna <omar.luna@linaro.org>
Cc: Guzman Lugo Fernando <fernando.lugo@ti.com>
Cc: Anna Suman <s-anna@ti.com>
Cc: Clark Rob <rob@ti.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Saravana Kannan <skannan@codeaurora.org>
Cc: David Brown <davidb@codeaurora.org>
Cc: Kieran Bingham <kieranbingham@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
---
 drivers/remoteproc/omap_remoteproc.c |   11 +----------
 1 files changed, 1 insertions(+), 10 deletions(-)

diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c
index aa3ce52..69425c4 100644
--- a/drivers/remoteproc/omap_remoteproc.c
+++ b/drivers/remoteproc/omap_remoteproc.c
@@ -80,16 +80,7 @@ static int omap_rproc_mbox_callback(struct notifier_block *this,
 		dev_info(dev, "received echo reply from %s\n", name);
 		break;
 	default:
-		/* ignore vq indices which are too large to be valid */
-		if (msg >= 2) {
-			dev_warn(dev, "invalid mbox msg: 0x%x\n", msg);
-			break;
-		}
-
-		/*
-		 * At this point, 'msg' contains the index of the vring
-		 * which was just triggered.
-		 */
+		/* msg contains the index of the triggered vring */
 		if (rproc_vq_interrupt(oproc->rproc, msg) == IRQ_NONE)
 			dev_dbg(dev, "no message was found in vqid %d\n", msg);
 	}
-- 
1.7.5.4

[PATCH 6/7] remoteproc: remove the hardcoded vring alignment

[Ohad Ben-Cohen]

Remove the hardcoded vring alignment of 4096 bytes,
and instead utilize tha vring alignment as specified in
the resource table.

This is needed for remote processors that have rigid
memory requirement, and which have found the alignment of
4096 bytes to be excessively big.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Brian Swetland <swetland@google.com>
Cc: Iliyan Malchev <malchev@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Mark Grosen <mgrosen@ti.com>
Cc: John Williams <john.williams@petalogix.com>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Loic PALLARDY <loic.pallardy@stericsson.com>
Cc: Ludovic BARRE <ludovic.barre@stericsson.com>
Cc: Omar Ramirez Luna <omar.luna@linaro.org>
Cc: Guzman Lugo Fernando <fernando.lugo@ti.com>
Cc: Anna Suman <s-anna@ti.com>
Cc: Clark Rob <rob@ti.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Saravana Kannan <skannan@codeaurora.org>
Cc: David Brown <davidb@codeaurora.org>
Cc: Kieran Bingham <kieranbingham@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
---
 drivers/remoteproc/remoteproc_core.c   |   12 +++++++-----
 drivers/remoteproc/remoteproc_virtio.c |    2 +-
 include/linux/remoteproc.h             |    9 ++-------
 3 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ca02f12..9be5dad 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -298,14 +298,15 @@ __rproc_handle_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i)
 		return -EINVAL;
 	}
 
-	/* the firmware must provide the expected queue size */
-	if (!vring->num) {
-		dev_err(dev, "invalid qsz (%d)\n", vring->num);
+	/* verify queue size and vring alignment are sane */
+	if (!vring->num || !vring->align) {
+		dev_err(dev, "invalid qsz (%d) or alignment (%d)\n",
+						vring->num, vring->align);
 		return -EINVAL;
 	}
 
 	/* actual size of vring (in bytes) */
-	size = PAGE_ALIGN(vring_size(vring->num, AMP_VRING_ALIGN));
+	size = PAGE_ALIGN(vring_size(vring->num, vring->align));
 
 	if (!idr_pre_get(&rproc->notifyids, GFP_KERNEL)) {
 		dev_err(dev, "idr_pre_get failed\n");
@@ -340,6 +341,7 @@ __rproc_handle_vring(struct rproc_vdev *rvdev, struct fw_rsc_vdev *rsc, int i)
 					dma, size, notifyid);
 
 	rvdev->vring[i].len = vring->num;
+	rvdev->vring[i].align = vring->align;
 	rvdev->vring[i].va = va;
 	rvdev->vring[i].dma = dma;
 	rvdev->vring[i].notifyid = notifyid;
@@ -354,7 +356,7 @@ static void __rproc_free_vrings(struct rproc_vdev *rvdev, int i)
 
 	for (i--; i > 0; i--) {
 		struct rproc_vring *rvring = &rvdev->vring[i];
-		int size = PAGE_ALIGN(vring_size(rvring->len, AMP_VRING_ALIGN));
+		int size = PAGE_ALIGN(vring_size(rvring->len, rvring->align));
 
 		dma_free_coherent(rproc->dev, size, rvring->va, rvring->dma);
 		idr_remove(&rproc->notifyids, rvring->notifyid);
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 0700410..ecf6121 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -99,7 +99,7 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
 	 * Create the new vq, and tell virtio we're not interested in
 	 * the 'weak' smp barriers, since we're talking with a real device.
 	 */
-	vq = vring_new_virtqueue(len, AMP_VRING_ALIGN, vdev, false, addr,
+	vq = vring_new_virtqueue(len, rvring->align, vdev, false, addr,
 					rproc_virtio_notify, callback, name);
 	if (!vq) {
 		dev_err(rproc->dev, "vring_new_virtqueue %s failed\n", name);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 7750d8a..f1ffabb 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -43,13 +43,6 @@
 #include <linux/completion.h>
 #include <linux/idr.h>
 
-/*
- * The alignment between the consumer and producer parts of the vring.
- * Note: this is part of the "wire" protocol. If you change this, you need
- * to update your peers too.
- */
-#define AMP_VRING_ALIGN	(4096)
-
 /**
  * struct resource_table - firmware resource table header
  * @ver: version number
@@ -423,6 +416,7 @@ struct rproc {
  * @dma: dma address
  * @len: length, in bytes
  * @da: device address
+ * @align: vring alignment
  * @notifyid: rproc-specific unique vring index
  * @rvdev: remote vdev
  * @vq: the virtqueue of this vring
@@ -432,6 +426,7 @@ struct rproc_vring {
 	dma_addr_t dma;
 	int len;
 	u32 da;
+	u32 align;
 	int notifyid;
 	struct rproc_vdev *rvdev;
 	struct virtqueue *vq;
-- 
1.7.5.4

[PATCH 7/7] remoteproc: cleanup resource table parsing paths

[Ohad Ben-Cohen]

rproc_handle_resources() looks for the resource table and then
invokes a resource handler function which it took as a parameter.

This works, but it's a bit unintuitive to follow.

Instead of passing around function pointers, this patch changes
rproc_handle_resource() to just find and return the resource table,
and then the calling sites of rproc_handle_resource() invoke their
resource handlers directly.

Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Brian Swetland <swetland@google.com>
Cc: Iliyan Malchev <malchev@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Mark Grosen <mgrosen@ti.com>
Cc: John Williams <john.williams@petalogix.com>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Loic PALLARDY <loic.pallardy@stericsson.com>
Cc: Ludovic BARRE <ludovic.barre@stericsson.com>
Cc: Omar Ramirez Luna <omar.luna@linaro.org>
Cc: Guzman Lugo Fernando <fernando.lugo@ti.com>
Cc: Anna Suman <s-anna@ti.com>
Cc: Clark Rob <rob@ti.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Saravana Kannan <skannan@codeaurora.org>
Cc: David Brown <davidb@codeaurora.org>
Cc: Kieran Bingham <kieranbingham@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
---
 drivers/remoteproc/remoteproc_core.c |   70 ++++++++++++++++++---------------
 1 files changed, 38 insertions(+), 32 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 9be5dad..ee15c68 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -822,32 +822,31 @@ rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table *table, int l
 }
 
 /**
- * rproc_handle_resources() - find and handle the resource table
+ * rproc_find_rsc_table() - find the resource table
  * @rproc: the rproc handle
  * @elf_data: the content of the ELF firmware image
  * @len: firmware size (in bytes)
- * @handler: function that should be used to handle the resource table
+ * @tablesz: place holder for providing back the table size
  *
  * This function finds the resource table inside the remote processor's
- * firmware, and invoke a user-supplied handler with it (we have two
- * possible handlers: one is invoked upon registration of @rproc,
- * in order to register the supported virito devices, and the other is
- * invoked when @rproc is actually booted).
- *
- * Currently this function fails if a resource table doesn't exist.
- * This restriction will be removed when we'll start supporting remote
- * processors that don't need a resource table.
+ * firmware. It is used both upon the registration of @rproc (in order
+ * to look for and register the supported virito devices), and when the
+ * @rproc is booted.
+ *
+ * Returns the pointer to the resource table if it is found, and write its
+ * size into @tablesz. If a valid table isn't found, NULL is returned
+ * (and @tablesz isn't set).
  */
-static int rproc_handle_resources(struct rproc *rproc, const u8 *elf_data,
-				size_t len, rproc_handle_resources_t handler)
-
+static struct resource_table *
+rproc_find_rsc_table(struct rproc *rproc, const u8 *elf_data, size_t len,
+							int *tablesz)
 {
 	struct elf32_hdr *ehdr;
 	struct elf32_shdr *shdr;
 	const char *name_table;
 	struct device *dev = rproc->dev;
-	int i, ret = -EINVAL;
-	struct resource_table *table;
+	struct resource_table *table = NULL;
+	int i;
 
 	ehdr = (struct elf32_hdr *)elf_data;
 	shdr = (struct elf32_shdr *)(elf_data + ehdr->e_shoff);
@@ -866,39 +865,39 @@ static int rproc_handle_resources(struct rproc *rproc, const u8 *elf_data,
 		/* make sure we have the entire table */
 		if (offset + size > len) {
 			dev_err(dev, "resource table truncated\n");
-			return -EINVAL;
+			return NULL;
 		}
 
 		/* make sure table has at least the header */
 		if (sizeof(struct resource_table) > size) {
 			dev_err(dev, "header-less resource table\n");
-			return -EINVAL;
+			return NULL;
 		}
 
 		/* we don't support any version beyond the first */
 		if (table->ver != 1) {
 			dev_err(dev, "unsupported fw ver: %d\n", table->ver);
-			return -EINVAL;
+			return NULL;
 		}
 
 		/* make sure reserved bytes are zeroes */
 		if (table->reserved[0] || table->reserved[1]) {
 			dev_err(dev, "non zero reserved bytes\n");
-			return -EINVAL;
+			return NULL;
 		}
 
 		/* make sure the offsets array isn't truncated */
 		if (table->num * sizeof(table->offset[0]) +
 				sizeof(struct resource_table) > size) {
 			dev_err(dev, "resource table incomplete\n");
-			return -EINVAL;
+			return NULL;
 		}
 
-		ret = handler(rproc, table, shdr->sh_size);
+		*tablesz = shdr->sh_size;
 		break;
 	}
 
-	return ret;
+	return table;
 }
 
 /**
@@ -1012,7 +1011,8 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	struct device *dev = rproc->dev;
 	const char *name = rproc->firmware;
 	struct elf32_hdr *ehdr;
-	int ret;
+	struct resource_table *table;
+	int ret, tablesz;
 
 	ret = rproc_fw_sanity_check(rproc, fw);
 	if (ret)
@@ -1039,9 +1039,13 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	 */
 	rproc->bootaddr = ehdr->e_entry;
 
+	/* look for the resource table */
+	table = rproc_find_rsc_table(rproc, fw->data, fw->size, &tablesz);
+	if (!table)
+		goto clean_up;
+
 	/* handle fw resources which are required to boot rproc */
-	ret = rproc_handle_resources(rproc, fw->data, fw->size,
-						rproc_handle_boot_rsc);
+	ret = rproc_handle_boot_rsc(rproc, table, tablesz);
 	if (ret) {
 		dev_err(dev, "Failed to process resources: %d\n", ret);
 		goto clean_up;
@@ -1084,19 +1088,21 @@ clean_up:
 static void rproc_fw_config_virtio(const struct firmware *fw, void *context)
 {
 	struct rproc *rproc = context;
-	struct device *dev = rproc->dev;
-	int ret;
+	struct resource_table *table;
+	int ret, tablesz;
 
 	if (rproc_fw_sanity_check(rproc, fw) < 0)
 		goto out;
 
-	/* does the fw support any virtio devices ? */
-	ret = rproc_handle_resources(rproc, fw->data, fw->size,
-						rproc_handle_virtio_rsc);
-	if (ret) {
-		dev_info(dev, "No fw virtio device was found\n");
+	/* look for the resource table */
+	table = rproc_find_rsc_table(rproc, fw->data, fw->size, &tablesz);
+	if (!table)
+		goto out;
+
+	/* look for virtio devices and register them */
+	ret = rproc_handle_virtio_rsc(rproc, table, tablesz);
+	if (ret)
 		goto out;
-	}
 
 out:
 	if (fw)
-- 
1.7.5.4