Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

From: Marek Behún <[email protected]>

Add "syscon" compatible to the North Bridge clocks node to allow the cpufreq driver to access these registers via syscon API.

This is needed for a fix of cpufreq driver.

Signed-off-by Marek Behún <[email protected]>

Fixes e8d66e7927b2 ("arm64: dts: marvell: armada-37xx: add nodes...")

Cc [email protected]

Cc Gregory CLEMENT <[email protected]>

Cc Miquel Raynal <[email protected]>

---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/arm64/boot/dts/marvell/armada-37xx.dtsi +2 -1

index d5b6c0a1c54a..a89e47d95eef 100644
@@ -156,7 +156,8 @@
 			};
 
 			nb_periph_clk: nb-periph-clk@13000 {
-				compatible = "marvell,armada-3700-periph-clock-nb";
+				compatible = "marvell,armada-3700-periph-clock-nb",
+					     "syscon";
 				reg = <0x13000 0x100>;
 				clocks = <&tbg 0>, <&tbg 1>, <&tbg 2>,
 				<&tbg 3>, <&xtalclk>;

Signature · 1 line

2.20.1

From: Marek Behún <[email protected]>

Remove the .set_parent method in clk_pm_cpu_ops.

This method was supposed to be needed by the armada-37xx-cpufreq driver, but was never actually called due to wrong assumptions in the cpufreq driver. After this was fixed in the cpufreq driver, this method is not needed anymore.

Signed-off-by Marek Behún <[email protected]>

Fixes 2089dc33ea0e ("clk: mvebu: armada-37xx-periph: add DVFS support for cpu clocks")

Cc Gregory CLEMENT <[email protected]>

Cc Miquel Raynal <[email protected]>

---

It was observed that the workaround introduced by commit 61c40f35f5cd ("clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz") when base CPU frequency is 1.2 GHz is also required when base CPU frequency is 1 GHz. Otherwise switching CPU frequency directly from L2 (250 MHz) to L0 (1 GHz) causes a crash.

When base CPU frequency is just 800 MHz no crashed were observed during switch from L2 to L0.

Signed-off-by Pali Rohár <[email protected]>

Fixes 2089dc33ea0e ("clk: mvebu: armada-37xx-periph: add DVFS support for cpu clocks")

Cc [email protected] # 61c40f35f5cd ("clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz")

---

When CPU frequency is at 250 MHz and set_rate() is called with 500 MHz (L1) quickly followed by a call with 1 GHz (L0), the CPU does not necessarily stay in L1 for at least 20ms as is required by Marvell errata.

This situation happens frequently with the ondemand cpufreq governor and can be also reproduced with userspace governor. In most cases it causes CPU to crash.

This change fixes the above issue and ensures that the CPU always stays in L1 for at least 20ms when switching from any state to L0.

Signed-off-by Marek Behún <[email protected]>

Signed-off-by Pali Rohár <[email protected]>

Fixes 61c40f35f5cd ("clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz")

Cc [email protected]

---

Variable cur_frequency in armada37xx_cpufreq_driver_init() is unused.

Signed-off-by Pali Rohár <[email protected]>

---
 drivers/cpufreq/armada-37xx-cpufreq.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

drivers/cpufreq/armada-37xx-cpufreq.c +1 -9

index f08281fc525c..f13646d143de 100644
@@ -389,7 +389,7 @@ static int __init armada37xx_cpufreq_driver_init(void)
 	struct armada_37xx_dvfs *dvfs;
 	struct platform_device *pdev;
 	unsigned long freq;
-	unsigned int cur_frequency, base_frequency;
+	unsigned int base_frequency;
 	struct regmap *nb_clk_base, *nb_pm_base, *avs_base;
 	struct device *cpu_dev;
 	int load_lvl, ret;

@@ -450,14 +450,6 @@ static int __init armada37xx_cpufreq_driver_init(void)
 		return -EINVAL;
 	}
 
