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

[Varadarajan Narayanan]

On Tue, Mar 11, 2025 at 08:54:55AM +0100, Krzysztof Kozlowski wrote:
> 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@xxxxxxxxxx>
> >>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> >>>>>
> >>>>> Unreviewed.
> >>>>>
> >>>>>> Signed-off-by: Varadarajan Narayanan <quic_varada@xxxxxxxxxxx>
> >>>>>> ---
> >>>>>> 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@xxxxxxxxxx/
> >>>>
> >>>> 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.

Ok.

> > 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.

Ok.

> >>> 		(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.

By duplication, I meant the same set of reg-names (in different order).

> > 	if (adding mhi to ipq9574 reg-names != duplication)
> >
> > 		/* Keep existing order */
> >
> > 		* Append "mhi" to ipq9574
>
> ipq9574 does not have mhi, does it?

ipq9574 also has it.

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

Will discard these patches from the patchset -

	dt-bindings: PCI: qcom: Use sdx55 reg description for ipq9574 Varadarajan Narayanan
	arm64: dts: qcom: ipq9574: Reorder reg and reg-names Varadarajan Narayanan

Will add mhi for ipq9574 and post the next version. Is that ok?

Thanks
Varada