[PATCH 0/3] Simple DVFS support for Allwinner A64 SoC

[PATCH 0/3] Simple DVFS support for Allwinner A64 SoC

[Icenowy Zheng]

This patchset imports simple DVFS support for Allwinner A64 SoC.

As the thermal sensor driver is not yet implemented and some boards
have still no AXP PMIC support, now only two OPPs are present --
648MHz@1.04V and 816MHz@1.1V to prevent overheat or undervoltage.

PATCH 1 is a fix to the CCU driver of A64, and the remaining patches
set up the device tree bits of the DVFS on Pine64.

Icenowy Zheng (3):
  clk: sunxi-ng: add mux and pll notifiers for A64 CPU clock
  arm64: allwinner: a64: add CPU opp table
  arm64: allwinner: a64: set CPU regulator for Pine64

 .../arm64/boot/dts/allwinner/sun50i-a64-pine64.dts |  4 ++++
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi      | 24 +++++++++++++++++++
 drivers/clk/sunxi-ng/ccu-sun50i-a64.c              | 28 +++++++++++++++++++++-
 3 files changed, 55 insertions(+), 1 deletion(-)

-- 
2.13.5

[PATCH 1/3] clk: sunxi-ng: add mux and pll notifiers for A64 CPU clock

[Icenowy Zheng]

The A64 PLL_CPU clock has the same instability if some factor changed
without the PLL gated like other SoCs with sun6i-style CCU, e.g. A33,
H3.

Add the mux and pll notifiers for A64 CPU clock to workaround the
problem.

Fixes: c6a0637460c2 ("clk: sunxi-ng: Add A64 clocks")
Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
index 2bb4cabf802f..b55fa69dd0c1 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -879,11 +879,26 @@ static const struct sunxi_ccu_desc sun50i_a64_ccu_desc = {
 	.num_resets	= ARRAY_SIZE(sun50i_a64_ccu_resets),
 };
 
+static struct ccu_pll_nb sun50i_a64_pll_cpu_nb = {
+	.common	= &pll_cpux_clk.common,
+	/* copy from pll_cpux_clk */
+	.enable	= BIT(31),
+	.lock	= BIT(28),
+};
+
+static struct ccu_mux_nb sun50i_a64_cpu_nb = {
+	.common		= &cpux_clk.common,
+	.cm		= &cpux_clk.mux,
+	.delay_us	= 1, /* > 8 clock cycles at 24 MHz */
+	.bypass_index	= 1, /* index of 24 MHz oscillator */
+};
+
 static int sun50i_a64_ccu_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	void __iomem *reg;
 	u32 val;
+	int ret;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	reg = devm_ioremap_resource(&pdev->dev, res);
@@ -897,7 +912,18 @@ static int sun50i_a64_ccu_probe(struct platform_device *pdev)
 
 	writel(0x515, reg + SUN50I_A64_PLL_MIPI_REG);
 
-	return sunxi_ccu_probe(pdev->dev.of_node, reg, &sun50i_a64_ccu_desc);
+	ret = sunxi_ccu_probe(pdev->dev.of_node, reg, &sun50i_a64_ccu_desc);
+	if (ret)
+		return ret;
+
+	/* Gate then ungate PLL CPU after any rate changes */
+	ccu_pll_notifier_register(&sun50i_a64_pll_cpu_nb);
+
+	/* Reparent CPU during PLL CPU rate changes */
+	ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk,
+				  &sun50i_a64_cpu_nb);
+
+	return 0;
 }
 
 static const struct of_device_id sun50i_a64_ccu_ids[] = {
-- 
2.13.5

[PATCH 2/3] arm64: allwinner: a64: add CPU opp table

[Icenowy Zheng]

Add the operating table for the CPU (ARM cores) on Allwinner A64 SoC.

OPPs higher to 816MHz is temporarily dropped, to prevent overheat on
boards with AXP803 support and undervoltage on boards without AXP803
support.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 20aba7b186aa..0532da4939eb 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -52,6 +52,23 @@
 	#address-cells = <1>;
 	#size-cells = <1>;
 
+	cpu0_opp_table: opp_table0 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		opp-648000000 {
+			opp-hz = /bits/ 64 <648000000>;
+			opp-microvolt = <1040000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+		};
+
+		opp-816000000 {
+			opp-hz = /bits/ 64 <816000000>;
+			opp-microvolt = <1100000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+		};
+	};
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
@@ -61,6 +78,10 @@
 			device_type = "cpu";
 			reg = <0>;
 			enable-method = "psci";
