Re: [PATCH v15 4/5] clk: sophgo: Add SG2042 clock driver

[Chen Wang]

On 2024/4/30 15:48, Emil Renner Berthing wrote:
> Chen Wang wrote:
[......]
> > +#define R_MP14_CONTROL_REG	(0x03F4 - R_SYSGATE_BEGIN)
> > +#define R_MP15_STATUS_REG	(0x03F8 - R_SYSGATE_BEGIN)
> > +#define R_MP15_CONTROL_REG	(0x03FC - R_SYSGATE_BEGIN)
> All these seem pretty regular to me. Couldn't they just be
> #define R_MP_STATUS_REG(x)	(0x380 + 8*(x) - R_SYSGATE_BEGIN)
> #define R_MP_CONTROL_REG(x)	(0x384 + 8*(x) - R_SYSGATE_BEGIN)

I still prefer to keep the original writing method, because using 
immediate values ​​can be consistent with the register description in 
the chip technical manual, which is convenient for comparison and search.

Thanks anyway.

[......]

> > +#define R_CLKDIVREG29		0xB4
> > +#define R_CLKDIVREG30		0xB8
> #define R_CLKDIVREG(x)	(0x40 + 4*(x))

Same reason as above.

[......]

> > +#define PLLCTRL_FBDIV_SHIFT	16
> > +#define PLLCTRL_FBDIV_MASK	(GENMASK(27, 16) >> PLLCTRL_FBDIV_SHIFT)
> > +#define PLLCTRL_POSTDIV2_SHIFT	12
> > +#define PLLCTRL_POSTDIV2_MASK	(GENMASK(14, 12) >> PLLCTRL_POSTDIV2_SHIFT)
> > +#define PLLCTRL_POSTDIV1_SHIFT	8
> > +#define PLLCTRL_POSTDIV1_MASK	(GENMASK(10, 8) >> PLLCTRL_POSTDIV1_SHIFT)
> > +#define PLLCTRL_REFDIV_SHIFT	0
> > +#define PLLCTRL_REFDIV_MASK	(GENMASK(5, 0) >> PLLCTRL_REFDIV_SHIFT)
> You'll only need half of these defines if you use..
Yes, great advice, will change it.
> +
> +static inline u32 sg2042_pll_ctrl_encode(struct sg2042_pll_ctrl *ctrl)
> +{
> +	return ((ctrl->fbdiv & PLLCTRL_FBDIV_MASK) << PLLCTRL_FBDIV_SHIFT) |
> +	       ((ctrl->postdiv2 & PLLCTRL_POSTDIV2_MASK) << PLLCTRL_POSTDIV2_SHIFT) |
> +	       ((ctrl->postdiv1 & PLLCTRL_POSTDIV1_MASK) << PLLCTRL_POSTDIV1_SHIFT) |
> +	       ((ctrl->refdiv & PLLCTRL_REFDIV_MASK) << PLLCTRL_REFDIV_SHIFT);
> ..the FIELD_PREP macro here..
>
> > +}
> > +
> > +static inline void sg2042_pll_ctrl_decode(unsigned int reg_value,
> > +					  struct sg2042_pll_ctrl *ctrl)
> > +{
> > +	ctrl->fbdiv = (reg_value >> PLLCTRL_FBDIV_SHIFT) & PLLCTRL_FBDIV_MASK;
> > +	ctrl->refdiv = (reg_value >> PLLCTRL_REFDIV_SHIFT) & PLLCTRL_REFDIV_MASK;
> > +	ctrl->postdiv1 = (reg_value >> PLLCTRL_POSTDIV1_SHIFT) & PLLCTRL_POSTDIV1_MASK;
> > +	ctrl->postdiv2 = (reg_value >> PLLCTRL_POSTDIV2_SHIFT) & PLLCTRL_POSTDIV2_MASK;
> ..and the FIELD_GET macro here.
[......]
> +
> +#define SG2042_PLL_FW_RO(_id, _name, _parent, _r_stat, _r_enable, _r_ctrl, _shift) \
> +	{								\
> +		.hw.init = CLK_HW_INIT_FW_NAME(				\
> +				_name,					\
> +				_parent,				\
> +				&sg2042_clk_pll_ro_ops,			\
> +				CLK_GET_RATE_NOCACHE | CLK_GET_ACCURACY_NOCACHE),\
> +		.id = _id,						\
> +		.offset_ctrl = _r_ctrl,					\
> +		.offset_status = _r_stat,				\
> +		.offset_enable = _r_enable,				\
> +		.shift_status_lock = 8 + (_shift),			\
> +		.shift_status_updating = _shift,			\
> +		.shift_enable = _shift,					\
> +	}
> These macros are pretty confusing. Please at least try to keep the arguments
> and assignments in close to the same order.
Accept, thanks.
> > +
> > +static struct sg2042_pll_clock sg2042_pll_clks[] = {
> > +	SG2042_PLL_FW(MPLL_CLK, "mpll_clock", "cgi_main",
> > +		      R_PLL_STAT, R_PLL_CLKEN_CONTROL, R_MPLL_CONTROL, 0),
> > +	SG2042_PLL_FW_RO(FPLL_CLK, "fpll_clock", "cgi_main",
> > +			 R_PLL_STAT, R_PLL_CLKEN_CONTROL, R_FPLL_CONTROL, 3),
> > +	SG2042_PLL_FW_RO(DPLL0_CLK, "dpll0_clock", "cgi_dpll0",
> > +			 R_PLL_STAT, R_PLL_CLKEN_CONTROL, R_DPLL0_CONTROL, 4),
> > +	SG2042_PLL_FW_RO(DPLL1_CLK, "dpll1_clock", "cgi_dpll1",
> > +			 R_PLL_STAT, R_PLL_CLKEN_CONTROL, R_DPLL1_CONTROL, 5),
> Also the STAT and CLK_GEN_CONTROL registers seem to be the same offset for all
> the PLLs. Why do you need to store them in memory?

Yes, you are right, will correct this.

[......]

> > +static int sg2042_mux_notifier_cb(struct notifier_block *nb,
> > +				  unsigned long event,
> > +				  void *data)
> > +{
> > +	int ret = 0;
> > +	struct clk_notifier_data *ndata = data;
> > +	struct clk_hw *hw = __clk_get_hw(ndata->clk);
> > +	const struct clk_ops *ops = &clk_mux_ops;
> > +	struct sg2042_mux_clock *mux = to_sg2042_mux_nb(nb);
> Please order these by line length where possible. That goes for the whole file.
>
> Eg. look up 'reverse xmas tree' in fx.
> Documentation/process/maintainer-netdev.rst

OK, will improve this.

[......]

> > +static int sg2042_clk_register_muxs(struct device *dev,
> > +				    struct sg2042_clk_data *clk_data,
> > +				    struct sg2042_mux_clock mux_clks[],
> > +				    int num_mux_clks)
> > +{
> > +	struct clk_hw *hw;
> > +	struct sg2042_mux_clock *mux;
> > +	int i, ret = 0;
> > +
> > +	for (i = 0; i < num_mux_clks; i++) {
> > +		mux = &mux_clks[i];
> > +
> > +		hw = __devm_clk_hw_register_mux
> > +			(dev,
> > +			 NULL,
> > +			 mux->hw.init->name,
> > +			 mux->hw.init->num_parents,
> > +			 NULL,
> > +			 mux->hw.init->parent_hws,
> > +			 NULL,
> > +			 mux->hw.init->flags,
> > +			 clk_data->iobase + mux->offset_select,
> > +			 mux->shift,
> > +			 BIT(mux->width) - 1,
> > +			 0,
> > +			 sg2042_mux_table,
> > +			 &sg2042_clk_lock);
> Does checkpatch.pl --strict not warn about this interesting indentation?

No, I always run checkpatch --strict before sending out and don't see 
warn on this.

[......]

> > +static int sg2042_init_clkdata(struct platform_device *pdev,
> > +			       int num_clks,
> > +			       struct sg2042_clk_data **pp_clk_data)
> > +{
> > +	struct sg2042_clk_data *clk_data = NULL;
> No need to initialize this. You're setting it on the line just below. There are
> many other instances of this throughout the file.

Accept，I will double check this.

[......]

> > +static const struct of_device_id sg2042_clkgen_match[] = {
> > +	{ .compatible = "sophgo,sg2042-clkgen" },
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static struct platform_driver sg2042_clkgen_driver = {
> > +	.probe = sg2042_clkgen_probe,
> > +	.driver = {
> > +		.name = "clk-sophgo-sg2042-clkgen",
> > +		.of_match_table = sg2042_clkgen_match,
> > +		.suppress_bind_attrs = true,
> > +	},
> > +};
> > +builtin_platform_driver(sg2042_clkgen_driver);
> > +
> > +static const struct of_device_id sg2042_rpgate_match[] = {
> > +	{ .compatible = "sophgo,sg2042-rpgate" },
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static struct platform_driver sg2042_rpgate_driver = {
> > +	.probe = sg2042_rpgate_probe,
> > +	.driver = {
> > +		.name = "clk-sophgo-sg2042-rpgate",
> > +		.of_match_table = sg2042_rpgate_match,
> > +		.suppress_bind_attrs = true,
> > +	},
> > +};
> > +builtin_platform_driver(sg2042_rpgate_driver);
> > +
> > +static const struct of_device_id sg2042_pll_match[] = {
> > +	{ .compatible = "sophgo,sg2042-pll" },
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static struct platform_driver sg2042_pll_driver = {
> > +	.probe = sg2042_pll_probe,
> > +	.driver = {
> > +		.name = "clk-sophgo-sg2042-pll",
> > +		.of_match_table = sg2042_pll_match,
> > +		.suppress_bind_attrs = true,
> > +	},
> > +};
> > +builtin_platform_driver(sg2042_pll_driver);
> You have 3 different drivers in one file. Could this not be split into 3
> different modules? This file is almost 2k lines long already.
OK, splitting this file into 3 should be better.
> Also do they all need to be built in? Eg. could some of them not be loaded as a
> module later in the boot process or are they all critical to get to loading the
> initramfs?

It doesn't seem to be a built-in, I will modify it, thanks for your 
suggestion.

Regards,

Chen

> /Emil
>
> > --
> > 2.25.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-riscv