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

5 Nov 2021

Hi Pali,
...
-----Original Message-----
From: Pali Rohár pali@kernel.org
Sent: Wednesday, November 3, 2021 4:14 PM
To: Roman Bacik roman.bacik@broadcom.com
Cc: U-Boot Mailing List u-boot@lists.denx.de; Bharat Gooty
bharat.gooty@broadcom.com; Joe Hershberger
joe.hershberger@ni.com; Ramon Fried rfried.dev@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@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?
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.

    