+			clocks = <&ccu CLK_CPUX>;
+			clock-names = "cpu";
+			operating-points-v2 = <&cpu0_opp_table>;
+			#cooling-cells = <2>;
 		};
 
 		cpu1: cpu@1 {
@@ -68,6 +89,7 @@
 			device_type = "cpu";
 			reg = <1>;
 			enable-method = "psci";
+			operating-points-v2 = <&cpu0_opp_table>;
 		};
 
 		cpu2: cpu@2 {
@@ -75,6 +97,7 @@
 			device_type = "cpu";
 			reg = <2>;
 			enable-method = "psci";
+			operating-points-v2 = <&cpu0_opp_table>;
 		};
 
 		cpu3: cpu@3 {
@@ -82,6 +105,7 @@
 			device_type = "cpu";
 			reg = <3>;
 			enable-method = "psci";
+			operating-points-v2 = <&cpu0_opp_table>;
 		};
 	};
 
-- 
2.13.5

[PATCH 3/3] arm64: allwinner: a64: set CPU regulator for Pine64

[Icenowy Zheng]

The DCDC2 regulator of the AXP803 PMIC is used for the voltage scaling
of the ARM cores on the A64 SoC.

Add this definition to enable it on Pine64.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
index d06e34b5d192..cc77503091f1 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
@@ -63,6 +63,10 @@
 	};
 };
 
+&cpu0 {
+	cpu-supply = <&reg_dcdc2>;
+};
+
 &ehci0 {
 	status = "okay";
 };
-- 
2.13.5

Re: [PATCH 0/3] Simple DVFS support for Allwinner A64 SoC

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 687 bytes --]

Hi,

On Sat, Sep 23, 2017 at 12:15:28AM +0000, Icenowy Zheng wrote:
> This patchset imports simple DVFS support for Allwinner A64 SoC.
> 
> As the thermal sensor driver is not yet implemented and some boards
> have still no AXP PMIC support, now only two OPPs are present --
> 648MHz@1.04V and 816MHz@1.1V to prevent overheat or undervoltage.
> 
> PATCH 1 is a fix to the CCU driver of A64, and the remaining patches
> set up the device tree bits of the DVFS on Pine64.

How has this been tested?

What tasks did you run, with what governor, etc...

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 0/3] Simple DVFS support for Allwinner A64 SoC

[Icenowy Zheng]

于 2017年9月25日 GMT+08:00 下午6:10:27, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>Hi,
>
>On Sat, Sep 23, 2017 at 12:15:28AM +0000, Icenowy Zheng wrote:
>> This patchset imports simple DVFS support for Allwinner A64 SoC.
>> 
>> As the thermal sensor driver is not yet implemented and some boards
>> have still no AXP PMIC support, now only two OPPs are present --
>> 648MHz@1.04V and 816MHz@1.1V to prevent overheat or undervoltage.
>> 
>> PATCH 1 is a fix to the CCU driver of A64, and the remaining patches
>> set up the device tree bits of the DVFS on Pine64.
>
>How has this been tested?
>
>What tasks did you run, with what governor, etc...

I only tested manual frequency switching between 648MHz and
816MHz, and tested the PLL stuck issue by change the OPPs to
some random value.

>
>Thanks!
>Maxime

Re: [PATCH 0/3] Simple DVFS support for Allwinner A64 SoC

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1338 bytes --]

On Mon, Sep 25, 2017 at 10:12:09AM +0000, Icenowy Zheng wrote:
> 于 2017年9月25日 GMT+08:00 下午6:10:27, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
> >Hi,
> >
> >On Sat, Sep 23, 2017 at 12:15:28AM +0000, Icenowy Zheng wrote:
> >> This patchset imports simple DVFS support for Allwinner A64 SoC.
> >> 
> >> As the thermal sensor driver is not yet implemented and some boards
> >> have still no AXP PMIC support, now only two OPPs are present --
> >> 648MHz@1.04V and 816MHz@1.1V to prevent overheat or undervoltage.
> >> 
> >> PATCH 1 is a fix to the CCU driver of A64, and the remaining patches
> >> set up the device tree bits of the DVFS on Pine64.
> >
> >How has this been tested?
> >
> >What tasks did you run, with what governor, etc...
> 
> I only tested manual frequency switching between 648MHz and
> 816MHz, and tested the PLL stuck issue by change the OPPs to
> some random value.

Ideally, we should test that it's actually reliable. Poorly chosen
OPPs might lead to corrupt data that you might not get before a while.

Please test using:
https://linux-sunxi.org/Hardware_Reliability_Tests#Reliability_of_cpufreq_voltage.2Ffrequency_settings

And post the report.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [linux-sunxi] Re: [PATCH 0/3] Simple DVFS support for Allwinner A64 SoC

[Icenowy Zheng]

于 2017年9月25日 GMT+08:00 下午6:27:44, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>On Mon, Sep 25, 2017 at 10:12:09AM +0000, Icenowy Zheng wrote:
>> 于 2017年9月25日 GMT+08:00 下午6:10:27, Maxime Ripard
><maxime.ripard@free-electrons.com> 写到:
>> >Hi,
>> >
>> >On Sat, Sep 23, 2017 at 12:15:28AM +0000, Icenowy Zheng wrote:
>> >> This patchset imports simple DVFS support for Allwinner A64 SoC.
>> >> 
>> >> As the thermal sensor driver is not yet implemented and some
>boards
>> >> have still no AXP PMIC support, now only two OPPs are present --
>> >> 648MHz@1.04V and 816MHz@1.1V to prevent overheat or undervoltage.
>> >> 
>> >> PATCH 1 is a fix to the CCU driver of A64, and the remaining
>patches
>> >> set up the device tree bits of the DVFS on Pine64.
>> >
>> >How has this been tested?
>> >
>> >What tasks did you run, with what governor, etc...
>> 
>> I only tested manual frequency switching between 648MHz and
>> 816MHz, and tested the PLL stuck issue by change the OPPs to
>> some random value.
>
>Ideally, we should test that it's actually reliable. Poorly chosen
>OPPs might lead to corrupt data that you might not get before a while.

These are OPPs from the official sys_config.fex .

>
>Please test using:
>https://linux-sunxi.org/Hardware_Reliability_Tests#Reliability_of_cpufreq_voltage.2Ffrequency_settings
>
>And post the report.
>
>Maxime

Re: [linux-sunxi] Re: [PATCH 0/3] Simple DVFS support for Allwinner A64 SoC

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]

On Mon, Sep 25, 2017 at 10:29:38AM +0000, Icenowy Zheng wrote:
> 
> 
> 于 2017年9月25日 GMT+08:00 下午6:27:44, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
> >On Mon, Sep 25, 2017 at 10:12:09AM +0000, Icenowy Zheng wrote:
> >> 于 2017年9月25日 GMT+08:00 下午6:10:27, Maxime Ripard
> ><maxime.ripard@free-electrons.com> 写到:
> >> >Hi,
> >> >
> >> >On Sat, Sep 23, 2017 at 12:15:28AM +0000, Icenowy Zheng wrote:
> >> >> This patchset imports simple DVFS support for Allwinner A64 SoC.
> >> >> 
> >> >> As the thermal sensor driver is not yet implemented and some
> >boards
> >> >> have still no AXP PMIC support, now only two OPPs are present --
> >> >> 648MHz@1.04V and 816MHz@1.1V to prevent overheat or undervoltage.
> >> >> 
> >> >> PATCH 1 is a fix to the CCU driver of A64, and the remaining
> >patches
> >> >> set up the device tree bits of the DVFS on Pine64.
> >> >
> >> >How has this been tested?
> >> >
> >> >What tasks did you run, with what governor, etc...
> >> 
> >> I only tested manual frequency switching between 648MHz and
> >> 816MHz, and tested the PLL stuck issue by change the OPPs to
> >> some random value.
> >
> >Ideally, we should test that it's actually reliable. Poorly chosen
> >OPPs might lead to corrupt data that you might not get before a while.
> 
> These are OPPs from the official sys_config.fex .

And the rest of the code isn't, such as the clock or regulator code
that is critical as well here.

I'm not asking this out of nowhere, we've had to debug this more than
once already.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [linux-sunxi] [PATCH 2/3] arm64: allwinner: a64: add CPU opp table

[Samuel Holland]

Hello,

On 09/22/17 19:15, Icenowy Zheng wrote:
> Add the operating table for the CPU (ARM cores) on Allwinner A64 SoC.
> 
> OPPs higher to 816MHz is temporarily dropped, to prevent overheat on
> boards with AXP803 support and undervoltage on boards without AXP803
> support.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
>   arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 20aba7b186aa..0532da4939eb 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -52,6 +52,23 @@
>   	#address-cells = <1>;
>   	#size-cells = <1>;
>   
> +	cpu0_opp_table: opp_table0 {
> +		compatible = "operating-points-v2";
> +		opp-shared;
> +
> +		opp-648000000 {
> +			opp-hz = /bits/ 64 <648000000>;
> +			opp-microvolt = <1040000>;
> +			clock-latency-ns = <244144>; /* 8 32k periods */
> +		};
> +
> +		opp-816000000 {
> +			opp-hz = /bits/ 64 <816000000>;
> +			opp-microvolt = <1100000>;
> +			clock-latency-ns = <244144>; /* 8 32k periods */
> +		};
> +	};
> +
>   	cpus {
>   		#address-cells = <1>;
>   		#size-cells = <0>;
> @@ -61,6 +78,10 @@
>   			device_type = "cpu";
>   			reg = <0>;
>   			enable-method = "psci";
> +			clocks = <&ccu CLK_CPUX>;
CLK_CPUX is not currently exposed in the dt-bindings header, so this
doesn't compile.

> +			clock-names = "cpu";
> +			operating-points-v2 = <&cpu0_opp_table>;
> +			#cooling-cells = <2>;
>   		};
>   
>   		cpu1: cpu@1 {
> @@ -68,6 +89,7 @@
>   			device_type = "cpu";
>   			reg = <1>;
>   			enable-method = "psci";
> +			operating-points-v2 = <&cpu0_opp_table>;
>   		};
>   
>   		cpu2: cpu@2 {
> @@ -75,6 +97,7 @@
>   			device_type = "cpu";
>   			reg = <2>;
>   			enable-method = "psci";
> +			operating-points-v2 = <&cpu0_opp_table>;
>   		};
>   
>   		cpu3: cpu@3 {
> @@ -82,6 +105,7 @@
>   			device_type = "cpu";
>   			reg = <3>;
>   			enable-method = "psci";
> +			operating-points-v2 = <&cpu0_opp_table>;
>   		};
>   	};
>   
> 

