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

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

On 1/20/2020 12:41 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.
> 
> Signed-off-by: Selwin Sebastian <Selwin.Sebastian@amd.com>
> ---
>  drivers/net/axgbe/axgbe_common.h |  2 ++
>  drivers/net/axgbe/axgbe_ethdev.c | 40 +++++++++++++++++++++++++++++---
>  2 files changed, 39 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..f5dbba292 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
>  
> @@ -574,6 +575,29 @@ static void axgbe_default_config(struct axgbe_port *pdata)
>  	pdata->power_down = 0;
>  }
>  
> +static inline int
> +pci_device_cmp(const struct rte_device *dev, const void *_pci_id)
> +{
> +	const struct rte_pci_device *pdev = RTE_DEV_TO_PCI_CONST(dev);
> +	const struct rte_pci_id *pcid = _pci_id;
> +
> +	if (pdev->id.vendor_id == AMD_PCI_VENDOR_ID &&
> +			pdev->id.device_id == pcid->device_id)
> +		return 0;
> +	return 1;
> +}

What is the point of 'inline' keyword, since the function will be used with its
pointer, can we drop it?

> +
> +static bool pci_search_device(int device_id)
> +{

Better to follow coding convention as done in above function.

> +	struct rte_bus *pci_bus;
> +	struct rte_pci_id dev_id;
> +
> +	dev_id.device_id = device_id;
> +	pci_bus = rte_bus_find_by_name("pci");
> +	return (pci_bus != NULL) &&
> +		(pci_bus->find_device(NULL, pci_device_cmp, &dev_id) != NULL);
> +}
> +
>  /*
>   * It returns 0 on success.
>   */
> @@ -605,6 +629,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;
>  
> +	/*
> +	 * Use root complex device ID to differentiate RV AXGBE vs SNOWY AXGBE
> +	 */
> +	if (pci_search_device(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 +655,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);
>