Re: [PATCH 08/10] clk: amlogic: Add A9 PLL clock controller driver

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

On 5/15/2026 12:12 AM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On lun. 11 mai 2026 at 20:47, Jian Hu via B4 Relay <devnull+jian.hu.amlogic.com@kernel.org> wrote:
>
>> From: Jian Hu <jian.hu@amlogic.com>
>>
>> Add the PLL clock controller driver for the Amlogic A9 SoC family.
>>
>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
[...]
>> +
>> +/*
>> + * Compared with previous SoC PLLs, the A9 PLL input path has an inherent
>> + * 2-divider. The N pre-divider follows the same calculation rule as OD,
>> + * where the pre-divider ratio equals 2^N.
>> + *
>> + * A9 PLL is composed as follows:
>> + *
>> + *                      PLL
>> + *         +---------------------------------+
>> + *         |                                 |
>> + *         |             +--+                |
>> + *  in/2 >>---[ /2^N ]-->|  |      +-----+   |
>> + *         |             |  |------| DCO |----->> out
>> + *         |  +--------->|  |      +--v--+   |
>> + *         |  |          +--+         |      |
>> + *         |  |                       |      |
>> + *         |  +--[ *(M + (F/Fmax) ]<--+      |
>> + *         |                                 |
>> + *         +---------------------------------+
>> + *
>> + * out = in / 2  * (m + frac / frac_max) / 2^n
>> + */
>> +
>> +static struct clk_fixed_factor a9_gp0_in_div2_div = {
>> +     .mult = 1,
>> +     .div = 2,
>> +     .hw.init = &(struct clk_init_data){
>> +             .name = "gp0_in_div2_div",
>> +             .ops = &clk_fixed_factor_ops,
>> +             .parent_data = &(const struct clk_parent_data) {
>> +                     .fw_name = "in0",
>> +             },
>> +             .num_parents = 1,
>> +     },
>> +};
>> +
>> +static struct clk_regmap a9_gp0_in_div2 = {
>> +     .data = &(struct clk_regmap_gate_data) {
>> +             .offset = GP0PLL_CTRL0,
>> +             .bit_idx = 27,
>> +     },
>> +     .hw.init = &(struct clk_init_data) {
>> +             .name = "gp0_in_div2",
>> +             .ops = &clk_regmap_gate_ops,
>> +             .parent_hws = (const struct clk_hw *[]) {
>> +                     &a9_gp0_in_div2_div.hw
>> +             },
>> +             .num_parents = 1,
>> +     },
>> +};
> When document something, be sure it matches what you are doing
> afterward. It is confusing otherwise. Your comments above clearly miss
> this gate.
>
> A fixed 2 divider followed by a power of 2 divider ? Is it actually how
> the HW works or your modelisation power of 2 that's shifted by 1,
> mapping :
>   * 0 -> 2
>   * 1 -> 4
>   * etc ...
>
> ?


Sorry for missing the gate in PLL block diagram, above block diagram 
focuses on mathematical formulas.


