[PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

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(-)

-- 
2.20.1

[PATCH mvebu v2 01/10] arm64: dts: marvell: armada-37xx: add syscon compatible to NB clk node

[Pali Rohár]

From: Marek Behún <kabel@kernel.org>

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 <kabel@kernel.org>
Fixes: e8d66e7927b2 ("arm64: dts: marvell: armada-37xx: add nodes...")
Cc: stable@vger.kernel.org
Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index d5b6c0a1c54a..a89e47d95eef 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -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>;
-- 
2.20.1

[PATCH mvebu v2 02/10] cpufreq: armada-37xx: Fix setting TBG parent for load levels

[Pali Rohár]

From: Marek Behún <kabel@kernel.org>

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 <kabel@kernel.org>
Fixes: 92ce45fb875d ("cpufreq: Add DVFS support for Armada 37xx")
Cc: stable@vger.kernel.org
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 35 ++++++++++++++++++---------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index b4af4094309b..b8dc6c849579 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -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;
-- 
2.20.1

[PATCH mvebu v2 03/10] clk: mvebu: armada-37xx-periph: remove .set_parent method for CPU PM clock

[Pali Rohár]

From: Marek Behún <kabel@kernel.org>

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 <kabel@kernel.org>
Fixes: 2089dc33ea0e ("clk: mvebu: armada-37xx-periph: add DVFS support for cpu clocks")
Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/clk/mvebu/armada-37xx-periph.c | 28 --------------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index f5746f9ea929..6507bd2c5f31 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -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,
-- 
2.20.1

[PATCH mvebu v2 04/10] cpufreq: armada-37xx: Fix the AVS value for loads L0 and L1

[Pali Rohár]

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 loads L0 and L1 accordingly when base
frequency is 1000 or 1200 MHz. The minimal value is updated from the
original 1.05V to 1.108V.

This change fixes instability issues on 1 GHz variant of Espressobin and
Turris MOX. It is based on previous work from Victor Gu <xigu@marvell.com>
for Espressobin kernel 4.4 [1]. Discussion about this issue is also at
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 <pali@kernel.org>
Fixes: 1c3528232f4b ("cpufreq: armada-37xx: Add AVS support")
Cc: stable@vger.kernel.org
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index b8dc6c849579..92e531f700f4 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -73,6 +73,7 @@
 #define LOAD_LEVEL_NR	4
 
 #define MIN_VOLT_MV 1000
+#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
 
 /*  AVS value for the corresponding voltage (in mV) */
 static int avs_map[] = {
@@ -208,6 +209,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 L0 and L1.
  */
 static void __init armada37xx_cpufreq_avs_configure(struct regmap *base,
 						struct armada_37xx_dvfs *dvfs)
@@ -239,6 +242,15 @@ 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 value for load L0 and L1 when base CPU frequency is 1000/1200 MHz,
+		 * otherwise the CPU gets stuck when switching from load L1 to load L0
+		 */
+		if (dvfs->cpu_freq_max >= 1000*1000*1000) {
+			avs_min = armada_37xx_avs_val_match(MIN_VOLT_MV_FOR_L0_L1_1GHZ);
+			dvfs->avs[0] = dvfs->avs[1] = avs_min;
+		}
+
 		return;
 	}
 
@@ -258,6 +270,20 @@ 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 L0 and L1 when base CPU frequency is 1000/1200 MHz,
+	 * otherwise the CPU gets stuck when switching from load L1 to load L0
+	 */
+	if (dvfs->cpu_freq_max >= 1000*1000*1000) {
+		u32 avs_min = armada_37xx_avs_val_match(MIN_VOLT_MV_FOR_L0_L1_1GHZ);
+
+		if (dvfs->avs[0] < avs_min)
+			dvfs->avs[0] = avs_min;
+
+		if (dvfs->avs[1] < avs_min)
+			dvfs->avs[1] = avs_min;
+	}
 }
 
 static void __init armada37xx_cpufreq_avs_setup(struct regmap *base,
-- 
2.20.1

[PATCH mvebu v2 05/10] clk: mvebu: armada-37xx-periph: Fix switching CPU freq from 250 Mhz to 1 GHz

[Pali Rohár]

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 <pali@kernel.org>
Fixes: 2089dc33ea0e ("clk: mvebu: armada-37xx-periph: add DVFS support for cpu clocks")
Cc: stable@vger.kernel.org # 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(-)

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index 6507bd2c5f31..b15e177bea7e 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -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);
-- 
2.20.1

[PATCH mvebu v2 06/10] clk: mvebu: armada-37xx-periph: Fix workaround for switching from L1 to L0

[Pali Rohár]

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 <kabel@kernel.org>
Signed-off-by: Pali Rohár <pali@kernel.org>
Fixes: 61c40f35f5cd ("clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz")
Cc: stable@vger.kernel.org
---
 drivers/clk/mvebu/armada-37xx-periph.c | 45 ++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index b15e177bea7e..32ac6b6b7530 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -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);
 
-- 
2.20.1

[PATCH mvebu v2 07/10] cpufreq: armada-37xx: Fix driver cleanup when registration failed

[Pali Rohár]

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 <pali@kernel.org>
Fixes: 8db82563451f ("cpufreq: armada-37xx: fix frequency calculation for opp")
Cc: stable@vger.kernel.org
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index 92e531f700f4..002a71775e08 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -510,7 +510,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);
 	}
 
-- 
2.20.1

[PATCH mvebu v2 08/10] cpufreq: armada-37xx: Fix determining base CPU frequency

[Pali Rohár]

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 <pali@kernel.org>
Fixes: 92ce45fb875d ("cpufreq: Add DVFS support for Armada 37xx")
Cc: stable@vger.kernel.org # 8db82563451f ("cpufreq: armada-37xx: fix frequency calculation for opp")
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index 002a71775e08..f08281fc525c 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -458,7 +458,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;
-- 
2.20.1

[PATCH mvebu v2 09/10] cpufreq: armada-37xx: Remove cur_frequency variable

[Pali Rohár]

Variable cur_frequency in armada37xx_cpufreq_driver_init() is unused.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index f08281fc525c..f13646d143de 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -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);
-- 
2.20.1

[PATCH mvebu v2 10/10] cpufreq: armada-37xx: Fix module unloading

[Pali Rohár]

