On Fri, Feb 18, 2022 at 03:43:29PM +0100, Marek Behún wrote: > On Thu, 17 Feb 2022 11:14:52 -0600 > Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > > > + 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. > > 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. This is implemented in terms of the host bridge windows and the translation offset in struct resource_entry, which should be set up via the pci_add_resource_offset() called from devm_of_pci_get_host_bridge_resources(). > Pali says that other drivers also use phys_addr_t, and most hardware > maps 1 to 1 by default. Yes. I think they're all technically incorrect. Most systems do map CPU phys == PCI bus, but I point it out because it's a case where copying that pattern to new drivers will eventually bite us. > So we think that until at least an API for such a function exists, we > shuld leave it as it is. I am not against converting the phys_addr_to > to a pci_bus_addr_t, but Pali thinks that for now we should leave even > that as it is, because the virt_to_phys() function returns phys_addr_t. > > We can add a comment there explaining this, if you want. > > What do you think? > > Marek
