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.
>
