Received-SPF: pass (google.com: domain of linux-kernel+bounces-199653-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 3 Jun 2024 21:33:19 +0200 (CEST)
From: Ben Schneider <ben@bens.haus>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Benjamin Schneider <bschnei@gmail.com>,
	Gregory Clement <gregory.clement@bootlin.com>,
	Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>,
	Linux Arm Kernel <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel <linux-kernel@vger.kernel.org>
Message-ID: <NzURcjd--3-9@bens.haus>
In-Reply-To: <f2284d6b-2e75-4896-9e10-caf2f72854a0@lunn.ch>
References: <20240603012804.122215-1-ben@bens.haus> <20240603012804.122215-2-ben@bens.haus> <f2284d6b-2e75-4896-9e10-caf2f72854a0@lunn.ch>
Subject: Re: [PATCH] cpufreq: enable 1200Mhz clock speed for armada-37xx
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

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 matte=
r
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. Firs=
t,
that DDRPHY=C2=A0value 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/c3a8d3c7=
ff4bd460770b0cf601e57a6f70cb1871
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 varian=
ts
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 frequenc=
y
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=C2=A0the bootloader=C2=A0clock 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