Thanks,
Samuel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] Simple DVFS support for Allwinner A64 SoC

[icenowy]

在 2017-09-25 18:27，Maxime Ripard 写道：
> On Mon, Sep 25, 2017 at 10:12:09AM +0000, Icenowy Zheng wrote:
>> 于 2017年9月25日 GMT+08:00 下午6:10:27, Maxime Ripard 
>> <maxime.ripard@free-electrons.com> 写到:
>> >Hi,
>> >
>> >On Sat, Sep 23, 2017 at 12:15:28AM +0000, Icenowy Zheng wrote:
>> >> This patchset imports simple DVFS support for Allwinner A64 SoC.
>> >>
>> >> As the thermal sensor driver is not yet implemented and some boards
>> >> have still no AXP PMIC support, now only two OPPs are present --
>> >> 648MHz@1.04V and 816MHz@1.1V to prevent overheat or undervoltage.
>> >>
>> >> PATCH 1 is a fix to the CCU driver of A64, and the remaining patches
>> >> set up the device tree bits of the DVFS on Pine64.
>> >
>> >How has this been tested?
>> >
>> >What tasks did you run, with what governor, etc...
>> 
>> I only tested manual frequency switching between 648MHz and
>> 816MHz, and tested the PLL stuck issue by change the OPPs to
>> some random value.
> 
> Ideally, we should test that it's actually reliable. Poorly chosen
> OPPs might lead to corrupt data that you might not get before a while.
> 
> Please test using:
> https://linux-sunxi.org/Hardware_Reliability_Tests#Reliability_of_cpufreq_voltage.2Ffrequency_settings
> 
> And post the report.

```
root@p64 [ cpuburn-arm@master ] # ./cpuburn-a53 &
[1] 2543
root@p64 [ cpuburn-arm@master ] # ./cpufreq-ljt-stress-test
Creating './whitenoise-1920x1080.jpg' ... done
CPU stress test, which is doing JPEG decoding by libjpeg-turbo
at different cpufreq operating points.

Testing CPU 0
   816 MHz ............................................................ 
OK
   648 MHz ............................................................ 
OK

Testing CPU 1
   816 MHz ............................................................ 
OK
   648 MHz ............................................................ 
OK

Testing CPU 2
   816 MHz ............................................................ 
OK
   648 MHz ............................................................ 
OK

Testing CPU 3
   816 MHz ............................................................ 
OK
   648 MHz ............................................................ 
OK

Overall result : PASSED
```

> 
> Maxime

Re: [PATCH 1/3] clk: sunxi-ng: add mux and pll notifiers for A64 CPU clock

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2462 bytes --]

Hi,

