Re: [PATCH v8 1/3] PCI: mediatek: Use actual physical address instead of virt_to_phys()

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, May 20, 2026 at 09:17:35PM +0200, Caleb James DeLisle wrote:
> 
> On 20/05/2026 20:55, Bjorn Helgaas wrote:
> > On Wed, May 20, 2026 at 06:38:25PM +0000, Caleb James DeLisle wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > 
> > > The driver previously used virt_to_phys() on the ioremapped register base
> > > (port->base) to compute the MSI message address. Using virt_to_phys() on an
> > > IO mapped address is incorrect because it expects a kernel virtual address.
> > > 
> > > To fix it, store the physical start of the I/O register region in
> > > mtk_pcie_port->phys_base and use it to build the MSI address. This replaces
> > > the incorrect virt_to_phys() usage and ensures MSI addresses are generated
> > > correctly.
> > > 
> > > Fixes: 43e6409db64d ("PCI: mediatek: Add MSI support for MT2712 and MT7622")
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > Tested-by: Caleb James DeLisle <cjd@cjdns.fr>
> > > ---
> > >   drivers/pci/controller/pcie-mediatek.c | 16 +++++++++++++---
> > >   1 file changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > > index 75722524fe74..c503fbd774d0 100644
> > > --- a/drivers/pci/controller/pcie-mediatek.c
> > > +++ b/drivers/pci/controller/pcie-mediatek.c
> > > @@ -175,6 +175,7 @@ struct mtk_pcie_soc {
> > >   /**
> > >    * struct mtk_pcie_port - PCIe port information
> > >    * @base: IO mapped register base
> > > + * @phys_base: Physical address of the I/O register base region
> > >    * @list: port list
> > >    * @pcie: pointer to PCIe host info
> > >    * @reset: pointer to port reset control
> > > @@ -196,6 +197,7 @@ struct mtk_pcie_soc {
> > >    */
> > >   struct mtk_pcie_port {
> > >   	void __iomem *base;
> > > +	phys_addr_t phys_base;
> > >   	struct list_head list;
> > >   	struct mtk_pcie *pcie;
> > >   	struct reset_control *reset;
> > > @@ -405,7 +407,7 @@ static void mtk_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> > >   	phys_addr_t addr;
> > >   	/* MT2712/MT7622 only support 32-bit MSI addresses */
> > > -	addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
> > > +	addr = port->phys_base + PCIE_MSI_VECTOR;
> >
> > This doesn't look right because the MSI address is a PCI bus address,
> > and port->phys_base is a CPU physical address.  Often a PCI bus
> > address is the same as the CPU physical address, but not always.
> > I think the DT 'ranges' property tells you the translation.

Oops, sorry, I muddied the waters here.

'ranges' tells you the translation applied by a bridge, e.g., when a
CPU does a load/store, the PCI host bridge turns it into a PCI
read/write transaction.  The bridge might add an offset to the CPU
load/store physical address to get the PCI read/write bus address.

But that's not the issue here.  The MSI is basically a DMA write
performed by the PCI device, not a store done by a CPU, so I don't
think 'ranges' is the right thing to look at.

Based on this:
https://elinux.org/Device_Tree_Usage#PCI_DMA_Address_Translation
I think 'dma-ranges' is the relevant property.  I don't think your DT
includes a 'dma-ranges' property, and in that case the default is that 
the system bus (CPU) address is the same as the PCI address.

So I think this patch works because it assumes DMA addresses like the
MSI address are mapped to identical system bus addresses.

It still seems to me that drivers should be prepared for the presence
of dma-ranges and use it when computing the MSI target address.  But I
don't think any drivers really do that, so for now I think you should
pretend that I never responded about this patch.

> This is all still a little over my head here, but my understanding was that
> this is in the middle of the device's register map because the DT has the
> following:
> 
> reg = <0x1fb83000 0x1000>;
> reg-names = "port1";
> 
> Per the manual, that offset (base + 0xc0) is in undocumented area but it's
> in the registers.
> 
> The PCI memory is 0x20000000 - 0x2fffffff and we split it between the two
> devices. Here's the one using the upper half:
> 
>         ranges = <0x81000000 0 0x00000000 0x1f608000 0 0x00008000>,  (IO)
> 
>              <0x82000000 0 0x28000000 0x28000000 0 0x08000000>;      (MEM)
> 
> Hope I'm adding something useful here... Let me know if you want me to get
> or test anything else.

Obviously it's over my head too, so I'm sorry I confused the
situation.

> > >   	msg->address_hi = 0;
> > >   	msg->address_lo = lower_32_bits(addr);
> > > @@ -520,7 +522,7 @@ static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
> > >   	u32 val;
> > >   	phys_addr_t msg_addr;
> > > -	msg_addr = virt_to_phys(port->base + PCIE_MSI_VECTOR);
> > > +	msg_addr = port->phys_base + PCIE_MSI_VECTOR;
> > >   	val = lower_32_bits(msg_addr);
> > >   	writel(val, port->base + PCIE_IMSI_ADDR);
> > > @@ -953,6 +955,7 @@ static int mtk_pcie_parse_port(struct mtk_pcie *pcie,
> > >   	struct mtk_pcie_port *port;
> > >   	struct device *dev = pcie->dev;
> > >   	struct platform_device *pdev = to_platform_device(dev);
> > > +	struct resource *res;
> > >   	char name[20];
> > >   	int err;
> > > @@ -961,7 +964,14 @@ static int mtk_pcie_parse_port(struct mtk_pcie *pcie,
> > >   		return -ENOMEM;
> > >   	snprintf(name, sizeof(name), "port%d", slot);
> > > -	port->base = devm_platform_ioremap_resource_byname(pdev, name);
> > > +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
> > > +	if (!res) {
> > > +		dev_err(dev, "failed to get port%d base\n", slot);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	port->phys_base = res->start;
> > > +	port->base = devm_ioremap_resource(&pdev->dev, res);
> > >   	if (IS_ERR(port->base)) {
> > >   		dev_err(dev, "failed to map port%d base\n", slot);
> > >   		return PTR_ERR(port->base);
> > > -- 
> > > 2.39.5
> > >