From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Elder Subject: Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support Date: Tue, 4 Dec 2018 15:41:47 -0600 Message-ID: <8dafe631-4b16-94cd-392e-84728f2bb382@linaro.org> References: <1543895413-1553-1-git-send-email-daidavid1@codeaurora.org> <1543895413-1553-2-git-send-email-daidavid1@codeaurora.org> <154395145491.88331.1174781210192403998@swboyd.mtv.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <154395145491.88331.1174781210192403998@swboyd.mtv.corp.google.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Stephen Boyd , David Dai , linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org Cc: georgi.djakov@linaro.org, bjorn.andersson@linaro.org, evgreen@google.com, tdas@codeaurora.org List-Id: linux-arm-msm@vger.kernel.org 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 >> --- >> 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; >>