Hi Bjorn and Rob, [...] > > > #define PCIE_ECAM_BUS(x) (((x) & 0xff) << 20) > > > #define PCIE_ECAM_DEV(x) (((x) & 0x1f) << 15) > > > #define PCIE_ECAM_FUNC(x) (((x) & 0x7) << 12) > > > > > > drivers/pci/controller/dwc/pcie-al.c: > > > > > > #define PCIE_ECAM_DEVFN(x) (((x) & 0xff) << 12) > > > > > > I can move PCIE_ECAM_BUS, PCIE_ECAM_DEV and PCIE_ECAM_FUNC (as > > > PCIE_ECAM_FUN) to the linux/pci-ecam.h file, as these seem useful, but > > > without the masks, and then update other files to use these. We could > > > then leverage these, for example: > > > > > > pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base + > > > - (busnr_ecam << 20) + > > > - PCIE_ECAM_DEVFN(devfn)); > > > + PCIE_ECAM_BUS(busnr_ecam) + > > > + PCIE_ECAM_FUN(devfn)); > > > > > > What do you think? Bjorn, would that be acceptable? > > > > It would be nice to use the same style and same macros for all of > > the following, which are all really doing the same thing: > > > > al_pcie_conf_addr_map() > > pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base + > > (busnr_ecam << 20) + > > PCIE_ECAM_DEVFN(devfn)); > > > > rockchip_pcie_rd_other_conf() > > busdev = PCIE_ECAM_ADDR(bus->number, PCI_SLOT(devfn), > > PCI_FUNC(devfn), where); > > > > nwl_pcie_map_bus() > > relbus = (bus->number << ECAM_BUS_LOC_SHIFT) | > > (devfn << ECAM_DEV_LOC_SHIFT); > > > > return pcie->ecam_base + relbus + where; > > > > xilinx_pcie_map_bus() > > relbus = (bus->number << ECAM_BUS_NUM_SHIFT) | > > (devfn << ECAM_DEV_NUM_SHIFT); > > > > return port->reg_base + relbus + where; > > > > Maybe that's something like using PCIE_ECAM_ADDR() everywhere? I'm > > not sure there's value in having the caller do the PCI_SLOT() and > > PCI_FUNC() decomposition, though, i.e., maybe it's something like > > this? > > > > #define PCIE_ECAM_REG(x) ((x) & 0xfff) > > > > #define PCI_ECAM_OFFSET(bus, devfn, where) \ > > PCIE_ECAM_BUS(bus->number) | \ > > PCIE_ECAM_DEVFN(devfn) | \ > > PCIE_ECAM_REG(where) [...] > LGTM. This was on my radar, but not something I've looked at. > > There's also aardvark which isn't ECAM, but does the same calculation. > Call it indirect ECAM: > > drivers/pci/controller/pci-aardvark.c:#define PCIE_CONF_BUS(bus) > (((bus) & 0xff) << 20) > drivers/pci/controller/pci-aardvark.c-#define PCIE_CONF_DEV(dev) > (((dev) & 0x1f) << 15) > drivers/pci/controller/pci-aardvark.c-#define PCIE_CONF_FUNC(fun) > (((fun) & 0x7) << 12) > drivers/pci/controller/pci-aardvark.c-#define PCIE_CONF_REG(reg) > ((reg) & 0xffc) > drivers/pci/controller/pci-aardvark.c-#define PCIE_CONF_ADDR(bus, > devfn, where) \ > drivers/pci/controller/pci-aardvark.c- (PCIE_CONF_BUS(bus) | > PCIE_CONF_DEV(PCI_SLOT(devfn)) | \ > drivers/pci/controller/pci-aardvark.c- > PCIE_CONF_FUNC(PCI_FUNC(devfn)) | PCIE_CONF_REG(where)) > > And VMD: > drivers/pci/controller/vmd.c- char __iomem *addr = vmd->cfgbar + > drivers/pci/controller/vmd.c: ((bus->number - > vmd->busn_start) << 20) + > drivers/pci/controller/vmd.c- (devfn << 12) + reg; > drivers/pci/controller/vmd.c- > > > And brcm_pcie_cfg_index(). Thank you both for good feedback! I will send a v2 later incorporating the feedback and suggestions. Krzysztof
