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/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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78486): https://edk2.groups.io/g/devel/message/78486
Mute This Topic: https://groups.io/mt/84605051/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78499): https://edk2.groups.io/g/devel/message/78499
Mute This Topic: https://groups.io/mt/84605051/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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
> >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78526): https://edk2.groups.io/g/devel/message/78526
Mute This Topic: https://groups.io/mt/84605051/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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
> > >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78583): https://edk2.groups.io/g/devel/message/78583
Mute This Topic: https://groups.io/mt/84605051/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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
> > > >


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78586): https://edk2.groups.io/g/devel/message/78586
Mute This Topic: https://groups.io/mt/84605051/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-