[PATCH v1 0/2] Use domain number replace the hardcodes

[PATCH v1 0/2] Use domain number replace the hardcodes

[Richard Zhu]

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

[PATCH v1 1/2] arm64: dts: imx8mq: Add linux,pci-domain into pcie-ep
[PATCH v1 2/2] PCI: imx6: Use domain number replace the hardcodes

arch/arm64/boot/dts/freescale/imx8mq.dtsi |  1 +
drivers/pci/controller/dwc/pci-imx6.c     | 14 ++++++--------
2 files changed, 7 insertions(+), 8 deletions(-)

[PATCH v1 1/2] arm64: dts: imx8mq: Add linux,pci-domain into pcie-ep node

[Richard Zhu]

Add linux,pci-domain into pcie-ep node of i.MX8MQ.

Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mq.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index d51de8d899b2..387b3e227cfc 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -1828,6 +1828,7 @@ pcie1_ep: pcie-ep@33c00000 {
 			interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "dma";
 			fsl,max-link-speed = <2>;
+			linux,pci-domain = <1>;
 			clocks = <&clk IMX8MQ_CLK_PCIE2_ROOT>,
 				 <&clk IMX8MQ_CLK_PCIE2_PHY>,
 				 <&clk IMX8MQ_CLK_PCIE2_PHY>,
-- 
2.37.1

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

[Richard Zhu]

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)
-#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)
 	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)
 			return PTR_ERR(imx_pcie->phy_base);
 	}
 
-	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev, 0, &dbi_base);
-	if (IS_ERR(pci->dbi_base))
-		return PTR_ERR(pci->dbi_base);
-
 	/* Fetch GPIOs */
 	imx_pcie->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(imx_pcie->reset_gpiod))
@@ -1565,8 +1559,12 @@ static int imx_pcie_probe(struct platform_device *pdev)
 	switch (imx_pcie->drvdata->variant) {
 	case IMX8MQ:
 	case IMX8MQ_EP:
-		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
-			imx_pcie->controller_id = 1;
+		ret = of_get_pci_domain_nr(node);
+		if (ret < 0 || ret > 1)
+			return dev_err_probe(dev, -ENODEV,
+					     "failed to get valid pcie domain\n");
+		else
+			imx_pcie->controller_id = ret;
 		break;
 	default:
 		break;
-- 
2.37.1

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

[Bjorn Helgaas]

On Wed, Feb 26, 2025 at 10:42:56AM +0800, Richard Zhu wrote:
> 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)
> -#define IMX8MQ_PCIE2_BASE_ADDR			0x33c00000

Nice, thanks for getting rid of this!

