From f21cf9c77ee82ef8adfeb2143adfacf21ec1d5cc Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Wed, 5 Feb 2020 15:27:59 -0800 Subject: clk: Don't cache errors from clk_ops::get_phase() We don't check for errors from clk_ops::get_phase() before storing away the result into the clk_core::phase member. This can lead to some fairly confusing debugfs information if these ops do return an error. Let's skip the store when this op fails to fix this. While we're here, move the locking outside of clk_core_get_phase() to simplify callers from the debugfs side. Cc: Douglas Anderson Cc: Heiko Stuebner Cc: Jerome Brunet Signed-off-by: Stephen Boyd Link: https://lkml.kernel.org/r/20200205232802.29184-2-sboyd@kernel.org Acked-by: Jerome Brunet --- drivers/clk/clk.c | 48 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 16 deletions(-) (limited to 'drivers/clk/clk.c') diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index f0f2b599fd7e..82d96d4071b1 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -2660,12 +2660,14 @@ static int clk_core_get_phase(struct clk_core *core) { int ret; - clk_prepare_lock(); + lockdep_assert_held(&prepare_lock); + if (!core->ops->get_phase) + return 0; + /* Always try to update cached phase if possible */ - if (core->ops->get_phase) - core->phase = core->ops->get_phase(core->hw); - ret = core->phase; - clk_prepare_unlock(); + ret = core->ops->get_phase(core->hw); + if (ret >= 0) + core->phase = ret; return ret; } @@ -2679,10 +2681,16 @@ static int clk_core_get_phase(struct clk_core *core) */ int clk_get_phase(struct clk *clk) { + int ret; + if (!clk) return 0; - return clk_core_get_phase(clk->core); + clk_prepare_lock(); + ret = clk_core_get_phase(clk->core); + clk_prepare_unlock(); + + return ret; } EXPORT_SYMBOL_GPL(clk_get_phase); @@ -2896,13 +2904,21 @@ static struct hlist_head *orphan_list[] = { static void clk_summary_show_one(struct seq_file *s, struct clk_core *c, int level) { - seq_printf(s, "%*s%-*s %7d %8d %8d %11lu %10lu %5d %6d\n", + int phase; + + seq_printf(s, "%*s%-*s %7d %8d %8d %11lu %10lu ", level * 3 + 1, "", 30 - level * 3, c->name, c->enable_count, c->prepare_count, c->protect_count, - clk_core_get_rate(c), clk_core_get_accuracy(c), - clk_core_get_phase(c), - clk_core_get_scaled_duty_cycle(c, 100000)); + clk_core_get_rate(c), clk_core_get_accuracy(c)); + + phase = clk_core_get_phase(c); + if (phase >= 0) + seq_printf(s, "%5d", phase); + else + seq_puts(s, "-----"); + + seq_printf(s, " %6d\n", clk_core_get_scaled_duty_cycle(c, 100000)); } static void clk_summary_show_subtree(struct seq_file *s, struct clk_core *c, @@ -2939,6 +2955,7 @@ DEFINE_SHOW_ATTRIBUTE(clk_summary); static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level) { + int phase; unsigned long min_rate, max_rate; clk_core_get_boundaries(c, &min_rate, &max_rate); @@ -2952,7 +2969,9 @@ static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level) seq_printf(s, "\"min_rate\": %lu,", min_rate); seq_printf(s, "\"max_rate\": %lu,", max_rate); seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c)); - seq_printf(s, "\"phase\": %d,", clk_core_get_phase(c)); + phase = clk_core_get_phase(c); + if (phase >= 0) + seq_printf(s, "\"phase\": %d,", phase); seq_printf(s, "\"duty_cycle\": %u", clk_core_get_scaled_duty_cycle(c, 100000)); } @@ -3434,14 +3453,11 @@ static int __clk_core_init(struct clk_core *core) core->accuracy = 0; /* - * Set clk's phase. + * Set clk's phase by clk_core_get_phase() caching the phase. * Since a phase is by definition relative to its parent, just * query the current clock phase, or just assume it's in phase. */ - if (core->ops->get_phase) - core->phase = core->ops->get_phase(core->hw); - else - core->phase = 0; + clk_core_get_phase(core); /* * Set clk's duty cycle. -- cgit v1.2.3 From 768a5d4f63c29d3bed5abb3c187312fcf623fa05 Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Wed, 5 Feb 2020 15:28:00 -0800 Subject: clk: Use 'parent' to shorten lines in __clk_core_init() Some lines are getting long in this function. Let's move 'parent' up to the top of the function and use it in many places whenever there is a parent for a clk. This shortens some lines by avoiding core->parent-> indirections. Cc: Douglas Anderson Cc: Heiko Stuebner Cc: Jerome Brunet Signed-off-by: Stephen Boyd Link: https://lkml.kernel.org/r/20200205232802.29184-3-sboyd@kernel.org Acked-by: Jerome Brunet --- drivers/clk/clk.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) (limited to 'drivers/clk/clk.c') diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 82d96d4071b1..3ddca6084e8e 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3342,6 +3342,7 @@ static void clk_core_reparent_orphans_nolock(void) static int __clk_core_init(struct clk_core *core) { int ret; + struct clk_core *parent; unsigned long rate; if (!core) @@ -3413,7 +3414,7 @@ static int __clk_core_init(struct clk_core *core) goto out; } - core->parent = __clk_init_parent(core); + parent = core->parent = __clk_init_parent(core); /* * Populate core->parent if parent has already been clk_core_init'd. If @@ -3425,10 +3426,9 @@ static int __clk_core_init(struct clk_core *core) * clocks and re-parent any that are children of the clock currently * being clk_init'd. */ - if (core->parent) { - hlist_add_head(&core->child_node, - &core->parent->children); - core->orphan = core->parent->orphan; + if (parent) { + hlist_add_head(&core->child_node, &parent->children); + core->orphan = parent->orphan; } else if (!core->num_parents) { hlist_add_head(&core->child_node, &clk_root_list); core->orphan = false; @@ -3446,9 +3446,9 @@ static int __clk_core_init(struct clk_core *core) */ if (core->ops->recalc_accuracy) core->accuracy = core->ops->recalc_accuracy(core->hw, - __clk_get_accuracy(core->parent)); - else if (core->parent) - core->accuracy = core->parent->accuracy; + __clk_get_accuracy(parent)); + else if (parent) + core->accuracy = parent->accuracy; else core->accuracy = 0; @@ -3472,9 +3472,9 @@ static int __clk_core_init(struct clk_core *core) */ if (core->ops->recalc_rate) rate = core->ops->recalc_rate(core->hw, - clk_core_get_rate_nolock(core->parent)); - else if (core->parent) - rate = core->parent->rate; + clk_core_get_rate_nolock(parent)); + else if (parent) + rate = parent->rate; else rate = 0; core->rate = core->req_rate = rate; -- cgit v1.2.3 From 0daa376d832f4ce585f153efee4233b52fa3fe58 Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Wed, 5 Feb 2020 15:28:01 -0800 Subject: clk: Move rate and accuracy recalc to mostly consumer APIs There's some confusion about when recalc is done for the rate and accuracy clk consumer APIs in relation to the prepare lock being taken. Oddly enough, we take the lock again in debugfs APIs so that we can call the internal "clk_core" APIs to get these fields with any necessary recalculations. Instead of having this confusion, let's introduce a recalc variant of these two consumer APIs as internal helpers and call them from the consumer APIs and the debugfs code so that we don't take the lock more than once. Cc: Douglas Anderson Cc: Heiko Stuebner Cc: Jerome Brunet Signed-off-by: Stephen Boyd Link: https://lkml.kernel.org/r/20200205232802.29184-4-sboyd@kernel.org Acked-by: Jerome Brunet --- drivers/clk/clk.c | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-) (limited to 'drivers/clk/clk.c') diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 3ddca6084e8e..dc8bdfbd6a0c 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -488,7 +488,7 @@ unsigned long clk_hw_get_rate(const struct clk_hw *hw) } EXPORT_SYMBOL_GPL(clk_hw_get_rate); -static unsigned long __clk_get_accuracy(struct clk_core *core) +static unsigned long clk_core_get_accuracy_no_lock(struct clk_core *core) { if (!core) return 0; @@ -1517,18 +1517,12 @@ static void __clk_recalc_accuracies(struct clk_core *core) __clk_recalc_accuracies(child); } -static long clk_core_get_accuracy(struct clk_core *core) +static long clk_core_get_accuracy_recalc(struct clk_core *core) { - unsigned long accuracy; - - clk_prepare_lock(); if (core && (core->flags & CLK_GET_ACCURACY_NOCACHE)) __clk_recalc_accuracies(core); - accuracy = __clk_get_accuracy(core); - clk_prepare_unlock(); - - return accuracy; + return clk_core_get_accuracy_no_lock(core); } /** @@ -1542,10 +1536,16 @@ static long clk_core_get_accuracy(struct clk_core *core) */ long clk_get_accuracy(struct clk *clk) { + long accuracy; + if (!clk) return 0; - return clk_core_get_accuracy(clk->core); + clk_prepare_lock(); + accuracy = clk_core_get_accuracy_recalc(clk->core); + clk_prepare_unlock(); + + return accuracy; } EXPORT_SYMBOL_GPL(clk_get_accuracy); @@ -1599,19 +1599,12 @@ static void __clk_recalc_rates(struct clk_core *core, unsigned long msg) __clk_recalc_rates(child, msg); } -static unsigned long clk_core_get_rate(struct clk_core *core) +static unsigned long clk_core_get_rate_recalc(struct clk_core *core) { - unsigned long rate; - - clk_prepare_lock(); - if (core && (core->flags & CLK_GET_RATE_NOCACHE)) __clk_recalc_rates(core, 0); - rate = clk_core_get_rate_nolock(core); - clk_prepare_unlock(); - - return rate; + return clk_core_get_rate_nolock(core); } /** @@ -1624,10 +1617,16 @@ static unsigned long clk_core_get_rate(struct clk_core *core) */ unsigned long clk_get_rate(struct clk *clk) { + unsigned long rate; + if (!clk) return 0; - return clk_core_get_rate(clk->core); + clk_prepare_lock(); + rate = clk_core_get_rate_recalc(clk->core); + clk_prepare_unlock(); + + return rate; } EXPORT_SYMBOL_GPL(clk_get_rate); @@ -2910,7 +2909,8 @@ static void clk_summary_show_one(struct seq_file *s, struct clk_core *c, level * 3 + 1, "", 30 - level * 3, c->name, c->enable_count, c->prepare_count, c->protect_count, - clk_core_get_rate(c), clk_core_get_accuracy(c)); + clk_core_get_rate_recalc(c), + clk_core_get_accuracy_recalc(c)); phase = clk_core_get_phase(c); if (phase >= 0) @@ -2965,10 +2965,10 @@ static void clk_dump_one(struct seq_file *s, struct clk_core *c, int level) seq_printf(s, "\"enable_count\": %d,", c->enable_count); seq_printf(s, "\"prepare_count\": %d,", c->prepare_count); seq_printf(s, "\"protect_count\": %d,", c->protect_count); - seq_printf(s, "\"rate\": %lu,", clk_core_get_rate(c)); + seq_printf(s, "\"rate\": %lu,", clk_core_get_rate_recalc(c)); seq_printf(s, "\"min_rate\": %lu,", min_rate); seq_printf(s, "\"max_rate\": %lu,", max_rate); - seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy(c)); + seq_printf(s, "\"accuracy\": %lu,", clk_core_get_accuracy_recalc(c)); phase = clk_core_get_phase(c); if (phase >= 0) seq_printf(s, "\"phase\": %d,", phase); @@ -3446,7 +3446,7 @@ static int __clk_core_init(struct clk_core *core) */ if (core->ops->recalc_accuracy) core->accuracy = core->ops->recalc_accuracy(core->hw, - __clk_get_accuracy(parent)); + clk_core_get_accuracy_no_lock(parent)); else if (parent) core->accuracy = parent->accuracy; else -- cgit v1.2.3 From 2760878662a290ac57cff8a5a8d8bda8f4dddc37 Mon Sep 17 00:00:00 2001 From: Stephen Boyd Date: Wed, 5 Feb 2020 15:28:02 -0800 Subject: clk: Bail out when calculating phase fails during clk registration Bail out of clk registration if we fail to get the phase for a clk that has a clk_ops::get_phase() callback. Print a warning too so that driver authors can easily figure out that some clk is unable to read back phase information at boot. Cc: Douglas Anderson Cc: Heiko Stuebner Suggested-by: Jerome Brunet Signed-off-by: Stephen Boyd Link: https://lkml.kernel.org/r/20200205232802.29184-5-sboyd@kernel.org Acked-by: Jerome Brunet --- drivers/clk/clk.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'drivers/clk/clk.c') diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index dc8bdfbd6a0c..ed1797857bae 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3457,7 +3457,12 @@ static int __clk_core_init(struct clk_core *core) * Since a phase is by definition relative to its parent, just * query the current clock phase, or just assume it's in phase. */ - clk_core_get_phase(core); + ret = clk_core_get_phase(core); + if (ret < 0) { + pr_warn("%s: Failed to get phase for clk '%s'\n", __func__, + core->name); + goto out; + } /* * Set clk's duty cycle. -- cgit v1.2.3 From c3944ec8c6df256ab480b56cb776f36df44b2ba5 Mon Sep 17 00:00:00 2001 From: Maxime Ripard Date: Tue, 25 Feb 2020 14:42:48 +0100 Subject: clk: Fix phase init check Commit 2760878662a2 ("clk: Bail out when calculating phase fails during clk registration") introduced a check on error values at the time the clock is registered to bail out when such an error occurs. However, it doesn't check whether the returned value is positive which will happen if the driver returns a non-zero phase. Since a phase is usually a non-zero positive number this ends up returning something that isn't 0 to the caller of __clk_core_init(), making most clks fail to register if they implement a phase clk op and return anything besides 0 for the phase. Fix this by returning the error if phase is less than zero or just return zero if the phase is a positive number. Fixes: 2760878662a2 ("clk: Bail out when calculating phase fails during clk registration") Signed-off-by: Maxime Ripard Link: https://lkml.kernel.org/r/20200225134248.919889-1-maxime@cerno.tech Reported-by: "kernelci.org bot" [sboyd@kernel.org: Reword commit text to provide clarity] Signed-off-by: Stephen Boyd --- drivers/clk/clk.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'drivers/clk/clk.c') diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index ed1797857bae..4d6fd7de05ae 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3344,6 +3344,7 @@ static int __clk_core_init(struct clk_core *core) int ret; struct clk_core *parent; unsigned long rate; + int phase; if (!core) return -EINVAL; @@ -3457,8 +3458,9 @@ static int __clk_core_init(struct clk_core *core) * Since a phase is by definition relative to its parent, just * query the current clock phase, or just assume it's in phase. */ - ret = clk_core_get_phase(core); - if (ret < 0) { + phase = clk_core_get_phase(core); + if (phase < 0) { + ret = phase; pr_warn("%s: Failed to get phase for clk '%s'\n", __func__, core->name); goto out; -- cgit v1.2.3