linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] NTB: Fix an error in get link status
@ 2019-11-07  9:35 Jiasen Lin
  2019-11-17 23:00 ` Jon Mason
  0 siblings, 1 reply; 10+ messages in thread
From: Jiasen Lin @ 2019-11-07  9:35 UTC (permalink / raw)
  To: jdmason, Shyam-sundar.S-k, dave.jiang, allenbh
  Cc: linux-kernel, linux-ntb, linjiasen, linjiasen007

The offset of PCIe Capability Header for AMD and HYGON NTB is 0x64,
but the macro which named "AMD_LINK_STATUS_OFFSET" is defined as 0x68.
It is offset of Device Capabilities Reg rather than Link Control Reg.

This code trigger an error in get link statsus:

	cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
		LNK STA -               0x8fa1
		Link Status -           Up
		Link Speed -            PCI-E Gen 0
		Link Width -            x0

This patch use pcie_capability_read_dword to get link status.
After fix this issue, we can get link status accurately:

	cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
		LNK STA -               0x11030042
		Link Status -           Up
		Link Speed -            PCI-E Gen 3
		Link Width -            x16

Signed-off-by: Jiasen Lin <linjiasen@hygon.cn>
---
 drivers/ntb/hw/amd/ntb_hw_amd.c | 5 +++--
 drivers/ntb/hw/amd/ntb_hw_amd.h | 1 -
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c
index 156c2a1..ae91105 100644
--- a/drivers/ntb/hw/amd/ntb_hw_amd.c
+++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
@@ -855,8 +855,8 @@ static int amd_poll_link(struct amd_ntb_dev *ndev)
 
 	ndev->cntl_sta = reg;
 
-	rc = pci_read_config_dword(ndev->ntb.pdev,
-				   AMD_LINK_STATUS_OFFSET, &stat);
+	rc = pcie_capability_read_dword(ndev->ntb.pdev,
+				   PCI_EXP_LNKCTL, &stat);
 	if (rc)
 		return 0;
 	ndev->lnk_sta = stat;
@@ -1139,6 +1139,7 @@ static const struct ntb_dev_data dev_data[] = {
 static const struct pci_device_id amd_ntb_pci_tbl[] = {
 	{ PCI_VDEVICE(AMD, 0x145b), (kernel_ulong_t)&dev_data[0] },
 	{ PCI_VDEVICE(AMD, 0x148b), (kernel_ulong_t)&dev_data[1] },
+	{ PCI_VDEVICE(HYGON, 0x145b), (kernel_ulong_t)&dev_data[0] },
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, amd_ntb_pci_tbl);
diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.h b/drivers/ntb/hw/amd/ntb_hw_amd.h
index 139a307..39e5d18 100644
--- a/drivers/ntb/hw/amd/ntb_hw_amd.h
+++ b/drivers/ntb/hw/amd/ntb_hw_amd.h
@@ -53,7 +53,6 @@
 #include <linux/pci.h>
 
 #define AMD_LINK_HB_TIMEOUT	msecs_to_jiffies(1000)
-#define AMD_LINK_STATUS_OFFSET	0x68
 #define NTB_LIN_STA_ACTIVE_BIT	0x00000002
 #define NTB_LNK_STA_SPEED_MASK	0x000F0000
 #define NTB_LNK_STA_WIDTH_MASK	0x03F00000
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] NTB: Fix an error in get link status
  2019-11-07  9:35 [PATCH] NTB: Fix an error in get link status Jiasen Lin
@ 2019-11-17 23:00 ` Jon Mason
  2019-11-18 10:17   ` Jiasen Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Jon Mason @ 2019-11-17 23:00 UTC (permalink / raw)
  To: Jiasen Lin
  Cc: S-k, Shyam-sundar, Dave Jiang, Allen Hubbe, linux-kernel,
	linux-ntb, linjiasen007

On Thu, Nov 7, 2019 at 4:37 AM Jiasen Lin <linjiasen@hygon.cn> wrote:
>
> The offset of PCIe Capability Header for AMD and HYGON NTB is 0x64,
> but the macro which named "AMD_LINK_STATUS_OFFSET" is defined as 0x68.
> It is offset of Device Capabilities Reg rather than Link Control Reg.
>
> This code trigger an error in get link statsus:
>
>         cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
>                 LNK STA -               0x8fa1
>                 Link Status -           Up
>                 Link Speed -            PCI-E Gen 0
>                 Link Width -            x0
>
> This patch use pcie_capability_read_dword to get link status.
> After fix this issue, we can get link status accurately:
>
>         cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
>                 LNK STA -               0x11030042
>                 Link Status -           Up
>                 Link Speed -            PCI-E Gen 3
>                 Link Width -            x16

No response from AMD maintainers, but it looks like you are correct.

This needs a "Fixes:" line here.  I took the liberty of adding one to
this patch.

> Signed-off-by: Jiasen Lin <linjiasen@hygon.cn>
> ---
>  drivers/ntb/hw/amd/ntb_hw_amd.c | 5 +++--
>  drivers/ntb/hw/amd/ntb_hw_amd.h | 1 -
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c
> index 156c2a1..ae91105 100644
> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
> @@ -855,8 +855,8 @@ static int amd_poll_link(struct amd_ntb_dev *ndev)
>
>         ndev->cntl_sta = reg;
>
> -       rc = pci_read_config_dword(ndev->ntb.pdev,
> -                                  AMD_LINK_STATUS_OFFSET, &stat);
> +       rc = pcie_capability_read_dword(ndev->ntb.pdev,
> +                                  PCI_EXP_LNKCTL, &stat);
>         if (rc)
>                 return 0;
>         ndev->lnk_sta = stat;
> @@ -1139,6 +1139,7 @@ static const struct ntb_dev_data dev_data[] = {
>  static const struct pci_device_id amd_ntb_pci_tbl[] = {
>         { PCI_VDEVICE(AMD, 0x145b), (kernel_ulong_t)&dev_data[0] },
>         { PCI_VDEVICE(AMD, 0x148b), (kernel_ulong_t)&dev_data[1] },
> +       { PCI_VDEVICE(HYGON, 0x145b), (kernel_ulong_t)&dev_data[0] },

This should be a separate patch.  I took the liberty of splitting it
off into a unique patch and attributing it to you.  I've pushed them
to the ntb-next branch on
https://github.com/jonmason/ntb

Please verify everything looks acceptable to you (given the changes I
did above that are attributed to you).  Also, testing of the latest
code is always appreciated.

Thanks,
Jon


>         { 0, }
>  };
>  MODULE_DEVICE_TABLE(pci, amd_ntb_pci_tbl);
> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.h b/drivers/ntb/hw/amd/ntb_hw_amd.h
> index 139a307..39e5d18 100644
> --- a/drivers/ntb/hw/amd/ntb_hw_amd.h
> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.h
> @@ -53,7 +53,6 @@
>  #include <linux/pci.h>
>
>  #define AMD_LINK_HB_TIMEOUT    msecs_to_jiffies(1000)
> -#define AMD_LINK_STATUS_OFFSET 0x68
>  #define NTB_LIN_STA_ACTIVE_BIT 0x00000002
>  #define NTB_LNK_STA_SPEED_MASK 0x000F0000
>  #define NTB_LNK_STA_WIDTH_MASK 0x03F00000
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/1573119336-107732-1-git-send-email-linjiasen%40hygon.cn.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] NTB: Fix an error in get link status
  2019-11-17 23:00 ` Jon Mason
@ 2019-11-18 10:17   ` Jiasen Lin
  2019-11-20  9:52     ` Jiasen Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Jiasen Lin @ 2019-11-18 10:17 UTC (permalink / raw)
  To: Jon Mason
  Cc: S-k, Shyam-sundar, Dave Jiang, Allen Hubbe, linux-kernel,
	linux-ntb, linjiasen007



On 2019/11/18 7:00, Jon Mason wrote:
> On Thu, Nov 7, 2019 at 4:37 AM Jiasen Lin <linjiasen@hygon.cn> wrote:
>>
>> The offset of PCIe Capability Header for AMD and HYGON NTB is 0x64,
>> but the macro which named "AMD_LINK_STATUS_OFFSET" is defined as 0x68.
>> It is offset of Device Capabilities Reg rather than Link Control Reg.
>>
>> This code trigger an error in get link statsus:
>>
>>          cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
>>                  LNK STA -               0x8fa1
>>                  Link Status -           Up
>>                  Link Speed -            PCI-E Gen 0
>>                  Link Width -            x0
>>
>> This patch use pcie_capability_read_dword to get link status.
>> After fix this issue, we can get link status accurately:
>>
>>          cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
>>                  LNK STA -               0x11030042
>>                  Link Status -           Up
>>                  Link Speed -            PCI-E Gen 3
>>                  Link Width -            x16
> 
> No response from AMD maintainers, but it looks like you are correct.
> 
> This needs a "Fixes:" line here.  I took the liberty of adding one to
> this patch.
> 

Thank you for your suggestions. Yes, this patch fix the commit id: 
a1b3695 ("NTB: Add support for AMD PCI-Express Non-Transparent Bridge").

