Re: [PATCH] dt-bindings: arm: sunxi: update H2+/H3 cpu clocks

[Wilken Gottwalt <wilken.gottwalt@mailbox.org>]

On Wed, 9 Sep 2020 14:08:59 +0200
Maxime Ripard <maxime@cerno.tech> wrote:
> Hi!
> 
> Thanks for contributing
> 
> The prefix isn't right though.
> 
> dt-bindings is used when you're modifying the binding itself, ie the
> description of what the node is supposed to look like, not when you
> actually use that node in a DT.
> 
> In that case, that would be ARM: dts: sunxi:
> 
> (we're on the ARM architecture, modifying dts's, for the sunxi platform)

Ah, I see, it was my first attempt to contribute and wasn't 100% sure, just
took the line from similar patches on the LKML. Thanks for the correction.

> On Thu, Sep 03, 2020 at 12:07:08PM +0200, Wilken Gottwalt wrote:
> > Change H2+/H3 clocks to 8 steps from 528 MHz up to 1200 MHz to support a
> > more fine-grained powersave setup. The SoCs are made for 1296 MHz, so
> > these clocks are still in a safe range. Tested on a NanoPi Duo and
> > OrangePi Zero.
> 
> How was this tested?

This is a longer story. It actually runs on hardware which is in production
for about 2-3 years, some use H2+ with full voltage regulators and some are
similar to the NanoPi DUO, where the voltage regulator can only switch
between 1.1 and 1.3 volts. It runs in two ways: A fully dynamic setup where
the ondemand scheduler is used and the second way where it is switched to
fixed values (based on load and temperature) using the cpufrequtils. The
devices running a 4.14.x kernel and are tested against 4.19.x kernels.
These devices are routers running a custom tcp/ip stack and have a high I/O
load. I also prepared devices based on a custom H3 design, which ran stable
at 1.392 GHz (but had passive coolers attached). Do these explanations
help?

> cpufreq OPP misconfiguration on Allwinner SoCs has been known to create
> some errors that are fairly hard to spot and be quite easy to go
> unnoticed (like caches corruptions).

Yeah, I noticed that in the beginning where I prepared the first kernels
for these devices. But after switching to multiples of 48MHz and bigger
steps these issues disappeared. I'm aware that this does not proof that
these issues do not appear, but thougth I share the values which I
consider stable.

> The only reliable test we have is:
> https://github.com/ssvb/cpuburn-arm/blob/master/cpufreq-ljt-stress-test
> 
> > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@mailbox.org>
> > ---
> >  arch/arm/boot/dts/sun8i-h3.dtsi | 34 +++++++++++++++++++++++++++++++--
> >  1 file changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> > index 4e89701df91f..5517fcc02b7d 100644
> > --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> > @@ -48,23 +48,53 @@ cpu0_opp_table: opp_table0 {
> >  		compatible = "operating-points-v2";
> >  		opp-shared;
> >  
> > -		opp-648000000 {
> > -			opp-hz = /bits/ 64 <648000000>;
> > +		opp-528000000 {
> > +			opp-hz = /bits/ 64 <528000000>;
> > +			opp-microvolt = <1020000 1020000 1300000>;
> > +			clock-latency-ns = <244144>; /* 8 32k periods */
> > +		};
> > +
> > +		opp-624000000 {
> > +			opp-hz = /bits/ 64 <624000000>;
> >  			opp-microvolt = <1040000 1040000 1300000>;
> >  			clock-latency-ns = <244144>; /* 8 32k periods */
> >  		};
> >  
> > +		opp-720000000 {
> > +			opp-hz = /bits/ 64 <720000000>;
> > +			opp-microvolt = <1060000 1060000 1300000>;
> > +			clock-latency-ns = <244144>; /* 8 32k periods */
> > +		};
> > +
> >  		opp-816000000 {
> >  			opp-hz = /bits/ 64 <816000000>;
> >  			opp-microvolt = <1100000 1100000 1300000>;
> >  			clock-latency-ns = <244144>; /* 8 32k periods */
> >  		};
> >  
> > +		opp-912000000 {
> > +			opp-hz = /bits/ 64 <912000000>;
> > +			opp-microvolt = <1140000 1140000 1300000>;
> > +			clock-latency-ns = <244144>; /* 8 32k periods */
> > +		};
> > +
> >  		opp-1008000000 {
> >  			opp-hz = /bits/ 64 <1008000000>;
> >  			opp-microvolt = <1200000 1200000 1300000>;
> >  			clock-latency-ns = <244144>; /* 8 32k periods */
> >  		};
> > +
> > +		opp-1104000000 {
> > +			opp-hz = /bits/ 64 <1104000000>;
> > +			opp-microvolt = <1240000 1240000 1300000>;
> > +			clock-latency-ns = <244144>; /* 8 32k periods */
> > +		};
> > +
> > +		opp-1200000000 {
> > +			opp-hz = /bits/ 64 <1200000000>;
> > +			opp-microvolt = <1300000 1300000 1300000>;
> > +			clock-latency-ns = <244144>; /* 8 32k periods */
> > +		};
> >  	};
> 
> IIRC U-Boot will start the CPU at 1008 MHz on the H3, so in the
> situation where a board doesn't list the regulators attached to the CPU,
> the kernel will happily use the 1104 and 1200 MHz frequencies, even
> though it won't adjust the voltage accordingly, causing errors.

Yeah, I know that. The u-boot running on the mentioned devices also have
these modifications, including a higher clocked DRAM (actually running at
624 MHz).

> The way we worked around that is to provide the extra OOPs in a separate
> DTSI so that the boards with regulator support can opt-in.

Oh, did I overlooked something? I was working on 4.14 up to 4.19 kernels
and may be a bit to eager to post the modifications.

greetings,
Will

> Maxime
> 
> >  
> >  	cpus {
> > -- 
> > 2.28.0
> >