[PATCH v2 0/4] Allwinner H6 GPU devfreq

[PATCH v2 0/4] Allwinner H6 GPU devfreq

[Clément Péron]

Hi,

This is a refresh of previous patches sent to enable GPU Devfreq on H6
Beelink GS1 but that wasn't stable at that time[0].

With the recent fix on GPU PLL from Roman Stratiienko I have retested
and everything seems stable and works as expected.

Regards,
Clement

0: https://lore.kernel.org/lkml/CAJiuCce58Gaxf_Qg2cnMwvOgUqYU__eKb3MDX1Fe_+47htg2bA@mail.gmail.com/
1: https://lore.kernel.org/linux-arm-kernel/2562485.k3LOHGUjKi@kista/T/

Changes since v1:
 - proper sign-off with my personal email
 - sent serie to SoC Maintainer w/ cover letter

Clément Péron (4):
  arm64: defconfig: Enable devfreq cooling device
  arm64: dts: allwinner: h6: Add cooling map for GPU
  arm64: dts: allwinner: h6: Add GPU OPP table
  arm64: dts: allwinner: beelink-gs1: Enable GPU OPP

 .../dts/allwinner/sun50i-h6-beelink-gs1.dts   |  2 +
 .../boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi | 88 +++++++++++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  | 22 +++++
 arch/arm64/configs/defconfig                  |  1 +
 4 files changed, 113 insertions(+)
 create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi

-- 
2.34.1

[PATCH v2 1/4] arm64: defconfig: Enable devfreq cooling device

[Clément Péron]

Devfreq cooling device framework is used in Panfrost
to throttle GPU in order to regulate its temperature.

Enable this driver for ARM64 SoC.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index d5b2d2dd4904..109004e44d21 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -584,6 +584,7 @@ CONFIG_SENSORS_INA2XX=m
 CONFIG_SENSORS_INA3221=m
 CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y
 CONFIG_CPU_THERMAL=y
+CONFIG_DEVFREQ_THERMAL=y
 CONFIG_THERMAL_EMULATION=y
 CONFIG_IMX_SC_THERMAL=m
 CONFIG_IMX8MM_THERMAL=m
-- 
2.34.1

Re: [PATCH v2 1/4] arm64: defconfig: Enable devfreq cooling device

[Jernej Škrabec]

Dne nedelja, 21. avgust 2022 ob 19:30:48 CEST je Clément Péron napisal(a):
> Devfreq cooling device framework is used in Panfrost
> to throttle GPU in order to regulate its temperature.
> 
> Enable this driver for ARM64 SoC.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>

Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> ---
>  arch/arm64/configs/defconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index d5b2d2dd4904..109004e44d21 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -584,6 +584,7 @@ CONFIG_SENSORS_INA2XX=m
>  CONFIG_SENSORS_INA3221=m
>  CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y
>  CONFIG_CPU_THERMAL=y
> +CONFIG_DEVFREQ_THERMAL=y
>  CONFIG_THERMAL_EMULATION=y
>  CONFIG_IMX_SC_THERMAL=m
>  CONFIG_IMX8MM_THERMAL=m
> --
> 2.34.1

[PATCH v2 2/4] arm64: dts: allwinner: h6: Add cooling map for GPU

[Clément Péron]

Add a simple cooling map for the GPU.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 22 ++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 5a28303d3d4c..943ae5374dd6 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -186,6 +186,7 @@ gpu: gpu@1800000 {
 			clocks = <&ccu CLK_GPU>, <&ccu CLK_BUS_GPU>;
 			clock-names = "core", "bus";
 			resets = <&ccu RST_BUS_GPU>;
+			#cooling-cells = <2>;
 			status = "disabled";
 		};
 
