[PATCH v7 3/3] clk: aspeed: add AST2700 clock driver.

[Ryan Chen]

> > > Subject: Re: [PATCH v7 3/3] clk: aspeed: add AST2700 clock driver.
> > >
> > > Quoting Ryan Chen (2024-10-27 22:30:18)
> > > > diff --git a/drivers/clk/clk-ast2700.c b/drivers/clk/clk-ast2700.c
> > > > new file mode 100644 index 000000000000..db9ee5031b7c
> > > > --- /dev/null
> > > > +++ b/drivers/clk/clk-ast2700.c
> > > > @@ -0,0 +1,1513 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> [...]
> > > > +struct ast2700_clk_info {
> > > > +       const char *name;
> > > > +       const char * const *parent_names;
> > >
> > > Please don't use strings for parent names.
> > Sorry, do you mean use clk_parent_data struct for parent?
> >         +const struct clk_parent_data   parent;         /* For gate */
> >         +const struct clk_parent_data   *parents;               /*
> For mux */
> 
> Yes.
And I find a better way for parent_data. The following is my modification. 
And parent_data will be union data structure. Like following.
Is this good direction? 

#define DIVIDER_CLK(_id, _name, _parent, _reg, _shift, _width, _div_table) \
	[_id] = { \
		.type = CLK_DIVIDER, \
		.name = _name, \
		.data = { \
			.div = { \
				.parent = _parent, \
				.reg = _reg, \
				.bit_shift = _shift, \
				.bit_width = _width, \
				.div_table = _div_table, \
			}, \
		}, \
	}
struct ast2700_clk_info {
	const char *name;
	u8 clk_idx;
	u32 reg;
	u32 type;
	union {
		struct ast2700_clk_fixed_factor_data factor;
		struct ast2700_clk_fixed_rate_data rate;
		struct ast2700_clk_gate_data gate;
		struct ast2700_clk_div_data div;
		struct ast2700_clk_pll_data pll;
		struct ast2700_clk_mux_data mux;
	} data;
};

struct ast2700_clk_div_data {
	const struct clk_div_table *div_table;
	const struct clk_parent_data *parent;
	u8 bit_shift;
	u8 bit_width;
	u32 reg;
};

static const struct ast2700_clk_info ast2700_scu0_clk_info[] __initconst = {
...........................
	DIVIDER_CLK(SCU0_CLK_AHB, "soc0-ahb", soc0_ahbmux,
		    SCU0_HWSTRAP1, 5, 2, ast2700_hclk_div_table),
......................
> 
> >
> > >
> > > > +       const struct clk_div_table *div_table;
> > > > +       unsigned long fixed_rate;
> > > > +       unsigned int mult;
> > > > +       unsigned int div;
> > > > +       u32 reg;
> > > > +       u32 flags;
> > > > +       u32 type;
> > > > +       u8 clk_idx;
> > > > +       u8 bit_shift;
> > > > +       u8 bit_width;
> > > > +       u8 num_parents;
> > > > +};
> > > > +
> > > [...]
> > > > +
> > > > +static const struct clk_div_table ast2700_clk_div_table2[] = {
> > > > +       { 0x0, 2 },
> > > > +       { 0x1, 4 },
> > > > +       { 0x2, 6 },
> > > > +       { 0x3, 8 },
> > > > +       { 0x4, 10 },
> > > > +       { 0x5, 12 },
> > > > +       { 0x6, 14 },
> > > > +       { 0x7, 16 },
> > >
> > > Isn't this the default divider setting for struct clk_divider?
> > Sorry, I don't catch your point.
> > the SoC do have default divider setting. But it can be modified.
> > And also have different divider table setting.
> 
> I mean that this is the way that struct clk_divider works already. So you don't
> need to make the clk_div_table array for what is supported in code.

Sorry, I understand your point. But I trace the code didn't get any clue. "clk_divider work already".
finally function call will be __clk_hw_register_divider
https://github.com/torvalds/linux/blob/master/drivers/clk/clk-divider.c#L589
It still need table point need to address. Can you give me more direction or example?


> 
> > >
> > > > +       { 0 }
> > > > +};
> > > > +
> > > > +static const struct clk_div_table ast2700_clk_uart_div_table[] = {
> > > > +       { 0x0, 1 },
> > > > +       { 0x1, 13 },
> > > > +       { 0 }
> > > [...]
> > > > +               .bit_shift = 23,
> > > > +               .bit_width = 3,
> > > > +               .div_table = ast2700_clk_div_table2,
> > > > +       },
> > > > +       [SCU0_CLK_GATE_MCLK] = {
> > > > +               .type = CLK_GATE_ASPEED,
> > > > +               .name = "mclk-gate",
> > > > +               .parent_names = (const char *[]){ "soc0-mpll", },
> > > > +               .reg = SCU0_CLK_STOP,
> > > > +               .clk_idx = 0,
> > > > +               .flags = CLK_IS_CRITICAL,
> > > > +       },
> > > > +       [SCU0_CLK_GATE_ECLK] = {
> > > > +               .type = CLK_GATE_ASPEED,
> > > > +               .name = "eclk-gate",
> > > > +               .parent_names = (const char *[]){  },
> > > > +               .reg = SCU0_CLK_STOP,
> > > > +               .clk_idx = 1,
> > > > +       },
> > > > +       [SCU0_CLK_GATE_2DCLK] = {
> > > > +               .type = CLK_GATE_ASPEED,
> > > > +               .name = "gclk-gate",
> > > > +               .parent_names = (const char *[]){  },
> > >
> > > This has no parent? Why is parent_names set to an empty array?
> > Due to I use clk->parent_names[0] for clk_hw_register_gate, const char
> *name parameter input.
> > If null, that will cause panic for NULL point.
> 
> But the parent is NULL? How many parents does this clk have?

I will remove this in array.
I will do parent_names = parent ? &parent->name : NULL; to check null.
> 
> > >
> > > > +       if (!clk_data)
> > > > +               return devm_of_platform_populate(dev);
> > >
> > > What is being populated? Isn't there always clk_data?
> > Yes, it is always clk_data, I will modify to be following, is it ok?
> > If(!clk_data)
> >         Return -ENODEV;
> >
> 
> Sure.
> 
> > >
> > > Please don't use strings for parent_names. Use clk_hw pointers or DT
> indices.
> > Use clk_pareent_data is it ok ?
> 
> Yes.