Message-ID: <20240905133412.6ba050de@donnerap.manchester.arm.com>
Date: Thu, 5 Sep 2024 13:34:12 +0100
From: Andre Przywara <andre.przywara@....com>
To: Chen-Yu Tsai <wens@...e.org>
Cc: Dragan Simic <dsimic@...jaro.org>, linux-sunxi@...ts.linux.dev,
 jernej.skrabec@...il.com, samuel@...lland.org,
 linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: dts: allwinner: a64: Move CPU OPPs to the SoC
 dtsi file

On Thu, 5 Sep 2024 20:26:15 +0800
Chen-Yu Tsai <wens@...e.org> wrote:

> Hi,
> 
> On Thu, Sep 5, 2024 at 8:17 PM Dragan Simic <dsimic@...jaro.org> wrote:
> >
> > Hello,
> >
> > Just checking, any further thoughts about this patch?  
> 
> Sorry, but I feel like it's not really worth the churn. There's not
> really a problem to be solved here. What you are arguing for is more
> about aesthetics, and we could argue that having them separate makes
> it easier to read and turn on/off.

Yeah, I agree. If a board wants to support OPPs, they just have to include
a single file and define the CPU regulator, and that's a nice opt-in,
IMHO.
But having this patch would make it quite hard to opt out, I believe. For
Linux there are probably ways to disable DVFS nevertheless, but I am not
sure this is true in an OS agnostic pure-DT-only way.

This could probably be solved, but same as Chen-Yu I don't see any good
enough reason for this patch in the first place.

Cheers,
Andre

