[PATCH v8 0/3] Add EcoNet EN7528 (and EN751221) PCIe support.

[PATCH v8 0/3] Add EcoNet EN7528 (and EN751221) PCIe support.

[Caleb James DeLisle]

Tested on SmartFiber XP8421-B (EN751221)

Changes from v7:
* mtk_pcie_retrain retrain all root ports not just first
* Include fix from Manivannan Sadhasivam, wrong usage of virt_to_phys()

Changes from v6:
* s/reset/resets/ in .yaml
* s/re-train/retrain/g
* s/Root bridge/Root port/
* If module not builtin, log at mtk_pcie_startup_port_en7528()
* Do not fail if error in mtk_pcie_retrain()
* v6: https://lore.kernel.org/linux-mips/20260513191652.3200607-1-cjd@cjdns.fr

Changes from v5:
* s/errno-base.h/errno.h/
* Breakout mtk_pcie_retrain() into a function
* Use for_each_pci_bridge() to find root bridge
* v5: https://lore.kernel.org/linux-mips/20260413140339.16238-1-cjd@cjdns.fr/

Changes from v4:
* Fixed missing Acked-by
* Rebased to commit 66672af7a095 ("Add linux-next specific files for 20260410")
* v4: https://lore.kernel.org/linux-mips/20260404182854.2183651-1-cjd@cjdns.fr/

Changes from v3:
* s/initiallized/initialized/
* Use PCIE_T_PVPERL_MS for sleep time
* Use PCI_PM_D3COLD_WAIT for startup wait time
* Clarify comment "Activate INTx interrupts"
* Add MTK_PCIE_RETRAIN quirk for devices which require link re-train
* Do not retrain *all* bridges, only root bridge
* Better comments and logging in retraining logic
* v3: https://lore.kernel.org/linux-mips/20260320094212.696671-1-cjd@cjdns.fr/

Changes from v2:
* mediatek-pcie.yaml -> s/power-domain/power-domains/ and drop example
* Patch 3 dropped as it has been applied (Thanks!)
* v2: https://lore.kernel.org/linux-mips/20260316155157.679533-1-cjd@cjdns.fr/

Changes from v1:
* mediatek-pcie.yaml slot0 needs device-type = "pci", fix dt_binding_check
Link: https://lore.kernel.org/linux-mips/177334026016.3889069.9474337544951486443.robh@kernel.org
* v1: https://lore.kernel.org/linux-mips/20260312165332.569772-1-cjd@cjdns.fr/

This was split from a larger PCIe patchset which crossed multiple
subsystems. I'm not labeling this a v3 because it's a new patchset, but
I'm keeping the historical record anyway.

Changes from econet-pcie v2:
* mediatek-pcie.yaml add missing constraints to PCI node properties
* econet-pcie v2: https://lore.kernel.org/linux-mips/20260309131818.74467-1-cjd@cjdns.fr

Changes from econet-pcie v1:
* pcie-mediatek.c Exclude pcie_retrain_link() when building as a module
* econet-pcie v1: https://lore.kernel.org/linux-mips/20260303190948.694783-1-cjd@cjdns.fr/


Caleb James DeLisle (2):
  dt-bindings: PCI: mediatek: Add support for EcoNet EN7528
  PCI: mediatek: Add support for EcoNet EN7528 SoC

Manivannan Sadhasivam (1):
  PCI: mediatek: Use actual physical address instead of virt_to_phys()

 .../bindings/pci/mediatek-pcie.yaml           |  26 +++
 drivers/pci/controller/Kconfig                |   2 +-
 drivers/pci/controller/pcie-mediatek.c        | 168 +++++++++++++++++-
 3 files changed, 192 insertions(+), 4 deletions(-)


base-commit: 687da68900cd1a46549f7d9430c7d40346cb86a0
-- 
2.39.5

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

[Caleb James DeLisle]

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;
 	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

[PATCH v8 2/3] dt-bindings: PCI: mediatek: Add support for EcoNet EN7528

[Caleb James DeLisle]

Introduce EcoNet EN7528 SoC compatible in MediaTek PCIe controller
binding.

