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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Wed 01 Apr 10:50 PDT 2020, 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.
> 
> 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.
> 

Thanks Loic, happy to see you tying up these loose ends! Just one minor
nit below.

> 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>;
> +};
> +
>  &smd_rpm_regulators {
>  	vdd_l1_l2_l3-supply = <&pm8916_s3>;
>  	vdd_l5-supply = <&pm8916_s3>;
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 9f31064..9805af0 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -342,15 +342,39 @@
>  
>  		opp-200000000 {
>  			opp-hz = /bits/ 64 <200000000>;
> +			opp-microvolt = <1050000>;
>  		};
>  		opp-400000000 {
>  			opp-hz = /bits/ 64 <400000000>;
> +			opp-microvolt = <1050000>;
> +		};
> +		opp-533330000 {
> +			opp-hz = /bits/ 64 <533330000>;
> +			opp-microvolt = <1150000>;
>  		};
>  		opp-800000000 {
>  			opp-hz = /bits/ 64 <800000000>;
> +			opp-microvolt = <1150000>;
>  		};
>  		opp-998400000 {
>  			opp-hz = /bits/ 64 <998400000>;
> +			opp-microvolt = <1350000>;
> +		};
> +		opp-1094400000 {
> +			opp-hz = /bits/ 64 <1094400000>;
> +			opp-microvolt = <1350000>;
> +		};
> +		opp-1152000000 {
> +			opp-hz = /bits/ 64 <1152000000>;
> +			opp-microvolt = <1350000>;
> +		};
> +		opp-1209600000 {
> +			opp-hz = /bits/ 64 <1209600000>;
> +			opp-microvolt = <1350000>;
> +		};
> +		opp-1363200000 {
> +			opp-hz = /bits/ 64 <1363200000>;
> +			opp-microvolt = <1350000>;
>  		};
>  	};
>  
> diff --git a/arch/arm64/boot/dts/qcom/pm8916.dtsi b/arch/arm64/boot/dts/qcom/pm8916.dtsi
> index 0bcdf04..c9b9c4f 100644
> --- a/arch/arm64/boot/dts/qcom/pm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/pm8916.dtsi
> @@ -157,5 +157,11 @@
>  			vdd-micbias-supply = <&pm8916_l13>;
>  			#sound-dai-cells = <1>;
>  		};
> +
> +		spm_regulators: spm_regulators  {

There is the SPM regulator interface in the CPU subsystem, which is used
for hardware assisted entering and exiting of low power modes. And then
there's the direct PMIC control over SPMI. Except for the fact that the
SPM hardware uses SPMI for controlling the PMIC these are two different
things, and this is the SPMI one.

So the label, and node should be
		spmi_regulators: regulators {


Regards,
Bjorn

> +			compatible = "qcom,pm8916-regulators";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +		};
>  	};
>  };
> -- 
> 2.7.4
>