RE: [PATCH v3 2/2] PCI: Versal CPM: Add support for Versal CPM Root Port driver

[Bharat Kumar Gogada]

> Subject: Re: [PATCH v3 2/2] PCI: Versal CPM: Add support for Versal CPM Root
> Port driver
> 
> s/PCI: Versal CPM: .../PCI: xilinx-cpm: Add Versal CPM Root Port driver/
> 
Thanks Bjorn, sorry for late response. 
> Format is "PCI: <driver-name>: Subject"
Accepted will change the subject.
> 
> On Mon, Jan 13, 2020 at 03:33:41PM +0530, Bharat Kumar Gogada wrote:
> > - Adding support for Versal CPM as Root Port.
> 
> s/- Adding/Add/
> 
> > - The Versal ACAP devices include CCIX-PCIe Module (CPM). The integrated
> >   block for CPM along with the integrated bridge can function
> >   as PCIe Root Port.
> > - CPM Versal uses GICv3 ITS feature for acheiving assigning MSI/MSI-X
> >   vectors and handling MSI/MSI-X interrupts.
> 
> s/acheiving//
> 
> > - Bridge error and legacy interrupts in Versal CPM are handled using
> >   Versal CPM specific MISC interrupt line.
> >
> > Changes v3:
> > Fix warnings reported.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xxxxxxxxxx>
> > Reported-by: kbuild test robot <lkp@xxxxxxxxx>
> > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> 
> "Reported-by" is for bug reports.  This makes it look like the lack of the driver is
> the bug, but it's not.  Personally, I'd thank Dan and the kbuild robot, but not add
> "Reported-by" here.  It's like patch reviews; I don't expect you to mention my
> feedback in the commit log.
Accepted above comments will fix these in next patch.
> 
> > +config PCIE_XILINX_CPM
> > +	bool "Xilinx Versal CPM host bridge support"
> > +	depends on ARCH_ZYNQMP || COMPILE_TEST
> > +	help
> > +	  Say 'Y' here if you want kernel to enable support the
> > +	  Xilinx Versal CPM host Bridge driver.The driver supports
> > +	  MSI/MSI-X interrupts using GICv3 ITS feature.
> 
> s/kernel to enable support the/kernel support for the/ s/host Bridge driver./host
> bridge. /  (note space after period)
Accepted , will fix these in next patch.
> 
> > + * xilinx_cpm_pcie_valid_device - Check if a valid device is present
> > + on bus
> 
> Technically this does not check if the device is present on the bus.
> It checks whether it's *possible* for a device to be at this address.
> For non-root bus devices in particular, it always returns true, and you have to do
> a config read to see whether a device responds.
Accepted, will change the comments.
> 
> > + * @bus: PCI Bus structure
> > + * @devfn: device/function
> > + *
> > + * Return: 'true' on success and 'false' if invalid device is found
> > +*/ static bool xilinx_cpm_pcie_valid_device(struct pci_bus *bus,
> > +					 unsigned int devfn)
> > +{
> > +	struct xilinx_cpm_pcie_port *port = bus->sysdata;
> > +
> > +	/* Only one device down on each root port */
> > +	if (bus->number == port->root_busno && devfn > 0)
> > +		return false;
> > +
> > +	return true;
> > +}
> 
> > +static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *data)
> > +{
> > +	struct xilinx_cpm_pcie_port *port =
> > +				(struct xilinx_cpm_pcie_port *)data;
> 
> No cast needed.
Accepted, will fix these in next patch.
> 
> > +static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie_port
> > +*port) {
> > +	if (cpm_pcie_link_up(port))
> > +		dev_info(port->dev, "PCIe Link is UP\n");
> > +	else
> > +		dev_info(port->dev, "PCIe Link is DOWN\n");
> > +
> > +	/* Disable all interrupts */
> > +	pcie_write(port, ~XILINX_CPM_PCIE_IDR_ALL_MASK,
> > +		   XILINX_CPM_PCIE_REG_IMR);
> > +
> > +	/* Clear pending interrupts */
> > +	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_IDR) &
> > +		   XILINX_CPM_PCIE_IMR_ALL_MASK,
> > +		   XILINX_CPM_PCIE_REG_IDR);
> > +
> > +	/* Enable all interrupts */
> > +	pcie_write(port, XILINX_CPM_PCIE_IMR_ALL_MASK,
> > +		   XILINX_CPM_PCIE_REG_IMR);
> > +	pcie_write(port, XILINX_CPM_PCIE_IDRN_MASK,
> > +		   XILINX_CPM_PCIE_REG_IDRN_MASK);
> > +
> > +	writel(XILINX_CPM_PCIE_MISC_IR_LOCAL,
> > +	       port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE);
> 
> This lonely writel() in the middle of all the pcie_write() and
> pcie_read() calls *looks* like a mistake.
> 
> I see that the writel() uses port->cpm_base, while pcie_write() uses
> port->reg_base, so I don't think it *is* a mistake, but it's sure not
> obvious.  A blank line after it and a comment at the _MISC_IR definitions about
> them being in a different register set would be nice hints.
Accepted, will add comments.
> 
> > +	/* Enable the Bridge enable bit */
> > +	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_RPSC) |
> > +		   XILINX_CPM_PCIE_REG_RPSC_BEN,
> > +		   XILINX_CPM_PCIE_REG_RPSC);
> > +}
> 
> > +static int xilinx_cpm_pcie_parse_dt(struct xilinx_cpm_pcie_port
> > +*port) {
> > +	struct device *dev = port->dev;
> > +	struct resource *res;
> > +	int err;
> > +	struct platform_device *pdev = to_platform_device(dev);
> 
> The "struct platform_device ..." line really should be first in the list.  Not because
> of "reverse Christmas tree", but because "pdev" is the first variable used in the
> code below.
Accepted, will fix these in next patch.
> 
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
> > +	port->reg_base = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(port->reg_base))
> > +		return PTR_ERR(port->reg_base);
> > +
> > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > +					   "cpm_slcr");
> > +	port->cpm_base = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(port->cpm_base))
> > +		return PTR_ERR(port->cpm_base);
> 
Regards,
Bharat