[PATCH v6 1/2] net: brcm: netXtreme driver

Fri Nov 5 20:54:24 CET 2021

Hi Pali,

> -----Original Message-----
> From: Pali Rohár <pali at kernel.org>
> Sent: Wednesday, November 3, 2021 4:14 PM
> To: Roman Bacik <roman.bacik at broadcom.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Bharat Gooty
> <bharat.gooty at broadcom.com>; Joe Hershberger
> <joe.hershberger at ni.com>; Ramon Fried <rfried.dev at gmail.com>
> Subject: Re: [PATCH v6 1/2] net: brcm: netXtreme driver
>
> Hello! See inline comments below.
>
> On Tuesday 02 November 2021 11:18:10 Roman Bacik wrote:
> > From: Bharat Gooty <bharat.gooty at broadcom.com>
> >
> > Broadcom bnxt L2 driver support. Used by the Broadcom
> > iproc platforms.
> >
> > Signed-off-by: Bharat Gooty <bharat.gooty at broadcom.com>
> > Reviewed-by: Ramon Fried <rfried.dev at gmail.com>
> >
> > Signed-off-by: Roman Bacik <roman.bacik at broadcom.com>
> > ---
> >
> > Changes in v6:
> > - remove bnxt_eth_* env variables
> > - clean up include headers
> >
> > Changes in v5:
> > - remove bnxt_env_set_ethaddr/bnxt_env_del_ethaddr methods
> > - add .read_rom_hwaddr = bnxt_read_rom_hwaddr
> > - move bnxt_bring_pci/bnxt_bring_chip to bnxt_read_rom_hwddr
> > - move mac copy from priv to plat to bnxt_read_rom_hwaddr
> >
> > Changes in v4:
> > - remove static num_cards and use dev_seq(dev) instead
> > - add .probe
> > - merged probe/remove methods
> > - select PCI_INIT_R when BNXT_ETH is selected
> >
> > Changes in v3:
> > - change printf to debug in display_banner
> > - remove get/set/print mac/speed
> > - remove bnxt_hwrm_set_nvmem
> >
> > Changes in v2:
> > - rebase and remove DM_PCI dependency from BNXT_ETH
> > - remove tautology assignment from debug_resp()
> >
> >  drivers/net/Kconfig         |    1 +
> >  drivers/net/Makefile        |    1 +
> >  drivers/net/bnxt/Kconfig    |    7 +
> >  drivers/net/bnxt/Makefile   |    5 +
> >  drivers/net/bnxt/bnxt.c     | 1727
> +++++++++++++++++++++++++++++++++++
> >  drivers/net/bnxt/bnxt_dbg.h |  537 +++++++++++
> >  drivers/net/bnxt/pci_ids.h  |   17 +
> >  include/broadcom/bnxt.h     |  395 ++++++++
> >  include/broadcom/bnxt_hsi.h |  889 ++++++++++++++++++
> >  include/broadcom/bnxt_ver.h |   22 +
>
> These 3 include files looks like that contain only private definitions
> for bnxt driver. So should not they be in the drivers/net/bnxt
directory?

We will move these files to drivers/net/bnxt/.