>> Signed-off-by: Jiasen Lin <linjiasen@hygon.cn>
>> ---
>>   drivers/ntb/hw/amd/ntb_hw_amd.c | 5 +++--
>>   drivers/ntb/hw/amd/ntb_hw_amd.h | 1 -
>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c
>> index 156c2a1..ae91105 100644
>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
>> @@ -855,8 +855,8 @@ static int amd_poll_link(struct amd_ntb_dev *ndev)
>>
>>          ndev->cntl_sta = reg;
>>
>> -       rc = pci_read_config_dword(ndev->ntb.pdev,
>> -                                  AMD_LINK_STATUS_OFFSET, &stat);
>> +       rc = pcie_capability_read_dword(ndev->ntb.pdev,
>> +                                  PCI_EXP_LNKCTL, &stat);
>>          if (rc)
>>                  return 0;
>>          ndev->lnk_sta = stat;
>> @@ -1139,6 +1139,7 @@ static const struct ntb_dev_data dev_data[] = {
>>   static const struct pci_device_id amd_ntb_pci_tbl[] = {
>>          { PCI_VDEVICE(AMD, 0x145b), (kernel_ulong_t)&dev_data[0] },
>>          { PCI_VDEVICE(AMD, 0x148b), (kernel_ulong_t)&dev_data[1] },
>> +       { PCI_VDEVICE(HYGON, 0x145b), (kernel_ulong_t)&dev_data[0] },
> 
> This should be a separate patch.  I took the liberty of splitting it
> off into a unique patch and attributing it to you.  I've pushed them
> to the ntb-next branch on
> https://github.com/jonmason/ntb
>
Thank you for your comment. We appreciate the time and effort you have 
spent to split it off, I will test it ASAP.

> Please verify everything looks acceptable to you (given the changes I
> did above that are attributed to you).  Also, testing of the latest
> code is always appreciated.
> 
> Thanks,
> Jon
> 
> 
>>          { 0, }
>>   };
>>   MODULE_DEVICE_TABLE(pci, amd_ntb_pci_tbl);
>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.h b/drivers/ntb/hw/amd/ntb_hw_amd.h
>> index 139a307..39e5d18 100644
>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.h
>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.h
>> @@ -53,7 +53,6 @@
>>   #include <linux/pci.h>
>>
>>   #define AMD_LINK_HB_TIMEOUT    msecs_to_jiffies(1000)
>> -#define AMD_LINK_STATUS_OFFSET 0x68
>>   #define NTB_LIN_STA_ACTIVE_BIT 0x00000002
>>   #define NTB_LNK_STA_SPEED_MASK 0x000F0000
>>   #define NTB_LNK_STA_WIDTH_MASK 0x03F00000
>> --
>> 2.7.4
>>
>> --
>> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/1573119336-107732-1-git-send-email-linjiasen%40hygon.cn.

Thanks,

Jiasen Lin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] NTB: Fix an error in get link status
  2019-11-18 10:17   ` Jiasen Lin
@ 2019-11-20  9:52     ` Jiasen Lin
       [not found]       ` <CAADLhr7bpb-F0eF1UFXy7AcN=z061mno_QsqGE8z-mvWKvUyCQ@mail.gmail.com>
  0 siblings, 1 reply; 10+ messages in thread
From: Jiasen Lin @ 2019-11-20  9:52 UTC (permalink / raw)
  To: Jon Mason
  Cc: S-k, Shyam-sundar, Dave Jiang, Allen Hubbe, linux-kernel,
	linux-ntb, linjiasen007, Jiasen Lin



On 2019/11/18 18:17, Jiasen Lin wrote:
> 
> 
> On 2019/11/18 7:00, Jon Mason wrote:
>> On Thu, Nov 7, 2019 at 4:37 AM Jiasen Lin <linjiasen@hygon.cn> wrote:
>>>
>>> The offset of PCIe Capability Header for AMD and HYGON NTB is 0x64,
>>> but the macro which named "AMD_LINK_STATUS_OFFSET" is defined as 0x68.
>>> It is offset of Device Capabilities Reg rather than Link Control Reg.
>>>
>>> This code trigger an error in get link statsus:
>>>
>>>          cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
>>>                  LNK STA -               0x8fa1
>>>                  Link Status -           Up
>>>                  Link Speed -            PCI-E Gen 0
>>>                  Link Width -            x0
>>>
>>> This patch use pcie_capability_read_dword to get link status.
>>> After fix this issue, we can get link status accurately:
>>>
>>>          cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
>>>                  LNK STA -               0x11030042
>>>                  Link Status -           Up
>>>                  Link Speed -            PCI-E Gen 3
>>>                  Link Width -            x16
>>
>> No response from AMD maintainers, but it looks like you are correct.
>>
>> This needs a "Fixes:" line here.  I took the liberty of adding one to
>> this patch.
>>
> 
> Thank you for your suggestions. Yes, this patch fix the commit id: 
> a1b3695 ("NTB: Add support for AMD PCI-Express Non-Transparent Bridge").
> 
>>> Signed-off-by: Jiasen Lin <linjiasen@hygon.cn>
>>> ---
>>>   drivers/ntb/hw/amd/ntb_hw_amd.c | 5 +++--
>>>   drivers/ntb/hw/amd/ntb_hw_amd.h | 1 -
>>>   2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c 
>>> b/drivers/ntb/hw/amd/ntb_hw_amd.c
>>> index 156c2a1..ae91105 100644
>>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
>>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
>>> @@ -855,8 +855,8 @@ static int amd_poll_link(struct amd_ntb_dev *ndev)
>>>
>>>          ndev->cntl_sta = reg;
>>>
>>> -       rc = pci_read_config_dword(ndev->ntb.pdev,
>>> -                                  AMD_LINK_STATUS_OFFSET, &stat);
>>> +       rc = pcie_capability_read_dword(ndev->ntb.pdev,
>>> +                                  PCI_EXP_LNKCTL, &stat);
>>>          if (rc)
>>>                  return 0;
>>>          ndev->lnk_sta = stat;
>>> @@ -1139,6 +1139,7 @@ static const struct ntb_dev_data dev_data[] = {
>>>   static const struct pci_device_id amd_ntb_pci_tbl[] = {
>>>          { PCI_VDEVICE(AMD, 0x145b), (kernel_ulong_t)&dev_data[0] },
>>>          { PCI_VDEVICE(AMD, 0x148b), (kernel_ulong_t)&dev_data[1] },
>>> +       { PCI_VDEVICE(HYGON, 0x145b), (kernel_ulong_t)&dev_data[0] },
>>
>> This should be a separate patch.  I took the liberty of splitting it
>> off into a unique patch and attributing it to you.  I've pushed them
>> to the ntb-next branch on
>> https://github.com/jonmason/ntb
>>
> Thank you for your comment. We appreciate the time and effort you have 
> spent to split it off, I will test it ASAP.
> 
>> Please verify everything looks acceptable to you (given the changes I
>> did above that are attributed to you).  Also, testing of the latest
>> code is always appreciated.
>>
>> Thanks,
>> Jon
>>

I have tested these patches that are pushed to ntb-next branch, they 
work well on HYGON platforms.

Thanks,
Jiasen Lin

>>
>>>          { 0, }
>>>   };
>>>   MODULE_DEVICE_TABLE(pci, amd_ntb_pci_tbl);
>>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.h 
>>> b/drivers/ntb/hw/amd/ntb_hw_amd.h
>>> index 139a307..39e5d18 100644
>>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.h
>>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.h
>>> @@ -53,7 +53,6 @@
>>>   #include <linux/pci.h>
>>>
>>>   #define AMD_LINK_HB_TIMEOUT    msecs_to_jiffies(1000)
>>> -#define AMD_LINK_STATUS_OFFSET 0x68
>>>   #define NTB_LIN_STA_ACTIVE_BIT 0x00000002
>>>   #define NTB_LNK_STA_SPEED_MASK 0x000F0000
>>>   #define NTB_LNK_STA_WIDTH_MASK 0x03F00000
>>> -- 
>>> 2.7.4
>>>
>>> -- 
>>> You received this message because you are subscribed to the Google 
>>> Groups "linux-ntb" group.
>>> To unsubscribe from this group and stop receiving emails from it, 
>>> send an email to linux-ntb+unsubscribe@googlegroups.com.
>>> To view this discussion on the web visit 
>>> https://groups.google.com/d/msgid/linux-ntb/1573119336-107732-1-git-send-email-linjiasen%40hygon.cn. 
>>>
> 
> Thanks,
> 
> Jiasen Lin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fwd: [PATCH] NTB: Fix an error in get link status
       [not found]       ` <CAADLhr7bpb-F0eF1UFXy7AcN=z061mno_QsqGE8z-mvWKvUyCQ@mail.gmail.com>
@ 2019-11-20 14:13         ` Sanjay R Mehta
  2019-11-21 13:30           ` Jiasen Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Sanjay R Mehta @ 2019-11-20 14:13 UTC (permalink / raw)
  To: Jiasen Lin
  Cc: S-k, Shyam-sundar, Dave Jiang, Allen Hubbe, linux-kernel,
	linux-ntb, linjiasen007

