Hi Bjorn,
> > + bridge DMA as soft IP say Y; if you are not sure, say N.
>
> s/is an soft/is soft/
> s/xilinx/Xilinx/
- Agreed, Will fix it in next patch.
> > +#define XILINX_PCIE_DMA_REG_IMR 0x0000013c
> > +#define XILINX_PCIE_DMA_REG_MSIBASE1 0x0000014c
> > +#define XILINX_PCIE_DMA_REG_MSI_HI_MASK 0x0000017c
> > ...
> > +#define XILINX_PCIE_DMA_IMR_ALL_MASK 0x0FF30FE9
> > +#define XILINX_PCIE_DMA_IDR_ALL_MASK 0xFFFFFFFF
>
> Pick upper-case hex or lower-case hex and use it consistently.
- Agreed, will fix it in next patch.
> > +static inline bool xilinx_pl_dma_pcie_linkup(struct pl_dma_pcie
> > +*port)
>
> Name this *_pcie_link_up() (not *_pcie_linkup()) to match other drivers.
- Agreed, Will fix it in next patch.
> > +static bool xilinx_pl_dma_pcie_valid_device(struct pci_bus *bus,
> > +unsigned int devfn) {
> > + struct pl_dma_pcie *port = bus->sysdata;
> > +
> > + /* Check if link is up when trying to access downstream ports */
> > + if (!pci_is_root_bus(bus)) {
> > + if (!xilinx_pl_dma_pcie_linkup(port))
> > + return false;
> > + } else if (devfn > 0)
> > + /* Only one device down on each root port */
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +static void __iomem *xilinx_pl_dma_pcie_map_bus(struct pci_bus *bus,
> > + unsigned int devfn, int where)
> > +{
> > + struct pl_dma_pcie *port = bus->sysdata;
> > +
> > + if (!xilinx_pl_dma_pcie_valid_device(bus, devfn))
> > + return NULL;
>
> Checking whether the link is up is racy because the link may be up, so
> xilinx_pl_dma_pcie_valid_device() returns true, then the link may go down
> before the read below.
>
> What happens then? If it's an error that you can recover from, it would
> better to skip the link up check and just handle the error.
- When link is down CPU stalls after RC Enumeration where it tries to send PIO requests to EP and waits for its acknowledgement. Hence we need link up check atleast boot successfully and show RootPort device in lspci. We will add comments in the above function describe the race condition.
> > + return port->reg_base + PCIE_ECAM_OFFSET(bus->number, devfn,
> where);
> > +}
>
> > + /*set the Bridge enable bit */
>
> /* Set ... */ (add space before "Set" and capitalize it)
>
> Bjorn
Regards,
Thippeswamy H
