| Message ID | 20190430144412.20950-1-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] clk: Ensure new parent is looked up when changing parents | expand |
Quoting Charles Keepax (2019-04-30 07:44:11) > clk_core_fill_parent_index is called from clk_mux_determine_rate_flags > and for the initial parent of the clock but seems to not get called on > the path changing a clocks parent. This can cause a clock parent change > to fail, fix this by adding a call in clk_fetch_parent_index. > > Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names") > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > --- > drivers/clk/clk.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index ffd33b63c37eb..5aa180180ee95 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1601,6 +1601,9 @@ static int clk_fetch_parent_index(struct clk_core *core, > return -EINVAL; > > for (i = 0; i < core->num_parents; i++) { > + if (!core->parents[i].core) > + clk_core_fill_parent_index(core, i); > + Hm... are you not specifying 'names' for the parent, so just clk_hw pointer? Maybe we need to compare clk_hw pointers with clk_hw pointers and then fill in the core pointer with what we have in hand. Pretty much at all costs we shouldn't call clk_core_fill_parent_index() here because drivers may fall into the trap of searching the entire clk tree for a pointer we already have. diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 114f0bffd630..c4fa341330fa 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1619,6 +1619,11 @@ static int clk_fetch_parent_index(struct clk_core *core, if (core->parents[i].core) continue; + if (core->parents[i].hw == parent->hw) { + core->parents[i].core = parent; + return i; + } + /* Fallback to comparing globally unique names */ if (!strcmp(parent->name, core->parents[i].name)) { core->parents[i].core = parent;
On Tue, Apr 30, 2019 at 09:44:49AM -0700, Stephen Boyd wrote: > Quoting Charles Keepax (2019-04-30 07:44:11) > > clk_core_fill_parent_index is called from clk_mux_determine_rate_flags > > and for the initial parent of the clock but seems to not get called on > > the path changing a clocks parent. This can cause a clock parent change > > to fail, fix this by adding a call in clk_fetch_parent_index. > > > > Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names") > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > > --- > > drivers/clk/clk.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index ffd33b63c37eb..5aa180180ee95 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -1601,6 +1601,9 @@ static int clk_fetch_parent_index(struct clk_core *core, > > return -EINVAL; > > > > for (i = 0; i < core->num_parents; i++) { > > + if (!core->parents[i].core) > > + clk_core_fill_parent_index(core, i); > > + > > Hm... are you not specifying 'names' for the parent, so just clk_hw > pointer? Maybe we need to compare clk_hw pointers with clk_hw pointers > and then fill in the core pointer with what we have in hand. Pretty much > at all costs we shouldn't call clk_core_fill_parent_index() here because > drivers may fall into the trap of searching the entire clk tree for a > pointer we already have. > Apologies perhaps I am misunderstanding how this new system works. In the event of the parent clocks being specified in DT, whilst going round this loop would you expect the clock to match on the core == parent check? Or on the fallback unique name check? My assumption was on the core == parent check, and if that is the case how would you expect the parents[i].core field to have been populated? Thanks, Charles
Quoting Charles Keepax (2019-05-01 01:33:17) > On Tue, Apr 30, 2019 at 09:44:49AM -0700, Stephen Boyd wrote: > > Quoting Charles Keepax (2019-04-30 07:44:11) > > > clk_core_fill_parent_index is called from clk_mux_determine_rate_flags > > > and for the initial parent of the clock but seems to not get called on > > > the path changing a clocks parent. This can cause a clock parent change > > > to fail, fix this by adding a call in clk_fetch_parent_index. > > > > > > Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names") > > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> > > > --- > > > drivers/clk/clk.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > index ffd33b63c37eb..5aa180180ee95 100644 > > > --- a/drivers/clk/clk.c > > > +++ b/drivers/clk/clk.c > > > @@ -1601,6 +1601,9 @@ static int clk_fetch_parent_index(struct clk_core *core, > > > return -EINVAL; > > > > > > for (i = 0; i < core->num_parents; i++) { > > > + if (!core->parents[i].core) > > > + clk_core_fill_parent_index(core, i); > > > + > > > > Hm... are you not specifying 'names' for the parent, so just clk_hw > > pointer? Maybe we need to compare clk_hw pointers with clk_hw pointers > > and then fill in the core pointer with what we have in hand. Pretty much > > at all costs we shouldn't call clk_core_fill_parent_index() here because > > drivers may fall into the trap of searching the entire clk tree for a > > pointer we already have. > > > > Apologies perhaps I am misunderstanding how this new system > works. In the event of the parent clocks being specified in > DT, whilst going round this loop would you expect the clock to > match on the core == parent check? Or on the fallback unique > name check? My assumption was on the core == parent check, and > if that is the case how would you expect the parents[i].core > field to have been populated? > I don't expect parents[i].core to be populated until we do the fallback string match in this function when the globally unique names match or if it's already populated by some other path calling clk_core_fill_parent_index(). The problem is we just fixed a long standing regression in this function with commit ede77858473a ("clk: Remove global clk traversal on fetch parent index"). Calling clk_core_fill_parent_index() will bring that performance problem back, so we need to figure out how to find the index for a clk without doing the global search. It seems that some clk providers specify parents that may never exist, so we'll possibly spend time looping through the parents each time doing a global string comparison on hundreds of clks. It would be best to avoid that, so we shouldn't really do any sort of caching here except for the one clk_core pointer we already have passed in. So you're saying this happens in the clk_set_parent() path, where the parent passed into this function has never been cached before? Otherwise, it looks like the only other case where it might be a problem would be if a .round_rate() or .determine_rate() clk_op returns a parent that hasn't been cached yet, maybe via hard-coding of the parent hw pointer or just because they don't call clk_hw_get_parent_by_index(). I'd rather not cater to those corner cases by trying to optimize them, so perhaps we can use this patch and avoid the regression while supporting the use-cases we need? ----8<----- diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index dc05cb339761..7ad1751bf2b4 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -327,8 +327,7 @@ static struct clk_core *clk_core_lookup(const char *name) /** * clk_core_get - Find the clk_core parent of a clk * @core: clk to find parent of - * @name: name to search for (if string based) - * @index: index to use for search (if DT index based) + * @p_index: parent index to search for * * This is the preferred method for clk providers to find the parent of a * clk when that parent is external to the clk controller. The parent_names @@ -360,9 +359,10 @@ static struct clk_core *clk_core_lookup(const char *name) * provider knows about the clk but it isn't provided on this system. * A valid clk_core pointer when the clk can be found in the provider. */ -static struct clk_core *clk_core_get(struct clk_core *core, const char *name, - int index) +static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index) { + const char *name = core->parents[p_index].fw_name; + u8 index = core->parents[p_index].index; struct clk_hw *hw = ERR_PTR(-ENOENT); struct device *dev = core->dev; const char *dev_id = dev ? dev_name(dev) : NULL; @@ -400,7 +400,7 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index) if (!parent) parent = ERR_PTR(-EPROBE_DEFER); } else { - parent = clk_core_get(core, entry->fw_name, entry->index); + parent = clk_core_get(core, index); if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT) parent = clk_core_lookup(entry->name); } @@ -1612,12 +1612,29 @@ static int clk_fetch_parent_index(struct clk_core *core, return -EINVAL; for (i = 0; i < core->num_parents; i++) { + /* Found it first try! */ if (core->parents[i].core == parent) return i; + /* Something else is here, so keep looking */ if (core->parents[i].core) continue; + /* Maybe core hasn't been cached but the hw is all we know? */ + if (core->parents[i].hw) { + if (core->parents[i].hw == parent->hw) { + core->parents[i].core = parent; + return i; + } + + /* Didn't match, but we're expecting a clk_hw */ + continue; + } + + /* Maybe it hasn't been cached (clk_set_parent() path) */ + if (parent == clk_core_get(core, i)) + return i; + /* Fallback to comparing globally unique names */ if (!strcmp(parent->name, core->parents[i].name)) { core->parents[i].core = parent;
On Wed, May 01, 2019 at 12:59:48PM -0700, Stephen Boyd wrote: > Quoting Charles Keepax (2019-05-01 01:33:17) > > On Tue, Apr 30, 2019 at 09:44:49AM -0700, Stephen Boyd wrote: > > > Quoting Charles Keepax (2019-04-30 07:44:11) Thank you for the explanation think I am starting to get there. > I don't expect parents[i].core to be populated until we do the fallback > string match in this function when the globally unique names match or if > it's already populated by some other path calling > clk_core_fill_parent_index(). The problem is we just fixed a long > standing regression in this function with commit ede77858473a ("clk: > Remove global clk traversal on fetch parent index"). Calling > clk_core_fill_parent_index() will bring that performance problem back, > so we need to figure out how to find the index for a clk without doing > the global search. > There is a slight error in the commit message there I think, the change doesn't affect clk_mux_determine_rate_flags which still calls clk_core_get_parent_by_index so will still do the full lookup. It looks like it affects clk_calc_new_rates instead. Which I guess does raise the question would an optimisation on the determine_rate path help these power issues as well? > It seems that some clk providers specify parents that may never exist, > so we'll possibly spend time looping through the parents each time doing > a global string comparison on hundreds of clks. It would be best to > avoid that, so we shouldn't really do any sort of caching here except > for the one clk_core pointer we already have passed in. > > So you're saying this happens in the clk_set_parent() path, where the > parent passed into this function has never been cached before? Yeah that seems to be what is happening in my case. As best I can figure out so far, this relates to our clocks not having any rate setting capabilities, as it looks like most of the cache population comes from those paths. > + > + /* Maybe it hasn't been cached (clk_set_parent() path) */ > + if (parent == clk_core_get(core, i)) > + return i; This part does fix my issue. Is there a reason not to update core->parents[i].core on this path? Thanks, Charles
Quoting Charles Keepax (2019-05-03 09:33:22) > On Wed, May 01, 2019 at 12:59:48PM -0700, Stephen Boyd wrote: > > Quoting Charles Keepax (2019-05-01 01:33:17) > > > On Tue, Apr 30, 2019 at 09:44:49AM -0700, Stephen Boyd wrote: > > > > Quoting Charles Keepax (2019-04-30 07:44:11) > > Thank you for the explanation think I am starting to get there. > > > I don't expect parents[i].core to be populated until we do the fallback > > string match in this function when the globally unique names match or if > > it's already populated by some other path calling > > clk_core_fill_parent_index(). The problem is we just fixed a long > > standing regression in this function with commit ede77858473a ("clk: > > Remove global clk traversal on fetch parent index"). Calling > > clk_core_fill_parent_index() will bring that performance problem back, > > so we need to figure out how to find the index for a clk without doing > > the global search. > > > > There is a slight error in the commit message there I think, the > change doesn't affect clk_mux_determine_rate_flags which still > calls clk_core_get_parent_by_index so will still do the full > lookup. It looks like it affects clk_calc_new_rates instead. > > Which I guess does raise the question would an optimisation on > the determine_rate path help these power issues as well? Yes I would think that determine_rate() is iterating over parents and trying to find them again and again even when there are holes and that's probably wasting time. I hope we can fill the parent cache of a clk with the error pointer indicating the clk will never appear (-ENOENT?) and then skip over it in this case. > > > It seems that some clk providers specify parents that may never exist, > > so we'll possibly spend time looping through the parents each time doing > > a global string comparison on hundreds of clks. It would be best to > > avoid that, so we shouldn't really do any sort of caching here except > > for the one clk_core pointer we already have passed in. > > > > So you're saying this happens in the clk_set_parent() path, where the > > parent passed into this function has never been cached before? > > Yeah that seems to be what is happening in my case. As best I > can figure out so far, this relates to our clocks not having > any rate setting capabilities, as it looks like most of the > cache population comes from those paths. Makes sense! > > > + > > + /* Maybe it hasn't been cached (clk_set_parent() path) */ > > + if (parent == clk_core_get(core, i)) > > + return i; > > This part does fix my issue. Is there a reason not to update > core->parents[i].core on this path? > Nope. I though clk_core_get() did it already, but it doesn't. Thanks for catching it. Find the new patch attached: ---8<--- Subject: [PATCH] clk: Cache core in clk_fetch_parent_index() without names If a clk has specified parents via clk_hw pointers it won't specify the globally unique names for the parents. Without the unique names, we can't fallback to comparing them against the name of the 'parent' pointer here. Therefore, do a pointer comparison against the clk_hw pointers too and cache the clk_core structure if they match. This fixes parent lookup code for clks that only specify clk_hw pointers and nothing else, like muxes that are purely inside a clk controller. Similarly, if the parent pointer isn't cached after trying to match clk_core or clk_hw pointers, lookup the pointer from DT or via clkdev lookups instead of relying purely on the globally unique clk name match. This should allow us to move away from having to specify global names for clk parents entirely. While we're in the area, add some comments so it's clearer what's going on. The if statements don't lend themselves to much clarity in their raw form. Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names") Reported-by: Charles Keepax <ckeepax@opensource.cirrus.com> Signed-off-by: Stephen Boyd <sboyd@kernel.org> --- drivers/clk/clk.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index dc05cb339761..dcb7e1cddd2d 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -327,8 +327,7 @@ static struct clk_core *clk_core_lookup(const char *name) /** * clk_core_get - Find the clk_core parent of a clk * @core: clk to find parent of - * @name: name to search for (if string based) - * @index: index to use for search (if DT index based) + * @p_index: parent index to search for * * This is the preferred method for clk providers to find the parent of a * clk when that parent is external to the clk controller. The parent_names @@ -360,9 +359,10 @@ static struct clk_core *clk_core_lookup(const char *name) * provider knows about the clk but it isn't provided on this system. * A valid clk_core pointer when the clk can be found in the provider. */ -static struct clk_core *clk_core_get(struct clk_core *core, const char *name, - int index) +static struct clk_core *clk_core_get(struct clk_core *core, u8 p_index) { + const char *name = core->parents[p_index].fw_name; + int index = core->parents[p_index].index; struct clk_hw *hw = ERR_PTR(-ENOENT); struct device *dev = core->dev; const char *dev_id = dev ? dev_name(dev) : NULL; @@ -400,7 +400,7 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index) if (!parent) parent = ERR_PTR(-EPROBE_DEFER); } else { - parent = clk_core_get(core, entry->fw_name, entry->index); + parent = clk_core_get(core, index); if (IS_ERR(parent) && PTR_ERR(parent) == -ENOENT) parent = clk_core_lookup(entry->name); } @@ -1612,20 +1612,37 @@ static int clk_fetch_parent_index(struct clk_core *core, return -EINVAL; for (i = 0; i < core->num_parents; i++) { + /* Found it first try! */ if (core->parents[i].core == parent) return i; + /* Something else is here, so keep looking */ if (core->parents[i].core) continue; - /* Fallback to comparing globally unique names */ - if (!strcmp(parent->name, core->parents[i].name)) { - core->parents[i].core = parent; - return i; + /* Maybe core hasn't been cached but the hw is all we know? */ + if (core->parents[i].hw) { + if (core->parents[i].hw == parent->hw) + break; + + /* Didn't match, but we're expecting a clk_hw */ + continue; } + + /* Maybe it hasn't been cached (clk_set_parent() path) */ + if (parent == clk_core_get(core, i)) + break; + + /* Fallback to comparing globally unique names */ + if (!strcmp(parent->name, core->parents[i].name)) + break; } - return -EINVAL; + if (i == core->num_parents) + return -EINVAL; + + core->parents[i].core = parent; + return i; } /*
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index ffd33b63c37eb..5aa180180ee95 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -1601,6 +1601,9 @@ static int clk_fetch_parent_index(struct clk_core *core, return -EINVAL; for (i = 0; i < core->num_parents; i++) { + if (!core->parents[i].core) + clk_core_fill_parent_index(core, i); + if (core->parents[i].core == parent) return i;
clk_core_fill_parent_index is called from clk_mux_determine_rate_flags and for the initial parent of the clock but seems to not get called on the path changing a clocks parent. This can cause a clock parent change to fail, fix this by adding a call in clk_fetch_parent_index. Fixes: fc0c209c147f ("clk: Allow parents to be specified without string names") Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- drivers/clk/clk.c | 3 +++ 1 file changed, 3 insertions(+)