| Message ID | 1367475929-32166-4-git-send-email-ambresh@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 29 May 2013 12:48 PM, Mike Turquette wrote: > Quoting Ambresh K (2013-05-01 23:25:29) >> From: Ambresh K <ambresh@ti.com> >> >> Add orhan clk nodes having invalid parent index to list and use >> the list to skip re-parenting orphan clk having invalid parents. >> >> Signed-off-by: Ambresh K <ambresh@ti.com> >> --- >> drivers/clk/clk.c | 21 +++++++++++++++++++-- >> 1 files changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index f4d2c73..54d2aa3 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -32,6 +32,7 @@ static int enable_refcnt; >> >> static HLIST_HEAD(clk_root_list); >> static HLIST_HEAD(clk_orphan_list); >> +static HLIST_HEAD(clk_orphan_invalid_parent); > > Why not re-use the clk_orphan_list? Having an invalid value programmed > into the hardware for selecting a parent essetially orphans a clock > from a software point of view. Is there a specific need for the new > list? Sorry for not being descriptive in commit message. a) Avoids unnecessary re-parenting cycle for orphan clock's with invalid parent for every clock b) With patch [1/3], after a clk with invalid parent was encountered, for every clk registered thereafter seeing following logs. <Snippet> [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent Please advice, if can be handled better. Thanks, Ambresh > > Regards, > Mike > >> static LIST_HEAD(clk_notifier_list); >> >> /*** locking ***/ >> @@ -1532,8 +1533,8 @@ EXPORT_SYMBOL_GPL(clk_set_parent); >> */ >> int __clk_init(struct device *dev, struct clk *clk) >> { >> - int i, ret = 0; >> - struct clk *orphan; >> + int i, ret = 0, skip = 0; >> + struct clk *orphan, *has_invalid_parent; >> struct hlist_node *tmp2; >> >> if (!clk) >> @@ -1639,11 +1640,27 @@ int __clk_init(struct device *dev, struct clk *clk) >> if (!strcmp(clk->name, orphan->name)) >> continue; >> >> + /* Skip iteration if orphan has invalid parent */ >> + hlist_for_each_entry(has_invalid_parent, >> + &clk_orphan_invalid_parent, child_node) { >> + if (!strcmp(orphan->name, has_invalid_parent->name)) { >> + skip = 1; >> + break; >> + } >> + } >> + >> + if (skip) { >> + skip = 0; >> + continue; >> + } >> + >> if (orphan->ops->get_parent) { >> i = orphan->ops->get_parent(orphan->hw); >> if (i < 0) { >> pr_err("%s: orphan clk(%s) invalid parent\n", >> __func__, orphan->name); >> + hlist_add_head(&orphan->child_node, >> + &clk_orphan_invalid_parent); >> continue; >> } >> if (!strcmp(clk->name, orphan->parent_names[i])) >> -- >> 1.7.4.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Sorry for not being descriptive in commit message. >> >> a) Avoids unnecessary re-parenting cycle for orphan clock's with invalid parent for every clock >> > > True, but this is a minor optimisation. If this is a big optimization > for you then you really need to fix your bootloader. We shouldn't be > optimizing slow error paths just because we refuse to fix the errors. Got it! Should be fixed in boot-loader. > >> b) With patch [1/3], after a clk with invalid parent was encountered, for every clk registered thereafter seeing following logs. >> <Snippet> >> [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent >> [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent >> [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent >> [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent >> [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent >> [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent >> [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent >> [ 0.000000] __clk_init: orphan clk(gpu_core_gclk_mux) invalid parent >> >> Please advice, if can be handled better. >> > > First off, I don't think we should create new structures to work around > bugs that should be fixed. pr_err_once() will let us know something is > wrong and won't flood the log. Even then I'm inclined to say that > flooding the log is OK and will motivate you to fix up your bootloader. > Error prints are there for a reason. > > Secondly, I spent like 10 minutes looking at this code and I'm still > confused. For a clock with invalid parent programming, are you adding > it to BOTH the orphan list and the has_invalid_parent list? Why? Is > this just avoid the spurious prints? For everyone clock registered we > walk the list of orphans to see if that orphan can be reparented. This > patch adds another nested list walk (likely a short list) for each of > those orphans in the first list walk, so it starts to look like O(n^2). > I don't like it. > > I think the first two patches in the series look good, but unless I am > misunderstanding this patch I feel that it can be dropped entirely. > Thanks for your time! Will drop this patch and send V2 for first 2 patches. Regards, Ambresh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index f4d2c73..54d2aa3 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -32,6 +32,7 @@ static int enable_refcnt; static HLIST_HEAD(clk_root_list); static HLIST_HEAD(clk_orphan_list); +static HLIST_HEAD(clk_orphan_invalid_parent); static LIST_HEAD(clk_notifier_list); /*** locking ***/ @@ -1532,8 +1533,8 @@ EXPORT_SYMBOL_GPL(clk_set_parent); */ int __clk_init(struct device *dev, struct clk *clk) { - int i, ret = 0; - struct clk *orphan; + int i, ret = 0, skip = 0; + struct clk *orphan, *has_invalid_parent; struct hlist_node *tmp2; if (!clk) @@ -1639,11 +1640,27 @@ int __clk_init(struct device *dev, struct clk *clk) if (!strcmp(clk->name, orphan->name)) continue; + /* Skip iteration if orphan has invalid parent */ + hlist_for_each_entry(has_invalid_parent, + &clk_orphan_invalid_parent, child_node) { + if (!strcmp(orphan->name, has_invalid_parent->name)) { + skip = 1; + break; + } + } + + if (skip) { + skip = 0; + continue; + } + if (orphan->ops->get_parent) { i = orphan->ops->get_parent(orphan->hw); if (i < 0) { pr_err("%s: orphan clk(%s) invalid parent\n", __func__, orphan->name); + hlist_add_head(&orphan->child_node, + &clk_orphan_invalid_parent); continue; } if (!strcmp(clk->name, orphan->parent_names[i]))