[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
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