> And even though the GPU OPPs are in the dtsi, it's just one OPP acting
> as a default clock rate.
> 
> 
> ChenYu
> 
> > On 2024-08-17 06:25, Dragan Simic wrote:  
> > > Hello Andre,
> > >
> > > On 2024-08-15 19:15, Andre Przywara wrote:  
> > >> On Thu, 15 Aug 2024 18:34:58 +0200
> > >> Dragan Simic <dsimic@...jaro.org> wrote:  
> > >>> On 2024-08-14 18:11, Chen-Yu Tsai wrote:  
> > >>> > On Wed, Aug 14, 2024 at 1:52 PM Dragan Simic <dsimic@...jaro.org>
> > >>> > wrote:  
> > >>> >>
> > >>> >> Move the Allwinner A64 CPU OPPs to the A64 SoC dtsi file and,
> > >>> >> consequently,
> > >>> >> adjust the contents of the affected board dts(i) files appropriately,
> > >>> >> to
> > >>> >> "encapsulate" the CPU OPPs into the SoC dtsi file.
> > >>> >>
> > >>> >> Moving the CPU OPPs to the SoC dtsi file, instead of requiring the
> > >>> >> board
> > >>> >> dts(i) files to include both the SoC dtsi file and the CPU OPP dtsi
> > >>> >> file,
> > >>> >> reduces the possibility for incomplete SoC data inclusion and improves
> > >>> >> the
> > >>> >> overall hierarchical representation of data.  Moreover, the CPU OPPs
> > >>> >> are
> > >>> >> not used anywhere but together with the SoC dtsi file, which
> > >>> >> additionally
> > >>> >> justifies the folding of the CPU OPPs into the SoC dtsi file.
> > >>> >>
> > >>> >> No functional changes are introduced, which was validated by
> > >>> >> decompiling and
> > >>> >> comparing all affected board dtb files before and after these changes.
> > >>> >>  When
> > >>> >> compared with the decompiled original dtb files, the updated dtb files
> > >>> >> have
> > >>> >> some of their blocks shuffled around a bit and some of their phandles
> > >>> >> have
> > >>> >> different values, as a result of the changes to the order in which the
> > >>> >> building blocks from the parent dtsi files are included into them, but
> > >>> >> they
> > >>> >> still effectively remain the same as the originals.  
> > >>> >
> > >>> > IIRC, this was a conscious decision requiring board dts files to set
> > >>> > their
> > >>> > CPU supply before OPPs are given. The bootloader does not boot the SoC
> > >>> > at the highest possible OPP / regulator voltage, so if the OPPs are
> > >>> > given
> > >>> > but the supply is not, the kernel will attempt to raise the frequency
> > >>> > beyond what the current voltage can supply, causing it to hang.  
> > >>
> > >> Yes, this is what I remember as well: this forces boards to opt in to
> > >> DVFS, otherwise they get a fixed 816 MHz. Since there is only one OPP
> > >> table for all boards with that SoC, I think it's reasonable to ask for
> > >> this, since the cooling could not be adequate for higher frequencies
> > >> in
> > >> the first place, or the power supply is not up to par.  
> > >
> > > If the cooling isn't capable enough to dissipate the additional heat
> > > generated at higher frequencies, the thermal governor is there to
> > > handle
> > > that by lowering the operating frequency.  If the PSU isn't capable to
> > > provide an additional watt or two, I think a better PSU is needed. :)
> > > No reasonably sized PSU should work at ~100% of its power output.
> > >
> > > On top of that, all currently supported A64-based boards have the CPU
> > > OPPs defined and CPU DVFS enabled, so no such issues are possible
> > > there.
> > > Though, there could be some issues with new A64-based boards, which is
> > > discussed further below.
> > >  
> > >>> > Now that all existing boards have it properly enabled, there should be
> > >>> > no
> > >>> > need for this. However I would appreciate a second opinion.  
> > >>
> > >> Well, since there is no way to opt *out* now, I am somewhat reluctant
> > >> to
> > >> just have this. What is the actual problem we are solving here? After
> > >> all
> > >> there is just one OPP table for all A64 boards, so there is less
> > >> confusion
> > >> about what to include in each board file. Which IIUC is a more
> > >> complicated
> > >> situation on the Rockchip side.  
> > >
> > > Well, this patch doesn't solve some real problem, but it makes the
> > > things
> > > neater and a bit more clean.  The things are more complicated with
> > > Rockchip
> > > SoCs, but following the concept of "encapsulating" the CPU OPPs into
> > > the
> > > A64 SoC dtsi makes things neater.  Moreover, the A64 GPU OPPs are
> > > already
> > > in the A64 SoC dtsi, so we could also say that folding the A64 CPU OPPs
> > > into the SoC dtsi follows the A64 GPU OPPs.
> > >  
> > >> I still have to try "operating-points-v2", but at least on the H616
> > >> side
> > >> putting a 'status = "disabled";' into the OPP node didn't prevent it
> > >> from
> > >> probing. Otherwise this would have been a nice compromise, I think.
> > >>  
> > >>> Good point, thanks for the clarification.  This is quite similar to
> > >>> how
> > >>> board dts(i) files for Rockchip SoCs need to enable the SoC's
> > >>> built-in
> > >>> TSADC for temperature sensing, before the CPU thermal throttling can
> > >>> actually work and prevent the SoC from overheating, etc.
> > >>>
> > >>> The consensus for Rockchip boards is that it's up to the authors and
> > >>> reviewers of the board dts(i) files to make sure that the built-in
> > >>> TSADC
> > >>> is enabled, etc.  With that approach in mind, and knowing that all
> > >>> Allwinner
> > >>> A64 board dts(i) files are in good shape when it comes to the
> > >>> associated
> > >>> voltage regulators, I think it's fine to follow the same approach of
> > >>> "encapsulating" the CPU OPPs into the A64 SoC dtsi file.  
> > >>
> > >> As mentioned above, I am not so sure about this. With this patch here,
> > >> *every* board gets DVFS. And while this seems to be fine when looking
> > >> at
> > >> the current DTs in the tree (which have it anyway), it creates a
> > >> potentially dangerous situation for new boards.
> > >>
> > >> So pragmatically speaking, this patch would be fine, but it leaves me
> > >> a
> > >> bit uneasy about future or downstream boards.  
> > >
> > > Frankly, I wouldn't be worried about that.  When a new A64-based board
> > > is added, it should be verified that CPU DVFS works as expected, etc.,
> > > before the new board dts file is accepted upstream.
> > >
> > > Maybe we could take into account some possible issues when someone
> > > starts
> > > putting together a new A64-based board dts file, but there are already
> > > many dangerous things that someone can do in the process, such as
> > > messing
> > > up various regulators and voltages unrelated to the CPU DVFS, so
> > > everyone
> > > putting a new board dts file together simply have to know what are they
> > > doing.  I see no way for escaping from that need.
> > >  
> 
> [...]

Powered by blists - more mailing lists