EcoNet PCIe controller has the same configuration model as
Mediatek v2 but is initialized more similarly to an MT7621
PCIe.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/pci/mediatek-pcie.yaml           | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie.yaml b/Documentation/devicetree/bindings/pci/mediatek-pcie.yaml
index 0b8c78ec4f91..c009a7a52bc6 100644
--- a/Documentation/devicetree/bindings/pci/mediatek-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/mediatek-pcie.yaml
@@ -14,6 +14,7 @@ properties:
     oneOf:
       - enum:
           - airoha,an7583-pcie
+          - econet,en7528-pcie
           - mediatek,mt2712-pcie
           - mediatek,mt7622-pcie
           - mediatek,mt7629-pcie
@@ -226,6 +227,31 @@ allOf:
 
         mediatek,pbus-csr: false
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: econet,en7528-pcie
+    then:
+      properties:
+        clocks:
+          maxItems: 1
+
+        clock-names:
+          maxItems: 1
+
+        resets: false
+
+        reset-names: false
+
+        power-domains: false
+
+        mediatek,pbus-csr: false
+
+      required:
+        - phys
+        - phy-names
+
 unevaluatedProperties: false
 
 examples:
-- 
2.39.5

[PATCH v8 3/3] PCI: mediatek: Add support for EcoNet EN7528 SoC

[Caleb James DeLisle]

Add support for the PCIe present on the EcoNet EN7528 (and EN751221) SoCs.

These SoCs have a mix of Gen1 and Gen2 capable ports, but the Gen2 ports
require re-training after startup.

Co-developed-by: Ahmed Naseef <naseefkm@gmail.com>
Signed-off-by: Ahmed Naseef <naseefkm@gmail.com>
Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/pci/controller/Kconfig         |   2 +-
 drivers/pci/controller/pcie-mediatek.c | 152 +++++++++++++++++++++++++
 2 files changed, 153 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig
index 2247709ef6d6..8a3a31b2bc12 100644
--- a/drivers/pci/controller/Kconfig
+++ b/drivers/pci/controller/Kconfig
@@ -209,7 +209,7 @@ config PCI_MVEBU
 
 config PCIE_MEDIATEK
 	tristate "MediaTek PCIe controller"
-	depends on ARCH_AIROHA || ARCH_MEDIATEK || COMPILE_TEST
+	depends on ARCH_AIROHA || ARCH_MEDIATEK || ECONET || COMPILE_TEST
 	depends on OF
 	depends on PCI_MSI
 	select IRQ_MSI_LIB
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index c503fbd774d0..4e4d4b1559f7 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -9,11 +9,13 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/errno.h>
 #include <linux/iopoll.h>
 #include <linux/irq.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/irqchip/irq-msi-lib.h>
 #include <linux/irqdomain.h>
+#include <linux/kconfig.h>
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
 #include <linux/msi.h>
@@ -77,6 +79,7 @@
 
 #define PCIE_CONF_VEND_ID	0x100
 #define PCIE_CONF_DEVICE_ID	0x102
+#define PCIE_CONF_REV_CLASS	0x104
 #define PCIE_CONF_CLASS_ID	0x106
 
 #define PCIE_INT_MASK		0x420
@@ -89,6 +92,11 @@
 #define MSI_MASK		BIT(23)
 #define MTK_MSI_IRQS_NUM	32
 
+#define EN7528_HOST_MODE	0x00804201
+#define EN7528_LINKUP_REG	0x50
+#define EN7528_RC0_LINKUP	BIT(1)
+#define EN7528_RC1_LINKUP	BIT(2)
+
 #define PCIE_AHB_TRANS_BASE0_L	0x438
 #define PCIE_AHB_TRANS_BASE0_H	0x43c
 #define AHB2PCIE_SIZE(x)	((x) & GENMASK(4, 0))
@@ -148,12 +156,15 @@ struct mtk_pcie_port;
  * @MTK_PCIE_FIX_DEVICE_ID: host's device ID needed to be fixed
  * @MTK_PCIE_NO_MSI: Bridge has no MSI support, and relies on an external block
  * @MTK_PCIE_SKIP_RSTB: Skip calling RSTB bits on PCIe probe
