Message-ID:
 <MA0P287MB28222ACCE844092F11B6008AFEFA2@MA0P287MB2822.INDP287.PROD.OUTLOOK.COM>
Date: Thu, 6 Jun 2024 13:57:08 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Emil Renner Berthing <emil.renner.berthing@...onical.com>,
 Chen Wang <unicornxw@...il.com>, aou@...s.berkeley.edu, chao.wei@...hgo.com,
 conor@...nel.org, krzysztof.kozlowski+dt@...aro.org,
 mturquette@...libre.com, palmer@...belt.com, paul.walmsley@...ive.com,
 richardcochran@...il.com, robh+dt@...nel.org, sboyd@...nel.org,
 devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
 haijiao.liu@...hgo.com, xiaoguang.xing@...hgo.com, guoren@...nel.org,
 jszhang@...nel.org, inochiama@...look.com, samuel.holland@...ive.com
Subject: Re: [PATCH v15 4/5] clk: sophgo: Add SG2042 clock driver


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@...ts.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists