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

[ALOK TIWARI <alok.a.tiwari@oracle.com>]

Hi Xukai,

Thanks for your patch.

On 15-04-2025 19:55, Xukai Wang wrote:
> This patch provides basic support for the K230 clock, which does not
> cover all clocks.
> 
> The clock tree of the K230 SoC consists of OSC24M, PLLs and sysclk.
> 
> Co-developed-by: Troy Mitchell <TroyMitchell988@gmail.com>
> Signed-off-by: Troy Mitchell <TroyMitchell988@gmail.com>
> Signed-off-by: Xukai Wang <kingxukai@zohomail.com>
> ---
[clip]
> +
> +/* PLL control register bits. */
> +#define K230_PLL_BYPASS_ENABLE				BIT(19)
> +#define K230_PLL_GATE_ENABLE				BIT(2)
> +#define K230_PLL_GATE_WRITE_ENABLE			BIT(18)
> +#define K230_PLL_OD_SHIFT				24
> +#define K230_PLL_OD_MASK				0xF
> +#define K230_PLL_R_SHIFT				16
> +#define K230_PLL_R_MASK					0x3F
> +#define K230_PLL_F_SHIFT				0
> +#define K230_PLL_F_MASK					0x1FFFF
> +#define K230_PLL0_OFFSET_BASE				0x00
> +#define K230_PLL1_OFFSET_BASE				0x10
> +#define K230_PLL2_OFFSET_BASE				0x20
> +#define K230_PLL3_OFFSET_BASE				0x30
> +#define K230_PLL_DIV_REG_OFFSET				0x00
> +#define K230_PLL_BYPASS_REG_OFFSET			0x04
> +#define K230_PLL_GATE_REG_OFFSET			0x08
> +#define K230_PLL_LOCK_REG_OFFSET			0x0C
> +

use lowercase hex


> +/* PLL lock register bits.  */

extra ' ' after bits.

