Re: [PATCH 2/5] clk: spacemit: make MIX rate selection consistent

From: Yao Zi <me@ziyao.cc>
To: Troy Mitchell <troy.mitchell@linux.spacemit.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Brian Masney <bmasney+clk@redhat.com>,
	Jerome Brunet <jbrunet+clk@baylibre.com>,
	Yixun Lan <dlan@kernel.org>, Alex Elder <elder@riscstar.com>,
	Inochi Amaoto <inochiama@outlook.com>,
	Haylen Chu <heylenay@4d2.org>
Cc: linux-clk@vger.kernel.org, linux-riscv@lists.infradead.org,
	spacemit@lists.linux.dev, linux-kernel@vger.kernel.org,
	Yao Zi <me@ziyao.cc>
Subject: Re: [PATCH 2/5] clk: spacemit: make MIX rate selection consistent
Date: Thu, 10 Sep 2026 13:01:30 +0000	[thread overview]
Message-ID: <aqKqKkXbTkcS-pnY@pie> (raw)
In-Reply-To: <20260909-spacemit-pll-init-v1-2-b3065ad5a4ac@linux.spacemit.com>

On Wed, Sep 09, 2026 at 10:07:02PM +0800, Troy Mitchell wrote:
> CCF passes the selected parent's rate to set_rate(). Searching other
> parents at that point can produce a divider for a different source,
> making the programmed rate disagree with CCF's selection.
> 
> Restrict divider selection to the supplied parent rate and use the same
> rounding as divider_recalc_rate(). Track the best error separately so
> low-rate requests do not leave the initial zero-Hz candidate selected.
> Skip zero-rate parents and have determine_rate() reject requests when no
> usable parent exists.
> 
> Fixes: 1b72c59db0ad ("clk: spacemit: Add clock support for SpacemiT K1 SoC")
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> ---
>  drivers/clk/spacemit/ccu_mix.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/spacemit/ccu_mix.c b/drivers/clk/spacemit/ccu_mix.c
> index a8b407049bf4d..da3c5685d4f65 100644
> --- a/drivers/clk/spacemit/ccu_mix.c
> +++ b/drivers/clk/spacemit/ccu_mix.c
> @@ -107,22 +107,27 @@ ccu_mix_calc_best_rate(struct clk_hw *hw, unsigned long rate,
>  	struct ccu_mix *mix = hw_to_ccu_mix(hw);
>  	unsigned int parent_num = clk_hw_get_num_parents(hw);
>  	struct ccu_div_config *div = &mix->div;
> -	u32 div_max = 1 << div->width;
>  	unsigned long best_rate = 0;
> +	unsigned long best_delta = ULONG_MAX;
>  
>  	for (int i = 0; i < parent_num; i++) {
>  		struct clk_hw *parent = clk_hw_get_parent_by_index(hw, i);
>  		unsigned long parent_rate;
> +		u32 div_max = 1 << div->width;

div_max should be invariant across iterations. Is there a reason moving
it inside the loop?

>  		if (!parent)
>  			continue;
>  
>  		parent_rate = clk_hw_get_rate(parent);
> +		if (!parent_rate)
> +			continue;
>  
>  		for (int j = 1; j <= div_max; j++) {
> -			unsigned long tmp = DIV_ROUND_CLOSEST_ULL(parent_rate, j);
> +			unsigned long tmp = DIV_ROUND_UP_ULL(parent_rate, j);
> +			unsigned long delta = abs_diff(tmp, rate);
>  
> -			if (abs(tmp - rate) < abs(best_rate - rate)) {
> +			if (delta < best_delta) {
> +				best_delta = delta;
>  				best_rate = tmp;
>  
>  				if (div_val)

Regards,
Yao Zi