On Thu, Mar 13, 2025 at 11:25:19AM +0530, Siddharth Vadapalli wrote:
On Wed, Mar 12, 2025 at 11:16:00AM -0500, Bjorn Helgaas wrote:
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.
So I guess without this patch, we incorrectly ignore link-down interrupts on J784S4. It's good to have a one-sentence motivation like that somewhere in the commit log that we can pull out and include in the merge commit log and the pull request.
I can only hope that the URL will redirect to the latest version of the User Guide if at all it becomes invalid.
OK, thanks, I guess there's nothing more to do ;) I guess that manual is not really designed for collaborative development.
Thanks for the patient hand holding!
Bjorn