On 10/4/2020 5:38 PM, Krzysztof Wilczyński wrote:
Unify ECAM-related constants into a single set of standard constants
defining memory address shift values for the byte-level address that can
be used when accessing the PCI Express Configuration Space, and then
move native PCI Express controller drivers to use newly introduced
definitions retiring any driver-specific ones.
The ECAM ("Enhanced Configuration Access Mechanism") is defined by the
PCI Express specification (see PCI Express Base Specification, Revision
5.0, Version 1.0, Section 7.2.2, p. 676), thus most hardware should
implement it the same way. Most of the native PCI Express controller
drivers define their ECAM-related constants, many of these could be
shared, or use open-coded values when setting the .bus_shift field of
the struct pci_ecam_ops.
All of the newly added constants should remove ambiguity and reduce the
number of open-coded values, and also correlate more strongly with the
descriptions in the aforementioned specification (see Table 7-1
"Enhanced Configuration Address Mapping", p. 677).
There is no change to functionality.
Suggested-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
Signed-off-by: Krzysztof Wilczyński <kw@xxxxxxxxx>
---
[snip]
-/* Configuration space read/write support */
-static inline int brcm_pcie_cfg_index(int busnr, int devfn, int reg)
-{
- return ((PCI_SLOT(devfn) & 0x1f) << PCIE_EXT_SLOT_SHIFT)
- | ((PCI_FUNC(devfn) & 0x07) << PCIE_EXT_FUNC_SHIFT)
- | (busnr << PCIE_EXT_BUSNUM_SHIFT)
- | (reg & ~3);
-}
-
static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
int where)
{
@@ -590,7 +578,7 @@ static void __iomem *brcm_pcie_map_conf(struct pci_bus *bus, unsigned int devfn,
return PCI_SLOT(devfn) ? NULL : base + where;
/* For devices, write to the config space index register */
- idx = brcm_pcie_cfg_index(bus->number, devfn, 0);
+ idx = PCIE_ECAM_BUS(bus->number) | PCIE_ECAM_DEVFN(devfn);
This appears to be correct, so:
Acked-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
however, I would have defined a couple of additional helper macros and do:
idx = PCIE_ECAM_BUS(bus->number) | PCIE_ECAM_DEV(devfn) |
PCIE_ECAM_FUN(devfn);
for clarity.
[snip]
+/*
+ * Memory address shift values for the byte-level address that
+ * can be used when accessing the PCI Express Configuration Space.
+ */
+
+/*
+ * Enhanced Configuration Access Mechanism (ECAM)
+ *
+ * See PCI Express Base Specification, Revision 5.0, Version 1.0,
+ * Section 7.2.2, Table 7-1, p. 677.
+ */
+#define PCIE_ECAM_BUS_SHIFT 20 /* Bus Number */
+#define PCIE_ECAM_DEV_SHIFT 15 /* Device Number */
+#define PCIE_ECAM_FUN_SHIFT 12 /* Function Number */
+
+#define PCIE_ECAM_BUS(x) (((x) & 0xff) << PCIE_ECAM_BUS_SHIFT)
+#define PCIE_ECAM_DEVFN(x) (((x) & 0xff) << PCIE_ECAM_FUN_SHIFT)
For instance, adding these two:
#define PCIE_ECAM_DEV(x) (((x) & 0x1f) << PCIE_ECAM_DEV_SHIFT)
#define PCIE_ECAM_FUN(x) (((x) & 0x7) << PCIE_ECAM_FUN_SHIFT)
may be clearer for use in drivers like pcie-brcmstb.c that used to treat
the device function in terms of device and function (though it was
called slot there).
--
Florian