This driver is missing module_exit hook. Add proper driver exit function
which unregisters the platform device and cleans up the data.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index f13646d143de..565c40f536ef 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -85,6 +85,8 @@ static int avs_map[] = {
 };
 
 struct armada37xx_cpufreq_state {
+	struct platform_device *pdev;
+	struct device *cpu_dev;
 	struct regmap *regmap;
 	u32 nb_l0l1;
 	u32 nb_l2l3;
@@ -495,6 +497,9 @@ static int __init armada37xx_cpufreq_driver_init(void)
 	if (ret)
 		goto disable_dvfs;
 
+	armada37xx_cpufreq_state->cpu_dev = cpu_dev;
+	armada37xx_cpufreq_state->pdev = pdev;
+	platform_set_drvdata(pdev, dvfs);
 	return 0;
 
 disable_dvfs:
@@ -513,6 +518,26 @@ static int __init armada37xx_cpufreq_driver_init(void)
 /* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
 late_initcall(armada37xx_cpufreq_driver_init);
 
+static void __exit armada37xx_cpufreq_driver_exit(void)
+{
+	struct platform_device *pdev = armada37xx_cpufreq_state->pdev;
+	struct armada_37xx_dvfs *dvfs = platform_get_drvdata(pdev);
+	unsigned long freq;
+	int load_lvl;
+
+	platform_device_unregister(pdev);
+
+	armada37xx_cpufreq_disable_dvfs(armada37xx_cpufreq_state->regmap);
+
+	for (load_lvl = ARMADA_37XX_DVFS_LOAD_0; load_lvl < LOAD_LEVEL_NR; load_lvl++) {
+		freq = dvfs->cpu_freq_max / dvfs->divider[load_lvl];
+		dev_pm_opp_remove(armada37xx_cpufreq_state->cpu_dev, freq);
+	}
+
+	kfree(armada37xx_cpufreq_state);
+}
+module_exit(armada37xx_cpufreq_driver_exit);
+
 static const struct of_device_id __maybe_unused armada37xx_cpufreq_of_match[] = {
 	{ .compatible = "marvell,armada-3700-nb-pm" },
 	{ },
-- 
2.20.1

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Tomasz Maciej Nowak]

W dniu 14.01.2021 o 13:40, Pali Rohár pisze:
> 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

Hi.
After running this series for three days, the system is stable and the 
issue with switching frequency doesn't seem to be there anymore. So:

Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>

Thanks.

> 
>  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(-)
> 


-- 
TMN

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Anders Trier Olesen]

Hi Marek & Pali

On 1/14/21 1:40 PM, Pali Rohár wrote:
> 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(-)
> 

Thank you for looking at this problem!

I have tested your a3720-cpufreq-issues branch on my Espressobin v7 (1GB 
variant) with the ondemand governor. No issues so far.

root@espressobin:~# uname -a
Linux espressobin 5.11.0-rc1+ #1 SMP PREEMPT Sun Jan 31 23:35:17 CET
2021 aarch64 aarch64 aarch64 GNU/Linux
root@espressobin:~# cpufreq-info
cpufrequtils 008: cpufreq-info (C) Dominik Brodowski 2004-2009
Report errors and bugs to cpufreq@vger.kernel.org, please.
analyzing CPU 0:
    driver: cpufreq-dt
    CPUs which run at the same hardware frequency: 0 1
    CPUs which need to have their frequency coordinated by software: 0 1
    maximum transition latency: 0.97 ms.
    hardware limits: 200 MHz - 1000 MHz
    available frequency steps: 200 MHz, 250 MHz, 500 MHz, 1000 MHz
    available cpufreq governors: conservative, ondemand, userspace,
powersave, performance, schedutil
    current policy: frequency should be within 200 MHz and 1000 MHz.
                    The governor "ondemand" may decide which speed to use
                    within this range.
    current CPU frequency is 1000 MHz (asserted by call to hardware).
    cpufreq stats: 200 MHz:34.71%, 250 MHz:13.20%, 500 MHz:6.26%, 1000
MHz:45.83%  (2015)
analyzing CPU 1:
    driver: cpufreq-dt
    CPUs which run at the same hardware frequency: 0 1
    CPUs which need to have their frequency coordinated by software: 0 1
    maximum transition latency: 0.97 ms.
    hardware limits: 200 MHz - 1000 MHz
    available frequency steps: 200 MHz, 250 MHz, 500 MHz, 1000 MHz
    available cpufreq governors: conservative, ondemand, userspace,
powersave, performance, schedutil
    current policy: frequency should be within 200 MHz and 1000 MHz.
                    The governor "ondemand" may decide which speed to use
                    within this range.
    current CPU frequency is 500 MHz (asserted by call to hardware).
    cpufreq stats: 200 MHz:34.71%, 250 MHz:13.20%, 500 MHz:6.26%, 1000
MHz:45.83%  (2015)

root@espressobin:~# 7za b

7-Zip (a) [64] 16.02 : Copyright (c) 1999-2016 Igor Pavlov : 2016-05-21
p7zip Version 16.02 (locale=en_US.UTF-8,Utf16=on,HugeFiles=on,64 bits,2
CPUs LE)

LE
CPU Freq:   974   997   993   996   997   997   996   997

RAM size:     983 MB,  # CPU hardware threads:   2
RAM usage:    441 MB,  # Benchmark threads:      2

                         Compressing  |                  Decompressing
Dict     Speed Usage    R/U Rating  |      Speed Usage    R/U Rating
           KiB/s     %   MIPS   MIPS  |      KiB/s     %   MIPS   MIPS

22:        879   153    560    856  |      21904   199    941   1870
23:        871   154    576    888  |      21518   199    938   1863
24:        863   154    603    928  |      21147   199    935   1857
25:        857   154    634    979  |      20885   199    934   1859
----------------------------------  | ------------------------------
Avr:             154    593    913  |              199    937   1862
Tot:             176    765   1387


Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>

Thanks,
Anders Trier Olesen

Re: [PATCH mvebu v2 03/10] clk: mvebu: armada-37xx-periph: remove .set_parent method for CPU PM clock

[Stephen Boyd]

Quoting Pali Rohár (2021-01-14 04:40:25)
> From: Marek Behún <kabel@kernel.org>
> 
> 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 <kabel@kernel.org>
> Fixes: 2089dc33ea0e ("clk: mvebu: armada-37xx-periph: add DVFS support for cpu clocks")
> Cc: Gregory CLEMENT <gregory.clement@bootlin.com>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

Re: [PATCH mvebu v2 05/10] clk: mvebu: armada-37xx-periph: Fix switching CPU freq from 250 Mhz to 1 GHz

[Stephen Boyd]

Quoting Pali Rohár (2021-01-14 04:40:27)
> 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 <pali@kernel.org>
> Fixes: 2089dc33ea0e ("clk: mvebu: armada-37xx-periph: add DVFS support for cpu clocks")
> Cc: stable@vger.kernel.org # 61c40f35f5cd ("clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz")
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

Re: [PATCH mvebu v2 06/10] clk: mvebu: armada-37xx-periph: Fix workaround for switching from L1 to L0

[Stephen Boyd]

Quoting Pali Rohár (2021-01-14 04:40:28)
> 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 <kabel@kernel.org>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Fixes: 61c40f35f5cd ("clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz")
> Cc: stable@vger.kernel.org
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

[PATCH mvebu v3 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

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(-)

-- 
2.20.1

[PATCH mvebu v3 01/10] arm64: dts: marvell: armada-37xx: add syscon compatible to NB clk node

[Pali Rohár]

From: Marek Behún <kabel@kernel.org>

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 <kabel@kernel.org>
Tested-by: Pali Rohár <pali@kernel.org>
Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
Tested-by: Philip Soares <philips@netisense.com>
Fixes: e8d66e7927b2 ("arm64: dts: marvell: armada-37xx: add nodes...")
Cc: stable@vger.kernel.org
---
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index d5b6c0a1c54a..a89e47d95eef 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -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>;
-- 
2.20.1

[PATCH mvebu v3 02/10] cpufreq: armada-37xx: Fix setting TBG parent for load levels

[Pali Rohár]

From: Marek Behún <kabel@kernel.org>

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 <kabel@kernel.org>
Tested-by: Pali Rohár <pali@kernel.org>
Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
Tested-by: Philip Soares <philips@netisense.com>
Fixes: 92ce45fb875d ("cpufreq: Add DVFS support for Armada 37xx")
Cc: stable@vger.kernel.org
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 35 ++++++++++++++++++---------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index b4af4094309b..b8dc6c849579 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -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;
-- 
2.20.1

[PATCH mvebu v3 03/10] clk: mvebu: armada-37xx-periph: remove .set_parent method for CPU PM clock

[Pali Rohár]

From: Marek Behún <kabel@kernel.org>

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 <kabel@kernel.org>
Acked-by: Stephen Boyd <sboyd@kernel.org>
Tested-by: Pali Rohár <pali@kernel.org>
Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
Tested-by: Philip Soares <philips@netisense.com>
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(-)

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index f5746f9ea929..6507bd2c5f31 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -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,
-- 
2.20.1

[PATCH mvebu v3 04/10] cpufreq: armada-37xx: Fix the AVS value for load L1

[Pali Rohár]

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 <pali@kernel.org>
Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
Tested-by: Philip Soares <philips@netisense.com>
Fixes: 1c3528232f4b ("cpufreq: armada-37xx: Add AVS support")
Cc: stable@vger.kernel.org
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 37 +++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index b8dc6c849579..c7683d447b11 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -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,
-- 
2.20.1

[PATCH mvebu v3 05/10] clk: mvebu: armada-37xx-periph: Fix switching CPU freq from 250 Mhz to 1 GHz

[Pali Rohár]

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 <pali@kernel.org>
Acked-by: Stephen Boyd <sboyd@kernel.org>
Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
Tested-by: Philip Soares <philips@netisense.com>
Fixes: 2089dc33ea0e ("clk: mvebu: armada-37xx-periph: add DVFS support for cpu clocks")
Cc: stable@vger.kernel.org # 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(-)

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index 6507bd2c5f31..b15e177bea7e 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -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);
-- 
2.20.1

[PATCH mvebu v3 06/10] clk: mvebu: armada-37xx-periph: Fix workaround for switching from L1 to L0

[Pali Rohár]

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 <kabel@kernel.org>
Signed-off-by: Pali Rohár <pali@kernel.org>
Acked-by: Stephen Boyd <sboyd@kernel.org>
Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
Tested-by: Philip Soares <philips@netisense.com>
Fixes: 61c40f35f5cd ("clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz")
Cc: stable@vger.kernel.org
---
 drivers/clk/mvebu/armada-37xx-periph.c | 45 ++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index b15e177bea7e..32ac6b6b7530 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -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);
 
-- 
2.20.1

[PATCH mvebu v3 07/10] cpufreq: armada-37xx: Fix driver cleanup when registration failed

[Pali Rohár]

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 <pali@kernel.org>
Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
Tested-by: Philip Soares <philips@netisense.com>
Fixes: 8db82563451f ("cpufreq: armada-37xx: fix frequency calculation for opp")
Cc: stable@vger.kernel.org
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index c7683d447b11..1ab2113daef5 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -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);
 	}
 
-- 
2.20.1

[PATCH mvebu v3 08/10] cpufreq: armada-37xx: Fix determining base CPU frequency

[Pali Rohár]

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 <pali@kernel.org>
Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
Tested-by: Philip Soares <philips@netisense.com>
Fixes: 92ce45fb875d ("cpufreq: Add DVFS support for Armada 37xx")
Cc: stable@vger.kernel.org # 8db82563451f ("cpufreq: armada-37xx: fix frequency calculation for opp")
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index 1ab2113daef5..e4782f562e7a 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -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;
-- 
2.20.1

[PATCH mvebu v3 09/10] cpufreq: armada-37xx: Remove cur_frequency variable

[Pali Rohár]

Variable cur_frequency in armada37xx_cpufreq_driver_init() is unused.

Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
Tested-by: Philip Soares <philips@netisense.com>
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index e4782f562e7a..050abff18308 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -400,7 +400,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;
@@ -461,14 +461,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);
-- 
2.20.1

[PATCH mvebu v3 10/10] cpufreq: armada-37xx: Fix module unloading

[Pali Rohár]

This driver is missing module_exit hook. Add proper driver exit function
which unregisters the platform device and cleans up the data.

Signed-off-by: Pali Rohár <pali@kernel.org>
Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
Tested-by: Philip Soares <philips@netisense.com>
---
 drivers/cpufreq/armada-37xx-cpufreq.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index 050abff18308..3fc98a3ffd91 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -86,6 +86,8 @@ static int avs_map[] = {
 };
 
 struct armada37xx_cpufreq_state {
+	struct platform_device *pdev;
+	struct device *cpu_dev;
 	struct regmap *regmap;
 	u32 nb_l0l1;
 	u32 nb_l2l3;
@@ -506,6 +508,9 @@ static int __init armada37xx_cpufreq_driver_init(void)
 	if (ret)
 		goto disable_dvfs;
 
+	armada37xx_cpufreq_state->cpu_dev = cpu_dev;
+	armada37xx_cpufreq_state->pdev = pdev;
+	platform_set_drvdata(pdev, dvfs);
 	return 0;
 
 disable_dvfs:
@@ -524,6 +529,26 @@ static int __init armada37xx_cpufreq_driver_init(void)
 /* late_initcall, to guarantee the driver is loaded after A37xx clock driver */
 late_initcall(armada37xx_cpufreq_driver_init);
 
+static void __exit armada37xx_cpufreq_driver_exit(void)
+{
+	struct platform_device *pdev = armada37xx_cpufreq_state->pdev;
+	struct armada_37xx_dvfs *dvfs = platform_get_drvdata(pdev);
+	unsigned long freq;
+	int load_lvl;
+
+	platform_device_unregister(pdev);
+
+	armada37xx_cpufreq_disable_dvfs(armada37xx_cpufreq_state->regmap);
+
+	for (load_lvl = ARMADA_37XX_DVFS_LOAD_0; load_lvl < LOAD_LEVEL_NR; load_lvl++) {
+		freq = dvfs->cpu_freq_max / dvfs->divider[load_lvl];
+		dev_pm_opp_remove(armada37xx_cpufreq_state->cpu_dev, freq);
+	}
+
+	kfree(armada37xx_cpufreq_state);
+}
+module_exit(armada37xx_cpufreq_driver_exit);
+
 static const struct of_device_id __maybe_unused armada37xx_cpufreq_of_match[] = {
 	{ .compatible = "marvell,armada-3700-nb-pm" },
 	{ },
-- 
2.20.1

Re: [PATCH mvebu v3 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

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?

On Monday 22 February 2021 20:41:48 Pali Rohár wrote:
> 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(-)
> 
> -- 
> 2.20.1
> 

Re: [PATCH mvebu v3 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Gregory CLEMENT]

Hello Pali,

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

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

>
> On Monday 22 February 2021 20:41:48 Pali Rohár wrote:
>> 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(-)
>> 
>> -- 
>> 2.20.1
>> 

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH mvebu v3 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Marek Behún]

On Fri, 12 Mar 2021 10:12:06 +0100
Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> Hello Pali,
> 
> > 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?  
> 
> 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

Gregory, could you at least add Acked-by, or Reviewed-by? So that
clk subsystem maintainer will trust these changes, since you are the
original author of armada-37xx-periph.

Marek

Re: [PATCH mvebu v3 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

On Friday 12 March 2021 10:12:06 Gregory CLEMENT wrote:
> Hello Pali,
> 
> > 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?
> 
> 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?

Re: [PATCH mvebu v3 02/10] cpufreq: armada-37xx: Fix setting TBG parent for load levels

[Gregory CLEMENT]

Pali Rohár <pali@kernel.org> writes:

> From: Marek Behún <kabel@kernel.org>
>
> 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 <kabel@kernel.org>
> Tested-by: Pali Rohár <pali@kernel.org>
> Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
> Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
> Tested-by: Philip Soares <philips@netisense.com>
> Fixes: 92ce45fb875d ("cpufreq: Add DVFS support for Armada 37xx")
> Cc: stable@vger.kernel.org

Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks,

Gregory
> ---
>  drivers/cpufreq/armada-37xx-cpufreq.c | 35 ++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
> index b4af4094309b..b8dc6c849579 100644
> --- a/drivers/cpufreq/armada-37xx-cpufreq.c
> +++ b/drivers/cpufreq/armada-37xx-cpufreq.c
> @@ -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;
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH mvebu v3 03/10] clk: mvebu: armada-37xx-periph: remove .set_parent method for CPU PM clock

[Gregory CLEMENT]

Pali Rohár <pali@kernel.org> writes:

> From: Marek Behún <kabel@kernel.org>
>
> 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 <kabel@kernel.org>
> Acked-by: Stephen Boyd <sboyd@kernel.org>
> Tested-by: Pali Rohár <pali@kernel.org>
> Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
> Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
> Tested-by: Philip Soares <philips@netisense.com>
> Fixes: 2089dc33ea0e ("clk: mvebu: armada-37xx-periph: add DVFS support for cpu clocks")

Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks,

Gregory
> ---
>  drivers/clk/mvebu/armada-37xx-periph.c | 28 --------------------------
>  1 file changed, 28 deletions(-)
>
> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> index f5746f9ea929..6507bd2c5f31 100644
> --- a/drivers/clk/mvebu/armada-37xx-periph.c
> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> @@ -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,
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH mvebu v3 04/10] cpufreq: armada-37xx: Fix the AVS value for load L1

[Gregory CLEMENT]

Pali Rohár <pali@kernel.org> writes:

> 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 <pali@kernel.org>
> Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
> Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
> Tested-by: Philip Soares <philips@netisense.com>
> Fixes: 1c3528232f4b ("cpufreq: armada-37xx: Add AVS support")
> Cc: stable@vger.kernel.org

Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks,

Gregory
> ---
>  drivers/cpufreq/armada-37xx-cpufreq.c | 37 +++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
> index b8dc6c849579..c7683d447b11 100644
> --- a/drivers/cpufreq/armada-37xx-cpufreq.c
> +++ b/drivers/cpufreq/armada-37xx-cpufreq.c
> @@ -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,
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH mvebu v3 05/10] clk: mvebu: armada-37xx-periph: Fix switching CPU freq from 250 Mhz to 1 GHz

[Gregory CLEMENT]

Pali Rohár <pali@kernel.org> writes:

> 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 <pali@kernel.org>
> Acked-by: Stephen Boyd <sboyd@kernel.org>
> Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
> Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
> Tested-by: Philip Soares <philips@netisense.com>
> Fixes: 2089dc33ea0e ("clk: mvebu: armada-37xx-periph: add DVFS support for cpu clocks")
> Cc: stable@vger.kernel.org # 61c40f35f5cd ("clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz")


Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks,

Gregory

> ---
>  drivers/clk/mvebu/armada-37xx-periph.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> index 6507bd2c5f31..b15e177bea7e 100644
> --- a/drivers/clk/mvebu/armada-37xx-periph.c
> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> @@ -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);
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH mvebu v3 06/10] clk: mvebu: armada-37xx-periph: Fix workaround for switching from L1 to L0

[Gregory CLEMENT]

Pali Rohár <pali@kernel.org> writes:

