On Sat, Jul 24, 2021 at 6:31 PM Hauke Mehrtens <[email protected]> wrote: > > Hi Robert, > > Could you plase comment on this?
Hi, The linked discussion already contains all of the available information and a good discussion. Upstream still has no better solution than to blacklist CPUFreq for the 1.2GHz models. https://lists.openwrt.org/pipermail/openwrt-devel/2021-June/035702.html I don't really have anything more to add, I understand the purpose of this patch and cant really advocate against it as it causes regression for other users. Regards, Robert > > 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 <[email protected]> > > Signed-off-by: Josef Schlehofer <[email protected]> > > --- > > ...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 <[email protected]> > > -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 <[email protected]> > > ---- > > - 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 <[email protected]> > > -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 <[email protected]> > > ---- > > - 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; > > > -- Robert Marko Staff Embedded Linux Engineer Sartura Ltd. Lendavska ulica 16a 10000 Zagreb, Croatia Email: [email protected] Web: www.sartura.hr _______________________________________________ openwrt-devel mailing list [email protected] https://lists.openwrt.org/mailman/listinfo/openwrt-devel
