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@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.
>
> Sure, will do.
>
> >
> > > 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.
>
> 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.
>
