[PATCH 1/1] arm64: dts: lx2160a: Change PCIe compatible string to fsl,ls2088a-pcie

[PATCH 1/1] arm64: dts: lx2160a: Change PCIe compatible string to fsl,ls2088a-pcie

[Frank Li]

The mass production lx2160 rev2 use designware PCIe Controller. Old Rev1
which use mobivel PCIe controller was not supported. Although uboot
fixup can change compatible string fsl,lx2160a-pcie to fsl,ls2088a-pcie
since 2019, it is quite confused and should correctly reflect hardware
status in fsl-lx2160a.dtsi.

- Rename fsl,lx2160a-pcie to fsl,ls2088a-pcie
- Only keep intr interrupt align binding doc
- Remove unused property apio-wins, ppio-wins
- Rename reg-names
- Add IO map range

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 .../arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 102 +++++++++---------
 1 file changed, 48 insertions(+), 54 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
index 927ecf66a7404..386b4fcfa16e6 100644
--- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
@@ -1166,22 +1166,21 @@ sata3: sata@3230000 {
 		};
 
 		pcie1: pcie@3400000 {
-			compatible = "fsl,lx2160a-pcie";
+			compatible = "fsl,ls2088a-pcie";
 			reg = <0x00 0x03400000 0x0 0x00100000>, /* controller registers */
 			      <0x80 0x00000000 0x0 0x00002000>; /* configuration space */
-			reg-names = "csr_axi_slave", "config_axi_slave";
-			interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>, /* AER interrupt */
-				     <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>, /* PME interrupt */
-				     <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */
-			interrupt-names = "aer", "pme", "intr";
+			reg-names = "regs", "config";
+			interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */
+			interrupt-names = "intr";
 			#address-cells = <3>;
 			#size-cells = <2>;
 			device_type = "pci";
 			dma-coherent;
-			apio-wins = <8>;
-			ppio-wins = <8>;
 			bus-range = <0x0 0xff>;
-			ranges = <0x82000000 0x0 0x40000000 0x80 0x40000000 0x0 0x40000000>; /* non-prefetchable memory */
+			ranges = /* downstream I/O */
+				 <0x81000000 0x0 0x00000000 0x80 0x00010000 0x0 0x00010000>,
+				 /* non-prefetchable memory */
+				 <0x82000000 0x0 0x40000000 0x80 0x40000000 0x0 0x40000000>;
 			msi-parent = <&its 0>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
@@ -1194,22 +1193,21 @@ pcie1: pcie@3400000 {
 		};
 
 		pcie2: pcie@3500000 {
-			compatible = "fsl,lx2160a-pcie";
+			compatible = "fsl,ls2088a-pcie";
 			reg = <0x00 0x03500000 0x0 0x00100000>, /* controller registers */
 			      <0x88 0x00000000 0x0 0x00002000>; /* configuration space */
-			reg-names = "csr_axi_slave", "config_axi_slave";
-			interrupts = <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>, /* AER interrupt */
-				     <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>, /* PME interrupt */
-				     <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */
-			interrupt-names = "aer", "pme", "intr";
+			reg-names = "regs", "config";
+			interrupts = <GIC_SPI 113 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */
+			interrupt-names = "intr";
 			#address-cells = <3>;
 			#size-cells = <2>;
 			device_type = "pci";
 			dma-coherent;
-			apio-wins = <8>;
-			ppio-wins = <8>;
 			bus-range = <0x0 0xff>;
-			ranges = <0x82000000 0x0 0x40000000 0x88 0x40000000 0x0 0x40000000>; /* non-prefetchable memory */
+			ranges = /* downstream I/O */
+				 <0x81000000 0x0 0x00000000 0x88 0x00010000 0x0 0x00010000>,
+				 /* non-prefetchable memory */
+				 <0x82000000 0x0 0x40000000 0x88 0x40000000 0x0 0x40000000>;
 			msi-parent = <&its 0>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
@@ -1222,22 +1220,21 @@ pcie2: pcie@3500000 {
 		};
 
 		pcie3: pcie@3600000 {
-			compatible = "fsl,lx2160a-pcie";
+			compatible = "fsl,ls2088a-pcie";
 			reg = <0x00 0x03600000 0x0 0x00100000>, /* controller registers */
 			      <0x90 0x00000000 0x0 0x00002000>; /* configuration space */
-			reg-names = "csr_axi_slave", "config_axi_slave";
-			interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>, /* AER interrupt */
-				     <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>, /* PME interrupt */
-				     <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */
-			interrupt-names = "aer", "pme", "intr";
+			reg-names = "regs", "config";
+			interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */
+			interrupt-names = "intr";
 			#address-cells = <3>;
 			#size-cells = <2>;
 			device_type = "pci";
 			dma-coherent;
-			apio-wins = <256>;
-			ppio-wins = <24>;
 			bus-range = <0x0 0xff>;
-			ranges = <0x82000000 0x0 0x40000000 0x90 0x40000000 0x0 0x40000000>; /* non-prefetchable memory */
+			ranges = /* downstream I/O */
+				 <0x81000000 0x0 0x00000000 0x90 0x00010000 0x0 0x00010000>,
+				 /* non-prefetchable memory */
+				 <0x82000000 0x0 0x40000000 0x90 0x40000000 0x0 0x40000000>;
 			msi-parent = <&its 0>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
@@ -1250,22 +1247,21 @@ pcie3: pcie@3600000 {
 		};
 
 		pcie4: pcie@3700000 {
-			compatible = "fsl,lx2160a-pcie";
+			compatible = "fsl,ls2088a-pcie";
 			reg = <0x00 0x03700000 0x0 0x00100000>, /* controller registers */
 			      <0x98 0x00000000 0x0 0x00002000>; /* configuration space */
-			reg-names = "csr_axi_slave", "config_axi_slave";
-			interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>, /* AER interrupt */
-				     <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>, /* PME interrupt */
-				     <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */
-			interrupt-names = "aer", "pme", "intr";
+			reg-names = "regs", "config";
+			interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */
+			interrupt-names = "intr";
 			#address-cells = <3>;
 			#size-cells = <2>;
 			device_type = "pci";
 			dma-coherent;
-			apio-wins = <8>;
-			ppio-wins = <8>;
 			bus-range = <0x0 0xff>;
-			ranges = <0x82000000 0x0 0x40000000 0x98 0x40000000 0x0 0x40000000>; /* non-prefetchable memory */
+			ranges = /* downstream I/O */
+				 <0x81000000 0x0 0x00000000 0x98 0x00010000 0x0 0x00010000>,
+				 /* non-prefetchable memory */
+				 <0x82000000 0x0 0x40000000 0x98 0x40000000 0x0 0x40000000>;
 			msi-parent = <&its 0>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
@@ -1278,22 +1274,21 @@ pcie4: pcie@3700000 {
 		};
 
 		pcie5: pcie@3800000 {
-			compatible = "fsl,lx2160a-pcie";
+			compatible = "fsl,ls2088a-pcie";
 			reg = <0x00 0x03800000 0x0 0x00100000>, /* controller registers */
 			      <0xa0 0x00000000 0x0 0x00002000>; /* configuration space */
-			reg-names = "csr_axi_slave", "config_axi_slave";
-			interrupts = <GIC_SPI 128 IRQ_TYPE_LEVEL_HIGH>, /* AER interrupt */
-				     <GIC_SPI 128 IRQ_TYPE_LEVEL_HIGH>, /* PME interrupt */
-				     <GIC_SPI 128 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */
-			interrupt-names = "aer", "pme", "intr";
+			reg-names = "regs", "config";
+			interrupts = <GIC_SPI 128 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */
+			interrupt-names = "intr";
 			#address-cells = <3>;
 			#size-cells = <2>;
 			device_type = "pci";
 			dma-coherent;
-			apio-wins = <256>;
-			ppio-wins = <24>;
 			bus-range = <0x0 0xff>;
-			ranges = <0x82000000 0x0 0x40000000 0xa0 0x40000000 0x0 0x40000000>; /* non-prefetchable memory */
+			ranges = /* downstream I/O */
+				 <0x81000000 0x0 0x00000000 0xa0 0x00010000 0x0 0x00010000>,
+				 /* non-prefetchable memory */
+				 <0x82000000 0x0 0x40000000 0xa0 0x40000000 0x0 0x40000000>;
 			msi-parent = <&its 0>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
@@ -1306,22 +1301,21 @@ pcie5: pcie@3800000 {
 		};
 
 		pcie6: pcie@3900000 {
-			compatible = "fsl,lx2160a-pcie";
+			compatible = "fsl,ls2088a-pcie";
 			reg = <0x00 0x03900000 0x0 0x00100000>, /* controller registers */
 			      <0xa8 0x00000000 0x0 0x00002000>; /* configuration space */
-			reg-names = "csr_axi_slave", "config_axi_slave";
-			interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>, /* AER interrupt */
-				     <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>, /* PME interrupt */
-				     <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */
-			interrupt-names = "aer", "pme", "intr";
+			reg-names = "regs", "config";
+			interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>; /* controller interrupt */
+			interrupt-names = "intr";
 			#address-cells = <3>;
 			#size-cells = <2>;
 			device_type = "pci";
 			dma-coherent;
-			apio-wins = <8>;
-			ppio-wins = <8>;
 			bus-range = <0x0 0xff>;
-			ranges = <0x82000000 0x0 0x40000000 0xa8 0x40000000 0x0 0x40000000>; /* non-prefetchable memory */
+			ranges = /* downstream I/O */
+				 <0x81000000 0x0 0x00000000 0xa8 0x00010000 0x0 0x00010000>,
+				 /* non-prefetchable memory */
+				 <0x82000000 0x0 0x40000000 0xa8 0x40000000 0x0 0x40000000>;
 			msi-parent = <&its 0>;
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 7>;
-- 
2.34.1

Re: [PATCH 1/1] arm64: dts: lx2160a: Change PCIe compatible string to fsl,ls2088a-pcie

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 669 bytes --]

On Thu, Aug 08, 2024 at 11:31:20AM -0400, Frank Li wrote:
> The mass production lx2160 rev2 use designware PCIe Controller. Old Rev1
> which use mobivel PCIe controller was not supported. Although uboot
> fixup can change compatible string fsl,lx2160a-pcie to fsl,ls2088a-pcie
> since 2019, it is quite confused and should correctly reflect hardware
> status in fsl-lx2160a.dtsi.

This does not begin to explain why removing the soc-specific compatible,
and instead putting the compatible for another soc is the right fix.
Come up with a new compatible for this device, that perhaps falls back
to the ls2088a, but this change doesn't seem right to me.


Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/1] arm64: dts: lx2160a: Change PCIe compatible string to fsl,ls2088a-pcie

[Frank Li]

On Thu, Aug 08, 2024 at 04:34:32PM +0100, Conor Dooley wrote:
> On Thu, Aug 08, 2024 at 11:31:20AM -0400, Frank Li wrote:
> > The mass production lx2160 rev2 use designware PCIe Controller. Old Rev1
> > which use mobivel PCIe controller was not supported. Although uboot
> > fixup can change compatible string fsl,lx2160a-pcie to fsl,ls2088a-pcie
> > since 2019, it is quite confused and should correctly reflect hardware
> > status in fsl-lx2160a.dtsi.
>
> This does not begin to explain why removing the soc-specific compatible,
> and instead putting the compatible for another soc is the right fix.
> Come up with a new compatible for this device, that perhaps falls back
> to the ls2088a, but this change doesn't seem right to me.

It can't fallback to fsl,ls2088a-pcie if fsl,lx2160a-pcie exist, which are
totally imcompatible between fsl,ls2088a-pcie and fsl,lx2160a-pcie.

Previous dtb can work just because uboot dynamtic change fsl,lx2160a-pcie
to fsl,ls2088a-pcie when boot kernel.

fsl,lx2160a-pcie should be removed because Rev1 have not mass productioned.

Frank
>
>
> Cheers,
> Conor.

Re: [PATCH 1/1] arm64: dts: lx2160a: Change PCIe compatible string to fsl,ls2088a-pcie

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

On Thu, Aug 08, 2024 at 11:51:34AM -0400, Frank Li wrote:
> On Thu, Aug 08, 2024 at 04:34:32PM +0100, Conor Dooley wrote:
> > On Thu, Aug 08, 2024 at 11:31:20AM -0400, Frank Li wrote:
> > > The mass production lx2160 rev2 use designware PCIe Controller. Old Rev1
> > > which use mobivel PCIe controller was not supported. Although uboot
> > > fixup can change compatible string fsl,lx2160a-pcie to fsl,ls2088a-pcie
> > > since 2019, it is quite confused and should correctly reflect hardware
> > > status in fsl-lx2160a.dtsi.
> >
> > This does not begin to explain why removing the soc-specific compatible,
> > and instead putting the compatible for another soc is the right fix.
> > Come up with a new compatible for this device, that perhaps falls back
> > to the ls2088a, but this change doesn't seem right to me.
> 
> It can't fallback to fsl,ls2088a-pcie if fsl,lx2160a-pcie exist, which are
> totally imcompatible between fsl,ls2088a-pcie and fsl,lx2160a-pcie.
> 
> Previous dtb can work just because uboot dynamtic change fsl,lx2160a-pcie
> to fsl,ls2088a-pcie when boot kernel.
> 
> fsl,lx2160a-pcie should be removed because Rev1 have not mass productioned.

Please re-read what I wrote. I said to come up with a new compatible for
this device, not fall back from the existing fsl,lx2160a-pcie to
fsl,ls2088a-pcie.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/1] arm64: dts: lx2160a: Change PCIe compatible string to fsl,ls2088a-pcie

[Frank Li]

On Thu, Aug 08, 2024 at 04:55:14PM +0100, Conor Dooley wrote:
> On Thu, Aug 08, 2024 at 11:51:34AM -0400, Frank Li wrote:
> > On Thu, Aug 08, 2024 at 04:34:32PM +0100, Conor Dooley wrote:
> > > On Thu, Aug 08, 2024 at 11:31:20AM -0400, Frank Li wrote:
> > > > The mass production lx2160 rev2 use designware PCIe Controller. Old Rev1
> > > > which use mobivel PCIe controller was not supported. Although uboot
> > > > fixup can change compatible string fsl,lx2160a-pcie to fsl,ls2088a-pcie
> > > > since 2019, it is quite confused and should correctly reflect hardware
> > > > status in fsl-lx2160a.dtsi.
> > >
> > > This does not begin to explain why removing the soc-specific compatible,
> > > and instead putting the compatible for another soc is the right fix.
> > > Come up with a new compatible for this device, that perhaps falls back
> > > to the ls2088a, but this change doesn't seem right to me.
> >
> > It can't fallback to fsl,ls2088a-pcie if fsl,lx2160a-pcie exist, which are
> > totally imcompatible between fsl,ls2088a-pcie and fsl,lx2160a-pcie.
> >
> > Previous dtb can work just because uboot dynamtic change fsl,lx2160a-pcie
> > to fsl,ls2088a-pcie when boot kernel.
> >
> > fsl,lx2160a-pcie should be removed because Rev1 have not mass productioned.
>
> Please re-read what I wrote. I said to come up with a new compatible for
> this device, not fall back from the existing fsl,lx2160a-pcie to
> fsl,ls2088a-pcie.

According to my understand, It needn't add new compatible string if nothing
difference. for example, it use fsl,vf610-i2c for all i2c without add
new soc-specific fsl,lx2160-i2c.

So far lx2160a-pcie is the same as ls2088a-pcie.

Frank

>
> Thanks,
> Conor.

Re: [PATCH 1/1] arm64: dts: lx2160a: Change PCIe compatible string to fsl,ls2088a-pcie

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]

On Thu, Aug 08, 2024 at 12:15:03PM -0400, Frank Li wrote:
> On Thu, Aug 08, 2024 at 04:55:14PM +0100, Conor Dooley wrote:
> > On Thu, Aug 08, 2024 at 11:51:34AM -0400, Frank Li wrote:
> > > On Thu, Aug 08, 2024 at 04:34:32PM +0100, Conor Dooley wrote:
> > > > On Thu, Aug 08, 2024 at 11:31:20AM -0400, Frank Li wrote:
> > > > > The mass production lx2160 rev2 use designware PCIe Controller. Old Rev1
> > > > > which use mobivel PCIe controller was not supported. Although uboot
> > > > > fixup can change compatible string fsl,lx2160a-pcie to fsl,ls2088a-pcie
> > > > > since 2019, it is quite confused and should correctly reflect hardware
> > > > > status in fsl-lx2160a.dtsi.
> > > >
> > > > This does not begin to explain why removing the soc-specific compatible,
> > > > and instead putting the compatible for another soc is the right fix.
> > > > Come up with a new compatible for this device, that perhaps falls back
> > > > to the ls2088a, but this change doesn't seem right to me.
> > >
> > > It can't fallback to fsl,ls2088a-pcie if fsl,lx2160a-pcie exist, which are
> > > totally imcompatible between fsl,ls2088a-pcie and fsl,lx2160a-pcie.
> > >
> > > Previous dtb can work just because uboot dynamtic change fsl,lx2160a-pcie
> > > to fsl,ls2088a-pcie when boot kernel.
> > >
> > > fsl,lx2160a-pcie should be removed because Rev1 have not mass productioned.
> >
> > Please re-read what I wrote. I said to come up with a new compatible for
> > this device, not fall back from the existing fsl,lx2160a-pcie to
> > fsl,ls2088a-pcie.
> 
> According to my understand, It needn't add new compatible string if nothing
> difference. for example, it use fsl,vf610-i2c for all i2c without add
> new soc-specific fsl,lx2160-i2c.

No, you should have soc-specific compatibles regardless. Just because
you got away with it once, doesn't mean I'm not going to complain about
it here!

> So far lx2160a-pcie is the same as ls2088a-pcie.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/1] arm64: dts: lx2160a: Change PCIe compatible string to fsl,ls2088a-pcie

[Frank Li]

On Fri, Aug 09, 2024 at 04:07:25PM +0100, Conor Dooley wrote:
> On Thu, Aug 08, 2024 at 12:15:03PM -0400, Frank Li wrote:
> > On Thu, Aug 08, 2024 at 04:55:14PM +0100, Conor Dooley wrote:
> > > On Thu, Aug 08, 2024 at 11:51:34AM -0400, Frank Li wrote:
> > > > On Thu, Aug 08, 2024 at 04:34:32PM +0100, Conor Dooley wrote:
> > > > > On Thu, Aug 08, 2024 at 11:31:20AM -0400, Frank Li wrote:
> > > > > > The mass production lx2160 rev2 use designware PCIe Controller. Old Rev1
> > > > > > which use mobivel PCIe controller was not supported. Although uboot
> > > > > > fixup can change compatible string fsl,lx2160a-pcie to fsl,ls2088a-pcie
> > > > > > since 2019, it is quite confused and should correctly reflect hardware
> > > > > > status in fsl-lx2160a.dtsi.
> > > > >
> > > > > This does not begin to explain why removing the soc-specific compatible,
> > > > > and instead putting the compatible for another soc is the right fix.
> > > > > Come up with a new compatible for this device, that perhaps falls back
> > > > > to the ls2088a, but this change doesn't seem right to me.
> > > >
> > > > It can't fallback to fsl,ls2088a-pcie if fsl,lx2160a-pcie exist, which are
> > > > totally imcompatible between fsl,ls2088a-pcie and fsl,lx2160a-pcie.
> > > >
> > > > Previous dtb can work just because uboot dynamtic change fsl,lx2160a-pcie
> > > > to fsl,ls2088a-pcie when boot kernel.
> > > >
> > > > fsl,lx2160a-pcie should be removed because Rev1 have not mass productioned.
> > >
> > > Please re-read what I wrote. I said to come up with a new compatible for
> > > this device, not fall back from the existing fsl,lx2160a-pcie to
> > > fsl,ls2088a-pcie.
> >
> > According to my understand, It needn't add new compatible string if nothing
> > difference. for example, it use fsl,vf610-i2c for all i2c without add
> > new soc-specific fsl,lx2160-i2c.
>
> No, you should have soc-specific compatibles regardless. Just because
> you got away with it once, doesn't mean I'm not going to complain about
> it here!

Rob:
	What's current policy for this? Not only for this one. If new SOC
appear such as iMX10 (maybe many derived chip i.MX101, i.MX102...), there
are bunch of IPs, Do we need add fsl,imx10* for everyone, which most part
is exactly the same as old one and bloat binding doc.

	I remember that I got a feedback that required provide the
difference during I try to add new compatible string. I am sorry, I can't
find origial dicussion thread.

	Anyways, a clear guide line will help us greatly.

Frank

>
> > So far lx2160a-pcie is the same as ls2088a-pcie.
>

Re: [PATCH 1/1] arm64: dts: lx2160a: Change PCIe compatible string to fsl,ls2088a-pcie

[Krzysztof Kozlowski]

On 09/08/2024 19:11, Frank Li wrote:
> On Fri, Aug 09, 2024 at 04:07:25PM +0100, Conor Dooley wrote:
>> On Thu, Aug 08, 2024 at 12:15:03PM -0400, Frank Li wrote:
>>> On Thu, Aug 08, 2024 at 04:55:14PM +0100, Conor Dooley wrote:
>>>> On Thu, Aug 08, 2024 at 11:51:34AM -0400, Frank Li wrote:
>>>>> On Thu, Aug 08, 2024 at 04:34:32PM +0100, Conor Dooley wrote:
>>>>>> On Thu, Aug 08, 2024 at 11:31:20AM -0400, Frank Li wrote:
>>>>>>> The mass production lx2160 rev2 use designware PCIe Controller. Old Rev1
>>>>>>> which use mobivel PCIe controller was not supported. Although uboot
>>>>>>> fixup can change compatible string fsl,lx2160a-pcie to fsl,ls2088a-pcie
>>>>>>> since 2019, it is quite confused and should correctly reflect hardware
>>>>>>> status in fsl-lx2160a.dtsi.
>>>>>>
>>>>>> This does not begin to explain why removing the soc-specific compatible,
>>>>>> and instead putting the compatible for another soc is the right fix.
>>>>>> Come up with a new compatible for this device, that perhaps falls back
>>>>>> to the ls2088a, but this change doesn't seem right to me.
>>>>>
>>>>> It can't fallback to fsl,ls2088a-pcie if fsl,lx2160a-pcie exist, which are
>>>>> totally imcompatible between fsl,ls2088a-pcie and fsl,lx2160a-pcie.
>>>>>
>>>>> Previous dtb can work just because uboot dynamtic change fsl,lx2160a-pcie
>>>>> to fsl,ls2088a-pcie when boot kernel.
>>>>>
>>>>> fsl,lx2160a-pcie should be removed because Rev1 have not mass productioned.
>>>>
>>>> Please re-read what I wrote. I said to come up with a new compatible for
>>>> this device, not fall back from the existing fsl,lx2160a-pcie to
>>>> fsl,ls2088a-pcie.
>>>
>>> According to my understand, It needn't add new compatible string if nothing
>>> difference. for example, it use fsl,vf610-i2c for all i2c without add
>>> new soc-specific fsl,lx2160-i2c.
>>
>> No, you should have soc-specific compatibles regardless. Just because
>> you got away with it once, doesn't mean I'm not going to complain about
>> it here!
> 

Above... and here:
https://lore.kernel.org/all/20220817202538.21493-2-leoyang.li@nxp.com/

Uh, this is so confusing. You have fsl,lx2160a device with PCIe and
fsl,lx2160a-pcie compatible. You claim that these are wrong. Instead of
fixing driver, you use entirely different device's compatible?

Wow, that's confusing.

> Rob:
> 	What's current policy for this? Not only for this one. If new SOC
> appear such as iMX10 (maybe many derived chip i.MX101, i.MX102...), there
> are bunch of IPs, Do we need add fsl,imx10* for everyone, which most part
> is exactly the same as old one and bloat binding doc.


NXP since early days was following this approach of having specific
compatibles, so why changing it now?

In general you need specific front-compatibles, except for different
pinout or fused values.

But that's not the problem here. Earlier confusion is the problem. This
is very weird change.

> 
> 	I remember that I got a feedback that required provide the
> difference during I try to add new compatible string. I am sorry, I can't
> find origial dicussion thread.
> 


Best regards,
Krzysztof

Re: [PATCH 1/1] arm64: dts: lx2160a: Change PCIe compatible string to fsl,ls2088a-pcie

[Rob Herring]

On Fri, Aug 9, 2024 at 11:11 AM Frank Li <Frank.li@nxp.com> wrote:
>
> On Fri, Aug 09, 2024 at 04:07:25PM +0100, Conor Dooley wrote:
> > On Thu, Aug 08, 2024 at 12:15:03PM -0400, Frank Li wrote:
> > > On Thu, Aug 08, 2024 at 04:55:14PM +0100, Conor Dooley wrote:
> > > > On Thu, Aug 08, 2024 at 11:51:34AM -0400, Frank Li wrote:
> > > > > On Thu, Aug 08, 2024 at 04:34:32PM +0100, Conor Dooley wrote:
> > > > > > On Thu, Aug 08, 2024 at 11:31:20AM -0400, Frank Li wrote:
> > > > > > > The mass production lx2160 rev2 use designware PCIe Controller. Old Rev1
> > > > > > > which use mobivel PCIe controller was not supported. Although uboot
> > > > > > > fixup can change compatible string fsl,lx2160a-pcie to fsl,ls2088a-pcie
> > > > > > > since 2019, it is quite confused and should correctly reflect hardware
> > > > > > > status in fsl-lx2160a.dtsi.
> > > > > >
> > > > > > This does not begin to explain why removing the soc-specific compatible,
> > > > > > and instead putting the compatible for another soc is the right fix.
> > > > > > Come up with a new compatible for this device, that perhaps falls back
> > > > > > to the ls2088a, but this change doesn't seem right to me.
> > > > >
> > > > > It can't fallback to fsl,ls2088a-pcie if fsl,lx2160a-pcie exist, which are
> > > > > totally imcompatible between fsl,ls2088a-pcie and fsl,lx2160a-pcie.
> > > > >
> > > > > Previous dtb can work just because uboot dynamtic change fsl,lx2160a-pcie
> > > > > to fsl,ls2088a-pcie when boot kernel.
> > > > >
> > > > > fsl,lx2160a-pcie should be removed because Rev1 have not mass productioned.
> > > >
> > > > Please re-read what I wrote. I said to come up with a new compatible for
> > > > this device, not fall back from the existing fsl,lx2160a-pcie to
> > > > fsl,ls2088a-pcie.
> > >
> > > According to my understand, It needn't add new compatible string if nothing
> > > difference. for example, it use fsl,vf610-i2c for all i2c without add
> > > new soc-specific fsl,lx2160-i2c.
> >
> > No, you should have soc-specific compatibles regardless. Just because
> > you got away with it once, doesn't mean I'm not going to complain about
> > it here!
>
> Rob:
>         What's current policy for this? Not only for this one. If new SOC
> appear such as iMX10 (maybe many derived chip i.MX101, i.MX102...), there
> are bunch of IPs, Do we need add fsl,imx10* for everyone, which most part
> is exactly the same as old one and bloat binding doc.

Yes, you do. Do you really know that something in the design hasn't
changed? Have you compared the RTL between the versions? The only way
to deal with quirks without changing the DT everytime is by having
specific compatibles *upfront*.

The "bloat" is never that much because the IP really always changes.
QCom wanted to (and did) use IP version numbers for the same reasons.
Guess what, the IP version number changed on almost every SoC.

The exceptions are really if different SoCs are just different
packaging or fusing.


In this case, I'm inclined to say just match what u-boot creates, but
please make that abundantly clear with a comment in the .dts file and
explain the situation in the commit message. OTOH, just adding a new
"fsl,lx2160a-dw-pcie" compatible with "fsl,ls2088a-pcie" fallback
doesn't hurt, and we can just move on from creating a special case.

Rob

Re: [PATCH 1/1] arm64: dts: lx2160a: Change PCIe compatible string to fsl,ls2088a-pcie

[Frank Li]

On Mon, Aug 12, 2024 at 11:29:38AM -0600, Rob Herring wrote:
> On Fri, Aug 9, 2024 at 11:11 AM Frank Li <Frank.li@nxp.com> wrote:
> >
> > On Fri, Aug 09, 2024 at 04:07:25PM +0100, Conor Dooley wrote:
> > > On Thu, Aug 08, 2024 at 12:15:03PM -0400, Frank Li wrote:
> > > > On Thu, Aug 08, 2024 at 04:55:14PM +0100, Conor Dooley wrote:
> > > > > On Thu, Aug 08, 2024 at 11:51:34AM -0400, Frank Li wrote:
> > > > > > On Thu, Aug 08, 2024 at 04:34:32PM +0100, Conor Dooley wrote:
> > > > > > > On Thu, Aug 08, 2024 at 11:31:20AM -0400, Frank Li wrote:
> > > > > > > > The mass production lx2160 rev2 use designware PCIe Controller. Old Rev1
> > > > > > > > which use mobivel PCIe controller was not supported. Although uboot
> > > > > > > > fixup can change compatible string fsl,lx2160a-pcie to fsl,ls2088a-pcie
> > > > > > > > since 2019, it is quite confused and should correctly reflect hardware
> > > > > > > > status in fsl-lx2160a.dtsi.
> > > > > > >
> > > > > > > This does not begin to explain why removing the soc-specific compatible,
> > > > > > > and instead putting the compatible for another soc is the right fix.
> > > > > > > Come up with a new compatible for this device, that perhaps falls back
> > > > > > > to the ls2088a, but this change doesn't seem right to me.
> > > > > >
> > > > > > It can't fallback to fsl,ls2088a-pcie if fsl,lx2160a-pcie exist, which are
> > > > > > totally imcompatible between fsl,ls2088a-pcie and fsl,lx2160a-pcie.
> > > > > >
> > > > > > Previous dtb can work just because uboot dynamtic change fsl,lx2160a-pcie
> > > > > > to fsl,ls2088a-pcie when boot kernel.
> > > > > >
> > > > > > fsl,lx2160a-pcie should be removed because Rev1 have not mass productioned.
> > > > >
> > > > > Please re-read what I wrote. I said to come up with a new compatible for
> > > > > this device, not fall back from the existing fsl,lx2160a-pcie to
> > > > > fsl,ls2088a-pcie.
> > > >
> > > > According to my understand, It needn't add new compatible string if nothing
> > > > difference. for example, it use fsl,vf610-i2c for all i2c without add
> > > > new soc-specific fsl,lx2160-i2c.
> > >
> > > No, you should have soc-specific compatibles regardless. Just because
> > > you got away with it once, doesn't mean I'm not going to complain about
> > > it here!
> >
> > Rob:
> >         What's current policy for this? Not only for this one. If new SOC
> > appear such as iMX10 (maybe many derived chip i.MX101, i.MX102...), there
> > are bunch of IPs, Do we need add fsl,imx10* for everyone, which most part
> > is exactly the same as old one and bloat binding doc.
>
> Yes, you do. Do you really know that something in the design hasn't
> changed? Have you compared the RTL between the versions? The only way
> to deal with quirks without changing the DT everytime is by having
> specific compatibles *upfront*.
>
> The "bloat" is never that much because the IP really always changes.
> QCom wanted to (and did) use IP version numbers for the same reasons.
> Guess what, the IP version number changed on almost every SoC.

It is quite dependent on IP itself. Some IP (such as I2C, SPI, UART) is
quite mature. There are not user visual change in difference SOC. for
example in drivers/pci/controller/dwc/pci-layerscape.c:

static const struct of_device_id ls_pcie_of_match[] = {
        { .compatible = "fsl,ls1012a-pcie", .data = &layerscape_drvdata },
        { .compatible = "fsl,ls1021a-pcie", .data = &ls1021a_drvdata },
        { .compatible = "fsl,ls1028a-pcie", .data = &layerscape_drvdata },
        { .compatible = "fsl,ls1043a-pcie", .data = &ls1043a_drvdata },
        { .compatible = "fsl,ls1046a-pcie", .data = &layerscape_drvdata },
        { .compatible = "fsl,ls2080a-pcie", .data = &layerscape_drvdata },
        { .compatible = "fsl,ls2085a-pcie", .data = &layerscape_drvdata },
        { .compatible = "fsl,ls2088a-pcie", .data = &layerscape_drvdata },
        { .compatible = "fsl,ls1088a-pcie", .data = &layerscape_drvdata },

Only 1021 and 1043 have difference, others just copy layerscape_drvdata.
I met similar case in many other drivers.

Can we consider just add new compatible string only when visualable change
happen?

I am confused that some use old SOC compatible string directly, some add
new SOC compatible without any actual change.

>
> The exceptions are really if different SoCs are just different
> packaging or fusing.
>

I see.

>
> In this case, I'm inclined to say just match what u-boot creates, but
> please make that abundantly clear with a comment in the .dts file and
> explain the situation in the commit message. OTOH, just adding a new
> "fsl,lx2160a-dw-pcie" compatible with "fsl,ls2088a-pcie" fallback
> doesn't hurt, and we can just move on from creating a special case.

It is small problem.
In https://lore.kernel.org/all/CAOesGMhz8PYNG_bgMX-6gka77k1hJOZUv6xqJRqATaJ6mFbk6A@mail.gmail.com/
There are still small number user use ver1 even NXP not support Rev1.
According to my best knowledge, rev1 never mass producetion, only few out
of NXP to do evaluation.

I may need create rev1 overlay dtb for old chip and default use rev2.

Frank
>
> Rob

Re: [PATCH 1/1] arm64: dts: lx2160a: Change PCIe compatible string to fsl,ls2088a-pcie

[Rob Herring]

On Mon, Aug 12, 2024 at 1:08 PM Frank Li <Frank.li@nxp.com> wrote:
>
> On Mon, Aug 12, 2024 at 11:29:38AM -0600, Rob Herring wrote:
> > On Fri, Aug 9, 2024 at 11:11 AM Frank Li <Frank.li@nxp.com> wrote:
> > >
> > > On Fri, Aug 09, 2024 at 04:07:25PM +0100, Conor Dooley wrote:
> > > > On Thu, Aug 08, 2024 at 12:15:03PM -0400, Frank Li wrote:
> > > > > On Thu, Aug 08, 2024 at 04:55:14PM +0100, Conor Dooley wrote:
> > > > > > On Thu, Aug 08, 2024 at 11:51:34AM -0400, Frank Li wrote:
> > > > > > > On Thu, Aug 08, 2024 at 04:34:32PM +0100, Conor Dooley wrote:
> > > > > > > > On Thu, Aug 08, 2024 at 11:31:20AM -0400, Frank Li wrote:
> > > > > > > > > The mass production lx2160 rev2 use designware PCIe Controller. Old Rev1
> > > > > > > > > which use mobivel PCIe controller was not supported. Although uboot
> > > > > > > > > fixup can change compatible string fsl,lx2160a-pcie to fsl,ls2088a-pcie
> > > > > > > > > since 2019, it is quite confused and should correctly reflect hardware
> > > > > > > > > status in fsl-lx2160a.dtsi.
> > > > > > > >
> > > > > > > > This does not begin to explain why removing the soc-specific compatible,
> > > > > > > > and instead putting the compatible for another soc is the right fix.
> > > > > > > > Come up with a new compatible for this device, that perhaps falls back
> > > > > > > > to the ls2088a, but this change doesn't seem right to me.
> > > > > > >
> > > > > > > It can't fallback to fsl,ls2088a-pcie if fsl,lx2160a-pcie exist, which are
> > > > > > > totally imcompatible between fsl,ls2088a-pcie and fsl,lx2160a-pcie.
> > > > > > >
> > > > > > > Previous dtb can work just because uboot dynamtic change fsl,lx2160a-pcie
> > > > > > > to fsl,ls2088a-pcie when boot kernel.
> > > > > > >
> > > > > > > fsl,lx2160a-pcie should be removed because Rev1 have not mass productioned.
> > > > > >
> > > > > > Please re-read what I wrote. I said to come up with a new compatible for
> > > > > > this device, not fall back from the existing fsl,lx2160a-pcie to
> > > > > > fsl,ls2088a-pcie.
> > > > >
> > > > > According to my understand, It needn't add new compatible string if nothing
> > > > > difference. for example, it use fsl,vf610-i2c for all i2c without add
> > > > > new soc-specific fsl,lx2160-i2c.
> > > >
> > > > No, you should have soc-specific compatibles regardless. Just because
> > > > you got away with it once, doesn't mean I'm not going to complain about
> > > > it here!
> > >
> > > Rob:
> > >         What's current policy for this? Not only for this one. If new SOC
> > > appear such as iMX10 (maybe many derived chip i.MX101, i.MX102...), there
> > > are bunch of IPs, Do we need add fsl,imx10* for everyone, which most part
> > > is exactly the same as old one and bloat binding doc.
> >
> > Yes, you do. Do you really know that something in the design hasn't
> > changed? Have you compared the RTL between the versions? The only way
> > to deal with quirks without changing the DT everytime is by having
> > specific compatibles *upfront*.
> >
> > The "bloat" is never that much because the IP really always changes.
> > QCom wanted to (and did) use IP version numbers for the same reasons.
> > Guess what, the IP version number changed on almost every SoC.
>
> It is quite dependent on IP itself. Some IP (such as I2C, SPI, UART) is
> quite mature. There are not user visual change in difference SOC. for
> example in drivers/pci/controller/dwc/pci-layerscape.c:
>
> static const struct of_device_id ls_pcie_of_match[] = {
>         { .compatible = "fsl,ls1012a-pcie", .data = &layerscape_drvdata },
>         { .compatible = "fsl,ls1021a-pcie", .data = &ls1021a_drvdata },
>         { .compatible = "fsl,ls1028a-pcie", .data = &layerscape_drvdata },
>         { .compatible = "fsl,ls1043a-pcie", .data = &ls1043a_drvdata },
>         { .compatible = "fsl,ls1046a-pcie", .data = &layerscape_drvdata },
>         { .compatible = "fsl,ls2080a-pcie", .data = &layerscape_drvdata },
>         { .compatible = "fsl,ls2085a-pcie", .data = &layerscape_drvdata },
>         { .compatible = "fsl,ls2088a-pcie", .data = &layerscape_drvdata },
>         { .compatible = "fsl,ls1088a-pcie", .data = &layerscape_drvdata },
>
> Only 1021 and 1043 have difference, others just copy layerscape_drvdata.
> I met similar case in many other drivers.

My guess is some of these are the same die. I doubt FSL taped out 9
different die.

> Can we consider just add new compatible string only when visualable change
> happen?

Only if you know all possible errata already. So no. You asked for the
policy and I gave it.

> I am confused that some use old SOC compatible string directly, some add
> new SOC compatible without any actual change.

Because people don't like to use old SoC names with their shiny new
SoC. Or they just don't understand how compatible works. We push back
more on not having a fallback now, but the only thing the DT
maintainers have to go on is the drvdata differences or lack of.


> > The exceptions are really if different SoCs are just different
> > packaging or fusing.
> >
>
> I see.
>
> >
> > In this case, I'm inclined to say just match what u-boot creates, but
> > please make that abundantly clear with a comment in the .dts file and
> > explain the situation in the commit message. OTOH, just adding a new
> > "fsl,lx2160a-dw-pcie" compatible with "fsl,ls2088a-pcie" fallback
> > doesn't hurt, and we can just move on from creating a special case.
>
> It is small problem.
> In https://lore.kernel.org/all/CAOesGMhz8PYNG_bgMX-6gka77k1hJOZUv6xqJRqATaJ6mFbk6A@mail.gmail.com/
> There are still small number user use ver1 even NXP not support Rev1.
> According to my best knowledge, rev1 never mass producetion, only few out
> of NXP to do evaluation.

Do those users even care about new kernels? Personally, I'd drop it
and add it back when and if there are complaints.

Rob