[v1] PCI: intel-gw: Fixes to make the driver working again

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

Posted by Florian Eckert 2 weeks, 6 days ago

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";

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 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

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

Posted by Manivannan Sadhasivam 1 week, 4 days ago

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.

- Mani

> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>  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
> 

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

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

Posted by Florian Eckert 1 week, 3 days ago

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@maxlinear.com>' 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@BY3PR19MB5076.namprd19.prod.outlook.com/
[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@dev.tdt.de>
>> ---
>>  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
>>

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

Posted by Manivannan Sadhasivam 1 week ago

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@maxlinear.com>' 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

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