[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9d4cea0c-16be-4341-9b48-2b32a92b75f4@quicinc.com>
Date: Mon, 24 Jun 2024 10:04:54 -0700
From: Mayank Rana <quic_mrana@...cinc.com>
To: Bjorn Helgaas <helgaas@...nel.org>,
Manivannan Sadhasivam
<manivannan.sadhasivam@...aro.org>
CC: Prudhvi Yarlagadda <quic_pyarlaga@...cinc.com>, <lpieralisi@...nel.org>,
<kw@...ux.com>, <bhelgaas@...gle.com>, <robh@...nel.org>,
<linux-pci@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] PCI: qcom: Avoid DBI and ATU register space mirror to
BAR/MMIO region
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
Powered by blists - more mailing lists