Re: [PATCH v1 2/2] PCI: imx6: Use domain number replace the hardcodes

On Thu, Mar 13, 2025 at 11:06:48AM -0500, Bjorn Helgaas wrote:

On Thu, Mar 13, 2025 at 09:54:25AM +0100, Lucas Stach wrote:

quoted

Am Mittwoch, dem 12.03.2025 um 10:22 -0400 schrieb Frank Li:

quoted

On Wed, Mar 12, 2025 at 09:28:02AM +0100, Lucas Stach wrote:

quoted

Am Mittwoch, dem 12.03.2025 um 04:05 +0000 schrieb Hongxing Zhu:

quoted

quoted

-----Original Message-----
From: Bjorn Helgaas <helgaas@kernel.org>
Sent: 2025年3月11日 23:55
To: Hongxing Zhu <hongxing.zhu@nxp.com>
Cc: robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org;
shawnguo@kernel.org; l.stach@pengutronix.de; lpieralisi@kernel.org;
kw@linux.com; manivannan.sadhasivam@linaro.org; bhelgaas@google.com;
s.hauer@pengutronix.de; festevam@gmail.com; devicetree@vger.kernel.org;
linux-pci@vger.kernel.org; imx@lists.linux.dev; kernel@pengutronix.de;
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/2] PCI: imx6: Use domain number replace the
hardcodes

On Tue, Mar 11, 2025 at 01:11:04AM +0000, Hongxing Zhu wrote:

quoted

quoted

-----Original Message-----
From: Bjorn Helgaas <helgaas@kernel.org>
Sent: 2025年3月10日 23:11
To: Hongxing Zhu <hongxing.zhu@nxp.com>
Cc: robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org;
shawnguo@kernel.org; l.stach@pengutronix.de; lpieralisi@kernel.org;
kw@linux.com; manivannan.sadhasivam@linaro.org;

bhelgaas@google.com;

quoted

quoted

s.hauer@pengutronix.de; festevam@gmail.com;
devicetree@vger.kernel.org; linux-pci@vger.kernel.org;
imx@lists.linux.dev; kernel@pengutronix.de;
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/2] PCI: imx6: Use domain number replace the
hardcodes

On Wed, Feb 26, 2025 at 10:42:56AM +0800, Richard Zhu wrote:

quoted

Use the domain number replace the hardcodes to uniquely identify
different controller on i.MX8MQ platforms. No function changes.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 drivers/pci/controller/dwc/pci-imx6.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c

b/drivers/pci/controller/dwc/pci-imx6.c
index 90ace941090f..ab9ebb783593 100644

--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c

@@ -41,7 +41,6 @@

 #define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE	BIT(11)
 #define IMX8MQ_GPR_PCIE_VREG_BYPASS		BIT(12)
 #define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE	GENMASK(11,

8)

quoted

quoted

quoted

-#define IMX8MQ_PCIE2_BASE_ADDR			0x33c00000

 #define IMX95_PCIE_PHY_GEN_CTRL			0x0
 #define IMX95_PCIE_REF_USE_PAD			BIT(17)

@@ -1474,7 +1473,6 @@ static int imx_pcie_probe(struct

platform_device

*pdev)

quoted

 	struct dw_pcie *pci;
 	struct imx_pcie *imx_pcie;
 	struct device_node *np;
-	struct resource *dbi_base;
 	struct device_node *node = dev->of_node;
 	int i, ret, req_cnt;
 	u16 val;

@@ -1515,10 +1513,6 @@ static int imx_pcie_probe(struct

platform_device *pdev)

quoted

 			return PTR_ERR(imx_pcie->phy_base);
 	}

-	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev,

0,

quoted

quoted

&dbi_base);

quoted

-	if (IS_ERR(pci->dbi_base))
-		return PTR_ERR(pci->dbi_base);

This makes me wonder.

IIUC this means that previously we set controller_id to
1 if the first item in devicetree "reg" was 0x33c00000,
and now we will set controller_id to 1 if the devicetree
"linux,pci-domain" property is 1.  This is good, but I
think this new dependency on the correct
"linux,pci-domain" in devicetree should be mentioned in
the commit log.

My bigger worry is that we no longer set pci->dbi_base
at all.  I see that the only use of pci->dbi_base in
pci-imx6.c was to determine the controller_id, but this
is a DWC-based driver, and the DWC core certainly uses
pci->dbi_base.  Are we sure that none of those DWC core
paths are important to pci-imx6.c?

Thanks for your concerns.  Don't worry about the
assignment of pci->dbi_base.  If pci-imx6.c driver doesn't
set it. DWC core driver would set it when
 dw_pcie_get_resources() is invoked.

Great, thanks!  Maybe we can amend the commit log to mention
that and the new "linux,pci-domain" dependency.

How about the following updates of the commit log?

Use the domain number replace the hardcodes to uniquely
identify different controller on i.MX8MQ platforms. No
function changes.  Please make sure the " linux,pci-domain" is
set for i.MX8MQ correctly, since  the controller id is relied
on it totally.

This breaks running a new kernel on an old DT without the
linux,pci-domain property, which I'm absolutely no fan of. We
tried really hard to keep this way around working in the i.MX
world.

8MQ already add linux,pci-domain since Jan, 2021

commit c0b70f05c87f3b09b391027c6f056d0facf331ef
Author: Peng Fan [off-list ref]
Date:   Fri Jan 15 11:26:57 2021 +0800

Only missed is pcie-ep side, which have not been used at all boards dts
file in upstream.

I wasn't aware of this. 2021 is quite a while ago, so I suspect that
nobody is going to run a new kernel with a DT this old. I retract my
objection.

Sounds good, thanks, Lucas.  We really do want to avoid breaking old
DTs, so I appreciate your highlighting of it.  Even if we believe none
of them will break, I think it's worth mentioning the
'linux,pci-domain' dependency and the commit that added it to the
.dtsi in the commit log.

Yes, I think you can add it? some thing like

"linux,pci-domain" must match the PCIe controller instance sequence for the
i.MX8MQ platform.

Frank

Bjorn