Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1395124 > unrolled thread

[PATCH 0/5] remoteproc: A few important improvements

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] remoteproc: A few important improvements Lee Jones <lee.jones@linaro.org> - 2016-05-05 15:40 +0200
    [PATCH 3/5] remoteproc: core: Add ability to select a firmware from the client Lee Jones <lee.jones@linaro.org> - 2016-05-05 15:40 +0200
      Re: [PATCH 3/5] remoteproc: core: Add ability to select a firmware  from the client Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-05-06 21:00 +0200
        Re: [PATCH 3/5] remoteproc: core: Add ability to select a firmware  from the client Lee Jones <lee.jones@linaro.org> - 2016-05-10 15:10 +0200
    [PATCH 4/5] remoteproc: core: Supply framework to request, declare and fetch shared memory Lee Jones <lee.jones@linaro.org> - 2016-05-05 15:40 +0200
      Re: [PATCH 4/5] remoteproc: core: Supply framework to request,  declare and fetch shared memory Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-05-12 00:40 +0200
    [PATCH 1/5] remoteproc: core: Task sync during rproc_fw_boot() Lee Jones <lee.jones@linaro.org> - 2016-05-05 15:40 +0200
      Re: [PATCH 1/5] remoteproc: core: Task sync during rproc_fw_boot() Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-05-06 20:50 +0200

#1395124 — [PATCH 0/5] remoteproc: A few important improvements

This set contains some extensions which allow greater interoperability
with other heavily used subsystems/booting mechanisms already in the
kernel.  This includes; Device Tree (OF) support, fix possible race
between remoteproc and rpmsg/virtio, remotely (from a client) set the
firmware name, introduction of a nice way to handle shared (between
subsystems) memory and automatic carveout clipping.

Lee Jones (5):
  remoteproc: core: Task sync during rproc_fw_boot()
  remoteproc: core: Add rproc OF look-up functions
  remoteproc: core: Add ability to select a firmware from the client
  remoteproc: core: Supply framework to request, declare and fetch
    shared memory
  remoteproc: core: Clip carveout if it's too big

 drivers/remoteproc/remoteproc_core.c     | 371 ++++++++++++++++++++++++++++++-
 drivers/remoteproc/remoteproc_internal.h |   1 +
 drivers/remoteproc/remoteproc_virtio.c   |   2 +-
 include/linux/remoteproc.h               |  16 ++
 4 files changed, 379 insertions(+), 11 deletions(-)

-- 
2.8.0

[toc] | [next] | [standalone]

#1395125 — [PATCH 3/5] remoteproc: core: Add ability to select a firmware from the client

ST Co-Processors can be loaded with different firmwares to execute
specific tasks without the need for unloading the rproc module.

This patch provides a function which can update the firmware name.

NB: This can only happen if the rproc is offline.

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 63 ++++++++++++++++++++++++++++++++++++
 include/linux/remoteproc.h           | 13 ++++++++
 2 files changed, 76 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 85e5fd8..03720c0 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1004,6 +1004,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
 
 	/* Free the copy of the resource table */
 	kfree(rproc->cached_table);
+	rproc->cached_table = NULL;
 
 	return rproc_add_virtio_devices(rproc);
 }
@@ -1329,6 +1330,66 @@ struct rproc *rproc_get_by_phandle(phandle phandle)
 EXPORT_SYMBOL(rproc_get_by_phandle);
 
 /**
+ * rproc_set_fw_name() - change rproc fw name
+ * @rproc: rproc handle
+ * @firmware: name of firmware file to load
+ *
+ * set a new firmware name for rproc handle
+ * firmware name can be updated only if the rproc is offline
+ * if firmware name is NULL the fw name is set on default name
+ *
+ * this function can wait, if the old fw config virtio is not yet finish
+ * (fw config request is asynchronous)
+ *
+ * Returns 0 on success and an appropriate error code otherwise.
+ */
+int rproc_set_fw_name(struct rproc *rproc, const char *firmware)
+{
+	struct rproc_vdev *rvdev, *rvtmp;
+
+	if (!rproc)
+		return -EINVAL;
+
+	/* if rproc is just being registered, wait */
+	wait_for_completion(&rproc->firmware_loading_complete);
+
+	mutex_lock(&rproc->lock);
+
+	if (rproc->state != RPROC_OFFLINE) {
+		mutex_unlock(&rproc->lock);
+		return -EBUSY;
+	}
+
+	if (rproc->firmware && rproc->firmware != rproc->orig_firmware)
+		kfree(rproc->firmware);
+
+	/* restore original fw name */
+	if (!firmware) {
+		rproc->firmware = rproc->orig_firmware;
+	} else {
+		rproc->firmware = kstrdup(firmware, GFP_KERNEL);
+		if (!rproc->firmware)
+			rproc->firmware = rproc->orig_firmware;
+	}
+
+	dev_info(&rproc->dev, "%s, fw name updated with:%s\n",
+		 rproc->name, rproc->firmware);
+
+	mutex_unlock(&rproc->lock);
+
+	/* clean up remote vdev entries */
+	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
+		rproc_remove_virtio_dev(rvdev);
+
+	/* Free the copy of the resource table */
+	kfree(rproc->cached_table);
+	rproc->cached_table = NULL;
+
+	return rproc_add_virtio_devices(rproc);
+}
+EXPORT_SYMBOL(rproc_set_fw_name);
+
+/**
  * rproc_add() - register a remote processor
  * @rproc: the remote processor handle to register
  *
@@ -1467,6 +1528,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	}
 
 	rproc->firmware = p;
+	rproc->orig_firmware = p;
 	rproc->name = name;
 	rproc->ops = ops;
 	rproc->priv = &rproc[1];
@@ -1554,6 +1616,7 @@ int rproc_del(struct rproc *rproc)
 
 	/* Free the copy of the resource table */
 	kfree(rproc->cached_table);
