Re: [PATCH v2 11/23] PCI: aardvark: Fix setting MSI address

[+cc Rob]

On Thu, Feb 24, 2022 at 01:59:01PM +0100, Pali Rohár wrote:

On Wednesday 23 February 2022 12:13:12 Bjorn Helgaas wrote:

quoted

On Fri, Feb 18, 2022 at 03:43:29PM +0100, Marek Behún wrote:

quoted

On Thu, 17 Feb 2022 11:14:52 -0600
Bjorn Helgaas [off-list ref] wrote:

quoted

quoted

+	phys_addr_t msi_addr;
 	u32 reg;
 	int i;
 

@@ -561,6 +561,11 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	reg |= LANE_COUNT_1;
 	advk_writel(pcie, reg, PCIE_CORE_CTRL0_REG);
 
+	/* Set MSI address */
+	msi_addr = virt_to_phys(pcie);  

Strictly speaking, msi_addr should be a pci_bus_addr_t, not a
phys_addr_t, and virt_to_phys() doesn't return a bus address.

On second thought, probably a dma_addr_t, not a pci_bus_addr_t.

quoted

quoted

the problem here is that as far as we know currently there is no
function that converts a virtual address to pci_bus_addr_t like
virt_to_phys() does to convert to phys_addr_t.

On Armada 3720 there are PCIe Controller Address Decoder Registers,
which such a translating function would need to consult to do the
translation. But the default settings of these registers is to map PCIe
addresses 1 to 1 to physical addresses, and no driver changes these
registers.

The poorly-named pcibios_resource_to_bus() (I think the name is my
fault) is the way to convert a CPU physical address to a PCI bus
address.

But here it is needed to do something different. It is needed to do
inverse mapping of function which converts PCI bus address to CPU
physical address of CPU memory. So to converting CPU physical address of
CPU memory to PCI bus address from PCI bus point of view.

I think that this information is stored in dma_ranges member of struct
pci_host_bridge. But function pcibios_resource_to_bus() looks at the
->windows member which converts CPU physical address of PCI memory (not
CPU memory) to PCI bus address, which is something different. So
pcibios_resource_to_bus() would not work here and it may return
incorrect values (as PCI memory may be different from CPU point of view
and PCI bus point of view).

Oh, sorry, indeed you are correct and I was completely on the wrong
track.  pcibios_resource_to_bus() is what you need for doing MMIO --
CPU accesses to things on PCI.

MSI is the reverse.  From the device's point of view, an MSI is
basically a DMA as you allude to above, so I would expect the DMA API
to be involved somehow.

I do see a couple drivers using the DMA API for this:

  struct pcie_port {
    dma_addr_t msi_data;
  };

  dw_pcie_host_init
    pp->msi_data = dma_map_single_attrs(..., &pp->msi_msg, ...)
    dw_pcie_setup_rc
      dw_pcie_msi_init
        u64 msi_target = (u64)pp->msi_data;
	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_LO, lower_32_bits(msi_target));

  dw_pci_setup_msi_msg
    u64 msi_target = (u64)pp->msi_data;
    msg->address_lo = lower_32_bits(msi_target);

  -----------------------------------------------------------

  struct tegra_msi {
    dma_addr_t phys;
  };

  tegra_pcie_probe
    tegra_pcie_msi_setup
      msi->virt = dma_alloc_attrs(..., &msi->phys, ...);

  tegra_pcie_enable_msi
    afi_writel(pcie, msi->phys, ...);

  tegra_compose_msi_msg
    msg->address_low = lower_32_bits(msi->phys);

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel