| 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: <3b9a5978-aa02-486b-85f5-6443dc607dd5@amlogic.com>
Date: Tue, 4 Nov 2025 17:17:50 +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 v4 5/5] clk: meson: t7: add t7 clock peripherals
controller driver
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@...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 | 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@...ogic.com>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_IMPORT_NS("CLK_MESON");
> --
> Jerome
Powered by blists - more mailing lists