On 6.03.2023 02:21, Dmitry Baryshkov wrote:
> 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.
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
