On Tue, Sep 29, 2020 at 3:23 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> On Fri, Sep 25, 2020 at 09:15:13PM +0000, 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 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.
>
> > --- 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
