| 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: <0b621dfa-4ecd-a9a6-8681-8ab8b00d7841@amlogic.com>
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