On Fri 09 Jul 15:03 CDT 2021, quic_vamslank@xxxxxxxxxxx wrote:
> From: Vamsi krishna Lanka <quic_vamslank@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> 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
