Re: [PATCH RFC 0/3] clk: meson: Refactor PLL pre-divider as a divider clock

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

Hi Jerome,


Thanks for your review.


On 9/24/2026 5:35 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On mer. 23 sept. 2026 at 19:14, Jian Hu via B4 Relay <devnull+jian.hu.amlogic.com@kernel.org> wrote:
>
>> This series refactors the Meson PLL framework to remove the dedicated
>> PLL pre-divider (N) parameter from the PLL implementation and model it
>> as a separate divider clock.
>>
>> Currently, the Meson PLL framework models the PLL pre-divider using a
>> dedicated n field in struct meson_clk_pll_data. This makes the
>> pre-divider part of the PLL-specific implementation, although the
>> Common Clock Framework already provides a generic divider clock.
>>
>> This series separates the pre-divider from the PLL and makes the PLL
>> DCO take the pre-divider clock as its parent. This allows the
>> pre-divider to be modeled using the standard CCF divider implementation
>> and simplifies the PLL framework.
>>
>> The series currently covers T7 as an RFC to get feedback on the
>> framework design before applying the same approach to other SoCs.
>>
>> Series:
>> clk: meson: pll: Remove the dedicated n parameter
>> dt-bindings: clock: amlogic: Add T7 pre-divider clock IDs
>> clk: meson: t7: Model PLL pre-divider as a divider clock
>>
>> The other Meson SoCs will be converted separately after the T7 PLL
>> framework refactoring has been reviewed and the overall approach is
>> agreed upon.
>>
>> Any feedback on the proposed clock hierarchy and the separation of the
>> PLL pre-divider from the PLL itself would be appreciated.
> So if I summarize this RFC, you have simply taken the divider out of the
> PLL, no futher addaptation. right ?
>
> I'm happy with it on the general principle and fine with the change as
> long as you test it on as much platform as you can, clearly flagging
> those you have just compiled tested.
>
> A change like this would likely need to land early in the cycle give as
> much time as possible for testing.
>
> However there a couple of thing I'm concerned about:
>
> * You've drop the table support: are you sure this is not needed anymore
>    ? don't you want to be able to restrict mutlipliers to specific values
>    sometimes ? If not, then OK.
>
> * the determine_rate() make no call to round the parent rate: Since the
>    parent will be the divier, how do you progate the rate change so N
>    moves and the best parent rate is found ? For sure this fractional
>    multiplier clock will need CLK_SET_RATE_PARENT to adjust the
>    pre-divider.
>
> * Goes with the point above, but I'm not seeing anything that favors
>    lower N for lower jitter, Or mention of a minimum input rate (which
>    could be a property) ?
>    Those are constraints I think I have understood from your explanation
>    here [1] but maybe you've got new information to share ?
>
> This is overall going in the right direction but determine_rate() and
> constraints need work.
>
> Note: you are more likely to get test feedback if you add g12 (sm1) as an
> example. Those are still the most widely used amlogic platforms with
> mainline.
>
> [1]: https://lore.kernel.org/linux-clk/c9c4945f-cdfc-4382-b8ca-71b69d91deb4@amlogic.com/


Yes, your summary is correct: the RFC simply takes the pre-divider out 
of the PLL.


1) Keeping the table support

Agreed, removing it was premature. Some tables cannot be expressed as
a multiplier range:

- pinned (m, n) pairs, e.g. axg PCIe GP0 (m=200, n=3) and meson8m2
   GP0 (m=182, n=3)
- a pinned m, e.g. g12a PCIe PLL (m=150)
- sparse tables, e.g. meson8b HDMI PLL

The per-platform conversions will turn the tables with contiguous m
and n = 1 into range, and keep the tables for the rest, so
the framework will support both.


2) N is fixed per PLL

The main new information: the pre-divider is not meant to be selected
dynamically. Per the hardware design, each PLL has a single fixed N
value, defined together with the rest of the PLL: N = 1 for most PLLs,
and N = 3 for a few special cases on older SoCs (the axg PCIe PLL and
the meson8m2 GP0 PLL). The PLL input frequency constraints are
respected by this fixed value.

Keeping N at 1 minimizes PLL jitter and yields the best performance.

So this is less about dropping the constraints than about the fact
that there is nothing to choose at runtime: no N search in
determine_rate(), and no PFD input frequency constraint to enforce,
since the PLL input frequency is a constant for each PLL.

3) No CLK_SET_RATE_PARENT on the DCO

With N fixed, the pre-divider rate never changes, so the DCO does not
set CLK_SET_RATE_PARENT on purpose: the flag would claim that the
parent rate may change to satisfy the child, which is not the case
here. determine_rate() never touches best_parent_rate so the flag
would be a no operation today.


4) Implementation methods for pre-divider clock

While testing the RFC I found that with the single-entry pre-divider
table ({val = 1, div = 1}), the pre-divider register can never actually
be programmed.

The pre-divider field resets to 0, which is not a valid setting.

During registration, recalc_rate() reports the parent rate for the
zero register value. With N = 1 the reported rate is the parent rate,
and the single-entry table also rounds every rate request back to the
parent rate, so clk_set_rate() always bails out early (rounded rate ==
current rate) and clk_regmap_div_set_rate() is never called. The
register keeps its invalid reset value.

v2 programs the fixed N at registration time, with a new
init_val field in clk_regmap_div_data, applied from .init() once the
regmap is available.

Or do you have any other good ideas?

The patch is available[1], Please help to review it.


5) Testing and rollout

So far this has been boot tested on T7. For v2 I will
convert the SoCs one by one, starting with g12a and sm1 which
have the most mainline users, then the remaining platforms.


[1]

--- a/drivers/clk/meson/clk-regmap.c
+++ b/drivers/clk/meson/clk-regmap.c
@@ -163,10 +163,28 @@ static int clk_regmap_div_set_rate(struct clk_hw 
*hw, unsigned long rate,
                                   clk_div_mask(div->width) << 
div->shift, val);
  };

+static int clk_regmap_div_init(struct clk_hw *hw)
+{
+       int ret;
+       struct clk_regmap *clk = to_clk_regmap(hw);
+       struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk);
+
+       ret = clk_regmap_init(hw);
+       if (ret)
+               return ret;
+
+       if (div->init_val)
+               regmap_update_bits(clk->map, div->offset,
+                                  clk_div_mask(div->width) << div->shift,
+                                  div->init_val << div->shift);
+
+       return 0;
+}
+
  /* Would prefer clk_regmap_div_ro_ops but clashes with qcom */

  const struct clk_ops clk_regmap_divider_ops = {
-       .init = clk_regmap_init,
+       .init = clk_regmap_div_init,

>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>> ---
>> Jian Hu (3):
>>        clk: meson: pll: Remove the dedicated n parameter
>>        dt-bindings: clock: amlogic: Add T7 pre-divider clock IDs
>>        clk: meson: t7: Model PLL pre-divider as a divider clock
>>
>>   drivers/clk/meson/clk-pll.c                     | 178 +++++------------------
>>   drivers/clk/meson/clk-pll.h                     |  13 --
>>   drivers/clk/meson/t7-pll.c                      | 183 ++++++++++++++++++------
>>   include/dt-bindings/clock/amlogic,t7-pll-clkc.h |   6 +
>>   4 files changed, 181 insertions(+), 199 deletions(-)
>> ---
>> base-commit: 43e1705ecab981c66baee89041e6f728c0436f19
>> change-id: 20260923-meson_refactor_n-e7f25904e536
>>
>> Best regards,
>> --
>> Jian Hu <jian.hu@amlogic.com>
>>
>>
>>
>> _______________________________________________
>> linux-amlogic mailing list
>> linux-amlogic@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-amlogic
> --
> Jerome

--

Jian