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

[Jerome Brunet <jbrunet@baylibre.com>]

On lun. 19 janv. 2026 at 20:16, Chuan Liu <chuan.liu@amlogic.com> wrote:

> 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 ...

You are redefining the PCLK here, the *exact* type of pointless differences
we've worked last year to remove. This is something you can't have
missed since you've complained about it taking too long.

And now, you've thought I was "just teasing" about it ?

I'm bored with your botched submissions Chuan.

> In the next revision, I will change this part to use a
> unified macro.

Yes please.

>
> We may also consider adjusting the "MESON_PCLK" macro later by removing the
> SoC prefix from the clock name,

No

> so that it is consistent with the naming
> style used by "MESON_COMP_SEL" / "MESON_COMP_DIV".
>

Just do the same as c3 and t7.

>> 
>>> +
>>> +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?

Reviewing these long patches takes time. I tend to stop reviewing when I
noticed some feedback was ignored, especially when it is recurrent
problem. I've told you that already. It is up to you to make sure you are
not missing anything before re-submitting if you don't want to waste time.

>
> [...]

-- 
Jerome