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

[Manivannan Sadhasivam]

On Fri, Mar 28, 2025 at 09:44:20PM +0100, Konrad Dybcio wrote:
> 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.
> 

Perfetchability only matters for MEM/IO space, not for config space. AFAIK,
recent SoCs seem to be supporting PCIe memory in GiB, so moving the 256MiB of
config space above iATU with 2^28 byte alignment seems plausible.

Otherwise, it becomes a mess to avoid remapping DBI/ELBI registers in the
driver.

- Mani

-- 
மணிவண்ணன் சதாசிவம்