RE: [PATCH v4 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver

[Havalige, Thippeswamy]

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