diff mbox

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

Message ID 1462454983-13168-3-git-send-email-lee.jones@linaro.org
State New
Headers show

Commit Message

Lee Jones May 5, 2016, 1:29 p.m. UTC
- 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(-)

-- 
2.8.0

Comments

Bjorn Andersson May 6, 2016, 6:48 p.m. UTC | #1
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
Lee Jones May 10, 2016, 2:16 p.m. UTC | #2
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
Bjorn Andersson May 10, 2016, 6:48 p.m. UTC | #3
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
Lee Jones July 14, 2016, 6:53 a.m. UTC | #4
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
diff mbox

Patch

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)
 {