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

[Jerome Brunet <jbrunet@baylibre.com>]

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.

>
>
> 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.

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

-- 
Jerome