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
