[RFC,1/6] remoteproc: add early probed subdevices

Commit Message

Arnaud POULIQUEN Nov. 24, 2017, 2:36 p.m. UTC

From: Fabien Dessenne <[e-mail]>

Add the option to use rproc subdevices that are probed *before* the
remote processor firmware boot.

Signed-off-by: Fabien Dessenne <[e-mail]>
Signed-off-by: Arnaud Pouliquen <[e-mail]>
Signed-off-by: Loic Pallardy <[e-mail]>
---
 drivers/remoteproc/remoteproc_core.c | 62 +++++++++++++++++++++++++++++-------
 include/linux/remoteproc.h           |  7 ++++
 2 files changed, 58 insertions(+), 11 deletions(-)

Comments

Bjorn Andersson Dec. 14, 2017, 6:11 a.m. UTC | #1

On Fri 24 Nov 06:36 PST 2017, Arnaud Pouliquen wrote:

> From: Fabien Dessenne <[e-mail]>
> 
> Add the option to use rproc subdevices that are probed *before* the
> remote processor firmware boot.
> 

Rather than adding a somewhat obscure variant of subdevices I think you
should take Loic's addition of a "release" callback on the memory node
and turn this into a struct rproc_resource that can be embedded in
various resource-types (using container_of to grab the specific struct).

Transitioning carveouts, mappings, vrings etc to using this should allow
us to turn all the different lists of resources that should be
acquired before boot and released after shutdown.


And rather than starting by introducing the SRM I would like to see an
example that hands over the control of "clk" in imx_rproc.c to the
remoteproc core (this is the simplest example I could find).

This can then be built upon to allow referencing additional clocks to
enable statically, by name, from the resource table.

> @@ -881,20 +881,27 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>  		rproc->table_ptr = loaded_table;
>  	}
>  
> +	/* early probe any subdevices for the remote processor */
> +	ret = rproc_probe_subdevices(&rproc->early_subdevs);
> +	if (ret) {
> +		dev_err(dev, "failed to early probe subdevices for %s: %d\n",
> +			rproc->name, ret);
> +		return ret;

Enabling the "early" resources this late will make it infeasible to use
for things like clocking memories of the remoteproc.

> @@ -1028,6 +1041,9 @@ static int rproc_stop(struct rproc *rproc)
>  		return ret;
>  	}
>  
> +	/* remove any early-probed subdevices for the remote processor */
> +	rproc_remove_subdevices(&rproc->early_subdevs);
> +

This will cause "early subdevs" to be disabled during recovery, just to
be enabled immediately again.

So I think that like the memory resources we want to hold on to them
from boot to shutdown. Preferably in a way that allows us to provide the
ordering that allow us to use this mechanism to describe clocks for
memories in the remoteproc.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index eab14b4..870b1fb 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -770,12 +770,12 @@  static int rproc_handle_resources(struct rproc *rproc, int len,
 	return ret;
 }
 
-static int rproc_probe_subdevices(struct rproc *rproc)
+static int rproc_probe_subdevices(struct list_head *head)
 {
 	struct rproc_subdev *subdev;
 	int ret;
 
-	list_for_each_entry(subdev, &rproc->subdevs, node) {
+	list_for_each_entry(subdev, head, node) {
 		ret = subdev->probe(subdev);
 		if (ret)
 			goto unroll_registration;
@@ -784,17 +784,17 @@  static int rproc_probe_subdevices(struct rproc *rproc)
 	return 0;
 
 unroll_registration:
-	list_for_each_entry_continue_reverse(subdev, &rproc->subdevs, node)
+	list_for_each_entry_continue_reverse(subdev, head, node)
 		subdev->remove(subdev);
 
 	return ret;
 }
 
-static void rproc_remove_subdevices(struct rproc *rproc)
+static void rproc_remove_subdevices(struct list_head *head)
 {
 	struct rproc_subdev *subdev;
 
-	list_for_each_entry_reverse(subdev, &rproc->subdevs, node)
+	list_for_each_entry_reverse(subdev, head, node)
 		subdev->remove(subdev);
 }
 
@@ -881,20 +881,27 @@  static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 		rproc->table_ptr = loaded_table;
 	}
 
+	/* early probe any subdevices for the remote processor */
+	ret = rproc_probe_subdevices(&rproc->early_subdevs);
+	if (ret) {
+		dev_err(dev, "failed to early probe subdevices for %s: %d\n",
+			rproc->name, ret);
+		return ret;
+	}
+
 	/* power up the remote processor */
 	ret = rproc->ops->start(rproc);
 	if (ret) {
 		dev_err(dev, "can't start rproc %s: %d\n", rproc->name, ret);
-		return ret;
+		goto remove_early;
 	}
 
 	/* probe any subdevices for the remote processor */
-	ret = rproc_probe_subdevices(rproc);
+	ret = rproc_probe_subdevices(&rproc->subdevs);
 	if (ret) {
 		dev_err(dev, "failed to probe subdevices for %s: %d\n",
 			rproc->name, ret);
-		rproc->ops->stop(rproc);
-		return ret;
+		goto stop_rproc;
 	}
 
 	rproc->state = RPROC_RUNNING;
@@ -902,6 +909,12 @@  static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 	dev_info(dev, "remote processor %s is now up\n", rproc->name);
 
 	return 0;
+
+stop_rproc:
+	rproc->ops->stop(rproc);
+remove_early:
+	rproc_remove_subdevices(&rproc->early_subdevs);
+	return ret;
 }
 
 /*
@@ -1019,7 +1032,7 @@  static int rproc_stop(struct rproc *rproc)
 	int ret;
 
 	/* remove any subdevices for the remote processor */
