| Message ID | 20160719154905.23344-1-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote: > - of_rproc_by_index(): look-up and obtain a reference to a rproc > using the DT phandle "rprocs" and a index. > > - of_rproc_by_name(): 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'm happy with this, so I whipped up a binding document describing our two new properties. Waiting for an opinion on that before I merge this. Regards, Bjorn > drivers/remoteproc/remoteproc_core.c | 78 +++++++++++++++++++++++++++++++++++- > include/linux/remoteproc.h | 25 +++++++++++- > 2 files changed, 101 insertions(+), 2 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index db3958b..6a0d158 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -41,6 +41,8 @@ > #include <linux/virtio_ids.h> > #include <linux/virtio_ring.h> > #include <asm/byteorder.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > > #include "remoteproc_internal.h" > > @@ -1191,6 +1193,81 @@ out: > } > EXPORT_SYMBOL(rproc_shutdown); > > +#ifdef CONFIG_OF > +/** > + * of_get_rproc_by_index() - lookup and obtain a reference to an rproc > + * @np: node to search for rproc phandle > + * @index: index into the phandle list > + * > + * This function increments the remote processor's refcount, so always > + * use rproc_put() to decrement it back once rproc isn't needed anymore. > + * > + * Returns a pointer to the rproc struct on success or an appropriate error > + * code otherwise. > + */ > +struct rproc *of_get_rproc_by_index(struct device_node *np, int index) > +{ > + struct rproc *rproc = NULL, *r; > + struct device_node *rproc_np; > + > + if (index < 0) { > + pr_err("Invalid index: %d\n", index); > + return ERR_PTR(-EINVAL); > + } > + > + rproc_np = of_parse_phandle(np, "rprocs", index); > + if (!rproc_np) { > + /* Unfortunately we have to support this, at least for now */ > + rproc_np = of_parse_phandle(np, "ti,rprocs", index); > + if (!rproc_np) { > + pr_err("Failed to obtain phandle\n"); > + return ERR_PTR(-ENODEV); > + } > + } > + > + mutex_lock(&rproc_list_mutex); > + list_for_each_entry(r, &rproc_list, node) { > + if (r->dev.parent && r->dev.parent->of_node == rproc_np) { > + get_device(&r->dev); > + rproc = r; > + break; > + } > + } > + mutex_unlock(&rproc_list_mutex); > + > + of_node_put(rproc_np); > + > + if (!rproc) > + pr_err("Could not find rproc, deferring\n"); > + > + return rproc ?: ERR_PTR(-EPROBE_DEFER); > +} > +EXPORT_SYMBOL(of_get_rproc_by_index); > + > +/** > + * of_get_rproc_by_name() - 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 > + * > + * This function increments the remote processor's refcount, so always > + * use rproc_put() to decrement it back once rproc isn't needed anymore. > + * > + * Returns a pointer to the rproc struct on success or an appropriate error > + * code otherwise. > + */ > +struct rproc *of_get_rproc_by_name(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_get_rproc_by_index(np, index); > +} > +EXPORT_SYMBOL(of_get_rproc_by_name); > + > /** > * rproc_get_by_phandle() - find a remote processor by phandle > * @phandle: phandle to the rproc > @@ -1203,7 +1280,6 @@ EXPORT_SYMBOL(rproc_shutdown); > * > * Returns the rproc handle on success, and NULL on failure. > */ > -#ifdef CONFIG_OF > struct rproc *rproc_get_by_phandle(phandle phandle) > { > struct rproc *rproc = NULL, *r; > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index 1c457a8..f130938 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -487,7 +487,6 @@ struct rproc_vdev { > u32 rsc_offset; > }; > > -struct rproc *rproc_get_by_phandle(phandle phandle); > struct rproc *rproc_alloc(struct device *dev, const char *name, > const struct rproc_ops *ops, > const char *firmware, int len); > @@ -511,4 +510,28 @@ static inline struct rproc *vdev_to_rproc(struct virtio_device *vdev) > return rvdev->rproc; > } > > +#ifdef CONFIG_OF > +extern struct rproc *of_get_rproc_by_index(struct device_node *np, > + int index); > +extern struct rproc *of_get_rproc_by_name(struct device_node *np, > + const char *name); > +extern struct rproc *rproc_get_by_phandle(phandle phandle); > +#else > +static inline > +struct rproc *of_get_rproc_by_index(struct device_node *np, int index) > +{ > + return NULL; > +} > +static inline > +struct rproc *of_get_rproc_by_name(struct device_node *np, const char *name) > +{ > + return NULL; > +} > +static inline > +struct rproc *rproc_get_by_phandle(phandle phandle) > +{ > + return NULL; > +} > +#endif /* CONFIG_OF */ > + > #endif /* REMOTEPROC_H */ > -- > 2.9.0 >
Hi Lee, Bjorn, On 08/10/2016 12:40 PM, Bjorn Andersson wrote: > On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote: > >> - of_rproc_by_index(): look-up and obtain a reference to a rproc >> using the DT phandle "rprocs" and a index. >> >> - of_rproc_by_name(): 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'm happy with this, so I whipped up a binding document describing our > two new properties. Waiting for an opinion on that before I merge this. Yeah, I like the two new API too, I have just about run into the need for the same on my product trees. We can probably make it less complicated than what the current series is. The wkup_m3_ipc usage was before there was any standardization on the property names, so we went with a ti, prefixed property specific to the wkup_m3_ipc node and a general API that is agnostic of property name. We don't have all the pieces for PM on AM335x/AM437x SoCs on upstream kernel yet, so we should be able to switch over to using the new property as we cannot achieve the desired functionality even though we can boot the wkup_m3. Here's what I propose: 1. Let's not burden the new of_get_rproc_by_index() API with having to fall-back and look for ti,rprocs. The rproc_get_by_phandle() core logic of looking up against the rproc list is self-contained, and the API usage is limited to just the wkup_m3_ipc driver at the moment. 2. Keep the rproc_get_by_phandle API as is but mark it as deprecated. IMHO, the rename of this API to of_get_rproc_by_phandle() in Patch 2 but using a device node pointer as input argument doesn't add any value. 3. Provide a fallback in wkup_m3_ipc driver to look for both rprocs and ti,rproc property, while switching over the node to use rprocs property. 4. We can get rid of the rproc_get_by_phandle() API after the wkup_m3_ipc transition is done to use of_get_rproc_by_index() API. Thoughts? regards Suman > > Regards, > Bjorn > >> drivers/remoteproc/remoteproc_core.c | 78 +++++++++++++++++++++++++++++++++++- >> include/linux/remoteproc.h | 25 +++++++++++- >> 2 files changed, 101 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index db3958b..6a0d158 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -41,6 +41,8 @@ >> #include <linux/virtio_ids.h> >> #include <linux/virtio_ring.h> >> #include <asm/byteorder.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> >> #include "remoteproc_internal.h" >> >> @@ -1191,6 +1193,81 @@ out: >> } >> EXPORT_SYMBOL(rproc_shutdown); >> >> +#ifdef CONFIG_OF >> +/** >> + * of_get_rproc_by_index() - lookup and obtain a reference to an rproc >> + * @np: node to search for rproc phandle >> + * @index: index into the phandle list >> + * >> + * This function increments the remote processor's refcount, so always >> + * use rproc_put() to decrement it back once rproc isn't needed anymore. >> + * >> + * Returns a pointer to the rproc struct on success or an appropriate error >> + * code otherwise. >> + */ >> +struct rproc *of_get_rproc_by_index(struct device_node *np, int index) >> +{ >> + struct rproc *rproc = NULL, *r; >> + struct device_node *rproc_np; >> + >> + if (index < 0) { >> + pr_err("Invalid index: %d\n", index); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + rproc_np = of_parse_phandle(np, "rprocs", index); >> + if (!rproc_np) { >> + /* Unfortunately we have to support this, at least for now */ >> + rproc_np = of_parse_phandle(np, "ti,rprocs", index); >> + if (!rproc_np) { >> + pr_err("Failed to obtain phandle\n"); >> + return ERR_PTR(-ENODEV); >> + } >> + } >> + >> + mutex_lock(&rproc_list_mutex); >> + list_for_each_entry(r, &rproc_list, node) { >> + if (r->dev.parent && r->dev.parent->of_node == rproc_np) { >> + get_device(&r->dev); >> + rproc = r; >> + break; >> + } >> + } >> + mutex_unlock(&rproc_list_mutex); >> + >> + of_node_put(rproc_np); >> + >> + if (!rproc) >> + pr_err("Could not find rproc, deferring\n"); >> + >> + return rproc ?: ERR_PTR(-EPROBE_DEFER); >> +} >> +EXPORT_SYMBOL(of_get_rproc_by_index); >> + >> +/** >> + * of_get_rproc_by_name() - 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 >> + * >> + * This function increments the remote processor's refcount, so always >> + * use rproc_put() to decrement it back once rproc isn't needed anymore. >> + * >> + * Returns a pointer to the rproc struct on success or an appropriate error >> + * code otherwise. >> + */ >> +struct rproc *of_get_rproc_by_name(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_get_rproc_by_index(np, index); >> +} >> +EXPORT_SYMBOL(of_get_rproc_by_name); >> + >> /** >> * rproc_get_by_phandle() - find a remote processor by phandle >> * @phandle: phandle to the rproc >> @@ -1203,7 +1280,6 @@ EXPORT_SYMBOL(rproc_shutdown); >> * >> * Returns the rproc handle on success, and NULL on failure. >> */ >> -#ifdef CONFIG_OF >> struct rproc *rproc_get_by_phandle(phandle phandle) >> { >> struct rproc *rproc = NULL, *r; >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >> index 1c457a8..f130938 100644 >> --- a/include/linux/remoteproc.h >> +++ b/include/linux/remoteproc.h >> @@ -487,7 +487,6 @@ struct rproc_vdev { >> u32 rsc_offset; >> }; >> >> -struct rproc *rproc_get_by_phandle(phandle phandle); >> struct rproc *rproc_alloc(struct device *dev, const char *name, >> const struct rproc_ops *ops, >> const char *firmware, int len); >> @@ -511,4 +510,28 @@ static inline struct rproc *vdev_to_rproc(struct virtio_device *vdev) >> return rvdev->rproc; >> } >> >> +#ifdef CONFIG_OF >> +extern struct rproc *of_get_rproc_by_index(struct device_node *np, >> + int index); >> +extern struct rproc *of_get_rproc_by_name(struct device_node *np, >> + const char *name); >> +extern struct rproc *rproc_get_by_phandle(phandle phandle); >> +#else >> +static inline >> +struct rproc *of_get_rproc_by_index(struct device_node *np, int index) >> +{ >> + return NULL; >> +} >> +static inline >> +struct rproc *of_get_rproc_by_name(struct device_node *np, const char *name) >> +{ >> + return NULL; >> +} >> +static inline >> +struct rproc *rproc_get_by_phandle(phandle phandle) >> +{ >> + return NULL; >> +} >> +#endif /* CONFIG_OF */ >> + >> #endif /* REMOTEPROC_H */ >> -- >> 2.9.0 >> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Wed 10 Aug 12:37 PDT 2016, Suman Anna wrote: > Hi Lee, Bjorn, > > On 08/10/2016 12:40 PM, Bjorn Andersson wrote: > > On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote: > > > >> - of_rproc_by_index(): look-up and obtain a reference to a rproc > >> using the DT phandle "rprocs" and a index. > >> > >> - of_rproc_by_name(): 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'm happy with this, so I whipped up a binding document describing our > > two new properties. Waiting for an opinion on that before I merge this. > > Yeah, I like the two new API too, I have just about run into the need > for the same on my product trees. We can probably make it less > complicated than what the current series is. The wkup_m3_ipc usage was > before there was any standardization on the property names, so we went > with a ti, prefixed property specific to the wkup_m3_ipc node and a > general API that is agnostic of property name. We don't have all the > pieces for PM on AM335x/AM437x SoCs on upstream kernel yet, so we should > be able to switch over to using the new property as we cannot achieve > the desired functionality even though we can boot the wkup_m3. > Glad to hear you're onboard with dropping the old property name, even if it's later. > Here's what I propose: > 1. Let's not burden the new of_get_rproc_by_index() API with having to > fall-back and look for ti,rprocs. The rproc_get_by_phandle() core logic > of looking up against the rproc list is self-contained, and the API > usage is limited to just the wkup_m3_ipc driver at the moment. > 2. Keep the rproc_get_by_phandle API as is but mark it as deprecated. > IMHO, the rename of this API to of_get_rproc_by_phandle() in Patch 2 but > using a device node pointer as input argument doesn't add any value. > 3. Provide a fallback in wkup_m3_ipc driver to look for both rprocs and > ti,rproc property, while switching over the node to use rprocs property. > 4. We can get rid of the rproc_get_by_phandle() API after the > wkup_m3_ipc transition is done to use of_get_rproc_by_index() API. > I don't fancy the idea of leaving the kernel builds with a deprecation warning for some time and I don't feel the cost of carrying those 2 extra statements is a bigger issue than keeping a deprecated API around. And it will be less than the dance we have to do in wkup_m3_ipc. So I think that we should merge these patches and if it can be concluded that we don't need backwards compatibility with the old DT property we can drop the inner conditional in the API. Regards, Bjorn
On 08/10/2016 03:40 PM, Bjorn Andersson wrote: > On Wed 10 Aug 12:37 PDT 2016, Suman Anna wrote: > >> Hi Lee, Bjorn, >> >> On 08/10/2016 12:40 PM, Bjorn Andersson wrote: >>> On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote: >>> >>>> - of_rproc_by_index(): look-up and obtain a reference to a rproc >>>> using the DT phandle "rprocs" and a index. >>>> >>>> - of_rproc_by_name(): 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'm happy with this, so I whipped up a binding document describing our >>> two new properties. Waiting for an opinion on that before I merge this. >> >> Yeah, I like the two new API too, I have just about run into the need >> for the same on my product trees. We can probably make it less >> complicated than what the current series is. The wkup_m3_ipc usage was >> before there was any standardization on the property names, so we went >> with a ti, prefixed property specific to the wkup_m3_ipc node and a >> general API that is agnostic of property name. We don't have all the >> pieces for PM on AM335x/AM437x SoCs on upstream kernel yet, so we should >> be able to switch over to using the new property as we cannot achieve >> the desired functionality even though we can boot the wkup_m3. >> > > Glad to hear you're onboard with dropping the old property name, even if > it's later. > >> Here's what I propose: >> 1. Let's not burden the new of_get_rproc_by_index() API with having to >> fall-back and look for ti,rprocs. The rproc_get_by_phandle() core logic >> of looking up against the rproc list is self-contained, and the API >> usage is limited to just the wkup_m3_ipc driver at the moment. >> 2. Keep the rproc_get_by_phandle API as is but mark it as deprecated. >> IMHO, the rename of this API to of_get_rproc_by_phandle() in Patch 2 but >> using a device node pointer as input argument doesn't add any value. >> 3. Provide a fallback in wkup_m3_ipc driver to look for both rprocs and >> ti,rproc property, while switching over the node to use rprocs property. >> 4. We can get rid of the rproc_get_by_phandle() API after the >> wkup_m3_ipc transition is done to use of_get_rproc_by_index() API. >> > > I don't fancy the idea of leaving the kernel builds with a deprecation > warning for some time and I don't feel the cost of carrying those 2 > extra statements is a bigger issue than keeping a deprecated API around. > And it will be less than the dance we have to do in wkup_m3_ipc. > > So I think that we should merge these patches and if it can be concluded > that we don't need backwards compatibility with the old DT property we > can drop the inner conditional in the API. Yeah, I am fine with dropping the inner ti,rproc processing in the new of_rproc_get_by_index() API and keeping it clean. What's not clear to me is why we would still need a get_by_phandle API alongside the two new API once the wkup_m3_ipc is converted to the new API? Or is it just to clean up the consumer interface? If latter, I will fixup the wkup_m3_ipc driver without the need for remoteproc core changes, and switch over to the new API. regards Suman
On Wed 10 Aug 14:04 PDT 2016, Suman Anna wrote: > On 08/10/2016 03:40 PM, Bjorn Andersson wrote: > > On Wed 10 Aug 12:37 PDT 2016, Suman Anna wrote: > > > >> Hi Lee, Bjorn, > >> > >> On 08/10/2016 12:40 PM, Bjorn Andersson wrote: > >>> On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote: > >>> > >>>> - of_rproc_by_index(): look-up and obtain a reference to a rproc > >>>> using the DT phandle "rprocs" and a index. > >>>> > >>>> - of_rproc_by_name(): 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'm happy with this, so I whipped up a binding document describing our > >>> two new properties. Waiting for an opinion on that before I merge this. > >> > >> Yeah, I like the two new API too, I have just about run into the need > >> for the same on my product trees. We can probably make it less > >> complicated than what the current series is. The wkup_m3_ipc usage was > >> before there was any standardization on the property names, so we went > >> with a ti, prefixed property specific to the wkup_m3_ipc node and a > >> general API that is agnostic of property name. We don't have all the > >> pieces for PM on AM335x/AM437x SoCs on upstream kernel yet, so we should > >> be able to switch over to using the new property as we cannot achieve > >> the desired functionality even though we can boot the wkup_m3. > >> > > > > Glad to hear you're onboard with dropping the old property name, even if > > it's later. > > > >> Here's what I propose: > >> 1. Let's not burden the new of_get_rproc_by_index() API with having to > >> fall-back and look for ti,rprocs. The rproc_get_by_phandle() core logic > >> of looking up against the rproc list is self-contained, and the API > >> usage is limited to just the wkup_m3_ipc driver at the moment. > >> 2. Keep the rproc_get_by_phandle API as is but mark it as deprecated. > >> IMHO, the rename of this API to of_get_rproc_by_phandle() in Patch 2 but > >> using a device node pointer as input argument doesn't add any value. > >> 3. Provide a fallback in wkup_m3_ipc driver to look for both rprocs and > >> ti,rproc property, while switching over the node to use rprocs property. > >> 4. We can get rid of the rproc_get_by_phandle() API after the > >> wkup_m3_ipc transition is done to use of_get_rproc_by_index() API. > >> > > > > I don't fancy the idea of leaving the kernel builds with a deprecation > > warning for some time and I don't feel the cost of carrying those 2 > > extra statements is a bigger issue than keeping a deprecated API around. > > And it will be less than the dance we have to do in wkup_m3_ipc. > > > > So I think that we should merge these patches and if it can be concluded > > that we don't need backwards compatibility with the old DT property we > > can drop the inner conditional in the API. > > Yeah, I am fine with dropping the inner ti,rproc processing in the new > of_rproc_get_by_index() API and keeping it clean. What's not clear to me > is why we would still need a get_by_phandle API alongside the two new > API once the wkup_m3_ipc is converted to the new API? Or is it just to > clean up the consumer interface? If latter, I will fixup the wkup_m3_ipc > driver without the need for remoteproc core changes, and switch over to > the new API. > of_get_rproc_by_phandle() is just a convenience for not having to get by index 0, as most drivers is only having one rproc. As far as cleaning up wkup_m3_ipc, patch 2 does that and cleans out the old implementation and with that wkup_m3_ipc is moved to the new API. So the only issue is that the wkup_m3_ipc DT binding states that the property should be named "ti,rproc" and as such someone has to send a patch to that and make an argument that we don't have to maintain backwards compatibility. But as this is used in am33xx.dtsi and am4372.dtsi that might be too late? Regards, Bjorn
On 08/10/2016 04:19 PM, Bjorn Andersson wrote: > On Wed 10 Aug 14:04 PDT 2016, Suman Anna wrote: > >> On 08/10/2016 03:40 PM, Bjorn Andersson wrote: >>> On Wed 10 Aug 12:37 PDT 2016, Suman Anna wrote: >>> >>>> Hi Lee, Bjorn, >>>> >>>> On 08/10/2016 12:40 PM, Bjorn Andersson wrote: >>>>> On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote: >>>>> >>>>>> - of_rproc_by_index(): look-up and obtain a reference to a rproc >>>>>> using the DT phandle "rprocs" and a index. >>>>>> >>>>>> - of_rproc_by_name(): 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'm happy with this, so I whipped up a binding document describing our >>>>> two new properties. Waiting for an opinion on that before I merge this. >>>> >>>> Yeah, I like the two new API too, I have just about run into the need >>>> for the same on my product trees. We can probably make it less >>>> complicated than what the current series is. The wkup_m3_ipc usage was >>>> before there was any standardization on the property names, so we went >>>> with a ti, prefixed property specific to the wkup_m3_ipc node and a >>>> general API that is agnostic of property name. We don't have all the >>>> pieces for PM on AM335x/AM437x SoCs on upstream kernel yet, so we should >>>> be able to switch over to using the new property as we cannot achieve >>>> the desired functionality even though we can boot the wkup_m3. >>>> >>> >>> Glad to hear you're onboard with dropping the old property name, even if >>> it's later. >>> >>>> Here's what I propose: >>>> 1. Let's not burden the new of_get_rproc_by_index() API with having to >>>> fall-back and look for ti,rprocs. The rproc_get_by_phandle() core logic >>>> of looking up against the rproc list is self-contained, and the API >>>> usage is limited to just the wkup_m3_ipc driver at the moment. >>>> 2. Keep the rproc_get_by_phandle API as is but mark it as deprecated. >>>> IMHO, the rename of this API to of_get_rproc_by_phandle() in Patch 2 but >>>> using a device node pointer as input argument doesn't add any value. >>>> 3. Provide a fallback in wkup_m3_ipc driver to look for both rprocs and >>>> ti,rproc property, while switching over the node to use rprocs property. >>>> 4. We can get rid of the rproc_get_by_phandle() API after the >>>> wkup_m3_ipc transition is done to use of_get_rproc_by_index() API. >>>> >>> >>> I don't fancy the idea of leaving the kernel builds with a deprecation >>> warning for some time and I don't feel the cost of carrying those 2 >>> extra statements is a bigger issue than keeping a deprecated API around. >>> And it will be less than the dance we have to do in wkup_m3_ipc. >>> >>> So I think that we should merge these patches and if it can be concluded >>> that we don't need backwards compatibility with the old DT property we >>> can drop the inner conditional in the API. >> >> Yeah, I am fine with dropping the inner ti,rproc processing in the new >> of_rproc_get_by_index() API and keeping it clean. What's not clear to me >> is why we would still need a get_by_phandle API alongside the two new >> API once the wkup_m3_ipc is converted to the new API? Or is it just to >> clean up the consumer interface? If latter, I will fixup the wkup_m3_ipc >> driver without the need for remoteproc core changes, and switch over to >> the new API. >> > > of_get_rproc_by_phandle() is just a convenience for not having to get by > index 0, as most drivers is only having one rproc. OK, then better to name this as simply of_rproc_get(), the _by_phandle() does not match up with the other two of_get_rproc_xxx API. Suggest also a rename from of_get_rproc_xxx to of_rproc_get_xxx like in other subsystems. > > As far as cleaning up wkup_m3_ipc, patch 2 does that and cleans out the > old implementation and with that wkup_m3_ipc is moved to the new API. > > So the only issue is that the wkup_m3_ipc DT binding states that the > property should be named "ti,rproc" and as such someone has to send a > patch to that and make an argument that we don't have to maintain > backwards compatibility. But as this is used in am33xx.dtsi and > am4372.dtsi that might be too late? I could fix up the wkup_m3_ipc driver using of_update_property() / of_remove_property(), but looks like these are not exported. I am not sure if there are any specific reasons as to why these were not exported. So, for now, we may have to go with the current code. Tony, Do you have any issues with us switching to the new properties in wkup_m3_ipc nodes? regards Suman
On Wed, 10 Aug 2016, Suman Anna wrote: > On 08/10/2016 04:19 PM, Bjorn Andersson wrote: > > On Wed 10 Aug 14:04 PDT 2016, Suman Anna wrote: > > > >> On 08/10/2016 03:40 PM, Bjorn Andersson wrote: > >>> On Wed 10 Aug 12:37 PDT 2016, Suman Anna wrote: > >>> > >>>> Hi Lee, Bjorn, > >>>> > >>>> On 08/10/2016 12:40 PM, Bjorn Andersson wrote: > >>>>> On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote: > >>>>> > >>>>>> - of_rproc_by_index(): look-up and obtain a reference to a rproc > >>>>>> using the DT phandle "rprocs" and a index. > >>>>>> > >>>>>> - of_rproc_by_name(): 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'm happy with this, so I whipped up a binding document describing our > >>>>> two new properties. Waiting for an opinion on that before I merge this. > >>>> > >>>> Yeah, I like the two new API too, I have just about run into the need > >>>> for the same on my product trees. We can probably make it less > >>>> complicated than what the current series is. The wkup_m3_ipc usage was > >>>> before there was any standardization on the property names, so we went > >>>> with a ti, prefixed property specific to the wkup_m3_ipc node and a > >>>> general API that is agnostic of property name. We don't have all the > >>>> pieces for PM on AM335x/AM437x SoCs on upstream kernel yet, so we should > >>>> be able to switch over to using the new property as we cannot achieve > >>>> the desired functionality even though we can boot the wkup_m3. > >>>> > >>> > >>> Glad to hear you're onboard with dropping the old property name, even if > >>> it's later. > >>> > >>>> Here's what I propose: > >>>> 1. Let's not burden the new of_get_rproc_by_index() API with having to > >>>> fall-back and look for ti,rprocs. The rproc_get_by_phandle() core logic > >>>> of looking up against the rproc list is self-contained, and the API > >>>> usage is limited to just the wkup_m3_ipc driver at the moment. > >>>> 2. Keep the rproc_get_by_phandle API as is but mark it as deprecated. > >>>> IMHO, the rename of this API to of_get_rproc_by_phandle() in Patch 2 but > >>>> using a device node pointer as input argument doesn't add any value. > >>>> 3. Provide a fallback in wkup_m3_ipc driver to look for both rprocs and > >>>> ti,rproc property, while switching over the node to use rprocs property. > >>>> 4. We can get rid of the rproc_get_by_phandle() API after the > >>>> wkup_m3_ipc transition is done to use of_get_rproc_by_index() API. > >>>> > >>> > >>> I don't fancy the idea of leaving the kernel builds with a deprecation > >>> warning for some time and I don't feel the cost of carrying those 2 > >>> extra statements is a bigger issue than keeping a deprecated API around. > >>> And it will be less than the dance we have to do in wkup_m3_ipc. > >>> > >>> So I think that we should merge these patches and if it can be concluded > >>> that we don't need backwards compatibility with the old DT property we > >>> can drop the inner conditional in the API. > >> > >> Yeah, I am fine with dropping the inner ti,rproc processing in the new > >> of_rproc_get_by_index() API and keeping it clean. What's not clear to me > >> is why we would still need a get_by_phandle API alongside the two new > >> API once the wkup_m3_ipc is converted to the new API? Or is it just to > >> clean up the consumer interface? If latter, I will fixup the wkup_m3_ipc > >> driver without the need for remoteproc core changes, and switch over to > >> the new API. > >> > > > > of_get_rproc_by_phandle() is just a convenience for not having to get by > > index 0, as most drivers is only having one rproc. > > OK, then better to name this as simply of_rproc_get(), the _by_phandle() > does not match up with the other two of_get_rproc_xxx API. I'm not opposed to changing the call to of_rproc_get(). However, I'm not sure what you mean about it not matching the other OF functions. The nomenclature should be the same of_get_rproc_*(), no? > Suggest also a rename from of_get_rproc_xxx to of_rproc_get_xxx like in > other subsystems. This suggestion does the opposite and does not fit in with the majority of the of_* calls scattered around in other subsystems: of_get_drm of_get_coresight of_get_fb of_get_i2c of_get_regulator of_get_gpio of_get_mac of_get_display of_get_videomode Vs of_irq_get of_reset_get of_graph_get of_msi_get of_usb_get of_genpd_get These guys have both oddly. of_get_dma, of_dma_get of_get_pci, of_pci_get of_get_phy, of_phy_get
Hi Lee, On 08/11/2016 02:31 AM, Lee Jones wrote: > On Wed, 10 Aug 2016, Suman Anna wrote: > >> On 08/10/2016 04:19 PM, Bjorn Andersson wrote: >>> On Wed 10 Aug 14:04 PDT 2016, Suman Anna wrote: >>> >>>> On 08/10/2016 03:40 PM, Bjorn Andersson wrote: >>>>> On Wed 10 Aug 12:37 PDT 2016, Suman Anna wrote: >>>>> >>>>>> Hi Lee, Bjorn, >>>>>> >>>>>> On 08/10/2016 12:40 PM, Bjorn Andersson wrote: >>>>>>> On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote: >>>>>>> >>>>>>>> - of_rproc_by_index(): look-up and obtain a reference to a rproc >>>>>>>> using the DT phandle "rprocs" and a index. >>>>>>>> >>>>>>>> - of_rproc_by_name(): 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'm happy with this, so I whipped up a binding document describing our >>>>>>> two new properties. Waiting for an opinion on that before I merge this. >>>>>> >>>>>> Yeah, I like the two new API too, I have just about run into the need >>>>>> for the same on my product trees. We can probably make it less >>>>>> complicated than what the current series is. The wkup_m3_ipc usage was >>>>>> before there was any standardization on the property names, so we went >>>>>> with a ti, prefixed property specific to the wkup_m3_ipc node and a >>>>>> general API that is agnostic of property name. We don't have all the >>>>>> pieces for PM on AM335x/AM437x SoCs on upstream kernel yet, so we should >>>>>> be able to switch over to using the new property as we cannot achieve >>>>>> the desired functionality even though we can boot the wkup_m3. >>>>>> >>>>> >>>>> Glad to hear you're onboard with dropping the old property name, even if >>>>> it's later. >>>>> >>>>>> Here's what I propose: >>>>>> 1. Let's not burden the new of_get_rproc_by_index() API with having to >>>>>> fall-back and look for ti,rprocs. The rproc_get_by_phandle() core logic >>>>>> of looking up against the rproc list is self-contained, and the API >>>>>> usage is limited to just the wkup_m3_ipc driver at the moment. >>>>>> 2. Keep the rproc_get_by_phandle API as is but mark it as deprecated. >>>>>> IMHO, the rename of this API to of_get_rproc_by_phandle() in Patch 2 but >>>>>> using a device node pointer as input argument doesn't add any value. >>>>>> 3. Provide a fallback in wkup_m3_ipc driver to look for both rprocs and >>>>>> ti,rproc property, while switching over the node to use rprocs property. >>>>>> 4. We can get rid of the rproc_get_by_phandle() API after the >>>>>> wkup_m3_ipc transition is done to use of_get_rproc_by_index() API. >>>>>> >>>>> >>>>> I don't fancy the idea of leaving the kernel builds with a deprecation >>>>> warning for some time and I don't feel the cost of carrying those 2 >>>>> extra statements is a bigger issue than keeping a deprecated API around. >>>>> And it will be less than the dance we have to do in wkup_m3_ipc. >>>>> >>>>> So I think that we should merge these patches and if it can be concluded >>>>> that we don't need backwards compatibility with the old DT property we >>>>> can drop the inner conditional in the API. >>>> >>>> Yeah, I am fine with dropping the inner ti,rproc processing in the new >>>> of_rproc_get_by_index() API and keeping it clean. What's not clear to me >>>> is why we would still need a get_by_phandle API alongside the two new >>>> API once the wkup_m3_ipc is converted to the new API? Or is it just to >>>> clean up the consumer interface? If latter, I will fixup the wkup_m3_ipc >>>> driver without the need for remoteproc core changes, and switch over to >>>> the new API. >>>> >>> >>> of_get_rproc_by_phandle() is just a convenience for not having to get by >>> index 0, as most drivers is only having one rproc. >> >> OK, then better to name this as simply of_rproc_get(), the _by_phandle() >> does not match up with the other two of_get_rproc_xxx API. > > I'm not opposed to changing the call to of_rproc_get(). However, I'm > not sure what you mean about it not matching the other OF functions. The _by_name and _by_index API take in a name and index arguments, and the rproc_get_by_phandle took in a phandle argument, the modified name in patch 2 retains the by_phandle but without any corresponding argument. That's what I meant by mismatch. > The nomenclature should be the same of_get_rproc_*(), no? > >> Suggest also a rename from of_get_rproc_xxx to of_rproc_get_xxx like in >> other subsystems. > > This suggestion does the opposite and does not fit in with the > majority of the of_* calls scattered around in other subsystems: > > of_get_drm > of_get_coresight > of_get_fb > of_get_i2c > of_get_regulator > of_get_gpio > of_get_mac > of_get_display > of_get_videomode > > Vs > > of_irq_get > of_reset_get > of_graph_get > of_msi_get > of_usb_get > of_genpd_get > > These guys have both oddly. > > of_get_dma, of_dma_get > of_get_pci, of_pci_get > of_get_phy, of_phy_get Hmm, looks like there are all sorts of combinations with some like <subsys>_of_get_xxx. I did search using get_name and get_index, and nothing popped up differently. The ones that typical rproc drivers deal with are clk, reset, irq which follow the latter convention. Given that the previous API within remoteproc used to be rproc_get, rproc_get_by_name (these were dropped sometime back) and rproc_get_by_phandle, I still like of_rproc_get_by_index, of_rproc_get_by_name, of_rproc_get and then the counter part is the regular rproc_put(). This patch also needs one correction, the inner loop string check should be ti,rproc and not ti,rprocs. regards Suman
On 08/11/2016 11:04 AM, Suman Anna wrote: > Hi Lee, > > On 08/11/2016 02:31 AM, Lee Jones wrote: >> On Wed, 10 Aug 2016, Suman Anna wrote: >> >>> On 08/10/2016 04:19 PM, Bjorn Andersson wrote: >>>> On Wed 10 Aug 14:04 PDT 2016, Suman Anna wrote: >>>> >>>>> On 08/10/2016 03:40 PM, Bjorn Andersson wrote: >>>>>> On Wed 10 Aug 12:37 PDT 2016, Suman Anna wrote: >>>>>> >>>>>>> Hi Lee, Bjorn, >>>>>>> >>>>>>> On 08/10/2016 12:40 PM, Bjorn Andersson wrote: >>>>>>>> On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote: >>>>>>>> >>>>>>>>> - of_rproc_by_index(): look-up and obtain a reference to a rproc >>>>>>>>> using the DT phandle "rprocs" and a index. >>>>>>>>> >>>>>>>>> - of_rproc_by_name(): 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'm happy with this, so I whipped up a binding document describing our >>>>>>>> two new properties. Waiting for an opinion on that before I merge this. >>>>>>> >>>>>>> Yeah, I like the two new API too, I have just about run into the need >>>>>>> for the same on my product trees. We can probably make it less >>>>>>> complicated than what the current series is. The wkup_m3_ipc usage was >>>>>>> before there was any standardization on the property names, so we went >>>>>>> with a ti, prefixed property specific to the wkup_m3_ipc node and a >>>>>>> general API that is agnostic of property name. We don't have all the >>>>>>> pieces for PM on AM335x/AM437x SoCs on upstream kernel yet, so we should >>>>>>> be able to switch over to using the new property as we cannot achieve >>>>>>> the desired functionality even though we can boot the wkup_m3. >>>>>>> >>>>>> >>>>>> Glad to hear you're onboard with dropping the old property name, even if >>>>>> it's later. >>>>>> >>>>>>> Here's what I propose: >>>>>>> 1. Let's not burden the new of_get_rproc_by_index() API with having to >>>>>>> fall-back and look for ti,rprocs. The rproc_get_by_phandle() core logic >>>>>>> of looking up against the rproc list is self-contained, and the API >>>>>>> usage is limited to just the wkup_m3_ipc driver at the moment. >>>>>>> 2. Keep the rproc_get_by_phandle API as is but mark it as deprecated. >>>>>>> IMHO, the rename of this API to of_get_rproc_by_phandle() in Patch 2 but >>>>>>> using a device node pointer as input argument doesn't add any value. >>>>>>> 3. Provide a fallback in wkup_m3_ipc driver to look for both rprocs and >>>>>>> ti,rproc property, while switching over the node to use rprocs property. >>>>>>> 4. We can get rid of the rproc_get_by_phandle() API after the >>>>>>> wkup_m3_ipc transition is done to use of_get_rproc_by_index() API. >>>>>>> >>>>>> >>>>>> I don't fancy the idea of leaving the kernel builds with a deprecation >>>>>> warning for some time and I don't feel the cost of carrying those 2 >>>>>> extra statements is a bigger issue than keeping a deprecated API around. >>>>>> And it will be less than the dance we have to do in wkup_m3_ipc. >>>>>> >>>>>> So I think that we should merge these patches and if it can be concluded >>>>>> that we don't need backwards compatibility with the old DT property we >>>>>> can drop the inner conditional in the API. >>>>> >>>>> Yeah, I am fine with dropping the inner ti,rproc processing in the new >>>>> of_rproc_get_by_index() API and keeping it clean. What's not clear to me >>>>> is why we would still need a get_by_phandle API alongside the two new >>>>> API once the wkup_m3_ipc is converted to the new API? Or is it just to >>>>> clean up the consumer interface? If latter, I will fixup the wkup_m3_ipc >>>>> driver without the need for remoteproc core changes, and switch over to >>>>> the new API. >>>>> >>>> >>>> of_get_rproc_by_phandle() is just a convenience for not having to get by >>>> index 0, as most drivers is only having one rproc. >>> >>> OK, then better to name this as simply of_rproc_get(), the _by_phandle() >>> does not match up with the other two of_get_rproc_xxx API. >> >> I'm not opposed to changing the call to of_rproc_get(). Also, since this is gonna be a simple helper, can we make this a static inline and move to the remoteproc.h header file. However, I'm >> not sure what you mean about it not matching the other OF functions. > > The _by_name and _by_index API take in a name and index arguments, and > the rproc_get_by_phandle took in a phandle argument, the modified name > in patch 2 retains the by_phandle but without any corresponding > argument. That's what I meant by mismatch. > >> The nomenclature should be the same of_get_rproc_*(), no? >> >>> Suggest also a rename from of_get_rproc_xxx to of_rproc_get_xxx like in >>> other subsystems. >> >> This suggestion does the opposite and does not fit in with the >> majority of the of_* calls scattered around in other subsystems: >> >> of_get_drm >> of_get_coresight >> of_get_fb >> of_get_i2c >> of_get_regulator >> of_get_gpio >> of_get_mac >> of_get_display >> of_get_videomode >> >> Vs >> >> of_irq_get >> of_reset_get >> of_graph_get >> of_msi_get >> of_usb_get >> of_genpd_get >> >> These guys have both oddly. >> >> of_get_dma, of_dma_get >> of_get_pci, of_pci_get >> of_get_phy, of_phy_get > > Hmm, looks like there are all sorts of combinations with some like > <subsys>_of_get_xxx. I did search using get_name and get_index, and > nothing popped up differently. The ones that typical rproc drivers deal > with are clk, reset, irq which follow the latter convention. Given that > the previous API within remoteproc used to be rproc_get, > rproc_get_by_name (these were dropped sometime back) and > rproc_get_by_phandle, I still like of_rproc_get_by_index, > of_rproc_get_by_name, of_rproc_get and then the counter part is the > regular rproc_put(). > > This patch also needs one correction, the inner loop string check should > be ti,rproc and not ti,rprocs. > > regards > Suman > > -- > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Wed 10 Aug 15:44 PDT 2016, Suman Anna wrote: > On 08/10/2016 04:19 PM, Bjorn Andersson wrote: > > On Wed 10 Aug 14:04 PDT 2016, Suman Anna wrote: > > [..] > > As far as cleaning up wkup_m3_ipc, patch 2 does that and cleans out the > > old implementation and with that wkup_m3_ipc is moved to the new API. > > > > So the only issue is that the wkup_m3_ipc DT binding states that the > > property should be named "ti,rproc" and as such someone has to send a > > patch to that and make an argument that we don't have to maintain > > backwards compatibility. But as this is used in am33xx.dtsi and > > am4372.dtsi that might be too late? > > I could fix up the wkup_m3_ipc driver using of_update_property() / > of_remove_property(), but looks like these are not exported. I am not > sure if there are any specific reasons as to why these were not > exported. So, for now, we may have to go with the current code. > Sorry I didn't think that far, that would be the preferred solution. Regards, Bjorn
Hi Bjorn, >> On 08/11/2016 02:31 AM, Lee Jones wrote: >>> On Wed, 10 Aug 2016, Suman Anna wrote: >>> >>>> On 08/10/2016 04:19 PM, Bjorn Andersson wrote: >>>>> On Wed 10 Aug 14:04 PDT 2016, Suman Anna wrote: >>>>> >>>>>> On 08/10/2016 03:40 PM, Bjorn Andersson wrote: >>>>>>> On Wed 10 Aug 12:37 PDT 2016, Suman Anna wrote: >>>>>>> >>>>>>>> Hi Lee, Bjorn, >>>>>>>> >>>>>>>> On 08/10/2016 12:40 PM, Bjorn Andersson wrote: >>>>>>>>> On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote: >>>>>>>>> >>>>>>>>>> - of_rproc_by_index(): look-up and obtain a reference to a rproc >>>>>>>>>> using the DT phandle "rprocs" and a index. >>>>>>>>>> >>>>>>>>>> - of_rproc_by_name(): 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'm happy with this, so I whipped up a binding document describing our >>>>>>>>> two new properties. Waiting for an opinion on that before I merge this. One last comment on this is the return code convention change on these rproc_get APIs. I am fine in general with returning ERR_PTRs, but most of the remoteproc code is using NULL checking for rproc. If you remember the discussion back during the hwspinlock DT conversion [1], Ohad preferred to return NULL, and that's why even the rproc_get_by_phandle was returning NULL. We ought to make this consistent across the board if we want to make this switch. regards Suman [1] http://marc.info/?t=138965891200008
On Fri 12 Aug 09:37 PDT 2016, Suman Anna wrote: > Hi Bjorn, > > >> On 08/11/2016 02:31 AM, Lee Jones wrote: > >>> On Wed, 10 Aug 2016, Suman Anna wrote: > >>> > >>>> On 08/10/2016 04:19 PM, Bjorn Andersson wrote: > >>>>> On Wed 10 Aug 14:04 PDT 2016, Suman Anna wrote: > >>>>> > >>>>>> On 08/10/2016 03:40 PM, Bjorn Andersson wrote: > >>>>>>> On Wed 10 Aug 12:37 PDT 2016, Suman Anna wrote: > >>>>>>> > >>>>>>>> Hi Lee, Bjorn, > >>>>>>>> > >>>>>>>> On 08/10/2016 12:40 PM, Bjorn Andersson wrote: > >>>>>>>>> On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote: > >>>>>>>>> > >>>>>>>>>> - of_rproc_by_index(): look-up and obtain a reference to a rproc > >>>>>>>>>> using the DT phandle "rprocs" and a index. > >>>>>>>>>> > >>>>>>>>>> - of_rproc_by_name(): 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'm happy with this, so I whipped up a binding document describing our > >>>>>>>>> two new properties. Waiting for an opinion on that before I merge this. > > One last comment on this is the return code convention change on these > rproc_get APIs. I am fine in general with returning ERR_PTRs, but most > of the remoteproc code is using NULL checking for rproc. If you remember > the discussion back during the hwspinlock DT conversion [1], Ohad > preferred to return NULL, and that's why even the rproc_get_by_phandle > was returning NULL. We ought to make this consistent across the board if > we want to make this switch. > I think it makes sense to return the actual error from these functions, if nothing else to keep it consistent with other frameworks. The other case I see returning NULL is rproc_alloc(), which is think is analog to kmalloc(), so I think that's fine to keep. Luckily wkup_m3 is the only consumer of this API in the kernel today, so we shouldn't have any issues wrt changing the return value here. > regards > Suman > > [1] http://marc.info/?t=138965891200008 Regards, Bjorn
On 08/12/2016 01:07 PM, Bjorn Andersson wrote: > On Fri 12 Aug 09:37 PDT 2016, Suman Anna wrote: > >> Hi Bjorn, >> >>>> On 08/11/2016 02:31 AM, Lee Jones wrote: >>>>> On Wed, 10 Aug 2016, Suman Anna wrote: >>>>> >>>>>> On 08/10/2016 04:19 PM, Bjorn Andersson wrote: >>>>>>> On Wed 10 Aug 14:04 PDT 2016, Suman Anna wrote: >>>>>>> >>>>>>>> On 08/10/2016 03:40 PM, Bjorn Andersson wrote: >>>>>>>>> On Wed 10 Aug 12:37 PDT 2016, Suman Anna wrote: >>>>>>>>> >>>>>>>>>> Hi Lee, Bjorn, >>>>>>>>>> >>>>>>>>>> On 08/10/2016 12:40 PM, Bjorn Andersson wrote: >>>>>>>>>>> On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote: >>>>>>>>>>> >>>>>>>>>>>> - of_rproc_by_index(): look-up and obtain a reference to a rproc >>>>>>>>>>>> using the DT phandle "rprocs" and a index. >>>>>>>>>>>> >>>>>>>>>>>> - of_rproc_by_name(): 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'm happy with this, so I whipped up a binding document describing our >>>>>>>>>>> two new properties. Waiting for an opinion on that before I merge this. >> >> One last comment on this is the return code convention change on these >> rproc_get APIs. I am fine in general with returning ERR_PTRs, but most >> of the remoteproc code is using NULL checking for rproc. If you remember >> the discussion back during the hwspinlock DT conversion [1], Ohad >> preferred to return NULL, and that's why even the rproc_get_by_phandle >> was returning NULL. We ought to make this consistent across the board if >> we want to make this switch. >> > > I think it makes sense to return the actual error from these functions, > if nothing else to keep it consistent with other frameworks. > > The other case I see returning NULL is rproc_alloc(), which is think is > analog to kmalloc(), so I think that's fine to keep. > > Luckily wkup_m3 is the only consumer of this API in the kernel today, so > we shouldn't have any issues wrt changing the return value here. Yeah, not worried about the consumers, I am talking about the few functions in remoteproc core code that do some checking like in rproc_del(), __rproc_boot() or rproc_report_crash(). We would need to modify these to use IS_ERR_OR_NULL instead of a NULL check atleast. regards Suman > >> regards >> Suman >> >> [1] http://marc.info/?t=138965891200008 > > Regards, > Bjorn >
On Fri, 12 Aug 2016, Suman Anna wrote: > On 08/12/2016 01:07 PM, Bjorn Andersson wrote: > > On Fri 12 Aug 09:37 PDT 2016, Suman Anna wrote: > > > >> Hi Bjorn, > >> > >>>> On 08/11/2016 02:31 AM, Lee Jones wrote: > >>>>> On Wed, 10 Aug 2016, Suman Anna wrote: > >>>>> > >>>>>> On 08/10/2016 04:19 PM, Bjorn Andersson wrote: > >>>>>>> On Wed 10 Aug 14:04 PDT 2016, Suman Anna wrote: > >>>>>>> > >>>>>>>> On 08/10/2016 03:40 PM, Bjorn Andersson wrote: > >>>>>>>>> On Wed 10 Aug 12:37 PDT 2016, Suman Anna wrote: > >>>>>>>>> > >>>>>>>>>> Hi Lee, Bjorn, > >>>>>>>>>> > >>>>>>>>>> On 08/10/2016 12:40 PM, Bjorn Andersson wrote: > >>>>>>>>>>> On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote: > >>>>>>>>>>> > >>>>>>>>>>>> - of_rproc_by_index(): look-up and obtain a reference to a rproc > >>>>>>>>>>>> using the DT phandle "rprocs" and a index. > >>>>>>>>>>>> > >>>>>>>>>>>> - of_rproc_by_name(): 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'm happy with this, so I whipped up a binding document describing our > >>>>>>>>>>> two new properties. Waiting for an opinion on that before I merge this. > >> > >> One last comment on this is the return code convention change on these > >> rproc_get APIs. I am fine in general with returning ERR_PTRs, but most > >> of the remoteproc code is using NULL checking for rproc. If you remember > >> the discussion back during the hwspinlock DT conversion [1], Ohad > >> preferred to return NULL, and that's why even the rproc_get_by_phandle > >> was returning NULL. We ought to make this consistent across the board if > >> we want to make this switch. IMHO it's always better to use the appropriate Linux error code in the event of a failure and if it's available. Returning NULL doesn't actually tell us anything about the failure. This will also come in handy when we start playing around with resource tables. For instance, NULL will insinuate "no table supplied", which is valid and execution should continue. Where as a positive IS_ERR() insinuates that something went wrong and execution should be cut short and an error value returned to the caller. > > I think it makes sense to return the actual error from these functions, > > if nothing else to keep it consistent with other frameworks. > > > > The other case I see returning NULL is rproc_alloc(), which is think is > > analog to kmalloc(), so I think that's fine to keep. > > > > Luckily wkup_m3 is the only consumer of this API in the kernel today, so > > we shouldn't have any issues wrt changing the return value here. > > Yeah, not worried about the consumers, I am talking about the few > functions in remoteproc core code that do some checking like in > rproc_del(), __rproc_boot() or rproc_report_crash(). We would need to > modify these to use IS_ERR_OR_NULL instead of a NULL check atleast. I haven't looked at the code, but +1 in principle. > >> [1] http://marc.info/?t=138965891200008 > > > > Regards, > > Bjorn > > >
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index db3958b..6a0d158 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -41,6 +41,8 @@ #include <linux/virtio_ids.h> #include <linux/virtio_ring.h> #include <asm/byteorder.h> +#include <linux/of.h> +#include <linux/of_platform.h> #include "remoteproc_internal.h" @@ -1191,6 +1193,81 @@ out: } EXPORT_SYMBOL(rproc_shutdown); +#ifdef CONFIG_OF +/** + * of_get_rproc_by_index() - lookup and obtain a reference to an rproc + * @np: node to search for rproc phandle + * @index: index into the phandle list + * + * This function increments the remote processor's refcount, so always + * use rproc_put() to decrement it back once rproc isn't needed anymore. + * + * Returns a pointer to the rproc struct on success or an appropriate error + * code otherwise. + */ +struct rproc *of_get_rproc_by_index(struct device_node *np, int index) +{ + struct rproc *rproc = NULL, *r; + struct device_node *rproc_np; + + if (index < 0) { + pr_err("Invalid index: %d\n", index); + return ERR_PTR(-EINVAL); + } + + rproc_np = of_parse_phandle(np, "rprocs", index); + if (!rproc_np) { + /* Unfortunately we have to support this, at least for now */ + rproc_np = of_parse_phandle(np, "ti,rprocs", index); + if (!rproc_np) { + pr_err("Failed to obtain phandle\n"); + return ERR_PTR(-ENODEV); + } + } + + mutex_lock(&rproc_list_mutex); + list_for_each_entry(r, &rproc_list, node) { + if (r->dev.parent && r->dev.parent->of_node == rproc_np) { + get_device(&r->dev); + rproc = r; + break; + } + } + mutex_unlock(&rproc_list_mutex); + + of_node_put(rproc_np); + + if (!rproc) + pr_err("Could not find rproc, deferring\n"); + + return rproc ?: ERR_PTR(-EPROBE_DEFER); +} +EXPORT_SYMBOL(of_get_rproc_by_index); + +/** + * of_get_rproc_by_name() - 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 + * + * This function increments the remote processor's refcount, so always + * use rproc_put() to decrement it back once rproc isn't needed anymore. + * + * Returns a pointer to the rproc struct on success or an appropriate error + * code otherwise. + */ +struct rproc *of_get_rproc_by_name(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_get_rproc_by_index(np, index); +} +EXPORT_SYMBOL(of_get_rproc_by_name); + /** * rproc_get_by_phandle() - find a remote processor by phandle * @phandle: phandle to the rproc @@ -1203,7 +1280,6 @@ EXPORT_SYMBOL(rproc_shutdown); * * Returns the rproc handle on success, and NULL on failure. */ -#ifdef CONFIG_OF struct rproc *rproc_get_by_phandle(phandle phandle) { struct rproc *rproc = NULL, *r; diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index 1c457a8..f130938 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -487,7 +487,6 @@ struct rproc_vdev { u32 rsc_offset; }; -struct rproc *rproc_get_by_phandle(phandle phandle); struct rproc *rproc_alloc(struct device *dev, const char *name, const struct rproc_ops *ops, const char *firmware, int len); @@ -511,4 +510,28 @@ static inline struct rproc *vdev_to_rproc(struct virtio_device *vdev) return rvdev->rproc; } +#ifdef CONFIG_OF +extern struct rproc *of_get_rproc_by_index(struct device_node *np, + int index); +extern struct rproc *of_get_rproc_by_name(struct device_node *np, + const char *name); +extern struct rproc *rproc_get_by_phandle(phandle phandle); +#else +static inline +struct rproc *of_get_rproc_by_index(struct device_node *np, int index) +{ + return NULL; +} +static inline +struct rproc *of_get_rproc_by_name(struct device_node *np, const char *name) +{ + return NULL; +} +static inline +struct rproc *rproc_get_by_phandle(phandle phandle) +{ + return NULL; +} +#endif /* CONFIG_OF */ + #endif /* REMOTEPROC_H */
- of_rproc_by_index(): look-up and obtain a reference to a rproc using the DT phandle "rprocs" and a index. - of_rproc_by_name(): 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 | 78 +++++++++++++++++++++++++++++++++++- include/linux/remoteproc.h | 25 +++++++++++- 2 files changed, 101 insertions(+), 2 deletions(-)