>  #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)
>  	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)
>  			return PTR_ERR(imx_pcie->phy_base);
>  	}
>  
> -	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev, 0, &dbi_base);
> -	if (IS_ERR(pci->dbi_base))
> -		return PTR_ERR(pci->dbi_base);
> -
>  	/* Fetch GPIOs */
>  	imx_pcie->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>  	if (IS_ERR(imx_pcie->reset_gpiod))
> @@ -1565,8 +1559,12 @@ static int imx_pcie_probe(struct platform_device *pdev)
>  	switch (imx_pcie->drvdata->variant) {
>  	case IMX8MQ:
>  	case IMX8MQ_EP:
> -		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> -			imx_pcie->controller_id = 1;
> +		ret = of_get_pci_domain_nr(node);
> +		if (ret < 0 || ret > 1)
> +			return dev_err_probe(dev, -ENODEV,
> +					     "failed to get valid pcie domain\n");
> +		else
> +			imx_pcie->controller_id = ret;
>  		break;
>  	default:
>  		break;
> -- 
> 2.37.1
> 

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

[Frank Li]

On Wed, Feb 26, 2025 at 04:08:26PM -0600, Bjorn Helgaas wrote:
> On Wed, Feb 26, 2025 at 10:42:56AM +0800, Richard Zhu wrote:
> > 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>
> > ---

Reviewed-by: Frank Li <Frank.Li@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)
> > -#define IMX8MQ_PCIE2_BASE_ADDR			0x33c00000
>
> Nice, thanks for getting rid of this!
>
> >  #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)
> >  	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)
> >  			return PTR_ERR(imx_pcie->phy_base);
> >  	}
> >
> > -	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev, 0, &dbi_base);
> > -	if (IS_ERR(pci->dbi_base))
> > -		return PTR_ERR(pci->dbi_base);
> > -
> >  	/* Fetch GPIOs */
> >  	imx_pcie->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> >  	if (IS_ERR(imx_pcie->reset_gpiod))
> > @@ -1565,8 +1559,12 @@ static int imx_pcie_probe(struct platform_device *pdev)
> >  	switch (imx_pcie->drvdata->variant) {
> >  	case IMX8MQ:
> >  	case IMX8MQ_EP:
> > -		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > -			imx_pcie->controller_id = 1;
> > +		ret = of_get_pci_domain_nr(node);
> > +		if (ret < 0 || ret > 1)
> > +			return dev_err_probe(dev, -ENODEV,
> > +					     "failed to get valid pcie domain\n");
> > +		else
> > +			imx_pcie->controller_id = ret;
> >  		break;
> >  	default:
> >  		break;
> > --
> > 2.37.1
> >

Re: [PATCH v1 1/2] arm64: dts: imx8mq: Add linux,pci-domain into pcie-ep node

[Frank Li]

On Wed, Feb 26, 2025 at 10:42:55AM +0800, Richard Zhu wrote:
> Add linux,pci-domain into pcie-ep node of i.MX8MQ.
>
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  arch/arm64/boot/dts/freescale/imx8mq.dtsi | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index d51de8d899b2..387b3e227cfc 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -1828,6 +1828,7 @@ pcie1_ep: pcie-ep@33c00000 {
>  			interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
>  			interrupt-names = "dma";
>  			fsl,max-link-speed = <2>;
> +			linux,pci-domain = <1>;
>  			clocks = <&clk IMX8MQ_CLK_PCIE2_ROOT>,
>  				 <&clk IMX8MQ_CLK_PCIE2_PHY>,
>  				 <&clk IMX8MQ_CLK_PCIE2_PHY>,
> --
> 2.37.1
>

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

[Krzysztof Wilczyński]

Hello,

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

Applied to controller/imx6, thank you!

	Krzysztof

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

[Bjorn Helgaas]

On Wed, Feb 26, 2025 at 10:42:56AM +0800, Richard Zhu wrote:
> 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)
> -#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)
>  	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)
>  			return PTR_ERR(imx_pcie->phy_base);
>  	}
>  
> -	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev, 0, &dbi_base);
> -	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?

>  	/* Fetch GPIOs */
>  	imx_pcie->reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>  	if (IS_ERR(imx_pcie->reset_gpiod))
> @@ -1565,8 +1559,12 @@ static int imx_pcie_probe(struct platform_device *pdev)
>  	switch (imx_pcie->drvdata->variant) {
>  	case IMX8MQ:
>  	case IMX8MQ_EP:
> -		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> -			imx_pcie->controller_id = 1;
> +		ret = of_get_pci_domain_nr(node);
> +		if (ret < 0 || ret > 1)
> +			return dev_err_probe(dev, -ENODEV,
> +					     "failed to get valid pcie domain\n");
> +		else
> +			imx_pcie->controller_id = ret;
>  		break;
>  	default:
>  		break;
> -- 
> 2.37.1
> 

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

[Hongxing Zhu]

> -----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;
> 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:
> > 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)
> > -#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)
> >  	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)
> >  			return PTR_ERR(imx_pcie->phy_base);
> >  	}
> >
> > -	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev, 0,
> &dbi_base);
> > -	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?
Hi Bjorn:
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.

dw_pcie_host_init()/dw_pcie_ep_init()
  ->dw_pcie_get_resources()

...
        if (!pci->dbi_base) {
                res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
                pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
                if (IS_ERR(pci->dbi_base))
                        return PTR_ERR(pci->dbi_base);
                pci->dbi_phys_addr = res->start;
        }
...


Best Regards
Richard Zhu
> 
> >  	/* Fetch GPIOs */
> >  	imx_pcie->reset_gpiod = devm_gpiod_get_optional(dev, "reset",
> GPIOD_OUT_HIGH);
> >  	if (IS_ERR(imx_pcie->reset_gpiod))
> > @@ -1565,8 +1559,12 @@ static int imx_pcie_probe(struct
> platform_device *pdev)
> >  	switch (imx_pcie->drvdata->variant) {
> >  	case IMX8MQ:
> >  	case IMX8MQ_EP:
> > -		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > -			imx_pcie->controller_id = 1;
> > +		ret = of_get_pci_domain_nr(node);
> > +		if (ret < 0 || ret > 1)
> > +			return dev_err_probe(dev, -ENODEV,
> > +					     "failed to get valid pcie domain\n");
> > +		else
> > +			imx_pcie->controller_id = ret;
> >  		break;
> >  	default:
> >  		break;
> > --
> > 2.37.1
> >

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

[Bjorn Helgaas]

On Tue, Mar 11, 2025 at 01:11:04AM +0000, Hongxing Zhu wrote:
> > -----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;
> > 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:
> > > 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)
> > > -#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)
> > >  	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)
> > >  			return PTR_ERR(imx_pcie->phy_base);
> > >  	}
> > >
> > > -	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev, 0,
> > &dbi_base);
> > > -	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?
> Hi Bjorn:
> 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.

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

[Hongxing Zhu]

> -----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:
> > > -----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;
> > > 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:
> > > > 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)
> > > > -#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)
> > > >  	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)
> > > >  			return PTR_ERR(imx_pcie->phy_base);
> > > >  	}
> > > >
> > > > -	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev,
> 0,
> > > &dbi_base);
> > > > -	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?
> > Hi Bjorn:
> > 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.

Best Regards
Richard Zhu

RE: [PATCH v1 1/2] arm64: dts: imx8mq: Add linux,pci-domain into pcie-ep node

[Hongxing Zhu]

> -----Original Message-----
> From: Frank Li <frank.li@nxp.com>
> Sent: 2025年3月1日 7:05
> 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 1/2] arm64: dts: imx8mq: Add linux,pci-domain into
> pcie-ep node
> 
> On Wed, Feb 26, 2025 at 10:42:55AM +0800, Richard Zhu wrote:
> > Add linux,pci-domain into pcie-ep node of i.MX8MQ.
> >
> > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > ---
> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> 
Hi Frank:
Thanks.

Hi Shawn:
Since the PCIe part is picked up by Krzysztof.
Can you help to pick up this dts changes for i.MX8MQ?
Thanks in advanced.

Best Regards
Richard Zhu
> >  arch/arm64/boot/dts/freescale/imx8mq.dtsi | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > index d51de8d899b2..387b3e227cfc 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > @@ -1828,6 +1828,7 @@ pcie1_ep: pcie-ep@33c00000 {
> >  			interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
> >  			interrupt-names = "dma";
> >  			fsl,max-link-speed = <2>;
> > +			linux,pci-domain = <1>;
> >  			clocks = <&clk IMX8MQ_CLK_PCIE2_ROOT>,
> >  				 <&clk IMX8MQ_CLK_PCIE2_PHY>,
> >  				 <&clk IMX8MQ_CLK_PCIE2_PHY>,
> > --
> > 2.37.1
> >

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

[Lucas Stach]

Am Mittwoch, dem 12.03.2025 um 04:05 +0000 schrieb Hongxing Zhu:
> > -----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:
> > > > -----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;
> > > > 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:
> > > > > 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)
> > > > > -#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)
> > > > >  	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)
> > > > >  			return PTR_ERR(imx_pcie->phy_base);
> > > > >  	}
> > > > > 
> > > > > -	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev,
> > 0,
> > > > &dbi_base);
> > > > > -	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?
> > > Hi Bjorn:
> > > 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.

