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:   Fri, 9 Jul 2021 22:26:13 -0500
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     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: <YOkTVXcZpSRUE+Wy@yoga>
References: <20210709200339.17638-1-quic_vamslank@quicinc.com>
 <20210709200339.17638-3-quic_vamslank@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210709200339.17638-3-quic_vamslank@quicinc.com>
Precedence: bulk

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.

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

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

>  };
>  
>  #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.

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

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

> +	[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()"

Thanks,
Bjorn