| 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:
<MA0P287MB28223E4F14E6D08E1139CA63FE042@MA0P287MB2822.INDP287.PROD.OUTLOOK.COM>
Date: Fri, 12 Apr 2024 18:23:08 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Stephen Boyd <sboyd@...nel.org>, Chen Wang <unicornxw@...il.com>,
aou@...s.berkeley.edu, chao.wei@...hgo.com, conor@...nel.org,
devicetree@...r.kernel.org, guoren@...nel.org, haijiao.liu@...hgo.com,
inochiama@...look.com, jszhang@...nel.org,
krzysztof.kozlowski+dt@...aro.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
mturquette@...libre.com, palmer@...belt.com, paul.walmsley@...ive.com,
richardcochran@...il.com, robh+dt@...nel.org, samuel.holland@...ive.com,
xiaoguang.xing@...hgo.com
Subject: Re: [PATCH v13 4/5] clk: sophgo: Add SG2042 clock driver
My feedback embedded.
On 2024/4/11 12:11, Stephen Boyd wrote:
> Quoting Chen Wang (2024-03-28 23:21:40)
>> From: Chen Wang <unicorn_wang@...look.com>
>>
>> Add a driver for the SOPHGO SG2042 clocks.
>>
>> Signed-off-by: Chen Wang <unicorn_wang@...look.com>
>> ---
>> drivers/clk/Kconfig | 1 +
>> drivers/clk/Makefile | 1 +
>> drivers/clk/sophgo/Kconfig | 7 +
>> drivers/clk/sophgo/Makefile | 2 +
>> drivers/clk/sophgo/clk-sophgo-sg2042.c | 1410 ++++++++++++++++++++++++
>> drivers/clk/sophgo/clk-sophgo-sg2042.h | 216 ++++
> Inline the contents of this file into the driver C file.
ok, now only one c source is needed.
>> diff --git a/drivers/clk/sophgo/clk-sophgo-sg2042.c b/drivers/clk/sophgo/clk-sophgo-sg2042.c
>> new file mode 100644
>> index 000000000000..7b468e7299ae
>> --- /dev/null
>> +++ b/drivers/clk/sophgo/clk-sophgo-sg2042.c
>> @@ -0,0 +1,1410 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Sophgo SG2042 Clock Generator Driver
>> + *
>> + * Copyright (C) 2024 Sophgo Technology Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/platform_device.h>
> Need include for BIT macro, io.h for readl/writel, do_div() header,
> ARRAY_SIZE, etc. please check.
ok, will check and add them, but I did not see warnings/errors when I
build with W=1
[...]
>> +static inline int sg2042_pll_enable(struct sg2042_pll_clock *pll, bool en)
>> +{
>> + unsigned int value = 0;
> Don't need to initialize value? Also, type should be u32.
Sure, no initializatation is required here, and u32 should be better, I
see some other places have the same problem, will fix them too.
[...]
>> +static unsigned long sg2042_pll_recalc_rate(unsigned int reg_value,
>> + unsigned long parent_rate)
>> +{
>> + struct sg2042_pll_ctrl ctrl_table;
>> + u64 rate, numerator, denominator;
>> +
>> + sg2042_pll_ctrl_decode(reg_value, &ctrl_table);
>> +
>> + numerator = parent_rate * ctrl_table.fbdiv;
>> + denominator = ctrl_table.refdiv * ctrl_table.postdiv1 * ctrl_table.postdiv2;
>> + do_div(numerator, denominator);
>> + rate = numerator;
>> +
>> + return rate;
> return numerator
yes, my stupid, will fix it, thanks.
[...]
>> +
>> + static int postdiv1_2[][3] = {
> const
Good catch, will fix.
[...]
>> +static unsigned long sg2042_clk_divider_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> + struct sg2042_divider_clock *divider = to_sg2042_clk_divider(hw);
>> + unsigned int val;
>> + unsigned long ret_rate;
>> +
>> + if (!(readl(divider->reg) & BIT(3))) {
> What is bit 3? Please make a define.
ok, will fix it and I see some other places have the similar problem,
will fix also.
[...]
>> + }
>> + val |= value << divider->shift;
>> + val |= 1 << 3;
> Make a define for bit 3 and bit 0 please.
Same as the issue upon, will fix it. thanks.
[...]
>> +
>> +static const char *const clk_mux_ddr01_p[] = {
>> + "clk_div_ddr01_0", "clk_div_ddr01_1"};
>> +static const char *const clk_mux_ddr23_p[] = {
>> + "clk_div_ddr23_0", "clk_div_ddr23_1"};
>> +static const char *const clk_mux_rp_cpu_normal_p[] = {
>> + "clk_div_rp_cpu_normal_0", "clk_div_rp_cpu_normal_1"};
>> +static const char *const clk_mux_axi_ddr_p[] = {
>> + "clk_div_axi_ddr_0", "clk_div_axi_ddr_1"};
>> +
>> +static struct sg2042_mux_clock sg2042_mux_clks[] = {
>> + SG2042_MUX(MUX_CLK_DDR01, "clk_mux_ddr01", clk_mux_ddr01_p,
> Please use struct clk_parent_data or struct clk_hw directly instead of
> string names.
Learned and will use clk_parent_data, thanks.
[......]
>> + hw = &pll->hw;
>> + ret = clk_hw_register(NULL, hw);
> Use devm_clk_hw_register() and pass a device.
yes, will use devm_xxx.
[......]
>> +
>> + hw = clk_hw_register_mux_table(NULL,
> Pass a device and use devm.
Yes, will use devm_xxx.
[......]
>> +
>> + clk_data->iobase = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
> Why can't we use devm_platform_ioremap_resource()?
yes, will change todevm_platform_ioremap_resource
[......]
>> +
>> + num_clks = ARRAY_SIZE(sg2042_pll_clks);
>> + if (num_clks == 0) {
>> + ret = -EINVAL;
>> + goto error_out;
>> + }
> This is dead code, please remove.
Thanks, you are right, I will remove it together with some other dead
code similiar.
>> +
>> +/*
>> + * Divider clock
>> + * @hw: clk_hw for initialization
>> + * @id: used to map clk_onecell_data
>> + * @reg: used for readl/writel.
>> + * **NOTE**: DIV registers are ALL in CLOCK!
>> + * @lock: spinlock to protect register access, modification of
>> + * frequency can only be served one at the time
>> + * @offset_ctrl: offset of divider control registers
>> + * @shift: shift of "Clock Divider Factor" in divider control register
>> + * @width: width of "Clock Divider Factor" in divider control register
>> + * @div_flags: private flags for this clock, not for framework-specific
>> + * @initval: In the divider control register, we can configure whether
>> + * to use the value of "Clock Divider Factor" or just use
>> + * the initial value pre-configured by IC. BIT[3] controls
>> + * this and by default (value is 0), means initial value
>> + * is used.
>> + * **NOTE** that we cannot read the initial value (default
>> + * value when poweron) and default value of "Clock Divider
>> + * Factor" is zero, which I think is a hardware design flaw
>> + * and should be sync-ed with the initial value. So in
>> + * software we have to add a configuration item (initval)
>> + * to manually configure this value and use it when BIT[3]
>> + * is zero.
> Do you use the init clk_op for this?
I think this does not apply to init ops, because I want to configure
some parameters here, rather than communicate with the physical clocks
and do any special initialization work. As I explained in the comments,
due to hardware design issues, when the clock use the IC default
initval, we cannot read the current initval from the register, so we can
only use the initval we configure in code as current factor value.
[......]
>> +
>> +/*
>> + * Gate clock
>> + * @hw: clk_hw for initialization
>> + * @id: used to map clk_onecell_data
>> + * @offset_enable: offset of gate enable registers
>> + * @bit_idx: which bit in the register controls gating of this clock
>> + */
>> +struct sg2042_gate_clock {
>> + struct clk_hw hw;
>> +
>> + unsigned int id;
>> +
>> + unsigned long offset_enable;
> Usually we use a u32 or a shorter size so that the member width is
> unchanged on different CPU architecture.
OK, I find some else similar problem and will fix them altogether.
[......]
>> + * Mux clock
> Please use kernel doc. See https://docs.kernel.org/doc-guide/kernel-doc.html
Sure，I will learn and try this.
>> + * @hw: clk_hw for initialization
>> + * @id: used to map clk_onecell_data
>> + * @offset_select: offset of mux selection registers
>> + * **NOTE**: MUX registers are ALL in CLOCK!
>> + * @shift: shift of "Clock Select" in mux selection register
> Is "Clock select" actually @offset_select?
No, the "offset_select" is the offset of register, while "Clock Select"
is the bit shift for each mux. For SG2042, every mux has two parents and
so just one bit is enough for it to select the parent, and one register
contains multiple bits, one for each mux.
[......]
Powered by blists - more mailing lists