> 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
