Hello! See inline comments below.
On Tuesday 02 November 2021 11:18:10 Roman Bacik wrote:
> From: Bharat Gooty <bharat.go...@broadcom.com>
>
> Broadcom bnxt L2 driver support. Used by the Broadcom
> iproc platforms.
>
> Signed-off-by: Bharat Gooty <bharat.go...@broadcom.com>
> Reviewed-by: Ramon Fried <rfried....@gmail.com>
>
> Signed-off-by: Roman Bacik <roman.ba...@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_ */
