Re: [PATCH v8 2/3] clk: canaan: Add clock driver for Canaan K230

[Xukai Wang <kingxukai@zohomail.com>]

On 2025/9/9 15:02, Vivian Wang wrote:
> On 9/8/25 22:13, Xukai Wang wrote:
>>>> [...]
>>>>
>>>> +
>>>> +static int k230_clk_set_rate_mul_div(struct clk_hw *hw, unsigned long rate,
>>>> +				     unsigned long parent_rate)
>>>> +{
>>>> +	struct k230_clk_rate *clk = hw_to_k230_clk_rate(hw);
>>>> +	struct k230_clk_rate_self *rate_self = &clk->clk;
>>>> +	u32 div, mul, div_reg, mul_reg;
>>>> +
>>>> +	if (rate > parent_rate)
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (rate_self->read_only)
>>>> +		return 0;
>>>> +
>>>> +	if (k230_clk_find_approximate_mul_div(rate_self->mul_min, rate_self->mul_max,
>>>> +					      rate_self->div_min, rate_self->div_max,
>>>> +					      rate, parent_rate, &div, &mul))
>>>> +		return -EINVAL;
>>>> +
>>>> +	guard(spinlock)(rate_self->lock);
>>>> +
>>>> +	div_reg = readl(rate_self->reg + clk->div_reg_off);
>>>> +	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 |= ((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);
>>>> +
>>>> +	return 0;
>>>> +}
>>> There are three variants of rate clocks, mul-only, div-only and mul-div
>>> ones, which are similar to clk-multiplier, clk-divider,
>>> clk-fractional-divider.
>>>
>>> The only difference is to setup new parameters for K230's rate clocks,
>>> a register bit, described as k230_clk_rate_self.write_enable_bit, must
>>> be set first.
>> Actually, I think the differences are not limited to just the
>> write_enable_bit. There are also distinct mul_min, mul_max, div_min, and
>> div_max values, which are not typically just 1 and (1 << bit_width) as
>> in standard clock divider or multiplier structures.
> So the part I have been thinking about is, consider just checking the
> {mul,div}_{min,max} values to determine which kind it is? As is this is
> just redundant information, since you can infer whether there is a
> configurable multiplier by checking if mul_{min,max} are equal. Same for
> div_{min,max}.
>
> Vivian "dramforever" Wang
Thanks for pointing it out. I see your idea, but I don’t think it’s
necessary to determine the clock type from {mul,div}_{min,max}
dynamically since we already statically specify each mul, div, and
mul-div clock by different macros.