[PATCH v3 0/5] Allwinner H6 GPU devfreq

[PATCH v3 0/5] 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[1].

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 v2:
 - Fixes device-tree warnings
 - Add panfrost fix to enable regulator
 - Remove always-on regulator from device-tree
 - Update cooling map from vendor kernel

Clément Péron (5):
  arm64: defconfig: Enable devfreq cooling device
  arm64: dts: allwinner: h6: Add cooling map for GPU
  arm64: dts: allwinner: h6: Add GPU OPP table
  drm/panfrost: devfreq: set opp to the recommended one to configure and
    enable regulator
  arm64: dts: allwinner: beelink-gs1: Enable GPU OPP

 .../dts/allwinner/sun50i-h6-beelink-gs1.dts   |  1 +
 .../boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi | 87 +++++++++++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  | 51 ++++++++++-
 arch/arm64/configs/defconfig                  |  1 +
 drivers/gpu/drm/panfrost/panfrost_devfreq.c   |  8 ++
 5 files changed, 146 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi

-- 
2.34.1

[PATCH v3 1/5] 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>
Acked-by: Jernej Skrabec <jernej.skrabec@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 91e58cf59c99..e557ccac8d9c 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -582,6 +582,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 v3 2/5] arm64: dts: allwinner: h6: Add cooling map for GPU

[Clément Péron]

Add a simple cooling map for the GPU.

This cooling map come from the vendor kernel 4.9 with a
2°C hysteresis added.

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

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 5a28303d3d4c..1259ab0c3956 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";
 		};
 
