[dpdk-dev] [EXT] Re: [PATCH v2 02/11] net/octeontx_ep: add ethdev probe and remove

[Pradeep Kumar Nalla]

-----Original Message-----
From: Ferruh Yigit <ferruh.yigit at intel.com> 
Sent: Tuesday, January 26, 2021 8:57 PM
To: Pradeep Kumar Nalla <pnalla at marvell.com>; Jerin Jacob Kollanukkaran <jerinj at marvell.com>; Nithin Kumar Dabilpuram <ndabilpuram at marvell.com>; Radha Chintakuntla <radhac at marvell.com>; Veerasenareddy Burru <vburru at marvell.com>
Cc: dev at dpdk.org; Satananda Burla <sburla at marvell.com>
Subject: [EXT] Re: [dpdk-dev] [PATCH v2 02/11] net/octeontx_ep: add ethdev probe and remove

External Email

----------------------------------------------------------------------
On 1/18/2021 9:35 AM, Nalla Pradeep wrote:
> add basic PCIe ethdev probe and remove.
> 
> Signed-off-by: Nalla Pradeep <pnalla at marvell.com>

<...>

> @@ -136,7 +136,10 @@ extern int otx2_logtype_ree;
>   #define PCI_DEVID_OCTEONTX2_RVU_CPT_VF		0xA0FE
>   #define PCI_DEVID_OCTEONTX2_RVU_AF_VF		0xA0f8
>   #define PCI_DEVID_OCTEONTX2_DPI_VF		0xA081
> -#define PCI_DEVID_OCTEONTX2_EP_VF		0xB203 /* OCTEON TX2 EP mode */
> +#define PCI_DEVID_OCTEONTX2_EP_NET_VF		0xB203 /* OCTEON TX2 EP mode */

You can considering doing this rename on its own patch, to reduce the noise for the actual patch.

<...>

> +++ b/drivers/net/octeontx_ep/meson.build
> @@ -6,3 +6,16 @@ sources = files(
>                  'otx_ep_ethdev.c',
>                  )
>   
> +extra_flags = []
> +# This integrated controller runs only on a arm64 machine, remove 
> +32bit warnings if not dpdk_conf.get('RTE_ARCH_64')
> +        extra_flags += ['-Wno-int-to-pointer-cast', 
> +'-Wno-pointer-to-int-cast'] endif

There is almost any code at this stage, are above compiler flags really needed? 
Can you add then only when they are really needed?

<...>

> --- a/drivers/net/octeontx_ep/otx_ep_ethdev.c
> +++ b/drivers/net/octeontx_ep/otx_ep_ethdev.c
> @@ -1,3 +1,65 @@
>   /* SPDX-License-Identifier: BSD-3-Clause
>    * Copyright(C) 2020 Marvell.
>    */
> +
> +#include <rte_ethdev_pci.h>
> +#include <rte_malloc.h>
> +#include <rte_io.h>
> +

Are all these headers needed, no io or mem allocation yet, can you please add them as they are needed?

<...>

> +++ b/drivers/net/octeontx_ep/otx_ep_vf.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2020 Marvell.
> + */
> +#ifndef _OTX_EP_VF_H_
> +#define _OTX_EP_VF_H_
> +
> +#define PCI_DEVID_OCTEONTX_EP_VF 0xa303
> +

>Why this PCI device id defined different place than all the others did, won't it be easier to keep them all in same >place?
This device id is from octeontx family, whereas rest all are from octeontx2 family. All the device ids of octeontx2 family are maintained in  drivers/common/octeontx2/otx2_common.h and this particular octeon tx device will be in drivers/net/octeontx_ep/otx_ep_vf.h