Hi Bjorn and Rob,

[...]

quoted

quoted

#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

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