Re: [PATCH 15/18] ARM: dts: qcom: apq8064: provide voltage scaling tables

[Stephan Gerhold <stephan@gerhold.net>]

On Mon, Jun 12, 2023 at 08:39:19AM +0300, Dmitry Baryshkov wrote:
> APQ8064 has 4 speed bins, each of them having from 4 to 6 categorization
> kinds. Provide tables necessary to handle voltage scaling on this SoC.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  arch/arm/boot/dts/qcom-apq8064.dtsi | 1017 +++++++++++++++++++++++++++
>  1 file changed, 1017 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
> index 4ef13f3d702b..f35853b59544 100644
> --- a/arch/arm/boot/dts/qcom-apq8064.dtsi
> +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
> @@ -49,6 +49,9 @@ CPU0: cpu@0 {
>  			clocks = <&kraitcc KRAIT_CPU_0>;
>  			clock-names = "cpu";
>  			clock-latency = <100000>;
> +			vdd-mem-supply = <&pm8921_l24>;
> +			vdd-dig-supply = <&pm8921_s3>;
> +			vdd-core-supply = <&saw0_vreg>;
>  			interconnects = <&kraitcc MASTER_KRAIT_L2 &kraitcc SLAVE_KRAIT_L2>;
>  			operating-points-v2 = <&cpu_opp_table>;
>  			#cooling-cells = <2>;
> @@ -66,6 +69,9 @@ CPU1: cpu@1 {
>  			clocks = <&kraitcc KRAIT_CPU_1>;
>  			clock-names = "cpu";
>  			clock-latency = <100000>;
> +			vdd-mem-supply = <&pm8921_l24>;
> +			vdd-dig-supply = <&pm8921_s3>;
> +			vdd-core-supply = <&saw1_vreg>;
>  			interconnects = <&kraitcc MASTER_KRAIT_L2 &kraitcc SLAVE_KRAIT_L2>;
>  			operating-points-v2 = <&cpu_opp_table>;
>  			#cooling-cells = <2>;
> @@ -83,6 +89,9 @@ CPU2: cpu@2 {
>  			clocks = <&kraitcc KRAIT_CPU_2>;
>  			clock-names = "cpu";
>  			clock-latency = <100000>;
> +			vdd-mem-supply = <&pm8921_l24>;
> +			vdd-dig-supply = <&pm8921_s3>;
> +			vdd-core-supply = <&saw2_vreg>;
>  			interconnects = <&kraitcc MASTER_KRAIT_L2 &kraitcc SLAVE_KRAIT_L2>;
>  			operating-points-v2 = <&cpu_opp_table>;
>  			#cooling-cells = <2>;
> @@ -100,6 +109,9 @@ CPU3: cpu@3 {
>  			clocks = <&kraitcc KRAIT_CPU_3>;
>  			clock-names = "cpu";
>  			clock-latency = <100000>;
> +			vdd-mem-supply = <&pm8921_l24>;
> +			vdd-dig-supply = <&pm8921_s3>;
> +			vdd-core-supply = <&saw3_vreg>;
>  			interconnects = <&kraitcc MASTER_KRAIT_L2 &kraitcc SLAVE_KRAIT_L2>;
>  			operating-points-v2 = <&cpu_opp_table>;
>  			#cooling-cells = <2>;
> @@ -132,6 +144,81 @@ cpu_opp_table: opp-table-cpu {
>  		opp-384000000 {
>  			opp-hz = /bits/ 64 <384000000>;
>  			opp-peak-kBps = <384000>;
> +			opp-microvolt-speed0-pvs0 = <1050000 1050000 1150000>,
> +						    <950000 950000 1150000>,
> +						    <950000 950000 975000>;

I think this won't result in the correct switch order without making
some changes to the OPP core. In _set_opp() the OPP core does

	/* Scaling up? Configure required OPPs before frequency */
	if (!scaling_down) {
		_set_required_opps();
		_set_opp_bw();
		opp_table->config_regulators();
	}

	opp_table->config_clks();

	/* Scaling down? Configure required OPPs after frequency */
	if (scaling_down) {
		opp_table->config_regulators();
		_set_opp_bw();
		_set_required_opps();
	}

Since the "bandwidth" for the L2 cache is set before the regulators
there is a short window where the L2 clock is running at a high
frequency with too low voltage, which could potentially cause
instability. On downstream this seems to be done in the proper order [1].

I'm not sure if the order in the OPP core is on purpose. If not, you
could propose moving the config_regulators() first (for scaling up)
and last (for scaling down). This would resolve the problem.

The alternative that I've already argued for on IRC in #linux-msm a
couple of days ago would be to give the L2 cache (here: "interconnect")
an own OPP table where it can describe its voltage requirements,
independent from the CPU. That way the icc_set_bw() would be guaranteed
to apply the correct voltage before adjusting the L2 cache clock. It
looks like the "l2_level" voltages for vdd_dig and vdd_mem are not
speedbin/PVS-specific [2] so this would also significantly reduce the DT
size, since you wouldn't need to repeat the same vdd_dig/vdd_mem
voltages for all of them.

Thanks,
Stephan

[1]: https://git.codelinaro.org/clo/la/kernel/msm/-/blob/LA.AF.1.2.1-08410-8064.0/arch/arm/mach-msm/acpuclock-krait.c#L529-588
[2]: https://git.codelinaro.org/clo/la/kernel/msm/-/blob/LA.AF.1.2.1-08410-8064.0/arch/arm/mach-msm/acpuclock-8064.c#L118-135