Hi Bjorn, > -----Original Message----- > From: Bjorn Helgaas <helgaas@xxxxxxxxxx> > Sent: Friday, July 28, 2023 4:58 AM > To: Havalige, Thippeswamy <thippeswamy.havalige@xxxxxxx> > Cc: krzysztof.kozlowski@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux- > pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; > bhelgaas@xxxxxxxxxx; lorenzo.pieralisi@xxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; Gogada, Bharat Kumar > <bharat.kumar.gogada@xxxxxxx>; Simek, Michal > <michal.simek@xxxxxxx>; Thomas Gleixner <tglx@xxxxxxxxxxxxx> > Subject: Re: [PATCH V5 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver > > On Mon, Jul 24, 2023 at 06:40:58AM +0000, Havalige, Thippeswamy wrote: > > > From: Bjorn Helgaas <helgaas@xxxxxxxxxx> On Thu, Jul 20, 2023 at > > > 06:37:03AM +0000, Havalige, Thippeswamy wrote: > > > > > From: Bjorn Helgaas <helgaas@xxxxxxxxxx> ... > > > > > On Wed, Jun 28, 2023 at 02:58:12PM +0530, Thippeswamy Havalige > wrote: > > > > > > Add support for Xilinx XDMA Soft IP core as Root Port. > > > > > > ... > > > > If you have more detail about the "error interrupt," that would be > > > useful as well. Does this refer to an AER interrupt, a "System > > > Error", something else? I'm looking at the diagram in PCIe r6.0, > > > Figure 6-3, wondering if this is related to anything there. I > > > suppose likely it's some Xilinx-specific thing? > > - Agreed, ll modify Legacy to INTx, and regarding error interrupts > > these are Xilinx controller specific interrupts which are used to > > notify the user about errors such as cfg timeout, slave unsupported > > requests,Fatal and non fatal error. > > This would be great material for comments and/or a revised commit log. - Agreed, I'll add this as a comment. > > > > > > + /* Plug the INTx chained handler */ > > > > > > + irq_set_chained_handler_and_data(port->intx_irq, > > > > > > + > xilinx_pl_dma_pcie_intx_flow, port); > > > > > > + > > > > > > + /* Plug the main event chained handler */ > > > > > > + irq_set_chained_handler_and_data(port->irq, > > > > > > + > xilinx_pl_dma_pcie_event_flow, > > > > > port); > > > > > > > > > > What's the reason for using chained IRQs? Can this be done > > > > > without them? I don't claim to understand all the issues here, > > > > > but it seems better to avoid chained IRQ handlers when possible: > > > > > https://lore.kernel.org/all/877csohcll.ffs@tglx/ > > > > > > > - As per the comments in this > > > > https://lkml.kernel.org/lkml/alpine.DEB.2.20.1705232307330.2409@na > > > > nos/ T/ "It is fine to have chained interrupts when bootloader, > > > > device tree and kernel under control. Only if BIOS/UEFI comes into > > > > play the user is helpless against interrupt storm which will cause > > > > system to hangs." > > > > > > > > We are using ARM embedded platform with Bootloader, Devicetree > flow. > > > > > > I read Thomas' comments as "in general it's better to use regular > > > interrupts, but we can live with chained interrupts if we have > > > control of bootloader, device tree, and kernel." > > > > > > I guess my questions are more like: > > > > > > - Could this be done with either chained interrupts or regular > > > interrupts? > > > - If so, what is the advantage to using chained interrupts? > > > With regular interrupts, these interrupts are self-consumed interrupts > > (interrupt is handled within driver) but where as chained interrupts > > are not self consumed (interrupts are not handled within the driver, > > but forwarded to different driver for which the actual interrupt is > > raised) but these interrupts are demultiplexed and forwards interrupt > > to another subsystem by calling generic_handle_irq(). > > > > As, MSI generic handlers are consumed by Endpoints and end point > > drivers, chained handlers forward the interrupt to the specific EP > > driver (For example NVME subsystem or any other subsystem). > > This doesn't really explain it for me, probably because of my IRQ ignorance. > > I compared xilinx_pl_dma (which uses chained interrupts) with pci-aardvark.c > (which does not). > > - xilinx_pl_dma_pcie_setup_irq() calls platform_get_irq(0) once and > sets up xilinx_pl_dma_pcie_event_flow() as the handler. > > - advk_pcie_probe() calls platform_get_irq(0) once and sets up > advk_pcie_irq_handler() as the handler. > > - xilinx_pl_dma_pcie_event_flow() reads XILINX_PCIE_DMA_REG_IDR to > learn which interrupts are pending and calls > generic_handle_domain_irq() for each. > > - advk_pcie_irq_handler() calls advk_pcie_handle_int(), which reads > PCIE_ISR0_REG and PCIE_ISR1_REG to learn which interrupts are > pending and calls generic_handle_domain_irq() for each. > > It seems like both drivers do essentially the same thing, but > xilinx_pl_dma_pcie_event_flow() is a chained handler and > advk_pcie_irq_handler() is not. > > Is there some underlying difference in the way the hardware works that > means xilinx_pl_dma needs a chained handler while aardvark does not? - Hardware doesn't need to have only chained IRQ, we will try to implement this feature with regular IRQ handlers and try to validate. > Bjorn
