clk: qcom: Misc updates for Root Clock Generators

In case of update config failure, return -EBUSY, so that consumers could handle the failure gracefully.

Signed-off-by Taniya Das <[email protected]>

---

drivers/clk/qcom/clk-rcg2.c +0 -1

        return regmap_update_bits(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg),
                                        mask, cfg);
 }
--

Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.

Quoting Taniya Das (2019-05-08 11:24:54)

drivers/clk/qcom/clk-rcg2.c static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) · line 294

Quoted 11 lines

drivers/clk/qcom/clk-rcg2.c +3 -0

index 57dbac9..5bb6d45 100644
@@ -289,6 +289,9 @@ static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f)
         cfg |= rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT;
         if (rcg->mnd_width && f->n && (f->m != f->n))
                 cfg |= CFG_MODE_DUAL_EDGE;
+       if (rcg->flags & HW_CLK_CTRL_MODE)
+               cfg |= CFG_HW_CLK_CTRL_MASK;
+

Above this we have commit bdc3bbdd40ba ("clk: qcom: Clear hardware clock control bit of RCG") that clears this bit. Is it possible to always set this bit and then have an override flag used in sdm845 that says to _not_ set this bit? Presumably on earlier platforms writing the bit is a no-op so it's safe to write the bit on those platforms.

This way, if it's going to be the default we can avoid setting the flag and only set the flag on older platforms where it shouldn't be done for some reason.

Quoted 7 lines

drivers/clk/qcom/clk-rcg2.c +0 -1

         return regmap_update_bits(rcg->clkr.regmap, RCG_CFG_OFFSET(rcg),
                                         mask, cfg);
  }
--

Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member of the Code Aurora Forum, hosted by the Linux Foundation.

Hello Stephen,

Thanks for your review.

Quoted 19 lines · Stephen Boyd

Quoting Taniya Das (2019-05-08 11:24:54)

drivers/clk/qcom/clk-rcg2.c static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) · line 294

Quoted 11 lines

drivers/clk/qcom/clk-rcg2.c +3 -0

index 57dbac9..5bb6d45 100644
@@ -289,6 +289,9 @@ static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f)
         cfg |= rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT;
         if (rcg->mnd_width && f->n && (f->m != f->n))
                 cfg |= CFG_MODE_DUAL_EDGE;
+       if (rcg->flags & HW_CLK_CTRL_MODE)
+               cfg |= CFG_HW_CLK_CTRL_MASK;
+

Above this we have commit bdc3bbdd40ba ("clk: qcom: Clear hardware clock control bit of RCG") that clears this bit. Is it possible to always set this bit and then have an override flag used in sdm845 that says to _not_ set this bit? Presumably on earlier platforms writing the bit is a no-op so it's safe to write the bit on those platforms.

Quoted 4 lines

This way, if it's going to be the default we can avoid setting the flag and only set the flag on older platforms where it shouldn't be done for some reason.

Not all the subsystem clock controllers might have this hardware control bit set from design. Thus we want to set them based on the flag.

Quoting Taniya Das (2019-07-15 21:19:02)

Quoted 29 lines

Hello Stephen,

Thanks for your review.

Quoted 23 lines · Stephen Boyd

Quoting Taniya Das (2019-05-08 11:24:54)

drivers/clk/qcom/clk-rcg2.c static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) · line 294

Quoted 11 lines

drivers/clk/qcom/clk-rcg2.c +3 -0

index 57dbac9..5bb6d45 100644
@@ -289,6 +289,9 @@ static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f)
          cfg |= rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT;
          if (rcg->mnd_width && f->n && (f->m != f->n))
                  cfg |= CFG_MODE_DUAL_EDGE;
+       if (rcg->flags & HW_CLK_CTRL_MODE)
+               cfg |= CFG_HW_CLK_CTRL_MASK;
+

Above this we have commit bdc3bbdd40ba ("clk: qcom: Clear hardware clock control bit of RCG") that clears this bit. Is it possible to always set this bit and then have an override flag used in sdm845 that says to _not_ set this bit? Presumably on earlier platforms writing the bit is a no-op so it's safe to write the bit on those platforms.

This way, if it's going to be the default we can avoid setting the flag and only set the flag on older platforms where it shouldn't be done for some reason.

Quoted 2 lines

Not all the subsystem clock controllers might have this hardware control bit set from design. Thus we want to set them based on the flag.

Yes but what's the percentage of clks that are going to set this flag vs. not set this flag? If that is low right now then it's fine but if it eventually becomes the standard mechanism it will be easier to opt-out of the feature if necessary instead of opt-in.

Hello Stephen,

Quoted 30 lines · Stephen Boyd