> 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 <kabel@kernel.org>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Acked-by: Stephen Boyd <sboyd@kernel.org>
> Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
> Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
> Tested-by: Philip Soares <philips@netisense.com>
> Fixes: 61c40f35f5cd ("clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz")
> Cc: stable@vger.kernel.org

Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks,

Gregory
> ---
>  drivers/clk/mvebu/armada-37xx-periph.c | 45 ++++++++++++++++++++++----
>  1 file changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> index b15e177bea7e..32ac6b6b7530 100644
> --- a/drivers/clk/mvebu/armada-37xx-periph.c
> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> @@ -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);
>  
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH mvebu v3 07/10] cpufreq: armada-37xx: Fix driver cleanup when registration failed

[Gregory CLEMENT]

Pali Rohár <pali@kernel.org> writes:

> 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 <pali@kernel.org>
> Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
> Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
> Tested-by: Philip Soares <philips@netisense.com>
> Fixes: 8db82563451f ("cpufreq: armada-37xx: fix frequency calculation for opp")
> Cc: stable@vger.kernel.org

Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks,

Gregory
> ---
>  drivers/cpufreq/armada-37xx-cpufreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
> index c7683d447b11..1ab2113daef5 100644
> --- a/drivers/cpufreq/armada-37xx-cpufreq.c
> +++ b/drivers/cpufreq/armada-37xx-cpufreq.c
> @@ -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);
>  	}
>  
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH mvebu v3 08/10] cpufreq: armada-37xx: Fix determining base CPU frequency

[Gregory CLEMENT]

Pali Rohár <pali@kernel.org> writes:

> 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 <pali@kernel.org>
> Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
> Tested-by: Anders Trier Olesen <anders.trier.olesen@gmail.com>
> Tested-by: Philip Soares <philips@netisense.com>
> Fixes: 92ce45fb875d ("cpufreq: Add DVFS support for Armada 37xx")
> Cc: stable@vger.kernel.org # 8db82563451f ("cpufreq: armada-37xx: fix
> frequency calculation for opp")

Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks,

Gregory

> ---
>  drivers/cpufreq/armada-37xx-cpufreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
> index 1ab2113daef5..e4782f562e7a 100644
> --- a/drivers/cpufreq/armada-37xx-cpufreq.c
> +++ b/drivers/cpufreq/armada-37xx-cpufreq.c
> @@ -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;
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH mvebu v2 09/10] cpufreq: armada-37xx: Remove cur_frequency variable

[Gregory CLEMENT]

Pali Rohár <pali@kernel.org> writes:

> Variable cur_frequency in armada37xx_cpufreq_driver_init() is unused.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>

Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks,

Gregory

> ---
>  drivers/cpufreq/armada-37xx-cpufreq.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
> index f08281fc525c..f13646d143de 100644
> --- a/drivers/cpufreq/armada-37xx-cpufreq.c
> +++ b/drivers/cpufreq/armada-37xx-cpufreq.c
> @@ -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);
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH mvebu v2 09/10] cpufreq: armada-37xx: Remove cur_frequency variable

[Marek Behún]

On Mon, 29 Mar 2021 17:00:12 +0200
Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> Pali Rohár <pali@kernel.org> writes:
> 
> > Variable cur_frequency in armada37xx_cpufreq_driver_init() is unused.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>  
> 
> Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Gregory, THX for the acks.

Will you be merging these patches or should this be done by
someone else? cpufreq maintainers maybe?

Marek

Re: [PATCH mvebu v2 01/10] arm64: dts: marvell: armada-37xx: add syscon compatible to NB clk node

[Gregory CLEMENT]

Hi Pali,

> From: Marek Behún <kabel@kernel.org>
>
> 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 <kabel@kernel.org>
> Fixes: e8d66e7927b2 ("arm64: dts: marvell: armada-37xx: add nodes...")
> Cc: stable@vger.kernel.org
> Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>

Applied on mvebu/dt64

Thanks,

Gregory

> ---
>  arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> index d5b6c0a1c54a..a89e47d95eef 100644
> --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> @@ -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>;
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH mvebu v3 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Stephen Boyd]

Quoting Pali Rohár (2021-03-28 04:31:02)
> On Friday 12 March 2021 10:12:06 Gregory CLEMENT wrote:
> > Hello Pali,
> > 
> > > 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?
> > 
> > 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?

Maybe it can merge through cpufreq tree? I've already acked the clk
driver patches I believe.

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Philip Soares]

Tested-by: Philip Soares <philips@netisense.com>

On Mon, Feb 22, 2021, at 8:36 AM, nnet wrote:
> > Could I add your Tested-by line to patch series?
> 
> Yes, by: Philip Soares <philips@netisense.com>
> 
> Thank you for the patches!
>

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

> Could I add your Tested-by line to patch series?

Yes, by: Philip Soares <philips@netisense.com>

Thank you for the patches!

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

On Sunday 21 February 2021 19:17:40 nnet wrote:
> > Could you test if 1.155V voltage for L1 is stable on 1.2 GHz variant?
> 
> ++#define MIN_VOLT_MV_FOR_L1_1200MHZ 1155
> ...
> ++              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;
> 
> This works fine. Tested with switching 600MHz to 1.2GHz under load.

Perfect! Therefore here is final version of patch 04/10 for both 1 GHz
and 1.2 GHz variants of A3720 SoC. I will resend whole patch series.
Could I add your Tested-by line to patch series?

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index b8dc6c849..c7683d447 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -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,

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

> Could you test if 1.155V voltage for L1 is stable on 1.2 GHz variant?

++#define MIN_VOLT_MV_FOR_L1_1200MHZ 1155
...
++              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;

This works fine. Tested with switching 600MHz to 1.2GHz under load.

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

On Tuesday 16 February 2021 08:27:10 nnet wrote:
> > Therefore I'm thinking if the correct way is instead to use L1 := L0 voltage value for 1/1.2 GHz mode.
> 
> This latest 04/10 works fine for me going 600MHz <-> 1.2GHz under with and without load.

Ok, thanks for testing! Just to note that typical documented value for
1.2GHz mode is 1.155V, so it would be useful to know if this value could
be stable for L1 with 1.2GHz mode.

I'm thinking that for 1GHz variant it would be better to rather use
1.108V like in my original patch as this is already tested by lot of
people, nobody complained yet and it can be lower value as L0 (so there
is benefit to decrease CPU frequency when CPU is idle).

For 1.2GHz variant I still do not know. You wrote that 1.132V is
unstable, so it cannot be used for sure. Documented typical value 1.155V
is bigger, so maybe it can be stable but needs testing. And stable seems
to be L0 value... But then I do not see a benefit for downclocking CPU
from 1.2 GHz frequency in L0 to 600 MHz freq. in L1 if it use same CPU
voltage... But it is still better than unstable CPU with crashes!

Could you test if 1.155V voltage for L1 is stable on 1.2 GHz variant?

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

> Therefore I'm thinking if the correct way is instead to use L1 := L0 voltage value for 1/1.2 GHz mode.

This latest 04/10 works fine for me going 600MHz <-> 1.2GHz under with and without load.

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

On Monday 15 February 2021 21:48:08 nnet wrote:
> > Could you test following change instead of PATCH 04/10? I added here also
> > logic for 1.2 GHz variant with 1.132 V value another change is that
> > value for load L0 is not touched as it is stable.
> 
> These changes to patch 04/10 worked going 600 MHz <-> 1.2 GHz , _but_ only with:
> 
> ++#define MIN_VOLT_MV_FOR_L1_1200MHZ 1213
> 
> During this latest testing I saw freezes with 1132 mV.
> 
> I've had no lockups with 1213 mV which I just used from the values for L1/L0 from OTP.

I still do not know what is the meaning of values stored in OTP...
And there are more non-zero bits which are not used (yet).

Anyway, with your another test it looks like that limit is not based on
fixed value but rather on current L0 value. Therefore I'm thinking if
the correct way is instead to use L1 := L0 voltage value for 1/1.2 GHz
mode. Could you try following change instead of previous and PATCH 04/10?

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index b8dc6c849..12d0ff7b1 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -208,6 +208,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,17 +241,36 @@ 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(1155);
+			else
+				avs_min = armada_37xx_avs_val_match(1108);
+			dvfs->avs[0] = dvfs->avs[1] = avs_min;
+		}
+
 		return;
 	}
 
 	/*
 	 * L1 voltage is equal to L0 voltage - 100mv and it must be
-	 * larger than 1000mv
+	 * larger than 1000mv. When base CPU frequency is 1000/1200 MHz,
+	 * L1 voltage must must be equal to L0 voltage, otherwise
+	 * the CPU gets stuck when switching from load L1 to load L0.
 	 */
 
-	target_vm = avs_map[l0_vdd_min] - 100;
-	target_vm = target_vm > MIN_VOLT_MV ? target_vm : MIN_VOLT_MV;
-	dvfs->avs[1] = armada_37xx_avs_val_match(target_vm);
+	if (dvfs->cpu_freq_max >= 1000*1000*1000) {
+		dvfs->avs[1] = dvfs->avs[0];
+	} else {
+		target_vm = avs_map[l0_vdd_min] - 100;
+		target_vm = target_vm > MIN_VOLT_MV ? target_vm : MIN_VOLT_MV;
+		dvfs->avs[1] = armada_37xx_avs_val_match(target_vm);
+	}
 
 	/*
 	 * L2 & L3 voltage is equal to L0 voltage - 150mv and it must

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

> Could you test following change instead of PATCH 04/10? I added here also
> logic for 1.2 GHz variant with 1.132 V value another change is that
> value for load L0 is not touched as it is stable.

These changes to patch 04/10 worked going 600 MHz <-> 1.2 GHz , _but_ only with:

++#define MIN_VOLT_MV_FOR_L1_1200MHZ 1213

During this latest testing I saw freezes with 1132 mV.

I've had no lockups with 1213 mV which I just used from the values for L1/L0 from OTP.

I only tested with those two values.

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

On Saturday 13 February 2021 10:30:19 nnet wrote:
> On Sat, Feb 13, 2021, at 2:01 AM, Pali Rohár wrote:
> > On Thursday 11 February 2021 16:41:13 nnet wrote:
> > > On Thu, Feb 11, 2021, at 3:44 PM, Pali Rohár wrote:
> > > > On Thursday 11 February 2021 12:22:52 nnet wrote:
> > > > > On Thu, Feb 11, 2021, at 11:55 AM, Pali Rohár wrote:
> > > > > > On Wednesday 10 February 2021 11:08:59 nnet wrote:
> > > > > > > On Wed, Feb 10, 2021, at 10:03 AM, Pali Rohár wrote:
> > > > > > > > > > Hello! Could you please enable userspace governor during kernel
> > > > > > > > > > compilation?
> > > > > > > > > > 
> > > > > > > > > >     CONFIG_CPU_FREQ_GOV_USERSPACE=y
> > > > > > > > > > 
> > > > > > > > > > It can be activated via command:
> > > > > > > > > > 
> > > > > > > > > >     echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > > > > > > 
> > > > > > > > > > After that you can "force" CPU frequency to specific value, e.g.:
> > > > > > > > > > 
> > > > > > > > > >     echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed
> > > > > > > > > > 
> > > > > > > > > > I need to know which switch (from --> to freq) cause this system hang.
> > > > > > > > > > 
> > > > > > > > > > This patch series (via MIN_VOLT_MV_FOR_L0_L1_1GHZ) is fixing only
> > > > > > > > > > switching from 500 MHz to 1000 MHz on 1 GHz variant. As only this switch
> > > > > > > > > > is causing issue.
> > > > > > > > > > 
> > > > > > > > > > I have used following simple bash script to check that switching between
> > > > > > > > > > 500 MHz and 1 GHz is stable:
> > > > > > > > > > 
> > > > > > > > > >     while true; do
> > > > > > > > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > > >     done
> > > > > > > > > 
> > > > > > > > > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > > > > > while true; do
> > > > > > > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > > done
> > > > > > > > > 
> > > > > > > > > >> +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > > > > > > > > 
> > > > > > > > > With 1108 I get a freeze within a minute. The last output to stdout is 600000.
> > > > > > > > > 
> > > > > > > > > With 1120 it takes a few minutes.
> > > > > > > > > 
> > > > > > > > > With any of 1225, 1155, 1132 the device doesn't freeze over the full 5 minute load test.
> > > > > > > > > 
> > > > > > > > > I'm using ondemand now with the above at 1132 without issue so far.
> > > > > > > > 
> > > > > > > > Great, thank you for testing!
> > > > > > > > 
> > > > > > > > Can you check if switching between any two lower frequencies 200000
> > > > > > > > 300000 600000 is stable?
> > > > > > > 
> > > > > > > This is stable using 1132 mV for MIN_VOLT_MV_FOR_L0_L1_1GHZ:
> > > > > > > 
> > > > > > > while true; do
> > > > > > >   # down
> > > > > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > ...
> > > > > > 
> > > > > > Hello!
> > > > > > 
> > > > > > Could you please re-run test without tee, in form as I have shown above?
> > > > > > UART is slow and printing something to console adds delay which decrease
> > > > > > probability that real issue is triggered as this is timing issue.
> > > > > 
> > > > > The test was done over SSH.
> > > > 
> > > > Ok! But it is still better to not print any results as it adds unwanted
> > > > delay between frequency switching.
> > > > 
> > > > > > Also please do tests just between two frequencies in loop as I observed
> > > > > > that switching between more decreased probability to hit issue.
> > > > > 
> > > > > > > > > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > > > > > while true; do
> > > > > > > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > > done
> > > > > 
> > > > > The first test ^ switched between 600 MHz and 1.2 GHz.
> > > > > 
> > > > > > The real issue for 1 GHz variant of A3720 is only when doing switch from
> > > > > > 500 MHz to 1 GHz. So could you try to do some tests also without
> > > > > > changing MIN_VOLT_MV_FOR_L0_L1_1GHZ and switching just between non-1.2
> > > > > > frequencies (to verify that on 1.2 GHz variant it is also from 600 MHz
> > > > > > to 1.2 GHz)?
> > > > > 
> > > > > With 1108 mV and switching between 600 MHz and 1.2GHz, I always saw a freeze within a minute.
> > > > 
> > > > I mean to try switching with 1.108 V between 200 MHz and 300 MHz or
> > > > between 300 MHz and 600 MHz. To check that issue is really only with
> > > > switch from 600 MHz to 1.2 GHz.
> > > 
> > > With:
> > > 
> > > +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > > 
> > > with 5 min load:
> > > 
> > > # no lock-up
> > > 
> > > echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > while true; do
> > >   echo 200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > >   echo 300000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > done
> > > 
> > > # no lock-up
> > > 
> > > echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > while true; do
> > >   echo 300000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > >   echo 600000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > done
> > > 
> > > # lock-up with 10 seconds of load applied
> > > 
> > > echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > while true; do
> > >   echo 600000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > >   echo 1200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > done
> > 
> > Ok! So it really looks like that on 1.2 GHz is the same issue. We need
> > to increase voltage for L1 load (600 MHz). But question is what is the
> > threshold (it is 1132 mV or lower?) 
> 
> Lower then 1132 mV causes freezes. No freezes with 1132 mV.
> 
> > and second question is what
> > increasing minimal voltage may cause with board.
> 
> I don't have any further information on the issue and device then that experience.
> 
> I've tested on two of these devices which suggests it's not an oddity with one instance of it.

Ok, thank you for information.

Could you test following change instead of PATCH 04/10? I added here also
logic for 1.2 GHz variant with 1.132 V value and another change is that
value for load L0 is not touched as it is stable.

diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index b8dc6c849..8a4afb048 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -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 1132
 
 /*  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,18 @@ 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 value for load L0 and L1 when base CPU frequency is 1000/1200 MHz,
+		 * otherwise the CPU gets stuck when switching from load L1 to load L0.
+		 */
+		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 +274,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,

> > 
> > Basically there is absolutely no information about 1.2 GHz variant and
> > this issue...
> >
> > > > I need to know if current settings are fine for 200, 300 and 600 MHz
> > > > frequencies and the only 600 --> 1200 needs to be fixed.
> > > >
> >

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

On Sat, Feb 13, 2021, at 2:01 AM, Pali Rohár wrote:
> On Thursday 11 February 2021 16:41:13 nnet wrote:
> > On Thu, Feb 11, 2021, at 3:44 PM, Pali Rohár wrote:
> > > On Thursday 11 February 2021 12:22:52 nnet wrote:
> > > > On Thu, Feb 11, 2021, at 11:55 AM, Pali Rohár wrote:
> > > > > On Wednesday 10 February 2021 11:08:59 nnet wrote:
> > > > > > On Wed, Feb 10, 2021, at 10:03 AM, Pali Rohár wrote:
> > > > > > > > > Hello! Could you please enable userspace governor during kernel
> > > > > > > > > compilation?
> > > > > > > > > 
> > > > > > > > >     CONFIG_CPU_FREQ_GOV_USERSPACE=y
> > > > > > > > > 
> > > > > > > > > It can be activated via command:
> > > > > > > > > 
> > > > > > > > >     echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > > > > > 
> > > > > > > > > After that you can "force" CPU frequency to specific value, e.g.:
> > > > > > > > > 
> > > > > > > > >     echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed
> > > > > > > > > 
> > > > > > > > > I need to know which switch (from --> to freq) cause this system hang.
> > > > > > > > > 
> > > > > > > > > This patch series (via MIN_VOLT_MV_FOR_L0_L1_1GHZ) is fixing only
> > > > > > > > > switching from 500 MHz to 1000 MHz on 1 GHz variant. As only this switch
> > > > > > > > > is causing issue.
> > > > > > > > > 
> > > > > > > > > I have used following simple bash script to check that switching between
> > > > > > > > > 500 MHz and 1 GHz is stable:
> > > > > > > > > 
> > > > > > > > >     while true; do
> > > > > > > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > >     done
> > > > > > > > 
> > > > > > > > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > > > > while true; do
> > > > > > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > done
> > > > > > > > 
> > > > > > > > >> +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > > > > > > > 
> > > > > > > > With 1108 I get a freeze within a minute. The last output to stdout is 600000.
> > > > > > > > 
> > > > > > > > With 1120 it takes a few minutes.
> > > > > > > > 
> > > > > > > > With any of 1225, 1155, 1132 the device doesn't freeze over the full 5 minute load test.
> > > > > > > > 
> > > > > > > > I'm using ondemand now with the above at 1132 without issue so far.
> > > > > > > 
> > > > > > > Great, thank you for testing!
> > > > > > > 
> > > > > > > Can you check if switching between any two lower frequencies 200000
> > > > > > > 300000 600000 is stable?
> > > > > > 
> > > > > > This is stable using 1132 mV for MIN_VOLT_MV_FOR_L0_L1_1GHZ:
> > > > > > 
> > > > > > while true; do
> > > > > >   # down
> > > > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > ...
> > > > > 
> > > > > Hello!
> > > > > 
> > > > > Could you please re-run test without tee, in form as I have shown above?
> > > > > UART is slow and printing something to console adds delay which decrease
> > > > > probability that real issue is triggered as this is timing issue.
> > > > 
> > > > The test was done over SSH.
> > > 
> > > Ok! But it is still better to not print any results as it adds unwanted
> > > delay between frequency switching.
> > > 
> > > > > Also please do tests just between two frequencies in loop as I observed
> > > > > that switching between more decreased probability to hit issue.
> > > > 
> > > > > > > > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > > > > while true; do
> > > > > > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > > done
> > > > 
> > > > The first test ^ switched between 600 MHz and 1.2 GHz.
> > > > 
> > > > > The real issue for 1 GHz variant of A3720 is only when doing switch from
> > > > > 500 MHz to 1 GHz. So could you try to do some tests also without
> > > > > changing MIN_VOLT_MV_FOR_L0_L1_1GHZ and switching just between non-1.2
> > > > > frequencies (to verify that on 1.2 GHz variant it is also from 600 MHz
> > > > > to 1.2 GHz)?
> > > > 
> > > > With 1108 mV and switching between 600 MHz and 1.2GHz, I always saw a freeze within a minute.
> > > 
> > > I mean to try switching with 1.108 V between 200 MHz and 300 MHz or
> > > between 300 MHz and 600 MHz. To check that issue is really only with
> > > switch from 600 MHz to 1.2 GHz.
> > 
> > With:
> > 
> > +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > 
> > with 5 min load:
> > 
> > # no lock-up
> > 
> > echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > while true; do
> >   echo 200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> >   echo 300000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > done
> > 
> > # no lock-up
> > 
> > echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > while true; do
> >   echo 300000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> >   echo 600000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > done
> > 
> > # lock-up with 10 seconds of load applied
> > 
> > echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > while true; do
> >   echo 600000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> >   echo 1200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > done
> 
> Ok! So it really looks like that on 1.2 GHz is the same issue. We need
> to increase voltage for L1 load (600 MHz). But question is what is the
> threshold (it is 1132 mV or lower?) 

Lower then 1132 mV causes freezes. No freezes with 1132 mV.

> and second question is what
> increasing minimal voltage may cause with board.

I don't have any further information on the issue and device then that experience.

I've tested on two of these devices which suggests it's not an oddity with one instance of it.

> 
> Basically there is absolutely no information about 1.2 GHz variant and
> this issue...
>
> > > I need to know if current settings are fine for 200, 300 and 600 MHz
> > > frequencies and the only 600 --> 1200 needs to be fixed.
> > >
>

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

On Thursday 11 February 2021 16:41:13 nnet wrote:
> On Thu, Feb 11, 2021, at 3:44 PM, Pali Rohár wrote:
> > On Thursday 11 February 2021 12:22:52 nnet wrote:
> > > On Thu, Feb 11, 2021, at 11:55 AM, Pali Rohár wrote:
> > > > On Wednesday 10 February 2021 11:08:59 nnet wrote:
> > > > > On Wed, Feb 10, 2021, at 10:03 AM, Pali Rohár wrote:
> > > > > > > > Hello! Could you please enable userspace governor during kernel
> > > > > > > > compilation?
> > > > > > > > 
> > > > > > > >     CONFIG_CPU_FREQ_GOV_USERSPACE=y
> > > > > > > > 
> > > > > > > > It can be activated via command:
> > > > > > > > 
> > > > > > > >     echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > > > > 
> > > > > > > > After that you can "force" CPU frequency to specific value, e.g.:
> > > > > > > > 
> > > > > > > >     echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed
> > > > > > > > 
> > > > > > > > I need to know which switch (from --> to freq) cause this system hang.
> > > > > > > > 
> > > > > > > > This patch series (via MIN_VOLT_MV_FOR_L0_L1_1GHZ) is fixing only
> > > > > > > > switching from 500 MHz to 1000 MHz on 1 GHz variant. As only this switch
> > > > > > > > is causing issue.
> > > > > > > > 
> > > > > > > > I have used following simple bash script to check that switching between
> > > > > > > > 500 MHz and 1 GHz is stable:
> > > > > > > > 
> > > > > > > >     while true; do
> > > > > > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > >     done
> > > > > > > 
> > > > > > > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > > > while true; do
> > > > > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > done
> > > > > > > 
> > > > > > > >> +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > > > > > > 
> > > > > > > With 1108 I get a freeze within a minute. The last output to stdout is 600000.
> > > > > > > 
> > > > > > > With 1120 it takes a few minutes.
> > > > > > > 
> > > > > > > With any of 1225, 1155, 1132 the device doesn't freeze over the full 5 minute load test.
> > > > > > > 
> > > > > > > I'm using ondemand now with the above at 1132 without issue so far.
> > > > > > 
> > > > > > Great, thank you for testing!
> > > > > > 
> > > > > > Can you check if switching between any two lower frequencies 200000
> > > > > > 300000 600000 is stable?
> > > > > 
> > > > > This is stable using 1132 mV for MIN_VOLT_MV_FOR_L0_L1_1GHZ:
> > > > > 
> > > > > while true; do
> > > > >   # down
> > > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > ...
> > > > 
> > > > Hello!
> > > > 
> > > > Could you please re-run test without tee, in form as I have shown above?
> > > > UART is slow and printing something to console adds delay which decrease
> > > > probability that real issue is triggered as this is timing issue.
> > > 
> > > The test was done over SSH.
> > 
> > Ok! But it is still better to not print any results as it adds unwanted
> > delay between frequency switching.
> > 
> > > > Also please do tests just between two frequencies in loop as I observed
> > > > that switching between more decreased probability to hit issue.
> > > 
> > > > > > > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > > > while true; do
> > > > > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > > done
> > > 
> > > The first test ^ switched between 600 MHz and 1.2 GHz.
> > > 
> > > > The real issue for 1 GHz variant of A3720 is only when doing switch from
> > > > 500 MHz to 1 GHz. So could you try to do some tests also without
> > > > changing MIN_VOLT_MV_FOR_L0_L1_1GHZ and switching just between non-1.2
> > > > frequencies (to verify that on 1.2 GHz variant it is also from 600 MHz
> > > > to 1.2 GHz)?
> > > 
> > > With 1108 mV and switching between 600 MHz and 1.2GHz, I always saw a freeze within a minute.
> > 
> > I mean to try switching with 1.108 V between 200 MHz and 300 MHz or
> > between 300 MHz and 600 MHz. To check that issue is really only with
> > switch from 600 MHz to 1.2 GHz.
> 
> With:
> 
> +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> 
> with 5 min load:
> 
> # no lock-up
> 
> echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> while true; do
>   echo 200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
>   echo 300000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> done
> 
> # no lock-up
> 
> echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> while true; do
>   echo 300000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
>   echo 600000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> done
> 
> # lock-up with 10 seconds of load applied
> 
> echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> while true; do
>   echo 600000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
>   echo 1200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> done

