Re: [PATCH 2/6] clk: qcom: msm8996-cpu: Statically define PLL dividers

[Yassine Oudjana <yassine.oudjana@gmail.com>]

On Tue, Jun 21 2022 at 20:02:28 +0300, Dmitry Baryshkov 
<dmitry.baryshkov@linaro.org> wrote:
> On Tue, 21 Jun 2022 at 19:07, Yassine Oudjana 
> <yassine.oudjana@gmail.com> wrote:
>> 
>>  From: Yassine Oudjana <y.oudjana@protonmail.com>
>> 
>>  This will allow for adding them to clk_parent_data arrays
>>  in an upcoming patch.
>> 
>>  Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>>  ---
>>   drivers/clk/qcom/clk-cpu-8996.c | 66 
>> +++++++++++++++++++++------------
>>   1 file changed, 42 insertions(+), 24 deletions(-)
>> 
>>  diff --git a/drivers/clk/qcom/clk-cpu-8996.c 
>> b/drivers/clk/qcom/clk-cpu-8996.c
>>  index 5dc68dc3621f..217f9392c23d 100644
>>  --- a/drivers/clk/qcom/clk-cpu-8996.c
>>  +++ b/drivers/clk/qcom/clk-cpu-8996.c
>>  @@ -135,6 +135,34 @@ static struct clk_alpha_pll pwrcl_pll = {
>>          },
>>   };
>> 
>>  +static struct clk_fixed_factor pwrcl_pll_postdiv = {
>>  +       .mult = 1,
>>  +       .div = 2,
>>  +       .hw.init = &(struct clk_init_data){
>>  +               .name = "pwrcl_pll_postdiv",
>>  +               .parent_data = &(const struct clk_parent_data){
>>  +                       .hw = &pwrcl_pll.clkr.hw
>>  +               },
>>  +               .num_parents = 1,
>>  +               .ops = &clk_fixed_factor_ops,
>>  +               .flags = CLK_SET_RATE_PARENT,
>>  +       },
>>  +};
>>  +
>>  +static struct clk_fixed_factor perfcl_pll_postdiv = {
>>  +       .mult = 1,
>>  +       .div = 2,
>>  +       .hw.init = &(struct clk_init_data){
>>  +               .name = "perfcl_pll_postdiv",
>>  +               .parent_data = &(const struct clk_parent_data){
>>  +                       .hw = &perfcl_pll.clkr.hw
>>  +               },
>>  +               .num_parents = 1,
>>  +               .ops = &clk_fixed_factor_ops,
>>  +               .flags = CLK_SET_RATE_PARENT,
>>  +       },
>>  +};
>>  +
>>   static const struct pll_vco alt_pll_vco_modes[] = {
>>          VCO(3,  250000000,  500000000),
>>          VCO(2,  500000000,  750000000),
>>  @@ -261,7 +289,7 @@ static struct clk_cpu_8996_mux pwrcl_smux = {
>>                  .name = "pwrcl_smux",
>>                  .parent_names = (const char *[]){
>>                          "xo",
>>  -                       "pwrcl_pll_main",
>>  +                       "pwrcl_pll_postdiv",
>>                  },
>>                  .num_parents = 2,
>>                  .ops = &clk_cpu_8996_mux_ops,
>>  @@ -277,7 +305,7 @@ static struct clk_cpu_8996_mux perfcl_smux = {
>>                  .name = "perfcl_smux",
>>                  .parent_names = (const char *[]){
>>                          "xo",
>>  -                       "perfcl_pll_main",
>>  +                       "perfcl_pll_postdiv",
>>                  },
>>                  .num_parents = 2,
>>                  .ops = &clk_cpu_8996_mux_ops,
>>  @@ -354,32 +382,25 @@ static int 
>> qcom_cpu_clk_msm8996_register_clks(struct device *dev,
>>   {
>>          int i, ret;
>> 
>>  -       perfcl_smux.pll = clk_hw_register_fixed_factor(dev, 
>> "perfcl_pll_main",
>>  -                                                      "perfcl_pll",
>>  -                                                      
>> CLK_SET_RATE_PARENT,
>>  -                                                      1, 2);
>>  -       if (IS_ERR(perfcl_smux.pll)) {
>>  -               dev_err(dev, "Failed to initialize 
>> perfcl_pll_main\n");
>>  -               return PTR_ERR(perfcl_smux.pll);
>>  +       ret = devm_clk_hw_register(dev, &pwrcl_pll_postdiv.hw);
> 
> No need to. I'd suggest picking up the
> devm_clk_hw_register_fixed_factor patch from my patchset and using
> this API.

I did it this way to be able to define it statically in the
`parent_data` arrays of the secondary muxes in patch 6/6. How
would I do it this way? Do I define global `static struct clk_hw *`s
for the postdivs and use them in the `parent_data` arrays, or
perhaps un-constify the arrays and insert the returned
`struct clk_hw *`s into them here? Also can you send a link to
your patch? or is it already applied?

> 
>>  +       if (ret) {
>>  +               dev_err(dev, "Failed to register pwrcl_pll_postdiv: 
>> %d", ret);
>>  +               return ret;
>>          }
>> 
>>  -       pwrcl_smux.pll = clk_hw_register_fixed_factor(dev, 
>> "pwrcl_pll_main",
>>  -                                                     "pwrcl_pll",
>>  -                                                     
>> CLK_SET_RATE_PARENT,
>>  -                                                     1, 2);
>>  -       if (IS_ERR(pwrcl_smux.pll)) {
>>  -               dev_err(dev, "Failed to initialize 
>> pwrcl_pll_main\n");
>>  -               clk_hw_unregister(perfcl_smux.pll);
>>  -               return PTR_ERR(pwrcl_smux.pll);
>>  +       ret = devm_clk_hw_register(dev, &perfcl_pll_postdiv.hw);
>>  +       if (ret) {
>>  +               dev_err(dev, "Failed to register 
>> perfcl_pll_postdiv: %d", ret);
>>  +               return ret;
>>          }
>> 
>>  +       pwrcl_smux.pll = &pwrcl_pll_postdiv.hw;
>>  +       perfcl_smux.pll = &perfcl_pll_postdiv.hw;
>>  +
>>          for (i = 0; i < ARRAY_SIZE(cpu_msm8996_clks); i++) {
>>                  ret = devm_clk_register_regmap(dev, 
>> cpu_msm8996_clks[i]);
>>  -               if (ret) {
>>  -                       clk_hw_unregister(perfcl_smux.pll);
>>  -                       clk_hw_unregister(pwrcl_smux.pll);
>>  +               if (ret)
>>                          return ret;
>>  -               }
>>          }
>> 
>>          clk_alpha_pll_configure(&perfcl_pll, regmap, &hfpll_config);
>>  @@ -409,9 +430,6 @@ static int 
>> qcom_cpu_clk_msm8996_unregister_clks(void)
>>          if (ret)
>>                  return ret;
>> 
>>  -       clk_hw_unregister(perfcl_smux.pll);
>>  -       clk_hw_unregister(pwrcl_smux.pll);
>>  -
>>          return 0;
>>   }
>> 
>>  --
>>  2.36.1
>> 
> 
> 
> --
> With best wishes
> Dmitry