Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
From: Stephen Boyd
Date: Tue Dec 04 2018 - 14:24:19 EST
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@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;
>