Re: [PATCH 4/5] PCI: intel-gw: Remove atu base assignment

[Florian Eckert]

On 2026-03-26 17:15, Manivannan Sadhasivam wrote:
> On Tue, Mar 17, 2026 at 11:12:52AM +0100, Florian Eckert wrote:
> > In the current implementation, only one PCIe bridge is recognised. 
> > This
> > change removes the assignment of the ATU base address during host 
> > setup.
> > Instead, the ATU base address is read from the device tree. To do 
> > this,
> > the 'atu' range of the DTS entry must be changed for PCIe.
> >
> > Old DTS entry for PCIe:
> > reg = <0xd1000000 0x1000>,
> > 	<0xd3000000 0x20000>,
> > 	<0xd0c41000.0x1000>;
> > reg-names = "dbi", "config", "app";
> >
> > New DTS entry for PCIe
> > reg = <0xd1000000 0x1000>,
> > 	<0xd10c0000 0x1000>,
> > 	<0xd3000000 0x20000>,
> > 	<0xd0c41000.0x1000>;
> > reg-names = "dbi", "atu", "config", "app";
>
> You just broke the DT backwards compatibility here. What if the driver 
> is used
> with an old DT that doesn't provide this 'atu' entry? The driver will 
> break.
> Moreover, this entry should be added to the DT binding first before any 
> driver
> change.

When I looked at the driver’s history, I found a note in a commit that 
the driver
is only used internally by Maxlinear [1]. The maintainer
'Lei Chuan Hua <lchuanhua@xxxxxxxxxxxxx>' from Maxlinear was still 
active at
that time. He mentioned this on the LKML [2].

Therefore, backward compatibility shouldn’t be necessary.

Unfortunately, it’s already been three months since I last looked into 
this issue.
But as far as I can remember I have to read the 'atu' resource from DTS 
and so have
to remove this line so the driver does work again.

Because the resource is already initialized by the dwc core during the 
function
call 'dw_pcie_get_resources()'on line 141 [3]. This function is called 
in the DWC
host core [4]. This raises the question of why the driver is doing this 
itself at
all, when the core already handles it for us.

Additional, I have noticed that it seems to be bad practice to include 
this
resource information in the source code. This information should instead 
be
loaded via the DTS see commit [5].

If backwards compatibility isn't an issue, then the DWC core should 
handle
loading the 'atu' resource from the DTS.

Is that how you see it too?


Thanks for your time and review

- Florian

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/pci/controller/dwc?h=master&id=07ae413e169da3697e633dd4489db0d681a04460
[2] 
https://lore.kernel.org/all/BY3PR19MB507667CE7531D863E1E5F8AEBDD82@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware.c#n141
[4] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware-host.c#n544
[5] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/Documentation/devicetree/bindings/pci?id=f500a2f1282750fb344ce535d78071cf1493efd1

> > Signed-off-by: Florian Eckert <fe@xxxxxxxxxx>
> > ---
> >  drivers/pci/controller/dwc/pcie-intel-gw.c | 2 --
> >  1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-intel-gw.c 
> > b/drivers/pci/controller/dwc/pcie-intel-gw.c
> > index 
> > 6bd25f8da605032bfdb97596fb3a1f6a03e88bfc..cec972d5aa9107d4708338bd7349415a31f0e688 
> > 100644
> > --- a/drivers/pci/controller/dwc/pcie-intel-gw.c
> > +++ b/drivers/pci/controller/dwc/pcie-intel-gw.c
> > @@ -311,8 +311,6 @@ static int intel_pcie_host_setup(struct intel_pcie 
> > *pcie)
> >  		goto clk_err;
> >  	}
> >
> > -	pci->atu_base = pci->dbi_base + 0xC0000;
> > -
> >  	ret = phy_init(pcie->phy);
> >  	if (ret)
> >  		goto phy_err;
> >
> > --
> > 2.47.3