On Sat, Sep 23, 2017 at 12:15:29AM +0000, Icenowy Zheng wrote:
> The A64 PLL_CPU clock has the same instability if some factor changed
> without the PLL gated like other SoCs with sun6i-style CCU, e.g. A33,
> H3.
> 
> Add the mux and pll notifiers for A64 CPU clock to workaround the
> problem.
> 
> Fixes: c6a0637460c2 ("clk: sunxi-ng: Add A64 clocks")
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
>  drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> index 2bb4cabf802f..b55fa69dd0c1 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> @@ -879,11 +879,26 @@ static const struct sunxi_ccu_desc sun50i_a64_ccu_desc = {
>  	.num_resets	= ARRAY_SIZE(sun50i_a64_ccu_resets),
>  };
>  
> +static struct ccu_pll_nb sun50i_a64_pll_cpu_nb = {
> +	.common	= &pll_cpux_clk.common,
> +	/* copy from pll_cpux_clk */
> +	.enable	= BIT(31),
> +	.lock	= BIT(28),
> +};
> +
> +static struct ccu_mux_nb sun50i_a64_cpu_nb = {
> +	.common		= &cpux_clk.common,
> +	.cm		= &cpux_clk.mux,
> +	.delay_us	= 1, /* > 8 clock cycles at 24 MHz */
> +	.bypass_index	= 1, /* index of 24 MHz oscillator */
> +};
> +
>
>  static int sun50i_a64_ccu_probe(struct platform_device *pdev)
>  {
>  	struct resource *res;
>  	void __iomem *reg;
>  	u32 val;
> +	int ret;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	reg = devm_ioremap_resource(&pdev->dev, res);
> @@ -897,7 +912,18 @@ static int sun50i_a64_ccu_probe(struct platform_device *pdev)
>  
>  	writel(0x515, reg + SUN50I_A64_PLL_MIPI_REG);
>  
> -	return sunxi_ccu_probe(pdev->dev.of_node, reg, &sun50i_a64_ccu_desc);
> +	ret = sunxi_ccu_probe(pdev->dev.of_node, reg, &sun50i_a64_ccu_desc);
> +	if (ret)
> +		return ret;
> +
> +	/* Gate then ungate PLL CPU after any rate changes */
> +	ccu_pll_notifier_register(&sun50i_a64_pll_cpu_nb);
> +
> +	/* Reparent CPU during PLL CPU rate changes */
> +	ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk,
> +				  &sun50i_a64_cpu_nb);
> +
> +	return 0;

So this is the fourth user of the exact same code, can you turn that
into a shared function?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 0/3] Simple DVFS support for Allwinner A64 SoC

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2698 bytes --]

Hi,

On Wed, Sep 27, 2017 at 11:51:30AM +0000, icenowy@aosc.io wrote:
> 在 2017-09-25 18:27，Maxime Ripard 写道：
> > On Mon, Sep 25, 2017 at 10:12:09AM +0000, Icenowy Zheng wrote:
> > > 于 2017年9月25日 GMT+08:00 下午6:10:27, Maxime Ripard
> > > <maxime.ripard@free-electrons.com> 写到:
> > > >Hi,
> > > >
> > > >On Sat, Sep 23, 2017 at 12:15:28AM +0000, Icenowy Zheng wrote:
> > > >> This patchset imports simple DVFS support for Allwinner A64 SoC.
> > > >>
> > > >> As the thermal sensor driver is not yet implemented and some boards
> > > >> have still no AXP PMIC support, now only two OPPs are present --
> > > >> 648MHz@1.04V and 816MHz@1.1V to prevent overheat or undervoltage.
> > > >>
> > > >> PATCH 1 is a fix to the CCU driver of A64, and the remaining patches
> > > >> set up the device tree bits of the DVFS on Pine64.
> > > >
> > > >How has this been tested?
> > > >
> > > >What tasks did you run, with what governor, etc...
> > > 
> > > I only tested manual frequency switching between 648MHz and
> > > 816MHz, and tested the PLL stuck issue by change the OPPs to
> > > some random value.
> > 
> > Ideally, we should test that it's actually reliable. Poorly chosen
> > OPPs might lead to corrupt data that you might not get before a while.
> > 
> > Please test using:
> > https://linux-sunxi.org/Hardware_Reliability_Tests#Reliability_of_cpufreq_voltage.2Ffrequency_settings
> > 
> > And post the report.
> 
> ```
> root@p64 [ cpuburn-arm@master ] # ./cpuburn-a53 &
> [1] 2543
> root@p64 [ cpuburn-arm@master ] # ./cpufreq-ljt-stress-test
> Creating './whitenoise-1920x1080.jpg' ... done
> CPU stress test, which is doing JPEG decoding by libjpeg-turbo
> at different cpufreq operating points.
> 
> Testing CPU 0
>   816 MHz ............................................................ OK
>   648 MHz ............................................................ OK
> 
> Testing CPU 1
>   816 MHz ............................................................ OK
>   648 MHz ............................................................ OK
> 
> Testing CPU 2
>   816 MHz ............................................................ OK
>   648 MHz ............................................................ OK
> 
> Testing CPU 3
>   816 MHz ............................................................ OK
>   648 MHz ............................................................ OK
> 
> Overall result : PASSED
> ```

Great, thanks!

Can you put that in your cover letter for the next version?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/3] clk: sunxi-ng: add mux and pll notifiers for A64 CPU clock

[icenowy]