@@ -1072,9 +1073,55 @@ map0 {
 		};
 
 		gpu-thermal {
-			polling-delay-passive = <0>;
-			polling-delay = <0>;
+			polling-delay-passive = <1000>;
+			polling-delay = <2000>;
 			thermal-sensors = <&ths 1>;
+
+			trips {
+				gpu_alert0: gpu-alert-0 {
+					temperature = <95000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				gpu_alert1: gpu-alert-1 {
+					temperature = <100000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				gpu_alert2: gpu-alert-2 {
+					temperature = <105000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				gpu-crit {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				// Fordid the GPU to go over 756MHz
+				map0 {
+					trip = <&gpu_alert0>;
+					cooling-device = <&gpu 1 THERMAL_NO_LIMIT>;
+				};
+
+				// Fordid the GPU to go over 624MHz
+				map1 {
+					trip = <&gpu_alert1>;
+					cooling-device = <&gpu 2 THERMAL_NO_LIMIT>;
+				};
+
+				// Fordid the GPU to go over 576MHz
+				map2 {
+					trip = <&gpu_alert2>;
+					cooling-device = <&gpu 3 THERMAL_NO_LIMIT>;
+				};
+			};
 		};
 	};
 };
-- 
2.34.1

[PATCH v3 3/5] 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 | 87 +++++++++++++++++++
 1 file changed, 87 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..b48049c4fc85
--- /dev/null
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-gpu-opp.dtsi
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+// Copyright (C) 2022 Clément Péron <peron.clem@gmail.com>
+
+/ {
+	gpu_opp_table: opp-table-gpu {
+		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

[PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator

[Clément Péron]

devm_pm_opp_set_regulators() doesn't enable regulator, which make
regulator framework switching it off during regulator_late_cleanup().

Call dev_pm_opp_set_opp() with the recommend OPP in
panfrost_devfreq_init() to enable the regulator and avoid any switch off
by regulator_late_cleanup().

Suggested-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 5110cd9b2425..67b242407156 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 		return PTR_ERR(opp);
 
 	panfrost_devfreq_profile.initial_freq = cur_freq;
+
+	/* Setup and enable regulator */
+	ret = dev_pm_opp_set_opp(dev, opp);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
+		return ret;
+	}
+
 	dev_pm_opp_put(opp);
 
 	/*
-- 
2.34.1

[PATCH v3 5/5] 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 | 1 +
 1 file changed, 1 insertion(+)

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..9ec49ac2f6fd 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>
 
-- 
2.34.1

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

[Jernej Škrabec]

Dne ponedeljek, 05. september 2022 ob 19:15:58 CEST je Clément Péron 
napisal(a):
> Add a simple cooling map for the GPU.
> 
> This cooling map come from the vendor kernel 4.9 with a
> 2°C hysteresis added.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 51 +++++++++++++++++++-
>  1 file changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi index
> 5a28303d3d4c..1259ab0c3956 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";
>  		};
> 
> @@ -1072,9 +1073,55 @@ map0 {
>  		};
> 
>  		gpu-thermal {
> -			polling-delay-passive = <0>;
> -			polling-delay = <0>;
> +			polling-delay-passive = <1000>;
> +			polling-delay = <2000>;
>  			thermal-sensors = <&ths 1>;
> +
> +			trips {
> +				gpu_alert0: gpu-alert-0 {
> +					temperature = <95000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};
> +
> +				gpu_alert1: gpu-alert-1 {
> +					temperature = 
> <100000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};
> +
> +				gpu_alert2: gpu-alert-2 {
> +					temperature = 
> <105000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};
> +
> +				gpu-crit {
> +					temperature = 
> <115000>;
> +					hysteresis = <0>;
> +					type = "critical";
> +				};
> +			};
> +
> +			cooling-maps {
> +				// Fordid the GPU to go over 
756MHz

Typo: Fordid -> Forbid

Also next below.

Best regards,
Jernej

> +				map0 {
> +					trip = <&gpu_alert0>;
> +					cooling-device = <&gpu
>  1 THERMAL_NO_LIMIT>;
> +				};
> +
> +				// Fordid the GPU to go over
> 624MHz
> +				map1 {
> +					trip = <&gpu_alert1>;
> +					cooling-device = <&gpu 
> 2 THERMAL_NO_LIMIT>;
> +				};
> +
> +				// Fordid the GPU to go over 
> 576MHz
> +				map2 {
> +					trip = <&gpu_alert2>;
> +					cooling-device = <&gpu 
> 3 THERMAL_NO_LIMIT>;
> +				};
> +			};
>  		};
>  	};
>  };
> --
> 2.34.1

RE: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator

[Biju Das]

Hi,

Thanks for the patch.

> Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended
> one to configure and enable regulator
> 
> devm_pm_opp_set_regulators() doesn't enable regulator, which make
> regulator framework switching it off during regulator_late_cleanup().

In that case, why not regulator_get()for 
Dynamic regulator(non fixed regulator)??

> 
> Call dev_pm_opp_set_opp() with the recommend OPP in
> panfrost_devfreq_init() to enable the regulator and avoid any switch off
> by regulator_late_cleanup().
> 
> Suggested-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 5110cd9b2425..67b242407156 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct panfrost_device
> *pfdev)
>  		return PTR_ERR(opp);
> 
>  	panfrost_devfreq_profile.initial_freq = cur_freq;
> +
> +	/* Setup and enable regulator */
> +	ret = dev_pm_opp_set_opp(dev, opp);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> +		return ret;
> +	}


FYI,
On RZ/G2L mali gpu, we have fixed regulator and
I was able to do GPU OPP transition without any issues previously.

root@smarc-rzg2l:~# cat /sys/class/devfreq/11840000.gpu/trans_stat
     From  :   To
           :  50000000  62500000 100000000 125000000 200000000 250000000 400000000 500000000   time(ms)
*  50000000:         0         0         0         0         0         0         0         1       144
   62500000:         0         0         0         0         0         0         0         0         0
  100000000:         0         0         0         0         0         0         0         9       524
  125000000:         0         0         9         0         0         0         0         3      2544
  200000000:         0         0         0        11         0         0         0        46      3304
  250000000:         1         0         0         0        33         0         0         0      7496
  400000000:         0         0         0         0        16        19         0         0      2024
  500000000:         1         0         0         1         8        15        35         0      4032
Total transition : 208

Cheers,
Biju

Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator

[Clément Péron]

Hi,

On Mon, 5 Sept 2022 at 20:17, Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi,
>
> Thanks for the patch.
>
> > Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended
> > one to configure and enable regulator
> >
> > devm_pm_opp_set_regulators() doesn't enable regulator, which make
> > regulator framework switching it off during regulator_late_cleanup().
>
> In that case, why not regulator_get()for
> Dynamic regulator(non fixed regulator)??

Sorry I don't understand, what do you mean?

>
> >
> > Call dev_pm_opp_set_opp() with the recommend OPP in
> > panfrost_devfreq_init() to enable the regulator and avoid any switch off
> > by regulator_late_cleanup().
> >
> > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > index 5110cd9b2425..67b242407156 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct panfrost_device
> > *pfdev)
> >               return PTR_ERR(opp);
> >
> >       panfrost_devfreq_profile.initial_freq = cur_freq;
> > +
> > +     /* Setup and enable regulator */
> > +     ret = dev_pm_opp_set_opp(dev, opp);
> > +     if (ret) {
> > +             DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> > +             return ret;
> > +     }
>
>
> FYI,
> On RZ/G2L mali gpu, we have fixed regulator and
> I was able to do GPU OPP transition without any issues previously.

rzg2l-smarc-som.dtsi uses regulator reg_1p1v;
which is marked as regulator-always-on; that's why
regulator_late_cleanup() doesn't switch it off.

Regards,
Clement

>
> root@smarc-rzg2l:~# cat /sys/class/devfreq/11840000.gpu/trans_stat
>      From  :   To
>            :  50000000  62500000 100000000 125000000 200000000 250000000 400000000 500000000   time(ms)
> *  50000000:         0         0         0         0         0         0         0         1       144
>    62500000:         0         0         0         0         0         0         0         0         0
>   100000000:         0         0         0         0         0         0         0         9       524
>   125000000:         0         0         9         0         0         0         0         3      2544
>   200000000:         0         0         0        11         0         0         0        46      3304
>   250000000:         1         0         0         0        33         0         0         0      7496
>   400000000:         0         0         0         0        16        19         0         0      2024
>   500000000:         1         0         0         1         8        15        35         0      4032
> Total transition : 208
>
> Cheers,
> Biju
>

Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator

[Viresh Kumar]

Your subject is 87 columns long, better to squeeze it a bit.

On 05-09-22, 19:16, Clément Péron wrote:
> devm_pm_opp_set_regulators() doesn't enable regulator, which make
> regulator framework switching it off during regulator_late_cleanup().

This isn't the normal behavior as it works for everyone at the moment.
You need to explain what special you are doing here, because of which
you are reaching such a situation.

i.e. you are disabling some code that uses GPU ? Please specify exact
code so others can reproduce it as well.

> Call dev_pm_opp_set_opp() with the recommend OPP in
> panfrost_devfreq_init() to enable the regulator and avoid any switch off
> by regulator_late_cleanup().

The regulator is already enabled I think at this point by the
bootloader. What you are doing here is syncing the state of the
hardware with the software, which would disallow disabling of the
resource unnecessarily.

> Suggested-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 5110cd9b2425..67b242407156 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>  		return PTR_ERR(opp);
>  
>  	panfrost_devfreq_profile.initial_freq = cur_freq;
> +
> +	/* Setup and enable regulator */

Similarly here, explain why this is required to be done.

> +	ret = dev_pm_opp_set_opp(dev, opp);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> +		return ret;
> +	}
> +
>  	dev_pm_opp_put(opp);

Do this before checking if (ret), so the resource can be freed all the
time.

>  
>  	/*
> -- 
> 2.34.1

-- 
viresh

RE: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator

[Biju Das]

Hi Clement,

> 
> Hi,
> 
> On Mon, 5 Sept 2022 at 20:17, Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> >
> > Hi,
> >
> > Thanks for the patch.
> >
> > > Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the
> > > recommended one to configure and enable regulator
> > >
> > > devm_pm_opp_set_regulators() doesn't enable regulator, which make
> > > regulator framework switching it off during regulator_late_cleanup().
> >
> > In that case, why not regulator_get()for Dynamic regulator(non fixed
> > regulator)??
> 
> Sorry I don't understand, what do you mean?

Normally we need to turn on regulator and clock only when needed.
I am not sure with your new code, will make it always on and
drains the power unnecessarily and does it set lower opp or higher
opp at the start??

Compared to the fixed regulator, you have voltage regulator to
control that is the difference between my environment and
Your environment.

I am not sure any other SoC is using voltage regulator??
If yes, thenthere should be some bug or some difference in HW
which is giving different behaviour??

If you are the first one using voltage regulator with mali gpu,
Then Your implementation may be correct, as you have proper
HW to check.

> 
> >
> > >
> > > Call dev_pm_opp_set_opp() with the recommend OPP in
> > > panfrost_devfreq_init() to enable the regulator and avoid any switch
> > > off by regulator_late_cleanup().
> > >
> > > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > ---
> > >  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > index 5110cd9b2425..67b242407156 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct
> > > panfrost_device
> > > *pfdev)
> > >               return PTR_ERR(opp);
> > >
> > >       panfrost_devfreq_profile.initial_freq = cur_freq;
> > > +
> > > +     /* Setup and enable regulator */
> > > +     ret = dev_pm_opp_set_opp(dev, opp);
> > > +     if (ret) {
> > > +             DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> > > +             return ret;
> > > +     }
> >
> >
> > FYI,
> > On RZ/G2L mali gpu, we have fixed regulator and I was able to do GPU
> > OPP transition without any issues previously.
> 
> rzg2l-smarc-som.dtsi uses regulator reg_1p1v; which is marked as
> regulator-always-on; that's why
> regulator_late_cleanup() doesn't switch it off.

Yes that is correct. It is fixed regulator and always on.
We control only frequency.

Cheers,
Biju

> 
> >
> > root@smarc-rzg2l:~# cat /sys/class/devfreq/11840000.gpu/trans_stat
> >      From  :   To
> >            :  50000000  62500000 100000000 125000000 200000000
> 250000000 400000000 500000000   time(ms)
> > *  50000000:         0         0         0         0         0
> 0         0         1       144
> >    62500000:         0         0         0         0         0
> 0         0         0         0
> >   100000000:         0         0         0         0         0
> 0         0         9       524
> >   125000000:         0         0         9         0         0
> 0         0         3      2544
> >   200000000:         0         0         0        11         0
> 0         0        46      3304
> >   250000000:         1         0         0         0        33
> 0         0         0      7496
> >   400000000:         0         0         0         0        16
> 19         0         0      2024
> >   500000000:         1         0         0         1         8
> 15        35         0      4032
> > Total transition : 208
> >
> > Cheers,
> > Biju
> >

Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator

[Clément Péron]

Hi Biju,

On Tue, 6 Sept 2022 at 08:42, Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Clement,
>
> >
> > Hi,
> >
> > On Mon, 5 Sept 2022 at 20:17, Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > >
> > > Hi,
> > >
> > > Thanks for the patch.
> > >
> > > > Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the
> > > > recommended one to configure and enable regulator
> > > >
> > > > devm_pm_opp_set_regulators() doesn't enable regulator, which make
> > > > regulator framework switching it off during regulator_late_cleanup().
> > >
> > > In that case, why not regulator_get()for Dynamic regulator(non fixed
> > > regulator)??
> >
> > Sorry I don't understand, what do you mean?
>
> Normally we need to turn on regulator and clock only when needed.
> I am not sure with your new code, will make it always on and
> drains the power unnecessarily and does it set lower opp or higher
> opp at the start??

The code doesn't make it always on, it makes it how it should be at
the recommended OPP which is the "start point".

If the recommended OPP says to switch off the regulator then it will.

>
> Compared to the fixed regulator, you have voltage regulator to
> control that is the difference between my environment and
> Your environment.
>
> I am not sure any other SoC is using voltage regulator??
> If yes, thenthere should be some bug or some difference in HW
> which is giving different behaviour??
>
> If you are the first one using voltage regulator with mali gpu,
> Then Your implementation may be correct, as you have proper
> HW to check.

The issue is that my regulator is not marked as "always-on", if no OPP
is called before regulator_late_cleanup() then nobody sets the
regulator_enable() and the regulator is switched off, which makes my
board hang.

Like Viresh recommends I will send an update with more details in the
commit log.

Regards,
Clement


>
> >
> > >
> > > >
> > > > Call dev_pm_opp_set_opp() with the recommend OPP in
> > > > panfrost_devfreq_init() to enable the regulator and avoid any switch
> > > > off by regulator_late_cleanup().
> > > >
> > > > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > ---
> > > >  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > > index 5110cd9b2425..67b242407156 100644
> > > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > > @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct
> > > > panfrost_device
> > > > *pfdev)
> > > >               return PTR_ERR(opp);
> > > >
> > > >       panfrost_devfreq_profile.initial_freq = cur_freq;
> > > > +
> > > > +     /* Setup and enable regulator */
> > > > +     ret = dev_pm_opp_set_opp(dev, opp);
> > > > +     if (ret) {
> > > > +             DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> > > > +             return ret;
> > > > +     }
> > >
> > >
> > > FYI,
> > > On RZ/G2L mali gpu, we have fixed regulator and I was able to do GPU
> > > OPP transition without any issues previously.
> >
> > rzg2l-smarc-som.dtsi uses regulator reg_1p1v; which is marked as
> > regulator-always-on; that's why
> > regulator_late_cleanup() doesn't switch it off.
>
> Yes that is correct. It is fixed regulator and always on.
> We control only frequency.
>
> Cheers,
> Biju
>
> >
> > >
> > > root@smarc-rzg2l:~# cat /sys/class/devfreq/11840000.gpu/trans_stat
> > >      From  :   To
> > >            :  50000000  62500000 100000000 125000000 200000000
> > 250000000 400000000 500000000   time(ms)
> > > *  50000000:         0         0         0         0         0
> > 0         0         1       144
> > >    62500000:         0         0         0         0         0
> > 0         0         0         0
> > >   100000000:         0         0         0         0         0
> > 0         0         9       524
> > >   125000000:         0         0         9         0         0
> > 0         0         3      2544
> > >   200000000:         0         0         0        11         0
> > 0         0        46      3304
> > >   250000000:         1         0         0         0        33
> > 0         0         0      7496
> > >   400000000:         0         0         0         0        16
> > 19         0         0      2024
> > >   500000000:         1         0         0         1         8
> > 15        35         0      4032
> > > Total transition : 208
> > >
> > > Cheers,
> > > Biju
> > >

