| Message ID | 20250305132018.2260771-1-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Krzysztof Wilczyński |
| 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 >
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(-)