[RFC PATCH] Add IPA clock support for clk-rpmh

[RFC PATCH] Add IPA clock support for clk-rpmh

[David Dai]

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.

David Dai (1):
  clk: qcom: clk-rpmh: Add IPA clock support

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

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

[David Dai]

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 <daidavid1@codeaurora.org>
---
 drivers/clk/qcom/clk-rpmh.c           | 142 ++++++++++++++++++++++++++++++++++
 include/dt-bindings/clock/qcom,rpmh.h |   1 +
 2 files changed, 143 insertions(+)

diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
index 9f4fc77..42e2cd2 100644
--- a/drivers/clk/qcom/clk-rpmh.c
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -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 |\
+	((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
+ */
+
+struct bcm_db {
+	u32 unit;
+	u16 width;
+	u8 vcd;
+	u8 reserved;
+};
+
 /**
  * struct clk_rpmh - individual rpmh clock data structure
  * @hw:			handle between common and hardware-specific interfaces
@@ -29,6 +55,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
  * @dev:		device to which it is attached
  * @peer:		pointer to the clock rpmh sibling
  */
@@ -42,6 +69,7 @@ struct clk_rpmh {
 	u32 aggr_state;
 	u32 last_sent_aggr_state;
 	u32 valid_state_mask;
+	struct bcm_db aux_data;
 	struct device *dev;
 	struct clk_rpmh *peer;
 };
@@ -98,6 +126,17 @@ struct clk_rpmh_desc {
 	__DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name,	\
 			  CLK_RPMH_VRM_EN_OFFSET, 1, _div)
 
+#define DEFINE_CLK_RPMH_BCM(_platform, _name, _res_name)		\
+	static struct clk_rpmh _platform##_##_name = {			\
+		.res_name = _res_name,					\
+		.valid_state_mask = BIT(RPMH_ACTIVE_ONLY_STATE),	\
+		.div = 1,						\
+		.hw.init = &(struct clk_init_data){			\
+			.ops = &clk_rpmh_bcm_ops,			\
+			.name = #_name,					\
+		},							\
+	}
+
 static inline struct clk_rpmh *to_clk_rpmh(struct clk_hw *_hw)
 {
 	return container_of(_hw, struct clk_rpmh, hw);
@@ -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;
+
+	return 0;
+}
+
+static int clk_rpmh_bcm_prepare(struct clk_hw *hw)
+{
+	struct clk_rpmh *c = to_clk_rpmh(hw);
+	int ret = 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)
+{
+	struct clk_rpmh *c = to_clk_rpmh(hw);
+
+	c->aggr_state = rate / (c->aux_data.unit * 1000);
+
+	if (clk_hw_is_prepared(hw)) {
+		mutex_lock(&rpmh_clk_lock);
+		clk_rpmh_bcm_send_cmd(c, true);
+		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;
+}
+
+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;
+}
+
+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);
@@ -217,6 +341,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");
 
 static struct clk_hw *sdm845_rpmh_clocks[] = {
 	[RPMH_CXO_CLK]		= &sdm845_bi_tcxo.hw,
@@ -231,6 +356,7 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
 	[RPMH_RF_CLK2_A]	= &sdm845_rf_clk2_ao.hw,
 	[RPMH_RF_CLK3]		= &sdm845_rf_clk3.hw,
 	[RPMH_RF_CLK3_A]	= &sdm845_rf_clk3_ao.hw,
+	[RPMH_IPA_CLK]		= &sdm845_ipa.hw,
 };
 
 static const struct clk_rpmh_desc clk_rpmh_sdm845 = {
@@ -267,6 +393,7 @@ static int clk_rpmh_probe(struct platform_device *pdev)
 
 	for (i = 0; i < desc->num_clks; i++) {
 		u32 res_addr;
+		u32 aux_data_len;
 
 		rpmh_clk = to_clk_rpmh(hw_clks[i]);
 		res_addr = cmd_db_read_addr(rpmh_clk->res_name);
@@ -275,6 +402,21 @@ static int clk_rpmh_probe(struct platform_device *pdev)
 				rpmh_clk->res_name);
 			return -ENODEV;
 		}
+		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,
+						   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);
+		}
 		rpmh_clk->res_addr += res_addr;
 		rpmh_clk->dev = &pdev->dev;
 
diff --git a/include/dt-bindings/clock/qcom,rpmh.h b/include/dt-bindings/clock/qcom,rpmh.h
index f48fbd6..edcab3f 100644
--- a/include/dt-bindings/clock/qcom,rpmh.h
+++ b/include/dt-bindings/clock/qcom,rpmh.h
@@ -18,5 +18,6 @@
 #define RPMH_RF_CLK2_A				9
 #define RPMH_RF_CLK3				10
 #define RPMH_RF_CLK3_A				11
+#define RPMH_IPA_CLK				12
 
 #endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

[Stephen Boyd]

Quoting David Dai (2018-12-03 19:50:13)
> 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?

> 
> Signed-off-by: David Dai <daidavid1@codeaurora.org>
> ---
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> index 9f4fc77..42e2cd2 100644
> --- a/drivers/clk/qcom/clk-rpmh.c
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -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.

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

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

These would need __le32 and __le16 for the byte swap.

> +};
> +
>  /**
>   * 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;
> +
> +       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.

> +
> +       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)
> +{
> +       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?

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

> +               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;
> +}
> +
> +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?

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

Nitpick: Please add newline here.

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

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

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

Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

[Stephen Boyd]

Quoting David Dai (2018-12-03 19:50:13)
> 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?

> 
> Signed-off-by: David Dai <daidavid1@codeaurora.org>
> ---
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> index 9f4fc77..42e2cd2 100644
> --- a/drivers/clk/qcom/clk-rpmh.c
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -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.

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

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

These would need __le32 and __le16 for the byte swap.

> +};
> +
>  /**
>   * 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;
> +
> +       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.

> +
> +       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)
> +{
> +       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?

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

> +               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;
> +}
> +
> +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?

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

Nitpick: Please add newline here.

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

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

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

Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

[Alex Elder]

On 12/4/18 1:24 PM, Stephen Boyd wrote:
> Quoting David Dai (2018-12-03 19:50:13)
>> 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?

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.

					-Alex

>> Signed-off-by: David Dai <daidavid1@codeaurora.org>
>> ---
>> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
>> index 9f4fc77..42e2cd2 100644
>> --- a/drivers/clk/qcom/clk-rpmh.c
>> +++ b/drivers/clk/qcom/clk-rpmh.c
>> @@ -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.
> 
>> +       ((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.
> 
>> +struct bcm_db {
>> +       u32 unit;
>> +       u16 width;
>> +       u8 vcd;
>> +       u8 reserved;
> 
> These would need __le32 and __le16 for the byte swap.
> 
>> +};
>> +
>>  /**
>>   * 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;
>> +
>> +       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.
> 
>> +
>> +       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)
>> +{
>> +       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?
> 
>> +               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?
> 
>> +               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;
>> +}
>> +
>> +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?
> 
>> +}
>> +
>> +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);
>> @@ -275,6 +402,21 @@ static int clk_rpmh_probe(struct platform_device *pdev)
>>                                 rpmh_clk->res_name);
>>                         return -ENODEV;
>>                 }
> 
> Nitpick: Please add newline here.
> 
>> +               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.
> 
>> +                                                  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.
> 
>>                 rpmh_clk->res_addr += res_addr;
>>                 rpmh_clk->dev = &pdev->dev;
>>  

Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

[Stephen Boyd]

Quoting Alex Elder (2018-12-04 13:41:47)
> On 12/4/18 1:24 PM, Stephen Boyd wrote:
> > Quoting David Dai (2018-12-03 19:50:13)
> >> 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?
> 
> 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.

Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

[David Dai]

On 12/4/2018 2:34 PM, Stephen Boyd wrote:
> Quoting Alex Elder (2018-12-04 13:41:47)
>> On 12/4/18 1:24 PM, Stephen Boyd wrote:
>>> Quoting David Dai (2018-12-03 19:50:13)
>>>> 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?
>> 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?
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

>
> 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.
Understood, I'll be as detailed and as explicit as I can in the future.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

[Stephen Boyd]

Quoting David Dai (2018-12-04 17:14:10)
> 
> On 12/4/2018 2:34 PM, Stephen Boyd wrote:
> > Quoting Alex Elder (2018-12-04 13:41:47)
> >> On 12/4/18 1:24 PM, Stephen Boyd wrote:
> >>> Quoting David Dai (2018-12-03 19:50:13)
> >>>> 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?
> >> 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?
> 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?

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

Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

[David Dai]

On 12/4/2018 11:15 PM, Stephen Boyd wrote:
> Quoting David Dai (2018-12-04 17:14:10)
>> On 12/4/2018 2:34 PM, Stephen Boyd wrote:
>>> Quoting Alex Elder (2018-12-04 13:41:47)
>>>> On 12/4/18 1:24 PM, Stephen Boyd wrote:
>>>>> Quoting David Dai (2018-12-03 19:50:13)
>>>>>> 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?
>>>> 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?
>> 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?

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).

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

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

[Stephen Boyd]

Quoting David Dai (2018-12-05 17:24:18)
> 
> 
> On 12/4/2018 11:15 PM, Stephen Boyd wrote:
> > Quoting David Dai (2018-12-04 17:14:10)
> >> On 12/4/2018 2:34 PM, Stephen Boyd wrote:
> >>> Quoting Alex Elder (2018-12-04 13:41:47)
> >>>>
> >>> 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.
> > 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).
> 

Got it. Thanks!

Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

[Bjorn Andersson]

On Tue 04 Dec 23:15 PST 2018, Stephen Boyd wrote:

> Quoting David Dai (2018-12-04 17:14:10)
> > 
> > On 12/4/2018 2:34 PM, Stephen Boyd wrote:
> > > Quoting Alex Elder (2018-12-04 13:41:47)
> > >> On 12/4/18 1:24 PM, Stephen Boyd wrote:
> > >>> Quoting David Dai (2018-12-03 19:50:13)
> > >>>> 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?
> > >> 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?
> > 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?
> 

My objection to the use of msm_bus vs clock framework is not related to
how the actual interface of configuring the hardware looks like. It's
simply a matter of how this is represented in software, between some
provider and the IPA driver.

The IPA driver wants the IPA block to tick at 75MHz and I do not think
it's appropriate to achieve that by requesting a path between IPA Core
and IPA Core of 75000000 Kbytes/s.

Regards,
Bjorn

Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

[David Dai]

Thanks for the quick feedback!


On 12/4/2018 11:24 AM, Stephen Boyd wrote:
> Quoting David Dai (2018-12-03 19:50:13)
>> 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?
>
>> Signed-off-by: David Dai <daidavid1@codeaurora.org>
>> ---
>> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
>> index 9f4fc77..42e2cd2 100644
>> --- a/drivers/clk/qcom/clk-rpmh.c
>> +++ b/drivers/clk/qcom/clk-rpmh.c
>> @@ -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.
Ok.
>
>> +       ((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.
Ok.
>> +struct bcm_db {
>> +       u32 unit;
>> +       u16 width;
>> +       u8 vcd;
>> +       u8 reserved;
> These would need __le32 and __le16 for the byte swap.
Ok.
>> +};
>> +
>>   /**
>>    * 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;
>> +
>> +       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.
Ok.
>> +
>> +       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)
>> +{
>> +       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?

Since the BCM only takes in numerical values as part of its msg (as 
opposed to enable or disable of some sort), this is so that we don't 
actually enable the clock if it hasn't been prepared yet. Sending a 
value of zero vs a non-zero would be the equivalent turning the clocks 
on and off and the behavior I'm trying to emulate here is to only change 
the rate in the system if the clock is on.
>
>> +               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?
Makes sense, done.
>
>> +               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;
>> +}
>> +
>> +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?
The unit is in order of Khz, I can do the conversion for KHz to Hz the 
time when we fetch the aux data.
>
>> +}
>> +
>> +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);
>> @@ -275,6 +402,21 @@ static int clk_rpmh_probe(struct platform_device *pdev)
>>                                  rpmh_clk->res_name);
>>                          return -ENODEV;
>>                  }
> Nitpick: Please add newline here.
Ok.
>> +               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.

Sounds reasonable, the only thing we need from the aux_data is just the 
unit in this case.

>> +                                                  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.
Ok.
>>                  rpmh_clk->res_addr += res_addr;
>>                  rpmh_clk->dev = &pdev->dev;
>>   

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH] Add IPA clock support for clk-rpmh

[Stephen Boyd]

Quoting David Dai (2018-12-03 19:50:12)
> 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.

Please don't send cover letters for single patches.

Re: [RFC PATCH] Add IPA clock support for clk-rpmh

[Stephen Boyd]

Quoting David Dai (2018-12-03 19:50:12)
> 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.

Please don't send cover letters for single patches.