Gustavo,

On 06/11/19 3:10 PM, Gustavo Pimentel wrote:

On Thu, Aug 29, 2019 at 6:13:18, Kishon Vijay Abraham I [off-list ref] 
wrote:

Hi, this email slip away from my attention...

quoted

Gustavo,

On 27/08/19 6:55 PM, Andrew Murray wrote:

quoted

On Sat, Aug 24, 2019 at 12:08:40AM +0000, Xiaowei Bao wrote:

quoted

quoted

-----Original Message-----
From: Andrew Murray <redacted>
Sent: 2019年8月23日 21:58
To: Xiaowei Bao <redacted>
Cc: bhelgaas@google.com; robh+dt@kernel.org; mark.rutland@arm.com;
shawnguo@kernel.org; Leo Li [off-list ref]; kishon@ti.com;
lorenzo.pieralisi@arm.co; arnd@arndb.de; gregkh@linuxfoundation.org; M.h.
Lian [off-list ref]; Mingkai Hu [off-list ref]; Roy
Zang [off-list ref]; jingoohan1@gmail.com;
gustavo.pimentel@synopsys.com; linux-pci@vger.kernel.org;
devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
linux-arm-kernel@lists.infradead.org; linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v2 07/10] PCI: layerscape: Modify the MSIX to the
doorbell way

On Thu, Aug 22, 2019 at 07:22:39PM +0800, Xiaowei Bao wrote:

quoted

The layerscape platform use the doorbell way to trigger MSIX interrupt
in EP mode.

I have no problems with this patch, however...

Are you able to add to this message a reason for why you are making this
change? Did dw_pcie_ep_raise_msix_irq not work when func_no != 0? Or did
it work yet dw_pcie_ep_raise_msix_irq_doorbell is more efficient?

The fact is that, this driver is verified in ls1046a platform of NXP before, and ls1046a don't
support MSIX feature, so I set the msix_capable of pci_epc_features struct is false,
but in other platform, e.g. ls1088a, it support the MSIX feature, I verified the MSIX
feature in ls1088a, it is not OK, so I changed to another way. Thanks.

Right, so the existing pci-layerscape-ep.c driver never supported MSIX yet it
erroneously had a switch case statement to call dw_pcie_ep_raise_msix_irq which
would never get used.

Now that we're adding a platform with MSIX support the existing
dw_pcie_ep_raise_msix_irq doesn't work (for this platform) so we are adding a
different method.

Gustavo, can you confirm dw_pcie_ep_raise_msix_irq() works for designware as it
didn't work for both me and Xiaowei?

When I implemented the dw_pcie_ep_raise_msix_irq(), the implementation 
was working quite fine on DesignWare solution. Otherwise, I wouldn't 
submit it to the kernel.
From what I have seen and if I recall well, Xiaowei implementation was 
done having PF's configurated on his solution, which is a configuration 
that I don't have in my solution, I believe this could be the missing 
piece that differs between our 2 implementations.

I haven't debugged the issue yet but in my understanding the MSI-X table should
be in the memory (DDR) of EP system. This table will be populated by RC while
configuring MSI-X (with msg address and msg data). The EP will use the
populated msg address and msg data for raising MSI-X interrupt.

From the dw_pcie_ep_raise_msix_irq() (copied below), nowhere the MSI-X table is
being read from the memory of EP system. I've given my comments below.

int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
			     u16 interrupt_num)
{
	.
	.
	reg = PCI_BASE_ADDRESS_0 + (4 * bir);
	bar_addr_upper = 0;
	bar_addr_lower = dw_pcie_readl_dbi(pci, reg);

BAR register will hold the "PCI address" programmed by the host. So
"bar_addr_lower" will have PCI address.

	reg_u64 = (bar_addr_lower & PCI_BASE_ADDRESS_MEM_TYPE_MASK);
	if (reg_u64 == PCI_BASE_ADDRESS_MEM_TYPE_64)
		bar_addr_upper = dw_pcie_readl_dbi(pci, reg + 4);

	tbl_addr = ((u64) bar_addr_upper) << 32 | bar_addr_lower;

The "tbl_addr" now has the PCI address programmed by the host.

	tbl_addr += (tbl_offset + ((interrupt_num - 1) * PCI_MSIX_ENTRY_SIZE));
	tbl_addr &= PCI_BASE_ADDRESS_MEM_MASK;

	msix_tbl = ioremap_nocache(ep->phys_base + tbl_addr,
				   PCI_MSIX_ENTRY_SIZE);

"ep->phys_base" will have EPs outbound memory address and "tbl_addr" will have
PCI address. So msix_tbl points to the EPs outbound memory region.
	if (!msix_tbl)
		return -EINVAL;

	msg_addr_lower = readl(msix_tbl + PCI_MSIX_ENTRY_LOWER_ADDR);
	msg_addr_upper = readl(msix_tbl + PCI_MSIX_ENTRY_UPPER_ADDR);

Here an access to the EP outbound region is made (and the transaction will be
based on ATU configuration).
The message address should ideally be obtained from the MSI-X table present in
the EP system. There need not be any access to the OB region for getting data
from MSI-X table.

	msg_addr = ((u64) msg_addr_upper) << 32 | msg_addr_lower;
	msg_data = readl(msix_tbl + PCI_MSIX_ENTRY_DATA);
	vec_ctrl = readl(msix_tbl + PCI_MSIX_ENTRY_VECTOR_CTRL);

All this should be obtained from the memory of EP.
	.
	.
}

I'm not sure how this worked for you.

Thanks
Kishon

Since patch submission into the kernel related to msix feature on pcitest 
tool, I didn't touch or re-tested the msix feature by lack of time (other 
projects requires my full attention for now). However is on my roadmap to 
came back to add some other features on DesignWare eDMA driver and I can 
do at that time some tests to see if the 
dw_pcie_ep_raise_msix_irq_doorbell() is compatible or not with my 
solution. If so, I can do some patch to simplify and use the 
dw_pcie_ep_raise_msix_irq_doorbell() if it still works as expected like 
on dw_pcie_ep_raise_msix_irq(). Agree?

Gustavo

quoted

Thanks
Kishon