[PATCH v8 1/2] net: brcm: netXtreme driver

[Roman Bacik]

> -----Original Message-----
> From: Pali Rohár <pali at kernel.org>
> Sent: Sunday, November 7, 2021 1:44 AM
> To: Roman Bacik <roman.bacik at broadcom.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Bharat Gooty
> <bharat.gooty at broadcom.com>; Joe Hershberger
> <joe.hershberger at ni.com>; Ramon Fried <rfried.dev at gmail.com>
> Subject: Re: [PATCH v8 1/2] net: brcm: netXtreme driver
>
> Hello!
>
> When sending a new version of patch, try to put into CC reviewers of
> previous versions, so they can approve new version (if objections were
> fixed). Not everybody is following all emails on mailing list as there
> are lot of emails...

Ok, we will.

>
> On Friday 05 November 2021 15:36:51 Roman Bacik wrote:
> > --- /dev/null
> > +++ b/drivers/net/bnxt/bnxt.c
> > @@ -0,0 +1,1710 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2019-2021 Broadcom.
> > + */
> > +
> > +#include <common.h>
> > +
> > +#include <asm/io.h>
> > +#include <dm.h>
> > +#include <linux/delay.h>
> > +#include <memalign.h>
> > +#include <net.h>
> > +
> > +#include "bnxt.h"
> > +#include "bnxt_dbg.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)
> > +
> > +/* 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, PCI_INTERRUPT_LINE, &bp->irq);
> > +	bp->bar0 = dm_pci_map_bar(bp->pdev, PCI_BASE_ADDRESS_0,
> PCI_REGION_MEM);
> > +	bp->bar1 = dm_pci_map_bar(bp->pdev, PCI_BASE_ADDRESS_2,
> PCI_REGION_MEM);
> > +	bp->bar2 = dm_pci_map_bar(bp->pdev, PCI_BASE_ADDRESS_4,
> PCI_REGION_MEM);
> > +	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);
>
> I cannot find any pci_read_word16() function in the current U-Boot
> master repository. So this patch cannot be compiled.

It is defined in bnxt.h as dm_pci_read_config16. But we will remove
pci_read_/pci_write_/pci_map_ definitions from the header and replace.

>
> Could you check that you are developing this patch on top of the recent
> version of U-Boot git master branch?

We rebase, compile and test with the latest before posting the patch.

>
> Also you should use pci_dm_* functions as Driver Model API is preferred
> now and drivers are being converting to this new API, so old API can be
> later dropped.
>
> > +	dbg_pci(bp, __func__, cmd_reg);
> > +}

This is our method declared in bnxt_dbg.h.

> ...
> > diff --git a/drivers/net/bnxt/bnxt_ver.h b/drivers/net/bnxt/bnxt_ver.h
> > new file mode 100644
> > index 000000000000..fa84397338dd
> > --- /dev/null
> > +++ b/drivers/net/bnxt/bnxt_ver.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright 2019-2021 Broadcom.
> > + */
> > +
> > +#ifndef _BNXT_VER_H_
> > +#define _BNXT_VER_H_
> > +
> > +#ifndef BNXT_EXTRA_VER_H
> > +#define BNXT_EXTRA_VER_H
> > +#define DRV_MODULE_EXTRA_VER         "-216.1.182.0"
> > +#endif
> > +
> > +#define DRV_MODULE_NAME              "bnxt"
> > +#define DRV_MODULE_VERSION           "1.0.0" DRV_MODULE_EXTRA_VER
> > +#define UBOOT_MODULE_VER             "1.0.0"
> > +#define UBOOT_VERSION_MAJOR          1
> > +#define UBOOT_VERSION_MINOR          0
> > +#define UBOOT_VERSION_UPDATE         0
> > +#define DRV_MODULE_DESC              "Broadcom NetXtreme-C/E driver"
> > +
> > +#endif /* _BNXT_VER_H_ */
>
> It looks like that more macros from this file are completely unused.
> Could you re-check it? Macros which are unused and do not bring any
> value even for documentation purposes is dead code, which should be
> eliminated due to maintenance cost.
>
> For example for documentation purposes it could make sense to define
> unused macro which describe some existing HE register even when this
> macros is not used currently in the driver.
>
> But defining macro UBOOT_VERSION_* is suspicious as 1) U-Boot version is
> already defined in other global header file and 2) U-Boot version is not
> 1.0.0 anymore.

We will remove unused defines from the headers.
Thanks,

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.
-------------- 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/20211108/2515eb4c/attachment.bin>