| Message ID | 20240703100036.17896-1-francesco@dolcini.it |
|---|---|
| Headers | show |
| Series | PCI: ti: k3: Fix TI J721E PERST# polarity | expand |
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.
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
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
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
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/
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
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(-)