Quoting Taniya Das (2019-07-15 21:19:02)

Quoted 29 lines

Hello Stephen,

Thanks for your review.

Quoted 23 lines · Stephen Boyd

Quoting Taniya Das (2019-05-08 11:24:54)

drivers/clk/qcom/clk-rcg2.c static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f) · line 294

Quoted 11 lines

drivers/clk/qcom/clk-rcg2.c +3 -0

index 57dbac9..5bb6d45 100644
@@ -289,6 +289,9 @@ static int __clk_rcg2_configure(struct clk_rcg2 *rcg, const struct freq_tbl *f)
          cfg |= rcg->parent_map[index].cfg << CFG_SRC_SEL_SHIFT;
          if (rcg->mnd_width && f->n && (f->m != f->n))
                  cfg |= CFG_MODE_DUAL_EDGE;
+       if (rcg->flags & HW_CLK_CTRL_MODE)
+               cfg |= CFG_HW_CLK_CTRL_MASK;
+

Above this we have commit bdc3bbdd40ba ("clk: qcom: Clear hardware clock control bit of RCG") that clears this bit. Is it possible to always set this bit and then have an override flag used in sdm845 that says to _not_ set this bit? Presumably on earlier platforms writing the bit is a no-op so it's safe to write the bit on those platforms.

This way, if it's going to be the default we can avoid setting the flag and only set the flag on older platforms where it shouldn't be done for some reason.

Quoted 8 lines

Quoted 2 lines

Not all the subsystem clock controllers might have this hardware control bit set from design. Thus we want to set them based on the flag.

Yes but what's the percentage of clks that are going to set this flag vs. not set this flag? If that is low right now then it's fine but if it eventually becomes the standard mechanism it will be easier to opt-out of the feature if necessary instead of opt-in.

Currently all the RCGs in GCC need to clear the bit and few RCGs from the other CCs(DISPCC/VIDEOCC) where the bit is not set from HW requires this bit to be set. Thus we want to use this flag mechanism to have the flexibility to set.

Once it is a standard we could cleanup to remove this.

Update the init data name for each of the dynamic frequency switch controlled clock associated with the RCG clock name, so that it can be generated as per the hardware plan. Thus update the macro accordingly.

Signed-off-by Taniya Das <[email protected]>

---
 drivers/clk/qcom/clk-rcg.h    |  2 +-
 drivers/clk/qcom/gcc-sdm845.c | 96 +++++++++++++++++++++----------------------
 2 files changed, 49 insertions(+), 49 deletions(-)

drivers/clk/qcom/clk-rcg.h +0 -0

index 5562f38..e40e8f8 100644

