Re: [PATCH 1/3] dt-bindings: interconnect: add clocks property to enable QoS on sa8775p

[Krzysztof Kozlowski <krzk@kernel.org>]

On 20/08/2025 10:51, Odelu Kukatla wrote:
> 
> 
> On 8/13/2025 11:32 AM, Krzysztof Kozlowski wrote:
>> On 13/08/2025 07:55, Odelu Kukatla wrote:
>>>
>>>
>>> On 8/12/2025 3:47 PM, Krzysztof Kozlowski wrote:
>>>> On 08/08/2025 16:02, Odelu Kukatla wrote:
>>>>> Add reg and clocks properties to enable the clocks required
>>>>> for accessing QoS configuration.
>>>>
>>>>
>>>> Nothing here explains why EXISTING hardware is being changed. I also
>>>> remember big discussions and big confusing patches regarding sa8775p
>>>> (its rename, dropping/changing all providers), and this patch feels like
>>>> pieces of it without proper justification.
>>>>
>>> Thanks for the review.
>>> I have added description in cover letter, i will add here as well in next revision.> And this is hidden ABI break, no justification, no mentioning either.
>>>> Again we are discussing basics of ABI breaking patches?
>>>>
>>> If you are talking ABI break if we load old DT which may lead to crash, we have .qos_requires_clocks flag which takes care of skipping QoS if required clocks are not enabled.we have addressed this issue through https://lore.kernel.org/all/20240704125515.22194-1-quic_okukatla@quicinc.com/ 
>>
>> Format your emails correctly, it's difficult to read.
>>
>> Your binding did not require reg and clocks. Now it requires reg and
>> clocks. This is called ABI break.
>>
>> Please follow Qualcomm extensive upstreaming guide, it explains this,
>> doesn't it? Or follow writing bindings...
>>
> 
> Thanks for your review and guidance.
> 
> I agree that adding reg and clocks properties to existing bindings is an
> ABI break. The sa8775p is a relatively older platform, and when the
> interconnect provider driver was initially upstreamed, QoS configuration
> support was not available in the framework. As a result, QoS was not
> enabled at that time.


That's irrelevant reason. Writing bindings since long time ask pretty
clearly to describe hardware completely, regardless whether Linux
supports this or not.

It does not matter if you enable QoS or not.

> 
> The motivation for this change is that certain interconnect paths on
> sa8775p require specific clocks to be enabled to access QoS registers.

This does not look at all like existing device is completely broken.

You just add new feature, so no ABI break.

> QoS configuration is essential for managing latency and bandwidth across
> subsystems such as CPU, GPU, and multimedia engines. Without it, the
> system may experience performance degradation, especially under

So how was it working for the last 2 years?


> concurrent workloads. Enabling QoS improves system responsiveness and
> ensures more predictable behavior in high-throughput scenarios.



Best regards,
Krzysztof