Re: [dpdk-dev] [PATCH v5 02/24] net/ngbe: add device IDs

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Tuesday, June 15, 2021 1:09 AM, Andrew Rybchenko wrote:
> On 6/2/21 12:40 PM, Jiawen Wu wrote:
> > Add device IDs for Wangxun 1Gb NICs, and register rte_ngbe_pmd.
> >
> > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> > ---
> >   drivers/net/ngbe/base/meson.build   | 18 +++++++
> >   drivers/net/ngbe/base/ngbe_devids.h | 84 +++++++++++++++++++++++++++++
> >   drivers/net/ngbe/meson.build        |  6 +++
> >   drivers/net/ngbe/ngbe_ethdev.c      | 51 ++++++++++++++++++
> >   4 files changed, 159 insertions(+)
> >   create mode 100644 drivers/net/ngbe/base/meson.build
> >   create mode 100644 drivers/net/ngbe/base/ngbe_devids.h
> >
> > diff --git a/drivers/net/ngbe/base/meson.build
> > b/drivers/net/ngbe/base/meson.build
> > new file mode 100644
> > index 0000000000..c5f6467743
> > --- /dev/null
> > +++ b/drivers/net/ngbe/base/meson.build
> > @@ -0,0 +1,18 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2018-2020
> > +Beijing WangXun Technology Co., Ltd.
> > +
> > +sources = []
> > +
> > +error_cflags = []
> > +
> > +c_args = cflags
> > +foreach flag: error_cflags
> > +	if cc.has_argument(flag)
> > +		c_args += flag
> > +	endif
> > +endforeach
> > +
> > +base_lib = static_library('ngbe_base', sources,
> > +	dependencies: [static_rte_eal, static_rte_ethdev, static_rte_bus_pci],
> > +	c_args: c_args)
> > +base_objs = base_lib.extract_all_objects()
> > diff --git a/drivers/net/ngbe/base/ngbe_devids.h
> > b/drivers/net/ngbe/base/ngbe_devids.h
> > new file mode 100644
> > index 0000000000..81671f71da
> > --- /dev/null
> > +++ b/drivers/net/ngbe/base/ngbe_devids.h
> > @@ -0,0 +1,84 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2018-2020 Beijing WangXun Technology Co., Ltd.
> > + * Copyright(c) 2010-2017 Intel Corporation  */
> > +
> > +#ifndef _NGBE_DEVIDS_H_
> > +#define _NGBE_DEVIDS_H_
> > +
> > +/*
> > + * Vendor ID
> > + */
> > +#ifndef PCI_VENDOR_ID_WANGXUN
> > +#define PCI_VENDOR_ID_WANGXUN                   0x8088
> > +#endif
> > +
> > +/*
> > + * Device IDs
> > + */
> > +#define NGBE_DEV_ID_EM_VF			0x0110
> > +#define   NGBE_SUB_DEV_ID_EM_VF			0x0110
> > +#define NGBE_DEV_ID_EM				0x0100
> > +#define   NGBE_SUB_DEV_ID_EM_MVL_RGMII		0x0200
> > +#define   NGBE_SUB_DEV_ID_EM_MVL_SFP		0x0403
> > +#define   NGBE_SUB_DEV_ID_EM_RTL_SGMII		0x0410
> > +#define   NGBE_SUB_DEV_ID_EM_YT8521S_SFP	0x0460
> > +
> > +#define NGBE_DEV_ID_EM_WX1860AL_W		0x0100
> > +#define NGBE_DEV_ID_EM_WX1860AL_W_VF		0x0110
> > +#define NGBE_DEV_ID_EM_WX1860A2			0x0101
> > +#define NGBE_DEV_ID_EM_WX1860A2_VF		0x0111
> > +#define NGBE_DEV_ID_EM_WX1860A2S		0x0102
> > +#define NGBE_DEV_ID_EM_WX1860A2S_VF		0x0112
> > +#define NGBE_DEV_ID_EM_WX1860A4			0x0103
> > +#define NGBE_DEV_ID_EM_WX1860A4_VF		0x0113
> > +#define NGBE_DEV_ID_EM_WX1860A4S		0x0104
> > +#define NGBE_DEV_ID_EM_WX1860A4S_VF		0x0114
> > +#define NGBE_DEV_ID_EM_WX1860AL2		0x0105
> > +#define NGBE_DEV_ID_EM_WX1860AL2_VF		0x0115
> > +#define NGBE_DEV_ID_EM_WX1860AL2S		0x0106
> > +#define NGBE_DEV_ID_EM_WX1860AL2S_VF		0x0116
> > +#define NGBE_DEV_ID_EM_WX1860AL4		0x0107
> > +#define NGBE_DEV_ID_EM_WX1860AL4_VF		0x0117
> > +#define NGBE_DEV_ID_EM_WX1860AL4S		0x0108
> > +#define NGBE_DEV_ID_EM_WX1860AL4S_VF		0x0118
> > +#define NGBE_DEV_ID_EM_WX1860NCSI		0x0109
> > +#define NGBE_DEV_ID_EM_WX1860NCSI_VF		0x0119
> > +#define NGBE_DEV_ID_EM_WX1860A1			0x010A
> > +#define NGBE_DEV_ID_EM_WX1860A1_VF		0x011A
> > +#define NGBE_DEV_ID_EM_WX1860A1L		0x010B
> > +#define NGBE_DEV_ID_EM_WX1860A1L_VF		0x011B
> > +#define   NGBE_SUB_DEV_ID_EM_ZTE5201_RJ45	0x0100
> > +#define   NGBE_SUB_DEV_ID_EM_SF100F_LP		0x0103
> > +#define   NGBE_SUB_DEV_ID_EM_M88E1512_RJ45	0x0200
> > +#define   NGBE_SUB_DEV_ID_EM_SF100HT		0x0102
> > +#define   NGBE_SUB_DEV_ID_EM_SF200T		0x0201
> > +#define   NGBE_SUB_DEV_ID_EM_SF200HT		0x0202
> > +#define   NGBE_SUB_DEV_ID_EM_SF200T_S		0x0210
> > +#define   NGBE_SUB_DEV_ID_EM_SF200HT_S		0x0220
> > +#define   NGBE_SUB_DEV_ID_EM_SF200HXT		0x0230
> > +#define   NGBE_SUB_DEV_ID_EM_SF400T		0x0401
> > +#define   NGBE_SUB_DEV_ID_EM_SF400HT		0x0402
> > +#define   NGBE_SUB_DEV_ID_EM_M88E1512_SFP	0x0403
> > +#define   NGBE_SUB_DEV_ID_EM_SF400T_S		0x0410
> > +#define   NGBE_SUB_DEV_ID_EM_SF400HT_S		0x0420
> > +#define   NGBE_SUB_DEV_ID_EM_SF400HXT		0x0430
> > +#define   NGBE_SUB_DEV_ID_EM_SF400_OCP		0x0440
> > +#define   NGBE_SUB_DEV_ID_EM_SF400_LY		0x0450
> > +#define   NGBE_SUB_DEV_ID_EM_SF400_LY_YT	0x0470
> > +
> > +/* Assign excessive id with masks */
> > +#define NGBE_INTERNAL_MASK			0x000F
> > +#define NGBE_OEM_MASK				0x00F0
> > +#define NGBE_WOL_SUP_MASK			0x4000
> > +#define NGBE_NCSI_SUP_MASK			0x8000
> > +
> > +#define NGBE_INTERNAL_SFP			0x0003
> > +#define NGBE_OCP_CARD				0x0040
> > +#define NGBE_LY_M88E1512_SFP			0x0050
> > +#define NGBE_YT8521S_SFP			0x0060
> > +#define NGBE_LY_YT8521S_SFP			0x0070
> > +#define NGBE_WOL_SUP				0x4000
> > +#define NGBE_NCSI_SUP				0x8000
> > +
> > +#endif /* _NGBE_DEVIDS_H_ */
> > diff --git a/drivers/net/ngbe/meson.build
> > b/drivers/net/ngbe/meson.build index de2d7be716..81173fa7f0 100644
> > --- a/drivers/net/ngbe/meson.build
> > +++ b/drivers/net/ngbe/meson.build
> > @@ -7,6 +7,12 @@ if is_windows
> >   	subdir_done()
> >   endif
> >
> > +subdir('base')
> > +objs = [base_objs]
> > +
> >   sources = files(
> >   	'ngbe_ethdev.c',
> >   )
> > +
> > +includes += include_directories('base')
> > +
> > diff --git a/drivers/net/ngbe/ngbe_ethdev.c
> > b/drivers/net/ngbe/ngbe_ethdev.c index e424ff11a2..0f1fa86fe6 100644
> > --- a/drivers/net/ngbe/ngbe_ethdev.c
> > +++ b/drivers/net/ngbe/ngbe_ethdev.c
> > @@ -3,3 +3,54 @@
> >    * Copyright(c) 2010-2017 Intel Corporation
> >    */
> >
> > +#include <ethdev_pci.h>
> > +
> > +#include <base/ngbe_devids.h>
> > +
> > +/*
> > + * The set of PCI devices this driver supports  */ static const
> > +struct rte_pci_id pci_id_ngbe_map[] = {
> > +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_WANGXUN, NGBE_DEV_ID_EM_WX1860A2) },
> > +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_WANGXUN, NGBE_DEV_ID_EM_WX1860A2S) },
> > +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_WANGXUN, NGBE_DEV_ID_EM_WX1860A4) },
> > +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_WANGXUN, NGBE_DEV_ID_EM_WX1860A4S) },
> > +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_WANGXUN, NGBE_DEV_ID_EM_WX1860AL2) },
> > +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_WANGXUN, NGBE_DEV_ID_EM_WX1860AL2S) },
> > +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_WANGXUN, NGBE_DEV_ID_EM_WX1860AL4) },
> > +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_WANGXUN, NGBE_DEV_ID_EM_WX1860AL4S) },
> > +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_WANGXUN, NGBE_DEV_ID_EM_WX1860NCSI) },
> > +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_WANGXUN, NGBE_DEV_ID_EM_WX1860A1) },
> > +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_WANGXUN, NGBE_DEV_ID_EM_WX1860A1L) },
> > +	{ RTE_PCI_DEVICE(PCI_VENDOR_ID_WANGXUN, NGBE_DEV_ID_EM_WX1860AL_W)
> > +},
> 
> Are all these devices supported at once? Or do some devices require extra code
> and it would be clear to add its IDs later?

