On Tue, Mar 11, 2025 at 12:23:35PM -0700, Stephen Boyd wrote:
> 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)?
>
Yeah, using abs is more better.
> > +}
> > +
> > +
> > +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.
>
This is OK for me, I will change all to parent_hws.
> > +
> > +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?
>
It can not be, we need a non const clk_hw to register. It is
defined in this structure. Although these array can be set as
"struct sg2044_clk_common * const", but I think this is kind
of meaningless.
> > + &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.
>
OK, I will take it.
> > +#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;
> > +};
> > +
