Re: [dpdk-dev] [PATCH v3] drivers: add a HW quirk for register definitions

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

On 1/14/2020 5:30 PM, Selwin Sebastian wrote:
> V1000/R1000 processors are using the same PCI ids for the network
> device as SNOWYOWL processor 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.
> 
> Added a new routine rte_pci_search_device to pci driver to search
> for a device.
> 
> Signed-off-by: Selwin Sebastian <Selwin.Sebastian@amd.com>
> ---
>  drivers/bus/pci/pci_common.c     | 17 +++++++++++++++++
>  drivers/bus/pci/rte_bus_pci.h    | 12 ++++++++++++
>  drivers/net/axgbe/axgbe_common.h |  2 ++
>  drivers/net/axgbe/axgbe_ethdev.c | 18 +++++++++++++++---
>  4 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> index 3f5542076..e991a2580 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -393,6 +393,23 @@ rte_pci_add_device(struct rte_pci_device *pci_dev)
>  	TAILQ_INSERT_TAIL(&rte_pci_bus.device_list, pci_dev, next);
>  }
>  
> +/* Search for a specific device in PCI bus */
> +
> +int
> +rte_pci_search_device(int vendor_id, int device_id)
> +{
> +	struct rte_pci_device *pdev;
> +	pdev = TAILQ_FIRST(&rte_pci_bus.device_list);
> +
> +	while (pdev != NULL) {
> +		if (pdev->id.vendor_id == vendor_id &&
> +			pdev->id.device_id == device_id)
> +			return true;
> +		pdev = TAILQ_NEXT(pdev, next);
> +	}
> +	return false;
> +}

Overall this is what I was asking, thanks.
A few details on the implementation,

What do you think returning the "struct rte_pci_device *", having it NULL
meaning device not found?

Also you can provide the "struct rte_pci_id" as input, instead of the just
vendor and device id?

Can be multiple ways to search the device in the bus, what about changing the
API name to 'rte_pci_search_device_by_id()' to clarify this search is by id?

What about having the API to use the existing 'pci_find_device' function, 'cmp'
function becomes simple and I think it makes the logic simpler but that is open
to discussion I guess.

> +
>  /* Insert a device into a predefined position in PCI bus */
>  void
>  rte_pci_insert_device(struct rte_pci_device *exist_pci_dev,
> diff --git a/drivers/bus/pci/rte_bus_pci.h b/drivers/bus/pci/rte_bus_pci.h
> index 29bea6d70..42b6c7e03 100644
> --- a/drivers/bus/pci/rte_bus_pci.h
> +++ b/drivers/bus/pci/rte_bus_pci.h
> @@ -354,6 +354,18 @@ void rte_pci_ioport_read(struct rte_pci_ioport *p,
>  void rte_pci_ioport_write(struct rte_pci_ioport *p,
>  		const void *data, size_t len, off_t offset);
>  
> +/*
> + * Search for a specific device in pci bus
> + *
> + * @param vendor_id
> + *   vendor_id of the device to search
> + * @param device_id
> + *   device_id of the device to search
> + * @return
> + *   1 on success, 0 on failure
> + */
> +int rte_pci_search_device(int vendor_id, int device_id);

There are a few process details to follow,
- Need to put the API to .map file, so it can be called by other libraries or
application.
- New APIs need to be experimental at least one release, this is done by:
  -- put '__rte_experimental' to the API decleration
  -- put API in .map file into the EXPERIMENTAL section

> +
>  #ifdef __cplusplus
>  }
>  #endif
> 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..01975236b 100644
> --- a/drivers/net/axgbe/axgbe_ethdev.c
> +++ b/drivers/net/axgbe/axgbe_ethdev.c
> @@ -29,6 +29,7 @@ static int  axgbe_dev_info_get(struct rte_eth_dev *dev,
>  
>  /* The set of PCI devices this driver supports */
>  #define AMD_PCI_VENDOR_ID       0x1022
> +#define AMD_PCI_RV_ROOT_COMPLEX_ID	0x15d0
>  #define AMD_PCI_AXGBE_DEVICE_V2A 0x1458
>  #define AMD_PCI_AXGBE_DEVICE_V2B 0x1459
>  
> @@ -605,6 +606,18 @@ eth_axgbe_dev_init(struct rte_eth_dev *eth_dev)
>  	pci_dev = RTE_DEV_TO_PCI(eth_dev->device);
>  	pdata->pci_dev = pci_dev;
>  
> +	/*
> +	 * Use root complex device ID to differentiate RV AXGBE vs SNOWY AXGBE
> +	 */
> +	if (rte_pci_search_device(AMD_PCI_VENDOR_ID,
> +				AMD_PCI_RV_ROOT_COMPLEX_ID)) {
> +			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);
>