Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <837facd5-2b8e-2bad-6b62-9550656a2dad@kernel.org>
Date:   Thu, 28 Apr 2022 14:20:36 +0300
MIME-Version: 1.0
Subject: Re: [PATCH] interconnect: qcom: use icc_sync_state
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        linux-arm-msm@vger.kernel.org, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220427145616.523557-1-krzysztof.kozlowski@linaro.org>
 <4769c796-6edd-c23a-ee2a-ce54495548f7@kernel.org>
 <e77d9ff9-67a5-e0f3-8ad8-848342ed4dfb@linaro.org>
From:   Georgi Djakov <djakov@kernel.org>
In-Reply-To: <e77d9ff9-67a5-e0f3-8ad8-848342ed4dfb@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 28.04.22 13:23, Krzysztof Kozlowski wrote:
> On 27/04/2022 17:34, Georgi Djakov wrote:
>> On 27.04.22 17:56, Krzysztof Kozlowski wrote:
>>> Use icc_sync_state for interconnect providers, so that the bandwidth
>>> request doesn't need to stay on maximum value.
>>
>> Did you test this? In general, we should not enable this on boards that
>> do not have full interconnect scaling support in consumer drivers yet.
>> Some of the interconnects could be enabled by default by the bootloader
>> and usually later during boot the consumer drivers request the bandwidth
>> that they need. But if the requests are missing, the interconnects
>> without bandwidth users will be disabled when we reach sync state. So
>> this may (or not) cause issues...
> 
> I understand, thanks for bringing this up. It does not look like an
> issue of interconnect provider but instead consumers and DTS. It's not
> the job of provider driver to know all possible uses and DTS files. The
> driver should expose itself and if platform is not ready, should not use
> it by not enabling the interconnect. It's a job for DTS, not for the
> interconnect provider.

Agree, but we still need to make sure this is tested and does not
introduce any regressions at least with the DT that is upstream.

> Imagine some out of tree DTS which cannot use interconnects because we
> assume that all users of that provider are missing bandwidth requests.
> No, instead provider should allow anyone to use it.

I have an idea to introduce a kernel parameter like clk_ignore_unused,
but for interconnects.

> I understand my change might cause unexpected issues, but it is still
> technically correct, just maybe should be followed with disabling in DTS
> the providers without proper consumers?

Not sure that enabling/disabling stuff in DT is a good option. DT should
be just a description of the hardware and it might not be updated as often
as the kernel.

Thanks,
Georgi