Message-ID: <YLcLV+p4yZGjdMHO@unreal>
Date:   Wed, 2 Jun 2021 07:38:47 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     David Thompson <davthompson@...dia.com>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        limings@...dia.com, Asmaa Mnebhi <asmaa@...dia.com>
Subject: Re: [PATCH net-next v6] Add Mellanox BlueField Gigabit Ethernet
 driver

On Tue, Jun 01, 2021 at 08:24:55AM -0400, David Thompson wrote:
> This patch adds build and driver logic for the "mlxbf_gige"
> Ethernet driver from Mellanox Technologies. The second
> generation BlueField SoC from Mellanox supports an
> out-of-band GigaBit Ethernet management port to the Arm
> subsystem.  This driver supports TCP/IP network connectivity
> for that port, and provides back-end routines to handle
> basic ethtool requests.
> 
> The driver interfaces to the Gigabit Ethernet block of
> BlueField SoC via MMIO accesses to registers, which contain
> control information or pointers describing transmit and
> receive resources.  There is a single transmit queue, and
> the port supports transmit ring sizes of 4 to 256 entries.
> There is a single receive queue, and the port supports
> receive ring sizes of 32 to 32K entries. The transmit and
> receive rings are allocated from DMA coherent memory. There
> is a 16-bit producer and consumer index per ring to denote
> software ownership and hardware ownership, respectively.
> 
> The main driver logic such as probe(), remove(), and netdev
> ops are in "mlxbf_gige_main.c".  Logic in "mlxbf_gige_rx.c"
> and "mlxbf_gige_tx.c" handles the packet processing for
> receive and transmit respectively.
> 
> The logic in "mlxbf_gige_ethtool.c" supports the handling
> of some basic ethtool requests: get driver info, get ring
> parameters, get registers, and get statistics.
> 
> The logic in "mlxbf_gige_mdio.c" is the driver controlling
> the Mellanox BlueField hardware that interacts with a PHY
> device via MDIO/MDC pins.  This driver does the following:
>   - At driver probe time, it configures several BlueField MDIO
>     parameters such as sample rate, full drive, voltage and MDC
>   - It defines functions to read and write MDIO registers and
>     registers the MDIO bus.
>   - It defines the phy interrupt handler reporting a
>     link up/down status change
>   - This driver's probe is invoked from the main driver logic
>     while the phy interrupt handler is registered in ndo_open.
> 
> Driver limitations
>   - Only supports 1Gbps speed
>   - Only supports GMII protocol
>   - Supports maximum packet size of 2KB
>   - Does not support scatter-gather buffering
> 
> Testing
>   - Successful build of kernel for ARM64, ARM32, X86_64
>   - Tested ARM64 build on FastModels & Palladium
>   - Tested ARM64 build on several Mellanox boards that are built with
>     the BlueField-2 SoC.  The testing includes coverage in the areas
>     of networking (e.g. ping, iperf, ifconfig, route), file transfers
>     (e.g. SCP), and various ethtool options relevant to this driver.
> 
> v5 -> v6

Please put changelog under "---" below your SOBs. We don't need to see
this history in the git log.

>   Fixed use of COMPILE_TEST for ARM32 build; changed driver to not
>   depend on CONFIG_ACPI for ARM32 build
> v4 -> v5
>   Created a separate interrupt controller for the GPIO PHY interrupt
>   and as a result, the GIGE driver no longer depends on GPIO driver
>   Updated the logic in mlxbf_gige_adjust_link() to store the negotiated
>   pause settings into the driver's private settings.
>   Modified logic to only change enable bit in RX_DMA register
>   Changed logic to only map and unmap the actual length of the TX SKB,
>   instead using the default size.
>   Added better error handling to open() method
>   Modified receive packet logic to use polarity bit to signify ownership
>   (software vs. hardware) of the RX CQE slot
> v3 -> v4
>   Main driver module broken out into rx, tx, intr, and ethtool modules
>   Removed some GPIO PHY interrupt logic; moved to GPIO_MLXBF2 driver
> v2 -> v3
>   Added logic to handle PHY link up/down interrupts
>   Use streaming DMA mapping for packet buffers
>   Changed logic to use standard iopoll methods
>   Changed PHY logic to not allow C45 transactions
>   Enhanced the error handling in open() method
>   Enhanced start_xmit() method to use xmit_more mechanism
>   Added support for ndo_get_stats64
>   Removed standard stats from "ethtool -S" output
> v1 -> v2:
>   Fixed all warnings raised by "make C=1" and "make W=1"
>     a) Changed logic in mlxbf_gige_rx_deinit() and mlxbf_gige_tx_deinit()
>        to initialize relevant pointers as NULL, not 0
>     b) Change mlxbf_gige_get_mac_rx_filter() to return void,
>        as this function's return status is not used by caller
>     c) Fixed type definition of "buff" in mlxbf_gige_get_regs()
> 
> Signed-off-by: David Thompson <davthompson@...dia.com>
> Signed-off-by: Asmaa Mnebhi <asmaa@...dia.com>
> Reviewed-by: Liming Sun <limings@...dia.com>
> ---

The patch generates checkpatch warnings.

 CHECK: spinlock_t definition without comment
 #272: FILE: drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h:87:
 +	spinlock_t lock;

 CHECK: spinlock_t definition without comment
 #273: FILE: drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h:88:
 +	spinlock_t gpio_lock;

 CHECK: Macro argument 'tx_wqe_addr' may be better as '(tx_wqe_addr)' to avoid precedence issues
 #328: FILE: drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h:143:
 +#define MLXBF_GIGE_TX_WQE_PKT_LEN(tx_wqe_addr) \
 +	(*(tx_wqe_addr + 1) & MLXBF_GIGE_TX_WQE_PKT_LEN_MASK)

 CHECK: multiple assignments should be avoided
 #1271: FILE: drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c:374:
 +	phydev->irq = priv->mdiobus->irq[addr] = priv->phy_irq;

Thanks

Powered by blists - more mailing lists