Add IPA clock support for clk-rpmh

Add IPA clock support by extending the current clk rpmh driver to support clocks that are managed by a different type of RPMh resource known as Bus Clock Manager(BCM).

Signed-off-by David Dai <[email protected]>

---

drivers/clk/qcom/clk-rpmh.c +0 -0

index 9f4fc77..42e2cd2 100644

drivers/clk/qcom/clk-rpmh.c +22 -0

+};
+
 /**
  * struct clk_rpmh - individual rpmh clock data structure
  * @hw:                        handle between common and hardware-specific interfaces

@@ -210,6 +249,91 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
        .recalc_rate    = clk_rpmh_recalc_rate,
 };
 
+static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
+{
+       struct tcs_cmd cmd = { 0 };
+       u32 cmd_state;
+       int ret;
+
+       cmd_state = enable ? (c->aggr_state ? c->aggr_state : 1) : 0;
+
+       if (c->last_sent_aggr_state == cmd_state)
+               return 0;
+
+       cmd.addr = c->res_addr;
+       cmd.data = BCM_TCS_CMD(enable, cmd_state);
+
+       ret = rpmh_write_async(c->dev, RPMH_ACTIVE_ONLY_STATE, &cmd, 1);
+       if (ret) {
+               dev_err(c->dev, "set active state of %s failed: (%d)\n",
+                       c->res_name, ret);
+               return ret;
+       }

drivers/clk/qcom/clk-rpmh.c +15 -0

+
+       mutex_lock(&rpmh_clk_lock);
+       ret = clk_rpmh_bcm_send_cmd(c, true);
+       mutex_unlock(&rpmh_clk_lock);
+
+       return ret;
+};
+
+static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
+{
+       struct clk_rpmh *c = to_clk_rpmh(hw);
+
+       mutex_lock(&rpmh_clk_lock);
+       clk_rpmh_bcm_send_cmd(c, false);
+       mutex_unlock(&rpmh_clk_lock);

drivers/clk/qcom/clk-rpmh.c +8 -0

+               mutex_unlock(&rpmh_clk_lock);
+       }
+
+       return 0;
+};
+
+static long clk_rpmh_round_rate(struct clk_hw *hw, unsigned long rate,
+                               unsigned long *parent_rate)

drivers/clk/qcom/clk-rpmh.c +10 -0

+}
+
+static const struct clk_ops clk_rpmh_bcm_ops = {
+       .prepare        = clk_rpmh_bcm_prepare,
+       .unprepare      = clk_rpmh_bcm_unprepare,
+       .set_rate       = clk_rpmh_bcm_set_rate,
+       .round_rate     = clk_rpmh_round_rate,
+       .recalc_rate    = clk_rpmh_bcm_recalc_rate,
+};
+
 /* Resource name must match resource id present in cmd-db. */
 DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 2);
 DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2", 2);

drivers/clk/qcom/clk-rpmh.c +0 -0

                rpmh_clk->res_addr += res_addr;
                rpmh_clk->dev = &pdev->dev;
 

Quoting David Dai (2018-12-03 19:50:13)

Quoted 3 lines

Add IPA clock support by extending the current clk rpmh driver to support clocks that are managed by a different type of RPMh resource known as Bus Clock Manager(BCM).

Yes, but why? Does the IPA driver need to set clk rates and that somehow doesn't work as a bandwidth request?

Quoted 7 lines

Signed-off-by David Dai <[email protected]>

---

drivers/clk/qcom/clk-rpmh.c +0 -0

index 9f4fc77..42e2cd2 100644

drivers/clk/qcom/clk-rpmh.c line 27

Quoted 10 lines

drivers/clk/qcom/clk-rpmh.c +7 -0

@@ -18,6 +18,32 @@
 #define CLK_RPMH_ARC_EN_OFFSET         0
 #define CLK_RPMH_VRM_EN_OFFSET         4
 
