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 [off-list ref] wrote:

quoted

Add Peripheral clock controller driver for the Amlogic T7 SoC family.

Signed-off-by: Jian Hu <redacted>
---
  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

quoted

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

quoted

+     },
+};
+
+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.

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+MODULE_AUTHOR("Jian Hu [off-list ref]");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("CLK_MESON");

--
Jerome