Re: [PATCH v2 2/2] clk: amlogic: Add A9 AO clock controller driver
Jian Hu <[email protected]>
| Newsgroups | org.infradead.lists.linux-amlogic,org.infradead.lists.linux-arm-kernel,org.kernel.vger.linux-clk,org.kernel.vger.linux-devicetree,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
On 6/10/2026 8:26 PM, Jerome Brunet wrote: > [ EXTERNAL EMAIL ] > > On mer. 10 juin 2026 at 12:18, Jian Hu <[email protected]> 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 <[email protected]> wrote: >>> >>>> From: Jian Hu <[email protected]> >>>> >>>> Add the Always-on clock controller driver for the Amlogic A9 SoC family. >>>> >>>> Signed-off-by: Jian Hu <[email protected]> >>>> --- >>>> 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 _______________________________________________ linux-amlogic mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-amlogic