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

From: Jian Hu

Date: Thu Jun 11 2026 - 08:24:35 EST


On 6/10/2026 8:26 PM, Jerome Brunet wrote:
[ EXTERNAL EMAIL ]

On mer. 10 juin 2026 at 12:18, Jian Hu <jian.hu@xxxxxxxxxxx> 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@xxxxxxxxxx> wrote:

From: Jian Hu <jian.hu@xxxxxxxxxxx>

Add the Always-on clock controller driver for the Amlogic A9 SoC family.

Signed-off-by: Jian Hu <jian.hu@xxxxxxxxxxx>
---
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