在 2017-09-28 18:27，Maxime Ripard 写道：
> Hi,
> 
> On Sat, Sep 23, 2017 at 12:15:29AM +0000, Icenowy Zheng wrote:
>> The A64 PLL_CPU clock has the same instability if some factor changed
>> without the PLL gated like other SoCs with sun6i-style CCU, e.g. A33,
>> H3.
>> 
>> Add the mux and pll notifiers for A64 CPU clock to workaround the
>> problem.
>> 
>> Fixes: c6a0637460c2 ("clk: sunxi-ng: Add A64 clocks")
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>>  drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 28 
>> +++++++++++++++++++++++++++-
>>  1 file changed, 27 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c 
>> b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
>> index 2bb4cabf802f..b55fa69dd0c1 100644
>> --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
>> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
>> @@ -879,11 +879,26 @@ static const struct sunxi_ccu_desc 
>> sun50i_a64_ccu_desc = {
>>  	.num_resets	= ARRAY_SIZE(sun50i_a64_ccu_resets),
>>  };
>> 
>> +static struct ccu_pll_nb sun50i_a64_pll_cpu_nb = {
>> +	.common	= &pll_cpux_clk.common,
>> +	/* copy from pll_cpux_clk */
>> +	.enable	= BIT(31),
>> +	.lock	= BIT(28),
>> +};
>> +
>> +static struct ccu_mux_nb sun50i_a64_cpu_nb = {
>> +	.common		= &cpux_clk.common,
>> +	.cm		= &cpux_clk.mux,
>> +	.delay_us	= 1, /* > 8 clock cycles at 24 MHz */
>> +	.bypass_index	= 1, /* index of 24 MHz oscillator */
>> +};
>> +
>> 
>>  static int sun50i_a64_ccu_probe(struct platform_device *pdev)
>>  {
>>  	struct resource *res;
>>  	void __iomem *reg;
>>  	u32 val;
>> +	int ret;
>> 
>>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  	reg = devm_ioremap_resource(&pdev->dev, res);
>> @@ -897,7 +912,18 @@ static int sun50i_a64_ccu_probe(struct 
>> platform_device *pdev)
>> 
>>  	writel(0x515, reg + SUN50I_A64_PLL_MIPI_REG);
>> 
>> -	return sunxi_ccu_probe(pdev->dev.of_node, reg, 
>> &sun50i_a64_ccu_desc);
>> +	ret = sunxi_ccu_probe(pdev->dev.of_node, reg, &sun50i_a64_ccu_desc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Gate then ungate PLL CPU after any rate changes */
>> +	ccu_pll_notifier_register(&sun50i_a64_pll_cpu_nb);
>> +
>> +	/* Reparent CPU during PLL CPU rate changes */
>> +	ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk,
>> +				  &sun50i_a64_cpu_nb);
>> +
>> +	return 0;
> 
> So this is the fourth user of the exact same code, can you turn that
> into a shared function?

I think it's not so worthful to extract the code, as:
- the notifier structs contains info of the clocks
- A31 seems not to need the PLL notifier.

> 
> Thanks!
> Maxime
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] clk: sunxi-ng: add mux and pll notifiers for A64 CPU clock

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3295 bytes --]

On Thu, Sep 28, 2017 at 10:42:39AM +0000, icenowy@aosc.io wrote:
> > On Sat, Sep 23, 2017 at 12:15:29AM +0000, Icenowy Zheng wrote:
> > > The A64 PLL_CPU clock has the same instability if some factor changed
> > > without the PLL gated like other SoCs with sun6i-style CCU, e.g. A33,
> > > H3.
> > > 
> > > Add the mux and pll notifiers for A64 CPU clock to workaround the
> > > problem.
> > > 
> > > Fixes: c6a0637460c2 ("clk: sunxi-ng: Add A64 clocks")
> > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > > ---
> > >  drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 28
> > > +++++++++++++++++++++++++++-
> > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> > > b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> > > index 2bb4cabf802f..b55fa69dd0c1 100644
> > > --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> > > +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> > > @@ -879,11 +879,26 @@ static const struct sunxi_ccu_desc
> > > sun50i_a64_ccu_desc = {
> > >  	.num_resets	= ARRAY_SIZE(sun50i_a64_ccu_resets),
> > >  };
> > > 
> > > +static struct ccu_pll_nb sun50i_a64_pll_cpu_nb = {
> > > +	.common	= &pll_cpux_clk.common,
> > > +	/* copy from pll_cpux_clk */
> > > +	.enable	= BIT(31),
> > > +	.lock	= BIT(28),
> > > +};
> > > +
> > > +static struct ccu_mux_nb sun50i_a64_cpu_nb = {
> > > +	.common		= &cpux_clk.common,
> > > +	.cm		= &cpux_clk.mux,
> > > +	.delay_us	= 1, /* > 8 clock cycles at 24 MHz */
> > > +	.bypass_index	= 1, /* index of 24 MHz oscillator */
> > > +};
> > > +
> > > 
> > >  static int sun50i_a64_ccu_probe(struct platform_device *pdev)
> > >  {
> > >  	struct resource *res;
> > >  	void __iomem *reg;
> > >  	u32 val;
> > > +	int ret;
> > > 
> > >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >  	reg = devm_ioremap_resource(&pdev->dev, res);
> > > @@ -897,7 +912,18 @@ static int sun50i_a64_ccu_probe(struct
> > > platform_device *pdev)
> > > 
> > >  	writel(0x515, reg + SUN50I_A64_PLL_MIPI_REG);
> > > 
> > > -	return sunxi_ccu_probe(pdev->dev.of_node, reg,
> > > &sun50i_a64_ccu_desc);
> > > +	ret = sunxi_ccu_probe(pdev->dev.of_node, reg, &sun50i_a64_ccu_desc);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* Gate then ungate PLL CPU after any rate changes */
> > > +	ccu_pll_notifier_register(&sun50i_a64_pll_cpu_nb);
> > > +
> > > +	/* Reparent CPU during PLL CPU rate changes */
> > > +	ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk,
> > > +				  &sun50i_a64_cpu_nb);
> > > +
> > > +	return 0;
> > 
> > So this is the fourth user of the exact same code, can you turn that
> > into a shared function?
> 
> I think it's not so worthful to extract the code, as:

