| Message ID | 1344422117-20707-1-git-send-email-vaibhav.bedia@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Vaibhav, On Wed, 8 Aug 2012, Vaibhav Bedia wrote: > If no match is found for a hwmod class, omap_hwmod_for_each_by_class() > currently does not spit out any error messages. To make debugging > issues related to non-existent hwmod classes simpler make this function > print an error message in case no match is found for the supplied hwmod > class. > > Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com> It doesn't seem to me that this would necessarily always be an error. Suppose some init code to create cpsw devices is running on an OMAP3430. Then a omap_hwmod_for_each_by_class() that doesn't locate anything wouldn't be an error, right? - Paul
Hi Paul, On Wed, Aug 08, 2012 at 19:35:50, Paul Walmsley wrote: [...] > It doesn't seem to me that this would necessarily always be an error. > Suppose some init code to create cpsw devices is running on an OMAP3430. > Then a omap_hwmod_for_each_by_class() that doesn't locate anything > wouldn't be an error, right? > Well, I think a failed lookup operation will be a bit expensive since it iterates over the whole list of registered hwmods and hence put in the pr_err(). But I don't see how the SoC specific example that you pointed out could be handled without putting in cpu_is_* checks in the registration process itself :( Would a pr_debug() be ok? Regards, Vaibhav B.
Hi Vaibhav On Wed, 8 Aug 2012, Bedia, Vaibhav wrote: > On Wed, Aug 08, 2012 at 19:35:50, Paul Walmsley wrote: > > > It doesn't seem to me that this would necessarily always be an error. > > Suppose some init code to create cpsw devices is running on an OMAP3430. > > Then a omap_hwmod_for_each_by_class() that doesn't locate anything > > wouldn't be an error, right? > > Well, I think a failed lookup operation will be a bit expensive since it > iterates over the whole list of registered hwmods and hence put in the > pr_err(). If performance is a serious problem there, we can maintain a list of hwmod pointers inside each struct omap_hwmod_class, updated when the hwmods are registered. But as far as I know, performance hasn't been an issue here yet. If performance is the real motivation, I'd rather see an implementation of per-class lists as described above. > But I don't see how the SoC specific example that you pointed out > could be handled without putting in cpu_is_* checks in the registration > process itself :( Hmm, why not? To continue the earlier example, the init code in the platform code simply calls omap_hwmod_for_each_by_class(), and creates the appropriate device if anything was found. If no cpsw IP blocks are present on a given SoC, nothing would happen. No cpu_is_*() would be needed. - Paul
Hi Paul, On Thu, Aug 09, 2012 at 01:49:34, Paul Walmsley wrote: > Hi Vaibhav > > On Wed, 8 Aug 2012, Bedia, Vaibhav wrote: > > > On Wed, Aug 08, 2012 at 19:35:50, Paul Walmsley wrote: > > > > > It doesn't seem to me that this would necessarily always be an error. > > > Suppose some init code to create cpsw devices is running on an OMAP3430. > > > Then a omap_hwmod_for_each_by_class() that doesn't locate anything > > > wouldn't be an error, right? > > > > Well, I think a failed lookup operation will be a bit expensive since it > > iterates over the whole list of registered hwmods and hence put in the > > pr_err(). > > If performance is a serious problem there, we can maintain a list of hwmod > pointers inside each struct omap_hwmod_class, updated when the hwmods are > registered. But as far as I know, performance hasn't been an issue here > yet. If performance is the real motivation, I'd rather see an > implementation of per-class lists as described above. > Ok. I'll try to do some profiling using a timer to see how much time we spend over here. On AM335x there are 2 such lookups, one for the dma and one for the mcbsp. I could get this done today but will get to it tomorrow. Based on how this goes I'll try out the omap_hwmod_class list like you suggested. > > But I don't see how the SoC specific example that you pointed out > > could be handled without putting in cpu_is_* checks in the registration > > process itself :( > > Hmm, why not? > > To continue the earlier example, the init code in the platform code simply > calls omap_hwmod_for_each_by_class(), and creates the appropriate device > if anything was found. If no cpsw IP blocks are present on a given SoC, > nothing would happen. No cpu_is_*() would be needed. > > I was thinking of avoiding the lookup process itself and hence mentioned the need for cpu checks surrounding the platform init code. The current API does help avoid all this in an excellent manner. Regards, Vaibhav B.
Hi Paul, [...] > > > On Wed, Aug 08, 2012 at 19:35:50, Paul Walmsley wrote: > > > > > > > It doesn't seem to me that this would necessarily always be an error. > > > > Suppose some init code to create cpsw devices is running on an OMAP3430. > > > > Then a omap_hwmod_for_each_by_class() that doesn't locate anything > > > > wouldn't be an error, right? > > > > > > Well, I think a failed lookup operation will be a bit expensive since it > > > iterates over the whole list of registered hwmods and hence put in the > > > pr_err(). > > > > If performance is a serious problem there, we can maintain a list of hwmod > > pointers inside each struct omap_hwmod_class, updated when the hwmods are > > registered. But as far as I know, performance hasn't been an issue here > > yet. If performance is the real motivation, I'd rather see an > > implementation of per-class lists as described above. > > > > Ok. I'll try to do some profiling using a timer to see how much time we > spend over here. On AM335x there are 2 such lookups, one for the dma > and one for the mcbsp. I could get this done today but will get to it > tomorrow. Based on how this goes I'll try out the omap_hwmod_class list > like you suggested. > I used a dmtimer clocked at 24MHz (41ns resolution) to get the time spent in the lookup. There are two failed lookups that happen in AM335x and following are the times taken Run1 Lookup for dma - 435 cycles -> 17.835 usecs Lookup for mcbsp - 83 cycles -> 3.4 usecs Run2 Lookup for dma - 503 cycles -> 20.623 usecs Lookup for mcbsp - 90 cycles -> 3.69 usescs Run3 Lookup for dma - 375 cycles -> 15.375 usecs Lookup for mcbsp - 91 cycles -> 3.731 usecs The 2nd lookup happens immediately after the 1st one so probably the difference is coming due to the data sitting in the cache? The average times don't look too high to me but I guess you already knew that ;) If you are ok with a pr_debug() instead of a pr_err() I'll resubmit the patch with this change. Regards, Vaibhav B.
Hello Vaibhav On Fri, 10 Aug 2012, Bedia, Vaibhav wrote: > > Ok. I'll try to do some profiling using a timer to see how much time we > > spend over here. On AM335x there are 2 such lookups, one for the dma > > and one for the mcbsp. I could get this done today but will get to it > > tomorrow. Based on how this goes I'll try out the omap_hwmod_class list > > like you suggested. > > I used a dmtimer clocked at 24MHz (41ns resolution) to get the time > spent in the lookup. There are two failed lookups that happen in AM335x > and following are the times taken > > Run1 > Lookup for dma - 435 cycles -> 17.835 usecs > Lookup for mcbsp - 83 cycles -> 3.4 usecs > > Run2 > Lookup for dma - 503 cycles -> 20.623 usecs > Lookup for mcbsp - 90 cycles -> 3.69 usescs > > Run3 > Lookup for dma - 375 cycles -> 15.375 usecs > Lookup for mcbsp - 91 cycles -> 3.731 usecs > > The 2nd lookup happens immediately after the 1st one so probably > the difference is coming due to the data sitting in the cache? > > The average times don't look too high to me but I guess you already > knew that ;) > > If you are ok with a pr_debug() instead of a pr_err() I'll resubmit > the patch with this change. Well I'd much rather see a patch that maintains a list of hwmods pertaining to each class. That would solve the performance issue at fairly minimal cost (especially if those lists were freed after kernel init). It sounds like the basic difference in opinion is about whether an omap_hwmod_for_each_by_class() that doesn't find anything is an error condition or not. My sense is that it is not. - Paul
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c index 6ca8e51..90e2306 100644 --- a/arch/arm/mach-omap2/omap_hwmod.c +++ b/arch/arm/mach-omap2/omap_hwmod.c @@ -3480,7 +3480,8 @@ int omap_hwmod_read_hardreset(struct omap_hwmod *oh, const char *name) * If the callback function returns something other than * zero, the iterator is terminated, and the callback function's return * value is passed back to the caller. Returns 0 upon success, -EINVAL - * if @classname or @fn are NULL, or passes back the error code from @fn. + * if @classname or @fn are NULL or no match is found for @classname, + * or passes back the error code from @fn. */ int omap_hwmod_for_each_by_class(const char *classname, int (*fn)(struct omap_hwmod *oh, @@ -3489,6 +3490,7 @@ int omap_hwmod_for_each_by_class(const char *classname, { struct omap_hwmod *temp_oh; int ret = 0; + bool found = false; if (!classname || !fn) return -EINVAL; @@ -3498,6 +3500,7 @@ int omap_hwmod_for_each_by_class(const char *classname, list_for_each_entry(temp_oh, &omap_hwmod_list, node) { if (!strcmp(temp_oh->class->name, classname)) { + found = true; pr_debug("omap_hwmod: %s: %s: calling callback fn\n", __func__, temp_oh->name); ret = (*fn)(temp_oh, user); @@ -3506,6 +3509,12 @@ int omap_hwmod_for_each_by_class(const char *classname, } } + if (!found) { + pr_err("omap_hwmod: %s: no match for class %s\n", + __func__, classname); + return -EINVAL; + } + if (ret) pr_debug("omap_hwmod: %s: iterator terminated early: %d\n", __func__, ret);
If no match is found for a hwmod class, omap_hwmod_for_each_by_class() currently does not spit out any error messages. To make debugging issues related to non-existent hwmod classes simpler make this function print an error message in case no match is found for the supplied hwmod class. Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com> --- arch/arm/mach-omap2/omap_hwmod.c | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-)