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