Re: [PATCH] arch: arm64: dts: apq8016-dbc: Add missing cpu opps

[Loic Poulain <loic.poulain@linaro.org>]

Hi Stephan,

On Thu, 2 Apr 2020 at 10:14, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> Hi,
>
> On Wed, Apr 01, 2020 at 07:50:59PM +0200, Loic Poulain wrote:
> > The highest cpu frequency opps have been dropped because CPR is not
> > supported. However, we can simply specify operating voltage so that
> > they match the max corner voltages for each freq. With that, we can
> > support up to 1.36Ghz. Ideally, msm8916 CPR should be implemented to
> > fine tune operating voltages and optimize power consumption.
>
> Thanks for the patch! I was wondering how to enable the higher CPU
> frequencies for a while now...
>
> I was actually quite excited to see CPR being mainlined for QCS404.
> If we are trying to add such a workaround (rather than CPR) for MSM8916
> now, does that mean it's unlikely to see CPR working for MSM8916
> anytime soon?
>
> AFAICT, there is a WIP branch from Niklas Cassel here:
> https://git.linaro.org/people/nicolas.dechesne/niklas.cassel/kernel.git/log/?h=cpr-msm8916-mainline
> but it hasn't been updated for a while. (Not sure if it was working
> already...)
>
> Can someone explain what is missing to make CPR work for MSM8916?

Support should be relatively straightforward to add, but would request
some time to implement and validate.
This patch can be considered as a first step, using the ceiling voltages.

>
> One other minor comment/question below.
>
> >
> > This patch:
> > - Adds missing opps and corresponding target voltages to msm8916.dtsi.
> > - Adds cpu-supply to apq8016-sbc.dtsi (board level info).
> > - Adds pm8916 spmi regulator node to pm8916.dtsi.
> >
> > Tested with a dragonboard-410c.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi | 24 ++++++++++++++++++++++++
> >  arch/arm64/boot/dts/qcom/msm8916.dtsi     | 24 ++++++++++++++++++++++++
> >  arch/arm64/boot/dts/qcom/pm8916.dtsi      |  6 ++++++
> >  3 files changed, 54 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > index 037e26b..f1c1216 100644
> > --- a/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> > @@ -560,6 +560,30 @@
> >       qcom,mbhc-vthreshold-high = <75 150 237 450 500>;
> >  };
> >
> > +&spm_regulators {
> > +     vdd_cpu: s2 {
> > +             regulator-always-on;
> > +             regulator-min-microvolt = <1050000>;
> > +             regulator-max-microvolt = <1350000>;
> > +     };
> > +};
> > +
> > +&CPU0 {
> > +     cpu-supply = <&vdd_cpu>;
> > +};
> > +
> > +&CPU1 {
> > +     cpu-supply = <&vdd_cpu>;
> > +};
> > +
> > +&CPU2 {
> > +     cpu-supply = <&vdd_cpu>;
> > +};
> > +
> > +&CPU3 {
> > +     cpu-supply = <&vdd_cpu>;
> > +};
> > +
>
> I'm a bit confused about the separation here. The cpu-supply is defined
> in the board-specific device tree, yet the voltages are set in the
> common device tree below.
>
> Is it even possible that the CPU is supplied by something other than S2
> and if yes, how likely is this?

Well nothing prevents to use a different PMIC, but you're right in
practice msm8916 is tightly coupled to pm8916 and s2 is clearly
specified as application processor regulator in pm8916 spec. So I'll
move that into msm8916 for the v2.

> I'm asking because I have two other MSM8916 devices in mainline
> (and a few more pending upstreaming), and it seems like I would need to
> duplicate this into each of them.

Would you be able to test and ack the V2 with these boards?

Regards,
Loic