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

[Jian Hu <jian.hu@amlogic.com>]

On 2025/1/14 2:18, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Wed 08 Jan 2025 at 17:40, Jian Hu <jian.hu@amlogic.com> wrote:
>
>> Add Peripheral clock controller driver for the Amlogic T7 SoC family.
>>
>> Signed-off-by: Jian Hu <jian.hu@amlogic.com>
>> ---
>>   drivers/clk/meson/Kconfig          |   13 +
>>   drivers/clk/meson/Makefile         |    1 +
>>   drivers/clk/meson/t7-peripherals.c | 2323 ++++++++++++++++++++++++++++
>>   3 files changed, 2337 insertions(+)
>>   create mode 100644 drivers/clk/meson/t7-peripherals.c
>>
>> ......
>> +
>> +#define SPI_PWM_CLK_MUX(_name, _reg, _mask, _shift, _parent_data) {  \
> The same macros keeps getting defined again and again.
> This has been going on for too long now.
>
> I'm addressing the problem it will take a bit of time and I guess it
> will delay t7 and a5 a bit.


Great,  a common macro can be applied to c3/t7/a5. Wait for your good news.

>> +     .data = &(struct clk_regmap_mux_data) {                 \
>> +             .offset = _reg,                                 \
>> +             .mask = _mask,                                  \
>> +             .shift = _shift,                                \
>> +     },                                                      \
>> +     .hw.init = &(struct clk_init_data) {                    \
>> +             .name = #_name "_sel",                          \
>> +             .ops = &clk_regmap_mux_ops,                     \
>> +             .parent_data = _parent_data,                    \
>> +             .num_parents = ARRAY_SIZE(_parent_data),        \
>> +     },                                                      \
>> +}
>> +
>> +#define SPI_PWM_CLK_DIV(_name, _reg, _shift, _width, _parent) {      \
>> +     .data = &(struct clk_regmap_div_data) {                 \
>> +             .offset = _reg,                                 \
>> +             .shift = _shift,                                \
>> +             .width = _width,                                \
>> +     },                                                      \
>> +     .hw.init = &(struct clk_init_data) {                    \
>> +             .name = #_name "_div",                          \
>> +             .ops = &clk_regmap_divider_ops,                 \
>> +             .parent_hws = (const struct clk_hw *[]) {       \
>> +                     &_parent.hw                             \
>> +             },                                              \
>> +             .num_parents = 1,                               \
>> +             .flags = CLK_SET_RATE_PARENT,                   \
>> +     },                                                      \
>> +}
>> +
>> +#define SPI_PWM_CLK_GATE(_name, _reg, _bit, _parent) {               \
>> +     .data = &(struct clk_regmap_gate_data) {                \
>> +             .offset = _reg,                                 \
>> +             .bit_idx = _bit,                                \
>> +     },                                                      \
>> +     .hw.init = &(struct clk_init_data) {                    \
>> +             .name = #_name,                                 \
>> +             .ops = &clk_regmap_gate_ops,                    \
>> +             .parent_hws = (const struct clk_hw *[]) {       \
>> +                     &_parent.hw                             \
>> +             },                                              \
>> +             .num_parents = 1,                               \
>> +             .flags = CLK_SET_RATE_PARENT,                   \
>> +     },                                                      \
>> +}
>> +
>> +static const struct clk_parent_data spicc_parents[] = {
>> +     { .fw_name = "xtal", },
>> +     { .fw_name = "sys", },
>> +     { .fw_name = "fdiv4", },
>> +     { .fw_name = "fdiv3", },
>> +     { .fw_name = "fdiv2", },
>> +     { .fw_name = "fdiv5", },
>> +     { .fw_name = "fdiv7", },
>> +     { .fw_name = "gp1", },
>> +};
>> +
>> +static struct clk_regmap spicc0_sel =
>> +     SPI_PWM_CLK_MUX(spicc0, CLKCTRL_SPICC_CLK_CTRL, 0x7, 7, spicc_parents);
>> +static struct clk_regmap spicc0_div = SPI_PWM_CLK_DIV(spicc0, CLKCTRL_SPICC_CLK_CTRL, 0, 6, spicc0_sel);
>> +static struct clk_regmap spicc0 = SPI_PWM_CLK_GATE(spicc0, CLKCTRL_SPICC_CLK_CTRL, 6, spicc0_div);
>> +
>> ......
>> +
>> +#define T7_CLK_GATE(_name, _reg, _bit, _fw_name, _flags)             \
> See, redefining the peripheral once again ... something all the SoCs
> uses with minor variation.


A common macro is better for it.

there is common macro in clk/meson/clk-regmap.h, but CLK_IGNORE_UNUSED 
flag is added.

it can not be used here. maybe we can rework the macro or new one for it.

