Re: [PATCH v3 6/6] clk: meson: t7: add t7 clock peripherals controller driver

[Jian Hu]

On 2025/5/14 16:00, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 09 May 2025 at 07:48, Jian Hu <jian.hu@xxxxxxxxxxx> wrote:
>
> > Add Peripheral clock controller driver for the Amlogic T7 SoC family.
> >
> > Signed-off-by: Jian Hu <jian.hu@xxxxxxxxxxx>
> > ---
> >   drivers/clk/meson/Kconfig          |   13 +
> >   drivers/clk/meson/Makefile         |    1 +
> >   drivers/clk/meson/t7-peripherals.c | 2359 ++++++++++++++++++++++++++++
> >   3 files changed, 2373 insertions(+)
> >   create mode 100644 drivers/clk/meson/t7-peripherals.c
> >
> > ......
> >
> > +
> > +static u32 t7_eth_rmii_table[] = { 0, 7 };
> > +
> > +static const struct clk_parent_data t7_eth_rmii_parents[] = {
> > +     { .fw_name = "fdiv2", },
> > +     { .fw_name = "rmii_pad", },
> Are you sure about that ? check the bindings


Ok, rmii_pad is optional parent, I will update it in DT bindings.

> > +
> > +#define SPI_PWM_CLK_MUX(_name, _reg, _mask, _shift, _parent_data) {  \
> > +     .data = &(struct clk_regmap_mux_data) {                 \
> > +             .offset = _reg,                                 \
> > +             .mask = _mask,                                  \
> > +             .shift = _shift,                                \
> > +     },                                                      \
> > +     .hw.init = &(struct clk_init_data) {                    \
> > +             .name = #_name "_sel",                          \
> > +             .ops = &clk_regmap_mux_ops,                     \
> > +             .parent_data = _parent_data,                    \
> > +             .num_parents = ARRAY_SIZE(_parent_data),        \
> > +     },                                                      \
> > +}
> > +
> > +#define SPI_PWM_CLK_DIV(_name, _reg, _shift, _width, _parent) {      \
> > +     .data = &(struct clk_regmap_div_data) {                 \
> > +             .offset = _reg,                                 \
> > +             .shift = _shift,                                \
> > +             .width = _width,                                \
> > +     },                                                      \
> > +     .hw.init = &(struct clk_init_data) {                    \
> > +             .name = #_name "_div",                          \
> > +             .ops = &clk_regmap_divider_ops,                 \
> > +             .parent_hws = (const struct clk_hw *[]) {       \
> > +                     &_parent.hw                             \
> > +             },                                              \
> > +             .num_parents = 1,                               \
> > +             .flags = CLK_SET_RATE_PARENT,                   \
> > +     },                                                      \
> > +}
> > +
> > +#define SPI_PWM_CLK_GATE(_name, _reg, _bit, _parent) {               \
> > +     .data = &(struct clk_regmap_gate_data) {                \
> > +             .offset = _reg,                                 \
> > +             .bit_idx = _bit,                                \
> > +     },                                                      \
> > +     .hw.init = &(struct clk_init_data) {                    \
> > +             .name = #_name,                                 \
> > +             .ops = &clk_regmap_gate_ops,                    \
> > +             .parent_hws = (const struct clk_hw *[]) {       \
> > +                     &_parent.hw                             \
> > +             },                                              \
> > +             .num_parents = 1,                               \
> > +             .flags = CLK_SET_RATE_PARENT,                   \
> > +     },                                                      \
> > +}
>
> Again that something that has been repeated for way too long.
> You'll wait for the clean-up to be done. If you want to help, you can
> review and test the patch being sent. It may speed things up.


I want to confirm here .

you said the clean-up patch is not related to spi and pwm clocks, Right ?


I can see the regmap drop table patch [0], is it the sending patch?


[0]: 
https://patchwork.kernel.org/project/linux-amlogic/patch/20250120-amlogic-clk-drop-clk-regmap-tables-v3-0-126244146947@xxxxxxxxxxxx/


I will apply the regmap clean-up patch serial and verified it on T7.


If not， Please correct me.

> > +
> > +static const struct clk_parent_data t7_spicc_parents[] = {
> > +     { .fw_name = "xtal", },
> > +     { .fw_name = "sys", },
> > +     { .fw_name = "fdiv4", },
> > +     { .fw_name = "fdiv3", },
> > +     { .fw_name = "fdiv2", },
> > +     { .fw_name = "fdiv5", },
> > +     { .fw_name = "fdiv7", },
> > +     { .fw_name = "gp1", },
> > +};
> > +
> > ......
> > +static struct clk_regmap t7_sys_gic = {
> > +     .data = &(struct clk_regmap_gate_data) {
> > +             .offset = CLKCTRL_SYS_CLK_EN0_REG2,
> > +             .bit_idx = 30,
> > +     },
> > +     .hw.init = &(struct clk_init_data){
> > +             .name = "t7_sys_gic",
> > +             .ops = &clk_regmap_gate_ops,
> > +             .parent_data = &(const struct clk_parent_data) {
> > +                     .fw_name = "sys",
> > +             },
> > +             .num_parents = 1,
> > +             .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
> Do you really intend to for the rate of the sys pll to be set through
> this clock ?


Ok, CLK_SET_RATE_PARENT is not necessary here, I will remove it.

> > ......
> --
> Jerome