| Message ID | 20190308164710.10597-2-gregory.clement@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix various issue on CPU freq for Armada 37xx | expand |
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. > + */ > clk_set_parent(clk, 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. >> + */ >> clk_set_parent(clk, 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 > */ > 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); > } > > -- > 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
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); }