Re: [PATCH v10 5/6] soc: qcom: Add support for Core Power Reduction v3, v4 and Hardened

[AngeloGioacchino Del Regno]

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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > [Konrad: rebase, apply review comments]
> > Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxx>
> > + */
> > +
> > +#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.

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)
 - CPR-Hardened (voltage control, mandatory for stability)
 - OSM (for cpufreq-hw frequency steps [1..N])

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.

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.

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__ */