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

[Bjorn Helgaas <helgaas@kernel.org>]

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.