Re: [PATCH RFT v2 01/14] dt-bindings: clock: qcom,rpmcc: Add a way to enable unused clock cleanup

[Konrad Dybcio]

On 19.04.2023 16:00, Stephan Gerhold wrote:
> On Wed, Apr 19, 2023 at 01:31:01PM +0200, Konrad Dybcio wrote:
>> What should we do about the non-bus RPM clocks though? I don't fancy
>> IPA_CLK running 24/7.. And Stephan Gerhold was able to achieve VDD_MIN
>> on msm8909 with these clocks shut down (albeit with a very basic dt setup)!
>>
>> Taking into account the old interconnect-enabled DTs, some of the
>> clocks would need to be on so that the QoS writes can succeed
>> (e.g. the MAS_IPA endpoint needs IPA_CLK), it gets complicated again..
>>
> 
> I guess MSM8996 is the only platform affected by this? sdm630.dtsi seems
> to list the clock already in the a2noc and all others don't seem to have
> an interconnect driver yet.
> 
> This will be subjective and someone will surely disagree but...
> 
> IMO forcing all RPM clocks on during boot and keeping them enabled is
> not part of the DT ABI. If you don't describe the hardware correctly and
> are missing necessary clocks in the description (like the IPA_CLK on the
> interconnect node) then your DT is wrong and should be fixed.
> 
> I would see this a bit like typical optimizing C compilers nowadays. If
> you write correct code it can optimize, e.g. drop unnecessary function
> calls. But if you write incorrect code with undefined behavior it's not
> the fault of the compiler if you run into trouble. The code must be
> fixed.
> 
> The DT bindings don't specify that unused resources (clocks, ...) stay
> "magically" active. They specify that that the resources you reference
> are available. As such, I would say the OS is free to optimize here and
> turn off unused resources.
> 
> The more important point IMO is not breaking all platforms without
> interconnect drivers. This goes beyond just adding a missing clock to
> the DT, you need to write the driver first. But having the max vote in
> icc_smd_rpm (somehow) should hopefully take care of that.
Hm, interesting argument.

Krzysztof, Bjorn, what's your stance on this?

We *need* to add unused cleanup to rpmcc for feature completion and
there's no good way of discerning whether it's safe to do so..

Doing so will make clk_ignore_unused necessary to boot with legacy DTs.

Stephan argues the DTs were incomplete from the start and the breakage
is only a result of us previously abusing what's essentially undefined
behavior.. I think I second this, but it is *a* breakage so I want to
know your opinion.

FWIW the same happens when we have simple-framebuffer enabled and then
introduce dispcc on a given platform without adding the clocks under
the simplefb node and we've not been frowning upon that too much, so I'd
be willing to give it a pass if you're okay with it..

Not caring about this would make things far, far easier really..

Konrad
> 
>> I suppose something like this would work-ish:
>>
>> 0. remove clock handles as they're now contained within icc and
>>    use them as a "legacy marker"
>> 1. add:
>> 	if (qp->bus_clocks)
>> 		// skip qos writes
> 
> Maybe you can just check if all necessary clocks for QOS are there or
> not? I don't think it's a problem to skip it on broken DTs. I think it
> would be even fine to refuse loading the interconnect driver completely
> and just have the standard max vote (as long as that results in a
> booting system).
> 
>>
>> This will:
>> - let us add is_enabled so that all RPM clocks bar XO_A will be cleaned up
>> - save massively on code complexity
>>
> 
> +1
> 
>> at the cost of retroactively removing features (QoS settings) for people
>> with old DTs and new kernels (don't tell Torvalds!)
>>
> 
> I doubt anyone will notice :p
> 
>> This DTB ABI stuff really gets in the way sometimes :/ We're only now
>> fixing up U-Boot to be able to use upstream Linux DTs and other than
>> that I think only OpenBSD uses it with 8280.. Wish we could get rid of
>> all old junk once and then establish immutability but oh well..
> 
> Nice, thanks a lot for working on addressing the Qualcomm DT mess in
> U-Boot. I've been meaning to work this myself for a long time but never
> found the time to start... :')
> 
> Thanks,
> Stephan