Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
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>,
        "bhelgaas@google.com" <bhelgaas@google.com>,
        "lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
        "robh@kernel.org" <robh@kernel.org>, Marc Zyngier <maz@kernel.org>
Subject: RE: [PATCH v8 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver
Thread-Topic: [PATCH v8 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver
Thread-Index: AQHWPZdq6W68nNHNZEqBlWufdyFfUqjT6lAAgAFU0zA=
Date:   Fri, 12 Jun 2020 17:37:39 +0000
Message-ID: <BYAPR02MB555986C1464705810BCBBBE8A5810@BYAPR02MB5559.namprd02.prod.outlook.com>
References: <1591622338-22652-3-git-send-email-bharat.kumar.gogada@xilinx.com>
 <20200611205616.GA1607864@bjorn-Precision-5520>
In-Reply-To: <20200611205616.GA1607864@bjorn-Precision-5520>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: ef7820b7-2f05-45d9-2361-08d80ef74dfc
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Jun 2020 17:37:39.8807
 (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: hqdib+00QWbgP88LOkwY5OZWKI1yOEwdLRsr3fXABkGBKW0PUdsp6OSsqWXkJ3pC7XcspeM0HRnGT//3BDt7xQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR02MB5829
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 v8 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port dri=
ver
>=20
> On Mon, Jun 08, 2020 at 06:48:58PM +0530, Bharat Kumar Gogada wrote:
> > - Add support for Versal CPM as Root Port.
> > - 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.
> > - Bridge error and legacy interrupts in Versal CPM are handled using
> >   Versal CPM specific interrupt line.
>=20
> > +static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void
> > +*dev_id) {
> > +	struct xilinx_cpm_pcie_port *port =3D dev_id;
> > +	struct device *dev =3D port->dev;
> > +	struct irq_data *d;
> > +
> > +	d =3D irq_domain_get_irq_data(port->cpm_domain, irq);
> > +
> > +	switch (d->hwirq) {
> > +	case XILINX_CPM_PCIE_INTR_CORRECTABLE:
> > +	case XILINX_CPM_PCIE_INTR_NONFATAL:
> > +	case XILINX_CPM_PCIE_INTR_FATAL:
> > +		cpm_pcie_clear_err_interrupts(port);
> > +		fallthrough;
> > +
> > +	default:
> > +		if (intr_cause[d->hwirq].str)
> > +			dev_warn(dev, "%s\n", intr_cause[d->hwirq].str);
> > +		else
> > +			dev_warn(dev, "Unknown interrupt\n");
>=20
> Maybe include d->hwirq in the "Unknown interrupt" message?
Hi Bjorn,
Yes, we can add.
>=20
> I assume if we take the default case, there's no need to clear the interr=
upt?
 It's being handled in xilinx_cpm_pcie_event_flow.
>=20
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
>=20
> > +static int xilinx_cpm_setup_irq(struct xilinx_cpm_pcie_port *port) {
> > +	struct device *dev =3D port->dev;
> > +	struct platform_device *pdev =3D to_platform_device(dev);
> > +	int i, irq;
> > +
> > +	port->irq =3D platform_get_irq(pdev, 0);
> > +	if (port->irq < 0) {
> > +		dev_err(dev, "Unable to find misc IRQ line\n");
>=20
> platform_get_irq() already prints an error message; you probably don't ne=
ed
> another.
>=20
> > +		return port->irq;
> > +	}
> > +
> > +	for (i =3D 0; i < ARRAY_SIZE(intr_cause); i++) {
> > +		int err;
> > +
> > +		if (!intr_cause[i].str)
> > +			continue;
> > +
> > +		irq =3D irq_create_mapping(port->cpm_domain, i);
> > +		if (WARN_ON(irq <=3D 0))
>=20
> I'm not a huge fan of WARN_ON() inside "if" statements, but ... OK.
>=20
> Why do these all need to be WARN_ON() instead of dev_warn()?
Yes, it can be replaced with dev_warn().
Will fix these.
Regards,
Bharat
>=20
> > +			return -ENXIO;
> > +
> > +		err =3D devm_request_irq(dev, irq,
> xilinx_cpm_pcie_intr_handler,
> > +				       0, intr_cause[i].sym, port);
> > +		if (WARN_ON(err))
> > +			return err;
> > +	}
> > +
> > +	port->intx_irq =3D irq_create_mapping(port->cpm_domain,
> > +					    XILINX_CPM_PCIE_INTR_INTX);
> > +	if (WARN_ON(port->intx_irq <=3D 0))
> > +		return -ENXIO;
> > +
> > +	/* Plug the INTx chained handler */
> > +	irq_set_chained_handler_and_data(port->intx_irq,
> > +					 xilinx_cpm_pcie_intx_flow, port);