| 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.
[v1] * Fabia PLLs could fail latching in the case where the PLL is not calibrated, so add support to calibrate in prepare clock ops. * Add driver support for Graphics clock controller for SC7180 and also update device tree bindings for the various clocks supported in the clock controller. * Add driver support for Video clock controller for SC7180 and also update device tree bindings for the various clocks supported in the clock controller. This change depends on below GCC clock driver series https://patchwork.kernel.org/project/linux-clk/list/?series=187089 Taniya Das (7): clk: qcom: clk-alpha-pll: Add support for Fabia PLL calibration dt-bindings: clock: Add YAML schemas for the QCOM GPUCC clock bindings dt-bindings: clock: Introduce QCOM Graphics clock bindings clk: qcom: Add graphics clock controller driver for SC7180 dt-bindings: clock: Add YAML schemas for the QCOM VIDEOCC clock bindings dt-bindings: clock: Introduce QCOM Video clock bindings clk: qcom: Add video clock controller driver for SC7180 .../devicetree/bindings/clock/qcom,gpucc.txt | 24 -- .../devicetree/bindings/clock/qcom,gpucc.yaml | 70 ++++++ .../devicetree/bindings/clock/qcom,videocc.txt | 18 -- .../devicetree/bindings/clock/qcom,videocc.yaml | 62 +++++ drivers/clk/qcom/Kconfig | 16 ++ drivers/clk/qcom/Makefile | 2 + drivers/clk/qcom/clk-alpha-pll.c | 84 ++++++- drivers/clk/qcom/clk-alpha-pll.h | 4 + drivers/clk/qcom/gpucc-sc7180.c | 274 +++++++++++++++++++++ drivers/clk/qcom/videocc-sc7180.c | 263 ++++++++++++++++++++ include/dt-bindings/clock/qcom,gpucc-sc7180.h | 21 ++ include/dt-bindings/clock/qcom,videocc-sc7180.h | 23 ++ 12 files changed, 814 insertions(+), 47 deletions(-) delete mode 100644 Documentation/devicetree/bindings/clock/qcom,gpucc.txt create mode 100644 Documentation/devicetree/bindings/clock/qcom,gpucc.yaml delete mode 100644 Documentation/devicetree/bindings/clock/qcom,videocc.txt create mode 100644 Documentation/devicetree/bindings/clock/qcom,videocc.yaml create mode 100644 drivers/clk/qcom/gpucc-sc7180.c create mode 100644 drivers/clk/qcom/videocc-sc7180.c create mode 100644 include/dt-bindings/clock/qcom,gpucc-sc7180.h create mode 100644 include/dt-bindings/clock/qcom,videocc-sc7180.h -- Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.