On Wed, Mar 12, 2025 at 11:16:00AM -0500, Bjorn Helgaas wrote: Hello Bjorn, > [+to Matt, author of e49ad667815d] I dropped Matt's email on purpose since it will bounce as the email is no longer valid. > > On Wed, Mar 05, 2025 at 06:50:18PM +0530, Siddharth Vadapalli wrote: > > Commit under Fixes assigned the value of 'linkdown_irq_regfield' for the > > J784S4 SoC as 'LINK_DOWN' which corresponds to BIT(1). However, according > > to the Technical Reference Manual and Register Documentation for the J784S4 > > SoC [0], BIT(1) corresponds to "ENABLE_SYS_EN_PCIE_DPA_1" which is __NOT__ > > the field for the link-state interrupt. Instead, it is BIT(10) of the > > "PCIE_INTD_ENABLE_REG_SYS_2" register that corresponds to the link-state > > field named as "ENABLE_SYS_EN_PCIE_LINK_STATE". > > I guess the reason we want this is that on J784S4, we ignore actual > link-down interrupts (and we don't write STATUS_CLR_REG_SYS_2 to clear > the interrupt indication, so maybe there's an interrupt storm), and we > think some other interrupt (DPA_1, whatever that is) is actually a > link-down interrupt? While it is true that actual link-down interrupts are ignored, it is not the case that there's an interrupt storm because the same incorrect macro is used to enable the interrupt line. Since the enables an interrupt for DPA_1 which never fires, we don't run into the situation where we are not clearing the interrupt (the interrupt handler will look for the same incorrect field to clear the interrupt if it does fire for DPA_1, but that doesn't happen). The 'linkdown_irq_regfield' corresponds to the "link-state" field not just in the J784S4 SoC, but in all SoCs supported by the pci-j721e.c driver. It is only in J721E that it is BIT(1) [LINK_DOWN macro], while in all other SoCs (J784S4 included), it is BIT(10) [J7200_LINK_DOWN macro since it was first added for J7200 SoC]. Matt probably referred to J721E's Technical Reference Manual and ended up incorrectly assigning "LINK_DOWN", due to which the driver is enabling the DPA_1 interrupt and the interrupt handler is also going to look for the field corresponding to receiving an interrupt for DPA_1. > > > Hence, set 'linkdown_irq_regfield' to the macro 'J7200_LINK_DOWN' which > > expands to BIT(10) and was first defined for the J7200 SoC. Other SoCs > > already reuse this macro since it accurately represents the link-state > > field in their respective "PCIE_INTD_ENABLE_REG_SYS_2" register. > > > > [0]: https://www.ti.com/lit/zip/spruj52 > > Thanks for the spec URL. Can you include a relevant section number? > I searched for some of this stuff but couldn't find it. The URL above is taken from the "User Guide" section of the following webpage: https://www.ti.com/product/TDA4VH-Q1 corresponding to the J784S4 SoC (TDA4VH is another name for it). The User Guide [0] is a zip file containing the Technical Reference Manual (without Registers) along with an Excel Sheet containing the Registers. There unfortunately is no particular section that I can quote in the Excel Sheet. The PCIe registers described in the Excel Sheet contain the "PCIE_INTD_ENABLE_REG_SYS_2" register in one of the rows (I didn't want to mention the row number since things could change over time, similar to how you pointed out below that the URL could potentially change). However, the register name should remain the same, the reason being that the name is consistent across all SoCs supported by the pci-j721e.c. > > Since I have low confidence that the URL will be valid after a few > years, I wish the spec also had a human-readable name and revision > number. But maybe the alphabet soup or "SPRUJ52D", "revised July > 2024" is all we can hope for. I can only hope that the URL will redirect to the latest version of the User Guide if at all it becomes invalid. Regards, Siddharth.
