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

[Prudhvi Yarlagadda <quic_pyarlaga@quicinc.com>]

Hi Mayank,

Thanks for the review comments.

On 7/18/2024 2:52 PM, Mayank Rana wrote:
> Hi Prudhvi
> 
> On 7/17/2024 10:12 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
>> 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>
>> ---
>>   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..845c7641431f 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
> [...]> +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);
> We can't update PARF_SLV_ADDR_SPACE_SIZE without updating PARF_DBI_BASE_ADDR.
> Please make dbi_phys_addr mandatory to update PARF_SLV_ADDR_SPACE_SIZE.

ACK. I will update it in the next patch.

>> +}
>> +
>> +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);
> Same as above. atu_phys_addr shall be optional here but not dbi_phys_addr to update PARF_SLV_ADDR_SPACE_SIZE.

ACK. I will update it in the next patch.

Thanks,
Prudhvi
>> +}
>> +
>>   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,