[PATCH v2 2/2] cmd: brcm: netXtreme commands
Roman Bacik
roman.bacik at broadcom.com
Mon Oct 25 18:54:37 CEST 2021
> -----Original Message-----
> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
> Sent: Saturday, October 23, 2021 1:02 AM
> To: Roman Bacik <roman.bacik at broadcom.com>; U-Boot Mailing List <u-
> boot at lists.denx.de>
> Cc: Bharat Gooty <bharat.gooty at broadcom.com>; Bin Meng
> <bmeng.cn at gmail.com>; Franck LENORMAND
> <franck.lenormand at nxp.com>; Kory Maincent
> <kory.maincent at bootlin.com>; Michal Simek <michal.simek at xilinx.com>;
> Patrick Delaunay <patrick.delaunay at foss.st.com>; Peng Fan
> <peng.fan at nxp.com>; Priyanka Jain <priyanka.jain at nxp.com>; Rayagonda
> Kokatanur <rayagonda.kokatanur at broadcom.com>; Sean Anderson
> <sean.anderson at seco.com>; Simon Glass <sjg at chromium.org>
> Subject: Re: [PATCH v2 2/2] cmd: brcm: netXtreme commands
>
>
>
> On 10/23/21 01:23, Roman Bacik wrote:
> > From: Bharat Gooty <bharat.gooty at broadcom.com>
> >
> > Following netXtreme commands are supported:-
> > Device probe, remove, supported speeds, get/set speeds and
> > get/set MAC address.
> >
> > Signed-off-by: Bharat Gooty <bharat.gooty at broadcom.com>
> >
> > Signed-off-by: Roman Bacik <roman.bacik at broadcom.com>
>
> Please, add a man-page for the new command in doc/usage/.
> Here is an example: doc/usage/loady.rst
> Add the new man-page to doc/usage/index.rst
> Test building with 'make htmldocs'.
>
> > ---
> >
> > (no changes since v1)
> >
> > cmd/Kconfig | 2 +
> > cmd/broadcom/Kconfig | 10 ++
> > cmd/broadcom/Makefile | 3 +-
> > cmd/broadcom/bnxt.c | 237
> ++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 251 insertions(+), 1 deletion(-)
> > create mode 100644 cmd/broadcom/Kconfig
> > create mode 100644 cmd/broadcom/bnxt.c
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 5b30b13e438f..e054292dbcd0 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -1953,6 +1953,8 @@ endmenu
> >
> > source "cmd/ti/Kconfig"
> >
> > +source "cmd/broadcom/Kconfig"
> > +
> > config CMD_BOOTSTAGE
> > bool "Enable the 'bootstage' command"
> > depends on BOOTSTAGE
> > diff --git a/cmd/broadcom/Kconfig b/cmd/broadcom/Kconfig
> > new file mode 100644
> > index 000000000000..6f16b09d1425
> > --- /dev/null
> > +++ b/cmd/broadcom/Kconfig
> > @@ -0,0 +1,10 @@
> > +menu "Broadcom specific command line interface"
> > +
> > +config BNXT_ETH_CMD
> > + bool "BNXT commands"
> > + depends on BNXT_ETH
> > + help
> > + Broadcom NXS ethernet controller commands. Commands
> supported are:-
> > + Driver probe, Driver remove, Supported speeds, get/set MAC
> address and get/set Link speeds.
> > +
> > +endmenu
> > diff --git a/cmd/broadcom/Makefile b/cmd/broadcom/Makefile
> > index 62268d98d0dd..0027c1c15e5a 100644
> > --- a/cmd/broadcom/Makefile
> > +++ b/cmd/broadcom/Makefile
> > @@ -1,6 +1,7 @@
> > # SPDX-License-Identifier: GPL-2.0+
> > -# Copyright 2020 Broadcom
> > +# Copyright 2020-2021 Broadcom
> >
> > obj-y += chimp_boot.o
> > obj-y += nitro_image_load.o
> > obj-y += chimp_handshake.o
> > +obj-$(CONFIG_BNXT_ETH_CMD) += bnxt.o
> > diff --git a/cmd/broadcom/bnxt.c b/cmd/broadcom/bnxt.c
> > new file mode 100644
> > index 000000000000..b9d1e59a74fb
> > --- /dev/null
> > +++ b/cmd/broadcom/bnxt.c
> > @@ -0,0 +1,237 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2021 Broadcom
> > + */
> > +
> > +#include <common.h>
> > +#include <command.h>
> > +
> > +#include <asm/io.h>
> > +#include <pci.h>
> > +#include <broadcom/bnxt.h>
> > +#include <broadcom/bnxt_ver.h>
> > +#include <broadcom/bnxt_hsi.h>
> > +#include <dm.h>
> > +#include <env_flags.h>
> > +#include <env_internal.h>
> > +#include <errno.h>
> > +#include <linux/if_ether.h>
> > +#include <net.h>
> > +#include <dm/device-internal.h>
> > +#include <linux/ctype.h>
> > +#include <linux/delay.h>
> > +#include <linux/types.h>
> > +
> > +static int do_bnxt_set_link(struct bnxt *bp, char *link_str)
> > +{
> > + bp->link_set = simple_strtoul(link_str, NULL, 16);
> > +
> > + switch (bp->link_set) {
> > + case LINK_SPEED_DRV_AUTONEG:
> > + printf("- AutoNeg Not Supported\n");
> > + return 0;
>
> Please, remove the leading '- '. It just increases code size.
> In case of an error, please, return CMD_RET_FAILURE.
> Please, remove captitalization of 'Not Supported'
>
> > + case LINK_SPEED_DRV_1G:
> > + if (!(bp->support_speeds &
> PORT_QCFG_SUPPORT_SPEEDS_1GB)) {
> > + printf("- 1 GBPS: Link Speed is not supported\n");
>
> ditto
>
> > + return 0;
> > + }
> > +
> > + break;
> > + case LINK_SPEED_DRV_10G:
> > + if (!(bp->support_speeds &
> PORT_QCFG_SUPPORT_SPEEDS_10GB)) {
> > + printf("- 10 GBPS: Link Speed is not supported\n");
> > + return 0;
>
> ditto
>
>
> > + }
> > +
> > + break;
> > + case LINK_SPEED_DRV_25G:
> > + if (!(bp->support_speeds &
> PORT_QCFG_SUPPORT_SPEEDS_25GB)) {
> > + printf("- 25 GBPS: Link Speed is not supported\n");
> > + return 0;
>
> ditto
>
> > + }
> > +
> > + break;
> > + case LINK_SPEED_DRV_40G:
> > + printf("- 40 GBPS Not Supported\n");
> > + return 0;
>
> ditto
>
> > + case LINK_SPEED_DRV_50G:
> > + if (!(bp->support_speeds &
> PORT_QCFG_SUPPORT_SPEEDS_50GB)) {
> > + printf("- 50 GBPS: Link Speed is not supported\n");
> > + return 0;
>
> ditto
>
> > + }
> > +
> > + break;
> > + case LINK_SPEED_DRV_100G:
> > + if (!(bp->support_speeds &
> PORT_QCFG_SUPPORT_SPEEDS_100GB)) {
> > + printf("- 100 GBPS: Link Speed is not supported\n");
> > + return 0;
>
> ditto
>
> > + }
> > +
> > + break;
> > + case LINK_SPEED_DRV_200G:
> > + if (!(bp->support_speeds &
> PORT_QCFG_SUPPORT_SPEEDS_200GB)) {
> > + printf("- 200 GBPS: Link Speed is not supported\n");
> > + return 0;
>
> ditto
>
> > + }
> > +
> > + break;
> > + case LINK_SPEED_DRV_2_5G:
> > + if (!(bp->support_speeds &
> PORT_QCFG_SUPPORT_SPEEDS_2_5GB)) {
> > + printf("- 2.5 GBPS: Link Speed is not supported\n");
>
> ditto
>
> > + return 0;
> > + }
> > +
> > + break;
> > + case LINK_SPEED_DRV_100M:
> > + if (!(bp->support_speeds &
> PORT_QCFG_SUPPORT_SPEEDS_100MB)) {
> > + printf("- 100 MBPS: Link Speed is not supported\n");
> > + return 0;
>
> ditto
>
> > + }
> > +
> > + break;
> > + default:
> > + printf("- Invalid Link Speed specified\n");
> > + return CMD_RET_USAGE;
> > + }
> > +
> > + prn_link_speed(bp->link_set, 1);
> > +
> > + return bnxt_set_link_speed(bp);
> > +}
> > +
> > +static int (struct bnxt *bp, char *mac_str)
> > +{
> > + struct eth_pdata *plat = dev_get_plat(bp->pdev);
> > + u8 addr[ETH_ALEN];
> > + int ret = CMD_RET_USAGE;
> > +
> > + if (eth_validate_ethaddr_str(mac_str)) {
> > + printf("Invalid MAC Address %s\n", mac_str);
> > + return 0;
>
> return CMD_RET_FAILURE
>
> %s/Invalid MAC Address/Invalid MAC address/
>
>
> > + }
> > +
> > + string_to_enetaddr(mac_str, &addr[0]);
> > +
> > + if (!is_valid_ethaddr(&addr[0])) {
> > + printf("- Warning: Invalid MAC address to set\n");
>
> Please, be consistent.
>
> %s/- Warning: //
>
> > + return 0;
>
> return CMD_RET_FAILURE
>
> > + }
> > +
> > + memcpy(bp->mac_set, addr, ETH_ALEN);
> > +
> > + print_mac(&bp->mac_set[0], 1);
> > + ret = bnxt_set_mac(bp);
> > + if (ret)
> > + return ret;
> > +
> > + bnxt_hwrm_nvm_flush(bp);
> > +
> > + /* copy ROM MAC to environment. */
> > + memcpy(plat->enetaddr, bp->mac_set, ETH_ALEN);
> > + bnxt_env_set_ethaddr(bp->pdev);
> > +
> > + return CMD_RET_SUCCESS;
> > +}
> > +
> > +static int print_drv(u8 flag)
> > +{
> > + printf("bnxt - Device ");
> > + if (flag)
> > + printf("already");
> > + else
> > + printf("not");
> > +
> > + printf(" initialized\n");
> > +
> > + return CMD_RET_SUCCESS;
> > +}
> > +
> > +static int bnxt_parse_input(int argc, char *const argv[])
> > +{
> > + if (!strcmp(argv[2], "probe")) {
> > + return CMD_PROBE;
> > + } else if (!strcmp(argv[2], "remove")) {
> > + return CMD_REMOVE;
> > + } else if (!strcmp(argv[2], "get")) {
> > + if (!strncmp(argv[3], "supported_speed", 16))
> > + return CMD_GET_SUPPORTED_SPEED;
> > + else if (!strncmp(argv[3], "link_speed", 11))
> > + return CMD_GET_LINK_SPEED;
> > + else if (!strncmp(argv[3], "mac", 4))
> > + return CMD_GET_MAC;
> > + }
> > +
> > + return CMD_UNKNOWN;
> > +}
> > +
> > +static int do_bnxt(struct cmd_tbl *cmdtp, int flag, int argc, char
> > *const
> argv[])
> > +{
>
> Please, implement the subcommands as described in
> doc/develop/commands.rst.
> https://u-boot.readthedocs.io/en/latest/develop/commands.html
> This avoids reinventing the wheel and reduces code size.
>
> > + struct udevice *dev;
> > + struct bnxt *bp;
> > + char name[30];
> > + int ret = CMD_RET_USAGE;
> > + int cardnum;
> > + int op;
> > +
> > + printf("\n");
> > + if (argc < 2)
> > + return ret;
> > +
> > + cardnum = simple_strtoul(argv[1], NULL, 10);
> > + sprintf(name, "bnxt_eth%u", cardnum);
> > + ret = uclass_get_device_by_name(UCLASS_ETH, name, &dev);
> > + if (ret)
> > + return CMD_RET_USAGE;
> > +
> > + bp = dev_get_priv(dev);
> > + op = bnxt_parse_input(argc, argv);
> > + ret = CMD_RET_USAGE;
> > + switch (op) {
> > + case CMD_PROBE:
> > + if (!bp->drv_load)
> > + ret = bnxt_drv_probe(dev);
> > + else
> > + ret = print_drv(1);
> > + break;
> > + case CMD_REMOVE:
> > + ret = bnxt_drv_remove(dev);
> > + break;
> > + case CMD_GET_SUPPORTED_SPEED:
> > + ret = bnxt_supported_speed(bp);
> > + break;
> > + case CMD_GET_MAC:
> > + ret = bnxt_get_mac(bp);
> > + break;
> > + case CMD_GET_LINK_SPEED:
> > + ret = bnxt_get_link_speed(bp);
> > + break;
> > + case CMD_SET_MAC:
> > + ret = do_bnxt_set_mac(bp, argv[4]);
> > + break;
> > + case CMD_SET_LINK_SPEED:
> > + ret = do_bnxt_set_link(bp, argv[4]);
> > + break;
> > + default:
> > + if (op && !bp->drv_load)
> > + ret = print_drv(0);
> > + }
> > +
> > + printf("\n");
> > +
> > + return ret;
> > +}
> > +
> > +U_BOOT_CMD
> > + (bnxt, 5, 1, do_bnxt,
> > + "Broadcom NetXtreme-C/E Management",
> > + /* */"<bnxt_eth#> probe\n"
> > + " - Load Driver Instance.\n"
>
> Please, remove capitalization and period at end of enumberation lines.
> Saves us a few bytes.
>
> Best regards
>
> Heinrich
Heinrich,
We will address your comments in the next version.
Thank you very much for your review,
Roman
>
> > + "bnxt <bnxt_eth#> remove\n"
> > + " - Unload Driver Instance.\n"
> > + "bnxt <bnxt_eth#> get supported_speed\n"
> > + " - Get Supported Link Speeds.\n"
> > + "bnxt <bnxt_eth#> get link_speed\n"
> > + " - Get Current Link Speed.\n"
> > + "bnxt <bnxt_eth#> get mac\n"
> > + " - Get MAC Address.\n"
> > +);
> >
--
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/20211025/0c57e5c4/attachment.bin>
More information about the U-Boot
mailing list