diff mbox series

[v1] net/axgbe: Add a HW quirk for register definitions

Message ID 20191210152915.9544-1-Selwin.Sebastian@amd.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v1] net/axgbe: Add a HW quirk for register definitions |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation fail Compilation issues
ci/iol-intel-Performance fail Performance Testing issues
ci/iol-testing fail Testing issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot warning Travis build: failed

Commit Message

Sebastian, Selwin Dec. 10, 2019, 3:29 p.m. UTC 
  V1000/R1000 processors are using the same PCI ids for the network
device but has altered register definitions for determining the
window settings for the indirect PCS access.Add support to check
for this hardware and if found use the new register values

Signed-off-by: Selwin Sebastian <selwin.sebastian@amd.com>
---
 drivers/net/axgbe/axgbe_common.h |  2 ++
 drivers/net/axgbe/axgbe_ethdev.c | 18 +++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

Comments

Ferruh Yigit Dec. 11, 2019, 11:42 a.m. UTC | #1 
On 12/10/2019 3:29 PM, Selwin Sebastian wrote:
> V1000/R1000 processors are using the same PCI ids for the network
> device but has altered register definitions for determining the
> window settings for the indirect PCS access.Add support to check
> for this hardware and if found use the new register values

How they are differentiated, subdevice ids?
If so should we add subdevice fields check into DPDK?

> 
> Signed-off-by: Selwin Sebastian <selwin.sebastian@amd.com>
> ---
>  drivers/net/axgbe/axgbe_common.h |  2 ++
>  drivers/net/axgbe/axgbe_ethdev.c | 18 +++++++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/axgbe/axgbe_common.h b/drivers/net/axgbe/axgbe_common.h
> index 34f60f156..4a3fbac16 100644
> --- a/drivers/net/axgbe/axgbe_common.h
> +++ b/drivers/net/axgbe/axgbe_common.h
> @@ -841,6 +841,8 @@
>  #define PCS_V1_WINDOW_SELECT		0x03fc
>  #define PCS_V2_WINDOW_DEF		0x9060
>  #define PCS_V2_WINDOW_SELECT		0x9064
> +#define PCS_V2_RV_WINDOW_DEF		0x1060
> +#define PCS_V2_RV_WINDOW_SELECT		0x1064
>  
>  /* PCS register entry bit positions and sizes */
>  #define PCS_V2_WINDOW_DEF_OFFSET_INDEX	6
> diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
> index d1f160e79..25e182b8d 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -31,6 +31,7 @@ static int  axgbe_dev_info_get(struct rte_eth_dev *dev,
>  #define AMD_PCI_VENDOR_ID       0x1022
>  #define AMD_PCI_AXGBE_DEVICE_V2A 0x1458
>  #define AMD_PCI_AXGBE_DEVICE_V2B 0x1459
> +extern struct rte_pci_bus rte_pci_bus;

Not sure about accessing the bus device list from a PMD...

>  
>  int axgbe_logtype_init;
>  int axgbe_logtype_driver;
> @@ -585,6 +586,7 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>  	struct rte_pci_device *pci_dev;
>  	uint32_t reg, mac_lo, mac_hi;
>  	int ret;
> +	struct rte_pci_device *pdev;
>  
>  	eth_dev->dev_ops = &axgbe_eth_dev_ops;
>  	eth_dev->rx_pkt_burst = &axgbe_recv_pkts;
> @@ -605,6 +607,17 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>  	pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
>  	pdata->pci_dev = pci_dev;
>  
> +	pdev = TAILQ_FIRST(&rte_pci_bus.device_list);

Can you please describe what this does? You are reading first pci device and do
you assume it is an axgbe device? And do you also assume there is single axgbe
device?

Why you are not simply using 'pci_dev' above?

> +
> +	if (pdev->id.vendor_id == AMD_PCI_VENDOR_ID &&
> +		pdev->id.device_id == 0x15d0) {

As far as I can see, '0x15d0' is not in the supported pci_id list, so why you
are checking it here? That devices shouldn't be probed at all ...

> +			pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF;
> +			pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT;
> +	} else {
> +		pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
> +		pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
> +	}
> +
>  	pdata->xgmac_regs =
>  		(void *)pci_dev->mem_resource[AXGBE_AXGMAC_BAR].addr;
>  	pdata->xprop_regs = (void *)((uint8_t *)pdata->xgmac_regs
> @@ -620,14 +633,13 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>  		pdata->vdata = &axgbe_v2b;
>  
>  	/* Configure the PCS indirect addressing support */
> -	reg = XPCS32_IOREAD(pdata, PCS_V2_WINDOW_DEF);
> +	reg = XPCS32_IOREAD(pdata, pdata->xpcs_window_def_reg);
>  	pdata->xpcs_window = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, OFFSET);
>  	pdata->xpcs_window <<= 6;
>  	pdata->xpcs_window_size = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, SIZE);
>  	pdata->xpcs_window_size = 1 << (pdata->xpcs_window_size + 7);
>  	pdata->xpcs_window_mask = pdata->xpcs_window_size - 1;
> -	pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
> -	pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
> +
>  	PMD_INIT_LOG(DEBUG,
>  		     "xpcs window :%x, size :%x, mask :%x ", pdata->xpcs_window,
>  		     pdata->xpcs_window_size, pdata->xpcs_window_mask);
>
Sebastian, Selwin Dec. 17, 2019, 6:44 a.m. UTC | #2 
[AMD Official Use Only - Internal Distribution Only]

Hi Ferruh,
	Current driver was developed for EPYC 3000 processors. New processors V1000/R1000 is also using the same PCI id for axgbe but register definitions for determining the window settings for indirect PCS access is changed. In order to identify processor, we are adding a quirk.
