[PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Siddharth Vadapalli]

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(-)

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

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Krzysztof Wilczyński]

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

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Siddharth Vadapalli]

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.

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Krzysztof Wilczyński]

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

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Siddharth Vadapalli]

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.

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Krzysztof Wilczyński]

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

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Siddharth Vadapalli]

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.

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Krzysztof Wilczyński]

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

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Krzysztof Wilczyński]

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

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Bjorn Helgaas]

[+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
> 

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Siddharth Vadapalli]

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.

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Bjorn Helgaas]

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

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Siddharth Vadapalli]

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.

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Krzysztof Wilczyński]

Hello,

[...]
> > 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....

How does this look like?

  https://web.git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/j721e&id=b97b5b8cb603a4ba6b3f7f1b6065fa76e69bdb56

Let me know if you want any changes.

Thank you!

	Krzysztof

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Siddharth Vadapalli]

On Wed, Mar 26, 2025 at 03:54:47PM +0900, Krzysztof Wilczyński wrote:
> Hello,
> 
> [...]
> > > 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....
> 
> How does this look like?
> 
>   https://web.git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/j721e&id=b97b5b8cb603a4ba6b3f7f1b6065fa76e69bdb56
> 
> Let me know if you want any changes.

I will suggest minor changes to the first paragraph of the commit
message resulting in the following paragraph:

Commit under Fixes assigned the value of .linkdown_irq_regfield for the
J784S4 SoC as the "LINK_DOWN" macro corresponding to BIT(1), and as a
result, the Link Down interrupts on J784S4 SoC are missed.

Regards,
Siddharth.

Re: [PATCH] PCI: j721e: Fix the value of linkdown_irq_regfield for J784S4

[Krzysztof Wilczyński]

Hello,

[...]
> > > > 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....
> > 
> > How does this look like?
> > 
> >   https://web.git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/j721e&id=b97b5b8cb603a4ba6b3f7f1b6065fa76e69bdb56
> > 
> > Let me know if you want any changes.
> 
> I will suggest minor changes to the first paragraph of the commit
> message resulting in the following paragraph:
> 
> Commit under Fixes assigned the value of .linkdown_irq_regfield for the
> J784S4 SoC as the "LINK_DOWN" macro corresponding to BIT(1), and as a
> result, the Link Down interrupts on J784S4 SoC are missed.

OK.  Updated.  We are going to meet there half-way. :)

Thank you!

	Krzysztof