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

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 6.03.2023 02:21, Dmitry Baryshkov wrote:
> On Sat, 4 Mar 2023 at 15:27, Konrad Dybcio <konrad.dybcio@linaro.org> 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@linaro.org>
>> Co-developed-by: Taniya Das <quic_tdas@quicinc.com>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  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.
Yeah, I think that makes sense.

> 
>> +
>> +               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?
Hm, sounds like a good idea..

> 
>> +               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).
Well, as you noticed, this patch tries to address a situation where a
critical clock could be disabled. The interconnect driver (as per my
other recent patchset) also has a concept of "keepalive", but:

1. not very many SoCs already have a functional icc driver
2. devices with an existing interconnect driver should also be
   testable without one (through painful ripping out everything-icc
   from the dts) for regression tracking

Konrad

> 
> 
> --
> With best wishes
> Dmitry