Message-ID: <8a813713-c60d-4726-0c62-de032db99ede@collabora.com>
Date:   Mon, 27 Feb 2023 14:06:14 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        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

Il 27/02/23 13:01, Dmitry Baryshkov ha scritto:
> 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.
> 

This is handled by the SAW/SPM driver that we have in the kernel for all mentioned
platforms (soc/qcom/spm.c)

>>   - CPR-Hardened (voltage control, mandatory for stability)
> 
> This driver (nit: 8996 has cpr3)
> 

Yes, sorry, 8996 is not CPRh.

>>   - 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).
> 

This is valid for all targets having OSM.. and I think you're right, the only ones
that do have OSM are CPR-Hardened as the introduction of OSM is part of the actual
hardening of CPR4.

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

Right! I even forgot about that :-)

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

Exactly, I thought you were suggesting to drop it - having a more careful read,
you said "for now", so, eh, that was misinterpretation on my side, sorry!

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

Even though my memories are a bit confused (changeset too old...), should be used
only for CPR-Hardened, not for CPR3, not for CPR4.

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

When I engineered that solution, I kept the cpufreq-hw reading *again* the values
from OSM to keep the driver *fully* compatible with the bootloader-programmed OSM
flow, which makes one thing (in my opinion) perfectly clear: that programming
sequence is exactly the same as what happens "under the hood" on SDM845 (and later)
but performed here-instead-of-there (linux instead of bootloader), with the actual
scaling driver being 100% the same between the two flows in the end.

Having two drivers as you suggested would indeed achieve the same, but wouldn't be
any easier... if you do that, you'd have to *somehow* make sure that the
programming driver does its job before the cpufreq driver tries to read the OSM
status, adding one more link to an already long chain.

Besides, I remember that this question got asked a while ago on the mailing lists
and there was a short discussion about it:

https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2555580.html

Powered by blists - more mailing lists