2022-06-21 16:57:33

by Yassine Oudjana

[permalink] [raw]
Subject: [PATCH 0/6] clk: qcom: msm8996-cpu: Cleanup and migrate to parent_data

This series includes some cleanup of the MSM8996 CPU clock driver, as well as
migration from parent_names to parent_data for all of its clocks. The DT schema
is also fixed in this series to show the actual clocks consumed by the clock
controller and pass checks.

Yassine Oudjana (6):
clk: qcom: msm8996-cpu: Rename DIV_2_INDEX to SMUX_INDEX
clk: qcom: msm8996-cpu: Statically define PLL dividers
clk: qcom: msm8996-cpu: Unify cluster order
clk: qcom: msm8996-cpu: Convert secondary muxes to clk_regmap_mux
dt-bindings: clock: qcom,msm8996-apcc: Fix clocks
clk: qcom: msm8996-cpu: Use parent_data for all clocks

.../bindings/clock/qcom,msm8996-apcc.yaml | 15 +-
drivers/clk/qcom/clk-cpu-8996.c | 235 ++++++++++--------
2 files changed, 140 insertions(+), 110 deletions(-)

--
2.36.1


2022-06-21 17:00:27

by Yassine Oudjana

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

From: Yassine Oudjana <[email protected]>

This will allow for adding them to clk_parent_data arrays
in an upcoming patch.

Signed-off-by: Yassine Oudjana <[email protected]>
---
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);
+ 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

2022-06-21 17:11:18

by Dmitry Baryshkov

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

On Tue, 21 Jun 2022 at 19:07, Yassine Oudjana <[email protected]> wrote:
>
> From: Yassine Oudjana <[email protected]>
>
> This will allow for adding them to clk_parent_data arrays
> in an upcoming patch.
>
> Signed-off-by: Yassine Oudjana <[email protected]>
> ---
> 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.

> + 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

2022-07-13 22:07:43

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 0/6] clk: qcom: msm8996-cpu: Cleanup and migrate to parent_data

On Tue 21 Jun 11:06 CDT 2022, Yassine Oudjana wrote:

> This series includes some cleanup of the MSM8996 CPU clock driver, as well as
> migration from parent_names to parent_data for all of its clocks. The DT schema
> is also fixed in this series to show the actual clocks consumed by the clock
> controller and pass checks.

This series looks almost ready to be merged, could you (or Dmitry?)
update the two outstanding items?

Thanks,
Bjorn

>
> Yassine Oudjana (6):
> clk: qcom: msm8996-cpu: Rename DIV_2_INDEX to SMUX_INDEX
> clk: qcom: msm8996-cpu: Statically define PLL dividers
> clk: qcom: msm8996-cpu: Unify cluster order
> clk: qcom: msm8996-cpu: Convert secondary muxes to clk_regmap_mux
> dt-bindings: clock: qcom,msm8996-apcc: Fix clocks
> clk: qcom: msm8996-cpu: Use parent_data for all clocks
>
> .../bindings/clock/qcom,msm8996-apcc.yaml | 15 +-
> drivers/clk/qcom/clk-cpu-8996.c | 235 ++++++++++--------
> 2 files changed, 140 insertions(+), 110 deletions(-)
>
> --
> 2.36.1
>

2022-07-14 08:37:49

by Yassine Oudjana

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


On Tue, Jun 21 2022 at 20:02:28 +0300, Dmitry Baryshkov
<[email protected]> wrote:
> On Tue, 21 Jun 2022 at 19:07, Yassine Oudjana
> <[email protected]> wrote:
>>
>> From: Yassine Oudjana <[email protected]>
>>
>> This will allow for adding them to clk_parent_data arrays
>> in an upcoming patch.
>>
>> Signed-off-by: Yassine Oudjana <[email protected]>
>> ---
>> 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


2022-07-14 10:15:34

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 0/6] clk: qcom: msm8996-cpu: Cleanup and migrate to parent_data

On 14/07/2022 00:32, Bjorn Andersson wrote:
> On Tue 21 Jun 11:06 CDT 2022, Yassine Oudjana wrote:
>
>> This series includes some cleanup of the MSM8996 CPU clock driver, as well as
>> migration from parent_names to parent_data for all of its clocks. The DT schema
>> is also fixed in this series to show the actual clocks consumed by the clock
>> controller and pass checks.
>
> This series looks almost ready to be merged, could you (or Dmitry?)
> update the two outstanding items?

I have acked the patch 2 and sent the slightly updated revision of
patch6 (together with the rest of small changes).

>
> Thanks,
> Bjorn
>
>>
>> Yassine Oudjana (6):
>> clk: qcom: msm8996-cpu: Rename DIV_2_INDEX to SMUX_INDEX
>> clk: qcom: msm8996-cpu: Statically define PLL dividers
>> clk: qcom: msm8996-cpu: Unify cluster order
>> clk: qcom: msm8996-cpu: Convert secondary muxes to clk_regmap_mux
>> dt-bindings: clock: qcom,msm8996-apcc: Fix clocks
>> clk: qcom: msm8996-cpu: Use parent_data for all clocks
>>
>> .../bindings/clock/qcom,msm8996-apcc.yaml | 15 +-
>> drivers/clk/qcom/clk-cpu-8996.c | 235 ++++++++++--------
>> 2 files changed, 140 insertions(+), 110 deletions(-)
>>
>> --
>> 2.36.1
>>


--
With best wishes
Dmitry

2022-07-14 10:16:00

by Dmitry Baryshkov

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

On 14/07/2022 11:32, Yassine Oudjana wrote:
>
> On Tue, Jun 21 2022 at 20:02:28 +0300, Dmitry Baryshkov
> <[email protected]> wrote:
>> On Tue, 21 Jun 2022 at 19:07, Yassine Oudjana
>> <[email protected]> wrote:
>>>
>>>  From: Yassine Oudjana <[email protected]>
>>>
>>>  This will allow for adding them to clk_parent_data arrays
>>>  in an upcoming patch.
>>>
>>>  Signed-off-by: Yassine Oudjana <[email protected]>
>>>  ---
>>>   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?

I have been playing with your patchset. In the end I have dropped the
idea of using devm_clk_hw_register_fixed_factor() and just used
devm_clk_hw_register too. So:

Reviewed-by: Dmitry Baryshkov <[email protected]>

>
>>
>>>  +       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
>
>


--
With best wishes
Dmitry

2022-09-09 11:01:40

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH 0/6] clk: qcom: msm8996-cpu: Cleanup and migrate to parent_data

On 14/07/2022 13:06, Dmitry Baryshkov wrote:
> On 14/07/2022 00:32, Bjorn Andersson wrote:
>> On Tue 21 Jun 11:06 CDT 2022, Yassine Oudjana wrote:
>>
>>> This series includes some cleanup of the MSM8996 CPU clock driver, as
>>> well as
>>> migration from parent_names to parent_data for all of its clocks. The
>>> DT schema
>>> is also fixed in this series to show the actual clocks consumed by
>>> the clock
>>> controller and pass checks.
>>
>> This series looks almost ready to be merged, could you (or Dmitry?)
>> update the two outstanding items?
>
> I have acked the patch 2 and sent the slightly updated revision of
> patch6 (together with the rest of small changes).

Bjorn, could you please pick up patches 1-5?

>
>>
>> Thanks,
>> Bjorn
>>
>>>
>>> Yassine Oudjana (6):
>>>    clk: qcom: msm8996-cpu: Rename DIV_2_INDEX to SMUX_INDEX
>>>    clk: qcom: msm8996-cpu: Statically define PLL dividers
>>>    clk: qcom: msm8996-cpu: Unify cluster order
>>>    clk: qcom: msm8996-cpu: Convert secondary muxes to clk_regmap_mux
>>>    dt-bindings: clock: qcom,msm8996-apcc: Fix clocks
>>>    clk: qcom: msm8996-cpu: Use parent_data for all clocks
>>>
>>>   .../bindings/clock/qcom,msm8996-apcc.yaml     |  15 +-
>>>   drivers/clk/qcom/clk-cpu-8996.c               | 235 ++++++++++--------
>>>   2 files changed, 140 insertions(+), 110 deletions(-)
>>>
>>> --
>>> 2.36.1
>>>
>
>

--
With best wishes
Dmitry

2022-09-27 04:04:40

by Bjorn Andersson

[permalink] [raw]
Subject: Re: (subset) [PATCH 0/6] clk: qcom: msm8996-cpu: Cleanup and migrate to parent_data

On Tue, 21 Jun 2022 20:06:15 +0400, Yassine Oudjana wrote:
> This series includes some cleanup of the MSM8996 CPU clock driver, as well as
> migration from parent_names to parent_data for all of its clocks. The DT schema
> is also fixed in this series to show the actual clocks consumed by the clock
> controller and pass checks.
>
> Yassine Oudjana (6):
> clk: qcom: msm8996-cpu: Rename DIV_2_INDEX to SMUX_INDEX
> clk: qcom: msm8996-cpu: Statically define PLL dividers
> clk: qcom: msm8996-cpu: Unify cluster order
> clk: qcom: msm8996-cpu: Convert secondary muxes to clk_regmap_mux
> dt-bindings: clock: qcom,msm8996-apcc: Fix clocks
> clk: qcom: msm8996-cpu: Use parent_data for all clocks
>
> [...]

Applied, thanks!

[1/6] clk: qcom: msm8996-cpu: Rename DIV_2_INDEX to SMUX_INDEX
commit: 1ba0a3bbd5ed5a1bb8d0165912d9904b812af74b
[2/6] clk: qcom: msm8996-cpu: Statically define PLL dividers
commit: de37e0214c28330cf0dbf4fe51db1d9d38c13c93
[3/6] clk: qcom: msm8996-cpu: Unify cluster order
commit: 382139bfd68fe6cc9dc94ffe3b9d783b85be3b1c
[4/6] clk: qcom: msm8996-cpu: Convert secondary muxes to clk_regmap_mux
commit: 9a9f5f9a5a0ca3f463eb28ba5920a6fd18dc9956
[5/6] dt-bindings: clock: qcom,msm8996-apcc: Fix clocks
commit: b4feed4a3d0a6b8cef4a574a9df707c556928ec2

Best regards,
--
Bjorn Andersson <[email protected]>