Message-ID: <1jtu4rzevv.fsf@starbuckisacylon.baylibre.com>
Date:   Wed, 28 Sep 2022 17:35:00 +0200
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Yu Tu <yu.tu@...ogic.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


On Wed 21 Sep 2022 at 17:01, Yu Tu <yu.tu@...ogic.com> wrote:

> 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.
>

I missed the vid_pll type, Ok then.

>> 
>>>>
>>>>>
>>>>>> +        .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?
>

ok

>> 
>>>
>>>>>
>>>>>> +    },
>>>>>> +};
>>>>>> +
>>>>>> +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".
>

If you clock rate propagation is ok, then OK

>> 
>>>
>>>>>> +
>>>>>> +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?
>

ok

>> 
>>>>>
>>>
>>>
>>>>>> +/*
>>>>>> + * 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