> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: Wednesday, October 27, 2021 7:49 PM
> To: Apeksha Gupta <apeksha.gu...@nxp.com>;
> david.march...@redhat.com; andrew.rybche...@oktetlabs.ru
> Cc: dev@dpdk.org; Sachin Saxena <sachin.sax...@nxp.com>; Hemant
> Agrawal <hemant.agra...@nxp.com>
> Subject: [EXT] Re: [dpdk-dev] [PATCH v6 1/5] net/enetfec: introduce NXP
> ENETFEC driver
>
> Caution: EXT Email
>
> On 10/21/2021 5:46 AM, Apeksha Gupta wrote:
> > ENETFEC (Fast Ethernet Controller) is a network poll mode driver
> > for NXP SoC i.MX 8M Mini.
> >
> > This patch adds skeleton for enetfec driver with probe function.
> >
> > Signed-off-by: Sachin Saxena <sachin.sax...@nxp.com>
> > Signed-off-by: Apeksha Gupta <apeksha.gu...@nxp.com>
>
> <...>
>
> > +Follow instructions available in the document
> > +:ref:`compiling and testing a PMD for a NIC <pmd_build_and_test>`
> > +to launch **dpdk-testpmd**
> > +
> > +Limitations
> > +~~~~~~~~~~~
> > +
> > +- Multi queue is not supported.
>
> in 'enetfec_eth_info()'
> max_rx_queues/max_tx_queues returned as 3 (ENETFEC_MAX_Q).
> If multi queue is not supported why it is not one?
[Apeksha] I agree, As multi queue is not supported we will update ENETFEC_MAX_Q
from 3 to 1 in v8 patch series.
>
> <...>
>
> > --- a/doc/guides/rel_notes/release_21_11.rst
> > +++ b/doc/guides/rel_notes/release_21_11.rst
> > @@ -20,6 +20,10 @@ DPDK Release 21.11
> > ninja -C build doc
> > xdg-open build/doc/guides/html/rel_notes/release_21_11.html
> >
> > +* **Added NXP ENETFEC PMD.**
> > +
> > + Added the new ENETFEC driver for the NXP IMX8MMEVK platform. See
> the
> > + :doc:`../nics/enetfec` NIC driver guide for more details on this new
> > driver.
> >
>
> Update is in the doc comment, can you please move it down, within the
> ethdev
> driver group in alphabetically sorted manner.
[Apeksha] okay.
>
> <...>
>
> > +static int
> > +pmd_enetfec_probe(struct rte_vdev_device *vdev)
> > +{
> > + struct rte_eth_dev *dev = NULL;
> > + struct enetfec_private *fep;
> > + const char *name;
> > + int rc;
> > +
> > + name = rte_vdev_device_name(vdev);
> > + if (name == NULL)
> > + return -EINVAL;
>
> Can name be 'NULL'? Not sure if we need this check, can you please check?
[Apeksha] yes, this check is required as ' rte_vdev_device_name' may return
NULL.
rte_vdev_device_name(const struct rte_vdev_device *dev)
{
if (dev && dev->device.name)
return dev->device.name;
return NULL;
}
>
> <...>
>
> > +static int
> > +pmd_enetfec_remove(struct rte_vdev_device *vdev)
> > +{
> > + struct rte_eth_dev *eth_dev = NULL;
> > + int ret;
> > +
> > + /* find the ethdev entry */
> > + eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(vdev));
> > + if (eth_dev == NULL)
> > + return -ENODEV;
> > +
> > + ret = rte_eth_dev_release_port(eth_dev);
> > + if (ret != 0)
> > + return -EINVAL;
> > +
> > + ENETFEC_PMD_INFO("Closing sw device");
>
> Log can be misleading, there is another dev_ops to close the device.
[Apeksha] Okay. Updated in v7 series.
>
> <...>
>
> > @@ -0,0 +1,179 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright 2020-2021 NXP
> > + */
> > +
> > +#ifndef __ENETFEC_ETHDEV_H__
> > +#define __ENETFEC_ETHDEV_H__
> > +
> > +#include <rte_ethdev.h>
> > +
> > +/*
> > + * ENETFEC with AVB IP can support maximum 3 rx and tx queues.
> > + */
> > +#define ENETFEC_MAX_Q 3
> > +
> > +#define ETHER_ADDR_LEN 6
> > +#define BD_LEN 49152
> > +#define ENETFEC_TX_FR_SIZE 2048
> > +#define MAX_TX_BD_RING_SIZE 512 /* It should be power of 2 */
> > +#define MAX_RX_BD_RING_SIZE 512
> > +
> > +/* full duplex or half duplex */
> > +#define HALF_DUPLEX 0x00
> > +#define FULL_DUPLEX 0x01
> > +#define UNKNOWN_DUPLEX 0xff
> > +
>
> Some of the defines in this header is not used at all. What about
> only adding structs/defines that are used? And add them as they are
> needed?
> This guarantees not having unused clutter in the code.
[Apeksha] I agree. We will update in v8 version.
>
> <...>
>
> > +/* Required types */
> > +typedef uint8_t u8;
> > +typedef uint16_t u16;
> > +typedef uint32_t u32;
> > +typedef uint64_t u64;
> > +
>
> Do we need these type definitions, as far as I can see they are used only
> in a few places, why not just use uint##_t types?
>
> <...>
>
> > +static inline struct
> > +bufdesc *enet_get_nextdesc(struct bufdesc *bdp, struct bufdesc_prop
> *bd)
> > +{
> > + return (bdp >= bd->last) ? bd->base
> > + : (struct bufdesc *)(((uintptr_t)bdp) + bd->d_size);
> > +}
> > +
> > +static inline struct
> > +bufdesc *enet_get_prevdesc(struct bufdesc *bdp, struct bufdesc_prop
> *bd)
> > +{
> > + return (bdp <= bd->base) ? bd->last
> > + : (struct bufdesc *)(((uintptr_t)bdp) - bd->d_size);
> > +}
> > +
> > +static inline int
> > +enet_get_bd_index(struct bufdesc *bdp, struct bufdesc_prop *bd)
> > +{
> > + return ((const char *)bdp - (const char *)bd->base) >>
> > bd->d_size_log2;
> > +}
> > +
> > +static inline int
> > +fls64(unsigned long word)
> > +{
> > + return (64 - __builtin_clzl(word)) - 1;
> > +}
> > +
>
> Same for these static inline functions, can you please add they when that are
> needed?
>
> > +uint16_t enetfec_recv_pkts(void *rxq1, __rte_unused struct rte_mbuf
> **rx_pkts,
> > + uint16_t nb_pkts);
> > +uint16_t enetfec_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
> > + uint16_t nb_pkts);
>
> These function definitions are not declared, at least not in this patch.
>
> > +struct bufdesc *enet_get_nextdesc(struct bufdesc *bdp,
> > + struct bufdesc_prop *bd);
>
> This is already static inline function, do we need separate definition for it?
>
> > +int enet_new_rxbdp(struct enetfec_private *fep, struct bufdesc *bdp,
> > + struct rte_mbuf *mbuf);
> > +
>
> ditto, no function declaration.
>
> <...>
>
> > +
> > +/* DP Logs, toggled out at compile time if level lower than current level
> > */
> > +#define ENETFEC_DP_LOG(level, fmt, args...) \
> > + RTE_LOG_DP(level, PMD, fmt, ## args)
> > +
>
> Not used at all.
>
> > +#endif /* _ENETFEC_LOGS_H_ */
> > diff --git a/drivers/net/enetfec/meson.build
> b/drivers/net/enetfec/meson.build
> > new file mode 100644
> > index 0000000000..79dca58dea
> > --- /dev/null
> > +++ b/drivers/net/enetfec/meson.build
> > @@ -0,0 +1,11 @@
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +# Copyright 2021 NXP
> > +
> > +if not is_linux
> > + build = false
> > + reason = 'only supported on linux'
> > +endif
> > +
> > +sources = files('enet_ethdev.c',
> > + 'enet_uio.c',
> > + 'enet_rxtx.c')
>
> This should cause build error on this patch, isn't it? Since these files don't
> exist in this patch.
> Each patch should be built successfully.
[Apeksha] I agree, all code cleanup comments are handled in v7 patch series.
Also as suggested each patch is built successfully.
