Re: [PATCH 2/2] interconnect: qcom: qcm2290: Enable sync state

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 23.07.2023 13:01, Stephan Gerhold wrote:
> On Fri, Jul 21, 2023 at 02:03:16PM +0200, Konrad Dybcio wrote:
>> On 20.07.2023 21:52, Stephan Gerhold wrote:
>>> On Thu, Jul 20, 2023 at 08:24:01PM +0200, Konrad Dybcio wrote:
>>>> Very surprisingly, qcm2290 does not seem to require any interface
>>>> clocks.
>>>
>>> What does this mean exactly? The interconnect .sync_state() is
>>> responsible to drop the initial maximum bandwidth votes, with the
>>> assumption that all active devices have voted for the bandwidth they
>>> need. How does this relate to "requiring interface clocks"?
>> If it required such clocks to be present, sync_state could not
>> complete, as trying to access some nodes would crash the platform
>> due to unclocked access.
> 
> You mean something like the IPA clock that must be active to do the QoS
> writes?
> 
> Wouldn't it already crash before .sync_state() then, when the initial
> max bandwidth votes are being made?
No, the bandwidth votes are fully handled by RPM with no external deps.

Only accessing the QoS registers from the AP seems to trigger crashes
on unclocked access. But surprisingly, not on this SoC.

>>>> It's therefore safe to enable sync_state to park unused devices.
>>>> Do so.
>>>
>>> Doesn't this make everything painfully slow? There are no interconnect
>>> consumers at all in qcm2290.dtsi. I would expect that all bandwidths
>>> end up at minimum.
>> There are no interconnect providers defined in qcm2290.dtsi.
> 
> Ack, so I guess you're going to add them together with the actual
> consumers?
Correct.

> 
> I think the patch itself is fine. Only the commit message is a bit
> misleading. The actual change that is being done here is enabling the
> bandwidth scaling (dropping the max bandwidth votes after
> .sync_state()). Can you try to clarify the commit message a bit?
Yes, I'll resend.

Konrad