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.yi...@intel.com> > Sent: Thursday, January 9, 2020 3:48 PM > To: Sebastian, Selwin <selwin.sebast...@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.yi...@intel.com> >> Sent: Tuesday, January 7, 2020 7:28 PM >> To: Sebastian, Selwin <selwin.sebast...@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&data=02%7C01%7CSelwin.Sebastian%40amd.com%7C4ca73d7c9e8b4271935c08d794ed3d57%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637141618970643135&sdata=DMGjrIDvnKMjjdOp2E4fVDd8YU0RFZALqTHoMpazwc4%3D&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.yi...@intel.com> >>> Sent: Wednesday, December 11, 2019 5:12 PM >>> To: Sebastian, Selwin <selwin.sebast...@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.sebast...@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); >>>>
