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

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

[Lee Jones]

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

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

[Lee Jones]

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

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

[Lee Jones]

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

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

[Lee Jones]

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

[PATCH 2/5] remoteproc: core: Add rproc OF look-up functions

[Lee Jones]

- of_rproc_byindex(): look-up and obtain a reference to a rproc
  		      using the DT phandle "rprocs" and a index.

- of_rproc_byname():  lookup and obtain a reference to a rproc
  		      using the DT phandle "rprocs" and "rproc-names".

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 7db2818..85e5fd8 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -41,12 +41,19 @@
 #include <linux/virtio_ids.h>
 #include <linux/virtio_ring.h>
 #include <asm/byteorder.h>
+#include <linux/klist.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
 
 #include "remoteproc_internal.h"
 
 static DEFINE_MUTEX(rproc_list_mutex);
 static LIST_HEAD(rproc_list);
 
+static void klist_rproc_get(struct klist_node *n);
+static void klist_rproc_put(struct klist_node *n);
+static DEFINE_KLIST(rprocs, klist_rproc_get, klist_rproc_put);
+
 typedef int (*rproc_handle_resources_t)(struct rproc *rproc,
 				struct resource_table *table, int len);
 typedef int (*rproc_handle_resource_t)(struct rproc *rproc,
@@ -1196,6 +1203,87 @@ out:
 }
 EXPORT_SYMBOL(rproc_shutdown);
 
+/* 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, klist);
+
+	get_device(&rproc->dev);
+}
+
+/* will be called when an rproc is removed from the rprocs klist */
+static void klist_rproc_put(struct klist_node *n)
+{
+	struct rproc *rproc = container_of(n, struct rproc, klist);
+
+	put_device(&rproc->dev);
+}
+
+static struct rproc *next_rproc(struct klist_iter *i)
+{
+	struct klist_node *n;
+
+	n = klist_next(i);
+	if (!n)
+		return NULL;
+
+	return container_of(n, struct rproc, klist);
+}
+
+/**
+ * of_rproc_by_index() - lookup and obtain a reference to an rproc
+ * @np: node to search for rproc
+ * @index: index into the phandle list
+ *
+ * Returns the rproc driver on success and an appropriate error code otherwise.
+ */
+struct rproc *of_rproc_byindex(struct device_node *np, int index)
+{
+	struct rproc *rproc;
+	struct device_node *rproc_node;
+	struct platform_device *pdev;
+	struct klist_iter i;
+
+	if (index < 0)
+		return ERR_PTR(-EINVAL);
+
+	rproc_node = of_parse_phandle(np, "rprocs", index);
+	if (!rproc_node)
+		return ERR_PTR(-ENODEV);
+
+	pdev = of_find_device_by_node(rproc_node);
+	if (!pdev)
+		return ERR_PTR(-ENODEV);
+
+	klist_iter_init(&rprocs, &i);
+	while ((rproc = next_rproc(&i)) != NULL)
+		if (rproc->dev.parent == &pdev->dev)
+			break;
+	klist_iter_exit(&i);
+
+	return rproc;
+}
+EXPORT_SYMBOL(of_rproc_byindex);
+
+/**
+ * of_rproc_byname() - lookup and obtain a reference to an rproc
+ * @np: node to search for rproc
+ * @name: name of the remoteproc from device's point of view
+ *
+ * Returns the rproc driver on success and an appropriate error code otherwise.
+ */
+struct rproc *of_rproc_byname(struct device_node *np, const char *name)
+{
+	int index;
+
+	if (unlikely(!name))
+		return ERR_PTR(-EINVAL);
+
+	index = of_property_match_string(np, "rproc-names", name);
+	return of_rproc_byindex(np, index);
+}
+EXPORT_SYMBOL(of_rproc_byname);
+
 /**
  * rproc_get_by_phandle() - find a remote processor by phandle
  * @phandle: phandle to the rproc
@@ -1282,7 +1370,13 @@ int rproc_add(struct rproc *rproc)
 	/* create debugfs entries */
 	rproc_create_debug_dir(rproc);
 
