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

[Havalige, Thippeswamy]

Hi Rob Herring,

Thanks for reviewing, please find my inline response to your comments.

Regards,
Thippeswamy H 

> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/msi.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_pci.h>
> 
> > +#include <linux/of_platform.h>
> > +#include <linux/of_irq.h>
> 
> I don't think you need either of these.
- Agreed and will fix this in next patch.
> 
> > +#define XILINX_PCIE_DMA_REG_MSI_LOW_MASK	0x00000178
> > +#define XILINX_PCIE_DMA_REG_MSI_HI_MASK		0x0000017c
> > +
> > +/* Interrupt registers definitions */
> > +#define XILINX_PCIE_DMA_INTR_LINK_DOWN		0
> > +#define XILINX_PCIE_DMA_INTR_HOT_RESET		3
> > +#define XILINX_PCIE_DMA_INTR_CFG_TIMEOUT	8
> > +#define XILINX_PCIE_DMA_INTR_CORRECTABLE	9
> > +#define XILINX_PCIE_DMA_INTR_NONFATAL		10
> > +#define XILINX_PCIE_DMA_INTR_FATAL		11
> > +#define XILINX_PCIE_DMA_INTR_INTX		16
> > +#define XILINX_PCIE_DMA_INTR_MSI		17
> > +#define XILINX_PCIE_DMA_INTR_SLV_UNSUPP		20
> > +#define XILINX_PCIE_DMA_INTR_SLV_UNEXP		21
> > +#define XILINX_PCIE_DMA_INTR_SLV_COMPL		22
> > +#define XILINX_PCIE_DMA_INTR_SLV_ERRP		23
> > +#define XILINX_PCIE_DMA_INTR_SLV_CMPABT		24
> > +#define XILINX_PCIE_DMA_INTR_SLV_ILLBUR		25
> > +#define XILINX_PCIE_DMA_INTR_MST_DECERR		26
> > +#define XILINX_PCIE_DMA_INTR_MST_SLVERR		27
> 
> Looks like a superset of the pcie-xilinx-cpm.c registers. You can't share some
> code here? Like all the interrupt handling code which does nothing more
> than print debug messages...
- Agreed, will add above macro's to common header file and fix it next patch
> 

> > + * struct xilinx_pcie_dma - PCIe port information
> > + * @reg_base: IO Mapped Register Base
> > + * @irq: Interrupt number
> > + * @cfg: Holds mappings of config space window
> > + * @root_busno: Root Bus number
> > + * @dev: Device pointer
> > + * @phys_reg_base: Physical address of reg base
> > + * @leg_domain: Legacy IRQ domain pointer
> > + * @pldma_domain: PL DMA IRQ domain pointer
> > + * @resources: Bus Resources
> > + * @msi: MSI information
> > + * @irq_misc: Legacy and error interrupt number
> > + * @intx_irq: legacy interrupt number
> > + * @lock: lock protecting shared register access  */ struct
> > +xilinx_pcie_dma {
> > +	void __iomem			*reg_base;
> > +	u32				irq;
> > +	struct pci_config_window	*cfg;
> > +	u8				root_busno;
> 
> No need to store this. You can use pci_is_root_bus().
	- Agreed and fix it in next patch
> > +	struct device			*dev;
> > +	phys_addr_t			phys_reg_base;
> > +*port) {
> > +	return (pcie_read(port, XILINX_PCIE_DMA_REG_PSCR) &
> > +		XILINX_PCIE_DMA_REG_PSCR_LNKUP) ? 1 : 0; }
> > +
> > +/**
> > + * xilinx_pcie_dma_clear_err_interrupts - Clear Error Interrupts
> > + * @port: PCIe port information
> 
> You don't really need kerneldoc comments on private functions.
- Agreed, will fix it in next patch
> > + */
> > +static void xilinx_pcie_dma_clear_err_interrupts(struct
> > +xilinx_pcie_dma *port) {
> > +	unsigned long val = pcie_read(port, XILINX_PCIE_DMA_REG_RPEFR);
> > +
> > +	if (val & XILINX_PCIE_DMA_RPEFR_ERR_VALID) {
> > +		dev_dbg(port->dev, "Requester ID %lu\n",
> > +			val & XILINX_PCIE_DMA_RPEFR_REQ_ID);
> > +		pcie_write(port, XILINX_PCIE_DMA_RPEFR_ALL_MASK,
> > +			   XILINX_PCIE_DMA_REG_RPEFR);
> > +	}
> > +}
> > + *
> > + * Return: 'true' on success and 'false' if invalid device is found
> > +*/ static bool xilinx_pcie_dma_valid_device(struct pci_bus *bus,
> > +unsigned int devfn) {
> > +	struct xilinx_pcie_dma *port = bus->sysdata;
> > +
> > +	/* Check if link is up when trying to access downstream ports */
> > +	if (bus->number != port->root_busno)
> 
> Use pci_is_root_bus()
    - Agreed,fix it in next patch
> > +		if (!xilinx_pcie_dma_linkup(port))
> > +			return false;
> 
> The link went down right here after you checked. What happens next?
    - for above mentioned case, for the transactions which are sent but not completed, when link goes down, the bridge responds with SLVERR for these requests.
> > +
> > +	/* Only one device down on each root port */
> > +	if (bus->number == port->root_busno && devfn > 0)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +/**
> > + * Return: '0' on success and error value on failure  */ static int
> > +xilinx_pcie_dma_init_irq_domain(struct xilinx_pcie_dma *port) {
> > +	struct device *dev = port->dev;
> > +	struct device_node *node = dev->of_node;
> > +	struct device_node *pcie_intc_node;
> > +	int ret;
> > +
> > +	/* Setup INTx */
> > +	pcie_intc_node = of_get_next_child(node, NULL);
> 
> This breaks if someone puts the PCI devices into DT (which is perfectly valid
> to do on any PCI host bridge). It also assumes some order of child nodes
> which is undefined. Be specific about what child node you want.
	--> Agreed, fix it in next patch
> > +	if (!pcie_intc_node) {
> > +		dev_err(dev, "No PCIe Intc node found\n");
> > +		return PTR_ERR(pcie_intc_node);
> > +	}
> > +
> > +	port->pldma_domain = irq_domain_add_linear(pcie_intc_node, 32,
> > +						   &event_domain_ops, port);
> > +	if (!port->pldma_domain)
> > +		goto out;
> > +
> > +	irq_domain_update_bus_token(port->pldma_domain,
> DOMAIN_BUS_NEXUS);
> > +
> > +	port->leg_domain = irq_domain_add_linear(pcie_intc_node,
> PCI_NUM_INTX,
> > +						 &intx_domain_ops,
> > +	struct resource regs;
> > +	int err;
> > +
> > +	err = of_address_to_resource(node, 0, &regs);
 
Use platform_get_resource() instead.
--> Agreed, fix it in next patch 
> > +	if (err) {
> > +		dev_err(dev, "missing \"reg\" property\n");
> > +		return err;
> &xilinx_pcie_dma_ops);
> > +	if (IS_ERR(port->cfg))
> > +		return -1;
> 
> Return the original error value. -1 should never be used.
 --> Agreed, fix in next patch
> > +
> > +	port->reg_base = port->cfg->win;
> > +