It does, because the order is important. If you do not register the
notifiers in the right order, you have a bug, and:

> - the notifier structs contains info of the clocks

this should be passed as a parameter anyway,

> - A31 seems not to need the PLL notifier.

And you don't care about the ordering in that case, since there's just
one. If was talking about the H3, A64, R40 and A33 that all have that
code.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/3] clk: sunxi-ng: add mux and pll notifiers for A64 CPU clock

[icenowy]

在 2017-09-28 22:20，Maxime Ripard 写道：
> On Thu, Sep 28, 2017 at 10:42:39AM +0000, icenowy@aosc.io wrote:
>> > On Sat, Sep 23, 2017 at 12:15:29AM +0000, Icenowy Zheng wrote:
>> > > The A64 PLL_CPU clock has the same instability if some factor changed
>> > > without the PLL gated like other SoCs with sun6i-style CCU, e.g. A33,
>> > > H3.
>> > >
>> > > Add the mux and pll notifiers for A64 CPU clock to workaround the
>> > > problem.
>> > >
>> > > Fixes: c6a0637460c2 ("clk: sunxi-ng: Add A64 clocks")
>> > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> > > ---
>> > >  drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 28
>> > > +++++++++++++++++++++++++++-
>> > >  1 file changed, 27 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
>> > > b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
>> > > index 2bb4cabf802f..b55fa69dd0c1 100644
>> > > --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
>> > > +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
>> > > @@ -879,11 +879,26 @@ static const struct sunxi_ccu_desc
>> > > sun50i_a64_ccu_desc = {
>> > >  	.num_resets	= ARRAY_SIZE(sun50i_a64_ccu_resets),
>> > >  };
>> > >
>> > > +static struct ccu_pll_nb sun50i_a64_pll_cpu_nb = {
>> > > +	.common	= &pll_cpux_clk.common,
>> > > +	/* copy from pll_cpux_clk */
>> > > +	.enable	= BIT(31),
>> > > +	.lock	= BIT(28),
>> > > +};
>> > > +
>> > > +static struct ccu_mux_nb sun50i_a64_cpu_nb = {
>> > > +	.common		= &cpux_clk.common,
>> > > +	.cm		= &cpux_clk.mux,
>> > > +	.delay_us	= 1, /* > 8 clock cycles at 24 MHz */
>> > > +	.bypass_index	= 1, /* index of 24 MHz oscillator */
>> > > +};
>> > > +
>> > >
>> > >  static int sun50i_a64_ccu_probe(struct platform_device *pdev)
>> > >  {
>> > >  	struct resource *res;
>> > >  	void __iomem *reg;
>> > >  	u32 val;
>> > > +	int ret;
>> > >
>> > >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > >  	reg = devm_ioremap_resource(&pdev->dev, res);
>> > > @@ -897,7 +912,18 @@ static int sun50i_a64_ccu_probe(struct
>> > > platform_device *pdev)
>> > >
>> > >  	writel(0x515, reg + SUN50I_A64_PLL_MIPI_REG);
>> > >
>> > > -	return sunxi_ccu_probe(pdev->dev.of_node, reg,
>> > > &sun50i_a64_ccu_desc);
>> > > +	ret = sunxi_ccu_probe(pdev->dev.of_node, reg, &sun50i_a64_ccu_desc);
>> > > +	if (ret)
>> > > +		return ret;
>> > > +
>> > > +	/* Gate then ungate PLL CPU after any rate changes */
>> > > +	ccu_pll_notifier_register(&sun50i_a64_pll_cpu_nb);
>> > > +
>> > > +	/* Reparent CPU during PLL CPU rate changes */
>> > > +	ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk,
>> > > +				  &sun50i_a64_cpu_nb);
>> > > +
>> > > +	return 0;
>> >
>> > So this is the fourth user of the exact same code, can you turn that
>> > into a shared function?
>> 
>> I think it's not so worthful to extract the code, as:
> 
> It does, because the order is important. If you do not register the
> notifiers in the right order, you have a bug, and:
> 
>> - the notifier structs contains info of the clocks
> 
> this should be passed as a parameter anyway,

So the function only does these two registers?

> 
>> - A31 seems not to need the PLL notifier.
> 
> And you don't care about the ordering in that case, since there's just
> one. If was talking about the H3, A64, R40 and A33 that all have that
> code.
> 
> Maxime

Re: [PATCH 1/3] clk: sunxi-ng: add mux and pll notifiers for A64 CPU clock

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3589 bytes --]

On Thu, Sep 28, 2017 at 02:24:18PM +0000, icenowy@aosc.io wrote:
> 在 2017-09-28 22:20，Maxime Ripard 写道：
> > On Thu, Sep 28, 2017 at 10:42:39AM +0000, icenowy@aosc.io wrote:
> > > > On Sat, Sep 23, 2017 at 12:15:29AM +0000, Icenowy Zheng wrote:
> > > > > The A64 PLL_CPU clock has the same instability if some factor changed
> > > > > without the PLL gated like other SoCs with sun6i-style CCU, e.g. A33,
> > > > > H3.
> > > > >
> > > > > Add the mux and pll notifiers for A64 CPU clock to workaround the
> > > > > problem.
> > > > >
> > > > > Fixes: c6a0637460c2 ("clk: sunxi-ng: Add A64 clocks")
> > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > > > > ---
> > > > >  drivers/clk/sunxi-ng/ccu-sun50i-a64.c | 28
> > > > > +++++++++++++++++++++++++++-
> > > > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> > > > > b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> > > > > index 2bb4cabf802f..b55fa69dd0c1 100644
> > > > > --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> > > > > +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> > > > > @@ -879,11 +879,26 @@ static const struct sunxi_ccu_desc
> > > > > sun50i_a64_ccu_desc = {
> > > > >  	.num_resets	= ARRAY_SIZE(sun50i_a64_ccu_resets),
> > > > >  };
> > > > >
> > > > > +static struct ccu_pll_nb sun50i_a64_pll_cpu_nb = {
> > > > > +	.common	= &pll_cpux_clk.common,
> > > > > +	/* copy from pll_cpux_clk */
> > > > > +	.enable	= BIT(31),
> > > > > +	.lock	= BIT(28),
> > > > > +};
> > > > > +
> > > > > +static struct ccu_mux_nb sun50i_a64_cpu_nb = {
> > > > > +	.common		= &cpux_clk.common,
> > > > > +	.cm		= &cpux_clk.mux,
> > > > > +	.delay_us	= 1, /* > 8 clock cycles at 24 MHz */
> > > > > +	.bypass_index	= 1, /* index of 24 MHz oscillator */
> > > > > +};
> > > > > +
> > > > >
> > > > >  static int sun50i_a64_ccu_probe(struct platform_device *pdev)
> > > > >  {
> > > > >  	struct resource *res;
> > > > >  	void __iomem *reg;
> > > > >  	u32 val;
> > > > > +	int ret;
> > > > >
> > > > >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > >  	reg = devm_ioremap_resource(&pdev->dev, res);
> > > > > @@ -897,7 +912,18 @@ static int sun50i_a64_ccu_probe(struct
> > > > > platform_device *pdev)
> > > > >
> > > > >  	writel(0x515, reg + SUN50I_A64_PLL_MIPI_REG);
> > > > >
> > > > > -	return sunxi_ccu_probe(pdev->dev.of_node, reg,
> > > > > &sun50i_a64_ccu_desc);
> > > > > +	ret = sunxi_ccu_probe(pdev->dev.of_node, reg, &sun50i_a64_ccu_desc);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	/* Gate then ungate PLL CPU after any rate changes */
> > > > > +	ccu_pll_notifier_register(&sun50i_a64_pll_cpu_nb);
> > > > > +
> > > > > +	/* Reparent CPU during PLL CPU rate changes */
> > > > > +	ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk,
> > > > > +				  &sun50i_a64_cpu_nb);
> > > > > +
> > > > > +	return 0;
> > > >
> > > > So this is the fourth user of the exact same code, can you turn that
> > > > into a shared function?
> > > 
> > > I think it's not so worthful to extract the code, as:
> > 
> > It does, because the order is important. If you do not register the
> > notifiers in the right order, you have a bug, and:
> > 
> > > - the notifier structs contains info of the clocks
> > 
> > this should be passed as a parameter anyway,
> 
> So the function only does these two registers?

Yes.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]