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

[Lucas Stach <l.stach@pengutronix.de>]

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