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

[Pali Rohár]

On Monday 08 November 2021 09:31:14 Roman Bacik wrote:
> > -----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.

Ou, sorry for that. I have not caught this.

But it is not a good idea to define custom driver functions with pci_
prefix, moreover exported in header file. It could confuse also other
people who would thing that it is some global API function... Also it
could cause issues if into pci.h will be added new API function with
same name as you used in driver.

As I was not able to find it in pci.h file and I found more other
pci_read and pci_write functions, I thought that there needs to be some
typo or usage of older U-Boot version (as I remember introduction of a
new DM API).

Prefixing your custom driver functions and macros with bnxt_ makes it
clear that function is driver specific and would not cause any namespace
conflict issues.

> >
> > 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.