I'm fine with using the property if present and even mandating it for
new platforms, but getting rid of the existing code for the i.MX8MQ
platform is only a marginal cleanup of the driver code with the obvious
downside of the above breakage.

Regards,
Lucas

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

[Frank Li]

On Wed, Mar 12, 2025 at 09:28:02AM +0100, Lucas Stach wrote:
> Am Mittwoch, dem 12.03.2025 um 04:05 +0000 schrieb Hongxing Zhu:
> > > -----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:
> > > > > -----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;
> > > > > 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:
> > > > > > 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)
> > > > > > -#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)
> > > > > >  	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)
> > > > > >  			return PTR_ERR(imx_pcie->phy_base);
> > > > > >  	}
> > > > > >
> > > > > > -	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev,
> > > 0,
> > > > > &dbi_base);
> > > > > > -	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?
> > > > Hi Bjorn:
> > > > 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 <peng.fan@nxp.com>
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.

Frank

>
> I'm fine with using the property if present and even mandating it for
> new platforms, but getting rid of the existing code for the i.MX8MQ
> platform is only a marginal cleanup of the driver code with the obvious
> downside of the above breakage.



>
> Regards,
> Lucas

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

[Bjorn Helgaas]

On Wed, Mar 12, 2025 at 09:28:02AM +0100, Lucas Stach wrote:
> Am Mittwoch, dem 12.03.2025 um 04:05 +0000 schrieb Hongxing Zhu:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > Sent: 2025年3月11日 23:55
> > > On Tue, Mar 11, 2025 at 01:11:04AM +0000, Hongxing Zhu wrote:
> > > > > -----Original Message-----
> > > > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > > > Sent: 2025年3月10日 23:11
> > > > > On Wed, Feb 26, 2025 at 10:42:56AM +0800, Richard Zhu wrote:
> > > > > > 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)
> > > > > > -#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)
> > > > > >  	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)
> > > > > >  			return PTR_ERR(imx_pcie->phy_base);
> > > > > >  	}
> > > > > > 
> > > > > > -	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev,
> > > 0,
> > > > > &dbi_base);
> > > > > > -	if (IS_ERR(pci->dbi_base))
> > > > > > -		return PTR_ERR(pci->dbi_base);

> > 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.
> 
> I'm fine with using the property if present and even mandating it for
> new platforms, but getting rid of the existing code for the i.MX8MQ
> platform is only a marginal cleanup of the driver code with the obvious
> downside of the above breakage.

I don't know the history of these DTs, but if there are any old DTs
for platforms that use controller 1 but lack 'linux,pci-domain', I
agree that we should not break them.

If we need to retain the dbi_base check so that old DTs continue to
work, I think it should look something like this:

  domain = of_get_pci_domain_nr(node);
  if (domain >= 0) {
      if (domain > 1)
          return dev_err_probe(..., "invalid domain %d\n", domain);
      imx_pcie->controller_id = domain;
  } else {
      dev_warn(..., "DT lacks linux,pci-domain, falling back to DBI addr\n");
      dbi_res = platform_get_resource(pdev, IORESOURCE_MEM, index);
      if (dbi_res->start == IMX8MQ_PCIE2_BASE_ADDR)
          imx_pcie->controller_id = 1;
  }

The previous code used devm_platform_get_and_ioremap_resource() and
set pci->dbi_base, but (1) there's no need to set pci->dbi_base since
the DWC core does that anyway, and (2) I think using ioremap() means
we depend on CPU virt == CPU phys, and I don't think we need to depend
on that.

Bjorn

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

[Lucas Stach]

