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@broadcom.com

Broadcom bnxt L2 driver support. Used by the Broadcom iproc platforms.

Signed-off-by: Bharat Gooty bharat.gooty@broadcom.com Reviewed-by: Ramon Fried rfried.dev@gmail.com

Signed-off-by: Roman Bacik roman.bacik@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?

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.

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

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

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

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

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

• 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().

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

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

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

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

+#endif /* _PCI_IDS_H_ */