RE: [PATCH v6 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver

Hi Bjorn,

-----Original Message-----
From: Havalige, Thippeswamy
Sent: Monday, August 28, 2023 2:39 PM
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
pci@vger.kernel.org; devicetree@vger.kernel.org; bhelgaas@google.com;
lpieralisi@kernel.org; robh@kernel.org; kw@linux.com; Simek, Michal
[off-list ref]; krzysztof.kozlowski+dt@linaro.org; Gogada,
Bharat Kumar [off-list ref]
Subject: RE: [PATCH v6 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver

Hi Bjorn,

Thanks, ll take all your comments and update them in next patch.

Regards,
Thippeswamy H

quoted

-----Original Message-----
From: Bjorn Helgaas <helgaas@kernel.org>
Sent: Tuesday, August 22, 2023 1:44 AM
To: Havalige, Thippeswamy <redacted>
Cc: linux-arm-kernel@lists.infradead.org;
linux-kernel@vger.kernel.org; linux- pci@vger.kernel.org;
devicetree@vger.kernel.org; bhelgaas@google.com;
lpieralisi@kernel.org; robh@kernel.org; kw@linux.com; Simek, Michal
[off-list ref]; krzysztof.kozlowski+dt@linaro.org; Gogada,
Bharat Kumar [off-list ref]
Subject: Re: [PATCH v6 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root
Port driver

On Fri, Aug 18, 2023 at 03:05:07PM +0530, Thippeswamy Havalige wrote:

quoted

Add support for Xilinx XDMA Soft IP core as Root Port.

The Zynq UltraScale+ MPSoCs devices support XDMA soft IP module in
programmable logic.

The integrated XDMA soft IP block has integrated bridge function
that can act as PCIe Root Port.

Signed-off-by: Thippeswamy Havalige <redacted>
Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@amd.com>
---
changes in v6:
- Replaced chained irq's with regular interrupts.

Thanks a million for working this out!

Trivial comments below, wait a couple days before reposting in case
there are other comments.

quoted

+static inline bool xilinx_pl_dma_pcie_link_up(struct pl_dma_pcie
+*port) {
+	return (pcie_read(port, XILINX_PCIE_DMA_REG_PSCR) &
+		XILINX_PCIE_DMA_REG_PSCR_LNKUP) ? 1 : 0;

This function returns bool, so I think true/false would be more
appropriate than 1/0.

quoted

+static bool xilinx_pl_dma_pcie_valid_device(struct pci_bus *bus,
+					    unsigned int devfn)
+{
+	struct pl_dma_pcie *port = bus->sysdata;
+
+	/* Check if link is up when trying to access downstream ports */
+	if (!pci_is_root_bus(bus)) {
+		/*
+		 * Checking whether link is up here is a last line of defence,
+		 * if the link goes down after we check for link-up, we have a
+		 * problem: if a PIO request is initiated while link-down, the
+		 * whole controller hangs, and even after link comes up again,
+		 * previous PIO requests won't work, and a reset of the whole
+		 * PCIe controller is needed. Henceforth we need link-up

check

quoted

+		 * here to avoid sending PIO request when link is down. This
+		 * check is racy by definition and does not make controller

hang

quoted

+		 * if the link goes down after this check is performed.

This comment doesn't make sense to me.  "If PIO request initiated
while link- down, controller hangs ... This check is racy and does not
make controller hang if link goes down."  Which is it?

- Here checking link up treats device as invalid.

Please find comment that I ll update in next patch and 
Please letme know if any changes are needed.

  /*
                 * Checking whether the link is up. Here is the last line of
                 * defence. If the link goes down after we check for link-up,
                 * we have a problem. If a PIO request is initiated while link
                 * is down, the whole controller hangs. Even after link comes up
                 * again, previous PIO requests won't work, and a reset of the
                 * whole PCIe controller is needed. Henceforth we need link-up
                 * check here to treat device as invalid and avoid sending PIO
                 * request when link is down and this check is inherently racy
                 * by definition.
*/

quoted

My *guess* is that this check narrows the window but doesn't close it,
so if
xilinx_pl_dma_pcie_link_up() finds the link up, but the link goes down
before
pci_generic_config_read() initiates the PIO request, the controller
hangs, and a reset is required.

quoted

+		 */
+		if (!xilinx_pl_dma_pcie_link_up(port))
+			return false;
+	} else if (devfn > 0)
+		/* Only one device down on each root port */
+		return false;
+
+	return true;
+}

quoted

+/* INTx error interrupts are Xilinx controller specific interrupt,
+used to
+ * notify user about error's such as cfg timeout, slave unsupported
+requests,

s/error's/errors/

quoted

+ * fatal and non fatal error etc.

quoted

+		err = devm_request_irq(dev, irq,

xilinx_pl_dma_pcie_intr_handler,

quoted

+				       IRQF_SHARED | IRQF_NO_THREAD,

intr_cause[i].sym, port);

Rewrap to fit in 80 columns.

quoted

+	/* Needed for MSI DECODE MODE */
+	pcie_write(port, XILINX_PCIE_DMA_IDR_ALL_MASK,

XILINX_PCIE_DMA_REG_MSI_LOW_MASK);

quoted

+	pcie_write(port, XILINX_PCIE_DMA_IDR_ALL_MASK,
+XILINX_PCIE_DMA_REG_MSI_HI_MASK);

Rewrap.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel