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

From: Maxime Ripard <maxime@cerno.tech>
To: Alexander Stein <alexander.stein@ew.tq-group.com>
Cc: Tony Lindgren <tony@atomide.com>,
	linux-arm-kernel@lists.infradead.org,
	Marek Szyprowski <m.szyprowski@samsung.com>,
	Mike Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	linux-clk@vger.kernel.org,
	Dmitry Osipenko <dmitry.osipenko@collabora.com>,
	'Linux Samsung SOC' <linux-samsung-soc@vger.kernel.org>,
	linux-amlogic@lists.infradead.org, linux-omap@vger.kernel.org
Subject: Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH v2 3/3] clk: Drop the rate range on clk_put
Date: Fri, 1 Apr 2022 15:34:09 +0200	[thread overview]
Message-ID: <20220401133409.wtilnpgp4fqlrrew@houat> (raw)
In-Reply-To: <2823402.e9J7NaK4W3@steina-w>

[-- Attachment #1: Type: text/plain, Size: 7198 bytes --]

On Fri, Apr 01, 2022 at 03:07:10PM +0200, Alexander Stein wrote:
> > Does it also happen if you only apply the patch I had above, and not all
> > the debugging?
> 
> Yes, these are the last lines I see:
> ---
> [    1.236306] mmcblk0rpmb: mmc0:0001 DA6016 4.00 MiB, chardev (235:0)                                                               
> [    1.241031] i2c i2c-1: IMX I2C adapter registered                                                                                 
> [    1.251771] i2c i2c-3: IMX I2C adapter registered                                                                                 
> [    1.256957] i2c i2c-5: IMX I2C adapter registered

Could you add on top of next (so dropping everything we did so far)

---- >8 -----

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 91f863b7a824..552b1e16a82d 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -540,6 +540,8 @@ static bool mux_is_better_rate(unsigned long rate, unsigned long now,
 	if (flags & CLK_MUX_ROUND_CLOSEST)
 		return abs(now - rate) < abs(best - rate);

+	pr_crit("%s +%d rate %lu now %lu best %lu\n", __func__, __LINE__, rate, now, best);
+
 	return now <= rate && now > best;
 }

@@ -552,6 +554,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: %s: requested rate %lu\n", __func__, core->name, req->rate);
+
+	parent = core->parent;
+	pr_crit("%s: %s: current parent %s\n", __func__, core->name, parent ? parent->name : "(null)");
+	pr_crit("%s: %s: current parent rate %lu\n", __func__, core->name, 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,24 +586,37 @@ 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) {
+			pr_crit("%s +%d\n", __func__, __LINE__);
 			parent_req = *req;
 			ret = __clk_determine_rate(parent->hw, &parent_req);
+			pr_crit("%s +%d %d\n", __func__, __LINE__, ret);
 			if (ret)
 				continue;
 		} else {
+			pr_crit("%s +%d\n", __func__, __LINE__);
 			parent_req.rate = clk_core_get_rate_nolock(parent);
 		}

+		pr_crit("%s +%d\n", __func__, __LINE__);
+
 		if (mux_is_better_rate(req->rate, parent_req.rate,
 				       best, flags)) {
+			pr_crit("%s +%d\n", __func__, __LINE__);
 			best_parent = parent;
 			best = parent_req.rate;
 		}
 	}

-	if (!best_parent)
+	if (!best_parent) {
+		pr_crit("%s +%d\n", __func__, __LINE__);
 		return -EINVAL;
+	}

 out:
 	if (best_parent)
