Re: [PATCH v5 5/5] clk: meson: t7: add t7 clock peripherals controller driver

[Jian Hu]

On 11/24/2025 5:42 PM, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Fri 21 Nov 2025 at 18:59, Jian Hu <jian.hu@xxxxxxxxxxx> wrote:
>
> > Add Peripheral clock controller driver for the Amlogic T7 SoC family.
> >
> > Signed-off-by: Jian Hu <jian.hu@xxxxxxxxxxx>
> > ---
> >   drivers/clk/meson/Kconfig          |   13 +
> >   drivers/clk/meson/Makefile         |    1 +
> >   drivers/clk/meson/t7-peripherals.c | 1266 ++++++++++++++++++++++++++++
> >   3 files changed, 1280 insertions(+)
> >   create mode 100644 drivers/clk/meson/t7-peripherals.c
> >
> > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> > index 6cdc6a96e105..d2442ae0f5be 100644
> > --- a/drivers/clk/meson/Kconfig
> > +++ b/drivers/clk/meson/Kconfig
> > @@ -215,4 +215,17 @@ config COMMON_CLK_T7_PLL
> >          device, AKA T7. PLLs are required by most peripheral to operate
> >          Say Y if you are a T7 based device.
> >
> > +config COMMON_CLK_T7_PERIPHERALS
> > +     tristate "Amlogic T7 SoC peripherals clock controller support"
> > +     depends on ARM64
> > +     default ARCH_MESON
> > +     select COMMON_CLK_MESON_REGMAP
> > +     select COMMON_CLK_MESON_CLKC_UTILS
> > +     select COMMON_CLK_MESON_DUALDIV
> > +     imply COMMON_CLK_SCMI
> > +     imply COMMON_CLK_T7_PLL
> > +     help
> > +       Support for the Peripherals clock controller on Amlogic A311D2 based
> > +       device, AKA T7. Peripherals are required by most peripheral to operate
> > +       Say Y if you are a T7 based device.
> I don't think anybody "is" a clock controller ;)


Okay , I will update the help message.

Say Y if you want T7 peripherals clock controller to work.

Same with T7 PLL help message.

Say Y if you want T7 PLL clock controller to work.

> >   endmenu
> > diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> > index 8e3f7f94c639..c6719694a242 100644
> > --- a/drivers/clk/meson/Makefile
> > +++ b/drivers/clk/meson/Makefile
> > @@ -27,3 +27,4 @@ obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o meson8-ddr.o
> >   obj-$(CONFIG_COMMON_CLK_S4_PLL) += s4-pll.o
> >   obj-$(CONFIG_COMMON_CLK_S4_PERIPHERALS) += s4-peripherals.o
> >   obj-$(CONFIG_COMMON_CLK_T7_PLL) += t7-pll.o
> > +obj-$(CONFIG_COMMON_CLK_T7_PERIPHERALS) += t7-peripherals.o
> > diff --git a/drivers/clk/meson/t7-peripherals.c b/drivers/clk/meson/t7-peripherals.c
> > new file mode 100644
> > index 000000000000..10e77456b0d0
> > --- /dev/null
> > +++ b/drivers/clk/meson/t7-peripherals.c
> > @@ -0,0 +1,1266 @@
> > +// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
> > +/*
> > + * Copyright (C) 2024-2025 Amlogic, Inc. All rights reserved.
> > + * Author: Jian Hu <jian.hu@xxxxxxxxxxx>
> > + */
> > +
> > +#include <linux/clk-provider.h>
> > +#include <linux/platform_device.h>
> > +#include "clk-dualdiv.h"
> > +#include "clk-regmap.h"
> > +#include "meson-clkc-utils.h"
> > +#include <dt-bindings/clock/amlogic,t7-peripherals-clkc.h>
> > +
> > +#define RTC_BY_OSCIN_CTRL0   0x8
> > +#define RTC_BY_OSCIN_CTRL1   0xc
> > +#define RTC_CTRL             0x10
> > +#define SYS_CLK_CTRL0                0x40
> > +#define SYS_CLK_EN0_REG0     0x44
> > +#define SYS_CLK_EN0_REG1     0x48
> > +#define SYS_CLK_EN0_REG2     0x4c
> > +#define SYS_CLK_EN0_REG3     0x50
> > +#define CECA_CTRL0           0x88
> > +#define CECA_CTRL1           0x8c
> > +#define CECB_CTRL0           0x90
> > +#define CECB_CTRL1           0x94
> > +#define SC_CLK_CTRL          0x98
> > +#define DSPA_CLK_CTRL0               0x9c
> > +#define DSPB_CLK_CTRL0               0xa0
> > +#define CLK12_24_CTRL                0xa8
> > +#define ANAKIN_CLK_CTRL              0xac
> > +#define MIPI_CSI_PHY_CLK_CTRL        0x10c
> > +#define MIPI_ISP_CLK_CTRL    0x110
> > +#define TS_CLK_CTRL          0x158
> > +#define MALI_CLK_CTRL                0x15c
> > +#define ETH_CLK_CTRL         0x164
> > +#define NAND_CLK_CTRL                0x168
> > +#define SD_EMMC_CLK_CTRL     0x16c
> > +#define SPICC_CLK_CTRL               0x174
> > +#define SAR_CLK_CTRL0                0x17c
> > +#define PWM_CLK_AB_CTRL              0x180
> > +#define PWM_CLK_CD_CTRL              0x184
> > +#define PWM_CLK_EF_CTRL              0x188
> > +#define PWM_CLK_AO_AB_CTRL   0x1a0
> > +#define PWM_CLK_AO_CD_CTRL   0x1a4
> > +#define PWM_CLK_AO_EF_CTRL   0x1a8
> > +#define PWM_CLK_AO_GH_CTRL   0x1ac
> > +#define SPICC_CLK_CTRL1              0x1c0
> > +#define SPICC_CLK_CTRL2              0x1c4
> > +
> > +#define T7_COMP_SEL(_name, _reg, _shift, _mask, _pdata) \
> > +     MESON_COMP_SEL(t7_, _name, _reg, _shift, _mask, _pdata, NULL, 0, 0)
> > +
> > +#define T7_COMP_DIV(_name, _reg, _shift, _width) \
> > +     MESON_COMP_DIV(t7_, _name, _reg, _shift, _width, 0, CLK_SET_RATE_PARENT)
> > +
> > +#define T7_COMP_GATE(_name, _reg, _bit, _iflags) \
> > +     MESON_COMP_GATE(t7_, _name, _reg, _bit, CLK_SET_RATE_PARENT | _iflags)
> > +
> parathesis around _iflags please