drivers/clk/qcom/clk-rcg.h +0 -0

 extern int qcom_cc_register_rcg_dfs(struct regmap *regmap,
                                    const struct clk_rcg_dfs_data *rcgs,

drivers/clk/qcom/gcc-sdm845.c +1 -1

index 7131dcf..a76178b 100644
@@ -408,7 +408,7 @@ enum {
        { }
 };
 
-static struct clk_init_data gcc_qupv3_wrap0_s0_clk_init = {
+static struct clk_init_data gcc_qupv3_wrap0_s0_clk_src_init = {
        .name = "gcc_qupv3_wrap0_s0_clk_src",
        .parent_names = gcc_parent_names_0,
        .num_parents = 4,

Quoting Taniya Das (2019-05-08 11:24:55)

Quoted 5 lines

Update the init data name for each of the dynamic frequency switch controlled clock associated with the RCG clock name, so that it can be generated as per the hardware plan. Thus update the macro accordingly.

Signed-off-by Taniya Das <[email protected]>

This patch doesn't make any sense to me.

Quoted 12 lines

---
  drivers/clk/qcom/clk-rcg.h    |  2 +-
  drivers/clk/qcom/gcc-sdm845.c | 96 +++++++++++++++++++++----------------------
  2 files changed, 49 insertions(+), 49 deletions(-)

drivers/clk/qcom/clk-rcg.h +0 -0

index 5562f38..e40e8f8 100644
@@ -171,7 +171,7 @@ struct clk_rcg_dfs_data {
  };
 

drivers/clk/qcom/clk-rcg.h struct clk_rcg_dfs_data { · line 174

Quoted 3 lines

drivers/clk/qcom/clk-rcg.h +1 -1

  #define DEFINE_RCG_DFS(r) \
-       { .rcg = &r##_src, .init = &r##_init }
+       { .rcg = &r, .init = &r##_init }

Why do we need to rename the init data?

Quoted 17 lines

drivers/clk/qcom/clk-rcg.h +0 -0

  extern int qcom_cc_register_rcg_dfs(struct regmap *regmap,
                                     const struct clk_rcg_dfs_data *rcgs,

drivers/clk/qcom/gcc-sdm845.c +1 -1

index 7131dcf..a76178b 100644
@@ -408,7 +408,7 @@ enum {
         { }
  };
 
-static struct clk_init_data gcc_qupv3_wrap0_s0_clk_init = {
+static struct clk_init_data gcc_qupv3_wrap0_s0_clk_src_init = {
         .name = "gcc_qupv3_wrap0_s0_clk_src",
         .parent_names = gcc_parent_names_0,
         .num_parents = 4,

drivers/clk/qcom/gcc-sdm845.c enum { · line 3580

Quoted 5 lines

drivers/clk/qcom/gcc-sdm845.c +1 -1

@@ -3577,22 +3577,22 @@ enum {
  MODULE_DEVICE_TABLE(of, gcc_sdm845_match_table);
 
  static const struct clk_rcg_dfs_data gcc_dfs_clocks[] = {
-       DEFINE_RCG_DFS(gcc_qupv3_wrap0_s0_clk),
+       DEFINE_RCG_DFS(gcc_qupv3_wrap0_s0_clk_src),

I've trimmed the above to try and see what's changed but it doesn't make sense still.

Hello Stephen,

Thanks for the review.

Quoted 21 lines · Stephen Boyd

Quoting Taniya Das (2019-05-08 11:24:55)

Quoted 5 lines

Update the init data name for each of the dynamic frequency switch controlled clock associated with the RCG clock name, so that it can be generated as per the hardware plan. Thus update the macro accordingly.

Signed-off-by Taniya Das <[email protected]>

This patch doesn't make any sense to me.

Quoted 12 lines

---
  drivers/clk/qcom/clk-rcg.h    |  2 +-
  drivers/clk/qcom/gcc-sdm845.c | 96 +++++++++++++++++++++----------------------
  2 files changed, 49 insertions(+), 49 deletions(-)

drivers/clk/qcom/clk-rcg.h +0 -0

index 5562f38..e40e8f8 100644
@@ -171,7 +171,7 @@ struct clk_rcg_dfs_data {
  };
 

Quoted 6 lines

drivers/clk/qcom/clk-rcg.h struct clk_rcg_dfs_data { · line 174

Quoted 3 lines

drivers/clk/qcom/clk-rcg.h +1 -1

  #define DEFINE_RCG_DFS(r) \
-       { .rcg = &r##_src, .init = &r##_init }
+       { .rcg = &r, .init = &r##_init }

Why do we need to rename the init data?

We want to manage the init data as the clock source name, so that we could manage to auto generate our code. So that we do not have to re-name the clock init data manually if the DFS source names gets updated at any point of time.

Quoting Taniya Das (2019-05-09 19:58:39)

Quoted 33 lines

Hello Stephen,

Thanks for the review.

Quoted 27 lines · Stephen Boyd

Quoting Taniya Das (2019-05-08 11:24:55)

Quoted 5 lines

Update the init data name for each of the dynamic frequency switch controlled clock associated with the RCG clock name, so that it can be generated as per the hardware plan. Thus update the macro accordingly.

Signed-off-by Taniya Das <[email protected]>

This patch doesn't make any sense to me.

Quoted 10 lines

---
   drivers/clk/qcom/clk-rcg.h    |  2 +-
   drivers/clk/qcom/gcc-sdm845.c | 96 +++++++++++++++++++++----------------------
   2 files changed, 49 insertions(+), 49 deletions(-)

drivers/clk/qcom/clk-rcg.h +0 -0

index 5562f38..e40e8f8 100644

drivers/clk/qcom/clk-rcg.h struct clk_rcg_dfs_data { · line 174

Quoted 5 lines

drivers/clk/qcom/clk-rcg.h +1 -1

@@ -171,7 +171,7 @@ struct clk_rcg_dfs_data {
   };
 
   #define DEFINE_RCG_DFS(r) \
-       { .rcg = &r##_src, .init = &r##_init }
+       { .rcg = &r, .init = &r##_init }

Why do we need to rename the init data?

Quoted 5 lines

We want to manage the init data as the clock source name, so that we could manage to auto generate our code. So that we do not have to re-name the clock init data manually if the DFS source names gets updated at any point of time.

Why is the clk name changing to not have a _src after the "root" of the clk name? As long as I can remember, RCGs have a "_src" postfix.

Quoted 20 lines

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

drivers/clk/qcom/clk-rcg.h +0 -0

index 5562f38..e40e8f8 100644
@@ -171,7 +171,7 @@ struct clk_rcg_dfs_data {
   };
 

Quoted 6 lines

drivers/clk/qcom/clk-rcg.h struct clk_rcg_dfs_data { · line 174

Quoted 3 lines

drivers/clk/qcom/clk-rcg.h +1 -1

   #define DEFINE_RCG_DFS(r) \
-       { .rcg = &r##_src, .init = &r##_init }
+       { .rcg = &r, .init = &r##_init }

Why do we need to rename the init data?

We want to manage the init data as the clock source name, so that we could manage to auto generate our code. So that we do not have to re-name the clock init data manually if the DFS source names gets updated at any point of time.

Quoted 3 lines

Why is the clk name changing to not have a _src after the "root" of the clk name? As long as I can remember, RCGs have a "_src" postfix.

Yes, the RCGs would have _src, so we do want the init data also to be generated with _src postfix. So that we do not have to manually clean up the generated code.

Quoting Taniya Das (2019-05-12 20:44:46)

Quoted 25 lines · Stephen Boyd

Quoted 23 lines

Quoted 14 lines

Quoted 7 lines

Quoted 7 lines

drivers/clk/qcom/clk-rcg.h +0 -0

index 5562f38..e40e8f8 100644
@@ -171,7 +171,7 @@ struct clk_rcg_dfs_data {
    };
 

Quoted 6 lines

drivers/clk/qcom/clk-rcg.h struct clk_rcg_dfs_data { · line 174

Quoted 3 lines

drivers/clk/qcom/clk-rcg.h +1 -1

    #define DEFINE_RCG_DFS(r) \
-       { .rcg = &r##_src, .init = &r##_init }
+       { .rcg = &r, .init = &r##_init }

Why do we need to rename the init data?

Quoted 5 lines

We want to manage the init data as the clock source name, so that we could manage to auto generate our code. So that we do not have to re-name the clock init data manually if the DFS source names gets updated at any point of time.

Why is the clk name changing to not have a _src after the "root" of the clk name? As long as I can remember, RCGs have a "_src" postfix.

Quoted 4 lines

Yes, the RCGs would have _src, so we do want the init data also to be generated with _src postfix. So that we do not have to manually clean up the generated code.

Please manually cleanup the generated code, or fix the code generator to do what you want.

Hello Stephen,

Thanks for the review.

Quoted 11 lines · Stephen Boyd

Quoting Taniya Das (2019-05-12 20:44:46)

Quoted 9 lines · Stephen Boyd

Quoted 3 lines

Why is the clk name changing to not have a _src after the "root" of the clk name? As long as I can remember, RCGs have a "_src" postfix.

Yes, the RCGs would have _src, so we do want the init data also to be generated with _src postfix. So that we do not have to manually clean up the generated code.

Quoted 3 lines

Please manually cleanup the generated code, or fix the code generator to do what you want.

Fixing the code manually is not what we intend to do and it is time consuming with too many DFS controlled clocks. This really helps us align to internal code.

Quoting Taniya Das (2019-07-15 21:22:02)

Quoted 20 lines

Hello Stephen,

Thanks for the review.

Quoted 14 lines · Stephen Boyd

Quoting Taniya Das (2019-05-12 20:44:46)

Quoted 9 lines · Stephen Boyd

Quoted 3 lines

Why is the clk name changing to not have a _src after the "root" of the clk name? As long as I can remember, RCGs have a "_src" postfix.

Yes, the RCGs would have _src, so we do want the init data also to be generated with _src postfix. So that we do not have to manually clean up the generated code.

Please manually cleanup the generated code, or fix the code generator to do what you want.

Quoted 4 lines

Fixing the code manually is not what we intend to do and it is time consuming with too many DFS controlled clocks. This really helps us align to internal code.

And you can't fix the code generator to drop the _src part of whatever is spit out for the DFS lines?

Quoted 26 lines

Quoting Taniya Das (2019-07-15 21:22:02)

Quoted 24 lines

Hello Stephen,

Thanks for the review.

Quoted 6 lines · Stephen Boyd

Quoting Taniya Das (2019-05-12 20:44:46)

Quoted 5 lines · Stephen Boyd

Quoted 3 lines

Why is the clk name changing to not have a _src after the "root" of the clk name? As long as I can remember, RCGs have a "_src" postfix.

Quoted 8 lines

Quoted 4 lines

Yes, the RCGs would have _src, so we do want the init data also to be generated with _src postfix. So that we do not have to manually clean up the generated code.

Please manually cleanup the generated code, or fix the code generator to do what you want.

Fixing the code manually is not what we intend to do and it is time consuming with too many DFS controlled clocks. This really helps us align to internal code.

Quoted 3 lines

And you can't fix the code generator to drop the _src part of whatever is spit out for the DFS lines?

Sure, will drop this.