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 <[email protected]>
> Signed-off-by: Bharat Kumar Gogada <[email protected]>
> ---
> 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
Hi Bjorn,
Thanks, ll take all your comments and update them in next patch.
Regards,
Thippeswamy H
> -----Original Message-----
> From: Bjorn Helgaas <[email protected]>
> Sent: Tuesday, August 22, 2023 1:44 AM
> To: Havalige, Thippeswamy <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Simek, Michal
> <[email protected]>; [email protected]; Gogada,
> Bharat Kumar <[email protected]>
> 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 <[email protected]>
> > Signed-off-by: Bharat Kumar Gogada <[email protected]>
> > ---
> > 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