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

[Konrad Dybcio]

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@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>
>>> ---
[...]

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