[PATCH v3 0/2] PCI: qcom: Avoid DBI and ATU register space mirroring

[PATCH v3 0/2] PCI: qcom: Avoid DBI and ATU register space mirroring

[Prudhvi Yarlagadda]

Qualcomm PCIe controller has a wrapper called PARF which supports the
relocation of DBI and ATU address space within the system memory. PARF
gets the location of DBI and ATU from PARF_DBI_BASE_ADDR and
PARF_ATU_BASE_ADDR registers. PARF also mirrors the memory block
containing DBI and ATU registers within the system memory. And the size
of this memory block is programmed in PARF_SLV_ADDR_SPACE_SIZE register.

Power on reset of values of the above mentioned registers are good enough
on platforms which require smaller size (less than 16MB) BAR memory. For
platforms that need bigger BAR memory size, this mirroring of DBI and ATU
address space by PARF conflicts with BAR memory.

So to allow usage of bigger size of BAR, it is required to program
PARF registers to prevent mirroring of DBI and ATU blocks and provide
the physical addresses of DBI and ATU to PARF.

This patch series stores physical addresses of DBI and ATU address space
in 'struct dw_pcie' and programs the required PARF registers in the
pcie_qcom.c driver.

Changes in v3:
- Updated the functions qcom_pcie_configure_dbi_atu_base() and
  qcom_pcie_configure_dbi_base() to make having 'dbi_phys_addr' mandatory
  before programming PARF_SLV_ADDR_SPACE_SIZE register as suggested by
  Mayank Rana.
- Link to v2: https://lore.kernel.org/linux-pci/20240718051258.1115271-1-quic_pyarlaga@quicinc.com/T/

Changes in v2:
- Updated commit message as suggested by Bjorn Helgaas.
- Updated function name from qcom_pcie_avoid_dbi_atu_mirroring()
  to qcom_pcie_configure_dbi_atu_base() as suggested by Bjorn Helgaas.
- Removed check for pdev in qcom_pcie_configure_dbi_atu_base() as
  suggested by Bjorn Helgaas.
- Moved the qcom_pcie_configure_dbi_atu_base() call in the
  qcom_pcie_init_2_7_0() to the same place where PARF_DBI_BASE_ADDR
  register is being programmed as suggested by Bjorn Helgaas.
- Added 'dbi_phys_addr', 'atu_phys_addr' in the 'struct dw_pcie' to store
  the physical addresses of dbi, atu base registers in
  dw_pcie_get_resources() as suggested by Manivannan Sadhasivam.
- Added separate functions qcom_pcie_configure_dbi_atu_base() and
  qcom_pcie_configure_dbi_base() to program PARF register of different
  PARF versions. This is to disable DBI mirroring in all Qualcomm PCIe
  controllers as suggested by Manivannan Sadhasivam.
- Link to v1: https://lore.kernel.org/linux-pci/a01404d2-2f4d-4fb8-af9d-3db66d39acf7@quicinc.com/T/

Tested:
- Validated NVME functionality with PCIe6a on x1e80100 platform.
- Validated WiFi functionality with PCIe4 on x1e80100 platform.

Prudhvi Yarlagadda (2):
  PCI: dwc: Add dbi_phys_addr and atu_phys_addr to struct dw_pcie
  PCI: qcom: Avoid DBI and ATU register space mirror to BAR/MMIO region

 drivers/pci/controller/dwc/pcie-designware.c |  2 +
 drivers/pci/controller/dwc/pcie-designware.h |  2 +
 drivers/pci/controller/dwc/pcie-qcom.c       | 62 ++++++++++++++------
 3 files changed, 49 insertions(+), 17 deletions(-)

-- 
2.25.1

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

[Prudhvi Yarlagadda]

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

[PATCH v3 2/2] PCI: qcom: Avoid DBI and ATU register space mirror to BAR/MMIO region

[Prudhvi Yarlagadda]

PARF hardware block which is a wrapper on top of DWC PCIe controller
mirrors the DBI and ATU register space. It uses PARF_SLV_ADDR_SPACE_SIZE
register to get the size of the memory block to be mirrored and uses
PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR registers to determine the base
address of DBI and ATU space inside the memory block that is being
mirrored.

When a memory region which is located above the SLV_ADDR_SPACE_SIZE
boundary is used for BAR region then there could be an overlap of DBI and
ATU address space that is getting mirrored and the BAR region. This
results in DBI and ATU address space contents getting updated when a PCIe
function driver tries updating the BAR/MMIO memory region. Reference
memory map of the PCIe memory region with DBI and ATU address space
overlapping BAR region is as below.

                        |---------------|
                        |               |
                        |               |
        ------- --------|---------------|
           |       |    |---------------|
           |       |    |       DBI     |
           |       |    |---------------|---->DBI_BASE_ADDR
           |       |    |               |
           |       |    |               |
           |    PCIe    |               |---->2*SLV_ADDR_SPACE_SIZE
           |    BAR/MMIO|---------------|
           |    Region  |       ATU     |
           |       |    |---------------|---->ATU_BASE_ADDR
           |       |    |               |
        PCIe       |    |---------------|
        Memory     |    |       DBI     |
        Region     |    |---------------|---->DBI_BASE_ADDR
           |       |    |               |
           |    --------|               |
           |            |               |---->SLV_ADDR_SPACE_SIZE
           |            |---------------|
           |            |       ATU     |
           |            |---------------|---->ATU_BASE_ADDR
           |            |               |
           |            |---------------|
           |            |       DBI     |
           |            |---------------|---->DBI_BASE_ADDR
           |            |               |
           |            |               |
        ----------------|---------------|
                        |               |
                        |               |
                        |               |
                        |---------------|

Currently memory region beyond the SLV_ADDR_SPACE_SIZE boundary is not
used for BAR region which is why the above mentioned issue is not
encountered. This issue is discovered as part of internal testing when we
tried moving the BAR region beyond the SLV_ADDR_SPACE_SIZE boundary. Hence
we are trying to fix this.

As PARF hardware block mirrors DBI and ATU register space after every
PARF_SLV_ADDR_SPACE_SIZE (default 0x1000000) boundary multiple, write
U32_MAX (all 0xFF's) to PARF_SLV_ADDR_SPACE_SIZE register to avoid
mirroring DBI and ATU to BAR/MMIO region. Write the physical base address
of DBI and ATU register blocks to PARF_DBI_BASE_ADDR (default 0x0) and
PARF_ATU_BASE_ADDR (default 0x1000) respectively to make sure DBI and ATU
blocks are at expected memory locations.

The register offsets PARF_DBI_BASE_ADDR_V2, PARF_SLV_ADDR_SPACE_SIZE_V2
and PARF_ATU_BASE_ADDR are applicable for platforms that use PARF
Qcom IP rev 1.9.0, 2.7.0 and 2.9.0. PARF_DBI_BASE_ADDR_V2 and
PARF_SLV_ADDR_SPACE_SIZE_V2 are applicable for PARF Qcom IP rev 2.3.3.
PARF_DBI_BASE_ADDR and PARF_SLV_ADDR_SPACE_SIZE are applicable for PARF
Qcom IP rev 1.0.0, 2.3.2 and 2.4.0. Updating the init()/post_init()
functions of the respective PARF versions to program applicable
PARF_DBI_BASE_ADDR, PARF_SLV_ADDR_SPACE_SIZE and PARF_ATU_BASE_ADDR
register offsets. And remove the unused SLV_ADDR_SPACE_SZ macro.

Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 62 +++++++++++++++++++-------
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 0180edf3310e..6976efb8e2f0 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -45,6 +45,7 @@
 #define PARF_PHY_REFCLK				0x4c
 #define PARF_CONFIG_BITS			0x50
 #define PARF_DBI_BASE_ADDR			0x168
+#define PARF_SLV_ADDR_SPACE_SIZE		0x16C
 #define PARF_MHI_CLOCK_RESET_CTRL		0x174
 #define PARF_AXI_MSTR_WR_ADDR_HALT		0x178
 #define PARF_AXI_MSTR_WR_ADDR_HALT_V2		0x1a8
@@ -52,8 +53,13 @@
 #define PARF_LTSSM				0x1b0
 #define PARF_SID_OFFSET				0x234
 #define PARF_BDF_TRANSLATE_CFG			0x24c
-#define PARF_SLV_ADDR_SPACE_SIZE		0x358
+#define PARF_DBI_BASE_ADDR_V2			0x350
+#define PARF_DBI_BASE_ADDR_V2_HI		0x354
+#define PARF_SLV_ADDR_SPACE_SIZE_V2		0x358
+#define PARF_SLV_ADDR_SPACE_SIZE_V2_HI		0x35C
 #define PARF_NO_SNOOP_OVERIDE			0x3d4
+#define PARF_ATU_BASE_ADDR			0x634
+#define PARF_ATU_BASE_ADDR_HI			0x638
 #define PARF_DEVICE_TYPE			0x1000
 #define PARF_BDF_TO_SID_TABLE_N			0x2000
 #define PARF_BDF_TO_SID_CFG			0x2c00
@@ -107,9 +113,6 @@
 /* PARF_CONFIG_BITS register fields */
 #define PHY_RX0_EQ(x)				FIELD_PREP(GENMASK(26, 24), x)
 
-/* PARF_SLV_ADDR_SPACE_SIZE register value */
-#define SLV_ADDR_SPACE_SZ			0x10000000
-
 /* PARF_MHI_CLOCK_RESET_CTRL register fields */
 #define AHB_CLK_EN				BIT(0)
 #define MSTR_AXI_CLK_EN				BIT(1)
@@ -324,6 +327,39 @@ static void qcom_pcie_clear_hpc(struct dw_pcie *pci)
 	dw_pcie_dbi_ro_wr_dis(pci);
 }
 
