Re: [PATCH v8 0/5] Add support for Core Power Reduction v3, v4 and Hardened

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 11.01.2023 03:52, Dmitry Baryshkov wrote:
> On 10/01/2023 19:56, Konrad Dybcio wrote:
>> Changes in v8:
>> - Overtake this series from AGdR
>> - Apply all review comments from v7 except Vladimir's request to
>>    not create the include/ header; it will be strictly necessary for
>>    OSM-aware cpufreq_hw programming, which this series was more or
>>    less created just for..
>> - Drop QCS404 dtsi change, account for not breaking backwards compat
>>    in [3/5]
>> - Add type phandle type reference to acc-syscon in [1/5]
>> - Update AGdR's email addresses for maintainer entries
>> - Add [2/5] to make dt_binding_check happy
>> - Separate the CPRh DT addition from cpufreq_hw addition, sort and
>>    properly indent new nodes
>> - Drop CPR yaml conversion, that happened in meantime
>> - Reorder the patches to make a bit more sense
>> - Tested again on MSM8998 Xperia XZ Premium (Maple)
>> - I take no responsibility for AGdR's cheeky jokes, only the code!
>>
>> Link to v7:
>> https://lore.kernel.org/lkml/20210901155735.629282-1-angelogioacchino.delregno@somainline.org/
>>
>> Changes in v7:
>> - Rebased on linux-next as of 210901
>> - Changed cpr_read_efuse calls to nvmem_cell_read_variable_le_u32,
>>    following what was done in commit c77634b9d916
>>
>> Changes in v6:
>> - Fixes from Bjorn's review
>> - After a conversation with Viresh, it turned out I was abusing the
>>    OPP API to pass the APM and MEM-ACC thresholds to qcom-cpufreq-hw,
>>    so now the driver is using the genpd created virtual device and
>>    passing drvdata instead to stop the abuse
>> - Since the CPR commonization was ignored for more than 6 months,
>>    it is now included in the CPRv3/4/h series, as there is no point
>>    in commonizing without having this driver
>> - Rebased on v5.13
>>
>> Changes in v5:
>> - Fixed getting OPP table when not yet installed by the caller
>>    of power domain attachment
>>
>> Changes in v4:
>> - Huge patch series has been split for better reviewability,
>>    as suggested by Bjorn
>>
>> Changes in v3:
>> - Fixed YAML doc issues
>> - Removed unused variables and redundant if branch
>>
>> Changes in v2:
>> - Implemented dynamic Memory Accelerator corners support, needed
>>    by MSM8998
>> - Added MSM8998 Silver/Gold parameters
>>
>> 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.
>>
>> As to clarify, SDM845 does the CPR/SAW/OSM setup in TZ firmware, but
>> this is limited to the CPU context; despite GPU CPR support being not
>> implemented in this series, it is planned for the future, and some
>> SDM845 need the CPR (in the context of GPU CPR) to be configured from
>> this driver.
>>
>> It is also planned to add the CPR data for MSM8996, since this driver
>> does support the CPRv4 found on that SoC, but I currently have no time
>> to properly test that on a real device, so I prefer getting this big
>> implementation merged before adding more things on top.
> 
> MSM8996 is CPRv3, both CPU and GFX.
> 
> After hacking up 8996 I found it quite hard to retrofit it into this driver. Especially if we look at the GFX CPR (it uses multiple ROs, which this driver is not very well prepared for).
CPR(>3) has 16 ROs and that's taken care of with regards to programming
the Qualcomm-given values in cpr3_corner_init().

I have the feeling that this patchset/driver should concentrate on getting CPRh done. We can consolidate CPR3/CPR3-GFX/CPR4/CPRh after getting (some of) them done. In the least case I'd suggest using the per-generation functions that call each other rather than having generation-conditional code in a single big function.
In my opinion, they are not *that* different and the main changes from
3/4 to h are that some code paths are just skipped, as they're
partially handled in the secure world. The complexity of this hardware
pretty much requires gigantic functions, unless we want to split them
"by paragraph", which may not be much beneficial to readability if we
separate out functions that are only called once etc.

> 
> Some design questions: how do you handle the mem-acc programming?
There's a code path for setting memacc, involving cpr_set_acc() that
is not taken on either 630 or 8998 that are the initially-supported
SoCs, as they implement CPRh which mostly forbids memacc access.

My understanding is that for CPR3 it should be set on a thread-by-thread basis rather than having a single global setting.
cpr_set_acc() callers, cpr_{pre,post}_voltage() have the thread ID
passed to them, so if I understand correctly, this would just be
a matter of turning acc_desc->settings into an array of reg_sequence
arrays, instead of it being a 1d array like it is now?

Does CPRh need additional handling of APM or is it handled by the hardware?
Firmware*, AFAIU.

> 
> For the reference, on msm8996, CPR3 has two threads (one for power and one for performance clusters).
(Just like msm8998+CPRh)

And then the power thread should scale accordingly to both power cluster and the CBF (an interconnect between clusters).
If by "scale" you mean "work with scaling values provided", it's
already taken care of, check msm8998_{gold,silver}_scaling_factor.
> 
> And CPR3-GFX is a lightweight (or reduced) version of CPR3 (and of course just a single thread/single vreg).
For once something with less complexity ;)

Konrad

> 
> 
>>
>> As for MSM8953, we (read: nobody from SoMainline) have no device with
>> this chip: since we are unable to test the cpr data and the entire
>> driver on that one, we currently have no plans to do this addition
>> in the future. This is left to other nice developers: I'm sure that
>> somebody will come up with that, sooner or later ;)
>>
>> Tested on the following smartphones:
>> - Sony Xperia XA2        (SDM630)
>> - Sony Xperia XA2 Ultra  (SDM630)
>> - Sony Xperia 10         (SDM630)
>> - Sony Xperia XZ Premium (MSM8998)
>> - F(x)Tec Pro 1          (MSM8998)
>