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

[Rob Herring <robh@kernel.org>]

On Tue, Sep 22, 2020 at 5:27 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Rob, who's doing a lot of cleanup in these drivers]
>
> On Sat, Sep 05, 2020 at 10:44:16PM +0200, Krzysztof Wilczyński wrote:
> > Hello Jonathan,
> >
> > Thank you for the review!  Also, apologies for late reply.
> >
> > On 20-08-28 10:08:43, Jonathan Cameron wrote:
> > [...]
> > >
> > > Might potentially be worth tidying up the masks as well?
> > > Or potentially drop them given I suspect that there are no cases
> > > in which the mask is actually doing anything...
> >
> > Just to confirm - you have the following constants in mind?
> >
> > drivers/pci/controller/pcie-rockchip.h:
> >
> > #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().

Rob