diff mbox series

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

Message ID 20250309-ecam_v4-v5-1-8eff4b59790d@oss.qualcomm.com
State New
Headers show
Series PCI: dwc: Add ECAM support with iATU configuration | expand

Commit Message

Krishna Chaitanya Chundru March 9, 2025, 5:45 a.m. UTC
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@oss.qualcomm.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Krzysztof Kozlowski March 10, 2025, 9:34 a.m. UTC | #1
On Sun, Mar 09, 2025 at 11:15:23AM +0530, 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@oss.qualcomm.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 0f2caf36910b..64c46221d8bf 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -2201,10 +2201,10 @@ wifi: wifi@17a10040 {
>  		pcie1: pcie@1c08000 {
>  			compatible = "qcom,pcie-sc7280";
>  			reg = <0 0x01c08000 0 0x3000>,
> -			      <0 0x40000000 0 0xf1d>,
> -			      <0 0x40000f20 0 0xa8>,
> -			      <0 0x40001000 0 0x1000>,
> -			      <0 0x40100000 0 0x100000>;
> +			      <4 0x00000000 0 0xf1d>,
> +			      <4 0x00000f20 0 0xa8>,
> +			      <4 0x10000000 0 0x1000>,
> +			      <4 0x00000000 0 0x10000000>;

This makes no sense - you change in next patch. Either this is correct
or not. If this is correct, then next patch is wrong. If this is not
correct, then you send us known wrong code.

Best regards,
Krzysztof
Konrad Dybcio March 11, 2025, 11:13 a.m. UTC | #2
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@oss.qualcomm.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad
Konrad Dybcio March 26, 2025, 5:56 p.m. UTC | #3
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@oss.qualcomm.com>
>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> ---
> 
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

I took a second look - why are dbi and config regions overlapping?

I would imagine the latter to be at a certain offset

Konrad
Manivannan Sadhasivam March 28, 2025, 11:44 a.m. UTC | #4
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@oss.qualcomm.com>
> >> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >> ---
> > 
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> 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.

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:

diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
index 3394ae2d1300..e41c8e3dd30c 100644
--- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
+++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
@@ -6395,18 +6395,18 @@ arch_timer: timer {
        pcie0: pcie@1c00000 {
                compatible = "qcom,pcie-sa8775p";
                reg = <0x0 0x01c00000 0x0 0x3000>,
-                     <0x0 0x40000000 0x0 0xf20>,
-                     <0x0 0x40000f20 0x0 0xa8>,
-                     <0x0 0x40001000 0x0 0x4000>,
-                     <0x0 0x40100000 0x0 0x100000>,
+                     <0x4 0x00000000 0x0 0xf20>,
+                     <0x4 0x00000f20 0x0 0xa8>,
+                     <0x4 0x10000000 0x0 0x4000>,
+                     <0x4 0x10004000 0x0 0x10000000>,
                      <0x0 0x01c03000 0x0 0x1000>;
                reg-names = "parf", "dbi", "elbi", "atu", "config", "mhi";
                device_type = "pci";
 
                #address-cells = <3>;
                #size-cells = <2>;
-               ranges = <0x01000000 0x0 0x00000000 0x0 0x40200000 0x0 0x100000>,
-                        <0x02000000 0x0 0x40300000 0x0 0x40300000 0x0 0x1fd00000>;
+               ranges = <0x01000000 0x0 0x00000000 0x0 0x40000000 0x0 0x100000>,
+                        <0x02000000 0x0 0x40100000 0x0 0x40100000 0x0 0x1ff00000>;
                bus-range = <0x00 0xff>;
 
                dma-coherent;
 

Krishna: Could you also try similar change on SC7280 and see if it works?

- Mani
Krishna Chaitanya Chundru March 28, 2025, 12:54 p.m. UTC | #5
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@oss.qualcomm.com>
>>>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>> ---
>>>
>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> 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 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.

If this is not acceptable we have mimic the ecam driver in dwc driver
which is also not recommended.

- Krishna Chaitanya.
> diff --git a/arch/arm64/boot/dts/qcom/sa8775p.dtsi b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> index 3394ae2d1300..e41c8e3dd30c 100644
> --- a/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sa8775p.dtsi
> @@ -6395,18 +6395,18 @@ arch_timer: timer {
>          pcie0: pcie@1c00000 {
>                  compatible = "qcom,pcie-sa8775p";
>                  reg = <0x0 0x01c00000 0x0 0x3000>,
> -                     <0x0 0x40000000 0x0 0xf20>,
> -                     <0x0 0x40000f20 0x0 0xa8>,
> -                     <0x0 0x40001000 0x0 0x4000>,
> -                     <0x0 0x40100000 0x0 0x100000>,
> +                     <0x4 0x00000000 0x0 0xf20>,
> +                     <0x4 0x00000f20 0x0 0xa8>,
> +                     <0x4 0x10000000 0x0 0x4000>,
> +                     <0x4 0x10004000 0x0 0x10000000>,
>                        <0x0 0x01c03000 0x0 0x1000>;
>                  reg-names = "parf", "dbi", "elbi", "atu", "config", "mhi";
>                  device_type = "pci";
>   
>                  #address-cells = <3>;
>                  #size-cells = <2>;
> -               ranges = <0x01000000 0x0 0x00000000 0x0 0x40200000 0x0 0x100000>,
> -                        <0x02000000 0x0 0x40300000 0x0 0x40300000 0x0 0x1fd00000>;
> +               ranges = <0x01000000 0x0 0x00000000 0x0 0x40000000 0x0 0x100000>,
> +                        <0x02000000 0x0 0x40100000 0x0 0x40100000 0x0 0x1ff00000>;
>                  bus-range = <0x00 0xff>;
>   
>                  dma-coherent;
>   
> 
> Krishna: Could you also try similar change on SC7280 and see if it works?
> 
> - Mani
>
Manivannan Sadhasivam March 28, 2025, 3:29 p.m. UTC | #6
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@oss.qualcomm.com>
> > > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > ---
> > > > 
> > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > 
> > > 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).

- Mani
Konrad Dybcio March 28, 2025, 8:44 p.m. UTC | #7
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@oss.qualcomm.com>
>>>>>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>>>> ---
>>>>>
>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>
>>>> 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
Manivannan Sadhasivam March 29, 2025, 6:25 a.m. UTC | #8
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@oss.qualcomm.com>
> >>>>>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >>>>>> ---
> >>>>>
> >>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>
> >>>> 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
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 0f2caf36910b..64c46221d8bf 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2201,10 +2201,10 @@  wifi: wifi@17a10040 {
 		pcie1: pcie@1c08000 {
 			compatible = "qcom,pcie-sc7280";
 			reg = <0 0x01c08000 0 0x3000>,
-			      <0 0x40000000 0 0xf1d>,
-			      <0 0x40000f20 0 0xa8>,
-			      <0 0x40001000 0 0x1000>,
-			      <0 0x40100000 0 0x100000>;
+			      <4 0x00000000 0 0xf1d>,
+			      <4 0x00000f20 0 0xa8>,
+			      <4 0x10000000 0 0x1000>,
+			      <4 0x00000000 0 0x10000000>;
 
 			reg-names = "parf", "dbi", "elbi", "atu", "config";
 			device_type = "pci";
@@ -2215,8 +2215,8 @@  pcie1: pcie@1c08000 {
 			#address-cells = <3>;
 			#size-cells = <2>;
 
-			ranges = <0x01000000 0x0 0x00000000 0x0 0x40200000 0x0 0x100000>,
-				 <0x02000000 0x0 0x40300000 0x0 0x40300000 0x0 0x1fd00000>;
+			ranges = <0x01000000 0x0 0x00000000 0x0 0x40000000 0x0 0x100000>,
+				 <0x02000000 0x0 0x40100000 0x0 0x40100000 0x0 0x1ff00000>;
 
 			interrupts = <GIC_SPI 307 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 308 IRQ_TYPE_LEVEL_HIGH>,