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

[Manivannan Sadhasivam]

On Fri, Mar 27, 2026 at 12:44:34PM +0100, Florian Eckert wrote:
> 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.
> 

Ok, fair enough. But you do need the DT binding change and you should justify
in the commit log of both binding and driver on why you are breaking ABI and why
it is OK.

> 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.
> 

dw_pcie_get_resources() got added recently. So this driver has been passing the
iATU address from its inception.

> 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.
> 

Yes, we should always avoid hardcoding the MMIO address in the driver and obtain
it from the hardware description like DT. But some DWC drivers do hardcode the
MMIO address as they often have resources like 'atu' at a fixed offset from the
DBI base address. And those drivers often ended up updating the offset based on
the IP version or compatible. So it is best to get this info from DT.

- Mani

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