Message-ID: <BN9PR12MB534065BE42BA2951929F6CEEC7389@BN9PR12MB5340.namprd12.prod.outlook.com>
Date:   Mon, 7 Jun 2021 14:47:28 +0000
From:   David Thompson <davthompson@...dia.com>
To:     Leon Romanovsky <leon@...nel.org>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Liming Sun <limings@...dia.com>,
        Asmaa Mnebhi <asmaa@...dia.com>
Subject: RE: [PATCH net-next v6] Add Mellanox BlueField Gigabit Ethernet
 driver

> -----Original Message-----
> From: Leon Romanovsky <leon@...nel.org>
> Sent: Wednesday, June 2, 2021 12:39 AM
> To: David Thompson <davthompson@...dia.com>
> Cc: davem@...emloft.net; kuba@...nel.org; netdev@...r.kernel.org; Liming
> Sun <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.
> 

Sure, will adjust this in the next patch version.

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

Leon,

Thank you for pointing out these checkpatch warnings.
We'll address these in the next patch version.

- Dave

Powered by blists - more mailing lists