[PATCH 1/2] remoteproc: maintain a generic child device for each rproc

[PATCH 1/2] remoteproc: maintain a generic child device for each rproc

[Ohad Ben-Cohen]

For each registered rproc, maintain a generic remoteproc device whose
parent is the low level platform-specific device (commonly a pdev, but
it may certainly be any other type of device too).

With this in hand, the resulting device hierarchy might then look like:

omap-rproc.0
 |
 - remoteproc.0
    |
    - virtio0
    |
    - virtio1
       |
       - rpmsg0
       |
       - rpmsg1
       |
       - rpmsg2

Where:
- omap-rproc.0 is the low level device that's bound to the
  driver which invokes rproc_register()
- remoteproc.0 is the result of this patch, and will be added by the
  remoteproc framework when rproc_register() is invoked
- virtio0 and virtio1 are vdevs that are registered by remoteproc
  when it realizes that they are supported by the firmware
  of the physical remote processor represented by omap-rproc.0
- rpmsg0, rpmsg1 and rpmsg2 are rpmsg devices that represent rpmsg
  channels, and are registerd by the rpmsg bus when it gets notified
  about their existence

Technically, this patch:
- changes 'struct rproc' to contain this generic remoteproc.x device
- creates a new "remoteproc" class, to which this new generic remoteproc.x
  device belong to.
- adds a super simple enumeration method for the indices of the
  remoteproc.x devices
- updates all dev_* messaging to use the generic remoteproc.x device
  instead of the low level platform-specific device
- updates all dma_* allocations to use the parent of remoteproc.x (where
  the platform-specific memory pools, most commonly CMA, are to be found)

Adding this generic device has several merits:
- we can now add remoteproc runtime PM support simply by hooking onto the
  new "remoteproc" class
- all remoteproc log messages will now carry a common name prefix
  instead of having a platform-specific one
- having a device as part of the rproc struct makes it possible to simplify
  refcounting (see subsequent patch)

Thanks to Stephen Boyd <sboyd@codeaurora.org> for suggesting and
discussing these ideas in one of the remoteproc review threads and
to Fernando Guzman Lugo <fernando.lugo@ti.com> for trying them out
with the (upcoming) runtime PM support for remoteproc.

Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Fernando Guzman Lugo <fernando.lugo@ti.com>
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/remoteproc/omap_remoteproc.c    |   17 +++--
 drivers/remoteproc/remoteproc_core.c    |  125 ++++++++++++++++++++-----------
 drivers/remoteproc/remoteproc_debugfs.c |    4 +-
 drivers/remoteproc/remoteproc_virtio.c  |   13 ++--
 drivers/rpmsg/virtio_rpmsg_bus.c        |    3 +-
 include/linux/remoteproc.h              |    4 +-
 6 files changed, 106 insertions(+), 60 deletions(-)

diff --git a/drivers/remoteproc/omap_remoteproc.c b/drivers/remoteproc/omap_remoteproc.c
index de138e30..f45230c 100644
--- a/drivers/remoteproc/omap_remoteproc.c
+++ b/drivers/remoteproc/omap_remoteproc.c
@@ -66,7 +66,7 @@ static int omap_rproc_mbox_callback(struct notifier_block *this,
 {
 	mbox_msg_t msg = (mbox_msg_t) data;
 	struct omap_rproc *oproc = container_of(this, struct omap_rproc, nb);
-	struct device *dev = oproc->rproc->dev;
+	struct device *dev = oproc->rproc->dev.parent;
 	const char *name = oproc->rproc->name;
 
 	dev_dbg(dev, "mbox msg: 0x%x\n", msg);
@@ -92,12 +92,13 @@ static int omap_rproc_mbox_callback(struct notifier_block *this,
 static void omap_rproc_kick(struct rproc *rproc, int vqid)
 {
 	struct omap_rproc *oproc = rproc->priv;
+	struct device *dev = rproc->dev.parent;
 	int ret;
 
 	/* send the index of the triggered virtqueue in the mailbox payload */
 	ret = omap_mbox_msg_send(oproc->mbox, vqid);
 	if (ret)
-		dev_err(rproc->dev, "omap_mbox_msg_send failed: %d\n", ret);
+		dev_err(dev, "omap_mbox_msg_send failed: %d\n", ret);
 }
 
 /*
@@ -110,7 +111,8 @@ static void omap_rproc_kick(struct rproc *rproc, int vqid)
 static int omap_rproc_start(struct rproc *rproc)
 {
 	struct omap_rproc *oproc = rproc->priv;
-	struct platform_device *pdev = to_platform_device(rproc->dev);
+	struct device *dev = rproc->dev.parent;
+	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_rproc_pdata *pdata = pdev->dev.platform_data;
 	int ret;
 
@@ -120,7 +122,7 @@ static int omap_rproc_start(struct rproc *rproc)
 	oproc->mbox = omap_mbox_get(pdata->mbox_name, &oproc->nb);
 	if (IS_ERR(oproc->mbox)) {
 		ret = PTR_ERR(oproc->mbox);
-		dev_err(rproc->dev, "omap_mbox_get failed: %d\n", ret);
+		dev_err(dev, "omap_mbox_get failed: %d\n", ret);
 		return ret;
 	}
 
@@ -133,13 +135,13 @@ static int omap_rproc_start(struct rproc *rproc)
 	 */
 	ret = omap_mbox_msg_send(oproc->mbox, RP_MBOX_ECHO_REQUEST);
 	if (ret) {
-		dev_err(rproc->dev, "omap_mbox_get failed: %d\n", ret);
+		dev_err(dev, "omap_mbox_get failed: %d\n", ret);
 		goto put_mbox;
 	}
 
 	ret = pdata->device_enable(pdev);
 	if (ret) {
-		dev_err(rproc->dev, "omap_device_enable failed: %d\n", ret);
+		dev_err(dev, "omap_device_enable failed: %d\n", ret);
 		goto put_mbox;
 	}
 
@@ -153,7 +155,8 @@ put_mbox:
 /* power off the remote processor */
 static int omap_rproc_stop(struct rproc *rproc)
 {
-	struct platform_device *pdev = to_platform_device(rproc->dev);
+	struct device *dev = rproc->dev.parent;
+	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_rproc_pdata *pdata = pdev->dev.platform_data;
 	struct omap_rproc *oproc = rproc->priv;
 	int ret;
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 464ea4f..9e3d4cf 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -66,6 +66,9 @@ typedef int (*rproc_handle_resources_t)(struct rproc *rproc,
 				struct resource_table *table, int len);
 typedef int (*rproc_handle_resource_t)(struct rproc *rproc, void *, int avail);
 
+/* Unique numbering for remoteproc devices */
+static unsigned int dev_index;
+
 /*
  * This is the IOMMU fault handler we register with the IOMMU API
  * (when relevant; not all remote processors access memory through
@@ -92,7 +95,7 @@ static int rproc_iommu_fault(struct iommu_domain *domain, struct device *dev,
 static int rproc_enable_iommu(struct rproc *rproc)
 {
 	struct iommu_domain *domain;
-	struct device *dev = rproc->dev;
+	struct device *dev = rproc->dev.parent;
 	int ret;
 
 	/*
@@ -137,7 +140,7 @@ free_domain:
 static void rproc_disable_iommu(struct rproc *rproc)
 {
 	struct iommu_domain *domain = rproc->domain;
-	struct device *dev = rproc->dev;
+	struct device *dev = rproc->dev.parent;
 
 	if (!domain)
 		return;
@@ -217,7 +220,7 @@ static void *rproc_da_to_va(struct rproc *rproc, u64 da, int len)
 static int
 rproc_load_segments(struct rproc *rproc, const u8 *elf_data, size_t len)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	struct elf32_hdr *ehdr;
 	struct elf32_phdr *phdr;
 	int i, ret = 0;
@@ -282,7 +285,7 @@ rproc_load_segments(struct rproc *rproc, const u8 *elf_data, size_t len)
 int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 {
 	struct rproc *rproc = rvdev->rproc;
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	struct rproc_vring *rvring = &rvdev->vring[i];
 	dma_addr_t dma;
 	void *va;
@@ -301,9 +304,9 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	 * 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);
+	va = dma_alloc_coherent(dev->parent, size, &dma, GFP_KERNEL);
 	if (!va) {
-		dev_err(dev, "dma_alloc_coherent failed\n");
+		dev_err(dev->parent, "dma_alloc_coherent failed\n");
 		return -EINVAL;
 	}
 
@@ -316,7 +319,7 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	ret = idr_get_new(&rproc->notifyids, rvring, &notifyid);
 	if (ret) {
 		dev_err(dev, "idr_get_new failed: %d\n", ret);
-		dma_free_coherent(dev, size, va, dma);
+		dma_free_coherent(dev->parent, size, va, dma);
 		return ret;
 	}
 
@@ -334,7 +337,7 @@ 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 device *dev = &rproc->dev;
 	struct fw_rsc_vdev_vring *vring = &rsc->vring[i];
 	struct rproc_vring *rvring = &rvdev->vring[i];
 
@@ -366,7 +369,7 @@ 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);
+	dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);
 	idr_remove(&rproc->notifyids, rvring->notifyid);
 }
 
@@ -400,14 +403,14 @@ void rproc_free_vring(struct rproc_vring *rvring)
 static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 								int avail)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	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)
 			+ rsc->config_len > avail) {
-		dev_err(rproc->dev, "vdev rsc is truncated\n");
+		dev_err(dev, "vdev rsc is truncated\n");
 		return -EINVAL;
 	}
 
@@ -476,12 +479,12 @@ 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;
+	struct device *dev = &rproc->dev;
 	void *ptr;
 	char name[15];
 
 	if (sizeof(*rsc) > avail) {
-		dev_err(rproc->dev, "trace rsc is truncated\n");
+		dev_err(dev, "trace rsc is truncated\n");
 		return -EINVAL;
 	}
 
@@ -558,6 +561,7 @@ static int rproc_handle_devmem(struct rproc *rproc, struct fw_rsc_devmem *rsc,
 								int avail)
 {
 	struct rproc_mem_entry *mapping;
+	struct device *dev = &rproc->dev;
 	int ret;
 
 	/* no point in handling this resource without a valid iommu domain */
@@ -565,25 +569,25 @@ static int rproc_handle_devmem(struct rproc *rproc, struct fw_rsc_devmem *rsc,
 		return -EINVAL;
 
 	if (sizeof(*rsc) > avail) {
-		dev_err(rproc->dev, "devmem rsc is truncated\n");
+		dev_err(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");
+		dev_err(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");
+		dev_err(dev, "kzalloc mapping failed\n");
 		return -ENOMEM;
 	}
 
 	ret = iommu_map(rproc->domain, rsc->da, rsc->pa, rsc->len, rsc->flags);
 	if (ret) {
-		dev_err(rproc->dev, "failed to map devmem: %d\n", ret);
+		dev_err(dev, "failed to map devmem: %d\n", ret);
 		goto out;
 	}
 
@@ -598,7 +602,7 @@ static int rproc_handle_devmem(struct rproc *rproc, struct fw_rsc_devmem *rsc,
 	mapping->len = rsc->len;
 	list_add_tail(&mapping->node, &rproc->mappings);
 
-	dev_dbg(rproc->dev, "mapped devmem pa 0x%x, da 0x%x, len 0x%x\n",
+	dev_dbg(dev, "mapped devmem pa 0x%x, da 0x%x, len 0x%x\n",
 					rsc->pa, rsc->da, rsc->len);
 
 	return 0;
@@ -630,13 +634,13 @@ 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;
+	struct device *dev = &rproc->dev;
 	dma_addr_t dma;
 	void *va;
 	int ret;
 
 	if (sizeof(*rsc) > avail) {
-		dev_err(rproc->dev, "carveout rsc is truncated\n");
+		dev_err(dev, "carveout rsc is truncated\n");
 		return -EINVAL;
 	}
 
@@ -662,9 +666,9 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		goto free_mapping;
 	}
 
-	va = dma_alloc_coherent(dev, rsc->len, &dma, GFP_KERNEL);
+	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
 	if (!va) {
-		dev_err(dev, "failed to dma alloc carveout: %d\n", rsc->len);
+		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
 		ret = -ENOMEM;
 		goto free_carv;
 	}
@@ -735,7 +739,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	return 0;
 
 dma_free:
-	dma_free_coherent(dev, rsc->len, va, dma);
+	dma_free_coherent(dev->parent, rsc->len, va, dma);
 free_carv:
 	kfree(carveout);
 free_mapping:
@@ -758,7 +762,7 @@ static rproc_handle_resource_t rproc_handle_rsc[] = {
 static int
 rproc_handle_boot_rsc(struct rproc *rproc, struct resource_table *table, int len)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	rproc_handle_resource_t handler;
 	int ret = 0, i;
 
@@ -797,7 +801,7 @@ rproc_handle_boot_rsc(struct rproc *rproc, struct resource_table *table, int len
 static int
 rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table *table, int len)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	int ret = 0, i;
 
 	for (i = 0; i < table->num; i++) {
@@ -850,7 +854,7 @@ rproc_find_rsc_table(struct rproc *rproc, const u8 *elf_data, size_t len,
 	struct elf32_hdr *ehdr;
 	struct elf32_shdr *shdr;
 	const char *name_table;
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	struct resource_table *table = NULL;
 	int i;
 
@@ -916,7 +920,7 @@ rproc_find_rsc_table(struct rproc *rproc, const u8 *elf_data, size_t len,
 static void rproc_resource_cleanup(struct rproc *rproc)
 {
 	struct rproc_mem_entry *entry, *tmp;
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 
 	/* clean up debugfs trace entries */
 	list_for_each_entry_safe(entry, tmp, &rproc->traces, node) {
@@ -928,7 +932,7 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 
 	/* clean up carveout allocations */
 	list_for_each_entry_safe(entry, tmp, &rproc->carveouts, node) {
-		dma_free_coherent(dev, entry->len, entry->va, entry->dma);
+		dma_free_coherent(dev->parent, entry->len, entry->va, entry->dma);
 		list_del(&entry->node);
 		kfree(entry);
 	}
@@ -953,7 +957,7 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 static int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
 {
 	const char *name = rproc->firmware;
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	struct elf32_hdr *ehdr;
 	char class;
 
@@ -1014,7 +1018,7 @@ static int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
  */
 static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	const char *name = rproc->firmware;
 	struct elf32_hdr *ehdr;
 	struct resource_table *table;
@@ -1139,7 +1143,7 @@ int rproc_boot(struct rproc *rproc)
 		return -EINVAL;
 	}
 
-	dev = rproc->dev;
+	dev = &rproc->dev;
 
 	/*
 	 * if asynchronoush fw loading is underway, wait up to 65 secs
@@ -1167,7 +1171,7 @@ int rproc_boot(struct rproc *rproc)
 	}
 
 	/* prevent underlying implementation from being removed */
-	if (!try_module_get(dev->driver->owner)) {
+	if (!try_module_get(dev->parent->driver->owner)) {
 		dev_err(dev, "%s: can't get owner\n", __func__);
 		ret = -EINVAL;
 		goto unlock_mutex;
@@ -1194,7 +1198,7 @@ int rproc_boot(struct rproc *rproc)
 
 downref_rproc:
 	if (ret) {
-		module_put(dev->driver->owner);
+		module_put(dev->parent->driver->owner);
 		atomic_dec(&rproc->power);
 	}
 unlock_mutex:
@@ -1228,7 +1232,7 @@ EXPORT_SYMBOL(rproc_boot);
  */
 void rproc_shutdown(struct rproc *rproc)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	int ret;
 
 	ret = mutex_lock_interruptible(&rproc->lock);
@@ -1261,7 +1265,7 @@ void rproc_shutdown(struct rproc *rproc)
 out:
 	mutex_unlock(&rproc->lock);
 	if (!ret)
-		module_put(dev->driver->owner);
+		module_put(dev->parent->driver->owner);
 }
 EXPORT_SYMBOL(rproc_shutdown);
 
@@ -1284,7 +1288,7 @@ void rproc_release(struct kref *kref)
 {
 	struct rproc *rproc = container_of(kref, struct rproc, refcount);
 
-	dev_info(rproc->dev, "removing %s\n", rproc->name);
+	dev_info(&rproc->dev, "removing %s\n", rproc->name);
 
 	rproc_delete_debug_dir(rproc);
 
@@ -1416,13 +1420,17 @@ EXPORT_SYMBOL(rproc_put);
  */
 int rproc_register(struct rproc *rproc)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	int ret = 0;
 
+	ret = device_add(dev);
+	if (ret < 0)
+		return ret;
+
 	/* expose to rproc_get_by_name users */
 	klist_add_tail(&rproc->node, &rprocs);
 
-	dev_info(rproc->dev, "%s is available\n", rproc->name);
+	dev_info(dev, "%s is available\n", rproc->name);
 
 	dev_info(dev, "Note: remoteproc is still under development and considered experimental.\n");
 	dev_info(dev, "THE BINARY FORMAT IS NOT YET FINALIZED, and backward compatibility isn't yet guaranteed.\n");
@@ -1454,6 +1462,22 @@ int rproc_register(struct rproc *rproc)
 }
 EXPORT_SYMBOL(rproc_register);
 
+static void rproc_class_release(struct device *dev)
+{
+	struct rproc *rproc = container_of(dev, struct rproc, dev);
+
+	idr_remove_all(&rproc->notifyids);
+	idr_destroy(&rproc->notifyids);
+
+	kfree(rproc);
+}
+
+static struct class rproc_class = {
+	.name		= "remoteproc",
+	.owner		= THIS_MODULE,
+	.dev_release	= rproc_class_release,
+};
+
 /**
  * rproc_alloc() - allocate a remote processor handle
  * @dev: the underlying device
@@ -1492,12 +1516,19 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 		return NULL;
 	}
 
-	rproc->dev = dev;
 	rproc->name = name;
 	rproc->ops = ops;
 	rproc->firmware = firmware;
 	rproc->priv = &rproc[1];
 
+	device_initialize(&rproc->dev);
+	rproc->dev.parent = dev;
+	rproc->dev.class = &rproc_class;
+
+	/* Assign a unique device index and name */
+	rproc->index = dev_index++;
+	dev_set_name(&rproc->dev, "remoteproc%d", rproc->index);
+
 	atomic_set(&rproc->power, 0);
 
 	kref_init(&rproc->refcount);
@@ -1529,10 +1560,7 @@ EXPORT_SYMBOL(rproc_alloc);
  */
 void rproc_free(struct rproc *rproc)
 {
-	idr_remove_all(&rproc->notifyids);
-	idr_destroy(&rproc->notifyids);
-
-	kfree(rproc);
+	put_device(&rproc->dev);
 }
 EXPORT_SYMBOL(rproc_free);
 
@@ -1573,6 +1601,8 @@ int rproc_unregister(struct rproc *rproc)
 	/* the rproc is downref'ed as soon as it's removed from the klist */
 	klist_del(&rproc->node);
 
+	device_del(&rproc->dev);
+
 	/* the rproc will only be released after its refcount drops to zero */
 	kref_put(&rproc->refcount, rproc_release);
 
@@ -1582,7 +1612,14 @@ EXPORT_SYMBOL(rproc_unregister);
 
 static int __init remoteproc_init(void)
 {
+	int ret;
+
+	ret = class_register(&rproc_class);
+	if (ret)
+		return ret;
+
 	rproc_init_debugfs();
+
 	return 0;
 }
 module_init(remoteproc_init);
@@ -1590,6 +1627,8 @@ module_init(remoteproc_init);
 static void __exit remoteproc_exit(void)
 {
 	rproc_exit_debugfs();
+
+	class_unregister(&rproc_class);
 }
 module_exit(remoteproc_exit);
 
diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index 85d31a6..0383385 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -124,7 +124,7 @@ struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
 	tfile = debugfs_create_file(name, 0400, rproc->dbg_dir,
 						trace, &trace_rproc_ops);
 	if (!tfile) {
-		dev_err(rproc->dev, "failed to create debugfs trace entry\n");
+		dev_err(&rproc->dev, "failed to create debugfs trace entry\n");
 		return NULL;
 	}
 
@@ -141,7 +141,7 @@ void rproc_delete_debug_dir(struct rproc *rproc)
 
 void rproc_create_debug_dir(struct rproc *rproc)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 
 	if (!rproc_dbg)
 		return;
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 26a7144..b662183 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -36,7 +36,7 @@ static void rproc_virtio_notify(struct virtqueue *vq)
 	struct rproc *rproc = rvring->rvdev->rproc;
 	int notifyid = rvring->notifyid;
 
-	dev_dbg(rproc->dev, "kicking vq index: %d\n", notifyid);
+	dev_dbg(&rproc->dev, "kicking vq index: %d\n", notifyid);
 
 	rproc->ops->kick(rproc, notifyid);
 }
@@ -57,7 +57,7 @@ irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int notifyid)
 {
 	struct rproc_vring *rvring;
 
-	dev_dbg(rproc->dev, "vq index %d is interrupted\n", notifyid);
+	dev_dbg(&rproc->dev, "vq index %d is interrupted\n", notifyid);
 
 	rvring = idr_find(&rproc->notifyids, notifyid);
 	if (!rvring || !rvring->vq)
@@ -74,6 +74,7 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
 {
 	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
 	struct rproc *rproc = vdev_to_rproc(vdev);
+	struct device *dev = &rproc->dev;
 	struct rproc_vring *rvring;
 	struct virtqueue *vq;
 	void *addr;
@@ -95,7 +96,7 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
 	size = vring_size(len, rvring->align);
 	memset(addr, 0, size);
 
-	dev_dbg(rproc->dev, "vring%d: va %p qsz %d notifyid %d\n",
+	dev_dbg(dev, "vring%d: va %p qsz %d notifyid %d\n",
 					id, addr, len, rvring->notifyid);
 
 	/*
@@ -105,7 +106,7 @@ static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
 	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);
+		dev_err(dev, "vring_new_virtqueue %s failed\n", name);
 		rproc_free_vring(rvring);
 		return ERR_PTR(-ENOMEM);
 	}
@@ -152,7 +153,7 @@ 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);
+		dev_err(&rproc->dev, "rproc_boot() failed %d\n", ret);
 		goto error;
 	}
 
@@ -254,7 +255,7 @@ static void rproc_vdev_release(struct device *dev)
 int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 {
 	struct rproc *rproc = rvdev->rproc;
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	struct virtio_device *vdev = &rvdev->vdev;
 	int ret;
 
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index d5572e8..0af7fd3 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -964,7 +964,8 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	vrp->svq = vqs[1];
 
 	/* allocate coherent memory for the buffers */
-	bufs_va = dma_alloc_coherent(vdev->dev.parent, RPMSG_TOTAL_BUF_SPACE,
+	bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
+				RPMSG_TOTAL_BUF_SPACE,
 				&vrp->bufs_dma, GFP_KERNEL);
 	if (!bufs_va)
 		goto vqs_del;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index f1ffabb..0b835d3 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -383,6 +383,7 @@ enum rproc_state {
  * @bootaddr: address of first instruction to boot rproc with (optional)
  * @rvdevs: list of remote virtio devices
  * @notifyids: idr for dynamically assigning rproc-wide unique notify ids
+ * @index: index of this rproc device
  */
 struct rproc {
 	struct klist_node node;
@@ -391,7 +392,7 @@ struct rproc {
 	const char *firmware;
 	void *priv;
 	const struct rproc_ops *ops;
-	struct device *dev;
+	struct device dev;
 	struct kref refcount;
 	atomic_t power;
 	unsigned int state;
@@ -405,6 +406,7 @@ struct rproc {
 	u32 bootaddr;
 	struct list_head rvdevs;
 	struct idr notifyids;
+	int index;
 };
 
 /* we currently support only two vrings per rvdev */
-- 
1.7.5.4

[PATCH 2/2] remoteproc: remove the now-redundant kref

[Ohad Ben-Cohen]

Now that every rproc instance contains a device, we don't need a
kref anymore to maintain the refcount of the rproc instances:
that's what device are good with!

This patch removes the now-redundant kref, and switches to
{get, put}_device instead of kref_{get, put}.

We also don't need the kref's release function anymore, and instead,
we just utilize the class's release handler (which is now responsible
for all memory de-allocations).

Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Fernando Guzman Lugo <fernando.lugo@ti.com>
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/remoteproc/remoteproc_core.c   |   59 +++++++++++---------------------
 drivers/remoteproc/remoteproc_virtio.c |    8 ++--
 include/linux/remoteproc.h             |    3 --
 3 files changed, 24 insertions(+), 46 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 9e3d4cf..7214393 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1269,42 +1269,12 @@ out:
 }
 EXPORT_SYMBOL(rproc_shutdown);
 
-/**
- * rproc_release() - completely deletes the existence of a remote processor
- * @kref: the rproc's kref
- *
- * This function should _never_ be called directly.
- *
- * The only reasonable location to use it is as an argument when kref_put'ing
- * @rproc's refcount.
- *
- * This way it will be called when no one holds a valid pointer to this @rproc
- * anymore (and obviously after it is removed from the rprocs klist).
- *
- * Note: this function is not static because rproc_vdev_release() needs it when
- * it decrements @rproc's refcount.
- */
-void rproc_release(struct kref *kref)
-{
-	struct rproc *rproc = container_of(kref, struct rproc, refcount);
-
-	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,
-	 * so we can directly unroll rproc_alloc()
-	 */
-	rproc_free(rproc);
-}
-
 /* will be called when an rproc is added to the rprocs klist */
 static void klist_rproc_get(struct klist_node *n)
 {
 	struct rproc *rproc = container_of(n, struct rproc, node);
 
-	kref_get(&rproc->refcount);
+	get_device(&rproc->dev);
 }
 
 /* will be called when an rproc is removed from the rprocs klist */
@@ -1312,7 +1282,7 @@ static void klist_rproc_put(struct klist_node *n)
 {
 	struct rproc *rproc = container_of(n, struct rproc, node);
 
-	kref_put(&rproc->refcount, rproc_release);
+	put_device(&rproc->dev);
 }
 
 static struct rproc *next_rproc(struct klist_iter *i)
@@ -1354,7 +1324,7 @@ struct rproc *rproc_get_by_name(const char *name)
 	klist_iter_init(&rprocs, &i);
 	while ((rproc = next_rproc(&i)) != NULL)
 		if (!strcmp(rproc->name, name)) {
-			kref_get(&rproc->refcount);
+			get_device(&rproc->dev);
 			break;
 		}
 	klist_iter_exit(&i);
@@ -1367,7 +1337,7 @@ struct rproc *rproc_get_by_name(const char *name)
 
 	ret = rproc_boot(rproc);
 	if (ret < 0) {
-		kref_put(&rproc->refcount, rproc_release);
+		put_device(&rproc->dev);
 		return NULL;
 	}
 
@@ -1394,7 +1364,7 @@ void rproc_put(struct rproc *rproc)
 	rproc_shutdown(rproc);
 
 	/* downref rproc's refcount */
-	kref_put(&rproc->refcount, rproc_release);
+	put_device(&rproc->dev);
 }
 EXPORT_SYMBOL(rproc_put);
 
@@ -1462,10 +1432,23 @@ int rproc_register(struct rproc *rproc)
 }
 EXPORT_SYMBOL(rproc_register);
 
+/**
+ * rproc_class_release() - release a remote processor instance
+ * @dev: the rproc's device
+ *
+ * This function should _never_ be called directly.
+ *
+ * It will be called by the driver core when no one holds a valid pointer
+ * to @dev anymore.
+ */
 static void rproc_class_release(struct device *dev)
 {
 	struct rproc *rproc = container_of(dev, struct rproc, dev);
 
+	dev_info(&rproc->dev, "releasing %s\n", rproc->name);
+
+	rproc_delete_debug_dir(rproc);
+
 	idr_remove_all(&rproc->notifyids);
 	idr_destroy(&rproc->notifyids);
 
@@ -1531,8 +1514,6 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 
 	atomic_set(&rproc->power, 0);
 
-	kref_init(&rproc->refcount);
-
 	mutex_init(&rproc->lock);
 
 	idr_init(&rproc->notifyids);
@@ -1603,8 +1584,8 @@ int rproc_unregister(struct rproc *rproc)
 
 	device_del(&rproc->dev);
 
-	/* the rproc will only be released after its refcount drops to zero */
-	kref_put(&rproc->refcount, rproc_release);
+	/* unroll rproc_alloc. TODO: we may want to let the users do that */
+	put_device(&rproc->dev);
 
 	return 0;
 }
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index b662183..3541b44 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -225,7 +225,7 @@ static struct virtio_config_ops rproc_virtio_config_ops = {
 
 /*
  * This function is called whenever vdev is released, and is responsible
- * to decrement the remote processor's refcount taken when vdev was
+ * to decrement the remote processor's refcount which was taken when vdev was
  * added.
  *
  * Never call this function directly; it will be called by the driver
@@ -240,7 +240,7 @@ static void rproc_vdev_release(struct device *dev)
 	list_del(&rvdev->node);
 	kfree(rvdev);
 
-	kref_put(&rproc->refcount, rproc_release);
+	put_device(&rproc->dev);
 }
 
 /**
@@ -272,11 +272,11 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 	 * Therefore we must increment the rproc refcount here, and decrement
 	 * it _only_ when the vdev is released.
 	 */
-	kref_get(&rproc->refcount);
+	get_device(&rproc->dev);
 
 	ret = register_virtio_device(vdev);
 	if (ret) {
-		kref_put(&rproc->refcount, rproc_release);
+		put_device(&rproc->dev);
 		dev_err(dev, "failed to register vdev: %d\n", ret);
 		goto out;
 	}
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 0b835d3..c6e46fa 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -36,7 +36,6 @@
 #define REMOTEPROC_H
 
 #include <linux/types.h>
-#include <linux/kref.h>
 #include <linux/klist.h>
 #include <linux/mutex.h>
 #include <linux/virtio.h>
@@ -370,7 +369,6 @@ enum rproc_state {
  * @priv: private data which belongs to the platform-specific rproc module
  * @ops: platform-specific start/stop rproc handlers
  * @dev: underlying device
- * @refcount: refcount of users that have a valid pointer to this rproc
  * @power: refcount of users who need this rproc powered up
  * @state: state of the device
  * @lock: lock which protects concurrent manipulations of the rproc
@@ -393,7 +391,6 @@ struct rproc {
 	void *priv;
 	const struct rproc_ops *ops;
 	struct device dev;
-	struct kref refcount;
 	atomic_t power;
 	unsigned int state;
 	struct mutex lock;
-- 
1.7.5.4

Re: [PATCH 1/2] remoteproc: maintain a generic child device for each rproc

[Stephen Boyd]

On 5/26/2012 12:36 AM, Ohad Ben-Cohen wrote:
> For each registered rproc, maintain a generic remoteproc device whose
> parent is the low level platform-specific device (commonly a pdev, but
> it may certainly be any other type of device too).
>
> With this in hand, the resulting device hierarchy might then look like:
>
> omap-rproc.0
>  |
>  - remoteproc.0

It looks like remoteproc0, remoteproc1, etc. is what's used.

>     |
>     - virtio0
>     |
>     - virtio1
>        |
>        - rpmsg0
>        |
>        - rpmsg1
>        |
>        - rpmsg2
>
> Where:
> - omap-rproc.0 is the low level device that's bound to the
>   driver which invokes rproc_register()
> - remoteproc.0 is the result of this patch, and will be added by the
>   remoteproc framework when rproc_register() is invoked
> - virtio0 and virtio1 are vdevs that are registered by remoteproc
>   when it realizes that they are supported by the firmware
>   of the physical remote processor represented by omap-rproc.0
> - rpmsg0, rpmsg1 and rpmsg2 are rpmsg devices that represent rpmsg
>   channels, and are registerd by the rpmsg bus when it gets notified
>   about their existence
>
> Technically, this patch:
> - changes 'struct rproc' to contain this generic remoteproc.x device
> - creates a new "remoteproc" class, to which this new generic remoteproc.x
>   device belong to.
> - adds a super simple enumeration method for the indices of the
>   remoteproc.x devices
> - updates all dev_* messaging to use the generic remoteproc.x device
>   instead of the low level platform-specific device

One complaint I've gotten is that the error messages are essentially
useless now. I believe there are some ongoing discussions on lkml to fix
this by traversing the device hierarchy to find the "real" device but
the hard part is finding the real device.

> - updates all dma_* allocations to use the parent of remoteproc.x (where
>   the platform-specific memory pools, most commonly CMA, are to be found)
>
> Adding this generic device has several merits:
> - we can now add remoteproc runtime PM support simply by hooking onto the
>   new "remoteproc" class
> - all remoteproc log messages will now carry a common name prefix
>   instead of having a platform-specific one
> - having a device as part of the rproc struct makes it possible to simplify
>   refcounting (see subsequent patch)
>
> Thanks to Stephen Boyd <sboyd@codeaurora.org> for suggesting and
> discussing these ideas in one of the remoteproc review threads and
> to Fernando Guzman Lugo <fernando.lugo@ti.com> for trying them out
> with the (upcoming) runtime PM support for remoteproc.
[snip]
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 464ea4f..9e3d4cf 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -66,6 +66,9 @@ typedef int (*rproc_handle_resources_t)(struct rproc *rproc,
>  				struct resource_table *table, int len);
>  typedef int (*rproc_handle_resource_t)(struct rproc *rproc, void *, int avail);
>  
> +/* Unique numbering for remoteproc devices */
> +static unsigned int dev_index;
> +

Hm... perhaps use that ida stuff instead of a raw integer?

> +static struct class rproc_class = {
> +	.name		= "remoteproc",
> +	.owner		= THIS_MODULE,
> +	.dev_release	= rproc_class_release,
> +};

I'm not clear on busses versus classes. I recall seeing a thread where
someone said classes were on the way out and shouldn't be used but I
can't find it anymore. Should we use classes for devices that will never
have a matching driver?

> +
>  /**
>   * rproc_alloc() - allocate a remote processor handle
>   * @dev: the underlying device
> @@ -1492,12 +1516,19 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>  		return NULL;
>  	}
>  
> -	rproc->dev = dev;
>  	rproc->name = name;
>  	rproc->ops = ops;
>  	rproc->firmware = firmware;
>  	rproc->priv = &rproc[1];
>  
> +	device_initialize(&rproc->dev);
> +	rproc->dev.parent = dev;
> +	rproc->dev.class = &rproc_class;
> +
> +	/* Assign a unique device index and name */
> +	rproc->index = dev_index++;
> +	dev_set_name(&rproc->dev, "remoteproc%d", rproc->index);
> +

This doesn't look thread safe. ida would fix this (ida_simple_get/remove
looks like what you want).

> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index f1ffabb..0b835d3 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -383,6 +383,7 @@ enum rproc_state {
>   * @bootaddr: address of first instruction to boot rproc with (optional)
>   * @rvdevs: list of remote virtio devices
>   * @notifyids: idr for dynamically assigning rproc-wide unique notify ids
> + * @index: index of this rproc device
>   */
>  struct rproc {
>  	struct klist_node node;
> @@ -391,7 +392,7 @@ struct rproc {
>  	const char *firmware;
>  	void *priv;
>  	const struct rproc_ops *ops;
> -	struct device *dev;
> +	struct device dev;

I'm not sure if the kernel-doc for this field is accurate anymore. Is it
an 'underlying device' still?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] remoteproc: remove the now-redundant kref

[Stephen Boyd]

On 5/26/2012 12:36 AM, Ohad Ben-Cohen wrote:
>  /* will be called when an rproc is added to the rprocs klist */
>  static void klist_rproc_get(struct klist_node *n)
>  {
>  	struct rproc *rproc = container_of(n, struct rproc, node);
>  
> -	kref_get(&rproc->refcount);
> +	get_device(&rproc->dev);
>  }
>  
>  /* will be called when an rproc is removed from the rprocs klist */
> @@ -1312,7 +1282,7 @@ static void klist_rproc_put(struct klist_node *n)
>  {
>  	struct rproc *rproc = container_of(n, struct rproc, node);
>  
> -	kref_put(&rproc->refcount, rproc_release);
> +	put_device(&rproc->dev);
>  }
>  
>  static struct rproc *next_rproc(struct klist_iter *i)
> @@ -1354,7 +1324,7 @@ struct rproc *rproc_get_by_name(const char *name)
>  	klist_iter_init(&rprocs, &i);
>  	while ((rproc = next_rproc(&i)) != NULL)
>  		if (!strcmp(rproc->name, name)) {
> -			kref_get(&rproc->refcount);
> +			get_device(&rproc->dev);
>  			break;
>  		}
>  	klist_iter_exit(&i);
> @@ -1367,7 +1337,7 @@ struct rproc *rproc_get_by_name(const char *name)
>  
>  	ret = rproc_boot(rproc);
>  	if (ret < 0) {
> -		kref_put(&rproc->refcount, rproc_release);
> +		put_device(&rproc->dev);
>  		return NULL;
>  	}

I was hoping we could use class_for_each_device() and
class_find_device() to replace all this code. Then we wouldn't need all
this klist stuff that the class is taking care of already.

> @@ -1462,10 +1432,23 @@ int rproc_register(struct rproc *rproc)
>  }
>  EXPORT_SYMBOL(rproc_register);
>  
> +/**
> + * rproc_class_release() - release a remote processor instance
> + * @dev: the rproc's device
> + *
> + * This function should _never_ be called directly.
> + *
> + * It will be called by the driver core when no one holds a valid pointer
> + * to @dev anymore.
> + */

Why is this added now and not in the previous patch?

>  static void rproc_class_release(struct device *dev)
>  {
>  	struct rproc *rproc = container_of(dev, struct rproc, dev);
>  
> +	dev_info(&rproc->dev, "releasing %s\n", rproc->name);
> +
> +	rproc_delete_debug_dir(rproc);
> +
>  	idr_remove_all(&rproc->notifyids);
>  	idr_destroy(&rproc->notifyids);
>  
[snip]
> @@ -1603,8 +1584,8 @@ int rproc_unregister(struct rproc *rproc)
>  
>  	device_del(&rproc->dev);
>  
> -	/* the rproc will only be released after its refcount drops to zero */
> -	kref_put(&rproc->refcount, rproc_release);
> +	/* unroll rproc_alloc. TODO: we may want to let the users do that */
> +	put_device(&rproc->dev);

Yes I think we want rproc_free() to actually call put_device() the last
time and free the resources.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 1/2] remoteproc: maintain a generic child device for each rproc

[Ohad Ben-Cohen]

Hi Stephen,

As always - thanks for your review!

On Wed, May 30, 2012 at 11:36 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> It looks like remoteproc0, remoteproc1, etc. is what's used.

Thanks, I'll update the commit log.

> One complaint I've gotten is that the error messages are essentially
> useless now. I believe there are some ongoing discussions on lkml to fix
> this by traversing the device hierarchy to find the "real" device but
> the hard part is finding the real device.

You probably refer to the discussions around the input subsystem's pull request.

I was thinking about that too when creating this patch, and it looks
like whatever Greg will come up with on that matter will benefit us
too. So taking that into account, it might make more sense to do stick
with the virtual device rather than use the real one here (we'll end
up having more information in the long run).

>> +/* Unique numbering for remoteproc devices */
>> +static unsigned int dev_index;
>> +
>
> Hm... perhaps use that ida stuff instead of a raw integer?

That one got me thinking.

The immediate instinct is to do want to have a fully dynamic and
recyclable enumeration method, like ida provides.

But if you think of it, a mere integer have a strong advantage here:
the fact that the indices it provides don't recycle so fast is a plus,
because if a device was removed and recreated (or just removed and
another one then shows up), you get different indices. So a quick
glimpse at the logs is enough to tell that a new device was created.

But adding a spin lock to make this thread safe takes the simplicity
charm away. So in that respect, using an ida is much more attractive.

> I'm not clear on busses versus classes.

I think that busses is a whole lot more complex beast. Probably the
main indication we want one is when we need to match drivers to
devices.

In this case, I was more wondering between using a class to a device type.

> I recall seeing a thread where
> someone said classes were on the way out and shouldn't be used but I
> can't find it anymore.

I also remembered a similar discussion at a plumbers mini-conf about
2-3 years ago too, so I looked at device_type as an alternative to
class. The former looks somewhat simpler, but I couldn't find any
major advantage for using one over the other, and both seem to be in
use by many subsystems.

> Should we use classes for devices that will never
> have a matching driver?

It's not strictly required, but in case we want to provide these
devices some common behavior (and in our case we want them all to have
the same release handler, and very soon, the same PM handlers, too),
then a class (or a type) is helpful.

It looks like moving from a class to a type is quite trivial, in case
classes do eventually go away (or an advantage of using the latter
shows up), but I'm not aware of any other viable alternative for us
other than class/type.

>> +     /* Assign a unique device index and name */
>> +     rproc->index = dev_index++;
>> +     dev_set_name(&rproc->dev, "remoteproc%d", rproc->index);
>> +
>
> This doesn't look thread safe. ida would fix this (ida_simple_get/remove
> looks like what you want).

Yes, that's a good point, and will probably win this integer vs. ida case.

>> @@ -391,7 +392,7 @@ struct rproc {
>>       const char *firmware;
>>       void *priv;
>>       const struct rproc_ops *ops;
>> -     struct device *dev;
>> +     struct device dev;
>
> I'm not sure if the kernel-doc for this field is accurate anymore. Is it
> an 'underlying device' still?

It's not, thanks for pointing it out!

Thanks,
Ohad.

Re: [PATCH 2/2] remoteproc: remove the now-redundant kref

[Ohad Ben-Cohen]

On Wed, May 30, 2012 at 11:42 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> I was hoping we could use class_for_each_device() and
> class_find_device() to replace all this code. Then we wouldn't need all
> this klist stuff that the class is taking care of already.

Awesome! This really is worth a shot.

>> +/**
>> + * rproc_class_release() - release a remote processor instance
>> + * @dev: the rproc's device
>> + *
>> + * This function should _never_ be called directly.
>> + *
>> + * It will be called by the driver core when no one holds a valid pointer
>> + * to @dev anymore.
>> + */
>
> Why is this added now and not in the previous patch?

Hmm, probably because it was copied from rproc_release, which was
killed in this patch. I can probably shift it to the first patch since
I'm anyway doing some changes.

>> -     /* the rproc will only be released after its refcount drops to zero */
>> -     kref_put(&rproc->refcount, rproc_release);
>> +     /* unroll rproc_alloc. TODO: we may want to let the users do that */
>> +     put_device(&rproc->dev);
>
> Yes I think we want rproc_free() to actually call put_device() the last
> time and free the resources.

Yeah that was one of the options I considered.

In general, we have three options here:
1. Remove this last put_device invocation, and require users to call
rproc_free() even after they call rproc_unregister().
2. Let rproc_unregister() still do this, by calling rproc_free().
3. Let rproc_unregister() still do this, by invoking put_device().

I think that (1) looks better since it makes the interface symmetric
and straight forward.

(2) and (3) may be simper because users only need to call
rproc_unregister and that's it.

I eventually decided against (1) because I was concerned it will only
confuse users at this point.

But if you think that (1) is nicer too then maybe we should go ahead
and do that change.

Thanks,
Ohad.

Re: [PATCH 1/2] remoteproc: maintain a generic child device for each rproc

[Stephen Boyd]

(Sorry your mail was lost due to mail outage)

On 05/30/12 05:16, Ohad Ben-Cohen wrote:
>
> On Wed, May 30, 2012 at 11:36 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>
>> One complaint I've gotten is that the error messages are essentially
>> useless now. I believe there are some ongoing discussions on lkml to fix
>> this by traversing the device hierarchy to find the "real" device but
>> the hard part is finding the real device.
> You probably refer to the discussions around the input subsystem's pull request.
>
> I was thinking about that too when creating this patch, and it looks
> like whatever Greg will come up with on that matter will benefit us
> too. So taking that into account, it might make more sense to do stick
> with the virtual device rather than use the real one here (we'll end
> up having more information in the long run).

Fair enough. Hopefully something comes out of that discussion since this
will need it.

>> I'm not clear on busses versus classes.
> I think that busses is a whole lot more complex beast. Probably the
> main indication we want one is when we need to match drivers to
> devices.
>
> In this case, I was more wondering between using a class to a device type.
>
>> I recall seeing a thread where
>> someone said classes were on the way out and shouldn't be used but I
>> can't find it anymore.
> I also remembered a similar discussion at a plumbers mini-conf about
> 2-3 years ago too, so I looked at device_type as an alternative to
> class. The former looks somewhat simpler, but I couldn't find any
> major advantage for using one over the other, and both seem to be in
> use by many subsystems.
>
>> Should we use classes for devices that will never
>> have a matching driver?
> It's not strictly required, but in case we want to provide these
> devices some common behavior (and in our case we want them all to have
> the same release handler, and very soon, the same PM handlers, too),
> then a class (or a type) is helpful.
>
> It looks like moving from a class to a type is quite trivial, in case
> classes do eventually go away (or an advantage of using the latter
> shows up), but I'm not aware of any other viable alternative for us
> other than class/type.
>

Ok. Will moving from a class to a device type disrupt the kernel ABI?
First it will be under /sys/class/ and then under /sys/bus? Greg, can
you shed some light on when to use a class versus a device type?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] remoteproc: remove the now-redundant kref

[Stephen Boyd]

On 05/30/12 05:38, Ohad Ben-Cohen wrote:
> On Wed, May 30, 2012 at 11:42 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>> -     /* the rproc will only be released after its refcount drops to zero */
>>> -     kref_put(&rproc->refcount, rproc_release);
>>> +     /* unroll rproc_alloc. TODO: we may want to let the users do that */
>>> +     put_device(&rproc->dev);
>> Yes I think we want rproc_free() to actually call put_device() the last
>> time and free the resources.
> Yeah that was one of the options I considered.
>
> In general, we have three options here:
> 1. Remove this last put_device invocation, and require users to call
> rproc_free() even after they call rproc_unregister().
> 2. Let rproc_unregister() still do this, by calling rproc_free().
> 3. Let rproc_unregister() still do this, by invoking put_device().
>
> I think that (1) looks better since it makes the interface symmetric
> and straight forward.
>
> (2) and (3) may be simper because users only need to call
> rproc_unregister and that's it.
>
> I eventually decided against (1) because I was concerned it will only
> confuse users at this point.
>
> But if you think that (1) is nicer too then maybe we should go ahead
> and do that change.

Option 1 is nicer and it also follows the model other subsystems have
put forth such as the input subsystem.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 2/2] remoteproc: remove the now-redundant kref

[Ohad Ben-Cohen]

On Tue, Jun 5, 2012 at 12:22 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> Option 1 is nicer and it also follows the model other subsystems have
> put forth such as the input subsystem.

Sounds good, thanks!

Re: [PATCH 1/2] remoteproc: maintain a generic child device for each rproc

[Ohad Ben-Cohen]

On Wed, May 30, 2012 at 3:16 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> In this case, I was more wondering between using a class to a device type.
>
>> I recall seeing a thread where
>> someone said classes were on the way out and shouldn't be used but I
>> can't find it anymore.
>
> I also remembered a similar discussion at a plumbers mini-conf about
> 2-3 years ago too, so I looked at device_type as an alternative to
> class. The former looks somewhat simpler, but I couldn't find any
> major advantage for using one over the other, and both seem to be in
> use by many subsystems.

Moving to device_type is so trivial that I gave it a spin (and moved
to IDA too while at it):

>From caf8db2945ffe445e6b2b9e42b7afa14bae87d2c Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen <ohad@wizery.com>
Date: Wed, 30 May 2012 22:01:25 +0300
Subject: [PATCH 1/2] remoteproc: maintain a generic child device for each
 rproc

For each registered rproc, maintain a generic remoteproc device whose
parent is the low level platform-specific device (commonly a pdev, but
it may certainly be any other type of device too).

With this in hand, the resulting device hierarchy might then look like:

omap-rproc.0
 |
 - remoteproc0  <---- new !
    |
    - virtio0
    |
    - virtio1
       |
       - rpmsg0
       |
       - rpmsg1
       |
       - rpmsg2

Where:
- omap-rproc.0 is the low level device that's bound to the
  driver which invokes rproc_register()
- remoteproc0 is the result of this patch, and will be added by the
  remoteproc framework when rproc_register() is invoked
- virtio0 and virtio1 are vdevs that are registered by remoteproc
  when it realizes that they are supported by the firmware
  of the physical remote processor represented by omap-rproc.0
- rpmsg0, rpmsg1 and rpmsg2 are rpmsg devices that represent rpmsg
  channels, and are registerd by the rpmsg bus when it gets notified
  about their existence

Technically, this patch:
- changes 'struct rproc' to contain this generic remoteproc.x device
- creates a new "remoteproc" type, to which this new generic remoteproc.x
  device belong to.
- adds a super simple enumeration method for the indices of the
  remoteproc.x devices
- updates all dev_* messaging to use the generic remoteproc.x device
  instead of the low level platform-specific device
- updates all dma_* allocations to use the parent of remoteproc.x (where
  the platform-specific memory pools, most commonly CMA, are to be found)

Adding this generic device has several merits:
- we can now add remoteproc runtime PM support simply by hooking onto the
  new "remoteproc" type
- all remoteproc log messages will now carry a common name prefix
  instead of having a platform-specific one
- having a device as part of the rproc struct makes it possible to simplify
  refcounting (see subsequent patch)

Thanks to Stephen Boyd <sboyd@codeaurora.org> for suggesting and
discussing these ideas in one of the remoteproc review threads and
to Fernando Guzman Lugo <fernando.lugo@ti.com> for trying them out
with the (upcoming) runtime PM support for remoteproc.

Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Fernando Guzman Lugo <fernando.lugo@ti.com>
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 drivers/remoteproc/omap_remoteproc.c    |  17 ++--
 drivers/remoteproc/remoteproc_core.c    | 135 ++++++++++++++++++++++----------
 drivers/remoteproc/remoteproc_debugfs.c |   4 +-
 drivers/remoteproc/remoteproc_virtio.c  |  13 +--
 drivers/rpmsg/virtio_rpmsg_bus.c        |   3 +-
 include/linux/remoteproc.h              |   6 +-
 6 files changed, 117 insertions(+), 61 deletions(-)

diff --git a/drivers/remoteproc/omap_remoteproc.c
b/drivers/remoteproc/omap_remoteproc.c
index de138e30..f45230c 100644
--- a/drivers/remoteproc/omap_remoteproc.c
+++ b/drivers/remoteproc/omap_remoteproc.c
@@ -66,7 +66,7 @@ static int omap_rproc_mbox_callback(struct
notifier_block *this,
 {
 	mbox_msg_t msg = (mbox_msg_t) data;
 	struct omap_rproc *oproc = container_of(this, struct omap_rproc, nb);
-	struct device *dev = oproc->rproc->dev;
+	struct device *dev = oproc->rproc->dev.parent;
 	const char *name = oproc->rproc->name;

 	dev_dbg(dev, "mbox msg: 0x%x\n", msg);
@@ -92,12 +92,13 @@ static int omap_rproc_mbox_callback(struct
notifier_block *this,
 static void omap_rproc_kick(struct rproc *rproc, int vqid)
 {
 	struct omap_rproc *oproc = rproc->priv;
+	struct device *dev = rproc->dev.parent;
 	int ret;

 	/* send the index of the triggered virtqueue in the mailbox payload */
 	ret = omap_mbox_msg_send(oproc->mbox, vqid);
 	if (ret)
-		dev_err(rproc->dev, "omap_mbox_msg_send failed: %d\n", ret);
+		dev_err(dev, "omap_mbox_msg_send failed: %d\n", ret);
 }

 /*
@@ -110,7 +111,8 @@ static void omap_rproc_kick(struct rproc *rproc, int vqid)
 static int omap_rproc_start(struct rproc *rproc)
 {
 	struct omap_rproc *oproc = rproc->priv;
-	struct platform_device *pdev = to_platform_device(rproc->dev);
+	struct device *dev = rproc->dev.parent;
+	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_rproc_pdata *pdata = pdev->dev.platform_data;
 	int ret;

@@ -120,7 +122,7 @@ static int omap_rproc_start(struct rproc *rproc)
 	oproc->mbox = omap_mbox_get(pdata->mbox_name, &oproc->nb);
 	if (IS_ERR(oproc->mbox)) {
 		ret = PTR_ERR(oproc->mbox);
-		dev_err(rproc->dev, "omap_mbox_get failed: %d\n", ret);
+		dev_err(dev, "omap_mbox_get failed: %d\n", ret);
 		return ret;
 	}

@@ -133,13 +135,13 @@ static int omap_rproc_start(struct rproc *rproc)
 	 */
 	ret = omap_mbox_msg_send(oproc->mbox, RP_MBOX_ECHO_REQUEST);
 	if (ret) {
-		dev_err(rproc->dev, "omap_mbox_get failed: %d\n", ret);
+		dev_err(dev, "omap_mbox_get failed: %d\n", ret);
 		goto put_mbox;
 	}

 	ret = pdata->device_enable(pdev);
 	if (ret) {
-		dev_err(rproc->dev, "omap_device_enable failed: %d\n", ret);
+		dev_err(dev, "omap_device_enable failed: %d\n", ret);
 		goto put_mbox;
 	}

@@ -153,7 +155,8 @@ put_mbox:
 /* power off the remote processor */
 static int omap_rproc_stop(struct rproc *rproc)
 {
-	struct platform_device *pdev = to_platform_device(rproc->dev);
+	struct device *dev = rproc->dev.parent;
+	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_rproc_pdata *pdata = pdev->dev.platform_data;
 	struct omap_rproc *oproc = rproc->priv;
 	int ret;
diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index 464ea4f..46469b2 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -35,6 +35,7 @@
 #include <linux/debugfs.h>
 #include <linux/remoteproc.h>
 #include <linux/iommu.h>
+#include <linux/idr.h>
 #include <linux/klist.h>
 #include <linux/elf.h>
 #include <linux/virtio_ids.h>
@@ -66,6 +67,9 @@ typedef int (*rproc_handle_resources_t)(struct rproc *rproc,
 				struct resource_table *table, int len);
 typedef int (*rproc_handle_resource_t)(struct rproc *rproc, void *, int avail);

+/* Unique indices for remoteproc devices */
+static DEFINE_IDA(rproc_dev_index);
+
 /*
  * This is the IOMMU fault handler we register with the IOMMU API
  * (when relevant; not all remote processors access memory through
@@ -92,7 +96,7 @@ static int rproc_iommu_fault(struct iommu_domain
*domain, struct device *dev,
 static int rproc_enable_iommu(struct rproc *rproc)
 {
 	struct iommu_domain *domain;
-	struct device *dev = rproc->dev;
+	struct device *dev = rproc->dev.parent;
 	int ret;

 	/*
@@ -137,7 +141,7 @@ free_domain:
 static void rproc_disable_iommu(struct rproc *rproc)
 {
 	struct iommu_domain *domain = rproc->domain;
-	struct device *dev = rproc->dev;
+	struct device *dev = rproc->dev.parent;

 	if (!domain)
 		return;
@@ -217,7 +221,7 @@ static void *rproc_da_to_va(struct rproc *rproc,
u64 da, int len)
 static int
 rproc_load_segments(struct rproc *rproc, const u8 *elf_data, size_t len)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	struct elf32_hdr *ehdr;
 	struct elf32_phdr *phdr;
 	int i, ret = 0;
@@ -282,7 +286,7 @@ rproc_load_segments(struct rproc *rproc, const u8
*elf_data, size_t len)
 int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 {
 	struct rproc *rproc = rvdev->rproc;
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	struct rproc_vring *rvring = &rvdev->vring[i];
 	dma_addr_t dma;
 	void *va;
@@ -301,9 +305,9 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	 * 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);
+	va = dma_alloc_coherent(dev->parent, size, &dma, GFP_KERNEL);
 	if (!va) {
-		dev_err(dev, "dma_alloc_coherent failed\n");
+		dev_err(dev->parent, "dma_alloc_coherent failed\n");
 		return -EINVAL;
 	}

@@ -316,7 +320,7 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	ret = idr_get_new(&rproc->notifyids, rvring, &notifyid);
 	if (ret) {
 		dev_err(dev, "idr_get_new failed: %d\n", ret);
-		dma_free_coherent(dev, size, va, dma);
+		dma_free_coherent(dev->parent, size, va, dma);
 		return ret;
 	}

@@ -334,7 +338,7 @@ 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 device *dev = &rproc->dev;
 	struct fw_rsc_vdev_vring *vring = &rsc->vring[i];
 	struct rproc_vring *rvring = &rvdev->vring[i];

@@ -366,7 +370,7 @@ 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);
+	dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);
 	idr_remove(&rproc->notifyids, rvring->notifyid);
 }

@@ -400,14 +404,14 @@ void rproc_free_vring(struct rproc_vring *rvring)
 static int rproc_handle_vdev(struct rproc *rproc, struct fw_rsc_vdev *rsc,
 								int avail)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	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)
 			+ rsc->config_len > avail) {
-		dev_err(rproc->dev, "vdev rsc is truncated\n");
+		dev_err(dev, "vdev rsc is truncated\n");
 		return -EINVAL;
 	}

@@ -476,12 +480,12 @@ 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;
+	struct device *dev = &rproc->dev;
 	void *ptr;
 	char name[15];

 	if (sizeof(*rsc) > avail) {
-		dev_err(rproc->dev, "trace rsc is truncated\n");
+		dev_err(dev, "trace rsc is truncated\n");
 		return -EINVAL;
 	}

@@ -558,6 +562,7 @@ static int rproc_handle_devmem(struct rproc
*rproc, struct fw_rsc_devmem *rsc,
 								int avail)
 {
 	struct rproc_mem_entry *mapping;
+	struct device *dev = &rproc->dev;
 	int ret;

 	/* no point in handling this resource without a valid iommu domain */
@@ -565,25 +570,25 @@ static int rproc_handle_devmem(struct rproc
*rproc, struct fw_rsc_devmem *rsc,
 		return -EINVAL;

 	if (sizeof(*rsc) > avail) {
-		dev_err(rproc->dev, "devmem rsc is truncated\n");
+		dev_err(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");
+		dev_err(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");
+		dev_err(dev, "kzalloc mapping failed\n");
 		return -ENOMEM;
 	}

 	ret = iommu_map(rproc->domain, rsc->da, rsc->pa, rsc->len, rsc->flags);
 	if (ret) {
-		dev_err(rproc->dev, "failed to map devmem: %d\n", ret);
+		dev_err(dev, "failed to map devmem: %d\n", ret);
 		goto out;
 	}

@@ -598,7 +603,7 @@ static int rproc_handle_devmem(struct rproc
*rproc, struct fw_rsc_devmem *rsc,
 	mapping->len = rsc->len;
 	list_add_tail(&mapping->node, &rproc->mappings);

-	dev_dbg(rproc->dev, "mapped devmem pa 0x%x, da 0x%x, len 0x%x\n",
+	dev_dbg(dev, "mapped devmem pa 0x%x, da 0x%x, len 0x%x\n",
 					rsc->pa, rsc->da, rsc->len);

 	return 0;
@@ -630,13 +635,13 @@ 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;
+	struct device *dev = &rproc->dev;
 	dma_addr_t dma;
 	void *va;
 	int ret;

 	if (sizeof(*rsc) > avail) {
-		dev_err(rproc->dev, "carveout rsc is truncated\n");
+		dev_err(dev, "carveout rsc is truncated\n");
 		return -EINVAL;
 	}

@@ -662,9 +667,9 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		goto free_mapping;
 	}

-	va = dma_alloc_coherent(dev, rsc->len, &dma, GFP_KERNEL);
+	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
 	if (!va) {
-		dev_err(dev, "failed to dma alloc carveout: %d\n", rsc->len);
+		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
 		ret = -ENOMEM;
 		goto free_carv;
 	}
@@ -735,7 +740,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	return 0;

 dma_free:
-	dma_free_coherent(dev, rsc->len, va, dma);
+	dma_free_coherent(dev->parent, rsc->len, va, dma);
 free_carv:
 	kfree(carveout);
 free_mapping:
@@ -758,7 +763,7 @@ static rproc_handle_resource_t rproc_handle_rsc[] = {
 static int
 rproc_handle_boot_rsc(struct rproc *rproc, struct resource_table
*table, int len)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	rproc_handle_resource_t handler;
 	int ret = 0, i;

@@ -797,7 +802,7 @@ rproc_handle_boot_rsc(struct rproc *rproc, struct
resource_table *table, int len
 static int
 rproc_handle_virtio_rsc(struct rproc *rproc, struct resource_table
*table, int len)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	int ret = 0, i;

 	for (i = 0; i < table->num; i++) {
@@ -850,7 +855,7 @@ rproc_find_rsc_table(struct rproc *rproc, const u8
*elf_data, size_t len,
 	struct elf32_hdr *ehdr;
 	struct elf32_shdr *shdr;
 	const char *name_table;
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	struct resource_table *table = NULL;
 	int i;

@@ -916,7 +921,7 @@ rproc_find_rsc_table(struct rproc *rproc, const u8
*elf_data, size_t len,
 static void rproc_resource_cleanup(struct rproc *rproc)
 {
 	struct rproc_mem_entry *entry, *tmp;
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;

 	/* clean up debugfs trace entries */
 	list_for_each_entry_safe(entry, tmp, &rproc->traces, node) {
@@ -928,7 +933,7 @@ static void rproc_resource_cleanup(struct rproc *rproc)

 	/* clean up carveout allocations */
 	list_for_each_entry_safe(entry, tmp, &rproc->carveouts, node) {
-		dma_free_coherent(dev, entry->len, entry->va, entry->dma);
+		dma_free_coherent(dev->parent, entry->len, entry->va, entry->dma);
 		list_del(&entry->node);
 		kfree(entry);
 	}
@@ -953,7 +958,7 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 static int rproc_fw_sanity_check(struct rproc *rproc, const struct
firmware *fw)
 {
 	const char *name = rproc->firmware;
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	struct elf32_hdr *ehdr;
 	char class;

@@ -1014,7 +1019,7 @@ static int rproc_fw_sanity_check(struct rproc
*rproc, const struct firmware *fw)
  */
 static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	const char *name = rproc->firmware;
 	struct elf32_hdr *ehdr;
 	struct resource_table *table;
@@ -1139,7 +1144,7 @@ int rproc_boot(struct rproc *rproc)
 		return -EINVAL;
 	}

-	dev = rproc->dev;
+	dev = &rproc->dev;

 	/*
 	 * if asynchronoush fw loading is underway, wait up to 65 secs
@@ -1167,7 +1172,7 @@ int rproc_boot(struct rproc *rproc)
 	}

 	/* prevent underlying implementation from being removed */
-	if (!try_module_get(dev->driver->owner)) {
+	if (!try_module_get(dev->parent->driver->owner)) {
 		dev_err(dev, "%s: can't get owner\n", __func__);
 		ret = -EINVAL;
 		goto unlock_mutex;
@@ -1194,7 +1199,7 @@ int rproc_boot(struct rproc *rproc)

 downref_rproc:
 	if (ret) {
-		module_put(dev->driver->owner);
+		module_put(dev->parent->driver->owner);
 		atomic_dec(&rproc->power);
 	}
 unlock_mutex:
@@ -1228,7 +1233,7 @@ EXPORT_SYMBOL(rproc_boot);
  */
 void rproc_shutdown(struct rproc *rproc)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	int ret;

 	ret = mutex_lock_interruptible(&rproc->lock);
@@ -1261,7 +1266,7 @@ void rproc_shutdown(struct rproc *rproc)
 out:
 	mutex_unlock(&rproc->lock);
 	if (!ret)
-		module_put(dev->driver->owner);
+		module_put(dev->parent->driver->owner);
 }
 EXPORT_SYMBOL(rproc_shutdown);

@@ -1284,7 +1289,7 @@ void rproc_release(struct kref *kref)
 {
 	struct rproc *rproc = container_of(kref, struct rproc, refcount);

-	dev_info(rproc->dev, "removing %s\n", rproc->name);
+	dev_info(&rproc->dev, "removing %s\n", rproc->name);

 	rproc_delete_debug_dir(rproc);

@@ -1416,13 +1421,17 @@ EXPORT_SYMBOL(rproc_put);
  */
 int rproc_register(struct rproc *rproc)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	int ret = 0;

+	ret = device_add(dev);
+	if (ret < 0)
+		return ret;
+
 	/* expose to rproc_get_by_name users */
 	klist_add_tail(&rproc->node, &rprocs);

-	dev_info(rproc->dev, "%s is available\n", rproc->name);
+	dev_info(dev, "%s is available\n", rproc->name);

 	dev_info(dev, "Note: remoteproc is still under development and
considered experimental.\n");
 	dev_info(dev, "THE BINARY FORMAT IS NOT YET FINALIZED, and backward
compatibility isn't yet guaranteed.\n");
@@ -1455,6 +1464,33 @@ int rproc_register(struct rproc *rproc)
 EXPORT_SYMBOL(rproc_register);

 /**
+ * rproc_type_release() - release a remote processor instance
+ * @dev: the rproc's device
+ *
+ * This function should _never_ be called directly.
+ *
+ * It will be called by the driver core when no one holds a valid pointer
+ * to @dev anymore.
+ */
+static void rproc_type_release(struct device *dev)
+{
+	struct rproc *rproc = container_of(dev, struct rproc, dev);
+
+	idr_remove_all(&rproc->notifyids);
+	idr_destroy(&rproc->notifyids);
+
+	if (rproc->index >= 0)
+		ida_simple_remove(&rproc_dev_index, rproc->index);
+
+	kfree(rproc);
+}
+
+static struct device_type rproc_type = {
+	.name		= "remoteproc",
+	.release	= rproc_type_release,
+};
+
+/**
  * rproc_alloc() - allocate a remote processor handle
  * @dev: the underlying device
  * @name: name of this remote processor
@@ -1492,12 +1528,25 @@ struct rproc *rproc_alloc(struct device *dev,
const char *name,
 		return NULL;
 	}

-	rproc->dev = dev;
 	rproc->name = name;
 	rproc->ops = ops;
 	rproc->firmware = firmware;
 	rproc->priv = &rproc[1];

+	device_initialize(&rproc->dev);
+	rproc->dev.parent = dev;
+	rproc->dev.type = &rproc_type;
+
+	/* Assign a unique device index and name */
+	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
+	if (rproc->index < 0) {
+		dev_err(dev, "ida_simple_get failed: %d\n", rproc->index);
+		put_device(&rproc->dev);
+		return NULL;
+	}
+
+	dev_set_name(&rproc->dev, "remoteproc%d", rproc->index);
+
 	atomic_set(&rproc->power, 0);

 	kref_init(&rproc->refcount);
@@ -1529,10 +1578,7 @@ EXPORT_SYMBOL(rproc_alloc);
  */
 void rproc_free(struct rproc *rproc)
 {
-	idr_remove_all(&rproc->notifyids);
-	idr_destroy(&rproc->notifyids);
-
-	kfree(rproc);
+	put_device(&rproc->dev);
 }
 EXPORT_SYMBOL(rproc_free);

@@ -1573,6 +1619,8 @@ int rproc_unregister(struct rproc *rproc)
 	/* the rproc is downref'ed as soon as it's removed from the klist */
 	klist_del(&rproc->node);

+	device_del(&rproc->dev);
+
 	/* the rproc will only be released after its refcount drops to zero */
 	kref_put(&rproc->refcount, rproc_release);

@@ -1583,6 +1631,7 @@ EXPORT_SYMBOL(rproc_unregister);
 static int __init remoteproc_init(void)
 {
 	rproc_init_debugfs();
+
 	return 0;
 }
 module_init(remoteproc_init);
diff --git a/drivers/remoteproc/remoteproc_debugfs.c
b/drivers/remoteproc/remoteproc_debugfs.c
index 85d31a6..0383385 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -124,7 +124,7 @@ struct dentry *rproc_create_trace_file(const char
*name, struct rproc *rproc,
 	tfile = debugfs_create_file(name, 0400, rproc->dbg_dir,
 						trace, &trace_rproc_ops);
 	if (!tfile) {
-		dev_err(rproc->dev, "failed to create debugfs trace entry\n");
+		dev_err(&rproc->dev, "failed to create debugfs trace entry\n");
 		return NULL;
 	}

@@ -141,7 +141,7 @@ void rproc_delete_debug_dir(struct rproc *rproc)

 void rproc_create_debug_dir(struct rproc *rproc)
 {
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;

 	if (!rproc_dbg)
 		return;
diff --git a/drivers/remoteproc/remoteproc_virtio.c
b/drivers/remoteproc/remoteproc_virtio.c
index 26a7144..b662183 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -36,7 +36,7 @@ static void rproc_virtio_notify(struct virtqueue *vq)
 	struct rproc *rproc = rvring->rvdev->rproc;
 	int notifyid = rvring->notifyid;

-	dev_dbg(rproc->dev, "kicking vq index: %d\n", notifyid);
+	dev_dbg(&rproc->dev, "kicking vq index: %d\n", notifyid);

 	rproc->ops->kick(rproc, notifyid);
 }
@@ -57,7 +57,7 @@ irqreturn_t rproc_vq_interrupt(struct rproc *rproc,
int notifyid)
 {
 	struct rproc_vring *rvring;

-	dev_dbg(rproc->dev, "vq index %d is interrupted\n", notifyid);
+	dev_dbg(&rproc->dev, "vq index %d is interrupted\n", notifyid);

 	rvring = idr_find(&rproc->notifyids, notifyid);
 	if (!rvring || !rvring->vq)
@@ -74,6 +74,7 @@ static struct virtqueue *rp_find_vq(struct
virtio_device *vdev,
 {
 	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
 	struct rproc *rproc = vdev_to_rproc(vdev);
+	struct device *dev = &rproc->dev;
 	struct rproc_vring *rvring;
 	struct virtqueue *vq;
 	void *addr;
@@ -95,7 +96,7 @@ static struct virtqueue *rp_find_vq(struct
virtio_device *vdev,
 	size = vring_size(len, rvring->align);
 	memset(addr, 0, size);

-	dev_dbg(rproc->dev, "vring%d: va %p qsz %d notifyid %d\n",
+	dev_dbg(dev, "vring%d: va %p qsz %d notifyid %d\n",
 					id, addr, len, rvring->notifyid);

 	/*
@@ -105,7 +106,7 @@ static struct virtqueue *rp_find_vq(struct
virtio_device *vdev,
 	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);
+		dev_err(dev, "vring_new_virtqueue %s failed\n", name);
 		rproc_free_vring(rvring);
 		return ERR_PTR(-ENOMEM);
 	}
@@ -152,7 +153,7 @@ 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);
+		dev_err(&rproc->dev, "rproc_boot() failed %d\n", ret);
 		goto error;
 	}

@@ -254,7 +255,7 @@ static void rproc_vdev_release(struct device *dev)
 int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 {
 	struct rproc *rproc = rvdev->rproc;
-	struct device *dev = rproc->dev;
+	struct device *dev = &rproc->dev;
 	struct virtio_device *vdev = &rvdev->vdev;
 	int ret;

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index d5572e8..0af7fd3 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -964,7 +964,8 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	vrp->svq = vqs[1];

 	/* allocate coherent memory for the buffers */
-	bufs_va = dma_alloc_coherent(vdev->dev.parent, RPMSG_TOTAL_BUF_SPACE,
+	bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
+				RPMSG_TOTAL_BUF_SPACE,
 				&vrp->bufs_dma, GFP_KERNEL);
 	if (!bufs_va)
 		goto vqs_del;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index f1ffabb..7f806dc 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -369,7 +369,7 @@ enum rproc_state {
  * @firmware: name of firmware file to be loaded
  * @priv: private data which belongs to the platform-specific rproc module
  * @ops: platform-specific start/stop rproc handlers
- * @dev: underlying device
+ * @dev: virtual device for refcounting and common remoteproc behavior
  * @refcount: refcount of users that have a valid pointer to this rproc
  * @power: refcount of users who need this rproc powered up
  * @state: state of the device
@@ -383,6 +383,7 @@ enum rproc_state {
  * @bootaddr: address of first instruction to boot rproc with (optional)
  * @rvdevs: list of remote virtio devices
  * @notifyids: idr for dynamically assigning rproc-wide unique notify ids
+ * @index: index of this rproc device
  */
 struct rproc {
 	struct klist_node node;
@@ -391,7 +392,7 @@ struct rproc {
 	const char *firmware;
 	void *priv;
 	const struct rproc_ops *ops;
-	struct device *dev;
+	struct device dev;
 	struct kref refcount;
 	atomic_t power;
 	unsigned int state;
@@ -405,6 +406,7 @@ struct rproc {
 	u32 bootaddr;
 	struct list_head rvdevs;
 	struct idr notifyids;
+	int index;
 };

 /* we currently support only two vrings per rvdev */
-- 
1.7.10.rc3.743.gaa3bb87

Re: [PATCH 2/2] remoteproc: remove the now-redundant kref

[Ohad Ben-Cohen]

On Tue, Jun 5, 2012 at 12:22 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 05/30/12 05:38, Ohad Ben-Cohen wrote:
>> On Wed, May 30, 2012 at 11:42 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>>> -     /* the rproc will only be released after its refcount drops to zero */
>>>> -     kref_put(&rproc->refcount, rproc_release);
>>>> +     /* unroll rproc_alloc. TODO: we may want to let the users do that */
>>>> +     put_device(&rproc->dev);
>>> Yes I think we want rproc_free() to actually call put_device() the last
>>> time and free the resources.
>> Yeah that was one of the options I considered.
>>
>> In general, we have three options here:
>> 1. Remove this last put_device invocation, and require users to call
>> rproc_free() even after they call rproc_unregister().
>> 2. Let rproc_unregister() still do this, by calling rproc_free().
>> 3. Let rproc_unregister() still do this, by invoking put_device().
>>
>> I think that (1) looks better since it makes the interface symmetric
>> and straight forward.
>>
>> (2) and (3) may be simper because users only need to call
>> rproc_unregister and that's it.
>>
>> I eventually decided against (1) because I was concerned it will only
>> confuse users at this point.
>>
>> But if you think that (1) is nicer too then maybe we should go ahead
>> and do that change.
>
> Option 1 is nicer and it also follows the model other subsystems have
> put forth such as the input subsystem.

>From 0fbf3004c1a52ae4c0554366409a2bfe401801ef Mon Sep 17 00:00:00 2001
From: Ohad Ben-Cohen <ohad@wizery.com>
Date: Mon, 2 Jul 2012 11:41:16 +0300
Subject: [PATCH] remoteproc: simplify unregister/free interfaces

Simplify the unregister/free interfaces, and make them easier
to understand and use, by moving to a symmetric and consistent
alloc() -> register() -> unregister() -> free() flow.

To create and register an rproc instance, one needed to invoke
rproc_alloc() followed by rproc_register().

To unregister and free an rproc instance, one now needs to invoke
rproc_unregister() followed by rproc_free().

Cc: Stephen Boyd <sboyd@codeaurora.org>
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
---
 Documentation/remoteproc.txt         | 21 ++++++++-------------
 drivers/remoteproc/omap_remoteproc.c |  5 ++++-
 drivers/remoteproc/remoteproc_core.c | 25 ++++++++-----------------
 3 files changed, 20 insertions(+), 31 deletions(-)

diff --git a/Documentation/remoteproc.txt b/Documentation/remoteproc.txt
index 70a048c..ad6ded4 100644
--- a/Documentation/remoteproc.txt
+++ b/Documentation/remoteproc.txt
@@ -120,14 +120,14 @@ int dummy_rproc_example(struct rproc *my_rproc)
       On success, the new rproc is returned, and on failure, NULL.

       Note: _never_ directly deallocate @rproc, even if it was not registered
-      yet. Instead, if you just need to unroll rproc_alloc(), use rproc_free().
+      yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().

   void rproc_free(struct rproc *rproc)
     - Free an rproc handle that was allocated by rproc_alloc.
-      This function should _only_ be used if @rproc was only allocated,
-      but not registered yet.
-      If @rproc was already successfully registered (by calling
-      rproc_register()), then use rproc_unregister() instead.
+      This function essentially unrolls rproc_alloc(), by decrementing the
+      rproc's refcount. It doesn't directly free rproc; that would happen
+      only if there are no other references to rproc and its refcount now
+      dropped to zero.

   int rproc_register(struct rproc *rproc)
     - Register @rproc with the remoteproc framework, after it has been
@@ -143,19 +143,14 @@ int dummy_rproc_example(struct rproc *my_rproc)
       probed.

   int rproc_unregister(struct rproc *rproc)
-    - Unregister a remote processor, and decrement its refcount.
-      If its refcount drops to zero, then @rproc will be freed. If not,
-      it will be freed later once the last reference is dropped.
-
+    - Unroll rproc_register().
       This function should be called when the platform specific rproc
       implementation decides to remove the rproc device. it should
       _only_ be called if a previous invocation of rproc_register()
       has completed successfully.

-      After rproc_unregister() returns, @rproc is _not_ valid anymore and
-      it shouldn't be used. More specifically, don't call rproc_free()
-      or try to directly free @rproc after rproc_unregister() returns;
-      none of these are needed, and calling them is a bug.
+      After rproc_unregister() returns, @rproc is still valid, and its
+      last refcount should be decremented by calling rproc_free().

       Returns 0 on success and -EINVAL if @rproc isn't valid.

diff --git a/drivers/remoteproc/omap_remoteproc.c
b/drivers/remoteproc/omap_remoteproc.c
index f45230c..0f1afc9 100644
--- a/drivers/remoteproc/omap_remoteproc.c
+++ b/drivers/remoteproc/omap_remoteproc.c
@@ -214,7 +214,10 @@ static int __devexit omap_rproc_remove(struct
platform_device *pdev)
 {
 	struct rproc *rproc = platform_get_drvdata(pdev);

-	return rproc_unregister(rproc);
+	rproc_unregister(rproc);
+	rproc_free(rproc);
+
+	return 0;
 }

 static struct platform_driver omap_rproc_driver = {
diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index c0c0311..ac2d7163 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1485,7 +1485,7 @@ static struct device_type rproc_type = {
  * On success the new rproc is returned, and on failure, NULL.
  *
  * Note: _never_ directly deallocate @rproc, even if it was not registered
- * yet. Instead, if you just need to unroll rproc_alloc(), use rproc_free().
+ * yet. Instead, when you need to unroll rproc_alloc(), use rproc_free().
  */
 struct rproc *rproc_alloc(struct device *dev, const char *name,
 				const struct rproc_ops *ops,
@@ -1539,14 +1539,13 @@ struct rproc *rproc_alloc(struct device *dev,
const char *name,
 EXPORT_SYMBOL(rproc_alloc);

 /**
- * rproc_free() - free an rproc handle that was allocated by rproc_alloc
+ * rproc_free() - unroll rproc_alloc()
  * @rproc: the remote processor handle
  *
- * This function should _only_ be used if @rproc was only allocated,
- * but not registered yet.
+ * This function decrements the rproc dev refcount.
  *
- * If @rproc was already successfully registered (by calling rproc_register()),
- * then use rproc_unregister() instead.
+ * If no one holds any reference to rproc anymore, then its refcount would
+ * now drop to zero, and it would be freed.
  */
 void rproc_free(struct rproc *rproc)
 {
@@ -1558,19 +1557,14 @@ EXPORT_SYMBOL(rproc_free);
  * rproc_unregister() - unregister a remote processor
  * @rproc: rproc handle to unregister
  *
- * Unregisters a remote processor, and decrements its refcount.
- * If its refcount drops to zero, then @rproc will be freed. If not,
- * it will be freed later once the last reference is dropped.
- *
  * This function should be called when the platform specific rproc
  * implementation decides to remove the rproc device. it should
  * _only_ be called if a previous invocation of rproc_register()
  * has completed successfully.
  *
- * After rproc_unregister() returns, @rproc is _not_ valid anymore and
- * it shouldn't be used. More specifically, don't call rproc_free()
- * or try to directly free @rproc after rproc_unregister() returns;
- * none of these are needed, and calling them is a bug.
+ * After rproc_unregister() returns, @rproc isn't freed yet, because
+ * of the outstanding reference created by rproc_alloc. To decrement that
+ * one last refcount, one still needs to call rproc_free().
  *
  * Returns 0 on success and -EINVAL if @rproc isn't valid.
  */
@@ -1593,9 +1587,6 @@ int rproc_unregister(struct rproc *rproc)

 	device_del(&rproc->dev);

-	/* unroll rproc_alloc. TODO: we may want to let the users do that */
-	put_device(&rproc->dev);
-
 	return 0;
 }
 EXPORT_SYMBOL(rproc_unregister);
-- 
1.7.10.rc3.743.gaa3bb87

Re: [PATCH 2/2] remoteproc: remove the now-redundant kref

[Russell King - ARM Linux]

On Mon, Jul 02, 2012 at 11:52:27AM +0300, Ohad Ben-Cohen wrote:
> Simplify the unregister/free interfaces, and make them easier
> to understand and use, by moving to a symmetric and consistent
> alloc() -> register() -> unregister() -> free() flow.

The naming in the driver model is:

alloc() -> add() -> del() -> put()

where alloc() is an allocation + initialization, and

register() -> unregister()

where register() is initialization + add() and
unregister() is del() + put().

Re: [PATCH 2/2] remoteproc: remove the now-redundant kref

[Ohad Ben-Cohen]

On Mon, Jul 2, 2012 at 11:59 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jul 02, 2012 at 11:52:27AM +0300, Ohad Ben-Cohen wrote:
>> Simplify the unregister/free interfaces, and make them easier
>> to understand and use, by moving to a symmetric and consistent
>> alloc() -> register() -> unregister() -> free() flow.
>
> The naming in the driver model is:
>
> alloc() -> add() -> del() -> put()
>
> where alloc() is an allocation + initialization, and
>
> register() -> unregister()
>
> where register() is initialization + add() and
> unregister() is del() + put().

I wasn't sure if it'd help to adopt the driver model's naming, but I
guess it just might do, so I'll do that.

Thanks!

Re: [PATCH 1/2] remoteproc: maintain a generic child device for each rproc

[Stephen Boyd]

On 06/29/12 01:13, Ohad Ben-Cohen wrote:
> On Wed, May 30, 2012 at 3:16 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
>> In this case, I was more wondering between using a class to a device type.
>>
>>> I recall seeing a thread where
>>> someone said classes were on the way out and shouldn't be used but I
>>> can't find it anymore.
>> I also remembered a similar discussion at a plumbers mini-conf about
>> 2-3 years ago too, so I looked at device_type as an alternative to
>> class. The former looks somewhat simpler, but I couldn't find any
>> major advantage for using one over the other, and both seem to be in
>> use by many subsystems.
> Moving to device_type is so trivial that I gave it a spin (and moved
> to IDA too while at it):

Great! It looks like device_type doesn't have any list iteration support
though. Is that requirement gone? If that requirement is still there I
would think we need something like a class or bus still.

Will you resend this as part of a series? It will be easier to review then.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 1/2] remoteproc: maintain a generic child device for each rproc

[Ohad Ben-Cohen]

On Mon, Jul 2, 2012 at 10:06 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> Great! It looks like device_type doesn't have any list iteration support
> though. Is that requirement gone?

Pretty much, yeah. I'll soon post a separate patch which removes the
get_by_name functionality (together with its related klist).

> Will you resend this as part of a series? It will be easier to review then.

Not sure. There's a collection of discrete patches that I've been
posting, but they really aren't an organic series: as long as the
dependencies are met, each and every one of them is applicable even if
applied alone.

So I'd prefer (when possible) to treat patches in a discrete fashion
so we can start applying them and unblock others who depend on them
(e.g. Fernando's runtime PM work depends on this one).

But if you prefer me to send this one patch differently to make it
easier to review, let me know!

Thanks,
Ohad.

Re: [PATCH 1/2] remoteproc: maintain a generic child device for each rproc

[Stephen Boyd]

On 07/02/12 12:54, Ohad Ben-Cohen wrote:
> On Mon, Jul 2, 2012 at 10:06 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> Great! It looks like device_type doesn't have any list iteration support
>> though. Is that requirement gone?
> Pretty much, yeah. I'll soon post a separate patch which removes the
> get_by_name functionality (together with its related klist).
>
>> Will you resend this as part of a series? It will be easier to review then.
> Not sure. There's a collection of discrete patches that I've been
> posting, but they really aren't an organic series: as long as the
> dependencies are met, each and every one of them is applicable even if
> applied alone.
>
> So I'd prefer (when possible) to treat patches in a discrete fashion
> so we can start applying them and unblock others who depend on them
> (e.g. Fernando's runtime PM work depends on this one).
>
> But if you prefer me to send this one patch differently to make it
> easier to review, let me know!
>

Ok then. Please add

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

It would be nice if you got an ack from Greg or Kay on the device_type
usage too.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH 1/2] remoteproc: maintain a generic child device for each rproc

[Ohad Ben-Cohen]

On Thu, Jul 5, 2012 at 11:35 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> Ok then. Please add
>
> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

Added, and applied patch. thanks!

> It would be nice if you got an ack from Greg or Kay on the device_type
> usage too.

I agree, I'd just hate bothering them on this now. Probably a topic
for a conference chat :)

Re: [PATCH 2/2] remoteproc: remove the now-redundant kref

[Ohad Ben-Cohen]

On Sat, May 26, 2012 at 10:36 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Now that every rproc instance contains a device, we don't need a
> kref anymore to maintain the refcount of the rproc instances:
> that's what device are good with!
>
> This patch removes the now-redundant kref, and switches to
> {get, put}_device instead of kref_{get, put}.
>
> We also don't need the kref's release function anymore, and instead,
> we just utilize the class's release handler (which is now responsible
> for all memory de-allocations).
>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: Fernando Guzman Lugo <fernando.lugo@ti.com>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> ---
>  drivers/remoteproc/remoteproc_core.c   |   59 +++++++++++---------------------
>  drivers/remoteproc/remoteproc_virtio.c |    8 ++--
>  include/linux/remoteproc.h             |    3 --
>  3 files changed, 24 insertions(+), 46 deletions(-)

Applied (after moving the kerneldoc for rproc_class_release() to the
patch where that function was introduced).

Re: [PATCH 2/2] remoteproc: remove the now-redundant kref

[Ohad Ben-Cohen]

On Mon, Jul 2, 2012 at 11:52 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> From 0fbf3004c1a52ae4c0554366409a2bfe401801ef Mon Sep 17 00:00:00 2001
> From: Ohad Ben-Cohen <ohad@wizery.com>
> Date: Mon, 2 Jul 2012 11:41:16 +0300
> Subject: [PATCH] remoteproc: simplify unregister/free interfaces
>
> Simplify the unregister/free interfaces, and make them easier
> to understand and use, by moving to a symmetric and consistent
> alloc() -> register() -> unregister() -> free() flow.
>
> To create and register an rproc instance, one needed to invoke
> rproc_alloc() followed by rproc_register().
>
> To unregister and free an rproc instance, one now needs to invoke
> rproc_unregister() followed by rproc_free().
>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> ---
>  Documentation/remoteproc.txt         | 21 ++++++++-------------
>  drivers/remoteproc/omap_remoteproc.c |  5 ++++-
>  drivers/remoteproc/remoteproc_core.c | 25 ++++++++-----------------
>  3 files changed, 20 insertions(+), 31 deletions(-)

Applied.