| Message ID | 20240620213405.3120611-1-quic_pyarlaga@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] PCI: qcom: Avoid DBI and ATU register space mirror to BAR/MMIO region | expand |
On 6/20/2024 2:34 PM, 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 > 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. > > 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 > #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) > +{ > + 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; > + > + 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); > + > /* 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; Reviewed-by: Mayank Rana <quic_mrana@quicinc.com>
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;
Hi Bjorn, Thanks for the review comments. On 6/21/2024 1:47 PM, Bjorn Helgaas wrote: > 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. ACK. I will update it in the next patch. >> 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)? Sorry for the confusion caused. Yes, it is a Root Complex. The DBI and ATU resources are passed via DT and the BAR region is also passed using ranges property in 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? No, this overlap is caused entirely by the Qcom PARF hardware block (wrapper on top of the Root Complex) and the way NOC routing being done. PARF doesn't know where the BAR region is and it just keeps mirroring the memory region of size PARF_SLV_ADDR_SPACE_SIZE to the memory space accessible by the Root Complex. This mirroring effect is causing the DBI and ATU contents to overlap with BAR region even though there is no actual overlap in the addresses of DBI, ATU and BAR passed in the DT. The proposed change will help to avoid the mirroring by giving U64_MAX to PARF_SLV_ADDR_SPACE_SIZE register. As there won't be system memory greater than 64 bit address, PARF block will not be able to do the mirroring. Power on reset values are good enough to use smaller BAR size (less than 16MB in size as the default value of PARF_SLV_ADDR_SPACE_SIZE is 16MB). If we are using bigger BAR size, then above set of registers are required to be programmed to avoid overlap. >> 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? Thanks for pointing this. The new PARF_DBI_BASE_ADDR_V2, PARF_ATU_BASE_ADDR offsets are only applicable to 2.7.0. I will update the next patchset to have these change only in 2.7.0 and remove the usage with 2.3.2. The same problem is applicable for all the versions of init/post_init functions except 2.1.0. But the PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR and PARF_SLV_ADDR_SPACE_SIZE register offsets are different for most of the versions compared to 2.7.0. And probably need to review all those devices use cases and if they need more BAR size we shall come up with seperate patch for those. > It'd be nice to have a hint in the commit log about which versions are > and are not affected. ACK. I will update it in the next patch. >> #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. ACK. I will update it in the next patch. >> +{ >> + 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. ACK. I will update it in the next patch. >> + 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? No, there is no requirement to move this. I wanted to do this setup before configuring the PARF block to be in Root Complex mode in the line below. I will move this back to the original position in the next patch to avoid confusion. Thanks, Prudhvi >> /* 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; > >
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. > This PARF_SLV_ADDR_SPACE register is a mystery to me. I tried getting to the bottom of it, but nobody could explain it to me clearly. Looks like you know more about it... From your description, it seems like this register specifies the size of the mirroring region (ATU + DBI), but the response from your colleague indicates something different [1]. [1] https://lore.kernel.org/linux-pci/f42559f5-9d4c-4667-bf0e-7abfd9983c36@quicinc.com/ > 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. > I don't quite understand this. PoR value of SLV_ADDR_SPACE_SIZE is 16MB and most of the platforms have the size of whole PCIe region defined in DT as 512MB (registers + I/O + MEM). So the range is already crossing the SLV_ADDR_SPACE_SIZE boundary. Ironically, the patch I pointed out above changes the value of this register as 128MB, and the PCIe region size of that platform is also 128MB. The author of that patch pointed out that if the SLV_ADDR_SPACE_SIZE is set to 256MB, then they are seeing enumeration failures. If we go by your description of that register, the SLV_ADDR_SPACE_SIZE of 256MB should be > PCIe region size of 128MB. So they should not see any issues, right? > 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. Looks like you are trying to avoid this mirroring on a whole. First of all, what is the reasoning behind this mirroring? > 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. > Why is this needed? Some configs in this driver writes 0 to PARF_DBI_BASE_ADDR. Does the hardware doesn't know where the registers are located? > 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 > #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) > +{ > + 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; > + > + 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); > + } > + Above 2 resources are already fetched by dw_pcie_get_resources(). So we should just store the phys addresses in 'struct dw_pcie' and make use of them here. - Mani > + 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); > + > /* 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; > -- > 2.25.1 >
On Sat, Jun 22, 2024 at 09:24:44AM +0530, Manivannan Sadhasivam wrote: > 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. > > This PARF_SLV_ADDR_SPACE register is a mystery to me. I tried getting to the > bottom of it, but nobody could explain it to me clearly. Looks like you know > more about it... > > From your description, it seems like this register specifies the size of the > mirroring region (ATU + DBI), but the response from your colleague indicates > something different [1]. > > [1] https://lore.kernel.org/linux-pci/f42559f5-9d4c-4667-bf0e-7abfd9983c36@quicinc.com/ > > > 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. > > I don't quite understand this. PoR value of SLV_ADDR_SPACE_SIZE is > 16MB and most of the platforms have the size of whole PCIe region > defined in DT as 512MB (registers + I/O + MEM). So the range is > already crossing the SLV_ADDR_SPACE_SIZE boundary. > > Ironically, the patch I pointed out above changes the value of this > register as 128MB, and the PCIe region size of that platform is also > 128MB. The author of that patch pointed out that if the > SLV_ADDR_SPACE_SIZE is set to 256MB, then they are seeing > enumeration failures. If we go by your description of that register, > the SLV_ADDR_SPACE_SIZE of 256MB should be > PCIe region size of > 128MB. So they should not see any issues, right? > > > 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. > > Looks like you are trying to avoid this mirroring on a whole. First > of all, what is the reasoning behind this mirroring? This sounds like what we usually call "aliasing" that happens when some upper address bits are ignored.
On 6/22/2024 10:41 AM, Bjorn Helgaas wrote: > On Sat, Jun 22, 2024 at 09:24:44AM +0530, Manivannan Sadhasivam wrote: >> 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. >> >> This PARF_SLV_ADDR_SPACE register is a mystery to me. I tried getting to the >> bottom of it, but nobody could explain it to me clearly. Looks like you know >> more about it... >> >> From your description, it seems like this register specifies the size of the >> mirroring region (ATU + DBI), but the response from your colleague indicates >> something different [1]. >> >> [1] https://lore.kernel.org/linux-pci/f42559f5-9d4c-4667-bf0e-7abfd9983c36@quicinc.com/ >> >>> 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. >> >> I don't quite understand this. PoR value of SLV_ADDR_SPACE_SIZE is >> 16MB and most of the platforms have the size of whole PCIe region >> defined in DT as 512MB (registers + I/O + MEM). So the range is >> already crossing the SLV_ADDR_SPACE_SIZE boundary. >> >> Ironically, the patch I pointed out above changes the value of this >> register as 128MB, and the PCIe region size of that platform is also >> 128MB. The author of that patch pointed out that if the >> SLV_ADDR_SPACE_SIZE is set to 256MB, then they are seeing >> enumeration failures. If we go by your description of that register, >> the SLV_ADDR_SPACE_SIZE of 256MB should be > PCIe region size of >> 128MB. So they should not see any issues, right? >> >>> 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. >> >> Looks like you are trying to avoid this mirroring on a whole. First >> of all, what is the reasoning behind this mirroring? > > This sounds like what we usually call "aliasing" that happens when > some upper address bits are ignored. Yes, here we are trying to disable altogether aliasing (mirroring of DBI/ATU) address space). Regards, Mayank
Hi Manivannan, Thanks for the review comments. On 6/21/2024 8:54 PM, Manivannan Sadhasivam wrote: > 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. >> > > This PARF_SLV_ADDR_SPACE register is a mystery to me. I tried getting to the > bottom of it, but nobody could explain it to me clearly. Looks like you know > more about it... > > From your description, it seems like this register specifies the size of the > mirroring region (ATU + DBI), but the response from your colleague indicates > something different [1]. > > [1] https://lore.kernel.org/linux-pci/f42559f5-9d4c-4667-bf0e-7abfd9983c36@quicinc.com/ > PARF_SLV_ADDR_SPACE_SIZE is used for mirroring the region containing ATU + DBI. But the issue being observed in the patch pointed above and the issue I am observing are a bit different even though the same fix could be used for both issues. The issue I am observing is that the DBI and ATU region is getting mirrored into the BAR/MMIO region and thereby the DBI and ATU registers contents are getting modified while accessing the BAR region content. As per my discussions internally with Devi Priya (author of the patch pointed above), the issue being seen there is that the DBI register contents are not available at the expected address by software and this is causing enumeration failures. Below is the memory map of the IPQ9574 platform being mentioned in the above patch along with the memory locations of the DBI of respective PCIe Root Complexes. |--------------------| | | | | | | | | |--------------------| | | | PCIe2 | | | |--------------------|---->0x2000_0000 ->DBI | | | PCIe3 | | | |--------------------|---->0x1800_0000 ->DBI | | | PCIe1 | | | |--------------------|---->0x1000_0000 ->DBI | | | | | | |--------------------| Previously PARF_SLV_ADDR_SPACE_SIZE is configured as 256MB (0x1000_0000) and PARF_DBI_BASE_ADDR is configured as 0x0 for each of the PCIe Root complex. With this configuration, in the case of PCIe1 DBI contents get accessible at 0x0, 0x1000_0000 and 0x2000_0000 and so on due to mirroring. Although NOC allows access only to region 0x1000_0000 to 0x1800_0000 for PCIe1. So in the case of PCIe1 DBI is accessible at the expected location 0x1000_0000. Similarly in the case of PCIe3 its DBI contents are accessible at 0x0, 0x1000_0000 and 0x2000_0000 but the expectation is to have DBI at 0x1800_0000 (as 0x1800_0000 is the physical address of DBI per devicetree). This is causing enumeration failures as DBI is not at the expected location (same issue w.r.t ATU). When PARF_SLV_ADDR_SPACE_SIZE is modified to 128MB (0x800_0000) and PARF_DBI_BASE_ADDR is kept 0x0, for PCIe3 the DBI gets accessible at 0x0, 0x800_0000, 0x1000_0000, 0x1800_0000, 0x2000_0000 and so on. So, now the DBI becomes accessible at the expected location of 0x1800_0000 and its fixing the issue in the above patch. Alternate way to fix the above issue is if we use the current patch to disable mirroring and configure the PARF_DBI_BASE_ADDR then the DBI gets accessible only at the location given in PARF_DBI_BASE_ADDR register which will be the same location mentioned in devicetree. >> 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. >> > > I don't quite understand this. PoR value of SLV_ADDR_SPACE_SIZE is 16MB and most > of the platforms have the size of whole PCIe region defined in DT as 512MB > (registers + I/O + MEM). So the range is already crossing the > SLV_ADDR_SPACE_SIZE boundary. > > Ironically, the patch I pointed out above changes the value of this register as > 128MB, and the PCIe region size of that platform is also 128MB. The author of > that patch pointed out that if the SLV_ADDR_SPACE_SIZE is set to 256MB, then > they are seeing enumeration failures. If we go by your description of that > register, the SLV_ADDR_SPACE_SIZE of 256MB should be > PCIe region size of > 128MB. So they should not see any issues, right? > As mentioned above, configuring PARF_SLV_ADDR_SPACE_SIZE as 256MB is causing issue with the PCIe instances in which DBI is not aligned with the multiples of 256MB and due to PARF_DBI_BASE_ADDR being configured as 0x0 instead of the actual DBI address given in devicetree. >> 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. > > Looks like you are trying to avoid this mirroring on a whole. First of all, what > is the reasoning behind this mirroring? > The reason is to have more control over where to have the DBI and ATU register contents in the system memory using the PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR. For the PARF_SLV_ADDR_SPACE_SIZE register we don't have an existing use case to utilize mirroring functionality. >> 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. >> > > Why is this needed? Some configs in this driver writes 0 to PARF_DBI_BASE_ADDR. > Does the hardware doesn't know where the registers are located? > Yes, hardware doesn't know where the DBI, ATU registers are located in the PARF_SLV_ADDR_SPACE_SIZE memory block or system memory. Hardware gets the location of DBI and ATU registers from the PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR registers. So these registers must be programmed to have the DBI and ATU at expected memory locations. >> 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 >> #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) >> +{ >> + 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; >> + >> + 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); >> + } >> + > > Above 2 resources are already fetched by dw_pcie_get_resources(). So we should > just store the phys addresses in 'struct dw_pcie' and make use of them here. > > - Mani > This function is using the DBI and ATU physical addresses here to program PARF registers that are specific to pcie-qcom driver. Currently there is no use for DBI and ATU physical addresses for other platform drivers that us dwc PCIe controller. Could you please let me know if you still want me to save the DBI and ATU physical addresses in dw_pcie structure ? Thanks, Prudhvi >> + 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); >> + >> /* 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; >> -- >> 2.25.1 >> >
On Fri, Jun 28, 2024 at 07:02:04PM -0700, Prudhvi Yarlagadda wrote: > Hi Manivannan, > > Thanks for the review comments. > > On 6/21/2024 8:54 PM, Manivannan Sadhasivam wrote: > > 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. > >> > > > > This PARF_SLV_ADDR_SPACE register is a mystery to me. I tried getting to the > > bottom of it, but nobody could explain it to me clearly. Looks like you know > > more about it... > > > > From your description, it seems like this register specifies the size of the > > mirroring region (ATU + DBI), but the response from your colleague indicates > > something different [1]. > > > > [1] https://lore.kernel.org/linux-pci/f42559f5-9d4c-4667-bf0e-7abfd9983c36@quicinc.com/ > > > > PARF_SLV_ADDR_SPACE_SIZE is used for mirroring the region containing ATU + DBI. > But the issue being observed in the patch pointed above and the issue I am > observing are a bit different even though the same fix could be used for both issues. > > The issue I am observing is that the DBI and ATU region is getting mirrored into the > BAR/MMIO region and thereby the DBI and ATU registers contents are getting modified > while accessing the BAR region content. > > As per my discussions internally with Devi Priya (author of the patch pointed above), > the issue being seen there is that the DBI register contents are not available > at the expected address by software and this is causing enumeration failures. > > Below is the memory map of the IPQ9574 platform being mentioned in the above patch > along with the memory locations of the DBI of respective PCIe Root Complexes. > > |--------------------| > | | > | | > | | > | | > |--------------------| > | | > | PCIe2 | > | | > |--------------------|---->0x2000_0000 ->DBI > | | > | PCIe3 | > | | > |--------------------|---->0x1800_0000 ->DBI > | | > | PCIe1 | > | | > |--------------------|---->0x1000_0000 ->DBI > | | > | | > | | > |--------------------| > > Previously PARF_SLV_ADDR_SPACE_SIZE is configured as 256MB (0x1000_0000) and > PARF_DBI_BASE_ADDR is configured as 0x0 for each of the PCIe Root complex. With > this configuration, in the case of PCIe1 DBI contents get accessible at 0x0, > 0x1000_0000 and 0x2000_0000 and so on due to mirroring. Although NOC allows access > only to region 0x1000_0000 to 0x1800_0000 for PCIe1. So in the case of PCIe1 DBI > is accessible at the expected location 0x1000_0000. > > Similarly in the case of PCIe3 its DBI contents are accessible at 0x0, 0x1000_0000 > and 0x2000_0000 but the expectation is to have DBI at 0x1800_0000 (as 0x1800_0000 is > the physical address of DBI per devicetree). This is causing enumeration failures as > DBI is not at the expected location (same issue w.r.t ATU). > > When PARF_SLV_ADDR_SPACE_SIZE is modified to 128MB (0x800_0000) and PARF_DBI_BASE_ADDR > is kept 0x0, for PCIe3 the DBI gets accessible at 0x0, 0x800_0000, 0x1000_0000, > 0x1800_0000, 0x2000_0000 and so on. So, now the DBI becomes accessible at the > expected location of 0x1800_0000 and its fixing the issue in the above patch. > Thanks for the explanation. This really clarifies. > Alternate way to fix the above issue is if we use the current patch to disable > mirroring and configure the PARF_DBI_BASE_ADDR then the DBI gets accessible only at > the location given in PARF_DBI_BASE_ADDR register which will be the same location > mentioned in devicetree. > Agree. > >> 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. > >> > > > > I don't quite understand this. PoR value of SLV_ADDR_SPACE_SIZE is 16MB and most > > of the platforms have the size of whole PCIe region defined in DT as 512MB > > (registers + I/O + MEM). So the range is already crossing the > > SLV_ADDR_SPACE_SIZE boundary. > > > > Ironically, the patch I pointed out above changes the value of this register as > > 128MB, and the PCIe region size of that platform is also 128MB. The author of > > that patch pointed out that if the SLV_ADDR_SPACE_SIZE is set to 256MB, then > > they are seeing enumeration failures. If we go by your description of that > > register, the SLV_ADDR_SPACE_SIZE of 256MB should be > PCIe region size of > > 128MB. So they should not see any issues, right? > > > > As mentioned above, configuring PARF_SLV_ADDR_SPACE_SIZE as 256MB is causing > issue with the PCIe instances in which DBI is not aligned with the multiples of > 256MB and due to PARF_DBI_BASE_ADDR being configured as 0x0 instead of the > actual DBI address given in devicetree. > > >> 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. > > > > Looks like you are trying to avoid this mirroring on a whole. First of all, what > > is the reasoning behind this mirroring? > > > > The reason is to have more control over where to have the DBI and ATU register > contents in the system memory using the PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR. > For the PARF_SLV_ADDR_SPACE_SIZE register we don't have an existing use case > to utilize mirroring functionality. > Okay. Then I guess you could disable mirroring globally for all SoCs. Some SoCs doesn't have both DBU and ATU regions, so the helper could conditionally write the base address if available in DT. > >> 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. > >> > > > > Why is this needed? Some configs in this driver writes 0 to PARF_DBI_BASE_ADDR. > > Does the hardware doesn't know where the registers are located? > > > > Yes, hardware doesn't know where the DBI, ATU registers are located in the > PARF_SLV_ADDR_SPACE_SIZE memory block or system memory. Hardware gets the location > of DBI and ATU registers from the PARF_DBI_BASE_ADDR, PARF_ATU_BASE_ADDR > registers. So these registers must be programmed to have the DBI and ATU at > expected memory locations. > Sounds good. - Mani
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 #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) +{ + 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; + + 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); + /* 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;
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 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. 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.