>
> >  10 files changed, 3601 insertions(+)
> >  create mode 100644 drivers/net/bnxt/Kconfig
> >  create mode 100644 drivers/net/bnxt/Makefile
> >  create mode 100644 drivers/net/bnxt/bnxt.c
> >  create mode 100644 drivers/net/bnxt/bnxt_dbg.h
> >  create mode 100644 drivers/net/bnxt/pci_ids.h
> >  create mode 100644 include/broadcom/bnxt.h
> >  create mode 100644 include/broadcom/bnxt_hsi.h
> >  create mode 100644 include/broadcom/bnxt_ver.h
> >
> > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > index 6c12959f3794..8dc81a3d6cf9 100644
> > --- a/drivers/net/Kconfig
> > +++ b/drivers/net/Kconfig
> > @@ -1,6 +1,7 @@
> >  source "drivers/net/phy/Kconfig"
> >  source "drivers/net/pfe_eth/Kconfig"
> >  source "drivers/net/fsl-mc/Kconfig"
> > +source "drivers/net/bnxt/Kconfig"
> >
> >  config ETH
> >  	def_bool y
> > diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> > index e4078d15a99f..1d9fbd6693cc 100644
> > --- a/drivers/net/Makefile
> > +++ b/drivers/net/Makefile
> > @@ -101,3 +101,4 @@ obj-$(CONFIG_HIGMACV300_ETH) += higmacv300.o
> >  obj-$(CONFIG_MDIO_SANDBOX) += mdio_sandbox.o
> >  obj-$(CONFIG_FSL_ENETC) += fsl_enetc.o fsl_enetc_mdio.o
> >  obj-$(CONFIG_FSL_LS_MDIO) += fsl_ls_mdio.o
> > +obj-$(CONFIG_BNXT_ETH) += bnxt/
> > diff --git a/drivers/net/bnxt/Kconfig b/drivers/net/bnxt/Kconfig
> > new file mode 100644
> > index 000000000000..412ecd430335
> > --- /dev/null
> > +++ b/drivers/net/bnxt/Kconfig
> > @@ -0,0 +1,7 @@
> > +config BNXT_ETH
> > +	bool "BNXT PCI support"
> > +	depends on DM_ETH
> > +	select PCI_INIT_R
> > +	help
> > +	  This driver implements support for bnxt pci controller
> > +	  driver of ethernet class.
> > diff --git a/drivers/net/bnxt/Makefile b/drivers/net/bnxt/Makefile
> > new file mode 100644
> > index 000000000000..a9d6ce00d5e0
> > --- /dev/null
> > +++ b/drivers/net/bnxt/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> > +# Copyright 2019-2021 Broadcom.
> > +
> > +# Broadcom nxe Ethernet driver
> > +obj-y += bnxt.o
> > diff --git a/drivers/net/bnxt/bnxt.c b/drivers/net/bnxt/bnxt.c
> > new file mode 100644
> > index 000000000000..60a65b20a8f1
> > --- /dev/null
> > +++ b/drivers/net/bnxt/bnxt.c
> > @@ -0,0 +1,1727 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2019-2021 Broadcom.
> > + */
> > +
> > +#include <common.h>
> > +
> > +#include <asm/io.h>
> > +#include <broadcom/bnxt.h>
> > +#include <dm.h>
> > +#include <linux/delay.h>
> > +#include <memalign.h>
> > +#include <net.h>
> > +
> > +#include "bnxt_dbg.h"
> > +#include "pci_ids.h"
> > +
> > +#define bnxt_down_chip(bp)     bnxt_hwrm_run(down_chip, bp, 0)
> > +#define bnxt_bring_chip(bp)    bnxt_hwrm_run(bring_chip, bp, 1)
> > +
> > +static const char banner[]  = DRV_MODULE_DESC " v"
> UBOOT_MODULE_VER ",";
>
> You do not need banner with custom version number. U-Boot version is
> printed automatically and specify also which version of driver includes.

We will remove display_banner().

