Re: [PATCH v2] clk: spacemit: program the mux when setting a mix clock's rate

[Yixun Lan <dlan@kernel.org>]

Hi Dave,

On 02:39 Sat 03 Oct     , Dave Klotz wrote:
> ccu_mix_set_rate() picks the best parent and divider but writes only
> the divider. U-Boot's CCF has no determine_rate step that would reparent
> the clock first, so the clock keeps running from whatever source the
> mux selects, at a rate the caller never asked for. Worse, when the
> hardware mux selects a source that this build's parent list does not
> contain (the K1 SPL lists only pll1_d6_409p6 for the SDH clocks), init
> registers the clock under parents[0] and the clock framework's idea of
> the parent no longer matches the hardware.
> 
> Reparent the clock to the chosen parent with clk_set_parent() before
> programming the divider, as the clock core does in Linux. Do so also
> when the framework already has that parent but the mux selects another
> source. Nothing is written when the mux and divider already match, so a
> clock left running at the requested rate by an earlier stage is not
> disturbed.
> 
> Signed-off-by: Dave Klotz <DaveWK225@protonmail.com>
> ---
> Changes in v2:
> - Reparent through clk_set_parent() before programming the divider, as
>   suggested by Yixun Lan. CCF enables the new parent only when the clock
>   itself is enabled.
> - Reparent also when only the hardware mux disagrees with the chosen
>   parent, including the SPL's single-parent clock registration case.
> - Share parent-index lookup with ccu_mux_set_parent(). Keep the no-write
>   case when the mux and divider already match.
> 
> v1: https://lists.u-boot-project.org/pipermail/u-boot/2026-October/633805.html
> 
> The parent/divider selection algorithm is unchanged from v1. This
> revision changes how the selected parent is applied.
> 
> Retested on 2026-10-02, with the integrated board-support lane on next
> bd2f5794b7e2, OpenSBI v1.9, and "clk: spacemit: ccu_mix: fix inverted
> condition in ccu_mix_trigger_fc()" from Yixun's k3/for-next tree:
> - OrangePi R2S: eMMC SPL boot, SDH2 I/O clock 204.8 MHz instead of the
>   BootROM's 409.6 MHz; U-Boot HS200 at 200 MHz/8-bit, normal EFI and
>   FreeBSD boot, and a final unattended reboot.
> - OrangePi RV2: microSD SPL boot, U-Boot SD High Speed at 50 MHz, EFI and
>   FreeBSD boot with root on NVMe; two boots checked.
> 
> Both CPU clusters remain at 1600 MHz on both boards, with SSH working
> following boot. The R2S, RV2, generic K1 and RV2 SPI configurations build
> SPL and U-Boot proper without compiler warnings; checkpatch passes.
> Generic K1 and RV2 SPI are BUILD-ONLY for this revision. No before/after
> performance benchmarks were run.
> 
>  drivers/clk/spacemit/clk_mix.c | 79 ++++++++++++++++++++++++----------
>  1 file changed, 56 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/clk/spacemit/clk_mix.c b/drivers/clk/spacemit/clk_mix.c
> index a1158512a9..fc8e0fd41d 100644
> --- a/drivers/clk/spacemit/clk_mix.c
> +++ b/drivers/clk/spacemit/clk_mix.c
> @@ -136,14 +136,63 @@ ccu_mix_calc_best_rate(struct clk *clk, unsigned long rate,
>  	return best_rate;
>  }
>  
..
> +static u8 ccu_mux_get_parent(struct clk *clk)
This problem isn't introduced in this patch, but while review it..

I have difficulty to understand the function name with what it exactly do
here.. I think it trys to retrieve current parent index where the hardware
programmed to, so I think it would be better to rename as
ccu_mux_current_parent_index() or ccu_mux_get_parent_index()

> +{
> +	struct ccu_mix *mix = clk_to_ccu_mix(clk);
> +	struct ccu_mux_config *mux = &mix->mux;
..
> +	u8 parent;
I'd like to simply name it as 'index' instead of 'parent', we already have
"struct clk *parent" which will brings confusion..
> +
> +	parent = ccu_read(&mix->common, ctrl) >> mux->shift;
> +	parent &= (1 << mux->width) - 1;
> +
> +	return parent;
> +}
> +
> +static int ccu_mux_parent_index(struct ccu_common *common, struct clk *parent)
same reasone, how about ccu_mux_wanted_parent_index()? it's a parent index that
expected to program to the hardware..
> +{
> +	int i;
> +
> +	for (i = 0; i < common->num_parents; i++)
> +		if (!strcmp(parent->dev->name, common->parents[i]))
> +			return i;
> +
> +	return -EINVAL;
> +}
> +

The patch looks good to me, so with or without changes

Reviewed-by: Yixun Lan <dlan@kernel.org>

--
Yixun Lan (dlan)