| Message ID | 1617440059-2478-3-git-send-email-liudongdong3@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Enable 10-Bit tag support for PCIe devices | expand |
> + ret = pcie_capability_read_dword(dev, PCI_EXP_DEVCAP2, &cap); > + if (ret) > + return; Wouldn't it make sense to store the devcap value in the pci_dev structure instead of reading it multiple times? > + /* 10-Bit Tag Requester Enable in Device Control 2 Register is RsvdP for VF */ Please avoid the overly long lines. > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT && > + dev->ext_10bit_tag_comp_path == 1 && > + (cap & PCI_EXP_DEVCAP2_10BIT_TAG_REQ)) { > + pci_info(dev, "enabling 10-Bit Tag Requester\n"); I think that printk might become a little too noisy when lots of devices support this capability. > + unsigned int ext_10bit_tag_comp_path:1; /* 10-Bit Tag Completer Supported from root to here */ Another crazy long line. And why not just name this 10bit_tags? Also a lot of this walk the upstream bridges until we hit the root port code seems duplicatated for different capabilities. Shouldn't we have one such walk that checks all the interesting capabilities? Or even turn the thing around and set them on the fly while scanning the topology?
Hi Christoph Many Thanks for your review. On 2021/4/3 17:50, Christoph Hellwig wrote: >> + ret = pcie_capability_read_dword(dev, PCI_EXP_DEVCAP2, &cap); >> + if (ret) >> + return; > > Wouldn't it make sense to store the devcap value in the pci_dev > structure instead of reading it multiple times? > Good suggestion. >> + /* 10-Bit Tag Requester Enable in Device Control 2 Register is RsvdP for VF */ > > Please avoid the overly long lines. Will fix. > >> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT && >> + dev->ext_10bit_tag_comp_path == 1 && >> + (cap & PCI_EXP_DEVCAP2_10BIT_TAG_REQ)) { >> + pci_info(dev, "enabling 10-Bit Tag Requester\n"); > > I think that printk might become a little too noisy when lots of > devices support this capability. > Yes, How about change to pci_dbg. >> + unsigned int ext_10bit_tag_comp_path:1; /* 10-Bit Tag Completer Supported from root to here */ > > Another crazy long line. And why not just name this > 10bit_tags? OK, will fix. How about name this _10bit_tag or ext_10bit_tag as C language variable names cannot start with a number. > > Also a lot of this walk the upstream bridges until we hit the > root port code seems duplicatated for different capabilities. Shouldn't > we have one such walk that checks all the interesting capabilities? Or > even turn the thing around and set them on the fly while scanning > the topology? I will investigate more about this. Thanks for your suggestion. Thanks, Dongdong > . >
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 953f15a..3efe1cc 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2051,6 +2051,44 @@ int pci_configure_extended_tags(struct pci_dev *dev, void *ign) return 0; } +static void pci_configure_10bit_tags(struct pci_dev *dev) +{ + u32 cap; + int ret; + struct pci_dev *bridge; + + if (!pci_is_pcie(dev)) + return; + + ret = pcie_capability_read_dword(dev, PCI_EXP_DEVCAP2, &cap); + if (ret) + return; + + if (!(cap & PCI_EXP_DEVCAP2_10BIT_TAG_COMP)) + return; + + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) { + dev->ext_10bit_tag_comp_path = 1; + return; + } + + bridge = pci_upstream_bridge(dev); + if (bridge && bridge->ext_10bit_tag_comp_path) + dev->ext_10bit_tag_comp_path = 1; + + /* 10-Bit Tag Requester Enable in Device Control 2 Register is RsvdP for VF */ + if (dev->is_virtfn) + return; + + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ENDPOINT && + dev->ext_10bit_tag_comp_path == 1 && + (cap & PCI_EXP_DEVCAP2_10BIT_TAG_REQ)) { + pci_info(dev, "enabling 10-Bit Tag Requester\n"); + pcie_capability_set_word(dev, PCI_EXP_DEVCTL2, + PCI_EXP_DEVCTL2_10BIT_TAG_REQ_EN); + } +} + /** * pcie_relaxed_ordering_enabled - Probe for PCIe relaxed ordering enable * @dev: PCI device to query @@ -2190,6 +2228,7 @@ static void pci_configure_device(struct pci_dev *dev) { pci_configure_mps(dev); pci_configure_extended_tags(dev, NULL); + pci_configure_10bit_tags(dev); pci_configure_relaxed_ordering(dev); pci_configure_ltr(dev); pci_configure_eetlp_prefix(dev); diff --git a/include/linux/pci.h b/include/linux/pci.h index 86c799c..1cd0ee0 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -390,6 +390,7 @@ struct pci_dev { #endif unsigned int eetlp_prefix_path:1; /* End-to-End TLP Prefix */ + unsigned int ext_10bit_tag_comp_path:1; /* 10-Bit Tag Completer Supported from root to here */ pci_channel_state_t error_state; /* Current connectivity state */ struct device dev; /* Generic device interface */
10-Bit Tag capability, introduced in PCIe-4.0 increases the total Tag field size from 8 bits to 10 bits. For platforms where the RC supports 10-Bit Tag Completer capability, it is highly recommended for platform firmware or operating software that configures PCIe hierarchies to Set the 10-Bit Tag Requester Enable bit automatically in Endpoints with 10-Bit Tag Requester capability. This enables the important class of 10-Bit Tag capable adapters that send Memory Read Requests only to host memory. Signed-off-by: Dongdong Liu <liudongdong3@huawei.com> --- drivers/pci/probe.c | 39 +++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 1 + 2 files changed, 40 insertions(+)