Re: [PATCH v2 2/2] clk: amlogic: Add A9 AO clock controller driver

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

On 6/10/2026 8:26 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On mer. 10 juin 2026 at 12:18, Jian Hu <jian.hu@amlogic.com> wrote:
>
>> Hi Jerome,
>>
>> Thanks for your review
>>
>> On 6/3/2026 10:29 PM, Jerome Brunet wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On Wed 03 Jun 2026 at 20:17, Jian Hu via B4 Relay <devnull+jian.hu.amlogic.com@kernel.org> wrote:
>>>
>>>> From: Jian Hu <jian.hu@amlogic.com>
>>>>
>>>> Add the Always-on clock controller driver for the Amlogic A9 SoC family.
>>>>
>>>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>>>> ---
>>>>    drivers/clk/meson/Kconfig    |  13 ++
>>>>    drivers/clk/meson/Makefile   |   1 +
>>>>    drivers/clk/meson/a9-aoclk.c | 419 +++++++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 433 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>>>> index cf8cf3f9e4ee..625e6788b940 100644
>>>> --- a/drivers/clk/meson/Kconfig
>>>> +++ b/drivers/clk/meson/Kconfig
>>>> @@ -132,6 +132,19 @@ config COMMON_CLK_A1_PERIPHERALS
>>>>           device, A1 SoC Family. Say Y if you want A1 Peripherals clock
>>>>           controller to work.
>>>>
>>>> +config COMMON_CLK_A9_AO
>>>> +     tristate "Amlogic A9 SoC AO clock controller support"
>>>> +     depends on ARM64
>>>> +     default ARCH_MESON || COMPILE_TEST
>>>> +     select COMMON_CLK_MESON_REGMAP
>>>> +     select COMMON_CLK_MESON_CLKC_UTILS
>>>> +     select COMMON_CLK_MESON_DUALDIV
>>>> +     imply COMMON_CLK_SCMI
>>>> +     help
>>>> +       Support for the AO clock controller on Amlogic A311Y3 based
>>>> +       device, AKA A9.
>>>> +       Say Y if you want A9 AO clock controller to work.
>>>> +
>>>>    config COMMON_CLK_C3_PLL
>>>>         tristate "Amlogic C3 PLL clock controller"
>>>>         depends on ARM64
>>>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>>>> index c6719694a242..f89d027c282c 100644
>>>> --- a/drivers/clk/meson/Makefile
>>>> +++ b/drivers/clk/meson/Makefile
>>>> @@ -19,6 +19,7 @@ obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o
>>>>    obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
>>>>    obj-$(CONFIG_COMMON_CLK_A1_PLL) += a1-pll.o
>>>>    obj-$(CONFIG_COMMON_CLK_A1_PERIPHERALS) += a1-peripherals.o
>>>> +obj-$(CONFIG_COMMON_CLK_A9_AO) += a9-aoclk.o
>>>>    obj-$(CONFIG_COMMON_CLK_C3_PLL) += c3-pll.o
>>>>    obj-$(CONFIG_COMMON_CLK_C3_PERIPHERALS) += c3-peripherals.o
>>>>    obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o
>>>> diff --git a/drivers/clk/meson/a9-aoclk.c b/drivers/clk/meson/a9-aoclk.c
>>>> new file mode 100644
>>>> index 000000000000..b7b3ca231a42
>>>> --- /dev/null
>>>> +++ b/drivers/clk/meson/a9-aoclk.c
>>>> @@ -0,0 +1,419 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
>>>> +/*
>>>> + * Copyright (C) 2026 Amlogic, Inc. All rights reserved
>>>> + */
>>>> +
>>>> +#include <dt-bindings/clock/amlogic,a9-aoclkc.h>
>>>> +#include <linux/clk-provider.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include "clk-regmap.h"
>>>> +#include "clk-dualdiv.h"
>>>> +#include "meson-clkc-utils.h"
>>>> +
>>>> +#define AO_OSCIN_CTRL                        0x00
>>>> +#define AO_SYS_CLK0                  0x04
>>>> +#define AO_PWM_CLK_A_CTRL            0x1c
>>>> +#define AO_PWM_CLK_B_CTRL            0x20
>>>> +#define AO_PWM_CLK_C_CTRL            0x24
>>>> +#define AO_PWM_CLK_D_CTRL            0x28
>>>> +#define AO_PWM_CLK_E_CTRL            0x2c
>>>> +#define AO_PWM_CLK_F_CTRL            0x30
>>>> +#define AO_PWM_CLK_G_CTRL            0x34
>>>> +#define AO_CEC_CTRL0                 0x38
>>>> +#define AO_CEC_CTRL1                 0x3c
>>>> +#define AO_RTC_BY_OSCIN_CTRL0                0x50
>>>> +#define AO_RTC_BY_OSCIN_CTRL1                0x54
>>>> +
>>>> +#define A9_COMP_SEL(_name, _reg, _shift, _mask, _pdata) \
>>>> +     MESON_COMP_SEL(a9_ao_, _name, _reg, _shift, _mask, _pdata, NULL, 0, 0)
>>>> +
>>>> +#define A9_COMP_DIV(_name, _reg, _shift, _width) \
>>>> +     MESON_COMP_DIV(a9_ao_, _name, _reg, _shift, _width, 0, CLK_SET_RATE_PARENT)
>>>> +
>>>> +#define A9_COMP_GATE(_name, _reg, _bit) \
>>>> +     MESON_COMP_GATE(a9_ao_, _name, _reg, _bit, CLK_SET_RATE_PARENT)
>>>> +
>>>> +static struct clk_regmap a9_ao_xtal_in = {
>>>> +     .data = &(struct clk_regmap_gate_data){
>>>> +             .offset = AO_OSCIN_CTRL,
>>>> +             .bit_idx = 3,
>>>> +     },
>>>> +     /*
>>>> +      * It may be ao_sys's parent clock, its child clocks mark
>>>> +      * CLK_IS_CRITICAL, So mark CLK_IS_CRITICAL for it.
>>>> +      */
>>> I don't really get what you mean ... Could you rephrase ?
>>
>> The AO sys gate clock chain may be:
>>
>> ao_xtal_in->ao_xtal->ao_sys-> AO sys gate clocks
>>
>> "ao_xtal_in" is part of the parent chain of the AO sys gate clocks.
>>
>> Some of its downstream clocks are marked with CLK_IS_CRITICAL. To ensure
>> those clocks remain functional, ao_xtal_in must not be disabled and is
>> therefore marked as CLK_IS_CRITICAL as well.
> If any of the downstream clocks are critical and marked as such, there is not
> need to mark this one as well.
>
> You should only mark the clocks that are actually critical with the flag
> and let CCF figure out the dependencies.


Thanks for the clarification.


Understood. CCF already keeps the parent clocks of critical clocks enabled
during __clk_core_init(), so the CLK_IS_CRITICAL flag is not needed here.
I'll drop it in the next revision.

>>
>> I will rephrase it like this in the next version:
>>
>>          /*
>>           * ao_sys can select different clock sources. One possible clock
>> path is:
>>           *      ao_xtal_in->ao_xtal->ao_sys-> ao sys gate clocks
>>           *
>>           * ao_xtal_in is in the parent chain of AO sys gate clocks.
>>           * Since some downstream clocks are marked CLK_IS_CRITICAL,
>>           * ao_xtal_in must remain enabled and is therefore marked
>>           * CLK_IS_CRITICAL as well.
>>           */
>>
>>>> +     .hw.init = CLK_HW_INIT_FW_NAME("ao_xtal_in", "xtal",
>>>> +                                    &clk_regmap_gate_ops, CLK_IS_CRITICAL),
>>> I'm honestly not sure about this. It is correct, sure and the macro exist to be
>>> used but ... It does not really help readability here, does it ?
>>>
>>> (I know that was a feedback you've got on v1)
>>>
>>> Other than that, this looks good to me.
>>>
>> Ok, I will use the original clk_init_data for this one.
> Well my comment applies to whole thing really.
>
> There are surely ways in which the macro but the way we statically
> declare things, it adds a level of indirection that makes things harder
> to review IMO.


Understood. The same reasoning applies to the PLL and peripheral clock 
controllers too.

I'll switch back to the explicit clk_init_data initialization and drop
CLK_HW_INIT_FW_NAME in the next revision.

>>
>> [ ... ]
>>
>>> --
>>> Jerome
> --
> Jerome

--

Jian