@@ -1075,6 +1076,27 @@ gpu-thermal {
 			polling-delay-passive = <0>;
 			polling-delay = <0>;
 			thermal-sensors = <&ths 1>;
+
+			trips {
+				gpu_alert: gpu-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				gpu-crit {
+					temperature = <100000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map0 {
+					trip = <&gpu_alert>;
+					cooling-device = <&gpu THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
 		};
 	};
 };
-- 
2.34.1

Re: [PATCH v2 2/4] arm64: dts: allwinner: h6: Add cooling map for GPU

[Samuel Holland]

On 8/21/22 12:30 PM, Clément Péron wrote:
> Add a simple cooling map for the GPU.

It would be good to document where the trip point temperatures came from.

> Signed-off-by: Clément Péron <peron.clem@gmail.com>

Acked-by: Samuel Holland <samuel@sholland.org>

> ---
>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 22 ++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> index 5a28303d3d4c..943ae5374dd6 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> @@ -186,6 +186,7 @@ gpu: gpu@1800000 {
>  			clocks = <&ccu CLK_GPU>, <&ccu CLK_BUS_GPU>;
>  			clock-names = "core", "bus";
>  			resets = <&ccu RST_BUS_GPU>;
> +			#cooling-cells = <2>;
>  			status = "disabled";
>  		};
>  
> @@ -1075,6 +1076,27 @@ gpu-thermal {
>  			polling-delay-passive = <0>;
>  			polling-delay = <0>;
>  			thermal-sensors = <&ths 1>;
> +
> +			trips {
> +				gpu_alert: gpu-alert {
> +					temperature = <85000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};
> +
> +				gpu-crit {
> +					temperature = <100000>;
> +					hysteresis = <0>;
> +					type = "critical";
> +				};
> +			};
> +
> +			cooling-maps {
> +				map0 {
> +					trip = <&gpu_alert>;
> +					cooling-device = <&gpu THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +				};
> +			};
>  		};
>  	};
>  };
> 

Re: [PATCH v2 2/4] arm64: dts: allwinner: h6: Add cooling map for GPU

[Clément Péron]

Hi Samuel,

On Tue, 23 Aug 2022 at 05:16, Samuel Holland <samuel@sholland.org> wrote:
>
> On 8/21/22 12:30 PM, Clément Péron wrote:
> > Add a simple cooling map for the GPU.
>
> It would be good to document where the trip point temperatures came from.

If I remember correctly, I got those when getting the dtb from my
Beelink GS1 when it was running vendor Android with the oldest
Allwinner kernel.

But now that you ask I double check with the "new" vendor kernel source:
https://github.com/Allwinner-Homlet/H6-BSP4.9-linux/blob/master/arch/arm64/boot/dts/sunxi/sun50iw6p1.dtsi#L2034-L2053

And It's different from what I got in the past.
The throttling starts when the GPU is already very hot (95, 100 and
105°C) and seems to only disable the highest frequency (756, 624 and
576MHz).
Which let the GPU running at 0.91V @ 540MHz.

Which is far to be the lowest possible consumption for the GPU (0.81V
@ 336MHz would be better in the hottest situation)

I'm not an expert but either I could just try to copy/paste the same
behavior or try to have a more smooth cooling map (70, 85, 100°C).

What do you think?

Thanks,
Clement



>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
>
> Acked-by: Samuel Holland <samuel@sholland.org>
>
> > ---
> >  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 22 ++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > index 5a28303d3d4c..943ae5374dd6 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > @@ -186,6 +186,7 @@ gpu: gpu@1800000 {
> >                       clocks = <&ccu CLK_GPU>, <&ccu CLK_BUS_GPU>;
> >                       clock-names = "core", "bus";
> >                       resets = <&ccu RST_BUS_GPU>;
> > +                     #cooling-cells = <2>;
> >                       status = "disabled";
> >               };
> >
> > @@ -1075,6 +1076,27 @@ gpu-thermal {
> >                       polling-delay-passive = <0>;
> >                       polling-delay = <0>;
> >                       thermal-sensors = <&ths 1>;
> > +
> > +                     trips {
> > +                             gpu_alert: gpu-alert {
> > +                                     temperature = <85000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "passive";
> > +                             };
> > +
> > +                             gpu-crit {
> > +                                     temperature = <100000>;
> > +                                     hysteresis = <0>;
> > +                                     type = "critical";
> > +                             };
> > +                     };
> > +
> > +                     cooling-maps {
> > +                             map0 {
> > +                                     trip = <&gpu_alert>;
> > +                                     cooling-device = <&gpu THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > +                             };
> > +                     };
> >               };
> >       };
> >  };
> >
>

Re: [PATCH v2 2/4] arm64: dts: allwinner: h6: Add cooling map for GPU

[Samuel Holland]