Ok! So it really looks like that on 1.2 GHz is the same issue. We need
to increase voltage for L1 load (600 MHz). But question is what is the
threshold (it is 1132 mV or lower?) and second question is what
increasing minimal voltage may cause with board.

Basically there is absolutely no information about 1.2 GHz variant and
this issue...

> > I need to know if current settings are fine for 200, 300 and 600 MHz
> > frequencies and the only 600 --> 1200 needs to be fixed.
> >

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

On Thu, Feb 11, 2021, at 3:44 PM, Pali Rohár wrote:
> On Thursday 11 February 2021 12:22:52 nnet wrote:
> > On Thu, Feb 11, 2021, at 11:55 AM, Pali Rohár wrote:
> > > On Wednesday 10 February 2021 11:08:59 nnet wrote:
> > > > On Wed, Feb 10, 2021, at 10:03 AM, Pali Rohár wrote:
> > > > > > > Hello! Could you please enable userspace governor during kernel
> > > > > > > compilation?
> > > > > > > 
> > > > > > >     CONFIG_CPU_FREQ_GOV_USERSPACE=y
> > > > > > > 
> > > > > > > It can be activated via command:
> > > > > > > 
> > > > > > >     echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > > > 
> > > > > > > After that you can "force" CPU frequency to specific value, e.g.:
> > > > > > > 
> > > > > > >     echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed
> > > > > > > 
> > > > > > > I need to know which switch (from --> to freq) cause this system hang.
> > > > > > > 
> > > > > > > This patch series (via MIN_VOLT_MV_FOR_L0_L1_1GHZ) is fixing only
> > > > > > > switching from 500 MHz to 1000 MHz on 1 GHz variant. As only this switch
> > > > > > > is causing issue.
> > > > > > > 
> > > > > > > I have used following simple bash script to check that switching between
> > > > > > > 500 MHz and 1 GHz is stable:
> > > > > > > 
> > > > > > >     while true; do
> > > > > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > >     done
> > > > > > 
> > > > > > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > > while true; do
> > > > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > done
> > > > > > 
> > > > > > >> +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > > > > > 
> > > > > > With 1108 I get a freeze within a minute. The last output to stdout is 600000.
> > > > > > 
> > > > > > With 1120 it takes a few minutes.
> > > > > > 
> > > > > > With any of 1225, 1155, 1132 the device doesn't freeze over the full 5 minute load test.
> > > > > > 
> > > > > > I'm using ondemand now with the above at 1132 without issue so far.
> > > > > 
> > > > > Great, thank you for testing!
> > > > > 
> > > > > Can you check if switching between any two lower frequencies 200000
> > > > > 300000 600000 is stable?
> > > > 
> > > > This is stable using 1132 mV for MIN_VOLT_MV_FOR_L0_L1_1GHZ:
> > > > 
> > > > while true; do
> > > >   # down
> > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > ...
> > > 
> > > Hello!
> > > 
> > > Could you please re-run test without tee, in form as I have shown above?
> > > UART is slow and printing something to console adds delay which decrease
> > > probability that real issue is triggered as this is timing issue.
> > 
> > The test was done over SSH.
> 
> Ok! But it is still better to not print any results as it adds unwanted
> delay between frequency switching.
> 
> > > Also please do tests just between two frequencies in loop as I observed
> > > that switching between more decreased probability to hit issue.
> > 
> > > > > > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > > while true; do
> > > > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > > done
> > 
> > The first test ^ switched between 600 MHz and 1.2 GHz.
> > 
> > > The real issue for 1 GHz variant of A3720 is only when doing switch from
> > > 500 MHz to 1 GHz. So could you try to do some tests also without
> > > changing MIN_VOLT_MV_FOR_L0_L1_1GHZ and switching just between non-1.2
> > > frequencies (to verify that on 1.2 GHz variant it is also from 600 MHz
> > > to 1.2 GHz)?
> > 
> > With 1108 mV and switching between 600 MHz and 1.2GHz, I always saw a freeze within a minute.
> 
> I mean to try switching with 1.108 V between 200 MHz and 300 MHz or
> between 300 MHz and 600 MHz. To check that issue is really only with
> switch from 600 MHz to 1.2 GHz.

With:

+#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108

with 5 min load:

# no lock-up

echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
while true; do
  echo 200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
  echo 300000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
done

# no lock-up

echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
while true; do
  echo 300000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
  echo 600000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
done

# lock-up with 10 seconds of load applied

echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
while true; do
  echo 600000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
  echo 1200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
done

> I need to know if current settings are fine for 200, 300 and 600 MHz
> frequencies and the only 600 --> 1200 needs to be fixed.
>

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

On Thursday 11 February 2021 12:22:52 nnet wrote:
> On Thu, Feb 11, 2021, at 11:55 AM, Pali Rohár wrote:
> > On Wednesday 10 February 2021 11:08:59 nnet wrote:
> > > On Wed, Feb 10, 2021, at 10:03 AM, Pali Rohár wrote:
> > > > > > Hello! Could you please enable userspace governor during kernel
> > > > > > compilation?
> > > > > > 
> > > > > >     CONFIG_CPU_FREQ_GOV_USERSPACE=y
> > > > > > 
> > > > > > It can be activated via command:
> > > > > > 
> > > > > >     echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > > 
> > > > > > After that you can "force" CPU frequency to specific value, e.g.:
> > > > > > 
> > > > > >     echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed
> > > > > > 
> > > > > > I need to know which switch (from --> to freq) cause this system hang.
> > > > > > 
> > > > > > This patch series (via MIN_VOLT_MV_FOR_L0_L1_1GHZ) is fixing only
> > > > > > switching from 500 MHz to 1000 MHz on 1 GHz variant. As only this switch
> > > > > > is causing issue.
> > > > > > 
> > > > > > I have used following simple bash script to check that switching between
> > > > > > 500 MHz and 1 GHz is stable:
> > > > > > 
> > > > > >     while true; do
> > > > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > >     done
> > > > > 
> > > > > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > while true; do
> > > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > done
> > > > > 
> > > > > >> +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > > > > 
> > > > > With 1108 I get a freeze within a minute. The last output to stdout is 600000.
> > > > > 
> > > > > With 1120 it takes a few minutes.
> > > > > 
> > > > > With any of 1225, 1155, 1132 the device doesn't freeze over the full 5 minute load test.
> > > > > 
> > > > > I'm using ondemand now with the above at 1132 without issue so far.
> > > > 
> > > > Great, thank you for testing!
> > > > 
> > > > Can you check if switching between any two lower frequencies 200000
> > > > 300000 600000 is stable?
> > > 
> > > This is stable using 1132 mV for MIN_VOLT_MV_FOR_L0_L1_1GHZ:
> > > 
> > > while true; do
> > >   # down
> > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > ...
> > 
> > Hello!
> > 
> > Could you please re-run test without tee, in form as I have shown above?
> > UART is slow and printing something to console adds delay which decrease
> > probability that real issue is triggered as this is timing issue.
> 
> The test was done over SSH.

Ok! But it is still better to not print any results as it adds unwanted
delay between frequency switching.

> > Also please do tests just between two frequencies in loop as I observed
> > that switching between more decreased probability to hit issue.
> 
> > > > > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > while true; do
> > > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > > done
> 
> The first test ^ switched between 600 MHz and 1.2 GHz.
> 
> > The real issue for 1 GHz variant of A3720 is only when doing switch from
> > 500 MHz to 1 GHz. So could you try to do some tests also without
> > changing MIN_VOLT_MV_FOR_L0_L1_1GHZ and switching just between non-1.2
> > frequencies (to verify that on 1.2 GHz variant it is also from 600 MHz
> > to 1.2 GHz)?
> 
> With 1108 mV and switching between 600 MHz and 1.2GHz, I always saw a freeze within a minute.

I mean to try switching with 1.108 V between 200 MHz and 300 MHz or
between 300 MHz and 600 MHz. To check that issue is really only with
switch from 600 MHz to 1.2 GHz.

I need to know if current settings are fine for 200, 300 and 600 MHz
frequencies and the only 600 --> 1200 needs to be fixed.

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

On Thu, Feb 11, 2021, at 11:55 AM, Pali Rohár wrote:
> On Wednesday 10 February 2021 11:08:59 nnet wrote:
> > On Wed, Feb 10, 2021, at 10:03 AM, Pali Rohár wrote:
> > > > > Hello! Could you please enable userspace governor during kernel
> > > > > compilation?
> > > > > 
> > > > >     CONFIG_CPU_FREQ_GOV_USERSPACE=y
> > > > > 
> > > > > It can be activated via command:
> > > > > 
> > > > >     echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > > 
> > > > > After that you can "force" CPU frequency to specific value, e.g.:
> > > > > 
> > > > >     echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed
> > > > > 
> > > > > I need to know which switch (from --> to freq) cause this system hang.
> > > > > 
> > > > > This patch series (via MIN_VOLT_MV_FOR_L0_L1_1GHZ) is fixing only
> > > > > switching from 500 MHz to 1000 MHz on 1 GHz variant. As only this switch
> > > > > is causing issue.
> > > > > 
> > > > > I have used following simple bash script to check that switching between
> > > > > 500 MHz and 1 GHz is stable:
> > > > > 
> > > > >     while true; do
> > > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > >     done
> > > > 
> > > > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > while true; do
> > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > done
> > > > 
> > > > >> +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > > > 
> > > > With 1108 I get a freeze within a minute. The last output to stdout is 600000.
> > > > 
> > > > With 1120 it takes a few minutes.
> > > > 
> > > > With any of 1225, 1155, 1132 the device doesn't freeze over the full 5 minute load test.
> > > > 
> > > > I'm using ondemand now with the above at 1132 without issue so far.
> > > 
> > > Great, thank you for testing!
> > > 
> > > Can you check if switching between any two lower frequencies 200000
> > > 300000 600000 is stable?
> > 
> > This is stable using 1132 mV for MIN_VOLT_MV_FOR_L0_L1_1GHZ:
> > 
> > while true; do
> >   # down
> >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> ...
> 
> Hello!
> 
> Could you please re-run test without tee, in form as I have shown above?
> UART is slow and printing something to console adds delay which decrease
> probability that real issue is triggered as this is timing issue.

The test was done over SSH.

> Also please do tests just between two frequencies in loop as I observed
> that switching between more decreased probability to hit issue.

> > > > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > while true; do
> > > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > > done

The first test ^ switched between 600 MHz and 1.2 GHz.

> The real issue for 1 GHz variant of A3720 is only when doing switch from
> 500 MHz to 1 GHz. So could you try to do some tests also without
> changing MIN_VOLT_MV_FOR_L0_L1_1GHZ and switching just between non-1.2
> frequencies (to verify that on 1.2 GHz variant it is also from 600 MHz
> to 1.2 GHz)?

With 1108 mV and switching between 600 MHz and 1.2GHz, I always saw a freeze within a minute.

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

On Wednesday 10 February 2021 11:08:59 nnet wrote:
> On Wed, Feb 10, 2021, at 10:03 AM, Pali Rohár wrote:
> > > > Hello! Could you please enable userspace governor during kernel
> > > > compilation?
> > > > 
> > > >     CONFIG_CPU_FREQ_GOV_USERSPACE=y
> > > > 
> > > > It can be activated via command:
> > > > 
> > > >     echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > > 
> > > > After that you can "force" CPU frequency to specific value, e.g.:
> > > > 
> > > >     echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed
> > > > 
> > > > I need to know which switch (from --> to freq) cause this system hang.
> > > > 
> > > > This patch series (via MIN_VOLT_MV_FOR_L0_L1_1GHZ) is fixing only
> > > > switching from 500 MHz to 1000 MHz on 1 GHz variant. As only this switch
> > > > is causing issue.
> > > > 
> > > > I have used following simple bash script to check that switching between
> > > > 500 MHz and 1 GHz is stable:
> > > > 
> > > >     while true; do
> > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > >     done
> > > 
> > > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > while true; do
> > >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > > done
> > > 
> > > >> +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > > 
> > > With 1108 I get a freeze within a minute. The last output to stdout is 600000.
> > > 
> > > With 1120 it takes a few minutes.
> > > 
> > > With any of 1225, 1155, 1132 the device doesn't freeze over the full 5 minute load test.
> > > 
> > > I'm using ondemand now with the above at 1132 without issue so far.
> > 
> > Great, thank you for testing!
> > 
> > Can you check if switching between any two lower frequencies 200000
> > 300000 600000 is stable?
> 
> This is stable using 1132 mV for MIN_VOLT_MV_FOR_L0_L1_1GHZ:
> 
> while true; do
>   # down
>   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
...