15d0 is the pci id for V1000/R1000/Raven root complex( https://pci-ids.ucw.cz/read/PC/1022 ). Hence read pci-id of root complex  to determine which processor and set the registers accordingly. 

Thanks and Regards
Selwin Sebastian
 

-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com> 
Sent: Wednesday, December 11, 2019 5:12 PM
To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: Add a HW quirk for register definitions

[CAUTION: External Email]

On 12/10/2019 3:29 PM, Selwin Sebastian wrote:
> V1000/R1000 processors are using the same PCI ids for the network 
> device but has altered register definitions for determining the window 
> settings for the indirect PCS access.Add support to check for this 
> hardware and if found use the new register values

How they are differentiated, subdevice ids?
If so should we add subdevice fields check into DPDK?

>
> Signed-off-by: Selwin Sebastian <selwin.sebastian@amd.com>
> ---
>  drivers/net/axgbe/axgbe_common.h |  2 ++  
> drivers/net/axgbe/axgbe_ethdev.c | 18 +++++++++++++++---
>  2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/axgbe/axgbe_common.h 
> b/drivers/net/axgbe/axgbe_common.h
> index 34f60f156..4a3fbac16 100644
> --- a/drivers/net/axgbe/axgbe_common.h
> +++ b/drivers/net/axgbe/axgbe_common.h
> @@ -841,6 +841,8 @@
>  #define PCS_V1_WINDOW_SELECT         0x03fc
>  #define PCS_V2_WINDOW_DEF            0x9060
>  #define PCS_V2_WINDOW_SELECT         0x9064
> +#define PCS_V2_RV_WINDOW_DEF         0x1060
> +#define PCS_V2_RV_WINDOW_SELECT              0x1064
>
>  /* PCS register entry bit positions and sizes */
>  #define PCS_V2_WINDOW_DEF_OFFSET_INDEX       6
> diff --git a/drivers/net/axgbe/axgbe_ethdev.c 
> b/drivers/net/axgbe/axgbe_ethdev.c
> index d1f160e79..25e182b8d 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -31,6 +31,7 @@ static int  axgbe_dev_info_get(struct rte_eth_dev *dev,
>  #define AMD_PCI_VENDOR_ID       0x1022
>  #define AMD_PCI_AXGBE_DEVICE_V2A 0x1458  #define 
> AMD_PCI_AXGBE_DEVICE_V2B 0x1459
> +extern struct rte_pci_bus rte_pci_bus;

Not sure about accessing the bus device list from a PMD...

>
>  int axgbe_logtype_init;
>  int axgbe_logtype_driver;
> @@ -585,6 +586,7 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>       struct rte_pci_device *pci_dev;
>       uint32_t reg, mac_lo, mac_hi;
>       int ret;
> +     struct rte_pci_device *pdev;
>
>       eth_dev->dev_ops = &axgbe_eth_dev_ops;
>       eth_dev->rx_pkt_burst = &axgbe_recv_pkts; @@ -605,6 +607,17 @@ 
> eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>       pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
>       pdata->pci_dev = pci_dev;
>
> +     pdev = TAILQ_FIRST(&rte_pci_bus.device_list);

Can you please describe what this does? You are reading first pci device and do you assume it is an axgbe device? And do you also assume there is single axgbe device?

Why you are not simply using 'pci_dev' above?

> +
> +     if (pdev->id.vendor_id == AMD_PCI_VENDOR_ID &&
> +             pdev->id.device_id == 0x15d0) {

As far as I can see, '0x15d0' is not in the supported pci_id list, so why you are checking it here? That devices shouldn't be probed at all ...

> +                     pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF;
> +                     pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT;
> +     } else {
> +             pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
> +             pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
> +     }
> +
>       pdata->xgmac_regs =
>               (void *)pci_dev->mem_resource[AXGBE_AXGMAC_BAR].addr;
>       pdata->xprop_regs = (void *)((uint8_t *)pdata->xgmac_regs @@ 
> -620,14 +633,13 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>               pdata->vdata = &axgbe_v2b;
>
>       /* Configure the PCS indirect addressing support */
> -     reg = XPCS32_IOREAD(pdata, PCS_V2_WINDOW_DEF);
> +     reg = XPCS32_IOREAD(pdata, pdata->xpcs_window_def_reg);
>       pdata->xpcs_window = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, OFFSET);
>       pdata->xpcs_window <<= 6;
>       pdata->xpcs_window_size = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, SIZE);
>       pdata->xpcs_window_size = 1 << (pdata->xpcs_window_size + 7);
>       pdata->xpcs_window_mask = pdata->xpcs_window_size - 1;
> -     pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
> -     pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
> +
>       PMD_INIT_LOG(DEBUG,
>                    "xpcs window :%x, size :%x, mask :%x ", pdata->xpcs_window,
>                    pdata->xpcs_window_size, pdata->xpcs_window_mask);
>
Ferruh Yigit Jan. 7, 2020, 1:57 p.m. UTC | #3 
On 12/17/2019 6:44 AM, Sebastian, Selwin wrote:
> [AMD Official Use Only - Internal Distribution Only]
> 
> Hi Ferruh,
> 	Current driver was developed for EPYC 3000 processors. New processors V1000/R1000 is also using the same PCI id for axgbe but register definitions for determining the window settings for indirect PCS access is changed. In order to identify processor, we are adding a quirk.
> 15d0 is the pci id for V1000/R1000/Raven root complex( https://pci-ids.ucw.cz/read/PC/1022 ). Hence read pci-id of root complex  to determine which processor and set the registers accordingly. 
> 

Got it, it is better to add a define for 0x15d0 with an explanation, and for the
root complex device use a more descriptive variable name that 'pdev'.

But still it is not really good idea to access the pci device list, isn't there
any other way to differentiate the devices, sub-device id etc? And how does
linux driver manages this?

And is it guaranteed that root complex device always will be the first device?


> Thanks and Regards
> Selwin Sebastian
>  
> 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com> 
> Sent: Wednesday, December 11, 2019 5:12 PM
> To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: Add a HW quirk for register definitions
> 
> [CAUTION: External Email]
> 
> On 12/10/2019 3:29 PM, Selwin Sebastian wrote:
>> V1000/R1000 processors are using the same PCI ids for the network 
>> device but has altered register definitions for determining the window 
>> settings for the indirect PCS access.Add support to check for this 
>> hardware and if found use the new register values
> 
> How they are differentiated, subdevice ids?
> If so should we add subdevice fields check into DPDK?
> 
>>
>> Signed-off-by: Selwin Sebastian <selwin.sebastian@amd.com>
>> ---
>>  drivers/net/axgbe/axgbe_common.h |  2 ++  
>> drivers/net/axgbe/axgbe_ethdev.c | 18 +++++++++++++++---
>>  2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/axgbe/axgbe_common.h 
>> b/drivers/net/axgbe/axgbe_common.h
>> index 34f60f156..4a3fbac16 100644
>> --- a/drivers/net/axgbe/axgbe_common.h
>> +++ b/drivers/net/axgbe/axgbe_common.h
>> @@ -841,6 +841,8 @@
>>  #define PCS_V1_WINDOW_SELECT         0x03fc
>>  #define PCS_V2_WINDOW_DEF            0x9060
>>  #define PCS_V2_WINDOW_SELECT         0x9064
>> +#define PCS_V2_RV_WINDOW_DEF         0x1060
>> +#define PCS_V2_RV_WINDOW_SELECT              0x1064
>>
>>  /* PCS register entry bit positions and sizes */
>>  #define PCS_V2_WINDOW_DEF_OFFSET_INDEX       6
>> diff --git a/drivers/net/axgbe/axgbe_ethdev.c 
>> b/drivers/net/axgbe/axgbe_ethdev.c
>> index d1f160e79..25e182b8d 100644
>> --- a/drivers/net/axgbe/axgbe_ethdev.c
>> +++ b/drivers/net/axgbe/axgbe_ethdev.c
>> @@ -31,6 +31,7 @@ static int  axgbe_dev_info_get(struct rte_eth_dev *dev,
>>  #define AMD_PCI_VENDOR_ID       0x1022
>>  #define AMD_PCI_AXGBE_DEVICE_V2A 0x1458  #define 
>> AMD_PCI_AXGBE_DEVICE_V2B 0x1459
>> +extern struct rte_pci_bus rte_pci_bus;
> 
> Not sure about accessing the bus device list from a PMD...
> 
>>
>>  int axgbe_logtype_init;
>>  int axgbe_logtype_driver;
>> @@ -585,6 +586,7 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>       struct rte_pci_device *pci_dev;
>>       uint32_t reg, mac_lo, mac_hi;
>>       int ret;
>> +     struct rte_pci_device *pdev;
>>
>>       eth_dev->dev_ops = &axgbe_eth_dev_ops;
>>       eth_dev->rx_pkt_burst = &axgbe_recv_pkts; @@ -605,6 +607,17 @@ 
>> eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>       pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
>>       pdata->pci_dev = pci_dev;
>>
>> +     pdev = TAILQ_FIRST(&rte_pci_bus.device_list);
> 
> Can you please describe what this does? You are reading first pci device and do you assume it is an axgbe device? And do you also assume there is single axgbe device?
> 
> Why you are not simply using 'pci_dev' above?
> 
>> +
>> +     if (pdev->id.vendor_id == AMD_PCI_VENDOR_ID &&
>> +             pdev->id.device_id == 0x15d0) {
> 
> As far as I can see, '0x15d0' is not in the supported pci_id list, so why you are checking it here? That devices shouldn't be probed at all ...
> 
>> +                     pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF;
>> +                     pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT;
>> +     } else {
>> +             pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
>> +             pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
>> +     }
>> +
>>       pdata->xgmac_regs =
>>               (void *)pci_dev->mem_resource[AXGBE_AXGMAC_BAR].addr;
>>       pdata->xprop_regs = (void *)((uint8_t *)pdata->xgmac_regs @@ 
>> -620,14 +633,13 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>               pdata->vdata = &axgbe_v2b;
>>
>>       /* Configure the PCS indirect addressing support */
>> -     reg = XPCS32_IOREAD(pdata, PCS_V2_WINDOW_DEF);
>> +     reg = XPCS32_IOREAD(pdata, pdata->xpcs_window_def_reg);
>>       pdata->xpcs_window = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, OFFSET);
>>       pdata->xpcs_window <<= 6;
>>       pdata->xpcs_window_size = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, SIZE);
>>       pdata->xpcs_window_size = 1 << (pdata->xpcs_window_size + 7);
>>       pdata->xpcs_window_mask = pdata->xpcs_window_size - 1;
>> -     pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
>> -     pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
>> +
>>       PMD_INIT_LOG(DEBUG,
>>                    "xpcs window :%x, size :%x, mask :%x ", pdata->xpcs_window,
>>                    pdata->xpcs_window_size, pdata->xpcs_window_mask);
>>
Sebastian, Selwin Jan. 9, 2020, 7:15 a.m. UTC | #4 
[AMD Official Use Only - Internal Distribution Only]

Hi Ferruh,
	I submitted v2 of the patch as per your guidelines. I checked sub-device ids and they are also the same. I am not aware of a better way to address this issue and even Linux driver is handling it using the same quirk. 
Yes,  root complex device will always be the first device.

Thanks and Regards
Selwin Sebastian
 
-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com> 
Sent: Tuesday, January 7, 2020 7:28 PM
To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: Add a HW quirk for register definitions

[CAUTION: External Email]

On 12/17/2019 6:44 AM, Sebastian, Selwin wrote:
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi Ferruh,
>       Current driver was developed for EPYC 3000 processors. New processors V1000/R1000 is also using the same PCI id for axgbe but register definitions for determining the window settings for indirect PCS access is changed. In order to identify processor, we are adding a quirk.
> 15d0 is the pci id for V1000/R1000/Raven root complex( https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpci-ids.ucw.cz%2Fread%2FPC%2F1022&amp;data=02%7C01%7CSelwin.Sebastian%40amd.com%7C2730af7407924ee8bea308d793798ebb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637140022603089567&amp;sdata=myGZCIkcjEOI31SLvkVvqdDww0AbpxZrsr47LpBiDHA%3D&amp;reserved=0 ). Hence read pci-id of root complex  to determine which processor and set the registers accordingly.
>

Got it, it is better to add a define for 0x15d0 with an explanation, and for the root complex device use a more descriptive variable name that 'pdev'.

But still it is not really good idea to access the pci device list, isn't there any other way to differentiate the devices, sub-device id etc? And how does linux driver manages this?

And is it guaranteed that root complex device always will be the first device?


> Thanks and Regards
> Selwin Sebastian
>
>
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, December 11, 2019 5:12 PM
> To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: Add a HW quirk for 
> register definitions
>
> [CAUTION: External Email]
>
> On 12/10/2019 3:29 PM, Selwin Sebastian wrote:
>> V1000/R1000 processors are using the same PCI ids for the network 
>> device but has altered register definitions for determining the 
>> window settings for the indirect PCS access.Add support to check for 
>> this hardware and if found use the new register values
>
> How they are differentiated, subdevice ids?
> If so should we add subdevice fields check into DPDK?
>
>>
>> Signed-off-by: Selwin Sebastian <selwin.sebastian@amd.com>
>> ---
>>  drivers/net/axgbe/axgbe_common.h |  2 ++ 
>> drivers/net/axgbe/axgbe_ethdev.c | 18 +++++++++++++++---
>>  2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/axgbe/axgbe_common.h
>> b/drivers/net/axgbe/axgbe_common.h
>> index 34f60f156..4a3fbac16 100644
>> --- a/drivers/net/axgbe/axgbe_common.h
>> +++ b/drivers/net/axgbe/axgbe_common.h
>> @@ -841,6 +841,8 @@
>>  #define PCS_V1_WINDOW_SELECT         0x03fc
>>  #define PCS_V2_WINDOW_DEF            0x9060
>>  #define PCS_V2_WINDOW_SELECT         0x9064
>> +#define PCS_V2_RV_WINDOW_DEF         0x1060
>> +#define PCS_V2_RV_WINDOW_SELECT              0x1064
>>
>>  /* PCS register entry bit positions and sizes */
>>  #define PCS_V2_WINDOW_DEF_OFFSET_INDEX       6
>> diff --git a/drivers/net/axgbe/axgbe_ethdev.c
>> b/drivers/net/axgbe/axgbe_ethdev.c
>> index d1f160e79..25e182b8d 100644
>> --- a/drivers/net/axgbe/axgbe_ethdev.c
>> +++ b/drivers/net/axgbe/axgbe_ethdev.c
>> @@ -31,6 +31,7 @@ static int  axgbe_dev_info_get(struct rte_eth_dev *dev,
>>  #define AMD_PCI_VENDOR_ID       0x1022
>>  #define AMD_PCI_AXGBE_DEVICE_V2A 0x1458  #define 
>> AMD_PCI_AXGBE_DEVICE_V2B 0x1459
>> +extern struct rte_pci_bus rte_pci_bus;
>
> Not sure about accessing the bus device list from a PMD...
>
>>
>>  int axgbe_logtype_init;
>>  int axgbe_logtype_driver;
>> @@ -585,6 +586,7 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>       struct rte_pci_device *pci_dev;
>>       uint32_t reg, mac_lo, mac_hi;
>>       int ret;
>> +     struct rte_pci_device *pdev;
>>
>>       eth_dev->dev_ops = &axgbe_eth_dev_ops;
>>       eth_dev->rx_pkt_burst = &axgbe_recv_pkts; @@ -605,6 +607,17 @@ 
>> eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>       pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
>>       pdata->pci_dev = pci_dev;
>>
>> +     pdev = TAILQ_FIRST(&rte_pci_bus.device_list);
>
> Can you please describe what this does? You are reading first pci device and do you assume it is an axgbe device? And do you also assume there is single axgbe device?
>
> Why you are not simply using 'pci_dev' above?
>
>> +
>> +     if (pdev->id.vendor_id == AMD_PCI_VENDOR_ID &&
>> +             pdev->id.device_id == 0x15d0) {
>
> As far as I can see, '0x15d0' is not in the supported pci_id list, so why you are checking it here? That devices shouldn't be probed at all ...
>
>> +                     pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF;
>> +                     pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT;
>> +     } else {
>> +             pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
>> +             pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
>> +     }
>> +
>>       pdata->xgmac_regs =
>>               (void *)pci_dev->mem_resource[AXGBE_AXGMAC_BAR].addr;
>>       pdata->xprop_regs = (void *)((uint8_t *)pdata->xgmac_regs @@
>> -620,14 +633,13 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>               pdata->vdata = &axgbe_v2b;
>>
>>       /* Configure the PCS indirect addressing support */
>> -     reg = XPCS32_IOREAD(pdata, PCS_V2_WINDOW_DEF);
>> +     reg = XPCS32_IOREAD(pdata, pdata->xpcs_window_def_reg);
>>       pdata->xpcs_window = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, OFFSET);
>>       pdata->xpcs_window <<= 6;
>>       pdata->xpcs_window_size = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, SIZE);
>>       pdata->xpcs_window_size = 1 << (pdata->xpcs_window_size + 7);
>>       pdata->xpcs_window_mask = pdata->xpcs_window_size - 1;
>> -     pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
>> -     pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
>> +
>>       PMD_INIT_LOG(DEBUG,
>>                    "xpcs window :%x, size :%x, mask :%x ", pdata->xpcs_window,
>>                    pdata->xpcs_window_size, pdata->xpcs_window_mask);
>>
Ferruh Yigit Jan. 9, 2020, 10:18 a.m. UTC | #5 
On 1/9/2020 7:15 AM, Sebastian, Selwin wrote:
> [AMD Official Use Only - Internal Distribution Only]
> 
> Hi Ferruh,
> 	I submitted v2 of the patch as per your guidelines. I checked sub-device ids and they are also the same. I am not aware of a better way to address this issue and even Linux driver is handling it using the same quirk. 

Unfortunately HW quirks are happens. As a last try, can there be any FW version,
PHY/MAC type, any specific register value to differentiate the device?
to prevent accessing the pci device list...

> Yes,  root complex device will always be the first device.
> 
> Thanks and Regards
> Selwin Sebastian
>  
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com> 
> Sent: Tuesday, January 7, 2020 7:28 PM
> To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: Add a HW quirk for register definitions
> 
> [CAUTION: External Email]
> 
> On 12/17/2019 6:44 AM, Sebastian, Selwin wrote:
>> [AMD Official Use Only - Internal Distribution Only]
>>
>> Hi Ferruh,
>>       Current driver was developed for EPYC 3000 processors. New processors V1000/R1000 is also using the same PCI id for axgbe but register definitions for determining the window settings for indirect PCS access is changed. In order to identify processor, we are adding a quirk.
>> 15d0 is the pci id for V1000/R1000/Raven root complex( https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpci-ids.ucw.cz%2Fread%2FPC%2F1022&amp;data=02%7C01%7CSelwin.Sebastian%40amd.com%7C2730af7407924ee8bea308d793798ebb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637140022603089567&amp;sdata=myGZCIkcjEOI31SLvkVvqdDww0AbpxZrsr47LpBiDHA%3D&amp;reserved=0 ). Hence read pci-id of root complex  to determine which processor and set the registers accordingly.
>>
> 
> Got it, it is better to add a define for 0x15d0 with an explanation, and for the root complex device use a more descriptive variable name that 'pdev'.
> 
> But still it is not really good idea to access the pci device list, isn't there any other way to differentiate the devices, sub-device id etc? And how does linux driver manages this?
> 
> And is it guaranteed that root complex device always will be the first device?
> 
> 
>> Thanks and Regards
>> Selwin Sebastian
>>
>>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, December 11, 2019 5:12 PM
>> To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: Add a HW quirk for 
>> register definitions
>>
>> [CAUTION: External Email]
>>
>> On 12/10/2019 3:29 PM, Selwin Sebastian wrote:
>>> V1000/R1000 processors are using the same PCI ids for the network 
>>> device but has altered register definitions for determining the 
>>> window settings for the indirect PCS access.Add support to check for 
>>> this hardware and if found use the new register values
>>
>> How they are differentiated, subdevice ids?
>> If so should we add subdevice fields check into DPDK?
>>
>>>
>>> Signed-off-by: Selwin Sebastian <selwin.sebastian@amd.com>
>>> ---
>>>  drivers/net/axgbe/axgbe_common.h |  2 ++ 
>>> drivers/net/axgbe/axgbe_ethdev.c | 18 +++++++++++++++---
>>>  2 files changed, 17 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/axgbe/axgbe_common.h
>>> b/drivers/net/axgbe/axgbe_common.h
>>> index 34f60f156..4a3fbac16 100644
>>> --- a/drivers/net/axgbe/axgbe_common.h
>>> +++ b/drivers/net/axgbe/axgbe_common.h
>>> @@ -841,6 +841,8 @@
>>>  #define PCS_V1_WINDOW_SELECT         0x03fc
>>>  #define PCS_V2_WINDOW_DEF            0x9060
>>>  #define PCS_V2_WINDOW_SELECT         0x9064
>>> +#define PCS_V2_RV_WINDOW_DEF         0x1060
>>> +#define PCS_V2_RV_WINDOW_SELECT              0x1064
>>>
>>>  /* PCS register entry bit positions and sizes */
>>>  #define PCS_V2_WINDOW_DEF_OFFSET_INDEX       6
>>> diff --git a/drivers/net/axgbe/axgbe_ethdev.c
>>> b/drivers/net/axgbe/axgbe_ethdev.c
>>> index d1f160e79..25e182b8d 100644
>>> --- a/drivers/net/axgbe/axgbe_ethdev.c
>>> +++ b/drivers/net/axgbe/axgbe_ethdev.c
>>> @@ -31,6 +31,7 @@ static int  axgbe_dev_info_get(struct rte_eth_dev *dev,
>>>  #define AMD_PCI_VENDOR_ID       0x1022
>>>  #define AMD_PCI_AXGBE_DEVICE_V2A 0x1458  #define 
>>> AMD_PCI_AXGBE_DEVICE_V2B 0x1459
>>> +extern struct rte_pci_bus rte_pci_bus;
>>
>> Not sure about accessing the bus device list from a PMD...
>>
>>>
>>>  int axgbe_logtype_init;
>>>  int axgbe_logtype_driver;
>>> @@ -585,6 +586,7 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>>       struct rte_pci_device *pci_dev;
>>>       uint32_t reg, mac_lo, mac_hi;
>>>       int ret;
>>> +     struct rte_pci_device *pdev;
>>>
>>>       eth_dev->dev_ops = &axgbe_eth_dev_ops;
>>>       eth_dev->rx_pkt_burst = &axgbe_recv_pkts; @@ -605,6 +607,17 @@ 
>>> eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>>       pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
>>>       pdata->pci_dev = pci_dev;
>>>
>>> +     pdev = TAILQ_FIRST(&rte_pci_bus.device_list);
>>
>> Can you please describe what this does? You are reading first pci device and do you assume it is an axgbe device? And do you also assume there is single axgbe device?
>>
>> Why you are not simply using 'pci_dev' above?
>>
>>> +
>>> +     if (pdev->id.vendor_id == AMD_PCI_VENDOR_ID &&
>>> +             pdev->id.device_id == 0x15d0) {
>>
>> As far as I can see, '0x15d0' is not in the supported pci_id list, so why you are checking it here? That devices shouldn't be probed at all ...
>>
>>> +                     pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF;
>>> +                     pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT;
>>> +     } else {
>>> +             pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
>>> +             pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
>>> +     }
>>> +
>>>       pdata->xgmac_regs =
>>>               (void *)pci_dev->mem_resource[AXGBE_AXGMAC_BAR].addr;
>>>       pdata->xprop_regs = (void *)((uint8_t *)pdata->xgmac_regs @@
>>> -620,14 +633,13 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>>               pdata->vdata = &axgbe_v2b;
>>>
>>>       /* Configure the PCS indirect addressing support */
>>> -     reg = XPCS32_IOREAD(pdata, PCS_V2_WINDOW_DEF);
>>> +     reg = XPCS32_IOREAD(pdata, pdata->xpcs_window_def_reg);
>>>       pdata->xpcs_window = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, OFFSET);
>>>       pdata->xpcs_window <<= 6;
>>>       pdata->xpcs_window_size = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, SIZE);
>>>       pdata->xpcs_window_size = 1 << (pdata->xpcs_window_size + 7);
>>>       pdata->xpcs_window_mask = pdata->xpcs_window_size - 1;
>>> -     pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
>>> -     pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
>>> +
>>>       PMD_INIT_LOG(DEBUG,
>>>                    "xpcs window :%x, size :%x, mask :%x ", pdata->xpcs_window,
>>>                    pdata->xpcs_window_size, pdata->xpcs_window_mask);
>>>
Sebastian, Selwin Jan. 10, 2020, 1:24 p.m. UTC | #6 
[AMD Official Use Only - Internal Distribution Only]

Hi Ferruh,
	I checked for FW version, PHY/MAC , registers for differentiating and it cannot be used. 

Thanks and Regards
Selwin Sebastian

-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com> 
Sent: Thursday, January 9, 2020 3:48 PM
To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: Add a HW quirk for register definitions

[CAUTION: External Email]

On 1/9/2020 7:15 AM, Sebastian, Selwin wrote:
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi Ferruh,
>       I submitted v2 of the patch as per your guidelines. I checked sub-device ids and they are also the same. I am not aware of a better way to address this issue and even Linux driver is handling it using the same quirk.

Unfortunately HW quirks are happens. As a last try, can there be any FW version, PHY/MAC type, any specific register value to differentiate the device?
to prevent accessing the pci device list...

> Yes,  root complex device will always be the first device.
>
> Thanks and Regards
> Selwin Sebastian
>
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, January 7, 2020 7:28 PM
> To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: Add a HW quirk for 
> register definitions
>
> [CAUTION: External Email]
>
> On 12/17/2019 6:44 AM, Sebastian, Selwin wrote:
>> [AMD Official Use Only - Internal Distribution Only]
>>
>> Hi Ferruh,
>>       Current driver was developed for EPYC 3000 processors. New processors V1000/R1000 is also using the same PCI id for axgbe but register definitions for determining the window settings for indirect PCS access is changed. In order to identify processor, we are adding a quirk.
>> 15d0 is the pci id for V1000/R1000/Raven root complex( https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpci-ids.ucw.cz%2Fread%2FPC%2F1022&amp;data=02%7C01%7CSelwin.Sebastian%40amd.com%7C4ca73d7c9e8b4271935c08d794ed3d57%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637141618970643135&amp;sdata=DMGjrIDvnKMjjdOp2E4fVDd8YU0RFZALqTHoMpazwc4%3D&amp;reserved=0 ). Hence read pci-id of root complex  to determine which processor and set the registers accordingly.
>>
>
> Got it, it is better to add a define for 0x15d0 with an explanation, and for the root complex device use a more descriptive variable name that 'pdev'.
>
> But still it is not really good idea to access the pci device list, isn't there any other way to differentiate the devices, sub-device id etc? And how does linux driver manages this?
>
> And is it guaranteed that root complex device always will be the first device?
>
>
>> Thanks and Regards
>> Selwin Sebastian
>>
>>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, December 11, 2019 5:12 PM
>> To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: Add a HW quirk for 
>> register definitions
>>
>> [CAUTION: External Email]
>>
>> On 12/10/2019 3:29 PM, Selwin Sebastian wrote:
>>> V1000/R1000 processors are using the same PCI ids for the network 
>>> device but has altered register definitions for determining the 
>>> window settings for the indirect PCS access.Add support to check for 
>>> this hardware and if found use the new register values
>>
>> How they are differentiated, subdevice ids?
>> If so should we add subdevice fields check into DPDK?
>>
>>>
>>> Signed-off-by: Selwin Sebastian <selwin.sebastian@amd.com>
>>> ---
>>>  drivers/net/axgbe/axgbe_common.h |  2 ++ 
>>> drivers/net/axgbe/axgbe_ethdev.c | 18 +++++++++++++++---
>>>  2 files changed, 17 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/axgbe/axgbe_common.h
>>> b/drivers/net/axgbe/axgbe_common.h
>>> index 34f60f156..4a3fbac16 100644
>>> --- a/drivers/net/axgbe/axgbe_common.h
>>> +++ b/drivers/net/axgbe/axgbe_common.h
>>> @@ -841,6 +841,8 @@
>>>  #define PCS_V1_WINDOW_SELECT         0x03fc
>>>  #define PCS_V2_WINDOW_DEF            0x9060
>>>  #define PCS_V2_WINDOW_SELECT         0x9064
>>> +#define PCS_V2_RV_WINDOW_DEF         0x1060
>>> +#define PCS_V2_RV_WINDOW_SELECT              0x1064
>>>
>>>  /* PCS register entry bit positions and sizes */
>>>  #define PCS_V2_WINDOW_DEF_OFFSET_INDEX       6
>>> diff --git a/drivers/net/axgbe/axgbe_ethdev.c
>>> b/drivers/net/axgbe/axgbe_ethdev.c
>>> index d1f160e79..25e182b8d 100644
>>> --- a/drivers/net/axgbe/axgbe_ethdev.c
>>> +++ b/drivers/net/axgbe/axgbe_ethdev.c
>>> @@ -31,6 +31,7 @@ static int  axgbe_dev_info_get(struct rte_eth_dev *dev,
>>>  #define AMD_PCI_VENDOR_ID       0x1022
>>>  #define AMD_PCI_AXGBE_DEVICE_V2A 0x1458  #define 
>>> AMD_PCI_AXGBE_DEVICE_V2B 0x1459
>>> +extern struct rte_pci_bus rte_pci_bus;
>>
>> Not sure about accessing the bus device list from a PMD...
>>
>>>
>>>  int axgbe_logtype_init;
>>>  int axgbe_logtype_driver;
>>> @@ -585,6 +586,7 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>>       struct rte_pci_device *pci_dev;
>>>       uint32_t reg, mac_lo, mac_hi;
>>>       int ret;
>>> +     struct rte_pci_device *pdev;
>>>
>>>       eth_dev->dev_ops = &axgbe_eth_dev_ops;
>>>       eth_dev->rx_pkt_burst = &axgbe_recv_pkts; @@ -605,6 +607,17 @@ 
>>> eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>>       pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
>>>       pdata->pci_dev = pci_dev;
>>>
>>> +     pdev = TAILQ_FIRST(&rte_pci_bus.device_list);
>>
>> Can you please describe what this does? You are reading first pci device and do you assume it is an axgbe device? And do you also assume there is single axgbe device?
>>
>> Why you are not simply using 'pci_dev' above?
>>
>>> +
>>> +     if (pdev->id.vendor_id == AMD_PCI_VENDOR_ID &&
>>> +             pdev->id.device_id == 0x15d0) {
>>
>> As far as I can see, '0x15d0' is not in the supported pci_id list, so why you are checking it here? That devices shouldn't be probed at all ...
>>
>>> +                     pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF;
>>> +                     pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT;
>>> +     } else {
>>> +             pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
>>> +             pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
>>> +     }
>>> +
>>>       pdata->xgmac_regs =
>>>               (void *)pci_dev->mem_resource[AXGBE_AXGMAC_BAR].addr;
>>>       pdata->xprop_regs = (void *)((uint8_t *)pdata->xgmac_regs @@
>>> -620,14 +633,13 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>>               pdata->vdata = &axgbe_v2b;
>>>
>>>       /* Configure the PCS indirect addressing support */
>>> -     reg = XPCS32_IOREAD(pdata, PCS_V2_WINDOW_DEF);
>>> +     reg = XPCS32_IOREAD(pdata, pdata->xpcs_window_def_reg);
>>>       pdata->xpcs_window = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, OFFSET);
>>>       pdata->xpcs_window <<= 6;
>>>       pdata->xpcs_window_size = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, SIZE);
>>>       pdata->xpcs_window_size = 1 << (pdata->xpcs_window_size + 7);
>>>       pdata->xpcs_window_mask = pdata->xpcs_window_size - 1;
>>> -     pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
>>> -     pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
>>> +
>>>       PMD_INIT_LOG(DEBUG,
>>>                    "xpcs window :%x, size :%x, mask :%x ", pdata->xpcs_window,
>>>                    pdata->xpcs_window_size, 
>>> pdata->xpcs_window_mask);
>>>
Ferruh Yigit Jan. 10, 2020, 1:26 p.m. UTC | #7 
On 1/10/2020 1:24 PM, Sebastian, Selwin wrote:
> [AMD Official Use Only - Internal Distribution Only]
> 
> Hi Ferruh,
> 	I checked for FW version, PHY/MAC , registers for differentiating and it cannot be used. 

Thanks for double checking, I will proceed with your v2.

> 
> Thanks and Regards
> Selwin Sebastian
> 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com> 
> Sent: Thursday, January 9, 2020 3:48 PM
> To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: Add a HW quirk for register definitions
> 
> [CAUTION: External Email]
> 
> On 1/9/2020 7:15 AM, Sebastian, Selwin wrote:
>> [AMD Official Use Only - Internal Distribution Only]
>>
>> Hi Ferruh,
>>       I submitted v2 of the patch as per your guidelines. I checked sub-device ids and they are also the same. I am not aware of a better way to address this issue and even Linux driver is handling it using the same quirk.
> 
> Unfortunately HW quirks are happens. As a last try, can there be any FW version, PHY/MAC type, any specific register value to differentiate the device?
> to prevent accessing the pci device list...
> 
>> Yes,  root complex device will always be the first device.
>>
>> Thanks and Regards
>> Selwin Sebastian
>>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Tuesday, January 7, 2020 7:28 PM
>> To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: Add a HW quirk for 
>> register definitions
>>
>> [CAUTION: External Email]
>>
>> On 12/17/2019 6:44 AM, Sebastian, Selwin wrote:
>>> [AMD Official Use Only - Internal Distribution Only]
>>>
>>> Hi Ferruh,
>>>       Current driver was developed for EPYC 3000 processors. New processors V1000/R1000 is also using the same PCI id for axgbe but register definitions for determining the window settings for indirect PCS access is changed. In order to identify processor, we are adding a quirk.
>>> 15d0 is the pci id for V1000/R1000/Raven root complex( https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpci-ids.ucw.cz%2Fread%2FPC%2F1022&amp;data=02%7C01%7CSelwin.Sebastian%40amd.com%7C4ca73d7c9e8b4271935c08d794ed3d57%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637141618970643135&amp;sdata=DMGjrIDvnKMjjdOp2E4fVDd8YU0RFZALqTHoMpazwc4%3D&amp;reserved=0 ). Hence read pci-id of root complex  to determine which processor and set the registers accordingly.
>>>
>>
>> Got it, it is better to add a define for 0x15d0 with an explanation, and for the root complex device use a more descriptive variable name that 'pdev'.
>>
>> But still it is not really good idea to access the pci device list, isn't there any other way to differentiate the devices, sub-device id etc? And how does linux driver manages this?
>>
>> And is it guaranteed that root complex device always will be the first device?
>>
>>
>>> Thanks and Regards
>>> Selwin Sebastian
>>>
>>>
>>> -----Original Message-----
>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>> Sent: Wednesday, December 11, 2019 5:12 PM
>>> To: Sebastian, Selwin <Selwin.Sebastian@amd.com>; dev@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH v1] net/axgbe: Add a HW quirk for 
>>> register definitions
>>>
>>> [CAUTION: External Email]
>>>
>>> On 12/10/2019 3:29 PM, Selwin Sebastian wrote:
>>>> V1000/R1000 processors are using the same PCI ids for the network 
>>>> device but has altered register definitions for determining the 
>>>> window settings for the indirect PCS access.Add support to check for 
>>>> this hardware and if found use the new register values
>>>
>>> How they are differentiated, subdevice ids?
>>> If so should we add subdevice fields check into DPDK?
>>>
>>>>
>>>> Signed-off-by: Selwin Sebastian <selwin.sebastian@amd.com>
>>>> ---
>>>>  drivers/net/axgbe/axgbe_common.h |  2 ++ 
>>>> drivers/net/axgbe/axgbe_ethdev.c | 18 +++++++++++++++---
>>>>  2 files changed, 17 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/axgbe/axgbe_common.h
>>>> b/drivers/net/axgbe/axgbe_common.h
>>>> index 34f60f156..4a3fbac16 100644
>>>> --- a/drivers/net/axgbe/axgbe_common.h
>>>> +++ b/drivers/net/axgbe/axgbe_common.h
>>>> @@ -841,6 +841,8 @@
>>>>  #define PCS_V1_WINDOW_SELECT         0x03fc
>>>>  #define PCS_V2_WINDOW_DEF            0x9060
>>>>  #define PCS_V2_WINDOW_SELECT         0x9064
>>>> +#define PCS_V2_RV_WINDOW_DEF         0x1060
>>>> +#define PCS_V2_RV_WINDOW_SELECT              0x1064
>>>>
>>>>  /* PCS register entry bit positions and sizes */
>>>>  #define PCS_V2_WINDOW_DEF_OFFSET_INDEX       6
>>>> diff --git a/drivers/net/axgbe/axgbe_ethdev.c
>>>> b/drivers/net/axgbe/axgbe_ethdev.c
>>>> index d1f160e79..25e182b8d 100644
>>>> --- a/drivers/net/axgbe/axgbe_ethdev.c
>>>> +++ b/drivers/net/axgbe/axgbe_ethdev.c
>>>> @@ -31,6 +31,7 @@ static int  axgbe_dev_info_get(struct rte_eth_dev *dev,
>>>>  #define AMD_PCI_VENDOR_ID       0x1022
>>>>  #define AMD_PCI_AXGBE_DEVICE_V2A 0x1458  #define 
>>>> AMD_PCI_AXGBE_DEVICE_V2B 0x1459
>>>> +extern struct rte_pci_bus rte_pci_bus;
>>>
>>> Not sure about accessing the bus device list from a PMD...
>>>
>>>>
>>>>  int axgbe_logtype_init;
>>>>  int axgbe_logtype_driver;
>>>> @@ -585,6 +586,7 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>>>       struct rte_pci_device *pci_dev;
>>>>       uint32_t reg, mac_lo, mac_hi;
>>>>       int ret;
>>>> +     struct rte_pci_device *pdev;
>>>>
>>>>       eth_dev->dev_ops = &axgbe_eth_dev_ops;
>>>>       eth_dev->rx_pkt_burst = &axgbe_recv_pkts; @@ -605,6 +607,17 @@ 
>>>> eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>>>       pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
>>>>       pdata->pci_dev = pci_dev;
>>>>
>>>> +     pdev = TAILQ_FIRST(&rte_pci_bus.device_list);
>>>
>>> Can you please describe what this does? You are reading first pci device and do you assume it is an axgbe device? And do you also assume there is single axgbe device?
>>>
>>> Why you are not simply using 'pci_dev' above?
>>>
>>>> +
>>>> +     if (pdev->id.vendor_id == AMD_PCI_VENDOR_ID &&
>>>> +             pdev->id.device_id == 0x15d0) {
>>>
>>> As far as I can see, '0x15d0' is not in the supported pci_id list, so why you are checking it here? That devices shouldn't be probed at all ...
>>>
>>>> +                     pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF;
>>>> +                     pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT;
>>>> +     } else {
>>>> +             pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
>>>> +             pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
>>>> +     }
>>>> +
>>>>       pdata->xgmac_regs =
>>>>               (void *)pci_dev->mem_resource[AXGBE_AXGMAC_BAR].addr;
>>>>       pdata->xprop_regs = (void *)((uint8_t *)pdata->xgmac_regs @@
>>>> -620,14 +633,13 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>>>>               pdata->vdata = &axgbe_v2b;
>>>>
>>>>       /* Configure the PCS indirect addressing support */
>>>> -     reg = XPCS32_IOREAD(pdata, PCS_V2_WINDOW_DEF);
>>>> +     reg = XPCS32_IOREAD(pdata, pdata->xpcs_window_def_reg);
>>>>       pdata->xpcs_window = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, OFFSET);
>>>>       pdata->xpcs_window <<= 6;
>>>>       pdata->xpcs_window_size = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, SIZE);
>>>>       pdata->xpcs_window_size = 1 << (pdata->xpcs_window_size + 7);
>>>>       pdata->xpcs_window_mask = pdata->xpcs_window_size - 1;
>>>> -     pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
>>>> -     pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
>>>> +
>>>>       PMD_INIT_LOG(DEBUG,
>>>>                    "xpcs window :%x, size :%x, mask :%x ", pdata->xpcs_window,
>>>>                    pdata->xpcs_window_size, 
>>>> pdata->xpcs_window_mask);
>>>>
diff mbox series

Patch

diff --git a/drivers/net/axgbe/axgbe_common.h b/drivers/net/axgbe/axgbe_common.h
index 34f60f156..4a3fbac16 100644
--- a/drivers/net/axgbe/axgbe_common.h
+++ b/drivers/net/axgbe/axgbe_common.h
@@ -841,6 +841,8 @@ 
 #define PCS_V1_WINDOW_SELECT		0x03fc
 #define PCS_V2_WINDOW_DEF		0x9060
 #define PCS_V2_WINDOW_SELECT		0x9064
+#define PCS_V2_RV_WINDOW_DEF		0x1060
+#define PCS_V2_RV_WINDOW_SELECT		0x1064
 
 /* PCS register entry bit positions and sizes */
 #define PCS_V2_WINDOW_DEF_OFFSET_INDEX	6
diff --git a/drivers/net/axgbe/axgbe_ethdev.c b/drivers/net/axgbe/axgbe_ethdev.c
index d1f160e79..25e182b8d 100644
--- a/drivers/net/axgbe/axgbe_ethdev.c
+++ b/drivers/net/axgbe/axgbe_ethdev.c
@@ -31,6 +31,7 @@  static int  axgbe_dev_info_get(struct rte_eth_dev *dev,
 #define AMD_PCI_VENDOR_ID       0x1022
 #define AMD_PCI_AXGBE_DEVICE_V2A 0x1458
 #define AMD_PCI_AXGBE_DEVICE_V2B 0x1459
+extern struct rte_pci_bus rte_pci_bus;
 
 int axgbe_logtype_init;
 int axgbe_logtype_driver;
@@ -585,6 +586,7 @@  eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
 	struct rte_pci_device *pci_dev;
 	uint32_t reg, mac_lo, mac_hi;
 	int ret;
+	struct rte_pci_device *pdev;
 
 	eth_dev->dev_ops = &axgbe_eth_dev_ops;
 	eth_dev->rx_pkt_burst = &axgbe_recv_pkts;
@@ -605,6 +607,17 @@  eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
 	pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
 	pdata->pci_dev = pci_dev;
 
+	pdev = TAILQ_FIRST(&rte_pci_bus.device_list);
+
+	if (pdev->id.vendor_id == AMD_PCI_VENDOR_ID &&
+		pdev->id.device_id == 0x15d0) {
+			pdata->xpcs_window_def_reg = PCS_V2_RV_WINDOW_DEF;
+			pdata->xpcs_window_sel_reg = PCS_V2_RV_WINDOW_SELECT;
+	} else {
+		pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
+		pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
+	}
+
 	pdata->xgmac_regs =
 		(void *)pci_dev->mem_resource[AXGBE_AXGMAC_BAR].addr;
 	pdata->xprop_regs = (void *)((uint8_t *)pdata->xgmac_regs
@@ -620,14 +633,13 @@  eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
 		pdata->vdata = &axgbe_v2b;
 
 	/* Configure the PCS indirect addressing support */
-	reg = XPCS32_IOREAD(pdata, PCS_V2_WINDOW_DEF);
+	reg = XPCS32_IOREAD(pdata, pdata->xpcs_window_def_reg);
 	pdata->xpcs_window = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, OFFSET);
 	pdata->xpcs_window <<= 6;
 	pdata->xpcs_window_size = XPCS_GET_BITS(reg, PCS_V2_WINDOW_DEF, SIZE);
 	pdata->xpcs_window_size = 1 << (pdata->xpcs_window_size + 7);
 	pdata->xpcs_window_mask = pdata->xpcs_window_size - 1;
-	pdata->xpcs_window_def_reg = PCS_V2_WINDOW_DEF;
-	pdata->xpcs_window_sel_reg = PCS_V2_WINDOW_SELECT;
+
 	PMD_INIT_LOG(DEBUG,
 		     "xpcs window :%x, size :%x, mask :%x ", pdata->xpcs_window,
 		     pdata->xpcs_window_size, pdata->xpcs_window_mask);