[PATCH 0/3] arm64: allwinner: a64: Enable DVFS on A64

[PATCH 0/3] arm64: allwinner: a64: Enable DVFS on A64

[Vasily Khoruzhick]

This series enables DVFS on Allwinner A64. Operating points table is taken
from BSP kernel.

Icenowy Zheng (1):
  clk: sunxi-ng: add mux and pll notifiers for A64 CPU clock

Vasily Khoruzhick (2):
  clk: sunxi-ng: a64: export CLK_CPUX clock for DVFS
  arm64: dts: allwinner: a64: enable DVFS

 .../allwinner/sun50i-a64-amarula-relic.dts    |   4 +
 .../dts/allwinner/sun50i-a64-bananapi-m64.dts |   4 +
 .../dts/allwinner/sun50i-a64-nanopi-a64.dts   |   4 +
 .../dts/allwinner/sun50i-a64-olinuxino.dts    |   4 +
 .../dts/allwinner/sun50i-a64-orangepi-win.dts |   4 +
 .../boot/dts/allwinner/sun50i-a64-pine64.dts  |   4 +
 .../dts/allwinner/sun50i-a64-pinebook.dts     |   4 +
 .../allwinner/sun50i-a64-sopine-baseboard.dts |   4 +
 .../boot/dts/allwinner/sun50i-a64-sopine.dtsi |   4 +
 .../boot/dts/allwinner/sun50i-a64-teres-i.dts |   4 +
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 102 ++++++++++++++++++
 drivers/clk/sunxi-ng/ccu-sun50i-a64.c         |  28 ++++-
 drivers/clk/sunxi-ng/ccu-sun50i-a64.h         |   1 -
 include/dt-bindings/clock/sun50i-a64-ccu.h    |   1 +
 14 files changed, 170 insertions(+), 2 deletions(-)

-- 
2.24.1

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

[Vasily Khoruzhick]

From: Icenowy Zheng <icenowy@aosc.io>

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>
Signed-off-by: Vasily Khoruzhick <vasilykh@arista.com>
---
 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 49bd7a4c015c..5f66bf879772 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
@@ -921,11 +921,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);
@@ -939,7 +954,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.24.1

[PATCH 2/3] clk: sunxi-ng: a64: export CLK_CPUX clock for DVFS

[Vasily Khoruzhick]

Export CLK_CPUX so we can reference it in CPU node.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 drivers/clk/sunxi-ng/ccu-sun50i-a64.h      | 1 -
 include/dt-bindings/clock/sun50i-a64-ccu.h | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.h b/drivers/clk/sunxi-ng/ccu-sun50i-a64.h
index 979929276709..116e6f826d04 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.h
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.h
@@ -36,7 +36,6 @@
 #define CLK_PLL_HSIC			18
 #define CLK_PLL_DE			19
 #define CLK_PLL_DDR1			20
-#define CLK_CPUX			21
 #define CLK_AXI				22
 #define CLK_APB				23
 #define CLK_AHB1			24
diff --git a/include/dt-bindings/clock/sun50i-a64-ccu.h b/include/dt-bindings/clock/sun50i-a64-ccu.h
index a8ac4cfcdcbc..e512a1c9b0fc 100644
--- a/include/dt-bindings/clock/sun50i-a64-ccu.h
+++ b/include/dt-bindings/clock/sun50i-a64-ccu.h
@@ -46,6 +46,7 @@
 #define CLK_PLL_VIDEO0		7
 #define CLK_PLL_PERIPH0		11
 
+#define CLK_CPUX		21
 #define CLK_BUS_MIPI_DSI	28
 #define CLK_BUS_CE		29
 #define CLK_BUS_DMA		30
-- 
2.24.1

[PATCH 3/3] arm64: dts: allwinner: a64: enable DVFS

[Vasily Khoruzhick]

Add CPU regulator, CPU clock, operation points and thermal trip points
to enable DVFS on A64

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 .../allwinner/sun50i-a64-amarula-relic.dts    |   4 +
 .../dts/allwinner/sun50i-a64-bananapi-m64.dts |   4 +
 .../dts/allwinner/sun50i-a64-nanopi-a64.dts   |   4 +
 .../dts/allwinner/sun50i-a64-olinuxino.dts    |   4 +
 .../dts/allwinner/sun50i-a64-orangepi-win.dts |   4 +
 .../boot/dts/allwinner/sun50i-a64-pine64.dts  |   4 +
 .../dts/allwinner/sun50i-a64-pinebook.dts     |   4 +
 .../allwinner/sun50i-a64-sopine-baseboard.dts |   4 +
 .../boot/dts/allwinner/sun50i-a64-sopine.dtsi |   4 +
 .../boot/dts/allwinner/sun50i-a64-teres-i.dts |   4 +
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 102 ++++++++++++++++++
 11 files changed, 142 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
index 5634245d11db..43f6ac0beed6 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
@@ -65,6 +65,10 @@ wifi_pwrseq: wifi-pwrseq {
 	};
 };
 
+&cpu0 {
+	cpu-supply = <&reg_dcdc2>;
+};
+
 &csi {
 	status = "okay";
 
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
index 208373efee49..cbcab80de8c5 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
@@ -108,6 +108,10 @@ &codec_analog {
 	status = "okay";
 };
 
+&cpu0 {
+	cpu-supply = <&reg_dcdc2>;
+};
+
 &dai {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts
index 9b9d9157128c..6708acf94d01 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts
@@ -87,6 +87,10 @@ wifi_pwrseq: wifi_pwrseq {
 	};
 };
 
+&cpu0 {
+	cpu-supply = <&reg_dcdc2>;
+};
+
 &de {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
index 01a9a52edae4..f94e60eb4ec2 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
@@ -87,6 +87,10 @@ wifi_pwrseq: wifi_pwrseq {
 	};
 };
 
+&cpu0 {
+	cpu-supply = <&reg_dcdc2>;
+};
+
 &de {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts
index f54a415f2e3b..2793afba4884 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts
@@ -123,6 +123,10 @@ &codec_analog {
 	status = "okay";
 };
 
+&cpu0 {
+	cpu-supply = <&reg_dcdc2>;
+};
+
 &dai {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
index 409523cb0950..04aa452d4400 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
@@ -84,6 +84,10 @@ &codec_analog {
 	status = "okay";
 };
 
+&cpu0 {
+	cpu-supply = <&reg_dcdc2>;
+};
+
 &dai {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
index 78c82a665c84..365d85c16deb 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
@@ -98,6 +98,10 @@ &codec_analog {
 	status = "okay";
 };
 
+&cpu0 {
+	cpu-supply = <&reg_dcdc2>;
+};
+
 &dai {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
index 920103ec0046..e760e8efdf3e 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
@@ -100,6 +100,10 @@ &codec_analog {
 	status = "okay";
 };
 
+&cpu0 {
+	cpu-supply = <&reg_dcdc2>;
+};
+
 &dai {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi
index 9d20e13f0c02..3d8e2d452ce1 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi
@@ -51,6 +51,10 @@ &codec_analog {
 	cpvdd-supply = <&reg_eldo1>;
 };
 
+&cpu0 {
+	cpu-supply = <&reg_dcdc2>;
+};
+
 &mmc0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&mmc0_pins>;
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
index 970415106dcf..28edcef8bed1 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
@@ -104,6 +104,10 @@ &de {
 	status = "okay";
 };
 
+&cpu0 {
+	cpu-supply = <&reg_dcdc2>;
+};
+
 &ehci1 {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 5e3f16c3b706..7f37c8cd1b6e 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -80,6 +80,52 @@ simplefb_hdmi: framebuffer-hdmi {
 		};
 	};
 
+	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 */
+		};
+		opp-912000000 {
+			opp-hz = /bits/ 64 <912000000>;
+			opp-microvolt = <1120000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+		};
+		opp-960000000 {
+			opp-hz = /bits/ 64 <960000000>;
+			opp-microvolt = <1160000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+		};
+		opp-1008000000 {
+			opp-hz = /bits/ 64 <1008000000>;
+			opp-microvolt = <1200000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+		};
+		opp-1056000000 {
+			opp-hz = /bits/ 64 <1056000000>;
+			opp-microvolt = <1240000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+		};
+		opp-1104000000 {
+			opp-hz = /bits/ 64 <1104000000>;
+			opp-microvolt = <1260000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+		};
+		opp-1152000000 {
+			opp-hz = /bits/ 64 <1152000000>;
+			opp-microvolt = <1300000>;
+			clock-latency-ns = <244144>; /* 8 32k periods */
+		};
+	};
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
@@ -90,6 +136,10 @@ cpu0: cpu@0 {
 			reg = <0>;
 			enable-method = "psci";
 			next-level-cache = <&L2>;
+			clocks = <&ccu CLK_CPUX>;
+			clock-names = "cpu";
+			operating-points-v2 = <&cpu0_opp_table>;
+			#cooling-cells = <2>;
 		};
 
 		cpu1: cpu@1 {
@@ -98,6 +148,10 @@ cpu1: cpu@1 {
 			reg = <1>;
 			enable-method = "psci";
 			next-level-cache = <&L2>;
+			clocks = <&ccu CLK_CPUX>;
+			clock-names = "cpu";
+			operating-points-v2 = <&cpu0_opp_table>;
+			#cooling-cells = <2>;
 		};
 
 		cpu2: cpu@2 {
@@ -106,6 +160,10 @@ cpu2: cpu@2 {
 			reg = <2>;
 			enable-method = "psci";
 			next-level-cache = <&L2>;
+			clocks = <&ccu CLK_CPUX>;
+			clock-names = "cpu";
+			operating-points-v2 = <&cpu0_opp_table>;
+			#cooling-cells = <2>;
 		};
 
 		cpu3: cpu@3 {
@@ -114,6 +172,10 @@ cpu3: cpu@3 {
 			reg = <3>;
 			enable-method = "psci";
 			next-level-cache = <&L2>;
+			clocks = <&ccu CLK_CPUX>;
+			clock-names = "cpu";
+			operating-points-v2 = <&cpu0_opp_table>;
+			#cooling-cells = <2>;
 		};
 
 		L2: l2-cache {
@@ -218,6 +280,46 @@ cpu_thermal: cpu0-thermal {
 			polling-delay-passive = <0>;
 			polling-delay = <0>;
 			thermal-sensors = <&ths 0>;
+
+			cooling-maps {
+				map0 {
+					trip = <&cpu_alert0>;
+					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+				map1 {
+					trip = <&cpu_alert1>;
+					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+
+			trips {
+				cpu_alert0: cpu_alert0 {
+					/* milliCelsius */
+					temperature = <75000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				cpu_alert1: cpu_alert1 {
+					/* milliCelsius */
+					temperature = <90000>;
+					hysteresis = <2000>;
+					type = "hot";
+				};
+
+				cpu_crit: cpu_crit {
+					/* milliCelsius */
+					temperature = <110000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
 		};
 
 		gpu0_thermal: gpu0-thermal {
-- 
2.24.1

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

[Vasily Khoruzhick]

On Fri, Jan 3, 2020 at 10:35 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> From: Icenowy Zheng <icenowy@aosc.io>
>
> 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>
> Signed-off-by: Vasily Khoruzhick <vasilykh@arista.com>

Ugh, didn't notice that email is wrong here, this patch is not related
to my daytime job. Maxime, if patchset is OK please fix it up to be
"Vasily Khoruzhick <anarsoul@gmail.com>", otherwise I'll resend v2
with correct sign off.


> ---
>  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 49bd7a4c015c..5f66bf879772 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.c
> @@ -921,11 +921,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);
> @@ -939,7 +954,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.24.1
>

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

[Maxime Ripard]

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

On Fri, Jan 03, 2020 at 10:42:35PM -0800, Vasily Khoruzhick wrote:
> On Fri, Jan 3, 2020 at 10:35 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> >
> > From: Icenowy Zheng <icenowy@aosc.io>
> >
> > 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>
> > Signed-off-by: Vasily Khoruzhick <vasilykh@arista.com>
>
> Ugh, didn't notice that email is wrong here, this patch is not related
> to my daytime job. Maxime, if patchset is OK please fix it up to be
> "Vasily Khoruzhick <anarsoul@gmail.com>", otherwise I'll resend v2
> with correct sign off.

Applied with your mail fixed, thanks!
Maxime

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

Re: [PATCH 2/3] clk: sunxi-ng: a64: export CLK_CPUX clock for DVFS

[Maxime Ripard]

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

On Fri, Jan 03, 2020 at 10:35:04PM -0800, Vasily Khoruzhick wrote:
> Export CLK_CPUX so we can reference it in CPU node.
>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>

Applied, thanks!
Maxime

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

Re: [PATCH 3/3] arm64: dts: allwinner: a64: enable DVFS

[Maxime Ripard]

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

Hi!

On Fri, Jan 03, 2020 at 10:35:05PM -0800, Vasily Khoruzhick wrote:
> Add CPU regulator, CPU clock, operation points and thermal trip points
> to enable DVFS on A64
>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  .../allwinner/sun50i-a64-amarula-relic.dts    |   4 +
>  .../dts/allwinner/sun50i-a64-bananapi-m64.dts |   4 +
>  .../dts/allwinner/sun50i-a64-nanopi-a64.dts   |   4 +
>  .../dts/allwinner/sun50i-a64-olinuxino.dts    |   4 +
>  .../dts/allwinner/sun50i-a64-orangepi-win.dts |   4 +
>  .../boot/dts/allwinner/sun50i-a64-pine64.dts  |   4 +
>  .../dts/allwinner/sun50i-a64-pinebook.dts     |   4 +
>  .../allwinner/sun50i-a64-sopine-baseboard.dts |   4 +
>  .../boot/dts/allwinner/sun50i-a64-sopine.dtsi |   4 +
>  .../boot/dts/allwinner/sun50i-a64-teres-i.dts |   4 +
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 102 ++++++++++++++++++
>  11 files changed, 142 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
> index 5634245d11db..43f6ac0beed6 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
> @@ -65,6 +65,10 @@ wifi_pwrseq: wifi-pwrseq {
>  	};
>  };
>
> +&cpu0 {
> +	cpu-supply = <&reg_dcdc2>;
> +};
> +
>  &csi {
>  	status = "okay";
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> index 208373efee49..cbcab80de8c5 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> @@ -108,6 +108,10 @@ &codec_analog {
>  	status = "okay";
>  };
>
> +&cpu0 {
> +	cpu-supply = <&reg_dcdc2>;
> +};
> +
>  &dai {
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts
> index 9b9d9157128c..6708acf94d01 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts
> @@ -87,6 +87,10 @@ wifi_pwrseq: wifi_pwrseq {
>  	};
>  };
>
> +&cpu0 {
> +	cpu-supply = <&reg_dcdc2>;
> +};
> +
>  &de {
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
> index 01a9a52edae4..f94e60eb4ec2 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
> @@ -87,6 +87,10 @@ wifi_pwrseq: wifi_pwrseq {
>  	};
>  };
>
> +&cpu0 {
> +	cpu-supply = <&reg_dcdc2>;
> +};
> +
>  &de {
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts
> index f54a415f2e3b..2793afba4884 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts
> @@ -123,6 +123,10 @@ &codec_analog {
>  	status = "okay";
>  };
>
> +&cpu0 {
> +	cpu-supply = <&reg_dcdc2>;
> +};
> +
>  &dai {
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
> index 409523cb0950..04aa452d4400 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
> @@ -84,6 +84,10 @@ &codec_analog {
>  	status = "okay";
>  };
>
> +&cpu0 {
> +	cpu-supply = <&reg_dcdc2>;
> +};
> +
>  &dai {
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
> index 78c82a665c84..365d85c16deb 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
> @@ -98,6 +98,10 @@ &codec_analog {
>  	status = "okay";
>  };
>
> +&cpu0 {
> +	cpu-supply = <&reg_dcdc2>;
> +};
> +
>  &dai {
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
> index 920103ec0046..e760e8efdf3e 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
> @@ -100,6 +100,10 @@ &codec_analog {
>  	status = "okay";
>  };
>
> +&cpu0 {
> +	cpu-supply = <&reg_dcdc2>;
> +};
> +
>  &dai {
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi
> index 9d20e13f0c02..3d8e2d452ce1 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi
> @@ -51,6 +51,10 @@ &codec_analog {
>  	cpvdd-supply = <&reg_eldo1>;
>  };
>
> +&cpu0 {
> +	cpu-supply = <&reg_dcdc2>;
> +};
> +
>  &mmc0 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&mmc0_pins>;
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
> index 970415106dcf..28edcef8bed1 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
> @@ -104,6 +104,10 @@ &de {
>  	status = "okay";
>  };
>
> +&cpu0 {
> +	cpu-supply = <&reg_dcdc2>;
> +};
> +
>  &ehci1 {
>  	status = "okay";
>  };

Did you test it on all those boards (and if so, how were the OPP
tested), or was it done simply by looking at the schematics?

> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 5e3f16c3b706..7f37c8cd1b6e 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -80,6 +80,52 @@ simplefb_hdmi: framebuffer-hdmi {
>  		};
>  	};
>
> +	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 */
> +		};

Nit: having new lines between the nodes would be great

> +		opp-912000000 {
> +			opp-hz = /bits/ 64 <912000000>;
> +			opp-microvolt = <1120000>;
> +			clock-latency-ns = <244144>; /* 8 32k periods */
> +		};
> +		opp-960000000 {
> +			opp-hz = /bits/ 64 <960000000>;
> +			opp-microvolt = <1160000>;
> +			clock-latency-ns = <244144>; /* 8 32k periods */
> +		};
> +		opp-1008000000 {
> +			opp-hz = /bits/ 64 <1008000000>;
> +			opp-microvolt = <1200000>;
> +			clock-latency-ns = <244144>; /* 8 32k periods */
> +		};
> +		opp-1056000000 {
> +			opp-hz = /bits/ 64 <1056000000>;
> +			opp-microvolt = <1240000>;
> +			clock-latency-ns = <244144>; /* 8 32k periods */
> +		};
> +		opp-1104000000 {
> +			opp-hz = /bits/ 64 <1104000000>;
> +			opp-microvolt = <1260000>;
> +			clock-latency-ns = <244144>; /* 8 32k periods */
> +		};
> +		opp-1152000000 {
> +			opp-hz = /bits/ 64 <1152000000>;
> +			opp-microvolt = <1300000>;
> +			clock-latency-ns = <244144>; /* 8 32k periods */
> +		};
> +	};
> +

What frequency is setup by U-Boot?

If it's 1008 MHz as usual, then having the OPP above that frequency by
default means that a board that doesn't set cpu-supply (or doesn't set
it up properly) will be unstable, since it will increase the
frequency, but not the voltage, and the voltage isn't enough for that
new frequency.

The way we've done it so far on the SoCs in a similar situation (A33,
A83t) is only to list the OPP below the frequency set up by U-Boot in
the DTSI, and list the frequencies above that in the boards that add
cpu-supply.

Given that it seems that we need to have them listed on number of
boards, I guess we could add a separate DTSI that would be included
only by the boards setting cpu-supply?

>  	cpus {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
> @@ -90,6 +136,10 @@ cpu0: cpu@0 {
>  			reg = <0>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2>;
> +			clocks = <&ccu CLK_CPUX>;
> +			clock-names = "cpu";
> +			operating-points-v2 = <&cpu0_opp_table>;
> +			#cooling-cells = <2>;
>  		};
>
>  		cpu1: cpu@1 {
> @@ -98,6 +148,10 @@ cpu1: cpu@1 {
>  			reg = <1>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2>;
> +			clocks = <&ccu CLK_CPUX>;
> +			clock-names = "cpu";
> +			operating-points-v2 = <&cpu0_opp_table>;
> +			#cooling-cells = <2>;
>  		};
>
>  		cpu2: cpu@2 {
> @@ -106,6 +160,10 @@ cpu2: cpu@2 {
>  			reg = <2>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2>;
> +			clocks = <&ccu CLK_CPUX>;
> +			clock-names = "cpu";
> +			operating-points-v2 = <&cpu0_opp_table>;
> +			#cooling-cells = <2>;
>  		};
>
>  		cpu3: cpu@3 {
> @@ -114,6 +172,10 @@ cpu3: cpu@3 {
>  			reg = <3>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2>;
> +			clocks = <&ccu CLK_CPUX>;
> +			clock-names = "cpu";
> +			operating-points-v2 = <&cpu0_opp_table>;
> +			#cooling-cells = <2>;
>  		};
>
>  		L2: l2-cache {
> @@ -218,6 +280,46 @@ cpu_thermal: cpu0-thermal {
>  			polling-delay-passive = <0>;
>  			polling-delay = <0>;
>  			thermal-sensors = <&ths 0>;
> +
> +			cooling-maps {
> +				map0 {
> +					trip = <&cpu_alert0>;
> +					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +				};
> +				map1 {
> +					trip = <&cpu_alert1>;
> +					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +				};
> +			};
> +
> +			trips {
> +				cpu_alert0: cpu_alert0 {
> +					/* milliCelsius */
> +					temperature = <75000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};
> +
> +				cpu_alert1: cpu_alert1 {
> +					/* milliCelsius */
> +					temperature = <90000>;
> +					hysteresis = <2000>;
> +					type = "hot";
> +				};
> +
> +				cpu_crit: cpu_crit {
> +					/* milliCelsius */
> +					temperature = <110000>;
> +					hysteresis = <2000>;
> +					type = "critical";
> +				};
> +			};
>  		};

Where are those tripping points coming from?

Thanks!
Maxime

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

Re: [PATCH 3/3] arm64: dts: allwinner: a64: enable DVFS

[Vasily Khoruzhick]

On Sat, Jan 4, 2020 at 12:37 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi!
>
> On Fri, Jan 03, 2020 at 10:35:05PM -0800, Vasily Khoruzhick wrote:
> > Add CPU regulator, CPU clock, operation points and thermal trip points
> > to enable DVFS on A64
> >
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > ---
> >  .../allwinner/sun50i-a64-amarula-relic.dts    |   4 +
> >  .../dts/allwinner/sun50i-a64-bananapi-m64.dts |   4 +
> >  .../dts/allwinner/sun50i-a64-nanopi-a64.dts   |   4 +
> >  .../dts/allwinner/sun50i-a64-olinuxino.dts    |   4 +
> >  .../dts/allwinner/sun50i-a64-orangepi-win.dts |   4 +
> >  .../boot/dts/allwinner/sun50i-a64-pine64.dts  |   4 +
> >  .../dts/allwinner/sun50i-a64-pinebook.dts     |   4 +
> >  .../allwinner/sun50i-a64-sopine-baseboard.dts |   4 +
> >  .../boot/dts/allwinner/sun50i-a64-sopine.dtsi |   4 +
> >  .../boot/dts/allwinner/sun50i-a64-teres-i.dts |   4 +
> >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 102 ++++++++++++++++++
> >  11 files changed, 142 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
> > index 5634245d11db..43f6ac0beed6 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-amarula-relic.dts
> > @@ -65,6 +65,10 @@ wifi_pwrseq: wifi-pwrseq {
> >       };
> >  };
> >
> > +&cpu0 {
> > +     cpu-supply = <&reg_dcdc2>;
> > +};
> > +
> >  &csi {
> >       status = "okay";
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> > index 208373efee49..cbcab80de8c5 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
> > @@ -108,6 +108,10 @@ &codec_analog {
> >       status = "okay";
> >  };
> >
> > +&cpu0 {
> > +     cpu-supply = <&reg_dcdc2>;
> > +};
> > +
> >  &dai {
> >       status = "okay";
> >  };
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts
> > index 9b9d9157128c..6708acf94d01 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-nanopi-a64.dts
> > @@ -87,6 +87,10 @@ wifi_pwrseq: wifi_pwrseq {
> >       };
> >  };
> >
> > +&cpu0 {
> > +     cpu-supply = <&reg_dcdc2>;
> > +};
> > +
> >  &de {
> >       status = "okay";
> >  };
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
> > index 01a9a52edae4..f94e60eb4ec2 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-olinuxino.dts
> > @@ -87,6 +87,10 @@ wifi_pwrseq: wifi_pwrseq {
> >       };
> >  };
> >
> > +&cpu0 {
> > +     cpu-supply = <&reg_dcdc2>;
> > +};
> > +
> >  &de {
> >       status = "okay";
> >  };
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts
> > index f54a415f2e3b..2793afba4884 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-orangepi-win.dts
> > @@ -123,6 +123,10 @@ &codec_analog {
> >       status = "okay";
> >  };
> >
> > +&cpu0 {
> > +     cpu-supply = <&reg_dcdc2>;
> > +};
> > +
> >  &dai {
> >       status = "okay";
> >  };
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
> > index 409523cb0950..04aa452d4400 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
> > @@ -84,6 +84,10 @@ &codec_analog {
> >       status = "okay";
> >  };
> >
> > +&cpu0 {
> > +     cpu-supply = <&reg_dcdc2>;
> > +};
> > +
> >  &dai {
> >       status = "okay";
> >  };
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
> > index 78c82a665c84..365d85c16deb 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts
> > @@ -98,6 +98,10 @@ &codec_analog {
> >       status = "okay";
> >  };
> >
> > +&cpu0 {
> > +     cpu-supply = <&reg_dcdc2>;
> > +};
> > +
> >  &dai {
> >       status = "okay";
> >  };
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
> > index 920103ec0046..e760e8efdf3e 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
> > @@ -100,6 +100,10 @@ &codec_analog {
> >       status = "okay";
> >  };
> >
> > +&cpu0 {
> > +     cpu-supply = <&reg_dcdc2>;
> > +};
> > +
> >  &dai {
> >       status = "okay";
> >  };
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi
> > index 9d20e13f0c02..3d8e2d452ce1 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine.dtsi
> > @@ -51,6 +51,10 @@ &codec_analog {
> >       cpvdd-supply = <&reg_eldo1>;
> >  };
> >
> > +&cpu0 {
> > +     cpu-supply = <&reg_dcdc2>;
> > +};
> > +
> >  &mmc0 {
> >       pinctrl-names = "default";
> >       pinctrl-0 = <&mmc0_pins>;
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
> > index 970415106dcf..28edcef8bed1 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
> > @@ -104,6 +104,10 @@ &de {
> >       status = "okay";
> >  };
> >
> > +&cpu0 {
> > +     cpu-supply = <&reg_dcdc2>;
> > +};
> > +
> >  &ehci1 {
> >       status = "okay";
> >  };
>
> Did you test it on all those boards (and if so, how were the OPP
> tested), or was it done simply by looking at the schematics?

I tested it only on Pine64-LTS and Pinebook. But all these boards use
AXP803 which always uses DCDC2 as CPU regulator.

> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > index 5e3f16c3b706..7f37c8cd1b6e 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > @@ -80,6 +80,52 @@ simplefb_hdmi: framebuffer-hdmi {
> >               };
> >       };
> >
> > +     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 */
> > +             };
>
> Nit: having new lines between the nodes would be great

Will do.

> > +             opp-912000000 {
> > +                     opp-hz = /bits/ 64 <912000000>;
> > +                     opp-microvolt = <1120000>;
> > +                     clock-latency-ns = <244144>; /* 8 32k periods */
> > +             };
> > +             opp-960000000 {
> > +                     opp-hz = /bits/ 64 <960000000>;
> > +                     opp-microvolt = <1160000>;
> > +                     clock-latency-ns = <244144>; /* 8 32k periods */
> > +             };
> > +             opp-1008000000 {
> > +                     opp-hz = /bits/ 64 <1008000000>;
> > +                     opp-microvolt = <1200000>;
> > +                     clock-latency-ns = <244144>; /* 8 32k periods */
> > +             };
> > +             opp-1056000000 {
> > +                     opp-hz = /bits/ 64 <1056000000>;
> > +                     opp-microvolt = <1240000>;
> > +                     clock-latency-ns = <244144>; /* 8 32k periods */
> > +             };
> > +             opp-1104000000 {
> > +                     opp-hz = /bits/ 64 <1104000000>;
> > +                     opp-microvolt = <1260000>;
> > +                     clock-latency-ns = <244144>; /* 8 32k periods */
> > +             };
> > +             opp-1152000000 {
> > +                     opp-hz = /bits/ 64 <1152000000>;
> > +                     opp-microvolt = <1300000>;
> > +                     clock-latency-ns = <244144>; /* 8 32k periods */
> > +             };
> > +     };
> > +
>
> What frequency is setup by U-Boot?

It's 816 MHz

> If it's 1008 MHz as usual, then having the OPP above that frequency by
> default means that a board that doesn't set cpu-supply (or doesn't set
> it up properly) will be unstable, since it will increase the
> frequency, but not the voltage, and the voltage isn't enough for that
> new frequency.
>
> The way we've done it so far on the SoCs in a similar situation (A33,
> A83t) is only to list the OPP below the frequency set up by U-Boot in
> the DTSI, and list the frequencies above that in the boards that add
> cpu-supply.
>
> Given that it seems that we need to have them listed on number of
> boards, I guess we could add a separate DTSI that would be included
> only by the boards setting cpu-supply?

Currently all the A64 boards that we support use AXP803, so I'm not
sure whether we actually need that. We can just define CPU regulator
for them and use default OPP and that's exactly what I did.

> >       cpus {
> >               #address-cells = <1>;
> >               #size-cells = <0>;
> > @@ -90,6 +136,10 @@ cpu0: cpu@0 {
> >                       reg = <0>;
> >                       enable-method = "psci";
> >                       next-level-cache = <&L2>;
> > +                     clocks = <&ccu CLK_CPUX>;
> > +                     clock-names = "cpu";
> > +                     operating-points-v2 = <&cpu0_opp_table>;
> > +                     #cooling-cells = <2>;
> >               };
> >
> >               cpu1: cpu@1 {
> > @@ -98,6 +148,10 @@ cpu1: cpu@1 {
> >                       reg = <1>;
> >                       enable-method = "psci";
> >                       next-level-cache = <&L2>;
> > +                     clocks = <&ccu CLK_CPUX>;
> > +                     clock-names = "cpu";
> > +                     operating-points-v2 = <&cpu0_opp_table>;
> > +                     #cooling-cells = <2>;
> >               };
> >
> >               cpu2: cpu@2 {
> > @@ -106,6 +160,10 @@ cpu2: cpu@2 {
> >                       reg = <2>;
> >                       enable-method = "psci";
> >                       next-level-cache = <&L2>;
> > +                     clocks = <&ccu CLK_CPUX>;
> > +                     clock-names = "cpu";
> > +                     operating-points-v2 = <&cpu0_opp_table>;
> > +                     #cooling-cells = <2>;
> >               };
> >
> >               cpu3: cpu@3 {
> > @@ -114,6 +172,10 @@ cpu3: cpu@3 {
> >                       reg = <3>;
> >                       enable-method = "psci";
> >                       next-level-cache = <&L2>;
> > +                     clocks = <&ccu CLK_CPUX>;
> > +                     clock-names = "cpu";
> > +                     operating-points-v2 = <&cpu0_opp_table>;
> > +                     #cooling-cells = <2>;
> >               };
> >
> >               L2: l2-cache {
> > @@ -218,6 +280,46 @@ cpu_thermal: cpu0-thermal {
> >                       polling-delay-passive = <0>;
> >                       polling-delay = <0>;
> >                       thermal-sensors = <&ths 0>;
> > +
> > +                     cooling-maps {
> > +                             map0 {
> > +                                     trip = <&cpu_alert0>;
> > +                                     cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                                      <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                                      <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                                      <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > +                             };
> > +                             map1 {
> > +                                     trip = <&cpu_alert1>;
> > +                                     cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                                      <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                                      <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                                      <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > +                             };
> > +                     };
> > +
> > +                     trips {
> > +                             cpu_alert0: cpu_alert0 {
> > +                                     /* milliCelsius */
> > +                                     temperature = <75000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "passive";
> > +                             };
> > +
> > +                             cpu_alert1: cpu_alert1 {
> > +                                     /* milliCelsius */
> > +                                     temperature = <90000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "hot";
> > +                             };
> > +
> > +                             cpu_crit: cpu_crit {
> > +                                     /* milliCelsius */
> > +                                     temperature = <110000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "critical";
> > +                             };
> > +                     };
> >               };
>
> Where are those tripping points coming from?

These are taken from A33 dtsi. I couldn't find any recommended
throttling and critical temp in A64 user manual [1].

[1] http://linux-sunxi.org/images/b/b4/Allwinner_A64_User_Manual_V1.1.pdf

> Thanks!
> Maxime

Re: [PATCH 3/3] arm64: dts: allwinner: a64: enable DVFS

[Maxime Ripard]

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

Hi,

On Sat, Jan 04, 2020 at 08:24:01AM -0800, Vasily Khoruzhick wrote:
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts
> > > @@ -104,6 +104,10 @@ &de {
> > >       status = "okay";
> > >  };
> > >
> > > +&cpu0 {
> > > +     cpu-supply = <&reg_dcdc2>;
> > > +};
> > > +
> > >  &ehci1 {
> > >       status = "okay";
> > >  };
> >
> > Did you test it on all those boards (and if so, how were the OPP
> > tested), or was it done simply by looking at the schematics?
>
> I tested it only on Pine64-LTS and Pinebook. But all these boards use
> AXP803 which always uses DCDC2 as CPU regulator.

Ok. How were those OPPs picked and tested?

> > > +             opp-912000000 {
> > > +                     opp-hz = /bits/ 64 <912000000>;
> > > +                     opp-microvolt = <1120000>;
> > > +                     clock-latency-ns = <244144>; /* 8 32k periods */
> > > +             };
> > > +             opp-960000000 {
> > > +                     opp-hz = /bits/ 64 <960000000>;
> > > +                     opp-microvolt = <1160000>;
> > > +                     clock-latency-ns = <244144>; /* 8 32k periods */
> > > +             };
> > > +             opp-1008000000 {
> > > +                     opp-hz = /bits/ 64 <1008000000>;
> > > +                     opp-microvolt = <1200000>;
> > > +                     clock-latency-ns = <244144>; /* 8 32k periods */
> > > +             };
> > > +             opp-1056000000 {
> > > +                     opp-hz = /bits/ 64 <1056000000>;
> > > +                     opp-microvolt = <1240000>;
> > > +                     clock-latency-ns = <244144>; /* 8 32k periods */
> > > +             };
> > > +             opp-1104000000 {
> > > +                     opp-hz = /bits/ 64 <1104000000>;
> > > +                     opp-microvolt = <1260000>;
> > > +                     clock-latency-ns = <244144>; /* 8 32k periods */
> > > +             };
> > > +             opp-1152000000 {
> > > +                     opp-hz = /bits/ 64 <1152000000>;
> > > +                     opp-microvolt = <1300000>;
> > > +                     clock-latency-ns = <244144>; /* 8 32k periods */
> > > +             };
> > > +     };
> > > +
> >
> > What frequency is setup by U-Boot?
>
> It's 816 MHz
>
> > If it's 1008 MHz as usual, then having the OPP above that frequency by
> > default means that a board that doesn't set cpu-supply (or doesn't set
> > it up properly) will be unstable, since it will increase the
> > frequency, but not the voltage, and the voltage isn't enough for that
> > new frequency.
> >
> > The way we've done it so far on the SoCs in a similar situation (A33,
> > A83t) is only to list the OPP below the frequency set up by U-Boot in
> > the DTSI, and list the frequencies above that in the boards that add
> > cpu-supply.
> >
> > Given that it seems that we need to have them listed on number of
> > boards, I guess we could add a separate DTSI that would be included
> > only by the boards setting cpu-supply?
>
> Currently all the A64 boards that we support use AXP803, so I'm not
> sure whether we actually need that. We can just define CPU regulator
> for them and use default OPP and that's exactly what I did.

Yeah, but none of them have been supporting it since day one. Having
these OPPs now mean that the very first thing you'll need to do is to
bringup the PMIC as well, otherwise the system will crash.

And to the person doing the bringup, this will be far from obvious
that a) we have that requirement b) the crash that they are seeing is
due to cpufreq.

> > >       cpus {
> > >               #address-cells = <1>;
> > >               #size-cells = <0>;
> > > @@ -90,6 +136,10 @@ cpu0: cpu@0 {
> > >                       reg = <0>;
> > >                       enable-method = "psci";
> > >                       next-level-cache = <&L2>;
> > > +                     clocks = <&ccu CLK_CPUX>;
> > > +                     clock-names = "cpu";
> > > +                     operating-points-v2 = <&cpu0_opp_table>;
> > > +                     #cooling-cells = <2>;
> > >               };
> > >
> > >               cpu1: cpu@1 {
> > > @@ -98,6 +148,10 @@ cpu1: cpu@1 {
> > >                       reg = <1>;
> > >                       enable-method = "psci";
> > >                       next-level-cache = <&L2>;
> > > +                     clocks = <&ccu CLK_CPUX>;
> > > +                     clock-names = "cpu";
> > > +                     operating-points-v2 = <&cpu0_opp_table>;
> > > +                     #cooling-cells = <2>;
> > >               };
> > >
> > >               cpu2: cpu@2 {
> > > @@ -106,6 +160,10 @@ cpu2: cpu@2 {
> > >                       reg = <2>;
> > >                       enable-method = "psci";
> > >                       next-level-cache = <&L2>;
> > > +                     clocks = <&ccu CLK_CPUX>;
> > > +                     clock-names = "cpu";
> > > +                     operating-points-v2 = <&cpu0_opp_table>;
> > > +                     #cooling-cells = <2>;
> > >               };
> > >
> > >               cpu3: cpu@3 {
> > > @@ -114,6 +172,10 @@ cpu3: cpu@3 {
> > >                       reg = <3>;
> > >                       enable-method = "psci";
> > >                       next-level-cache = <&L2>;
> > > +                     clocks = <&ccu CLK_CPUX>;
> > > +                     clock-names = "cpu";
> > > +                     operating-points-v2 = <&cpu0_opp_table>;
> > > +                     #cooling-cells = <2>;
> > >               };
> > >
> > >               L2: l2-cache {
> > > @@ -218,6 +280,46 @@ cpu_thermal: cpu0-thermal {
> > >                       polling-delay-passive = <0>;
> > >                       polling-delay = <0>;
> > >                       thermal-sensors = <&ths 0>;
> > > +
> > > +                     cooling-maps {
> > > +                             map0 {
> > > +                                     trip = <&cpu_alert0>;
> > > +                                     cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > > +                                                      <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > > +                                                      <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > > +                                                      <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > > +                             };
> > > +                             map1 {
> > > +                                     trip = <&cpu_alert1>;
> > > +                                     cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > > +                                                      <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > > +                                                      <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > > +                                                      <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > > +                             };
> > > +                     };
> > > +
> > > +                     trips {
> > > +                             cpu_alert0: cpu_alert0 {
> > > +                                     /* milliCelsius */
> > > +                                     temperature = <75000>;
> > > +                                     hysteresis = <2000>;
> > > +                                     type = "passive";
> > > +                             };
> > > +
> > > +                             cpu_alert1: cpu_alert1 {
> > > +                                     /* milliCelsius */
> > > +                                     temperature = <90000>;
> > > +                                     hysteresis = <2000>;
> > > +                                     type = "hot";
> > > +                             };
> > > +
> > > +                             cpu_crit: cpu_crit {
> > > +                                     /* milliCelsius */
> > > +                                     temperature = <110000>;
> > > +                                     hysteresis = <2000>;
> > > +                                     type = "critical";
> > > +                             };
> > > +                     };
> > >               };
> >
> > Where are those tripping points coming from?
>
> These are taken from A33 dtsi. I couldn't find any recommended
> throttling and critical temp in A64 user manual [1].
>
> [1] http://linux-sunxi.org/images/b/b4/Allwinner_A64_User_Manual_V1.1.pdf

Ok. Can you mention that in the commit log?

Thanks!
Maxime

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

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 --]

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: 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 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: 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 --]

[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