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

From: Jian Hu
Date: Tue Nov 04 2025 - 04:30:43 EST

Next message: lihuisong (C): "Re: [PATCH v2 1/7] ACPI: processor: idle: Mark the state as invalid if its entry method is illegal"
Previous message: Kanchana P Sridhar: "[PATCH v13 19/22] mm: zswap: Per-CPU acomp_ctx resources exist from pool creation to deletion."
Next in thread: Jerome Brunet: "Re: [PATCH v4 5/5] clk: meson: t7: add t7 clock peripherals controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi, Jerome

Thanks for your review.

On 2025/10/31 17:51, Jerome Brunet wrote:

[ EXTERNAL EMAIL ]

On Thu 30 Oct 2025 at 17:43, 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 | 1734 ++++++++++++++++++++++++++++
3 files changed, 1748 insertions(+)
create mode 100644 drivers/clk/meson/t7-peripherals.c

......
+
+static struct clk_regmap t7_rtc_32k_in = {
+ .data = &(struct clk_regmap_gate_data){
+ .offset = RTC_BY_OSCIN_CTRL0,
+ .bit_idx = 31,
+ },
+ .hw.init = &(struct clk_init_data) {
+ .name = "rtc_32k_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_clk_32k_div_table[] = {
+ {
+ .n1 = 733, .m1 = 8,
+ .n2 = 732, .m2 = 11,
+ .dual = 1,
+ },
+ {}
+};
+
+static struct clk_regmap t7_rtc_32k_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_clk_32k_div_table,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "rtc_32k_div",
+ .ops = &meson_clk_dualdiv_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_rtc_32k_in.hw
+ },
+ .num_parents = 1,
+ },
+};
+
+static struct clk_regmap t7_rtc_32k_force_sel = {
+ .data = &(struct clk_regmap_mux_data) {
+ .offset = RTC_BY_OSCIN_CTRL1,
+ .mask = 0x1,
+ .shift = 24,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "rtc_32k_force_sel",
+ .ops = &clk_regmap_mux_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_rtc_32k_div.hw,
+ &t7_rtc_32k_in.hw,
+ },
+ .num_parents = 2,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+};
+
+static struct clk_regmap t7_rtc_32k_out = {
+ .data = &(struct clk_regmap_gate_data){
+ .offset = RTC_BY_OSCIN_CTRL0,
+ .bit_idx = 30,
+ },
+ .hw.init = &(struct clk_init_data) {
+ .name = "rtc_32k_out",
+ .ops = &clk_regmap_gate_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_rtc_32k_force_sel.hw
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+};
+
+static struct clk_regmap t7_rtc_32k_mux0_0 = {
+ .data = &(struct clk_regmap_mux_data) {
+ .offset = RTC_CTRL,
+ .mask = 0x1,
+ .shift = 0,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "rtc_32k_mux0_0",
+ .ops = &clk_regmap_mux_ops,
+ .parent_data = (const struct clk_parent_data []) {
+ { .fw_name = "xtal", },
+ { .hw = &t7_rtc_32k_out.hw },
+ },
+ .num_parents = 2,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+};
+
+static struct clk_regmap t7_rtc_32k_mux0_1 = {

mux0_0 and mux0_1 ? Those are terrible names to search and grep.
Is this really what your documentation is using ? If not, please come up
with something better

ok, I will update rtc clock name. It is a mux clock, and the parents are

'xtal, rtc_32k_out, pad, xtal' controlled by RTC_CTRL bit [0:1]. the first and last parent are same.

here was defined as two mux clocks. I will use one mux clock here.

After updated, the rtc clocks are:

    rtc_dualdiv_in

    rtc_dualdiv_div

    rtc_dualdiv_sel

    rtc_dualdiv

    rtc

+ .data = &(struct clk_regmap_mux_data) {
+ .offset = RTC_CTRL,
+ .mask = 0x1,
+ .shift = 0,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "rtc_32k_mux0_1",
+ .ops = &clk_regmap_mux_ops,
+ .parent_data = (const struct clk_parent_data []) {
+ { .fw_name = "pad", },
+ { .fw_name = "xtal", },
+ },
+ .num_parents = 2,
+ .flags = CLK_SET_RATE_PARENT,

You probably want CLK_SET_RATE_NO_REPARENT for clock with a pad source
like this.

ok, I will add this flag when for clock with pad source.

+ },
+};
+
+static struct clk_regmap t7_rtc = {
+ .data = &(struct clk_regmap_mux_data) {
+ .offset = RTC_CTRL,
+ .mask = 0x1,
+ .shift = 1,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "rtc",
+ .ops = &clk_regmap_mux_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_rtc_32k_mux0_0.hw,
+ &t7_rtc_32k_mux0_1.hw,
+ },
+ .num_parents = 2,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+};
+
+static struct clk_regmap t7_ceca_32k_in = {
+ .data = &(struct clk_regmap_gate_data){
+ .offset = CECA_CTRL0,
+ .bit_idx = 31,
+ },
+ .hw.init = &(struct clk_init_data) {
+ .name = "ceca_32k_in",
+ .ops = &clk_regmap_gate_ops,
+ .parent_data = &(const struct clk_parent_data) {
+ .fw_name = "xtal",
+ },
+ .num_parents = 1,
+ },
+};
+
+static struct clk_regmap t7_ceca_32k_div = {
+ .data = &(struct meson_clk_dualdiv_data){
+ .n1 = {
+ .reg_off = CECA_CTRL0,
+ .shift = 0,
+ .width = 12,
+ },
+ .n2 = {
+ .reg_off = CECA_CTRL0,
+ .shift = 12,
+ .width = 12,
+ },
+ .m1 = {
+ .reg_off = CECA_CTRL1,
+ .shift = 0,
+ .width = 12,
+ },
+ .m2 = {
+ .reg_off = CECA_CTRL1,
+ .shift = 12,
+ .width = 12,
+ },
+ .dual = {
+ .reg_off = CECA_CTRL0,
+ .shift = 28,
+ .width = 1,
+ },
+ .table = t7_clk_32k_div_table,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "ceca_32k_div",
+ .ops = &meson_clk_dualdiv_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_ceca_32k_in.hw
+ },
+ .num_parents = 1,
+ },
+};
+
+static struct clk_regmap t7_ceca_32k_sel_pre = {
+ .data = &(struct clk_regmap_mux_data) {
+ .offset = CECA_CTRL1,
+ .mask = 0x1,
+ .shift = 24,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "ceca_32k_sel_pre",
+ .ops = &clk_regmap_mux_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_ceca_32k_div.hw,
+ &t7_ceca_32k_in.hw,
+ },
+ .num_parents = 2,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+};
+
+static struct clk_regmap t7_ceca_32k_sel = {
+ .data = &(struct clk_regmap_mux_data) {
+ .offset = CECA_CTRL1,
+ .mask = 0x1,
+ .shift = 31,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "ceca_32k_sel",
+ .ops = &clk_regmap_mux_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_ceca_32k_sel_pre.hw,
+ &t7_rtc.hw,
+ },
+ .num_parents = 2,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+};
+
+static struct clk_regmap t7_ceca_32k_out = {

Is the "_out" really necessary ? usually the last element just the base
clock name. Same for the other occurence.

ok, I will rename as ceca here. Same with cecb clocks.

+ .data = &(struct clk_regmap_gate_data){
+ .offset = CECA_CTRL0,
+ .bit_idx = 30,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "ceca_32k_out",
+ .ops = &clk_regmap_gate_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_ceca_32k_sel.hw
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+};
+
......
+static const struct clk_parent_data t7_dsp_ab_parent_data[] = {
+ { .fw_name = "xtal", },
+ { .fw_name = "fdiv2p5", },
+ { .fw_name = "fdiv3", },
+ { .fw_name = "fdiv5", },
+ { .fw_name = "hifi", },
+ { .fw_name = "fdiv4", },
+ { .fw_name = "fdiv7", },
+ { .hw = &t7_rtc.hw },
+};
+
+static struct clk_regmap t7_dspa_a_sel = {
+ .data = &(struct clk_regmap_mux_data){
+ .offset = DSPA_CLK_CTRL0,
+ .mask = 0x7,
+ .shift = 10,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "dspa_a_sel",
+ .ops = &clk_regmap_mux_ops,
+ .parent_data = t7_dsp_ab_parent_data,
+ .num_parents = ARRAY_SIZE(t7_dsp_ab_parent_data),
+ },
+};
+
+static struct clk_regmap t7_dspa_a_div = {
+ .data = &(struct clk_regmap_div_data){
+ .offset = DSPA_CLK_CTRL0,
+ .shift = 0,
+ .width = 10,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "dspa_a_div",
+ .ops = &clk_regmap_divider_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_dspa_a_sel.hw
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+};
+
+static struct clk_regmap t7_dspa_a = {
+ .data = &(struct clk_regmap_gate_data){
+ .offset = DSPA_CLK_CTRL0,
+ .bit_idx = 13,
+ },
+ .hw.init = &(struct clk_init_data) {
+ .name = "dspa_a",
+ .ops = &clk_regmap_gate_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_dspa_a_div.hw
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
+ },
+};
+
+static struct clk_regmap t7_dspa_b_sel = {
+ .data = &(struct clk_regmap_mux_data){
+ .offset = DSPA_CLK_CTRL0,
+ .mask = 0x7,
+ .shift = 26,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "dspa_b_sel",
+ .ops = &clk_regmap_mux_ops,
+ .parent_data = t7_dsp_ab_parent_data,
+ .num_parents = ARRAY_SIZE(t7_dsp_ab_parent_data),
+ },
+};
+
+static struct clk_regmap t7_dspa_b_div = {
+ .data = &(struct clk_regmap_div_data){
+ .offset = DSPA_CLK_CTRL0,
+ .shift = 16,
+ .width = 10,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "dspa_b_div",
+ .ops = &clk_regmap_divider_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_dspa_b_sel.hw
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+};
+
+static struct clk_regmap t7_dspa_b = {
+ .data = &(struct clk_regmap_gate_data){
+ .offset = DSPA_CLK_CTRL0,
+ .bit_idx = 29,
+ },
+ .hw.init = &(struct clk_init_data) {
+ .name = "dspa_b",
+ .ops = &clk_regmap_gate_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_dspa_b_div.hw
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
+ },
+};

You have several instances of these glitch free mux (dsp here, anakin
below, etc ...). That's quite verbose.

I suggest defining T7_COMP_GATE() such that flags is a parameter
Then use it for the sel, div and gate of element.

You'll just have to fully define the final mux element, like you have
below.

ok, I will use T7_COMP_SEL/DIV/GATE for dspa, dspb, anakin clocks. and leave the final mux.

+
+static struct clk_regmap t7_dspa = {
+ .data = &(struct clk_regmap_mux_data){
+ .offset = DSPA_CLK_CTRL0,
+ .mask = 0x1,
+ .shift = 15,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "dspa",
+ .ops = &clk_regmap_mux_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_dspa_a.hw,
+ &t7_dspa_b.hw,
+ },
+ .num_parents = 2,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+};
+
......
+
+static struct clk_regmap t7_anakin_0 = {

Nitpick: for the DSP it was a/b, here it is 0/1
Could you pick one way or the other and stick to it ?

ok , I will use 0/1 for DSP.

+ .data = &(struct clk_regmap_gate_data){
+ .offset = ANAKIN_CLK_CTRL,
+ .bit_idx = 8,
+ },
+ .hw.init = &(struct clk_init_data) {
+ .name = "anakin_0",
+ .ops = &clk_regmap_gate_ops,
+ .parent_hws = (const struct clk_hw *[]) { &t7_anakin_0_div.hw },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
+ },
+};
+
+static struct clk_regmap t7_anakin_1_sel = {
+ .data = &(struct clk_regmap_mux_data){
+ .offset = ANAKIN_CLK_CTRL,
+ .mask = 0x7,
+ .shift = 25,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "anakin_1_sel",
+ .ops = &clk_regmap_mux_ops,
+ .parent_data = t7_anakin_parent_data,
+ .num_parents = ARRAY_SIZE(t7_anakin_parent_data),
+ },
+};
+
+static struct clk_regmap t7_anakin_1_div = {
+ .data = &(struct clk_regmap_div_data){
+ .offset = ANAKIN_CLK_CTRL,
+ .shift = 16,
+ .width = 7,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "anakin_1_div",
+ .ops = &clk_regmap_divider_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_anakin_1_sel.hw
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT,
+ },
+};
+
+static struct clk_regmap t7_anakin_1 = {
+ .data = &(struct clk_regmap_gate_data){
+ .offset = ANAKIN_CLK_CTRL,
+ .bit_idx = 24,
+ },
+ .hw.init = &(struct clk_init_data) {
+ .name = "anakin_1",
+ .ops = &clk_regmap_gate_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_anakin_1_div.hw
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
+ },
+};
+
+static struct clk_regmap t7_anakin = {
+ .data = &(struct clk_regmap_mux_data){
+ .offset = ANAKIN_CLK_CTRL,
+ .mask = 1,
+ .shift = 31,
+ },
+ .hw.init = &(struct clk_init_data){
+ .name = "anakin_sel",
+ .ops = &clk_regmap_mux_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_anakin_0.hw,
+ &t7_anakin_1.hw
+ },
+ .num_parents = 2,
+ .flags = CLK_SET_RATE_PARENT
+ },
+};
+
+static struct clk_regmap t7_anakin_clk = {
+ .data = &(struct clk_regmap_gate_data){
+ .offset = ANAKIN_CLK_CTRL,
+ .bit_idx = 30,
+ },
+ .hw.init = &(struct clk_init_data) {
+ .name = "anakin_clk",

Again, not a great name, especially considering the one above.
Is this really really how the doc refers to these 2 clocks ?

bit30 gate clock is after bit31 mux clock, and the gate clock is the final output clock, it is used to gate anakin clock.

I will rename bit31 as anakin_pre, rename bit30 as anakin.

+ .ops = &clk_regmap_gate_ops,
+ .parent_hws = (const struct clk_hw *[]) {
+ &t7_anakin.hw
+ },
+ .num_parents = 1,
+ .flags = CLK_SET_RATE_PARENT
+ },
+};
+
......
+
+/* parent index 2, 3, 4, 5, 6 not connect any clock signal, they are empty */

Remove "they are empty" ... the rest is fine

ok.

+static u32 t7_eth_rmii_table[] = { 0, 1, 7 };

t7_eth_rmii_parents_val_table[] ... consistency (please check for other occurences)

ok, I will update the table name.

+
+static const struct clk_parent_data t7_eth_rmii_parents[] = {
+ { .fw_name = "fdiv2", },
+ { .fw_name = "gp1", },
+ { .fw_name = "ext_rmii", },
+};
+
......
+
+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_DESCRIPTION("Amlogic T7 Peripherals Clock Controller driver");
+module_platform_driver(t7_peripherals_clkc_driver);

This is an odd placement for this.
Please move this right after the platform_driver definition. Same goes
for the other controller

ok, I will move it after t7_peripherals_clkc_driver. same with T7 PLL driver.

+MODULE_AUTHOR("Jian Hu <jian.hu@xxxxxxxxxxx>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("CLK_MESON");

--
Jerome

Next message: lihuisong (C): "Re: [PATCH v2 1/7] ACPI: processor: idle: Mark the state as invalid if its entry method is illegal"
Previous message: Kanchana P Sridhar: "[PATCH v13 19/22] mm: zswap: Per-CPU acomp_ctx resources exist from pool creation to deletion."
Next in thread: Jerome Brunet: "Re: [PATCH v4 5/5] clk: meson: t7: add t7 clock peripherals controller driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]