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

[Jian Hu <jian.hu@amlogic.com>]

On 2025/5/14 16:00, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 09 May 2025 at 07:48, Jian Hu <jian.hu@amlogic.com> wrote:
>
>> Add Peripheral clock controller driver for the Amlogic T7 SoC family.
>>
>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>> ---
>>   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@baylibre.com/


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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic