Re: [PATCH v11 3/7] dt-bindings: PCI: qcom: Use sdx55 reg description for ipq9574

Krzysztof Kozlowski
2025-03-11T07:55:05Z

On 11/03/2025 06:01, Varadarajan Narayanan wrote:
> On Mon, Mar 10, 2025 at 12:37:28PM +0100, Krzysztof Kozlowski wrote:
>> On 10/03/2025 08:44, Varadarajan Narayanan wrote:
>>> On Thu, Mar 06, 2025 at 01:06:13PM +0100, Krzysztof Kozlowski wrote:
>>>> On 06/03/2025 12:52, Krzysztof Kozlowski wrote:
>>>>> On 20/02/2025 10:42, Varadarajan Narayanan wrote:
>>>>>> All DT entries except "reg" is similar between ipq5332 and ipq9574. ipq9574
>>>>>> has 5 registers while ipq5332 has 6. MHI is the additional (i.e. sixth
>>>>>> entry). Since this matches with the sdx55's "reg" definition which allows
>>>>>> for 5 or 6 registers, combine ipq9574 with sdx55.
>>>>>>
>>>>>> This change is to prepare ipq9574 to be used as ipq5332's fallback
>>>>>> compatible.
>>>>>>
>>>>>> Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>
>>>>> Unreviewed.
>>>>>
>>>>>> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>>>>>> ---
>>>>>> v8: Add 'Reviewed-by: Krzysztof Kozlowski'
>>>>>> ---
>>>>>> Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>>> index 7235d6554cfb..4b4927178abc 100644
>>>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>>>> @@ -169,7 +169,6 @@ allOf:
>>>>>> enum:
>>>>>> - qcom,pcie-ipq6018
>>>>>> - qcom,pcie-ipq8074-gen3
>>>>>> - - qcom,pcie-ipq9574
>>>>>
>>>>> Why you did not explain that you are going to affect users of DTS?
>>>>>
>>>>> NAK
>>>
>>> Sorry for not explicitly calling this out. I thought that would be seen from the
>>> following DTS related patches.
>>>
>>>> I did not connect the dots, but I pointed out that you break users and
>>>> your DTS is wrong:
>>>> https://lore.kernel.org/all/f7551daa-cce5-47b3-873f-21b9c5026ed2@kernel.org/
>>>>
>>>> so you should come back with questions to clarify what to do, not keep
>>>> pushing this incorrect patchset.
>>>>
>>>> My bad, I should really have zero trust.
>>>
>>> It looks like it is not possible to have ipq9574 as fallback (for ipq5332)
>>> without making changes to ipq9574 since the "reg" constraint is different
>>> between the two. And this in turn would break the ABI w.r.t. ipq9574.
>>
>> I don't get why this is not possible. You have one list for ipq9574 and
>> existing compatible devices, and you add second list for new device.
>>
>> ... or you just keep existing order. Why you need to keep changing order
>> every time you add new device?
>
> Presently, sdx55 and ipq9574 have the following reg/reg-names constraints.
>
> compatible | qcom,pcie-sdx55 | qcom,pcie-ipq9574
> ----------------+-----------------------+------------------
> reg minItems| 5 | 5
> maxItems| 6 | 5
> ----------------+-----------------------+------------------
> reg-names | |
> minItems| 5 | 5
> ----------------+-----------------------+------------------
> maxItems| | 5 (6 for ipq5332)
> ----------------+-----------------------+------------------
> items | |
> | parf | dbi
> | dbi | elbi
> | elbi | atu
> | atu | parf
> | config | config
> | mhi | (add mhi for ipq5332)
> ----------------+-----------------------+------------------
>
> To make ipq9574 as fallback for ipq5332, have to add "mhi" to reg-names of
> ipq9574.

only ipq5332 gets additional item, not ipq9574. Your sentence is not
correct. You do not have to add mhi to ipq9574. Neither we, nor schema
asked you to do this.

> Once I add that, the sdx55 and ipq9574 is the same list but in
> different order.
>

You cannot change the order in existing devices.

> If this would not be considered as duplication of the same constraint, then I
> can club ipq5332 with ipq9574.
>
> If this would be considered as duplication, then sdx55 and ipq9574 would have to
> use the same reg-names list and sdx55 or ipq9574 reg-names order would change.
>
>>> To overcome this, two approaches seem to be availabe
>>>
>>> 1. Document that ipq9574 is impacted and rework these patches to
>>> minimize the impact as much as possible
>>
>> What impact? What is the reason to impact ipq9574? What is the actual issue?
>
> By impact, I meant the change in the reg-names order as mentioned above (for
> considered as duplication).

Then you must eliminate the impact, not minimize it.

>
>>> (or)
>>>
>>> 2. Handle ipq5332 as a separate compatible (without fallback) and reuse
>>> the constraints of sdx55 for "reg" and ipq9574 for the others (like
>>> clock etc.). This approach will also have to revert [1], as it
>>> assumes ipq9574 as fallback.
>>>
>>> Please advice which of the above would be appropriate. If there is a better 3rd
>>> alternative please let me know, will align with that approach.
>>
>> Keep existing order. Why every time we see new device, it comes up with
>> a different order?
>
> Will be able to do that based on the answer to 'duplication' question and how to
> handle that.

I don't understand what is duplication of something here.

>
> if (adding mhi to ipq9574 reg-names != duplication)
>
> /* Keep existing order */
>
> * Append "mhi" to ipq9574

ipq9574 does not have mhi, does it?

If it has, it should be separate patch with its own explanation of the
hardware.

Best regards,
Krzysztof