[PATCH] Revert "mvebu: 5.10 fix DVFS caused random boot crashes"

Hauke Mehrtens hauke at hauke-m.de
Sat Jul 24 09:31:29 PDT 2021


Hi Robert,

Could you plase comment on this?

Hauke

On 7/19/21 1:46 PM, Josef Schlehofer wrote:
> Based on the discussion on the mailing list [1], the patch which was
> reverted, it reverts only one patch without the subsequent ones.
> 
> This leads to the SoC scaling issue not using a CPU parent clock, but
> it uses DDR clock. This is done for all variants, and it's wrong because
> commits (hacks) that were using the DDR clock are no longer in the mainline kernel.
> 
> If someone has stability issues on 1.2 GHz, it should not affect all
> routers (1 GHz, 800 MHz) and it should be rather consulted with guys, who are trying to
> improve the situation in the kernel and not making the situation worse.
> 
> There are two solutions in cases of instability:
> a) disable cpufreq
> b) underclock it up to 1 GHz
> 
> This reverts commit 080a0b74e39d159eecf69c468debec42f28bf4d8.
> 
> [1] https://lists.openwrt.org/pipermail/openwrt-devel/2021-June/035702.html
> 
> CC: Pali Rohár <pali at kernel.org>
> Signed-off-by: Josef Schlehofer <pepe.schlehofer at gmail.com>
> ---
>   ...rmada-37xx-Fix-setting-TBG-parent-fo.patch | 107 ------------------
>   ...rmada-37xx-Fix-setting-TBG-parent-fo.patch | 107 ------------------
>   2 files changed, 214 deletions(-)
>   delete mode 100644 target/linux/mvebu/patches-5.10/803-Revert-cpufreq-armada-37xx-Fix-setting-TBG-parent-fo.patch
>   delete mode 100644 target/linux/mvebu/patches-5.4/803-Revert-cpufreq-armada-37xx-Fix-setting-TBG-parent-fo.patch
> 
> diff --git a/target/linux/mvebu/patches-5.10/803-Revert-cpufreq-armada-37xx-Fix-setting-TBG-parent-fo.patch b/target/linux/mvebu/patches-5.10/803-Revert-cpufreq-armada-37xx-Fix-setting-TBG-parent-fo.patch
> deleted file mode 100644
> index 65184df584..0000000000
> --- a/target/linux/mvebu/patches-5.10/803-Revert-cpufreq-armada-37xx-Fix-setting-TBG-parent-fo.patch
> +++ /dev/null
> @@ -1,107 +0,0 @@
> -From 35639bac13927d1476398b740b11cbed0ee3ddb2 Mon Sep 17 00:00:00 2001
> -From: Robert Marko <robert.marko at sartura.hr>
> -Date: Tue, 18 May 2021 13:24:30 +0200
> -Subject: [PATCH] Revert "cpufreq: armada-37xx: Fix setting TBG parent for load
> - levels"
> -
> -This reverts commit a13b110e7c9e0dc2edcc7a19d4255fc88abd83cc.
> -
> -This patch actually corrects the things so that 1 or 1.2GHz models would
> -actually get scaled to their native frequency.
> -
> -However, due to a AVS setting voltages too low this will cause random
> -crashes on 1.2GHz models.
> -
> -So, until a new safe for everybody voltage is agreed on
> -lets revert the patch.
> -
> -Signed-off-by: Robert Marko <robert.marko at sartura.hr>
> ----
> - drivers/cpufreq/armada-37xx-cpufreq.c | 35 +++++++++------------------
> - 1 file changed, 12 insertions(+), 23 deletions(-)
> -
> ---- a/drivers/cpufreq/armada-37xx-cpufreq.c
> -+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
> -@@ -25,10 +25,6 @@
> -
> - #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
> -@@ -126,15 +122,10 @@ static struct armada_37xx_dvfs *armada_3
> -  * will be configured then the DVFS will be enabled.
> -  */
> - static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base,
> --						 struct regmap *clk_base, u8 *divider)
> -+						 struct clk *clk, u8 *divider)
> - {
> --	u32 cpu_tbg_sel;
> - 	int load_lvl;
> --
> --	/* 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;
> -+	struct clk *parent;
> -
> - 	for (load_lvl = 0; load_lvl < LOAD_LEVEL_NR; load_lvl++) {
> - 		unsigned int reg, mask, val, offset = 0;
> -@@ -153,11 +144,6 @@ static void __init armada37xx_cpufreq_dv
> - 		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.
> -@@ -176,6 +162,14 @@ static void __init armada37xx_cpufreq_dv
> -
> - 		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);
> - }
> -
> - /*
> -@@ -401,16 +395,11 @@ static int __init armada37xx_cpufreq_dri
> - 	struct platform_device *pdev;
> - 	unsigned long freq;
> - 	unsigned int cur_frequency, base_frequency;
> --	struct regmap *nb_clk_base, *nb_pm_base, *avs_base;
> -+	struct regmap *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");
> -
> -@@ -487,7 +476,7 @@ static int __init armada37xx_cpufreq_dri
> - 	armada37xx_cpufreq_avs_configure(avs_base, dvfs);
> - 	armada37xx_cpufreq_avs_setup(avs_base, dvfs);
> -
> --	armada37xx_cpufreq_dvfs_setup(nb_pm_base, nb_clk_base, dvfs->divider);
> -+	armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
> - 	clk_put(clk);
> -
> - 	for (load_lvl = ARMADA_37XX_DVFS_LOAD_0; load_lvl < LOAD_LEVEL_NR;
> diff --git a/target/linux/mvebu/patches-5.4/803-Revert-cpufreq-armada-37xx-Fix-setting-TBG-parent-fo.patch b/target/linux/mvebu/patches-5.4/803-Revert-cpufreq-armada-37xx-Fix-setting-TBG-parent-fo.patch
> deleted file mode 100644
> index 65184df584..0000000000
> --- a/target/linux/mvebu/patches-5.4/803-Revert-cpufreq-armada-37xx-Fix-setting-TBG-parent-fo.patch
> +++ /dev/null
> @@ -1,107 +0,0 @@
> -From 35639bac13927d1476398b740b11cbed0ee3ddb2 Mon Sep 17 00:00:00 2001
> -From: Robert Marko <robert.marko at sartura.hr>
> -Date: Tue, 18 May 2021 13:24:30 +0200
> -Subject: [PATCH] Revert "cpufreq: armada-37xx: Fix setting TBG parent for load
> - levels"
> -
> -This reverts commit a13b110e7c9e0dc2edcc7a19d4255fc88abd83cc.
> -
> -This patch actually corrects the things so that 1 or 1.2GHz models would
> -actually get scaled to their native frequency.
> -
> -However, due to a AVS setting voltages too low this will cause random
> -crashes on 1.2GHz models.
> -
> -So, until a new safe for everybody voltage is agreed on
> -lets revert the patch.
> -
> -Signed-off-by: Robert Marko <robert.marko at sartura.hr>
> ----
> - drivers/cpufreq/armada-37xx-cpufreq.c | 35 +++++++++------------------
> - 1 file changed, 12 insertions(+), 23 deletions(-)
> -
> ---- a/drivers/cpufreq/armada-37xx-cpufreq.c
> -+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
> -@@ -25,10 +25,6 @@
> -
> - #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
> -@@ -126,15 +122,10 @@ static struct armada_37xx_dvfs *armada_3
> -  * will be configured then the DVFS will be enabled.
> -  */
> - static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base,
> --						 struct regmap *clk_base, u8 *divider)
> -+						 struct clk *clk, u8 *divider)
> - {
> --	u32 cpu_tbg_sel;
> - 	int load_lvl;
> --
> --	/* 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;
> -+	struct clk *parent;
> -
> - 	for (load_lvl = 0; load_lvl < LOAD_LEVEL_NR; load_lvl++) {
> - 		unsigned int reg, mask, val, offset = 0;
> -@@ -153,11 +144,6 @@ static void __init armada37xx_cpufreq_dv
> - 		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.
> -@@ -176,6 +162,14 @@ static void __init armada37xx_cpufreq_dv
> -
> - 		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);
> - }
> -
> - /*
> -@@ -401,16 +395,11 @@ static int __init armada37xx_cpufreq_dri
> - 	struct platform_device *pdev;
> - 	unsigned long freq;
> - 	unsigned int cur_frequency, base_frequency;
> --	struct regmap *nb_clk_base, *nb_pm_base, *avs_base;
> -+	struct regmap *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");
> -
> -@@ -487,7 +476,7 @@ static int __init armada37xx_cpufreq_dri
> - 	armada37xx_cpufreq_avs_configure(avs_base, dvfs);
> - 	armada37xx_cpufreq_avs_setup(avs_base, dvfs);
> -
> --	armada37xx_cpufreq_dvfs_setup(nb_pm_base, nb_clk_base, dvfs->divider);
> -+	armada37xx_cpufreq_dvfs_setup(nb_pm_base, clk, dvfs->divider);
> - 	clk_put(clk);
> -
> - 	for (load_lvl = ARMADA_37XX_DVFS_LOAD_0; load_lvl < LOAD_LEVEL_NR;
> 




More information about the openwrt-devel mailing list