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://pci-ids.ucw.cz/read/PC/1022 ). 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);
>>