RE: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator

[Biju Das]

Hi Clement,

> Subject: Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the
> recommended one to configure and enable regulator
> 
> Hi Biju,
> 
> On Tue, 6 Sept 2022 at 08:42, Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> >
> > Hi Clement,
> >
> > >
> > > Hi,
> > >
> > > On Mon, 5 Sept 2022 at 20:17, Biju Das <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > >
> > > > Hi,
> > > >
> > > > Thanks for the patch.
> > > >
> > > > > Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the
> > > > > recommended one to configure and enable regulator
> > > > >
> > > > > devm_pm_opp_set_regulators() doesn't enable regulator, which
> > > > > make regulator framework switching it off during
> regulator_late_cleanup().
> > > >
> > > > In that case, why not regulator_get()for Dynamic regulator(non
> > > > fixed regulator)??
> > >
> > > Sorry I don't understand, what do you mean?
> >
> > Normally we need to turn on regulator and clock only when needed.
> > I am not sure with your new code, will make it always on and drains
> > the power unnecessarily and does it set lower opp or higher opp at the
> > start??
> 
> The code doesn't make it always on, it makes it how it should be at the
> recommended OPP which is the "start point".
> 
> If the recommended OPP says to switch off the regulator then it will.
> 
> >
> > Compared to the fixed regulator, you have voltage regulator to control
> > that is the difference between my environment and Your environment.
> >
> > I am not sure any other SoC is using voltage regulator??
> > If yes, thenthere should be some bug or some difference in HW which is
> > giving different behaviour??
> >
> > If you are the first one using voltage regulator with mali gpu, Then
> > Your implementation may be correct, as you have proper HW to check.
> 
> The issue is that my regulator is not marked as "always-on", if no OPP is
> called before regulator_late_cleanup() then nobody sets the
> regulator_enable() and the regulator is switched off, which makes my
> board hang.

