[PATCH v1 0/2] PCI: ti: k3: Fix TI J721E PERST# polarity

[PATCH v1 0/2] PCI: ti: k3: Fix TI J721E PERST# polarity

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Fix PCIe PERST# signal polarity in TI J721E used on TI K3 machines.

PCIe PERST# needs to be de-asserted for PCIe to work, however, the driver is
doing the opposite and the device tree files are defining the signal with the
wrong polarity to cope with that. Fix both the driver and the affected DT
files.

Emanuele Ghidoli (2):
  arm64: dts: ti: Mark PCIe PERST# polarity active low in DT
  PCI: j721e: Fix PERST# polarity

 arch/arm64/boot/dts/ti/k3-am642-evm.dts                  | 2 +-
 arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dts | 2 +-
 arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dts | 2 +-
 arch/arm64/boot/dts/ti/k3-am68-sk-base-board.dts         | 2 +-
 arch/arm64/boot/dts/ti/k3-am69-sk.dts                    | 6 +++---
 arch/arm64/boot/dts/ti/k3-j7200-common-proc-board.dts    | 2 +-
 arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts       | 2 +-
 arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts    | 6 +++---
 arch/arm64/boot/dts/ti/k3-j721e-sk.dts                   | 4 ++--
 arch/arm64/boot/dts/ti/k3-j721s2-common-proc-board.dts   | 2 +-
 arch/arm64/boot/dts/ti/k3-j722s-evm.dts                  | 2 +-
 arch/arm64/boot/dts/ti/k3-j784s4-evm.dts                 | 4 ++--
 drivers/pci/controller/cadence/pci-j721e.c               | 4 ++--
 13 files changed, 20 insertions(+), 20 deletions(-)

-- 
2.39.2

[PATCH v1 1/2] arm64: dts: ti: Mark PCIe PERST# polarity active low in DT

[Francesco Dolcini]

From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>

As the name indicates, PERST# is active low. Fix the DT description to
match the HW behaviour.

Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 arch/arm64/boot/dts/ti/k3-am642-evm.dts                  | 2 +-
 arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dts | 2 +-
 arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dts | 2 +-
 arch/arm64/boot/dts/ti/k3-am68-sk-base-board.dts         | 2 +-
 arch/arm64/boot/dts/ti/k3-am69-sk.dts                    | 6 +++---
 arch/arm64/boot/dts/ti/k3-j7200-common-proc-board.dts    | 2 +-
 arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts       | 2 +-
 arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts    | 6 +++---
 arch/arm64/boot/dts/ti/k3-j721e-sk.dts                   | 4 ++--
 arch/arm64/boot/dts/ti/k3-j721s2-common-proc-board.dts   | 2 +-
 arch/arm64/boot/dts/ti/k3-j722s-evm.dts                  | 2 +-
 arch/arm64/boot/dts/ti/k3-j784s4-evm.dts                 | 4 ++--
 12 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/boot/dts/ti/k3-am642-evm.dts b/arch/arm64/boot/dts/ti/k3-am642-evm.dts
