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

[Stephen Boyd <sboyd@kernel.org>]

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