diff mbox series

[v3,1/2] PCI: dwc: Add dbi_phys_addr and atu_phys_addr to struct dw_pcie

Message ID 20240724022719.2868490-2-quic_pyarlaga@quicinc.com (mailing list archive)
State Not Applicable
Headers show
Series PCI: qcom: Avoid DBI and ATU register space mirroring | expand

Commit Message

Prudhvi Yarlagadda July 24, 2024, 2:27 a.m. UTC
Both DBI and ATU physical base addresses are needed by pcie_qcom.c
driver to program the location of DBI and ATU blocks in Qualcomm
PCIe Controller specific PARF hardware block.

Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-designware.c | 2 ++
 drivers/pci/controller/dwc/pcie-designware.h | 2 ++
 2 files changed, 4 insertions(+)

Comments

Manivannan Sadhasivam July 24, 2024, 1:53 p.m. UTC | #1
On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote:

Subject could be modified as below:

PCI: dwc: Cache DBI and iATU physical addresses in 'struct dw_pcie_ops'

> Both DBI and ATU physical base addresses are needed by pcie_qcom.c
> driver to program the location of DBI and ATU blocks in Qualcomm
> PCIe Controller specific PARF hardware block.
> 
> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>

Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 2 ++
>  drivers/pci/controller/dwc/pcie-designware.h | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 1b5aba1f0c92..bc3a5d6b0177 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>  		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
>  		if (IS_ERR(pci->dbi_base))
>  			return PTR_ERR(pci->dbi_base);
> +		pci->dbi_phys_addr = res->start;
>  	}
>  
>  	/* DBI2 is mainly useful for the endpoint controller */
> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>  			pci->atu_base = devm_ioremap_resource(pci->dev, res);
>  			if (IS_ERR(pci->atu_base))
>  				return PTR_ERR(pci->atu_base);
> +			pci->atu_phys_addr = res->start;
>  		} else {
>  			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>  		}
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 53c4c8f399c8..efc72989330c 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -407,8 +407,10 @@ struct dw_pcie_ops {
>  struct dw_pcie {
>  	struct device		*dev;
>  	void __iomem		*dbi_base;
> +	phys_addr_t		dbi_phys_addr;
>  	void __iomem		*dbi_base2;
>  	void __iomem		*atu_base;
> +	phys_addr_t		atu_phys_addr;
>  	size_t			atu_size;
>  	u32			num_ib_windows;
>  	u32			num_ob_windows;
> -- 
> 2.25.1
>
Prudhvi Yarlagadda July 24, 2024, 6:28 p.m. UTC | #2
Hi Manivannan,

Thanks for the review comments.

On 7/24/2024 6:53 AM, Manivannan Sadhasivam wrote:
> On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote:
> 
> Subject could be modified as below:
> 
> PCI: dwc: Cache DBI and iATU physical addresses in 'struct dw_pcie_ops'
> 

ACK. I will update it in the next patch.

>> Both DBI and ATU physical base addresses are needed by pcie_qcom.c
>> driver to program the location of DBI and ATU blocks in Qualcomm
>> PCIe Controller specific PARF hardware block.
>>
>> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
> 
> Suggested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> - Mani
> 

ACK. I will add it in the next patch.

Thanks,
Prudhvi
>> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
>> ---
>>  drivers/pci/controller/dwc/pcie-designware.c | 2 ++
>>  drivers/pci/controller/dwc/pcie-designware.h | 2 ++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
>> index 1b5aba1f0c92..bc3a5d6b0177 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
>> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>>  		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
>>  		if (IS_ERR(pci->dbi_base))
>>  			return PTR_ERR(pci->dbi_base);
>> +		pci->dbi_phys_addr = res->start;
>>  	}
>>  
>>  	/* DBI2 is mainly useful for the endpoint controller */
>> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>>  			pci->atu_base = devm_ioremap_resource(pci->dev, res);
>>  			if (IS_ERR(pci->atu_base))
>>  				return PTR_ERR(pci->atu_base);
>> +			pci->atu_phys_addr = res->start;
>>  		} else {
>>  			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>>  		}
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index 53c4c8f399c8..efc72989330c 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -407,8 +407,10 @@ struct dw_pcie_ops {
>>  struct dw_pcie {
>>  	struct device		*dev;
>>  	void __iomem		*dbi_base;
>> +	phys_addr_t		dbi_phys_addr;
>>  	void __iomem		*dbi_base2;
>>  	void __iomem		*atu_base;
>> +	phys_addr_t		atu_phys_addr;
>>  	size_t			atu_size;
>>  	u32			num_ib_windows;
>>  	u32			num_ob_windows;
>> -- 
>> 2.25.1
>>
>
Bjorn Helgaas July 24, 2024, 6:39 p.m. UTC | #3
On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote:
> Both DBI and ATU physical base addresses are needed by pcie_qcom.c
> driver to program the location of DBI and ATU blocks in Qualcomm
> PCIe Controller specific PARF hardware block.
> 
> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 2 ++
>  drivers/pci/controller/dwc/pcie-designware.h | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 1b5aba1f0c92..bc3a5d6b0177 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>  		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
>  		if (IS_ERR(pci->dbi_base))
>  			return PTR_ERR(pci->dbi_base);
> +		pci->dbi_phys_addr = res->start;
>  	}
>  
>  	/* DBI2 is mainly useful for the endpoint controller */
> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>  			pci->atu_base = devm_ioremap_resource(pci->dev, res);
>  			if (IS_ERR(pci->atu_base))
>  				return PTR_ERR(pci->atu_base);
> +			pci->atu_phys_addr = res->start;
>  		} else {
>  			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>  		}
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 53c4c8f399c8..efc72989330c 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -407,8 +407,10 @@ struct dw_pcie_ops {
>  struct dw_pcie {
>  	struct device		*dev;
>  	void __iomem		*dbi_base;
> +	phys_addr_t		dbi_phys_addr;
>  	void __iomem		*dbi_base2;
>  	void __iomem		*atu_base;
> +	phys_addr_t		atu_phys_addr;
>  	size_t			atu_size;
>  	u32			num_ib_windows;
>  	u32			num_ob_windows;

This patch is pretty trivial and it doesn't show anything to justify
the need to keep these addresses.  I think this should be squashed
with the next patch that actually *uses* them.
Prudhvi Yarlagadda July 25, 2024, 11:05 p.m. UTC | #4
Hi Bjorn,

Thanks for the review comments.

On 7/24/2024 11:39 AM, Bjorn Helgaas wrote:
> On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote:
>> Both DBI and ATU physical base addresses are needed by pcie_qcom.c
>> driver to program the location of DBI and ATU blocks in Qualcomm
>> PCIe Controller specific PARF hardware block.
>>
>> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
>> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
>> ---
>>  drivers/pci/controller/dwc/pcie-designware.c | 2 ++
>>  drivers/pci/controller/dwc/pcie-designware.h | 2 ++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
>> index 1b5aba1f0c92..bc3a5d6b0177 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
>> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>>  		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
>>  		if (IS_ERR(pci->dbi_base))
>>  			return PTR_ERR(pci->dbi_base);
>> +		pci->dbi_phys_addr = res->start;
>>  	}
>>  
>>  	/* DBI2 is mainly useful for the endpoint controller */
>> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>>  			pci->atu_base = devm_ioremap_resource(pci->dev, res);
>>  			if (IS_ERR(pci->atu_base))
>>  				return PTR_ERR(pci->atu_base);
>> +			pci->atu_phys_addr = res->start;
>>  		} else {
>>  			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>>  		}
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index 53c4c8f399c8..efc72989330c 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -407,8 +407,10 @@ struct dw_pcie_ops {
>>  struct dw_pcie {
>>  	struct device		*dev;
>>  	void __iomem		*dbi_base;
>> +	phys_addr_t		dbi_phys_addr;
>>  	void __iomem		*dbi_base2;
>>  	void __iomem		*atu_base;
>> +	phys_addr_t		atu_phys_addr;
>>  	size_t			atu_size;
>>  	u32			num_ib_windows;
>>  	u32			num_ob_windows;
> 
> This patch is pretty trivial and it doesn't show anything to justify
> the need to keep these addresses.  I think this should be squashed
> with the next patch that actually *uses* them.
> 

ACK. I will update it in the next patch.

Thanks,
Prudhvi
Serge Semin Aug. 1, 2024, 7:25 p.m. UTC | #5
On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote:
> Both DBI and ATU physical base addresses are needed by pcie_qcom.c
> driver to program the location of DBI and ATU blocks in Qualcomm
> PCIe Controller specific PARF hardware block.
> 
> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 2 ++
>  drivers/pci/controller/dwc/pcie-designware.h | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 1b5aba1f0c92..bc3a5d6b0177 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>  		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
>  		if (IS_ERR(pci->dbi_base))
>  			return PTR_ERR(pci->dbi_base);
> +		pci->dbi_phys_addr = res->start;
>  	}
>  
>  	/* DBI2 is mainly useful for the endpoint controller */
> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>  			pci->atu_base = devm_ioremap_resource(pci->dev, res);
>  			if (IS_ERR(pci->atu_base))
>  				return PTR_ERR(pci->atu_base);
> +			pci->atu_phys_addr = res->start;
>  		} else {
>  			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>  		}
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 53c4c8f399c8..efc72989330c 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -407,8 +407,10 @@ struct dw_pcie_ops {
>  struct dw_pcie {
>  	struct device		*dev;
>  	void __iomem		*dbi_base;

> +	phys_addr_t		dbi_phys_addr;
>  	void __iomem		*dbi_base2;
>  	void __iomem		*atu_base;
> +	phys_addr_t		atu_phys_addr;

What's the point in adding these fields to the generic DW PCIe private
data if they are going to be used in the Qcom glue driver only?

What about moving them to the qcom_pcie structure and initializing the
fields in some place of the pcie-qcom.c driver?

-Serge(y)

>  	size_t			atu_size;
>  	u32			num_ib_windows;
>  	u32			num_ob_windows;
> -- 
> 2.25.1
> 
>
Prudhvi Yarlagadda Aug. 1, 2024, 9:29 p.m. UTC | #6
Hi Serge,

Thanks for the review comment.

On 8/1/2024 12:25 PM, Serge Semin wrote:
> On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote:
>> Both DBI and ATU physical base addresses are needed by pcie_qcom.c
>> driver to program the location of DBI and ATU blocks in Qualcomm
>> PCIe Controller specific PARF hardware block.
>>
>> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
>> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
>> ---
>>  drivers/pci/controller/dwc/pcie-designware.c | 2 ++
>>  drivers/pci/controller/dwc/pcie-designware.h | 2 ++
>>  2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
>> index 1b5aba1f0c92..bc3a5d6b0177 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
>> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>>  		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
>>  		if (IS_ERR(pci->dbi_base))
>>  			return PTR_ERR(pci->dbi_base);
>> +		pci->dbi_phys_addr = res->start;
>>  	}
>>  
>>  	/* DBI2 is mainly useful for the endpoint controller */
>> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>>  			pci->atu_base = devm_ioremap_resource(pci->dev, res);
>>  			if (IS_ERR(pci->atu_base))
>>  				return PTR_ERR(pci->atu_base);
>> +			pci->atu_phys_addr = res->start;
>>  		} else {
>>  			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>>  		}
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>> index 53c4c8f399c8..efc72989330c 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>> @@ -407,8 +407,10 @@ struct dw_pcie_ops {
>>  struct dw_pcie {
>>  	struct device		*dev;
>>  	void __iomem		*dbi_base;
> 
>> +	phys_addr_t		dbi_phys_addr;
>>  	void __iomem		*dbi_base2;
>>  	void __iomem		*atu_base;
>> +	phys_addr_t		atu_phys_addr;
> 
> What's the point in adding these fields to the generic DW PCIe private
> data if they are going to be used in the Qcom glue driver only?
> 
> What about moving them to the qcom_pcie structure and initializing the
> fields in some place of the pcie-qcom.c driver?
> 
> -Serge(y)
> 

These fields were in pcie-qcom.c driver in the v1 patch[1] and
Manivannan suggested to move these fields to 'struct dw_pcie' so that duplication
of resource fetching code 'platform_get_resource_byname()' can be avoided.

[1] https://lore.kernel.org/linux-pci/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/T/#mf9843386d57e9003de983e24e17de4d54314ff73

Thanks,
Prudhvi
>>  	size_t			atu_size;
>>  	u32			num_ib_windows;
>>  	u32			num_ob_windows;
>> -- 
>> 2.25.1
>>
>>
Serge Semin Aug. 1, 2024, 9:59 p.m. UTC | #7
On Thu, Aug 01, 2024 at 02:29:49PM -0700, Prudhvi Yarlagadda wrote:
> Hi Serge,
> 
> Thanks for the review comment.
> 
> On 8/1/2024 12:25 PM, Serge Semin wrote:
> > On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote:
> >> Both DBI and ATU physical base addresses are needed by pcie_qcom.c
> >> driver to program the location of DBI and ATU blocks in Qualcomm
> >> PCIe Controller specific PARF hardware block.
> >>
> >> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
> >> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
> >> ---
> >>  drivers/pci/controller/dwc/pcie-designware.c | 2 ++
> >>  drivers/pci/controller/dwc/pcie-designware.h | 2 ++
> >>  2 files changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> >> index 1b5aba1f0c92..bc3a5d6b0177 100644
> >> --- a/drivers/pci/controller/dwc/pcie-designware.c
> >> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> >> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
> >>  		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
> >>  		if (IS_ERR(pci->dbi_base))
> >>  			return PTR_ERR(pci->dbi_base);
> >> +		pci->dbi_phys_addr = res->start;
> >>  	}
> >>  
> >>  	/* DBI2 is mainly useful for the endpoint controller */
> >> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
> >>  			pci->atu_base = devm_ioremap_resource(pci->dev, res);
> >>  			if (IS_ERR(pci->atu_base))
> >>  				return PTR_ERR(pci->atu_base);
> >> +			pci->atu_phys_addr = res->start;
> >>  		} else {
> >>  			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
> >>  		}
> >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> >> index 53c4c8f399c8..efc72989330c 100644
> >> --- a/drivers/pci/controller/dwc/pcie-designware.h
> >> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> >> @@ -407,8 +407,10 @@ struct dw_pcie_ops {
> >>  struct dw_pcie {
> >>  	struct device		*dev;
> >>  	void __iomem		*dbi_base;
> > 
> >> +	phys_addr_t		dbi_phys_addr;
> >>  	void __iomem		*dbi_base2;
> >>  	void __iomem		*atu_base;
> >> +	phys_addr_t		atu_phys_addr;
> > 
> > What's the point in adding these fields to the generic DW PCIe private
> > data if they are going to be used in the Qcom glue driver only?
> > 
> > What about moving them to the qcom_pcie structure and initializing the
> > fields in some place of the pcie-qcom.c driver?
> > 
> > -Serge(y)
> > 
> 

> These fields were in pcie-qcom.c driver in the v1 patch[1] and
> Manivannan suggested to move these fields to 'struct dw_pcie' so that duplication
> of resource fetching code 'platform_get_resource_byname()' can be avoided.
> 
> [1] https://lore.kernel.org/linux-pci/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/T/#mf9843386d57e9003de983e24e17de4d54314ff73

Em, polluting the core driver structure with data not being used by
the core driver but by the glue-code doesn't seem like a better
alternative to additional platform_get_resource_byname() call in the
glue-driver. I would have got back v1 version so to keep the core
driver simpler. Bjorn?

-Serge(y)

> 
> Thanks,
> Prudhvi
> >>  	size_t			atu_size;
> >>  	u32			num_ib_windows;
> >>  	u32			num_ob_windows;
> >> -- 
> >> 2.25.1
> >>
> >>
Manivannan Sadhasivam Aug. 2, 2024, 5:22 a.m. UTC | #8
On Fri, Aug 02, 2024 at 12:59:57AM +0300, Serge Semin wrote:
> On Thu, Aug 01, 2024 at 02:29:49PM -0700, Prudhvi Yarlagadda wrote:
> > Hi Serge,
> > 
> > Thanks for the review comment.
> > 
> > On 8/1/2024 12:25 PM, Serge Semin wrote:
> > > On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote:
> > >> Both DBI and ATU physical base addresses are needed by pcie_qcom.c
> > >> driver to program the location of DBI and ATU blocks in Qualcomm
> > >> PCIe Controller specific PARF hardware block.
> > >>
> > >> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
> > >> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
> > >> ---
> > >>  drivers/pci/controller/dwc/pcie-designware.c | 2 ++
> > >>  drivers/pci/controller/dwc/pcie-designware.h | 2 ++
> > >>  2 files changed, 4 insertions(+)
> > >>
> > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > >> index 1b5aba1f0c92..bc3a5d6b0177 100644
> > >> --- a/drivers/pci/controller/dwc/pcie-designware.c
> > >> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > >> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
> > >>  		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
> > >>  		if (IS_ERR(pci->dbi_base))
> > >>  			return PTR_ERR(pci->dbi_base);
> > >> +		pci->dbi_phys_addr = res->start;
> > >>  	}
> > >>  
> > >>  	/* DBI2 is mainly useful for the endpoint controller */
> > >> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
> > >>  			pci->atu_base = devm_ioremap_resource(pci->dev, res);
> > >>  			if (IS_ERR(pci->atu_base))
> > >>  				return PTR_ERR(pci->atu_base);
> > >> +			pci->atu_phys_addr = res->start;
> > >>  		} else {
> > >>  			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
> > >>  		}
> > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > >> index 53c4c8f399c8..efc72989330c 100644
> > >> --- a/drivers/pci/controller/dwc/pcie-designware.h
> > >> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > >> @@ -407,8 +407,10 @@ struct dw_pcie_ops {
> > >>  struct dw_pcie {
> > >>  	struct device		*dev;
> > >>  	void __iomem		*dbi_base;
> > > 
> > >> +	phys_addr_t		dbi_phys_addr;
> > >>  	void __iomem		*dbi_base2;
> > >>  	void __iomem		*atu_base;
> > >> +	phys_addr_t		atu_phys_addr;
> > > 
> > > What's the point in adding these fields to the generic DW PCIe private
> > > data if they are going to be used in the Qcom glue driver only?
> > > 
> > > What about moving them to the qcom_pcie structure and initializing the
> > > fields in some place of the pcie-qcom.c driver?
> > > 
> > > -Serge(y)
> > > 
> > 
> 
> > These fields were in pcie-qcom.c driver in the v1 patch[1] and
> > Manivannan suggested to move these fields to 'struct dw_pcie' so that duplication
> > of resource fetching code 'platform_get_resource_byname()' can be avoided.
> > 
> > [1] https://lore.kernel.org/linux-pci/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/T/#mf9843386d57e9003de983e24e17de4d54314ff73
> 
> Em, polluting the core driver structure with data not being used by
> the core driver but by the glue-code doesn't seem like a better
> alternative to additional platform_get_resource_byname() call in the
> glue-driver. I would have got back v1 version so to keep the core
> driver simpler. Bjorn?
> 

IDK how adding two fields which is very related to DWC code *pollutes* it. Since
there is already 'dbi_base', adding 'dbi_phys_addr' made sense to me even though
only glue drivers are using it. Otherwise, glue drivers have to duplicate the
platform_get_resource_byname() code which I find annoying.

- Mani
Serge Semin Aug. 2, 2024, 9:22 a.m. UTC | #9
On Fri, Aug 02, 2024 at 10:52:06AM +0530, Manivannan Sadhasivam wrote:
> On Fri, Aug 02, 2024 at 12:59:57AM +0300, Serge Semin wrote:
> > On Thu, Aug 01, 2024 at 02:29:49PM -0700, Prudhvi Yarlagadda wrote:
> > > Hi Serge,
> > > 
> > > Thanks for the review comment.
> > > 
> > > On 8/1/2024 12:25 PM, Serge Semin wrote:
> > > > On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote:
> > > >> Both DBI and ATU physical base addresses are needed by pcie_qcom.c
> > > >> driver to program the location of DBI and ATU blocks in Qualcomm
> > > >> PCIe Controller specific PARF hardware block.
> > > >>
> > > >> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
> > > >> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
> > > >> ---
> > > >>  drivers/pci/controller/dwc/pcie-designware.c | 2 ++
> > > >>  drivers/pci/controller/dwc/pcie-designware.h | 2 ++
> > > >>  2 files changed, 4 insertions(+)
> > > >>
> > > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > >> index 1b5aba1f0c92..bc3a5d6b0177 100644
> > > >> --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > >> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > >> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
> > > >>  		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
> > > >>  		if (IS_ERR(pci->dbi_base))
> > > >>  			return PTR_ERR(pci->dbi_base);
> > > >> +		pci->dbi_phys_addr = res->start;
> > > >>  	}
> > > >>  
> > > >>  	/* DBI2 is mainly useful for the endpoint controller */
> > > >> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
> > > >>  			pci->atu_base = devm_ioremap_resource(pci->dev, res);
> > > >>  			if (IS_ERR(pci->atu_base))
> > > >>  				return PTR_ERR(pci->atu_base);
> > > >> +			pci->atu_phys_addr = res->start;
> > > >>  		} else {
> > > >>  			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
> > > >>  		}
> > > >> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > >> index 53c4c8f399c8..efc72989330c 100644
> > > >> --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > >> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > >> @@ -407,8 +407,10 @@ struct dw_pcie_ops {
> > > >>  struct dw_pcie {
> > > >>  	struct device		*dev;
> > > >>  	void __iomem		*dbi_base;
> > > > 
> > > >> +	phys_addr_t		dbi_phys_addr;
> > > >>  	void __iomem		*dbi_base2;
> > > >>  	void __iomem		*atu_base;
> > > >> +	phys_addr_t		atu_phys_addr;
> > > > 
> > > > What's the point in adding these fields to the generic DW PCIe private
> > > > data if they are going to be used in the Qcom glue driver only?
> > > > 
> > > > What about moving them to the qcom_pcie structure and initializing the
> > > > fields in some place of the pcie-qcom.c driver?
> > > > 
> > > > -Serge(y)
> > > > 
> > > 
> > 
> > > These fields were in pcie-qcom.c driver in the v1 patch[1] and
> > > Manivannan suggested to move these fields to 'struct dw_pcie' so that duplication
> > > of resource fetching code 'platform_get_resource_byname()' can be avoided.
> > > 
> > > [1] https://lore.kernel.org/linux-pci/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/T/#mf9843386d57e9003de983e24e17de4d54314ff73
> > 
> > Em, polluting the core driver structure with data not being used by
> > the core driver but by the glue-code doesn't seem like a better
> > alternative to additional platform_get_resource_byname() call in the
> > glue-driver. I would have got back v1 version so to keep the core
> > driver simpler. Bjorn?
> > 
> 
> IDK how adding two fields which is very related to DWC code *pollutes* it. Since
> there is already 'dbi_base', adding 'dbi_phys_addr' made sense to me even though
> only glue drivers are using it. Otherwise, glue drivers have to duplicate the
> platform_get_resource_byname() code which I find annoying.

I just explained why it was redundant:
1. adding the fields expands the core private data size for _all_
platforms for no reason. (a few bytes but still)
2. the new fields aren't utilized by the core driver, but still
defined in the core private data which is first confusing and
second implicitly encourages the kernel developers to add another
unused or even weakly-related fields in there.
3. the new fields utilized in a single glue-driver and there is a small
chance they will be used in another ones. Another story would have
been if we had them used in more than one glue-driver...

So from that perspective I find adding these fields to the driver core
data less appropriate than duplicating the
platform_get_resource_byname() call in a _single_ glue driver. It
seems more reasonable to have them defined and utilized in the code
that actually needs them, but not in the place that doesn't annoy you.)

Anyway I read your v1 command and did understand your point in the
first place. That's why my question was addressed to Bjorn.

Please also note the resource::start field is of the resource_size_t
type. So wherever the fields are added, it's better to have them
defined of that type instead.

-Serge(y)

> 
> - Mani
> 
> -- 
> மணிவண்ணன் சதாசிவம்
Prudhvi Yarlagadda Aug. 8, 2024, 6:30 p.m. UTC | #10
On 8/2/2024 2:22 AM, Serge Semin wrote:
> On Fri, Aug 02, 2024 at 10:52:06AM +0530, Manivannan Sadhasivam wrote:
>> On Fri, Aug 02, 2024 at 12:59:57AM +0300, Serge Semin wrote:
>>> On Thu, Aug 01, 2024 at 02:29:49PM -0700, Prudhvi Yarlagadda wrote:
>>>> Hi Serge,
>>>>
>>>> Thanks for the review comment.
>>>>
>>>> On 8/1/2024 12:25 PM, Serge Semin wrote:
>>>>> On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote:
>>>>>> Both DBI and ATU physical base addresses are needed by pcie_qcom.c
>>>>>> driver to program the location of DBI and ATU blocks in Qualcomm
>>>>>> PCIe Controller specific PARF hardware block.
>>>>>>
>>>>>> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
>>>>>> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
>>>>>> ---
>>>>>>  drivers/pci/controller/dwc/pcie-designware.c | 2 ++
>>>>>>  drivers/pci/controller/dwc/pcie-designware.h | 2 ++
>>>>>>  2 files changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
>>>>>> index 1b5aba1f0c92..bc3a5d6b0177 100644
>>>>>> --- a/drivers/pci/controller/dwc/pcie-designware.c
>>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
>>>>>> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>>>>>>  		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
>>>>>>  		if (IS_ERR(pci->dbi_base))
>>>>>>  			return PTR_ERR(pci->dbi_base);
>>>>>> +		pci->dbi_phys_addr = res->start;
>>>>>>  	}
>>>>>>  
>>>>>>  	/* DBI2 is mainly useful for the endpoint controller */
>>>>>> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
>>>>>>  			pci->atu_base = devm_ioremap_resource(pci->dev, res);
>>>>>>  			if (IS_ERR(pci->atu_base))
>>>>>>  				return PTR_ERR(pci->atu_base);
>>>>>> +			pci->atu_phys_addr = res->start;
>>>>>>  		} else {
>>>>>>  			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
>>>>>>  		}
>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
>>>>>> index 53c4c8f399c8..efc72989330c 100644
>>>>>> --- a/drivers/pci/controller/dwc/pcie-designware.h
>>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
>>>>>> @@ -407,8 +407,10 @@ struct dw_pcie_ops {
>>>>>>  struct dw_pcie {
>>>>>>  	struct device		*dev;
>>>>>>  	void __iomem		*dbi_base;
>>>>>
>>>>>> +	phys_addr_t		dbi_phys_addr;
>>>>>>  	void __iomem		*dbi_base2;
>>>>>>  	void __iomem		*atu_base;
>>>>>> +	phys_addr_t		atu_phys_addr;
>>>>>
>>>>> What's the point in adding these fields to the generic DW PCIe private
>>>>> data if they are going to be used in the Qcom glue driver only?
>>>>>
>>>>> What about moving them to the qcom_pcie structure and initializing the
>>>>> fields in some place of the pcie-qcom.c driver?
>>>>>
>>>>> -Serge(y)
>>>>>
>>>>
>>>
>>>> These fields were in pcie-qcom.c driver in the v1 patch[1] and
>>>> Manivannan suggested to move these fields to 'struct dw_pcie' so that duplication
>>>> of resource fetching code 'platform_get_resource_byname()' can be avoided.
>>>>
>>>> [1] https://lore.kernel.org/linux-pci/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/T/#mf9843386d57e9003de983e24e17de4d54314ff73
>>>
>>> Em, polluting the core driver structure with data not being used by
>>> the core driver but by the glue-code doesn't seem like a better
>>> alternative to additional platform_get_resource_byname() call in the
>>> glue-driver. I would have got back v1 version so to keep the core
>>> driver simpler. Bjorn?
>>>
>>
>> IDK how adding two fields which is very related to DWC code *pollutes* it. Since
>> there is already 'dbi_base', adding 'dbi_phys_addr' made sense to me even though
>> only glue drivers are using it. Otherwise, glue drivers have to duplicate the
>> platform_get_resource_byname() code which I find annoying.
> 
> I just explained why it was redundant:
> 1. adding the fields expands the core private data size for _all_
> platforms for no reason. (a few bytes but still)
> 2. the new fields aren't utilized by the core driver, but still
> defined in the core private data which is first confusing and
> second implicitly encourages the kernel developers to add another
> unused or even weakly-related fields in there.
> 3. the new fields utilized in a single glue-driver and there is a small
> chance they will be used in another ones. Another story would have
> been if we had them used in more than one glue-driver...
> 
> So from that perspective I find adding these fields to the driver core
> data less appropriate than duplicating the
> platform_get_resource_byname() call in a _single_ glue driver. It
> seems more reasonable to have them defined and utilized in the code
> that actually needs them, but not in the place that doesn't annoy you.)
> 
> Anyway I read your v1 command and did understand your point in the
> first place. That's why my question was addressed to Bjorn.
> 
> Please also note the resource::start field is of the resource_size_t
> type. So wherever the fields are added, it's better to have them
> defined of that type instead.
> 
> -Serge(y)
> 

Hi Bjorn,

Gentle ping for your feedback on the above discussed two approaches.

Thanks,
Prudhvi
>>
>> - Mani
>>
>> -- 
>> மணிவண்ணன் சதாசிவம்
>
Bjorn Helgaas Aug. 8, 2024, 7:04 p.m. UTC | #11
On Thu, Aug 08, 2024 at 11:30:52AM -0700, Prudhvi Yarlagadda wrote:
> On 8/2/2024 2:22 AM, Serge Semin wrote:
> > On Fri, Aug 02, 2024 at 10:52:06AM +0530, Manivannan Sadhasivam wrote:
> >> On Fri, Aug 02, 2024 at 12:59:57AM +0300, Serge Semin wrote:
> >>> On Thu, Aug 01, 2024 at 02:29:49PM -0700, Prudhvi Yarlagadda wrote:
> >>>> Hi Serge,
> >>>>
> >>>> Thanks for the review comment.
> >>>>
> >>>> On 8/1/2024 12:25 PM, Serge Semin wrote:
> >>>>> On Tue, Jul 23, 2024 at 07:27:18PM -0700, Prudhvi Yarlagadda wrote:
> >>>>>> Both DBI and ATU physical base addresses are needed by pcie_qcom.c
> >>>>>> driver to program the location of DBI and ATU blocks in Qualcomm
> >>>>>> PCIe Controller specific PARF hardware block.
> >>>>>>
> >>>>>> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
> >>>>>> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
> >>>>>> ---
> >>>>>>  drivers/pci/controller/dwc/pcie-designware.c | 2 ++
> >>>>>>  drivers/pci/controller/dwc/pcie-designware.h | 2 ++
> >>>>>>  2 files changed, 4 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> >>>>>> index 1b5aba1f0c92..bc3a5d6b0177 100644
> >>>>>> --- a/drivers/pci/controller/dwc/pcie-designware.c
> >>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> >>>>>> @@ -112,6 +112,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
> >>>>>>  		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
> >>>>>>  		if (IS_ERR(pci->dbi_base))
> >>>>>>  			return PTR_ERR(pci->dbi_base);
> >>>>>> +		pci->dbi_phys_addr = res->start;
> >>>>>>  	}
> >>>>>>  
> >>>>>>  	/* DBI2 is mainly useful for the endpoint controller */
> >>>>>> @@ -134,6 +135,7 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
> >>>>>>  			pci->atu_base = devm_ioremap_resource(pci->dev, res);
> >>>>>>  			if (IS_ERR(pci->atu_base))
> >>>>>>  				return PTR_ERR(pci->atu_base);
> >>>>>> +			pci->atu_phys_addr = res->start;
> >>>>>>  		} else {
> >>>>>>  			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
> >>>>>>  		}
> >>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> >>>>>> index 53c4c8f399c8..efc72989330c 100644
> >>>>>> --- a/drivers/pci/controller/dwc/pcie-designware.h
> >>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> >>>>>> @@ -407,8 +407,10 @@ struct dw_pcie_ops {
> >>>>>>  struct dw_pcie {
> >>>>>>  	struct device		*dev;
> >>>>>>  	void __iomem		*dbi_base;
> >>>>>
> >>>>>> +	phys_addr_t		dbi_phys_addr;
> >>>>>>  	void __iomem		*dbi_base2;
> >>>>>>  	void __iomem		*atu_base;
> >>>>>> +	phys_addr_t		atu_phys_addr;
> >>>>>
> >>>>> What's the point in adding these fields to the generic DW PCIe private
> >>>>> data if they are going to be used in the Qcom glue driver only?
> >>>>>
> >>>>> What about moving them to the qcom_pcie structure and initializing the
> >>>>> fields in some place of the pcie-qcom.c driver?
> >>>>>
> >>>>> -Serge(y)
> >>>>>
> >>>>
> >>>
> >>>> These fields were in pcie-qcom.c driver in the v1 patch[1] and
> >>>> Manivannan suggested to move these fields to 'struct dw_pcie' so that duplication
> >>>> of resource fetching code 'platform_get_resource_byname()' can be avoided.
> >>>>
> >>>> [1] https://lore.kernel.org/linux-pci/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/T/#mf9843386d57e9003de983e24e17de4d54314ff73
> >>>
> >>> Em, polluting the core driver structure with data not being used by
> >>> the core driver but by the glue-code doesn't seem like a better
> >>> alternative to additional platform_get_resource_byname() call in the
> >>> glue-driver. I would have got back v1 version so to keep the core
> >>> driver simpler. Bjorn?
> >>>
> >>
> >> IDK how adding two fields which is very related to DWC code *pollutes* it. Since
> >> there is already 'dbi_base', adding 'dbi_phys_addr' made sense to me even though
> >> only glue drivers are using it. Otherwise, glue drivers have to duplicate the
> >> platform_get_resource_byname() code which I find annoying.
> > 
> > I just explained why it was redundant:
> > 1. adding the fields expands the core private data size for _all_
> > platforms for no reason. (a few bytes but still)
> > 2. the new fields aren't utilized by the core driver, but still
> > defined in the core private data which is first confusing and
> > second implicitly encourages the kernel developers to add another
> > unused or even weakly-related fields in there.
> > 3. the new fields utilized in a single glue-driver and there is a small
> > chance they will be used in another ones. Another story would have
> > been if we had them used in more than one glue-driver...
> > 
> > So from that perspective I find adding these fields to the driver core
> > data less appropriate than duplicating the
> > platform_get_resource_byname() call in a _single_ glue driver. It
> > seems more reasonable to have them defined and utilized in the code
> > that actually needs them, but not in the place that doesn't annoy you.)
> > 
> > Anyway I read your v1 command and did understand your point in the
> > first place. That's why my question was addressed to Bjorn.
> > 
> > Please also note the resource::start field is of the resource_size_t
> > type. So wherever the fields are added, it's better to have them
> > defined of that type instead.
> 
> Gentle ping for your feedback on the above discussed two approaches.

I don't really care one way or the other.  Jingoo and Mani can sort it
out.
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 1b5aba1f0c92..bc3a5d6b0177 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -112,6 +112,7 @@  int dw_pcie_get_resources(struct dw_pcie *pci)
 		pci->dbi_base = devm_pci_remap_cfg_resource(pci->dev, res);
 		if (IS_ERR(pci->dbi_base))
 			return PTR_ERR(pci->dbi_base);
+		pci->dbi_phys_addr = res->start;
 	}
 
 	/* DBI2 is mainly useful for the endpoint controller */
@@ -134,6 +135,7 @@  int dw_pcie_get_resources(struct dw_pcie *pci)
 			pci->atu_base = devm_ioremap_resource(pci->dev, res);
 			if (IS_ERR(pci->atu_base))
 				return PTR_ERR(pci->atu_base);
+			pci->atu_phys_addr = res->start;
 		} else {
 			pci->atu_base = pci->dbi_base + DEFAULT_DBI_ATU_OFFSET;
 		}
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index 53c4c8f399c8..efc72989330c 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -407,8 +407,10 @@  struct dw_pcie_ops {
 struct dw_pcie {
 	struct device		*dev;
 	void __iomem		*dbi_base;
+	phys_addr_t		dbi_phys_addr;
 	void __iomem		*dbi_base2;
 	void __iomem		*atu_base;
+	phys_addr_t		atu_phys_addr;
 	size_t			atu_size;
 	u32			num_ib_windows;
 	u32			num_ob_windows;