Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Bharat Kumar Gogada <bharatku@xilinx.com>
To:     Bjorn Helgaas <helgaas@kernel.org>
CC:     "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Ravikiran Gummaluri <rgummal@xilinx.com>
Subject: RE: [PATCH v3 2/2] PCI: Versal CPM: Add support for Versal CPM Root
 Port driver
Thread-Topic: [PATCH v3 2/2] PCI: Versal CPM: Add support for Versal CPM Root
 Port driver
Thread-Index: AQHVyfjOYP1OKhHeMUCSmomtiUQ5I6fpL1IAgBVd0IA=
Date:   Mon, 27 Jan 2020 13:06:54 +0000
Message-ID: <MN2PR02MB633624087571F37F95F1D915A50B0@MN2PR02MB6336.namprd02.prod.outlook.com>
References: <1578909821-10604-3-git-send-email-bharat.kumar.gogada@xilinx.com>
 <20200113223436.GA128724@google.com>
In-Reply-To: <20200113223436.GA128724@google.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: xilinx.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 51931b81-d1d9-40a9-2342-08d7a329c838
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Jan 2020 13:06:54.2954
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Y9sOwI/DU18iHzcwNWf2IFpymFEmhf6d8r27dab6QyPkXxAh8GQsr+gDZNvhIep9eCq4U3g1cw2+QgqOGoUwgA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR02MB6671
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> Subject: Re: [PATCH v3 2/2] PCI: Versal CPM: Add support for Versal CPM R=
oot
> Port driver
>=20
> s/PCI: Versal CPM: .../PCI: xilinx-cpm: Add Versal CPM Root Port driver/
>=20
Thanks Bjorn, sorry for late response.=20
> Format is "PCI: <driver-name>: Subject"
Accepted will change the subject.
>=20
> On Mon, Jan 13, 2020 at 03:33:41PM +0530, Bharat Kumar Gogada wrote:
> > - Adding support for Versal CPM as Root Port.
>=20
> s/- Adding/Add/
>=20
> > - The Versal ACAP devices include CCIX-PCIe Module (CPM). The integrate=
d
> >   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.
>=20
> s/acheiving//
>=20
> > - 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@xilinx.com>
> > Reported-by: kbuild test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>=20
> "Reported-by" is for bug reports.  This makes it look like the lack of th=
e driver is
> the bug, but it's not.  Personally, I'd thank Dan and the kbuild robot, b=
ut not add
> "Reported-by" here.  It's like patch reviews; I don't expect you to menti=
on my
> feedback in the commit log.
Accepted above comments will fix these in next patch.
>=20
> > +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.
>=20
> s/kernel to enable support the/kernel support for the/ s/host Bridge driv=
er./host
> bridge. /  (note space after period)
Accepted , will fix these in next patch.
>=20
> > + * xilinx_cpm_pcie_valid_device - Check if a valid device is present
> > + on bus
>=20
> 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 h=
ave to do
> a config read to see whether a device responds.
Accepted, will change the comments.
>=20
> > + * @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 =3D bus->sysdata;
> > +
> > +	/* Only one device down on each root port */
> > +	if (bus->number =3D=3D port->root_busno && devfn > 0)
> > +		return false;
> > +
> > +	return true;
> > +}
>=20
> > +static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *data)
> > +{
> > +	struct xilinx_cpm_pcie_port *port =3D
> > +				(struct xilinx_cpm_pcie_port *)data;
>=20
> No cast needed.
Accepted, will fix these in next patch.
>=20
> > +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);
>=20
> This lonely writel() in the middle of all the pcie_write() and
> pcie_read() calls *looks* like a mistake.
>=20
> 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.
>=20
> > +	/* 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);
> > +}
>=20
> > +static int xilinx_cpm_pcie_parse_dt(struct xilinx_cpm_pcie_port
> > +*port) {
> > +	struct device *dev =3D port->dev;
> > +	struct resource *res;
> > +	int err;
> > +	struct platform_device *pdev =3D to_platform_device(dev);
>=20
> 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 use=
d in the
> code below.
Accepted, will fix these in next patch.
>=20
> > +	res =3D platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
> > +	port->reg_base =3D devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(port->reg_base))
> > +		return PTR_ERR(port->reg_base);
> > +
> > +	res =3D platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > +					   "cpm_slcr");
> > +	port->cpm_base =3D devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(port->cpm_base))
> > +		return PTR_ERR(port->cpm_base);
>=20
Regards,
Bharat