Yes, all these device IDs need to be supported at once.
Some extra code is added based on different subsystem IDs.

> 
> > +	{ .vendor_id = 0, /* sentinel */ },
> > +};
> > +
> > +static int
> > +eth_ngbe_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> > +		struct rte_pci_device *pci_dev)
> > +{
> > +	RTE_SET_USED(pci_dev);
> > +
> > +	return 0;
> 
> IMHO more correct behaviour of such dummy functions is to return failure.

Get it.

> 
> > +}
> > +
> > +static int eth_ngbe_pci_remove(struct rte_pci_device *pci_dev) {
> > +	RTE_SET_USED(pci_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct rte_pci_driver rte_ngbe_pmd = {
> > +	.id_table = pci_id_ngbe_map,
> > +	.drv_flags = RTE_PCI_DRV_NEED_MAPPING |
> > +		     RTE_PCI_DRV_INTR_LSC,
> 
> LSC should be added here when it is actually supported.
> 
> > +	.probe = eth_ngbe_pci_probe,
> > +	.remove = eth_ngbe_pci_remove,
> > +};
> > +
> > +RTE_PMD_REGISTER_PCI(net_ngbe, rte_ngbe_pmd);
> > +RTE_PMD_REGISTER_PCI_TABLE(net_ngbe, pci_id_ngbe_map);
> > +RTE_PMD_REGISTER_KMOD_DEP(net_ngbe, "* igb_uio | uio_pci_generic |
> > +vfio-pci");
> > +
> >