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

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(+)

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.

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

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

On 6.03.2023 12:28, Konrad Dybcio wrote:
> 
> 
> 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>
>>> ---
[...]

>>
>>> +
>>> +               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..
Thinking about it again, I'd have to do it before the clocks are registered
and we'd either end up with 2 loops, one assigning the CLK_IS_CRITICAL flag
and the other one setting the rate.. Will that not be too hacky?

Konrad

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