mbox series

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

Message ID 20240703100036.17896-1-francesco@dolcini.it (mailing list archive)
Headers show
Series PCI: ti: k3: Fix TI J721E PERST# polarity | expand

Message

Francesco Dolcini July 3, 2024, 10 a.m. UTC
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(-)

Comments

Siddharth Vadapalli July 6, 2024, 6:44 a.m. UTC | #1
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.
Niklas Cassel July 11, 2024, 2:21 p.m. UTC | #2
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
Francesco Dolcini July 11, 2024, 3:25 p.m. UTC | #3
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
Niklas Cassel July 12, 2024, 10:34 a.m. UTC | #4
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
Niklas Cassel July 12, 2024, 10:40 a.m. UTC | #5
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/
Francesco Dolcini Aug. 6, 2024, 2:35 p.m. UTC | #6
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