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

[Maxime Ripard]

Hi,

On Mon, Sep 14, 2020 at 06:00:56AM +0200, Wilken Gottwalt wrote:
> > > > 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 really reliable test we've had so far is the one I pointed out,
> > so please run it on one board at least
> > 
> > > > 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@xxxxxxxxxxx>
> > > > > ---
> > > > >  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.
> > 
> > It's not really my point :)
> > 
> > My point is that since it's in the DTSI, every board using an H2/H3 will
> > now get to use those OPPs, even if they don't have the regulator support
> > and the voltage set for the highest frequency, so you can end up on
> > those boards with the CPU running at 1.2GHz and the voltage associated
> > to 1GHz
> 
> Yeah, I understand. So the solution would be to drop the two highest OPPs
> or make these changes only for the tested devices. What would you prefer?

Like I said earlier, the easiest would be to have another DTSI with the
OPPs that would be included by the boards with regulator support

Maxime