@@ -603,6 +624,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);
@@ -1345,11 +1371,15 @@ static int clk_core_determine_round_nolock(struct clk_core *core,

 	lockdep_assert_held(&prepare_lock);

+	pr_crit("%s +%d %s\n", __func__, __LINE__, core->name);
 	if (!core)
 		return 0;

+	pr_crit("%s +%d\n", __func__, __LINE__);
 	req->rate = clamp(req->rate, req->min_rate, req->max_rate);

+	pr_crit("%s +%d\n", __func__, __LINE__);
+
 	/*
 	 * At this point, core protection will be disabled
 	 * - if the provider is not protected at all
@@ -1357,10 +1387,13 @@ static int clk_core_determine_round_nolock(struct clk_core *core,
 	 *   over the provider
 	 */
 	if (clk_core_rate_is_protected(core)) {
+		pr_crit("%s +%d\n", __func__, __LINE__);
 		req->rate = core->rate;
 	} else if (core->ops->determine_rate) {
+		pr_crit("%s +%d\n", __func__, __LINE__);
 		return core->ops->determine_rate(core->hw, req);
 	} else if (core->ops->round_rate) {
+		pr_crit("%s +%d\n", __func__, __LINE__);
 		rate = core->ops->round_rate(core->hw, req->rate,
 					     &req->best_parent_rate);
 		if (rate < 0)
@@ -1368,6 +1401,7 @@ static int clk_core_determine_round_nolock(struct clk_core *core,

 		req->rate = rate;
 	} else {
+		pr_crit("%s +%d\n", __func__, __LINE__);
 		return -EINVAL;
 	}

@@ -1402,17 +1436,26 @@ static int clk_core_round_rate_nolock(struct clk_core *core,
 {
 	lockdep_assert_held(&prepare_lock);

+	pr_crit("%s +%d\n", __func__, __LINE__);
+
 	if (!core) {
 		req->rate = 0;
 		return 0;
 	}

+	pr_crit("%s +%d\n", __func__, __LINE__);
+
 	clk_core_init_rate_req(core, req);

-	if (clk_core_can_round(core))
+	pr_crit("%s +%d\n", __func__, __LINE__);
+
+	if (clk_core_can_round(core)) {
+		pr_crit("%s +%d\n", __func__, __LINE__);
 		return clk_core_determine_round_nolock(core, req);
-	else if (core->flags & CLK_SET_RATE_PARENT)
+	} else if (core->flags & CLK_SET_RATE_PARENT) {
+		pr_crit("%s +%d\n", __func__, __LINE__);
 		return clk_core_round_rate_nolock(core->parent, req);
+	}

 	req->rate = core->rate;
 	return 0;
@@ -2201,21 +2244,31 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	if (!core)
 		return 0;

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

+	pr_crit("%s: %s: rounded rate %lu\n", __func__, core->name, req_rate);
+
 	/* bail early if nothing to do */
 	if (rate == clk_core_get_rate_nolock(core))
 		return 0;

+	pr_crit("%s +%d\n", __func__, __LINE__);
+
 	/* fail on a direct rate set of a protected provider */
 	if (clk_core_rate_is_protected(core))
 		return -EBUSY;

+	pr_crit("%s +%d\n", __func__, __LINE__);
+
 	/* calculate new rates and get the topmost changed clock */
 	top = clk_calc_new_rates(core, req_rate);
 	if (!top)
 		return -EINVAL;

+	pr_crit("%s +%d\n", __func__, __LINE__);
+
 	ret = clk_pm_runtime_get(core);
 	if (ret)
 		return ret;
@@ -2367,6 +2420,16 @@ static int clk_set_rate_range_nolock(struct clk *clk,
 		goto out;
 	}

+	pr_crit("%s: %s: orphan ? %c\n",
+		__func__,
+		clk->core->name,
+		clk->core->orphan ? 'y' : 'n');
+
+	pr_crit("%s: %s: core req rate %lu\n",
+		__func__,
+		clk->core->name,
+		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
@@ -2384,7 +2447,11 @@ static int clk_set_rate_range_nolock(struct clk *clk,
 	 * - the determine_rate() callback does not really check for
 	 *   this corner case when determining the rate
 	 */
+
 	rate = clamp(clk->core->req_rate, min, max);
+
+	pr_crit("%s: %s: clamped rate %lu\n", __func__, clk->core->name, rate);
+
 	ret = clk_core_set_rate_nolock(clk->core, rate);
 	if (ret) {
 		/* rollback the changes */
@@ -2599,6 +2666,8 @@ static int clk_core_set_parent_nolock(struct clk_core *core,
 	} else {
 		__clk_recalc_rates(core, POST_RATE_CHANGE);
 		__clk_recalc_accuracies(core);
+
+		core->req_rate = core->rate;
 	}

 runtime_put:
---- >8 -----

Thanks!
Maxime

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