Message-ID: <d2784517-0f0c-43a5-63a6-57f6aa3e5912@linaro.org>
Date:   Mon, 27 Feb 2023 14:01:15 +0200
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>,
        Niklas Cassel <nks@...wful.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>
Cc:     Robert Marko <robimarko@...il.com>, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-pm@...r.kernel.org,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...ainline.org>
Subject: Re: [PATCH v10 5/6] soc: qcom: Add support for Core Power Reduction
 v3, v4 and Hardened

On 27/02/2023 11:13, AngeloGioacchino Del Regno wrote:
> Il 27/02/23 03:55, Dmitry Baryshkov ha scritto:
>> On 17/02/2023 13:08, Konrad Dybcio wrote:
>>> From: AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@...ainline.org>
>>>
>>> This commit introduces a new driver, based on the one for cpr v1,
>>> to enable support for the newer Qualcomm Core Power Reduction
>>> hardware, known downstream as CPR3, CPR4 and CPRh, and support
>>> for MSM8998 and SDM630 CPU power reduction.
>>>
>>> In these new versions of the hardware, support for various new
>>> features was introduced, including voltage reduction for the GPU,
>>> security hardening and a new way of controlling CPU DVFS,
>>> consisting in internal communication between microcontrollers,
>>> specifically the CPR-Hardened and the Operating State Manager.
>>>
>>> The CPR v3, v4 and CPRh are present in a broad range of SoCs,
>>> from the mid-range to the high end ones including, but not limited
>>> to, MSM8953/8996/8998, SDM630/636/660/845.
>>>
>>> Signed-off-by: AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@...ainline.org>
>>> [Konrad: rebase, apply review comments]
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
>>> ---
>>>   drivers/soc/qcom/Kconfig      |   22 +
>>>   drivers/soc/qcom/Makefile     |    4 +-
>>>   drivers/soc/qcom/cpr-common.h |    2 +
>>>   drivers/soc/qcom/cpr3.c       | 2923 
>>> +++++++++++++++++++++++++++++++++++++++++
>>>   include/soc/qcom/cpr.h        |   17 +
>>>   5 files changed, 2967 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/soc/qcom/cpr.h b/include/soc/qcom/cpr.h
>>> new file mode 100644
>>> index 000000000000..2ba4324d18f6
>>> --- /dev/null
>>> +++ b/include/soc/qcom/cpr.h
>>> @@ -0,0 +1,17 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Copyright (c) 2013-2020, The Linux Foundation. All rights reserved.
>>> + * Copyright (c) 2019 Linaro Limited
>>> + * Copyright (c) 2021, AngeloGioacchino Del Regno
>>> + *                     <angelogioacchino.delregno@...ainline.org>
>>> + */
>>> +
>>> +#ifndef __CPR_H__
>>> +#define __CPR_H__
>>> +
>>> +struct cpr_ext_data {
>>> +    int mem_acc_threshold_uV;
>>> +    int apm_threshold_uV;
>>> +};
>>
>> Who is going to use this? Is it the cpufreq driver or some other driver?
>> We are adding an API without a clean user, can we drop it for now?
>>
> 
> This is mandatory: qcom-cpufreq-hw is supposed to program the OSM before
> starting.

Thanks for the explanation!

> 
>  From SDM845 onwards, the OSM is programmed by the bootloader before 
> booting
> Linux;
> In MSM8996/98, SDM630/636/660, others, the bootloader does not program 
> the OSM
> uC, so this has to be done in Linux - specifically, in the CPUFREQ driver
> (qcom-cpufreq-hw), otherwise this driver is completely pointless to have.
> 
> CPU DVFS requires three uC to be correctly programmed in order to work:
>   - SAW (for sleep states)

I believe this is handled by the PCSI for all mentioned platforms.

>   - CPR-Hardened (voltage control, mandatory for stability)

This driver (nit: 8996 has cpr3)

>   - OSM (for cpufreq-hw frequency steps [1..N])

I think this is valid only for the CPRh targets. And for OSM programming 
the driver populates OPP tables with voltage levels (which should then 
be handled by the cpufreq-hw).

I'd toss another coin into the list: for 8996 we also have to program 
APM and cluster (kryo) regulators _manually_.

> 
> Failing to *correctly* program either of the three will render CPU DVFS 
> unusable.
> 
> 
> That clarified, my opinion is:
> No, you can't drop this. It's an essential piece for functionality.
> 
> I agree in that this commit introduces a header that has only an 
> internal (as in
> cpr3.c) user and no external ones, but I think that Konrad didn't want 
> to include
> the qcom-cpufreq-hw.c commits in this series because it's already huge 
> and pretty
> difficult to review; adding the cpufreq-hw commits would make the 
> situation worse.

Perhaps we misunderstand each other here. I suggest dropping the header 
from _this_ patchset only and submit/merge corresponding code together 
with the cpufreq-hw changes. This might sound like a complication, but 
in reality it allows one to assess corresponding code separately.

(Moreover, please correct me if I'm wrong, I think this header will be 
used only with the CPRh, and so this has no use for CPR3/4. Is this 
correct?)

I took a glance at the 'cpufreq: qcom-hw: Implement CPRh aware OSM 
programming' patch, it doesn't seem to use the header (maybe I checked 
the older version of the patch). As for me, this is another signal that 
cpr_ext_data should come together with the LUT programming rather than 
with the CPRh itself.

> Konrad, perhaps you can send the cpufreq-hw commits in a separate 
> series, in
> which cover letter you mention a dependency on this one?
> That would *clearly* show the full picture to reviewers.

Yes, that would be great. A small note regarding those patches. I see 
that you patched the qcom-cpufreq-hw.c. This way first the driver 
programs the LUT, then it reads it back to setup the OPPs. Would it be 
easier to split OSM-not-programmed driver?

> 
> I remember that when I sent the cpufreq-hw series along with this one 
> (~2 years
> ago, I think?) that code had positive reviews from Bjorn, so it should 
> be OK.
> It wasn't picked just-only-because of the cpr3 dependency.
> 
> Regards,
> Angelo
> 
>>> +
>>> +#endif /* __CPR_H__ */
>>>
>>
> 
> 
> 

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists