Re: [PATCH 06/13] clk: qcom: cpu-8996: simplify the cpu_clk_notifier_cb

[Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>]

On 11.01.2023 23:01, Dmitry Baryshkov wrote:
> On 11/01/2023 23:03, Konrad Dybcio wrote:
>>
>>
>> On 11.01.2023 20:19, Dmitry Baryshkov wrote:
>>> - Do not use the Alt PLL completely. Switch to smux when necessary to
>>>    prevent overvolting
>> Is this empirical evidence, or did Qualcomm recommendations change since
>> msm-3.18 was released?
> 
> I think this is what they are doing, see https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.7.5.r1-05300-8x96.0/drivers/clk/msm/clock-cpu-8996.c#L675
> 
> They switch altpll frequency for whatever reasons, then they do the dance of switching the parent rate to half rate if necessary and then they finally switch the parent's rate to the target rate. That's the only way I can interpret the cpu_clk_8996_set_rate().
> 
> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.7.5.r1-05300-8x96.0/drivers/clk/msm/clock-cpu-8996.c#L675

Okay, so on rate switch they:

PWR:
  - set (rate > 1190400 kHz ? 556800000 : 307200000) on ALT_PLL
  - use_alt_pll is 0 because it's an uninitialized global var
    - do_half_rate is TRUE on PWRCL_CLK
    - " /* Special handling needed " path is entered if the frequency
      is being requested to go from above 600 MHz to below 600 MHz
    - clk_set_rate sets OLD_RATE/2 on pwrcl_hf_mux which
      has 2 parents:
        * pwrcl_pll for 600 MHz - 3000 MHz
        * pwrcl_lf_mux for anything else fed by:
           * pwrcl_pll_main = pwrcl_pll/2 (300 MHz - 600 MHz???)
           * sys_apcsaux_clk (SMUX) (<300 MHz??)

PERF:
  - use_alt_pll is 0 because it's an uninitialized global var
    - do_half_rate is TRUE on PERFCL_CLK
    - " /* Special handling needed " path is entered if the frequency
      is being requested to go from above 600 MHz to below 600 MHz
    - clk_set_rate sets OLD_RATE/2 (/4 on Pro) on perfcl_hf_mux which
      has 2 parents:
        * perfcl_pll for 600 MHz - 3000 MHz
        * perfcl_lf_mux for anything else fed by:
           * perfcl_pll_main = perfcl_pll/2 (300 MHz - 600 MHz???)
           * sys_apcsaux_clk (SMUX) (<300 MHz??)

So I think the alt_plls are NOT used and this is correct..

Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>

Konrad

> 
>>
>>
>>> - Restore the parent in case the rate change aborts for some reason
>>> - Do not duplicate resetting the parent in set_parent operation.
>> These sound good.
>>
>> Konrad
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>>> ---
>>>   drivers/clk/qcom/clk-cpu-8996.c | 31 +++++++++++++++++++------------
>>>   1 file changed, 19 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/clk/qcom/clk-cpu-8996.c b/drivers/clk/qcom/clk-cpu-8996.c
>>> index 7e5246ca7e7f..ee7e18b37832 100644
>>> --- a/drivers/clk/qcom/clk-cpu-8996.c
>>> +++ b/drivers/clk/qcom/clk-cpu-8996.c
>>> @@ -506,27 +506,34 @@ static int cpu_clk_notifier_cb(struct notifier_block *nb, unsigned long event,
>>>   {
>>>       struct clk_cpu_8996_pmux *cpuclk = to_clk_cpu_8996_pmux_nb(nb);
>>>       struct clk_notifier_data *cnd = data;
>>> -    int ret;
>>>         switch (event) {
>>>       case PRE_RATE_CHANGE:
>>> -        ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, ALT_INDEX);
>>>           qcom_cpu_clk_msm8996_acd_init(base);
>>> +
>>> +        /*
>>> +         * Avoid overvolting. clk_core_set_rate_nolock() walks from top
>>> +         * to bottom, so it will change the rate of the PLL before
>>> +         * chaging the parent of PMUX. This can result in pmux getting
>>> +         * clocked twice the expected rate.
>>> +         *
>>> +         * Manually switch to PLL/2 here.
>>> +         */
>>> +        if (cnd->new_rate < DIV_2_THRESHOLD &&
>>> +            cnd->old_rate > DIV_2_THRESHOLD)
>>> +            clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, SMUX_INDEX);
>>> +
>>>           break;
>>> -    case POST_RATE_CHANGE:
>>> -        if (cnd->new_rate < DIV_2_THRESHOLD)
>>> -            ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw,
>>> -                               SMUX_INDEX);
>>> -        else
>>> -            ret = clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw,
>>> -                               ACD_INDEX);
>>> -        break;
>>> +    case ABORT_RATE_CHANGE:
>>> +        /* Revert manual change */
>>> +        if (cnd->new_rate < DIV_2_THRESHOLD &&
>>> +            cnd->old_rate > DIV_2_THRESHOLD)
>>> +            clk_cpu_8996_pmux_set_parent(&cpuclk->clkr.hw, ACD_INDEX);
>>>       default:
>>> -        ret = 0;
>>>           break;
>>>       }
>>>   -    return notifier_from_errno(ret);
>>> +    return NOTIFY_OK;
>>>   };
>>>     static int qcom_cpu_clk_msm8996_driver_probe(struct platform_device *pdev)
>