| 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: <aUS9bLVLhIPMOcWa@kernel.org>
Date: Fri, 19 Dec 2025 10:50:20 +0800
From: Troy Mitchell <troy.mitchell@...ux.dev>
To: dongxuyang@...incomputing.com, mturquette@...libre.com,
sboyd@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
troy.mitchell@...ux.dev, bmasney@...hat.com
Cc: ningyu@...incomputing.com, linmin@...incomputing.com,
huangyifeng@...incomputing.com, pinkesh.vaghela@...fochips.com
Subject: Re: [PATCH v8 2/3] clock: eswin: Add eic7700 clock driver
Hi Xuyang,
This is a quick review.
On Thu, Nov 13, 2025 at 09:38:46AM +0800, dongxuyang@...incomputing.com wrote:
> From: Xuyang Dong <dongxuyang@...incomputing.com>
>
> Add clock drivers for the EIC7700 SoC. The clock controller on the ESWIN
> EIC7700 provides various clocks to different IP blocks within the SoC.
>
> Signed-off-by: Yifeng Huang <huangyifeng@...incomputing.com>
> Signed-off-by: Xuyang Dong <dongxuyang@...incomputing.com>
> ---
> drivers/clk/Kconfig | 1 +
> drivers/clk/Makefile | 1 +
> drivers/clk/eswin/Kconfig | 15 +
> drivers/clk/eswin/Makefile | 8 +
> drivers/clk/eswin/clk-eic7700.c | 1033 +++++++++++++++++++++++++++++++
> drivers/clk/eswin/clk-eic7700.h | 122 ++++
> drivers/clk/eswin/clk.c | 481 ++++++++++++++
> drivers/clk/eswin/clk.h | 256 ++++++++
> 8 files changed, 1917 insertions(+)
> create mode 100644 drivers/clk/eswin/Kconfig
> create mode 100644 drivers/clk/eswin/Makefile
> create mode 100644 drivers/clk/eswin/clk-eic7700.c
> create mode 100644 drivers/clk/eswin/clk-eic7700.h
> create mode 100644 drivers/clk/eswin/clk.c
> create mode 100644 drivers/clk/eswin/clk.h
>
[...]
> diff --git a/drivers/clk/eswin/clk-eic7700.c b/drivers/clk/eswin/clk-eic7700.c
> new file mode 100644
> index 000000000000..03540db9cb7d
> --- /dev/null
> +++ b/drivers/clk/eswin/clk-eic7700.c
[...]
> +static int eic7700_clk_probe(struct platform_device *pdev)
> +{
> + struct eswin_clock_data *clk_data;
> + struct device *dev = &pdev->dev;
> +
> + clk_data = eswin_clk_init(dev, EIC7700_NR_CLKS);
> + if (!clk_data)
> + return dev_err_probe(dev, -EAGAIN, "failed to get clk data!\n");
> +
> + eswin_clk_register_fixed_rate(eic7700_fixed_rate_clks,
> + ARRAY_SIZE(eic7700_fixed_rate_clks),
> + clk_data, dev);
Please check returned value here and check other places.
> +
> + eswin_clk_register_pll(eic7700_pll_clks, ARRAY_SIZE(eic7700_pll_clks),
> + clk_data, dev);
> +
> + eswin_clk_register_fixed_factor(eic7700_fixed_factor_clks,
> + ARRAY_SIZE(eic7700_fixed_factor_clks),
> + clk_data, dev);
> +
> + eswin_clk_register_mux(eic7700_mux_clks, ARRAY_SIZE(eic7700_mux_clks),
> + clk_data, dev);
> +
> + eswin_clk_register_divider(eic7700_div_clks,
> + ARRAY_SIZE(eic7700_div_clks), clk_data, dev);
> +
> + eswin_clk_register_gate(eic7700_gate_clks,
> + ARRAY_SIZE(eic7700_gate_clks), clk_data, dev);
> +
> + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> + &clk_data->clk_data);
> +}
> +
[...]
> diff --git a/drivers/clk/eswin/clk.c b/drivers/clk/eswin/clk.c
> new file mode 100644
> index 000000000000..92998da98009
> --- /dev/null
> +++ b/drivers/clk/eswin/clk.c
[...]
> +static int eswin_calc_pll(u32 *frac_val, u32 *fbdiv_val, u64 rate,
> + const struct eswin_clk_pll *clk)
> +{
> + u64 rem = 0;
> + u32 tmp1 = 0, tmp2 = 0;
> +
> + if (clk->id == EIC7700_CLK_APLL_FOUT1 ||
> + clk->id == EIC7700_CLK_PLL_CPU) {
clk.c appears to be a generic library for ESWIN SoCs, but it references
specific IDs (EIC7700_...) from a specific chip.
If you add the chip like EIC77XX later, will you add more if/else here?
Could you ove the algorithmic parameters (like rate * 4) into the
eswin_clk_pll structure flags or data fields. The calculation logic
should be generic based on the struct fields, not the ID.
Please check other places.
> + rate = rate * 4;
> + rem = do_div(rate, 1000);
> + if (rem)
> + tmp1 = rem;
> +
> + rem = do_div(rate, 1000);
> + if (rem)
> + tmp2 = rem;
> +
> + rem = do_div(rate, 24);
> + /* fbdiv = rate * 4 / 24000000 */
> + *fbdiv_val = rate;
> + /* frac = rate * 4 % 24000000 * (2 ^ 24) */
> + *frac_val = (u64)((1000 * (1000 * rem + tmp2) + tmp1) << 24)
> + / 24 / 1000000;
> + } else {
> + pr_err("Invalid pll set req, rate %lld, clk id %d\n", rate,
> + clk->id);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static inline struct eswin_clk_pll *to_pll_clk(struct clk_hw *hw)
> +{
> + return container_of(hw, struct eswin_clk_pll, hw);
> +}
> +
> +static int clk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct eswin_clk_pll *clk = to_pll_clk(hw);
> + struct clk *clk_cpu_lp_pll = NULL;
> + struct clk *clk_cpu_mux = NULL;
> + struct clk *clk_cpu_pll = NULL;
> + u32 postdiv1_val = 0, refdiv_val = 1;
> + u32 frac_val, fbdiv_val, val;
> + bool lock_flag = false;
> + int try_count = 0;
> + int ret;
> +
> + ret = eswin_calc_pll(&frac_val, &fbdiv_val, (u64)rate, clk);
> + if (ret)
> + return ret;
> +
> + /* Must switch the CPU to other CLK before we change the CPU PLL. */
> + if (clk->id == EIC7700_CLK_PLL_CPU) {
> + clk_cpu_mux = __clk_lookup("mux_cpu_root_3mux1_gfree");
It seems you want to switch to a safe clock source before setting up the
PLL, right?
I am not sure whether your approach is correct, but the use of
__clk_lookup() should be avoided whenever possible.
Would it be feasible to obtain a proper clock handle somewhere and
perform the necessary configuration from within a clk_notifier instead?
> + if (!clk_cpu_mux) {
> + pr_err("failed to get clk: %s\n",
> + "mux_cpu_root_3mux1_gfree");
> + return -EINVAL;
> + }
Powered by blists - more mailing lists