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%7C2730af7407924ee8bea308d793798ebb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637140022603089567&sdata=myGZCIkcjEOI31SLvkVvqdDww0AbpxZrsr47LpBiDHA%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); >>>
