-----Original Message-----
From: Ferruh Yigit <redacted>
Sent: Wednesday, October 27, 2021 7:49 PM
To: Apeksha Gupta <redacted>;
david.marchand@redhat.com; andrew.rybchenko@oktetlabs.ru
Cc: dev@dpdk.org; Sachin Saxena <redacted>; Hemant
Agrawal [off-list ref]
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:

quoted

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 <redacted>
Signed-off-by: Apeksha Gupta <redacted>

<...>

quoted

+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.

<...>

quoted

--- 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

quoted

+  :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.

<...>

quoted

+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;
}

<...>

quoted

+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.

<...>

quoted

@@ -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.

<...>

quoted

+/* 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?

<...>

quoted

+static inline struct
+bufdesc *enet_get_nextdesc(struct bufdesc *bdp, struct bufdesc_prop

*bd)

quoted

+{
+     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)

quoted

+{
+     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?

quoted

+uint16_t enetfec_recv_pkts(void *rxq1, __rte_unused struct rte_mbuf

**rx_pkts,

quoted

+             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.

quoted

+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?

quoted

+int enet_new_rxbdp(struct enetfec_private *fep, struct bufdesc *bdp,
+             struct rte_mbuf *mbuf);
+

ditto, no function declaration.

<...>

quoted

+
+/* 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.

quoted

+#endif /* _ENETFEC_LOGS_H_ */

diff --git a/drivers/net/enetfec/meson.build

b/drivers/net/enetfec/meson.build

quoted

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.