Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 15 Jul 2021 14:50:01 -0500
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Vamsi Krishna Lanka <quic_vamslank@quicinc.com>
Cc:     agross@kernel.org, linus.walleij@linaro.org,
        linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org, manivannan.sadhasivam@linaro.org
Subject: Re: [PATCH 2/5] clk: qcom: Add SDX65 GCC support
Message-ID: <YPCRadKzQRZdtOfO@yoga>
References: <20210709200339.17638-1-quic_vamslank@quicinc.com>
 <20210709200339.17638-3-quic_vamslank@quicinc.com>
 <YOkTVXcZpSRUE+Wy@yoga>
 <20210715184325.GB6897@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210715184325.GB6897@quicinc.com>
Precedence: bulk

On Thu 15 Jul 13:43 CDT 2021, Vamsi Krishna Lanka wrote:

> On Fri, Jul 09, 2021 at 10:26:13PM -0500, Bjorn Andersson wrote:
> > On Fri 09 Jul 15:03 CDT 2021, quic_vamslank@quicinc.com wrote:
> > 
> > > From: Vamsi krishna Lanka <quic_vamslank@quicinc.com>
> > > 
> > > Add Global Clock Controller (GCC) support for SDX65 SoCs from Qualcomm.
> > > 
> > 
> > This doesn't mention the fact that you add a new PLL type. I do however
> > think you should do that in a separate commit, preceding the gcc driver
> > patch.
> 
> Sure, will do.
> 
> > 
> > > Signed-off-by: Vamsi Krishna Lanka <quic_vamslank@quicinc.com>
> > > ---
> > >  drivers/clk/qcom/Kconfig         |    8 +
> > >  drivers/clk/qcom/Makefile        |    1 +
> > >  drivers/clk/qcom/clk-alpha-pll.c |  170 +++
> > >  drivers/clk/qcom/clk-alpha-pll.h |    4 +
> > >  drivers/clk/qcom/clk-rcg.h       |    4 +
> > >  drivers/clk/qcom/gcc-sdx65.c     | 1648 ++++++++++++++++++++++++++++++
> > >  6 files changed, 1835 insertions(+)
> > >  create mode 100644 drivers/clk/qcom/gcc-sdx65.c
> > [..]
> > > diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h
> > > index 99efcc7f8d88..33a7fe992207 100644
> > > --- a/drivers/clk/qcom/clk-rcg.h
> > > +++ b/drivers/clk/qcom/clk-rcg.h
> > > @@ -149,6 +149,10 @@ struct clk_rcg2 {
> > >  	const struct freq_tbl	*freq_tbl;
> > >  	struct clk_regmap	clkr;
> > >  	u8			cfg_off;
> > > +	u8                      flags;
> > > +#define FORCE_ENABLE_RCG        BIT(0)
> > 
> > Unused.
> 
> Will remove.
> 
> > 
> > > +#define HW_CLK_CTRL_MODE        BIT(1)
> > 
> > We don't implement HW_CLK_CTRL_MODE upstream yet, so you shouldn't
> > specify it for your clocks - or just add the definefor it.
> > 
> > > +#define DFS_SUPPORT             BIT(2)
> > 
> > Unused.
> 
> Will remove.
> 
> > 
> > >  };
> > >  
> > >  #define to_clk_rcg2(_hw) container_of(to_clk_regmap(_hw), struct clk_rcg2, clkr)
> > > diff --git a/drivers/clk/qcom/gcc-sdx65.c b/drivers/clk/qcom/gcc-sdx65.c
> > [..]
> > > +static struct clk_alpha_pll_postdiv gpll0_out_even = {
> > > +	.offset = 0x0,
> > > +	.post_div_shift = 10,
> > > +	.post_div_table = post_div_table_gpll0_out_even,
> > > +	.num_post_div = ARRAY_SIZE(post_div_table_gpll0_out_even),
> > > +	.width = 4,
> > > +	.regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_LUCID_EVO],
> > > +	.clkr.hw.init = &(struct clk_init_data){
> > > +		.name = "gpll0_out_even",
> > > +		.parent_data = &(const struct clk_parent_data){
> > 
> > A parent_data with a single .hw is better specified using parent_hws.
> 
> Sure, will use parent_hws.
> 
> > 
> > > +			.hw = &gpll0.clkr.hw,
> > > +		},
> > > +		.num_parents = 1,
> > > +		.ops = &clk_alpha_pll_postdiv_lucid_evo_ops,
> > > +	},
> > > +};
> > > +
> > > +static const struct parent_map gcc_parent_map_0[] = {
> > > +	{ P_BI_TCXO, 0 },
> > > +	{ P_GPLL0_OUT_MAIN, 1 },
> > > +	{ P_GPLL0_OUT_EVEN, 6 },
> > > +	{ P_CORE_BI_PLL_TEST_SE, 7 },
> > > +};
> > > +
> > > +static const struct clk_parent_data gcc_parent_data_0[] = {
> > > +	{ .fw_name = "bi_tcxo" },
> > > +	{ .hw = &gpll0.clkr.hw },
> > > +	{ .hw = &gpll0_out_even.clkr.hw },
> > > +	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> > > +};
> > 
> > Nice to see that you use clk_parent_data, fw_name and hw right from the
> > start.
> > 
> > We typically remove core_bi_pll_test_se from the various parent lists,
> > as this is not something that's expected to be ever used. Please do the
> > same.
> 
> Sorry, I couldn't completely follow. Should I remove core_bi_pll_test_se from
> both fw_name and name ? 
> 