Cool, From your testing looks like no one tested this feature with
mali GPU on mainline??

Cheers,
Biju


> 
> Like Viresh recommends I will send an update with more details in the
> commit log.
> 
> Regards,
> Clement
> 
> 
> >
> > >
> > > >
> > > > >
> > > > > Call dev_pm_opp_set_opp() with the recommend OPP in
> > > > > panfrost_devfreq_init() to enable the regulator and avoid any
> > > > > switch off by regulator_late_cleanup().
> > > > >
> > > > > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > > ---
> > > > >  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 ++++++++
> > > > >  1 file changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > > > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > > > index 5110cd9b2425..67b242407156 100644
> > > > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > > > @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct
> > > > > panfrost_device
> > > > > *pfdev)
> > > > >               return PTR_ERR(opp);
> > > > >
> > > > >       panfrost_devfreq_profile.initial_freq = cur_freq;
> > > > > +
> > > > > +     /* Setup and enable regulator */
> > > > > +     ret = dev_pm_opp_set_opp(dev, opp);
> > > > > +     if (ret) {
> > > > > +             DRM_DEV_ERROR(dev, "Couldn't set recommended
> OPP\n");
> > > > > +             return ret;
> > > > > +     }
> > > >
> > > >
> > > > FYI,
> > > > On RZ/G2L mali gpu, we have fixed regulator and I was able to do
> > > > GPU OPP transition without any issues previously.
> > >
> > > rzg2l-smarc-som.dtsi uses regulator reg_1p1v; which is marked as
> > > regulator-always-on; that's why
> > > regulator_late_cleanup() doesn't switch it off.
> >
> > Yes that is correct. It is fixed regulator and always on.
> > We control only frequency.
> >
> > Cheers,
> > Biju
> >
> > >
> > > >
> > > > root@smarc-rzg2l:~# cat /sys/class/devfreq/11840000.gpu/trans_stat
> > > >      From  :   To
> > > >            :  50000000  62500000 100000000 125000000 200000000
> > > 250000000 400000000 500000000   time(ms)
> > > > *  50000000:         0         0         0         0         0
> > > 0         0         1       144
> > > >    62500000:         0         0         0         0         0
> > > 0         0         0         0
> > > >   100000000:         0         0         0         0         0
> > > 0         0         9       524
> > > >   125000000:         0         0         9         0         0
> > > 0         0         3      2544
> > > >   200000000:         0         0         0        11         0
> > > 0         0        46      3304
> > > >   250000000:         1         0         0         0        33
> > > 0         0         0      7496
> > > >   400000000:         0         0         0         0        16
> > > 19         0         0      2024
> > > >   500000000:         1         0         0         1         8
> > > 15        35         0      4032
> > > > Total transition : 208
> > > >
> > > > Cheers,
> > > > Biju
> > > >

Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the recommended one to configure and enable regulator

[Clément Péron]

Hi,

On Tue, 6 Sept 2022 at 10:06, Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Clement,
>
> > Subject: Re: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the
> > recommended one to configure and enable regulator
> >
> > Hi Biju,
> >
> > On Tue, 6 Sept 2022 at 08:42, Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > >
> > > Hi Clement,
> > >
> > > >
> > > > Hi,
> > > >
> > > > On Mon, 5 Sept 2022 at 20:17, Biju Das <biju.das.jz@bp.renesas.com>
> > > > wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > Thanks for the patch.
> > > > >
> > > > > > Subject: [PATCH v3 4/5] drm/panfrost: devfreq: set opp to the
> > > > > > recommended one to configure and enable regulator
> > > > > >
> > > > > > devm_pm_opp_set_regulators() doesn't enable regulator, which
> > > > > > make regulator framework switching it off during
> > regulator_late_cleanup().
> > > > >
> > > > > In that case, why not regulator_get()for Dynamic regulator(non
> > > > > fixed regulator)??
> > > >
> > > > Sorry I don't understand, what do you mean?
> > >
> > > Normally we need to turn on regulator and clock only when needed.
> > > I am not sure with your new code, will make it always on and drains
> > > the power unnecessarily and does it set lower opp or higher opp at the
> > > start??
> >
> > The code doesn't make it always on, it makes it how it should be at the
> > recommended OPP which is the "start point".
> >
> > If the recommended OPP says to switch off the regulator then it will.
> >
> > >
> > > Compared to the fixed regulator, you have voltage regulator to control
> > > that is the difference between my environment and Your environment.
> > >
> > > I am not sure any other SoC is using voltage regulator??
> > > If yes, thenthere should be some bug or some difference in HW which is
> > > giving different behaviour??
> > >
> > > If you are the first one using voltage regulator with mali gpu, Then
> > > Your implementation may be correct, as you have proper HW to check.
> >
> > The issue is that my regulator is not marked as "always-on", if no OPP is
> > called before regulator_late_cleanup() then nobody sets the
> > regulator_enable() and the regulator is switched off, which makes my
> > board hang.
>
> Cool, From your testing looks like no one tested this feature with
> mali GPU on mainline??

Or no one without always-on.

Clement

>
> Cheers,
> Biju
>
>
> >
> > Like Viresh recommends I will send an update with more details in the
> > commit log.
> >
> > Regards,
> > Clement
> >
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > Call dev_pm_opp_set_opp() with the recommend OPP in
> > > > > > panfrost_devfreq_init() to enable the regulator and avoid any
> > > > > > switch off by regulator_late_cleanup().
> > > > > >
> > > > > > Suggested-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 ++++++++
> > > > > >  1 file changed, 8 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > > > > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > > > > index 5110cd9b2425..67b242407156 100644
> > > > > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > > > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > > > > @@ -131,6 +131,14 @@ int panfrost_devfreq_init(struct
> > > > > > panfrost_device
> > > > > > *pfdev)
> > > > > >               return PTR_ERR(opp);
> > > > > >
> > > > > >       panfrost_devfreq_profile.initial_freq = cur_freq;
> > > > > > +
> > > > > > +     /* Setup and enable regulator */
> > > > > > +     ret = dev_pm_opp_set_opp(dev, opp);
> > > > > > +     if (ret) {
> > > > > > +             DRM_DEV_ERROR(dev, "Couldn't set recommended
> > OPP\n");
> > > > > > +             return ret;
> > > > > > +     }
> > > > >
> > > > >
> > > > > FYI,
> > > > > On RZ/G2L mali gpu, we have fixed regulator and I was able to do
> > > > > GPU OPP transition without any issues previously.
> > > >
> > > > rzg2l-smarc-som.dtsi uses regulator reg_1p1v; which is marked as
> > > > regulator-always-on; that's why
> > > > regulator_late_cleanup() doesn't switch it off.
> > >
> > > Yes that is correct. It is fixed regulator and always on.
> > > We control only frequency.
> > >
> > > Cheers,
> > > Biju
> > >
> > > >
> > > > >
> > > > > root@smarc-rzg2l:~# cat /sys/class/devfreq/11840000.gpu/trans_stat
> > > > >      From  :   To
> > > > >            :  50000000  62500000 100000000 125000000 200000000
> > > > 250000000 400000000 500000000   time(ms)
> > > > > *  50000000:         0         0         0         0         0
> > > > 0         0         1       144
> > > > >    62500000:         0         0         0         0         0
> > > > 0         0         0         0
> > > > >   100000000:         0         0         0         0         0
> > > > 0         0         9       524
> > > > >   125000000:         0         0         9         0         0
> > > > 0         0         3      2544
> > > > >   200000000:         0         0         0        11         0
> > > > 0         0        46      3304
> > > > >   250000000:         1         0         0         0        33
> > > > 0         0         0      7496
> > > > >   400000000:         0         0         0         0        16
> > > > 19         0         0      2024
> > > > >   500000000:         1         0         0         1         8
> > > > 15        35         0      4032
> > > > > Total transition : 208
> > > > >
> > > > > Cheers,
> > > > > Biju
> > > > >