Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20230417065005.24967-1-yu.tu@amlogic.com>
 <20230417065005.24967-5-yu.tu@amlogic.com>
 <20230426110538.y2d4qhv2qffc62vj@CAB-WSD-L081021>
 <6cc4c3f2-4df5-6456-fe65-71aa4a13c866@amlogic.com>
User-agent: mu4e 1.8.13; emacs 28.2
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Yu Tu <yu.tu@amlogic.com>,
        Dmitry Rokosov <ddrokosov@sberdevices.ru>
Cc:     linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        kelvin.zhang@amlogic.com, qi.duan@amlogic.com
Subject: Re: [PATCH V7 4/4] clk: meson: s4: add support for Amlogic S4 SoC
 peripheral clock controller
Date:   Mon, 01 May 2023 19:37:58 +0200
In-reply-to: <6cc4c3f2-4df5-6456-fe65-71aa4a13c866@amlogic.com>
Message-ID: <1jpm7kuev7.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


On Thu 27 Apr 2023 at 16:15, Yu Tu <yu.tu@amlogic.com> wrote:

> On 2023/4/26 19:05, Dmitry Rokosov wrote:
>> [Some people who received this message don't often get email from
>> ddrokosov@sberdevices.ru. Learn why this is important at
>> https://aka.ms/LearnAboutSenderIdentification ]
>> [ EXTERNAL EMAIL ]
>> On Mon, Apr 17, 2023 at 02:50:05PM +0800, Yu Tu wrote:
>>> Add the peripherals clock controller driver in the s4 SoC family.
>>>
>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>> ---
>>>   drivers/clk/meson/Kconfig          |   12 +
>>>   drivers/clk/meson/Makefile         |    1 +
>>>   drivers/clk/meson/s4-peripherals.c | 3814 ++++++++++++++++++++++++++++
>>>   drivers/clk/meson/s4-peripherals.h |  217 ++
>>>   4 files changed, 4044 insertions(+)
>>>   create mode 100644 drivers/clk/meson/s4-peripherals.c
>>>   create mode 100644 drivers/clk/meson/s4-peripherals.h
>>>
>>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>>> index a663c90a3f3b..a6eb9fa15c74 100644
>>> --- a/drivers/clk/meson/Kconfig
>>> +++ b/drivers/clk/meson/Kconfig
>>> @@ -128,4 +128,16 @@ config COMMON_CLK_S4_PLL
>>>          aka s4. Amlogic S805X2 and S905Y4 devices include AQ222 and AQ229.
>>>          Say Y if you want the board to work, because plls are the parent of most
>>>          peripherals.
>>> +
>>> +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
>>> +     help
>>> +       Support for the Peripherals 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 to work.
>>>   endmenu
>>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>>> index 376f49cc13f1..c9130afccb48 100644
>>> --- a/drivers/clk/meson/Makefile
>>> +++ b/drivers/clk/meson/Makefile
>>> @@ -20,3 +20,4 @@ obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o
>>>   obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o
>>>   obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o meson8-ddr.o
>>>   obj-$(CONFIG_COMMON_CLK_S4_PLL) += s4-pll.o
>>> +obj-$(CONFIG_COMMON_CLK_S4) += s4-peripherals.o
>> [...]
>
> ?
>
>> 
>>> +static struct clk_regmap s4_ceca_32k_clkin = {
>>> +     .data = &(struct clk_regmap_gate_data){
>>> +             .offset = CLKCTRL_CECA_CTRL0,
>>> +             .bit_idx = 31,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data) {
>>> +             .name = "ceca_32k_clkin",
>>> +             .ops = &clk_regmap_gate_ops,
>>> +             .parent_data = (const struct clk_parent_data []) {
>>> +                     { .fw_name = "xtal", }
>>> +             },
>>> +             .num_parents = 1,
>>> +     },
>>> +};
>>> +
>>> +static struct clk_regmap s4_ceca_32k_div = {
>>> +     .data = &(struct meson_clk_dualdiv_data){
>>> +             .n1 = {
>>> +                     .reg_off = CLKCTRL_CECA_CTRL0,
>>> +                     .shift   = 0,
>>> +                     .width   = 12,
>>> +             },
>>> +             .n2 = {
>>> +                     .reg_off = CLKCTRL_CECA_CTRL0,
>>> +                     .shift   = 12,
>>> +                     .width   = 12,
>>> +             },
>>> +             .m1 = {
>>> +                     .reg_off = CLKCTRL_CECA_CTRL1,
>>> +                     .shift   = 0,
>>> +                     .width   = 12,
>>> +             },
>>> +             .m2 = {
>>> +                     .reg_off = CLKCTRL_CECA_CTRL1,
>>> +                     .shift   = 12,
>>> +                     .width   = 12,
>>> +             },
>>> +             .dual = {
>>> +                     .reg_off = CLKCTRL_CECA_CTRL0,
>>> +                     .shift   = 28,
>>> +                     .width   = 1,
>>> +             },
>>> +             .table = s4_32k_div_table,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data){
>>> +             .name = "ceca_32k_div",
>>> +             .ops = &meson_clk_dualdiv_ops,
>>> +             .parent_hws = (const struct clk_hw *[]) {
>>> +                     &s4_ceca_32k_clkin.hw
>>> +             },
>>> +             .num_parents = 1,
>>> +     },
>>> +};
>>> +
>>> +static struct clk_regmap s4_ceca_32k_sel_pre = {
>>> +     .data = &(struct clk_regmap_mux_data) {
>>> +             .offset = CLKCTRL_CECA_CTRL1,
>>> +             .mask = 0x1,
>>> +             .shift = 24,
>>> +             .flags = CLK_MUX_ROUND_CLOSEST,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data){
>>> +             .name = "ceca_32k_sel_pre",
>>> +             .ops = &clk_regmap_mux_ops,
>>> +             .parent_hws = (const struct clk_hw *[]) {
>>> +                     &s4_ceca_32k_div.hw,
>>> +                     &s4_ceca_32k_clkin.hw
>>> +             },
>>> +             .num_parents = 2,
>>> +             .flags = CLK_SET_RATE_PARENT,
>>> +     },
>>> +};
>>> +
>>> +static struct clk_regmap s4_ceca_32k_sel = {
>>> +     .data = &(struct clk_regmap_mux_data) {
>>> +             .offset = CLKCTRL_CECA_CTRL1,
>>> +             .mask = 0x1,
>>> +             .shift = 31,
>>> +             .flags = CLK_MUX_ROUND_CLOSEST,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data){
>>> +             .name = "ceca_32k_sel",
>>> +             .ops = &clk_regmap_mux_ops,
>>> +             .parent_hws = (const struct clk_hw *[]) {
>>> +                     &s4_ceca_32k_sel_pre.hw,
>>> +                     &s4_rtc_clk.hw
>>> +             },
>>> +             .num_parents = 2,
>>> +             .flags = CLK_SET_RATE_PARENT,
>> In my opinion, all clocks that can inherit from a more accurate RTC clock
>> should be marked with the CLK_SET_RATE_NO_REPARENT flag.
>> This is necessary because in certain situations, it may be required to
>> freeze their parent. The setup of these clocks' parent should be located
>> on the device tree's side.
>
> We don't need to freeze parent,in a real project.

"a real project" to whom ?

Dmitry remark makes sense to me.

>
>> [...]
>> 
>>> +
>>> +/*
>>> + * gen clk is designed for debug/monitor some internal clock quality. Some of the
>>> + * corresponding clock sources are not described in the clock tree and internal clock
>>> + * for debug, so they are skipped.
>>> + */
>>> +static u32 s4_gen_clk_mux_table[] = { 0, 4, 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", },
>>> +     { .hw = &s4_vid_pll.hw },
>>> +     { .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", },
>>> +};
>>> +
>>> +static struct clk_regmap s4_gen_clk_sel = {
>>> +     .data = &(struct clk_regmap_mux_data){
>>> +             .offset = CLKCTRL_GEN_CLK_CTRL,
>>> +             .mask = 0x1f,
>>> +             .shift = 12,
>>> +             .table = s4_gen_clk_mux_table,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data){
>>> +             .name = "gen_clk_sel",
>>> +             .ops = &clk_regmap_mux_ops,
>>> +             .parent_data = s4_gen_clk_parent_data,
>>> +             .num_parents = ARRAY_SIZE(s4_gen_clk_parent_data),
>> I think, the gen_clk selector should be marked with the
>> CLK_SET_RATE_NO_REPARENT flag. This is because the GEN clock can be
>> connected to an external pad and may be set up directly from the
>> device tree.
>
> This is used by the debug table clock and is not connected externally.
>

Again, Dmitry remark is very interresting.
This debug clock is typacally one you don't really want to automatically reparent

>> 
>>> +     },
>>> +};
>>> +
>>> +static struct clk_regmap s4_gen_clk_div = {
>>> +     .data = &(struct clk_regmap_div_data){
>>> +             .offset = CLKCTRL_GEN_CLK_CTRL,
>>> +             .shift = 0,
>>> +             .width = 11,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data){
>>> +             .name = "gen_clk_div",
>>> +             .ops = &clk_regmap_divider_ops,
>>> +             .parent_hws = (const struct clk_hw *[]) {
>>> +                     &s4_gen_clk_sel.hw
>>> +             },
>>> +             .num_parents = 1,
>>> +             .flags = CLK_SET_RATE_PARENT,
>>> +     },
>>> +};
>>> +
>>> +static struct clk_regmap s4_gen_clk = {
>>> +     .data = &(struct clk_regmap_gate_data){
>>> +             .offset = CLKCTRL_GEN_CLK_CTRL,
>>> +             .bit_idx = 11,
>>> +     },
>>> +     .hw.init = &(struct clk_init_data) {
>>> +             .name = "gen_clk",
>>> +             .ops = &clk_regmap_gate_ops,
>>> +             .parent_hws = (const struct clk_hw *[]) {
>>> +                     &s4_gen_clk_div.hw
>>> +             },
>>> +             .num_parents = 1,
>>> +             .flags = CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,
>>> +     },
>>> +};
>>> +
>> [...]
>> --
>> Thank you,
>> Dmitry