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@xxxxxxxxxx> > > > > > > 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@xxxxxxxxxx> > > > Reviewed-by: Marek Behún <kabel@xxxxxxxxxx> 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@xxxxxxxxxx> > > > --- > > > 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 > > >
