[+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)
