[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-12-01 19:12:01)
> > > > > 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?
> 
> Looks OK.
> 
> >
> > #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,
> 
> Can you also show what soc0_ahbmux is?
It will be following. 

static const struct clk_parent_data soc0_ahbmux[] = {
	{ .fw_name = "soc0-ahbmux", .name = "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?
> >
> 
> The 'table' member is optional. When the table is NULL, we treat each
> number that fits into the divider field width as a divider. It may be
> CLK_DIVIDER_EVEN_INTEGERS that you have here though.

Got it, will use this flag.