+	rproc->cached_table = NULL;
 
 	/* the rproc is downref'ed as soon as it's removed from the klist */
 	mutex_lock(&rproc_list_mutex);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 4c96e78..978e866 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -414,6 +414,7 @@ struct rproc {
 	struct iommu_domain *domain;
 	const char *name;
 	const char *firmware;
+	const char *orig_firmware;
 	void *priv;
 	const struct rproc_ops *ops;
 	struct device dev;
@@ -484,6 +485,18 @@ struct rproc_vdev {
 	u32 rsc_offset;
 };
 
+struct rproc_subdev {
+	struct device dev;
+	struct rproc *rproc;
+	struct resource *res;
+};
+
+#define to_subdevice(d) container_of(d, struct rproc_subdev, dev)
+struct rproc_subdev *rproc_subdev_add(struct rproc *rproc,
+				      struct resource *res);
+void rproc_subdev_del(struct rproc_subdev *subdev);
+struct device *rproc_subdev_lookup(struct rproc *rproc, const char *name);
+int rproc_set_fw_name(struct rproc *rproc, const char *firmware);
 struct rproc *rproc_get_by_phandle(phandle phandle);
 struct rproc *rproc_alloc(struct device *dev, const char *name,
 				const struct rproc_ops *ops,
-- 
2.8.0

[toc] | [prev] | [next] | [standalone]

#1396069 — Re: [PATCH 3/5] remoteproc: core: Add ability to select a firmware from the client

On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:

> ST Co-Processors can be loaded with different firmwares to execute
> specific tasks without the need for unloading the rproc module.
> 

I'm very much interested in ideas related to this and who "owns" the
life cycle of remoteprocs, do you have any code I can take a look at
that's using this interface?

> This patch provides a function which can update the firmware name.
> 
> NB: This can only happen if the rproc is offline.
> 

How is this working in the case when the remoteproc provides vdevs that
is holding references towards the rproc?

Do you unload rpmsg et al before doing this?

> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 63 ++++++++++++++++++++++++++++++++++++
>  include/linux/remoteproc.h           | 13 ++++++++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 85e5fd8..03720c0 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1004,6 +1004,7 @@ int rproc_trigger_recovery(struct rproc *rproc)
>  
>  	/* Free the copy of the resource table */
>  	kfree(rproc->cached_table);
> +	rproc->cached_table = NULL;
>  

Somewhat unrelated, but should be fixed, so let's go with it.

>  	return rproc_add_virtio_devices(rproc);
>  }
> @@ -1329,6 +1330,66 @@ struct rproc *rproc_get_by_phandle(phandle phandle)
>  EXPORT_SYMBOL(rproc_get_by_phandle);
>  
>  /**
> + * rproc_set_fw_name() - change rproc fw name
> + * @rproc: rproc handle
> + * @firmware: name of firmware file to load
> + *
> + * set a new firmware name for rproc handle
> + * firmware name can be updated only if the rproc is offline
> + * if firmware name is NULL the fw name is set on default name
> + *
> + * this function can wait, if the old fw config virtio is not yet finish
> + * (fw config request is asynchronous)
> + *
> + * Returns 0 on success and an appropriate error code otherwise.
> + */
> +int rproc_set_fw_name(struct rproc *rproc, const char *firmware)
> +{
> +	struct rproc_vdev *rvdev, *rvtmp;
> +
> +	if (!rproc)
> +		return -EINVAL;
> +
> +	/* if rproc is just being registered, wait */
> +	wait_for_completion(&rproc->firmware_loading_complete);
> +
> +	mutex_lock(&rproc->lock);
> +
> +	if (rproc->state != RPROC_OFFLINE) {
> +		mutex_unlock(&rproc->lock);
> +		return -EBUSY;
> +	}
> +
> +	if (rproc->firmware && rproc->firmware != rproc->orig_firmware)
> +		kfree(rproc->firmware);

kfree(NULL) is fine, so you can drop the first part of the expression.

> +
> +	/* restore original fw name */
> +	if (!firmware) {
> +		rproc->firmware = rproc->orig_firmware;
> +	} else {
> +		rproc->firmware = kstrdup(firmware, GFP_KERNEL);
> +		if (!rproc->firmware)
> +			rproc->firmware = rproc->orig_firmware;

In the unlikely event that kstrdup fails I we should leave
rproc->firmware unchanged and return an error here.

As this is written we will silently (with a blarg in the log if anyone
checks) switch back to the default firmware and boot that.

> +	}
> +
> +	dev_info(&rproc->dev, "%s, fw name updated with:%s\n",
> +		 rproc->name, rproc->firmware);
> +
> +	mutex_unlock(&rproc->lock);
> +
> +	/* clean up remote vdev entries */
> +	list_for_each_entry_safe(rvdev, rvtmp, &rproc->rvdevs, node)
> +		rproc_remove_virtio_dev(rvdev);
> +
> +	/* Free the copy of the resource table */
> +	kfree(rproc->cached_table);
> +	rproc->cached_table = NULL;
> +
> +	return rproc_add_virtio_devices(rproc);
> +}
> +EXPORT_SYMBOL(rproc_set_fw_name);
[..]
>  
> +struct rproc_subdev {
> +	struct device dev;
> +	struct rproc *rproc;
> +	struct resource *res;
> +};
> +
> +#define to_subdevice(d) container_of(d, struct rproc_subdev, dev)
> +struct rproc_subdev *rproc_subdev_add(struct rproc *rproc,
> +				      struct resource *res);
> +void rproc_subdev_del(struct rproc_subdev *subdev);
> +struct device *rproc_subdev_lookup(struct rproc *rproc, const char *name);
> +int rproc_set_fw_name(struct rproc *rproc, const char *firmware);

These belongs to the next patch.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1398072 — Re: [PATCH 3/5] remoteproc: core: Add ability to select a firmware from the client

On Fri, 06 May 2016, Bjorn Andersson wrote:

> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
> 
> > ST Co-Processors can be loaded with different firmwares to execute
> > specific tasks without the need for unloading the rproc module.
> > 
> 
> I'm very much interested in ideas related to this and who "owns" the
> life cycle of remoteprocs, do you have any code I can take a look at
> that's using this interface?

I was part of a conversation on the list which should explain some of
your queries [0].  The conclusion was, that only the client can know
what which firmware it is compatible with.  This information can not
realistically either live in DT (or any other platform data) or within
RemoteProc.  I hope that link answers your questions.

[0] http://www.spinics.net/lists/devicetree-spec/msg00144.html

> > This patch provides a function which can update the firmware name.
> > 
> > NB: This can only happen if the rproc is offline.
> 
> How is this working in the case when the remoteproc provides vdevs that
> is holding references towards the rproc?
> 
> Do you unload rpmsg et al before doing this?

RemoteProc needs to be offline for the firmware name to be set, this
includes links to RPMsg, so yes, they would have to be unloaded.

> > Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 63 ++++++++++++++++++++++++++++++++++++
> >  include/linux/remoteproc.h           | 13 ++++++++
> >  2 files changed, 76 insertions(+)

Happy with the remainder of your comments -- will fix.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1395129 — [PATCH 4/5] remoteproc: core: Supply framework to request, declare and fetch shared memory

Normally used for management of; carveout, devmem and trace memory.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c | 174 +++++++++++++++++++++++++++++++++--
 1 file changed, 167 insertions(+), 7 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 03720c0..3d9798c 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -209,6 +209,7 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 {
 	struct rproc *rproc = rvdev->rproc;
 	struct device *dev = &rproc->dev;
+	struct device *dma_dev;
 	struct rproc_vring *rvring = &rvdev->vring[i];
 	struct fw_rsc_vdev *rsc;
 	dma_addr_t dma;
@@ -222,7 +223,8 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	 * Allocate non-cacheable memory for the vring. In the future
 	 * this call will also configure the IOMMU for us
 	 */
-	va = dma_alloc_coherent(dev->parent, size, &dma, GFP_KERNEL);
+	dma_dev = rproc_subdev_lookup(rproc, "vring");
+	va = dma_alloc_coherent(dma_dev, size, &dma, GFP_KERNEL);
 	if (!va) {
 		dev_err(dev->parent, "dma_alloc_coherent failed\n");
 		return -EINVAL;
@@ -236,7 +238,7 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
 	ret = idr_alloc(&rproc->notifyids, rvring, 0, 0, GFP_KERNEL);
 	if (ret < 0) {
 		dev_err(dev, "idr_alloc failed: %d\n", ret);
-		dma_free_coherent(dev->parent, size, va, dma);
+		dma_free_coherent(dma_dev, size, va, dma);
 		return ret;
 	}
 	notifyid = ret;
@@ -297,8 +299,10 @@ void rproc_free_vring(struct rproc_vring *rvring)
 	struct rproc *rproc = rvring->rvdev->rproc;
 	int idx = rvring->rvdev->vring - rvring;
 	struct fw_rsc_vdev *rsc;
+	struct device *dma_dev;
 
-	dma_free_coherent(rproc->dev.parent, size, rvring->va, rvring->dma);
+	dma_dev = rproc_subdev_lookup(rproc, "vring");
+	dma_free_coherent(dma_dev, size, rvring->va, rvring->dma);
 	idr_remove(&rproc->notifyids, rvring->notifyid);
 
 	/* reset resource entry info */
@@ -572,6 +576,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 {
 	struct rproc_mem_entry *carveout, *mapping;
 	struct device *dev = &rproc->dev;
+	struct device *dma_dev;
 	dma_addr_t dma;
 	void *va;
 	int ret;
@@ -594,7 +599,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	if (!carveout)
 		return -ENOMEM;
 
-	va = dma_alloc_coherent(dev->parent, rsc->len, &dma, GFP_KERNEL);
+	dma_dev = rproc_subdev_lookup(rproc, "carveout");
+	va = dma_alloc_coherent(dma_dev, rsc->len, &dma, GFP_KERNEL);
 	if (!va) {
 		dev_err(dev->parent, "dma_alloc_coherent err: %d\n", rsc->len);
 		ret = -ENOMEM;
@@ -682,7 +688,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 free_mapping:
 	kfree(mapping);
 dma_free:
-	dma_free_coherent(dev->parent, rsc->len, va, dma);
+	dma_free_coherent(dma_dev, rsc->len, va, dma);
 free_carv:
 	kfree(carveout);
 	return ret;
@@ -766,6 +772,7 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 {
 	struct rproc_mem_entry *entry, *tmp;
 	struct device *dev = &rproc->dev;
+	struct device *dma_dev;
 
 	/* clean up debugfs trace entries */
 	list_for_each_entry_safe(entry, tmp, &rproc->traces, node) {
@@ -791,9 +798,9 @@ static void rproc_resource_cleanup(struct rproc *rproc)
 	}
 
 	/* clean up carveout allocations */
+	dma_dev = rproc_subdev_lookup(rproc, "carveout");
 	list_for_each_entry_safe(entry, tmp, &rproc->carveouts, node) {
-		dma_free_coherent(dev->parent, entry->len, entry->va,
-				  entry->dma);
+		dma_free_coherent(dma_dev, entry->len, entry->va, entry->dma);
 		list_del(&entry->node);
 		kfree(entry);
 	}
@@ -1329,6 +1336,156 @@ struct rproc *rproc_get_by_phandle(phandle phandle)
 #endif
 EXPORT_SYMBOL(rproc_get_by_phandle);
 
+/*
+ * resource structure of rproc_subdev is used for identify the right subdevice
+ * that has the dma coherent memory.
+ */
+static int rproc_subdev_match(struct device *dev, void *data)
+{
+	char *sub_name;
+
+	if (!dev_name(dev))
+		return 0;
+
+	sub_name = strpbrk(dev_name(dev), "#");
+	if (!sub_name)
+		return 0;
+
+	return !strcmp(++sub_name, (char *)data);
+}
+
+/*
+ * find the subdevice child dma coherent memory that match with name region
+ * the rproc parent is the default device, if there is no match
+ */
+struct device *rproc_subdev_lookup(struct rproc *rproc, const char *name)
+{
+	struct device *dev;
+
+	dev = device_find_child(rproc->dev.parent, (void *)name,
+				rproc_subdev_match);
+	if (dev) {
+		/* decrement the matched device's refcount back */
+		put_device(dev);
+		return dev;
+	}
+
+	return rproc->dev.parent;
+}
+EXPORT_SYMBOL(rproc_subdev_lookup);
+
+/**
+ * rproc_subdev_release() - release the existence of a subdevice
+ *
+ * @dev: the subdevice's dev
+ */
+static void rproc_subdev_release(struct device *dev)
+{
+	struct rproc_subdev *sub = to_subdevice(dev);
+
+	kfree(sub);
+}
+
+/**
+ * rproc_subdev_unregister() - unregister sub-device of remote processor
+ *
+ * @dev: rproc sub-device
+ * @data: Not use (just to be compliant with device_for_each_child)
+ *
+ * This function is called by device_for_each_child function when unregister
+ * remote processor.
+ */
+static int rproc_subdev_unregister(struct device *dev, void *data)
+{
+	struct rproc_subdev *sub = to_subdevice(dev);
+	struct rproc *rproc = data;
+
+	if (dev != &(rproc->dev))
+		rproc_subdev_del(sub);
+	return 0;
+}
+
+/**
+ * rproc_subdev_add() - add a sub-device on remote processor
+ *
+ * @rproc: the parent remote processor
+ * @res: resource allow to define the dma coherent memory of sub-device
+ *
+ * This function add a sub-device child on rproc parent. This sub-device allow
+ * to define a new dma coherent memory area. when the rproc would alloc a
+ * dma coherent memory it's find the subdevice that match with physical memory
+ * asked (if there is no children that match, the rproc is the default device)
+ *
+ * Returns the sub-device handle on success, and error on failure.
+ */
+struct rproc_subdev *rproc_subdev_add(struct rproc *rproc, struct resource *res)
+{
+	struct rproc_subdev *sub;
+	int ret;
+
+	if (!res || res->flags != IORESOURCE_MEM || res->name == NULL) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	sub = kzalloc(sizeof(*sub), GFP_KERNEL);
+	if (!sub) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	sub->rproc = rproc;
+	sub->res = res;
+	sub->dev.parent = rproc->dev.parent;
+	sub->dev.release = rproc_subdev_release;
+	dev_set_name(&sub->dev, "%s#%s", dev_name(sub->dev.parent), res->name);
+	dev_set_drvdata(&sub->dev, sub);
+
+	ret = device_register(&sub->dev);
+	if (ret)
+		goto err_dev;
+
+	if (!devm_request_mem_region(&sub->dev, res->start,
+				     resource_size(res),
+				     dev_name(&sub->dev))) {
+		dev_err(&rproc->dev, "failed to get memory region\n");
+		ret = -EINVAL;
+		goto err_dev;
+	}
+
+	ret = dmam_declare_coherent_memory(&sub->dev,
+					   res->start, res->start,
+					   resource_size(res),
+					   DMA_MEMORY_MAP |
+					   DMA_MEMORY_EXCLUSIVE);
+	if (ret < 0)
+		goto err_dev;
+
+	return sub;
+
+err_dev:
+	put_device(&sub->dev);
+err:
+	dev_err(&rproc->dev, "unable to register subdev %s, err = %d\n",
+		(res && res->name) ? res->name : "unnamed", ret);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL(rproc_subdev_add);
+
+/**
+ * rproc_subdev_del() - delete a sub-device of remote processor
+ *
+ * @subdev: rproc sub-device
+ */
+void rproc_subdev_del(struct rproc_subdev *subdev)
+{
+	if (get_device(&subdev->dev)) {
+		device_unregister(&subdev->dev);
+		put_device(&subdev->dev);
+	}
+}
+EXPORT_SYMBOL(rproc_subdev_del);
+
 /**
  * rproc_set_fw_name() - change rproc fw name
  * @rproc: rproc handle
@@ -1618,6 +1775,9 @@ int rproc_del(struct rproc *rproc)
 	kfree(rproc->cached_table);
 	rproc->cached_table = NULL;
 
+	device_for_each_child(rproc->dev.parent, rproc,
+			      rproc_subdev_unregister);
+
 	/* the rproc is downref'ed as soon as it's removed from the klist */
 	mutex_lock(&rproc_list_mutex);
 	list_del(&rproc->node);
-- 
2.8.0

[toc] | [prev] | [next] | [standalone]

#1399556 — Re: [PATCH 4/5] remoteproc: core: Supply framework to request, declare and fetch shared memory

On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:

> Normally used for management of; carveout, devmem and trace memory.
> 

I like the gist of this, but I have two issues from the Qualcomm land
that I think would tie into a slightly more generic version of this.

1) Registering carveouts from memory-regions, without having a resource
   table.
2) Tying the Qualcomm shared memory implementation to the appropriate
   remoteproc instance (e.g. for crash handling).

I also think we should match the subdev with entries from the resource
table based on some key, so that we don't add a limitation of only
having a single carveout per rproc.


I will do some prototyping based on your patch and will get back to you.

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1395130 — [PATCH 1/5] remoteproc: core: Task sync during rproc_fw_boot()

By default, rproc_fw_boot() needs to wait for rproc to be configured,
but a race may occur when using rpmsg/virtio.  In this case, it can
be called locally in a safe manor.

This patch represents two usecases:

 - External call (via exported rproc_boot()), which waits
 - Internal call can use 'nowait' version of rproc_boot()

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c     | 29 +++++++++++++++++++++++++++--
 drivers/remoteproc/remoteproc_internal.h |  1 +
 drivers/remoteproc/remoteproc_virtio.c   |  2 +-
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ee44df5..7db2818 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1033,8 +1033,9 @@ static void rproc_crash_handler_work(struct work_struct *work)
 }
 
 /**
- * rproc_boot() - boot a remote processor
+ * __rproc_boot() - boot a remote processor
  * @rproc: handle of a remote processor
+ * @wait: wait for rproc registration completion
  *
  * Boot a remote processor (i.e. load its firmware, power it on, ...).
  *
@@ -1043,7 +1044,7 @@ static void rproc_crash_handler_work(struct work_struct *work)
  *
  * Returns 0 on success, and an appropriate error value otherwise.
  */
-int rproc_boot(struct rproc *rproc)
+static int __rproc_boot(struct rproc *rproc, bool wait)
 {
 	const struct firmware *firmware_p;
 	struct device *dev;
@@ -1091,6 +1092,10 @@ int rproc_boot(struct rproc *rproc)
 		goto downref_rproc;
 	}
 
+	/* if rproc virtio is not yet configured, wait */
+	if (wait)
+		wait_for_completion(&rproc->firmware_loading_complete);
+
 	ret = rproc_fw_boot(rproc, firmware_p);
 
 	release_firmware(firmware_p);
@@ -1104,9 +1109,29 @@ unlock_mutex:
 	mutex_unlock(&rproc->lock);
 	return ret;
 }
+
+/**
+ * rproc_boot() - boot a remote processor
+ * @rproc: handle of a remote processor
+ */
+int rproc_boot(struct rproc *rproc)
+{
+	return __rproc_boot(rproc, true);
+}
 EXPORT_SYMBOL(rproc_boot);
 
 /**
+ * rproc_boot_nowait() - boot a remote processor
+ * @rproc: handle of a remote processor
+ *
+ * Same as rproc_boot() but don't wait for rproc registration completion
+ */
+int rproc_boot_nowait(struct rproc *rproc)
+{
+	return __rproc_boot(rproc, false);
+}
+
+/**
  * rproc_shutdown() - power off the remote processor
  * @rproc: the remote processor
  *
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 8041b95..57e1de5 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -48,6 +48,7 @@ struct rproc_fw_ops {
 /* from remoteproc_core.c */
 void rproc_release(struct kref *kref);
 irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
+int rproc_boot_nowait(struct rproc *rproc);
 
 /* from remoteproc_virtio.c */
 int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id);
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index ff30684..106d9e4 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -175,7 +175,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);
+	ret = rproc_boot_nowait(rproc);
 	if (ret) {
 		dev_err(&rproc->dev, "rproc_boot() failed %d\n", ret);
 		goto error;
-- 
2.8.0

[toc] | [prev] | [next] | [standalone]

#1396068 — Re: [PATCH 1/5] remoteproc: core: Task sync during rproc_fw_boot()

On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:

> By default, rproc_fw_boot() needs to wait for rproc to be configured,
> but a race may occur when using rpmsg/virtio.  In this case, it can
> be called locally in a safe manor.
> 
> This patch represents two usecases:
> 
>  - External call (via exported rproc_boot()), which waits
>  - Internal call can use 'nowait' version of rproc_boot()
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>

Looks good

Applied to rproc-next.

Regards,
Bjorn

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web