Okay, I will add '()' for _iflags.

MESON_COMP_GATE(t7_, _name, _reg, _bit, CLK_SET_RATE_PARENT | (_iflags))

> > +static struct clk_regmap t7_rtc_dualdiv_in = {
> > +     .data = &(struct clk_regmap_gate_data){
> > +             .offset = RTC_BY_OSCIN_CTRL0,
> > +             .bit_idx = 31,
> > +     },
> > +     .hw.init = &(struct clk_init_data) {
> > +             .name = "rtc_duandiv_in",
> > +             .ops = &clk_regmap_gate_ops,
> > +             .parent_data = &(const struct clk_parent_data) {
> > +                     .fw_name = "xtal",
> > +             },
> > +             .num_parents = 1,
> > +     },
> > +};
> > +
> > +static const struct meson_clk_dualdiv_param t7_dualdiv_table[] = {
> > +     {
> > +             .n1     = 733, .m1      = 8,
> > +             .n2     = 732, .m2      = 11,
> > +             .dual   = 1,
> > +     },
> > +     {}
> > +};
> > +
> > +static struct clk_regmap t7_rtc_dualdiv_div = {
> > +     .data = &(struct meson_clk_dualdiv_data){
> > +             .n1 = {
> > +                     .reg_off = RTC_BY_OSCIN_CTRL0,
> > +                     .shift   = 0,
> > +                     .width   = 12,
> > +             },
> > +             .n2 = {
> > +                     .reg_off = RTC_BY_OSCIN_CTRL0,
> > +                     .shift   = 12,
> > +                     .width   = 12,
> > +             },
> > +             .m1 = {
> > +                     .reg_off = RTC_BY_OSCIN_CTRL1,
> > +                     .shift   = 0,
> > +                     .width   = 12,
> > +             },
> > +             .m2 = {
> > +                     .reg_off = RTC_BY_OSCIN_CTRL1,
> > +                     .shift   = 12,
> > +                     .width   = 12,
> > +             },
> > +             .dual = {
> > +                     .reg_off = RTC_BY_OSCIN_CTRL0,
> > +                     .shift   = 28,
> > +                     .width   = 1,
> > +             },
> > +             .table = t7_dualdiv_table,
> > +     },
> > +     .hw.init = &(struct clk_init_data){
> > +             .name = "rtc_dualdiv_div",
> > +             .ops = &meson_clk_dualdiv_ops,
> > +             .parent_hws = (const struct clk_hw *[]) {
> > +                     &t7_rtc_dualdiv_in.hw
> > +             },
> > +             .num_parents = 1,
> > +     },
> > +};
> > +
> > +static struct clk_regmap t7_rtc_dualdiv_sel = {
> > +     .data = &(struct clk_regmap_mux_data) {
> > +             .offset = RTC_BY_OSCIN_CTRL1,
> > +             .mask = 0x1,
> > +             .shift = 24,
> > +     },
> > +     .hw.init = &(struct clk_init_data){
> > +             .name = "rtc_dualdiv_sel",
> > +             .ops = &clk_regmap_mux_ops,
> > +             .parent_hws = (const struct clk_hw *[]) {
> > +                     &t7_rtc_dualdiv_div.hw,
> > +                     &t7_rtc_dualdiv_in.hw,
> > +             },
> > +             .num_parents = 2,
> > +             .flags = CLK_SET_RATE_PARENT,
> > +     },
> > +};
> > +
> > +static struct clk_regmap t7_rtc_dualdiv = {
> > +     .data = &(struct clk_regmap_gate_data){
> > +             .offset = RTC_BY_OSCIN_CTRL0,
> > +             .bit_idx = 30,
> > +     },
> > +     .hw.init = &(struct clk_init_data) {
> > +             .name = "rtc_dualdiv",
> > +             .ops = &clk_regmap_gate_ops,
> > +             .parent_hws = (const struct clk_hw *[]) {
> > +                     &t7_rtc_dualdiv_sel.hw
> > +             },
> > +             .num_parents = 1,
> > +             .flags = CLK_SET_RATE_PARENT,
> > +     },
> > +};
> > +
> > +static struct clk_regmap t7_rtc = {
> > +     .data = &(struct clk_regmap_mux_data) {
> > +             .offset = RTC_CTRL,
> > +             .mask = 0x3,
> > +             .shift = 0,
> > +     },
> > +     .hw.init = &(struct clk_init_data){
> > +             .name = "rtc",
> > +             .ops = &clk_regmap_mux_ops,
> > +             /* The first and fourth clock sources are identical in RTC clock design. */
> > +             .parent_data = (const struct clk_parent_data []) {
> > +                     { .fw_name = "xtal", },
> > +                     { .hw = &t7_rtc_dualdiv.hw },
> > +                     { .fw_name = "ext_rtc", },
> > +                     { .fw_name = "xtal", },
> > +             },
> > +             .num_parents = 2,
> > +             .flags = CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT,
> Are you about sure CLK_SET_RATE_PARENT here ?
>
> Also, no need to have the 4th entry:
> * Just drop the the 4th entry in the list above
> * Add comment that xtal is also on parent input #3 but that it is not
>    useful to CCF since the same parent is available with parent input #0
>
> ... assuming this is not a mistake


Okay , I will remove the CLK_SET_RATE_PARENT flag here.

I will also remove the 4th entry and add comment for it.

> > ......
> > +
> > +static const struct clk_parent_data t7_sd_emmc_parents[] = {
> > +     { .fw_name = "xtal", },
> > +     { .fw_name = "fdiv2", },
> > +     { .fw_name = "fdiv3", },
> > +     { .fw_name = "hifi", },
> > +     { .fw_name = "fdiv2p5", },
> > +     { .fw_name = "mpll2", },
> > +     { .fw_name = "mpll3", },
> > +     { .fw_name = "gp0", },
> > +};
> > +
> > +static T7_COMP_SEL(sd_emmc_c, NAND_CLK_CTRL, 9, 0x7, t7_sd_emmc_parents);
> > +static T7_COMP_DIV(sd_emmc_c, NAND_CLK_CTRL, 0, 7);
> > +static T7_COMP_GATE(sd_emmc_c, NAND_CLK_CTRL, 7, 0);
> > +
> > +static T7_COMP_SEL(sd_emmc_a, SD_EMMC_CLK_CTRL, 9, 0x7, t7_sd_emmc_parents);
> > +static T7_COMP_DIV(sd_emmc_a, SD_EMMC_CLK_CTRL, 0, 7);
> > +static T7_COMP_GATE(sd_emmc_a, SD_EMMC_CLK_CTRL, 7, 0);
> > +
> > +static T7_COMP_SEL(sd_emmc_b, SD_EMMC_CLK_CTRL, 25, 0x7, t7_sd_emmc_parents);
> > +static T7_COMP_DIV(sd_emmc_b, SD_EMMC_CLK_CTRL, 16, 7);
> > +static T7_COMP_GATE(sd_emmc_b, SD_EMMC_CLK_CTRL, 23, 0);
> I admit it is a touch pedantic but C, then A then B is odd ...


Okay, I will move sd_emmc_c after sd_emmc_b here.

> > ......
> > +
> > +static const struct meson_clkc_data t7_peripherals_data = {
> > +     .hw_clks = {
> > +             .hws = t7_peripherals_hw_clks,
> > +             .num = ARRAY_SIZE(t7_peripherals_hw_clks),
> > +     },
> > +};
> > +
> > +static const struct of_device_id t7_peripherals_clkc_match_table[] = {
> > +     { .compatible = "amlogic,t7-peripherals-clkc", .data = &t7_peripherals_data },
> Nitpick again, please use multiline format when it is long like that.


Okay, I will change the format.

> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, t7_peripherals_clkc_match_table);
> > +
> > +static struct platform_driver t7_peripherals_clkc_driver = {
> > +     .probe = meson_clkc_mmio_probe,
> > +     .driver = {
> > +             .name = "t7-peripherals-clkc",
> > +             .of_match_table = t7_peripherals_clkc_match_table,
> > +     },
> > +};
> > +module_platform_driver(t7_peripherals_clkc_driver);
> > +
> > +MODULE_DESCRIPTION("Amlogic T7 Peripherals Clock Controller driver");
> > +MODULE_AUTHOR("Jian Hu <jian.hu@xxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL");
> > +MODULE_IMPORT_NS("CLK_MESON");
> --
> Jerome