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@xxxxxxxxxxxxxx>
>> ---
>> 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;
>>
