[edk2-platforms PATCH 3/6] Marvell: Armada7k8kPciHostBridgeLib: Remove ECAM base limitation


Marcin Wojtas
 

On CN913x-based platforms it is possible to have up to 9 PCIE
root complexes. In such case it may be necessary to configure
more configuration spaces with smaller bus count, so that
to fit the memory layout constraints. For that purpose remove
forcing ECAM base to be divisible by SIZE_256MB.

Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridg=
eLibConstructor.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/=
PciHostBridgeLibConstructor.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k=
8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
index 067e57a2dc..87e57aeae3 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHost=
BridgeLibConstructor.c
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHost=
BridgeLibConstructor.c
@@ -219,7 +219,6 @@ Armada7k8kPciHostBridgeLibConstructor (
PcieController =3D &(BoardPcieDescription->PcieControllers[Index]);=0D
=0D
ASSERT (PcieController->PcieBusMin =3D=3D 0);=0D
- ASSERT (PcieController->ConfigSpaceAddress % SIZE_256MB =3D=3D 0);=0D
=0D
if (PcieController->HaveResetGpio =3D=3D TRUE) {=0D
/* Reset PCIE slot */=0D
--=20
2.29.0


Ard Biesheuvel
 

On Mon, 2 Aug 2021 at 07:01, Marcin Wojtas <mw@semihalf.com> wrote:

On CN913x-based platforms it is possible to have up to 9 PCIE
root complexes. In such case it may be necessary to configure
more configuration spaces with smaller bus count, so that
to fit the memory layout constraints. For that purpose remove
forcing ECAM base to be divisible by SIZE_256MB.
There is one subtlety here that we need to take into account: IIUC,
PCIe requires that the ECAM start address of bus N equals N MB modulo
256 MB. In other words, if your ECAM range lives at 1 GB + 128 MB, the
bus range has to start at bus 128.

I think OSes are usually quite lax about this, but it is something to
double check regardless, even for existing platforms


Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
index 067e57a2dc..87e57aeae3 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
@@ -219,7 +219,6 @@ Armada7k8kPciHostBridgeLibConstructor (
PcieController = &(BoardPcieDescription->PcieControllers[Index]);

ASSERT (PcieController->PcieBusMin == 0);
- ASSERT (PcieController->ConfigSpaceAddress % SIZE_256MB == 0);

if (PcieController->HaveResetGpio == TRUE) {
/* Reset PCIE slot */
--
2.29.0


Marcin Wojtas
 

Hi Ard,

pon., 2 sie 2021 o 10:43 Ard Biesheuvel <ardb@kernel.org> napisał(a):

On Mon, 2 Aug 2021 at 07:01, Marcin Wojtas <mw@semihalf.com> wrote:

On CN913x-based platforms it is possible to have up to 9 PCIE
root complexes. In such case it may be necessary to configure
more configuration spaces with smaller bus count, so that
to fit the memory layout constraints. For that purpose remove
forcing ECAM base to be divisible by SIZE_256MB.
There is one subtlety here that we need to take into account: IIUC,
PCIe requires that the ECAM start address of bus N equals N MB modulo
256 MB. In other words, if your ECAM range lives at 1 GB + 128 MB, the
bus range has to start at bus 128.

I think OSes are usually quite lax about this, but it is something to
double check regardless, even for existing platforms
I tested a wide range of OSs (various Linux distributions, Win10 PE,
FreeBSD, OpenBSD and of course EDK2) and with 7 ECAMs, of which 6 are
squeezed within 256MB memory chunk together with their mmio32 and no
issue was observed. Moreover, if you recall, contrary to the EDK2,
where the full bus number is used, in ACPI we expose a single 1MB
space with the ECAM base address aligned to 0x8000.

Do you wish to change the assertion in EDK2 instead of removing?

Thanks,
Marcin


Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
index 067e57a2dc..87e57aeae3 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
@@ -219,7 +219,6 @@ Armada7k8kPciHostBridgeLibConstructor (
PcieController = &(BoardPcieDescription->PcieControllers[Index]);

ASSERT (PcieController->PcieBusMin == 0);
- ASSERT (PcieController->ConfigSpaceAddress % SIZE_256MB == 0);

if (PcieController->HaveResetGpio == TRUE) {
/* Reset PCIE slot */
--
2.29.0


Ard Biesheuvel
 

On Mon, 2 Aug 2021 at 19:00, Marcin Wojtas <mw@semihalf.com> wrote:

Hi Ard,

pon., 2 sie 2021 o 10:43 Ard Biesheuvel <ardb@kernel.org> napisał(a):

On Mon, 2 Aug 2021 at 07:01, Marcin Wojtas <mw@semihalf.com> wrote:

On CN913x-based platforms it is possible to have up to 9 PCIE
root complexes. In such case it may be necessary to configure
more configuration spaces with smaller bus count, so that
to fit the memory layout constraints. For that purpose remove
forcing ECAM base to be divisible by SIZE_256MB.
There is one subtlety here that we need to take into account: IIUC,
PCIe requires that the ECAM start address of bus N equals N MB modulo
256 MB. In other words, if your ECAM range lives at 1 GB + 128 MB, the
bus range has to start at bus 128.

I think OSes are usually quite lax about this, but it is something to
double check regardless, even for existing platforms
I tested a wide range of OSs (various Linux distributions, Win10 PE,
FreeBSD, OpenBSD and of course EDK2) and with 7 ECAMs, of which 6 are
squeezed within 256MB memory chunk together with their mmio32 and no
issue was observed. Moreover, if you recall, contrary to the EDK2,
where the full bus number is used, in ACPI we expose a single 1MB
space with the ECAM base address aligned to 0x8000.
Ah yes, I had forgotten about that hack :-)

Do you wish to change the assertion in EDK2 instead of removing?
No worries - if all those OSes are fine with this, I don't see a point
in being pedantic. I will note, however, that you can still comply
with this requirement by changing the bus ranges: each RC only uses a
single bus, but that bus number could be (ECAM base address / 1M) %
256



Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
index 067e57a2dc..87e57aeae3 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
@@ -219,7 +219,6 @@ Armada7k8kPciHostBridgeLibConstructor (
PcieController = &(BoardPcieDescription->PcieControllers[Index]);

ASSERT (PcieController->PcieBusMin == 0);
- ASSERT (PcieController->ConfigSpaceAddress % SIZE_256MB == 0);

if (PcieController->HaveResetGpio == TRUE) {
/* Reset PCIE slot */
--
2.29.0


Marcin Wojtas
 

wt., 3 sie 2021 o 08:53 Ard Biesheuvel <ardb@kernel.org> napisał(a):

On Mon, 2 Aug 2021 at 19:00, Marcin Wojtas <mw@semihalf.com> wrote:

Hi Ard,

pon., 2 sie 2021 o 10:43 Ard Biesheuvel <ardb@kernel.org> napisał(a):

On Mon, 2 Aug 2021 at 07:01, Marcin Wojtas <mw@semihalf.com> wrote:

On CN913x-based platforms it is possible to have up to 9 PCIE
root complexes. In such case it may be necessary to configure
more configuration spaces with smaller bus count, so that
to fit the memory layout constraints. For that purpose remove
forcing ECAM base to be divisible by SIZE_256MB.
There is one subtlety here that we need to take into account: IIUC,
PCIe requires that the ECAM start address of bus N equals N MB modulo
256 MB. In other words, if your ECAM range lives at 1 GB + 128 MB, the
bus range has to start at bus 128.

I think OSes are usually quite lax about this, but it is something to
double check regardless, even for existing platforms
I tested a wide range of OSs (various Linux distributions, Win10 PE,
FreeBSD, OpenBSD and of course EDK2) and with 7 ECAMs, of which 6 are
squeezed within 256MB memory chunk together with their mmio32 and no
issue was observed. Moreover, if you recall, contrary to the EDK2,
where the full bus number is used, in ACPI we expose a single 1MB
space with the ECAM base address aligned to 0x8000.
Ah yes, I had forgotten about that hack :-)
A great one though.


Do you wish to change the assertion in EDK2 instead of removing?
No worries - if all those OSes are fine with this, I don't see a point
in being pedantic. I will note, however, that you can still comply
with this requirement by changing the bus ranges: each RC only uses a
single bus, but that bus number could be (ECAM base address / 1M) %
256
For OS's there is indeed only bus0 exposed, but I plan to make it
tunable, so that to use entire range (e.g. for FreeBSD). In EDK2 there
is full coverage. FYI, in the platform I plan to submit after this
patchset there 7 RC's: 1 with 255 and 6 with 15 busses (the last 1 MB
in each case is used for IO space).

Best regards,
Marcin



Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
index 067e57a2dc..87e57aeae3 100644
--- a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
+++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kPciHostBridgeLib/PciHostBridgeLibConstructor.c
@@ -219,7 +219,6 @@ Armada7k8kPciHostBridgeLibConstructor (
PcieController = &(BoardPcieDescription->PcieControllers[Index]);

ASSERT (PcieController->PcieBusMin == 0);
- ASSERT (PcieController->ConfigSpaceAddress % SIZE_256MB == 0);

if (PcieController->HaveResetGpio == TRUE) {
/* Reset PCIE slot */
--
2.29.0