Messages in this thread | | | Date | Mon, 21 Aug 2023 15:14:00 -0500 | From | Bjorn Helgaas <> | 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: > 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 <thippeswamy.havalige@amd.com> > 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.
> +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.
> +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 > + * here to avoid sending PIO request when link is down. This > + * check is racy by definition and does not make controller hang > + * 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?
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.
> + */ > + 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; > +}
> +/* 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/
> + * fatal and non fatal error etc.
> + err = devm_request_irq(dev, irq, xilinx_pl_dma_pcie_intr_handler, > + IRQF_SHARED | IRQF_NO_THREAD, intr_cause[i].sym, port);
Rewrap to fit in 80 columns.
> + /* Needed for MSI DECODE MODE */ > + pcie_write(port, XILINX_PCIE_DMA_IDR_ALL_MASK, XILINX_PCIE_DMA_REG_MSI_LOW_MASK); > + pcie_write(port, XILINX_PCIE_DMA_IDR_ALL_MASK, XILINX_PCIE_DMA_REG_MSI_HI_MASK);
Rewrap.
Bjorn
| |