Re: [PATCH] Update H700 opp values

[Andre Przywara <andre.przywara@arm.com>]

Hi Philippe,

thanks for taking care of sending a patch!
You would need prefixes on the subject line to point to the right 
subsystem. A "git log path/to/changed_file" would give you the prefix of 
previous patches, for you to copy from. In this case it would be:
arm64: dts: allwinner: h616: update H700 OPP values

On 28/11/2024 15:45, Philippe Simons wrote:
> My H700 (RG35XX-H, RG40XX-V and RG CubeXX) devices are very unstable,
> especially with some OPPs.
> Crashes were fairly easy to reproduce with any dynamic cpufreq governor
> and some load on CPU, usually in matter of minutes.
> Crashes manifested randomly as simply hanging or various kernel oops
> 
> Manufacturer (Anbernic) is using more conservative mircrovolt values,
> so let's use these.
> While using performance gov seems stables at 1.5Ghz, it still crashes
> using a dynamic gov (even with Andre reparenting patch), so let's drop
> it for now, like manufacturer does.
> 
> Signed-off-by: Philippe Simons <simons.philippe@gmail.com

The change itself looks alright, bumping the OPP voltages by 50mV looks 
reasonable, and is a no-brainer if it stops the issues you have seen.

The loss of the 1.5GHz top OPP is a bummer, but stability comes 
definitely first, otherwise we will just fail faster ;-)

I would like to hear opinions from Ryan and Chris (both CC:ed). Did you 
see similar issues with say the ondemand governor?

The patch itself looks correct, so for that:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>   .../dts/allwinner/sun50i-h616-cpu-opp.dtsi    | 19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616-cpu-opp.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616-cpu-opp.dtsi
> index dd10aaf47..ac13fe169 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h616-cpu-opp.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h616-cpu-opp.dtsi
> @@ -50,24 +50,21 @@ opp-1008000000 {
>   			opp-microvolt-speed2 = <950000>;
>   			opp-microvolt-speed3 = <950000>;
>   			opp-microvolt-speed4 = <1020000>;
> -			opp-microvolt-speed5 = <900000>;
> +			opp-microvolt-speed5 = <950000>;
>   			clock-latency-ns = <244144>; /* 8 32k periods */
>   			opp-supported-hw = <0x3f>;
>   		};
>   
>   		opp-1032000000 {
>   			opp-hz = /bits/ 64 <1032000000>;
> -			opp-microvolt = <900000>;
> +			opp-microvolt = <950000>;
>   			clock-latency-ns = <244144>; /* 8 32k periods */
>   			opp-supported-hw = <0x20>;
>   		};
>   
>   		opp-1104000000 {
>   			opp-hz = /bits/ 64 <1104000000>;
> -			opp-microvolt-speed0 = <1000000>;
> -			opp-microvolt-speed2 = <1000000>;
> -			opp-microvolt-speed3 = <1000000>;
> -			opp-microvolt-speed5 = <950000>;
> +			opp-microvolt = <1000000>;			
>   			clock-latency-ns = <244144>; /* 8 32k periods */
>   			opp-supported-hw = <0x2d>;
>   		};
> @@ -79,7 +76,7 @@ opp-1200000000 {
>   			opp-microvolt-speed2 = <1050000>;
>   			opp-microvolt-speed3 = <1050000>;
>   			opp-microvolt-speed4 = <1100000>;
> -			opp-microvolt-speed5 = <1020000>;
> +			opp-microvolt-speed5 = <1050000>;
>   			clock-latency-ns = <244144>; /* 8 32k periods */
>   			opp-supported-hw = <0x3f>;
>   		};
> @@ -93,7 +90,10 @@ opp-1320000000 {
>   
>   		opp-1416000000 {
>   			opp-hz = /bits/ 64 <1416000000>;
> -			opp-microvolt = <1100000>;
> +			opp-microvolt-speed0 = <1100000>;
> +			opp-microvolt-speed2 = <1100000>;
> +			opp-microvolt-speed3 = <1100000>;
> +			opp-microvolt-speed5 = <1160000>;
>   			clock-latency-ns = <244144>; /* 8 32k periods */
>   			opp-supported-hw = <0x2d>;
>   		};
> @@ -102,9 +102,8 @@ opp-1512000000 {
>   			opp-hz = /bits/ 64 <1512000000>;
>   			opp-microvolt-speed1 = <1100000>;
>   			opp-microvolt-speed3 = <1100000>;
> -			opp-microvolt-speed5 = <1160000>;
>   			clock-latency-ns = <244144>; /* 8 32k periods */
> -			opp-supported-hw = <0x2a>;
> +			opp-supported-hw = <0x0a>;
>   		};
>   	};
>   };