+#define BCM_TCS_CMD_COMMIT_MASK                0x40000000
+#define BCM_TCS_CMD_VALID_SHIFT                29
+#define BCM_TCS_CMD_VOTE_MASK          0x3fff
+#define BCM_TCS_CMD_VOTE_SHIFT         0
+
+#define BCM_TCS_CMD(valid, vote) \
+       (BCM_TCS_CMD_COMMIT_MASK |\

Nitpick: Add space before the \ and align them all up on the right side of the page.

drivers/clk/qcom/clk-rpmh.c line 39

Quoted 12 lines

drivers/clk/qcom/clk-rpmh.c +12 -0

+       ((valid) << BCM_TCS_CMD_VALID_SHIFT) |\
+       ((cpu_to_le32(vote) &\
+       BCM_TCS_CMD_VOTE_MASK) << BCM_TCS_CMD_VOTE_SHIFT))
+
+/**
+ * struct bcm_db - Auxiliary data pertaining to each Bus Clock Manager(BCM)
+ * @unit: divisor used to convert Hz value to an RPMh msg
+ * @width: multiplier used to convert Hz value to an RPMh msg
+ * @vcd: virtual clock domain that this bcm belongs to
+ * @reserved: reserved to pad the struct
+ */
+

Nitpick: Please remove the newline between comment and struct.

drivers/clk/qcom/clk-rpmh.c line 44

Quoted 5 lines

drivers/clk/qcom/clk-rpmh.c +5 -0

+struct bcm_db {
+       u32 unit;
+       u16 width;
+       u8 vcd;
+       u8 reserved;

These would need __le32 and __le16 for the byte swap.

Quoted 29 lines

drivers/clk/qcom/clk-rpmh.c +22 -0

+};
+
 /**
  * struct clk_rpmh - individual rpmh clock data structure
  * @hw:                        handle between common and hardware-specific interfaces

@@ -210,6 +249,91 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
        .recalc_rate    = clk_rpmh_recalc_rate,
 };
 
+static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
+{
+       struct tcs_cmd cmd = { 0 };
+       u32 cmd_state;
+       int ret;
+
+       cmd_state = enable ? (c->aggr_state ? c->aggr_state : 1) : 0;
+
+       if (c->last_sent_aggr_state == cmd_state)
+               return 0;
+
+       cmd.addr = c->res_addr;
+       cmd.data = BCM_TCS_CMD(enable, cmd_state);
+
+       ret = rpmh_write_async(c->dev, RPMH_ACTIVE_ONLY_STATE, &cmd, 1);
+       if (ret) {
+               dev_err(c->dev, "set active state of %s failed: (%d)\n",
+                       c->res_name, ret);
+               return ret;
+       }

drivers/clk/qcom/clk-rpmh.c static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, · line 281

Quoted 10 lines

drivers/clk/qcom/clk-rpmh.c +10 -0

+
+       c->last_sent_aggr_state = cmd_state;
+
+       return 0;
+}
+
+static int clk_rpmh_bcm_prepare(struct clk_hw *hw)
+{
+       struct clk_rpmh *c = to_clk_rpmh(hw);
+       int ret = 0;

Don't initialize variables and then reassign them right after.

Quoted 15 lines

drivers/clk/qcom/clk-rpmh.c +15 -0

+
+       mutex_lock(&rpmh_clk_lock);
+       ret = clk_rpmh_bcm_send_cmd(c, true);
+       mutex_unlock(&rpmh_clk_lock);
+
+       return ret;
+};
+
+static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
+{
+       struct clk_rpmh *c = to_clk_rpmh(hw);
+
+       mutex_lock(&rpmh_clk_lock);
+       clk_rpmh_bcm_send_cmd(c, false);
+       mutex_unlock(&rpmh_clk_lock);

drivers/clk/qcom/clk-rpmh.c static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, · line 306

Quoted 10 lines

drivers/clk/qcom/clk-rpmh.c +10 -0

+};
+
+static int clk_rpmh_bcm_set_rate(struct clk_hw *hw, unsigned long rate,
+                                unsigned long parent_rate)
+{
+       struct clk_rpmh *c = to_clk_rpmh(hw);
+
+       c->aggr_state = rate / (c->aux_data.unit * 1000);
+
+       if (clk_hw_is_prepared(hw)) {

Why do we need to check is_prepared? Add a comment indicating we can't send the request when the clk is disabled?

drivers/clk/qcom/clk-rpmh.c static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, · line 308

Quoted 2 lines

drivers/clk/qcom/clk-rpmh.c +2 -0

+               mutex_lock(&rpmh_clk_lock);
+               clk_rpmh_bcm_send_cmd(c, true);

This function is always inside mutex_lock()/unlock() pair, so push the lock into the function?

Quoted 8 lines

drivers/clk/qcom/clk-rpmh.c +8 -0

+               mutex_unlock(&rpmh_clk_lock);
+       }
+
+       return 0;
+};
+
+static long clk_rpmh_round_rate(struct clk_hw *hw, unsigned long rate,
+                               unsigned long *parent_rate)

drivers/clk/qcom/clk-rpmh.c static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, · line 326

Quoted 10 lines

drivers/clk/qcom/clk-rpmh.c +10 -0

+{
+       return rate;
+}
+
+static unsigned long clk_rpmh_bcm_recalc_rate(struct clk_hw *hw,
+                                       unsigned long prate)
+{
+       struct clk_rpmh *c = to_clk_rpmh(hw);
+
+       return c->aggr_state * c->aux_data.unit * 1000;

What is 1000 for?

Quoted 14 lines

drivers/clk/qcom/clk-rpmh.c +10 -0

+}
+
+static const struct clk_ops clk_rpmh_bcm_ops = {
+       .prepare        = clk_rpmh_bcm_prepare,
+       .unprepare      = clk_rpmh_bcm_unprepare,
+       .set_rate       = clk_rpmh_bcm_set_rate,
+       .round_rate     = clk_rpmh_round_rate,
+       .recalc_rate    = clk_rpmh_bcm_recalc_rate,
+};
+
 /* Resource name must match resource id present in cmd-db. */
 DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 2);
 DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2", 2);

