Re: [PATCH 08/10] clk: amlogic: Add A9 PLL clock controller driver
From: Jian Hu
Date: Wed May 20 2026 - 03:33:44 EST
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@xxxxxxxxxx> wrote:
From: Jian Hu <jian.hu@xxxxxxxxxxx>
Add the PLL clock controller driver for the Amlogic A9 SoC family.
Signed-off-by: Jian Hu <jian.hu@xxxxxxxxxxx>
+When document something, be sure it matches what you are doing
+/*
+ * 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,
+ },
+};
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.
+If PLL restriction is actually the DCO output rate, and only the reason
+/* 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,
+};
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.
+It look like GP0 and HIFI PLL are exactly the same IP, you've even
+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 }
+};
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