index 6bb1ad2e56ec..afefce706647 100644
--- a/arch/arm64/boot/dts/ti/k3-am642-evm.dts
+++ b/arch/arm64/boot/dts/ti/k3-am642-evm.dts
@@ -785,7 +785,7 @@ serdes0_pcie_link: phy@0 {
 
 &pcie0_rc {
 	status = "okay";
-	reset-gpios = <&exp1 5 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&exp1 5 GPIO_ACTIVE_LOW>;
 	phys = <&serdes0_pcie_link>;
 	phy-names = "pcie-phy";
 	num-lanes = <1>;
diff --git a/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dts b/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dts
index cc619bbec181..0ea5edb830cb 100644
--- a/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dts
+++ b/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-m2.dts
@@ -86,7 +86,7 @@ &pcie0_rc {
 	num-lanes = <2>;
 	phys = <&serdes0 PHY_TYPE_PCIE 1>, <&serdes1 PHY_TYPE_PCIE 1>;
 	phy-names = "pcie-phy0","pcie-phy1";
-	reset-gpios = <&main_gpio1 15 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&main_gpio1 15 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
 
diff --git a/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dts b/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dts
index b829f4bcab69..0a23c7f4e1cd 100644
--- a/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dts
+++ b/arch/arm64/boot/dts/ti/k3-am6548-iot2050-advanced-sm.dts
@@ -180,7 +180,7 @@ &pcie0_rc {
 	num-lanes = <1>;
 	phys = <&serdes0 PHY_TYPE_PCIE 1>;
 	phy-names = "pcie-phy0";
-	reset-gpios = <&wkup_gpio0 27 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&wkup_gpio0 27 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
 
diff --git a/arch/arm64/boot/dts/ti/k3-am68-sk-base-board.dts b/arch/arm64/boot/dts/ti/k3-am68-sk-base-board.dts
index 90dbe31c5b81..ba7bd40f1535 100644
--- a/arch/arm64/boot/dts/ti/k3-am68-sk-base-board.dts
+++ b/arch/arm64/boot/dts/ti/k3-am68-sk-base-board.dts
@@ -713,7 +713,7 @@ serdes0_usb_link: phy@2 {
 
 &pcie1_rc {
 	status = "okay";
-	reset-gpios = <&exp1 10 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&exp1 10 GPIO_ACTIVE_LOW>;
 	phys = <&serdes0_pcie_link>;
 	phy-names = "pcie-phy";
 	num-lanes = <2>;
diff --git a/arch/arm64/boot/dts/ti/k3-am69-sk.dts b/arch/arm64/boot/dts/ti/k3-am69-sk.dts
index 3f655852244e..c62ab4caf354 100644
--- a/arch/arm64/boot/dts/ti/k3-am69-sk.dts
+++ b/arch/arm64/boot/dts/ti/k3-am69-sk.dts
@@ -1266,14 +1266,14 @@ serdes1_pcie_link: phy@0 {
 
 &pcie0_rc {
 	status = "okay";
-	reset-gpios = <&exp1 4 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&exp1 4 GPIO_ACTIVE_LOW>;
 	phys = <&serdes1_pcie_link>;
 	phy-names = "pcie-phy";
 };
 
 &pcie1_rc {
 	status = "okay";
-	reset-gpios = <&exp1 5 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&exp1 5 GPIO_ACTIVE_LOW>;
 	phys = <&serdes0_pcie_link>;
 	phy-names = "pcie-phy";
 	num-lanes = <2>;
@@ -1281,7 +1281,7 @@ &pcie1_rc {
 
 &pcie3_rc {
 	status = "okay";
-	reset-gpios = <&exp1 6 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&exp1 6 GPIO_ACTIVE_LOW>;
 	phys = <&serdes0_pcie_link>;
 	phy-names = "pcie-phy";
 	num-lanes = <1>;
diff --git a/arch/arm64/boot/dts/ti/k3-j7200-common-proc-board.dts b/arch/arm64/boot/dts/ti/k3-j7200-common-proc-board.dts
index 6593c5da82c0..8cf336c2d5c5 100644
--- a/arch/arm64/boot/dts/ti/k3-j7200-common-proc-board.dts
+++ b/arch/arm64/boot/dts/ti/k3-j7200-common-proc-board.dts
@@ -445,7 +445,7 @@ serdes0_qsgmii_link: phy@1 {
 
 &pcie1_rc {
 	status = "okay";
-	reset-gpios = <&exp1 2 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&exp1 2 GPIO_ACTIVE_LOW>;
 	phys = <&serdes0_pcie_link>;
 	phy-names = "pcie-phy";
 	num-lanes = <2>;
diff --git a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
index a2925555fe81..4c4fdda146ab 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
+++ b/arch/arm64/boot/dts/ti/k3-j721e-beagleboneai64.dts
@@ -858,7 +858,7 @@ &pcie1_rc {
 	phy-names = "pcie-phy";
 	num-lanes = <2>;
 	max-link-speed = <3>;
-	reset-gpios = <&main_gpio0 22 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&main_gpio0 22 GPIO_ACTIVE_LOW>;
 };
 
 &ufs_wrapper {
diff --git a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
index 8230d53cd696..8709fb11bb6a 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
+++ b/arch/arm64/boot/dts/ti/k3-j721e-common-proc-board.dts
@@ -925,7 +925,7 @@ &mhdp {
 
 &pcie0_rc {
 	status = "okay";
-	reset-gpios = <&exp1 6 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&exp1 6 GPIO_ACTIVE_LOW>;
 	phys = <&serdes0_pcie_link>;
 	phy-names = "pcie-phy";
 	num-lanes = <1>;
@@ -933,7 +933,7 @@ &pcie0_rc {
 
 &pcie1_rc {
 	status = "okay";
-	reset-gpios = <&exp1 2 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&exp1 2 GPIO_ACTIVE_LOW>;
 	phys = <&serdes1_pcie_link>;
 	phy-names = "pcie-phy";
 	num-lanes = <2>;
@@ -941,7 +941,7 @@ &pcie1_rc {
 
 &pcie2_rc {
 	status = "okay";
-	reset-gpios = <&exp2 20 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&exp2 20 GPIO_ACTIVE_LOW>;
 	phys = <&serdes2_pcie_link>;
 	phy-names = "pcie-phy";
 	num-lanes = <2>;
diff --git a/arch/arm64/boot/dts/ti/k3-j721e-sk.dts b/arch/arm64/boot/dts/ti/k3-j721e-sk.dts
index 89fbfb21e5d3..6dc9966b52a1 100644
--- a/arch/arm64/boot/dts/ti/k3-j721e-sk.dts
+++ b/arch/arm64/boot/dts/ti/k3-j721e-sk.dts
@@ -1179,7 +1179,7 @@ &pcie0_rc {
 	status = "okay";
 	pinctrl-names = "default";
 	pinctrl-0 = <&ekey_reset_pins_default>;
-	reset-gpios = <&main_gpio0 72 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&main_gpio0 72 GPIO_ACTIVE_LOW>;
 
 	phys = <&serdes0_pcie_link>;
 	phy-names = "pcie-phy";
@@ -1190,7 +1190,7 @@ &pcie1_rc {
 	status = "okay";
 	pinctrl-names = "default";
 	pinctrl-0 = <&mkey_reset_pins_default>;
-	reset-gpios = <&wkup_gpio0 11 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&wkup_gpio0 11 GPIO_ACTIVE_LOW>;
 
 	phys = <&serdes1_pcie_link>;
 	phy-names = "pcie-phy";
diff --git a/arch/arm64/boot/dts/ti/k3-j721s2-common-proc-board.dts b/arch/arm64/boot/dts/ti/k3-j721s2-common-proc-board.dts
index c5a0b7cbb14f..0830cba9dc61 100644
--- a/arch/arm64/boot/dts/ti/k3-j721s2-common-proc-board.dts
+++ b/arch/arm64/boot/dts/ti/k3-j721s2-common-proc-board.dts
@@ -474,7 +474,7 @@ flash@0 {
 
 &pcie1_rc {
 	status = "okay";
-	reset-gpios = <&exp1 2 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&exp1 2 GPIO_ACTIVE_LOW>;
 	phys = <&serdes0_pcie_link>;
 	phy-names = "pcie-phy";
 	num-lanes = <1>;
diff --git a/arch/arm64/boot/dts/ti/k3-j722s-evm.dts b/arch/arm64/boot/dts/ti/k3-j722s-evm.dts
index dd3b5f7039d7..001c6fe7e95e 100644
--- a/arch/arm64/boot/dts/ti/k3-j722s-evm.dts
+++ b/arch/arm64/boot/dts/ti/k3-j722s-evm.dts
@@ -522,7 +522,7 @@ serdes1_pcie_link: phy@0 {
 };
 
 &pcie0_rc {
-	reset-gpios = <&exp1 18 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&exp1 18 GPIO_ACTIVE_LOW>;
 	phys = <&serdes1_pcie_link>;
 	phy-names = "pcie-phy";
 	status = "okay";
diff --git a/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts b/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts
index 9338d987180d..e3a393740997 100644
--- a/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts
+++ b/arch/arm64/boot/dts/ti/k3-j784s4-evm.dts
@@ -1406,7 +1406,7 @@ &serdes_wiz0 {
 &pcie1_rc {
 	status = "okay";
 	num-lanes = <2>;
-	reset-gpios = <&exp1 2 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&exp1 2 GPIO_ACTIVE_LOW>;
 	phys = <&serdes0_pcie1_link>;
 	phy-names = "pcie-phy";
 };
@@ -1429,7 +1429,7 @@ &serdes_wiz1 {
 
 &pcie0_rc {
 	status = "okay";
-	reset-gpios = <&exp1 6 GPIO_ACTIVE_HIGH>;
+	reset-gpios = <&exp1 6 GPIO_ACTIVE_LOW>;
 	phys = <&serdes1_pcie0_link>;
 	phy-names = "pcie-phy";
 };
-- 
2.39.2

Re: [PATCH v1 0/2] PCI: ti: k3: Fix TI J721E PERST# polarity

[Siddharth Vadapalli]

On Wed, Jul 03, 2024 at 12:00:34PM +0200, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Fix PCIe PERST# signal polarity in TI J721E used on TI K3 machines.
> 
> PCIe PERST# needs to be de-asserted for PCIe to work, however, the driver is
> doing the opposite and the device tree files are defining the signal with the
> wrong polarity to cope with that. Fix both the driver and the affected DT
> files.

For the series,

Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Since DT and driver patches go to different subsystems, both patches need
to go in simultaneously to avoid making devices non-functional if one of
the patches gets applied but the other one doesn't.

Regards,
Siddharth.

Re: [PATCH v1 0/2] PCI: ti: k3: Fix TI J721E PERST# polarity

[Niklas Cassel]

On Wed, Jul 03, 2024 at 12:00:34PM +0200, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Fix PCIe PERST# signal polarity in TI J721E used on TI K3 machines.
> 
> PCIe PERST# needs to be de-asserted for PCIe to work, however, the driver is
> doing the opposite and the device tree files are defining the signal with the
> wrong polarity to cope with that. Fix both the driver and the affected DT
> files.

Hello there,

While I understand why you want to fix this,
I'm not sure if you can actually do so without breaking device tree backwards
compatibility.

Imagine e.g. a board that has the DTB in ROM.
The user upgrades the kernel, and the DTB that was working with the old
kernel will now no longer work with the new kernel (because of your driver
change).

Just because you update the DTS files, you cannot assume that all DTBs
out there in the wild will automatically be updated.

That is what the DT maintainers told me many years ago when I wanted to do a
similar change as yours, for another PCIe controller driver.

Perhaps you could add a comment in the driver and the DTS files explaining
that the DTS is actually wrong, but cannot be changed because of DT backwards
compatibility.


Kind regards,
Niklas

Re: [PATCH v1 0/2] PCI: ti: k3: Fix TI J721E PERST# polarity

[Francesco Dolcini]

Hello Niklas,

On Thu, Jul 11, 2024 at 04:21:34PM +0200, Niklas Cassel wrote:
> On Wed, Jul 03, 2024 at 12:00:34PM +0200, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > Fix PCIe PERST# signal polarity in TI J721E used on TI K3 machines.
> > 
> > PCIe PERST# needs to be de-asserted for PCIe to work, however, the driver is
> > doing the opposite and the device tree files are defining the signal with the
> > wrong polarity to cope with that. Fix both the driver and the affected DT
> > files.
> 
> While I understand why you want to fix this,
> I'm not sure if you can actually do so without breaking device tree backwards
> compatibility.

I understand this, and at the same time I know that this was done in the
past for exactly the same reason, see for example commit 87620512681a
("PCI: apple: Fix PERST# polarity").

This patch was send not because the issue was noticed analyzing the
code, but because during a bring-up of a new platform (based on
k3-j784s4) using this PCIe controller driver the PCIe was not working
and this lead to some time consuming debugging on both the
hardware/software before finding this issue. That was worked around just
by describing the HW incorrectly in the DT (the device tree of this
board is not in mainline - yet).

With that said I cannot 100% judge the exact impact, I know most (but
not all) of the boards and I think that making the change is beneficial
despite what you correctly write.

Most of the boards affected are from Texas Instruments (eval boards),
plus one beagle and one board from Siemens. Let's see what these folks
think about this change, these boards are all relatively recent.

> Perhaps you could add a comment in the driver and the DTS files explaining
> that the DTS is actually wrong, but cannot be changed because of DT backwards
> compatibility.
As I wrote my concern is on new boards. 

BTW, the RS485 polarity for the UART used on all TI platform (including
the very old ones) have a similar bug [1], however this bug is so old and
deep into the code that we'll have to live with it.

[1] https://lore.kernel.org/all/ZBItlBhzo+YETcJO@francesco-nb.int.toradex.com/

Francesco

Re: [PATCH v1 0/2] PCI: ti: k3: Fix TI J721E PERST# polarity

[Niklas Cassel]

On Thu, Jul 11, 2024 at 05:25:31PM +0200, Francesco Dolcini wrote:
> Hello Niklas,
> 
> On Thu, Jul 11, 2024 at 04:21:34PM +0200, Niklas Cassel wrote:
> > On Wed, Jul 03, 2024 at 12:00:34PM +0200, Francesco Dolcini wrote:
> > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > 
> > > Fix PCIe PERST# signal polarity in TI J721E used on TI K3 machines.
> > > 
> > > PCIe PERST# needs to be de-asserted for PCIe to work, however, the driver is
> > > doing the opposite and the device tree files are defining the signal with the
> > > wrong polarity to cope with that. Fix both the driver and the affected DT
> > > files.
> > 
> > While I understand why you want to fix this,
> > I'm not sure if you can actually do so without breaking device tree backwards
> > compatibility.
> 
> I understand this, and at the same time I know that this was done in the
> past for exactly the same reason, see for example commit 87620512681a
> ("PCI: apple: Fix PERST# polarity").

If you knew about it, I think that you should have stated that your are
breaking DT compatibility in the commit message, while also explaining it
is acceptable in your specific case.

I didn't know that there were other examples of drivers doing this.
Looking at your example, it seems that both:
1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
and
87620512681a ("PCI: apple: Fix PERST# polarity")

were first included in v5.16, so there was never a kernel release
with only one of the commits.

Anyway, I will eagerly await the DT maintainers feedback on this series.


Kind regards,
Niklas

Re: [PATCH v1 0/2] PCI: ti: k3: Fix TI J721E PERST# polarity

[Niklas Cassel]

On Fri, Jul 12, 2024 at 12:34:01PM +0200, Niklas Cassel wrote:
> 
> If you knew about it, I think that you should have stated that your are
> breaking DT compatibility in the commit message, while also explaining it
> is acceptable in your specific case.

s/while also explaining it is/while also explaining why it is/

Re: [PATCH v1 0/2] PCI: ti: k3: Fix TI J721E PERST# polarity

[Francesco Dolcini]

Hello Bjorn, Krzysztof W., Lorenzo

On Wed, Jul 03, 2024 at 12:00:34PM +0200, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Fix PCIe PERST# signal polarity in TI J721E used on TI K3 machines.
> 
> PCIe PERST# needs to be de-asserted for PCIe to work, however, the driver is
> doing the opposite and the device tree files are defining the signal with the
> wrong polarity to cope with that. Fix both the driver and the affected DT
> files.

I just had a chat in IRC about this series with Nishanth. He agrees that
this should be merged, even considering that this implies breaking the
compatibility with old device tree blobs.

However we should be sure that both patches get merged in a coordinated
way, to avoid breaking stuff within the same kernel release.

What would be your advise to move forward? Are you ok with the change?
Should I split this series in 2 separated patch?

Francesco