RE: [PATCH v5 1/2] net: brcm: netXtreme driver
Hi Marek,
> -Original Message-
> From: Marek Behún
> Sent: Tuesday, November 2, 2021 5:07 AM
> To: Roman Bacik
> Cc: U-Boot Mailing List ; Bharat Gooty
> ; Joe Hershberger
> ; Ramon Fried
> Subject: Re: [PATCH v5 1/2] net: brcm: netXtreme driver
>
> On Mon, 1 Nov 2021 13:21:44 -0700
> Roman Bacik wrote:
>
> > +static int set_phy_speed(struct bnxt *bp)
> > +{
> > + char name[20];
> > + char name1[30];
> > + u16 flag = PHY_STATUS | PHY_SPEED | DETECT_MEDIA;
> > +
> > + /* Query Link Status */
> > + if (bnxt_hwrm_port_phy_qcfg(bp, flag) != STATUS_SUCCESS)
> > + return STATUS_FAILURE;
> > +
> > + switch (bp->current_link_speed) {
> > + case PORT_PHY_QCFG_RESP_LINK_SPEED_100GB:
> > + sprintf(name, "%s %s", str_100, str_gbps);
> > + break;
> > + case PORT_PHY_QCFG_RESP_LINK_SPEED_50GB:
> > + sprintf(name, "%s %s", str_50, str_gbps);
> > + break;
> > + case PORT_PHY_QCFG_RESP_LINK_SPEED_40GB:
> > + sprintf(name, "%s %s", str_40, str_gbps);
> > + break;
> > + case PORT_PHY_QCFG_RESP_LINK_SPEED_25GB:
> > + sprintf(name, "%s %s", str_25, str_gbps);
> > + break;
> > + case PORT_PHY_QCFG_RESP_LINK_SPEED_20GB:
> > + sprintf(name, "%s %s", str_20, str_gbps);
> > + break;
> > + case PORT_PHY_QCFG_RESP_LINK_SPEED_10GB:
> > + sprintf(name, "%s %s", str_10, str_gbps);
> > + break;
> > + case PORT_PHY_QCFG_RESP_LINK_SPEED_2_5GB:
> > + sprintf(name, "%s %s", str_2_5, str_gbps);
> > + break;
> > + case PORT_PHY_QCFG_RESP_LINK_SPEED_2GB:
> > + sprintf(name, "%s %s", str_2, str_gbps);
> > + break;
> > + case PORT_PHY_QCFG_RESP_LINK_SPEED_1GB:
> > + sprintf(name, "%s %s", str_1, str_gbps);
> > + break;
> > + case PORT_PHY_QCFG_RESP_LINK_SPEED_100MB:
> > + sprintf(name, "%s %s", str_100, str_mbps);
> > + break;
> > + case PORT_PHY_QCFG_RESP_LINK_SPEED_10MB:
> > + sprintf(name, "%s %s", str_10, str_mbps);
> > + break;
> > + default:
> > + sprintf(name, "%s %x", str_unknown, bp-
> >current_link_speed);
> > + }
> > +
> > + sprintf(name1, "bnxt_eth%u_phy_speed", bp->cardnum);
> > + env_set(name1, name);
> > + dbg_phy_speed(bp, name);
> > +
> > + return STATUS_SUCCESS;
> > +}
> > +
> > +static int set_phy_link(struct bnxt *bp, u32 tmo)
> > +{
> > + char name[32];
> > + int ret;
> > +
> > + set_phy_speed(bp);
> > + dbg_link_status(bp);
> > + if (bp->link_status == STATUS_LINK_ACTIVE) {
> > + dbg_link_state(bp, tmo);
> > + sprintf(name, "bnxt_eth%u_link", bp->cardnum);
> > + env_set(name, "up");
> > + sprintf(name, "bnxt_eth%u_media", bp->cardnum);
> > + env_set(name, "connected");
> > + ret = STATUS_SUCCESS;
> > + } else {
> > + sprintf(name, "bnxt_eth%u_link", bp->cardnum);
> > + env_set(name, "down");
> > + sprintf(name, "bnxt_eth%u_media", bp->cardnum);
> > + env_set(name, "disconnected");
> > + ret = STATUS_FAILURE;
> > + }
> > +
> > + return ret;
> > +}
>
> Hi Roman,
>
> your proposal still contains non-standard and unneeded setting of
> environment variables. An ethernet driver should never do this. In fact
> no driver besides board code or sysinfo driver should do this directly.
>
> There are other mechanisms for reporting PHY connection information in
> U-Boot, please use those if you need them (e.g. implement a PHY
> driver), but remove all env_set() calls from your ethernet driver.
>
> Rationale: historically, many times things were solved with ad-hoc code
> in U-Boot, which did this kind of thing and similar. It got out of hand
> pretty fast, and it was horrible. So some people dedided to fix it,
> proposing APIs, unifying code, deduplicating code and so on. This is
> still, in fact, going on. For your driver to have it's own mechanism
> for reporting link status, by setting env variables, is going against
> this whole work.
>
> I suggest for now just to remove these calls. When the driver is
> merged, w
Re: [PATCH v5 1/2] net: brcm: netXtreme driver
On Mon, 1 Nov 2021 13:21:44 -0700
Roman Bacik wrote:
> +static int set_phy_speed(struct bnxt *bp)
> +{
> + char name[20];
> + char name1[30];
> + u16 flag = PHY_STATUS | PHY_SPEED | DETECT_MEDIA;
> +
> + /* Query Link Status */
> + if (bnxt_hwrm_port_phy_qcfg(bp, flag) != STATUS_SUCCESS)
> + return STATUS_FAILURE;
> +
> + switch (bp->current_link_speed) {
> + case PORT_PHY_QCFG_RESP_LINK_SPEED_100GB:
> + sprintf(name, "%s %s", str_100, str_gbps);
> + break;
> + case PORT_PHY_QCFG_RESP_LINK_SPEED_50GB:
> + sprintf(name, "%s %s", str_50, str_gbps);
> + break;
> + case PORT_PHY_QCFG_RESP_LINK_SPEED_40GB:
> + sprintf(name, "%s %s", str_40, str_gbps);
> + break;
> + case PORT_PHY_QCFG_RESP_LINK_SPEED_25GB:
> + sprintf(name, "%s %s", str_25, str_gbps);
> + break;
> + case PORT_PHY_QCFG_RESP_LINK_SPEED_20GB:
> + sprintf(name, "%s %s", str_20, str_gbps);
> + break;
> + case PORT_PHY_QCFG_RESP_LINK_SPEED_10GB:
> + sprintf(name, "%s %s", str_10, str_gbps);
> + break;
> + case PORT_PHY_QCFG_RESP_LINK_SPEED_2_5GB:
> + sprintf(name, "%s %s", str_2_5, str_gbps);
> + break;
> + case PORT_PHY_QCFG_RESP_LINK_SPEED_2GB:
> + sprintf(name, "%s %s", str_2, str_gbps);
> + break;
> + case PORT_PHY_QCFG_RESP_LINK_SPEED_1GB:
> + sprintf(name, "%s %s", str_1, str_gbps);
> + break;
> + case PORT_PHY_QCFG_RESP_LINK_SPEED_100MB:
> + sprintf(name, "%s %s", str_100, str_mbps);
> + break;
> + case PORT_PHY_QCFG_RESP_LINK_SPEED_10MB:
> + sprintf(name, "%s %s", str_10, str_mbps);
> + break;
> + default:
> + sprintf(name, "%s %x", str_unknown, bp->current_link_speed);
> + }
> +
> + sprintf(name1, "bnxt_eth%u_phy_speed", bp->cardnum);
> + env_set(name1, name);
> + dbg_phy_speed(bp, name);
> +
> + return STATUS_SUCCESS;
> +}
> +
> +static int set_phy_link(struct bnxt *bp, u32 tmo)
> +{
> + char name[32];
> + int ret;
> +
> + set_phy_speed(bp);
> + dbg_link_status(bp);
> + if (bp->link_status == STATUS_LINK_ACTIVE) {
> + dbg_link_state(bp, tmo);
> + sprintf(name, "bnxt_eth%u_link", bp->cardnum);
> + env_set(name, "up");
> + sprintf(name, "bnxt_eth%u_media", bp->cardnum);
> + env_set(name, "connected");
> + ret = STATUS_SUCCESS;
> + } else {
> + sprintf(name, "bnxt_eth%u_link", bp->cardnum);
> + env_set(name, "down");
> + sprintf(name, "bnxt_eth%u_media", bp->cardnum);
> + env_set(name, "disconnected");
> + ret = STATUS_FAILURE;
> + }
> +
> + return ret;
> +}
Hi Roman,
your proposal still contains non-standard and unneeded setting of
environment variables. An ethernet driver should never do this. In fact
no driver besides board code or sysinfo driver should do this directly.
There are other mechanisms for reporting PHY connection information in
U-Boot, please use those if you need them (e.g. implement a PHY
driver), but remove all env_set() calls from your ethernet driver.
Rationale: historically, many times things were solved with ad-hoc code
in U-Boot, which did this kind of thing and similar. It got out of hand
pretty fast, and it was horrible. So some people dedided to fix it,
proposing APIs, unifying code, deduplicating code and so on. This is
still, in fact, going on. For your driver to have it's own mechanism
for reporting link status, by setting env variables, is going against
this whole work.
I suggest for now just to remove these calls. When the driver is
merged, we can work on extending support for passing link information
to U-Boot via ethernet PHY API, if you need it.
Marek
[PATCH v5 1/2] net: brcm: netXtreme driver
From: Bharat Gooty
Broadcom bnxt L2 driver support. Used by the Broadcom
iproc platforms.
Signed-off-by: Bharat Gooty
Reviewed-by: Ramon Fried
Signed-off-by: Roman Bacik
---
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 | 1748 +++
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 +
10 files changed, 3622 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 ..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 ..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 ..fe0f0833cd99
--- /dev/null
+++ b/drivers/net/bnxt/bnxt.c
@@ -0,0 +1,1748 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2019-2021 Broadcom.
+ */
+
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#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 ",";
+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);
+ 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);
+ pci_read_byte(bp->pdev, PCI_INTERRUPT_LINE, &bp->irq);
+ bp->bar0 = pci_map_bar(bp
