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

[Roman Bacik]

On Fri, Nov 5, 2021 at 3:04 PM Pali Rohár <pali at kernel.org> wrote:
>
> On Friday 05 November 2021 22:09:47 Pali Rohár wrote:
> > On Friday 05 November 2021 12:54:24 Roman Bacik wrote:
> > > > > +       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() reads from PCI(e) config space, which is standardized.
> > Therefore only standard macro constants from include/pci.h should be
> > used. Standard PCI config header is 64 byte long and after that is
> > linked list of capabilities. Order of capabilities is not defined.
> > Extended capabilities from linked list should be located by macro
> > constants PCI_CAP_ID_*.
> >
> > So above register is part of some extended capability. Correctly it
> > should be used some function to locate starting offset of that extended
> > capability based on PCI_CAP_ID_* (see pci.h file for these functions)
> > and then access that register as offset + PCI_* constant (which defined
> > as relative to the start of extended capability). In case standard macro
> > for this constant in pci.h is missing, it is a good idea to define it,
> > or copy it from linux header file pci_regs.h (to have consistent naming
> > of macros).
>
> Just one example how to read PCIe Link Control Register (to make it
> clear what I mean):
>
> int ret;
> u16 lnkctl;
> int pci_exp_off;
> pci_exp_off = dm_pci_find_capability(dev, PCI_CAP_ID_EXP);
> if (!pci_exp_off) return -ENOENT;
> ret = dm_pci_read_config16(dev, pci_exp_off + PCI_EXP_LNKCTL, &lnkctl);
> if (ret) return ret;

The driver should work for 0x14e4:0x16F0 and we usually test it with
this one. We will use your recommended method if needed. We will try
to remove it in v8, since I do not see bp->pf_num being used.

Thank you very much,

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.