On 8/27/22 11:49 AM, Clément Péron wrote:
> Hi Samuel,
> 
> On Tue, 23 Aug 2022 at 05:16, Samuel Holland <samuel@sholland.org> wrote:
>>
>> On 8/21/22 12:30 PM, Clément Péron wrote:
>>> Add a simple cooling map for the GPU.
>>
>> It would be good to document where the trip point temperatures came from.
> 
> If I remember correctly, I got those when getting the dtb from my
> Beelink GS1 when it was running vendor Android with the oldest
> Allwinner kernel.
> 
> But now that you ask I double check with the "new" vendor kernel source:
> https://github.com/Allwinner-Homlet/H6-BSP4.9-linux/blob/master/arch/arm64/boot/dts/sunxi/sun50iw6p1.dtsi#L2034-L2053
> 
> And It's different from what I got in the past.
> The throttling starts when the GPU is already very hot (95, 100 and
> 105°C) and seems to only disable the highest frequency (756, 624 and
> 576MHz).
> Which let the GPU running at 0.91V @ 540MHz.
> 
> Which is far to be the lowest possible consumption for the GPU (0.81V
> @ 336MHz would be better in the hottest situation)
> 
> I'm not an expert but either I could just try to copy/paste the same
> behavior or try to have a more smooth cooling map (70, 85, 100°C).
> 
> What do you think?

I would generally prefer something conservative, to be appropriate for most
boards. But I do not have any strong opinion on the exact trip points used; I am
fine with adjusting them.

Mostly I want to know the source of the cooling map -- where the numbers came
from, and what changes were made. That provides context if we want to change the
map in the future, or if we run in to issues on some other board.

Regards,
Samuel

[PATCH v2 3/4] arm64: dts: allwinner: h6: Add GPU OPP table

[Clément Péron]

Add an Operating Performance Points table for the GPU to
enable Dynamic Voltage & Frequency Scaling on the H6.

The voltage range is set with minival voltage set to the target
and the maximal voltage set to 1.2V. This allow DVFS framework to
work properly on board with fixed regulator.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 .../boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi | 88 +++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi
new file mode 100644
index 000000000000..a66204243515
--- /dev/null
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+// Copyright (C) 2022 Clément Péron <peron.clem@gmail.com>
+
+/ {
+	gpu_opp_table: gpu-opp-table {
+		compatible = "operating-points-v2";
+
+		opp@216000000 {
+			opp-hz = /bits/ 64 <216000000>;
+			opp-microvolt = <810000 810000 1200000>;
+		};
+
+		opp@264000000 {
+			opp-hz = /bits/ 64 <264000000>;
+			opp-microvolt = <810000 810000 1200000>;
+		};
+
+		opp@312000000 {
+			opp-hz = /bits/ 64 <312000000>;
+			opp-microvolt = <810000 810000 1200000>;
+		};
+
+		opp@336000000 {
+			opp-hz = /bits/ 64 <336000000>;
+			opp-microvolt = <810000 810000 1200000>;
+		};
+
+		opp@360000000 {
+			opp-hz = /bits/ 64 <360000000>;
+			opp-microvolt = <820000 820000 1200000>;
+		};
+
+		opp@384000000 {
+			opp-hz = /bits/ 64 <384000000>;
+			opp-microvolt = <830000 830000 1200000>;
+		};
+
+		opp@408000000 {
+			opp-hz = /bits/ 64 <408000000>;
+			opp-microvolt = <840000 840000 1200000>;
+		};
+
+		opp@420000000 {
+			opp-hz = /bits/ 64 <420000000>;
+			opp-microvolt = <850000 850000 1200000>;
+		};
+
+		opp@432000000 {
+			opp-hz = /bits/ 64 <432000000>;
+			opp-microvolt = <860000 860000 1200000>;
+		};
+
+		opp@456000000 {
+			opp-hz = /bits/ 64 <456000000>;
+			opp-microvolt = <870000 870000 1200000>;
+		};
+
+		opp@504000000 {
+			opp-hz = /bits/ 64 <504000000>;
+			opp-microvolt = <890000 890000 1200000>;
+		};
+
+		opp@540000000 {
+			opp-hz = /bits/ 64 <540000000>;
+			opp-microvolt = <910000 910000 1200000>;
+		};
+
+		opp@576000000 {
+			opp-hz = /bits/ 64 <576000000>;
+			opp-microvolt = <930000 930000 1200000>;
+		};
+
+		opp@624000000 {
+			opp-hz = /bits/ 64 <624000000>;
+			opp-microvolt = <950000 950000 1200000>;
+		};
+
+		opp@756000000 {
+			opp-hz = /bits/ 64 <756000000>;
+			opp-microvolt = <1040000 1040000 1200000>;
+		};
+	};
+
+};
+
+&gpu {
+	operating-points-v2 = <&gpu_opp_table>;
+};
-- 
2.34.1

