Date:   Wed, 19 Apr 2023 16:00:34 +0200
From:   Stephan Gerhold <stephan@...hold.net>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>
Cc:     Stephen Boyd <sboyd@...nel.org>, Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        devicetree@...r.kernel.org
Subject: Re: [PATCH RFT v2 01/14] dt-bindings: clock: qcom,rpmcc: Add a way
 to enable unused clock cleanup

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.

> 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

Powered by blists - more mailing lists