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

From: Bjorn Helgaas <helgaas@kernel.org>
To: "Marek Behún" <kabel@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	pali@kernel.org, linux-pci@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 11/23] PCI: aardvark: Fix setting MSI address
Date: Wed, 23 Feb 2022 12:13:12 -0600	[thread overview]
Message-ID: <20220223181312.GA141319@bhelgaas> (raw)
In-Reply-To: <20220218154329.762831dd@thinkpad>

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@kernel.org> 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