Message-ID: <4bcf4bd9-e638-22a2-044d-47fd5fd5d104@quicinc.com>
Date:   Tue, 7 Feb 2023 09:59:57 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.0
Subject: Re: [PATCH V3 5/9] clk: qcom: add Global Clock controller (GCC)
 driver for IPQ5332 SoC
Content-Language: en-US
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        <krzysztof.kozlowski@linaro.org>, <agross@kernel.org>,
        <andersson@kernel.org>, <konrad.dybcio@linaro.org>,
        <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <mturquette@baylibre.com>, <sboyd@kernel.org>,
        <linus.walleij@linaro.org>, <catalin.marinas@arm.com>,
        <will@kernel.org>, <shawnguo@kernel.org>, <arnd@arndb.de>,
        <marcel.ziswiler@toradex.com>, <nfraprado@collabora.com>,
        <robimarko@gmail.com>, <quic_gurus@quicinc.com>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-clk@vger.kernel.org>,
        <linux-gpio@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>
CC:     <quic_varada@quicinc.com>, <quic_srichara@quicinc.com>
References: <20230206071217.29313-1-quic_kathirav@quicinc.com>
 <20230206071217.29313-6-quic_kathirav@quicinc.com>
 <019bf6ae-cdc4-8fa7-4d76-c39403d93b34@linaro.org>
From:   Kathiravan T <quic_kathirav@quicinc.com>
In-Reply-To: <019bf6ae-cdc4-8fa7-4d76-c39403d93b34@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Thanks Dmirty for the review!


On 2/6/2023 3:26 PM, Dmitry Baryshkov wrote:
> On 06/02/2023 09:12, Kathiravan T wrote:
> > Add support for the global clock controller found on IPQ5332 SoC.
> >
> > Signed-off-by: Kathiravan T <quic_kathirav@quicinc.com>
> > ---
> > Changes in V3:
> >     - As I mentined the bindings, changes need to be done in V2 got
> >       missed out and same has been done in V3, to call out
> >       specifically dropped the CLK_IS_CRITICAL and dropped the
> >       gcc_apss_ahb_clk, its source clock and gcc_apss_axi_clk
> >     - Used gcc_parent_data_xo wherever applicable and dropped the
> >       duplicate entries
> >     - dropped the unused parent_map_10 and parent_data_10
> >     - Used qcom_cc_probe instead of qcom_cc_really_probe
> > Changes in V2:
> >     - Added the 'dependes on' for Kconfig symbol
> >     - Dropped the CLK_IS_CRITICAL flag throughout the file
> >     - Dropped the gcc_apss_ahb_clk and gcc_apss_axi_clk as these are
> >       managed by bootloaders
>
>
> [skipped]
>
> > +static const struct freq_tbl ftbl_gcc_pcie_aux_clk_src[] = {
> > +    F(2000000, P_XO, 12, 0, 0),
> > +    { }
> > +};
> > +
> > +static struct clk_rcg2 gcc_pcie_aux_clk_src = {
> > +    .cmd_rcgr = 0x28004,
> > +    .mnd_width = 16,
> > +    .hid_width = 5,
> > +    .parent_map = gcc_parent_map_6,
> > +    .freq_tbl = ftbl_gcc_pcie_aux_clk_src,
> > +    .clkr.hw.init = &(const struct clk_init_data){
> > +        .name = "gcc_pcie_aux_clk_src",
> > +        .parent_data = gcc_parent_data_6,
> > +        .num_parents = ARRAY_SIZE(gcc_parent_data_6),
> > +        .ops = &clk_rcg2_ops,
> > +    },
> > +};
> > +
> > +static struct clk_regmap_mux pcie3x2_pipe_clk_src = {
> > +    .reg = 0x28064,
> > +    .shift = 8,
> > +    .width = 2,
> > +    .parent_map = gcc_parent_map_14,
> > +    .clkr = {
> > +        .hw.init = &(struct clk_init_data){
> > +            .name = "pcie3x2_phy_pipe_clk_src",
> > +            .parent_data = gcc_parent_data_14,
> > +            .num_parents = ARRAY_SIZE(gcc_parent_data_14),
> > +            .ops = &clk_regmap_mux_closest_ops,
>
> Should we use clk_regmap_phy_mux_ops here instead?


Sure, I will check this one. Also looking at the commit, 74e4190cdebe 
("clk: qcom: regmap: add PHY clock source implementation"), looks like I 
can use the clk_regmap_phy_mux struct instead of clk_regmap_mux. I will 
check into this and update accordingly.


>
> > +            .flags = CLK_SET_RATE_PARENT,
> > +        },
> > +    },
> > +};
> > +
> > +static struct clk_regmap_mux pcie3x1_0_pipe_clk_src = {
> > +    .reg = 0x29064,
> > +    .shift = 8,
> > +    .width = 2,
> > +    .parent_map = gcc_parent_map_15,
> > +    .clkr = {
> > +        .hw.init = &(struct clk_init_data){
> > +            .name = "pcie3x1_0_phy_pipe_clk_src",
> > +            .parent_data = gcc_parent_data_15,
> > +            .num_parents = ARRAY_SIZE(gcc_parent_data_15),
> > +            .ops = &clk_regmap_mux_closest_ops,
>
> And clk_regmap_phy_mux_ops here too?


Ack.


>
> > +            .flags = CLK_SET_RATE_PARENT,
> > +        },
> > +    },
> > +};
> > +
> > +static struct clk_regmap_mux pcie3x1_1_pipe_clk_src = {
> > +    .reg = 0x2A064,
> > +    .shift = 8,
> > +    .width = 2,
> > +    .parent_map = gcc_parent_map_16,
> > +    .clkr = {
> > +        .hw.init = &(struct clk_init_data){
> > +            .name = "pcie3x1_1_phy_pipe_clk_src",
> > +            .parent_data = gcc_parent_data_16,
> > +            .num_parents = ARRAY_SIZE(gcc_parent_data_16),
> > +            .ops = &clk_regmap_mux_closest_ops,
>
> And here?


Ack.


>
> > +            .flags = CLK_SET_RATE_PARENT,
> > +        },
> > +    },
> > +};
> > +
>
> [skipped]
>
>
> > +
> > +static struct clk_branch gcc_pcie3x1_0_pipe_clk = {
> > +    .halt_reg = 0x29068,
> > +    .halt_check = BRANCH_HALT_DELAY,
> > +    .clkr = {
> > +        .enable_reg = 0x29068,
> > +        .enable_mask = BIT(0),
> > +        .hw.init = &(const struct clk_init_data){
> > +            .name = "gcc_pcie3x1_0_pipe_clk",
> > +            .parent_names = (const char *[]){
> > +                "pcie3x1_0_pipe_clk_src"
> > +            },
>
> Nooo. No parent_names please. You have several of them in the driver


I missed this. I will fix in next spin.


>
> > +            .num_parents = 1,
> > +            .flags = CLK_SET_RATE_PARENT,
> > +            .ops = &clk_branch2_ops,
> > +        },
> > +    },
> > +};
> > +
>