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

["Krzysztof Wilczyński" <kw@linux.com>]

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