[PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Benjamin Schneider]

Hi, I've been running two Marvell A3720 devices with CPU frequency scaling
enabled and 1.2Ghz enabled for several months without issues. In order to do
this I had to patch the DDR initialization code in Marvell's repos
(https://github.com/MarvellEmbeddedProcessors/mv-ddr-marvell/pull/44).

Without that patch, the device would indeed crash with frequency scaling
enabled at any clock speed. This indicates that at least some of the stability
issues experienced by users in the past are a result of improperly configured
bootloaders rather than an issue with the kernel. As a result, it seems
reasonable to re-enable the 1.2Ghz clock speed for users.

Ben


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Benjamin Schneider]

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 <ben@bens.haus>
---
 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Andrew Lunn]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Ben Schneider]

Jun 3, 2024, 05:46 by andrew@lunn.ch:

> 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Pali Rohár]

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 <ben@bens.haus>
> ---
>  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 <pali@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Andrew Lunn]

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 <ben@bens.haus>
> > ---
> >  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 <pali@kernel.org>

I defer to your knowledge in this matter.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Ben Schneider]

Jun 5, 2024, 16:44 by andrew@lunn.ch:

> On Wed, Jun 05, 2024 at 09:44:22PM +0200, Pali Rohár wrote:
>
>> So, go ahead with this change with my
>>
>> Reviewed-by: Pali Rohár <pali@kernel.org>
>>
>
> I defer to your knowledge in this matter.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>
>  Andrew
>

Hi all, just checking in to see if there are any outstanding concerns/issues outstanding with this patch? I am happy to report several more weeks of stability on two A3720 devices. Thanks!

Ben

Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Gregory CLEMENT]

Hello Ben,

> Jun 5, 2024, 16:44 by andrew@lunn.ch:
>
>> On Wed, Jun 05, 2024 at 09:44:22PM +0200, Pali Rohár wrote:
>>
>>> So, go ahead with this change with my
>>>
>>> Reviewed-by: Pali Rohár <pali@kernel.org>
>>>
>>
>> I defer to your knowledge in this matter.
>>
>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>>
>>  Andrew
>>
>
> Hi all, just checking in to see if there are any outstanding
> concerns/issues outstanding with this patch? I am happy to report
> several more weeks of stability on two A3720 devices. Thanks!
>

You received two "reviewed-by" approvals, and Marek Behún, who wrote the
patch removing the 1.2GHz limitation, did not object. Therefore, from my
point of view, there is no problem merging your change.

As the original author of this driver, you can add my "Acked-by" to your
patch if it can help:

Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Gregory


> Ben

Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Ben Schneider]

Jul 16, 2024, 01:51 by gregory.clement@bootlin.com:

> You received two "reviewed-by" approvals, and Marek Behún, who wrote the
> patch removing the 1.2GHz limitation, did not object. Therefore, from my
> point of view, there is no problem merging your change.
> Acked-by: Gregory CLEMENT <> gregory.clement@bootlin.com> >
>

Hi all, please let me know if there is anything else needed from me before this can be merged. The two devices I have continue to be stable while using the ondemand scaling policy.

Thanks!

Ben

Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Ben Schneider]

Jul 16, 2024, 01:51 by gregory.clement@bootlin.com:

> Hello Ben,
>
>> Jun 5, 2024, 16:44 by >> andrew@lunn.ch>> :
>>
>>> On Wed, Jun 05, 2024 at 09:44:22PM +0200, Pali Rohár wrote:
>>>
>>>> So, go ahead with this change with my
>>>> Reviewed-by: Pali Rohár <>>>> pali@kernel.org>>>> >
>>>>
>>> I defer to your knowledge in this matter.
>>> Reviewed-by: Andrew Lunn <>>> andrew@lunn.ch>>> >
>>>
> Acked-by: Gregory CLEMENT <> gregory.clement@bootlin.com> >
>

Hi maintainers, I don't believe there are any outstanding concerns with this patch. Would one of you please merge or let me know next steps? Marvell accepted the patch to fix the bootloader instability in May, and I've since had two devices running stably with frequency scaling enabled up to 1.2Ghz. I've kept track of my work here: https://github.com/bschnei/ebu-bootloader

Thank you!

Ben

[PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[bschnei]

From: Benjamin Schneider <ben@bens.haus>

This frequency was disabled because of stability problems whose source could
not be accurately identified[1]. After seven months of testing, the evidence
points to an incorrectly configured bootloader as the source of the historical
instability. Testing was performed on two A3720 devices with this frequency
enabled and the ondemand policy in use. Marvell merged[2] changes to their
bootloader source needed to address the stability issue. This driver should
expose this frequency option to users.

[1] https://github.com/torvalds/linux/commit/484f2b7c61b9ae58cc00c5127bcbcd9177af8dfe
[2] https://github.com/MarvellEmbeddedProcessors/mv-ddr-marvell/pull/44

Signed-off-by: Benjamin Schneider <ben@bens.haus>
Reviewed-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 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 bea41ccabf1f..f28a4435fba7 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.47.1

Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Viresh Kumar]

Marek,

On 25-11-24, 13:14, bschnei@gmail.com wrote:
> From: Benjamin Schneider <ben@bens.haus>
> 
> This frequency was disabled because of stability problems whose source could
> not be accurately identified[1]. After seven months of testing, the evidence
> points to an incorrectly configured bootloader as the source of the historical
> instability. Testing was performed on two A3720 devices with this frequency
> enabled and the ondemand policy in use. Marvell merged[2] changes to their
> bootloader source needed to address the stability issue. This driver should
> expose this frequency option to users.
> 
> [1] https://github.com/torvalds/linux/commit/484f2b7c61b9ae58cc00c5127bcbcd9177af8dfe
> [2] https://github.com/MarvellEmbeddedProcessors/mv-ddr-marvell/pull/44
> 
> Signed-off-by: Benjamin Schneider <ben@bens.haus>
> Reviewed-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
>  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 bea41ccabf1f..f28a4435fba7 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} },

Any inputs on this before I apply it ?

-- 
viresh

Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Marek Behún]

On Thu, Dec 12, 2024 at 12:37:12PM +0530, Viresh Kumar wrote:
> Marek,

...

> Any inputs on this before I apply it ?

Viresh, let me try to test it this week on Turris Mox.

Marek

Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Ben Schneider]

On Sunday, December 15th, 2024 at 1:23 PM, Marek Behún <kabel@kernel.org> wrote:
> 
> Viresh, let me try to test it this week on Turris Mox.

Hi Marek, were you able to test this change on the Turris Mox? If I can help, please let me know.

Ben

Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Viresh Kumar]

On 15-12-24, 22:23, Marek Behún wrote:
> On Thu, Dec 12, 2024 at 12:37:12PM +0530, Viresh Kumar wrote:
> > Marek,
> 
> ...
> 
> > Any inputs on this before I apply it ?
> 
> Viresh, let me try to test it this week on Turris Mox.

Hi Marek,

I am inclined to apply this unless you have an objection ..
Please let me know.

-- 
viresh

Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx

[Viresh Kumar]

On 25-11-24, 13:14, bschnei@gmail.com wrote:
> From: Benjamin Schneider <ben@bens.haus>
> 
> This frequency was disabled because of stability problems whose source could
> not be accurately identified[1]. After seven months of testing, the evidence
> points to an incorrectly configured bootloader as the source of the historical
> instability. Testing was performed on two A3720 devices with this frequency
> enabled and the ondemand policy in use. Marvell merged[2] changes to their
> bootloader source needed to address the stability issue. This driver should
> expose this frequency option to users.
> 
> [1] https://github.com/torvalds/linux/commit/484f2b7c61b9ae58cc00c5127bcbcd9177af8dfe
> [2] https://github.com/MarvellEmbeddedProcessors/mv-ddr-marvell/pull/44
> 
> Signed-off-by: Benjamin Schneider <ben@bens.haus>
> Reviewed-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
>  drivers/cpufreq/armada-37xx-cpufreq.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)

Applied. Thanks.

-- 
viresh