RE: [PATCH v3 00/32] provide interfaces to access PCIe capabilities registers

["Cui, Dexuan" <dexuan.cui@intel.com>]

Bjorn Helgaas wrote on 2012-08-21:
> On Mon, Aug 20, 2012 at 10:10 AM, Bjorn Helgaas <bhelgaas@google.com>
> wrote:
> 
>> So I'll try pulling your branch (I'll do something about the tsi721.c
>> stuff myself).
> 
> I pulled this into
> git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git
> pci/jiang-pcie-cap with the following changes:
> 
>   - Dropped some "pci_" prefixes on internal functions in access.c -
>   Minor restructure of pcie_capability_read_*() - Removed export of
>   pcie_capability_reg_implemented() - Reworked
>   reset_intel_82599_sfp_virtfn() to check for FLR bit in DEVCAP rather
>   than using pcie_capability_reg_implemented() - Split driver patches
>   into one driver per patch - Fixed myri10ge_toggle_relaxed() --
>   previous code returned useful value, but your patch made it always
>   return zero - Use 16-bit, not 32-bit, accesses for DEVCTL, DEVCTL2
>   (tsi721)
> I am still concerned about reset_intel_82599_sfp_virtfn().  It looks
> wrong and possibly unnecessary.  It looks wrong because it sets
> PCI_EXP_DEVCTL_BCR_FLR and blindly clears all other bits in
> PCI_EXP_DEVCTL.  Most of the bits are probably cleared by the FLR
> anyway, but Aux Power PM Enable is RWS ("sticky"), so it is *not*
> modified by FLR.  Therefore, using reset_intel_82599_sfp_virtfn() has
> the probably unintended side effect of clearing Aux Power PM Enable.
> 
> It looks possibly unnecessary because the generic pcie_flr() does
> essentially the same thing, so it's not clear why we need a special
> case for 82599.
> 
> Yu or Dexuan, can you comment on these 82599 questions?
(Removed Yu Zhao from the To list since he has left Intel and the same
email is now used by another unrelated person...)

Hi Bjorn
I think reset_intel_82599_sfp_virtfn() is correct AND necessary.
(pcie_flr() doesn't work here)

Please note the 82599 VF is a special PCIe device that doesn't report
PCIe FLR capability though actually it does support that.
That is why we put it in quirks.c. :-)

The PCI_EXP_DEVCTL_AUX_PME bit of the 82599 VF is read-only and
always zero.

Please see 
http://www.intel.com/content/dam/doc/datasheet/82599-10-gbe-controller-datasheet.pdf
9.5 Virtual Functions Configuration Space
  Table 9.7 VF PCIe Configuration Space
  9.5.2.2.1 VF Device Control Register (0xA8; RW)

Surely I should say sorry for not putting these necessary information
in the git commit description.
Now I don't know why I forgot to do that at that time(almost 3 years ago...).

It would be great if you could help to add a comment in the code. :-)

> 
> The proposed new code is here:
> http://git.kernel.org/?p=linux/kernel/git/helgaas/pci.git;a=blob;f=driver
> s/pci/
> quirks.c;h=9abbf56e93fe5c98364a7dfe2b0b724047dfa4a9;hb=ef529bf0cb560
> 6ff5bd1422d2d2700a821d8218b#l3082
> 
> Bjorn

Thanks,
-- Dexuan