Message ID | 1572524473-19344-1-git-send-email-tdas@codeaurora.org |
---|---|
Headers | show |
Series | Add GPU & Video Clock controller driver for SC7180 | expand |
Quoting Taniya Das (2019-10-31 05:21:10) > diff --git a/drivers/clk/qcom/gpucc-sc7180.c b/drivers/clk/qcom/gpucc-sc7180.c > new file mode 100644 > index 0000000..0d893e6 > --- /dev/null > +++ b/drivers/clk/qcom/gpucc-sc7180.c > @@ -0,0 +1,274 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2019, The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/err.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> Are these of includes used? > +#include <linux/regmap.h> > + > +#include <dt-bindings/clock/qcom,gpucc-sc7180.h> > + > +#include "clk-alpha-pll.h" > +#include "clk-branch.h" > +#include "clk-rcg.h" > +#include "clk-regmap.h" > +#include "common.h" > +#include "gdsc.h" > + > +#define CX_GMU_CBCR_SLEEP_MASK 0xF > +#define CX_GMU_CBCR_SLEEP_SHIFT 4 > +#define CX_GMU_CBCR_WAKE_MASK 0xF > +#define CX_GMU_CBCR_WAKE_SHIFT 8 > +#define CLK_DIS_WAIT_SHIFT 12 > +#define CLK_DIS_WAIT_MASK (0xf << CLK_DIS_WAIT_SHIFT) > + > +enum { > + P_BI_TCXO, > + P_CORE_BI_PLL_TEST_SE, > + P_GPLL0_OUT_MAIN, > + P_GPLL0_OUT_MAIN_DIV, > + P_GPU_CC_PLL1_OUT_EVEN, > + P_GPU_CC_PLL1_OUT_MAIN, > + P_GPU_CC_PLL1_OUT_ODD, > +}; > + > +static struct pll_vco fabia_vco[] = { const? > + { 249600000, 2000000000, 0 }, > +}; > + > +static struct clk_alpha_pll gpu_cc_pll1 = { > + .offset = 0x100, > + .vco_table = fabia_vco, > + .num_vco = ARRAY_SIZE(fabia_vco), > + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA], > + .clkr = { > + .hw.init = &(struct clk_init_data){ > + .name = "gpu_cc_pll1", > + .parent_data = &(const struct clk_parent_data){ > + .fw_name = "bi_tcxo", > + .name = "bi_tcxo", Do we need both? This is new so it should just work with .fw_name right? > + }, > + .num_parents = 1, > + .ops = &clk_alpha_pll_fabia_ops, > + }, > + }, > +}; > + > +static const struct parent_map gpu_cc_parent_map_0[] = { > + { P_BI_TCXO, 0 }, > + { P_GPU_CC_PLL1_OUT_MAIN, 3 }, > + { P_GPLL0_OUT_MAIN, 5 }, > + { P_GPLL0_OUT_MAIN_DIV, 6 }, > + { P_CORE_BI_PLL_TEST_SE, 7 }, > +}; > + > +static const struct clk_parent_data gpu_cc_parent_data_0[] = { > + { .fw_name = "bi_tcxo", .name = "bi_tcxo" }, > + { .hw = &gpu_cc_pll1.clkr.hw }, > + { .fw_name = "gcc_gpu_gpll0_clk_src", .name = "gcc_gpu_gpll0_clk_src" }, > + { .fw_name = "gcc_gpu_gpll0_div_clk_src", > + .name = "gcc_gpu_gpll0_div_clk_src" }, > + { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" }, > +}; Same for these. > + > +static const struct freq_tbl ftbl_gpu_cc_gmu_clk_src[] = { > + F(19200000, P_BI_TCXO, 1, 0, 0), > + F(200000000, P_GPLL0_OUT_MAIN_DIV, 1.5, 0, 0), > + { } > +}; [..] > + > + > +static int gpu_cc_sc7180_probe(struct platform_device *pdev) > +{ > + struct regmap *regmap; > + struct alpha_pll_config gpu_cc_pll_config = {}; > + unsigned int value, mask; > + > + regmap = qcom_cc_map(pdev, &gpu_cc_sc7180_desc); > + if (IS_ERR(regmap)) > + return PTR_ERR(regmap); > + > + /* 360MHz Configuration */ > + gpu_cc_pll_config.l = 0x12; > + gpu_cc_pll_config.alpha = 0xC000; > + gpu_cc_pll_config.config_ctl_val = 0x20485699; > + gpu_cc_pll_config.config_ctl_hi_val = 0x00002067; > + gpu_cc_pll_config.user_ctl_val = 0x00000001; > + gpu_cc_pll_config.user_ctl_hi_val = 0x00004805; > + gpu_cc_pll_config.test_ctl_hi_val = 0x40000000; Is there a reason this is built on the stack? Save space or something? > + > + clk_fabia_pll_configure(&gpu_cc_pll1, regmap, &gpu_cc_pll_config); > + > + /* Recommended WAKEUP/SLEEP settings for the gpu_cc_cx_gmu_clk */ > + mask = CX_GMU_CBCR_WAKE_MASK << CX_GMU_CBCR_WAKE_SHIFT; > + mask |= CX_GMU_CBCR_SLEEP_MASK << CX_GMU_CBCR_SLEEP_SHIFT; > + value = 0xF << CX_GMU_CBCR_WAKE_SHIFT | 0xF << CX_GMU_CBCR_SLEEP_SHIFT; mask and value can just be big constants? I'm not sure anyone cares to tweak this later, but I guess it's fine this way too. > + regmap_update_bits(regmap, 0x1098, mask, value); > + > + /* gpu_cc_ahb_clk */ What are we doing to gpu_cc_ahb_clk?? > + regmap_update_bits(regmap, 0x1078, 0x1, 0x1); > + > + /* Configure clk_dis_wait for gpu_cx_gdsc */ > + regmap_update_bits(regmap, 0x106c, CLK_DIS_WAIT_MASK, > + 8 << CLK_DIS_WAIT_SHIFT); > + > + return qcom_cc_really_probe(pdev, &gpu_cc_sc7180_desc, regmap); > +} > + > +static struct platform_driver gpu_cc_sc7180_driver = { > + .probe = gpu_cc_sc7180_probe, > + .driver = { > + .name = "sc7180-gpucc", > + .of_match_table = gpu_cc_sc7180_match_table, > + }, > +}; > + > +static int __init gpu_cc_sc7180_init(void) > +{ > + return platform_driver_register(&gpu_cc_sc7180_driver); > +} > +core_initcall(gpu_cc_sc7180_init); Does it need to be core initcal? Maybe module_platform_driver() works just as well? > + > +static void __exit gpu_cc_sc7180_exit(void) > +{ > + platform_driver_unregister(&gpu_cc_sc7180_driver); > +} > +module_exit(gpu_cc_sc7180_exit); > + > +MODULE_DESCRIPTION("QTI GPU_CC SC7180 Driver"); > +MODULE_LICENSE("GPL v2");
Quoting Taniya Das (2019-10-31 05:21:07) > diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c > index 055318f..8cb77ca 100644 > --- a/drivers/clk/qcom/clk-alpha-pll.c > +++ b/drivers/clk/qcom/clk-alpha-pll.c > @@ -1141,15 +1160,11 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate, > unsigned long prate) > { > struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > - u32 val, l, alpha_width = pll_alpha_width(pll); > + u32 l, alpha_width = pll_alpha_width(pll); > u64 a; > unsigned long rrate; > int ret = 0; > > - ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val); > - if (ret) > - return ret; > - > rrate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width); > > /* How is this diff related? Looks like it should be split off into another patch to remove a useless register read. > @@ -1167,7 +1182,66 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate, > return __clk_alpha_pll_update_latch(pll); > } > > +static int alpha_pll_fabia_prepare(struct clk_hw *hw) > +{ Why are we doing this in prepare vs. doing it at PLL configuration time? Does it need to be recalibrated each time the PLL is enabled? > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > + const struct pll_vco *vco; > + struct clk_hw *parent_hw; > + unsigned long cal_freq, rrate; > + u32 cal_l, regval, alpha_width = pll_alpha_width(pll); > + u64 a; > + int ret; > + > + /* Check if calibration needs to be done i.e. PLL is in reset */ > + ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), ®val); Please use 'val' instead of 'regval' as regval almost never appears in this file already. > + if (ret) > + return ret; > + > + /* Return early if calibration is not needed. */ > + if (regval & PLL_RESET_N) > + return 0; > + > + vco = alpha_pll_find_vco(pll, clk_hw_get_rate(hw)); > + if (!vco) { > + pr_err("alpha pll: not in a valid vco range\n"); > + return -EINVAL; > + } > + > + cal_freq = DIV_ROUND_CLOSEST((pll->vco_table[0].min_freq + > + pll->vco_table[0].max_freq) * 54, 100); Do we need to cast the first argument to a u64 to avoid overflow? > + > + parent_hw = clk_hw_get_parent(hw); > + if (!parent_hw) > + return -EINVAL; > + > + rrate = alpha_pll_round_rate(cal_freq, clk_hw_get_rate(parent_hw), > + &cal_l, &a, alpha_width); > + /* > + * Due to a limited number of bits for fractional rate programming, the > + * rounded up rate could be marginally higher than the requested rate. > + */ > + if (rrate > (cal_freq + FABIA_PLL_RATE_MARGIN) || rrate < cal_freq) { > + pr_err("Call set rate on the PLL with rounded rates!\n"); This message is weird. Drivers shouldn't need to call set rate with rounded rates. What is going on? > + return -EINVAL; > + } > + > + /* Setup PLL for calibration frequency */ > + regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), cal_l); > + > + /* Bringup the pll at calibration frequency */ capitalize PLL. > + ret = clk_alpha_pll_enable(hw); > + if (ret) { > + pr_err("alpha pll calibration failed\n"); > + return ret; > + } > + > + clk_alpha_pll_disable(hw); > + > + return 0; > +} > +
Quoting Taniya Das (2019-10-31 05:21:13) > diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c > new file mode 100644 > index 0000000..bef034b > --- /dev/null > +++ b/drivers/clk/qcom/videocc-sc7180.c > @@ -0,0 +1,263 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2019, The Linux Foundation. All rights reserved. > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/err.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> Are these of includes used? > +#include <linux/regmap.h> > + > +#include <dt-bindings/clock/qcom,videocc-sc7180.h> > + > +#include "clk-alpha-pll.h" > +#include "clk-branch.h" > +#include "clk-rcg.h" > +#include "clk-regmap.h" > +#include "common.h" > +#include "gdsc.h" > + > +enum { > + P_BI_TCXO, > + P_CHIP_SLEEP_CLK, > + P_CORE_BI_PLL_TEST_SE, > + P_VIDEO_PLL0_OUT_EVEN, > + P_VIDEO_PLL0_OUT_MAIN, > + P_VIDEO_PLL0_OUT_ODD, > +}; > + > +static struct pll_vco fabia_vco[] = { const? > + { 249600000, 2000000000, 0 }, > +}; > + [...] > + > +static int video_cc_sc7180_probe(struct platform_device *pdev) > +{ > + struct regmap *regmap; > + struct alpha_pll_config video_pll0_config = {}; > + > + regmap = qcom_cc_map(pdev, &video_cc_sc7180_desc); > + if (IS_ERR(regmap)) > + return PTR_ERR(regmap); > + > + video_pll0_config.l = 0x1F; lowercase hex please. > + video_pll0_config.alpha = 0x4000; > + video_pll0_config.user_ctl_val = 0x00000001; > + video_pll0_config.user_ctl_hi_val = 0x00004805; Same question, why on stack? > + > + clk_fabia_pll_configure(&video_pll0, regmap, &video_pll0_config); > + > + /* video_cc_xo_clk */ What are we doing? Enabling it? > + regmap_update_bits(regmap, 0x984, 0x1, 0x1); > + > + return qcom_cc_really_probe(pdev, &video_cc_sc7180_desc, regmap); > +} > + > +static struct platform_driver video_cc_sc7180_driver = { > + .probe = video_cc_sc7180_probe, > + .driver = { > + .name = "sc7180-videocc", > + .of_match_table = video_cc_sc7180_match_table, > + }, > +}; > + > +static int __init video_cc_sc7180_init(void) > +{ > + return platform_driver_register(&video_cc_sc7180_driver); > +} > +core_initcall(video_cc_sc7180_init); > + > +static void __exit video_cc_sc7180_exit(void) > +{ > + platform_driver_unregister(&video_cc_sc7180_driver); > +} > +module_exit(video_cc_sc7180_exit); Same question, module platform driver perhaps?
Hi, On Thu, Oct 31, 2019 at 5:21 AM Taniya Das <tdas@codeaurora.org> wrote: > > @@ -1141,15 +1160,11 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate, > unsigned long prate) > { > struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > - u32 val, l, alpha_width = pll_alpha_width(pll); > + u32 l, alpha_width = pll_alpha_width(pll); > u64 a; > unsigned long rrate; > int ret = 0; > > - ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val); > - if (ret) > - return ret; My compiler happened to notice that with the change to this function: drivers/clk/qcom/clk-alpha-pll.c:1166:6: error: unused variable 'ret' [-Werror,-Wunused-variable] int ret = 0; -Doug
Hi Stephen, Thanks for your review. On 11/6/2019 6:06 AM, Stephen Boyd wrote: > Quoting Taniya Das (2019-10-31 05:21:07) >> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c >> index 055318f..8cb77ca 100644 >> --- a/drivers/clk/qcom/clk-alpha-pll.c >> +++ b/drivers/clk/qcom/clk-alpha-pll.c >> @@ -1141,15 +1160,11 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate, >> unsigned long prate) >> { >> struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); >> - u32 val, l, alpha_width = pll_alpha_width(pll); >> + u32 l, alpha_width = pll_alpha_width(pll); >> u64 a; >> unsigned long rrate; >> int ret = 0; >> >> - ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val); >> - if (ret) >> - return ret; >> - >> rrate = alpha_pll_round_rate(rate, prate, &l, &a, alpha_width); >> >> /* > > How is this diff related? Looks like it should be split off into another > patch to remove a useless register read. > I will split this in different patch. >> @@ -1167,7 +1182,66 @@ static int alpha_pll_fabia_set_rate(struct clk_hw *hw, unsigned long rate, >> return __clk_alpha_pll_update_latch(pll); >> } >> >> +static int alpha_pll_fabia_prepare(struct clk_hw *hw) >> +{ > > Why are we doing this in prepare vs. doing it at PLL configuration time? > Does it need to be recalibrated each time the PLL is enabled? > In the case if PLL looses the configuration then we would encounter PLL locking issues. Thus want to go ahead with prepare. In the case it is calibrated it would return. >> + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); >> + const struct pll_vco *vco; >> + struct clk_hw *parent_hw; >> + unsigned long cal_freq, rrate; >> + u32 cal_l, regval, alpha_width = pll_alpha_width(pll); >> + u64 a; >> + int ret; >> + >> + /* Check if calibration needs to be done i.e. PLL is in reset */ >> + ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), ®val); > > Please use 'val' instead of 'regval' as regval almost never appears in > this file already. > Sure, will use 'val'. >> + if (ret) >> + return ret; >> + >> + /* Return early if calibration is not needed. */ >> + if (regval & PLL_RESET_N) >> + return 0; >> + >> + vco = alpha_pll_find_vco(pll, clk_hw_get_rate(hw)); >> + if (!vco) { >> + pr_err("alpha pll: not in a valid vco range\n"); >> + return -EINVAL; >> + } >> + >> + cal_freq = DIV_ROUND_CLOSEST((pll->vco_table[0].min_freq + >> + pll->vco_table[0].max_freq) * 54, 100); > > Do we need to cast the first argument to a u64 to avoid overflow? > No we do not need. >> + >> + parent_hw = clk_hw_get_parent(hw); >> + if (!parent_hw) >> + return -EINVAL; >> + >> + rrate = alpha_pll_round_rate(cal_freq, clk_hw_get_rate(parent_hw), >> + &cal_l, &a, alpha_width); >> + /* >> + * Due to a limited number of bits for fractional rate programming, the >> + * rounded up rate could be marginally higher than the requested rate. >> + */ >> + if (rrate > (cal_freq + FABIA_PLL_RATE_MARGIN) || rrate < cal_freq) { >> + pr_err("Call set rate on the PLL with rounded rates!\n"); > > This message is weird. Drivers shouldn't need to call set rate with > rounded rates. What is going on? > :), my bad, copy paste from another function. I will remove this print. >> + return -EINVAL; >> + } >> + >> + /* Setup PLL for calibration frequency */ >> + regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), cal_l); >> + >> + /* Bringup the pll at calibration frequency */ > > capitalize PLL. > Will take care of it. >> + ret = clk_alpha_pll_enable(hw); >> + if (ret) { >> + pr_err("alpha pll calibration failed\n"); >> + return ret; >> + } >> + >> + clk_alpha_pll_disable(hw); >> + >> + return 0; >> +} >> +
Hi Doug, On 11/7/2019 9:54 AM, Doug Anderson wrote: >> - ret = regmap_read(pll->clkr.regmap, PLL_MODE(pll), &val); >> - if (ret) >> - return ret; > > My compiler happened to notice that with the change to this function: > > drivers/clk/qcom/clk-alpha-pll.c:1166:6: error: unused variable 'ret' > [-Werror,-Wunused-variable] > int ret = 0; > > -Doug > Thanks for the review, will remove the unused variable.
Hi Stephen, Thanks for the review. On 11/6/2019 6:00 AM, Stephen Boyd wrote: > Quoting Taniya Das (2019-10-31 05:21:10) >> diff --git a/drivers/clk/qcom/gpucc-sc7180.c b/drivers/clk/qcom/gpucc-sc7180.c >> new file mode 100644 >> index 0000000..0d893e6 >> --- /dev/null >> +++ b/drivers/clk/qcom/gpucc-sc7180.c >> @@ -0,0 +1,274 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2019, The Linux Foundation. All rights reserved. >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/err.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> > > Are these of includes used? > yes, would clean up these headers. >> +#include <linux/regmap.h> >> + >> +#include <dt-bindings/clock/qcom,gpucc-sc7180.h> >> + >> +#include "clk-alpha-pll.h" >> +#include "clk-branch.h" >> +#include "clk-rcg.h" >> +#include "clk-regmap.h" >> +#include "common.h" >> +#include "gdsc.h" >> + >> +#define CX_GMU_CBCR_SLEEP_MASK 0xF >> +#define CX_GMU_CBCR_SLEEP_SHIFT 4 >> +#define CX_GMU_CBCR_WAKE_MASK 0xF >> +#define CX_GMU_CBCR_WAKE_SHIFT 8 >> +#define CLK_DIS_WAIT_SHIFT 12 >> +#define CLK_DIS_WAIT_MASK (0xf << CLK_DIS_WAIT_SHIFT) >> + >> +enum { >> + P_BI_TCXO, >> + P_CORE_BI_PLL_TEST_SE, >> + P_GPLL0_OUT_MAIN, >> + P_GPLL0_OUT_MAIN_DIV, >> + P_GPU_CC_PLL1_OUT_EVEN, >> + P_GPU_CC_PLL1_OUT_MAIN, >> + P_GPU_CC_PLL1_OUT_ODD, >> +}; >> + >> +static struct pll_vco fabia_vco[] = { > > const? > Will take care. >> + { 249600000, 2000000000, 0 }, >> +}; >> + >> +static struct clk_alpha_pll gpu_cc_pll1 = { >> + .offset = 0x100, >> + .vco_table = fabia_vco, >> + .num_vco = ARRAY_SIZE(fabia_vco), >> + .regs = clk_alpha_pll_regs[CLK_ALPHA_PLL_TYPE_FABIA], >> + .clkr = { >> + .hw.init = &(struct clk_init_data){ >> + .name = "gpu_cc_pll1", >> + .parent_data = &(const struct clk_parent_data){ >> + .fw_name = "bi_tcxo", >> + .name = "bi_tcxo", > > Do we need both? This is new so it should just work with .fw_name right? > yes, will clean this up. >> + }, >> + .num_parents = 1, >> + .ops = &clk_alpha_pll_fabia_ops, >> + }, >> + }, >> +}; >> + >> +static const struct parent_map gpu_cc_parent_map_0[] = { >> + { P_BI_TCXO, 0 }, >> + { P_GPU_CC_PLL1_OUT_MAIN, 3 }, >> + { P_GPLL0_OUT_MAIN, 5 }, >> + { P_GPLL0_OUT_MAIN_DIV, 6 }, >> + { P_CORE_BI_PLL_TEST_SE, 7 }, >> +}; >> + >> +static const struct clk_parent_data gpu_cc_parent_data_0[] = { >> + { .fw_name = "bi_tcxo", .name = "bi_tcxo" }, >> + { .hw = &gpu_cc_pll1.clkr.hw }, >> + { .fw_name = "gcc_gpu_gpll0_clk_src", .name = "gcc_gpu_gpll0_clk_src" }, >> + { .fw_name = "gcc_gpu_gpll0_div_clk_src", >> + .name = "gcc_gpu_gpll0_div_clk_src" }, >> + { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" }, >> +}; > > Same for these. > >> + >> +static const struct freq_tbl ftbl_gpu_cc_gmu_clk_src[] = { >> + F(19200000, P_BI_TCXO, 1, 0, 0), >> + F(200000000, P_GPLL0_OUT_MAIN_DIV, 1.5, 0, 0), >> + { } >> +}; > [..] >> + >> + >> +static int gpu_cc_sc7180_probe(struct platform_device *pdev) >> +{ >> + struct regmap *regmap; >> + struct alpha_pll_config gpu_cc_pll_config = {}; >> + unsigned int value, mask; >> + >> + regmap = qcom_cc_map(pdev, &gpu_cc_sc7180_desc); >> + if (IS_ERR(regmap)) >> + return PTR_ERR(regmap); >> + >> + /* 360MHz Configuration */ >> + gpu_cc_pll_config.l = 0x12; >> + gpu_cc_pll_config.alpha = 0xC000; >> + gpu_cc_pll_config.config_ctl_val = 0x20485699; >> + gpu_cc_pll_config.config_ctl_hi_val = 0x00002067; >> + gpu_cc_pll_config.user_ctl_val = 0x00000001; >> + gpu_cc_pll_config.user_ctl_hi_val = 0x00004805; >> + gpu_cc_pll_config.test_ctl_hi_val = 0x40000000; > > Is there a reason this is built on the stack? Save space or something? > I have done as we had discussed during the dispcc review for SDM845 https://patchwork.kernel.org/patch/10446073/ >>> >> +static const struct alpha_pll_config disp_cc_pll0_config = { >> + .l = 0x2c, >> + .alpha = 0xcaaa, >> +}; > > Any reason this can't be put on the stack in the probe function? > I would move it. >>> In case you think I should move it outside I can do that too. >> + >> + clk_fabia_pll_configure(&gpu_cc_pll1, regmap, &gpu_cc_pll_config); >> + >> + /* Recommended WAKEUP/SLEEP settings for the gpu_cc_cx_gmu_clk */ >> + mask = CX_GMU_CBCR_WAKE_MASK << CX_GMU_CBCR_WAKE_SHIFT; >> + mask |= CX_GMU_CBCR_SLEEP_MASK << CX_GMU_CBCR_SLEEP_SHIFT; >> + value = 0xF << CX_GMU_CBCR_WAKE_SHIFT | 0xF << CX_GMU_CBCR_SLEEP_SHIFT; > > mask and value can just be big constants? I'm not sure anyone cares to > tweak this later, but I guess it's fine this way too. > >> + regmap_update_bits(regmap, 0x1098, mask, value); >> + >> + /* gpu_cc_ahb_clk */ > > What are we doing to gpu_cc_ahb_clk?? > I was marking this CRITICAL clock, but I checked the HW specs and it is always left ON from HW, so will remove it. >> + regmap_update_bits(regmap, 0x1078, 0x1, 0x1); >> + >> + /* Configure clk_dis_wait for gpu_cx_gdsc */ >> + regmap_update_bits(regmap, 0x106c, CLK_DIS_WAIT_MASK, >> + 8 << CLK_DIS_WAIT_SHIFT); >> + >> + return qcom_cc_really_probe(pdev, &gpu_cc_sc7180_desc, regmap); >> +} >> + >> +static struct platform_driver gpu_cc_sc7180_driver = { >> + .probe = gpu_cc_sc7180_probe, >> + .driver = { >> + .name = "sc7180-gpucc", >> + .of_match_table = gpu_cc_sc7180_match_table, >> + }, >> +}; >> + >> +static int __init gpu_cc_sc7180_init(void) >> +{ >> + return platform_driver_register(&gpu_cc_sc7180_driver); >> +} >> +core_initcall(gpu_cc_sc7180_init); > > Does it need to be core initcal? Maybe module_platform_driver() works > just as well? > I will leave it to subsys_initcall. >> + >> +static void __exit gpu_cc_sc7180_exit(void) >> +{ >> + platform_driver_unregister(&gpu_cc_sc7180_driver); >> +} >> +module_exit(gpu_cc_sc7180_exit); >> + >> +MODULE_DESCRIPTION("QTI GPU_CC SC7180 Driver"); >> +MODULE_LICENSE("GPL v2");
Hi Stephen, Thanks for the review. On 11/6/2019 6:09 AM, Stephen Boyd wrote: > Quoting Taniya Das (2019-10-31 05:21:13) >> diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c >> new file mode 100644 >> index 0000000..bef034b >> --- /dev/null >> +++ b/drivers/clk/qcom/videocc-sc7180.c >> @@ -0,0 +1,263 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2019, The Linux Foundation. All rights reserved. >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/err.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> > > Are these of includes used? > I will clean them up. >> +#include <linux/regmap.h> >> + >> +#include <dt-bindings/clock/qcom,videocc-sc7180.h> >> + >> +#include "clk-alpha-pll.h" >> +#include "clk-branch.h" >> +#include "clk-rcg.h" >> +#include "clk-regmap.h" >> +#include "common.h" >> +#include "gdsc.h" >> + >> +enum { >> + P_BI_TCXO, >> + P_CHIP_SLEEP_CLK, >> + P_CORE_BI_PLL_TEST_SE, >> + P_VIDEO_PLL0_OUT_EVEN, >> + P_VIDEO_PLL0_OUT_MAIN, >> + P_VIDEO_PLL0_OUT_ODD, >> +}; >> + >> +static struct pll_vco fabia_vco[] = { > > const? > yes, will add it. >> + { 249600000, 2000000000, 0 }, >> +}; >> + > [...] >> + >> +static int video_cc_sc7180_probe(struct platform_device *pdev) >> +{ >> + struct regmap *regmap; >> + struct alpha_pll_config video_pll0_config = {}; >> + >> + regmap = qcom_cc_map(pdev, &video_cc_sc7180_desc); >> + if (IS_ERR(regmap)) >> + return PTR_ERR(regmap); >> + >> + video_pll0_config.l = 0x1F; > > lowercase hex please. > >> + video_pll0_config.alpha = 0x4000; >> + video_pll0_config.user_ctl_val = 0x00000001; >> + video_pll0_config.user_ctl_hi_val = 0x00004805; > > Same question, why on stack? > Will update the same. >> + >> + clk_fabia_pll_configure(&video_pll0, regmap, &video_pll0_config); >> + >> + /* video_cc_xo_clk */ > > What are we doing? Enabling it? > yes, enabling it. Did not model and mark it CRITICAL. >> + regmap_update_bits(regmap, 0x984, 0x1, 0x1); >> + >> + return qcom_cc_really_probe(pdev, &video_cc_sc7180_desc, regmap); >> +} >> + >> +static struct platform_driver video_cc_sc7180_driver = { >> + .probe = video_cc_sc7180_probe, >> + .driver = { >> + .name = "sc7180-videocc", >> + .of_match_table = video_cc_sc7180_match_table, >> + }, >> +}; >> + >> +static int __init video_cc_sc7180_init(void) >> +{ >> + return platform_driver_register(&video_cc_sc7180_driver); >> +} >> +core_initcall(video_cc_sc7180_init); >> + >> +static void __exit video_cc_sc7180_exit(void) >> +{ >> + platform_driver_unregister(&video_cc_sc7180_driver); >> +} >> +module_exit(video_cc_sc7180_exit); > > Same question, module platform driver perhaps? > I will move it to subsys_initcall().
Quoting Taniya Das (2019-11-15 00:11:28) > Hi Stephen, > > Thanks for the review. > > On 11/6/2019 6:00 AM, Stephen Boyd wrote: > > Quoting Taniya Das (2019-10-31 05:21:10) > >> diff --git a/drivers/clk/qcom/gpucc-sc7180.c b/drivers/clk/qcom/gpucc-sc7180.c > >> new file mode 100644 > >> index 0000000..0d893e6 > >> --- /dev/null > >> +++ b/drivers/clk/qcom/gpucc-sc7180.c > >> @@ -0,0 +1,274 @@ > >> +// SPDX-License-Identifier: GPL-2.0-only > >> +/* > >> + * Copyright (c) 2019, The Linux Foundation. All rights reserved. > >> + */ > >> + > >> +#include <linux/clk-provider.h> > >> +#include <linux/err.h> > >> +#include <linux/kernel.h> > >> +#include <linux/module.h> > >> +#include <linux/of.h> > >> +#include <linux/of_device.h> > > > > Are these of includes used? > > > > yes, would clean up these headers. > Maybe they're used. I'm not sure. > >> + regmap = qcom_cc_map(pdev, &gpu_cc_sc7180_desc); > >> + if (IS_ERR(regmap)) > >> + return PTR_ERR(regmap); > >> + > >> + /* 360MHz Configuration */ > >> + gpu_cc_pll_config.l = 0x12; > >> + gpu_cc_pll_config.alpha = 0xC000; > >> + gpu_cc_pll_config.config_ctl_val = 0x20485699; > >> + gpu_cc_pll_config.config_ctl_hi_val = 0x00002067; > >> + gpu_cc_pll_config.user_ctl_val = 0x00000001; > >> + gpu_cc_pll_config.user_ctl_hi_val = 0x00004805; > >> + gpu_cc_pll_config.test_ctl_hi_val = 0x40000000; > > > > Is there a reason this is built on the stack? Save space or something? > > > > I have done as we had discussed during the dispcc review for SDM845 > https://patchwork.kernel.org/patch/10446073/ > >>> > >> +static const struct alpha_pll_config disp_cc_pll0_config = { > >> + .l = 0x2c, > >> + .alpha = 0xcaaa, > >> +}; > > > > Any reason this can't be put on the stack in the probe function? > > > I would move it. > >>> > > In case you think I should move it outside I can do that too. No I was just wondering what prompted it.
Quoting Taniya Das (2019-11-15 00:11:31) > Hi Stephen, > > Thanks for the review. > > On 11/6/2019 6:09 AM, Stephen Boyd wrote: > > Quoting Taniya Das (2019-10-31 05:21:13) > >> diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c > >> new file mode 100644 > >> index 0000000..bef034b > >> --- /dev/null > >> +++ b/drivers/clk/qcom/videocc-sc7180.c > >> @@ -0,0 +1,263 @@ > > > >> + video_pll0_config.alpha = 0x4000; > >> + video_pll0_config.user_ctl_val = 0x00000001; > >> + video_pll0_config.user_ctl_hi_val = 0x00004805; > > > > Same question, why on stack? > > > > Will update the same. Sounds like nothing to do. > > >> + > >> + clk_fabia_pll_configure(&video_pll0, regmap, &video_pll0_config); > >> + > >> + /* video_cc_xo_clk */ > > > > What are we doing? Enabling it? > > > > yes, enabling it. Did not model and mark it CRITICAL. Ok. Please describe that in the comment. > >> +} > >> +module_exit(video_cc_sc7180_exit); > > > > Same question, module platform driver perhaps? > > > > I will move it to subsys_initcall(). > Hmm ok.