Re: [PATCH v3 2/2] clk: amlogic: Add A9 peripherals clock controller driver

[Jerome Brunet <jbrunet@baylibre.com>]

On lun. 15 juin 2026 at 19:25, Jian Hu <jian.hu@amlogic.com> wrote:

> On 6/10/2026 8:49 PM, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On mer. 10 juin 2026 at 16:14, Jian Hu via B4 Relay <devnull+jian.hu.amlogic.com@kernel.org> wrote:
>>
>>> From: Jian Hu <jian.hu@amlogic.com>
>>>
>>> Add the peripherals clock controller driver for the Amlogic A9 SoC family.
>>>
>>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>>> ---
>>>   drivers/clk/meson/Kconfig          |   15 +
>>>   drivers/clk/meson/Makefile         |    1 +
>>>   drivers/clk/meson/a9-peripherals.c | 1925 ++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 1941 insertions(+)
>>>
>
> [ ... ]
>
>>> +
>>> +/* Channel 6 is unconnected. */
>>> +static u32 a9_glb_parents_val_table[] = { 0, 1, 2, 3, 4, 5, 7 };
>>> +static struct clk_regmap a9_dspa;
>> What is this ?
>
>
> The peripheral clock definitions are ordered by register offset.
>
> dspa is one of the parents of the glb clock, while the dsp clock registers
> are located after the GLB clock registers.
>
> Since glb references a9_dspa before its full definition appears, the
> declaration
>
> static struct clk_regmap a9_dspa;
>
> is added as a forward declaration to satisfy the compiler.
>
>
> Would it make sense to relax the register-offset ordering in this case?
>

I don't think we ever enforced such ordering (or any other ordering) in
the clock driver, so yes please.


> By defining the DSP clock before the GLB clock, we could remove the forward
> declaration of a9_dspa.

Unless it is absolutely necessary, please avoid forward declaration.

Declare what is needed first, keep related things together and use your
best judgement ... IOW, make it easy for me to review ;) 

>
>>> +
>>> +static const struct clk_parent_data a9_glb_parents[] = {
>>> +};

[...]

>>> +
>>> +static struct clk_regmap a9_vclk_div2_en = {
>>> +     .data = &(struct clk_regmap_gate_data){
>>> +             .offset = VID_CLK_CTRL,
>>> +             .bit_idx = 1,
>>> +     },
>>> +     .hw.init = CLK_HW_INIT_HW("vclk_div2_en", &a9_vclk.hw,
>>> +                               &clk_regmap_gate_ops, CLK_SET_RATE_PARENT),
>>> +};
>> Looks to me all this div_en / div repeating pattern would be easier to review
>> with tiny macro .
>
>
> Good point.
>
> I tried to reduce the repeated div_en/div pattern using a helper macro.
>
> It keeps the relationship between gate and fixed-factor clock more compact
> and easier to review.
>
> After using the helper macro, the div_en/div code can be simplified to the
> following:
>
> #define A9_VCLK(_name, _reg, _bit, _div, _parent)        \
> struct clk_regmap a9_##_name##_en = {      \
                       ^- not strictly necessary, a touch too agressive
                        

>         .data = &(struct clk_regmap_gate_data){          \
>                 .offset = _reg,      \
>                 .bit_idx = _bit,       \
>         },       \
>         .hw.init = &(struct clk_init_data) {           \
>                 .name = #_name "_en",      \
>                 .ops = &clk_regmap_gate_ops,           \
>                 .parent_hws = (const struct clk_hw *[]) { _parent },    \
>                 .num_parents = 1,      \
>                 .flags = CLK_SET_RATE_PARENT,      \
>         },       \
> };       \
>       \
> struct clk_fixed_factor a9_##_name = {       \
>         .mult = 1,       \
>         .div = _div,       \
>         .hw.init = &(struct clk_init_data){          \
>                 .name = #_name,      \
>                 .ops = &clk_fixed_factor_ops,          \
>                 .parent_hws = (const struct clk_hw *[]) {      \
>                         &a9_##_name##_en.hw          \
>                 },       \
>                 .num_parents = 1,      \
>                 .flags = CLK_SET_RATE_PARENT,      \
>         },       \
> };       \
>
> static A9_VCLK(vclk_div2, VID_CLK_CTRL, 1, 2, &a9_vclk.hw);
> static A9_VCLK(vclk_div4, VID_CLK_CTRL, 2, 4, &a9_vclk.hw);
> static A9_VCLK(vclk_div6, VID_CLK_CTRL, 3, 6, &a9_vclk.hw);
> static A9_VCLK(vclk_div6, VID_CLK_CTRL, 4, 12, &a9_vclk.hw);
> static A9_VCLK(vclk2_div2, VIID_CLK_CTRL, 1, 2, &a9_vclk2.hw);
> static A9_VCLK(vclk2_div4, VIID_CLK_CTRL, 2, 4, &a9_vclk2.hw);
> static A9_VCLK(vclk2_div6, VIID_CLK_CTRL, 3, 6, &a9_vclk2.hw);
> static A9_VCLK(vclk2_div6, VIID_CLK_CTRL, 4, 12, &a9_vclk2.hw);
>
>
> If you think splitting it further into separate helper macros would improve
> readability.

One clock per macro please. Hidding 2 declaration is recipe for
disaster. For ex, here the first one is static, the 2nd is not 

>
> I can do that as well.
>