Re: [PATCH v2 3/3] clk: Drop the rate range on clk_put

Maxime Ripard
2022-03-31T15:33:25Z

[-- Attachment #1.1: Type: text/plain, Size: 3443 bytes --]

Hi Tony,

On Thu, Mar 31, 2022 at 06:00:42PM +0300, Tony Lindgren wrote:
> * Maxime Ripard <maxime@cerno.tech> [220331 09:52]:
> > On Thu, Mar 31, 2022 at 12:42:10PM +0300, Tony Lindgren wrote:
> > > It seems the dts assigned-clock-parents no longer works now?
> >
> > That would make some kind of sense, __set_clk_parents calls clk_put on
> > both the assigned clock and its parent.
> >
> > Could you see what parent (and why?) it tries to enforce then?
>
> It picks the other option available for the mux clock that only has
> two options. No idea why, but if you have some debug patch in mind I
> can give it a try.
>
> > It looks like the gpt1_fck driver might favor another parent for that
> > rate, which, if it's an invalid configuration, shouldn't really happen?
>
> Hmm there's a gate clock and a mux clock, there's not really a rate
> selection available here for the sources.

If I followed the OMAP driver properly, clk_mux_determine_rate_flags is
doing the heavy lifting, could you run your test with

-- >8 --

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 91f863b7a824..c2ab1be62f96 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -552,6 +552,12 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
unsigned long best = 0;
struct clk_rate_request parent_req = *req;

+ pr_crit("%s: requested rate %lu\n", __func__, req->rate);
+
+ parent = core->parent;
+ pr_crit("%s: current parent %s\n", __func__, parent->name);
+ pr_crit("%s: current parent rate %lu\n", __func__, clk_core_get_rate_nolock(parent));
+
/* if NO_REPARENT flag set, pass through to current parent */
if (core->flags & CLK_SET_RATE_NO_REPARENT) {
parent = core->parent;
@@ -578,6 +584,11 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
if (!parent)
continue;

+ pr_crit("%s: Trying parent %s (%lu)\n",
+ __func__,
+ parent->name,
+ clk_core_get_rate_nolock(parent));
+
if (core->flags & CLK_SET_RATE_PARENT) {
parent_req = *req;
ret = __clk_determine_rate(parent->hw, &parent_req);
@@ -603,6 +614,11 @@ int clk_mux_determine_rate_flags(struct clk_hw *hw,
req->best_parent_rate = best;
req->rate = best;

+ pr_crit("%s: Best parent %s (%lu)\n",
+ __func__,
+ best_parent->name,
+ best);
+
return 0;
}
EXPORT_SYMBOL_GPL(clk_mux_determine_rate_flags);
@@ -2201,8 +2217,12 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
if (!core)
return 0;

+ pr_crit("%s: rate %lu\n", __func__, req_rate);
+
rate = clk_core_req_round_rate_nolock(core, req_rate);

+ pr_crit("%s: rounded rate %lu\n", __func__, req_rate);
+
/* bail early if nothing to do */
if (rate == clk_core_get_rate_nolock(core))
return 0;
@@ -2367,6 +2387,8 @@ static int clk_set_rate_range_nolock(struct clk *clk,
goto out;
}

+ pr_crit("%s: core req rate %lu\n", __func__, clk->core->req_rate);
+
/*
* Since the boundaries have been changed, let's give the
* opportunity to the provider to adjust the clock rate based on
@@ -2385,6 +2407,9 @@ static int clk_set_rate_range_nolock(struct clk *clk,
* this corner case when determining the rate
*/
rate = clamp(clk->core->req_rate, min, max);
+
+ pr_crit("%s: clamped rate %lu\n", __func__, rate);
+
ret = clk_core_set_rate_nolock(clk->core, rate);
if (ret) {
/* rollback the changes */

-- >8 --

Thanks!
Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]