Yes, please drop "core_bi_pll_test_se" (and hence P_CORE_BI_PLL_TEST_SE)
throughout the driver.

Thanks,
Bjorn

> > 
> > > +
> > > +static const struct clk_parent_data gcc_parent_data_0_ao[] = {
> > > +	{ .fw_name = "bi_tcxo_ao" },
> > > +	{ .hw = &gpll0.clkr.hw },
> > > +	{ .hw = &gpll0_out_even.clkr.hw },
> > > +	{ .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" },
> > > +};
> > [..]
> > > +static struct clk_regmap *gcc_sdx65_clocks[] = {
> > > +	[GCC_AHB_PCIE_LINK_CLK] = &gcc_ahb_pcie_link_clk.clkr,
> > > +	[GCC_BLSP1_AHB_CLK] = &gcc_blsp1_ahb_clk.clkr,
> > > +	[GCC_BLSP1_QUP1_I2C_APPS_CLK] = &gcc_blsp1_qup1_i2c_apps_clk.clkr,
> > > +	[GCC_BLSP1_QUP1_I2C_APPS_CLK_SRC] =
> > > +		&gcc_blsp1_qup1_i2c_apps_clk_src.clkr,
> > 
> > Skip the line wrap.
> 
> Done.
> 
> > 
> > > +	[GCC_BLSP1_QUP1_SPI_APPS_CLK] = &gcc_blsp1_qup1_spi_apps_clk.clkr,
> > > +	[GCC_BLSP1_QUP1_SPI_APPS_CLK_SRC] =
> > > +		&gcc_blsp1_qup1_spi_apps_clk_src.clkr,
> > > +	[GCC_BLSP1_QUP2_I2C_APPS_CLK] = &gcc_blsp1_qup2_i2c_apps_clk.clkr,
> > > +	[GCC_BLSP1_QUP2_I2C_APPS_CLK_SRC] =
> > > +		&gcc_blsp1_qup2_i2c_apps_clk_src.clkr,
> > > +	[GCC_BLSP1_QUP2_SPI_APPS_CLK] = &gcc_blsp1_qup2_spi_apps_clk.clkr,
> > > +	[GCC_BLSP1_QUP2_SPI_APPS_CLK_SRC] =
> > [..]
> > > +static int gcc_sdx65_probe(struct platform_device *pdev)
> > > +{
> > > +	struct regmap *regmap;
> > > +	int ret;
> > > +
> > > +	regmap = qcom_cc_map(pdev, &gcc_sdx65_desc);
> > > +	if (IS_ERR(regmap))
> > > +		return PTR_ERR(regmap);
> > > +	/*
> > > +	 * Keep the clocks always-ON as they are critical to the functioning
> > > +	 * of the system:
> > > +	 * GCC_SYS_NOC_CPUSS_AHB_CLK, GCC_CPUSS_AHB_CLK, GCC_CPUSS_GNOC_CLK
> > > +	 */
> > > +	regmap_update_bits(regmap, 0x6d008, BIT(0), BIT(0));
> > > +	regmap_update_bits(regmap, 0x6d008, BIT(21), BIT(21));
> > > +	regmap_update_bits(regmap, 0x6d008, BIT(22), BIT(22));
> > > +
> > > +	ret = qcom_cc_really_probe(pdev, &gcc_sdx65_desc, regmap);
> > 
> > Just "return qcom_cc_really_probe()"
> 
> Done.
>