-	return rproc_add_virtio_devices(rproc);
+	ret = rproc_add_virtio_devices(rproc);
+	if (ret < 0)
+		klist_remove(&rproc->klist);
+	else
+		klist_add_tail(&rproc->klist, &rprocs);
+
+	return ret;
 }
 EXPORT_SYMBOL(rproc_add);
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 9c4e138..4c96e78 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -410,6 +410,7 @@ enum rproc_crash_type {
  */
 struct rproc {
 	struct list_head node;
+	struct klist_node klist;
 	struct iommu_domain *domain;
 	const char *name;
 	const char *firmware;
@@ -494,6 +495,8 @@ int rproc_del(struct rproc *rproc);
 int rproc_boot(struct rproc *rproc);
 void rproc_shutdown(struct rproc *rproc);
 void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
+struct rproc *of_rproc_byindex(struct device_node *np, int index);
+struct rproc *of_rproc_byname(struct device_node *np, const char *name);
 
 static inline struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
 {
-- 
2.8.0

[PATCH 2/5] remoteproc: core: Add rproc OF look-up functions

[Lee Jones]

- of_rproc_byindex(): look-up and obtain a reference to a rproc
  		      using the DT phandle "rprocs" and a index.

- of_rproc_byname():  lookup and obtain a reference to a rproc
  		      using the DT phandle "rprocs" and "rproc-names".

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 7db2818..85e5fd8 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -41,12 +41,19 @@
 #include <linux/virtio_ids.h>
 #include <linux/virtio_ring.h>
 #include <asm/byteorder.h>
+#include <linux/klist.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
 
 #include "remoteproc_internal.h"
 
 static DEFINE_MUTEX(rproc_list_mutex);
 static LIST_HEAD(rproc_list);
 
+static void klist_rproc_get(struct klist_node *n);
+static void klist_rproc_put(struct klist_node *n);
+static DEFINE_KLIST(rprocs, klist_rproc_get, klist_rproc_put);
+
 typedef int (*rproc_handle_resources_t)(struct rproc *rproc,
 				struct resource_table *table, int len);
 typedef int (*rproc_handle_resource_t)(struct rproc *rproc,
@@ -1196,6 +1203,87 @@ out:
 }
 EXPORT_SYMBOL(rproc_shutdown);
 
+/* 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, klist);
+
+	get_device(&rproc->dev);
+}
+
+/* will be called when an rproc is removed from the rprocs klist */
+static void klist_rproc_put(struct klist_node *n)
+{
+	struct rproc *rproc = container_of(n, struct rproc, klist);
+
+	put_device(&rproc->dev);
+}
+
+static struct rproc *next_rproc(struct klist_iter *i)
+{
+	struct klist_node *n;
+
+	n = klist_next(i);
+	if (!n)
+		return NULL;
+
+	return container_of(n, struct rproc, klist);
+}
+
+/**
+ * of_rproc_by_index() - lookup and obtain a reference to an rproc
+ * @np: node to search for rproc
+ * @index: index into the phandle list
+ *
+ * Returns the rproc driver on success and an appropriate error code otherwise.
+ */
+struct rproc *of_rproc_byindex(struct device_node *np, int index)
+{
+	struct rproc *rproc;
+	struct device_node *rproc_node;
+	struct platform_device *pdev;
+	struct klist_iter i;
+
+	if (index < 0)
+		return ERR_PTR(-EINVAL);
+
+	rproc_node = of_parse_phandle(np, "rprocs", index);
+	if (!rproc_node)
+		return ERR_PTR(-ENODEV);
+
+	pdev = of_find_device_by_node(rproc_node);
+	if (!pdev)
+		return ERR_PTR(-ENODEV);
+
+	klist_iter_init(&rprocs, &i);
+	while ((rproc = next_rproc(&i)) != NULL)
+		if (rproc->dev.parent == &pdev->dev)
+			break;
+	klist_iter_exit(&i);
+
+	return rproc;
+}
+EXPORT_SYMBOL(of_rproc_byindex);
+
+/**
+ * of_rproc_byname() - lookup and obtain a reference to an rproc
+ * @np: node to search for rproc
+ * @name: name of the remoteproc from device's point of view
+ *
+ * Returns the rproc driver on success and an appropriate error code otherwise.
+ */
+struct rproc *of_rproc_byname(struct device_node *np, const char *name)
+{
+	int index;
+
+	if (unlikely(!name))
+		return ERR_PTR(-EINVAL);
+
+	index = of_property_match_string(np, "rproc-names", name);
+	return of_rproc_byindex(np, index);
+}
+EXPORT_SYMBOL(of_rproc_byname);
+
 /**
  * rproc_get_by_phandle() - find a remote processor by phandle
  * @phandle: phandle to the rproc
@@ -1282,7 +1370,13 @@ int rproc_add(struct rproc *rproc)
 	/* create debugfs entries */
 	rproc_create_debug_dir(rproc);
 
-	return rproc_add_virtio_devices(rproc);
+	ret = rproc_add_virtio_devices(rproc);
+	if (ret < 0)
+		klist_remove(&rproc->klist);
+	else
+		klist_add_tail(&rproc->klist, &rprocs);
+
+	return ret;
 }
 EXPORT_SYMBOL(rproc_add);
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 9c4e138..4c96e78 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -410,6 +410,7 @@ enum rproc_crash_type {
  */
 struct rproc {
 	struct list_head node;
+	struct klist_node klist;
 	struct iommu_domain *domain;
 	const char *name;
 	const char *firmware;
@@ -494,6 +495,8 @@ int rproc_del(struct rproc *rproc);
 int rproc_boot(struct rproc *rproc);
 void rproc_shutdown(struct rproc *rproc);
 void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
+struct rproc *of_rproc_byindex(struct device_node *np, int index);
+struct rproc *of_rproc_byname(struct device_node *np, const char *name);
 
 static inline struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
 {
-- 
2.8.0

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

[Lee Jones]

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

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

[Lee Jones]

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

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

[Lee Jones]

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

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

[Lee Jones]

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

[PATCH 5/5] remoteproc: core: Clip carveout if it's too big

[Lee Jones]

Carveout size is primarily extracted from the firmware binary.  However,
DT can over-ride this by providing a different (smaller) size.  We're
adding protection here to ensure the we only allocate the smaller of the
two provided sizes in order to decrease the risk of clashes.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3d9798c..c3cad708 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -577,6 +577,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	struct rproc_mem_entry *carveout, *mapping;
 	struct device *dev = &rproc->dev;
 	struct device *dma_dev;
+	struct rproc_subdev *sub;
 	dma_addr_t dma;
 	void *va;
 	int ret;
@@ -600,6 +601,14 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		return -ENOMEM;
 
 	dma_dev = rproc_subdev_lookup(rproc, "carveout");
+	sub = dev_get_drvdata(dma_dev);
+
+	if (rsc->len > resource_size(sub->res)) {
+		dev_warn(dev, "carveout too big (0x%x): clipping to 0x%x\n",
+			 rsc->len, resource_size(sub->res));
+		rsc->len = resource_size(sub->res);
+	}
+
 	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);
-- 
2.8.0

[PATCH 5/5] remoteproc: core: Clip carveout if it's too big

[Lee Jones]

Carveout size is primarily extracted from the firmware binary.  However,
DT can over-ride this by providing a different (smaller) size.  We're
adding protection here to ensure the we only allocate the smaller of the
two provided sizes in order to decrease the risk of clashes.

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

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3d9798c..c3cad708 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -577,6 +577,7 @@ static int rproc_handle_carveout(struct rproc *rproc,
 	struct rproc_mem_entry *carveout, *mapping;
 	struct device *dev = &rproc->dev;
 	struct device *dma_dev;
+	struct rproc_subdev *sub;
 	dma_addr_t dma;
 	void *va;
 	int ret;
@@ -600,6 +601,14 @@ static int rproc_handle_carveout(struct rproc *rproc,
 		return -ENOMEM;
 
 	dma_dev = rproc_subdev_lookup(rproc, "carveout");
+	sub = dev_get_drvdata(dma_dev);
+
+	if (rsc->len > resource_size(sub->res)) {
+		dev_warn(dev, "carveout too big (0x%x): clipping to 0x%x\n",
+			 rsc->len, resource_size(sub->res));
+		rsc->len = resource_size(sub->res);
+	}
+
 	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);
-- 
2.8.0

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

[Bjorn Andersson]

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

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

[Bjorn Andersson]

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

Re: [PATCH 2/5] remoteproc: core: Add rproc OF look-up functions

[Bjorn Andersson]

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

> - of_rproc_byindex(): look-up and obtain a reference to a rproc
>   		      using the DT phandle "rprocs" and a index.
> 
> - of_rproc_byname():  lookup and obtain a reference to a rproc
>   		      using the DT phandle "rprocs" and "rproc-names".
> 
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---

I like the idea of having these helpers, but we already have
rproc_get_by_phandle() so these helpers should be built upon that rather
than adding the additional list.

Regards,
Bjorn

[PATCH 2/5] remoteproc: core: Add rproc OF look-up functions

[Bjorn Andersson]

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

> - of_rproc_byindex(): look-up and obtain a reference to a rproc
>   		      using the DT phandle "rprocs" and a index.
> 
> - of_rproc_byname():  lookup and obtain a reference to a rproc
>   		      using the DT phandle "rprocs" and "rproc-names".
> 
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---

I like the idea of having these helpers, but we already have
rproc_get_by_phandle() so these helpers should be built upon that rather
than adding the additional list.

Regards,
Bjorn

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

[Bjorn Andersson]

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

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

[Bjorn Andersson]

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

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

[Lee Jones]

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

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

[Lee Jones]

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

Re: [PATCH 2/5] remoteproc: core: Add rproc OF look-up functions

[Lee Jones]

On Fri, 06 May 2016, Bjorn Andersson wrote:

> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
> 
> > - of_rproc_byindex(): look-up and obtain a reference to a rproc
> >   		      using the DT phandle "rprocs" and a index.
> > 
> > - of_rproc_byname():  lookup and obtain a reference to a rproc
> >   		      using the DT phandle "rprocs" and "rproc-names".
> > 
> > Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> 
> I like the idea of having these helpers, but we already have
> rproc_get_by_phandle() so these helpers should be built upon that rather
> than adding the additional list.

Since this is a framework consideration, don't you think it would be
better to standardise the phandle name?  This is common practice when
coding at a subsystem level.  Some in use examples include; "clocks",
"power-domains", "mboxes", "dmas", "phys", "resets", "gpios", etc.

Considering the aforementioned examples, I figured "rprocs" would be
suitable.

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

[PATCH 2/5] remoteproc: core: Add rproc OF look-up functions

[Lee Jones]

On Fri, 06 May 2016, Bjorn Andersson wrote:

> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
> 
> > - of_rproc_byindex(): look-up and obtain a reference to a rproc
> >   		      using the DT phandle "rprocs" and a index.
> > 
> > - of_rproc_byname():  lookup and obtain a reference to a rproc
> >   		      using the DT phandle "rprocs" and "rproc-names".
> > 
> > Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> 
> I like the idea of having these helpers, but we already have
> rproc_get_by_phandle() so these helpers should be built upon that rather
> than adding the additional list.

Since this is a framework consideration, don't you think it would be
better to standardise the phandle name?  This is common practice when
coding at a subsystem level.  Some in use examples include; "clocks",
"power-domains", "mboxes", "dmas", "phys", "resets", "gpios", etc.

Considering the aforementioned examples, I figured "rprocs" would be
suitable.

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

Re: [PATCH 2/5] remoteproc: core: Add rproc OF look-up functions

[Bjorn Andersson]

On Tue 10 May 07:16 PDT 2016, Lee Jones wrote:

> On Fri, 06 May 2016, Bjorn Andersson wrote:
> 
> > On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
> > 
> > > - of_rproc_byindex(): look-up and obtain a reference to a rproc
> > >   		      using the DT phandle "rprocs" and a index.
> > > 
> > > - of_rproc_byname():  lookup and obtain a reference to a rproc
> > >   		      using the DT phandle "rprocs" and "rproc-names".
> > > 
> > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > 
> > I like the idea of having these helpers, but we already have
> > rproc_get_by_phandle() so these helpers should be built upon that rather
> > than adding the additional list.
> 
> Since this is a framework consideration, don't you think it would be
> better to standardise the phandle name?  This is common practice when
> coding at a subsystem level.  Some in use examples include; "clocks",
> "power-domains", "mboxes", "dmas", "phys", "resets", "gpios", etc.
> 
> Considering the aforementioned examples, I figured "rprocs" would be
> suitable.
> 

To summarize our chat for the record and others.


I'm definitely in favour of this and think "rprocs" and "rproc-names"
sounds good.  My comment was only regarding the duplicated
implementation.

Regards,
Bjorn

[PATCH 2/5] remoteproc: core: Add rproc OF look-up functions

[Bjorn Andersson]

On Tue 10 May 07:16 PDT 2016, Lee Jones wrote:

> On Fri, 06 May 2016, Bjorn Andersson wrote:
> 
> > On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
> > 
> > > - of_rproc_byindex(): look-up and obtain a reference to a rproc
> > >   		      using the DT phandle "rprocs" and a index.
> > > 
> > > - of_rproc_byname():  lookup and obtain a reference to a rproc
> > >   		      using the DT phandle "rprocs" and "rproc-names".
> > > 
> > > Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > 
> > I like the idea of having these helpers, but we already have
> > rproc_get_by_phandle() so these helpers should be built upon that rather
> > than adding the additional list.
> 
> Since this is a framework consideration, don't you think it would be
> better to standardise the phandle name?  This is common practice when
> coding at a subsystem level.  Some in use examples include; "clocks",
> "power-domains", "mboxes", "dmas", "phys", "resets", "gpios", etc.
> 
> Considering the aforementioned examples, I figured "rprocs" would be
> suitable.
> 

To summarize our chat for the record and others.


I'm definitely in favour of this and think "rprocs" and "rproc-names"
sounds good.  My comment was only regarding the duplicated
implementation.

Regards,
Bjorn

Re: [PATCH 5/5] remoteproc: core: Clip carveout if it's too big

[Bjorn Andersson]

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

> Carveout size is primarily extracted from the firmware binary.  However,
> DT can over-ride this by providing a different (smaller) size.  We're
> adding protection here to ensure the we only allocate the smaller of the
> two provided sizes in order to decrease the risk of clashes.
> 

Is this really the right thing to do?

The firmware is bundled with a resource table stating a certain size of
this the carveout and this would "silently" give it less space. On some
systems an IOMMU will save us (killing the firmware) but on others I
fear the firmware might just access memory outside its expected buffer.

> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
[..]
> @@ -600,6 +601,14 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  		return -ENOMEM;
>  
>  	dma_dev = rproc_subdev_lookup(rproc, "carveout");
> +	sub = dev_get_drvdata(dma_dev);
> +
> +	if (rsc->len > resource_size(sub->res)) {
> +		dev_warn(dev, "carveout too big (0x%x): clipping to 0x%x\n",
> +			 rsc->len, resource_size(sub->res));
> +		rsc->len = resource_size(sub->res);
> +	}

I would rather expect this to say:

if (resource_size(sub->res) < rsc->len) {
	dev_err(dev, "defined carveout to small for firmware\n");
	return -EINVAL;
}

Unless we trust the remote firmware to dynamically follow what we put in
the resource table. (And how does it tell us if that limit isn't
enough?)

> +
>  	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);


Apart from this concern I'm still need to review the subdev patch; here
related the part that there's only room for one carveout with only one
size.

Regards,
Bjorn

[PATCH 5/5] remoteproc: core: Clip carveout if it's too big

[Bjorn Andersson]

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

> Carveout size is primarily extracted from the firmware binary.  However,
> DT can over-ride this by providing a different (smaller) size.  We're
> adding protection here to ensure the we only allocate the smaller of the
> two provided sizes in order to decrease the risk of clashes.
> 

Is this really the right thing to do?

The firmware is bundled with a resource table stating a certain size of
this the carveout and this would "silently" give it less space. On some
systems an IOMMU will save us (killing the firmware) but on others I
fear the firmware might just access memory outside its expected buffer.

> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
[..]
> @@ -600,6 +601,14 @@ static int rproc_handle_carveout(struct rproc *rproc,
>  		return -ENOMEM;
>  
>  	dma_dev = rproc_subdev_lookup(rproc, "carveout");
> +	sub = dev_get_drvdata(dma_dev);
> +
> +	if (rsc->len > resource_size(sub->res)) {
> +		dev_warn(dev, "carveout too big (0x%x): clipping to 0x%x\n",
> +			 rsc->len, resource_size(sub->res));
> +		rsc->len = resource_size(sub->res);
> +	}

I would rather expect this to say:

if (resource_size(sub->res) < rsc->len) {
	dev_err(dev, "defined carveout to small for firmware\n");
	return -EINVAL;
}

Unless we trust the remote firmware to dynamically follow what we put in
the resource table. (And how does it tell us if that limit isn't
enough?)

> +
>  	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);


Apart from this concern I'm still need to review the subdev patch; here
related the part that there's only room for one carveout with only one
size.

Regards,
Bjorn

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

[Bjorn Andersson]

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

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

[Bjorn Andersson]

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

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

[Bjorn Andersson]

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

> 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
[..]
> @@ -222,7 +223,8 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
[..]
> -	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);
[..]
> @@ -594,7 +599,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
[..]
> -	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);
[..]
> +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);
> +}
> +
[..]
> +struct rproc_subdev *rproc_subdev_add(struct rproc *rproc, struct resource *res)
> +{
[..]
> +	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);
[..]
> +}
> +EXPORT_SYMBOL(rproc_subdev_add);