>> +struct clk_regmap _name = {                                          \
>> +     .data = &(struct clk_regmap_gate_data){                         \
>> +             .offset = (_reg),                                       \
>> +             .bit_idx = (_bit),                                      \
>> +     },                                                              \
>> +     .hw.init = &(struct clk_init_data) {                            \
>> +             .name = #_name,                                         \
> There is an exception in the naming convention for peripheral clocks.
>
> The name is soc id prefixed in most SoC. It is these pointless minor
> diff that makes factorisation difficult.


Yes.  I think a common MESON_GATE or AMLOGIC_GATE is okay for it.

And how about this change for it? it remove the old defination and add 
new one 'MESON_GATE' in clk-regmap.h :

--- a/drivers/clk/meson/clk-regmap.h
+++ b/drivers/clk/meson/clk-regmap.h
@@ -114,7 +114,7 @@ clk_get_regmap_mux_data(struct clk_regmap *clk)
  extern const struct clk_ops clk_regmap_mux_ops;
  extern const struct clk_ops clk_regmap_mux_ro_ops;

-#define __MESON_PCLK(_name, _reg, _bit, _ops, _pname)                  \
+#define __MESON_PCLK(_name, _reg, _bit, _ops, _pname, _flag)           \
  struct clk_regmap _name = {                                            \
         .data = &(struct clk_regmap_gate_data){                         \
                 .offset = (_reg),                                       \
@@ -125,13 +125,19 @@ struct clk_regmap _name = 
{                                               \
                 .ops = _ops,                                            \
                 .parent_hws = (const struct clk_hw *[]) { _pname },     \
                 .num_parents = 1,                                       \
-               .flags = (CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED),     \
+               .flags = (CLK_SET_RATE_PARENT | flag),                  \
},                                                              \
  }

  #define MESON_PCLK(_name, _reg, _bit, _pname)  \
-       __MESON_PCLK(_name, _reg, _bit, &clk_regmap_gate_ops, _pname)
+       __MESON_PCLK(_name, _reg, _bit, &clk_regmap_gate_ops, _pname, 
CLK_IGNORE_UNUSED)

  #define MESON_PCLK_RO(_name, _reg, _bit, _pname)       \
-       __MESON_PCLK(_name, _reg, _bit, &clk_regmap_gate_ro_ops, _pname)
+       __MESON_PCLK(_name, _reg, _bit, &clk_regmap_gate_ro_ops, _pname, 
CLK_IGNORE_UNUSED)
+
+#define MESON_GATE(_name, _reg, _bit, _pname, _flag)   \
+       __MESON_PCLK(_name, _reg, _bit, &clk_regmap_gate_ops, _pname, _flag)
+
+#define MESON_GATE_RO(_name, _reg, _bit, _pname, _flag)        \
+       __MESON_PCLK(_name, _reg, _bit, &clk_regmap_gate_ro_ops, _pname, 
_flag)
  #endif /* __CLK_REGMAP_H */
diff --git a/drivers/clk/meson/t7-peripherals.c 
b/drivers/clk/meson/t7-peripherals.c
index 362000fe4a7f..3a1aec703618 100644
--- a/drivers/clk/meson/t7-peripherals.c
+++ b/drivers/clk/meson/t7-peripherals.c
@@ -1750,25 +1750,10 @@ static struct clk_regmap t7_pwm_ao_h_div =
  static struct clk_regmap t7_pwm_ao_h =
         SPI_PWM_CLK_GATE(t7_pwm_ao_h, CLKCTRL_PWM_CLK_AO_GH_CTRL, 24, 
t7_pwm_ao_h_div);

-#define T7_CLK_GATE(_name, _reg, _bit, _fw_name, _flags)               \
-struct clk_regmap _name = {                                            \
-       .data = &(struct clk_regmap_gate_data){                         \
-               .offset = (_reg),                                       \
-               .bit_idx = (_bit),                                      \
- },                                                              \
-       .hw.init = &(struct clk_init_data) {                            \
-               .name = #_name,                                         \
-               .ops = &clk_regmap_gate_ops,                            \
-               .parent_data = &(const struct clk_parent_data) {        \
-                       .fw_name = #_fw_name,                           \
- },                                                      \
-               .num_parents = 1,                                       \
-               .flags = (_flags),                                      \
- },                                                              \
-}
+#define T7_SYS_GATE(_name, _reg, _bit, _flag)                          \
+       MESON_GATE(_name, _reg, _bit, &sys.hw, _flag)


-#define T7_SYS_GATE(_name, _reg, _bit, _flags)                         \
-       T7_CLK_GATE(_name, _reg, _bit, sys, _flags)
+static T7_SYS_GATE(t7_sys_ddr, CLKCTRL_SYS_CLK_EN0_REG0, 0,    0);


or another patch based above.  T7_SYS_GATE macro is not necessary,  and 
'&sys.hw' is needed for each sys clock.

and we can define the clocks like:

-#define T7_SYS_GATE(_name, _reg, _bit, _flag)                          \
-       MESON_GATE(_name, _reg, _bit, &sys.hw, _flag)

+static MESON_GATE(t7_sys_ddr, CLKCTRL_SYS_CLK_EN0_REG0, 0, &sys.hw,   0);

+static MESON_GATE(t7_sys_dos, CLKCTRL_SYS_CLK_EN0_REG0, 0, &sys.hw,   1);


Please help to review it,

>> +             .ops = &clk_regmap_gate_ops,                            \
>> +             .parent_data = &(const struct clk_parent_data) {        \
>> +                     .fw_name = #_fw_name,                           \
>> +             },                                                      \
>> +             .num_parents = 1,                                       \
>> +             .flags = (_flags),                                      \
>> +     },                                                              \
>> +}
>> +
>> +#define T7_SYS_GATE(_name, _reg, _bit, _flags)                               \
>> +     T7_CLK_GATE(_name, _reg, _bit, sys, _flags)
>> +
>> +static T7_SYS_GATE(sys_ddr,          CLKCTRL_SYS_CLK_EN0_REG0, 0,    0);
>>
>> ......
> --
> Jerome

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic