Re: [dpdk-dev] [EXT] Re: [PATCH v6 1/5] net/enetfec: introduce NXP ENETFEC driver

[Apeksha Gupta <apeksha.gupta@nxp.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, October 27, 2021 7:49 PM
> To: Apeksha Gupta <apeksha.gupta@nxp.com>;
> david.marchand@redhat.com; andrew.rybchenko@oktetlabs.ru
> Cc: dev@dpdk.org; Sachin Saxena <sachin.saxena@nxp.com>; Hemant
> Agrawal <hemant.agrawal@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.saxena@nxp.com>
> > Signed-off-by: Apeksha Gupta <apeksha.gupta@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.