I worked up a prototype that allows remoteproc drivers to
programmatically specify carveout resources, which then are matched and
merged with entires from the resource table. I've given these
programmatically allocated carveouts a device so that the associated
allocation comes out of the specified region - or the rproc region if no
match is found.


This solves my use case of carving out memory from two disjoint memory
regions for one of my remoteprocs, but differs from your approach in
that you specify one large carveout (and vrings) region and any
carveouts (or vrings) will chip away from this.

Would this approach work for you, or do you depend on having 1:N
relationship between your memory region and your resources?

Regards,
Bjorn

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

[Bjorn Andersson]

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

> 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
[..]
> @@ -222,7 +223,8 @@ int rproc_alloc_vring(struct rproc_vdev *rvdev, int i)
[..]
> -	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);
[..]
> @@ -594,7 +599,8 @@ static int rproc_handle_carveout(struct rproc *rproc,
[..]
> -	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);
[..]
> +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);
> +}
> +
[..]
> +struct rproc_subdev *rproc_subdev_add(struct rproc *rproc, struct resource *res)
> +{
[..]
> +	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);
[..]
> +}
> +EXPORT_SYMBOL(rproc_subdev_add);

I worked up a prototype that allows remoteproc drivers to
programmatically specify carveout resources, which then are matched and
merged with entires from the resource table. I've given these
programmatically allocated carveouts a device so that the associated
allocation comes out of the specified region - or the rproc region if no
match is found.