+static void qcom_pcie_configure_dbi_base(struct qcom_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+
+	if (pci->dbi_phys_addr) {
+		writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf +
+							PARF_DBI_BASE_ADDR);
+		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
+	}
+}
+
+static void qcom_pcie_configure_dbi_atu_base(struct qcom_pcie *pcie)
+{
+	struct dw_pcie *pci = pcie->pci;
+
+	if (pci->dbi_phys_addr) {
+		writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf +
+							PARF_DBI_BASE_ADDR_V2);
+		writel(upper_32_bits(pci->dbi_phys_addr), pcie->parf +
+						PARF_DBI_BASE_ADDR_V2_HI);
+
+		if (pci->atu_phys_addr) {
+			writel(lower_32_bits(pci->atu_phys_addr), pcie->parf +
+							PARF_ATU_BASE_ADDR);
+			writel(upper_32_bits(pci->atu_phys_addr), pcie->parf +
+							PARF_ATU_BASE_ADDR_HI);
+		}
+
+		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE_V2);
+		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE_V2_HI);
+	}
+}
+
 static void qcom_pcie_2_1_0_ltssm_enable(struct qcom_pcie *pcie)
 {
 	u32 val;
@@ -540,8 +576,7 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
 
 static int qcom_pcie_post_init_1_0_0(struct qcom_pcie *pcie)
 {
-	/* change DBI base address */
-	writel(0, pcie->parf + PARF_DBI_BASE_ADDR);
+	qcom_pcie_configure_dbi_base(pcie);
 
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		u32 val = readl(pcie->parf + PARF_AXI_MSTR_WR_ADDR_HALT);
@@ -628,8 +663,7 @@ static int qcom_pcie_post_init_2_3_2(struct qcom_pcie *pcie)
 	val &= ~PHY_TEST_PWR_DOWN;
 	writel(val, pcie->parf + PARF_PHY_CTRL);
 
-	/* change DBI base address */
-	writel(0, pcie->parf + PARF_DBI_BASE_ADDR);
+	qcom_pcie_configure_dbi_base(pcie);
 
 	/* MAC PHY_POWERDOWN MUX DISABLE  */
 	val = readl(pcie->parf + PARF_SYS_CTRL);
@@ -811,13 +845,11 @@ static int qcom_pcie_post_init_2_3_3(struct qcom_pcie *pcie)
 	u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
 	u32 val;
 
-	writel(SLV_ADDR_SPACE_SZ, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
-
 	val = readl(pcie->parf + PARF_PHY_CTRL);
 	val &= ~PHY_TEST_PWR_DOWN;
 	writel(val, pcie->parf + PARF_PHY_CTRL);
 
-	writel(0, pcie->parf + PARF_DBI_BASE_ADDR);
+	qcom_pcie_configure_dbi_atu_base(pcie);
 
 	writel(MST_WAKEUP_EN | SLV_WAKEUP_EN | MSTR_ACLK_CGC_DIS
 		| SLV_ACLK_CGC_DIS | CORE_CLK_CGC_DIS |
@@ -913,8 +945,7 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 	val &= ~PHY_TEST_PWR_DOWN;
 	writel(val, pcie->parf + PARF_PHY_CTRL);
 
-	/* change DBI base address */
-	writel(0, pcie->parf + PARF_DBI_BASE_ADDR);
+	qcom_pcie_configure_dbi_atu_base(pcie);
 
 	/* MAC PHY_POWERDOWN MUX DISABLE  */
 	val = readl(pcie->parf + PARF_SYS_CTRL);
@@ -1123,14 +1154,11 @@ static int qcom_pcie_post_init_2_9_0(struct qcom_pcie *pcie)
 	u32 val;
 	int i;
 
-	writel(SLV_ADDR_SPACE_SZ,
-		pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
-
 	val = readl(pcie->parf + PARF_PHY_CTRL);
 	val &= ~PHY_TEST_PWR_DOWN;
 	writel(val, pcie->parf + PARF_PHY_CTRL);
 
-	writel(0, pcie->parf + PARF_DBI_BASE_ADDR);
+	qcom_pcie_configure_dbi_atu_base(pcie);
 
 	writel(DEVICE_TYPE_RC, pcie->parf + PARF_DEVICE_TYPE);
 	writel(BYPASS | MSTR_AXI_CLK_EN | AHB_CLK_EN,
-- 
2.25.1

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

[Manivannan Sadhasivam]

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
> 

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

Re: [PATCH v3 2/2] PCI: qcom: Avoid DBI and ATU register space mirror to BAR/MMIO region

[Manivannan Sadhasivam]

Subject:

PCI: qcom: Disable mirroring of DBI and iATU register space in BAR/MMIO region

On Tue, Jul 23, 2024 at 07:27:19PM -0700, Prudhvi Yarlagadda wrote:
> PARF hardware block which is a wrapper on top of DWC PCIe controller
> mirrors the DBI and ATU register space. It uses PARF_SLV_ADDR_SPACE_SIZE
> register to get the size of the memory block to be mirrored and uses
> PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR registers to determine the base
> address of DBI and ATU space inside the memory block that is being
> mirrored.
> 
> When a memory region which is located above the SLV_ADDR_SPACE_SIZE
> boundary is used for BAR region then there could be an overlap of DBI and
> ATU address space that is getting mirrored and the BAR region. This
> results in DBI and ATU address space contents getting updated when a PCIe
> function driver tries updating the BAR/MMIO memory region. Reference
> memory map of the PCIe memory region with DBI and ATU address space
> overlapping BAR region is as below.
> 
>                         |---------------|
>                         |               |
>                         |               |
>         ------- --------|---------------|
>            |       |    |---------------|
>            |       |    |       DBI     |
>            |       |    |---------------|---->DBI_BASE_ADDR
>            |       |    |               |
>            |       |    |               |
>            |    PCIe    |               |---->2*SLV_ADDR_SPACE_SIZE
>            |    BAR/MMIO|---------------|
>            |    Region  |       ATU     |
>            |       |    |---------------|---->ATU_BASE_ADDR
>            |       |    |               |
>         PCIe       |    |---------------|
>         Memory     |    |       DBI     |
>         Region     |    |---------------|---->DBI_BASE_ADDR
>            |       |    |               |
>            |    --------|               |
>            |            |               |---->SLV_ADDR_SPACE_SIZE
>            |            |---------------|
>            |            |       ATU     |
>            |            |---------------|---->ATU_BASE_ADDR
>            |            |               |
>            |            |---------------|
>            |            |       DBI     |
>            |            |---------------|---->DBI_BASE_ADDR
>            |            |               |
>            |            |               |
>         ----------------|---------------|
>                         |               |
>                         |               |
>                         |               |
>                         |---------------|
> 
> Currently memory region beyond the SLV_ADDR_SPACE_SIZE boundary is not
> used for BAR region which is why the above mentioned issue is not
> encountered. This issue is discovered as part of internal testing when we
> tried moving the BAR region beyond the SLV_ADDR_SPACE_SIZE boundary. Hence
> we are trying to fix this.
> 
> As PARF hardware block mirrors DBI and ATU register space after every
> PARF_SLV_ADDR_SPACE_SIZE (default 0x1000000) boundary multiple, write
> U32_MAX (all 0xFF's) to PARF_SLV_ADDR_SPACE_SIZE register to avoid
> mirroring DBI and ATU to BAR/MMIO region. Write the physical base address
> of DBI and ATU register blocks to PARF_DBI_BASE_ADDR (default 0x0) and
> PARF_ATU_BASE_ADDR (default 0x1000) respectively to make sure DBI and ATU
> blocks are at expected memory locations.
> 
> The register offsets PARF_DBI_BASE_ADDR_V2, PARF_SLV_ADDR_SPACE_SIZE_V2
> and PARF_ATU_BASE_ADDR are applicable for platforms that use PARF

There is no 'PARF Qcom IP', just 'Qcom IP'. Here and below.

> Qcom IP rev 1.9.0, 2.7.0 and 2.9.0. PARF_DBI_BASE_ADDR_V2 and
> PARF_SLV_ADDR_SPACE_SIZE_V2 are applicable for PARF Qcom IP rev 2.3.3.
> PARF_DBI_BASE_ADDR and PARF_SLV_ADDR_SPACE_SIZE are applicable for PARF
> Qcom IP rev 1.0.0, 2.3.2 and 2.4.0. Updating the init()/post_init()

Use imperative tone in commit message. s/Updating/Update

> functions of the respective PARF versions to program applicable
> PARF_DBI_BASE_ADDR, PARF_SLV_ADDR_SPACE_SIZE and PARF_ATU_BASE_ADDR
> register offsets. And remove the unused SLV_ADDR_SPACE_SZ macro.
> 
> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 62 +++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 0180edf3310e..6976efb8e2f0 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -45,6 +45,7 @@
>  #define PARF_PHY_REFCLK				0x4c
>  #define PARF_CONFIG_BITS			0x50
>  #define PARF_DBI_BASE_ADDR			0x168
> +#define PARF_SLV_ADDR_SPACE_SIZE		0x16C

Use lowercase for hex.

Rest LGTM! With above mentioned changes,

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

- Mani

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

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

[Prudhvi Yarlagadda]

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

Re: [PATCH v3 2/2] PCI: qcom: Avoid DBI and ATU register space mirror to BAR/MMIO region

[Prudhvi Yarlagadda]

Hi Manivannan,

Thanks for the review comments.

On 7/24/2024 7:10 AM, Manivannan Sadhasivam wrote:
> Subject:
> 
> PCI: qcom: Disable mirroring of DBI and iATU register space in BAR/MMIO region
> 

ACK. I will update it in the next patch.

> On Tue, Jul 23, 2024 at 07:27:19PM -0700, Prudhvi Yarlagadda wrote:
>> PARF hardware block which is a wrapper on top of DWC PCIe controller
>> mirrors the DBI and ATU register space. It uses PARF_SLV_ADDR_SPACE_SIZE
>> register to get the size of the memory block to be mirrored and uses
>> PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR registers to determine the base
>> address of DBI and ATU space inside the memory block that is being
>> mirrored.
>>
>> When a memory region which is located above the SLV_ADDR_SPACE_SIZE
>> boundary is used for BAR region then there could be an overlap of DBI and
>> ATU address space that is getting mirrored and the BAR region. This
>> results in DBI and ATU address space contents getting updated when a PCIe
>> function driver tries updating the BAR/MMIO memory region. Reference
>> memory map of the PCIe memory region with DBI and ATU address space
>> overlapping BAR region is as below.
>>
>>                         |---------------|
>>                         |               |
>>                         |               |
>>         ------- --------|---------------|
>>            |       |    |---------------|
>>            |       |    |       DBI     |
>>            |       |    |---------------|---->DBI_BASE_ADDR
>>            |       |    |               |
>>            |       |    |               |
>>            |    PCIe    |               |---->2*SLV_ADDR_SPACE_SIZE
>>            |    BAR/MMIO|---------------|
>>            |    Region  |       ATU     |
>>            |       |    |---------------|---->ATU_BASE_ADDR
>>            |       |    |               |
>>         PCIe       |    |---------------|
>>         Memory     |    |       DBI     |
>>         Region     |    |---------------|---->DBI_BASE_ADDR
>>            |       |    |               |
>>            |    --------|               |
>>            |            |               |---->SLV_ADDR_SPACE_SIZE
>>            |            |---------------|
>>            |            |       ATU     |
>>            |            |---------------|---->ATU_BASE_ADDR
>>            |            |               |
>>            |            |---------------|
>>            |            |       DBI     |
>>            |            |---------------|---->DBI_BASE_ADDR
>>            |            |               |
>>            |            |               |
>>         ----------------|---------------|
>>                         |               |
>>                         |               |
>>                         |               |
>>                         |---------------|
>>
>> Currently memory region beyond the SLV_ADDR_SPACE_SIZE boundary is not
>> used for BAR region which is why the above mentioned issue is not
>> encountered. This issue is discovered as part of internal testing when we
>> tried moving the BAR region beyond the SLV_ADDR_SPACE_SIZE boundary. Hence
>> we are trying to fix this.
>>
>> As PARF hardware block mirrors DBI and ATU register space after every
>> PARF_SLV_ADDR_SPACE_SIZE (default 0x1000000) boundary multiple, write
>> U32_MAX (all 0xFF's) to PARF_SLV_ADDR_SPACE_SIZE register to avoid
>> mirroring DBI and ATU to BAR/MMIO region. Write the physical base address
>> of DBI and ATU register blocks to PARF_DBI_BASE_ADDR (default 0x0) and
>> PARF_ATU_BASE_ADDR (default 0x1000) respectively to make sure DBI and ATU
>> blocks are at expected memory locations.
>>
>> The register offsets PARF_DBI_BASE_ADDR_V2, PARF_SLV_ADDR_SPACE_SIZE_V2
>> and PARF_ATU_BASE_ADDR are applicable for platforms that use PARF
> 
> There is no 'PARF Qcom IP', just 'Qcom IP'. Here and below.
> 

ACK. I will update it in the next patch.

>> Qcom IP rev 1.9.0, 2.7.0 and 2.9.0. PARF_DBI_BASE_ADDR_V2 and
>> PARF_SLV_ADDR_SPACE_SIZE_V2 are applicable for PARF Qcom IP rev 2.3.3.
>> PARF_DBI_BASE_ADDR and PARF_SLV_ADDR_SPACE_SIZE are applicable for PARF
>> Qcom IP rev 1.0.0, 2.3.2 and 2.4.0. Updating the init()/post_init()
> 
> Use imperative tone in commit message. s/Updating/Update
> 

ACK. I will update it in the next patch.

>> functions of the respective PARF versions to program applicable
>> PARF_DBI_BASE_ADDR, PARF_SLV_ADDR_SPACE_SIZE and PARF_ATU_BASE_ADDR
>> register offsets. And remove the unused SLV_ADDR_SPACE_SZ macro.
>>
>> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
>> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
>> ---
>>  drivers/pci/controller/dwc/pcie-qcom.c | 62 +++++++++++++++++++-------
>>  1 file changed, 45 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 0180edf3310e..6976efb8e2f0 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -45,6 +45,7 @@
>>  #define PARF_PHY_REFCLK				0x4c
>>  #define PARF_CONFIG_BITS			0x50
>>  #define PARF_DBI_BASE_ADDR			0x168
>> +#define PARF_SLV_ADDR_SPACE_SIZE		0x16C
> 
> Use lowercase for hex.
> 

ACK. I will update it in the next patch.

Thanks,
Prudhvi
> Rest LGTM! With above mentioned changes,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> - Mani
> 

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

[Bjorn Helgaas]

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.

Re: [PATCH v3 2/2] PCI: qcom: Avoid DBI and ATU register space mirror to BAR/MMIO region

[Bjorn Helgaas]

On Tue, Jul 23, 2024 at 07:27:19PM -0700, Prudhvi Yarlagadda wrote:
> PARF hardware block which is a wrapper on top of DWC PCIe controller
> mirrors the DBI and ATU register space. It uses PARF_SLV_ADDR_SPACE_SIZE
> register to get the size of the memory block to be mirrored and uses
> PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR registers to determine the base
> address of DBI and ATU space inside the memory block that is being
> mirrored.
> 
> When a memory region which is located above the SLV_ADDR_SPACE_SIZE
> boundary is used for BAR region then there could be an overlap of DBI and
> ATU address space that is getting mirrored and the BAR region. This
> results in DBI and ATU address space contents getting updated when a PCIe
> function driver tries updating the BAR/MMIO memory region. Reference
> memory map of the PCIe memory region with DBI and ATU address space
> overlapping BAR region is as below.
> 
>                         |---------------|
>                         |               |
>                         |               |
>         ------- --------|---------------|
>            |       |    |---------------|
>            |       |    |       DBI     |
>            |       |    |---------------|---->DBI_BASE_ADDR
>            |       |    |               |
>            |       |    |               |
>            |    PCIe    |               |---->2*SLV_ADDR_SPACE_SIZE
>            |    BAR/MMIO|---------------|
>            |    Region  |       ATU     |
>            |       |    |---------------|---->ATU_BASE_ADDR
>            |       |    |               |
>         PCIe       |    |---------------|
>         Memory     |    |       DBI     |
>         Region     |    |---------------|---->DBI_BASE_ADDR
>            |       |    |               |
>            |    --------|               |
>            |            |               |---->SLV_ADDR_SPACE_SIZE
>            |            |---------------|
>            |            |       ATU     |
>            |            |---------------|---->ATU_BASE_ADDR
>            |            |               |
>            |            |---------------|
>            |            |       DBI     |
>            |            |---------------|---->DBI_BASE_ADDR
>            |            |               |
>            |            |               |
>         ----------------|---------------|
>                         |               |
>                         |               |
>                         |               |
>                         |---------------|
> 
> Currently memory region beyond the SLV_ADDR_SPACE_SIZE boundary is not
> used for BAR region which is why the above mentioned issue is not
> encountered. This issue is discovered as part of internal testing when we
> tried moving the BAR region beyond the SLV_ADDR_SPACE_SIZE boundary. Hence
> we are trying to fix this.
> 
> As PARF hardware block mirrors DBI and ATU register space after every
> PARF_SLV_ADDR_SPACE_SIZE (default 0x1000000) boundary multiple, write
> U32_MAX (all 0xFF's) to PARF_SLV_ADDR_SPACE_SIZE register to avoid
> mirroring DBI and ATU to BAR/MMIO region. Write the physical base address
> of DBI and ATU register blocks to PARF_DBI_BASE_ADDR (default 0x0) and
> PARF_ATU_BASE_ADDR (default 0x1000) respectively to make sure DBI and ATU
> blocks are at expected memory locations.
> 
> The register offsets PARF_DBI_BASE_ADDR_V2, PARF_SLV_ADDR_SPACE_SIZE_V2
> and PARF_ATU_BASE_ADDR are applicable for platforms that use PARF
> Qcom IP rev 1.9.0, 2.7.0 and 2.9.0. PARF_DBI_BASE_ADDR_V2 and
> PARF_SLV_ADDR_SPACE_SIZE_V2 are applicable for PARF Qcom IP rev 2.3.3.
> PARF_DBI_BASE_ADDR and PARF_SLV_ADDR_SPACE_SIZE are applicable for PARF
> Qcom IP rev 1.0.0, 2.3.2 and 2.4.0. Updating the init()/post_init()
> functions of the respective PARF versions to program applicable
> PARF_DBI_BASE_ADDR, PARF_SLV_ADDR_SPACE_SIZE and PARF_ATU_BASE_ADDR
> register offsets. And remove the unused SLV_ADDR_SPACE_SZ macro.
> 
> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 62 +++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 0180edf3310e..6976efb8e2f0 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -45,6 +45,7 @@
>  #define PARF_PHY_REFCLK				0x4c
>  #define PARF_CONFIG_BITS			0x50
>  #define PARF_DBI_BASE_ADDR			0x168
> +#define PARF_SLV_ADDR_SPACE_SIZE		0x16C
>  #define PARF_MHI_CLOCK_RESET_CTRL		0x174
>  #define PARF_AXI_MSTR_WR_ADDR_HALT		0x178
>  #define PARF_AXI_MSTR_WR_ADDR_HALT_V2		0x1a8
> @@ -52,8 +53,13 @@
>  #define PARF_LTSSM				0x1b0
>  #define PARF_SID_OFFSET				0x234
>  #define PARF_BDF_TRANSLATE_CFG			0x24c
> -#define PARF_SLV_ADDR_SPACE_SIZE		0x358
> +#define PARF_DBI_BASE_ADDR_V2			0x350
> +#define PARF_DBI_BASE_ADDR_V2_HI		0x354
> +#define PARF_SLV_ADDR_SPACE_SIZE_V2		0x358
> +#define PARF_SLV_ADDR_SPACE_SIZE_V2_HI		0x35C
>  #define PARF_NO_SNOOP_OVERIDE			0x3d4
> +#define PARF_ATU_BASE_ADDR			0x634
> +#define PARF_ATU_BASE_ADDR_HI			0x638
>  #define PARF_DEVICE_TYPE			0x1000
>  #define PARF_BDF_TO_SID_TABLE_N			0x2000
>  #define PARF_BDF_TO_SID_CFG			0x2c00
> @@ -107,9 +113,6 @@
>  /* PARF_CONFIG_BITS register fields */
>  #define PHY_RX0_EQ(x)				FIELD_PREP(GENMASK(26, 24), x)
>  
> -/* PARF_SLV_ADDR_SPACE_SIZE register value */
> -#define SLV_ADDR_SPACE_SZ			0x10000000
> -
>  /* PARF_MHI_CLOCK_RESET_CTRL register fields */
>  #define AHB_CLK_EN				BIT(0)
>  #define MSTR_AXI_CLK_EN				BIT(1)
> @@ -324,6 +327,39 @@ static void qcom_pcie_clear_hpc(struct dw_pcie *pci)
>  	dw_pcie_dbi_ro_wr_dis(pci);
>  }
>  
> +static void qcom_pcie_configure_dbi_base(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +
> +	if (pci->dbi_phys_addr) {
> +		writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf +
> +							PARF_DBI_BASE_ADDR);
> +		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> +	}
> +}
> +
> +static void qcom_pcie_configure_dbi_atu_base(struct qcom_pcie *pcie)
> +{
> +	struct dw_pcie *pci = pcie->pci;
> +
> +	if (pci->dbi_phys_addr) {
> +		writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf +
> +							PARF_DBI_BASE_ADDR_V2);
> +		writel(upper_32_bits(pci->dbi_phys_addr), pcie->parf +
> +						PARF_DBI_BASE_ADDR_V2_HI);
> +
> +		if (pci->atu_phys_addr) {
> +			writel(lower_32_bits(pci->atu_phys_addr), pcie->parf +
> +							PARF_ATU_BASE_ADDR);
> +			writel(upper_32_bits(pci->atu_phys_addr), pcie->parf +
> +							PARF_ATU_BASE_ADDR_HI);
> +		}
> +
> +		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE_V2);
> +		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE_V2_HI);
> +	}
> +}

These functions write CPU physical addresses into registers.  I don't
know where these registers live.  If they are on the PCI side of the
world, they most likely should contain PCI bus addresses, not CPU
physical addresses.

In some systems, PCI bus addresses are the same as CPU physical
addresses, but on many systems they are not the same, so it's better
if we don't make implicit assumptions that they are the same.  

Bjorn

Re: [PATCH v3 2/2] PCI: qcom: Avoid DBI and ATU register space mirror to BAR/MMIO region

[Prudhvi Yarlagadda]

Hi Bjorn,

Thanks for the review comments.

On 7/24/2024 11:43 AM, Bjorn Helgaas wrote:
> On Tue, Jul 23, 2024 at 07:27:19PM -0700, Prudhvi Yarlagadda wrote:
>> PARF hardware block which is a wrapper on top of DWC PCIe controller
>> mirrors the DBI and ATU register space. It uses PARF_SLV_ADDR_SPACE_SIZE
>> register to get the size of the memory block to be mirrored and uses
>> PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR registers to determine the base
>> address of DBI and ATU space inside the memory block that is being
>> mirrored.
>>
>> When a memory region which is located above the SLV_ADDR_SPACE_SIZE
>> boundary is used for BAR region then there could be an overlap of DBI and
>> ATU address space that is getting mirrored and the BAR region. This
>> results in DBI and ATU address space contents getting updated when a PCIe
>> function driver tries updating the BAR/MMIO memory region. Reference
>> memory map of the PCIe memory region with DBI and ATU address space
>> overlapping BAR region is as below.
>>
>>                         |---------------|
>>                         |               |
>>                         |               |
>>         ------- --------|---------------|
>>            |       |    |---------------|
>>            |       |    |       DBI     |
>>            |       |    |---------------|---->DBI_BASE_ADDR
>>            |       |    |               |
>>            |       |    |               |
>>            |    PCIe    |               |---->2*SLV_ADDR_SPACE_SIZE
>>            |    BAR/MMIO|---------------|
>>            |    Region  |       ATU     |
>>            |       |    |---------------|---->ATU_BASE_ADDR
>>            |       |    |               |
>>         PCIe       |    |---------------|
>>         Memory     |    |       DBI     |
>>         Region     |    |---------------|---->DBI_BASE_ADDR
>>            |       |    |               |
>>            |    --------|               |
>>            |            |               |---->SLV_ADDR_SPACE_SIZE
>>            |            |---------------|
>>            |            |       ATU     |
>>            |            |---------------|---->ATU_BASE_ADDR
>>            |            |               |
>>            |            |---------------|
>>            |            |       DBI     |
>>            |            |---------------|---->DBI_BASE_ADDR
>>            |            |               |
>>            |            |               |
>>         ----------------|---------------|
>>                         |               |
>>                         |               |
>>                         |               |
>>                         |---------------|
>>
>> Currently memory region beyond the SLV_ADDR_SPACE_SIZE boundary is not
>> used for BAR region which is why the above mentioned issue is not
>> encountered. This issue is discovered as part of internal testing when we
>> tried moving the BAR region beyond the SLV_ADDR_SPACE_SIZE boundary. Hence
>> we are trying to fix this.
>>
>> As PARF hardware block mirrors DBI and ATU register space after every
>> PARF_SLV_ADDR_SPACE_SIZE (default 0x1000000) boundary multiple, write
>> U32_MAX (all 0xFF's) to PARF_SLV_ADDR_SPACE_SIZE register to avoid
>> mirroring DBI and ATU to BAR/MMIO region. Write the physical base address
>> of DBI and ATU register blocks to PARF_DBI_BASE_ADDR (default 0x0) and
>> PARF_ATU_BASE_ADDR (default 0x1000) respectively to make sure DBI and ATU
>> blocks are at expected memory locations.
>>
>> The register offsets PARF_DBI_BASE_ADDR_V2, PARF_SLV_ADDR_SPACE_SIZE_V2
>> and PARF_ATU_BASE_ADDR are applicable for platforms that use PARF
>> Qcom IP rev 1.9.0, 2.7.0 and 2.9.0. PARF_DBI_BASE_ADDR_V2 and
>> PARF_SLV_ADDR_SPACE_SIZE_V2 are applicable for PARF Qcom IP rev 2.3.3.
>> PARF_DBI_BASE_ADDR and PARF_SLV_ADDR_SPACE_SIZE are applicable for PARF
>> Qcom IP rev 1.0.0, 2.3.2 and 2.4.0. Updating the init()/post_init()
>> functions of the respective PARF versions to program applicable
>> PARF_DBI_BASE_ADDR, PARF_SLV_ADDR_SPACE_SIZE and PARF_ATU_BASE_ADDR
>> register offsets. And remove the unused SLV_ADDR_SPACE_SZ macro.
>>
>> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
>> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
>> ---
>>  drivers/pci/controller/dwc/pcie-qcom.c | 62 +++++++++++++++++++-------
>>  1 file changed, 45 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 0180edf3310e..6976efb8e2f0 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -45,6 +45,7 @@
>>  #define PARF_PHY_REFCLK				0x4c
>>  #define PARF_CONFIG_BITS			0x50
>>  #define PARF_DBI_BASE_ADDR			0x168
>> +#define PARF_SLV_ADDR_SPACE_SIZE		0x16C
>>  #define PARF_MHI_CLOCK_RESET_CTRL		0x174
>>  #define PARF_AXI_MSTR_WR_ADDR_HALT		0x178
>>  #define PARF_AXI_MSTR_WR_ADDR_HALT_V2		0x1a8
>> @@ -52,8 +53,13 @@
>>  #define PARF_LTSSM				0x1b0
>>  #define PARF_SID_OFFSET				0x234
>>  #define PARF_BDF_TRANSLATE_CFG			0x24c
>> -#define PARF_SLV_ADDR_SPACE_SIZE		0x358
>> +#define PARF_DBI_BASE_ADDR_V2			0x350
>> +#define PARF_DBI_BASE_ADDR_V2_HI		0x354
>> +#define PARF_SLV_ADDR_SPACE_SIZE_V2		0x358
>> +#define PARF_SLV_ADDR_SPACE_SIZE_V2_HI		0x35C
>>  #define PARF_NO_SNOOP_OVERIDE			0x3d4
>> +#define PARF_ATU_BASE_ADDR			0x634
>> +#define PARF_ATU_BASE_ADDR_HI			0x638
>>  #define PARF_DEVICE_TYPE			0x1000
>>  #define PARF_BDF_TO_SID_TABLE_N			0x2000
>>  #define PARF_BDF_TO_SID_CFG			0x2c00
>> @@ -107,9 +113,6 @@
>>  /* PARF_CONFIG_BITS register fields */
>>  #define PHY_RX0_EQ(x)				FIELD_PREP(GENMASK(26, 24), x)
>>  
>> -/* PARF_SLV_ADDR_SPACE_SIZE register value */
>> -#define SLV_ADDR_SPACE_SZ			0x10000000
>> -
>>  /* PARF_MHI_CLOCK_RESET_CTRL register fields */
>>  #define AHB_CLK_EN				BIT(0)
>>  #define MSTR_AXI_CLK_EN				BIT(1)
>> @@ -324,6 +327,39 @@ static void qcom_pcie_clear_hpc(struct dw_pcie *pci)
>>  	dw_pcie_dbi_ro_wr_dis(pci);
>>  }
>>  
>> +static void qcom_pcie_configure_dbi_base(struct qcom_pcie *pcie)
>> +{
>> +	struct dw_pcie *pci = pcie->pci;
>> +
>> +	if (pci->dbi_phys_addr) {
>> +		writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf +
>> +							PARF_DBI_BASE_ADDR);
>> +		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
>> +	}
>> +}
>> +
>> +static void qcom_pcie_configure_dbi_atu_base(struct qcom_pcie *pcie)
>> +{
>> +	struct dw_pcie *pci = pcie->pci;
>> +
>> +	if (pci->dbi_phys_addr) {
>> +		writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf +
>> +							PARF_DBI_BASE_ADDR_V2);
>> +		writel(upper_32_bits(pci->dbi_phys_addr), pcie->parf +
>> +						PARF_DBI_BASE_ADDR_V2_HI);
>> +
>> +		if (pci->atu_phys_addr) {
>> +			writel(lower_32_bits(pci->atu_phys_addr), pcie->parf +
>> +							PARF_ATU_BASE_ADDR);
>> +			writel(upper_32_bits(pci->atu_phys_addr), pcie->parf +
>> +							PARF_ATU_BASE_ADDR_HI);
>> +		}
>> +
>> +		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE_V2);
>> +		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE_V2_HI);
>> +	}
>> +}
> 
> These functions write CPU physical addresses into registers.  I don't
> know where these registers live.  If they are on the PCI side of the
> world, they most likely should contain PCI bus addresses, not CPU
> physical addresses.
> 
> In some systems, PCI bus addresses are the same as CPU physical
> addresses, but on many systems they are not the same, so it's better
> if we don't make implicit assumptions that they are the same.  
> 
> Bjorn