-	rproc_remove_subdevices(rproc);
+	rproc_remove_subdevices(&rproc->subdevs);
 
 	/* power off the remote processor */
 	ret = rproc->ops->stop(rproc);
@@ -1028,6 +1041,9 @@  static int rproc_stop(struct rproc *rproc)
 		return ret;
 	}
 
+	/* remove any early-probed subdevices for the remote processor */
+	rproc_remove_subdevices(&rproc->early_subdevs);
+
 	/* if in crash state, unlock crash handler */
 	if (rproc->state == RPROC_CRASHED)
 		complete_all(&rproc->crash_comp);
@@ -1457,6 +1473,7 @@  struct rproc *rproc_alloc(struct device *dev, const char *name,
 	INIT_LIST_HEAD(&rproc->traces);
 	INIT_LIST_HEAD(&rproc->rvdevs);
 	INIT_LIST_HEAD(&rproc->subdevs);
+	INIT_LIST_HEAD(&rproc->early_subdevs);
 
 	INIT_WORK(&rproc->crash_handler, rproc_crash_handler_work);
 	init_completion(&rproc->crash_comp);
@@ -1560,9 +1577,32 @@  void rproc_add_subdev(struct rproc *rproc,
 EXPORT_SYMBOL(rproc_add_subdev);
 
 /**
+ * rproc_add_early_subdev() - add an early subdevice to a remoteproc
+ * @rproc: rproc handle to add the subdevice to
+ * @subdev: subdev handle to register
+ * @probe: function to call before the rproc boots
+ * @remove: function to call after the rproc shuts down
+ *
+ * This function differs from rproc_add_subdev() in the sense that the added
+ * subdevices are probed BEFORE the rproc boots.
+ */
+void rproc_add_early_subdev(struct rproc *rproc,
+			    struct rproc_subdev *subdev,
+			    int (*probe)(struct rproc_subdev *subdev),
+			    void (*remove)(struct rproc_subdev *subdev))
+{
+	subdev->probe = probe;
+	subdev->remove = remove;
+
+	list_add_tail(&subdev->node, &rproc->early_subdevs);
+}
+EXPORT_SYMBOL(rproc_add_early_subdev);
+
+/**
  * rproc_remove_subdev() - remove a subdevice from a remoteproc
  * @rproc: rproc handle to remove the subdevice from
- * @subdev: subdev handle, previously registered with rproc_add_subdev()
+ * @subdev: subdev handle, previously registered with rproc_add_subdev() or
+ *          rproc_add_early_subdev()
  */
 void rproc_remove_subdev(struct rproc *rproc, struct rproc_subdev *subdev)
 {
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 44e630e..e6b02d8 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -402,6 +402,7 @@  enum rproc_crash_type {
  * @bootaddr: address of first instruction to boot rproc with (optional)
  * @rvdevs: list of remote virtio devices
  * @subdevs: list of subdevices, to following the running state
+ * @early_subdevs: list of early-probed subdevices
  * @notifyids: idr for dynamically assigning rproc-wide unique notify ids
  * @index: index of this rproc device
  * @crash_handler: workqueue for handling a crash
@@ -433,6 +434,7 @@  struct rproc {
 	u32 bootaddr;
 	struct list_head rvdevs;
 	struct list_head subdevs;
+	struct list_head early_subdevs;
 	struct idr notifyids;
 	int index;
 	struct work_struct crash_handler;
@@ -541,6 +543,11 @@  void rproc_add_subdev(struct rproc *rproc,
 		      int (*probe)(struct rproc_subdev *subdev),
 		      void (*remove)(struct rproc_subdev *subdev));
 
+void rproc_add_early_subdev(struct rproc *rproc,
+			    struct rproc_subdev *subdev,
+			    int (*probe)(struct rproc_subdev *subdev),
+			    void (*remove)(struct rproc_subdev *subdev));
+
 void rproc_remove_subdev(struct rproc *rproc, struct rproc_subdev *subdev);
 
 #endif /* REMOTEPROC_H */