-	/* Get nominal (current) CPU frequency */
-	cur_frequency = clk_get_rate(clk);
-	if (!cur_frequency) {
-		dev_err(cpu_dev, "Failed to get clock rate for CPU\n");
-		clk_put(clk);
-		return -EINVAL;
-	}
-
 	dvfs = armada_37xx_cpu_freq_info_get(base_frequency);
 	if (!dvfs) {
 		clk_put(clk);

Signature · 1 line

2.20.1

Quoted 3 lines · Pali Rohár

Variable cur_frequency in armada37xx_cpufreq_driver_init() is unused.

Signed-off-by Pali Rohár <[email protected]>

Acked-by Gregory CLEMENT <[email protected]>

Hello!

The armada-37xx-cpufreq driver changes base CPU speed from 1000 MHz to 800 MHz on EspressoBIN and Turris MOX. The commit message in patch 2/10 explains why and how can this be discovered.

That patch 2/10 led us to discover another bug, in the SOC itself, that causes the CPU to behave weirdly when frequency changes to 1 GHz. A similar erratum is documented by Marvell but only for systems where base frequency is 1.2 GHz.

We've discovered that to make cpufreq scaling stable on Armada 3720 systems with base frequency 1 GHz, we also have to set voltage levels for L0 and L1 loads to at least 1108 mV. We were led to this by patch we found in Marvell kernel fork. Fix is in the patch 4/10.

https://github.com/MarvellEmbeddedProcessors/linux-marvell/commit/dc33b62c90696afb6adc7dbcc4ebbd48bedec269

During fixing this voltage issue for 1 GHz we discovered another bug in armada-37xx-cpufreq driver that causes CPU instability. Erratum for VDD stabilization was improperly implemented, details are in patch 6/10.

This patch series is also available in my git tree in branch a3720-cpufreq-issues:

https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=a3720-cpufreq-issues

We have tested this patch series on Espressobin v5 and Turris MOX boards. If you have other Armada 3720 boards (Espressobin v5/v7, uDPU, Devel Board, ...) then it will be nice to do an additional tests and check if instability issues are finally fixed.

There is a discussion on armbian forum that Espressobin v7 is unstable when running at 1 GHz and in this thread was also mentioned above voltage patch from Marvell kernel fork:

https://forum.armbian.com/topic/10429-how-to-make-espressobin-v7-stable/

Marek & Pali

Marek Behún (3):
arm64: dts: marvell: armada-37xx: add syscon compatible to NB clk node
cpufreq: armada-37xx: Fix setting TBG parent for load levels
clk: mvebu: armada-37xx-periph: remove .set_parent method for CPU PM
clock

Pali Rohár (7):
cpufreq: armada-37xx: Fix the AVS value for loads L0 and L1
clk: mvebu: armada-37xx-periph: Fix switching CPU freq from 250 Mhz to
1 GHz
clk: mvebu: armada-37xx-periph: Fix workaround for switching from L1
to L0
cpufreq: armada-37xx: Fix driver cleanup when registration failed
cpufreq: armada-37xx: Fix determining base CPU frequency
cpufreq: armada-37xx: Remove cur_frequency variable
cpufreq: armada-37xx: Fix module unloading

 arch/arm64/boot/dts/marvell/armada-37xx.dtsi |   3 +-
 drivers/clk/mvebu/armada-37xx-periph.c       |  83 ++++++++-------
 drivers/cpufreq/armada-37xx-cpufreq.c        | 100 ++++++++++++++-----
 3 files changed, 124 insertions(+), 62 deletions(-)

Hello!

The armada-37xx-cpufreq driver changes base CPU speed from 1000 MHz to 800 MHz on EspressoBIN and Turris MOX. The commit message in patch 2/10 explains why and how can this be discovered.

That patch 2/10 led us to discover another bug, in the SOC itself, that causes the CPU to behave weirdly when frequency changes to 1 GHz. A similar erratum is documented by Marvell but only for systems where base frequency is 1.2 GHz.

We've discovered that to make cpufreq scaling stable on Armada 3720 systems with base frequency 1 GHz, we also have to set voltage levels for L0 and L1 loads to at least 1108 mV. We were led to this by patch we found in Marvell kernel fork. Fix is in the patch 4/10.

https://github.com/MarvellEmbeddedProcessors/linux-marvell/commit/dc33b62c90696afb6adc7dbcc4ebbd48bedec269

During fixing this voltage issue for 1 GHz we discovered another bug in armada-37xx-cpufreq driver that causes CPU instability. Erratum for VDD stabilization was improperly implemented, details are in patch 6/10.

This patch series is also available in my git tree in branch a3720-cpufreq-issues:

https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=a3720-cpufreq-issues

We have tested this patch series on Espressobin v5 and Turris MOX boards. If you have other Armada 3720 boards (Espressobin v5/v7, uDPU, Devel Board, ...) then it will be nice to do an additional tests and check if instability issues are finally fixed.

There is a discussion on armbian forum that Espressobin v7 is unstable when running at 1 GHz and in this thread was also mentioned above voltage patch from Marvell kernel fork:

https://forum.armbian.com/topic/10429-how-to-make-espressobin-v7-stable/

Marek & Pali

Marek Behún (3):
arm64: dts: marvell: armada-37xx: add syscon compatible to NB clk node
cpufreq: armada-37xx: Fix setting TBG parent for load levels
clk: mvebu: armada-37xx-periph: remove .set_parent method for CPU PM
clock

Pali Rohár (7):
cpufreq: armada-37xx: Fix the AVS value for loads L0 and L1
clk: mvebu: armada-37xx-periph: Fix switching CPU freq from 250 Mhz to
1 GHz
clk: mvebu: armada-37xx-periph: Fix workaround for switching from L1
to L0
cpufreq: armada-37xx: Fix driver cleanup when registration failed
cpufreq: armada-37xx: Fix determining base CPU frequency
cpufreq: armada-37xx: Remove cur_frequency variable
cpufreq: armada-37xx: Fix module unloading

  arch/arm64/boot/dts/marvell/armada-37xx.dtsi |   3 +-
  drivers/clk/mvebu/armada-37xx-periph.c       |  83 ++++++++-------
  drivers/cpufreq/armada-37xx-cpufreq.c        | 100 ++++++++++++++-----
  3 files changed, 124 insertions(+), 62 deletions(-)

From: Marek Behún <[email protected]>

With CPU frequency determining software [1] we have discovered that after this driver does one CPU frequency change, the base frequency of the CPU is set to the frequency of TBG-A-P clock, instead of the TBG that is parent to the CPU.

This can be reproduced on EspressoBIN and Turris MOX:
cd /sys/devices/system/cpu/cpufreq/policy0 echo powersave >scaling_governor
echo performance >scaling_governor

Running the mhz tool before this driver is loaded reports 1000 MHz, and after loading the driver and executing commands above the tool reports 800 MHz.

The change of TBG clock selector is supposed to happen in function armada37xx_cpufreq_dvfs_setup. Before the function returns, it does this:
parent = clk_get_parent(clk);
clk_set_parent(clk, parent);

The armada-37xx-periph clock driver has the .set_parent method implemented correctly for this, so if the method was actually called, this would work. But since the introduction of the common clock framework in commit b2476490ef11 ("clk: introduce the common clock..."), the clk_set_parent function checks whether the parent is actually changing, and if the requested new parent is same as the old parent (which is obviously the case for the code above), the .set_parent method is not called at all.

This patch fixes this issue by filling the correct TBG clock selector directly in the armada37xx_cpufreq_dvfs_setup during the filling of other registers at the same address. But the determination of CPU TBG index cannot be done via the common clock framework, therefore we need to access the North Bridge Peripheral Clock registers directly in this driver.

[1] https://github.com/wtarreau/mhz

Signed-off-by Marek Behún <[email protected]>

Tested-by Pali Rohár <[email protected]>

Tested-by Tomasz Maciej Nowak <[email protected]>

Tested-by Anders Trier Olesen <[email protected]>

Tested-by Philip Soares <[email protected]>

Fixes 92ce45fb875d ("cpufreq: Add DVFS support for Armada 37xx")

Cc [email protected]

---
 drivers/cpufreq/armada-37xx-cpufreq.c | 35 ++++++++++++++++++---------
 1 file changed, 23 insertions(+), 12 deletions(-)

drivers/cpufreq/armada-37xx-cpufreq.c +23 -12

index b4af4094309b..b8dc6c849579 100644
@@ -25,6 +25,10 @@
 
 #include "cpufreq-dt.h"
 
+/* Clk register set */
+#define ARMADA_37XX_CLK_TBG_SEL		0
+#define ARMADA_37XX_CLK_TBG_SEL_CPU_OFF	22
+
 /* Power management in North Bridge register set */
 #define ARMADA_37XX_NB_L0L1	0x18
 #define ARMADA_37XX_NB_L2L3	0x1C

@@ -120,10 +124,15 @@ static struct armada_37xx_dvfs *armada_37xx_cpu_freq_info_get(u32 freq)
  * will be configured then the DVFS will be enabled.
  */
 static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base,
-						 struct clk *clk, u8 *divider)
+						 struct regmap *clk_base, u8 *divider)
 {
+	u32 cpu_tbg_sel;
 	int load_lvl;
-	struct clk *parent;
+
+	/* Determine to which TBG clock is CPU connected */
+	regmap_read(clk_base, ARMADA_37XX_CLK_TBG_SEL, &cpu_tbg_sel);
+	cpu_tbg_sel >>= ARMADA_37XX_CLK_TBG_SEL_CPU_OFF;
+	cpu_tbg_sel &= ARMADA_37XX_NB_TBG_SEL_MASK;
 
 	for (load_lvl = 0; load_lvl < LOAD_LEVEL_NR; load_lvl++) {
 		unsigned int reg, mask, val, offset = 0;

@@ -142,6 +151,11 @@ static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base,
 		mask = (ARMADA_37XX_NB_CLK_SEL_MASK
 			<< ARMADA_37XX_NB_CLK_SEL_OFF);
 
+		/* Set TBG index, for all levels we use the same TBG */
+		val = cpu_tbg_sel << ARMADA_37XX_NB_TBG_SEL_OFF;
+		mask = (ARMADA_37XX_NB_TBG_SEL_MASK
+			<< ARMADA_37XX_NB_TBG_SEL_OFF);
+
 		/*
 		 * Set cpu divider based on the pre-computed array in
 		 * order to have balanced step.

@@ -160,14 +174,6 @@ static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base,
 
 		regmap_update_bits(base, reg, mask, val);
 	}
-
-	/*
-	 * Set cpu clock source, for all the level we keep the same
-	 * clock source that the one already configured. For this one
-	 * we need to use the clock framework
-	 */
-	parent = clk_get_parent(clk);
-	clk_set_parent(clk, parent);
 }
 
 /*

@@ -358,11 +364,16 @@ static int __init armada37xx_cpufreq_driver_init(void)
 	struct platform_device *pdev;
 	unsigned long freq;
 	unsigned int cur_frequency, base_frequency;
-	struct regmap *nb_pm_base, *avs_base;
+	struct regmap *nb_clk_base, *nb_pm_base, *avs_base;
 	struct device *cpu_dev;
 	int load_lvl, ret;
 	struct clk *clk, *parent;
 
+	nb_clk_base =
+		syscon_regmap_lookup_by_compatible("marvell,armada-3700-periph-clock-nb");
+	if (IS_ERR(nb_clk_base))
+		return -ENODEV;
+
 	nb_pm_base =
 		syscon_regmap_lookup_by_compatible("marvell,armada-3700-nb-pm");
 

@@ -439,7 +450,7 @@ static int __init armada37xx_cpufreq_driver_init(void)
 	armada37xx_cpufreq_avs_configure(avs_base, dvfs);
 	armada37xx_cpufreq_avs_setup(avs_base, dvfs);
 
-	armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
+	armada37xx_cpufreq_dvfs_setup(nb_pm_base, nb_clk_base, dvfs->divider);
 	clk_put(clk);
 
 	for (load_lvl = ARMADA_37XX_DVFS_LOAD_0; load_lvl < LOAD_LEVEL_NR;

Signature · 1 line

2.20.1

From: Marek Behún <[email protected]>

Remove the .set_parent method in clk_pm_cpu_ops.

This method was supposed to be needed by the armada-37xx-cpufreq driver, but was never actually called due to wrong assumptions in the cpufreq driver. After this was fixed in the cpufreq driver, this method is not needed anymore.

Signed-off-by Marek Behún <[email protected]>

Acked-by Stephen Boyd <[email protected]>

Tested-by Pali Rohár <[email protected]>

Tested-by Tomasz Maciej Nowak <[email protected]>

Tested-by Anders Trier Olesen <[email protected]>

Tested-by Philip Soares <[email protected]>

Fixes 2089dc33ea0e ("clk: mvebu: armada-37xx-periph: add DVFS support for cpu clocks")

---
 drivers/clk/mvebu/armada-37xx-periph.c | 28 --------------------------
 1 file changed, 28 deletions(-)

drivers/clk/mvebu/armada-37xx-periph.c +0 -28

index f5746f9ea929..6507bd2c5f31 100644
@@ -440,33 +440,6 @@ static u8 clk_pm_cpu_get_parent(struct clk_hw *hw)
 	return val;
 }
 
-static int clk_pm_cpu_set_parent(struct clk_hw *hw, u8 index)
-{
-	struct clk_pm_cpu *pm_cpu = to_clk_pm_cpu(hw);
-	struct regmap *base = pm_cpu->nb_pm_base;
-	int load_level;
-
-	/*
-	 * We set the clock parent only if the DVFS is available but
-	 * not enabled.
-	 */
-	if (IS_ERR(base) || armada_3700_pm_dvfs_is_enabled(base))
-		return -EINVAL;
-
-	/* Set the parent clock for all the load level */
-	for (load_level = 0; load_level < LOAD_LEVEL_NR; load_level++) {
-		unsigned int reg, mask,  val,
-			offset = ARMADA_37XX_NB_TBG_SEL_OFF;
-
-		armada_3700_pm_dvfs_update_regs(load_level, &reg, &offset);
-
-		val = index << offset;
-		mask = ARMADA_37XX_NB_TBG_SEL_MASK << offset;
-		regmap_update_bits(base, reg, mask, val);
-	}
-	return 0;
-}
-
 static unsigned long clk_pm_cpu_recalc_rate(struct clk_hw *hw,
 					    unsigned long parent_rate)
 {

@@ -592,7 +565,6 @@ static int clk_pm_cpu_set_rate(struct clk_hw *hw, unsigned long rate,
 
 static const struct clk_ops clk_pm_cpu_ops = {
 	.get_parent = clk_pm_cpu_get_parent,
-	.set_parent = clk_pm_cpu_set_parent,
 	.round_rate = clk_pm_cpu_round_rate,
 	.set_rate = clk_pm_cpu_set_rate,
 	.recalc_rate = clk_pm_cpu_recalc_rate,

Signature · 1 line

2.20.1

The original CPU voltage value for load L1 is too low for Armada 37xx SoC when base CPU frequency is 1000 or 1200 MHz. It leads to instabilities where CPU gets stuck soon after dynamic voltage scaling from load L1 to L0.

Update the CPU voltage value for load L1 accordingly when base frequency is 1000 or 1200 MHz. The minimal L1 value for base CPU frequency 1000 MHz is updated from the original 1.05V to 1.108V and for 1200 MHz is updated to 1.155V. This minimal L1 value is used only in the case when it is lower than value for L0.

This change fixes CPU instability issues on 1 GHz and 1.2 GHz variants of Espressobin and 1 GHz Turris Mox.

Marvell previously for 1 GHz variant of Espressobin provided a patch [1] suitable only for their Marvell Linux kernel 4.4 fork which workarounded this issue. Patch forced CPU voltage value to 1.108V in all loads. But such change does not fix CPU instability issues on 1.2 GHz variants of Armada 3720 SoC.

During testing we come to the conclusion that using 1.108V as minimal value for L1 load makes 1 GHz variants of Espressobin and Turris Mox boards stable. And similarly 1.155V for 1.2 GHz variant of Espressobin.

These two values 1.108V and 1.155V are documented in Armada 3700 Hardware Specifications as typical initial CPU voltage values.

Discussion about this issue is also at the Armbian forum [2].

[1] - https://github.com/MarvellEmbeddedProcessors/linux-marvell/commit/dc33b62c90696afb6adc7dbcc4ebbd48bedec269
[2] - https://forum.armbian.com/topic/10429-how-to-make-espressobin-v7-stable/

Signed-off-by Pali Rohár <[email protected]>

Tested-by Tomasz Maciej Nowak <[email protected]>

Tested-by Anders Trier Olesen <[email protected]>

Tested-by Philip Soares <[email protected]>

Fixes 1c3528232f4b ("cpufreq: armada-37xx: Add AVS support")

Cc [email protected]

---
 drivers/cpufreq/armada-37xx-cpufreq.c | 37 +++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

drivers/cpufreq/armada-37xx-cpufreq.c +37 -0

index b8dc6c849579..c7683d447b11 100644
@@ -73,6 +73,8 @@
 #define LOAD_LEVEL_NR	4
 
 #define MIN_VOLT_MV 1000
+#define MIN_VOLT_MV_FOR_L1_1000MHZ 1108
+#define MIN_VOLT_MV_FOR_L1_1200MHZ 1155
 
 /*  AVS value for the corresponding voltage (in mV) */
 static int avs_map[] = {

@@ -208,6 +210,8 @@ static u32 armada_37xx_avs_val_match(int target_vm)
  * - L2 & L3 voltage should be about 150mv smaller than L0 voltage.
  * This function calculates L1 & L2 & L3 AVS values dynamically based
  * on L0 voltage and fill all AVS values to the AVS value table.
+ * When base CPU frequency is 1000 or 1200 MHz then there is additional
+ * minimal avs value for load L1.
  */
 static void __init armada37xx_cpufreq_avs_configure(struct regmap *base,
 						struct armada_37xx_dvfs *dvfs)

@@ -239,6 +243,19 @@ static void __init armada37xx_cpufreq_avs_configure(struct regmap *base,
 		for (load_level = 1; load_level < LOAD_LEVEL_NR; load_level++)
 			dvfs->avs[load_level] = avs_min;
 
+		/*
+		 * Set the avs values for load L0 and L1 when base CPU frequency
+		 * is 1000/1200 MHz to its typical initial values according to
+		 * the Armada 3700 Hardware Specifications.
+		 */
+		if (dvfs->cpu_freq_max >= 1000*1000*1000) {
+			if (dvfs->cpu_freq_max >= 1200*1000*1000)
+				avs_min = armada_37xx_avs_val_match(MIN_VOLT_MV_FOR_L1_1200MHZ);
+			else
+				avs_min = armada_37xx_avs_val_match(MIN_VOLT_MV_FOR_L1_1000MHZ);
+			dvfs->avs[0] = dvfs->avs[1] = avs_min;
+		}
+
 		return;
 	}
 

@@ -258,6 +275,26 @@ static void __init armada37xx_cpufreq_avs_configure(struct regmap *base,
 	target_vm = avs_map[l0_vdd_min] - 150;
 	target_vm = target_vm > MIN_VOLT_MV ? target_vm : MIN_VOLT_MV;
 	dvfs->avs[2] = dvfs->avs[3] = armada_37xx_avs_val_match(target_vm);
+
+	/*
+	 * Fix the avs value for load L1 when base CPU frequency is 1000/1200 MHz,
+	 * otherwise the CPU gets stuck when switching from load L1 to load L0.
+	 * Also ensure that avs value for load L1 is not higher than for L0.
+	 */
+	if (dvfs->cpu_freq_max >= 1000*1000*1000) {
+		u32 avs_min_l1;
+
+		if (dvfs->cpu_freq_max >= 1200*1000*1000)
+			avs_min_l1 = armada_37xx_avs_val_match(MIN_VOLT_MV_FOR_L1_1200MHZ);
+		else
+			avs_min_l1 = armada_37xx_avs_val_match(MIN_VOLT_MV_FOR_L1_1000MHZ);
+
+		if (avs_min_l1 > dvfs->avs[0])
+			avs_min_l1 = dvfs->avs[0];
+
+		if (dvfs->avs[1] < avs_min_l1)
+			dvfs->avs[1] = avs_min_l1;
+	}
 }
 
 static void __init armada37xx_cpufreq_avs_setup(struct regmap *base,

Signature · 1 line

2.20.1

It was observed that the workaround introduced by commit 61c40f35f5cd ("clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz") when base CPU frequency is 1.2 GHz is also required when base CPU frequency is 1 GHz. Otherwise switching CPU frequency directly from L2 (250 MHz) to L0 (1 GHz) causes a crash.

When base CPU frequency is just 800 MHz no crashed were observed during switch from L2 to L0.

Signed-off-by Pali Rohár <[email protected]>

Acked-by Stephen Boyd <[email protected]>

Tested-by Tomasz Maciej Nowak <[email protected]>

Tested-by Anders Trier Olesen <[email protected]>

Tested-by Philip Soares <[email protected]>

Fixes 2089dc33ea0e ("clk: mvebu: armada-37xx-periph: add DVFS support for cpu clocks")

Cc [email protected] # 61c40f35f5cd ("clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz")

---
 drivers/clk/mvebu/armada-37xx-periph.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/clk/mvebu/armada-37xx-periph.c +7 -5

index 6507bd2c5f31..b15e177bea7e 100644
@@ -487,8 +487,10 @@ static long clk_pm_cpu_round_rate(struct clk_hw *hw, unsigned long rate,
 }
 
 /*
- * Switching the CPU from the L2 or L3 frequencies (300 and 200 Mhz
- * respectively) to L0 frequency (1.2 Ghz) requires a significant
+ * Workaround when base CPU frequnecy is 1000 or 1200 MHz
+ *
+ * Switching the CPU from the L2 or L3 frequencies (250/300 or 200 MHz
+ * respectively) to L0 frequency (1/1.2 GHz) requires a significant
  * amount of time to let VDD stabilize to the appropriate
  * voltage. This amount of time is large enough that it cannot be
  * covered by the hardware countdown register. Due to this, the CPU

@@ -498,15 +500,15 @@ static long clk_pm_cpu_round_rate(struct clk_hw *hw, unsigned long rate,
  * To work around this problem, we prevent switching directly from the
  * L2/L3 frequencies to the L0 frequency, and instead switch to the L1
  * frequency in-between. The sequence therefore becomes:
- * 1. First switch from L2/L3(200/300MHz) to L1(600MHZ)
+ * 1. First switch from L2/L3 (200/250/300 MHz) to L1 (500/600 MHz)
  * 2. Sleep 20ms for stabling VDD voltage
- * 3. Then switch from L1(600MHZ) to L0(1200Mhz).
+ * 3. Then switch from L1 (500/600 MHz) to L0 (1000/1200 MHz).
  */
 static void clk_pm_cpu_set_rate_wa(unsigned long rate, struct regmap *base)
 {
 	unsigned int cur_level;
 
-	if (rate != 1200 * 1000 * 1000)
+	if (rate < 1000 * 1000 * 1000)
 		return;
 
 	regmap_read(base, ARMADA_37XX_NB_CPU_LOAD, &cur_level);

Signature · 1 line

2.20.1

When CPU frequency is at 250 MHz and set_rate() is called with 500 MHz (L1) quickly followed by a call with 1 GHz (L0), the CPU does not necessarily stay in L1 for at least 20ms as is required by Marvell errata.

This situation happens frequently with the ondemand cpufreq governor and can be also reproduced with userspace governor. In most cases it causes CPU to crash.

This change fixes the above issue and ensures that the CPU always stays in L1 for at least 20ms when switching from any state to L0.

Signed-off-by Marek Behún <[email protected]>

Signed-off-by Pali Rohár <[email protected]>

Acked-by Stephen Boyd <[email protected]>

Tested-by Tomasz Maciej Nowak <[email protected]>

Tested-by Anders Trier Olesen <[email protected]>

Tested-by Philip Soares <[email protected]>

Fixes 61c40f35f5cd ("clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz")

Cc [email protected]

---
 drivers/clk/mvebu/armada-37xx-periph.c | 45 ++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 6 deletions(-)

drivers/clk/mvebu/armada-37xx-periph.c +39 -6

index b15e177bea7e..32ac6b6b7530 100644
@@ -84,6 +84,7 @@ struct clk_pm_cpu {
 	void __iomem *reg_div;
 	u8 shift_div;
 	struct regmap *nb_pm_base;
+	unsigned long l1_expiration;
 };
 
 #define to_clk_double_div(_hw) container_of(_hw, struct clk_double_div, hw)

@@ -504,22 +505,52 @@ static long clk_pm_cpu_round_rate(struct clk_hw *hw, unsigned long rate,
  * 2. Sleep 20ms for stabling VDD voltage
  * 3. Then switch from L1 (500/600 MHz) to L0 (1000/1200 MHz).
  */
-static void clk_pm_cpu_set_rate_wa(unsigned long rate, struct regmap *base)
+static void clk_pm_cpu_set_rate_wa(struct clk_pm_cpu *pm_cpu,
+				   unsigned int new_level, unsigned long rate,
+				   struct regmap *base)
 {
 	unsigned int cur_level;
 
-	if (rate < 1000 * 1000 * 1000)
-		return;
-
 	regmap_read(base, ARMADA_37XX_NB_CPU_LOAD, &cur_level);
 	cur_level &= ARMADA_37XX_NB_CPU_LOAD_MASK;
-	if (cur_level <= ARMADA_37XX_DVFS_LOAD_1)
+
+	if (cur_level == new_level)
+		return;
+
+	/*
+	 * System wants to go to L1 on its own. If we are going from L2/L3,
+	 * remember when 20ms will expire. If from L0, set the value so that
+	 * next switch to L0 won't have to wait.
+	 */
+	if (new_level == ARMADA_37XX_DVFS_LOAD_1) {
+		if (cur_level == ARMADA_37XX_DVFS_LOAD_0)
+			pm_cpu->l1_expiration = jiffies;
+		else
+			pm_cpu->l1_expiration = jiffies + msecs_to_jiffies(20);
 		return;
+	}
+
+	/*
+	 * If we are setting to L2/L3, just invalidate L1 expiration time,
+	 * sleeping is not needed.
+	 */
+	if (rate < 1000*1000*1000)
+		goto invalidate_l1_exp;
+
+	/*
+	 * We are going to L0 with rate >= 1GHz. Check whether we have been at
+	 * L1 for long enough time. If not, go to L1 for 20ms.
+	 */
+	if (pm_cpu->l1_expiration && jiffies >= pm_cpu->l1_expiration)
+		goto invalidate_l1_exp;
 
 	regmap_update_bits(base, ARMADA_37XX_NB_CPU_LOAD,
 			   ARMADA_37XX_NB_CPU_LOAD_MASK,
 			   ARMADA_37XX_DVFS_LOAD_1);
 	msleep(20);
+
+invalidate_l1_exp:
+	pm_cpu->l1_expiration = 0;
 }
 
 static int clk_pm_cpu_set_rate(struct clk_hw *hw, unsigned long rate,

@@ -553,7 +584,9 @@ static int clk_pm_cpu_set_rate(struct clk_hw *hw, unsigned long rate,
 			reg = ARMADA_37XX_NB_CPU_LOAD;
 			mask = ARMADA_37XX_NB_CPU_LOAD_MASK;
 
-			clk_pm_cpu_set_rate_wa(rate, base);
+			/* Apply workaround when base CPU frequency is 1000 or 1200 MHz */
+			if (parent_rate >= 1000*1000*1000)
+				clk_pm_cpu_set_rate_wa(pm_cpu, load_level, rate, base);
 
 			regmap_update_bits(base, reg, mask, load_level);
 

Signature · 1 line

2.20.1

Commit 8db82563451f ("cpufreq: armada-37xx: fix frequency calculation for opp") changed calculation of frequency passed to the dev_pm_opp_add() function call. But the code for dev_pm_opp_remove() function call was not updated, so the driver cleanup phase does not work when registration fails.

This fixes the issue by using the same frequency in both calls.

Signed-off-by Pali Rohár <[email protected]>

Tested-by Tomasz Maciej Nowak <[email protected]>

Tested-by Anders Trier Olesen <[email protected]>

Tested-by Philip Soares <[email protected]>

Fixes 8db82563451f ("cpufreq: armada-37xx: fix frequency calculation for opp")

Cc [email protected]

---
 drivers/cpufreq/armada-37xx-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cpufreq/armada-37xx-cpufreq.c +1 -1

index c7683d447b11..1ab2113daef5 100644
@@ -521,7 +521,7 @@ static int __init armada37xx_cpufreq_driver_init(void)
 remove_opp:
 	/* clean-up the already added opp before leaving */
 	while (load_lvl-- > ARMADA_37XX_DVFS_LOAD_0) {
-		freq = cur_frequency / dvfs->divider[load_lvl];
+		freq = base_frequency / dvfs->divider[load_lvl];
 		dev_pm_opp_remove(cpu_dev, freq);
 	}
 

Signature · 1 line

2.20.1

When current CPU load is not L0 then loading armada-37xx-cpufreq.ko driver fails with following error:

    # modprobe armada-37xx-cpufreq
    [  502.702097] Unsupported CPU frequency 250 MHz

This issue was partially fixed by commit 8db82563451f ("cpufreq:
armada-37xx: fix frequency calculation for opp"), but only for calculating CPU frequency for opp.

Fix this also for determination of base CPU frequency.

Signed-off-by Pali Rohár <[email protected]>

Tested-by Tomasz Maciej Nowak <[email protected]>

Tested-by Anders Trier Olesen <[email protected]>

Tested-by Philip Soares <[email protected]>

Fixes 92ce45fb875d ("cpufreq: Add DVFS support for Armada 37xx")

Cc [email protected] # 8db82563451f ("cpufreq: armada-37xx: fix

frequency calculation for opp")

---
 drivers/cpufreq/armada-37xx-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/cpufreq/armada-37xx-cpufreq.c +1 -1

index 1ab2113daef5..e4782f562e7a 100644
@@ -469,7 +469,7 @@ static int __init armada37xx_cpufreq_driver_init(void)
 		return -EINVAL;
 	}
 
-	dvfs = armada_37xx_cpu_freq_info_get(cur_frequency);
+	dvfs = armada_37xx_cpu_freq_info_get(base_frequency);
 	if (!dvfs) {
 		clk_put(clk);
 		return -EINVAL;

Signature · 1 line

2.20.1

Hello!

This is third version of patches for Armada 37xx cpufreq driver which fix CPU scaling with 1 GHz base frequency.

The only change in this third version is modified patch 04/10 with fixes for 1.2 GHz variant of Espressobin. Minimal CPU voltage in L1 load for 1.2 GHz variant was increased to 1.155V.

Patches are now rebased on top of the kernel version 5.11 with all collected Acked-by/Tested-by lines and are available also in my git tree in branch a3720-cpufreq-issues:

https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=a3720-cpufreq-issues

If you have other Armada 3720 boards with 1.2 GHz CPU, including Espressobin V7, let us know if it is working fine for you.

Marek & Pali

Marek Behún (3):
arm64: dts: marvell: armada-37xx: add syscon compatible to NB clk node
cpufreq: armada-37xx: Fix setting TBG parent for load levels
clk: mvebu: armada-37xx-periph: remove .set_parent method for CPU PM
clock

Pali Rohár (7):
cpufreq: armada-37xx: Fix the AVS value for load L1
clk: mvebu: armada-37xx-periph: Fix switching CPU freq from 250 Mhz to
1 GHz
clk: mvebu: armada-37xx-periph: Fix workaround for switching from L1
to L0
cpufreq: armada-37xx: Fix driver cleanup when registration failed
cpufreq: armada-37xx: Fix determining base CPU frequency
cpufreq: armada-37xx: Remove cur_frequency variable
cpufreq: armada-37xx: Fix module unloading

 arch/arm64/boot/dts/marvell/armada-37xx.dtsi |   3 +-
 drivers/clk/mvebu/armada-37xx-periph.c       |  83 +++++++-------
 drivers/cpufreq/armada-37xx-cpufreq.c        | 111 +++++++++++++++----
 3 files changed, 135 insertions(+), 62 deletions(-)

Signature · 1 line

2.20.1

Hello Gregory!

Patches are the for almost two months and more people have tested them. They are marked with Fixed/CC-stable tags, they should go also into stable trees as they are fixing CPU scaling and instability issues.

Are there any issues with these patches? If not, could you please merge them for upcoming Linux version?

Quoted 45 lines

Hello!

This is third version of patches for Armada 37xx cpufreq driver which fix CPU scaling with 1 GHz base frequency.

The only change in this third version is modified patch 04/10 with fixes for 1.2 GHz variant of Espressobin. Minimal CPU voltage in L1 load for 1.2 GHz variant was increased to 1.155V.

Patches are now rebased on top of the kernel version 5.11 with all collected Acked-by/Tested-by lines and are available also in my git tree in branch a3720-cpufreq-issues:

https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/log/?h=a3720-cpufreq-issues

If you have other Armada 3720 boards with 1.2 GHz CPU, including Espressobin V7, let us know if it is working fine for you.

Marek & Pali

Marek Behún (3):
arm64: dts: marvell: armada-37xx: add syscon compatible to NB clk node
cpufreq: armada-37xx: Fix setting TBG parent for load levels
clk: mvebu: armada-37xx-periph: remove .set_parent method for CPU PM
clock

Pali Rohár (7):
cpufreq: armada-37xx: Fix the AVS value for load L1
clk: mvebu: armada-37xx-periph: Fix switching CPU freq from 250 Mhz to
1 GHz
clk: mvebu: armada-37xx-periph: Fix workaround for switching from L1
to L0
cpufreq: armada-37xx: Fix driver cleanup when registration failed
cpufreq: armada-37xx: Fix determining base CPU frequency
cpufreq: armada-37xx: Remove cur_frequency variable
cpufreq: armada-37xx: Fix module unloading

 arch/arm64/boot/dts/marvell/armada-37xx.dtsi |   3 +-
 drivers/clk/mvebu/armada-37xx-periph.c       |  83 +++++++-------
 drivers/cpufreq/armada-37xx-cpufreq.c        | 111 +++++++++++++++----
 3 files changed, 135 insertions(+), 62 deletions(-)

Signature · 1 line

2.20.1

Hello Pali,

Quoted 6 lines

Hello Gregory!

Patches are the for almost two months and more people have tested them. They are marked with Fixed/CC-stable tags, they should go also into stable trees as they are fixing CPU scaling and instability issues.

Quoted 2 lines

Are there any issues with these patches? If not, could you please merge them for upcoming Linux version?

Actually I am not the maintainer of the clk and cpufreq subsystems, so the only thing I can apply is the device tree relative patch.

Gregory

Hello Pali,

Quoted 6 lines

Hello Gregory!

Patches are the for almost two months and more people have tested them. They are marked with Fixed/CC-stable tags, they should go also into stable trees as they are fixing CPU scaling and instability issues.

Quoted 2 lines

Are there any issues with these patches? If not, could you please merge them for upcoming Linux version?

Actually I am not the maintainer of the clk and cpufreq subsystems, so the only thing I can apply is the device tree relative patch.

Gregory

Quoted 11 lines

Hello Pali,

Quoted 8 lines

Hello Gregory!

Patches are the for almost two months and more people have tested them. They are marked with Fixed/CC-stable tags, they should go also into stable trees as they are fixing CPU scaling and instability issues.

Are there any issues with these patches? If not, could you please merge them for upcoming Linux version?

Quoted 4 lines

Actually I am not the maintainer of the clk and cpufreq subsystems, so the only thing I can apply is the device tree relative patch.

Gregory

Hello Gregory! Could you please at least review this patches, so other maintainers could merge them?