This solves my use case of carving out memory from two disjoint memory
regions for one of my remoteprocs, but differs from your approach in
that you specify one large carveout (and vrings) region and any
carveouts (or vrings) will chip away from this.

Would this approach work for you, or do you depend on having 1:N
relationship between your memory region and your resources?

Regards,
Bjorn

Re: [STLinux Kernel] [PATCH 5/5] remoteproc: core: Clip carveout if it's too big

[loic pallardy]

Hi,

On 05/10/2016 09:21 PM, Bjorn Andersson wrote:
> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
>
>> Carveout size is primarily extracted from the firmware binary.  However,
>> DT can over-ride this by providing a different (smaller) size.  We're
>> adding protection here to ensure the we only allocate the smaller of the
>> two provided sizes in order to decrease the risk of clashes.
>>
>
> Is this really the right thing to do?
>
> The firmware is bundled with a resource table stating a certain size of
> this the carveout and this would "silently" give it less space. On some
> systems an IOMMU will save us (killing the firmware) but on others I
> fear the firmware might just access memory outside its expected buffer.

Agree with Bjorn, not it is not possible to silently clip carveout 
memory.Firmware resource table should contain exact coprocessor needs. 
If resources are not available, firmware loading must failed with 
explicit message.

Regards,
Loic

>
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> ---
>>   drivers/remoteproc/remoteproc_core.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> [..]
>> @@ -600,6 +601,14 @@ static int rproc_handle_carveout(struct rproc *rproc,
>>   		return -ENOMEM;
>>
>>   	dma_dev = rproc_subdev_lookup(rproc, "carveout");
>> +	sub = dev_get_drvdata(dma_dev);
>> +
>> +	if (rsc->len > resource_size(sub->res)) {
>> +		dev_warn(dev, "carveout too big (0x%x): clipping to 0x%x\n",
>> +			 rsc->len, resource_size(sub->res));
>> +		rsc->len = resource_size(sub->res);
>> +	}
>
> I would rather expect this to say:
>
> if (resource_size(sub->res) < rsc->len) {
> 	dev_err(dev, "defined carveout to small for firmware\n");
> 	return -EINVAL;
> }
>
> Unless we trust the remote firmware to dynamically follow what we put in
> the resource table. (And how does it tell us if that limit isn't
> enough?)
>
>> +
>>   	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);
>
>
> Apart from this concern I'm still need to review the subdev patch; here
> related the part that there's only room for one carveout with only one
> size.
>
> Regards,
> Bjorn
>
> _______________________________________________
> Kernel mailing list
> Kernel@stlinux.com
> http://www.stlinux.com/mailman/listinfo/kernel
>

