[PATCH v15 4/5] clk: sophgo: Add SG2042 clock driver
Chen Wang
unicorn_wang at outlook.com
Wed Jun 5 22:57:08 PDT 2024
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 at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
More information about the linux-riscv
mailing list