Re: [PATCH v2 4/8] clk: sunxi-ng: a733: Add PLL clocks support

[Junhui Liu]

Hi Chen-Yu,
Thanks for your review. I will address the comments in v3.

On Sun Aug 23, 2026 at 5:38 PM CST, Chen-Yu Tsai wrote:
> (trimmed recipient list a bit)
>
> On Sat, Jul 11, 2026 at 4:12 PM Junhui Liu <junhui.liu@xxxxxxxxxxxxx> wrote:
>>
>> Add PLL clock support for the main CCU of the Allwinner A733 SoC. The
>> structure is mostly similar to the sun55i, with the addition of a
>> PLL_REF clock that normalizes the hardware-detected DCXO/hosc frequency
>> (19.2MHz, 24MHz, or 26MHz) into a consistent 24MHz reference for all
>> subsequent PLLs.
>>
>> The behaviors of PLL_AUDIO0 and PLL_AUDIO1 are ported from the vendor
>> driver. Specifically, PLL_AUDIO0 is configured with SDM parameters to
>> provide a 22.5792MHz * 4 output, while PLL_AUDIO1 is integrated into
>> the main CCU without using SDM.
>>
>> Tested-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
>> Signed-off-by: Junhui Liu <junhui.liu@xxxxxxxxxxxxx>
>> ---
>>  drivers/clk/sunxi-ng/Kconfig           |   5 +
>>  drivers/clk/sunxi-ng/Makefile          |   2 +
>>  drivers/clk/sunxi-ng/ccu-sun60i-a733.c | 539 +++++++++++++++++++++++++++++++++
>>  3 files changed, 546 insertions(+)

[...]

>> +
>> +/*
>> + * There is no actual clock output with that frequency (2.4 GHz), instead it
>> + * has multiple outputs with adjustable dividers from that base frequency.
>> + * Model them separately as divider clocks based on that parent here.
>> + */
>> +#define SUN60I_A733_PLL_PERIPH0_REG    0x0a0
>> +static struct ccu_nm pll_periph0_4x_clk = {
>> +       .enable         = BIT(25) | BIT(26) | BIT(27),
>
> The gate helper sort of expects this to be just one bit. And the three
> bits here are for the three individual outputs (2x, 800M, 480M). I think
> you should move the bits to the individual outputs, and use BIT(31) here
> instead (or just leave out the enable bit for this one).

I will leave out the enable bit for the PLL core and move the three
output gate bits to their corresponding child clocks.

[...]

>> +
>> +static const struct clk_hw *pll_audio1_hws[] = {
>> +       &pll_audio1_clk.common.hw
>> +};
>> +static SUNXI_CCU_M_HWS(pll_audio1_div2_clk, "pll-audio1-div2", pll_audio1_hws,
>> +                      SUN60I_A733_PLL_AUDIO1_REG, 20, 3, 0);
>> +static SUNXI_CCU_M_HWS(pll_audio1_div5_clk, "pll-audio1-div5", pll_audio1_hws,
>> +                      SUN60I_A733_PLL_AUDIO1_REG, 16, 3, 0);
>
> Perhaps these should be made fixed? Otherwise the names don't apply correctly.

Agreed. Since the names explicitly describe fixed /2 and /5 divisions,
I will model these two outputs as fixed-factor clocks and initialize the
corresponding divider fields accordingly.

[...]

>> +       .lock           = BIT(28),
>> +       .n              = _SUNXI_CCU_MULT_MIN(8, 8, 11),
>> +       .m              = _SUNXI_CCU_DIV(1, 1), /* input divider */
>> +       .common         = {
>> +               .reg            = SUN60I_A733_PLL_DE_REG,
>> +               .hw.init        = CLK_HW_INIT_PARENTS_HW("pll-de-12x", pll_ref_hws,
>> +                                                        &ccu_nm_ops,
>> +                                                        CLK_SET_RATE_GATE),
>> +       },
>> +};
>> +
>> +static const struct clk_hw *pll_de_hws[] = {
>> +       &pll_de_12x_clk.common.hw
>> +};
>> +static SUNXI_CCU_M_HWS(pll_de_4x_clk, "pll-de-4x", pll_de_hws,
>> +                      SUN60I_A733_PLL_DE_REG, 20, 3, 0);
>> +static SUNXI_CCU_M_HWS(pll_de_3x_clk, "pll-de-3x", pll_de_hws,
>> +                      SUN60I_A733_PLL_DE_REG, 16, 3, 0);
>
> Should we make the values fixed or the divider read-only?

After checking the manual again, I don't think these dividers should be
fixed or read-only.

The 4X and 3X suffixes appear to be hardware output names rather than
fixed multiplication factors. The manual describes both P0 and P1 as
programmable dividers and only gives their default frequencies; it does
not say that their values must remain fixed.

For example, the default frequencies of DEPLL4X and DEPLL3X are
1044 MHz and 696 MHz, respectively. If the suffixes represented
multiples of a common base frequency, 1044 MHz / 4 * 3 would be
783 MHz, not 696 MHz.

[...]

>> +
>> +       /*
>> +        * The PLL clock code does not model all bits, for instance it does
>> +        * not support a separate enable and gate bit. We present the
>> +        * gate bit(27) as the enable bit, but then have to set the
>> +        * PLL Enable, LDO Enable, and Lock Enable bits on all PLLs here.
>> +        */
>> +       for (i = 0; i < ARRAY_SIZE(pll_regs); i++) {
>> +               val = readl(reg + pll_regs[i]);
>> +               val |= BIT(31) | BIT(30) | BIT(29);
>> +               writel(val, reg + pll_regs[i]);
>> +       }
>> +
>> +       /* Enforce m1 = 0 for PLL_AUDIO0 */
>> +       val = readl(reg + SUN60I_A733_PLL_AUDIO0_REG);
>> +       val &= ~BIT(1);
>> +       writel(val, reg + SUN60I_A733_PLL_AUDIO0_REG);
>
> We should also force the divider for all the divN or Nx outputs.

For the reasons above, I plan to force the divider values only for the
divN outputs, whose names explicitly specify their division ratios, and
leave the Nx output dividers programmable.

>
>
> Thanks
> ChenYu
>

-- 
Best regards,
Junhui Liu