> +#define K230_PLL_STATUS_MASK				BIT(0)
> +
> +/* K230 CLK registers offset */
> +#define K230_CLK_AUDIO_CLKDIV_OFFSET			0x34
> +#define K230_CLK_PDM_CLKDIV_OFFSET			0x40
> +#define K230_CLK_CODEC_ADC_MCLKDIV_OFFSET		0x38
> +#define K230_CLK_CODEC_DAC_MCLKDIV_OFFSET		0x3c
> +
[clip]
> +static int k230_clk_find_approximate(struct k230_clk *clk,
> +				     u32 mul_min,
> +				     u32 mul_max,
> +				     u32 div_min,
> +				     u32 div_max,
> +				     enum k230_clk_div_type method,
> +				     unsigned long rate,
> +				     unsigned long parent_rate,
> +				     u32 *div,
> +				     u32 *mul)
> +{
> +	long abs_min;
> +	long abs_current;
> +	long perfect_divide;
> +	struct k230_clk_cfg *cfg = k230_clk_cfgs[clk->id];
> +	struct k230_clk_rate_cfg *rate_cfg = cfg->rate_cfg;
> +

hope all is non-zero (mul_min, mul_max , rate , parent_rate)
avoid division by zero possibility

> +	const u32 codec_clk[9] = {
> +		2048000,
> +		3072000,
> +		4096000,
> +		6144000,
> +		8192000,
> +		11289600,
> +		12288000,
> +		24576000,
> +		49152000
> +	};
> +
> +	const u32 codec_div[9][2] = {
> +		{3125, 16},
> +		{3125, 24},
> +		{3125, 32},
> +		{3125, 48},
> +		{3125, 64},
> +		{15625, 441},
> +		{3125, 96},
> +		{3125, 192},
> +		{3125, 384}
> +	};
> +
> +	const u32 pdm_clk[20] = {
> +		128000,
> +		192000,
> +		256000,
> +		384000,
> +		512000,
> +		768000,
> +		1024000,
> +		1411200,
> +		1536000,
> +		2048000,
> +		2822400,
> +		3072000,
> +		4096000,
> +		5644800,
> +		6144000,
> +		8192000,
> +		11289600,
> +		12288000,
> +		24576000,
> +		49152000
> +	};
> +
> +	const u32 pdm_div[20][2] = {
> +		{3125, 1},
> +		{6250, 3},
> +		{3125, 2},
> +		{3125, 3},
> +		{3125, 4},
> +		{3125, 6},
> +		{3125, 8},
> +		{125000, 441},
> +		{3125, 12},
> +		{3125, 16},
> +		{62500, 441},
> +		{3125, 24},
> +		{3125, 32},
> +		{31250, 441},
> +		{3125, 48},
> +		{3125, 64},
> +		{15625, 441},
> +		{3125, 96},
> +		{3125, 192},
> +		{3125, 384}
> +	};
> +
> +	switch (method) {
> +	/* only mul can be changeable 1/12,2/12,3/12...*/

need ' ' before */
Maybe something like this could work here
/* Only the multiplier is configurable: 1/12, 2/12, 3/12, ... */
/* Only mul can be changed: 1/12, 2/12, 3/12, ... */

> +	case K230_MUL:
> +		perfect_divide = (long)((parent_rate * 1000) / rate);
> +		abs_min = abs(perfect_divide -
> +			     (long)(((long)div_max * 1000) / (long)mul_min));
> +		*mul = mul_min;
> +
> +		for (u32 i = mul_min + 1; i <= mul_max; i++) {
> +			abs_current = abs(perfect_divide -
> +					(long)((long)((long)div_max * 1000) / (long)i));
> +			if (abs_min > abs_current) {
> +				abs_min = abs_current;
> +				*mul = i;
> +			}
> +		}
> +
> +		*div = div_max;
> +		break;
> +	/* only div can be changeable, 1/1,1/2,1/3...*/

need ' ' before */

> +	case K230_DIV:
> +		perfect_divide = (long)((parent_rate * 1000) / rate);
> +		abs_min = abs(perfect_divide -
> +			     (long)(((long)div_min * 1000) / (long)mul_max));
> +		*div = div_min;
> +
> +		for (u32 i = div_min + 1; i <= div_max; i++) {
> +			abs_current = abs(perfect_divide -
> +					 (long)((long)((long)i * 1000) / (long)mul_max));
> +			if (abs_min > abs_current) {
> +				abs_min = abs_current;
> +				*div = i;
> +			}
> +		}
> +
> +		*mul = mul_max;
> +		break;
> +	/* mul and div can be changeable. */
> +	case K230_MUL_DIV:
> +		if (rate_cfg->rate_reg_off == K230_CLK_CODEC_ADC_MCLKDIV_OFFSET ||
> +		    rate_cfg->rate_reg_off == K230_CLK_CODEC_DAC_MCLKDIV_OFFSET) {
> +			for (u32 j = 0; j < 9; j++) {
> +				if (0 == (rate - codec_clk[j])) {
> +					*div = codec_div[j][0];
> +					*mul = codec_div[j][1];
> +				}
> +			}
> +		} else if (rate_cfg->rate_reg_off == K230_CLK_AUDIO_CLKDIV_OFFSET ||
> +			   rate_cfg->rate_reg_off == K230_CLK_PDM_CLKDIV_OFFSET) {
> +			for (u32 j = 0; j < 20; j++) {
> +				if (0 == (rate - pdm_clk[j])) {
> +					*div = pdm_div[j][0];
> +					*mul = pdm_div[j][1];
> +				}
> +			}
> +		} else {
> +			return -EINVAL;
> +		}
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static long k230_clk_round_rate(struct clk_hw *hw, unsigned long rate, unsigned long *parent_rate)
> +{
> +	struct k230_clk *clk = to_k230_clk(hw);
> +	struct k230_sysclk *ksc = clk->ksc;
> +	struct k230_clk_cfg *cfg = k230_clk_cfgs[clk->id];
> +	struct k230_clk_rate_cfg *rate_cfg = cfg->rate_cfg;
> +	u32 div = 0, mul = 0;
> +

Do we need to check for rate == 0 or parent_rate == 0 here?"

> +	if (k230_clk_find_approximate(clk,
> +				      rate_cfg->rate_mul_min, rate_cfg->rate_mul_max,
> +				      rate_cfg->rate_div_min, rate_cfg->rate_div_max,
> +				      rate_cfg->method, rate, *parent_rate, &div, &mul)) {
> +		return 0;
> +	}
> +
> +	return mul_u64_u32_div(*parent_rate, mul, div);
> +}
> +
> +static int k230_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +			     unsigned long parent_rate)
> +{
> +	struct k230_clk *clk = to_k230_clk(hw);
> +	struct k230_sysclk *ksc = clk->ksc;
> +	struct k230_clk_cfg *cfg = k230_clk_cfgs[clk->id];
> +	struct k230_clk_rate_cfg *rate_cfg = cfg->rate_cfg;
> +	struct k230_clk_rate_cfg_c *rate_cfg_c = cfg->rate_cfg_c;
> +	u32 div = 0, mul = 0, reg = 0, reg_c;
> +
> +	if (rate > parent_rate || rate == 0 || parent_rate == 0) {

what about if (rate > parent_rate || !rate || !parent_rate)

> +		dev_err(&ksc->pdev->dev, "rate or parent_rate error\n");
> +		return -EINVAL;
> +	}
> +
> +	if (cfg->read_only) {
> +		dev_err(&ksc->pdev->dev, "This clk rate is read only\n");
> +		return -EPERM;
> +	}
> +
> +	if (k230_clk_find_approximate(clk,
> +				      rate_cfg->rate_mul_min, rate_cfg->rate_mul_max,
> +				      rate_cfg->rate_div_min, rate_cfg->rate_div_max,
> +				      rate_cfg->method, rate, parent_rate, &div, &mul)) {
> +		return -EINVAL;
> +	}
> +
> +	guard(spinlock)(&ksc->clk_lock);
> +	if (!rate_cfg_c) {
> +		reg = readl(rate_cfg->rate_reg);
> +		reg &= ~((rate_cfg->rate_div_mask) << (rate_cfg->rate_div_shift));
> +
> +		if (rate_cfg->method == K230_DIV) {
> +			reg &= ~((rate_cfg->rate_mul_mask) << (rate_cfg->rate_mul_shift));
> +			reg |= ((div - 1) & rate_cfg->rate_div_mask) << (rate_cfg->rate_div_shift);
> +		} else if (rate_cfg->method == K230_MUL) {
> +			reg |= ((mul - 1) & rate_cfg->rate_div_mask) << (rate_cfg->rate_div_shift);
> +		} else {
> +			reg |= (mul & rate_cfg->rate_mul_mask) << (rate_cfg->rate_mul_shift);
> +			reg |= (div & rate_cfg->rate_div_mask) << (rate_cfg->rate_div_shift);
> +		}
> +		reg |= BIT(rate_cfg->rate_write_enable_bit);
> +	} else {
> +		reg = readl(rate_cfg->rate_reg);
> +		reg_c = readl(rate_cfg_c->rate_reg_c);
> +		reg &= ~((rate_cfg->rate_div_mask) << (rate_cfg->rate_div_shift));
> +		reg_c &= ~((rate_cfg_c->rate_mul_mask_c) << (rate_cfg_c->rate_mul_shift_c));
> +		reg_c |= BIT(rate_cfg_c->rate_write_enable_bit_c);
> +
> +		reg_c |= (mul & rate_cfg_c->rate_mul_mask_c) << (rate_cfg_c->rate_mul_shift_c);
> +		reg |= (div & rate_cfg->rate_div_mask) << (rate_cfg->rate_div_shift);
> +
> +		writel(reg_c, rate_cfg_c->rate_reg_c);
> +	}
> +	writel(reg, rate_cfg->rate_reg);
> +
> +	return 0;
> +}
> +
[clip]

> +static int k230_register_clks(struct platform_device *pdev, struct k230_sysclk *ksc)
> +{
> +	struct k230_clk_cfg *cfg;
> +	struct k230_clk_parent *pclk;
> +	struct clk_parent_data parent_data[K230_CLK_MAX_PARENT_NUM];
> +	int ret, i;
> +
> +	/*
> +	 *  Single parent clock:
> +	 *  pll0_div2 sons: cpu0_src
> +	 *  pll0_div4 sons: cpu0_pclk
> +	 *  cpu0_src sons: cpu0_aclk, cpu0_plic, cpu0_noc_ddrcp4, pmu_pclk
> +	 *
> +	 *  Mux clock:
> +	 *  hs_ospi_src parents: pll0_div2, pll2_div4
> +	 */

extra ' ' after *

what is sons?
does not sound good -> child ?

> +	for (i = 0; i < K230_CLK_NUM; i++) {
> +		cfg = k230_clk_cfgs[i];
> +		if (!cfg)
> +			continue;
> +
> +		if (cfg->mux_cfg) {
> +			ret = k230_clk_mux_get_parent_data(cfg, parent_data);
> +			if (ret)
> +				return ret;
> +
[clip]
> +
> +static const struct of_device_id k230_clk_ids[] = {
> +	{ .compatible = "canaan,k230-clk" },
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, k230_clk_ids);
> +
> +static struct platform_driver k230_clk_driver = {
> +	.driver = {
> +		.name  = "k230_clock_controller",

extra ' ' after .name

> +		.of_match_table = k230_clk_ids,
> +	},
> +	.probe = k230_clk_probe,
> +};
> +builtin_platform_driver(k230_clk_driver);


Thanks,
Alok


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv