Hi, On Thu, Sep 5, 2024 at 8:17 PM Dragan Simic <dsimic@xxxxxxxxxxx> 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. 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@xxxxxxxxxxx> wrote: > >>> On 2024-08-14 18:11, Chen-Yu Tsai wrote: > >>> > On Wed, Aug 14, 2024 at 1:52 PM Dragan Simic <dsimic@xxxxxxxxxxx> > >>> > 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. > > [...]
