Re: [PATCH RFT 03/20] clk: qcom: smd-rpm: Add support for keepalive votes

[Dmitry Baryshkov]

On Sat, 4 Mar 2023 at 15:27, Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote:
>
> Some bus clock should always have a minimum (19.2 MHz) vote cast on
> them, otherwise the platform will fall apart, hang and reboot.
>
> Add support for specifying which clocks should be kept alive and
> always keep a vote on XO_A to make sure the clock tree doesn't
> collapse. This removes the need to keep a maximum vote that was
> previously guaranteed by clk_smd_rpm_handoff.
>
> This commit is a combination of existing (not-exactly-upstream) work
> by Taniya Das, Shawn Guo and myself.
>
> Co-developed-by: Shawn Guo <shawn.guo@xxxxxxxxxx>
> Co-developed-by: Taniya Das <quic_tdas@xxxxxxxxxxx>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> ---
>  drivers/clk/qcom/clk-smd-rpm.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index cce7daa97c1e..8e017c575361 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -4,6 +4,7 @@
>   * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>   */
>
> +#include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/err.h>
>  #include <linux/export.h>
> @@ -178,6 +179,8 @@ struct clk_smd_rpm_req {
>  struct rpm_smd_clk_desc {
>         struct clk_smd_rpm **clks;
>         size_t num_clks;
> +       struct clk_hw **keepalive_clks;
> +       size_t num_keepalive_clks;
>  };
>
>  static DEFINE_MUTEX(rpm_smd_clk_lock);
> @@ -1278,6 +1281,7 @@ static int rpm_smd_clk_probe(struct platform_device *pdev)
>         struct qcom_smd_rpm *rpm;
>         struct clk_smd_rpm **rpm_smd_clks;
>         const struct rpm_smd_clk_desc *desc;
> +       struct clk_hw **keepalive_clks;
>
>         rpm = dev_get_drvdata(pdev->dev.parent);
>         if (!rpm) {
> @@ -1291,6 +1295,7 @@ static int rpm_smd_clk_probe(struct platform_device *pdev)
>
>         rpm_smd_clks = desc->clks;
>         num_clks = desc->num_clks;
> +       keepalive_clks = desc->keepalive_clks;
>
>         for (i = 0; i < num_clks; i++) {
>                 if (!rpm_smd_clks[i])
> @@ -1321,6 +1326,24 @@ static int rpm_smd_clk_probe(struct platform_device *pdev)
>         if (ret)
>                 goto err;
>
> +       /* Leave a permanent active vote on clocks that require it. */
> +       for (i = 0; i < desc->num_keepalive_clks; i++) {
> +               if (WARN_ON(!keepalive_clks[i]))
> +                       continue;
> +
> +               ret = clk_prepare_enable(keepalive_clks[i]->clk);
> +               if (ret)
> +                       return ret;

Would it be better to use CLK_IS_CRITICAL instead? Using the existing
API has a bonus that it is more visible compared to the ad-hoc
solutions.

> +
> +               ret = clk_set_rate(keepalive_clks[i]->clk, 19200000);

Don't we also need to provide a determine_rate() that will not allow
one to set clock frequency below 19.2 MHz?

> +               if (ret)
> +                       return ret;
> +       }
> +
> +       /* Keep an active vote on CXO in case no other driver votes for it. */
> +       if (rpm_smd_clks[RPM_SMD_XO_A_CLK_SRC])
> +               return clk_prepare_enable(rpm_smd_clks[RPM_SMD_XO_A_CLK_SRC]->hw.clk);
> +
>         return 0;
>  err:
>         dev_err(&pdev->dev, "Error registering SMD clock driver (%d)\n", ret);


I have mixed feelings towards this patch (and the rest of the
patchset). It looks to me like we are trying to patch an issue of the
interconnect drivers (or in kernel configuration).


--
With best wishes
Dmitry