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;
Date:   Thu, 11 Jun 2020 15:56:16 -0500
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
Cc:     linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        bhelgaas@google.com, lorenzo.pieralisi@arm.com, robh@kernel.org,
        Marc Zyngier <maz@kernel.org>
Subject: Re: [PATCH v8 2/2] PCI: xilinx-cpm: Add Versal CPM Root Port driver
Message-ID: <20200611205616.GA1607864@bjorn-Precision-5520>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1591622338-22652-3-git-send-email-bharat.kumar.gogada@xilinx.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 integrated
>   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.

> +static irqreturn_t xilinx_cpm_pcie_intr_handler(int irq, void *dev_id)
> +{
> +	struct xilinx_cpm_pcie_port *port = dev_id;
> +	struct device *dev = port->dev;
> +	struct irq_data *d;
> +
> +	d = 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");

Maybe include d->hwirq in the "Unknown interrupt" message?

I assume if we take the default case, there's no need to clear the
interrupt?

> +	}
> +
> +	return IRQ_HANDLED;
> +}

> +static int xilinx_cpm_setup_irq(struct xilinx_cpm_pcie_port *port)
> +{
> +	struct device *dev = port->dev;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	int i, irq;
> +
> +	port->irq = platform_get_irq(pdev, 0);
> +	if (port->irq < 0) {
> +		dev_err(dev, "Unable to find misc IRQ line\n");

platform_get_irq() already prints an error message; you probably don't
need another.

> +		return port->irq;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(intr_cause); i++) {
> +		int err;
> +
> +		if (!intr_cause[i].str)
> +			continue;
> +
> +		irq = irq_create_mapping(port->cpm_domain, i);
> +		if (WARN_ON(irq <= 0))

I'm not a huge fan of WARN_ON() inside "if" statements, but ... OK.

Why do these all need to be WARN_ON() instead of dev_warn()?

> +			return -ENXIO;
> +
> +		err = 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 = irq_create_mapping(port->cpm_domain,
> +					    XILINX_CPM_PCIE_INTR_INTX);
> +	if (WARN_ON(port->intx_irq <= 0))
> +		return -ENXIO;
> +
> +	/* Plug the INTx chained handler */
> +	irq_set_chained_handler_and_data(port->intx_irq,
> +					 xilinx_cpm_pcie_intx_flow, port);