Re: [STLinux Kernel] [PATCH 5/5] remoteproc: core: Clip carveout if it's too big

[loic pallardy]

Hi,

On 05/10/2016 09:21 PM, Bjorn Andersson wrote:
> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
>
>> Carveout size is primarily extracted from the firmware binary.  However,
>> DT can over-ride this by providing a different (smaller) size.  We're
>> adding protection here to ensure the we only allocate the smaller of the
>> two provided sizes in order to decrease the risk of clashes.
>>
>
> Is this really the right thing to do?
>
> The firmware is bundled with a resource table stating a certain size of
> this the carveout and this would "silently" give it less space. On some
> systems an IOMMU will save us (killing the firmware) but on others I
> fear the firmware might just access memory outside its expected buffer.

Agree with Bjorn, not it is not possible to silently clip carveout 
memory.Firmware resource table should contain exact coprocessor needs. 
If resources are not available, firmware loading must failed with 
explicit message.

Regards,
Loic

>
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> ---
>>   drivers/remoteproc/remoteproc_core.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> [..]
>> @@ -600,6 +601,14 @@ static int rproc_handle_carveout(struct rproc *rproc,
>>   		return -ENOMEM;
>>
>>   	dma_dev = rproc_subdev_lookup(rproc, "carveout");
>> +	sub = dev_get_drvdata(dma_dev);
>> +
>> +	if (rsc->len > resource_size(sub->res)) {
>> +		dev_warn(dev, "carveout too big (0x%x): clipping to 0x%x\n",
>> +			 rsc->len, resource_size(sub->res));
>> +		rsc->len = resource_size(sub->res);
>> +	}
>
> I would rather expect this to say:
>
> if (resource_size(sub->res) < rsc->len) {
> 	dev_err(dev, "defined carveout to small for firmware\n");
> 	return -EINVAL;
> }
>
> Unless we trust the remote firmware to dynamically follow what we put in
> the resource table. (And how does it tell us if that limit isn't
> enough?)
>
>> +
>>   	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);
>
>
> Apart from this concern I'm still need to review the subdev patch; here
> related the part that there's only room for one carveout with only one
> size.
>
> Regards,
> Bjorn
>
> _______________________________________________
> Kernel mailing list
> Kernel@stlinux.com
> http://www.stlinux.com/mailman/listinfo/kernel
>

[STLinux Kernel] [PATCH 5/5] remoteproc: core: Clip carveout if it's too big

[loic pallardy]

Hi,

On 05/10/2016 09:21 PM, Bjorn Andersson wrote:
> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
>
>> Carveout size is primarily extracted from the firmware binary.  However,
>> DT can over-ride this by providing a different (smaller) size.  We're
>> adding protection here to ensure the we only allocate the smaller of the
>> two provided sizes in order to decrease the risk of clashes.
>>
>
> Is this really the right thing to do?
>
> The firmware is bundled with a resource table stating a certain size of
> this the carveout and this would "silently" give it less space. On some
> systems an IOMMU will save us (killing the firmware) but on others I
> fear the firmware might just access memory outside its expected buffer.

Agree with Bjorn, not it is not possible to silently clip carveout 
memory.Firmware resource table should contain exact coprocessor needs. 
If resources are not available, firmware loading must failed with 
explicit message.

Regards,
Loic

>
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> ---
>>   drivers/remoteproc/remoteproc_core.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> [..]
>> @@ -600,6 +601,14 @@ static int rproc_handle_carveout(struct rproc *rproc,
>>   		return -ENOMEM;
>>
>>   	dma_dev = rproc_subdev_lookup(rproc, "carveout");
>> +	sub = dev_get_drvdata(dma_dev);
>> +
>> +	if (rsc->len > resource_size(sub->res)) {
>> +		dev_warn(dev, "carveout too big (0x%x): clipping to 0x%x\n",
>> +			 rsc->len, resource_size(sub->res));
>> +		rsc->len = resource_size(sub->res);
>> +	}
>
> I would rather expect this to say:
>
> if (resource_size(sub->res) < rsc->len) {
> 	dev_err(dev, "defined carveout to small for firmware\n");
> 	return -EINVAL;
> }
>
> Unless we trust the remote firmware to dynamically follow what we put in
> the resource table. (And how does it tell us if that limit isn't
> enough?)
>
>> +
>>   	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);
>
>
> Apart from this concern I'm still need to review the subdev patch; here
> related the part that there's only room for one carveout with only one
> size.
>
> Regards,
> Bjorn
>
> _______________________________________________
> Kernel mailing list
> Kernel at stlinux.com
> http://www.stlinux.com/mailman/listinfo/kernel
>

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

[loic pallardy]

On 06/16/2016 12:06 AM, Bjorn Andersson wrote:
> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
>
>> 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(-)
>>
[..]
>> +}
>> +EXPORT_SYMBOL(rproc_subdev_add);
>
Hi Bjorn,

> I worked up a prototype that allows remoteproc drivers to
> programmatically specify carveout resources, which then are matched and
> merged with entires from the resource table. I've given these
> programmatically allocated carveouts a device so that the associated
> allocation comes out of the specified region - or the rproc region if no
> match is found.
>
No sure to catch your exact use case, but let me try to explain what we 
have today.
There are different rproc in ST. Some request a large chunk of memory 
for code and data, some have several dedicated memories like IRAM and DRAM.
In that case carevout memories are defined as subdev (like vrings).
Association is done thanks to carevout name in firmware resource table 
(in rproc_handle_carveout).

Moreover, as our coprocessors have no iommu, we add some check on 
allocated buffer to verify it fit with resource table declaration (pa).
This allows to guarantee complete alignment between resources needed by 
firmware and ones allocated by Linux kernel.
>
> This solves my use case of carving out memory from two disjoint memory
> regions for one of my remoteprocs, but differs from your approach in
> that you specify one large carveout (and vrings) region and any
> carveouts (or vrings) will chip away from this.
>
> Would this approach work for you, or do you depend on having 1:N
> relationship between your memory region and your resources?
>

Is this approach covering your needs?

Regards,
Loic

> Regards,
> Bjorn
>
> _______________________________________________
> Kernel mailing list
> Kernel@stlinux.com
> http://www.stlinux.com/mailman/listinfo/kernel
>

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

[loic pallardy]

On 06/16/2016 12:06 AM, Bjorn Andersson wrote:
> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
>
>> 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(-)
>>
[..]
>> +}
>> +EXPORT_SYMBOL(rproc_subdev_add);
>
Hi Bjorn,

> I worked up a prototype that allows remoteproc drivers to
> programmatically specify carveout resources, which then are matched and
> merged with entires from the resource table. I've given these
> programmatically allocated carveouts a device so that the associated
> allocation comes out of the specified region - or the rproc region if no
> match is found.
>
No sure to catch your exact use case, but let me try to explain what we 
have today.
There are different rproc in ST. Some request a large chunk of memory 
for code and data, some have several dedicated memories like IRAM and DRAM.
In that case carevout memories are defined as subdev (like vrings).
Association is done thanks to carevout name in firmware resource table 
(in rproc_handle_carveout).

Moreover, as our coprocessors have no iommu, we add some check on 
allocated buffer to verify it fit with resource table declaration (pa).
This allows to guarantee complete alignment between resources needed by 
firmware and ones allocated by Linux kernel.
>
> This solves my use case of carving out memory from two disjoint memory
> regions for one of my remoteprocs, but differs from your approach in
> that you specify one large carveout (and vrings) region and any
> carveouts (or vrings) will chip away from this.
>
> Would this approach work for you, or do you depend on having 1:N
> relationship between your memory region and your resources?
>

Is this approach covering your needs?

Regards,
Loic

> Regards,
> Bjorn
>
> _______________________________________________
> Kernel mailing list
> Kernel@stlinux.com
> http://www.stlinux.com/mailman/listinfo/kernel
>

[STLinux Kernel] [PATCH 4/5] remoteproc: core: Supply framework to request, declare and fetch shared memory

[loic pallardy]

On 06/16/2016 12:06 AM, Bjorn Andersson wrote:
> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
>
>> 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(-)
>>
[..]
>> +}
>> +EXPORT_SYMBOL(rproc_subdev_add);
>
Hi Bjorn,

> I worked up a prototype that allows remoteproc drivers to
> programmatically specify carveout resources, which then are matched and
> merged with entires from the resource table. I've given these
> programmatically allocated carveouts a device so that the associated
> allocation comes out of the specified region - or the rproc region if no
> match is found.
>
No sure to catch your exact use case, but let me try to explain what we 
have today.
There are different rproc in ST. Some request a large chunk of memory 
for code and data, some have several dedicated memories like IRAM and DRAM.
In that case carevout memories are defined as subdev (like vrings).
Association is done thanks to carevout name in firmware resource table 
(in rproc_handle_carveout).

