Hi Niklas, Bjorn,
On 12/27/2023 5:27 PM, Niklas Cassel wrote:
Hello Bjorn,
On Tue, Dec 26, 2023 at 04:17:14PM -0600, Bjorn Helgaas wrote:
On Tue, Nov 28, 2023 at 02:22:30PM +0100, Niklas Cassel wrote:
From: Niklas Cassel <niklas.cassel@xxxxxxx>
Commit 6f5e193bfb55 ("PCI: dwc: Fix dw_pcie_ep_raise_msix_irq() to get
correct MSI-X table address") modified dw_pcie_ep_raise_msix_irq() to
support iATUs which require a specific alignment.
However, this support cannot have been properly tested.
The whole point is for the iATU to map an address that is aligned,
using dw_pcie_ep_map_addr(), and then let the writel() write to
ep->msi_mem + aligned_offset.
Thus, modify the address that is mapped such that it is aligned.
With this change, dw_pcie_ep_raise_msix_irq() matches the logic in
dw_pcie_ep_raise_msi_irq().
For the record, this patch is already queued up:
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=controller/dwc
Was there a problem report for this? Since 6f5e193bfb55 appeared in
v5.7 (May 31 2020), and this should affect imx6, keystone am654,
dw-pcie (platform), and keembay, it seems a little weird that this bug
persisted so long. Maybe nobody really uses endpoint support yet?
But I assume you observed a failure and tested this fix somewhere.
Yes, I verified it on rockchip rk3588.
I'm working on upstreaming rk3588 EP support:
https://github.com/floatious/linux/commits/rockchip-pcie-ep
Right now rk3588 only has support for RC in mainline.
The fix is only needed for platforms which:
1) supports MSI-X
2) has an iATU alignment requirement,
so where epc->mem->window.page_size != 0.
pci_epc_mem_init() calls pci_epc_multi_mem_init() which
initializes epc->mem->window.page_size with ep->page_size.
$ git grep page_size drivers/pci/controller/dwc/
So it will not affect pcie-designware-plat.c, nor pcie-keembay.c,
since they don't set any ep->page_size.
It will not affect pcie-tegra194.c, since it doesn't use
dw_pcie_ep_raise_msix_irq().
Looking at pci-imx6.c, imx6_pcie_ep_raise_irq() calls
dw_pcie_ep_raise_msix_irq(), but:
static const struct pci_epc_features imx8m_pcie_epc_features = {
.msix_capable = false,
}
so while pci-imx6.c will call dw_pcie_ep_raise_msix_irq(),
I assume that it will return early, in this if-statement:
https://github.com/torvalds/linux/blob/v6.7-rc7/drivers/pci/controller/dwc/pcie-designware-ep.c#L596-L598
That leaves just pci-keystone.c (am654 compatible only).
I don't know why no one has reported this bug before,
I can only assume insufficient testing.
The HW enforces the alignment so there was no issues observed before.
I guess you might be lucky and happen to get an address that is
aligned to the iATU alignment requirement, but that is unlikely
to happen when rebooting and running pcitest.sh multiple times.
In AM654, the HW keeps the lower bits of the target address as '0' in
the ATU, so the address in the ATU is always aligned.
"Table 12-2815. PCIE_EP_IATU_LWR_TARGET_ADDR_OFF_OUTBOUND_0 Register
Field Descriptions" in https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
describes the below
<quote>
- LWR_TARGET_RW[31:n] forms MSB's of the Lower
Target part of the new address of the translated region
- LWR_TARGET_RW[n-1:0] are not used [The start address must be
aligned to a CX_ATU_MIN_REGION_SIZE kB boundary, so the
lower bits of the start address of the new address of the translated
region [bits n-1:0] are always '0'] - n is
log2[CX_ATU_MIN_REGION_SIZE]
</quote>
Thanks,
Kishon
