lkml.org 
[lkml]   [2023]   [Aug]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2023-08-21 22:14    [W:3.000 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site