Moreover, as our coprocessors have no iommu, we add some check on 
allocated buffer to verify it fit with resource table declaration (pa).
This allows to guarantee complete alignment between resources needed by 
firmware and ones allocated by Linux kernel.
>
> This solves my use case of carving out memory from two disjoint memory
> regions for one of my remoteprocs, but differs from your approach in
> that you specify one large carveout (and vrings) region and any
> carveouts (or vrings) will chip away from this.
>
> Would this approach work for you, or do you depend on having 1:N
> relationship between your memory region and your resources?
>

Is this approach covering your needs?

Regards,
Loic

> Regards,
> Bjorn
>
> _______________________________________________
> Kernel mailing list
> Kernel at stlinux.com
> http://www.stlinux.com/mailman/listinfo/kernel
>

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

[Bjorn Andersson]

On Tue 21 Jun 00:33 PDT 2016, loic pallardy wrote:

> 
> 
> On 06/16/2016 12:06 AM, Bjorn Andersson wrote:
> >On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
> >
> >>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(-)
> >>
> [..]
> >>+}
> >>+EXPORT_SYMBOL(rproc_subdev_add);
> >
> Hi Bjorn,
> 

Hi Loic, thanks for your answer.

> >I worked up a prototype that allows remoteproc drivers to
> >programmatically specify carveout resources, which then are matched and
> >merged with entires from the resource table. I've given these
> >programmatically allocated carveouts a device so that the associated
> >allocation comes out of the specified region - or the rproc region if no
> >match is found.
> >
> No sure to catch your exact use case, but let me try to explain what we have
> today.
> There are different rproc in ST. Some request a large chunk of memory for
> code and data, some have several dedicated memories like IRAM and DRAM.
> In that case carevout memories are defined as subdev (like vrings).
> Association is done thanks to carevout name in firmware resource table (in
> rproc_handle_carveout).
> 

With the proposed solution you will only be able to have a single subdev
defining the "carveout" segment. I have a similar setup where I have two
disjoint memory regions that I would like to carve out memory from.

Further more, I don't have a resource table in my firmware, so I would
like to be able to register these carveout regions programmatically.

> Moreover, as our coprocessors have no iommu, we add some check on allocated
> buffer to verify it fit with resource table declaration (pa).
> This allows to guarantee complete alignment between resources needed by
> firmware and ones allocated by Linux kernel.

I have memory-regions defined in devicetree representing where my
carveouts should be, so the subdev proposal gives me a way to control
the physical placing of the carveout.


So, based on these "requirements" I had in working a patchset that
allows me to register carveout sections programatically and during the
load of the resource table I can merge carveouts (and the other
resources) to form the complete picture.


But this would imply a 1-to-1 relationship between programmatically
defined carveouts and carveouts from the resource table. Would you be
able to handle this? Or do you depend on being able to have your subdev
define the span and then a set of resources fill this up?

Regards,
Bjorn

[STLinux Kernel] [PATCH 4/5] remoteproc: core: Supply framework to request, declare and fetch shared memory

[Bjorn Andersson]

On Tue 21 Jun 00:33 PDT 2016, loic pallardy wrote:

> 
> 
> On 06/16/2016 12:06 AM, Bjorn Andersson wrote:
> >On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
> >
> >>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(-)
> >>
> [..]
> >>+}
> >>+EXPORT_SYMBOL(rproc_subdev_add);
> >
> Hi Bjorn,
> 

Hi Loic, thanks for your answer.

> >I worked up a prototype that allows remoteproc drivers to
> >programmatically specify carveout resources, which then are matched and
> >merged with entires from the resource table. I've given these
> >programmatically allocated carveouts a device so that the associated
> >allocation comes out of the specified region - or the rproc region if no
> >match is found.
> >
> No sure to catch your exact use case, but let me try to explain what we have
> today.
> There are different rproc in ST. Some request a large chunk of memory for
> code and data, some have several dedicated memories like IRAM and DRAM.
> In that case carevout memories are defined as subdev (like vrings).
> Association is done thanks to carevout name in firmware resource table (in
> rproc_handle_carveout).
> 

With the proposed solution you will only be able to have a single subdev
defining the "carveout" segment. I have a similar setup where I have two
disjoint memory regions that I would like to carve out memory from.

Further more, I don't have a resource table in my firmware, so I would
like to be able to register these carveout regions programmatically.

> Moreover, as our coprocessors have no iommu, we add some check on allocated
> buffer to verify it fit with resource table declaration (pa).
> This allows to guarantee complete alignment between resources needed by
> firmware and ones allocated by Linux kernel.

I have memory-regions defined in devicetree representing where my
carveouts should be, so the subdev proposal gives me a way to control
the physical placing of the carveout.


So, based on these "requirements" I had in working a patchset that
allows me to register carveout sections programatically and during the
load of the resource table I can merge carveouts (and the other
resources) to form the complete picture.


But this would imply a 1-to-1 relationship between programmatically
defined carveouts and carveouts from the resource table. Would you be
able to handle this? Or do you depend on being able to have your subdev
define the span and then a set of resources fill this up?

Regards,
Bjorn

Re: [PATCH 2/5] remoteproc: core: Add rproc OF look-up functions

[Bjorn Andersson]

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

Lee,

I ran into this topic again while looking into some unrelated things.

> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> +struct rproc *of_rproc_byindex(struct device_node *np, int index)
> +{
> +	struct rproc *rproc;
> +	struct device_node *rproc_node;
> +	struct platform_device *pdev;
> +	struct klist_iter i;
> +
> +	if (index < 0)
> +		return ERR_PTR(-EINVAL);
> +
> +	rproc_node = of_parse_phandle(np, "rprocs", index);
> +	if (!rproc_node)

As I stated before I would like for you to use the existing rproc_list,
as done in rproc_get_by_phandle() today.

But as you resend this patch, could you please make this check fallback
to also checking for "ti,rproc", then simply drop the existing
rproc_get_by_phandle() and change the call to
of_rproc_byindex(dev->of_node, 0) in wkup_m3_ipc.c?

That way we're backwards compatible with the TI wakeup M3, without
having to maintain the then non-standard generic rproc phandle resolver.

> +		return ERR_PTR(-ENODEV);
> +
> +	pdev = of_find_device_by_node(rproc_node);
> +	if (!pdev)
> +		return ERR_PTR(-ENODEV);
> +
> +	klist_iter_init(&rprocs, &i);
> +	while ((rproc = next_rproc(&i)) != NULL)
> +		if (rproc->dev.parent == &pdev->dev)
> +			break;
> +	klist_iter_exit(&i);
> +
> +	return rproc;
> +}
> +EXPORT_SYMBOL(of_rproc_byindex);

Regards,
Bjorn

[PATCH 2/5] remoteproc: core: Add rproc OF look-up functions

[Bjorn Andersson]

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

Lee,

I ran into this topic again while looking into some unrelated things.

> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> +struct rproc *of_rproc_byindex(struct device_node *np, int index)
> +{
> +	struct rproc *rproc;
> +	struct device_node *rproc_node;
> +	struct platform_device *pdev;
> +	struct klist_iter i;
> +
> +	if (index < 0)
> +		return ERR_PTR(-EINVAL);
> +
> +	rproc_node = of_parse_phandle(np, "rprocs", index);
> +	if (!rproc_node)

As I stated before I would like for you to use the existing rproc_list,
as done in rproc_get_by_phandle() today.

But as you resend this patch, could you please make this check fallback
to also checking for "ti,rproc", then simply drop the existing
rproc_get_by_phandle() and change the call to
of_rproc_byindex(dev->of_node, 0) in wkup_m3_ipc.c?

That way we're backwards compatible with the TI wakeup M3, without
having to maintain the then non-standard generic rproc phandle resolver.

> +		return ERR_PTR(-ENODEV);
> +
> +	pdev = of_find_device_by_node(rproc_node);
> +	if (!pdev)
> +		return ERR_PTR(-ENODEV);
> +
> +	klist_iter_init(&rprocs, &i);
> +	while ((rproc = next_rproc(&i)) != NULL)
> +		if (rproc->dev.parent == &pdev->dev)
> +			break;
> +	klist_iter_exit(&i);
> +
> +	return rproc;
> +}
> +EXPORT_SYMBOL(of_rproc_byindex);

Regards,
Bjorn

Re: [PATCH 2/5] remoteproc: core: Add rproc OF look-up functions

[Lee Jones]

On Wed, 13 Jul 2016, Bjorn Andersson wrote:

> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
> 
> Lee,
> 
> I ran into this topic again while looking into some unrelated things.
> 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > +struct rproc *of_rproc_byindex(struct device_node *np, int index)
> > +{
> > +	struct rproc *rproc;
> > +	struct device_node *rproc_node;
> > +	struct platform_device *pdev;
> > +	struct klist_iter i;
> > +
> > +	if (index < 0)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	rproc_node = of_parse_phandle(np, "rprocs", index);
> > +	if (!rproc_node)
> 
> As I stated before I would like for you to use the existing rproc_list,
> as done in rproc_get_by_phandle() today.
> 
> But as you resend this patch, could you please make this check fallback
> to also checking for "ti,rproc", then simply drop the existing
> rproc_get_by_phandle() and change the call to
> of_rproc_byindex(dev->of_node, 0) in wkup_m3_ipc.c?
> 
> That way we're backwards compatible with the TI wakeup M3, without
> having to maintain the then non-standard generic rproc phandle resolver.

I started working on this yesterday.  Should be with you soon.

> > +		return ERR_PTR(-ENODEV);
> > +
> > +	pdev = of_find_device_by_node(rproc_node);
> > +	if (!pdev)
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	klist_iter_init(&rprocs, &i);
> > +	while ((rproc = next_rproc(&i)) != NULL)
> > +		if (rproc->dev.parent == &pdev->dev)
> > +			break;
> > +	klist_iter_exit(&i);
> > +
> > +	return rproc;
> > +}
> > +EXPORT_SYMBOL(of_rproc_byindex);
> 
> Regards,
> Bjorn

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

[PATCH 2/5] remoteproc: core: Add rproc OF look-up functions

[Lee Jones]

On Wed, 13 Jul 2016, Bjorn Andersson wrote:

> On Thu 05 May 06:29 PDT 2016, Lee Jones wrote:
> 
> Lee,
> 
> I ran into this topic again while looking into some unrelated things.
> 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > +struct rproc *of_rproc_byindex(struct device_node *np, int index)
> > +{
> > +	struct rproc *rproc;
> > +	struct device_node *rproc_node;
> > +	struct platform_device *pdev;
> > +	struct klist_iter i;
> > +
> > +	if (index < 0)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	rproc_node = of_parse_phandle(np, "rprocs", index);
> > +	if (!rproc_node)
> 
> As I stated before I would like for you to use the existing rproc_list,
> as done in rproc_get_by_phandle() today.
> 
> But as you resend this patch, could you please make this check fallback
> to also checking for "ti,rproc", then simply drop the existing
> rproc_get_by_phandle() and change the call to
> of_rproc_byindex(dev->of_node, 0) in wkup_m3_ipc.c?
> 
> That way we're backwards compatible with the TI wakeup M3, without
> having to maintain the then non-standard generic rproc phandle resolver.

I started working on this yesterday.  Should be with you soon.

> > +		return ERR_PTR(-ENODEV);
> > +
> > +	pdev = of_find_device_by_node(rproc_node);
> > +	if (!pdev)
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	klist_iter_init(&rprocs, &i);
> > +	while ((rproc = next_rproc(&i)) != NULL)
> > +		if (rproc->dev.parent == &pdev->dev)
> > +			break;
> > +	klist_iter_exit(&i);
> > +
> > +	return rproc;
> > +}
> > +EXPORT_SYMBOL(of_rproc_byindex);
> 
> Regards,
> Bjorn

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