On Tue, Sep 22, 2020 at 5:27 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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
