| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ec0de6c5-36a7-48ff-9ae9-2b4fce8bb8f0@amlogic.com>
Date: Fri, 17 Jan 2025 18:36:58 +0800
From: Jian Hu <jian.hu@...ogic.com>
To: Jerome Brunet <jbrunet@...libre.com>
Cc: Xianwei Zhao <xianwei.zhao@...ogic.com>, Chuan Liu
<chuan.liu@...ogic.com>, Neil Armstrong <neil.armstrong@...aro.org>,
Kevin Hilman <khilman@...libre.com>, Stephen Boyd <sboyd@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
Dmitry Rokosov <ddrokosov@...rdevices.ru>, robh+dt <robh+dt@...nel.org>,
Rob Herring <robh@...nel.org>, devicetree <devicetree@...r.kernel.org>,
linux-clk <linux-clk@...r.kernel.org>,
linux-amlogic <linux-amlogic@...ts.infradead.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 5/5] clk: meson: t7: add t7 clock peripherals
controller driver
On 2025/1/14 2:18, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
>
> On Wed 08 Jan 2025 at 17:40, Jian Hu <jian.hu@...ogic.com> wrote:
>
>> Add Peripheral clock controller driver for the Amlogic T7 SoC family.
>>
>> Signed-off-by: Jian Hu <jian.hu@...ogic.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
Powered by blists - more mailing lists