Quoting David Dai (2018-12-03 19:50:13)Ok.
Add IPA clock support by extending the current clk rpmh driver to supportYes, but why? Does the IPA driver need to set clk rates and that somehow
clocks that are managed by a different type of RPMh resource known as
Bus Clock Manager(BCM).
doesn't work as a bandwidth request?
Signed-off-by: David Dai <daidavid1@xxxxxxxxxxxxxx>Nitpick: Add space before the \ and align them all up on the right side
---
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 |\
of the page.
Ok.
+ ((valid) << BCM_TCS_CMD_VALID_SHIFT) |\Nitpick: Please remove the newline between comment and struct.
+ ((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
+ */
+
Ok.+struct bcm_db {These would need __le32 and __le16 for the byte swap.
+ u32 unit;
+ u16 width;
+ u8 vcd;
+ u8 reserved;
Ok.+};Don't initialize variables and then reassign them right after.
+
/**
* 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;
+Why do we need to check is_prepared? Add a comment indicating we can't
+ 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)) {
send the request when the clk is disabled?
Makes sense, done.
+ mutex_lock(&rpmh_clk_lock);This function is always inside mutex_lock()/unlock() pair, so push the
+ clk_rpmh_bcm_send_cmd(c, true);
lock into the function?
The unit is in order of Khz, I can do the conversion for KHz to Hz the time when we fetch the aux data.
+ mutex_unlock(&rpmh_clk_lock);What is 1000 for?
+ }
+
+ 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;
Ok.
+}Nitpick: Please add newline here.
+
+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;
}
+ aux_data_len = cmd_db_read_aux_data_len(rpmh_clk->res_name);Is the cast necessary? And I would just pick out the data you need from
+ if (aux_data_len == sizeof(struct bcm_db)) {
+ ret = cmd_db_read_aux_data(rpmh_clk->res_name,
+ (u8 *)&rpmh_clk->aux_data,
the aux_data instead of storing it forever (with the padding) in the
rpmh_clk structure.
Ok.+ sizeof(struct bcm_db));Nitpick: Newline here too.
+ 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;