Messages in this thread | | | Date | Mon, 27 Feb 2023 14:01:15 +0200 | Subject | Re: [PATCH v10 5/6] soc: qcom: Add support for Core Power Reduction v3, v4 and Hardened | From | Dmitry Baryshkov <> |
| |
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@somainline.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@somainline.org> >>> [Konrad: rebase, apply review comments] >>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.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@somainline.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
| |