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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux