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

[Yixun Lan <dlan@kernel.org>]

hi Dave,

On 21:29 Fri 02 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.
> 
> Write the chosen parent's mux index and the divider together and latch
> both with one frequency change. Enable the new parent first and reparent
> the clock device after, so the framework follows the hardware. 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>
> ---
> Tested on an OrangePi R2S booting from eMMC, where the SDH2 io clock
> moves from the BootROM's 409.6 MHz to 204.8 MHz in the SPL, together
> with "clk: spacemit: ccu_mix: fix inverted condition in
> ccu_mix_trigger_fc()" from the K3 clock series, and on an OrangePi RV2
> booting from microSD.
> 
>  drivers/clk/spacemit/clk_mix.c | 54 +++++++++++++++++++++++++++++-----
>  1 file changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/spacemit/clk_mix.c b/drivers/clk/spacemit/clk_mix.c
> index a1158512a9..3521ab9374 100644
> --- a/drivers/clk/spacemit/clk_mix.c
> +++ b/drivers/clk/spacemit/clk_mix.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <dm/device.h>
> +#include <dm/device-internal.h>
>  #include <dm/uclass.h>
>  #include <div64.h>
>  #include <regmap.h>
> @@ -136,26 +137,63 @@ ccu_mix_calc_best_rate(struct clk *clk, unsigned long rate,
>  	return best_rate;
>  }
>  
> +/*
> + * U-Boot's CCF has no determine_rate, so the parent is chosen here along with
> + * the divider. The mux is programmed from the chosen parent rather than left
> + * where it was found: init falls back to parents[0] when the hardware mux
> + * selects a source this build does not list (the SPL lists only a subset), and
> + * the clock would otherwise keep running from that source at a rate nobody
> + * computed. Mux and divider are written together and latched by one frequency
> + * change, and nothing is written when both already match, so a clock left
> + * running by an earlier stage is not disturbed.
> + */
>  static unsigned long ccu_mix_set_rate(struct clk *clk, unsigned long rate)
>  {
>  	struct ccu_mix *mix = clk_to_ccu_mix(clk);
>  	struct ccu_common *common = &mix->common;
>  	struct ccu_div_config *div = &mix->div;
> -	u32 current_div, target_div, mask;
> +	struct ccu_mux_config *mux = &mix->mux;
> +	struct clk *parent = NULL;
> +	unsigned long parent_rate;
> +	u32 target_div = 0, mask, val;
> +	int i, ret;
>  
> -	ccu_mix_calc_best_rate(clk, rate, NULL, NULL, &target_div);
> +	ccu_mix_calc_best_rate(clk, rate, &parent, &parent_rate, &target_div);
>  
> -	current_div = ccu_read(common, ctrl) >> div->shift;
> -	current_div &= (1 << div->width) - 1;
> +	mask = GENMASK(div->width + div->shift - 1, div->shift);
> +	val = target_div << div->shift;
> +
> +	if (mux->width && parent) {
> +		for (i = 0; i < common->num_parents; i++)
> +			if (!strcmp(parent->dev->name, common->parents[i]))
> +				break;
> +		if (i == common->num_parents)
> +			return -EINVAL;
> +		mask |= GENMASK(mux->width + mux->shift - 1, mux->shift);
> +		val |= i << mux->shift;
> +	} else {
> +		parent = NULL;
> +	}
>  
> -	if (current_div == target_div)
> +	if ((ccu_read(common, ctrl) & mask) == val)
>  		return 0;
>  
> -	mask = GENMASK(div->width + div->shift - 1, div->shift);
> +	if (parent && clk->dev->parent != parent->dev) {
> +		ret = clk_enable(parent);
> +		if (ret)
> +			return ret;
> +	}
>  
.. this will enable parent, would be it ok to combine below
device_reparent() together? to simply use clk_set_parent(clk,
parent)? it make sense to me to reparent first then program
the divider

> -	ccu_update(common, ctrl, mask, target_div << div->shift);
> +	ccu_update(common, ctrl, mask, val);
>  
> -	return ccu_mix_trigger_fc(clk);
> +	ret = ccu_mix_trigger_fc(clk);
> +	if (ret)
> +		return ret;
> +
..
> +	if (parent && clk->dev->parent != parent->dev)
> +		return device_reparent(clk->dev, parent->dev);
> +
reparent clk
> +	return 0;
>  }
>  

-- 
Yixun Lan (dlan)