Re: [dpdk-dev] [PATCH v1] net/axgbe: Add a HW quirk for register definitions

[Ferruh Yigit <ferruh.yigit@intel.com>]

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