drivers/clk/qcom/clk-rpmh.c static int clk_rpmh_probe(struct platform_device *pdev) · line 404

Quoted 3 lines

drivers/clk/qcom/clk-rpmh.c +0 -0

@@ -275,6 +402,21 @@ static int clk_rpmh_probe(struct platform_device *pdev)
                                rpmh_clk->res_name);
                        return -ENODEV;
                }

Nitpick: Please add newline here.

drivers/clk/qcom/clk-rpmh.c static int clk_rpmh_probe(struct platform_device *pdev) · line 408

Quoted 4 lines

drivers/clk/qcom/clk-rpmh.c +4 -0

+               aux_data_len = cmd_db_read_aux_data_len(rpmh_clk->res_name);
+               if (aux_data_len == sizeof(struct bcm_db)) {
+                       ret = cmd_db_read_aux_data(rpmh_clk->res_name,
+                                                  (u8 *)&rpmh_clk->aux_data,

Is the cast necessary? And I would just pick out the data you need from the aux_data instead of storing it forever (with the padding) in the rpmh_clk structure.

drivers/clk/qcom/clk-rpmh.c static int clk_rpmh_probe(struct platform_device *pdev) · line 419

Quoted 11 lines

drivers/clk/qcom/clk-rpmh.c +11 -0

+                                                  sizeof(struct bcm_db));
+                       if (ret < 0) {
+                               dev_err(&pdev->dev, "aux data read failure for %s (%d)\n",
+                               rpmh_clk->res_name, ret);
+                               return ret;
+                       }
+                       rpmh_clk->aux_data.unit =
+                                       le32_to_cpu(rpmh_clk->aux_data.unit);
+                       rpmh_clk->aux_data.width =
+                                       le16_to_cpu(rpmh_clk->aux_data.width);
+               }

Nitpick: Newline here too.

Quoted 3 lines

drivers/clk/qcom/clk-rpmh.c +0 -0

                rpmh_clk->res_addr += res_addr;
                rpmh_clk->dev = &pdev->dev;
 

Quoted 5 lines

Quoting David Dai (2018-12-03 19:50:13)

Quoted 3 lines

Add IPA clock support by extending the current clk rpmh driver to support clocks that are managed by a different type of RPMh resource known as Bus Clock Manager(BCM).

Quoted 2 lines

Yes, but why? Does the IPA driver need to set clk rates and that somehow doesn't work as a bandwidth request?

