clk: qcom: clk-rpmh: Add IPA clock support

The current clk-rpmh driver only supports on and off RPMh clock resources, let's extend the current driver by add support for clocks that are managed by a different type of RPMh resource known as Bus Clock Manager(BCM). The BCM is a configurable shared resource aggregator that scales performance based on a preset of frequency points. The Qualcomm IP Accelerator (IPA) clock is an example of a resource that is managed by the BCM and this a requirement from the IPA driver in order to scale its core clock.

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

---
 drivers/clk/qcom/clk-rpmh.c           | 141 ++++++++++++++++++++++++++++++++++
 include/dt-bindings/clock/qcom,rpmh.h |   1 +
 2 files changed, 142 insertions(+)

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

index 9f4fc77..ee78c4b 100644
@@ -18,6 +18,31 @@
 #define CLK_RPMH_ARC_EN_OFFSET         0
 #define CLK_RPMH_VRM_EN_OFFSET         4

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

+       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
+ */
+struct bcm_db {
+       __le32 unit;
+       __le16 width;
+       u8 vcd;
+       u8 reserved;
+};
+
 /**
  * struct clk_rpmh - individual rpmh clock data structure
  * @hw:                        handle between common and hardware-specific interfaces

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

  * @dev:               device to which it is attached
  * @peer:              pointer to the clock rpmh sibling
  */

@@ -42,6 +68,7 @@ struct clk_rpmh {
        u32 aggr_state;
        u32 last_sent_aggr_state;
        u32 valid_state_mask;
+       u32 unit;
        struct device *dev;
        struct clk_rpmh *peer;
 };

@@ -210,6 +248,91 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
        .recalc_rate    = clk_rpmh_recalc_rate,
 };

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

+       }
+
+       c->last_sent_aggr_state = cmd_state;
+
+       mutex_unlock(&rpmh_clk_lock);
+
+       return 0;

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

+};
+
+static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
+{
+       struct clk_rpmh *c = to_clk_rpmh(hw);
+
+       clk_rpmh_bcm_send_cmd(c, false);
+};
+
+static int clk_rpmh_bcm_set_rate(struct clk_hw *hw, unsigned long rate,

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

+               clk_rpmh_bcm_send_cmd(c, true);
+
+       return 0;
+};
+

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

 static struct clk_hw *sdm845_rpmh_clocks[] = {
        [RPMH_CXO_CLK]          = &sdm845_bi_tcxo.hw,

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

Quoting David Dai (2018-12-13 18:35:04)

Quoted 7 lines

The current clk-rpmh driver only supports on and off RPMh clock resources, let's extend the current driver by add support for clocks that are managed by a different type of RPMh resource known as Bus Clock Manager(BCM). The BCM is a configurable shared resource aggregator that scales performance based on a preset of frequency points. The Qualcomm IP Accelerator (IPA) clock is an example of a resource that is managed by the BCM and this a requirement from the IPA driver in order to scale its core clock.

Please use this commit text instead:

The clk-rpmh driver only supports on and off RPMh clock resources. Let's extend the driver by add support for clocks that are managed by a different type of RPMh resource known as Bus Clock Manager(BCM). The BCM is a configurable shared resource aggregator that scales performance based on a set of frequency points. The Qualcomm IP Accelerator (IPA) clock is an example of a resource that is managed by the BCM and this a requirement from the IPA driver in order to scale its core clock.

Quoted 14 lines

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

---
  drivers/clk/qcom/clk-rpmh.c           | 141 ++++++++++++++++++++++++++++++++++
  include/dt-bindings/clock/qcom,rpmh.h |   1 +
  2 files changed, 142 insertions(+)

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

index 9f4fc77..ee78c4b 100644
@@ -18,6 +18,31 @@
  #define CLK_RPMH_ARC_EN_OFFSET         0
  #define CLK_RPMH_VRM_EN_OFFSET         4
 

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

Quoted 9 lines

drivers/clk/qcom/clk-rpmh.c +9 -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 |                              \
+       ((valid) << BCM_TCS_CMD_VALID_SHIFT) |                  \
+       ((cpu_to_le32(vote) &                                   \

Why?

Quoted 19 lines

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

+       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
+ */
+struct bcm_db {
+       __le32 unit;
+       __le16 width;
+       u8 vcd;
+       u8 reserved;
+};
+
  /**
   * struct clk_rpmh - individual rpmh clock data structure

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

Quoted 6 lines

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

   * @hw:                        handle between common and hardware-specific interfaces

@@ -29,6 +54,7 @@
   * @aggr_state:                rpmh clock aggregated state
   * @last_sent_aggr_state: rpmh clock last aggr state sent to RPMh
   * @valid_state_mask:  mask to determine the state of the rpmh clock
+ * @aux_data:          data specific to the bcm rpmh resource

But there isn't aux_data. Is this supposed to be unit? And what is unit going to be? 1000?

Quoted 15 lines

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

   * @dev:               device to which it is attached
   * @peer:              pointer to the clock rpmh sibling
   */

@@ -42,6 +68,7 @@ struct clk_rpmh {
         u32 aggr_state;
         u32 last_sent_aggr_state;
         u32 valid_state_mask;
+       u32 unit;
         struct device *dev;
         struct clk_rpmh *peer;
  };

@@ -210,6 +248,91 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
         .recalc_rate    = clk_rpmh_recalc_rate,
  };
 

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

Quoted 9 lines

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

+static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
+{
+       struct tcs_cmd cmd = { 0 };
+       u32 cmd_state;
+       int ret;
+
+       mutex_lock(&rpmh_clk_lock);
+
+       cmd_state = enable ? (c->aggr_state ? c->aggr_state : 1) : 0;

Make this into an if statement instead of double ternary?

	cmd_state = 0;
	if (enable) {
		cmd_state = 1;
		if (c->aggr_state)
			cmd_state = c->aggr_state;

	}

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

Quoted 3 lines

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

+
+       if (c->last_sent_aggr_state == cmd_state)
+               return 0;

We just returned with a mutex held. Oops!

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

Quoted 9 lines

drivers/clk/qcom/clk-rpmh.c +9 -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;

Again!

Quoted 10 lines

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

+       }
+
+       c->last_sent_aggr_state = cmd_state;
+
+       mutex_unlock(&rpmh_clk_lock);
+
+       return 0;
+}
+
+static int clk_rpmh_bcm_prepare(struct clk_hw *hw)

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

Quoted 7 lines

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

+{
+       struct clk_rpmh *c = to_clk_rpmh(hw);
+       int ret;
+
+       ret = clk_rpmh_bcm_send_cmd(c, true);
+
+       return ret;

Just write return clk_rpmh_bcm_send_cmd(...)

Quoted 11 lines

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

+};
+
+static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
+{
+       struct clk_rpmh *c = to_clk_rpmh(hw);
+
+       clk_rpmh_bcm_send_cmd(c, false);
+};
+
+static int clk_rpmh_bcm_set_rate(struct clk_hw *hw, unsigned long rate,

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

Quoted 10 lines

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

+                                unsigned long parent_rate)
+{
+       struct clk_rpmh *c = to_clk_rpmh(hw);
+
+       c->aggr_state = rate / c->unit;
+       /*
+        * Since any non-zero value sent to hw would result in enabling the
+        * clock, only send the value if the clock has already been prepared.
+        */
+       if (clk_hw_is_prepared(hw))

This is sad, but OK.

Quoted 5 lines

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

+               clk_rpmh_bcm_send_cmd(c, true);
+
+       return 0;
+};
+

[...]

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

Quoted 5 lines

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

@@ -217,6 +340,7 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
  DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1", 1);
  DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2", 1);
  DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3", 1);
+DEFINE_CLK_RPMH_BCM(sdm845, ipa, "IP0");

It's really IP0?! What was wrong with IPA?

Quoted 5 lines

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

  static struct clk_hw *sdm845_rpmh_clocks[] = {
         [RPMH_CXO_CLK]          = &sdm845_bi_tcxo.hw,

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

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

Quoted 10 lines

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

                         return -ENODEV;
                 }
+
+               data = cmd_db_read_aux_data(rpmh_clk->res_name, &aux_data_len);
+               if (IS_ERR(data)) {
+                       ret = PTR_ERR(data);
+                       dev_err(&pdev->dev,
+                               "error reading RPMh aux data for %s (%d)\n",
+                               rpmh_clk->res_name, ret);
+                               return ret;

Weird indent here.

Quoted 7 lines

Quoting David Dai (2018-12-13 18:35:04)

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

Quoted 5 lines

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

+
+#define BCM_TCS_CMD(valid, vote)                               \
+       (BCM_TCS_CMD_COMMIT_MASK |                              \
+       ((valid) << BCM_TCS_CMD_VALID_SHIFT) |                  \
+       ((cpu_to_le32(vote) &                                   \

Why?

Sorry, could you clarify this question? If you're referring to the cpu_to_le32, shouldn't we be explicit about converting endianness even if it might be redundant for this particular qcom platform?

Quoted 7 lines

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

Quoted 5 lines

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

@@ -29,6 +54,7 @@
   * @aggr_state:                rpmh clock aggregated state
   * @last_sent_aggr_state: rpmh clock last aggr state sent to RPMh
   * @valid_state_mask:  mask to determine the state of the rpmh clock
+ * @aux_data:          data specific to the bcm rpmh resource

But there isn't aux_data. Is this supposed to be unit? And what is unit going to be? 1000?

Supposed to be unit, the value is on the order of Khz.

Quoted 6 lines

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

Quoted 5 lines

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

@@ -217,6 +340,7 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
  DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1", 1);
  DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2", 1);
  DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3", 1);
+DEFINE_CLK_RPMH_BCM(sdm845, ipa, "IP0");

It's really IP0?! What was wrong with IPA?

Yeah... convention seems to be 2 letters and then a number for most BCM resources.

Quoting David Dai (2019-01-11 16:56:14)

Quoted 8 lines · Stephen Boyd

Quoted 7 lines

Quoting David Dai (2018-12-13 18:35:04)

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

Quoted 5 lines

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

+
+#define BCM_TCS_CMD(valid, vote)                               \
+       (BCM_TCS_CMD_COMMIT_MASK |                              \
+       ((valid) << BCM_TCS_CMD_VALID_SHIFT) |                  \
+       ((cpu_to_le32(vote) &                                   \

Why?

Quoted 3 lines

Sorry, could you clarify this question? If you're referring to the cpu_to_le32, shouldn't we be explicit about converting endianness even if it might be redundant for this particular qcom platform?

Is only the vote part of the message in little endian format and the rest is native CPU endianess? It's very odd to see that jammed in the middle of a bit packing statement like that. Typically the whole u32 would be in one or the other endianness. Also, sparse right complains about this macro and it's usage, so something is wrong.

I think one other problem is that rpmh API doesn't really talk about endianness here and that's busted. So if you want to fix endianness issues that needs to be fixed first.

Quoted 8 lines

Quoted 7 lines

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

Quoted 5 lines

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

@@ -29,6 +54,7 @@
    * @aggr_state:                rpmh clock aggregated state
    * @last_sent_aggr_state: rpmh clock last aggr state sent to RPMh
    * @valid_state_mask:  mask to determine the state of the rpmh clock
+ * @aux_data:          data specific to the bcm rpmh resource

But there isn't aux_data. Is this supposed to be unit? And what is unit going to be? 1000?

Supposed to be unit, the value is on the order of Khz.

Ok, hopefully the kernel-doc comes out easy to understand.

Quoted 8 lines

Quoted 6 lines

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

Quoted 5 lines

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

@@ -217,6 +340,7 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
   DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1", 1);
   DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2", 1);
   DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3", 1);
+DEFINE_CLK_RPMH_BCM(sdm845, ipa, "IP0");

It's really IP0?! What was wrong with IPA?

Yeah... convention seems to be 2 letters and then a number for most BCM resources.

OK.

Quoted 9 lines

Quoting David Dai (2019-01-11 16:56:14)

Quoted 8 lines · Stephen Boyd

Quoted 7 lines

Quoting David Dai (2018-12-13 18:35:04)

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

Quoted 5 lines

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

+
+#define BCM_TCS_CMD(valid, vote)                               \
+       (BCM_TCS_CMD_COMMIT_MASK |                              \
+       ((valid) << BCM_TCS_CMD_VALID_SHIFT) |                  \
+       ((cpu_to_le32(vote) &                                   \

Why?

Quoted 8 lines

Quoted 3 lines

Sorry, could you clarify this question? If you're referring to the cpu_to_le32, shouldn't we be explicit about converting endianness even if it might be redundant for this particular qcom platform?

Is only the vote part of the message in little endian format and the rest is native CPU endianess? It's very odd to see that jammed in the middle of a bit packing statement like that. Typically the whole u32 would be in one or the other endianness. Also, sparse right complains about this macro and it's usage, so something is wrong.

Point taken, I'll leave it out of the macro for now.

Quoted 3 lines

I think one other problem is that rpmh API doesn't really talk about endianness here and that's busted. So if you want to fix endianness issues that needs to be fixed first.

Since a similar macro is being used as part of the interconnect provider driver, I was thinking a better place for this macro might be in the tcs.h as part of the rpmh driver? I could submit a different patch for rpmh that mentions endianness along with this change.