A9 PLL is composed as follows in fact, M and frac have a 0.5 weight factor:

                        PLL
           +-----------------------------------------------------+
           |                                                     |
           |             +--+                                    |
    in   >>---[ /N ]-->  |  |                     +-----+       |
           |             |  |---------------------| DCO | |----->> out
           |  +--------->|  |                     +--v--+ |
           |  |          +--+                        |           |
           |  |                                      |           |
           |  +--[ *(M + (F/Fmax) ] * 0.5 + Enable<--+  |
           |                                                     |
           +-----------------------------------------------------+


         out = in  * (M + frac / frac_max) * 0.5 / N


  If we ignore frac and set N = 1, it simplifies to:

         out = in  * M  * 0.5

This can be rewritten as:

         out = (in / 2) * M

The 0.5 weight is hardware-controlled:

         For GP0/HIFI PLL: controlled by CTRL0 bit27 (gate)
         For MCLK PLL: enabled by default, no gate bit

To model this in the clock tree, we add:

         A fixed /2 divider after input clock to represent the 0.5 weight
         A gate clock to represent the enable control

The resulting structure is:
         input --> fixed div2 --> gate--> dco

I would appreciate your guidance If this is not appropriate.


>> +
>> +/* The output frequency range of the A9 PLL_DCO is 1.4 GHz to 2.8 GHz. */
>> +static const struct pll_mult_range a9_pll_mult_range = {
>> +     .min = 117,
>> +     .max = 233,
>> +};
> If PLL restriction is actually the DCO output rate, and only the reason
> to keep the pre-devider in the range above, I would definitely welcome a
> rework to express the constraints properly and split the pre-divider out.
>
>> +
>> +static const struct reg_sequence a9_gp0_pll_init_regs[] = {
>> +     { .reg = GP0PLL_CTRL0, .def = 0x00010000 },
>> +     { .reg = GP0PLL_CTRL1, .def = 0x11480000 },
>> +     { .reg = GP0PLL_CTRL2, .def = 0x1219b010 },
>> +     { .reg = GP0PLL_CTRL3, .def = 0x00008010 }
>> +};
>> +
>> +static struct clk_regmap a9_gp0_pll_dco = {
>> +     .data = &(struct meson_clk_pll_data) {
>> +             .en = {
>> +                     .reg_off = GP0PLL_CTRL0,
>> +                     .shift   = 28,
>> +                     .width   = 1,
>> +             },
>> +             .m = {
>> +                     .reg_off = GP0PLL_CTRL0,
>> +                     .shift   = 0,
>> +                     .width   = 9,
>> +             },
>> +             .n = {
>> +                     .reg_off = GP0PLL_CTRL0,
>> +                     .shift   = 12,
>> +                     .width   = 3,
>> +             },
>> +             .frac = {
>> +                     .reg_off = GP0PLL_CTRL1,
>> +                     .shift   = 0,
>> +                     .width   = 17,
>> +             },
>> +             .l = {
>> +                     .reg_off = GP0PLL_CTRL0,
>> +                     .shift   = 31,
>> +                     .width   = 1,
>> +             },
>> +             .rst = {
>> +                     .reg_off = GP0PLL_CTRL0,
>> +                     .shift   = 29,
>> +                     .width   = 1,
>> +             },
>> +             .l_detect = {
>> +                     .reg_off = GP0PLL_CTRL0,
>> +                     .shift   = 30,
>> +                     .width   = 1,
>> +             },
>> +             .range = &a9_pll_mult_range,
>> +             .init_regs = a9_gp0_pll_init_regs,
>> +             .init_count = ARRAY_SIZE(a9_gp0_pll_init_regs),
>> +             .flags = CLK_MESON_PLL_RST_ACTIVE_LOW |
>> +                      CLK_MESON_PLL_N_POWER_OF_TWO |
>> +                      CLK_MESON_PLL_L_DETECT_ACTIVE_HIGH,
>> +     },
>> +     .hw.init = &(struct clk_init_data) {
>> +             .name = "gp0_pll_dco",
>> +             .ops = &meson_clk_pll_ops,
>> +             .parent_hws = (const struct clk_hw *[]) {
>> +                     &a9_gp0_in_div2.hw
>> +             },
>> +             .num_parents = 1,
>> +     },
>> +};
>> +
>> +/* For gp0, hifi and mclk pll, the maximum value of od is 4. */
>> +static const struct clk_div_table a9_pll_od_table[] = {
>> +     { 0,  1 },
>> +     { 1,  2 },
>> +     { 2,  4 },
>> +     { 3,  8 },
>> +     { 4,  16 },
>> +     { /* sentinel */ }
>> +};
>> +
>> +static struct clk_regmap a9_gp0_pll = {
>> +     .data = &(struct clk_regmap_div_data) {
>> +             .offset = GP0PLL_CTRL0,
>> +             .shift = 20,
>> +             .width = 3,
>> +             .table = a9_pll_od_table,
>> +     },
>> +     .hw.init = &(struct clk_init_data) {
>> +             .name = "gp0_pll",
>> +             .ops = &clk_regmap_divider_ops,
>> +             .parent_hws = (const struct clk_hw *[]) {
>> +                     &a9_gp0_pll_dco.hw
>> +             },
>> +             .num_parents = 1,
>> +             .flags = CLK_SET_RATE_PARENT,
>> +     },
>> +};
>> +
>> +static struct clk_fixed_factor a9_hifi0_in_div2_div = {
>> +     .mult = 1,
>> +     .div = 2,
>> +     .hw.init = &(struct clk_init_data){
>> +             .name = "hifi0_in_div2_div",
>> +             .ops = &clk_fixed_factor_ops,
>> +             .parent_data = &(const struct clk_parent_data) {
>> +                     .fw_name = "in0",
>> +             },
>> +             .num_parents = 1,
>> +     },
>> +};
>> +
>> +static struct clk_regmap a9_hifi0_in_div2 = {
>> +     .data = &(struct clk_regmap_gate_data) {
>> +             .offset = HIFIPLL_CTRL0,
>> +             .bit_idx = 27,
>> +     },
>> +     .hw.init = &(struct clk_init_data) {
>> +             .name = "hifi0_in_div2",
>> +             .ops = &clk_regmap_gate_ops,
>> +             .parent_hws = (const struct clk_hw *[]) {
>> +                     &a9_hifi0_in_div2_div.hw
>> +             },
>> +             .num_parents = 1,
>> +     },
>> +};
>> +
>> +static const struct reg_sequence a9_hifi0_pll_init_regs[] = {
>> +     { .reg = HIFIPLL_CTRL0, .def = 0x00010000 },
>> +     { .reg = HIFIPLL_CTRL1, .def = 0x11480000 },
>> +     { .reg = HIFIPLL_CTRL2, .def = 0x1219b010 },
>> +     { .reg = HIFIPLL_CTRL3, .def = 0x00008010 }
>> +};
> It look like GP0 and HIFI PLL are exactly the same IP, you've even
> documented it as such. Yet all the code is duplicated. That's not OK.
>
> I understand that way we statically declared the clocks so far pushed
> you in that direction. That's something I'd like to fix properly
> someday.
>
> In the meantime, you could at least duplicate the memory at runtime to
> avoid copy/pasting the code. A minor change to clkc utils as suggested
> at the end of this message could help you do so.
>
> Same probably applies to mclks.


You're right, the GP0 and HIFI PLLs are indeed the same IP, differing 
only by frac_max:
     GP0: frac_max = 2^17
     HIFI: frac_max = 100000


Each clock requires its own clk_regmap and clk_hw structure, though the 
data in
clk_regmap can be shared between HIFI0 and HIFI1.


I have tried duplicating HIFI1's clock structure from HIFI0 at runtime.
Most members of clk_init_data (except parent_hws / parent_data) can be 
easily copied.


However, I have a question regarding dynamic parent assignment:
For example:
Clock B is created dynamically, and its parent is clock A (also created 
dynamically).
How should I properly assign this parent relationship?


Furthermore, how to handle more complex parent configurations dynamically?
For example:
Clock D has three parents: C, B, A (in an irregular order).


I would appreciate your guidance on how to handle these dynamic clock 
relationships properly.

>
>
[...]


Best regards,

Jian