Re: [PATCH v2 3/4] arm64: dts: allwinner: h6: Add GPU OPP table

[Samuel Holland]

On 8/21/22 12:30 PM, Clément Péron wrote:
> Add an Operating Performance Points table for the GPU to
> enable Dynamic Voltage & Frequency Scaling on the H6.
> 
> The voltage range is set with minival voltage set to the target
> and the maximal voltage set to 1.2V. This allow DVFS framework to
> work properly on board with fixed regulator.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  .../boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi | 88 +++++++++++++++++++
>  1 file changed, 88 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi
> new file mode 100644
> index 000000000000..a66204243515
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +// Copyright (C) 2022 Clément Péron <peron.clem@gmail.com>
> +
> +/ {
> +	gpu_opp_table: gpu-opp-table {
> +		compatible = "operating-points-v2";
> +
> +		opp@216000000 {

Please fix the `make dtbs_check` warnings:

arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dtb: gpu-opp-table:
$nodename:0: 'gpu-opp-table' does not match '^opp-table(-[a-z0-9]+)?$'
        From schema: Documentation/devicetree/bindings/opp/opp-v2.yaml
arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dtb: gpu-opp-table:
Unevaluated properties are not allowed ('opp@216000000', 'opp@264000000',
'opp@312000000', 'opp@336000000', 'opp@360000000', 'opp@384000000',
'opp@408000000', 'opp@420000000', 'opp@432000000', 'opp@456000000',
'opp@504000000', 'opp@540000000', 'opp@576000000', 'opp@624000000',
'opp@756000000' were unexpected)
        From schema: Documentation/devicetree/bindings/opp/opp-v2.yaml

Regards,
Samuel

Re: [PATCH v2 3/4] arm64: dts: allwinner: h6: Add GPU OPP table

[Krzysztof Kozlowski]

On 21/08/2022 20:30, Clément Péron wrote:
> Add an Operating Performance Points table for the GPU to
> enable Dynamic Voltage & Frequency Scaling on the H6.
> 
> The voltage range is set with minival voltage set to the target
> and the maximal voltage set to 1.2V. This allow DVFS framework to
> work properly on board with fixed regulator.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  .../boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi | 88 +++++++++++++++++++
>  1 file changed, 88 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi
> new file mode 100644
> index 000000000000..a66204243515
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +// Copyright (C) 2022 Clément Péron <peron.clem@gmail.com>
> +
> +/ {
> +	gpu_opp_table: gpu-opp-table {

I don't think you tested the change with dtbs_check. This does not match
the bindings.

> +		compatible = "operating-points-v2";
> +
> +		opp@216000000 {
> +			opp-hz = /bits/ 64 <216000000>;
> +			opp-microvolt = <810000 810000 1200000>;
> +		};
> +
> +		opp@264000000 {
> +			opp-hz = /bits/ 64 <264000000>;
> +			opp-microvolt = <810000 810000 1200000>;
> +		};
> +
> +		opp@312000000 {
> +			opp-hz = /bits/ 64 <312000000>;
> +			opp-microvolt = <810000 810000 1200000>;
> +		};
> +
> +		opp@336000000 {
> +			opp-hz = /bits/ 64 <336000000>;
> +			opp-microvolt = <810000 810000 1200000>;
> +		};
> +
> +		opp@360000000 {
> +			opp-hz = /bits/ 64 <360000000>;
> +			opp-microvolt = <820000 820000 1200000>;
> +		};
> +
> +		opp@384000000 {
> +			opp-hz = /bits/ 64 <384000000>;
> +			opp-microvolt = <830000 830000 1200000>;
> +		};
> +
> +		opp@408000000 {
> +			opp-hz = /bits/ 64 <408000000>;
> +			opp-microvolt = <840000 840000 1200000>;
> +		};
> +
> +		opp@420000000 {
> +			opp-hz = /bits/ 64 <420000000>;
> +			opp-microvolt = <850000 850000 1200000>;
> +		};
> +
> +		opp@432000000 {
> +			opp-hz = /bits/ 64 <432000000>;
> +			opp-microvolt = <860000 860000 1200000>;
> +		};
> +
> +		opp@456000000 {
> +			opp-hz = /bits/ 64 <456000000>;
> +			opp-microvolt = <870000 870000 1200000>;
> +		};
> +
> +		opp@504000000 {
> +			opp-hz = /bits/ 64 <504000000>;
> +			opp-microvolt = <890000 890000 1200000>;
> +		};
> +
> +		opp@540000000 {
> +			opp-hz = /bits/ 64 <540000000>;
> +			opp-microvolt = <910000 910000 1200000>;
> +		};
> +
> +		opp@576000000 {
> +			opp-hz = /bits/ 64 <576000000>;
> +			opp-microvolt = <930000 930000 1200000>;
> +		};
> +
> +		opp@624000000 {
> +			opp-hz = /bits/ 64 <624000000>;
> +			opp-microvolt = <950000 950000 1200000>;
> +		};
> +
> +		opp@756000000 {
> +			opp-hz = /bits/ 64 <756000000>;
> +			opp-microvolt = <1040000 1040000 1200000>;
> +		};
> +	};
> +

No need for such blank lines.


Best regards,
Krzysztof

[PATCH v2 4/4] arm64: dts: allwinner: beelink-gs1: Enable GPU OPP

[Clément Péron]

Enable GPU OPP table for Beelink GS1

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
index 6249e9e02928..20fc0584d1c6 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
@@ -5,6 +5,7 @@
 
 #include "sun50i-h6.dtsi"
 #include "sun50i-h6-cpu-opp.dtsi"
+#include "sun50i-h6-gpu-opp.dtsi"
 
 #include <dt-bindings/gpio/gpio.h>
 
@@ -261,6 +262,7 @@ reg_dcdca: dcdca {
 			};
 
 			reg_dcdcc: dcdcc {
+				regulator-always-on;
 				regulator-enable-ramp-delay = <32000>;
 				regulator-min-microvolt = <810000>;
 				regulator-max-microvolt = <1080000>;
-- 
2.34.1

Re: [PATCH v2 4/4] arm64: dts: allwinner: beelink-gs1: Enable GPU OPP

[Samuel Holland]

On 8/21/22 12:30 PM, Clément Péron wrote:
> Enable GPU OPP table for Beelink GS1
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> index 6249e9e02928..20fc0584d1c6 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> @@ -5,6 +5,7 @@
>  
>  #include "sun50i-h6.dtsi"
>  #include "sun50i-h6-cpu-opp.dtsi"
> +#include "sun50i-h6-gpu-opp.dtsi"
>  
>  #include <dt-bindings/gpio/gpio.h>
>  
> @@ -261,6 +262,7 @@ reg_dcdca: dcdca {
>  			};
>  
>  			reg_dcdcc: dcdcc {
> +				regulator-always-on;

Why is this necessary? This file already has:

&gpu {
	mali-supply = <&reg_dcdcc>;
	status = "okay";
};

So there is a consumer for this regulator.

Regards,
Samuel

>  				regulator-enable-ramp-delay = <32000>;
>  				regulator-min-microvolt = <810000>;
>  				regulator-max-microvolt = <1080000>;
> 

Re: [PATCH v2 4/4] arm64: dts: allwinner: beelink-gs1: Enable GPU OPP

[Clément Péron]

Hi Samuel,

On Tue, 23 Aug 2022 at 05:07, Samuel Holland <samuel@sholland.org> wrote:
>
> On 8/21/22 12:30 PM, Clément Péron wrote:
> > Enable GPU OPP table for Beelink GS1
> >
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> >  arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > index 6249e9e02928..20fc0584d1c6 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > @@ -5,6 +5,7 @@
> >
> >  #include "sun50i-h6.dtsi"
> >  #include "sun50i-h6-cpu-opp.dtsi"
> > +#include "sun50i-h6-gpu-opp.dtsi"
> >
> >  #include <dt-bindings/gpio/gpio.h>
> >
> > @@ -261,6 +262,7 @@ reg_dcdca: dcdca {
> >                       };
> >
> >                       reg_dcdcc: dcdcc {
> > +                             regulator-always-on;
>
> Why is this necessary? This file already has:

This is a relica from the first serie at this time the OPP doesn't
properly enable the regulator it's now fixed since:
https://patchwork.kernel.org/project/linux-pm/patch/81eb2efeeed1556d124065252f32777838a6d850.1589528491.git.viresh.kumar@linaro.org/

I will drop it.

Thanks for the review.
Regards,
Clement




>
> &gpu {
>         mali-supply = <&reg_dcdcc>;
>         status = "okay";
> };
>
> So there is a consumer for this regulator.
>
> Regards,
> Samuel
>
> >                               regulator-enable-ramp-delay = <32000>;
> >                               regulator-min-microvolt = <810000>;
> >                               regulator-max-microvolt = <1080000>;
> >
>

Re: [PATCH v2 4/4] arm64: dts: allwinner: beelink-gs1: Enable GPU OPP

[Clément Péron]

Hi Samuel,

On Sat, 3 Sept 2022 at 20:41, Clément Péron <peron.clem@gmail.com> wrote:
>
> Hi Samuel,
>
> On Tue, 23 Aug 2022 at 05:07, Samuel Holland <samuel@sholland.org> wrote:
> >
> > On 8/21/22 12:30 PM, Clément Péron wrote:
> > > Enable GPU OPP table for Beelink GS1
> > >
> > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > ---
> > >  arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > index 6249e9e02928..20fc0584d1c6 100644
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > > @@ -5,6 +5,7 @@
> > >
> > >  #include "sun50i-h6.dtsi"
> > >  #include "sun50i-h6-cpu-opp.dtsi"
> > > +#include "sun50i-h6-gpu-opp.dtsi"
> > >
> > >  #include <dt-bindings/gpio/gpio.h>
> > >
> > > @@ -261,6 +262,7 @@ reg_dcdca: dcdca {
> > >                       };
> > >
> > >                       reg_dcdcc: dcdcc {
> > > +                             regulator-always-on;
> >
> > Why is this necessary? This file already has:
>
> This is a relica from the first serie at this time the OPP doesn't
> properly enable the regulator it's now fixed since:
> https://patchwork.kernel.org/project/linux-pm/patch/81eb2efeeed1556d124065252f32777838a6d850.1589528491.git.viresh.kumar@linaro.org/
>
> I will drop it.

After retesting it, it seems to still no take the regulator and make
my board hang... :(

[   17.698597] sun8i-dw-hdmi 6000000.hdmi: registered DesignWare HDMI
I2C bus driver
[   17.708475] sun4i-drm display-engine: bound 6000000.hdmi (ops
sun8i_dw_hdmi_ops [sun8i_drm_hdmi])
[   17.718350] [drm] Initialized sun4i-drm 1.0.0 20150629 for
display-engine on minor 1
[   17.877443] Console: switching to colour frame buffer device 320x90
[   17.936050] sun4i-drm display-engine: [drm] fb0: sun4i-drmdrmfb
frame buffer device
[   17.961881] platform 5200000.usb: deferred probe pending
>>>> [   31.710731] vdd-gpu: disabling <<<<<




>
> Thanks for the review.
> Regards,
> Clement
>
>
>
>
> >
> > &gpu {
> >         mali-supply = <&reg_dcdcc>;
> >         status = "okay";
> > };
> >
> > So there is a consumer for this regulator.
> >
> > Regards,
> > Samuel
> >
> > >                               regulator-enable-ramp-delay = <32000>;
> > >                               regulator-min-microvolt = <810000>;
> > >                               regulator-max-microvolt = <1080000>;
> > >
> >

Re: [PATCH v2 4/4] arm64: dts: allwinner: beelink-gs1: Enable GPU OPP

[Samuel Holland]

On 9/3/22 2:06 PM, Clément Péron wrote:
> Hi Samuel,
> 
> On Sat, 3 Sept 2022 at 20:41, Clément Péron <peron.clem@gmail.com> wrote:
>>
>> Hi Samuel,
>>
>> On Tue, 23 Aug 2022 at 05:07, Samuel Holland <samuel@sholland.org> wrote:
>>>
>>> On 8/21/22 12:30 PM, Clément Péron wrote:
>>>> Enable GPU OPP table for Beelink GS1
>>>>
>>>> Signed-off-by: Clément Péron <peron.clem@gmail.com>
>>>> ---
>>>>  arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
>>>> index 6249e9e02928..20fc0584d1c6 100644
>>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
>>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
>>>> @@ -5,6 +5,7 @@
>>>>
>>>>  #include "sun50i-h6.dtsi"
>>>>  #include "sun50i-h6-cpu-opp.dtsi"
>>>> +#include "sun50i-h6-gpu-opp.dtsi"
>>>>
>>>>  #include <dt-bindings/gpio/gpio.h>
>>>>
>>>> @@ -261,6 +262,7 @@ reg_dcdca: dcdca {
>>>>                       };
>>>>
>>>>                       reg_dcdcc: dcdcc {
>>>> +                             regulator-always-on;
>>>
>>> Why is this necessary? This file already has:
>>
>> This is a relica from the first serie at this time the OPP doesn't
>> properly enable the regulator it's now fixed since:
>> https://patchwork.kernel.org/project/linux-pm/patch/81eb2efeeed1556d124065252f32777838a6d850.1589528491.git.viresh.kumar@linaro.org/

This should have nothing to do with the OPP driver; lima already has a "mali"
regulator consumer that should be enabled whenever the GPU is in use.

_Adding_ a regulator consumer from the OPP driver should not somehow decrease
the refcount.

>> I will drop it.
> 
> After retesting it, it seems to still no take the regulator and make
> my board hang... :(
> 
> [   17.698597] sun8i-dw-hdmi 6000000.hdmi: registered DesignWare HDMI
> I2C bus driver
> [   17.708475] sun4i-drm display-engine: bound 6000000.hdmi (ops
> sun8i_dw_hdmi_ops [sun8i_drm_hdmi])
> [   17.718350] [drm] Initialized sun4i-drm 1.0.0 20150629 for
> display-engine on minor 1
> [   17.877443] Console: switching to colour frame buffer device 320x90
> [   17.936050] sun4i-drm display-engine: [drm] fb0: sun4i-drmdrmfb
> frame buffer device
> [   17.961881] platform 5200000.usb: deferred probe pending
>>>>> [   31.710731] vdd-gpu: disabling <<<<<

Are there any messages from lima, especially about mali-supply? Can you provide
regulators_summary from debugfs?

Regards,
Samuel

Re: [PATCH v2 4/4] arm64: dts: allwinner: beelink-gs1: Enable GPU OPP

[Clément Péron]

Hi Samuel,

On Sun, 4 Sept 2022 at 05:32, Samuel Holland <samuel@sholland.org> wrote:
>
> On 9/3/22 2:06 PM, Clément Péron wrote:
> > Hi Samuel,
> >
> > On Sat, 3 Sept 2022 at 20:41, Clément Péron <peron.clem@gmail.com> wrote:
> >>
> >> Hi Samuel,
> >>
> >> On Tue, 23 Aug 2022 at 05:07, Samuel Holland <samuel@sholland.org> wrote:
> >>>
> >>> On 8/21/22 12:30 PM, Clément Péron wrote:
> >>>> Enable GPU OPP table for Beelink GS1
> >>>>
> >>>> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> >>>> ---
> >>>>  arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> >>>> index 6249e9e02928..20fc0584d1c6 100644
> >>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> >>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> >>>> @@ -5,6 +5,7 @@
> >>>>
> >>>>  #include "sun50i-h6.dtsi"
> >>>>  #include "sun50i-h6-cpu-opp.dtsi"
> >>>> +#include "sun50i-h6-gpu-opp.dtsi"
> >>>>
> >>>>  #include <dt-bindings/gpio/gpio.h>
> >>>>
> >>>> @@ -261,6 +262,7 @@ reg_dcdca: dcdca {
> >>>>                       };
> >>>>
> >>>>                       reg_dcdcc: dcdcc {
> >>>> +                             regulator-always-on;
> >>>
> >>> Why is this necessary? This file already has:
> >>
> >> This is a relica from the first serie at this time the OPP doesn't
> >> properly enable the regulator it's now fixed since:
> >> https://patchwork.kernel.org/project/linux-pm/patch/81eb2efeeed1556d124065252f32777838a6d850.1589528491.git.viresh.kumar@linaro.org/
>
> This should have nothing to do with the OPP driver; lima already has a "mali"
> regulator consumer that should be enabled whenever the GPU is in use.

Okay so you propose to add a regulator_enable() in the panfrost
drivers in addition to the OPP consumer right?

I will send a patch to add this and see what's panfrost maintainer
think about it.

Regards,
Clement

>
> _Adding_ a regulator consumer from the OPP driver should not somehow decrease
> the refcount.
>
> >> I will drop it.
> >
> > After retesting it, it seems to still no take the regulator and make
> > my board hang... :(
> >
> > [   17.698597] sun8i-dw-hdmi 6000000.hdmi: registered DesignWare HDMI
> > I2C bus driver
> > [   17.708475] sun4i-drm display-engine: bound 6000000.hdmi (ops
> > sun8i_dw_hdmi_ops [sun8i_drm_hdmi])
> > [   17.718350] [drm] Initialized sun4i-drm 1.0.0 20150629 for
> > display-engine on minor 1
> > [   17.877443] Console: switching to colour frame buffer device 320x90
> > [   17.936050] sun4i-drm display-engine: [drm] fb0: sun4i-drmdrmfb
> > frame buffer device
> > [   17.961881] platform 5200000.usb: deferred probe pending
> >>>>> [   31.710731] vdd-gpu: disabling <<<<<
>
> Are there any messages from lima, especially about mali-supply? Can you provide
> regulators_summary from debugfs?
>
> Regards,
> Samuel

Re: [PATCH v2 4/4] arm64: dts: allwinner: beelink-gs1: Enable GPU OPP

[Clément Péron]

Hi Samuel,

On Sun, 4 Sept 2022 at 16:23, Clément Péron <peron.clem@gmail.com> wrote:
>
> Hi Samuel,
>
> On Sun, 4 Sept 2022 at 05:32, Samuel Holland <samuel@sholland.org> wrote:
> >
> > On 9/3/22 2:06 PM, Clément Péron wrote:
> > > Hi Samuel,
> > >
> > > On Sat, 3 Sept 2022 at 20:41, Clément Péron <peron.clem@gmail.com> wrote:
> > >>
> > >> Hi Samuel,
> > >>
> > >> On Tue, 23 Aug 2022 at 05:07, Samuel Holland <samuel@sholland.org> wrote:
> > >>>
> > >>> On 8/21/22 12:30 PM, Clément Péron wrote:
> > >>>> Enable GPU OPP table for Beelink GS1
> > >>>>
> > >>>> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > >>>> ---
> > >>>>  arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 2 ++
> > >>>>  1 file changed, 2 insertions(+)
> > >>>>
> > >>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > >>>> index 6249e9e02928..20fc0584d1c6 100644
> > >>>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > >>>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > >>>> @@ -5,6 +5,7 @@
> > >>>>
> > >>>>  #include "sun50i-h6.dtsi"
> > >>>>  #include "sun50i-h6-cpu-opp.dtsi"
> > >>>> +#include "sun50i-h6-gpu-opp.dtsi"
> > >>>>
> > >>>>  #include <dt-bindings/gpio/gpio.h>
> > >>>>
> > >>>> @@ -261,6 +262,7 @@ reg_dcdca: dcdca {
> > >>>>                       };
> > >>>>
> > >>>>                       reg_dcdcc: dcdcc {
> > >>>> +                             regulator-always-on;
> > >>>
> > >>> Why is this necessary? This file already has:
> > >>
> > >> This is a relica from the first serie at this time the OPP doesn't
> > >> properly enable the regulator it's now fixed since:
> > >> https://patchwork.kernel.org/project/linux-pm/patch/81eb2efeeed1556d124065252f32777838a6d850.1589528491.git.viresh.kumar@linaro.org/
> >
> > This should have nothing to do with the OPP driver; lima already has a "mali"
> > regulator consumer that should be enabled whenever the GPU is in use.
>
> Okay so you propose to add a regulator_enable() in the panfrost
> drivers in addition to the OPP consumer right?

After a quick look it seems that it's explicit that we don't want two
consumer for the same regulator and OPP should handle the enabling of
the regulator.

/* OPP will handle regulators */
if (!pfdev->pfdevfreq.opp_of_table_added) {
err = panfrost_regulator_init(pfdev);
if (err)
goto out_devfreq;
}


>
> I will send a patch to add this and see what's panfrost maintainer
> think about it.
>
> Regards,
> Clement
>
> >
> > _Adding_ a regulator consumer from the OPP driver should not somehow decrease
> > the refcount.
> >
> > >> I will drop it.
> > >
> > > After retesting it, it seems to still no take the regulator and make
> > > my board hang... :(
> > >
> > > [   17.698597] sun8i-dw-hdmi 6000000.hdmi: registered DesignWare HDMI
> > > I2C bus driver
> > > [   17.708475] sun4i-drm display-engine: bound 6000000.hdmi (ops
> > > sun8i_dw_hdmi_ops [sun8i_drm_hdmi])
> > > [   17.718350] [drm] Initialized sun4i-drm 1.0.0 20150629 for
> > > display-engine on minor 1
> > > [   17.877443] Console: switching to colour frame buffer device 320x90
> > > [   17.936050] sun4i-drm display-engine: [drm] fb0: sun4i-drmdrmfb
> > > frame buffer device
> > > [   17.961881] platform 5200000.usb: deferred probe pending
> > >>>>> [   31.710731] vdd-gpu: disabling <<<<<
> >
> > Are there any messages from lima, especially about mali-supply? Can you provide
> > regulators_summary from debugfs?
> >
> > Regards,
> > Samuel