Re: [PATCH 1/4] arm64: dts: rockchip: list all CPU supplies on ArmSoM Sige5

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Wednesday, 4 June 2025 21:12:35 Central European Summer Time Alexey Charkov wrote:
> On Wed, Jun 4, 2025 at 10:38 PM Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> >
> > On Tuesday, 3 June 2025 19:01:13 Central European Summer Time Alexey Charkov wrote:
> > > List both CPU supply regulators which drive the little and big CPU
> > > clusters, respectively, so that cpufreq can pick them up.
> > >
> > > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > > ---
> > >  .../boot/dts/rockchip/rk3576-armsom-sige5.dts      | 28 ++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3576-armsom-sige5.dts b/arch/arm64/boot/dts/rockchip/rk3576-armsom-sige5.dts
> > > index b09e789c75c47fec7cf7e9810ab0dcca32d9404a..d9c129be55a0d997e04e6d677cdc98fb50353418 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3576-armsom-sige5.dts
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3576-armsom-sige5.dts
> > > @@ -207,6 +207,22 @@ vcc_3v3_ufs_s0: regulator-vcc-ufs-s0 {
> > >       };
> > >  };
> > >
> > > +&cpu_b0 {
> > > +     cpu-supply = <&vdd_cpu_big_s0>;
> > > +};
> > > +
> > > +&cpu_b1 {
> > > +     cpu-supply = <&vdd_cpu_big_s0>;
> > > +};
> > > +
> > > +&cpu_b2 {
> > > +     cpu-supply = <&vdd_cpu_big_s0>;
> > > +};
> > > +
> > > +&cpu_b3 {
> > > +     cpu-supply = <&vdd_cpu_big_s0>;
> > > +};
> > > +
> > >  &combphy0_ps {
> > >       status = "okay";
> > >  };
> > > @@ -215,6 +231,18 @@ &cpu_l0 {
> > >       cpu-supply = <&vdd_cpu_lit_s0>;
> > >  };
> > >
> > > +&cpu_l1 {
> > > +     cpu-supply = <&vdd_cpu_lit_s0>;
> > > +};
> > > +
> > > +&cpu_l2 {
> > > +     cpu-supply = <&vdd_cpu_lit_s0>;
> > > +};
> > > +
> > > +&cpu_l3 {
> > > +     cpu-supply = <&vdd_cpu_lit_s0>;
> > > +};
> > > +
> > >  &gmac0 {
> > >       phy-mode = "rgmii-id";
> > >       clock_in_out = "output";
> > >
> > >
> >
> > Reviewed-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > Tested-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> >
> > > so that cpufreq can pick them up.
> >
> > Fwiw, even without this patch they're picked up by cpufreq-dt for me:
> >
> >     user@trixie:~$ sudo cpupower frequency-info
> >       analyzing CPU 5:
> >       driver: cpufreq-dt
> >       CPUs which run at the same hardware frequency: 4 5 6 7
> >       CPUs which need to have their frequency coordinated by software: 4 5 6 7
> >       maximum transition latency: 40.0 us
> >       hardware limits: 408 MHz - 2.30 GHz
> >       available frequency steps:  408 MHz, 600 MHz, 816 MHz, 1.01 GHz, 1.20 GHz, 1.42 GHz, 1.61 GHz, 1.80 GHz, 2.02 GHz, 2.21 GHz, 2.30 GHz
> >       available cpufreq governors: ondemand userspace performance schedutil
> >       current policy: frequency should be within 408 MHz and 2.30 GHz.
> >                       The governor "schedutil" may decide which speed to use
> >                       within this range.
> >       current CPU frequency: 1.61 GHz (asserted by call to hardware)
> >     user@trixie:~$ uname -a
> >     Linux trixie 6.15.0-11173-g546b1c9e93c2 #2 SMP PREEMPT Wed Jun  4 20:32:52 CEST 2025 aarch64 GNU/Linux
> 
> Frequencies are fine, but I don't think the more power hungry big CPU
> cluster gets any voltage scaling without it. Once I try to load the
> system enough that the governor decides to bump the big cluster
> frequency up, the regulator stays at 850000 microvolts, causing random
> reboots when the whole cluster starts starving. With the patch,
> voltage oscillates between 700000-737000 microvolts in idle and jumps
> up to 950000 under load, and the system seems stable.

Okay, that sounds pretty serious and in this case you should add the
following tag at the end of the commit message, usually as the first
thing after the line break:

  Fixes: 40f742b07ab2 ("arm64: dts: rockchip: Add rk3576-armsom-sige5 board")

That way, our stable kernel robot overlords will pick it up and
backport it to the kernels that already contain the mentioned commit.

> 
> Here's what I used to monitor the voltage (there must be a better way
> to do it, but it works):
> sige5 ~ # watch cat `grep -r . /sys/class/regulator/*/name | grep
> vdd_cpu_big_s0 | sed -e 's/name.*//'`/microvolts
> 
> And in another terminal:
> sige5 ~ # stress-ng -c8
> 
> This might warrant a note in the commit message I guess :)

Yes, definitely mention stuff like this in the commit message.

> 
> Thanks for your review and testing!
> 
> Best regards,
> Alexey
> 

Kind regards,
Nicolas Frattaroli