Re: [PATCH] clk: canaan: Clear rate fields before reprogramming dividers

[Xukai Wang]

On 2026/6/18 12:50, David Carlier wrote:
> The rate set_rate helpers perform a read-modify-write on the divider
> and multiplier registers but only ever OR the new value in, without
> first masking off the existing field. The first write after reset lands
> on a zeroed field and looks correct, but any later reprogramming leaves
> the old bits set: the field becomes the bitwise OR of the previous and
> new encodings, corrupting the divider or multiplier.
>
> Mask off each field before writing the new value so reprogramming a
> clock to a different rate produces the intended register contents.
>
> Fixes: a7b7c7c6c016 ("clk: canaan: Add clock driver for Canaan K230")
> Signed-off-by: David Carlier <devnexen@xxxxxxxxx>
> ---
>  drivers/clk/clk-k230.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/clk/clk-k230.c b/drivers/clk/clk-k230.c
> index cfc437038e4e..f34a3e6d3bca 100644
> --- a/drivers/clk/clk-k230.c
> +++ b/drivers/clk/clk-k230.c
> @@ -2227,6 +2227,7 @@ static int k230_clk_set_rate_mul(struct clk_hw *hw, unsigned long rate,
>  	guard(spinlock)(rate_self->lock);
>  
>  	mul_reg = readl(rate_self->reg + clk->mul_reg_off);
> +	mul_reg &= ~(rate_self->mul_mask << rate_self->mul_shift);
>  	mul_reg |= ((mul - 1) & rate_self->mul_mask) << (rate_self->mul_shift);
>  	mul_reg |= BIT(rate_self->write_enable_bit);
>  	writel(mul_reg, rate_self->reg + clk->mul_reg_off);
> @@ -2257,6 +2258,7 @@ static int k230_clk_set_rate_div(struct clk_hw *hw, unsigned long rate,
>  	guard(spinlock)(rate_self->lock);
>  
>  	div_reg = readl(rate_self->reg + clk->div_reg_off);
> +	div_reg &= ~(rate_self->div_mask << rate_self->div_shift);
>  	div_reg |= ((div - 1) & rate_self->div_mask) << (rate_self->div_shift);
>  	div_reg |= BIT(rate_self->write_enable_bit);
>  	writel(div_reg, rate_self->reg + clk->div_reg_off);
> @@ -2287,11 +2289,13 @@ static int k230_clk_set_rate_mul_div(struct clk_hw *hw, unsigned long rate,
>  	guard(spinlock)(rate_self->lock);
>  
>  	div_reg = readl(rate_self->reg + clk->div_reg_off);
> +	div_reg &= ~(rate_self->div_mask << rate_self->div_shift);
>  	div_reg |= ((div - 1) & rate_self->div_mask) << (rate_self->div_shift);
>  	div_reg |= BIT(rate_self->write_enable_bit);
>  	writel(div_reg, rate_self->reg + clk->div_reg_off);
>  
>  	mul_reg = readl(rate_self->reg + clk->mul_reg_off);
> +	mul_reg &= ~(rate_self->mul_mask << rate_self->mul_shift);
>  	mul_reg |= ((mul - 1) & rate_self->mul_mask) << (rate_self->mul_shift);
>  	mul_reg |= BIT(rate_self->write_enable_bit);
>  	writel(mul_reg, rate_self->reg + clk->mul_reg_off);

Good catch. This fix is correct and necessary -- without masking the old
bits first, reprogramming the clock rate would leave stale bits in the
register and result in incorrect divider/multiplier values.

All set_rate paths are properly covered.

Acked-by: Xukai Wang <kingxukai@xxxxxxxxxxxx>

-- 
Thanks,
Xukai