| 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
| ||
|
Date: Mon, 15 Aug 2016 15:19:56 -0700
From: Stephen Boyd <sboyd@...eaurora.org>
To: Abhishek Sahu <absahu@...eaurora.org>
Cc: andy.gross@...aro.org, david.brown@...aro.org, robh+dt@...nel.org,
pawel.moll@....com, mark.rutland@....com,
ijc+devicetree@...lion.org.uk, mturquette@...libre.com,
galak@...eaurora.org, pradeepb@...eaurora.org,
mmcclint@...eaurora.org, varada@...eaurora.org,
sricharan@...eaurora.org, architt@...eaurora.org,
ntelkar@...eaurora.org, linux-arm-msm@...r.kernel.org,
linux-soc@...r.kernel.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 1/5] clk: qcom: ipq4019: Added the clock nodes and
operations for pll
On 06/21, Abhishek Sahu wrote:
> The current ipq4019 clock driver registered the PLL clocks and
> dividers as fixed clock. These fixed clock needs to be removed
> from driver probe function and same need to be registered with
> clock framework. These PLL clocks should be programmed only
> once and the same are being programmed already by the boot
> loader so the set rate operation is not required for these
> clocks. Only the rate can be calculated by clock operations
> in clock driver file so this patch adds the same.
>
> The PLL takes the reference clock from XO and generates the
> intermediate VCO frequency. This VCO frequency will be divided
> down by different PLL internal dividers. Some of the PLL
> internal dividers are fixed while other are programmable.
>
> This patch does the following changes.
> 1. Operation for calculating PLL intermediate VCO frequency by
> reading the reference clock divider and feedback divider from
> register. Since VCO frequency falls outside the limit of
> unsigned long for IPQ4019, so this operation will return the
> VCO frequency in KHz.
>
> 2. Operation for calculating the internal PLL divider clock
> frequency. Clock Divider node should give either fixed
> divider value or divider table(maps the register divider
> value to actual divider value).
>
> 3. Adds and registers clock nodes for VCO(APPS DDR PLL and FE
> PLL) and PLL internal dividers(SDCC, FEPLL 500 Mhz, FEPLL
> 200 Mhz, FEPLL 125 Mhz, FEPLL 125 Mhz with delay,
> programmable WCSS 2G and 5G).
>
> 4. Changes the regmap limit from 0x2DFFF to 0x2FFFF for
> supporting the PLL registers read.
>
> 5. Changes the fixed clock name to have consistency in all
> clock names
>
> Signed-off-by: Abhishek Sahu <absahu@...eaurora.org>
Thanks for fixing this up, just some minor things to fix.
> diff --git a/drivers/clk/qcom/gcc-ipq4019.c b/drivers/clk/qcom/gcc-ipq4019.c
> index 3cd1af0..17ca6d3 100644
> --- a/drivers/clk/qcom/gcc-ipq4019.c
> +++ b/drivers/clk/qcom/gcc-ipq4019.c
> @@ -20,6 +20,11 @@
> #include <linux/clk-provider.h>
> #include <linux/regmap.h>
> #include <linux/reset-controller.h>
> +#include <linux/clk.h>
Is this include used?
> +#include <linux/delay.h>
Is this include used?
> +#include <linux/bitops.h>
> +#include <linux/math64.h>
> +#include <asm/div64.h>
Are both includes needed?
> @@ -40,6 +55,20 @@ enum {
> P_DDRPLLAPSS,
> };
>
> +struct clk_pll_vco {
> + u32 fdbkdiv_shift;
> + u32 fdbkdiv_width;
> + struct clk_regmap_div cdiv;
> +};
> +
> +struct clk_pll_div {
> + u32 fixed_div;
> + const u8 *parent_map;
> + struct clk_regmap_div cdiv;
> + const struct clk_div_table *div_table;
> + const struct freq_tbl *freq_tbl;
> +};
> +
> static struct parent_map gcc_xo_200_500_map[] = {
> { P_XO, 0 },
> { P_FEPLL200, 1 },
> @@ -48,8 +77,8 @@ static struct parent_map gcc_xo_200_500_map[] = {
>
> static const char * const gcc_xo_200_500[] = {
> "xo",
> - "fepll200",
> - "fepll500",
> + "gcc_fepll200_clk",
> + "gcc_fepll500_clk",
> };
>
> static struct parent_map gcc_xo_200_map[] = {
> @@ -59,7 +88,7 @@ static struct parent_map gcc_xo_200_map[] = {
>
> static const char * const gcc_xo_200[] = {
> "xo",
> - "fepll200",
> + "gcc_fepll200_clk",
> };
>
> static struct parent_map gcc_xo_200_spi_map[] = {
> @@ -69,7 +98,7 @@ static struct parent_map gcc_xo_200_spi_map[] = {
>
> static const char * const gcc_xo_200_spi[] = {
> "xo",
> - "fepll200",
> + "gcc_fepll200_clk",
> };
>
> static struct parent_map gcc_xo_sdcc1_500_map[] = {
> @@ -80,8 +109,8 @@ static struct parent_map gcc_xo_sdcc1_500_map[] = {
>
> static const char * const gcc_xo_sdcc1_500[] = {
> "xo",
> - "ddrpll",
> - "fepll500",
> + "gcc_apps_sdcc_clk",
> + "gcc_fepll500_clk",
> };
>
> static struct parent_map gcc_xo_wcss2g_map[] = {
> @@ -91,7 +120,7 @@ static struct parent_map gcc_xo_wcss2g_map[] = {
>
> static const char * const gcc_xo_wcss2g[] = {
> "xo",
> - "fepllwcss2g",
> + "gcc_fepllwcss2g_clk",
> };
>
> static struct parent_map gcc_xo_wcss5g_map[] = {
> @@ -101,7 +130,7 @@ static struct parent_map gcc_xo_wcss5g_map[] = {
>
> static const char * const gcc_xo_wcss5g[] = {
> "xo",
> - "fepllwcss5g",
> + "gcc_fepllwcss5g_clk",
> };
>
> static struct parent_map gcc_xo_125_dly_map[] = {
> @@ -111,7 +140,7 @@ static struct parent_map gcc_xo_125_dly_map[] = {
>
> static const char * const gcc_xo_125_dly[] = {
> "xo",
> - "fepll125dly",
> + "gcc_fepll125dly_clk",
> };
>
> static struct parent_map gcc_xo_ddr_500_200_map[] = {
> @@ -123,8 +152,8 @@ static struct parent_map gcc_xo_ddr_500_200_map[] = {
>
> static const char * const gcc_xo_ddr_500_200[] = {
> "xo",
> - "fepll200",
> - "fepll500",
> + "gcc_fepll200_clk",
> + "gcc_fepll500_clk",
> "ddrpllapss",
> };
Was it necessary to change all these names? The gcc_ prefix
doesn't seem to be adding much besides noise to this patch.
>
> @@ -506,7 +535,7 @@ static const struct freq_tbl ftbl_gcc_sdcc1_apps_clk[] = {
> F(25000000, P_FEPLL500, 1, 1, 20),
> F(50000000, P_FEPLL500, 1, 1, 10),
> F(100000000, P_FEPLL500, 1, 1, 5),
> - F(193000000, P_DDRPLL, 1, 0, 0),
> + F(190000000, P_DDRPLL, 1, 0, 0),
This looks like an unrelated bug fix.
> { }
> };
>
> @@ -658,7 +687,7 @@ static struct clk_branch gcc_crypto_axi_clk = {
> .hw.init = &(struct clk_init_data){
> .name = "gcc_crypto_axi_clk",
> .parent_names = (const char *[]){
> - "fepll125",
> + "gcc_fepll125_clk",
> },
> .num_parents = 1,
> .ops = &clk_branch2_ops,
> @@ -675,7 +704,7 @@ static struct clk_branch gcc_crypto_clk = {
> .hw.init = &(struct clk_init_data){
> .name = "gcc_crypto_clk",
> .parent_names = (const char *[]){
> - "fepll125",
> + "gcc_fepll125_clk",
> },
> .num_parents = 1,
> .ops = &clk_branch2_ops,
> @@ -709,7 +738,7 @@ static struct clk_branch gcc_imem_axi_clk = {
> .hw.init = &(struct clk_init_data){
> .name = "gcc_imem_axi_clk",
> .parent_names = (const char *[]){
> - "fepll200",
> + "gcc_fepll200_clk",
> },
> .num_parents = 1,
> .ops = &clk_branch2_ops,
> @@ -773,7 +802,7 @@ static struct clk_branch gcc_pcie_axi_s_clk = {
> .hw.init = &(struct clk_init_data){
> .name = "gcc_pcie_axi_s_clk",
> .parent_names = (const char *[]){
> - "fepll200",
> + "gcc_fepll200_clk",
> },
> .num_parents = 1,
> .ops = &clk_branch2_ops,
> @@ -955,7 +984,7 @@ static struct clk_branch gcc_usb3_master_clk = {
> .hw.init = &(struct clk_init_data){
> .name = "gcc_usb3_master_clk",
> .parent_names = (const char *[]){
> - "fepll125",
> + "gcc_fepll125_clk",
> },
> .num_parents = 1,
> .ops = &clk_branch2_ops,
> @@ -1155,6 +1184,238 @@ static struct clk_branch gcc_wcss5g_rtc_clk = {
> },
> };
>
> +/*
> + * Calculates the rate from parent rate and divider and round the rate
> + * in Mhz. This function takes the parent rate in Khz and returns the
> + * rate in Hz.
> + */
> +static unsigned long clk_calc_divider_rate(unsigned long parent_rate,
> + unsigned int div)
> +{
> + u64 rate = parent_rate;
> +
> + do_div(rate, div);
> +
> + /*
> + * This rate is in KHz so divide with 1000 and multiply with 1000000
s/KHz/kHz/
> + * to get the rate in Hz.
> + */
> + do_div(rate, 1000);
> + rate *= 1000000;
> +
> + return (unsigned long)rate;
Unnecessary cast, please remove.
> +}
> +
> +/*
> + * Calculates the VCO rate for PLL.
> + * VCO rate value is greater than unsigned long limit. Since this is an
> + * intermediate clock node for actual PLL dividers, so it returns the
> + * rate in Khz. The child nodes will return the value in Hz after its
> + * divide operation.
> + */
> +static unsigned long clk_regmap_vco_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct clk_pll_vco *rcg = to_clk_pll_vco(hw);
> + u32 fdbkdiv, refclkdiv, cdiv;
> + u64 vco;
> +
> + regmap_read(rcg->cdiv.clkr.regmap, rcg->cdiv.reg, &cdiv);
> + refclkdiv = (cdiv >> rcg->cdiv.shift) & (BIT(rcg->cdiv.width) - 1);
> + fdbkdiv = (cdiv >> rcg->fdbkdiv_shift) & (BIT(rcg->fdbkdiv_width) - 1);
> +
> + vco = parent_rate;
> + do_div(vco, refclkdiv);
> + do_div(vco, 1000);
> + vco *= 2;
> + vco *= fdbkdiv;
> +
> + return (unsigned long)vco;
unnecessary cast.
> +}
> +
> +const struct clk_ops clk_regmap_vco_ops = {
static?
> + .recalc_rate = clk_regmap_vco_recalc_rate,
> +};
> +
> +static struct clk_pll_vco gcc_apps_ddrpll_vco = {
> + .fdbkdiv_shift = 16,
> + .fdbkdiv_width = 8,
> + .cdiv.reg = 0x2E020,
lowercase hex please.
> + .cdiv.shift = 24,
> + .cdiv.width = 5,
> + .cdiv.clkr = {
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_apps_ddrpll_vco",
> + .parent_names = (const char *[]){
> + "xo",
> + },
> + .num_parents = 1,
> + .ops = &clk_regmap_vco_ops,
> + },
> + },
> +};
> +
> +static struct clk_pll_vco gcc_fepll_vco = {
> + .fdbkdiv_shift = 16,
> + .fdbkdiv_width = 8,
> + .cdiv.reg = 0x2F020,
> + .cdiv.shift = 24,
> + .cdiv.width = 5,
> + .cdiv.clkr = {
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_fepll_vco",
> + .parent_names = (const char *[]){
> + "xo",
> + },
> + .num_parents = 1,
> + .ops = &clk_regmap_vco_ops,
> + },
> + },
> +};
> +
> +/* Calculates the rate for PLL divider.
Style nit, /* goes on its own line
> + * If the divider value is not fixed then it gets the actual divider value
> + * from divider table. Then, it calculate the clock rate by dividing the
> + * parent rate with actual divider value.
> + */
> +static unsigned long clk_regmap_clk_div_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct clk_pll_div *rcg = to_clk_pll_div(hw);
> + u32 cdiv, pre_div = 1;
> + const struct clk_div_table *clkt;
> +
> + if (rcg->fixed_div) {
> + pre_div = rcg->fixed_div;
> + } else {
> + regmap_read(rcg->cdiv.clkr.regmap, rcg->cdiv.reg, &cdiv);
> + cdiv = (cdiv >> rcg->cdiv.shift) & (BIT(rcg->cdiv.width) - 1);
> +
> + for (clkt = rcg->div_table; clkt->div; clkt++) {
> + if (clkt->val == cdiv)
> + pre_div = clkt->div;
> + }
> + }
> +
> + return clk_calc_divider_rate(parent_rate, pre_div);
> +};
> +
> +const struct clk_ops clk_regmap_clk_div_ops = {
static?
> + .recalc_rate = clk_regmap_clk_div_recalc_rate,
> +};
> +
> +static struct clk_pll_div gcc_apps_sdcc_clk = {
> + .fixed_div = 28,
> + .cdiv.clkr = {
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_apps_sdcc_clk",
> + .parent_names = (const char *[]){
> + "gcc_apps_ddrpll_vco",
> + },
> + .num_parents = 1,
> + .ops = &clk_regmap_clk_div_ops,
> + },
> + },
> +};
> +
> +static struct clk_pll_div gcc_fepll125_clk = {
> + .fixed_div = 32,
> + .cdiv.clkr = {
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_fepll125_clk",
> + .parent_names = (const char *[]){
> + "gcc_fepll_vco",
> + },
> + .num_parents = 1,
> + .ops = &clk_regmap_clk_div_ops,
> + },
> + },
> +};
> +
> +static struct clk_pll_div gcc_fepll125dly_clk = {
> + .fixed_div = 32,
> + .cdiv.clkr = {
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_fepll125dly_clk",
> + .parent_names = (const char *[]){
> + "gcc_fepll_vco",
> + },
> + .num_parents = 1,
> + .ops = &clk_regmap_clk_div_ops,
> + },
> + },
> +};
> +
> +static struct clk_pll_div gcc_fepll200_clk = {
> + .fixed_div = 20,
> + .cdiv.clkr = {
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_fepll200_clk",
> + .parent_names = (const char *[]){
> + "gcc_fepll_vco",
> + },
> + .num_parents = 1,
> + .ops = &clk_regmap_clk_div_ops,
> + },
> + },
> +};
> +
> +static struct clk_pll_div gcc_fepll500_clk = {
> + .fixed_div = 8,
> + .cdiv.clkr = {
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_fepll500_clk",
> + .parent_names = (const char *[]){
> + "gcc_fepll_vco",
> + },
> + .num_parents = 1,
> + .ops = &clk_regmap_clk_div_ops,
> + },
> + },
> +};
> +
> +static const struct clk_div_table fepllwcss_clk_div_table[] = {
> + {0, 15},
> + {1, 16},
> + {2, 18},
> + {3, 20},
Nitpick: Please add some spaces here
{ 0, 15 },
> + {},
> +};
> +
> +static struct clk_pll_div gcc_fepllwcss2g_clk = {
> + .cdiv.reg = 0x2F020,
> + .cdiv.shift = 8,
> + .cdiv.width = 2,
> + .cdiv.clkr = {
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_fepllwcss2g_clk",
> + .parent_names = (const char *[]){
> + "gcc_fepll_vco",
> + },
> + .num_parents = 1,
> + .ops = &clk_regmap_clk_div_ops,
> + },
> + },
> + .div_table = fepllwcss_clk_div_table
> +};
> +
> +static struct clk_pll_div gcc_fepllwcss5g_clk = {
> + .cdiv.reg = 0x2F020,
> + .cdiv.shift = 12,
> + .cdiv.width = 2,
> + .cdiv.clkr = {
> + .hw.init = &(struct clk_init_data){
> + .name = "gcc_fepllwcss5g_clk",
> + .parent_names = (const char *[]){
> + "gcc_fepll_vco",
> + },
> + .num_parents = 1,
> + .ops = &clk_regmap_clk_div_ops,
> + },
> + },
> + .div_table = fepllwcss_clk_div_table
> +};
> +
> static struct clk_regmap *gcc_ipq4019_clocks[] = {
> [AUDIO_CLK_SRC] = &audio_clk_src.clkr,
> [BLSP1_QUP1_I2C_APPS_CLK_SRC] = &blsp1_qup1_i2c_apps_clk_src.clkr,
> @@ -1215,6 +1476,15 @@ static struct clk_regmap *gcc_ipq4019_clocks[] = {
> [GCC_WCSS5G_CLK] = &gcc_wcss5g_clk.clkr,
> [GCC_WCSS5G_REF_CLK] = &gcc_wcss5g_ref_clk.clkr,
> [GCC_WCSS5G_RTC_CLK] = &gcc_wcss5g_rtc_clk.clkr,
> + [GCC_APPS_DDRPLL_VCO] = &gcc_apps_ddrpll_vco.cdiv.clkr,
> + [GCC_FEPLL_VCO] = &gcc_fepll_vco.cdiv.clkr,
> + [GCC_SDCC_PLLDIV_CLK] = &gcc_apps_sdcc_clk.cdiv.clkr,
> + [GCC_FEPLL125_CLK] = &gcc_fepll125_clk.cdiv.clkr,
> + [GCC_FEPLL125DLY_CLK] = &gcc_fepll125dly_clk.cdiv.clkr,
> + [GCC_FEPLL200_CLK] = &gcc_fepll200_clk.cdiv.clkr,
> + [GCC_FEPLL500_CLK] = &gcc_fepll500_clk.cdiv.clkr,
> + [GCC_FEPLL_WCSS2G_CLK] = &gcc_fepllwcss2g_clk.cdiv.clkr,
> + [GCC_FEPLL_WCSS5G_CLK] = &gcc_fepllwcss5g_clk.cdiv.clkr,
> };
>
> static const struct qcom_reset_map gcc_ipq4019_resets[] = {
> @@ -1295,7 +1565,7 @@ static const struct regmap_config gcc_ipq4019_regmap_config = {
> .reg_bits = 32,
> .reg_stride = 4,
> .val_bits = 32,
> - .max_register = 0x2dfff,
> + .max_register = 0x2FFFF,
Lowercase.
> .fast_io = true,
> };
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Powered by blists - more mailing lists