Hello!

Could you please re-run test without tee, in form as I have shown above?
UART is slow and printing something to console adds delay which decrease
probability that real issue is triggered as this is timing issue.

Also please do tests just between two frequencies in loop as I observed
that switching between more decreased probability to hit issue.

The real issue for 1 GHz variant of A3720 is only when doing switch from
500 MHz to 1 GHz. So could you try to do some tests also without
changing MIN_VOLT_MV_FOR_L0_L1_1GHZ and switching just between non-1.2
frequencies (to verify that on 1.2 GHz variant it is also from 600 MHz
to 1.2 GHz)?

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Marek Behún]

On Wed, 10 Feb 2021 11:08:59 -0800
nnet <nnet@fastmail.fm> wrote:

> => md d0012604 1; md d0012604 1; md d0012604 1  
> d0012604: 2b417501                               .uA+
> d0012604: 0000945b                               [...
> d0012604: 00000000                               ....

So this means that in OTP you have this values:
1200 MHz - 1213 mV
1000 MHz - 1213 mV
 800 MHz - 1097 mV
 600 MHz -  898 mV

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

On Wed, Feb 10, 2021, at 10:03 AM, Pali Rohár wrote:
> On Wednesday 10 February 2021 09:34:07 nnet wrote:
> > On Wed, Feb 10, 2021, at 1:23 AM, Pali Rohár wrote:
> > > On Tuesday 09 February 2021 18:07:41 nnet wrote:
> > > > On Tue, Feb 9, 2021, at 5:51 PM, nnet wrote:
> > > > > On Tue, Feb 9, 2021, at 5:31 PM, nnet wrote:
> > > > > > On Tue, Feb 9, 2021, at 3:26 PM, Marek Behún wrote:
> > > > > > > On Tue, 09 Feb 2021 15:16:45 -0800
> > > > > > > nnet <nnet@fastmail.fm> wrote:
> > > > > > > 
> > > > > > > > I've two of these and I've just swapped them (and re-pasted the heat sinks).
> > > > > > > > 
> > > > > > > > The second one ran under load for awhile and now has frozen as well.
> > > > > > > > 
> > > > > > > > Under a moderate load `wget -O /dev/null <large.bin>` @X00Mbits they are fine.
> > > > > > > > 
> > > > > > > > Under a 1 min speed test of load ~200Mbits routed WireGuard they freeze.
> > > > > > > > 
> > > > > > > > They fine with both those workloads @1000_800.
> > > > > > > > 
> > > > > > > > Perhaps it's heat? Unfortunately I don't have any numbers on that ATM.
> > > > > > > 
> > > > > > > Try disabling cpufreq in kernel completely, compile boot image at
> > > > > > > 1200 MHz. If it continues freezing, then I fear we can't help you with
> > > > > > > 1200 MHz :(
> > > > > > 
> > > > > > cat /sys/devices/system/cpu/cpufreq/policy0/scaling_available_frequencies 
> > > > > > 200000 300000 600000 1200000 
> > > > > > 
> > > > > > I'm not getting any freezes with 1.2GHz fixed after 20 minutes of load:
> > > > > > 
> > > > > > echo 1200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq
> > > > > > 
> > > > > > Setting it back to min 200MHz I get a freeze within a minute:
> > > > > > 
> > > > > > echo 200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq
> > > 
> > > Hello! Could you please enable userspace governor during kernel
> > > compilation?
> > > 
> > >     CONFIG_CPU_FREQ_GOV_USERSPACE=y
> > > 
> > > It can be activated via command:
> > > 
> > >     echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > > 
> > > After that you can "force" CPU frequency to specific value, e.g.:
> > > 
> > >     echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed
> > > 
> > > I need to know which switch (from --> to freq) cause this system hang.
> > > 
> > > This patch series (via MIN_VOLT_MV_FOR_L0_L1_1GHZ) is fixing only
> > > switching from 500 MHz to 1000 MHz on 1 GHz variant. As only this switch
> > > is causing issue.
> > > 
> > > I have used following simple bash script to check that switching between
> > > 500 MHz and 1 GHz is stable:
> > > 
> > >     while true; do
> > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > >     done
> > 
> > echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > while true; do
> >   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> >   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> > done
> > 
> > >> +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > 
> > With 1108 I get a freeze within a minute. The last output to stdout is 600000.
> > 
> > With 1120 it takes a few minutes.
> > 
> > With any of 1225, 1155, 1132 the device doesn't freeze over the full 5 minute load test.
> > 
> > I'm using ondemand now with the above at 1132 without issue so far.
> 
> Great, thank you for testing!
> 
> Can you check if switching between any two lower frequencies 200000
> 300000 600000 is stable?

This is stable using 1132 mV for MIN_VOLT_MV_FOR_L0_L1_1GHZ:

while true; do
  # down
  echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
  echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
  echo 300000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
  echo 200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
  # up
  echo 300000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
  echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
  echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
  # all down
  echo 200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
  # all up
  echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
  # almost down
  echo 300000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
done    

> > >> Update the CPU voltage value for loads L0 and L1 accordingly when base
> > >> frequency is 1000 or 1200 MHz. The minimal value is updated from the
> > >> original 1.05V to 1.108V.
> > 
> > Perhaps similiar to how a minimum of 1108 mV is useful when shifting to 1GHz, using a minimum of 1132 is useful when shifting to 1.2GHz.
> 
> Maybe... We can only wait if Marvell provide some details about this
> issue.
> 
> > > (of course on 1.2 GHz variant you need to adjust values as only
> > > following frequencies 200000 300000 600000 1200000 are supported)
> > > 
> > > > > > > Marek
> > > > > > >
> > > > > 
> > > > > > +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > > > > 
> > > > > Based on the below at boot time might an equivalent of the above need 
> > > > > to be 1225 for 1.2GHz?
> > > > > 
> > > > > 1200_750
> > > > > SVC REV: 5, CPU VDD voltage: 1.225V
> > > > > 
> > > > > 1000_800
> > > > > SVC REV: 5, CPU VDD voltage: 1.108V
> > > 
> > > This value is printed in WTMI avs.c by following code:
> > > 
> > >     shift = OTP_SVC_SPEED_1000_OFF;
> > >     (OR)
> > >     shift = OTP_SVC_SPEED_1200_OFF;
> > > 
> > >     vdd_otp = ((otp_data[OTP_DATA_SVC_SPEED_ID] >> shift) +
> > >                 AVS_VDD_BASE) & AVS_VDD_MASK;
> > >     regval |= (vdd_otp << HIGH_VDD_LIMIT_OFF);
> > >     regval |= (vdd_otp << LOW_VDD_LIMIT_OFF);
> > >     printf("SVC REV: %d, CPU VDD voltage: %s\n", svc_rev,
> > >             avis_dump[vdd_otp].desc);
> > > 
> > > So voltage value is read from the OTP memory.
> > > 
> > > But I do not know what this value means.
> 
> Link to code: 
> https://github.com/MarvellEmbeddedProcessors/A3700-utils-marvell/blob/master/wtmi/sys_init/avs.c
> 
> You can read this OTP memory (otp_data[] array) in U-Boot by command:
> 
>     md d0012604 1; md d0012604 1; md d0012604 1

=> md d0012604 1; md d0012604 1; md d0012604 1
d0012604: 2b417501                               .uA+
d0012604: 0000945b                               [...
d0012604: 00000000                               ....

> > > > I did this for a quick test for 1.2GHz:
> > > > 
> > > > +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1225
> > > > 
> > > > This is working well so far. Frequency is shifting up/down with load applied/stopped.
> > >
>

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

On Wednesday 10 February 2021 09:34:07 nnet wrote:
> On Wed, Feb 10, 2021, at 1:23 AM, Pali Rohár wrote:
> > On Tuesday 09 February 2021 18:07:41 nnet wrote:
> > > On Tue, Feb 9, 2021, at 5:51 PM, nnet wrote:
> > > > On Tue, Feb 9, 2021, at 5:31 PM, nnet wrote:
> > > > > On Tue, Feb 9, 2021, at 3:26 PM, Marek Behún wrote:
> > > > > > On Tue, 09 Feb 2021 15:16:45 -0800
> > > > > > nnet <nnet@fastmail.fm> wrote:
> > > > > > 
> > > > > > > I've two of these and I've just swapped them (and re-pasted the heat sinks).
> > > > > > > 
> > > > > > > The second one ran under load for awhile and now has frozen as well.
> > > > > > > 
> > > > > > > Under a moderate load `wget -O /dev/null <large.bin>` @X00Mbits they are fine.
> > > > > > > 
> > > > > > > Under a 1 min speed test of load ~200Mbits routed WireGuard they freeze.
> > > > > > > 
> > > > > > > They fine with both those workloads @1000_800.
> > > > > > > 
> > > > > > > Perhaps it's heat? Unfortunately I don't have any numbers on that ATM.
> > > > > > 
> > > > > > Try disabling cpufreq in kernel completely, compile boot image at
> > > > > > 1200 MHz. If it continues freezing, then I fear we can't help you with
> > > > > > 1200 MHz :(
> > > > > 
> > > > > cat /sys/devices/system/cpu/cpufreq/policy0/scaling_available_frequencies 
> > > > > 200000 300000 600000 1200000 
> > > > > 
> > > > > I'm not getting any freezes with 1.2GHz fixed after 20 minutes of load:
> > > > > 
> > > > > echo 1200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq
> > > > > 
> > > > > Setting it back to min 200MHz I get a freeze within a minute:
> > > > > 
> > > > > echo 200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq
> > 
> > Hello! Could you please enable userspace governor during kernel
> > compilation?
> > 
> >     CONFIG_CPU_FREQ_GOV_USERSPACE=y
> > 
> > It can be activated via command:
> > 
> >     echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> > 
> > After that you can "force" CPU frequency to specific value, e.g.:
> > 
> >     echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed
> > 
> > I need to know which switch (from --> to freq) cause this system hang.
> > 
> > This patch series (via MIN_VOLT_MV_FOR_L0_L1_1GHZ) is fixing only
> > switching from 500 MHz to 1000 MHz on 1 GHz variant. As only this switch
> > is causing issue.
> > 
> > I have used following simple bash script to check that switching between
> > 500 MHz and 1 GHz is stable:
> > 
> >     while true; do
> >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> >         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> >         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> >     done
> 
> echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> while true; do
>   echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
>   echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
> done
> 
> >> +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> 
> With 1108 I get a freeze within a minute. The last output to stdout is 600000.
> 
> With 1120 it takes a few minutes.
> 
> With any of 1225, 1155, 1132 the device doesn't freeze over the full 5 minute load test.
> 
> I'm using ondemand now with the above at 1132 without issue so far.

Great, thank you for testing!

Can you check if switching between any two lower frequencies 200000
300000 600000 is stable?

> >> Update the CPU voltage value for loads L0 and L1 accordingly when base
> >> frequency is 1000 or 1200 MHz. The minimal value is updated from the
> >> original 1.05V to 1.108V.
> 
> Perhaps similiar to how a minimum of 1108 mV is useful when shifting to 1GHz, using a minimum of 1132 is useful when shifting to 1.2GHz.

Maybe... We can only wait if Marvell provide some details about this
issue.

> > (of course on 1.2 GHz variant you need to adjust values as only
> > following frequencies 200000 300000 600000 1200000 are supported)
> > 
> > > > > > Marek
> > > > > >
> > > > 
> > > > > +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > > > 
> > > > Based on the below at boot time might an equivalent of the above need 
> > > > to be 1225 for 1.2GHz?
> > > > 
> > > > 1200_750
> > > > SVC REV: 5, CPU VDD voltage: 1.225V
> > > > 
> > > > 1000_800
> > > > SVC REV: 5, CPU VDD voltage: 1.108V
> > 
> > This value is printed in WTMI avs.c by following code:
> > 
> >     shift = OTP_SVC_SPEED_1000_OFF;
> >     (OR)
> >     shift = OTP_SVC_SPEED_1200_OFF;
> > 
> >     vdd_otp = ((otp_data[OTP_DATA_SVC_SPEED_ID] >> shift) +
> >                 AVS_VDD_BASE) & AVS_VDD_MASK;
> >     regval |= (vdd_otp << HIGH_VDD_LIMIT_OFF);
> >     regval |= (vdd_otp << LOW_VDD_LIMIT_OFF);
> >     printf("SVC REV: %d, CPU VDD voltage: %s\n", svc_rev,
> >             avis_dump[vdd_otp].desc);
> > 
> > So voltage value is read from the OTP memory.
> > 
> > But I do not know what this value means.

Link to code: https://github.com/MarvellEmbeddedProcessors/A3700-utils-marvell/blob/master/wtmi/sys_init/avs.c

You can read this OTP memory (otp_data[] array) in U-Boot by command:

    md d0012604 1; md d0012604 1; md d0012604 1

> > > I did this for a quick test for 1.2GHz:
> > > 
> > > +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1225
> > > 
> > > This is working well so far. Frequency is shifting up/down with load applied/stopped.
> >

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

On Wed, Feb 10, 2021, at 1:23 AM, Pali Rohár wrote:
> On Tuesday 09 February 2021 18:07:41 nnet wrote:
> > On Tue, Feb 9, 2021, at 5:51 PM, nnet wrote:
> > > On Tue, Feb 9, 2021, at 5:31 PM, nnet wrote:
> > > > On Tue, Feb 9, 2021, at 3:26 PM, Marek Behún wrote:
> > > > > On Tue, 09 Feb 2021 15:16:45 -0800
> > > > > nnet <nnet@fastmail.fm> wrote:
> > > > > 
> > > > > > I've two of these and I've just swapped them (and re-pasted the heat sinks).
> > > > > > 
> > > > > > The second one ran under load for awhile and now has frozen as well.
> > > > > > 
> > > > > > Under a moderate load `wget -O /dev/null <large.bin>` @X00Mbits they are fine.
> > > > > > 
> > > > > > Under a 1 min speed test of load ~200Mbits routed WireGuard they freeze.
> > > > > > 
> > > > > > They fine with both those workloads @1000_800.
> > > > > > 
> > > > > > Perhaps it's heat? Unfortunately I don't have any numbers on that ATM.
> > > > > 
> > > > > Try disabling cpufreq in kernel completely, compile boot image at
> > > > > 1200 MHz. If it continues freezing, then I fear we can't help you with
> > > > > 1200 MHz :(
> > > > 
> > > > cat /sys/devices/system/cpu/cpufreq/policy0/scaling_available_frequencies 
> > > > 200000 300000 600000 1200000 
> > > > 
> > > > I'm not getting any freezes with 1.2GHz fixed after 20 minutes of load:
> > > > 
> > > > echo 1200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq
> > > > 
> > > > Setting it back to min 200MHz I get a freeze within a minute:
> > > > 
> > > > echo 200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq
> 
> Hello! Could you please enable userspace governor during kernel
> compilation?
> 
>     CONFIG_CPU_FREQ_GOV_USERSPACE=y
> 
> It can be activated via command:
> 
>     echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
> 
> After that you can "force" CPU frequency to specific value, e.g.:
> 
>     echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed
> 
> I need to know which switch (from --> to freq) cause this system hang.
> 
> This patch series (via MIN_VOLT_MV_FOR_L0_L1_1GHZ) is fixing only
> switching from 500 MHz to 1000 MHz on 1 GHz variant. As only this switch
> is causing issue.
> 
> I have used following simple bash script to check that switching between
> 500 MHz and 1 GHz is stable:
> 
>     while true; do
>         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
>         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
>         echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
>         echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
>     done

echo userspace | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_governor
while true; do
  echo 1200000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
  echo 600000 | tee /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
done

>> +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108

With 1108 I get a freeze within a minute. The last output to stdout is 600000.

With 1120 it takes a few minutes.

With any of 1225, 1155, 1132 the device doesn't freeze over the full 5 minute load test.

I'm using ondemand now with the above at 1132 without issue so far.

>> Update the CPU voltage value for loads L0 and L1 accordingly when base
>> frequency is 1000 or 1200 MHz. The minimal value is updated from the
>> original 1.05V to 1.108V.

Perhaps similiar to how a minimum of 1108 mV is useful when shifting to 1GHz, using a minimum of 1132 is useful when shifting to 1.2GHz.

> (of course on 1.2 GHz variant you need to adjust values as only
> following frequencies 200000 300000 600000 1200000 are supported)
> 
> > > > > Marek
> > > > >
> > > 
> > > > +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > > 
> > > Based on the below at boot time might an equivalent of the above need 
> > > to be 1225 for 1.2GHz?
> > > 
> > > 1200_750
> > > SVC REV: 5, CPU VDD voltage: 1.225V
> > > 
> > > 1000_800
> > > SVC REV: 5, CPU VDD voltage: 1.108V
> 
> This value is printed in WTMI avs.c by following code:
> 
>     shift = OTP_SVC_SPEED_1000_OFF;
>     (OR)
>     shift = OTP_SVC_SPEED_1200_OFF;
> 
>     vdd_otp = ((otp_data[OTP_DATA_SVC_SPEED_ID] >> shift) +
>                 AVS_VDD_BASE) & AVS_VDD_MASK;
>     regval |= (vdd_otp << HIGH_VDD_LIMIT_OFF);
>     regval |= (vdd_otp << LOW_VDD_LIMIT_OFF);
>     printf("SVC REV: %d, CPU VDD voltage: %s\n", svc_rev,
>             avis_dump[vdd_otp].desc);
> 
> So voltage value is read from the OTP memory.
> 
> But I do not know what this value means.
> 
> > I did this for a quick test for 1.2GHz:
> > 
> > +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1225
> > 
> > This is working well so far. Frequency is shifting up/down with load applied/stopped.
>

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

On Tuesday 09 February 2021 18:07:41 nnet wrote:
> On Tue, Feb 9, 2021, at 5:51 PM, nnet wrote:
> > On Tue, Feb 9, 2021, at 5:31 PM, nnet wrote:
> > > On Tue, Feb 9, 2021, at 3:26 PM, Marek Behún wrote:
> > > > On Tue, 09 Feb 2021 15:16:45 -0800
> > > > nnet <nnet@fastmail.fm> wrote:
> > > > 
> > > > > I've two of these and I've just swapped them (and re-pasted the heat sinks).
> > > > > 
> > > > > The second one ran under load for awhile and now has frozen as well.
> > > > > 
> > > > > Under a moderate load `wget -O /dev/null <large.bin>` @X00Mbits they are fine.
> > > > > 
> > > > > Under a 1 min speed test of load ~200Mbits routed WireGuard they freeze.
> > > > > 
> > > > > They fine with both those workloads @1000_800.
> > > > > 
> > > > > Perhaps it's heat? Unfortunately I don't have any numbers on that ATM.
> > > > 
> > > > Try disabling cpufreq in kernel completely, compile boot image at
> > > > 1200 MHz. If it continues freezing, then I fear we can't help you with
> > > > 1200 MHz :(
> > > 
> > > cat /sys/devices/system/cpu/cpufreq/policy0/scaling_available_frequencies 
> > > 200000 300000 600000 1200000 
> > > 
> > > I'm not getting any freezes with 1.2GHz fixed after 20 minutes of load:
> > > 
> > > echo 1200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq
> > > 
> > > Setting it back to min 200MHz I get a freeze within a minute:
> > > 
> > > echo 200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq

Hello! Could you please enable userspace governor during kernel
compilation?

    CONFIG_CPU_FREQ_GOV_USERSPACE=y

It can be activated via command:

    echo userspace > /sys/devices/system/cpu/cpufreq/policy0/scaling_governor

After that you can "force" CPU frequency to specific value, e.g.:

    echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed

I need to know which switch (from --> to freq) cause this system hang.

This patch series (via MIN_VOLT_MV_FOR_L0_L1_1GHZ) is fixing only
switching from 500 MHz to 1000 MHz on 1 GHz variant. As only this switch
is causing issue.

I have used following simple bash script to check that switching between
500 MHz and 1 GHz is stable:

    while true; do
        echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
        echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
        echo 1000000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
        echo 500000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_setspeed;
    done

(of course on 1.2 GHz variant you need to adjust values as only
following frequencies 200000 300000 600000 1200000 are supported)

> > > > Marek
> > > >
> > 
> > > +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> > 
> > Based on the below at boot time might an equivalent of the above need 
> > to be 1225 for 1.2GHz?
> > 
> > 1200_750
> > SVC REV: 5, CPU VDD voltage: 1.225V
> > 
> > 1000_800
> > SVC REV: 5, CPU VDD voltage: 1.108V

This value is printed in WTMI avs.c by following code:

    shift = OTP_SVC_SPEED_1000_OFF;
    (OR)
    shift = OTP_SVC_SPEED_1200_OFF;

    vdd_otp = ((otp_data[OTP_DATA_SVC_SPEED_ID] >> shift) +
                AVS_VDD_BASE) & AVS_VDD_MASK;
    regval |= (vdd_otp << HIGH_VDD_LIMIT_OFF);
    regval |= (vdd_otp << LOW_VDD_LIMIT_OFF);
    printf("SVC REV: %d, CPU VDD voltage: %s\n", svc_rev,
            avis_dump[vdd_otp].desc);

So voltage value is read from the OTP memory.

But I do not know what this value means.

> I did this for a quick test for 1.2GHz:
> 
> +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1225
> 
> This is working well so far. Frequency is shifting up/down with load applied/stopped.

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Marek Behún]

On Tue, 09 Feb 2021 17:51:53 -0800
nnet <nnet@fastmail.fm> wrote:

> On Tue, Feb 9, 2021, at 5:31 PM, nnet wrote:
> > On Tue, Feb 9, 2021, at 3:26 PM, Marek Behún wrote:  
> > > On Tue, 09 Feb 2021 15:16:45 -0800
> > > nnet <nnet@fastmail.fm> wrote:
> > >   
> > > > I've two of these and I've just swapped them (and re-pasted the heat sinks).
> > > > 
> > > > The second one ran under load for awhile and now has frozen as well.
> > > > 
> > > > Under a moderate load `wget -O /dev/null <large.bin>` @X00Mbits they are fine.
> > > > 
> > > > Under a 1 min speed test of load ~200Mbits routed WireGuard they freeze.
> > > > 
> > > > They fine with both those workloads @1000_800.
> > > > 
> > > > Perhaps it's heat? Unfortunately I don't have any numbers on that ATM.  
> > > 
> > > Try disabling cpufreq in kernel completely, compile boot image at
> > > 1200 MHz. If it continues freezing, then I fear we can't help you with
> > > 1200 MHz :(  
> > 
> > cat /sys/devices/system/cpu/cpufreq/policy0/scaling_available_frequencies 
> > 200000 300000 600000 1200000 
> > 
> > I'm not getting any freezes with 1.2GHz fixed after 20 minutes of load:
> > 
> > echo 1200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq
> > 
> > Setting it back to min 200MHz I get a freeze within a minute:
> > 
> > echo 200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq
> >   
> > > Marek
> > >  
> 
> > +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108  
> 
> Based on the below at boot time might an equivalent of the above need to be 1225 for 1.2GHz?
> 
> 1200_750
> SVC REV: 5, CPU VDD voltage: 1.225V
> 
> 1000_800
> SVC REV: 5, CPU VDD voltage: 1.108V

It may be... But it is possible that the value 1.225 is computed by the
code by default.

Marek

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

On Tue, Feb 9, 2021, at 5:51 PM, nnet wrote:
> On Tue, Feb 9, 2021, at 5:31 PM, nnet wrote:
> > On Tue, Feb 9, 2021, at 3:26 PM, Marek Behún wrote:
> > > On Tue, 09 Feb 2021 15:16:45 -0800
> > > nnet <nnet@fastmail.fm> wrote:
> > > 
> > > > I've two of these and I've just swapped them (and re-pasted the heat sinks).
> > > > 
> > > > The second one ran under load for awhile and now has frozen as well.
> > > > 
> > > > Under a moderate load `wget -O /dev/null <large.bin>` @X00Mbits they are fine.
> > > > 
> > > > Under a 1 min speed test of load ~200Mbits routed WireGuard they freeze.
> > > > 
> > > > They fine with both those workloads @1000_800.
> > > > 
> > > > Perhaps it's heat? Unfortunately I don't have any numbers on that ATM.
> > > 
> > > Try disabling cpufreq in kernel completely, compile boot image at
> > > 1200 MHz. If it continues freezing, then I fear we can't help you with
> > > 1200 MHz :(
> > 
> > cat /sys/devices/system/cpu/cpufreq/policy0/scaling_available_frequencies 
> > 200000 300000 600000 1200000 
> > 
> > I'm not getting any freezes with 1.2GHz fixed after 20 minutes of load:
> > 
> > echo 1200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq
> > 
> > Setting it back to min 200MHz I get a freeze within a minute:
> > 
> > echo 200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq
> > 
> > > Marek
> > >
> 
> > +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108
> 
> Based on the below at boot time might an equivalent of the above need 
> to be 1225 for 1.2GHz?
> 
> 1200_750
> SVC REV: 5, CPU VDD voltage: 1.225V
> 
> 1000_800
> SVC REV: 5, CPU VDD voltage: 1.108V

I did this for a quick test for 1.2GHz:

+#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1225

This is working well so far. Frequency is shifting up/down with load applied/stopped.

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

On Tue, Feb 9, 2021, at 5:31 PM, nnet wrote:
> On Tue, Feb 9, 2021, at 3:26 PM, Marek Behún wrote:
> > On Tue, 09 Feb 2021 15:16:45 -0800
> > nnet <nnet@fastmail.fm> wrote:
> > 
> > > I've two of these and I've just swapped them (and re-pasted the heat sinks).
> > > 
> > > The second one ran under load for awhile and now has frozen as well.
> > > 
> > > Under a moderate load `wget -O /dev/null <large.bin>` @X00Mbits they are fine.
> > > 
> > > Under a 1 min speed test of load ~200Mbits routed WireGuard they freeze.
> > > 
> > > They fine with both those workloads @1000_800.
> > > 
> > > Perhaps it's heat? Unfortunately I don't have any numbers on that ATM.
> > 
> > Try disabling cpufreq in kernel completely, compile boot image at
> > 1200 MHz. If it continues freezing, then I fear we can't help you with
> > 1200 MHz :(
> 
> cat /sys/devices/system/cpu/cpufreq/policy0/scaling_available_frequencies 
> 200000 300000 600000 1200000 
> 
> I'm not getting any freezes with 1.2GHz fixed after 20 minutes of load:
> 
> echo 1200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq
> 
> Setting it back to min 200MHz I get a freeze within a minute:
> 
> echo 200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq
> 
> > Marek
> >

> +#define MIN_VOLT_MV_FOR_L0_L1_1GHZ 1108

Based on the below at boot time might an equivalent of the above need to be 1225 for 1.2GHz?

1200_750
SVC REV: 5, CPU VDD voltage: 1.225V

1000_800
SVC REV: 5, CPU VDD voltage: 1.108V

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

On Tue, Feb 9, 2021, at 3:26 PM, Marek Behún wrote:
> On Tue, 09 Feb 2021 15:16:45 -0800
> nnet <nnet@fastmail.fm> wrote:
> 
> > I've two of these and I've just swapped them (and re-pasted the heat sinks).
> > 
> > The second one ran under load for awhile and now has frozen as well.
> > 
> > Under a moderate load `wget -O /dev/null <large.bin>` @X00Mbits they are fine.
> > 
> > Under a 1 min speed test of load ~200Mbits routed WireGuard they freeze.
> > 
> > They fine with both those workloads @1000_800.
> > 
> > Perhaps it's heat? Unfortunately I don't have any numbers on that ATM.
> 
> Try disabling cpufreq in kernel completely, compile boot image at
> 1200 MHz. If it continues freezing, then I fear we can't help you with
> 1200 MHz :(

cat /sys/devices/system/cpu/cpufreq/policy0/scaling_available_frequencies 
200000 300000 600000 1200000 

I'm not getting any freezes with 1.2GHz fixed after 20 minutes of load:

echo 1200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq

Setting it back to min 200MHz I get a freeze within a minute:

echo 200000 > /sys/devices/system/cpu/cpufreq/policy0/scaling_min_freq

> Marek
>

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Marek Behún]

On Tue, 09 Feb 2021 15:16:45 -0800
nnet <nnet@fastmail.fm> wrote:

> I've two of these and I've just swapped them (and re-pasted the heat sinks).
> 
> The second one ran under load for awhile and now has frozen as well.
> 
> Under a moderate load `wget -O /dev/null <large.bin>` @X00Mbits they are fine.
> 
> Under a 1 min speed test of load ~200Mbits routed WireGuard they freeze.
> 
> They fine with both those workloads @1000_800.
> 
> Perhaps it's heat? Unfortunately I don't have any numbers on that ATM.

Try disabling cpufreq in kernel completely, compile boot image at
1200 MHz. If it continues freezing, then I fear we can't help you with
1200 MHz :(

Marek

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

I've two of these and I've just swapped them (and re-pasted the heat sinks).

The second one ran under load for awhile and now has frozen as well.

Under a moderate load `wget -O /dev/null <large.bin>` @X00Mbits they are fine.

Under a 1 min speed test of load ~200Mbits routed WireGuard they freeze.

They fine with both those workloads @1000_800.

Perhaps it's heat? Unfortunately I don't have any numbers on that ATM.

On Tue, Feb 9, 2021, at 2:56 PM, Pali Rohár wrote:
> On Tuesday 09 February 2021 14:52:56 nnet wrote:
> > On Tue, Feb 9, 2021, at 2:42 PM, Pali Rohár wrote:
> > > On Tuesday 09 February 2021 13:45:08 nnet wrote:
> > > > On Tue, Feb 9, 2021, at 1:33 PM, Pali Rohár wrote:
> > > > > On Tuesday 09 February 2021 13:00:26 nnet wrote:
> > > > > > > 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.
> > > > > > 
> > > > > > These patches applied to the 5.4.96 in OpenWrt (98d61b5) work fine so far on an Espressobin v7 AFAICT per changing values in /sys/devices/system/cpu/cpufreq/policy0.
> > > > > > 
> > > > > > Are these changes intended to work @1.2 GHz on the v7?
> > > > > 
> > > > > Hello! Do you have 1.2 GHz A3720 SoC?
> > > > 
> > > > Maybe (not)? ESPRESSObin_V7_1-0 on the underside.
> > > 
> > > Look at the package of SoC chip. On top of the package is printed
> > > identifier 88F3720. On the last line should be one of the string:
> > > C080, C100, C120, I080, I100 which identifies frequency
> > > (080 = 800 MHz, 100 = 1 GHz, 120 = 1.2 GHz)
> > > 
> > > Can you check what is printed on A3720 SoC package?
> > 
> > Both of mine are 88F3720-A0-C120.
> 
> Interesting... it is really 1.2 GHz. I have not seen it yet. And few
> weeks ago I thought that it was never available on market.
> 
> Well, if 1.2 GHz variant has same issues as 1 GHz variants then this
> patch series should fix it. But you said that with this patch series is
> board crashing when running at 1.2 GHz?
>

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

On Tuesday 09 February 2021 14:52:56 nnet wrote:
> On Tue, Feb 9, 2021, at 2:42 PM, Pali Rohár wrote:
> > On Tuesday 09 February 2021 13:45:08 nnet wrote:
> > > On Tue, Feb 9, 2021, at 1:33 PM, Pali Rohár wrote:
> > > > On Tuesday 09 February 2021 13:00:26 nnet wrote:
> > > > > > 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.
> > > > > 
> > > > > These patches applied to the 5.4.96 in OpenWrt (98d61b5) work fine so far on an Espressobin v7 AFAICT per changing values in /sys/devices/system/cpu/cpufreq/policy0.
> > > > > 
> > > > > Are these changes intended to work @1.2 GHz on the v7?
> > > > 
> > > > Hello! Do you have 1.2 GHz A3720 SoC?
> > > 
> > > Maybe (not)? ESPRESSObin_V7_1-0 on the underside.
> > 
> > Look at the package of SoC chip. On top of the package is printed
> > identifier 88F3720. On the last line should be one of the string:
> > C080, C100, C120, I080, I100 which identifies frequency
> > (080 = 800 MHz, 100 = 1 GHz, 120 = 1.2 GHz)
> > 
> > Can you check what is printed on A3720 SoC package?
> 
> Both of mine are 88F3720-A0-C120.

Interesting... it is really 1.2 GHz. I have not seen it yet. And few
weeks ago I thought that it was never available on market.

Well, if 1.2 GHz variant has same issues as 1 GHz variants then this
patch series should fix it. But you said that with this patch series is
board crashing when running at 1.2 GHz?

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

On Tue, Feb 9, 2021, at 2:42 PM, Pali Rohár wrote:
> On Tuesday 09 February 2021 13:45:08 nnet wrote:
> > On Tue, Feb 9, 2021, at 1:33 PM, Pali Rohár wrote:
> > > On Tuesday 09 February 2021 13:00:26 nnet wrote:
> > > > > 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.
> > > > 
> > > > These patches applied to the 5.4.96 in OpenWrt (98d61b5) work fine so far on an Espressobin v7 AFAICT per changing values in /sys/devices/system/cpu/cpufreq/policy0.
> > > > 
> > > > Are these changes intended to work @1.2 GHz on the v7?
> > > 
> > > Hello! Do you have 1.2 GHz A3720 SoC?
> > 
> > Maybe (not)? ESPRESSObin_V7_1-0 on the underside.
> 
> Look at the package of SoC chip. On top of the package is printed
> identifier 88F3720. On the last line should be one of the string:
> C080, C100, C120, I080, I100 which identifies frequency
> (080 = 800 MHz, 100 = 1 GHz, 120 = 1.2 GHz)
> 
> Can you check what is printed on A3720 SoC package?

Both of mine are 88F3720-A0-C120.

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

On Tuesday 09 February 2021 13:45:08 nnet wrote:
> On Tue, Feb 9, 2021, at 1:33 PM, Pali Rohár wrote:
> > On Tuesday 09 February 2021 13:00:26 nnet wrote:
> > > > 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.
> > > 
> > > These patches applied to the 5.4.96 in OpenWrt (98d61b5) work fine so far on an Espressobin v7 AFAICT per changing values in /sys/devices/system/cpu/cpufreq/policy0.
> > > 
> > > Are these changes intended to work @1.2 GHz on the v7?
> > 
> > Hello! Do you have 1.2 GHz A3720 SoC?
> 
> Maybe (not)? ESPRESSObin_V7_1-0 on the underside.

Look at the package of SoC chip. On top of the package is printed
identifier 88F3720. On the last line should be one of the string:
C080, C100, C120, I080, I100 which identifies frequency
(080 = 800 MHz, 100 = 1 GHz, 120 = 1.2 GHz)

Can you check what is printed on A3720 SoC package?

> BTW, with the 1200_750 firmware and the patches:
> 
> root@OpenWrt:/sys/devices/system/cpu/cpufreq/policy0# cat scaling_available_frequencies 
> 200000 300000 600000 1200000
> 
> Of course that could mean nothing, but thought I'd mention what I do see.

This is value set by firmware file which you have started.

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

On Tue, Feb 9, 2021, at 1:33 PM, Pali Rohár wrote:
> On Tuesday 09 February 2021 13:00:26 nnet wrote:
> > > 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.
> > 
> > These patches applied to the 5.4.96 in OpenWrt (98d61b5) work fine so far on an Espressobin v7 AFAICT per changing values in /sys/devices/system/cpu/cpufreq/policy0.
> > 
> > Are these changes intended to work @1.2 GHz on the v7?
> 
> Hello! Do you have 1.2 GHz A3720 SoC?

Maybe (not)? ESPRESSObin_V7_1-0 on the underside.

BTW, with the 1200_750 firmware and the patches:

root@OpenWrt:/sys/devices/system/cpu/cpufreq/policy0# cat scaling_available_frequencies 
200000 300000 600000 1200000

Of course that could mean nothing, but thought I'd mention what I do see.

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[Pali Rohár]

On Tuesday 09 February 2021 13:00:26 nnet wrote:
> > 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.
> 
> These patches applied to the 5.4.96 in OpenWrt (98d61b5) work fine so far on an Espressobin v7 AFAICT per changing values in /sys/devices/system/cpu/cpufreq/policy0.
> 
> Are these changes intended to work @1.2 GHz on the v7?

Hello! Do you have 1.2 GHz A3720 SoC?

Re: [PATCH mvebu v2 00/10] Armada 37xx: Fix cpufreq changing base CPU speed to 800 MHz from 1000 MHz

[nnet]

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

These patches applied to the 5.4.96 in OpenWrt (98d61b5) work fine so far on an Espressobin v7 AFAICT per changing values in /sys/devices/system/cpu/cpufreq/policy0.

Are these changes intended to work @1.2 GHz on the v7?

I have the beige case and heat-sinks with paste on the board, and the power adapter is the Fairway for 2.0A. 

However, the device hangs fairly quickly under load with firmware flashed of the built as below:

diff --git a/package/boot/arm-trusted-firmware-mvebu/Makefile b/package/boot/arm-trusted-firmware-mvebu/Makefile
index c4669edb52..20899c29e7 100644
--- a/package/boot/arm-trusted-firmware-mvebu/Makefile
+++ b/package/boot/arm-trusted-firmware-mvebu/Makefile
@@ -72,7 +72,7 @@ define Trusted-Firmware-A/espressobin-v7-1gb
   BUILD_DEVICES:=globalscale_espressobin-v7
   UBOOT:=espressobin
   DDR_TOPOLOGY:=5
-  CLOCKSPRESET:=CPU_1000_DDR_800
+  CLOCKSPRESET:=CPU_1200_DDR_750
   PLAT:=a3700
 endef

See: https://github.com/openwrt/openwrt/commit/5fae94d987f2261b148c035130a00350c905a13e