| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20210715184325.GB6897@quicinc.com>
Date: Thu, 15 Jul 2021 11:43:25 -0700
From: Vamsi Krishna Lanka <quic_vamslank@...cinc.com>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
CC: <agross@...nel.org>, <linus.walleij@...aro.org>,
<linux-arm-msm@...r.kernel.org>, <linux-clk@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <manivannan.sadhasivam@...aro.org>
Subject: Re: [PATCH 2/5] clk: qcom: Add SDX65 GCC support
On Fri, Jul 09, 2021 at 10:26:13PM -0500, Bjorn Andersson wrote:
> On Fri 09 Jul 15:03 CDT 2021, quic_vamslank@...cinc.com wrote:
>
> > From: Vamsi krishna Lanka <quic_vamslank@...cinc.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@...cinc.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 ?
>
> > +
> > +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.
Powered by blists - more mailing lists