Am Mittwoch, dem 12.03.2025 um 10:22 -0400 schrieb Frank Li:
> On Wed, Mar 12, 2025 at 09:28:02AM +0100, Lucas Stach wrote:
> > Am Mittwoch, dem 12.03.2025 um 04:05 +0000 schrieb Hongxing Zhu:
> > > > -----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:
> > > > > > -----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;
> > > > > > 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:
> > > > > > > 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)
> > > > > > > -#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)
> > > > > > >  	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)
> > > > > > >  			return PTR_ERR(imx_pcie->phy_base);
> > > > > > >  	}
> > > > > > > 
> > > > > > > -	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev,
> > > > 0,
> > > > > > &dbi_base);
> > > > > > > -	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?
> > > > > Hi Bjorn:
> > > > > 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 <peng.fan@nxp.com>
> 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.

Regards,
Lucas

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

[Bjorn Helgaas]

On Thu, Mar 13, 2025 at 09:54:25AM +0100, Lucas Stach wrote:
> Am Mittwoch, dem 12.03.2025 um 10:22 -0400 schrieb Frank Li:
> > On Wed, Mar 12, 2025 at 09:28:02AM +0100, Lucas Stach wrote:
> > > Am Mittwoch, dem 12.03.2025 um 04:05 +0000 schrieb Hongxing Zhu:
> > > > > -----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:
> > > > > > > -----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;
> > > > > > > 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:
> > > > > > > > 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)
> > > > > > > > -#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)
> > > > > > > >  	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)
> > > > > > > >  			return PTR_ERR(imx_pcie->phy_base);
> > > > > > > >  	}
> > > > > > > > 
> > > > > > > > -	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev,
> > > > > 0,
> > > > > > > &dbi_base);
> > > > > > > > -	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 <peng.fan@nxp.com>
> > 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.

Bjorn

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

[Frank Li]

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:
> > Am Mittwoch, dem 12.03.2025 um 10:22 -0400 schrieb Frank Li:
> > > On Wed, Mar 12, 2025 at 09:28:02AM +0100, Lucas Stach wrote:
> > > > Am Mittwoch, dem 12.03.2025 um 04:05 +0000 schrieb Hongxing Zhu:
> > > > > > -----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:
> > > > > > > > -----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;
> > > > > > > > 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:
> > > > > > > > > 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)
> > > > > > > > > -#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)
> > > > > > > > >  	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)
> > > > > > > > >  			return PTR_ERR(imx_pcie->phy_base);
> > > > > > > > >  	}
> > > > > > > > >
> > > > > > > > > -	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev,
> > > > > > 0,
> > > > > > > > &dbi_base);
> > > > > > > > > -	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 <peng.fan@nxp.com>
> > > 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

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

[manivannan.sadhasivam]

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:
> > Am Mittwoch, dem 12.03.2025 um 10:22 -0400 schrieb Frank Li:
> > > On Wed, Mar 12, 2025 at 09:28:02AM +0100, Lucas Stach wrote:
> > > > Am Mittwoch, dem 12.03.2025 um 04:05 +0000 schrieb Hongxing Zhu:
> > > > > > -----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:
> > > > > > > > -----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;
> > > > > > > > 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:
> > > > > > > > > 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)
> > > > > > > > > -#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)
> > > > > > > > >  	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)
> > > > > > > > >  			return PTR_ERR(imx_pcie->phy_base);
> > > > > > > > >  	}
> > > > > > > > > 
> > > > > > > > > -	pci->dbi_base = devm_platform_get_and_ioremap_resource(pdev,
> > > > > > 0,
> > > > > > > > &dbi_base);
> > > > > > > > > -	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 <peng.fan@nxp.com>
> > > 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.
> 

If there is a dependency, then it should be added to the binding. Only that will
ensure that the DTs will have the dependent property present.

- Mani

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

Re: [PATCH v1 1/2] arm64: dts: imx8mq: Add linux,pci-domain into pcie-ep node

[Shawn Guo]

On Wed, Feb 26, 2025 at 10:42:55AM +0800, Richard Zhu wrote:
> Add linux,pci-domain into pcie-ep node of i.MX8MQ.
> 
> Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>

Applied, thanks!