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

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

[Selwin Sebastian]

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;
+}
+
 /* 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);
+
 #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);
-- 
2.17.1

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

[Ferruh Yigit]

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

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

[David Marchand]

On Tue, Jan 14, 2020 at 12:58 PM Selwin Sebastian
<Selwin.Sebastian@amd.com> 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.

You can already iterate on a bus devices.

struct rte_bus *pci_bus = rte_bus_find_by_name("pci");
return pci_bus != NULL && pci_bus->find_device(NULL, callback, args) != NULL;

See: https://git.dpdk.org/dpdk/tree/drivers/bus/pci/pci_params.c#n38


-- 
David Marchand

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

[Ferruh Yigit]

On 1/14/2020 12:35 PM, David Marchand wrote:
> On Tue, Jan 14, 2020 at 12:58 PM Selwin Sebastian
> <Selwin.Sebastian@amd.com> 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.
> 
> You can already iterate on a bus devices.
> 
> struct rte_bus *pci_bus = rte_bus_find_by_name("pci");
> return pci_bus != NULL && pci_bus->find_device(NULL, callback, args) != NULL;
> 
> See: https://git.dpdk.org/dpdk/tree/drivers/bus/pci/pci_params.c#n38
> 

Nice, this looks better approach.

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

[Sebastian, Selwin]

[AMD Official Use Only - Internal Distribution Only]

Thank you David, Ferruh,
I pushed v4 of the patch with changes. 

Thanks and Regards
Selwin Sebastian

-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com> 
Sent: Tuesday, January 14, 2020 6:48 PM
To: David Marchand <david.marchand@redhat.com>; Sebastian, Selwin <Selwin.Sebastian@amd.com>
Cc: dev <dev@dpdk.org>; Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [PATCH v3] drivers: add a HW quirk for register definitions

[CAUTION: External Email]

On 1/14/2020 12:35 PM, David Marchand wrote:
> On Tue, Jan 14, 2020 at 12:58 PM Selwin Sebastian 
> <Selwin.Sebastian@amd.com> 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.
>
> You can already iterate on a bus devices.
>
> struct rte_bus *pci_bus = rte_bus_find_by_name("pci"); return pci_bus 
> != NULL && pci_bus->find_device(NULL, callback, args) != NULL;
>
> See: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.
> dpdk.org%2Fdpdk%2Ftree%2Fdrivers%2Fbus%2Fpci%2Fpci_params.c%23n38&amp;
> data=02%7C01%7CSelwin.Sebastian%40amd.com%7C35db24f051c546e2d13b08d798
> f438b5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637146047002796925
> &amp;sdata=6wBRIab3588iwCsgBV7JJ%2FjcXU5no1mKQtHxbFxJJlc%3D&amp;reserv
> ed=0
>

Nice, this looks better approach.