Re: [PATCH v5 1/7] arm64: dts: qcom: sc7280: Increase config size to 256MB for ECAM feature

[Konrad Dybcio]

On 3/28/25 4:29 PM, Manivannan Sadhasivam wrote:
> On Fri, Mar 28, 2025 at 06:24:23PM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 3/28/2025 5:14 PM, Manivannan Sadhasivam wrote:
>>> On Wed, Mar 26, 2025 at 06:56:02PM +0100, Konrad Dybcio wrote:
>>>> On 3/11/25 12:13 PM, Konrad Dybcio wrote:
>>>>> On 3/9/25 6:45 AM, Krishna Chaitanya Chundru wrote:
>>>>>> PCIe ECAM(Enhanced Configuration Access Mechanism) feature requires
>>>>>> maximum of 256MB configuration space.
>>>>>>
>>>>>> To enable this feature increase configuration space size to 256MB. If
>>>>>> the config space is increased, the BAR space needs to be truncated as
>>>>>> it resides in the same location. To avoid the bar space truncation move
>>>>>> config space, DBI, ELBI, iATU to upper PCIe region and use lower PCIe
>>>>>> iregion entirely for BAR region.
>>>>>>
>>>>>> This depends on the commit: '10ba0854c5e6 ("PCI: qcom: Disable mirroring
>>>>>> of DBI and iATU register space in BAR region")'
>>>>>>
>>>>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@xxxxxxxxxxxxxxxx>
>>>>>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
>>>>>> ---
>>>>>
>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
>>>>
>>>> I took a second look - why are dbi and config regions overlapping?
>>>>
>>>
>>> Not just DBI, ELBI too.
>>>
>>>> I would imagine the latter to be at a certain offset
>>>>
>>>
>>> The problem is that for ECAM, we need config space region to be big enough to
>>> cover all 256 buses. For that reason Krishna overlapped the config region and
>>> DBI/ELBI. Initially I also questioned this and somehow convinced that there is
>>> no other way (no other memory). But looking at the internal documentation now,
>>> I realized that atleast 512MiB of PCIe space is available for each controller
>>> instance.
>>>
>> DBI is the config space of the root port0,  ecam expects all the config
>> space is continuous i.e 256MB and this 256MB config space is ioremaped
>> in ecam driver[1]. This 256 MB should contain the dbi memory too and
>> elbi always with dbi region we can't move it other locations. We are
>> keeping overlap region because once ecam driver io remaped all 256MB
>> including dbi and elbi memory dwc memory can't ioremap the dbi and elbi
>> region again. That is the reason for having this overlap region.
>>> So I just quickly tried this series on SA8775p and by moving the config space
>>> after the iATU region, I was able to have ECAM working without overlapping
>>> addresses in DT. Here is the change I did:
>>>
>> I am sure ecam is not enabled with this below change
> 
> ECAM is indeed enabled. But...
> 
>> because ecam block
>> have the address alignment requirement that address should be aligned to
>> the base address of the range is aligned to a 2(n+20)-byte memory address
>> boundary from pcie spec 6.0.1, sec 7.2.2 (PCI Express Enhanced
>> Configuration Access Mechanism (ECAM)), with out that address alignment
>> ecam will not work since ecam driver gets bus number function number
>> by shifting the address internally.
>>
> 
> You are right, but the ECAM driver doesn't have a check for the config space
> address alignment, so it didn't catch it (I will add the check now). But with
> the unaligned address, endpoint is not getting enumerated (though bridge is
> enumerated as it lives under root port, so I got misleaded).
> 
>> If this is not acceptable we have mimic the ecam driver in dwc driver
>> which is also not recommended.
>>
> 
> You can still move the config space in the upper region to satisfy alignment.
> Like,
> 
> +                     <0x4 0x00000000 0x0 0xf20>,
> +                     <0x4 0x00000f20 0x0 0xa8>,
> +                     <0x4 0x10000000 0x0 0x4000>,
> +                     <0x4 0x20000000 0x0 0x10000000>,
> 
> With this change, ECAM works fine and I can enumerate endpoint on the host. I
> believe this requires more PCIe space on the SoC. Not sure if SC7280 could
> support it or not. But IMO, we should enable ECAM for SoCs that satisfy this
> requirement. This will avoid overlapping and also simplify the code (w.r.t
> DBI/ELBI).

FWIW it seems like most recent SoCs have a <32b space, a _LOWER space which ACPI
describes as QWordMemory, and another _UPPER space that is way way above them.

Not sure about the prefetchability and other nuances of the last region though.

Konrad