| Message ID | 20200928084744.32478-1-jun.nie@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: emit warning if fail to get parent clk | expand |
Quoting Jun Nie (2020-09-28 01:47:44) > Emit warning if fail to get parent clk to expose potential issue earlier. > For example, clk_hw_get_rate() will return 0 for a clock without parent core > while parent number is not zero. This cause opp always think it is switching > frequency from 0 to some other frequency. Crash may happen if we switch > from high frequency to low frequency and lower CPU voltage before clk rate > switching. Thanks for the background reasoning. It's good to know what the problem is. Is there any way to change OPP so it can handle this scenario better? > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > --- > drivers/clk/clk.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 1a27e99ccb17..78b21b888e56 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -424,6 +424,7 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index) > { > struct clk_parent_map *entry = &core->parents[index]; > struct clk_core *parent = ERR_PTR(-ENOENT); > + int emit_warn = 0; > > if (entry->hw) { > parent = entry->hw->core; > @@ -443,6 +444,12 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index) > /* Only cache it if it's not an error */ > if (!IS_ERR(parent)) > entry->core = parent; > + else if (parent != ERR_PTR(-EPROBE_DEFER)) > + emit_warn = 1; > + > + if (emit_warn || (!parent && core->num_parents)) > + pr_warn("Fail to get indexed %d parent for clk %s.", > + index, core->name); How do we know that this error isn't because the parent hasn't been probed yet?
Stephen Boyd <sboyd@kernel.org> 于2020年10月8日周四 上午7:53写道： > > Quoting Jun Nie (2020-09-28 01:47:44) > > Emit warning if fail to get parent clk to expose potential issue earlier. > > For example, clk_hw_get_rate() will return 0 for a clock without parent core > > while parent number is not zero. This cause opp always think it is switching > > frequency from 0 to some other frequency. Crash may happen if we switch > > from high frequency to low frequency and lower CPU voltage before clk rate > > switching. > > Thanks for the background reasoning. It's good to know what the problem > is. Is there any way to change OPP so it can handle this scenario > better? Maybe we can save latest CPU voltage and lower the voltage per opp voltage requirement instead of clk rate requirement. > > > > > Signed-off-by: Jun Nie <jun.nie@linaro.org> > > --- > > drivers/clk/clk.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index 1a27e99ccb17..78b21b888e56 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -424,6 +424,7 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index) > > { > > struct clk_parent_map *entry = &core->parents[index]; > > struct clk_core *parent = ERR_PTR(-ENOENT); > > + int emit_warn = 0; > > > > if (entry->hw) { > > parent = entry->hw->core; > > @@ -443,6 +444,12 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index) > > /* Only cache it if it's not an error */ > > if (!IS_ERR(parent)) > > entry->core = parent; > > + else if (parent != ERR_PTR(-EPROBE_DEFER)) > > + emit_warn = 1; > > + > > + if (emit_warn || (!parent && core->num_parents)) > > + pr_warn("Fail to get indexed %d parent for clk %s.", > > + index, core->name); > > How do we know that this error isn't because the parent hasn't been > probed yet? Yes, this is chance that failure to probe parent also cause the error. But I had thought there may be unnecessary error message before the successful probing, though I did not see it. If you think it does not a matter, next version can emit warning on all error. Jun
Quoting Jun Nie (2020-10-08 20:08:13) > Stephen Boyd <sboyd@kernel.org> 于2020年10月8日周四 上午7:53写道： > > > > How do we know that this error isn't because the parent hasn't been > > probed yet? > > Yes, this is chance that failure to probe parent also cause the error. > But I had thought there may > be unnecessary error message before the successful probing, though I > did not see it. If you think > it does not a matter, next version can emit warning on all error. > I suspect this warning can't be emitted here because we don't know if the parent hasn't probed or if something else is wrong. It would be better to fix OPP from what I can tell.
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 1a27e99ccb17..78b21b888e56 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -424,6 +424,7 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index) { struct clk_parent_map *entry = &core->parents[index]; struct clk_core *parent = ERR_PTR(-ENOENT); + int emit_warn = 0; if (entry->hw) { parent = entry->hw->core; @@ -443,6 +444,12 @@ static void clk_core_fill_parent_index(struct clk_core *core, u8 index) /* Only cache it if it's not an error */ if (!IS_ERR(parent)) entry->core = parent; + else if (parent != ERR_PTR(-EPROBE_DEFER)) + emit_warn = 1; + + if (emit_warn || (!parent && core->num_parents)) + pr_warn("Fail to get indexed %d parent for clk %s.", + index, core->name); } static struct clk_core *clk_core_get_parent_by_index(struct clk_core *core,
Emit warning if fail to get parent clk to expose potential issue earlier. For example, clk_hw_get_rate() will return 0 for a clock without parent core while parent number is not zero. This cause opp always think it is switching frequency from 0 to some other frequency. Crash may happen if we switch from high frequency to low frequency and lower CPU voltage before clk rate switching. Signed-off-by: Jun Nie <jun.nie@linaro.org> --- drivers/clk/clk.c | 7 +++++++ 1 file changed, 7 insertions(+)