From: Christian Neubert christian.neubert.86@gmail.com
The clock parenting was not setup properly when DVFS was enabled. It was expected that the same clock source was used with and without DVFS which was not the case.
This patch fixes this issue, allowing to make the cpufreq support work when the CPU clock source are not the default ones.
Fixes: 92ce45fb875d ("cpufreq: Add DVFS support for Armada 37xx") Cc: stable@vger.kernel.org [gregory: extract from a larger patch, modify comments and commit log] Signed-off-by: Christian Neubert christian.neubert.86@gmail.com Signed-off-by: Gregory CLEMENT gregory.clement@bootlin.com --- drivers/cpufreq/armada-37xx-cpufreq.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c index 75491fc841a6..ad4463e4266e 100644 --- a/drivers/cpufreq/armada-37xx-cpufreq.c +++ b/drivers/cpufreq/armada-37xx-cpufreq.c @@ -162,11 +162,25 @@ static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base, }
/* - * 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 + * Set CPU clock source, for all the level we keep the same + * clock source that the one already configured with DVS + * disabled. For this one we need to use the clock framework */ parent = clk_get_parent(clk); + + /* + * Unset parent clock to force the clock framework setting again + * the clock parent + */ + clk_set_parent(clk, NULL); + + /* + * For the Armada 37xx CPU clocks, setting the parent will + * actually configure the parent when DVFS is enabled. At + * hardware level it will be a different register from the one + * read when doing clk_get_parent that will be set with + * clk_set_parent. + */ clk_set_parent(clk, parent); }
On 08-03-19, 17:47, Gregory CLEMENT wrote:
From: Christian Neubert christian.neubert.86@gmail.com
The clock parenting was not setup properly when DVFS was enabled. It was expected that the same clock source was used with and without DVFS which was not the case.
This patch fixes this issue, allowing to make the cpufreq support work when the CPU clock source are not the default ones.
Fixes: 92ce45fb875d ("cpufreq: Add DVFS support for Armada 37xx") Cc: stable@vger.kernel.org [gregory: extract from a larger patch, modify comments and commit log] Signed-off-by: Christian Neubert christian.neubert.86@gmail.com Signed-off-by: Gregory CLEMENT gregory.clement@bootlin.com
drivers/cpufreq/armada-37xx-cpufreq.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c index 75491fc841a6..ad4463e4266e 100644 --- a/drivers/cpufreq/armada-37xx-cpufreq.c +++ b/drivers/cpufreq/armada-37xx-cpufreq.c @@ -162,11 +162,25 @@ static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base, } /*
* 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
* Set CPU clock source, for all the level we keep the same* clock source that the one already configured with DVS
DVFS ?
* disabled. For this one we need to use the clock framework
Please add a full-stop (.) at the end.
*/parent = clk_get_parent(clk);
- /*
* Unset parent clock to force the clock framework setting again* the clock parent*/- clk_set_parent(clk, NULL);
- /*
* For the Armada 37xx CPU clocks, setting the parent will* actually configure the parent when DVFS is enabled. At* hardware level it will be a different register from the one* read when doing clk_get_parent that will be set with* clk_set_parent.*/}
Maybe this should be done right from the clock driver instead? As cpufreq may or maynot be enabled by default (Surely most of the people will always enable it, but I am just trying to find the right place for doing this). The way we are setting the clock parent isn't that great, and looks a bit hacky just because of the way clock framework is. Maybe doing it directly, without getting clock framework in between, from the clock driver may look sane ?
Hi Viresh,
On lun., mars 11 2019, Viresh Kumar viresh.kumar@linaro.org wrote:
On 08-03-19, 17:47, Gregory CLEMENT wrote:
[...]
parent = clk_get_parent(clk);
- /*
* Unset parent clock to force the clock framework setting again* the clock parent*/- clk_set_parent(clk, NULL);
- /*
* For the Armada 37xx CPU clocks, setting the parent will* actually configure the parent when DVFS is enabled. At* hardware level it will be a different register from the one* read when doing clk_get_parent that will be set with* clk_set_parent.*/}
Maybe this should be done right from the clock driver instead? As cpufreq may or maynot be enabled by default (Surely most of the people will always enable it, but I am just trying to find the right place for doing this). The way we are setting the clock parent isn't that great, and looks a bit hacky just because of the way clock framework is. Maybe doing it directly, without getting clock framework in between, from the clock driver may look sane ?
I've just sent a patch following your suggestion to the clock mailing but keeping you in CC too.
Gregory
-- viresh
Hi Christian, Gregory,
From: Christian Neubert christian.neubert.86@gmail.com
The clock parenting was not setup properly when DVFS was enabled. It was expected that the same clock source was used with and without DVFS which was not the case.
This patch fixes this issue, allowing to make the cpufreq support work when the CPU clock source are not the default ones.
Fixes: 92ce45fb875d ("cpufreq: Add DVFS support for Armada 37xx") Cc: stable@vger.kernel.org [gregory: extract from a larger patch, modify comments and commit log] Signed-off-by: Christian Neubert christian.neubert.86@gmail.com Signed-off-by: Gregory CLEMENT gregory.clement@bootlin.com
drivers/cpufreq/armada-37xx-cpufreq.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c index 75491fc841a6..ad4463e4266e 100644 --- a/drivers/cpufreq/armada-37xx-cpufreq.c +++ b/drivers/cpufreq/armada-37xx-cpufreq.c @@ -162,11 +162,25 @@ static void __init armada37xx_cpufreq_dvfs_setup(struct regmap *base, } /*
* 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
* Set CPU clock source, for all the level we keep the same* clock source that the one already configured with DVS* disabled. For this one we need to use the clock framework- /*
* Unset parent clock to force the clock framework setting again* the clock parent*/- clk_set_parent(clk, NULL);
- /*
* For the Armada 37xx CPU clocks, setting the parent will* actually configure the parent when DVFS is enabled. At* hardware level it will be a different register from the one* read when doing clk_get_parent that will be set with* clk_set_parent.*/} -- 2.20.1
I applied this and selected only CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE=y CONFIG_CPU_FREQ_GOV_PERFORMANCE=y CONFIG_CPU_FREQ_GOV_POWERSAVE=y
The board boots up with 'powersave' instead of 'performance' (maybe some init script?)
When i tried changing to 'performance' i got this: [ 164.134589] Internal error: synchronous parity or ECC error: 96000018 [#1] PREEMPT SMP [ 164.139899] Modules linked in: mvneta mvmdio phylink crct10dif_ce ip_tables x_tables ipv6 [ 164.148321] CPU: 1 PID: 2485 Comm: bash Not tainted 5.0.0espressobin+ #68 [ 164.155305] Hardware name: Globalscale Marvell ESPRESSOBin Board (DT) [ 164.161938] pstate: 000003c5 (nzcv DAIF -PAN -UAO) [ 164.166873] pc : do_mem_abort+0x48/0xb0 [ 164.170808] lr : el1_da+0x20/0x88 [ 164.174210] sp : ffff000011b5b1f0 [ 164.177614] x29: ffff000011b5b1f0 x28: ffff80003a0ab800 [ 164.183080] x27: 0000000000000000 x26: 0000000000000000 [ 164.188545] x25: 0000000000000000 x24: 0000000000000025 [ 164.194012] x23: 00000000000003c5 x22: ffff000010c37318 [ 164.199477] x21: ffff000010c371f0 x20: ffff000011b5b220 [ 164.204942] x19: 000000009600004f x18: 0000000000000000 [ 164.210408] x17: 0000000000000000 x16: 0000000000000005 [ 164.215873] x15: 0000000000000000 x14: 0000000000000000 [ 164.221339] x13: ffff80003ac55168 x12: ffff80003ac55168 [ 164.226805] x11: 0000000000000026 x10: 0101010101010101 [ 164.232270] x9 : 0000000000000000 x8 : 0000000000000000 [ 164.237735] x7 : 0000000000000000 x6 : 0000000000000000 [ 164.243201] x5 : 0000000000000000 x4 : 0000000000000000 [ 164.248667] x3 : 0000000000000000 x2 : 0000000000000000 [ 164.254133] x1 : 0000000000000000 x0 : 0000000000000000 [ 164.259601] Process bash (pid: 2485, stack limit = 0x00000000eb09c765) [ 164.266318] Call trace: [ 164.268830] do_mem_abort+0x48/0xb0 [ 164.272413] el1_da+0x20/0x88 [ 164.275460] el1_error+0x14/0xe4 [ 164.278778] Code: 2a1303e1 9ba07c63 aa1603e0 8b030095 (f8636883) [ 164.285053] ---[ end trace 78a878c96fcb4581 ]--- [ 164.289795] Kernel panic - not syncing: Fatal exception [ 164.295171] SMP: stopping secondary CPUs [ 164.299211] Kernel Offset: disabled [ 164.302788] CPU features: 0x002,2000200c [ 164.306817] Memory Limit: none [ 164.309961] ---[ end Kernel panic - not syncing: Fatal exception ]---
Cheers /Ilias
linux-stable-mirror@lists.linaro.org
-
Gregory CLEMENT -
Ilias Apalodimas -
Viresh Kumar