On 10/28/2024 7:30 AM, Howard Wang wrote:
> Meson build infrastructure, r8169_ethdev minimal skeleton,
> header with Realtek NIC device and vendor IDs.
>
> Signed-off-by: Howard Wang <howard_w...@realsil.com.cn>
> ---
> MAINTAINERS | 7 ++
> drivers/net/meson.build | 1 +
> drivers/net/r8169/meson.build | 6 ++
> drivers/net/r8169/r8169_base.h | 15 +++
> drivers/net/r8169/r8169_ethdev.c | 178 +++++++++++++++++++++++++++++++
> drivers/net/r8169/r8169_ethdev.h | 40 +++++++
> 6 files changed, 247 insertions(+)
> create mode 100644 drivers/net/r8169/meson.build
> create mode 100644 drivers/net/r8169/r8169_base.h
> create mode 100644 drivers/net/r8169/r8169_ethdev.c
> create mode 100644 drivers/net/r8169/r8169_ethdev.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c5a703b5c0..5f9eccc43f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1076,6 +1076,13 @@ F: drivers/net/memif/
> F: doc/guides/nics/memif.rst
> F: doc/guides/nics/features/memif.ini
>
> +Realtek r8169
> +M: Howard Wang <howard_w...@realsil.com.cn>
> +M: ChunHao Lin <h...@realtek.com>
> +M: Xing Wang <xing_w...@realsil.com.cn>
> +M: Realtek NIC SW <pro_nic_d...@realtek.com>
>
We prefer authors as maintainer, instead of group/alias, and as you
already provide names, can this be removed?
> +F: drivers/net/r8169
> +
>
Please sort alphabetically for 'Realtek'
Can you also add driver documentation and update release notes in this
patch?
>
> Crypto Drivers
> --------------
> diff --git a/drivers/net/meson.build b/drivers/net/meson.build
> index fb6d34b782..fddcf39655 100644
> --- a/drivers/net/meson.build
> +++ b/drivers/net/meson.build
> @@ -53,6 +53,7 @@ drivers = [
> 'pfe',
> 'qede',
> 'ring',
> + 'r8169',
> 'sfc',
> 'softnic',
> 'tap',
> diff --git a/drivers/net/r8169/meson.build b/drivers/net/r8169/meson.build
> new file mode 100644
> index 0000000000..f14d4ae8fb
> --- /dev/null
> +++ b/drivers/net/r8169/meson.build
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2024 Realtek Corporation. All rights reserved
> +
> +sources = files(
> + 'r8169_ethdev.c',
> +)
> diff --git a/drivers/net/r8169/r8169_base.h b/drivers/net/r8169/r8169_base.h
> new file mode 100644
> index 0000000000..6fc84592a6
> --- /dev/null
> +++ b/drivers/net/r8169/r8169_base.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2024 Realtek Corporation. All rights reserved
> + */
> +
> +#ifndef _R8169_BASE_H_
> +#define _R8169_BASE_H_
> +
> +typedef uint8_t u8;
> +typedef uint16_t u16;
> +typedef uint32_t u32;
> +typedef uint64_t u64;
>
most of the drivers has 'compat.h' or '<driver>_compat.h'
(r8169_compat.h) as compatibility layer for DPDK and define above like
structures there.
If there is no specific reason to have this file name, you can create a
compat.h for compatibility.
> +
> +#define PCI_VENDOR_ID_REALTEK 0x10EC
> +
> +#endif
> diff --git a/drivers/net/r8169/r8169_ethdev.c
> b/drivers/net/r8169/r8169_ethdev.c
> new file mode 100644
> index 0000000000..1f90a142c5
> --- /dev/null
> +++ b/drivers/net/r8169/r8169_ethdev.c
> @@ -0,0 +1,178 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2024 Realtek Corporation. All rights reserved
> + */
> +
> +#include <sys/queue.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <stdint.h>
> +#include <stdarg.h>
> +
> +#include <rte_eal.h>
> +
> +#include <rte_string_fns.h>
> +#include <rte_common.h>
> +#include <rte_interrupts.h>
> +#include <rte_byteorder.h>
> +#include <rte_log.h>
> +#include <rte_debug.h>
> +#include <rte_pci.h>
> +#include <bus_pci_driver.h>
> +#include <rte_ether.h>
> +#include <ethdev_driver.h>
> +#include <ethdev_pci.h>
> +#include <rte_memory.h>
> +#include <rte_malloc.h>
> +#include <dev_driver.h>
>
Do you really need all these headers here?
Please only include necessary ones and add them when you need them.
> +
> +#include "r8169_ethdev.h"
> +#include "r8169_base.h"
> +
> +static int rtl_dev_configure(struct rte_eth_dev *dev __rte_unused);
>
No need to have '__rte_unused' attribute for function decleration.
> +static int rtl_dev_start(struct rte_eth_dev *dev);
> +static int rtl_dev_stop(struct rte_eth_dev *dev);
> +static int rtl_dev_reset(struct rte_eth_dev *dev);
> +static int rtl_dev_close(struct rte_eth_dev *dev);
> +
> +/*
> + * The set of PCI devices this driver supports
> + */
> +static const struct rte_pci_id pci_id_r8169_map[] = {
> + { RTE_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8125) },
> + { RTE_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8162) },
> + { RTE_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x8126) },
> + { RTE_PCI_DEVICE(PCI_VENDOR_ID_REALTEK, 0x5000) },
> + {.vendor_id = 0, /* sentinel */ },
> +};
> +
> +static const struct eth_dev_ops rtl_eth_dev_ops = {
> + .dev_configure = rtl_dev_configure,
> + .dev_start = rtl_dev_start,
> + .dev_stop = rtl_dev_stop,
> + .dev_close = rtl_dev_close,
> + .dev_reset = rtl_dev_reset,
> +};
> +
> +static int
> +rtl_dev_configure(struct rte_eth_dev *dev __rte_unused)
> +{
> + return 0;
> +}
> +
> +/*
> + * Configure device link speed and setup link.
> + * It returns 0 on success.
> + */
> +static int
> +rtl_dev_start(struct rte_eth_dev *dev)
> +{
> + struct rtl_adapter *adapter = RTL_DEV_PRIVATE(dev);
> + struct rtl_hw *hw = &adapter->hw;
> +
> + hw->adapter_stopped = 0;
> +
> + return 0;
> +}
> +
> +/*
> + * Stop device: disable RX and TX functions to allow for reconfiguring.
> + */
> +static int
> +rtl_dev_stop(struct rte_eth_dev *dev)
> +{
> + struct rtl_adapter *adapter = RTL_DEV_PRIVATE(dev);
> + struct rtl_hw *hw = &adapter->hw;
> +
> + if (hw->adapter_stopped)
> + return 0;
>
There is 'dev->data->dev_started' variable for exact same reason, and it
is checked in 'rte_eth_dev_stop()' level, do you need this driver
version of same flag?
> +
> + hw->adapter_stopped = 1;
> + dev->data->dev_started = 0;
>
This flag is set in ethdev layer level, no need to set it here.
> +
> + return 0;
> +}
> +
> +/*
> + * Reset and stop device.
> + */
> +static int
> +rtl_dev_close(struct rte_eth_dev *dev)
> +{
> + int ret_stp;
> +
> + if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> + return 0;
> +
> + ret_stp = rtl_dev_stop(dev);
> +
> + return ret_stp;
> +}
> +
> +static int
> +rtl_dev_init(struct rte_eth_dev *dev)
> +{
> + struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> + struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
>
'intr_handle' not used yet, please add it when used.
> + struct rtl_adapter *adapter = RTL_DEV_PRIVATE(dev);
> + struct rtl_hw *hw = &adapter->hw;
> +
> + dev->dev_ops = &rtl_eth_dev_ops;
> +
> + /* For secondary processes, the primary process has done all the work */
> + if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> + return 0;
> +
> + rte_eth_copy_pci_info(dev, pci_dev);
>
This is already done as part of 'rte_eth_dev_pci_generic_probe()',
shouldn't need to do here. Can you please double check?
> +
> + return 0;
> +}
> +
> +static int
> +rtl_dev_uninit(struct rte_eth_dev *dev)
> +{
> + if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> + return -EPERM;
> +
> + rtl_dev_close(dev);
> +
> + return 0;
> +}
> +
> +static int
> +rtl_dev_reset(struct rte_eth_dev *dev)
> +{
> + int ret;
> +
> + ret = rtl_dev_uninit(dev);
> + if (ret)
> + return ret;
> +
> + ret = rtl_dev_init(dev);
> +
> + return ret;
> +}
> +
> +static int
> +rtl_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
> + struct rte_pci_device *pci_dev)
> +{
> + return rte_eth_dev_pci_generic_probe(pci_dev, sizeof(struct
> rtl_adapter),
> + rtl_dev_init);
> +}
> +
> +static int
> +rtl_pci_remove(struct rte_pci_device *pci_dev)
> +{
> + return rte_eth_dev_pci_generic_remove(pci_dev, rtl_dev_uninit);
> +}
> +
> +static struct rte_pci_driver rte_r8169_pmd = {
> + .id_table = pci_id_r8169_map,
> + .drv_flags = RTE_PCI_DRV_NEED_MAPPING | RTE_PCI_DRV_INTR_LSC,
> + .probe = rtl_pci_probe,
> + .remove = rtl_pci_remove,
> +};
> +
> +RTE_PMD_REGISTER_PCI(net_r8169, rte_r8169_pmd);
> +RTE_PMD_REGISTER_PCI_TABLE(net_r8169, pci_id_r8169_map);
> +RTE_PMD_REGISTER_KMOD_DEP(net_r8169, "* igb_uio | uio_pci_generic |
> vfio-pci");
> diff --git a/drivers/net/r8169/r8169_ethdev.h
> b/drivers/net/r8169/r8169_ethdev.h
> new file mode 100644
> index 0000000000..e37f05c153
> --- /dev/null
> +++ b/drivers/net/r8169/r8169_ethdev.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2024 Realtek Corporation. All rights reserved
> + */
> +
> +#ifndef _R8169_ETHDEV_H_
> +#define _R8169_ETHDEV_H_
> +
> +#include <stdint.h>
> +#include <stdbool.h>
> +
> +#include <rte_ethdev.h>
> +#include <rte_ethdev_core.h>
> +
> +#include "r8169_base.h"
> +
> +struct rtl_hw {
> + u8 adapter_stopped;
> +};
> +
> +struct rtl_sw_stats {
> + u64 tx_packets;
> + u64 tx_bytes;
> + u64 tx_errors;
> + u64 rx_packets;
> + u64 rx_bytes;
> + u64 rx_errors;
> +};
> +
> +struct rtl_adapter {
> + struct rtl_hw hw;
> + struct rtl_sw_stats sw_stats;
> +};
> +
> +#define RTL_DEV_PRIVATE(eth_dev) \
> + ((struct rtl_adapter *)((eth_dev)->data->dev_private))
> +
> +uint16_t rtl_xmit_pkts(void *txq, struct rte_mbuf **tx_pkts, uint16_t
> nb_pkts);
> +uint16_t rtl_recv_pkts(void *rxq, struct rte_mbuf **rx_pkts, uint16_t
> nb_pkts);
>
These functions don't exists yet, please add the declarations when
functions added.
> +
> +#endif
