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

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Jun 20, 2024 at 02:34:05PM -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
> 	   |		|		|
> 	   |		|		|
> 	----------------|---------------|
> 			|		|
> 			|		|
> 			|		|
> 			|---------------|

Nice diagram.  Run it through "expand" to convert the tabs to spaces
so it doesn't get messed up if indented.

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

This seems ... weird.  You mention BARs, which means a PCI device, so
that part must refer to a Root Port.

The diagram also mentions address space *outside* the BAR, so that
cannot be a PCI device (unless the device is defective and responds to
accesses outside its BARs).  Maybe this space belongs to the Root
Complex (which is not a PCI device, so its resources must be described
via DT)?

Is this overlap of BAR space and ATU/DBI stuff (I assume the ATU/DBI
is *not* supposed to be part of the BAR) a result of some invalid
configuration done by a bootloader?  Or is it a result of Linux
assigning invalid space for the BAR?

> Signed-off-by: Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 40 ++++++++++++++++++++++----
>  1 file changed, 35 insertions(+), 5 deletions(-)
> 
> Tested:
> - Validated NVME functionality with PCIe6a on x1e80100 platform.
> - Validated WiFi functionality with PCIe4 on x1e80100 platform.
> - Validated NVME functionality with PCIe0 and PCIe1 on SA8775p platform.
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 5f9f0ff19baa..864548657551 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -49,7 +49,12 @@
>  #define PARF_LTSSM				0x1b0
>  #define PARF_SID_OFFSET				0x234
>  #define PARF_BDF_TRANSLATE_CFG			0x24c
> +#define PARF_DBI_BASE_ADDR_V2			0x350
> +#define PARF_DBI_BASE_ADDR_V2_HI		0x354

Previously these functions wrote PARF_DBI_BASE_ADDR:

  qcom_pcie_post_init_1_0_0()
  qcom_pcie_post_init_2_3_2()
  qcom_pcie_post_init_2_3_3()
  qcom_pcie_init_2_7_0()      (weird that this is init, not post_init)
  qcom_pcie_post_init_2_9_0()

This patch updates qcom_pcie_post_init_2_3_2() and
qcom_pcie_init_2_7_0() to use PARF_DBI_BASE_ADDR_V2 instead.

I assume PARF_DBI_BASE_ADDR_V2 and PARF_ATU_BASE_ADDR are new for
2.3.2 and 2.7.0?  And they don't apply to 2.3.3 and 2.9.0?  And 1.0.0,
2.3.3, and 2.9.0 don't have this problem?

It'd be nice to have a hint in the commit log about which versions are
and are not affected.

>  #define PARF_SLV_ADDR_SPACE_SIZE		0x358
> +#define PARF_SLV_ADDR_SPACE_SIZE_HI		0x35C
> +#define PARF_ATU_BASE_ADDR			0x634
> +#define PARF_ATU_BASE_ADDR_HI			0x638
>  #define PARF_NO_SNOOP_OVERIDE			0x3d4
>  #define PARF_DEVICE_TYPE			0x1000
>  #define PARF_BDF_TO_SID_TABLE_N			0x2000
> @@ -319,6 +324,33 @@ static void qcom_pcie_clear_hpc(struct dw_pcie *pci)
>  	dw_pcie_dbi_ro_wr_dis(pci);
>  }
>  
> +static void qcom_pcie_avoid_dbi_atu_mirroring(struct qcom_pcie *pcie)

I think I would just call this "configure_dbi_atu" or similar, since I
think it's about configuring them correctly, and the mirroring is just
a consequence of doing it wrong.

> +{
> +	struct dw_pcie *pci = pcie->pci;
> +	struct platform_device *pdev;
> +	struct resource *atu_res;
> +	struct resource *dbi_res;
> +
> +	pdev = to_platform_device(pci->dev);
> +	if (!pdev)
> +		return;

I don't think "!pdev" is even possible.

> +	dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> +	if (dbi_res) {
> +		writel(lower_32_bits(dbi_res->start), pcie->parf + PARF_DBI_BASE_ADDR_V2);
> +		writel(upper_32_bits(dbi_res->start), pcie->parf + PARF_DBI_BASE_ADDR_V2_HI);
> +	}
> +
> +	atu_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "atu");
> +	if (atu_res) {
> +		writel(lower_32_bits(atu_res->start), pcie->parf + PARF_ATU_BASE_ADDR);
> +		writel(upper_32_bits(atu_res->start), pcie->parf + PARF_ATU_BASE_ADDR_HI);
> +	}
> +
> +	writel(lower_32_bits(U64_MAX), pcie->parf + PARF_SLV_ADDR_SPACE_SIZE);
> +	writel(upper_32_bits(U64_MAX), pcie->parf + PARF_SLV_ADDR_SPACE_SIZE_HI);
> +}
> +
>  static void qcom_pcie_2_1_0_ltssm_enable(struct qcom_pcie *pcie)
>  {
>  	u32 val;
> @@ -623,8 +655,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_avoid_dbi_atu_mirroring(pcie);
>  
>  	/* MAC PHY_POWERDOWN MUX DISABLE  */
>  	val = readl(pcie->parf + PARF_SYS_CTRL);
> @@ -900,6 +931,8 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>  	/* Wait for reset to complete, required on SM8450 */
>  	usleep_range(1000, 1500);
>  
> +	qcom_pcie_avoid_dbi_atu_mirroring(pcie);

Why did you move this setup earlier than the previous
PARF_DBI_BASE_ADDR write?  Is there some 2.7.0 difference that requires
this?

>  	/* configure PCIe to RC mode */
>  	writel(DEVICE_TYPE_RC, pcie->parf + PARF_DEVICE_TYPE);
>  
> @@ -908,9 +941,6 @@ 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);
> -
>  	/* MAC PHY_POWERDOWN MUX DISABLE  */
>  	val = readl(pcie->parf + PARF_SYS_CTRL);
>  	val &= ~MAC_PHY_POWERDOWN_IN_P2_D_MUX_EN;