+ * @MTK_PCIE_RETRAIN: Retrain link to bridge after startup because some
+ *                    Gen2-capable devices start as Gen1.
  */
 enum mtk_pcie_quirks {
 	MTK_PCIE_FIX_CLASS_ID = BIT(0),
 	MTK_PCIE_FIX_DEVICE_ID = BIT(1),
 	MTK_PCIE_NO_MSI = BIT(2),
 	MTK_PCIE_SKIP_RSTB = BIT(3),
+	MTK_PCIE_RETRAIN = BIT(4),
 };
 
 /**
@@ -755,6 +766,132 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
 	return 0;
 }
 
+static int mtk_pcie_startup_port_en7528(struct mtk_pcie_port *port)
+{
+	struct mtk_pcie *pcie = port->pcie;
+	struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie);
+	struct resource *mem = NULL;
+	struct resource_entry *entry;
+	u32 val, link_mask;
+	int err;
+
+	entry = resource_list_first_type(&host->windows, IORESOURCE_MEM);
+	if (entry)
+		mem = entry->res;
+	if (!mem)
+		return -EINVAL;
+
+	if (!pcie->cfg) {
+		dev_err(pcie->dev, "EN7528: pciecfg syscon not available\n");
+		return -EINVAL;
+	}
+
+	/* Assert all reset signals */
+	writel(0, port->base + PCIE_RST_CTRL);
+
+	/*
+	 * Enable PCIe link down reset, if link status changed from link up to
+	 * link down, this will reset MAC control registers and configuration
+	 * space.
+	 */
+	writel(PCIE_LINKDOWN_RST_EN, port->base + PCIE_RST_CTRL);
+
+	msleep(PCIE_T_PVPERL_MS);
+
+	/* De-assert PHY, PE, PIPE, MAC and configuration reset */
+	val = readl(port->base + PCIE_RST_CTRL);
+	val |= PCIE_PHY_RSTB | PCIE_PERSTB | PCIE_PIPE_SRSTB |
+	       PCIE_MAC_SRSTB | PCIE_CRSTB;
+	writel(val, port->base + PCIE_RST_CTRL);
+
+	writel(PCIE_CLASS_CODE | PCIE_REVISION_ID,
+	       port->base + PCIE_CONF_REV_CLASS);
+	writel(EN7528_HOST_MODE, port->base);
+
+	link_mask = (port->slot == 0) ? EN7528_RC0_LINKUP : EN7528_RC1_LINKUP;
+
+	/* 100ms timeout value should be enough for Gen1/2 training */
+	err = regmap_read_poll_timeout(pcie->cfg, EN7528_LINKUP_REG, val,
+				       !!(val & link_mask), 20,
+				       PCI_PM_D3COLD_WAIT * USEC_PER_MSEC);
+	if (err) {
+		dev_err(pcie->dev, "EN7528: port%d link timeout\n", port->slot);
+		return -ETIMEDOUT;
+	}
+
+	/* Activate INTx interrupts */
+	val = readl(port->base + PCIE_INT_MASK);
+	val &= ~INTX_MASK;
+	writel(val, port->base + PCIE_INT_MASK);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		mtk_pcie_enable_msi(port);
+
+	/* Set AHB to PCIe translation windows */
+	val = lower_32_bits(mem->start) |
+	      AHB2PCIE_SIZE(fls(resource_size(mem)));
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
+
+	val = upper_32_bits(mem->start);
+	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
+
+	writel(WIN_ENABLE, port->base + PCIE_AXI_WINDOW0);
+
+	if (!IS_BUILTIN(CONFIG_PCIE_MEDIATEK))
+		dev_info(pcie->dev,
+			 "module not built-in, Gen2 unavailable even if supported\n");
+
+	return 0;
+}
+
+/**
+ * mtk_pcie_retrain - retrain the root bridge link if needed
+ * @dev: The device, for use in logging
+ * @host: The host bridge which contains the link
+ *
+ * Due to what is likely a hardware bug, some devices (notably EcoNet) start up
+ * as Gen1, and must be retrained once after initial configuration in order to
+ * reach Gen2.
+ *
+ * These devices always self-identify as Gen2 capable, but sometimes the PHY is
+ * only capable of Gen1 operation, and sometimes the PCIe card (e.g. wifi) is
+ * only Gen1 capable. Therefore it is most convenient to retrain every port
+ * after startup.
+ */
+static int mtk_pcie_retrain(struct device *dev, struct pci_host_bridge *host)
+{
+	struct pci_dev *rp;
+	int ret = -ENOENT;
+	u16 lnksta = 0;
+	u32 speed;
+
+	/* Should already have been warned about during startup_port */
+	if (!IS_BUILTIN(CONFIG_PCIE_MEDIATEK))
+		return 0;
+
+	for_each_pci_bridge(rp, host->bus) {
+		if (pci_pcie_type(rp) != PCI_EXP_TYPE_ROOT_PORT)
+			continue;
+
+#if IS_BUILTIN(CONFIG_PCIE_MEDIATEK)
+		ret = pcie_retrain_link(rp, true);
+#endif
+
+		if (ret)
+			return dev_err_probe(&rp->dev, ret,
+					     "failed to retrain port\n");
+
+		pcie_capability_read_word(rp, PCI_EXP_LNKSTA, &lnksta);
+		speed = lnksta & PCI_EXP_LNKSTA_CLS;
+
+		pci_info(rp, "link retrained, speed %s\n",
+			 pci_speed_string(pcie_link_speed[speed]));
+
+	}
+
+	return 0;
+}
+
 static void __iomem *mtk_pcie_map_bus(struct pci_bus *bus,
 				      unsigned int devfn, int where)
 {
@@ -1159,6 +1296,13 @@ static int mtk_pcie_probe(struct platform_device *pdev)
 	if (err)
 		goto put_resources;
 
+	/*
+	 * Ignore error because pci_host_probe() was already called, and in any
+	 * case it is possible that the port will still work as Gen1.
+	 */
+	if (pcie->soc->quirks & MTK_PCIE_RETRAIN)
+		mtk_pcie_retrain(dev, host);
+
 	return 0;
 
 put_resources:
@@ -1274,8 +1418,16 @@ static const struct mtk_pcie_soc mtk_pcie_soc_mt7629 = {
 	.quirks = MTK_PCIE_FIX_CLASS_ID | MTK_PCIE_FIX_DEVICE_ID,
 };
 
+static const struct mtk_pcie_soc mtk_pcie_soc_en7528 = {
+	.ops = &mtk_pcie_ops_v2,
+	.startup = mtk_pcie_startup_port_en7528,
+	.setup_irq = mtk_pcie_setup_irq,
+	.quirks = MTK_PCIE_RETRAIN,
+};
+
 static const struct of_device_id mtk_pcie_ids[] = {
 	{ .compatible = "airoha,an7583-pcie", .data = &mtk_pcie_soc_an7583 },
+	{ .compatible = "econet,en7528-pcie", .data = &mtk_pcie_soc_en7528 },
 	{ .compatible = "mediatek,mt2701-pcie", .data = &mtk_pcie_soc_v1 },
 	{ .compatible = "mediatek,mt7623-pcie", .data = &mtk_pcie_soc_v1 },
 	{ .compatible = "mediatek,mt2712-pcie", .data = &mtk_pcie_soc_mt2712 },
-- 
2.39.5

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

[Bjorn Helgaas]

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.

>  	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
> 

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

[Caleb James DeLisle]

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.


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.

Thanks,

Caleb


>>   	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
>>

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

[Bjorn Helgaas]

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
> > > 

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

[Manivannan Sadhasivam]

On Wed, May 20, 2026 at 02:59:00PM -0500, Bjorn Helgaas wrote:
> 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.
> 

Yeah, it is so easy to confuse both. To summarise, 'ranges' describes the
outbound translation and 'dma-ranges' describes the inbound translation from
host perspective.

> 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.
> 

Your observations are correct. This driver assumes that the identical mapping
exists between CPU and PCI bus addresses. Usually, the drivers make use of
phys_to_dma() to handle the translations. This API internally makes use of the
'dma_range_map' which gets populated by the OF core based on the 'dma-ranges'
property (if present in DT).

But it makes sense to use it irrespective of whether the platform supports
non-identical DMA/inbound translation or not. Since this API behaves like a
no-op and returns the CPU physical address if there is an identical mapping,
there is literally zero overhead in using it.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Bjorn Helgaas]

On Thu, May 21, 2026 at 10:44:51AM +0530, Manivannan Sadhasivam wrote:
> On Wed, May 20, 2026 at 02:59:00PM -0500, Bjorn Helgaas wrote:
> > 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.
> 
> Yeah, it is so easy to confuse both. To summarise, 'ranges'
> describes the outbound translation and 'dma-ranges' describes the
> inbound translation from host perspective.
> 
> > 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.
> 
> Your observations are correct. This driver assumes that the
> identical mapping exists between CPU and PCI bus addresses. Usually,
> the drivers make use of phys_to_dma() to handle the translations.

What does this look like in the native host bridge drivers?  I don't
see any direct calls of phys_to_dma(), but there are some higher-level
interfaces that use it.

I don't really see a consistent style of constructing MSI addresses,
e.g., in *_compose_msi_msg() implementations.

> This API internally makes use of the 'dma_range_map' which gets
> populated by the OF core based on the 'dma-ranges' property (if
> present in DT).
> 
> But it makes sense to use it irrespective of whether the platform
> supports non-identical DMA/inbound translation or not. Since this
> API behaves like a no-op and returns the CPU physical address if
> there is an identical mapping, there is literally zero overhead in
> using it.

Thanks for rescuing me.

I wonder if there should be something in
Documentation/core-api/dma-api* about this.  I guess that is mostly
oriented toward things like PCI device drivers, not so much PCI host
bridge drivers.  But it would be nice to have a little intro to
dma-ranges and maybe even the restricted DMA usage.

Bjorn

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

[Manivannan Sadhasivam]

On Fri, May 22, 2026 at 05:43:25PM -0500, Bjorn Helgaas wrote:
> On Thu, May 21, 2026 at 10:44:51AM +0530, Manivannan Sadhasivam wrote:
> > On Wed, May 20, 2026 at 02:59:00PM -0500, Bjorn Helgaas wrote:
> > > 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.
> > 
> > Yeah, it is so easy to confuse both. To summarise, 'ranges'
> > describes the outbound translation and 'dma-ranges' describes the
> > inbound translation from host perspective.
> > 
> > > 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.
> > 
> > Your observations are correct. This driver assumes that the
> > identical mapping exists between CPU and PCI bus addresses. Usually,
> > the drivers make use of phys_to_dma() to handle the translations.
> 
> What does this look like in the native host bridge drivers?  I don't
> see any direct calls of phys_to_dma(), but there are some higher-level
> interfaces that use it.
> 
> I don't really see a consistent style of constructing MSI addresses,
> e.g., in *_compose_msi_msg() implementations.
> 

That's because, each driver may use different methods for constructing the MSI
address. Two commonly used methods are:

1. Using a pre-defined MSI address in the MMIO range (like the pcie-mediatek
driver). The PCIe controller hardware decodes MWr TLPs targeting this address
and raises an interrupt to the CPU via the platform interrupt controller (e.g.
GIC).

2. Allocating coherent memory using dma_alloc_coherent() and programming it
into platform MMIO registers (like pcie-designware-host driver). The Root
Complex inbound ATU matches writes to this address and raises an interrupt to
the CPU.

> > This API internally makes use of the 'dma_range_map' which gets
> > populated by the OF core based on the 'dma-ranges' property (if
> > present in DT).
> > 
> > But it makes sense to use it irrespective of whether the platform
> > supports non-identical DMA/inbound translation or not. Since this
> > API behaves like a no-op and returns the CPU physical address if
> > there is an identical mapping, there is literally zero overhead in
> > using it.
> 
> Thanks for rescuing me.
> 
> I wonder if there should be something in
> Documentation/core-api/dma-api* about this.  I guess that is mostly
> oriented toward things like PCI device drivers, not so much PCI host
> bridge drivers.  But it would be nice to have a little intro to
> dma-ranges and maybe even the restricted DMA usage.
> 

I think this information should belong to the PCI host controller documentation.
I started writing it, but didn't get time to finish and post it. Maybe I'll
atleast post an initial version and add more info on top.

- Mani

-- 
மணிவண்ணன் சதாசிவம்