Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Wed, Oct 27, 2021 at 04:23:07PM +0200, Pali Rohár wrote:
> On Wednesday 27 October 2021 15:12:46 Lorenzo Pieralisi wrote:
> > On Tue, Oct 12, 2021 at 06:41:41PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <pali@kernel.org>
> > > 
> > > According to PCI 3.0 specification, sending both MSI and MSI-X interrupts
> > > is done by DWORD memory write operation to doorbell message address. The
> > > write operation for MSI has zero upper 16 bits and the MSI interrupt number
> > > in the lower 16 bits, while the write operation for MSI-X contains a 32-bit
> > > value from MSI-X table.
> > > 
> > > Since the driver only uses interrupt numbers from range 0..31, the upper
> > > 16 bits of the DWORD memory write operation to doorbell message address
> > > are zero even for MSI-X interrupts. Thus we can enable MSI-X interrupts.
> > 
> > It is the controller driver that defines the MSI-X data field yes, what
> > I don't get is why we have to add this comment in the commit log.
> > 
> > Basically Aardvark can support MSI-X up to 32 MSI-X vectors and you
> > are enabling them with this patch.
> > 
> > Is there anything *else* I am missing wrt 16-bit/32-bit data fields
> > that we need to know ?
> > 
> > > Testing proves that kernel can correctly receive MSI-X interrupts from
> > > PCIe cards which supports both MSI and MSI-X interrupts.
> > 
> > I don't understand what you want to convey with this commit log.
> > 
> > To me, the whole comment does not add anything (if I understood it),
> > please let me know what you want to express with it.
> > 
> > To me this patch enables MSI-X support because the HW can support them,
> > that's it.
> 
> My understanding is that MSI-X by definition uses 32-bit write
> operations to doorbell address and so, HW needs to support catching of
> 32-bit write operations.
> 
> Aardvark hw seems to support only 16-bit write operation to doorbell
> address. But our testing proved that hw can catch also lower 16-bits of
> 32-bit write operation to doorbell address.
> 
> So if driver enforces that every 32-bit write operation to doorbell
> address would have upper 16-bit zeroed then MSI-X should work.

That's clearer than the current commit log.

> In commit message I originally tried to explain it that after applying
> all previous patches which are fixing MSI and Multi-MSI support (part of
> them is enforcement to use only MSI numbers 0..31), it makes driver
> compatible with also MSI-X interrupts.
> 
> If you want to rewrite commit message, let us know, there is no problem.

I think we should.

> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Reviewed-by: Marek Behún <kabel@kernel.org>

By the way, this tag should be removed. Marek signed it off, that
applies to other patches in this series as well.

Lorenzo

> > > Signed-off-by: Marek Behún <kabel@kernel.org>
> > > ---
> > >  drivers/pci/controller/pci-aardvark.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > index b703b271c6b1..337b61508799 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
> > >  
> > >  	msi_di = &pcie->msi_domain_info;
> > >  	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > > -		MSI_FLAG_MULTI_PCI_MSI;
> > > +			MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
> > >  	msi_di->chip = msi_ic;
> > >  
> > >  	pcie->msi_inner_domain =
> > > -- 
> > > 2.32.0
> > >