Re: [PATCH v2 3/3] PCI: imx6: Remove cpu_addr_fixup()

[Frank Li <Frank.li@nxp.com>]

On Fri, Sep 27, 2024 at 06:54:44PM -0500, Bjorn Helgaas wrote:
> On Thu, Sep 26, 2024 at 12:47:15PM -0400, Frank Li wrote:
> > Remove cpu_addr_fixup() because dwc common driver already handle address
> > translate.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > Change from v1 to v2
> > - set using_dtbus_info true
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 22 ++--------------------
> >  1 file changed, 2 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 1e58c24137e7f..94f3411352bf0 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -82,7 +82,6 @@ enum imx_pcie_variants {
> >  #define IMX_PCIE_FLAG_HAS_PHY_RESET		BIT(5)
> >  #define IMX_PCIE_FLAG_HAS_SERDES		BIT(6)
> >  #define IMX_PCIE_FLAG_SUPPORT_64BIT		BIT(7)
> > -#define IMX_PCIE_FLAG_CPU_ADDR_FIXUP		BIT(8)
> >
> >  #define imx_check_flag(pci, val)	(pci->drvdata->flags & val)
> >
> > @@ -1015,22 +1014,6 @@ static void imx_pcie_host_exit(struct dw_pcie_rp *pp)
> >  		regulator_disable(imx_pcie->vpcie);
> >  }
> >
> > -static u64 imx_pcie_cpu_addr_fixup(struct dw_pcie *pcie, u64 cpu_addr)
> > -{
> > -	struct imx_pcie *imx_pcie = to_imx_pcie(pcie);
> > -	struct dw_pcie_rp *pp = &pcie->pp;
> > -	struct resource_entry *entry;
> > -
> > -	if (!(imx_pcie->drvdata->flags & IMX_PCIE_FLAG_CPU_ADDR_FIXUP))
> > -		return cpu_addr;
> > -
> > -	entry = resource_list_first_type(&pp->bridge->windows, IORESOURCE_MEM);
> > -	if (!entry)
> > -		return cpu_addr;
> > -
> > -	return cpu_addr - entry->offset;
> > -}
> > -
> >  static const struct dw_pcie_host_ops imx_pcie_host_ops = {
> >  	.init = imx_pcie_host_init,
> >  	.deinit = imx_pcie_host_exit,
> > @@ -1039,7 +1022,6 @@ static const struct dw_pcie_host_ops imx_pcie_host_ops = {
> >  static const struct dw_pcie_ops dw_pcie_ops = {
> >  	.start_link = imx_pcie_start_link,
> >  	.stop_link = imx_pcie_stop_link,
> > -	.cpu_addr_fixup = imx_pcie_cpu_addr_fixup,
>
> This is tremendous, thank you very much for doing this!
>
> Have you looked at the other users of .cpu_addr_fixup()?  It looks
> like cadence, dra7xx, artpec6, intel-gw, and visconti all use it.
>
> Do we know whether any of them have to deal with DTs that don't
> describe the correct translations?  It would be even better if we
> could fix them all and we didn't need using_dtbus_info.

There are two case,

Case 1: .

bus {
	pci {
		ranges = <MEM: C, B, size>;
	};
}

Need update to

bus {
	ranges <A, B, Size>
	pci {
		ranges= <MEM, C, A, size>;
	}
}

The good thinks this change don't break back compatiblty, need change dts
first then remove fixed up. but it will be problem if use new kernel with
old dts.

Case 2: use fake transalation

bus {
	ranges = <0x8000_0000, 0xa_80000_0000, size>
	pci {
		ranges = <MEM, 0x8000_0000, 0x8000_0000, size>;
	}
}

This one need fix ranges first, then remove fixed up. The same as case1
it will be problem if use new kenrel with old dts.


Anyways, it's long way to remove all fixes up. I have not these hardware
to test change.

I feel like use using_dtbus_info first, then remove fixedup one by one.
after all fixedup removed, we can remove using_dtbus_info.


Frank

>
> >  };
> >
> >  static void imx_pcie_ep_init(struct dw_pcie_ep *ep)
> > @@ -1459,6 +1441,7 @@ static int imx_pcie_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >
> > +	pci->using_dtbus_info = true;
> >  	if (imx_pcie->drvdata->mode == DW_PCIE_EP_TYPE) {
> >  		ret = imx_add_pcie_ep(imx_pcie, pdev);
> >  		if (ret < 0)
> > @@ -1598,8 +1581,7 @@ static const struct imx_pcie_drvdata drvdata[] = {
> >  	},
> >  	[IMX8Q] = {
> >  		.variant = IMX8Q,
> > -		.flags = IMX_PCIE_FLAG_HAS_PHYDRV |
> > -			 IMX_PCIE_FLAG_CPU_ADDR_FIXUP,
> > +		.flags = IMX_PCIE_FLAG_HAS_PHYDRV,
> >  		.clk_names = imx8q_clks,
> >  		.clks_cnt = ARRAY_SIZE(imx8q_clks),
> >  	},
> >
> > --
> > 2.34.1
> >