Re: [PATCH] clk: qcom: smd: Disable unused clocks

[Jeffrey Hugo <jeffrey.l.hugo@gmail.com>]

On Mon, Aug 17, 2020 at 8:10 AM Stephan Gerhold <stephan@gerhold.net> wrote:
>
> At the moment, clk-smd-rpm forces all clocks on at probe time
> (for "handoff"). However, it does not make the clk core aware of that.
> This means that the clocks stay enabled forever if they are not used
> by anything.
>
> We can easily disable them again after bootup has been completed,
> by making the clk core aware of the state. This is implemented by
> returning the current state of the clock in is_prepared().
>
> Checking the SPMI clock registers reveals that this allows the RPM
> to disable unused BB/RF clocks. For example, on MSM8916 with all
> remote processors (except RPM) disabled, we get:
>
>  +--------------------------+------------+---------+--------+-------+
>  |                          | BOOTLOADER | HANDOFF | BEFORE | AFTER |
>  +--------------------------+------------+---------+--------+-------+
>  | BB_CLK1_STATUS1 (0x5108) |     ON*    |    ON   |   ON   |  ON*  |
>  | BB_CLK2_STATUS1 (0x5208) |     OFF    |    ON   |   ON   |  OFF  |
>  | RF_CLK1_STATUS1 (0x5408) |     OFF    |    ON   |   ON   |  OFF  |
>  | RF_CLK2_STATUS1 (0x5508) |     OFF    |    ON   |   ON   |  OFF  |
>  +--------------------------+------------+---------+--------+-------+
>   * BB_CLK1 seems to be always-on in RPM on MSM8916
>
> where:
>   - BOOTLOADER = clk-smd-rpm disabled entirely in device tree
>   - HANDOFF = temporarily after clk-smd-rpm was probed
>   - BEFORE/AFTER = after boot without/with the changes in this commit
>
> With this commit BB_CLK2/RF_CLK1/RF_CLK2 are disabled again when unused.
>
> Cc: Georgi Djakov <georgi.djakov@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> Originally I reported this here:
> https://lore.kernel.org/linux-arm-msm/20200523120810.GA166540@gerhold.net/
>
> Overall I'm not entirely sure why we need to force all these clocks
> on at all... But the downstream driver also seems to do it and the RPM
> interface is barely documented, so I didn't feel comfortable changing it...

So essentially, when the clk framework goes through late init, and
decides to turn off clocks that are not being used, it will also turn
off these clocks?

I think this is going to break other targets where other subsystems
happen to rely on these sorts of votes from Linux inorder to run/boot
(not saying it's a good thing, just that is how it is and since we
can't change the FW on those....).

I think this needs to be validated on every single qcom platform using
this driver.

Also, out of curiosity, how are you validating that BB_CLK2 is
actually off after this change?

> ---
>  drivers/clk/qcom/clk-smd-rpm.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index 0e1dfa89489e..fa960cb794a8 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -171,6 +171,9 @@ static int clk_smd_rpm_handoff(struct clk_smd_rpm *r)
>         if (ret)
>                 return ret;
>
> +       /* During handoff we force all clocks on */
> +       r->enabled = true;
> +
>         return 0;
>  }
>
> @@ -300,6 +303,13 @@ static void clk_smd_rpm_unprepare(struct clk_hw *hw)
>         mutex_unlock(&rpm_smd_clk_lock);
>  }
>
> +static int clk_smd_rpm_is_prepared(struct clk_hw *hw)
> +{
> +       struct clk_smd_rpm *r = to_clk_smd_rpm(hw);
> +
> +       return r->enabled;
> +}
> +
>  static int clk_smd_rpm_set_rate(struct clk_hw *hw, unsigned long rate,
>                                 unsigned long parent_rate)
>  {
> @@ -396,6 +406,7 @@ static int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm)
>  static const struct clk_ops clk_smd_rpm_ops = {
>         .prepare        = clk_smd_rpm_prepare,
>         .unprepare      = clk_smd_rpm_unprepare,
> +       .is_prepared    = clk_smd_rpm_is_prepared,
>         .set_rate       = clk_smd_rpm_set_rate,
>         .round_rate     = clk_smd_rpm_round_rate,
>         .recalc_rate    = clk_smd_rpm_recalc_rate,
> @@ -404,6 +415,7 @@ static const struct clk_ops clk_smd_rpm_ops = {
>  static const struct clk_ops clk_smd_rpm_branch_ops = {
>         .prepare        = clk_smd_rpm_prepare,
>         .unprepare      = clk_smd_rpm_unprepare,
> +       .is_prepared    = clk_smd_rpm_is_prepared,
>  };
>
>  /* msm8916 */
> --
> 2.28.0
>