On Qualcomm platforms, CPU physical address and PCI bus addresses for DBI
and ATU registers are same. PARF registers live outside the PCI address space
in the system memory.

There is a mapping logic in the QCOM PARF wrapper which detects any incoming
read/write transactions from the NOC towards PCIe controller and checks its
addresses against PARF_DBI_BASE_ADDR and PARF_ATU_BASE_ADDR registers so that
these transactions can be routed to DBI and ATU registers inside the PCIe
controller.

So, these PARF registers needs to be programmed with base CPU physical addresses
of DBI and ATU as the incoming DBI and ATU transactions from the NOC contain
CPU physical adresses.

Thanks,
Prudhvi

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

[Prudhvi Yarlagadda]

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

Re: [PATCH v3 2/2] PCI: qcom: Avoid DBI and ATU register space mirror to BAR/MMIO region

[Bjorn Helgaas]

On Thu, Jul 25, 2024 at 04:03:56PM -0700, Prudhvi Yarlagadda wrote:
> Hi Bjorn,
> 
> Thanks for the review comments.
> 
> On 7/24/2024 11:43 AM, Bjorn Helgaas wrote:
> > On Tue, Jul 23, 2024 at 07:27:19PM -0700, Prudhvi Yarlagadda wrote:
> >> PARF hardware block which is a wrapper on top of DWC PCIe controller
> >> mirrors the DBI and ATU register space. It uses PARF_SLV_ADDR_SPACE_SIZE
> >> register to get the size of the memory block to be mirrored and uses
> >> PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR registers to determine the base
> >> address of DBI and ATU space inside the memory block that is being
> >> mirrored.
> >>
> >> When a memory region which is located above the SLV_ADDR_SPACE_SIZE
> >> boundary is used for BAR region then there could be an overlap of DBI and
> >> ATU address space that is getting mirrored and the BAR region. This
> >> results in DBI and ATU address space contents getting updated when a PCIe
> >> function driver tries updating the BAR/MMIO memory region. Reference
> >> memory map of the PCIe memory region with DBI and ATU address space
> >> overlapping BAR region is as below.
> >>
> >>                         |---------------|
> >>                         |               |
> >>                         |               |
> >>         ------- --------|---------------|
> >>            |       |    |---------------|
> >>            |       |    |       DBI     |
> >>            |       |    |---------------|---->DBI_BASE_ADDR
> >>            |       |    |               |
> >>            |       |    |               |
> >>            |    PCIe    |               |---->2*SLV_ADDR_SPACE_SIZE
> >>            |    BAR/MMIO|---------------|
> >>            |    Region  |       ATU     |
> >>            |       |    |---------------|---->ATU_BASE_ADDR
> >>            |       |    |               |
> >>         PCIe       |    |---------------|
> >>         Memory     |    |       DBI     |
> >>         Region     |    |---------------|---->DBI_BASE_ADDR
> >>            |       |    |               |
> >>            |    --------|               |
> >>            |            |               |---->SLV_ADDR_SPACE_SIZE
> >>            |            |---------------|
> >>            |            |       ATU     |
> >>            |            |---------------|---->ATU_BASE_ADDR
> >>            |            |               |
> >>            |            |---------------|
> >>            |            |       DBI     |
> >>            |            |---------------|---->DBI_BASE_ADDR
> >>            |            |               |
> >>            |            |               |
> >>         ----------------|---------------|
> >>                         |               |
> >>                         |               |
> >>                         |               |
> >>                         |---------------|
> >>
> >> Currently memory region beyond the SLV_ADDR_SPACE_SIZE boundary is not
> >> used for BAR region which is why the above mentioned issue is not
> >> encountered. This issue is discovered as part of internal testing when we
> >> tried moving the BAR region beyond the SLV_ADDR_SPACE_SIZE boundary. Hence
> >> we are trying to fix this.
> >>
> >> As PARF hardware block mirrors DBI and ATU register space after every
> >> PARF_SLV_ADDR_SPACE_SIZE (default 0x1000000) boundary multiple, write
> >> U32_MAX (all 0xFF's) to PARF_SLV_ADDR_SPACE_SIZE register to avoid
> >> mirroring DBI and ATU to BAR/MMIO region. Write the physical base address
> >> of DBI and ATU register blocks to PARF_DBI_BASE_ADDR (default 0x0) and
> >> PARF_ATU_BASE_ADDR (default 0x1000) respectively to make sure DBI and ATU
> >> blocks are at expected memory locations.
> >>
> >> The register offsets PARF_DBI_BASE_ADDR_V2, PARF_SLV_ADDR_SPACE_SIZE_V2
> >> and PARF_ATU_BASE_ADDR are applicable for platforms that use PARF
> >> Qcom IP rev 1.9.0, 2.7.0 and 2.9.0. PARF_DBI_BASE_ADDR_V2 and
> >> PARF_SLV_ADDR_SPACE_SIZE_V2 are applicable for PARF Qcom IP rev 2.3.3.
> >> PARF_DBI_BASE_ADDR and PARF_SLV_ADDR_SPACE_SIZE are applicable for PARF
> >> Qcom IP rev 1.0.0, 2.3.2 and 2.4.0. Updating the init()/post_init()
> >> functions of the respective PARF versions to program applicable
> >> PARF_DBI_BASE_ADDR, PARF_SLV_ADDR_SPACE_SIZE and PARF_ATU_BASE_ADDR
> >> register offsets. And remove the unused SLV_ADDR_SPACE_SZ macro.
> >>
> >> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
> >> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
> >> ---
> >>  drivers/pci/controller/dwc/pcie-qcom.c | 62 +++++++++++++++++++-------
> >>  1 file changed, 45 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> >> index 0180edf3310e..6976efb8e2f0 100644
> >> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> >> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> >> @@ -45,6 +45,7 @@
> >>  #define PARF_PHY_REFCLK				0x4c
> >>  #define PARF_CONFIG_BITS			0x50
> >>  #define PARF_DBI_BASE_ADDR			0x168
> >> +#define PARF_SLV_ADDR_SPACE_SIZE		0x16C
> >>  #define PARF_MHI_CLOCK_RESET_CTRL		0x174
> >>  #define PARF_AXI_MSTR_WR_ADDR_HALT		0x178
> >>  #define PARF_AXI_MSTR_WR_ADDR_HALT_V2		0x1a8
> >> @@ -52,8 +53,13 @@
> >>  #define PARF_LTSSM				0x1b0
> >>  #define PARF_SID_OFFSET				0x234
> >>  #define PARF_BDF_TRANSLATE_CFG			0x24c
> >> -#define PARF_SLV_ADDR_SPACE_SIZE		0x358
> >> +#define PARF_DBI_BASE_ADDR_V2			0x350
> >> +#define PARF_DBI_BASE_ADDR_V2_HI		0x354
> >> +#define PARF_SLV_ADDR_SPACE_SIZE_V2		0x358
> >> +#define PARF_SLV_ADDR_SPACE_SIZE_V2_HI		0x35C
> >>  #define PARF_NO_SNOOP_OVERIDE			0x3d4
> >> +#define PARF_ATU_BASE_ADDR			0x634
> >> +#define PARF_ATU_BASE_ADDR_HI			0x638
> >>  #define PARF_DEVICE_TYPE			0x1000
> >>  #define PARF_BDF_TO_SID_TABLE_N			0x2000
> >>  #define PARF_BDF_TO_SID_CFG			0x2c00
> >> @@ -107,9 +113,6 @@
> >>  /* PARF_CONFIG_BITS register fields */
> >>  #define PHY_RX0_EQ(x)				FIELD_PREP(GENMASK(26, 24), x)
> >>  
> >> -/* PARF_SLV_ADDR_SPACE_SIZE register value */
> >> -#define SLV_ADDR_SPACE_SZ			0x10000000
> >> -
> >>  /* PARF_MHI_CLOCK_RESET_CTRL register fields */
> >>  #define AHB_CLK_EN				BIT(0)
> >>  #define MSTR_AXI_CLK_EN				BIT(1)
> >> @@ -324,6 +327,39 @@ static void qcom_pcie_clear_hpc(struct dw_pcie *pci)
> >>  	dw_pcie_dbi_ro_wr_dis(pci);
> >>  }
> >>  
> >> +static void qcom_pcie_configure_dbi_base(struct qcom_pcie *pcie)
> >> +{
> >> +	struct dw_pcie *pci = pcie->pci;
> >> +
> >> +	if (pci->dbi_phys_addr) {
> >> +		writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf +
> >> +							PARF_DBI_BASE_ADDR);
> >> +		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> >> +	}
> >> +}
> >> +
> >> +static void qcom_pcie_configure_dbi_atu_base(struct qcom_pcie *pcie)
> >> +{
> >> +	struct dw_pcie *pci = pcie->pci;
> >> +
> >> +	if (pci->dbi_phys_addr) {
> >> +		writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf +
> >> +							PARF_DBI_BASE_ADDR_V2);
> >> +		writel(upper_32_bits(pci->dbi_phys_addr), pcie->parf +
> >> +						PARF_DBI_BASE_ADDR_V2_HI);
> >> +
> >> +		if (pci->atu_phys_addr) {
> >> +			writel(lower_32_bits(pci->atu_phys_addr), pcie->parf +
> >> +							PARF_ATU_BASE_ADDR);
> >> +			writel(upper_32_bits(pci->atu_phys_addr), pcie->parf +
> >> +							PARF_ATU_BASE_ADDR_HI);
> >> +		}
> >> +
> >> +		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE_V2);
> >> +		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE_V2_HI);
> >> +	}
> >> +}
> > 
> > These functions write CPU physical addresses into registers.  I don't
> > know where these registers live.  If they are on the PCI side of the
> > world, they most likely should contain PCI bus addresses, not CPU
> > physical addresses.
> > 
> > In some systems, PCI bus addresses are the same as CPU physical
> > addresses, but on many systems they are not the same, so it's better
> > if we don't make implicit assumptions that they are the same.  
> 
> On Qualcomm platforms, CPU physical address and PCI bus addresses
> for DBI and ATU registers are same. PARF registers live outside the
> PCI address space in the system memory.
> 
> There is a mapping logic in the QCOM PARF wrapper which detects any
> incoming read/write transactions from the NOC towards PCIe
> controller and checks its addresses against PARF_DBI_BASE_ADDR and
> PARF_ATU_BASE_ADDR registers so that these transactions can be
> routed to DBI and ATU registers inside the PCIe controller.
> 
> So, these PARF registers needs to be programmed with base CPU
> physical addresses of DBI and ATU as the incoming DBI and ATU
> transactions from the NOC contain CPU physical adresses.

Can you add a comment to this effect that these registers are
effectively in the CPU address domain, not the PCI bus domain?
Otherwise the next person who reviews this will have the same
question, and somebody may even try to "fix" this by converting it to
a PCI bus address.

Bjorn

Re: [PATCH v3 2/2] PCI: qcom: Avoid DBI and ATU register space mirror to BAR/MMIO region

[Prudhvi Yarlagadda]

Hi Bjorn,

On 7/29/2024 3:51 PM, Bjorn Helgaas wrote:
> On Thu, Jul 25, 2024 at 04:03:56PM -0700, Prudhvi Yarlagadda wrote:
>> Hi Bjorn,
>>
>> Thanks for the review comments.
>>
>> On 7/24/2024 11:43 AM, Bjorn Helgaas wrote:
>>> On Tue, Jul 23, 2024 at 07:27:19PM -0700, Prudhvi Yarlagadda wrote:
>>>> PARF hardware block which is a wrapper on top of DWC PCIe controller
>>>> mirrors the DBI and ATU register space. It uses PARF_SLV_ADDR_SPACE_SIZE
>>>> register to get the size of the memory block to be mirrored and uses
>>>> PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR registers to determine the base
>>>> address of DBI and ATU space inside the memory block that is being
>>>> mirrored.
>>>>
>>>> When a memory region which is located above the SLV_ADDR_SPACE_SIZE
>>>> boundary is used for BAR region then there could be an overlap of DBI and
>>>> ATU address space that is getting mirrored and the BAR region. This
>>>> results in DBI and ATU address space contents getting updated when a PCIe
>>>> function driver tries updating the BAR/MMIO memory region. Reference
>>>> memory map of the PCIe memory region with DBI and ATU address space
>>>> overlapping BAR region is as below.
>>>>
>>>>                         |---------------|
>>>>                         |               |
>>>>                         |               |
>>>>         ------- --------|---------------|
>>>>            |       |    |---------------|
>>>>            |       |    |       DBI     |
>>>>            |       |    |---------------|---->DBI_BASE_ADDR
>>>>            |       |    |               |
>>>>            |       |    |               |
>>>>            |    PCIe    |               |---->2*SLV_ADDR_SPACE_SIZE
>>>>            |    BAR/MMIO|---------------|
>>>>            |    Region  |       ATU     |
>>>>            |       |    |---------------|---->ATU_BASE_ADDR
>>>>            |       |    |               |
>>>>         PCIe       |    |---------------|
>>>>         Memory     |    |       DBI     |
>>>>         Region     |    |---------------|---->DBI_BASE_ADDR
>>>>            |       |    |               |
>>>>            |    --------|               |
>>>>            |            |               |---->SLV_ADDR_SPACE_SIZE
>>>>            |            |---------------|
>>>>            |            |       ATU     |
>>>>            |            |---------------|---->ATU_BASE_ADDR
>>>>            |            |               |
>>>>            |            |---------------|
>>>>            |            |       DBI     |
>>>>            |            |---------------|---->DBI_BASE_ADDR
>>>>            |            |               |
>>>>            |            |               |
>>>>         ----------------|---------------|
>>>>                         |               |
>>>>                         |               |
>>>>                         |               |
>>>>                         |---------------|
>>>>
>>>> Currently memory region beyond the SLV_ADDR_SPACE_SIZE boundary is not
>>>> used for BAR region which is why the above mentioned issue is not
>>>> encountered. This issue is discovered as part of internal testing when we
>>>> tried moving the BAR region beyond the SLV_ADDR_SPACE_SIZE boundary. Hence
>>>> we are trying to fix this.
>>>>
>>>> As PARF hardware block mirrors DBI and ATU register space after every
>>>> PARF_SLV_ADDR_SPACE_SIZE (default 0x1000000) boundary multiple, write
>>>> U32_MAX (all 0xFF's) to PARF_SLV_ADDR_SPACE_SIZE register to avoid
>>>> mirroring DBI and ATU to BAR/MMIO region. Write the physical base address
>>>> of DBI and ATU register blocks to PARF_DBI_BASE_ADDR (default 0x0) and
>>>> PARF_ATU_BASE_ADDR (default 0x1000) respectively to make sure DBI and ATU
>>>> blocks are at expected memory locations.
>>>>
>>>> The register offsets PARF_DBI_BASE_ADDR_V2, PARF_SLV_ADDR_SPACE_SIZE_V2
>>>> and PARF_ATU_BASE_ADDR are applicable for platforms that use PARF
>>>> Qcom IP rev 1.9.0, 2.7.0 and 2.9.0. PARF_DBI_BASE_ADDR_V2 and
>>>> PARF_SLV_ADDR_SPACE_SIZE_V2 are applicable for PARF Qcom IP rev 2.3.3.
>>>> PARF_DBI_BASE_ADDR and PARF_SLV_ADDR_SPACE_SIZE are applicable for PARF
>>>> Qcom IP rev 1.0.0, 2.3.2 and 2.4.0. Updating the init()/post_init()
>>>> functions of the respective PARF versions to program applicable
>>>> PARF_DBI_BASE_ADDR, PARF_SLV_ADDR_SPACE_SIZE and PARF_ATU_BASE_ADDR
>>>> register offsets. And remove the unused SLV_ADDR_SPACE_SZ macro.
>>>>
>>>> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
>>>> Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
>>>> ---
>>>>  drivers/pci/controller/dwc/pcie-qcom.c | 62 +++++++++++++++++++-------
>>>>  1 file changed, 45 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>>> index 0180edf3310e..6976efb8e2f0 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>>> @@ -45,6 +45,7 @@
>>>>  #define PARF_PHY_REFCLK				0x4c
>>>>  #define PARF_CONFIG_BITS			0x50
>>>>  #define PARF_DBI_BASE_ADDR			0x168
>>>> +#define PARF_SLV_ADDR_SPACE_SIZE		0x16C
>>>>  #define PARF_MHI_CLOCK_RESET_CTRL		0x174
>>>>  #define PARF_AXI_MSTR_WR_ADDR_HALT		0x178
>>>>  #define PARF_AXI_MSTR_WR_ADDR_HALT_V2		0x1a8
>>>> @@ -52,8 +53,13 @@
>>>>  #define PARF_LTSSM				0x1b0
>>>>  #define PARF_SID_OFFSET				0x234
>>>>  #define PARF_BDF_TRANSLATE_CFG			0x24c
>>>> -#define PARF_SLV_ADDR_SPACE_SIZE		0x358
>>>> +#define PARF_DBI_BASE_ADDR_V2			0x350
>>>> +#define PARF_DBI_BASE_ADDR_V2_HI		0x354
>>>> +#define PARF_SLV_ADDR_SPACE_SIZE_V2		0x358
>>>> +#define PARF_SLV_ADDR_SPACE_SIZE_V2_HI		0x35C
>>>>  #define PARF_NO_SNOOP_OVERIDE			0x3d4
>>>> +#define PARF_ATU_BASE_ADDR			0x634
>>>> +#define PARF_ATU_BASE_ADDR_HI			0x638
>>>>  #define PARF_DEVICE_TYPE			0x1000
>>>>  #define PARF_BDF_TO_SID_TABLE_N			0x2000
>>>>  #define PARF_BDF_TO_SID_CFG			0x2c00
>>>> @@ -107,9 +113,6 @@
>>>>  /* PARF_CONFIG_BITS register fields */
>>>>  #define PHY_RX0_EQ(x)				FIELD_PREP(GENMASK(26, 24), x)
>>>>  
>>>> -/* PARF_SLV_ADDR_SPACE_SIZE register value */
>>>> -#define SLV_ADDR_SPACE_SZ			0x10000000
>>>> -
>>>>  /* PARF_MHI_CLOCK_RESET_CTRL register fields */
>>>>  #define AHB_CLK_EN				BIT(0)
>>>>  #define MSTR_AXI_CLK_EN				BIT(1)
>>>> @@ -324,6 +327,39 @@ static void qcom_pcie_clear_hpc(struct dw_pcie *pci)
>>>>  	dw_pcie_dbi_ro_wr_dis(pci);
>>>>  }
>>>>  
>>>> +static void qcom_pcie_configure_dbi_base(struct qcom_pcie *pcie)
>>>> +{
>>>> +	struct dw_pcie *pci = pcie->pci;
>>>> +
>>>> +	if (pci->dbi_phys_addr) {
>>>> +		writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf +
>>>> +							PARF_DBI_BASE_ADDR);
>>>> +		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
>>>> +	}
>>>> +}
>>>> +
>>>> +static void qcom_pcie_configure_dbi_atu_base(struct qcom_pcie *pcie)
>>>> +{
>>>> +	struct dw_pcie *pci = pcie->pci;
>>>> +
>>>> +	if (pci->dbi_phys_addr) {
>>>> +		writel(lower_32_bits(pci->dbi_phys_addr), pcie->parf +
>>>> +							PARF_DBI_BASE_ADDR_V2);
>>>> +		writel(upper_32_bits(pci->dbi_phys_addr), pcie->parf +
>>>> +						PARF_DBI_BASE_ADDR_V2_HI);
>>>> +
>>>> +		if (pci->atu_phys_addr) {
>>>> +			writel(lower_32_bits(pci->atu_phys_addr), pcie->parf +
>>>> +							PARF_ATU_BASE_ADDR);
>>>> +			writel(upper_32_bits(pci->atu_phys_addr), pcie->parf +
>>>> +							PARF_ATU_BASE_ADDR_HI);
>>>> +		}
>>>> +
>>>> +		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE_V2);
>>>> +		writel(U32_MAX, pcie->parf + PARF_SLV_ADDR_SPACE_SIZE_V2_HI);
>>>> +	}
>>>> +}
>>>
>>> These functions write CPU physical addresses into registers.  I don't
>>> know where these registers live.  If they are on the PCI side of the
>>> world, they most likely should contain PCI bus addresses, not CPU
>>> physical addresses.
>>>
>>> In some systems, PCI bus addresses are the same as CPU physical
>>> addresses, but on many systems they are not the same, so it's better
>>> if we don't make implicit assumptions that they are the same.  
>>
>> On Qualcomm platforms, CPU physical address and PCI bus addresses
>> for DBI and ATU registers are same. PARF registers live outside the
>> PCI address space in the system memory.
>>
>> There is a mapping logic in the QCOM PARF wrapper which detects any
>> incoming read/write transactions from the NOC towards PCIe
>> controller and checks its addresses against PARF_DBI_BASE_ADDR and
>> PARF_ATU_BASE_ADDR registers so that these transactions can be
>> routed to DBI and ATU registers inside the PCIe controller.
>>
>> So, these PARF registers needs to be programmed with base CPU
>> physical addresses of DBI and ATU as the incoming DBI and ATU
>> transactions from the NOC contain CPU physical adresses.
> 
> Can you add a comment to this effect that these registers are
> effectively in the CPU address domain, not the PCI bus domain?
> Otherwise the next person who reviews this will have the same
> question, and somebody may even try to "fix" this by converting it to
> a PCI bus address.
> 
> Bjorn

ACK. I will add a comment in the next patch.

Thanks,
Prudhvi

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

[Serge Semin]

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

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

[Prudhvi Yarlagadda]

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

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

[Serge Semin]

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

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

[Manivannan Sadhasivam]

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

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

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

[Serge Semin]

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
> 
> -- 
> மணிவண்ணன் சதாசிவம்

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

[Prudhvi Yarlagadda]

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
>>
>> -- 
>> மணிவண்ணன் சதாசிவம்
> 

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

[Bjorn Helgaas]

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.