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
| ||
|
Date: Wed, 21 Sep 2022 17:01:03 +0800 From: Yu Tu <yu.tu@...ogic.com> To: Jerome Brunet <jbrunet@...libre.com>, <linux-clk@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>, <linux-amlogic@...ts.infradead.org>, <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>, Rob Herring <robh+dt@...nel.org>, Neil Armstrong <narmstrong@...libre.com>, Kevin Hilman <khilman@...libre.com>, Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Martin Blumenstingl <martin.blumenstingl@...glemail.com> Subject: Re: [PATCH V3 6/6] clk: meson: s4: add s4 SoC peripheral clock controller driver Hi Jerome, On 2022/8/30 16:20, Yu Tu wrote: > > > On 2022/8/29 20:19, Jerome Brunet wrote: >> [ EXTERNAL EMAIL ] >> >> >> On Tue 16 Aug 2022 at 20:00, Yu Tu <yu.tu@...ogic.com> wrote: >> >> Please trim your replies >> >>>>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig >>>>> index f4244edc7b28..ec6beb9284d3 100644 >>>>> --- a/drivers/clk/meson/Kconfig >>>>> +++ b/drivers/clk/meson/Kconfig >>>>> @@ -127,4 +127,17 @@ config COMMON_CLK_S4_PLL >>>>> Support for the pll clock controller on Amlogic S805X2 and >>>>> S905Y4 devices, >>>>> aka s4. Amlogic S805X2 and S905Y4 devices include AQ222 >>>>> and AQ229. >>>>> Say Y if you want peripherals and CPU frequency scaling to >>>>> work. >>>>> + >>>>> +config COMMON_CLK_S4 >>>>> + tristate "S4 SoC Peripherals clock controllers support" >>>>> + depends on ARM64 >>>>> + default y >>>>> + select COMMON_CLK_MESON_REGMAP >>>>> + select COMMON_CLK_MESON_DUALDIV >>>>> + select COMMON_CLK_MESON_VID_PLL_DIV >>>>> + select COMMON_CLK_S4_PLL >>>> Do you really this ? your driver does not even include the related >>>> header. >>> If the PLL driver is not turned on in DTS, will it not cause an error? >>>> >> >> I don't get the question. >> Kconfig list compile deps. S4 PLL is not a compile dep of the peripheral >> controller. >> >> If you really want to, you may use 'imply'. > > V4 has been changed as you suggested. The next edition is being changed according to your requirements. Please give us your valuable opinions. > >>>> >>>>> +static const struct clk_parent_data sys_ab_clk_parent_data[] = { >>>>> + { .fw_name = "xtal" }, >>>>> + { .fw_name = "fclk_div2" }, >>>>> + { .fw_name = "fclk_div3" }, >>>>> + { .fw_name = "fclk_div4" }, >>>>> + { .fw_name = "fclk_div5" }, >>>>> + { .fw_name = "fclk_div7" }, >>>>> + { .hw = &s4_rtc_clk.hw } >>>>> +}; >>>>> + >>>>> +static struct clk_regmap s4_sysclk_b_sel = { >>>>> + .data = &(struct clk_regmap_mux_data){ >>>>> + .offset = CLKCTRL_SYS_CLK_CTRL0, >>>>> + .mask = 0x7, >>>>> + .shift = 26, >>>>> + .table = mux_table_sys_ab_clk_sel, >>>>> + }, >>>>> + .hw.init = &(struct clk_init_data){ >>>>> + .name = "sysclk_b_sel", >>>>> + .ops = &clk_regmap_mux_ro_ops, >>>> Why is this using the RO ops ? >>> Sys_clk is initialized during the Uboot phase and is fixed at >>> 166.666MHz. So I'm going to change it to ro. >> >> That really much depends on the bootloader and is a pretty weak design. >> The bootloader deps should be kept as minimal as possible. >> >> I see no reason for RO. >> >> You may cut rate propagation on the user if you need to and continue to >> whatever you want in your u-boot > > I think I know what you mean. But we let the user be in control and not > set the frequency, which can be risky. If you insist, I will change it > as you suggest. It has been changed as you requested. > >> >>>> >>>>> + .parent_data = sys_ab_clk_parent_data, >>>>> + .num_parents = ARRAY_SIZE(sys_ab_clk_parent_data), >>>>> + }, >>>>> +}; >>>>> + >>>>> +static struct clk_regmap s4_sysclk_b_div = { >>>>> + .data = &(struct clk_regmap_div_data){ >>>>> + .offset = CLKCTRL_SYS_CLK_CTRL0, >>>>> + .shift = 16, >>>>> + .width = 10, >>>>> + }, >>>>> + .hw.init = &(struct clk_init_data){ >>>>> + .name = "sysclk_b_div", >>>>> + .ops = &clk_regmap_divider_ro_ops, >>>> Same here and for the rest of the sys part >>> Same above. >> >> We can play that game for a while > > Ah, you're so funny. > >> >>>>> + >>>>> +/* Video Clocks */ >>>>> +static struct clk_regmap s4_vid_pll_div = { >>>>> + .data = &(struct meson_vid_pll_div_data){ >>>>> + .val = { >>>>> + .reg_off = CLKCTRL_VID_PLL_CLK_DIV, >>>>> + .shift = 0, >>>>> + .width = 15, >>>>> + }, >>>>> + .sel = { >>>>> + .reg_off = CLKCTRL_VID_PLL_CLK_DIV, >>>>> + .shift = 16, >>>>> + .width = 2, >>>>> + }, >>>>> + }, >>>>> + .hw.init = &(struct clk_init_data) { >>>>> + .name = "vid_pll_div", >>>>> + .ops = &meson_vid_pll_div_ro_ops, >>>> Why RO ? applies to the rest of the video part. >>> Because vid_pll_div parent is HDMI_PLL, and HDMI_PLL is a fixed >>> frequency. Flags is CLK_SET_RATE_PARENT. So we use RO. >> >> If the HDMI_PLL is fixed somehow, that is not reason for this clock to >> be RO >> >>> Can I remove RO and use CLK_SET_RATE_NO_REPARENT instead, which one >>> do you >>> think is more reasonable? >> >> Neither. CLK_SET_RATE_NO_REPARENT makes no sense, it is not mux >> > > "drivers/clk/meson/vid-pll-div.c" > This file only provides ro_ops. Maybe the submission records will give > us the answer. > > In fact, our hardware design is the same as the G12 series. I don't know if you checked this commit, but there is only one "ro_ops"in this place right now. The S4 SoC is consistent with the G12A/B and GX series. > >>> >>>> >>>>> + .parent_data = (const struct clk_parent_data []) { >>>>> + { .fw_name = "hdmi_pll", } >>>>> + }, >>>>> + .num_parents = 1, >>>>> + .flags = CLK_SET_RATE_PARENT, >>>>> + }, >>>>> +}; >>>>> + >>>>> +static struct clk_regmap s4_vid_pll_sel = { >>>>> + .data = &(struct clk_regmap_mux_data){ >>>>> + .offset = CLKCTRL_VID_PLL_CLK_DIV, >>>>> + .mask = 0x1, >>>>> + .shift = 18, >>>>> + }, >>>>> + .hw.init = &(struct clk_init_data){ >>>>> + .name = "vid_pll_sel", >>>>> + .ops = &clk_regmap_mux_ops, >>>>> + /* >>>>> + * bit 18 selects from 2 possible parents: >>>>> + * vid_pll_div or hdmi_pll >>>>> + */ >>>>> + .parent_data = (const struct clk_parent_data []) { >>>>> + { .hw = &s4_vid_pll_div.hw }, >>>>> + { .fw_name = "hdmi_pll", } >>>>> + }, >>>>> + .num_parents = 2, >>>>> + .flags = CLK_SET_RATE_NO_REPARENT, >>>> Why ? are you planning to DT assigned clocks to statically set this ? >>> Because vid_pll_sel one parent is HDMI_PLL, and HDMI_PLL is a fixed >>> frequency. To prevent modification, use CLK_SET_RATE_NO_REPARENT. >> >> Again, this makes no sense. > > Unfortunately you don't read V4, in fact I have corrected in V4. > > ".flags = CLK_SET_RATE_PARENT," in V4. Is that okay with you? I don't know what you think? > >> >>>> >>>>> + }, >>>>> +}; >>>>> + >>>>> +static struct clk_regmap s4_vid_pll = { >>>>> + .data = &(struct clk_regmap_gate_data){ >>>>> + .offset = CLKCTRL_VID_PLL_CLK_DIV, >>>>> + .bit_idx = 19, >>>>> + }, >>>>> + .hw.init = &(struct clk_init_data) { >>>>> + .name = "vid_pll", >>>>> + .ops = &clk_regmap_gate_ops, >>>>> + .parent_hws = (const struct clk_hw *[]) { >>>>> + &s4_vid_pll_sel.hw >>>>> + }, >>>>> + .num_parents = 1, >>>>> + .flags = CLK_SET_RATE_PARENT, >>>>> + }, >>>>> +}; >>>>> + >>>>> +static const struct clk_parent_data s4_vclk_parent_data[] = { >>>>> + { .hw = &s4_vid_pll.hw }, >>>>> + { .fw_name = "gp0_pll", }, >>>>> + { .fw_name = "hifi_pll", }, >>>>> + { .fw_name = "mpll1", }, >>>>> + { .fw_name = "fclk_div3", }, >>>>> + { .fw_name = "fclk_div4", }, >>>>> + { .fw_name = "fclk_div5", }, >>>>> + { .fw_name = "fclk_div7", }, >>>>> +}; >>>>> + >>>>> +static struct clk_regmap s4_vclk_sel = { >>>>> + .data = &(struct clk_regmap_mux_data){ >>>>> + .offset = CLKCTRL_VID_CLK_CTRL, >>>>> + .mask = 0x7, >>>>> + .shift = 16, >>>>> + }, >>>>> + .hw.init = &(struct clk_init_data){ >>>>> + .name = "vclk_sel", >>>>> + .ops = &clk_regmap_mux_ops, >>>>> + .parent_data = s4_vclk_parent_data, >>>>> + .num_parents = ARRAY_SIZE(s4_vclk_parent_data), >>>>> + .flags = CLK_SET_RATE_NO_REPARENT, >>>> Same >>> Since fclk_div* is a fixed frequency value, mplL1 and hifi_pll and >>> gp0_pll >>> are used by other specialized modules, vid_pll has >>> CLK_SET_RATE_PARENT. The >>> parent of vid_pll is that vid_pll_sel uses CLK_SET_RATE_NO_REPARENT. >> >> Still not good. >> >> You don't have CLK_SET_RATE, propagation is stopped and parent clock >> will not changed. The best parent will be picked but not changed. >> >> If one parent MUST NOT be picked, just remove it from the list and add a >> explaining why >> >> [...] > > Okay. In the next edition I will change it to ".flags = CLK_SET_RATE_PARENT". > >> >>>>> + >>>>> +static struct clk_regmap s4_ts_clk_div = { >>>>> + .data = &(struct clk_regmap_div_data){ >>>>> + .offset = CLKCTRL_TS_CLK_CTRL, >>>>> + .shift = 0, >>>>> + .width = 8, >>>>> + }, >>>>> + .hw.init = &(struct clk_init_data){ >>>>> + .name = "ts_clk_div", >>>>> + .ops = &clk_regmap_divider_ops, >>>>> + .parent_data = &(const struct clk_parent_data) { >>>>> + .fw_name = "xtal", >>>>> + }, >>>>> + .num_parents = 1, >>>> propagation stopped ? >>> Its parent is xtal, so I should use CLK_SET_RATE_NO_REPARENT. >> >> Still no. You seem to have problem with the meaning of >> CLK_SET_RATE_NO_REPARENT. >> >> * CLK_SET_RATE_NO_REPARENT: means the parent will no be changed, even if >> selecting another parent would result in a closer rate to the >> request. It makes sense only if the clock has several parents >> >> * CLK_SET_RATE_PARENT: means rate change may propagate the parent, >> meaning the rate of the parent may change if it help the child achieve >> a closer rate to the request > > Thank you for explaining.I got it. > >> >>>> >>>>> + }, >>>>> +}; >>>>> + >>>>> +static struct clk_regmap s4_ts_clk_gate = { >>>>> + .data = &(struct clk_regmap_gate_data){ >>>>> + .offset = CLKCTRL_TS_CLK_CTRL, >>>>> + .bit_idx = 8, >>>>> + }, >>>>> + .hw.init = &(struct clk_init_data){ >>>>> + .name = "ts_clk", >>>>> + .ops = &clk_regmap_gate_ops, >>>>> + .parent_hws = (const struct clk_hw *[]) { >>>>> + &s4_ts_clk_div.hw >>>>> + }, >>>>> + .num_parents = 1, >>>>> + }, >>>> propagation stopped ? >>> I will add CLK_SET_RATE_PARENT. >> >> [...] >> >>>>> +/* EMMC/NAND clock */ >>>>> + >>>>> +static const struct clk_parent_data s4_sd_emmc_clk0_parent_data[] = { >>>>> + { .fw_name = "xtal", }, >>>>> + { .fw_name = "fclk_div2", }, >>>>> + { .fw_name = "fclk_div3", }, >>>>> + { .fw_name = "hifi_pll", }, >>>>> + { .fw_name = "fclk_div2p5", }, >>>>> + /* >>>>> + * Following these parent clocks, we should also have had >>>>> mpll2, mpll3 >>>>> + * and gp0_pll but these clocks are too precious to be used >>>>> here. All >>>>> + * the necessary rates for MMC and NAND operation can be >>>>> acheived using >>>>> + * hifi_pll or fclk_div clocks >>>>> + */ >>>> You don't want to list mplls but hifi_pll is fine ? seems dangerous. >>> hifi pll is for EMMC and NAND on this SoC. >> >> That deserve a better explanation. >> Why can't it use fdiv2 and xtal like the previous SoCs ? >> >> Which PLLs are you using for Audio then ? >> Typical operation on these SoCs usually require 3 PLLs to acheive all >> rates >> > > I'll list all the clocks and let the driver itself select Parent as needed. I don't know what you think? > >>>> >> >> >>>>> +/* >>>>> + * gen clk is designed for debug/monitor some internal clock >>>>> quality. Some of the >>>>> + * corresponding clock sources are not described in the clock >>>>> tree, so they are skipped. >>>>> + */ >>>> Still feels a bit light, don't you think ? Among all the clocks, can't >>>> you add a bit more parents here ? It would certainly help debug down >>>> the road >>> [16:12] is gen_clk source select.All is: >>> 0: cts_oscin_clk >>> 1:cts_rtc_clk >>> 2:sys_pll_div16 (internal clock) >>> 3:ddr_pll_div32 (internal clock) >>> 4: vid_pll >>> 5: gp0_pll >>> 7: hifi_pll >>> 10:adc_dpll_clk_b3 (internal clock for debug) >>> 11:adc_dpll_intclk (internal clock for debug) >>> 12:clk_msr_src(select from all internal clock except PLLs); >>> 16: no used >>> 17: sys_cpu_clk_div16 (internal clock) >>> 19: fclk_div2 >>> 20: fclk_div2p5 >>> 21: fclk_div3 >>> 22: fclk_div4 >>> 23: fclk_div5 >>> 24: fclk_div7 >>> 25: mpll0 >>> 26: mpll1 >>> 27: mpll2 >>> 28: mpll3 >>> So i only added the clocks that will actually be used, and some >>> debugging >>> clock peripherals will not be used. >> >> you may at least add vid_pll > > Okay. It has been changed as you suggested. > >> >>>> >>>>> +static u32 s4_gen_clk_mux_table[] = { 0, 5, 7, 19, 21, 22, >>>>> + 23, 24, 25, 26, 27, 28 }; >>>>> +static const struct clk_parent_data s4_gen_clk_parent_data[] = { >>>>> + { .fw_name = "xtal", }, >>>>> + { .fw_name = "gp0_pll", }, >>>>> + { .fw_name = "hifi_pll", }, >>>>> + { .fw_name = "fclk_div2", }, >>>>> + { .fw_name = "fclk_div3", }, >>>>> + { .fw_name = "fclk_div4", }, >>>>> + { .fw_name = "fclk_div5", }, >>>>> + { .fw_name = "fclk_div7", }, >>>>> + { .fw_name = "mpll0", }, >>>>> + { .fw_name = "mpll1", }, >>>>> + { .fw_name = "mpll2", }, >>>>> + { .fw_name = "mpll3", }, >>>>> +}; >> >> .
Powered by blists - more mailing lists