This frequency was disabled because of unresolved stability problems.
However, based on several months of testing, the source of the
stability problems seems to be the bootloader, not the kernel.
Marvell has recently merged changes to their bootloader source that
addresses the stability issues when frequency scaling is enabled at
all frequencies including 1.2Ghz.
Signed-off-by: Benjamin Schneider <[email protected]>
---
drivers/cpufreq/armada-37xx-cpufreq.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
index bea41ccab..f28a4435f 100644
--- a/drivers/cpufreq/armada-37xx-cpufreq.c
+++ b/drivers/cpufreq/armada-37xx-cpufreq.c
@@ -102,11 +102,7 @@ struct armada_37xx_dvfs {
};
static struct armada_37xx_dvfs armada_37xx_dvfs[] = {
- /*
- * The cpufreq scaling for 1.2 GHz variant of the SOC is currently
- * unstable because we do not know how to configure it properly.
- */
- /* {.cpu_freq_max = 1200*1000*1000, .divider = {1, 2, 4, 6} }, */
+ {.cpu_freq_max = 1200*1000*1000, .divider = {1, 2, 4, 6} },
{.cpu_freq_max = 1000*1000*1000, .divider = {1, 2, 4, 5} },
{.cpu_freq_max = 800*1000*1000, .divider = {1, 2, 3, 4} },
{.cpu_freq_max = 600*1000*1000, .divider = {2, 4, 5, 6} },
--
2.45.1
On Sun, Jun 02, 2024 at 06:26:38PM -0700, Benjamin Schneider wrote:
> This frequency was disabled because of unresolved stability problems.
> However, based on several months of testing, the source of the
> stability problems seems to be the bootloader, not the kernel.
> Marvell has recently merged changes to their bootloader source that
> addresses the stability issues when frequency scaling is enabled at
> all frequencies including 1.2Ghz.
The problem is, most systems don't have the new bootloader. And so if
you enable 1.2GHz, they are going to be unstable.
Rather than making this unconditional, i think it needs to be
conditional on knowing the bootloader has been upgraded. Could you add
code which looks in the DDRPHY and see if 0xC0001004 has the correct
value. Only then enable the additional clock speed.
Andrew
Jun 3, 2024, 05:46 by [email protected]:
> The problem is, most systems don't have the new bootloader. And so if
> you enable 1.2GHz, they are going to be unstable.
>
Based on my testing, the A3720 was unstable using a bootloader built
with Marvell's source without regard to clock speed or frequency scaling.
That is, it didn't matter if 1.2Ghz was enabled or not, and it didn't matter
if cpufreq-dt was loaded or not, my devices were reliably crashing when
trying to use Marvell's source instead of Globalscale's for building the
bootloader. When I dug in to find the difference, this DDRPHY setting
was one of two that I found. I also found that setting it to the value in
Globalscale's repos restored stability to the devices.
I then tested 1.2Ghz bootloader speeds as well as frequency scaling and
found that they worked fine. I've been keeping track here:
https://github.com/bschnei/ebu-bootloader
> Rather than making this unconditional, i think it needs to be
> conditional on knowing the bootloader has been upgraded. Could you add
> code which looks in the DDRPHY and see if 0xC0001004 has the correct
> value. Only then enable the additional clock speed.
>
I think there are two potential issues with doing something like that. First,
that DDRPHY value has been flipped back and forth. The change I
submitted to Marvell just undoes this change from January 2021:
https://github.com/MarvellEmbeddedProcessors/mv-ddr-marvell/commit/c3a8d3c7ff4bd460770b0cf601e57a6f70cb1871
Perhaps it would be OK if the older code is also stable, but I haven't
tested it.
Second, the value seems to be deliberately different for other memory
configurations (DDR3) which makes the conditional logic more complex
if it's meant to work for all A37XX variants, and I don't have other variants
to test.
Given the history of this setting getting flipped back and forth, and
having read through a few old threads on this subject, it's my theory
that some of the instability issues that were attributed to kernel frequency
scaling and/or 1.2Ghz as a speed were more likely attributable to bad
bootloaders all along. I've reached out to Armbian and Arch communities
to let them know in the hope of finding other users of these devices
that might be willing to test, but have not received any responses.
It's also worth noting that my devices came from the factory with the
bootloader clocked at 800Mhz. I'm pretty sure the OS cannot set a
speed above the bootloader clock speed. As a result, at least for the
ESPRESSObin Ultra, the only devices I would expect to break are those
where users have put in the work to build (or taken the risk of flashing)
a bootloader clocked at 1.2Ghz. When the kernel encounters one of
those devices it currently disables frequency scaling entirely (cpufreq-dt
will not load) leaving them to run at full speed constantly. If there are
users who can't/won't update their bootloader and for which frequency
scaling is unstable, it seems like it would make more sense and facilitate
further testing to use kernel config or userspace tools as the place to
disable scaling.
Thanks!
Ben
On Sunday 02 June 2024 18:26:38 Benjamin Schneider wrote:
> This frequency was disabled because of unresolved stability problems.
> However, based on several months of testing, the source of the
> stability problems seems to be the bootloader, not the kernel.
> Marvell has recently merged changes to their bootloader source that
> addresses the stability issues when frequency scaling is enabled at
> all frequencies including 1.2Ghz.
>
> Signed-off-by: Benjamin Schneider <[email protected]>
> ---
> drivers/cpufreq/armada-37xx-cpufreq.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
> index bea41ccab..f28a4435f 100644
> --- a/drivers/cpufreq/armada-37xx-cpufreq.c
> +++ b/drivers/cpufreq/armada-37xx-cpufreq.c
> @@ -102,11 +102,7 @@ struct armada_37xx_dvfs {
> };
>
> static struct armada_37xx_dvfs armada_37xx_dvfs[] = {
> - /*
> - * The cpufreq scaling for 1.2 GHz variant of the SOC is currently
> - * unstable because we do not know how to configure it properly.
> - */
> - /* {.cpu_freq_max = 1200*1000*1000, .divider = {1, 2, 4, 6} }, */
> + {.cpu_freq_max = 1200*1000*1000, .divider = {1, 2, 4, 6} },
> {.cpu_freq_max = 1000*1000*1000, .divider = {1, 2, 4, 5} },
> {.cpu_freq_max = 800*1000*1000, .divider = {1, 2, 3, 4} },
> {.cpu_freq_max = 600*1000*1000, .divider = {2, 4, 5, 6} },
> --
> 2.45.1
As without the updated firmware on 1.2 GHz variant of the SoC is kernel
already crashing, even with commented line for .cpu_freq_max = 1200,
this change makes sense.
There is no reason to have 1.2 GHz line disabled as it does not solve
any issue (as was originally thought) and just prevent people with
updated firmware to use non-performance governor on that SoC.
(When cpufreq driver is not loaded then CPU frequency of the SoC is
locked at the max speed, which has observed behavior same as performance
cpufreq governor).
So, go ahead with this change with my
Reviewed-by: Pali Rohár <[email protected]>
On Wed, Jun 05, 2024 at 09:44:22PM +0200, Pali Roh?r wrote:
> On Sunday 02 June 2024 18:26:38 Benjamin Schneider wrote:
> > This frequency was disabled because of unresolved stability problems.
> > However, based on several months of testing, the source of the
> > stability problems seems to be the bootloader, not the kernel.
> > Marvell has recently merged changes to their bootloader source that
> > addresses the stability issues when frequency scaling is enabled at
> > all frequencies including 1.2Ghz.
> >
> > Signed-off-by: Benjamin Schneider <[email protected]>
> > ---
> > drivers/cpufreq/armada-37xx-cpufreq.c | 6 +-----
> > 1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/cpufreq/armada-37xx-cpufreq.c b/drivers/cpufreq/armada-37xx-cpufreq.c
> > index bea41ccab..f28a4435f 100644
> > --- a/drivers/cpufreq/armada-37xx-cpufreq.c
> > +++ b/drivers/cpufreq/armada-37xx-cpufreq.c
> > @@ -102,11 +102,7 @@ struct armada_37xx_dvfs {
> > };
> >
> > static struct armada_37xx_dvfs armada_37xx_dvfs[] = {
> > - /*
> > - * The cpufreq scaling for 1.2 GHz variant of the SOC is currently
> > - * unstable because we do not know how to configure it properly.
> > - */
> > - /* {.cpu_freq_max = 1200*1000*1000, .divider = {1, 2, 4, 6} }, */
> > + {.cpu_freq_max = 1200*1000*1000, .divider = {1, 2, 4, 6} },
> > {.cpu_freq_max = 1000*1000*1000, .divider = {1, 2, 4, 5} },
> > {.cpu_freq_max = 800*1000*1000, .divider = {1, 2, 3, 4} },
> > {.cpu_freq_max = 600*1000*1000, .divider = {2, 4, 5, 6} },
> > --
> > 2.45.1
>
> As without the updated firmware on 1.2 GHz variant of the SoC is kernel
> already crashing, even with commented line for .cpu_freq_max = 1200,
> this change makes sense.
>
> There is no reason to have 1.2 GHz line disabled as it does not solve
> any issue (as was originally thought) and just prevent people with
> updated firmware to use non-performance governor on that SoC.
> (When cpufreq driver is not loaded then CPU frequency of the SoC is
> locked at the max speed, which has observed behavior same as performance
> cpufreq governor).
>
> So, go ahead with this change with my
>
> Reviewed-by: Pali Roh?r <[email protected]>
I defer to your knowledge in this matter.
Reviewed-by: Andrew Lunn <[email protected]>
Andrew