Re: [PATCH v2] PCI: Unify ECAM constants in native PCI Express drivers

On Tue, Sep 29, 2020 at 3:23 PM Bjorn Helgaas [off-list ref] wrote:

On Fri, Sep 25, 2020 at 09:15:13PM +0000, Krzysztof Wilczyński wrote:

quoted

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

I like this a lot.  A few minor comments below.

quoted

--- a/drivers/pci/controller/dwc/pcie-al.c
+++ b/drivers/pci/controller/dwc/pcie-al.c

@@ -76,7 +76,7 @@ static int al_pcie_init(struct pci_config_window *cfg)
 }

 const struct pci_ecam_ops al_pcie_ops = {
-     .bus_shift    = 20,
+     .bus_shift    = PCIE_ECAM_BUS_SHIFT,
      .init         =  al_pcie_init,
      .pci_ops      = {
              .map_bus    = al_pcie_map_bus,

@@ -138,8 +138,6 @@ struct al_pcie {
      struct al_pcie_target_bus_cfg target_bus_cfg;
 };

-#define PCIE_ECAM_DEVFN(x)           (((x) & 0xff) << 12)
-
 #define to_al_pcie(x)                dev_get_drvdata((x)->dev)

 static inline u32 al_pcie_controller_readl(struct al_pcie *pcie, u32 offset)

@@ -228,7 +226,7 @@ static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
      void __iomem *pci_base_addr;

      pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
-                                      (busnr_ecam << 20) +
+                                      PCIE_ECAM_BUS(busnr_ecam) +
                                       PCIE_ECAM_DEVFN(devfn));

Apparently this driver lets you limit the number of buses, since it
does:

  unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;

I'm not entirely sure I believe that, but OK.  It's a shame because
it would be really nice to be able to use PCIE_ECAM_OFFSET() here
(with a little rework of the al_pcie_conf_addr_map() interface).

It's a bit different than that. It allows for a config window less
than 256MB and will reconfigure the window when the bus number is
outside the window. There's no dts upstream so who knows what the
normal case is. For all I know, Amazon could be using generic ECAM
driver now or only ACPI (note that another oddity is the root bridge
accesses go thru the DBI interface where as other DWC implementations
using generic ECAM use the ECAM space). Maybe Jonathan Chocron can
comment.

I think limiting the bus numbers based on the size of the config
window would have been better and that's aligned with what the generic
ECAM driver does.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel