| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <950ec364-53a8-4795-a8d7-788bedb91bd0@zohomail.com>
Date: Mon, 21 Apr 2025 18:47:18 +0800
From: Xukai Wang <kingxukai@...omail.com>
To: ALOK TIWARI <alok.a.tiwari@...cle.com>,
Michael Turquette <mturquette@...libre.com>, Stephen Boyd
<sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
Conor Dooley <conor@...nel.org>
Cc: linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
Samuel Holland <samuel.holland@...ive.com>,
Troy Mitchell <TroyMitchell988@...il.com>
Subject: Re: PATCH v6 2/3] clk: canaan: Add clock driver for Canaan K230
On 2025/4/21 02:08, ALOK TIWARI wrote:
> 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@...il.com>
>> Signed-off-by: Troy Mitchell <TroyMitchell988@...il.com>
>> Signed-off-by: Xukai Wang <kingxukai@...omail.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
I see, I'll replace them with lowercase next version.
>> +/* PLL lock register bits. */
>
> extra ' ' after bits.
OK, I'll drop it.
>
>> +#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
OK, I'll add a non-zeo check before division operation.
>
>> + 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, ... */
OK, I get it. I'll modify it next version.
>
>> + 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 */
>
OK.
>> + 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?"
I think so, and I'll add a check in `k230_clk_find_approximate()`.
>
>> + 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)
Seems better here. I'll modify it.
>
>> + 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 ?
>
Yes, child is more appropriate here. I'll modify the comment next version.
>> + 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
I'll drop it.
>
>> + .of_match_table = k230_clk_ids,
>> + },
>> + .probe = k230_clk_probe,
>> +};
>> +builtin_platform_driver(k230_clk_driver);
>
>
> Thanks,
> Alok
Thanks for your time and patient review! :)
Powered by blists - more mailing lists