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

[Havalige, Thippeswamy]

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