| Message ID | 20201208064702.3654324-1-vkoul@kernel.org |
|---|---|
| Headers | show |
| Series | Add clock drivers for SM8350 | expand |
Quoting Vinod Koul (2020-12-07 22:47:01) > diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c > index 564431130a76..6a399663d564 100644 > --- a/drivers/clk/qcom/clk-alpha-pll.c > +++ b/drivers/clk/qcom/clk-alpha-pll.c > @@ -146,6 +146,12 @@ EXPORT_SYMBOL_GPL(clk_alpha_pll_regs); > /* LUCID PLL specific settings and offsets */ > #define LUCID_PCAL_DONE BIT(27) > > +/* LUCID 5LPE PLL specific settings and offsets */ > +#define LUCID_5LPE_PCAL_DONE BIT(11) > +#define LUCID_5LPE_ENABLE_VOTE_RUN BIT(21) > +#define LUCID_5LPE_PLL_LATCH_INPUT BIT(14) > +#define LUCID_5LPE_ALPHA_PLL_ACK_LATCH BIT(13) Sort these by bit or define name? > + > #define pll_alpha_width(p) \ > ((PLL_ALPHA_VAL_U(p) - PLL_ALPHA_VAL(p) == 4) ? \ > ALPHA_REG_BITWIDTH : ALPHA_REG_16BIT_WIDTH) > @@ -1561,3 +1567,220 @@ const struct clk_ops clk_alpha_pll_postdiv_lucid_ops = { > .set_rate = clk_alpha_pll_postdiv_fabia_set_rate, > }; > EXPORT_SYMBOL_GPL(clk_alpha_pll_postdiv_lucid_ops); > + > +static int alpha_pll_lucid_5lpe_enable(struct clk_hw *hw) > +{ > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > + u32 val; > + int ret; > + > + ret = regmap_read(pll->clkr.regmap, PLL_USER_CTL(pll), &val); > + if (ret) > + return ret; > + > + /* If in FSM mode, just vote for it */ > + if (val & LUCID_5LPE_ENABLE_VOTE_RUN) { > + ret = clk_enable_regmap(hw); > + if (ret) > + return ret; > + return wait_for_pll_enable_lock(pll); > + } > + > + /* Check if PLL is already enabled */ Yeah that's obvious, but then what? > + ret = trion_pll_is_enabled(pll, pll->clkr.regmap); > + if (ret < 0) > + return ret; > + > + ret = regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), PLL_RESET_N, PLL_RESET_N); > + if (ret) > + return ret; > + > + /* Set operation mode to RUN */ This comment is worthless. > + regmap_write(pll->clkr.regmap, PLL_OPMODE(pll), PLL_RUN); > + > + ret = wait_for_pll_enable_lock(pll); > + if (ret) > + return ret; > + > + /* Enable the PLL outputs */ > + ret = regmap_update_bits(pll->clkr.regmap, PLL_USER_CTL(pll), PLL_OUT_MASK, PLL_OUT_MASK); > + if (ret) > + return ret; > + > + /* Enable the global PLL outputs */ > + ret = regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), PLL_OUTCTRL, PLL_OUTCTRL); > + if (ret) > + return ret; > + > + /* Ensure that the write above goes through before returning. */ > + mb(); Regmap has a memory barrier in writel. Drop this. > + return ret; > +} > + > +static void alpha_pll_lucid_5lpe_disable(struct clk_hw *hw) > +{ > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > + u32 val; > + int ret; > + > + ret = regmap_read(pll->clkr.regmap, PLL_USER_CTL(pll), &val); > + if (ret) > + return; > + > + /* If in FSM mode, just unvote it */ > + if (val & LUCID_5LPE_ENABLE_VOTE_RUN) { > + clk_disable_regmap(hw); > + return; > + } > + > + /* Disable the global PLL output */ > + ret = regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), PLL_OUTCTRL, 0); > + if (ret) > + return; > + > + /* Disable the PLL outputs */ > + ret = regmap_update_bits(pll->clkr.regmap, PLL_USER_CTL(pll), PLL_OUT_MASK, 0); > + if (ret) > + return; > + > + /* Place the PLL mode in STANDBY */ > + regmap_write(pll->clkr.regmap, PLL_OPMODE(pll), PLL_STANDBY); > +} > + > +/* > + * The Lucid 5LPE PLL requires a power-on self-calibration which happens > + * when the PLL comes out of reset. Calibrate in case it is not completed. > + */ > +static int alpha_pll_lucid_5lpe_prepare(struct clk_hw *hw) > +{ > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > + struct clk_hw *p; > + u32 regval; Can you use u32 val? And also include a patch to replace the couple times where there is 'regval' in this file. The former is shorter and used far more in qcom clk code. > + int ret; > + > + /* Return early if calibration is not needed. */ > + regmap_read(pll->clkr.regmap, PLL_MODE(pll), ®val); > + if (regval & LUCID_5LPE_PCAL_DONE) > + return 0; > + > + p = clk_hw_get_parent(hw); > + if (!p) > + return -EINVAL; > + > + ret = alpha_pll_lucid_5lpe_enable(hw); > + if (ret) > + return ret; > + > + alpha_pll_lucid_5lpe_disable(hw); > + > + return 0; > +} > + > +static int alpha_pll_lucid_5lpe_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long prate) > +{ > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > + unsigned long rrate; > + u32 regval, l; > + u64 a; > + int ret; > + > + rrate = alpha_pll_round_rate(rate, prate, &l, &a, ALPHA_REG_16BIT_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 > (rate + PLL_RATE_MARGIN) || rrate < rate) { > + pr_err("Call set rate on the PLL with rounded rates!\n"); > + return -EINVAL; > + } Can we use alpha_pll_check_rate_margin()? > + > + regmap_write(pll->clkr.regmap, PLL_L_VAL(pll), l); > + regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), a); > + > + /* Latch the PLL input */ > + ret = regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), > + LUCID_5LPE_PLL_LATCH_INPUT, LUCID_5LPE_PLL_LATCH_INPUT); > + if (ret) > + return ret; > + > + /* Wait for 2 reference cycles before checking the ACK bit. */ > + udelay(1); > + regmap_read(pll->clkr.regmap, PLL_MODE(pll), ®val); > + if (!(regval & LUCID_5LPE_ALPHA_PLL_ACK_LATCH)) { > + pr_err("Lucid 5LPE PLL latch failed. Output may be unstable!\n"); > + return -EINVAL; > + } > + > + /* Return the latch input to 0 */ > + ret = regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), LUCID_5LPE_PLL_LATCH_INPUT, 0); > + if (ret) > + return ret; > + > + if (clk_hw_is_enabled(hw)) { > + ret = wait_for_pll_enable_lock(pll); > + if (ret) > + return ret; > + } > + > + /* Wait for PLL output to stabilize */ > + udelay(100); > + return 0; > +} > + > +static int clk_lucid_5lpe_pll_postdiv_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + struct clk_alpha_pll_postdiv *pll = to_clk_alpha_pll_postdiv(hw); > + int i, val = 0, div, ret; > + > + /* > + * If the PLL is in FSM mode, then treat set_rate callback as a > + * no-operation. > + */ > + ret = regmap_read(pll->clkr.regmap, PLL_USER_CTL(pll), &val); > + if (ret) > + return ret; > + > + if (val & LUCID_5LPE_ENABLE_VOTE_RUN) > + return 0; > + > + if (!pll->post_div_table) { > + pr_err("Missing the post_div_table for the PLL\n"); Can this be rolled into the loop below? > + return -EINVAL; > + } > + > + div = DIV_ROUND_UP_ULL((u64)parent_rate, rate); > + for (i = 0; i < pll->num_post_div; i++) { So that this finds nothing. > + if (pll->post_div_table[i].div == div) { > + val = pll->post_div_table[i].val; > + break; > + } > + } and then if val == -1 we return -EINVAL? > + > + return regmap_update_bits(pll->clkr.regmap, PLL_USER_CTL(pll), > + (BIT(pll->width) - 1) << pll->post_div_shift, Use GENMASK? > + val << pll->post_div_shift); > +} > +
Quoting Vinod Koul (2020-12-07 22:46:59) > This adds the RPMH clocks present in SM8350 SoC > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Vinod Koul <vkoul@kernel.org> > --- Applied to clk-next with lots of noise!
On 10-12-20, 12:36, Stephen Boyd wrote: > Quoting Vinod Koul (2020-12-07 22:47:01) > > diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c > > index 564431130a76..6a399663d564 100644 > > --- a/drivers/clk/qcom/clk-alpha-pll.c > > +++ b/drivers/clk/qcom/clk-alpha-pll.c > > @@ -146,6 +146,12 @@ EXPORT_SYMBOL_GPL(clk_alpha_pll_regs); > > /* LUCID PLL specific settings and offsets */ > > #define LUCID_PCAL_DONE BIT(27) > > > > +/* LUCID 5LPE PLL specific settings and offsets */ > > +#define LUCID_5LPE_PCAL_DONE BIT(11) > > +#define LUCID_5LPE_ENABLE_VOTE_RUN BIT(21) > > +#define LUCID_5LPE_PLL_LATCH_INPUT BIT(14) > > +#define LUCID_5LPE_ALPHA_PLL_ACK_LATCH BIT(13) > > Sort these by bit or define name? Okay will sort by bit > > > + > > #define pll_alpha_width(p) \ > > ((PLL_ALPHA_VAL_U(p) - PLL_ALPHA_VAL(p) == 4) ? \ > > ALPHA_REG_BITWIDTH : ALPHA_REG_16BIT_WIDTH) > > @@ -1561,3 +1567,220 @@ const struct clk_ops clk_alpha_pll_postdiv_lucid_ops = { > > .set_rate = clk_alpha_pll_postdiv_fabia_set_rate, > > }; > > EXPORT_SYMBOL_GPL(clk_alpha_pll_postdiv_lucid_ops); > > + > > +static int alpha_pll_lucid_5lpe_enable(struct clk_hw *hw) > > +{ > > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > > + u32 val; > > + int ret; > > + > > + ret = regmap_read(pll->clkr.regmap, PLL_USER_CTL(pll), &val); > > + if (ret) > > + return ret; > > + > > + /* If in FSM mode, just vote for it */ > > + if (val & LUCID_5LPE_ENABLE_VOTE_RUN) { > > + ret = clk_enable_regmap(hw); > > + if (ret) > > + return ret; > > + return wait_for_pll_enable_lock(pll); > > + } > > + > > + /* Check if PLL is already enabled */ > > Yeah that's obvious, but then what? then dont proceed :) will update > > + ret = trion_pll_is_enabled(pll, pll->clkr.regmap); > > + if (ret < 0) > > + return ret; > > + > > + ret = regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), PLL_RESET_N, PLL_RESET_N); > > + if (ret) > > + return ret; > > + > > + /* Set operation mode to RUN */ > > This comment is worthless. Will drop > > > + regmap_write(pll->clkr.regmap, PLL_OPMODE(pll), PLL_RUN); > > + > > + ret = wait_for_pll_enable_lock(pll); > > + if (ret) > > + return ret; > > + > > + /* Enable the PLL outputs */ > > + ret = regmap_update_bits(pll->clkr.regmap, PLL_USER_CTL(pll), PLL_OUT_MASK, PLL_OUT_MASK); > > + if (ret) > > + return ret; > > + > > + /* Enable the global PLL outputs */ > > + ret = regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), PLL_OUTCTRL, PLL_OUTCTRL); > > + if (ret) > > + return ret; > > + > > + /* Ensure that the write above goes through before returning. */ > > + mb(); > > Regmap has a memory barrier in writel. Drop this. yes > > > + return ret; > > +} > > + > > +static void alpha_pll_lucid_5lpe_disable(struct clk_hw *hw) > > +{ > > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > > + u32 val; > > + int ret; > > + > > + ret = regmap_read(pll->clkr.regmap, PLL_USER_CTL(pll), &val); > > + if (ret) > > + return; > > + > > + /* If in FSM mode, just unvote it */ > > + if (val & LUCID_5LPE_ENABLE_VOTE_RUN) { > > + clk_disable_regmap(hw); > > + return; > > + } > > + > > + /* Disable the global PLL output */ > > + ret = regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), PLL_OUTCTRL, 0); > > + if (ret) > > + return; > > + > > + /* Disable the PLL outputs */ > > + ret = regmap_update_bits(pll->clkr.regmap, PLL_USER_CTL(pll), PLL_OUT_MASK, 0); > > + if (ret) > > + return; > > + > > + /* Place the PLL mode in STANDBY */ > > + regmap_write(pll->clkr.regmap, PLL_OPMODE(pll), PLL_STANDBY); > > +} > > + > > +/* > > + * The Lucid 5LPE PLL requires a power-on self-calibration which happens > > + * when the PLL comes out of reset. Calibrate in case it is not completed. > > + */ > > +static int alpha_pll_lucid_5lpe_prepare(struct clk_hw *hw) > > +{ > > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > > + struct clk_hw *p; > > + u32 regval; > > Can you use u32 val? And also include a patch to replace the couple > times where there is 'regval' in this file. The former is shorter and > used far more in qcom clk code. Will do > > > + int ret; > > + > > + /* Return early if calibration is not needed. */ > > + regmap_read(pll->clkr.regmap, PLL_MODE(pll), ®val); > > + if (regval & LUCID_5LPE_PCAL_DONE) > > + return 0; > > + > > + p = clk_hw_get_parent(hw); > > + if (!p) > > + return -EINVAL; > > + > > + ret = alpha_pll_lucid_5lpe_enable(hw); > > + if (ret) > > + return ret; > > + > > + alpha_pll_lucid_5lpe_disable(hw); > > + > > + return 0; > > +} > > + > > +static int alpha_pll_lucid_5lpe_set_rate(struct clk_hw *hw, unsigned long rate, > > + unsigned long prate) > > +{ > > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > > + unsigned long rrate; > > + u32 regval, l; > > + u64 a; > > + int ret; > > + > > + rrate = alpha_pll_round_rate(rate, prate, &l, &a, ALPHA_REG_16BIT_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 > (rate + PLL_RATE_MARGIN) || rrate < rate) { > > + pr_err("Call set rate on the PLL with rounded rates!\n"); > > + return -EINVAL; > > + } > > Can we use alpha_pll_check_rate_margin()? Ah a shiny new helper, looking at it yes we should > > > + > > + regmap_write(pll->clkr.regmap, PLL_L_VAL(pll), l); > > + regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), a); > > + > > + /* Latch the PLL input */ > > + ret = regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), > > + LUCID_5LPE_PLL_LATCH_INPUT, LUCID_5LPE_PLL_LATCH_INPUT); > > + if (ret) > > + return ret; > > + > > + /* Wait for 2 reference cycles before checking the ACK bit. */ > > + udelay(1); > > + regmap_read(pll->clkr.regmap, PLL_MODE(pll), ®val); > > + if (!(regval & LUCID_5LPE_ALPHA_PLL_ACK_LATCH)) { > > + pr_err("Lucid 5LPE PLL latch failed. Output may be unstable!\n"); > > + return -EINVAL; > > + } > > + > > + /* Return the latch input to 0 */ > > + ret = regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), LUCID_5LPE_PLL_LATCH_INPUT, 0); > > + if (ret) > > + return ret; > > + > > + if (clk_hw_is_enabled(hw)) { > > + ret = wait_for_pll_enable_lock(pll); > > + if (ret) > > + return ret; > > + } > > + > > + /* Wait for PLL output to stabilize */ > > + udelay(100); > > + return 0; > > +} > > + > > +static int clk_lucid_5lpe_pll_postdiv_set_rate(struct clk_hw *hw, unsigned long rate, > > + unsigned long parent_rate) > > +{ > > + struct clk_alpha_pll_postdiv *pll = to_clk_alpha_pll_postdiv(hw); > > + int i, val = 0, div, ret; > > + > > + /* > > + * If the PLL is in FSM mode, then treat set_rate callback as a > > + * no-operation. > > + */ > > + ret = regmap_read(pll->clkr.regmap, PLL_USER_CTL(pll), &val); > > + if (ret) > > + return ret; > > + > > + if (val & LUCID_5LPE_ENABLE_VOTE_RUN) > > + return 0; > > + > > + if (!pll->post_div_table) { > > + pr_err("Missing the post_div_table for the PLL\n"); > > Can this be rolled into the loop below? Yep > > + return -EINVAL; > > + } > > + > > + div = DIV_ROUND_UP_ULL((u64)parent_rate, rate); > > + for (i = 0; i < pll->num_post_div; i++) { > > So that this finds nothing. > > > + if (pll->post_div_table[i].div == div) { > > + val = pll->post_div_table[i].val; > > + break; > > + } > > + } > > and then if val == -1 we return -EINVAL? Correct, will update > > + > > + return regmap_update_bits(pll->clkr.regmap, PLL_USER_CTL(pll), > > + (BIT(pll->width) - 1) << pll->post_div_shift, > > Use GENMASK? Looks like this can be: GENMASK(pll->width + pll->post_div_shift - 1, pll->post_div_shift) Not sure which one you like :)
On 10-12-20, 12:43, Stephen Boyd wrote: > Quoting Vinod Koul (2020-12-07 22:47:02) > > +config SM_GCC_8350 > > + tristate "SM8350 Global Clock Controller" > > + select QCOM_GDSC > > + help > > + Support for the global clock controller on SM8350 devices. > > + Say Y if you want to use peripheral devices such as UART, > > + SPI, I2C, USB, SD/UFS, PCIe etc. > > + > > + > > Why double newline? Will drop > > +#include <linux/bitops.h> > > +#include <linux/clk.h> > > Is this include used? Will check this and others and drop unused ones > > > +#include <linux/clk-provider.h> > > +#include <linux/err.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/of_device.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/reset-controller.h> > > Please add newline here > > > +#include <dt-bindings/clock/qcom,gcc-sm8350.h> > > Please add newline here Ok to both > > +static const struct clk_parent_data gcc_parent_data_0[] = { > > + { .fw_name = "bi_tcxo", .name = "bi_tcxo" }, > > + { .hw = &gcc_gpll0.clkr.hw }, > > + { .hw = &gcc_gpll0_out_even.clkr.hw }, > > + { .fw_name = "core_bi_pll_test_se", .name = "core_bi_pll_test_se" }, > > Is this .fw_name in the binding? Please remove .name everywhere in this > driver as it shouldn't be necessary. Ack will drop > > +static const struct clk_parent_data gcc_parent_data_13[] = { > > + { .fw_name = "usb3_phy_wrapper_gcc_usb30_pipe_clk", .name = > > Is this documented in the binding? Not yet, will update > > +static struct clk_rcg2 gcc_sdcc2_apps_clk_src = { > > + .cmd_rcgr = 0x1400c, > > + .mnd_width = 8, > > + .hid_width = 5, > > + .parent_map = gcc_parent_map_6, > > + .freq_tbl = ftbl_gcc_sdcc2_apps_clk_src, > > + .clkr.hw.init = &(struct clk_init_data){ > > + .name = "gcc_sdcc2_apps_clk_src", > > + .parent_data = gcc_parent_data_6, > > + .num_parents = 6, > > + .flags = CLK_SET_RATE_PARENT, > > + .ops = &clk_rcg2_ops, > > Please use floor ops per Doug's recent patch. Yes > > +static struct clk_branch gcc_camera_ahb_clk = { > > + .halt_reg = 0x26004, > > + .halt_check = BRANCH_HALT_DELAY, > > + .hwcg_reg = 0x26004, > > + .hwcg_bit = 1, > > + .clkr = { > > + .enable_reg = 0x26004, > > + .enable_mask = BIT(0), > > + .hw.init = &(struct clk_init_data){ > > + .name = "gcc_camera_ahb_clk", > > + .flags = CLK_IS_CRITICAL, > > Why is it critical? Can we just enable it in driver probe and stop > modeling it as a clk? it does not have a parent we control, yeah it would make sense to do that. Tanya do you folks agree ..? > > +static struct clk_branch gcc_video_axi0_clk = { > > + .halt_reg = 0x28010, > > + .halt_check = BRANCH_HALT_SKIP, > > Do these need to be halt skip? Is the video axi clk stuff still broken? I will check on this and update accordingly > > +static int gcc_sm8350_probe(struct platform_device *pdev) > > +{ > > + struct regmap *regmap; > > + int ret; > > + > > + regmap = qcom_cc_map(pdev, &gcc_sm8350_desc); > > + if (IS_ERR(regmap)) { > > + dev_err(&pdev->dev, "Failed to map gcc registers\n"); > > + return PTR_ERR(regmap); > > + } > > + > > + ret = qcom_cc_register_rcg_dfs(regmap, gcc_dfs_clocks, ARRAY_SIZE(gcc_dfs_clocks)); > > + if (ret) > > + return ret; > > + > > + /* FORCE_MEM_CORE_ON for ufs phy ice core clocks */ > > Why? So I understood that this needs to be set so that ufs clocks can propagate to ufs mem stuff.. -- ~Vinod
Quoting Vinod Koul (2020-12-10 21:02:57) > On 10-12-20, 12:36, Stephen Boyd wrote: > > > + > > > + return regmap_update_bits(pll->clkr.regmap, PLL_USER_CTL(pll), > > > + (BIT(pll->width) - 1) << pll->post_div_shift, > > > > Use GENMASK? > > Looks like this can be: > GENMASK(pll->width + pll->post_div_shift - 1, pll->post_div_shift) > > Not sure which one you like :) Preferably a local u32 mask = GENMASK(...)
On 12/11/2020 12:40 PM, Stephen Boyd wrote: > Quoting Vinod Koul (2020-12-10 21:43:49) >> On 10-12-20, 12:43, Stephen Boyd wrote: >>>> +static struct clk_branch gcc_camera_ahb_clk = { >>>> + .halt_reg = 0x26004, >>>> + .halt_check = BRANCH_HALT_DELAY, >>>> + .hwcg_reg = 0x26004, >>>> + .hwcg_bit = 1, >>>> + .clkr = { >>>> + .enable_reg = 0x26004, >>>> + .enable_mask = BIT(0), >>>> + .hw.init = &(struct clk_init_data){ >>>> + .name = "gcc_camera_ahb_clk", >>>> + .flags = CLK_IS_CRITICAL, >>> >>> Why is it critical? Can we just enable it in driver probe and stop >>> modeling it as a clk? >> >> it does not have a parent we control, yeah it would make sense to do >> that. Tanya do you folks agree ..? >> > > Maybe it is needed for camera clk controller? Have to check other SoCs > and see if they're using it. > Yes, they would have to be left enabled. Vinod, could you please move them to probe, similar to kona/sc7180 where all the CRITICALs clocks are left enabled?
Hi Taniya, On 13-12-20, 14:00, Taniya Das wrote: > > > On 12/11/2020 12:40 PM, Stephen Boyd wrote: > > Quoting Vinod Koul (2020-12-10 21:43:49) > > > On 10-12-20, 12:43, Stephen Boyd wrote: > > > > > +static struct clk_branch gcc_camera_ahb_clk = { > > > > > + .halt_reg = 0x26004, > > > > > + .halt_check = BRANCH_HALT_DELAY, > > > > > + .hwcg_reg = 0x26004, > > > > > + .hwcg_bit = 1, > > > > > + .clkr = { > > > > > + .enable_reg = 0x26004, > > > > > + .enable_mask = BIT(0), > > > > > + .hw.init = &(struct clk_init_data){ > > > > > + .name = "gcc_camera_ahb_clk", > > > > > + .flags = CLK_IS_CRITICAL, > > > > > > > > Why is it critical? Can we just enable it in driver probe and stop > > > > modeling it as a clk? > > > > > > it does not have a parent we control, yeah it would make sense to do > > > that. Tanya do you folks agree ..? > > > > > > > Maybe it is needed for camera clk controller? Have to check other SoCs > > and see if they're using it. > > > > Yes, they would have to be left enabled. > > Vinod, could you please move them to probe, similar to kona/sc7180 where all > the CRITICALs clocks are left enabled? Thanks for the pointer, will do Thanks
This adds rpmhcc and gcc clock controller drivers for the controllers found in SM8350 SoC Changes in v2: - Add r-b from Bjorn - Add the gcc_qupv3_wrap_1_{m|s}_ahb_clk and gcc_qupv3_wrap1_s5_clk Vinod Koul (3): dt-bindings: clock: Add RPMHCC bindings for SM8350 clk: qcom: rpmh: add support for SM8350 rpmh clocks dt-bindings: clock: Add SM8350 GCC clock bindings Vivek Aknurwar (2): clk: qcom: clk-alpha-pll: Add support for Lucid 5LPE PLL clk: qcom: gcc: Add clock driver for SM8350 .../bindings/clock/qcom,gcc-sm8350.yaml | 68 + .../bindings/clock/qcom,rpmhcc.yaml | 1 + drivers/clk/qcom/Kconfig | 9 + drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/clk-alpha-pll.c | 223 + drivers/clk/qcom/clk-alpha-pll.h | 4 + drivers/clk/qcom/clk-rpmh.c | 34 + drivers/clk/qcom/gcc-sm8350.c | 3996 +++++++++++++++++ include/dt-bindings/clock/qcom,gcc-sm8350.h | 261 ++ include/dt-bindings/clock/qcom,rpmh.h | 8 + 10 files changed, 4605 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/qcom,gcc-sm8350.yaml create mode 100644 drivers/clk/qcom/gcc-sm8350.c create mode 100644 include/dt-bindings/clock/qcom,gcc-sm8350.h -- 2.26.2