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

[Pali Rohár]

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

Could you provide output of 'lspci -nn -vv' from linux for this card?
Or 'pci display.b ?.?.? 0 0x1000' dump from U-Boot?
This could help me to under what kind of register that 0x98 is.

I can write this part of code, no problem, just I need to see layout of
config space of that card.

> >
> > > +	pci_read_byte(bp->pdev, PCI_INTERRUPT_LINE, &bp->irq);
> > > +	bp->bar0 = pci_map_bar(bp->pdev, PCI_BASE_ADDRESS_0,
> > PCI_REGION_MEM);
> > > +	bp->bar1 = pci_map_bar(bp->pdev, PCI_BASE_ADDRESS_2,
> > PCI_REGION_MEM);
> > > +	bp->bar2 = pci_map_bar(bp->pdev, PCI_BASE_ADDRESS_4,
> > PCI_REGION_MEM);
> >
> > pci_map_bar() is obsolete and should not be used in a new code. Please
> > switch to DM and use new DM API.
> 
> We will replace with dm_pci_map_bar().
> 
> >
> > Check that you can compile driver without CONFIG_DM_PCI_COMPAT
> > option.
> > I think it should throw compile error on usage of this function.
> 
> We do not enable CONFIG_DM_PCI_COMPAT.

That is interesting as pci_map_bar() function is declared in section:
#if !defined(CONFIG_DM_PCI) || defined(CONFIG_DM_PCI_COMPAT)

So without COMPAT macro it should not be possible to use pci_map_bar()
function at all.

> >
> > > +	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);
> > > +	dbg_pci(bp, __func__, cmd_reg);
> > > +}
> > ...
> > > +static int bnxt_read_rom_hwaddr(struct udevice *dev)
> > > +{
> > > +	struct eth_pdata *plat = dev_get_plat(dev);
> > > +	struct bnxt *bp = dev_get_priv(dev);
> > > +
> > > +	bnxt_bring_pci(bp);
> > > +
> > > +	if (bnxt_bring_chip(bp))
> >
> > It looks suspicious if read_rom_hwaddr() function is doing
> > initialization of PCIe device. Is there any reason for it in this place?
> >
> > Opposite of bnxt_bring_pci+bnxt_bring_chip is bnxt_down_chip and it is
> > called in bnxt_eth_remove() function.
> 
> The method bnxt_bring_pci() fills priv data bp and needs to be called
> before bnxt_bring_chip(). The data does not need to be cleaned in
> bnxt_eth_remove().
> We were asked by another reviewer to use bnxt_read_rom_hwaddr() to set up
> hw address (instead of doing it in probe)

Yes, that is correct way.

> but bnxt_bring_chip() has to be
> called to get the hw addr. So both methods bnxt_bring_pci and
> bnxt_bring_chip were moved there.
> 
> >
> > > +		printf("*** warning: bnxt_bring_chip failed! ***\n");
> >
> > It is only warning? I guess that failed initialization is fatal error
> > and should be propagated via return value... but that is not easily
> > possible if initialization is called from read_rom_hwaddr().
> 
> It is only warning, since there is usually another 1G eth available,
> besides this 100G bnxt eth. Originally, we had it probed via bnxt commands
> on demand. But another reviewer asked to remove bnxt commands so we bring
> up bnxt each boot. But it is not necessarily an error causing the boot to
> fail.

Ok! So let it as is for now.