>
> > +static const char fw_ver[]  = " FW v";
> > +
> > +static void display_banner(struct bnxt *bp)
> > +{
> > +	int i;
> > +
> > +	debug(banner);
> > +	debug(fw_ver);
> > +	debug("%d.%d.", bp->fw_maj, bp->fw_min);
> > +	debug("%d.%d\n", bp->fw_bld, bp->fw_rsvd);
> > +	debug("ETH MAC: ");
> > +	for (i = 0; i < ETH_ALEN; i++) {
> > +		debug("%02x", bp->mac_set[i]);
> > +		if (i != (ETH_ALEN - 1))
> > +			debug(":");
> > +	}
> > +
> > +	debug(", Port(%d), PF(%d)\n", bp->port_idx, bp->ordinal_value);
> > +}
> > +
> > +/* Broadcom ethernet driver PCI APIs. */
> > +static void bnxt_bring_pci(struct bnxt *bp)
> > +{
> > +	u16 cmd_reg = 0;
> > +
> > +	pci_read_word16(bp->pdev, PCI_VENDOR_ID, &bp->vendor_id);
> > +	pci_read_word16(bp->pdev, PCI_DEVICE_ID, &bp->device_id);
>
> Do you really need to read device and vendor ids? Driver already knows
> them as it finds to device based on ids.

We find this helpful when debugging.

>
> > +	pci_read_word16(bp->pdev,
> > +			PCI_SUBSYSTEM_VENDOR_ID,
> > +			&bp->subsystem_vendor);
> > +	pci_read_word16(bp->pdev, PCI_SUBSYSTEM_ID, &bp-
> >subsystem_device);
> > +	pci_read_word16(bp->pdev, PCI_COMMAND, &bp->cmd_reg);
> > +	pci_read_byte(bp->pdev, PCICFG_ME_REGISTER, &bp->pf_num);
>
> PCICFG_ME_REGISTER looks like an error as there is no such PCI config
> space macro. What you are trying to read into pf_num? Currently I do not
> know what "pf" abbreviation could mean.

PF stands for physical function and pf_num is the number of physical
functions configured.
The macro is defined in bnxt.h:
#define PCICFG_ME_REGISTER                      0x98

>
> > +	pci_read_byte(bp->pdev, PCI_INTERRUPT_LINE, &bp->irq);
> > +	bp->bar0 = pci_map_bar(bp->pdev, PCI_BASE_ADDRESS_0,
> PCI_REGION_MEM);
> > +	bp->bar1 = pci_map_bar(bp->pdev, PCI_BASE_ADDRESS_2,
> PCI_REGION_MEM);
> > +	bp->bar2 = pci_map_bar(bp->pdev, PCI_BASE_ADDRESS_4,
> PCI_REGION_MEM);
>
> pci_map_bar() is obsolete and should not be used in a new code. Please
> switch to DM and use new DM API.

We will replace with dm_pci_map_bar().

>
> Check that you can compile driver without CONFIG_DM_PCI_COMPAT
> option.
> I think it should throw compile error on usage of this function.

We do not enable CONFIG_DM_PCI_COMPAT.

>
> > +	cmd_reg = bp->cmd_reg | PCI_COMMAND_MEMORY |
> PCI_COMMAND_MASTER;
> > +	cmd_reg |= PCI_COMMAND_INTX_DISABLE; /* disable intr */
> > +	pci_write_word(bp->pdev, PCI_COMMAND, cmd_reg);
> > +	pci_read_word16(bp->pdev, PCI_COMMAND, &cmd_reg);
> > +	dbg_pci(bp, __func__, cmd_reg);
> > +}
> ...
> > +static int bnxt_read_rom_hwaddr(struct udevice *dev)
> > +{
> > +	struct eth_pdata *plat = dev_get_plat(dev);
> > +	struct bnxt *bp = dev_get_priv(dev);
> > +
> > +	bnxt_bring_pci(bp);
> > +
> > +	if (bnxt_bring_chip(bp))
>
> It looks suspicious if read_rom_hwaddr() function is doing
> initialization of PCIe device. Is there any reason for it in this place?
>
> Opposite of bnxt_bring_pci+bnxt_bring_chip is bnxt_down_chip and it is
> called in bnxt_eth_remove() function.

The method bnxt_bring_pci() fills priv data bp and needs to be called
before bnxt_bring_chip(). The data does not need to be cleaned in
bnxt_eth_remove().
We were asked by another reviewer to use bnxt_read_rom_hwaddr() to set up
hw address (instead of doing it in probe) but bnxt_bring_chip() has to be
called to get the hw addr. So both methods bnxt_bring_pci and
bnxt_bring_chip were moved there.

>
> > +		printf("*** warning: bnxt_bring_chip failed! ***\n");
>
> It is only warning? I guess that failed initialization is fatal error
> and should be propagated via return value... but that is not easily
> possible if initialization is called from read_rom_hwaddr().

It is only warning, since there is usually another 1G eth available,
besides this 100G bnxt eth. Originally, we had it probed via bnxt commands
on demand. But another reviewer asked to remove bnxt commands so we bring
up bnxt each boot. But it is not necessarily an error causing the boot to
fail.

>
> > +	else
> > +		memcpy(plat->enetaddr, bp->mac_set, ETH_ALEN);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct eth_ops bnxt_eth_ops = {
> > +	.start           = bnxt_start,
> > +	.send            = bnxt_send,
> > +	.recv            = bnxt_recv,
> > +	.stop            = bnxt_stop,
> > +	.free_pkt        = bnxt_free_pkt,
> > +	.read_rom_hwaddr = bnxt_read_rom_hwaddr,
> > +};
> > +
> > +static const struct udevice_id bnxt_eth_ids[] = {
> > +	{ .compatible = "broadcom,nxe" },
> > +	{ }
> > +};
> > +
> > +static int bnxt_eth_bind(struct udevice *dev)
> > +{
> > +	char name[20];
> > +
> > +	sprintf(name, "bnxt_eth%u", dev_seq(dev));
> > +
> > +	return device_set_name(dev, name);
> > +}
> > +
> > +static int bnxt_eth_probe(struct udevice *dev)
> > +{
> > +	struct bnxt *bp = dev_get_priv(dev);
> > +	int err;
> > +
> > +	bp->name = dev->name;
> > +	bp->pdev = (struct udevice *)dev;
> > +	bp->cardnum = dev_seq(dev);
> > +	err = bnxt_alloc_mem(bp);
> > +	if (err)
> > +		return err;
> > +
> > +	display_banner(bp);
> > +	dev->flags_ = DM_FLAG_ACTIVATED;
>
> include/dm/device.h says:
>
>  * @flags_: Flags for this device DM_FLAG_... (do not access outside
driver
>  *	model)
>
> Really, you should not touch flags_ member in driver code.

We will remove the line with DM_FLAG_ACTIVATED.

>
> > +	return 0;
> > +}
> > +
> > +static int bnxt_eth_remove(struct udevice *dev)
> > +{
> > +	struct bnxt *bp = dev_get_priv(dev);
> > +
> > +	bnxt_down_chip(bp);  /* Down chip */
> > +	dev->flags_ &= ~DM_FLAG_ACTIVATED;
>
> Here too.

We will remove the line with ~DM_FLAG_ACTIVATED.

>
> > +	bnxt_free_mem(bp);
> > +
> > +	return 0;
> > +}
> > +
> > +U_BOOT_DRIVER(eth_bnxt) = {
> > +	.name                     = "eth_bnxt",
> > +	.id                       = UCLASS_ETH,
> > +	.of_match                 = bnxt_eth_ids,
> > +	.bind                     = bnxt_eth_bind,
> > +	.probe                    = bnxt_eth_probe,
> > +	.remove                   = bnxt_eth_remove,
> > +	.ops                      = &bnxt_eth_ops,
> > +	.priv_auto                = sizeof(struct bnxt),
> > +	.plat_auto                = sizeof(struct eth_pdata),
> > +	.flags                    = DM_FLAG_ACTIVE_DMA,
> > +};
> > +
> > +U_BOOT_PCI_DEVICE(eth_bnxt, bnxt_nics);
> ...
> > diff --git a/drivers/net/bnxt/pci_ids.h b/drivers/net/bnxt/pci_ids.h
> > new file mode 100644
> > index 000000000000..e9312be5acb4
> > --- /dev/null
> > +++ b/drivers/net/bnxt/pci_ids.h
> > @@ -0,0 +1,17 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright 2019-2021 Broadcom.
> > + */
> > +
> > +#ifndef _PCI_IDS_H_
> > +#define _PCI_IDS_H_
> > +
> > +#define PCI_VID_BROADCOM            0x14e4
> > +#define PCI_DID_NXT_57320_1         0x16F0
>
> These definitions should go into the include/pci_ids.h file.

We will add them to include/pci_ids.h.

>
> > +
> > +static struct pci_device_id bnxt_nics[] = {
> > +	{PCI_DEVICE(PCI_VID_BROADCOM, PCI_DID_NXT_57320_1)},
> > +	{}
> > +};
>
> bnxt_nics[] array is used only in bnxt.c. You can put PCI ids directly
> to that file. Defining + declaring variables in include file is strange
> as if you include such header file two or more times then you either get
> linker error (for non-static symbols) or you get multiple private
> declaration of one variable. Which is not probably intended behavior.
>

We will add bnxt_nics[] to bnxt.c.

> > +
> > +#endif /* _PCI_IDS_H_ */

Thank you very much for your review,

Roman

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4206 bytes
Desc: S/MIME Cryptographic Signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20211105/464e6dbb/attachment.bin>