| Message ID | 20250305132018.2260771-1-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4 | expand |
Hello, > 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. Can you confirm for me that the following use the correct macro? 333-static const struct j721e_pcie_data j721e_pcie_rc_data = { 337: .linkdown_irq_regfield = LINK_DOWN, 341-static const struct j721e_pcie_data j721e_pcie_ep_data = { 343: .linkdown_irq_regfield = LINK_DOWN, 347-static const struct j721e_pcie_data j7200_pcie_rc_data = { 350: .linkdown_irq_regfield = J7200_LINK_DOWN, 362-static const struct j721e_pcie_data am64_pcie_rc_data = { 364: .linkdown_irq_regfield = J7200_LINK_DOWN, 369-static const struct j721e_pcie_data am64_pcie_ep_data = { 371: .linkdown_irq_regfield = J7200_LINK_DOWN, 375-static const struct j721e_pcie_data j784s4_pcie_rc_data = { 379: .linkdown_irq_regfield = LINK_DOWN, 383-static const struct j721e_pcie_data j784s4_pcie_ep_data = { 385: .linkdown_irq_regfield = LINK_DOWN, 389-static const struct j721e_pcie_data j722s_pcie_rc_data = { 391: .linkdown_irq_regfield = J7200_LINK_DOWN, I am asking as some use LINK_DOWN, so I wanted to make sure. Tht said, the following has no .linkdown_irq_regfield property set: 355-static const struct j721e_pcie_data j7200_pcie_ep_data = { 356- .mode = PCI_MODE_EP, 357- .quirk_detect_quiet_flag = true, 358- .quirk_disable_flr = true, 359- .max_lanes = 2, 360-}; Would this be a problem? Or is this as expected? Thank you! Krzysztof
On Tue, Mar 11, 2025 at 06:07:46AM +0900, Krzysztof Wilczyński wrote: > Hello, > > > 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. > > Can you confirm for me that the following use the correct macro? > > 333-static const struct j721e_pcie_data j721e_pcie_rc_data = { > 337: .linkdown_irq_regfield = LINK_DOWN, > > 341-static const struct j721e_pcie_data j721e_pcie_ep_data = { > 343: .linkdown_irq_regfield = LINK_DOWN, > > 347-static const struct j721e_pcie_data j7200_pcie_rc_data = { > 350: .linkdown_irq_regfield = J7200_LINK_DOWN, > > 362-static const struct j721e_pcie_data am64_pcie_rc_data = { > 364: .linkdown_irq_regfield = J7200_LINK_DOWN, > > 369-static const struct j721e_pcie_data am64_pcie_ep_data = { > 371: .linkdown_irq_regfield = J7200_LINK_DOWN, > > 375-static const struct j721e_pcie_data j784s4_pcie_rc_data = { > 379: .linkdown_irq_regfield = LINK_DOWN, > > 383-static const struct j721e_pcie_data j784s4_pcie_ep_data = { > 385: .linkdown_irq_regfield = LINK_DOWN, > > 389-static const struct j721e_pcie_data j722s_pcie_rc_data = { > 391: .linkdown_irq_regfield = J7200_LINK_DOWN, > > I am asking as some use LINK_DOWN, so I wanted to make sure. Yes, the above are accurate except for J784S4 which is fixed by this patch. LINK_DOWN i.e. BIT(1) is applicable only to J721E which was the first SoC after which the driver has been named. For all other SoCs, the integration of the PCIe Controller into the SoC led to BIT(10) of the register being used to indicate the link status. > > Tht said, the following has no .linkdown_irq_regfield property set: > > 355-static const struct j721e_pcie_data j7200_pcie_ep_data = { > 356- .mode = PCI_MODE_EP, > 357- .quirk_detect_quiet_flag = true, > 358- .quirk_disable_flr = true, > 359- .max_lanes = 2, > 360-}; > > Would this be a problem? Or is this as expected? Thank you for pointing this out. This has to be fixed and the "linkdown_irq_regfield" member has to be added to match j7200_pcie_rc_data. I will post the fix for this. Regards, Siddharth.
Hello, > > > 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. > > > > Can you confirm for me that the following use the correct macro? > > > > 333-static const struct j721e_pcie_data j721e_pcie_rc_data = { > > 337: .linkdown_irq_regfield = LINK_DOWN, > > > > 341-static const struct j721e_pcie_data j721e_pcie_ep_data = { > > 343: .linkdown_irq_regfield = LINK_DOWN, > > > > 347-static const struct j721e_pcie_data j7200_pcie_rc_data = { > > 350: .linkdown_irq_regfield = J7200_LINK_DOWN, > > > > 362-static const struct j721e_pcie_data am64_pcie_rc_data = { > > 364: .linkdown_irq_regfield = J7200_LINK_DOWN, > > > > 369-static const struct j721e_pcie_data am64_pcie_ep_data = { > > 371: .linkdown_irq_regfield = J7200_LINK_DOWN, > > > > 375-static const struct j721e_pcie_data j784s4_pcie_rc_data = { > > 379: .linkdown_irq_regfield = LINK_DOWN, > > > > 383-static const struct j721e_pcie_data j784s4_pcie_ep_data = { > > 385: .linkdown_irq_regfield = LINK_DOWN, > > > > 389-static const struct j721e_pcie_data j722s_pcie_rc_data = { > > 391: .linkdown_irq_regfield = J7200_LINK_DOWN, > > > > I am asking as some use LINK_DOWN, so I wanted to make sure. > > Yes, the above are accurate except for J784S4 which is fixed by this > patch. LINK_DOWN i.e. BIT(1) is applicable only to J721E which was the > first SoC after which the driver has been named. For all other SoCs, the > integration of the PCIe Controller into the SoC led to BIT(10) of the > register being used to indicate the link status. Sounds good! Thank you for letting me know. > > Tht said, the following has no .linkdown_irq_regfield property set: > > > > 355-static const struct j721e_pcie_data j7200_pcie_ep_data = { > > 356- .mode = PCI_MODE_EP, > > 357- .quirk_detect_quiet_flag = true, > > 358- .quirk_disable_flr = true, > > 359- .max_lanes = 2, > > 360-}; > > > > Would this be a problem? Or is this as expected? > > Thank you for pointing this out. This has to be fixed and the > "linkdown_irq_regfield" member has to be added to match > j7200_pcie_rc_data. I will post the fix for this. No need to send a new version. I will update the branch directly when I pull the patch. Not to worry. Thank you! Krzysztof
On Tue, Mar 11, 2025 at 04:25:46PM +0900, Krzysztof Wilczyński wrote: > Hello, > > > > > 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. > > > > > > Can you confirm for me that the following use the correct macro? > > > > > > 333-static const struct j721e_pcie_data j721e_pcie_rc_data = { > > > 337: .linkdown_irq_regfield = LINK_DOWN, > > > > > > 341-static const struct j721e_pcie_data j721e_pcie_ep_data = { > > > 343: .linkdown_irq_regfield = LINK_DOWN, > > > > > > 347-static const struct j721e_pcie_data j7200_pcie_rc_data = { > > > 350: .linkdown_irq_regfield = J7200_LINK_DOWN, > > > > > > 362-static const struct j721e_pcie_data am64_pcie_rc_data = { > > > 364: .linkdown_irq_regfield = J7200_LINK_DOWN, > > > > > > 369-static const struct j721e_pcie_data am64_pcie_ep_data = { > > > 371: .linkdown_irq_regfield = J7200_LINK_DOWN, > > > > > > 375-static const struct j721e_pcie_data j784s4_pcie_rc_data = { > > > 379: .linkdown_irq_regfield = LINK_DOWN, > > > > > > 383-static const struct j721e_pcie_data j784s4_pcie_ep_data = { > > > 385: .linkdown_irq_regfield = LINK_DOWN, > > > > > > 389-static const struct j721e_pcie_data j722s_pcie_rc_data = { > > > 391: .linkdown_irq_regfield = J7200_LINK_DOWN, > > > > > > I am asking as some use LINK_DOWN, so I wanted to make sure. > > > > Yes, the above are accurate except for J784S4 which is fixed by this > > patch. LINK_DOWN i.e. BIT(1) is applicable only to J721E which was the > > first SoC after which the driver has been named. For all other SoCs, the > > integration of the PCIe Controller into the SoC led to BIT(10) of the > > register being used to indicate the link status. > > Sounds good! Thank you for letting me know. > > > > Tht said, the following has no .linkdown_irq_regfield property set: > > > > > > 355-static const struct j721e_pcie_data j7200_pcie_ep_data = { > > > 356- .mode = PCI_MODE_EP, > > > 357- .quirk_detect_quiet_flag = true, > > > 358- .quirk_disable_flr = true, > > > 359- .max_lanes = 2, > > > 360-}; > > > > > > Would this be a problem? Or is this as expected? > > > > Thank you for pointing this out. This has to be fixed and the > > "linkdown_irq_regfield" member has to be added to match > > j7200_pcie_rc_data. I will post the fix for this. > > No need to send a new version. > > I will update the branch directly when I pull the patch. Not to worry. Thank you Krzysztof :) Regards, Siddharth.
Hello, > 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". > > 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 Applied to controller/j721e, thank you! Krzysztof
Hello, [...] > > No need to send a new version. > > > > I will update the branch directly when I pull the patch. Not to worry. > > Thank you Krzysztof :) Done. Have a look at: https://web.git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/j721e&id=01d04dcd6e80f63ca5e97324ec17c20553947e35 Let me know if there is anything else to update. Thank you! Krzysztof
On Wed, Mar 12, 2025 at 12:21:33AM +0900, Krzysztof Wilczyński wrote: > Hello, > > [...] > > > No need to send a new version. > > > > > > I will update the branch directly when I pull the patch. Not to worry. > > > > Thank you Krzysztof :) > > Done. Have a look at: > > https://web.git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/j721e&id=01d04dcd6e80f63ca5e97324ec17c20553947e35 > > Let me know if there is anything else to update. The changes look good to me. There seems to be a minor typo in the commit message: [kwilczynski: add a issing .linkdown_irq_regfield member set to You probably meant "missing". Thank you once again for fixing it without the need for a new patch. Regards, Siddharth.
Hello, [...] > > > > No need to send a new version. > > > > > > > > I will update the branch directly when I pull the patch. Not to worry. > > > > > > Thank you Krzysztof :) > > > > Done. Have a look at: > > > > https://web.git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/j721e&id=01d04dcd6e80f63ca5e97324ec17c20553947e35 > > > > Let me know if there is anything else to update. > > The changes look good to me. There seems to be a minor typo in the > commit message: > [kwilczynski: add a issing .linkdown_irq_regfield member set to > > You probably meant "missing". Oops... Fixed. :) Thank you! > Thank you once again for fixing it without the need for a new patch. No worries. Krzysztof
[+to Matt, author of e49ad667815d] 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? > 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. 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. > Fixes: e49ad667815d ("PCI: j721e: Add TI J784S4 PCIe configuration") > Cc: stable@vger.kernel.org > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > > Hello, > > This patch is based on commit > 48a5eed9ad58 Merge tag 'devicetree-fixes-for-6.14-2' of git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux > of the master branch of Linux. > > Patch has been tested on J784S4-EVM, validating that disconnecting an > Endpoint Device connected to J784S4-EVM results in the following message > on the J784S4-EVM: > j721e-pcie 2900000.pcie: LINK DOWN! > which wasn't seen earlier. > > Regards, > Siddharth. > > drivers/pci/controller/cadence/pci-j721e.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c > index 0341d51d6aed..1da9d9918d0d 100644 > --- a/drivers/pci/controller/cadence/pci-j721e.c > +++ b/drivers/pci/controller/cadence/pci-j721e.c > @@ -376,13 +376,13 @@ static const struct j721e_pcie_data j784s4_pcie_rc_data = { > .mode = PCI_MODE_RC, > .quirk_retrain_flag = true, > .byte_access_allowed = false, > - .linkdown_irq_regfield = LINK_DOWN, > + .linkdown_irq_regfield = J7200_LINK_DOWN, > .max_lanes = 4, > }; > > static const struct j721e_pcie_data j784s4_pcie_ep_data = { > .mode = PCI_MODE_EP, > - .linkdown_irq_regfield = LINK_DOWN, > + .linkdown_irq_regfield = J7200_LINK_DOWN, > .max_lanes = 4, > }; > > -- > 2.34.1 >
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.
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
On Thu, Mar 13, 2025 at 11:02:15AM -0500, Bjorn Helgaas wrote: > 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. Yes, we can prepend the following to the existing commit message: "Link down interrupts on J784S4 SoC are missed because..." resulting in the following updated paragraph in the commit message: Link down interrupts on J784S4 SoC are missed because commit under Fixes assigned the value of 'linkdown_irq_regfield' for the.... > > > 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! :) Regards, Siddharth.
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c index 0341d51d6aed..1da9d9918d0d 100644 --- a/drivers/pci/controller/cadence/pci-j721e.c +++ b/drivers/pci/controller/cadence/pci-j721e.c @@ -376,13 +376,13 @@ static const struct j721e_pcie_data j784s4_pcie_rc_data = { .mode = PCI_MODE_RC, .quirk_retrain_flag = true, .byte_access_allowed = false, - .linkdown_irq_regfield = LINK_DOWN, + .linkdown_irq_regfield = J7200_LINK_DOWN, .max_lanes = 4, }; static const struct j721e_pcie_data j784s4_pcie_ep_data = { .mode = PCI_MODE_EP, - .linkdown_irq_regfield = LINK_DOWN, + .linkdown_irq_regfield = J7200_LINK_DOWN, .max_lanes = 4, };
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". 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 Fixes: e49ad667815d ("PCI: j721e: Add TI J784S4 PCIe configuration") Cc: stable@vger.kernel.org Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> --- Hello, This patch is based on commit 48a5eed9ad58 Merge tag 'devicetree-fixes-for-6.14-2' of git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux of the master branch of Linux. Patch has been tested on J784S4-EVM, validating that disconnecting an Endpoint Device connected to J784S4-EVM results in the following message on the J784S4-EVM: j721e-pcie 2900000.pcie: LINK DOWN! which wasn't seen earlier. Regards, Siddharth. drivers/pci/controller/cadence/pci-j721e.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)