Re: [PATCH v5 5/8] clk: amlogic: Add A5 clock peripherals controller driver

[Chuan Liu <chuan.liu@amlogic.com>]

Hi Jerome,

On 1/14/2026 5:25 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> On jeu. 08 janv. 2026 at 14:08, Chuan Liu via B4 Relay <devnull+chuan.liu.amlogic.com@kernel.org> wrote:
> 
>> +static struct clk_regmap a5_rtc_clk = {
>> +     .data = &(struct clk_regmap_mux_data) {
>> +             .offset = RTC_CTRL,
>> +             .mask = 0x3,
>> +             .shift = 0,
>> +     },
>> +     .hw.init = &(struct clk_init_data) {
>> +             .name = "rtc_clk",
>> +             .ops = &clk_regmap_mux_ops,
>> +             .parent_data = a5_rtc_clk_parents,
>> +             .num_parents = ARRAY_SIZE(a5_rtc_clk_parents),
>> +             .flags = CLK_SET_RATE_NO_REPARENT,
>> +     },
>> +};
>> +
>> +#define A5_PCLK(_name, _reg, _bit, _pdata, _flags)                   \
>> +struct clk_regmap a5_##_name = {                                     \
>> +     .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_data = (_pdata),                                \
>> +             .num_parents = 1,                                       \
>> +             .flags = (_flags),                                      \
>> +     },                                                              \
>> +}
> 
> I wonder why I bothered reviewing v4 ...

Regarding the comment you made on V4, my understanding is that you were 
just teasing ... In the next revision, I will change this part to use a 
unified macro.

We may also consider adjusting the "MESON_PCLK" macro later by removing 
the SoC prefix from the clock name, so that it is consistent with the 
naming style used by "MESON_COMP_SEL" / "MESON_COMP_DIV".

> 
>> +
>> +static const struct clk_parent_data a5_sys_pclk_parents = { .fw_name = "sysclk" };
>> +
>> +#define A5_SYS_PCLK(_name, _reg, _bit, _flags) \
>> +     A5_PCLK(_name, _reg, _bit, &a5_sys_pclk_parents, _flags)
>> +
>> +static A5_SYS_PCLK(sys_reset_ctrl,   SYS_CLK_EN0_REG0, 1, 0);
>> +static A5_SYS_PCLK(sys_pwr_ctrl,     SYS_CLK_EN0_REG0, 3, 0);
>> +static A5_SYS_PCLK(sys_pad_ctrl,     SYS_CLK_EN0_REG0, 4, 0);
>> +static A5_SYS_PCLK(sys_ctrl,         SYS_CLK_EN0_REG0, 5, 0);
>> +static A5_SYS_PCLK(sys_ts_pll,               SYS_CLK_EN0_REG0, 6, 0);
>> +
>>
> 
> [...]
> 
>> +
>> +static struct clk_regmap a5_gen = {
>> +     .data = &(struct clk_regmap_gate_data) {
>> +             .offset = GEN_CLK_CTRL,
>> +             .bit_idx = 11,
>> +     },
>> +     .hw.init = &(struct clk_init_data) {
>> +             .name = "gen",
>> +             .ops = &clk_regmap_gate_ops,
>> +             .parent_hws = (const struct clk_hw *[]) {
>> +                     &a5_gen_div.hw
>> +             },
>> +             .num_parents = 1,
>> +             .flags = CLK_SET_RATE_PARENT,
>> +     },
>> +};
>> +
>> +#define A5_COMP_SEL(_name, _reg, _shift, _mask, _pdata, _table) \
>> +     MESON_COMP_SEL(a5_, _name, _reg, _shift, _mask, _pdata, _table, 0, 0)
>> +
>> +#define A5_COMP_DIV(_name, _reg, _shift, _width) \
>> +     MESON_COMP_DIV(a5_, _name, _reg, _shift, _width, 0, CLK_SET_RATE_PARENT)
>> +
>> +#define A5_COMP_GATE(_name, _reg, _bit, _iflags) \
>> +     MESON_COMP_GATE(a5_, _name, _reg, _bit, CLK_SET_RATE_PARENT | (_iflags))
>> +
> 
> At the top. like C3 and T7

Except for A5_COMP_SEL, which differs slightly from T7 due to the 
additional "_table" parameter, the other macros are consistent with T7.

I also asked for your feedback on this in V4 and received your 
confirmation. Is there anything here that still needs to be updated?

[...]