From: *Jiasen Lin* <linjiasen@hygon.cn <mailto:linjiasen@hygon.cn>>
> Date: Wed, Nov 20, 2019 at 3:25 PM
> Subject: Re: [PATCH] NTB: Fix an error in get link status
> To: Jon Mason <jdmason@kudzu.us <mailto:jdmason@kudzu.us>>
> Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com <mailto:Shyam-sundar.S-k@amd.com>>, Dave Jiang <dave.jiang@intel.com <mailto:dave.jiang@intel.com>>, Allen Hubbe <allenbh@gmail.com
> <mailto:allenbh@gmail.com>>, linux-kernel <linux-kernel@vger.kernel.org <mailto:linux-kernel@vger.kernel.org>>, linux-ntb <linux-ntb@googlegroups.com <mailto:linux-ntb@googlegroups.com>>,
> <linjiasen007@gmail.com <mailto:linjiasen007@gmail.com>>, Jiasen Lin <linjiasen@hygon.cn <mailto:linjiasen@hygon.cn>>
>
>
>
>
> On 2019/11/18 18:17, Jiasen Lin wrote:
> >
> >
> > On 2019/11/18 7:00, Jon Mason wrote:
> >> On Thu, Nov 7, 2019 at 4:37 AM Jiasen Lin <linjiasen@hygon.cn <mailto:linjiasen@hygon.cn>> wrote:
> >>>
> >>> The offset of PCIe Capability Header for AMD and HYGON NTB is 0x64,
> >>> but the macro which named "AMD_LINK_STATUS_OFFSET" is defined as 0x68.
> >>> It is offset of Device Capabilities Reg rather than Link Control Reg.
> >>>
> >>> This code trigger an error in get link statsus:
> >>>
> >>>          cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
> >>>                  LNK STA -               0x8fa1
> >>>                  Link Status -           Up
> >>>                  Link Speed -            PCI-E Gen 0
> >>>                  Link Width -            x0
> >>>
> >>> This patch use pcie_capability_read_dword to get link status.
> >>> After fix this issue, we can get link status accurately:
> >>>
> >>>          cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
> >>>                  LNK STA -               0x11030042
> >>>                  Link Status -           Up
> >>>                  Link Speed -            PCI-E Gen 3
> >>>                  Link Width -            x16
> >>
> >> No response from AMD maintainers, but it looks like you are correct.
> >>
> >> This needs a "Fixes:" line here.  I took the liberty of adding one to
> >> this patch.
> >>
> >
> > Thank you for your suggestions. Yes, this patch fix the commit id:
> > a1b3695 ("NTB: Add support for AMD PCI-Express Non-Transparent Bridge").
> >
> >>> Signed-off-by: Jiasen Lin <linjiasen@hygon.cn <mailto:linjiasen@hygon.cn>>
> >>> ---
> >>>   drivers/ntb/hw/amd/ntb_hw_amd.c | 5 +++--
> >>>   drivers/ntb/hw/amd/ntb_hw_amd.h | 1 -
> >>>   2 files changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c
> >>> b/drivers/ntb/hw/amd/ntb_hw_amd.c
> >>> index 156c2a1..ae91105 100644
> >>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
> >>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
> >>> @@ -855,8 +855,8 @@ static int amd_poll_link(struct amd_ntb_dev *ndev)
> >>>
> >>>          ndev->cntl_sta = reg;
> >>>
> >>> -       rc = pci_read_config_dword(ndev->ntb.pdev,
> >>> -                                  AMD_LINK_STATUS_OFFSET, &stat);
> >>> +       rc = pcie_capability_read_dword(ndev->ntb.pdev,
> >>> +                                  PCI_EXP_LNKCTL, &stat);
> >>>          if (rc)
> >>>                  return 0;
> >>>          ndev->lnk_sta = stat;
> >>> @@ -1139,6 +1139,7 @@ static const struct ntb_dev_data dev_data[] = {
> >>>   static const struct pci_device_id amd_ntb_pci_tbl[] = {
> >>>          { PCI_VDEVICE(AMD, 0x145b), (kernel_ulong_t)&dev_data[0] },
> >>>          { PCI_VDEVICE(AMD, 0x148b), (kernel_ulong_t)&dev_data[1] },
> >>> +       { PCI_VDEVICE(HYGON, 0x145b), (kernel_ulong_t)&dev_data[0] },
> >>
> >> This should be a separate patch.  I took the liberty of splitting it
> >> off into a unique patch and attributing it to you.  I've pushed them
> >> to the ntb-next branch on
> >> https://github.com/jonmason/ntb
> >>
> > Thank you for your comment. We appreciate the time and effort you have
> > spent to split it off, I will test it ASAP.
> >
> >> Please verify everything looks acceptable to you (given the changes I
> >> did above that are attributed to you).  Also, testing of the latest
> >> code is always appreciated.
> >>
> >> Thanks,
> >> Jon
> >>
>
> I have tested these patches that are pushed to ntb-next branch, they
> work well on HYGON platforms.
>
> Thanks,
> Jiasen Lin

Hi Jiasen Lin,

Apologies for my delayed response. I was on vacation.

Your patch is a correct fix, but that would work only for primary system.

The design of AMD NTB implementation is such that NTB primary acts as an endpoint device and NTB secondary is a PCIe Root Port (RP). Considering that,
the link status and control register needs to be accessed differently based on the NTB topology.

So in the case of NTB secondary, we read the link status and control register of the PCIe RP capabilities structure and in the case of NTB primary, we read the
link status and control register from NTB device capabilities structure.

The code below is the proper fix for AMD platform. I will be sending incremental change above your patch.

would appreciate if you could test my patch and let me know whether that works for you.

---
 drivers/ntb/hw/amd/ntb_hw_amd.c | 27 +++++++++++++++++++++++----
 drivers/ntb/hw/amd/ntb_hw_amd.h |  1 -
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c
index 14ad69c..91e1966 100644
--- a/drivers/ntb/hw/amd/ntb_hw_amd.c
+++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
@@ -842,6 +842,8 @@ static inline void ndev_init_struct(struct amd_ntb_dev *ndev,
 static int amd_poll_link(struct amd_ntb_dev *ndev)
 {
     void __iomem *mmio = ndev->peer_mmio;
+    struct pci_dev *pci_rp = NULL;
+    struct pci_dev *pdev = NULL;
     u32 reg, stat;
     int rc;
 
@@ -855,10 +857,27 @@ static int amd_poll_link(struct amd_ntb_dev *ndev)
 
     ndev->cntl_sta = reg;
 
-    rc = pci_read_config_dword(ndev->ntb.pdev,
-                   AMD_LINK_STATUS_OFFSET, &stat);
-    if (rc)
-        return 0;
+    if (ndev->ntb.topo == NTB_TOPO_SEC) {
+        /* Locate the pointer to PCIe Root Port for this device */
+        pci_rp = pci_find_pcie_root_port(ndev->ntb.pdev);
+        /* Read the PCIe Link Control and Status register */
+        if (pci_rp) {
+            rc = pcie_capability_read_dword(pci_rp, PCI_EXP_LNKCTL,
+                            &stat);
+            if (rc)
+                return 0;
+        }
+    } else if (ndev->ntb.topo == NTB_TOPO_PRI) {
+        /*
+         * For NTB primary, we simply read the Link Status and control
+         * register of the NTB device itself.
+         */
+        pdev = ndev->ntb.pdev;
+        rc = pcie_capability_read_dword(pdev, PCI_EXP_LNKCTL, &stat);
+        if (rc)
+            return 0;
+    }
+
     ndev->lnk_sta = stat;
 
     return 1;
diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.h b/drivers/ntb/hw/amd/ntb_hw_amd.h
index 139a307..39e5d18 100644
--- a/drivers/ntb/hw/amd/ntb_hw_amd.h
+++ b/drivers/ntb/hw/amd/ntb_hw_amd.h
@@ -53,7 +53,6 @@
 #include <linux/pci.h>
 
 #define AMD_LINK_HB_TIMEOUT    msecs_to_jiffies(1000)
-#define AMD_LINK_STATUS_OFFSET    0x68
 #define NTB_LIN_STA_ACTIVE_BIT    0x00000002
 #define NTB_LNK_STA_SPEED_MASK    0x000F0000
 #define NTB_LNK_STA_WIDTH_MASK    0x03F00000
-- 
2.7.4

Thanks & Regards
Sanjay Mehta

>
> >>
> >>>          { 0, }
> >>>   };
> >>>   MODULE_DEVICE_TABLE(pci, amd_ntb_pci_tbl);
> >>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.h
> >>> b/drivers/ntb/hw/amd/ntb_hw_amd.h
> >>> index 139a307..39e5d18 100644
> >>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.h
> >>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.h
> >>> @@ -53,7 +53,6 @@
> >>>   #include <linux/pci.h>
> >>>
> >>>   #define AMD_LINK_HB_TIMEOUT    msecs_to_jiffies(1000)
> >>> -#define AMD_LINK_STATUS_OFFSET 0x68
> >>>   #define NTB_LIN_STA_ACTIVE_BIT 0x00000002
> >>>   #define NTB_LNK_STA_SPEED_MASK 0x000F0000
> >>>   #define NTB_LNK_STA_WIDTH_MASK 0x03F00000
> >>> --
> >>> 2.7.4
> >>>
> >>> --
> >>> You received this message because you are subscribed to the Google
> >>> Groups "linux-ntb" group.
> >>> To unsubscribe from this group and stop receiving emails from it,
> >>> send an email to linux-ntb+unsubscribe@googlegroups.com <mailto:linux-ntb%2Bunsubscribe@googlegroups.com>.
> >>> To view this discussion on the web visit
> >>> https://groups.google.com/d/msgid/linux-ntb/1573119336-107732-1-git-send-email-linjiasen%40hygon.cn.
> >>>
> >
> > Thanks,
> >
> > Jiasen Lin
>
> -- 
> You received this message because you are subscribed to the Google Groups "linux-ntb" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-ntb+unsubscribe@googlegroups.com <mailto:linux-ntb%2Bunsubscribe@googlegroups.com>.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-ntb/11b355a8-0fe0-f256-c510-ddf106017703%40hygon.cn.

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: Fwd: [PATCH] NTB: Fix an error in get link status
  2019-11-20 14:13         ` Fwd: " Sanjay R Mehta
@ 2019-11-21 13:30           ` Jiasen Lin
  2019-11-22  1:27             ` Jiasen Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Jiasen Lin @ 2019-11-21 13:30 UTC (permalink / raw)
  To: Sanjay R Mehta
  Cc: S-k, Shyam-sundar, Dave Jiang, Allen Hubbe, linux-kernel,
	linux-ntb, linjiasen007



On 2019/11/20 22:13, Sanjay R Mehta wrote:
> From: *Jiasen Lin* <linjiasen@hygon.cn <mailto:linjiasen@hygon.cn>>
>> Date: Wed, Nov 20, 2019 at 3:25 PM
>> Subject: Re: [PATCH] NTB: Fix an error in get link status
>> To: Jon Mason <jdmason@kudzu.us <mailto:jdmason@kudzu.us>>
>> Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com <mailto:Shyam-sundar.S-k@amd.com>>, Dave Jiang <dave.jiang@intel.com <mailto:dave.jiang@intel.com>>, Allen Hubbe <allenbh@gmail.com
>> <mailto:allenbh@gmail.com>>, linux-kernel <linux-kernel@vger.kernel.org <mailto:linux-kernel@vger.kernel.org>>, linux-ntb <linux-ntb@googlegroups.com <mailto:linux-ntb@googlegroups.com>>,
>> <linjiasen007@gmail.com <mailto:linjiasen007@gmail.com>>, Jiasen Lin <linjiasen@hygon.cn <mailto:linjiasen@hygon.cn>>
>>
>>
>>
>>
>> On 2019/11/18 18:17, Jiasen Lin wrote:
>>>
>>>
>>> On 2019/11/18 7:00, Jon Mason wrote:
>>>> On Thu, Nov 7, 2019 at 4:37 AM Jiasen Lin <linjiasen@hygon.cn <mailto:linjiasen@hygon.cn>> wrote:
>>>>>
>>>>> The offset of PCIe Capability Header for AMD and HYGON NTB is 0x64,
>>>>> but the macro which named "AMD_LINK_STATUS_OFFSET" is defined as 0x68.
>>>>> It is offset of Device Capabilities Reg rather than Link Control Reg.
>>>>>
>>>>> This code trigger an error in get link statsus:
>>>>>
>>>>>           cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
>>>>>                   LNK STA -               0x8fa1
>>>>>                   Link Status -           Up
>>>>>                   Link Speed -            PCI-E Gen 0
>>>>>                   Link Width -            x0
>>>>>
>>>>> This patch use pcie_capability_read_dword to get link status.
>>>>> After fix this issue, we can get link status accurately:
>>>>>
>>>>>           cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
>>>>>                   LNK STA -               0x11030042
>>>>>                   Link Status -           Up
>>>>>                   Link Speed -            PCI-E Gen 3
>>>>>                   Link Width -            x16
>>>>
>>>> No response from AMD maintainers, but it looks like you are correct.
>>>>
>>>> This needs a "Fixes:" line here.  I took the liberty of adding one to
>>>> this patch.
>>>>
>>>
>>> Thank you for your suggestions. Yes, this patch fix the commit id:
>>> a1b3695 ("NTB: Add support for AMD PCI-Express Non-Transparent Bridge").
>>>
>>>>> Signed-off-by: Jiasen Lin <linjiasen@hygon.cn <mailto:linjiasen@hygon.cn>>
>>>>> ---
>>>>>    drivers/ntb/hw/amd/ntb_hw_amd.c | 5 +++--
>>>>>    drivers/ntb/hw/amd/ntb_hw_amd.h | 1 -
>>>>>    2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>> b/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>> index 156c2a1..ae91105 100644
>>>>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>> @@ -855,8 +855,8 @@ static int amd_poll_link(struct amd_ntb_dev *ndev)
>>>>>
>>>>>           ndev->cntl_sta = reg;
>>>>>
>>>>> -       rc = pci_read_config_dword(ndev->ntb.pdev,
>>>>> -                                  AMD_LINK_STATUS_OFFSET, &stat);
>>>>> +       rc = pcie_capability_read_dword(ndev->ntb.pdev,
>>>>> +                                  PCI_EXP_LNKCTL, &stat);
>>>>>           if (rc)
>>>>>                   return 0;
>>>>>           ndev->lnk_sta = stat;
>>>>> @@ -1139,6 +1139,7 @@ static const struct ntb_dev_data dev_data[] = {
>>>>>    static const struct pci_device_id amd_ntb_pci_tbl[] = {
>>>>>           { PCI_VDEVICE(AMD, 0x145b), (kernel_ulong_t)&dev_data[0] },
>>>>>           { PCI_VDEVICE(AMD, 0x148b), (kernel_ulong_t)&dev_data[1] },
>>>>> +       { PCI_VDEVICE(HYGON, 0x145b), (kernel_ulong_t)&dev_data[0] },
>>>>
>>>> This should be a separate patch.  I took the liberty of splitting it
>>>> off into a unique patch and attributing it to you.  I've pushed them
>>>> to the ntb-next branch on
>>>> https://github.com/jonmason/ntb
>>>>
>>> Thank you for your comment. We appreciate the time and effort you have
>>> spent to split it off, I will test it ASAP.
>>>
>>>> Please verify everything looks acceptable to you (given the changes I
>>>> did above that are attributed to you).  Also, testing of the latest
>>>> code is always appreciated.
>>>>
>>>> Thanks,
>>>> Jon
>>>>
>>
>> I have tested these patches that are pushed to ntb-next branch, they
>> work well on HYGON platforms.
>>
>> Thanks,
>> Jiasen Lin
> 
> Hi Jiasen Lin,
> 
> Apologies for my delayed response. I was on vacation.
> 
> Your patch is a correct fix, but that would work only for primary system.
> 
> The design of AMD NTB implementation is such that NTB primary acts as an endpoint device and NTB secondary is a PCIe Root Port (RP). Considering that,
> the link status and control register needs to be accessed differently based on the NTB topology.
> 
> So in the case of NTB secondary, we read the link status and control register of the PCIe RP capabilities structure and in the case of NTB primary, we read the
> link status and control register from NTB device capabilities structure.
> 
> The code below is the proper fix for AMD platform. I will be sending incremental change above your patch.
> 
> would appreciate if you could test my patch and let me know whether that works for you.
> 

Dhyana CPU dones not support data transfer while both sides of PCIe link 
are configured as NTB, in other word, Dhyana only support NTB that is 
connected to RP rather than NT to NT.

As illustrated in the following topology, NTB consists of two PCIe 
endpoints, a Primary NTB, and a Secondary NTB. Primary CPU can find 
Priamry NTB, while Secondary NTB, Secondary internal SW.ds and Secondary 
internal SW.ds are enumerated by secondary CPU.

In this topology, to remove any ambiguity, your suggestion is more 
accurate method to get link status of NTB.

In primary PCI domain:
Primary RP--Primary NTB----------------------------------------
40:04.1-------41:00.1(Pri NTB)                                | 

                                                               | 
In secondary PCI domain:                                      |
Secondary RP--Secondary SW.us--Secondary SW.ds--Secondary NTB--
40:03.1---------41:00.0---------42:00.0---------43:00.1(Sec NTB)

I have modified the code according to your suggestion and tested it
on Dhyana platform, it works well, expect to receice your patch.

Before modify the code, read the Link Status and control register of the 
secondary NTB device to get link status.

cat /sys/kernel/debug/ntb_hw_amd/0000\:43\:00.1/info
NTB Device Information:
Connection Topology -   NTB_TOPO_SEC
LNK STA -               0x11030042
Link Status -           Up
Link Speed -            PCI-E Gen 3
Link Width -            x16

After modify the code, read the Link Status and control register of the 
secondary RP to get link status.

cat /sys/kernel/debug/ntb_hw_amd/0000\:43\:00.1/info
NTB Device Information:
Connection Topology -   NTB_TOPO_SEC
LNK STA -               0x70830042
Link Status -           Up
Link Speed -            PCI-E Gen 3
Link Width -            x8

Thanks,
Jiasen Lin

> ---
>   drivers/ntb/hw/amd/ntb_hw_amd.c | 27 +++++++++++++++++++++++----
>   drivers/ntb/hw/amd/ntb_hw_amd.h |  1 -
>   2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c b/drivers/ntb/hw/amd/ntb_hw_amd.c
> index 14ad69c..91e1966 100644
> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
> @@ -842,6 +842,8 @@ static inline void ndev_init_struct(struct amd_ntb_dev *ndev,
>   static int amd_poll_link(struct amd_ntb_dev *ndev)
>   {
>       void __iomem *mmio = ndev->peer_mmio;
> +    struct pci_dev *pci_rp = NULL;
> +    struct pci_dev *pdev = NULL;
>       u32 reg, stat;
>       int rc;
>   
> @@ -855,10 +857,27 @@ static int amd_poll_link(struct amd_ntb_dev *ndev)
>   
>       ndev->cntl_sta = reg;
>   
> -    rc = pci_read_config_dword(ndev->ntb.pdev,
> -                   AMD_LINK_STATUS_OFFSET, &stat);
> -    if (rc)
> -        return 0;
> +    if (ndev->ntb.topo == NTB_TOPO_SEC) {
> +        /* Locate the pointer to PCIe Root Port for this device */
> +        pci_rp = pci_find_pcie_root_port(ndev->ntb.pdev);
> +        /* Read the PCIe Link Control and Status register */
> +        if (pci_rp) {
> +            rc = pcie_capability_read_dword(pci_rp, PCI_EXP_LNKCTL,
> +                            &stat);
> +            if (rc)
> +                return 0;
> +        }
> +    } else if (ndev->ntb.topo == NTB_TOPO_PRI) {
> +        /*
> +         * For NTB primary, we simply read the Link Status and control
> +         * register of the NTB device itself.
> +         */
> +        pdev = ndev->ntb.pdev;
> +        rc = pcie_capability_read_dword(pdev, PCI_EXP_LNKCTL, &stat);
> +        if (rc)
> +            return 0;
> +    }
> +
>       ndev->lnk_sta = stat;
>   
>       return 1;
> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.h b/drivers/ntb/hw/amd/ntb_hw_amd.h
> index 139a307..39e5d18 100644
> --- a/drivers/ntb/hw/amd/ntb_hw_amd.h
> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.h
> @@ -53,7 +53,6 @@
>   #include <linux/pci.h>
>   
>   #define AMD_LINK_HB_TIMEOUT    msecs_to_jiffies(1000)
> -#define AMD_LINK_STATUS_OFFSET    0x68
>   #define NTB_LIN_STA_ACTIVE_BIT    0x00000002
>   #define NTB_LNK_STA_SPEED_MASK    0x000F0000
>   #define NTB_LNK_STA_WIDTH_MASK    0x03F00000
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fwd: [PATCH] NTB: Fix an error in get link status
  2019-11-21 13:30           ` Jiasen Lin
@ 2019-11-22  1:27             ` Jiasen Lin
  2019-11-26 13:10               ` Sanjay R Mehta
  0 siblings, 1 reply; 10+ messages in thread
From: Jiasen Lin @ 2019-11-22  1:27 UTC (permalink / raw)
  To: Sanjay R Mehta
  Cc: S-k, Shyam-sundar, Dave Jiang, Allen Hubbe, linux-kernel,
	linux-ntb, linjiasen007



On 2019/11/21 21:30, Jiasen Lin wrote:
> 
> 
> On 2019/11/20 22:13, Sanjay R Mehta wrote:
>> From: *Jiasen Lin* <linjiasen@hygon.cn <mailto:linjiasen@hygon.cn>>
>>> Date: Wed, Nov 20, 2019 at 3:25 PM
>>> Subject: Re: [PATCH] NTB: Fix an error in get link status
>>> To: Jon Mason <jdmason@kudzu.us <mailto:jdmason@kudzu.us>>
>>> Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com 
>>> <mailto:Shyam-sundar.S-k@amd.com>>, Dave Jiang <dave.jiang@intel.com 
>>> <mailto:dave.jiang@intel.com>>, Allen Hubbe <allenbh@gmail.com
>>> <mailto:allenbh@gmail.com>>, linux-kernel 
>>> <linux-kernel@vger.kernel.org <mailto:linux-kernel@vger.kernel.org>>, 
>>> linux-ntb <linux-ntb@googlegroups.com 
>>> <mailto:linux-ntb@googlegroups.com>>,
>>> <linjiasen007@gmail.com <mailto:linjiasen007@gmail.com>>, Jiasen Lin 
>>> <linjiasen@hygon.cn <mailto:linjiasen@hygon.cn>>
>>>
>>>
>>>
>>>
>>> On 2019/11/18 18:17, Jiasen Lin wrote:
>>>>
>>>>
>>>> On 2019/11/18 7:00, Jon Mason wrote:
>>>>> On Thu, Nov 7, 2019 at 4:37 AM Jiasen Lin <linjiasen@hygon.cn 
>>>>> <mailto:linjiasen@hygon.cn>> wrote:
>>>>>>
>>>>>> The offset of PCIe Capability Header for AMD and HYGON NTB is 0x64,
>>>>>> but the macro which named "AMD_LINK_STATUS_OFFSET" is defined as 
>>>>>> 0x68.
>>>>>> It is offset of Device Capabilities Reg rather than Link Control Reg.
>>>>>>
>>>>>> This code trigger an error in get link statsus:
>>>>>>
>>>>>>           cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
>>>>>>                   LNK STA -               0x8fa1
>>>>>>                   Link Status -           Up
>>>>>>                   Link Speed -            PCI-E Gen 0
>>>>>>                   Link Width -            x0
>>>>>>
>>>>>> This patch use pcie_capability_read_dword to get link status.
>>>>>> After fix this issue, we can get link status accurately:
>>>>>>
>>>>>>           cat /sys/kernel/debug/ntb_hw_amd/0000:43:00.1/info
>>>>>>                   LNK STA -               0x11030042
>>>>>>                   Link Status -           Up
>>>>>>                   Link Speed -            PCI-E Gen 3
>>>>>>                   Link Width -            x16
>>>>>
>>>>> No response from AMD maintainers, but it looks like you are correct.
>>>>>
>>>>> This needs a "Fixes:" line here.  I took the liberty of adding one to
>>>>> this patch.
>>>>>
>>>>
>>>> Thank you for your suggestions. Yes, this patch fix the commit id:
>>>> a1b3695 ("NTB: Add support for AMD PCI-Express Non-Transparent 
>>>> Bridge").
>>>>
>>>>>> Signed-off-by: Jiasen Lin <linjiasen@hygon.cn 
>>>>>> <mailto:linjiasen@hygon.cn>>
>>>>>> ---
>>>>>>    drivers/ntb/hw/amd/ntb_hw_amd.c | 5 +++--
>>>>>>    drivers/ntb/hw/amd/ntb_hw_amd.h | 1 -
>>>>>>    2 files changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>>> b/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>>> index 156c2a1..ae91105 100644
>>>>>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>>> @@ -855,8 +855,8 @@ static int amd_poll_link(struct amd_ntb_dev 
>>>>>> *ndev)
>>>>>>
>>>>>>           ndev->cntl_sta = reg;
>>>>>>
>>>>>> -       rc = pci_read_config_dword(ndev->ntb.pdev,
>>>>>> -                                  AMD_LINK_STATUS_OFFSET, &stat);
>>>>>> +       rc = pcie_capability_read_dword(ndev->ntb.pdev,
>>>>>> +                                  PCI_EXP_LNKCTL, &stat);
>>>>>>           if (rc)
>>>>>>                   return 0;
>>>>>>           ndev->lnk_sta = stat;
>>>>>> @@ -1139,6 +1139,7 @@ static const struct ntb_dev_data dev_data[] = {
>>>>>>    static const struct pci_device_id amd_ntb_pci_tbl[] = {
>>>>>>           { PCI_VDEVICE(AMD, 0x145b), (kernel_ulong_t)&dev_data[0] },
>>>>>>           { PCI_VDEVICE(AMD, 0x148b), (kernel_ulong_t)&dev_data[1] },
>>>>>> +       { PCI_VDEVICE(HYGON, 0x145b), (kernel_ulong_t)&dev_data[0] },
>>>>>
>>>>> This should be a separate patch.  I took the liberty of splitting it
>>>>> off into a unique patch and attributing it to you.  I've pushed them
>>>>> to the ntb-next branch on
>>>>> https://github.com/jonmason/ntb
>>>>>
>>>> Thank you for your comment. We appreciate the time and effort you have
>>>> spent to split it off, I will test it ASAP.
>>>>
>>>>> Please verify everything looks acceptable to you (given the changes I
>>>>> did above that are attributed to you).  Also, testing of the latest
>>>>> code is always appreciated.
>>>>>
>>>>> Thanks,
>>>>> Jon
>>>>>
>>>
>>> I have tested these patches that are pushed to ntb-next branch, they
>>> work well on HYGON platforms.
>>>
>>> Thanks,
>>> Jiasen Lin
>>
>> Hi Jiasen Lin,
>>
>> Apologies for my delayed response. I was on vacation.
>>
>> Your patch is a correct fix, but that would work only for primary system.
>>
>> The design of AMD NTB implementation is such that NTB primary acts as 
>> an endpoint device and NTB secondary is a PCIe Root Port (RP). 
>> Considering that,
>> the link status and control register needs to be accessed differently 
>> based on the NTB topology.
>>
>> So in the case of NTB secondary, we read the link status and control 
>> register of the PCIe RP capabilities structure and in the case of NTB 
>> primary, we read the
>> link status and control register from NTB device capabilities structure.
>>
>> The code below is the proper fix for AMD platform. I will be sending 
>> incremental change above your patch.
>>
>> would appreciate if you could test my patch and let me know whether 
>> that works for you.
>>
> 
> Dhyana CPU dones not support data transfer while both sides of PCIe link 
> are configured as NTB, in other word, Dhyana only support NTB that is 
> connected to RP rather than NT to NT.
> 
> As illustrated in the following topology, NTB consists of two PCIe 
> endpoints, a Primary NTB, and a Secondary NTB. Primary CPU can find 
> Priamry NTB, while Secondary NTB, Secondary internal SW.ds and Secondary 
> internal SW.ds are enumerated by secondary CPU.
> 
> In this topology, to remove any ambiguity, your suggestion is more 
> accurate method to get link status of NTB.
> 
> In primary PCI domain:
> Primary RP--Primary NTB----------------------------------------
> 40:04.1-------41:00.1(Pri NTB)                                |
>                                                                | In 
> secondary PCI domain:                                      |
> Secondary RP--Secondary SW.us--Secondary SW.ds--Secondary NTB--
> 40:03.1---------41:00.0---------42:00.0---------43:00.1(Sec NTB)
> 

Hi Sanjay R Mehta

In more complex topology, read the Link Status and Control register of 
RP is also wrong. Suppose that a PCIe switch bridge to the Secondary RP, 
and Secondary internal SW.ds is a child device for switch's downstream 
port as illustrated in the following topology.

In secondary PCI domain:
Secondary RP--Switch UP--Switch DP--Secondary internal SW.us--Secondary 
internal SW.ds--Secondary NTB

pci_rp = pci_find_pcie_root_port(ndev->ntb.pdev) will return the 
Secondary RP, and pcie_capability_read_dword(pci_rp, 
PCI_EXP_LNKCTL,&stat) will get the link status between Secondary RP and 
Switch UP. Maybe, read the Link Status and control register of Secondary 
internal SW.us is appropriate.

struct pci_dev *pci_up = NULL;
struct pci_dev *pci_dp = NULL;

if (ndev->ntb.topo == NTB_TOPO_SEC) {
     /* Locate the pointer to Secondary up for this device */
     pci_dp = pci_upstream_bridge(ndev->ntb.pdev);
     /* Read the PCIe Link Control and Status register */
     if (pci_dp) {
	pci_up = pci_upstream_bridge(pci_dp);
	if (pci_up) {
		rc = pcie_capability_read_dword(pci_up, PCI_EXP_LNKCTL,
                         &stat);
         	if (rc)
            		return 0;
		}	
	}
}

Thanks,
Jiansen Lin

> I have modified the code according to your suggestion and tested it
> on Dhyana platform, it works well, expect to receice your patch.
> 
> Before modify the code, read the Link Status and control register of the 
> secondary NTB device to get link status.
> 
> cat /sys/kernel/debug/ntb_hw_amd/0000\:43\:00.1/info
> NTB Device Information:
> Connection Topology -   NTB_TOPO_SEC
> LNK STA -               0x11030042
> Link Status -           Up
> Link Speed -            PCI-E Gen 3
> Link Width -            x16
> 
> After modify the code, read the Link Status and control register of the 
> secondary RP to get link status.
> 
> cat /sys/kernel/debug/ntb_hw_amd/0000\:43\:00.1/info
> NTB Device Information:
> Connection Topology -   NTB_TOPO_SEC
> LNK STA -               0x70830042
> Link Status -           Up
> Link Speed -            PCI-E Gen 3
> Link Width -            x8
> 
> Thanks,
> Jiasen Lin
> 
>> ---
>>   drivers/ntb/hw/amd/ntb_hw_amd.c | 27 +++++++++++++++++++++++----
>>   drivers/ntb/hw/amd/ntb_hw_amd.h |  1 -
>>   2 files changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c 
>> b/drivers/ntb/hw/amd/ntb_hw_amd.c
>> index 14ad69c..91e1966 100644
>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
>> @@ -842,6 +842,8 @@ static inline void ndev_init_struct(struct 
>> amd_ntb_dev *ndev,
>>   static int amd_poll_link(struct amd_ntb_dev *ndev)
>>   {
>>       void __iomem *mmio = ndev->peer_mmio;
>> +    struct pci_dev *pci_rp = NULL;
>> +    struct pci_dev *pdev = NULL;
>>       u32 reg, stat;
>>       int rc;
>> @@ -855,10 +857,27 @@ static int amd_poll_link(struct amd_ntb_dev *ndev)
>>       ndev->cntl_sta = reg;
>> -    rc = pci_read_config_dword(ndev->ntb.pdev,
>> -                   AMD_LINK_STATUS_OFFSET, &stat);
>> -    if (rc)
>> -        return 0;
>> +    if (ndev->ntb.topo == NTB_TOPO_SEC) {
>> +        /* Locate the pointer to PCIe Root Port for this device */
>> +        pci_rp = pci_find_pcie_root_port(ndev->ntb.pdev);
>> +        /* Read the PCIe Link Control and Status register */
>> +        if (pci_rp) {
>> +            rc = pcie_capability_read_dword(pci_rp, PCI_EXP_LNKCTL,
>> +                            &stat);
>> +            if (rc)
>> +                return 0;
>> +        }
>> +    } else if (ndev->ntb.topo == NTB_TOPO_PRI) {
>> +        /*
>> +         * For NTB primary, we simply read the Link Status and control
>> +         * register of the NTB device itself.
>> +         */
>> +        pdev = ndev->ntb.pdev;
>> +        rc = pcie_capability_read_dword(pdev, PCI_EXP_LNKCTL, &stat);
>> +        if (rc)
>> +            return 0;
>> +    }
>> +
>>       ndev->lnk_sta = stat;
>>       return 1;
>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.h 
>> b/drivers/ntb/hw/amd/ntb_hw_amd.h
>> index 139a307..39e5d18 100644
>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.h
>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.h
>> @@ -53,7 +53,6 @@
>>   #include <linux/pci.h>
>>   #define AMD_LINK_HB_TIMEOUT    msecs_to_jiffies(1000)
>> -#define AMD_LINK_STATUS_OFFSET    0x68
>>   #define NTB_LIN_STA_ACTIVE_BIT    0x00000002
>>   #define NTB_LNK_STA_SPEED_MASK    0x000F0000
>>   #define NTB_LNK_STA_WIDTH_MASK    0x03F00000
>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fwd: [PATCH] NTB: Fix an error in get link status
  2019-11-22  1:27             ` Jiasen Lin
@ 2019-11-26 13:10               ` Sanjay R Mehta
  2019-11-26 14:35                 ` Nath, Arindam
  0 siblings, 1 reply; 10+ messages in thread
From: Sanjay R Mehta @ 2019-11-26 13:10 UTC (permalink / raw)
  To: Jiasen Lin
  Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com> Dave Jiang,
	Arindam Nath, Allen Hubbe, linux-kernel, linux-ntb, linjiasen007


> Hi Sanjay R Mehta
>
> In more complex topology, read the Link Status and Control register of
> RP is also wrong. Suppose that a PCIe switch bridge to the Secondary RP,
> and Secondary internal SW.ds is a child device for switch's downstream
> port as illustrated in the following topology.
>
> In secondary PCI domain:
> Secondary RP--Switch UP--Switch DP--Secondary internal SW.us--Secondary
> internal SW.ds--Secondary NTB
>
> pci_rp = pci_find_pcie_root_port(ndev->ntb.pdev) will return the
> Secondary RP, and pcie_capability_read_dword(pci_rp,
> PCI_EXP_LNKCTL,&stat) will get the link status between Secondary RP and
> Switch UP. Maybe, read the Link Status and control register of Secondary
> internal SW.us is appropriate.
>
Hi Jiansen Lin,

I modified the code as per your suggestion and is working fine.
Adding Arindam for comments who was the co-author of the patch I was about to send for upstream review.

Thanks,
Sanjay Mehta
> struct pci_dev *pci_up = NULL;
> struct pci_dev *pci_dp = NULL;
>
> if (ndev->ntb.topo == NTB_TOPO_SEC) {
>     /* Locate the pointer to Secondary up for this device */
>     pci_dp = pci_upstream_bridge(ndev->ntb.pdev);
>     /* Read the PCIe Link Control and Status register */
>     if (pci_dp) {
>        pci_up = pci_upstream_bridge(pci_dp);
>        if (pci_up) {
>                rc = pcie_capability_read_dword(pci_up, PCI_EXP_LNKCTL,
>                         &stat);
>                if (rc)
>                        return 0;
>                }
>        }
> }
>
> Thanks,
> Jiansen Lin
>
>> I have modified the code according to your suggestion and tested it
>> on Dhyana platform, it works well, expect to receice your patch.
>>
>> Before modify the code, read the Link Status and control register of the
>> secondary NTB device to get link status.
>>
>> cat /sys/kernel/debug/ntb_hw_amd/0000\:43\:00.1/info
>> NTB Device Information:
>> Connection Topology -   NTB_TOPO_SEC
>> LNK STA -               0x11030042
>> Link Status -           Up
>> Link Speed -            PCI-E Gen 3
>> Link Width -            x16
>>
>> After modify the code, read the Link Status and control register of the
>> secondary RP to get link status.
>>
>> cat /sys/kernel/debug/ntb_hw_amd/0000\:43\:00.1/info
>> NTB Device Information:
>> Connection Topology -   NTB_TOPO_SEC
>> LNK STA -               0x70830042
>> Link Status -           Up
>> Link Speed -            PCI-E Gen 3
>> Link Width -            x8
>>
>> Thanks,
>> Jiasen Lin
>>
>>> ---
>>>   drivers/ntb/hw/amd/ntb_hw_amd.c | 27 +++++++++++++++++++++++----
>>>   drivers/ntb/hw/amd/ntb_hw_amd.h |  1 -
>>>   2 files changed, 23 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c
>>> b/drivers/ntb/hw/amd/ntb_hw_amd.c
>>> index 14ad69c..91e1966 100644
>>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
>>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
>>> @@ -842,6 +842,8 @@ static inline void ndev_init_struct(struct
>>> amd_ntb_dev *ndev,
>>>   static int amd_poll_link(struct amd_ntb_dev *ndev)
>>>   {
>>>       void __iomem *mmio = ndev->peer_mmio;
>>> +    struct pci_dev *pci_rp = NULL;
>>> +    struct pci_dev *pdev = NULL;
>>>       u32 reg, stat;
>>>       int rc;
>>> @@ -855,10 +857,27 @@ static int amd_poll_link(struct amd_ntb_dev *ndev)
>>>       ndev->cntl_sta = reg;
>>> -    rc = pci_read_config_dword(ndev->ntb.pdev,
>>> -                   AMD_LINK_STATUS_OFFSET, &stat);
>>> -    if (rc)
>>> -        return 0;
>>> +    if (ndev->ntb.topo == NTB_TOPO_SEC) {
>>> +        /* Locate the pointer to PCIe Root Port for this device */
>>> +        pci_rp = pci_find_pcie_root_port(ndev->ntb.pdev);
>>> +        /* Read the PCIe Link Control and Status register */
>>> +        if (pci_rp) {
>>> +            rc = pcie_capability_read_dword(pci_rp, PCI_EXP_LNKCTL,
>>> +                            &stat);
>>> +            if (rc)
>>> +                return 0;
>>> +        }
>>> +    } else if (ndev->ntb.topo == NTB_TOPO_PRI) {
>>> +        /*
>>> +         * For NTB primary, we simply read the Link Status and control
>>> +         * register of the NTB device itself.
>>> +         */
>>> +        pdev = ndev->ntb.pdev;
>>> +        rc = pcie_capability_read_dword(pdev, PCI_EXP_LNKCTL, &stat);
>>> +        if (rc)
>>> +            return 0;
>>> +    }
>>> +
>>>       ndev->lnk_sta = stat;
>>>       return 1;
>>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.h
>>> b/drivers/ntb/hw/amd/ntb_hw_amd.h
>>> index 139a307..39e5d18 100644
>>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.h
>>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.h
>>> @@ -53,7 +53,6 @@
>>>   #include <linux/pci.h>
>>>   #define AMD_LINK_HB_TIMEOUT    msecs_to_jiffies(1000)
>>> -#define AMD_LINK_STATUS_OFFSET    0x68
>>>   #define NTB_LIN_STA_ACTIVE_BIT    0x00000002
>>>   #define NTB_LNK_STA_SPEED_MASK    0x000F0000
>>>   #define NTB_LNK_STA_WIDTH_MASK    0x03F00000
>>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: Fwd: [PATCH] NTB: Fix an error in get link status
  2019-11-26 13:10               ` Sanjay R Mehta
@ 2019-11-26 14:35                 ` Nath, Arindam
  2019-12-03  2:54                   ` Jiasen Lin
  0 siblings, 1 reply; 10+ messages in thread
From: Nath, Arindam @ 2019-11-26 14:35 UTC (permalink / raw)
  To: Mehta, Sanju, Jiasen Lin
  Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com> Dave Jiang,
	Allen Hubbe, linux-kernel, linux-ntb, linjiasen007

> -----Original Message-----
> From: Mehta, Sanju <Sanju.Mehta@amd.com>
> Sent: Tuesday, November 26, 2019 18:40
> To: Jiasen Lin <linjiasen@hygon.cn>
> Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com> Dave Jiang
> <dave.jiang@intel.com>; Nath, Arindam <Arindam.Nath@amd.com>; Allen
> Hubbe <allenbh@gmail.com>; linux-kernel <linux-kernel@vger.kernel.org>;
> linux-ntb <linux-ntb@googlegroups.com>; linjiasen007@gmail.com
> Subject: Re: Fwd: [PATCH] NTB: Fix an error in get link status
> 
> 
> > Hi Sanjay R Mehta
> >
> > In more complex topology, read the Link Status and Control register of
> > RP is also wrong. Suppose that a PCIe switch bridge to the Secondary RP,
> > and Secondary internal SW.ds is a child device for switch's downstream
> > port as illustrated in the following topology.
> >
> > In secondary PCI domain:
> > Secondary RP--Switch UP--Switch DP--Secondary internal SW.us--
> Secondary
> > internal SW.ds--Secondary NTB
> >
> > pci_rp = pci_find_pcie_root_port(ndev->ntb.pdev) will return the
> > Secondary RP, and pcie_capability_read_dword(pci_rp,
> > PCI_EXP_LNKCTL,&stat) will get the link status between Secondary RP and
> > Switch UP. Maybe, read the Link Status and control register of Secondary
> > internal SW.us is appropriate.
> >
> Hi Jiansen Lin,
> 
> I modified the code as per your suggestion and is working fine.
> Adding Arindam for comments who was the co-author of the patch I was
> about to send for upstream review.

Hi Jiansen Lin,

I am okay with the changes proposed by you. But one thing we need to keep
in mind is that, the configuration SWUS+SWDS+EP as visible from the NTB
secondary, might change in future AMD implementations where these internal
switches are not present anymore. So we might have to re-visit the patch
again later.

Thanks,
Arindam

> 
> Thanks,
> Sanjay Mehta
> > struct pci_dev *pci_up = NULL;
> > struct pci_dev *pci_dp = NULL;
> >
> > if (ndev->ntb.topo == NTB_TOPO_SEC) {
> >     /* Locate the pointer to Secondary up for this device */
> >     pci_dp = pci_upstream_bridge(ndev->ntb.pdev);
> >     /* Read the PCIe Link Control and Status register */
> >     if (pci_dp) {
> >        pci_up = pci_upstream_bridge(pci_dp);
> >        if (pci_up) {
> >                rc = pcie_capability_read_dword(pci_up, PCI_EXP_LNKCTL,
> >                         &stat);
> >                if (rc)
> >                        return 0;
> >                }
> >        }
> > }
> >
> > Thanks,
> > Jiansen Lin
> >
> >> I have modified the code according to your suggestion and tested it
> >> on Dhyana platform, it works well, expect to receice your patch.
> >>
> >> Before modify the code, read the Link Status and control register of the
> >> secondary NTB device to get link status.
> >>
> >> cat /sys/kernel/debug/ntb_hw_amd/0000\:43\:00.1/info
> >> NTB Device Information:
> >> Connection Topology -   NTB_TOPO_SEC
> >> LNK STA -               0x11030042
> >> Link Status -           Up
> >> Link Speed -            PCI-E Gen 3
> >> Link Width -            x16
> >>
> >> After modify the code, read the Link Status and control register of the
> >> secondary RP to get link status.
> >>
> >> cat /sys/kernel/debug/ntb_hw_amd/0000\:43\:00.1/info
> >> NTB Device Information:
> >> Connection Topology -   NTB_TOPO_SEC
> >> LNK STA -               0x70830042
> >> Link Status -           Up
> >> Link Speed -            PCI-E Gen 3
> >> Link Width -            x8
> >>
> >> Thanks,
> >> Jiasen Lin
> >>
> >>> ---
> >>>   drivers/ntb/hw/amd/ntb_hw_amd.c | 27
> +++++++++++++++++++++++----
> >>>   drivers/ntb/hw/amd/ntb_hw_amd.h |  1 -
> >>>   2 files changed, 23 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c
> >>> b/drivers/ntb/hw/amd/ntb_hw_amd.c
> >>> index 14ad69c..91e1966 100644
> >>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
> >>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
> >>> @@ -842,6 +842,8 @@ static inline void ndev_init_struct(struct
> >>> amd_ntb_dev *ndev,
> >>>   static int amd_poll_link(struct amd_ntb_dev *ndev)
> >>>   {
> >>>       void __iomem *mmio = ndev->peer_mmio;
> >>> +    struct pci_dev *pci_rp = NULL;
> >>> +    struct pci_dev *pdev = NULL;
> >>>       u32 reg, stat;
> >>>       int rc;
> >>> @@ -855,10 +857,27 @@ static int amd_poll_link(struct amd_ntb_dev
> *ndev)
> >>>       ndev->cntl_sta = reg;
> >>> -    rc = pci_read_config_dword(ndev->ntb.pdev,
> >>> -                   AMD_LINK_STATUS_OFFSET, &stat);
> >>> -    if (rc)
> >>> -        return 0;
> >>> +    if (ndev->ntb.topo == NTB_TOPO_SEC) {
> >>> +        /* Locate the pointer to PCIe Root Port for this device */
> >>> +        pci_rp = pci_find_pcie_root_port(ndev->ntb.pdev);
> >>> +        /* Read the PCIe Link Control and Status register */
> >>> +        if (pci_rp) {
> >>> +            rc = pcie_capability_read_dword(pci_rp, PCI_EXP_LNKCTL,
> >>> +                            &stat);
> >>> +            if (rc)
> >>> +                return 0;
> >>> +        }
> >>> +    } else if (ndev->ntb.topo == NTB_TOPO_PRI) {
> >>> +        /*
> >>> +         * For NTB primary, we simply read the Link Status and control
> >>> +         * register of the NTB device itself.
> >>> +         */
> >>> +        pdev = ndev->ntb.pdev;
> >>> +        rc = pcie_capability_read_dword(pdev, PCI_EXP_LNKCTL, &stat);
> >>> +        if (rc)
> >>> +            return 0;
> >>> +    }
> >>> +
> >>>       ndev->lnk_sta = stat;
> >>>       return 1;
> >>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.h
> >>> b/drivers/ntb/hw/amd/ntb_hw_amd.h
> >>> index 139a307..39e5d18 100644
> >>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.h
> >>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.h
> >>> @@ -53,7 +53,6 @@
> >>>   #include <linux/pci.h>
> >>>   #define AMD_LINK_HB_TIMEOUT    msecs_to_jiffies(1000)
> >>> -#define AMD_LINK_STATUS_OFFSET    0x68
> >>>   #define NTB_LIN_STA_ACTIVE_BIT    0x00000002
> >>>   #define NTB_LNK_STA_SPEED_MASK    0x000F0000
> >>>   #define NTB_LNK_STA_WIDTH_MASK    0x03F00000
> >>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: Fwd: [PATCH] NTB: Fix an error in get link status
  2019-11-26 14:35                 ` Nath, Arindam
@ 2019-12-03  2:54                   ` Jiasen Lin
  0 siblings, 0 replies; 10+ messages in thread
From: Jiasen Lin @ 2019-12-03  2:54 UTC (permalink / raw)
  To: Nath, Arindam, Mehta, Sanju
  Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com> Dave Jiang,
	Allen Hubbe, linux-kernel, linux-ntb, linjiasen007



On 2019/11/26 22:35, Nath, Arindam wrote:
>> -----Original Message-----
>> From: Mehta, Sanju <Sanju.Mehta@amd.com>
>> Sent: Tuesday, November 26, 2019 18:40
>> To: Jiasen Lin <linjiasen@hygon.cn>
>> Cc: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com> Dave Jiang
>> <dave.jiang@intel.com>; Nath, Arindam <Arindam.Nath@amd.com>; Allen
>> Hubbe <allenbh@gmail.com>; linux-kernel <linux-kernel@vger.kernel.org>;
>> linux-ntb <linux-ntb@googlegroups.com>; linjiasen007@gmail.com
>> Subject: Re: Fwd: [PATCH] NTB: Fix an error in get link status
>>
>>
>>> Hi Sanjay R Mehta
>>>
>>> In more complex topology, read the Link Status and Control register of
>>> RP is also wrong. Suppose that a PCIe switch bridge to the Secondary RP,
>>> and Secondary internal SW.ds is a child device for switch's downstream
>>> port as illustrated in the following topology.
>>>
>>> In secondary PCI domain:
>>> Secondary RP--Switch UP--Switch DP--Secondary internal SW.us--
>> Secondary
>>> internal SW.ds--Secondary NTB
>>>
>>> pci_rp = pci_find_pcie_root_port(ndev->ntb.pdev) will return the
>>> Secondary RP, and pcie_capability_read_dword(pci_rp,
>>> PCI_EXP_LNKCTL,&stat) will get the link status between Secondary RP and
>>> Switch UP. Maybe, read the Link Status and control register of Secondary
>>> internal SW.us is appropriate.
>>>
>> Hi Jiansen Lin,
>>
>> I modified the code as per your suggestion and is working fine.
>> Adding Arindam for comments who was the co-author of the patch I was
>> about to send for upstream review.
> 
> Hi Jiansen Lin,
> 
> I am okay with the changes proposed by you. But one thing we need to keep
> in mind is that, the configuration SWUS+SWDS+EP as visible from the NTB
> secondary, might change in future AMD implementations where these internal
> switches are not present anymore. So we might have to re-visit the patch
> again later.
> 
> Thanks,
> Arindam
> 

Hi Adridam
We can define a depth value that from secondary NTB EP to the real link 
training device for the various devices, if internal switch is not
presnt in future. In amd_poll_link we traverse up the parent chain utill 
the depth is reached.
Now, for device 145b, the depth is defined to 2, because only one
internal switch is implemented for secondary NTB. For device 148b, maybe
also one internal switch, I guess.

static const struct ntb_dev_data dev_data[] = {
	{ /* for device 145b */
		.mw_count = 3,
		.mw_idx = 1,
		.depth = 2,
	},
	{ /* for device 148b */
		.mw_count = 2,
		.mw_idx = 2,
		.depth = 2,/*maybe is 2, according to implementation of the 148b */
	},
};

static const struct pci_device_id amd_ntb_pci_tbl[] = {
	{ PCI_VDEVICE(AMD, 0x145b), (kernel_ulong_t)&dev_data[0] },
	{ PCI_VDEVICE(AMD, 0x148b), (kernel_ulong_t)&dev_data[1] },
	{ PCI_VDEVICE(HYGON, 0x145b), (kernel_ulong_t)&dev_data[0] },
	{ 0, }
};

Thanks,
Jiasen Lin
>>
>> Thanks,
>> Sanjay Mehta
>>> struct pci_dev *pci_up = NULL;
>>> struct pci_dev *pci_dp = NULL;
>>>
>>> if (ndev->ntb.topo == NTB_TOPO_SEC) {
>>>      /* Locate the pointer to Secondary up for this device */
>>>      pci_dp = pci_upstream_bridge(ndev->ntb.pdev);
>>>      /* Read the PCIe Link Control and Status register */
>>>      if (pci_dp) {
>>>         pci_up = pci_upstream_bridge(pci_dp);
>>>         if (pci_up) {
>>>                 rc = pcie_capability_read_dword(pci_up, PCI_EXP_LNKCTL,
>>>                          &stat);
>>>                 if (rc)
>>>                         return 0;
>>>                 }
>>>         }
>>> }
>>>
>>> Thanks,
>>> Jiansen Lin
>>>
>>>> I have modified the code according to your suggestion and tested it
>>>> on Dhyana platform, it works well, expect to receice your patch.
>>>>
>>>> Before modify the code, read the Link Status and control register of the
>>>> secondary NTB device to get link status.
>>>>
>>>> cat /sys/kernel/debug/ntb_hw_amd/0000\:43\:00.1/info
>>>> NTB Device Information:
>>>> Connection Topology -   NTB_TOPO_SEC
>>>> LNK STA -               0x11030042
>>>> Link Status -           Up
>>>> Link Speed -            PCI-E Gen 3
>>>> Link Width -            x16
>>>>
>>>> After modify the code, read the Link Status and control register of the
>>>> secondary RP to get link status.
>>>>
>>>> cat /sys/kernel/debug/ntb_hw_amd/0000\:43\:00.1/info
>>>> NTB Device Information:
>>>> Connection Topology -   NTB_TOPO_SEC
>>>> LNK STA -               0x70830042
>>>> Link Status -           Up
>>>> Link Speed -            PCI-E Gen 3
>>>> Link Width -            x8
>>>>
>>>> Thanks,
>>>> Jiasen Lin
>>>>
>>>>> ---
>>>>>    drivers/ntb/hw/amd/ntb_hw_amd.c | 27
>> +++++++++++++++++++++++----
>>>>>    drivers/ntb/hw/amd/ntb_hw_amd.h |  1 -
>>>>>    2 files changed, 23 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>> b/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>> index 14ad69c..91e1966 100644
>>>>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.c
>>>>> @@ -842,6 +842,8 @@ static inline void ndev_init_struct(struct
>>>>> amd_ntb_dev *ndev,
>>>>>    static int amd_poll_link(struct amd_ntb_dev *ndev)
>>>>>    {
>>>>>        void __iomem *mmio = ndev->peer_mmio;
>>>>> +    struct pci_dev *pci_rp = NULL;
>>>>> +    struct pci_dev *pdev = NULL;
>>>>>        u32 reg, stat;
>>>>>        int rc;
>>>>> @@ -855,10 +857,27 @@ static int amd_poll_link(struct amd_ntb_dev
>> *ndev)
>>>>>        ndev->cntl_sta = reg;
>>>>> -    rc = pci_read_config_dword(ndev->ntb.pdev,
>>>>> -                   AMD_LINK_STATUS_OFFSET, &stat);
>>>>> -    if (rc)
>>>>> -        return 0;
>>>>> +    if (ndev->ntb.topo == NTB_TOPO_SEC) {
>>>>> +        /* Locate the pointer to PCIe Root Port for this device */
>>>>> +        pci_rp = pci_find_pcie_root_port(ndev->ntb.pdev);
>>>>> +        /* Read the PCIe Link Control and Status register */
>>>>> +        if (pci_rp) {
>>>>> +            rc = pcie_capability_read_dword(pci_rp, PCI_EXP_LNKCTL,
>>>>> +                            &stat);
>>>>> +            if (rc)
>>>>> +                return 0;
>>>>> +        }
>>>>> +    } else if (ndev->ntb.topo == NTB_TOPO_PRI) {
>>>>> +        /*
>>>>> +         * For NTB primary, we simply read the Link Status and control
>>>>> +         * register of the NTB device itself.
>>>>> +         */
>>>>> +        pdev = ndev->ntb.pdev;
>>>>> +        rc = pcie_capability_read_dword(pdev, PCI_EXP_LNKCTL, &stat);
>>>>> +        if (rc)
>>>>> +            return 0;
>>>>> +    }
>>>>> +
>>>>>        ndev->lnk_sta = stat;
>>>>>        return 1;
>>>>> diff --git a/drivers/ntb/hw/amd/ntb_hw_amd.h
>>>>> b/drivers/ntb/hw/amd/ntb_hw_amd.h
>>>>> index 139a307..39e5d18 100644
>>>>> --- a/drivers/ntb/hw/amd/ntb_hw_amd.h
>>>>> +++ b/drivers/ntb/hw/amd/ntb_hw_amd.h
>>>>> @@ -53,7 +53,6 @@
>>>>>    #include <linux/pci.h>
>>>>>    #define AMD_LINK_HB_TIMEOUT    msecs_to_jiffies(1000)
>>>>> -#define AMD_LINK_STATUS_OFFSET    0x68
>>>>>    #define NTB_LIN_STA_ACTIVE_BIT    0x00000002
>>>>>    #define NTB_LNK_STA_SPEED_MASK    0x000F0000
>>>>>    #define NTB_LNK_STA_WIDTH_MASK    0x03F00000
>>>>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-12-03  2:58 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-07  9:35 [PATCH] NTB: Fix an error in get link status Jiasen Lin
2019-11-17 23:00 ` Jon Mason
2019-11-18 10:17   ` Jiasen Lin
2019-11-20  9:52     ` Jiasen Lin
     [not found]       ` <CAADLhr7bpb-F0eF1UFXy7AcN=z061mno_QsqGE8z-mvWKvUyCQ@mail.gmail.com>
2019-11-20 14:13         ` Fwd: " Sanjay R Mehta
2019-11-21 13:30           ` Jiasen Lin
2019-11-22  1:27             ` Jiasen Lin
2019-11-26 13:10               ` Sanjay R Mehta
2019-11-26 14:35                 ` Nath, Arindam
2019-12-03  2:54                   ` Jiasen Lin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).