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

[Xukai Wang <kingxukai@zohomail.com>]

On 2025/9/9 10:51, Yao Zi wrote:
> On Mon, Sep 08, 2025 at 10:13:15PM +0800, Xukai Wang wrote:
>> On 2025/9/7 11:13, Yao Zi wrote:
>>>> On Fri, Sep 05, 2025 at 11:10:23AM +0800, Xukai Wang wrote:
>>>> This patch provides basic support for the K230 clock, which covers
>>>> all clocks in K230 SoC.
>>>>
>>>> The clock tree of the K230 SoC consists of a 24MHZ external crystal
>>>> oscillator, PLLs and an external pulse input for timerX, and their
>>>> derived clocks.
>>>>
>>>> Co-developed-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
>>>> Signed-off-by: Xukai Wang <kingxukai@zohomail.com>
>>>> ---
>>>>  drivers/clk/Kconfig    |    6 +
>>>>  drivers/clk/Makefile   |    1 +
>>>>  drivers/clk/clk-k230.c | 2456 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 2463 insertions(+)
> ...
>>>> +static int k230_clk_set_rate_mul(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, mul_reg;
>>>> +
>>>> +	if (rate > parent_rate)
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (rate_self->read_only)
>>>> +		return 0;
>>>> +
>>>> +	if (k230_clk_find_approximate_mul(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);
>>>> +
>>>> +	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;
>>>> +}
>>>> +
>>>> +static int k230_clk_set_rate_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;
>>>> +
>>>> +	if (rate > parent_rate)
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (rate_self->read_only)
>>>> +		return 0;
>>>> +
>>>> +	if (k230_clk_find_approximate_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);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +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.
> Oops, I missed these members, so there're more differences, but...
>
>> For example, the div_min for hs_sd_card_src_rate is 2, not 1. This
>> affects the calculation of the approximate divider, and cannot be fully
>> represented if we only use the clk_divider structure.
> Reading through the TRM[1], I cannot find why using one as divisor isn't
> valid for hs_sd_card_src_rate. The clock corresponds to field
> hs_SDCLK_CFG.sd_cclk_div, and is described as "Sd card clock divider.
> N: (N+1) divider. Sd0、sd1 cclk is divided from this clock".
>
> Do you have any extra information about the limitation?

This limitation comes from the vendor's hardware reference code[2],
which indicates this constraint, but unfortunately it's not documented
in the public TRM[1].

>
>> Another example is ls_codec_adc_rate, where mul_min is 0x10, mul_max is
>> 0x1B9, div_min is 0xC35, and div_max is 0x3D09. These specific ranges
>> cannot be described using the normal clk_fractional_divider structure.
> According to the TRM, the two fields in control of the fractional clock
> are described as
>
>> codec clock stup. For example, audio_clk: 25644.1K, source clock:
>> 400M, 400M/(25644.1K) can be simplied
> to : 15625/441. sum is set to :
>> 15625, step is set to 441
> and
>
>> codec clock sum
> still I cannot find any information about the range you described with
> mul_min and div_min. Could you confirm whether they're really
> necessary?
>
>>> What do you think of introducing support for such "write enable bit" to
>>> the generic implementation of multipler/divider/fractional? Then you
>>> could reuse the generic implementation in K230's driver, avoiding code
>>> duplication.
>> Therefore, in addition to the requirement of setting the
>> write_enable_bit, the customizable ranges for these parameters are also
>> important differences that should be considered.
> Best regards,
> Yao Zi
>
> [1]: https://github.com/revyos/external-docs/blob/master/K230/en-us/K230_Technical_Reference_Manual_V0.3.1_20241118.pdf
[2]:
https://github.com/ruyisdk/linux-xuantie-kernel/blob/4d69bb363fd873f2b0ac7daa488ca0206d0b6760/arch/riscv/boot/dts/canaan/k230_clock_provider.dtsi#L918