Hi,
On Thu, Apr 02, 2020 at 12:00:35PM +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.2Ghz. Ideally, msm8916 CPR should be implemented to
> fine tune operating voltages and optimize power consumption.
>
> The SPMI interface is directly used for AP regulator control since
> it offers a minimal transition latency (maximum transition latency
> with spmi is 250us, with rpm is 970us as reported by cpufreq-info).
>
> This patch:
> - Adds missing opps and corresponding target voltages to msm8916.dtsi.
> - Adds pm8916 spmi regulator node to pm8916.dtsi.
>
> Tested with a dragonboard-410c.
>
> Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxx>
> ---
> v2: - move cpu-supply to msm8916 since pm8916 s2 is tighly coupled
> to AP core (cf pm8916 specification) + other pm8916 supplies
> are already defined in msm8916.
Thanks for making these changes!
I will try to test this on my devices later today,
and will ask some more people to test it on theirs.
What is a good way to test that it works correctly?
If the device manages to reach the higher frequencies and still works
correctly it's fine?
> - s2 min/max are specified in pm8916 spec
Regarding this I have a small concern below.
> - Removed 1.36GHz op since freq seems capped to 1.21 anyway
>
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 25 +++++++++++++++++++++++++
> arch/arm64/boot/dts/qcom/pm8916.dtsi | 13 +++++++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 9f31064..7407157 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -103,6 +103,7 @@
> next-level-cache = <&L2_0>;
> enable-method = "psci";
> clocks = <&apcs>;
> + cpu-supply = <&pm8916_spmi_s2>;
> operating-points-v2 = <&cpu_opp_table>;
> #cooling-cells = <2>;
> power-domains = <&CPU_PD0>;
> @@ -116,6 +117,7 @@
> next-level-cache = <&L2_0>;
> enable-method = "psci";
> clocks = <&apcs>;
> + cpu-supply = <&pm8916_spmi_s2>;
> operating-points-v2 = <&cpu_opp_table>;
> #cooling-cells = <2>;
> power-domains = <&CPU_PD1>;
> @@ -129,6 +131,7 @@
> next-level-cache = <&L2_0>;
> enable-method = "psci";
> clocks = <&apcs>;
> + cpu-supply = <&pm8916_spmi_s2>;
> operating-points-v2 = <&cpu_opp_table>;
> #cooling-cells = <2>;
> power-domains = <&CPU_PD2>;
> @@ -142,6 +145,7 @@
> next-level-cache = <&L2_0>;
> enable-method = "psci";
> clocks = <&apcs>;
> + cpu-supply = <&pm8916_spmi_s2>;
> operating-points-v2 = <&cpu_opp_table>;
> #cooling-cells = <2>;
> power-domains = <&CPU_PD3>;
> @@ -342,15 +346,35 @@
>
> 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>;
> };
> };
>
> @@ -1605,6 +1629,7 @@
> compatible = "qcom,rpm-pm8916-regulators";
>
> pm8916_s1: s1 {};
> + /* s2 is directly controlled via spmi */
> pm8916_s3: s3 {};
> pm8916_s4: s4 {};
>
> diff --git a/arch/arm64/boot/dts/qcom/pm8916.dtsi b/arch/arm64/boot/dts/qcom/pm8916.dtsi
> index 0bcdf04..73d3b28 100644
> --- a/arch/arm64/boot/dts/qcom/pm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/pm8916.dtsi
> @@ -157,5 +157,18 @@
> vdd-micbias-supply = <&pm8916_l13>;
> #sound-dai-cells = <1>;
> };
> +
> + spmi_regulators: spmi_regulators {
> + compatible = "qcom,pm8916-regulators";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + pm8916_spmi_s2: s2 {
> + regulator-always-on;
> + regulator-min-microvolt = <900000>;
> + regulator-max-microvolt = <1562000>;
This might be just me but I'm usually cautious when it comes to setting
up the regulator constraints.
One way is to set the regulator constraints based on the capabilities of
the regulator itself (which is what you did here I think)?
The other way is to only allow voltages that actually make sense;
to ensure that setting incorrect voltages (for whatever reason) will
fail. (I actually know someone who managed to break a board by setting
some regulator voltages incorrectly...)
We don't actually set anything < 1050000 or > 1350000.
And if I'm reading the datasheet correctly, the CPU cores are not even
specified to operate correctly at > 1.42V.
I would personally prefer to keep the min/max voltages from your
previous patch set, i.e.
regulator-min-microvolt = <1050000>;
regulator-max-microvolt = <1350000>;
In case a higher/lower voltage is needed it could still be changed later.
But maybe that's just me being overly cautious?
Thanks,
Stephan
> + };
> + /* other regulators can be controlled via rpm */
> + };
> };
> };
> --
> 2.7.4
>
