Date:   Tue, 4 Dec 2018 18:01:49 -0800
From:   David Dai <daidavid1@...eaurora.org>
To:     Stephen Boyd <sboyd@...nel.org>, linux-arm-msm@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     georgi.djakov@...aro.org, bjorn.andersson@...aro.org,
        evgreen@...gle.com, tdas@...eaurora.org, elder@...aro.org
Subject: Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

Thanks for the quick feedback!


On 12/4/2018 11:24 AM, 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?
>
>> Signed-off-by: David Dai <daidavid1@...eaurora.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.
Ok.
>
>> +       ((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.
Ok.
>> +struct bcm_db {
>> +       u32 unit;
>> +       u16 width;
>> +       u8 vcd;
>> +       u8 reserved;
> These would need __le32 and __le16 for the byte swap.
Ok.
>> +};
>> +
>>   /**
>>    * 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.
Ok.
>> +
>> +       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?

Since the BCM only takes in numerical values as part of its msg (as 
opposed to enable or disable of some sort), this is so that we don't 
actually enable the clock if it hasn't been prepared yet. Sending a 
value of zero vs a non-zero would be the equivalent turning the clocks 
on and off and the behavior I'm trying to emulate here is to only change 
the rate in the system if the clock is on.
>
>> +               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?
Makes sense, done.
>
>> +               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?
The unit is in order of Khz, I can do the conversion for KHz to Hz the 
time when we fetch the aux data.
>
>> +}
>> +
>> +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.
Ok.
>> +               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.

Sounds reasonable, the only thing we need from the aux_data is just the 
unit in this case.

>> +                                                  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.
Ok.
>>                  rpmh_clk->res_addr += res_addr;
>>                  rpmh_clk->dev = &pdev->dev;
>>   

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Powered by blists - more mailing lists