Re: [PATCH v3 2/2] clk: sophgo: Add clock controller support for SG2044 SoC

[Stephen Boyd <sboyd@kernel.org>]

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@gmail.com>
> + */
> +
> +#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@outlook.com>
> + */
> +
> +#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;
> +};
> +