Re: [PATCH] arm64: dts: allwinner: a64: Move CPU OPPs to the SoC dtsi file

[Dragan Simic <dsimic@manjaro.org>]

Hello Andre,

On 2024-09-05 15:20, Andre Przywara wrote:
> On Thu, 05 Sep 2024 14:54:03 +0200 Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-09-05 14:42, Andre Przywara wrote:
>> > On Thu, 05 Sep 2024 14:38:53 +0200
>> > Dragan Simic <dsimic@manjaro.org> wrote:
>> >> On 2024-09-05 14:34, Andre Przywara wrote:
>> >> > On Thu, 5 Sep 2024 20:26:15 +0800
>> >> > Chen-Yu Tsai <wens@csie.org> wrote:
>> >> >> On Thu, Sep 5, 2024 at 8:17 PM Dragan Simic <dsimic@manjaro.org>
>> >> >> wrote:
>> >> >> > 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.
>> >>
>> >> Thanks for your response.  The only thing that still makes me wonder
>> >> is why would a board want to opt out of DVFS?  Frankly, I'd consider
>> >> the design of the boards that must keep DVFS disabled broken.
>> >
>> > Yes! Among the boards using Allwinner SoCs there are some, say
>> > less-optimal designs ;-)
>> 
>> I see, but such boards could simply disable the "cpu0_opp_table"
>> node in their dts(i) files, for the encapsulated CPU OPPs scenario,
>> and everything would still work and be defined in a clean(er) way.
> 
> I agree, and I was already about to suggest this as a reply to your 
> initial
> post, but I think I tried that, and IIRC this doesn't work: the 
> "status"
> property is not honoured for this node.
> But please double check that.

I apologize for my delayed response.

Perhaps a safer approach could be to introduce a new dtsi file, named
sun50i-a64-cpu-opps-disabled.dtsi, with the following contents:

/delete-node/ &cpu0_opp_table;

&cpu0 {
	/delete-property/ operating-points-v2;
};

&cpu1 {
	/delete-property/ operating-points-v2;
};

&cpu2 {
	/delete-property/ operating-points-v2;
};

&cpu3 {
	/delete-property/ operating-points-v2;
};

The purpose of this new file would be to delete the CPU OPPs for the
suboptimally designed boards, when included into their dts(i) files,
while the CPU OPPs would be "encapsulated" into the SoC dtsi.

Though, I'm not sure how much cleaner this approach would be, but
I think it would fit rather well with the suggested approach of having
such suboptimal board designs treated as an exception that would be
handled in some special way.  As a bonus, it would also make locating
dts(i) files for such suboptimal designs a bit simpler, by grepping
for "cpu-opps-disabled".

>> I mean, if there are some suboptimal designs, perhaps the defaults
>> should be tailored towards the good designs, and the suboptimal
>> designs should be some kind of exceptions.
>> 
>> >> > 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.
>> >> >
>> >> >> And even though the GPU OPPs are in the dtsi, it's just one OPP acting
>> >> >> as a default clock rate.