From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH 2/3] net/enetc: add ENETC PMD with basic operations Date: Wed, 19 Sep 2018 17:45:41 +0530 Message-ID: References: <20180906055449.21731-1-g.singh@nxp.com> <20180906055449.21731-3-g.singh@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org, ferruh.yigit@intel.com, pankaj.chauhan@nxp.com To: Gagandeep Singh Return-path: Received: from EUR03-AM5-obe.outbound.protection.outlook.com (mail-eopbgr30041.outbound.protection.outlook.com [40.107.3.41]) by dpdk.org (Postfix) with ESMTP id B9CB11B53 for ; Wed, 19 Sep 2018 14:16:03 +0200 (CEST) In-Reply-To: <20180906055449.21731-3-g.singh@nxp.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thursday 06 September 2018 11:24 AM, Gagandeep Singh wrote: > This patch introduces the enetc PMD with basic > initialisation functions includes probe, teardown, > hardware intialisation > > Signed-off-by: Gagandeep Singh > --- > MAINTAINERS | 1 + > config/common_base | 5 + > config/common_linuxapp | 5 + > doc/guides/nics/enetc.rst | 1 + > doc/guides/nics/features/enetc.ini | 2 + > drivers/net/Makefile | 1 + > drivers/net/enetc/Makefile | 24 ++ > drivers/net/enetc/base/enetc_hw.h | 220 ++++++++++++++++ > drivers/net/enetc/enetc.h | 111 ++++++++ > drivers/net/enetc/enetc_ethdev.c | 269 ++++++++++++++++++++ > drivers/net/enetc/enetc_logs.h | 40 +++ > drivers/net/enetc/meson.build | 10 + > drivers/net/enetc/rte_pmd_enetc_version.map | 4 + > drivers/net/meson.build | 1 + > mk/rte.app.mk | 1 + > 15 files changed, 695 insertions(+) > create mode 100644 drivers/net/enetc/Makefile > create mode 100644 drivers/net/enetc/base/enetc_hw.h > create mode 100644 drivers/net/enetc/enetc.h > create mode 100644 drivers/net/enetc/enetc_ethdev.c > create mode 100644 drivers/net/enetc/enetc_logs.h > create mode 100644 drivers/net/enetc/meson.build > create mode 100644 drivers/net/enetc/rte_pmd_enetc_version.map > [...] > diff --git a/drivers/net/enetc/base/enetc_hw.h b/drivers/net/enetc/base/enetc_hw.h > new file mode 100644 > index 000000000..806b26a2c > --- /dev/null > +++ b/drivers/net/enetc/base/enetc_hw.h > @@ -0,0 +1,220 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright 2018 NXP > + */ > + [...] > + > +/* PCI device info */ > +struct enetc_hw { > + /* SI registers, used by all PCI functions */ > + void *reg; > + /* Port registers, PF only */ > + void *port; > + /* IP global registers, PF only */ > + void *global; > +}; A trivial one: Some structures have comments, but some don't. Even the one above has comments *before* the variable. There is a no fixed standard, but it is expected that comments would be uniform across the file. If you see in file enetc/enetc.h, you would observe some comments *after* the variable. Can you make them uniform? > + > +struct enetc_eth_mac_info { > + uint8_t addr[ETH_ADDR_LEN]; > + uint8_t perm_addr[ETH_ADDR_LEN]; > + bool get_link_status; > +}; > + > +struct enetc_eth_hw { > + struct net_device *ndev; > + struct enetc_hw hw; > + uint16_t device_id; > + uint16_t vendor_id; > + uint8_t revision_id; > + struct enetc_eth_mac_info mac; > +}; > + > +/* Transmit Descriptor */ > +struct enetc_tx_desc { > + uint64_t addr; > + uint16_t frm_len; > + uint16_t buf_len; > + uint32_t flags_errors; > +}; > + > +/* TX Buffer Descriptors (BD) */ > +struct enetc_tx_bd { > + uint64_t addr; > + uint16_t buf_len; > + uint16_t frm_len; > + uint16_t err_csum; > + uint16_t flags; > +}; > + > +/* RX buffer descriptor */ > +union enetc_rx_bd { > + struct { > + uint64_t addr; > + uint8_t reserved[8]; > + } w; > + struct { > + uint16_t inet_csum; > + uint16_t parse_summary; > + uint32_t rss_hash; > + uint16_t buf_len; > + uint16_t vlan_opt; > + union { > + struct { > + uint16_t flags; > + uint16_t error; > + }; > + uint32_t lstatus; > + }; > + } r; > +}; [...] > +#endif /* _ENETC_H_ */ > diff --git a/drivers/net/enetc/enetc_ethdev.c b/drivers/net/enetc/enetc_ethdev.c > new file mode 100644 > index 000000000..06438835d > --- /dev/null > +++ b/drivers/net/enetc/enetc_ethdev.c > @@ -0,0 +1,269 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright 2018 NXP > + */ > + > +#include > +#include > + > +#include "enetc_logs.h" > +#include "enetc.h" > + > +int enetc_logtype_pmd; > + > +/* Functions Prototypes */ > +static int enetc_dev_configure(struct rte_eth_dev *dev); > +static int enetc_dev_start(struct rte_eth_dev *dev); > +static void enetc_dev_stop(struct rte_eth_dev *dev); > +static void enetc_dev_close(struct rte_eth_dev *dev); > +static void enetc_dev_infos_get(struct rte_eth_dev *dev, > + struct rte_eth_dev_info *dev_info); > +static int enetc_link_update(struct rte_eth_dev *dev, int wait_to_complete); > +static int enetc_hardware_init(struct enetc_eth_hw *hw); > + > +/* > + * The set of PCI devices this driver supports > + */ > +static const struct rte_pci_id pci_id_enetc_map[] = { > + { RTE_PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, ENETC_DEV_ID) }, > + { RTE_PCI_DEVICE(PCI_VENDOR_ID_FREESCALE, ENETC_DEV_ID_VF) }, > + { .vendor_id = 0, /* sentinel */ }, > +}; > + > +/* Features supported by this driver */ > +static const struct eth_dev_ops enetc_ops = { > + .dev_configure = enetc_dev_configure, > + .dev_start = enetc_dev_start, > + .dev_stop = enetc_dev_stop, > + .dev_close = enetc_dev_close, > + .link_update = enetc_link_update, > + .dev_infos_get = enetc_dev_infos_get, > +}; > + > +/** > + * Initialisation of the enetc device > + * > + * @param eth_dev > + * - Pointer to the structure rte_eth_dev > + * > + * @return > + * - On success, zero. > + * - On failure, negative value. > + */ > +static int > +enetc_dev_init(struct rte_eth_dev *eth_dev) > +{ > + int error = 0, i; > + struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev); > + struct enetc_eth_hw *hw = > + ENETC_DEV_PRIVATE_TO_HW(eth_dev->data->dev_private); > + struct enetc_eth_adapter *adapter = > + ENETC_DEV_PRIVATE(eth_dev->data->dev_private); > + > + PMD_INIT_FUNC_TRACE(); > + eth_dev->dev_ops = &enetc_ops; > + eth_dev->rx_pkt_burst = NULL; > + eth_dev->tx_pkt_burst = NULL; > + > + rte_eth_copy_pci_info(eth_dev, pci_dev); > + > + /* Retrieving and storing the HW base address of device */ > + hw->hw.reg = (void *)pci_dev->mem_resource[0].addr; > + > + adapter->tx_bd_count = MAX_BD_COUNT; > + adapter->rx_bd_count = MAX_BD_COUNT; > + > + adapter->num_rx_rings = MAX_RX_RINGS; > + adapter->num_tx_rings = MAX_TX_RINGS; > + > + for (i = 0; i < adapter->num_rx_rings; i++) { > + adapter->rx_ring[i] = rte_zmalloc(NULL, > + sizeof(struct enetc_bdr), 0); > + if (!adapter->rx_ring[i]) { > + ENETC_PMD_ERR("Failed to allocate RX ring memory"); > + while (--i >= 0) > + rte_free(adapter->rx_ring[i]); > + error = -ENOMEM; > + goto err_late; > + } > + adapter->rx_ring[i]->bd_count = adapter->rx_bd_count; > + } > + > + for (i = 0; i < adapter->num_tx_rings; i++) { > + adapter->tx_ring[i] = rte_zmalloc(NULL, > + sizeof(struct enetc_bdr), 0); > + if (!adapter->tx_ring[i]) { > + ENETC_PMD_ERR("Failed to allocate TX ring memory"); > + while (--i >= 0) > + rte_free(adapter->tx_ring[i]); > + error = -ENOMEM; > + goto err_second; > + } > + adapter->tx_ring[i]->bd_count = adapter->tx_bd_count; > + } > + > + error = enetc_hardware_init(hw); > + if (error != 0) { > + ENETC_PMD_ERR("Hardware initialization failed"); > + goto err_first; > + } > + > + /* Allocate memory for storing MAC addresses */ > + eth_dev->data->mac_addrs = rte_zmalloc("enetc_eth", > + ETHER_ADDR_LEN, 0); > + if (!eth_dev->data->mac_addrs) { > + ENETC_PMD_ERR("Failed to allocate %d bytes needed to " > + "store MAC addresses", > + ETHER_ADDR_LEN * 1); Formatting of the above log is not correct. It should be in format: FUNCTION_NAME(Some argument, argument on new line after just below the first, ...); > + error = -ENOMEM; > + goto err_first; > + } > + > + /* Copy the permanent MAC address */ > + ether_addr_copy((struct ether_addr *)hw->mac.addr, > + ð_dev->data->mac_addrs[0]); Alignment of the second argument should be below the first one. > + > + ENETC_PMD_DEBUG("port_id %d vendorID=0x%x deviceID=0x%x", > + eth_dev->data->port_id, pci_dev->id.vendor_id, > + pci_dev->id.device_id); > + return 0; > + > +err_first: > + for (i = 0; i < adapter->num_tx_rings; i++) > + rte_free(adapter->tx_ring[i]); > +err_second: > + for (i = 0; i < adapter->num_rx_rings; i++) > + rte_free(adapter->rx_ring[i]); > +err_late: > + return error; > +} > + > +static int > +enetc_dev_uninit(struct rte_eth_dev *eth_dev) > +{ Some functions have FUNC_TRACE, some don't. Maybe you should make it uniform. > + return 0; > +} > + > +static int > +enetc_dev_configure(struct rte_eth_dev *dev) > +{ > + PMD_INIT_FUNC_TRACE(); > + return 0; > +} > + > +static int > +enetc_dev_start(struct rte_eth_dev *dev) > +{ > + PMD_INIT_FUNC_TRACE(); > + return 0; > +} > + > +static void > +enetc_dev_stop(struct rte_eth_dev *dev) > +{ > +} > + > +static void > +enetc_dev_close(struct rte_eth_dev *dev) > +{ > +} > + > +/* return 0 means link status changed, -1 means not changed */ > +static int > +enetc_link_update(struct rte_eth_dev *dev, int wait_to_complete) > +{ > + struct enetc_eth_hw *hw = > + ENETC_DEV_PRIVATE_TO_HW(dev->data->dev_private); > + struct rte_eth_link link; > + > + hw->mac.get_link_status = 1; > + > + memset(&link, 0, sizeof(link)); > + rte_eth_linkstatus_get(dev, &link); > + > + link.link_duplex = ETH_LINK_FULL_DUPLEX; > + link.link_status = ETH_LINK_UP; > + rte_eth_linkstatus_set(dev, &link); > + > + return 0; > +} > + > +static int > +enetc_hardware_init(struct enetc_eth_hw *hw) > +{ > + uint32_t psipmr = 0; > + > + /* Calculating and storing the base HW addresses */ > + hw->hw.port = hw->hw.reg + ENETC_PORT_BASE; > + hw->hw.global = hw->hw.reg + ENETC_GLOBAL_BASE; > + > + /* Enabling Station Interface */ > + ENETC_REG_WRITE(ENETC_GET_HW_ADDR(hw->hw.reg, ENETC_SIMR), > + ENETC_SIMR_EN); Second argument on new line should start below first. > + > + /* Setting to accept broadcast packets for each inetrface */ > + psipmr |= ENETC_PSIPMR_SET_UP(0) | ENETC_PSIPMR_SET_MP(0) | > + ENETC_PSIPMR_SET_VLAN_MP(0); > + psipmr |= ENETC_PSIPMR_SET_UP(1) | ENETC_PSIPMR_SET_MP(1) | > + ENETC_PSIPMR_SET_VLAN_MP(1); > + psipmr |= ENETC_PSIPMR_SET_UP(2) | ENETC_PSIPMR_SET_MP(2) | > + ENETC_PSIPMR_SET_VLAN_MP(2); > + > + ENETC_REG_WRITE(ENETC_GET_HW_ADDR(hw->hw.port, ENETC_PSIPMR), > + psipmr); > + > + ENETC_REG_WRITE(ENETC_GET_HW_ADDR(hw->hw.port, ENETC_PM0_CMD_CFG), > + ENETC_PM0_TX_EN | ENETC_PM0_RX_EN); > + > + /* Enable port */ > + ENETC_REG_WRITE(ENETC_GET_HW_ADDR(hw->hw.port, ENETC_PMR), > + ENETC_PMR_EN); > + > + /* Enabling broadcast address */ > + ENETC_REG_WRITE(ENETC_GET_HW_ADDR(hw->hw.port, ENETC_PSIPMAR0(0)), > + 0xFFFFFFFF); > + ENETC_REG_WRITE(ENETC_GET_HW_ADDR(hw->hw.port, ENETC_PSIPMAR1(0)), > + 0xFFFF << 16); > + > + return 0; > +} [...] > diff --git a/mk/rte.app.mk b/mk/rte.app.mk > index de33883be..154ae3b2c 100644 > --- a/mk/rte.app.mk > +++ b/mk/rte.app.mk > @@ -135,6 +135,7 @@ endif > _LDLIBS-$(CONFIG_RTE_LIBRTE_E1000_PMD) += -lrte_pmd_e1000 > _LDLIBS-$(CONFIG_RTE_LIBRTE_ENA_PMD) += -lrte_pmd_ena > _LDLIBS-$(CONFIG_RTE_LIBRTE_ENIC_PMD) += -lrte_pmd_enic > +_LDLIBS-$(CONFIG_RTE_LIBRTE_ENETC_PMD) += -lrte_pmd_enetc > _LDLIBS-$(CONFIG_RTE_LIBRTE_FM10K_PMD) += -lrte_pmd_fm10k > _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_FAILSAFE) += -lrte_pmd_failsafe > _LDLIBS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += -lrte_pmd_i40e > 32 bit compilation for this is failing with errors like this: /home/shreyansh/build/DPDK/05_dpdk/drivers/net/enetc/enetc_rxtx.c: In function ‘enetc_xmit_pkts’: /home/shreyansh/build/DPDK/05_dpdk/drivers/net/enetc/enetc_rxtx.c:73:3: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] (uint64_t)rte_cpu_to_le_64(tx_swbd->buffer_addr->buf_addr + ^ /home/shreyansh/build/DPDK/05_dpdk/drivers/net/enetc/enetc_rxtx.c: In function ‘enetc_refill_rx_ring’: /home/shreyansh/build/DPDK/05_dpdk/drivers/net/enetc/enetc_rxtx.c:98:18: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] rxbd->w.addr = (uint64_t)rx_swbd->buffer_addr->buf_addr + ^ In file included from /home/shreyansh/build/DPDK/05_dpdk/drivers/net/enetc/enetc_rxtx.c:13:0: /home/shreyansh/build/DPDK/05_dpdk/drivers/net/enetc/enetc_rxtx.c: In function ‘enetc_setup_txbdr’: /home/shreyansh/build/DPDK/05_dpdk/drivers/net/enetc/enetc_rxtx.c:293:18: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] lower_32_bits((uint64_t)tx_ring->bd_base)); ^ /home/shreyansh/build/DPDK/05_dpdk/drivers/net/enetc/base/enetc_hw.h:108:45: note: in definition of macro ‘enetc_wr_reg’ #define enetc_wr_reg(reg, val) rte_write32((val), (reg)) ^~~ /home/shreyansh/build/DPDK/05_dpdk/drivers/net/enetc/base/enetc_hw.h:121:5: note: in expansion of macro ‘enetc_wr’ enetc_wr(hw, ENETC_BDR(t, n, off), val) ^~~~~~~~ /home/shreyansh/build/DPDK/05_dpdk/drivers/net/enetc/base/enetc_hw.h:126:5: note: in expansion of macro ‘enetc_bdr_wr’ enetc_bdr_wr(hw, TX, n, off, val) ^~~~~~~~~~~~ /home/shreyansh/build/DPDK/05_dpdk/drivers/net/enetc/enetc_rxtx.c:292:2: note: in expansion of macro ‘enetc_txbdr_wr’