Quoting Inochi Amaoto (2025-02-26 15:23:19)
> diff --git a/drivers/clk/sophgo/clk-sg2044.c b/drivers/clk/sophgo/clk-sg2044.c
> new file mode 100644
> index 000000000000..b4c15746de77
> --- /dev/null
> +++ b/drivers/clk/sophgo/clk-sg2044.c
Thanks for sticking with it. Some minor nits below but otherwise this
looks good to go.
> @@ -0,0 +1,2271 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sophgo SG2042 clock controller Driver
> + *
> + * Copyright (C) 2025 Inochi Amaoto <inochiama@xxxxxxxxx>
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/math64.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include "clk-sg2044.h"
> +
> +#include <dt-bindings/clock/sophgo,sg2044-clk.h>
> +
[...]
> +
> +static unsigned long sg2044_pll_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct sg2044_pll *pll = hw_to_sg2044_pll(hw);
> + u32 value;
> + int ret;
> +
> + ret = regmap_read(pll->top_syscon,
> + pll->syscon_offset + pll->pll.ctrl_offset + PLL_HIGH_CTRL_OFFSET,
> + &value);
> + if (ret < 0)
> + return 0;
> +
> + return sg2044_pll_calc_rate(parent_rate,
> + FIELD_GET(PLL_REFDIV_MASK, value),
> + FIELD_GET(PLL_FBDIV_MASK, value),
> + FIELD_GET(PLL_POSTDIV1_MASK, value),
> + FIELD_GET(PLL_POSTDIV2_MASK, value));
> +}
> +
> +static bool pll_is_better_rate(unsigned long target, unsigned long now,
> + unsigned long best)
> +{
> + return (target - now) < (target - best);
Is this more like abs_diff(target, now) < abs_diff(target, best)?
> +}
> +
> +
> +static int sg2044_pll_enable(struct sg2044_pll *pll, bool en)
> +{
> + if (en) {
> + if (sg2044_pll_poll_update(pll) < 0)
> + pr_warn("%s: fail to lock pll\n", clk_hw_get_name(&pll->common.hw));
> +
> + return regmap_set_bits(pll->top_syscon,
> + pll->syscon_offset + pll->pll.enable_offset,
> + BIT(pll->pll.enable_bit));
> + } else {
Drop the else please.
> + return regmap_clear_bits(pll->top_syscon,
> + pll->syscon_offset + pll->pll.enable_offset,
> + BIT(pll->pll.enable_bit));
> + }
> +}
> +
[...]
> +
> +static u32 sg2044_div_get_reg_div(u32 reg, struct sg2044_div_internal *div)
> +{
> + if ((reg & DIV_FACTOR_REG_SOURCE))
> + return (reg >> div->shift) & clk_div_mask(div->width);
> + else
Drop the else please.
> + return div->initval == 0 ? 1 : div->initval;
> +}
> +
> +
> +
[...]
> +
> +static const struct clk_parent_data clk_fpll0_parent[] = {
> + { .hw = &clk_fpll0.common.hw },
> +};
If the only parent is a clk_hw pointer it's preferred to use struct
clk_init_data::parent_hws directly instead of clk_parent_data.
> +
> +static const struct clk_parent_data clk_fpll1_parent[] = {
> + { .hw = &clk_fpll1.common.hw },
> +};
> +
[...]
> + CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO,
> + 80);
> +
> +static DEFINE_SG2044_DIV(CLK_DIV_PKA, clk_div_pka,
> + clk_fpll0_parent, 0,
> + 0x0f0, 16, 8,
> + CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO,
> + 2);
> +
> +static const struct clk_parent_data clk_mux_ddr0_parents[] = {
> + { .hw = &clk_div_ddr0_fixed.common.hw },
> + { .hw = &clk_div_ddr0_main.common.hw },
Similarly, if the only parents are clk_hw pointers we should be using
struct clk_init_data::parent_hws.
> +};
> +
> +static DEFINE_SG2044_MUX(CLK_MUX_DDR0, clk_mux_ddr0,
> +
> +static struct sg2044_clk_common *sg2044_pll_commons[] = {
> + &clk_fpll0.common,
[...]
> + &clk_mpll5.common,
> +};
> +
> +static struct sg2044_clk_common *sg2044_div_commons[] = {
> + &clk_div_ap_sys_fixed.common,
> + &clk_div_ap_sys_main.common,
[...]
> + &clk_div_pka.common,
> +};
> +
> +static struct sg2044_clk_common *sg2044_mux_commons[] = {
> + &clk_mux_ddr0.common,
[..]
> +};
> +
> +static struct sg2044_clk_common *sg2044_gate_commons[] = {
Can these arrays be const?
> + &clk_gate_ap_sys.common,
> diff --git a/drivers/clk/sophgo/clk-sg2044.h b/drivers/clk/sophgo/clk-sg2044.h
> new file mode 100644
> index 000000000000..bb69532fdf4f
> --- /dev/null
> +++ b/drivers/clk/sophgo/clk-sg2044.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2024 Inochi Amaoto <inochiama@xxxxxxxxxxx>
> + */
> +
> +#ifndef _CLK_SOPHGO_SG2044_H_
> +#define _CLK_SOPHGO_SG2044_H_
> +
> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/spinlock.h>
> +
Please inline the contents of this file in the one C file that uses the
header.
> +#define PLL_LIMIT_FOUTVCO 0
> +#define PLL_LIMIT_FOUT 1
> +#define PLL_LIMIT_REFDIV 2
> +#define PLL_LIMIT_FBDIV 3
> +#define PLL_LIMIT_POSTDIV1 4
> +#define PLL_LIMIT_POSTDIV2 5
> +
> +#define for_each_pll_limit_range(_var, _limit) \
> + for (_var = (_limit)->min; _var <= (_limit)->max; _var++)
> +
> +struct sg2044_clk_limit {
> + u64 min;
> + u64 max;
> +};
> +