The IPA core clock is a *clock*, not a bus. Representing it as if it were a bus, abusing the interconnect interface--pretending a bandwidth request is really a clock rate request--is kind of kludgy. I think Bjorn and David (and maybe Georgi? I don't know) decided a long time ago that exposing this as a clock is the right way to do it. I agree with that.

Quoting Alex Elder (2018-12-04 13:41:47)

Quoted 5 lines · Stephen Boyd

Quoted 4 lines

Quoting David Dai (2018-12-03 19:50:13)

Quoted 3 lines

Add IPA clock support by extending the current clk rpmh driver to support clocks that are managed by a different type of RPMh resource known as Bus Clock Manager(BCM).

Quoted 8 lines

Quoted 2 lines

Yes, but why? Does the IPA driver need to set clk rates and that somehow doesn't work as a bandwidth request?

The IPA core clock is a *clock*, not a bus. Representing it as if it were a bus, abusing the interconnect interface--pretending a bandwidth request is really a clock rate request--is kind of kludgy. I think Bjorn and David (and maybe Georgi? I don't know) decided a long time ago that exposing this as a clock is the right way to do it. I agree with that.

But then we translate that clock rate into a bandwidth request to the BCM hardware? Seems really weird because it's doing the opposite of what you say is abusive. What does the IPA driver plan to do with this clk? Calculate a frequency by knowing that it really boils down to some bandwidth that then gets converted back into some clock frequency? Do we have the user somewhere that can be pointed to?

Of course, none of these details are in the commit text so it's really hard for me as a bystander to figure this all out. So again, please add these sorts of details to the commit text so we can be "sold" on the idea of the patch instead of stating what the patch does.

Quoted 14 lines

Quoting Alex Elder (2018-12-04 13:41:47)

Quoted 5 lines · Stephen Boyd

Quoted 4 lines

Quoting David Dai (2018-12-03 19:50:13)

Quoted 3 lines

Add IPA clock support by extending the current clk rpmh driver to support clocks that are managed by a different type of RPMh resource known as Bus Clock Manager(BCM).

Quoted 8 lines

Quoted 2 lines

Yes, but why? Does the IPA driver need to set clk rates and that somehow doesn't work as a bandwidth request?

The IPA core clock is a *clock*, not a bus. Representing it as if it were a bus, abusing the interconnect interface--pretending a bandwidth request is really a clock rate request--is kind of kludgy. I think Bjorn and David (and maybe Georgi? I don't know) decided a long time ago that exposing this as a clock is the right way to do it. I agree with that.

Quoted 6 lines

But then we translate that clock rate into a bandwidth request to the BCM hardware? Seems really weird because it's doing the opposite of what you say is abusive. What does the IPA driver plan to do with this clk? Calculate a frequency by knowing that it really boils down to some bandwidth that then gets converted back into some clock frequency? Do we have the user somewhere that can be pointed to?

The clock rate is translated into a unitless threshold value sent as part of the rpmh msg
that BCM takes to select a performance. In this case, the unit conversion is based on
the unit value read from the aux data which is in Khz. I understand that this wasn't
explicitly mentioned anywhere and I'll improve on that next patch.

Here's a link to
the IPA driver implementation: https://lkml.org/lkml/2018/11/7/220

Quoting David Dai (2018-12-04 17:14:10)

Quoted 21 lines · Stephen Boyd

Quoted 14 lines

Quoting Alex Elder (2018-12-04 13:41:47)

Quoted 5 lines · Stephen Boyd

Quoted 4 lines

Quoting David Dai (2018-12-03 19:50:13)

Quoted 3 lines

Add IPA clock support by extending the current clk rpmh driver to support clocks that are managed by a different type of RPMh resource known as Bus Clock Manager(BCM).

Quoted 8 lines

Quoted 2 lines

Yes, but why? Does the IPA driver need to set clk rates and that somehow doesn't work as a bandwidth request?

The IPA core clock is a *clock*, not a bus. Representing it as if it were a bus, abusing the interconnect interface--pretending a bandwidth request is really a clock rate request--is kind of kludgy. I think Bjorn and David (and maybe Georgi? I don't know) decided a long time ago that exposing this as a clock is the right way to do it. I agree with that.

Quoted 6 lines

But then we translate that clock rate into a bandwidth request to the BCM hardware? Seems really weird because it's doing the opposite of what you say is abusive. What does the IPA driver plan to do with this clk? Calculate a frequency by knowing that it really boils down to some bandwidth that then gets converted back into some clock frequency? Do we have the user somewhere that can be pointed to?

Quoted 7 lines

The clock rate is translated into a unitless threshold value sent as part of the rpmh msg
that BCM takes to select a performance. In this case, the unit conversion is based on
the unit value read from the aux data which is in Khz. I understand that this wasn't
explicitly mentioned anywhere and I'll improve on that next patch.

How is this different from bus bandwidth requests? In those cases the bandwidth is calculated in bits per second or something like that, and written to the hardware so it can convert that bandwidth into kHz and set a bus clk frequency in the clock controller? So in the IPA case we've skipped the bps to kHz conversion step and gone straight to the clk frequency setting part? Is a BCM able to aggregate units of bandwidth or kHz depending on how it's configured and this BCM is configured for kHz?

Quoted 3 lines

Here's a link to
the IPA driver implementation: https://lkml.org/lkml/2018/11/7/220

Thanks for the link. It looks like the IPA driver hard codes a rate of 75 MHz.

Quoted 17 lines

Quoting David Dai (2018-12-04 17:14:10)

Quoted 9 lines · Stephen Boyd

Quoted 8 lines

Quoting Alex Elder (2018-12-04 13:41:47)

Quoted 1 line

But then we translate that clock rate into a bandwidth request to the BCM hardware? Seems really weird because it's doing the opposite of what you say is abusive. What does the IPA driver plan to do with this clk? Calculate a frequency by knowing that it really boils down to some bandwidth that then gets converted back into some clock frequency? Do we have the user somewhere that can be pointed to?

Quoted 7 lines

The clock rate is translated into a unitless threshold value sent as part of the rpmh msg
that BCM takes to select a performance. In this case, the unit conversion is based on
the unit value read from the aux data which is in Khz. I understand that this wasn't
explicitly mentioned anywhere and I'll improve on that next patch.

Quoted 8 lines

How is this different from bus bandwidth requests? In those cases the bandwidth is calculated in bits per second or something like that, and written to the hardware so it can convert that bandwidth into kHz and set a bus clk frequency in the clock controller? So in the IPA case we've skipped the bps to kHz conversion step and gone straight to the clk frequency setting part? Is a BCM able to aggregate units of bandwidth or kHz depending on how it's configured and this BCM is configured for kHz?

The data written to the hardware is just a 14bit scalar value that it takes to select a performance/frequency from a preset table. It's not really doing any sort of conversion in hardware in this case, instead the value is computed by software based on the aux data given. Think of it as a generic aggregator as opposed to being strictly bandwidth and the aggregator itself does not care what type of value it is(be it Khz or BW/s).

Quoting David Dai (2018-12-04 17:14:10)

Quoted 22 lines · Stephen Boyd

Quoted 14 lines

Quoting Alex Elder (2018-12-04 13:41:47)

Quoted 5 lines · Stephen Boyd

Quoted 4 lines

Quoting David Dai (2018-12-03 19:50:13)

Quoted 3 lines

Add IPA clock support by extending the current clk rpmh driver to support clocks that are managed by a different type of RPMh resource known as Bus Clock Manager(BCM).

Quoted 8 lines

Quoted 2 lines

Yes, but why? Does the IPA driver need to set clk rates and that somehow doesn't work as a bandwidth request?

The IPA core clock is a *clock*, not a bus. Representing it as if it were a bus, abusing the interconnect interface--pretending a bandwidth request is really a clock rate request--is kind of kludgy. I think Bjorn and David (and maybe Georgi? I don't know) decided a long time ago that exposing this as a clock is the right way to do it. I agree with that.

Quoted 6 lines

But then we translate that clock rate into a bandwidth request to the BCM hardware? Seems really weird because it's doing the opposite of what you say is abusive. What does the IPA driver plan to do with this clk? Calculate a frequency by knowing that it really boils down to some bandwidth that then gets converted back into some clock frequency? Do we have the user somewhere that can be pointed to?

Quoted 7 lines

The clock rate is translated into a unitless threshold value sent as part of the rpmh msg
that BCM takes to select a performance. In this case, the unit conversion is based on
the unit value read from the aux data which is in Khz. I understand that this wasn't
explicitly mentioned anywhere and I'll improve on that next patch.

How is this different from bus bandwidth requests? In those cases the bandwidth is calculated in bits per second or something like that, and written to the hardware so it can convert that bandwidth into kHz and set a bus clk frequency in the clock controller? So in the IPA case we've skipped the bps to kHz conversion step and gone straight to the clk frequency setting part? Is a BCM able to aggregate units of bandwidth or kHz depending on how it's configured and this BCM is configured for kHz?

Quoting David Dai (2018-12-03 19:50:13)

Quoted 3 lines

Add IPA clock support by extending the current clk rpmh driver to support clocks that are managed by a different type of RPMh resource known as Bus Clock Manager(BCM).

Yes, but why? Does the IPA driver need to set clk rates and that somehow doesn't work as a bandwidth request?

Quoted 10 lines

Signed-off-by David Dai <[email protected]>

---

drivers/clk/qcom/clk-rpmh.c +0 -0

index 9f4fc77..42e2cd2 100644
@@ -18,6 +18,32 @@
  #define CLK_RPMH_ARC_EN_OFFSET         0
  #define CLK_RPMH_VRM_EN_OFFSET         4
 

drivers/clk/qcom/clk-rpmh.c line 27

Quoted 7 lines

drivers/clk/qcom/clk-rpmh.c +7 -0

+#define BCM_TCS_CMD_COMMIT_MASK                0x40000000
+#define BCM_TCS_CMD_VALID_SHIFT                29
+#define BCM_TCS_CMD_VOTE_MASK          0x3fff
+#define BCM_TCS_CMD_VOTE_SHIFT         0
+
+#define BCM_TCS_CMD(valid, vote) \
+       (BCM_TCS_CMD_COMMIT_MASK |\

Nitpick: Add space before the \ and align them all up on the right side of the page.

drivers/clk/qcom/clk-rpmh.c line 39

Quoted 13 lines

drivers/clk/qcom/clk-rpmh.c +12 -0

+       ((valid) << BCM_TCS_CMD_VALID_SHIFT) |\
+       ((cpu_to_le32(vote) &\
+       BCM_TCS_CMD_VOTE_MASK) << BCM_TCS_CMD_VOTE_SHIFT))
+
+/**
+ * struct bcm_db - Auxiliary data pertaining to each Bus Clock Manager(BCM)
+ * @unit: divisor used to convert Hz value to an RPMh msg
+ * @width: multiplier used to convert Hz value to an RPMh msg
+ * @vcd: virtual clock domain that this bcm belongs to
+ * @reserved: reserved to pad the struct
+ */
+

Nitpick: Please remove the newline between comment and struct.

drivers/clk/qcom/clk-rpmh.c line 44

Quoted 5 lines

drivers/clk/qcom/clk-rpmh.c +5 -0

+struct bcm_db {
+       u32 unit;
+       u16 width;
+       u8 vcd;
+       u8 reserved;

These would need __le32 and __le16 for the byte swap.

Quoted 32 lines

drivers/clk/qcom/clk-rpmh.c +25 -0

+};
+
  /**
   * struct clk_rpmh - individual rpmh clock data structure
   * @hw:                        handle between common and hardware-specific interfaces

@@ -210,6 +249,91 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
         .recalc_rate    = clk_rpmh_recalc_rate,
  };
 
+static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
+{
+       struct tcs_cmd cmd = { 0 };
+       u32 cmd_state;
+       int ret;
+
+       cmd_state = enable ? (c->aggr_state ? c->aggr_state : 1) : 0;
+
+       if (c->last_sent_aggr_state == cmd_state)
+               return 0;
+
+       cmd.addr = c->res_addr;
+       cmd.data = BCM_TCS_CMD(enable, cmd_state);
+
+       ret = rpmh_write_async(c->dev, RPMH_ACTIVE_ONLY_STATE, &cmd, 1);
+       if (ret) {
+               dev_err(c->dev, "set active state of %s failed: (%d)\n",
+                       c->res_name, ret);
+               return ret;
+       }
+
+       c->last_sent_aggr_state = cmd_state;
+

drivers/clk/qcom/clk-rpmh.c static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, · line 281

Quoted 7 lines

drivers/clk/qcom/clk-rpmh.c +7 -0

+       return 0;
+}
+
+static int clk_rpmh_bcm_prepare(struct clk_hw *hw)
+{
+       struct clk_rpmh *c = to_clk_rpmh(hw);
+       int ret = 0;

Don't initialize variables and then reassign them right after.

Quoted 19 lines

drivers/clk/qcom/clk-rpmh.c +19 -0

+
+       mutex_lock(&rpmh_clk_lock);
+       ret = clk_rpmh_bcm_send_cmd(c, true);
+       mutex_unlock(&rpmh_clk_lock);
+
+       return ret;
+};
+
+static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
+{
+       struct clk_rpmh *c = to_clk_rpmh(hw);
+
+       mutex_lock(&rpmh_clk_lock);
+       clk_rpmh_bcm_send_cmd(c, false);
+       mutex_unlock(&rpmh_clk_lock);
+};
+
+static int clk_rpmh_bcm_set_rate(struct clk_hw *hw, unsigned long rate,
+                                unsigned long parent_rate)

drivers/clk/qcom/clk-rpmh.c static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, · line 306

Quoted 6 lines

drivers/clk/qcom/clk-rpmh.c +6 -0

+{
+       struct clk_rpmh *c = to_clk_rpmh(hw);
+
+       c->aggr_state = rate / (c->aux_data.unit * 1000);
+
+       if (clk_hw_is_prepared(hw)) {

Why do we need to check is_prepared? Add a comment indicating we can't send the request when the clk is disabled?

drivers/clk/qcom/clk-rpmh.c static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, · line 308

Quoted 3 lines

drivers/clk/qcom/clk-rpmh.c +2 -0

+               mutex_lock(&rpmh_clk_lock);
+               clk_rpmh_bcm_send_cmd(c, true);

This function is always inside mutex_lock()/unlock() pair, so push the lock into the function?

Quoted 12 lines

drivers/clk/qcom/clk-rpmh.c +11 -0

+               mutex_unlock(&rpmh_clk_lock);
+       }
+
+       return 0;
+};
+
+static long clk_rpmh_round_rate(struct clk_hw *hw, unsigned long rate,
+                               unsigned long *parent_rate)
+{
+       return rate;
+}

drivers/clk/qcom/clk-rpmh.c static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, · line 326

Quoted 7 lines

drivers/clk/qcom/clk-rpmh.c +7 -0

+
+static unsigned long clk_rpmh_bcm_recalc_rate(struct clk_hw *hw,
+                                       unsigned long prate)
+{
+       struct clk_rpmh *c = to_clk_rpmh(hw);
+
+       return c->aggr_state * c->aux_data.unit * 1000;

What is 1000 for?

Quoted 11 lines

drivers/clk/qcom/clk-rpmh.c +10 -0

+}
+
+static const struct clk_ops clk_rpmh_bcm_ops = {
+       .prepare        = clk_rpmh_bcm_prepare,
+       .unprepare      = clk_rpmh_bcm_unprepare,
+       .set_rate       = clk_rpmh_bcm_set_rate,
+       .round_rate     = clk_rpmh_round_rate,
+       .recalc_rate    = clk_rpmh_bcm_recalc_rate,
+};
+

drivers/clk/qcom/clk-rpmh.c static int clk_rpmh_probe(struct platform_device *pdev) · line 404

Quoted 7 lines

drivers/clk/qcom/clk-rpmh.c +0 -0

  /* Resource name must match resource id present in cmd-db. */
  DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 2);
  DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2", 2);

@@ -275,6 +402,21 @@ static int clk_rpmh_probe(struct platform_device *pdev)
                                 rpmh_clk->res_name);
                         return -ENODEV;
                 }

Nitpick: Please add newline here.

drivers/clk/qcom/clk-rpmh.c static int clk_rpmh_probe(struct platform_device *pdev) · line 408

Quoted 4 lines

drivers/clk/qcom/clk-rpmh.c +4 -0

+               aux_data_len = cmd_db_read_aux_data_len(rpmh_clk->res_name);
+               if (aux_data_len == sizeof(struct bcm_db)) {
+                       ret = cmd_db_read_aux_data(rpmh_clk->res_name,
+                                                  (u8 *)&rpmh_clk->aux_data,

Is the cast necessary? And I would just pick out the data you need from the aux_data instead of storing it forever (with the padding) in the rpmh_clk structure.

drivers/clk/qcom/clk-rpmh.c static int clk_rpmh_probe(struct platform_device *pdev) · line 419

Quoted 11 lines

drivers/clk/qcom/clk-rpmh.c +11 -0

+                                                  sizeof(struct bcm_db));
+                       if (ret < 0) {
+                               dev_err(&pdev->dev, "aux data read failure for %s (%d)\n",
+                               rpmh_clk->res_name, ret);
+                               return ret;
+                       }
+                       rpmh_clk->aux_data.unit =
+                                       le32_to_cpu(rpmh_clk->aux_data.unit);
+                       rpmh_clk->aux_data.width =
+                                       le16_to_cpu(rpmh_clk->aux_data.width);
+               }

Nitpick: Newline here too.

Quoted 3 lines

drivers/clk/qcom/clk-rpmh.c +0 -0

                 rpmh_clk->res_addr += res_addr;
                 rpmh_clk->dev = &pdev->dev;
 

This patch extends the existing clk-rpmh driver to support a different type of RPMh resource known as Bus Clock Manager(BCM) in order to scale performance for the Qualcomm IP Accelerator(IPA) core clock.