> Date: Wed, 2 Sep 2020 15:19:55 +1000
> From: Jonathan Gray <[email protected]>
>
> On Tue, Sep 01, 2020 at 11:44:03PM -0500, Jordan Hargrave wrote:
> > This patch adds a common function for scanning PCIE Express Capability list
> > The PCIE Capability list starts at 0x100 in extended PCI configuration
> > space.
>
> This seems to only handle extended capabilities?
> Something like pcie_get_ext_capability() would be a better name.
I think it should be pci_get_ext_capability() which follows the
pattern set by pci_get_ht_capability().
>
> It is 'PCI Express' not 'PCIExpress'
>
> 'ofs & 3' test doesn't make sense when PCI_PCIE_ECAP_NEXT() always
> masks those bits.
>
> >
> > ---
> > sys/dev/pci/pci.c | 28 ++++++++++++++++++++++++++++
> > sys/dev/pci/pcivar.h | 2 ++
> > 2 files changed, 30 insertions(+)
> >
> > diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
> > index bf75f875e..8f9a5ef7a 100644
> > --- a/sys/dev/pci/pci.c
> > +++ b/sys/dev/pci/pci.c
> > @@ -677,6 +677,34 @@ pci_get_ht_capability(pci_chipset_tag_t pc, pcitag_t
> > tag, int capid,
> > return (0);
> > }
> >
> > +int
> > +pcie_get_capability(pci_chipset_tag_t pc, pcitag_t tag, int capid,
> > + int *offset, pcireg_t *value)
> > +{
> > + pcireg_t reg;
> > + unsigned int ofs;
> > +
> > + /* Make sure we support PCIExpress device */
PCI Express like jsg@ already mentioned. Add a full stop at the end
of the sentence.
> > + if (pci_get_capability(pc, tag, PCI_CAP_PCIEXPRESS, NULL, NULL) == 0)
> > + return (0);
> > + /* Scan PCIExpress capabilities */
Drop this comment and replace with a blank line such that it matches
pci_get_ht_capability().
> > + ofs = PCI_PCIE_ECAP;
> > + while (ofs != 0) {
> > + if ((ofs & 3) || (ofs < PCI_PCIE_ECAP))
> > + return (0);
Make this check #ifdef DIAGNOSTIC like pci_get_ht_capability() doesn.
Dropping the (ofs & 3) bit is indeed a good idea.
> > + reg = pci_conf_read(pc, tag, ofs);
> > + if (PCI_PCIE_ECAP_ID(reg) == capid) {
> > + if (offset)
> > + *offset = ofs;
> > + if (value)
> > + *value = reg;
> > + return (1);
> > + }
> > + ofs = PCI_PCIE_ECAP_NEXT(reg);
> > + }
Blank line here please.
> > + return (0);
> > +}
> > +
> > uint16_t
> > pci_requester_id(pci_chipset_tag_t pc, pcitag_t tag)
> > {
> > diff --git a/sys/dev/pci/pcivar.h b/sys/dev/pci/pcivar.h
> > index bdfe0404f..0376ba992 100644
> > --- a/sys/dev/pci/pcivar.h
> > +++ b/sys/dev/pci/pcivar.h
> > @@ -233,6 +233,8 @@ int pci_io_find(pci_chipset_tag_t, pcitag_t, int,
> > bus_addr_t *,
> > int pci_mem_find(pci_chipset_tag_t, pcitag_t, int, bus_addr_t *,
> > bus_size_t *, int *);
> >
> > +int pcie_get_capability(pci_chipset_tag_t, pcitag_t, int,
> > + int *, pcireg_t *);
> > int pci_get_capability(pci_chipset_tag_t, pcitag_t, int,
> > int *, pcireg_t *);
> > int pci_get_ht_capability(pci_chipset_tag_t, pcitag_t, int,
> > --
> > 2.26.2
> >
> >
>
>
