Mike Banon has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
amd/agesa: Make BottomIo position configurable
Some PCI peripherals, such as discrete VGA adapters, require a great amount of memory mapped IO. This patch allows the user to select at build time the bottom IO to leave enough space for such devices.
We cannot calculate this value at runtime because it has to be set before the PCI devices are enumerated. 0x40000000 has been successfully boot-tested on A88XM-E (fam15tn), G505S (fam15tn) and AM1I-A (fam16kb).
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 --- M src/northbridge/amd/agesa/Kconfig M src/northbridge/amd/agesa/family14/state_machine.c M src/northbridge/amd/agesa/family15tn/state_machine.c M src/northbridge/amd/agesa/family16kb/state_machine.c 4 files changed, 16 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/38472/1
diff --git a/src/northbridge/amd/agesa/Kconfig b/src/northbridge/amd/agesa/Kconfig index e1e129a..346e2c3 100644 --- a/src/northbridge/amd/agesa/Kconfig +++ b/src/northbridge/amd/agesa/Kconfig @@ -20,6 +20,18 @@
if NORTHBRIDGE_AMD_AGESA
+config BOTTOMIO_POSITION + hex "Bottom of 32-bit IO space" + default 0x40000000 + help + If PCI peripherals with big BARs are connected to the system + the bottom of the IO must be decreased to allocate such + devices. + + Declare the beginning of the 128MB-aligned MMIO region. This + option is useful when PCI peripherals requesting large address + ranges are present. + config CONSOLE_VGA_MULTI bool default n diff --git a/src/northbridge/amd/agesa/family14/state_machine.c b/src/northbridge/amd/agesa/family14/state_machine.c index b49dac0..345f36a 100644 --- a/src/northbridge/amd/agesa/family14/state_machine.c +++ b/src/northbridge/amd/agesa/family14/state_machine.c @@ -60,6 +60,7 @@
void platform_BeforeInitPost(struct sysinfo *cb, AMD_POST_PARAMS *Post) { + Post->MemConfig.BottomIo = (UINT16)(CONFIG_BOTTOMIO_POSITION >> 24); }
void platform_AfterInitPost(struct sysinfo *cb, AMD_POST_PARAMS *Post) diff --git a/src/northbridge/amd/agesa/family15tn/state_machine.c b/src/northbridge/amd/agesa/family15tn/state_machine.c index dafb64c..092c781 100644 --- a/src/northbridge/amd/agesa/family15tn/state_machine.c +++ b/src/northbridge/amd/agesa/family15tn/state_machine.c @@ -30,6 +30,7 @@
void platform_BeforeInitPost(struct sysinfo *cb, AMD_POST_PARAMS *Post) { + Post->MemConfig.BottomIo = (UINT16)(CONFIG_BOTTOMIO_POSITION >> 24); }
void platform_AfterInitPost(struct sysinfo *cb, AMD_POST_PARAMS *Post) diff --git a/src/northbridge/amd/agesa/family16kb/state_machine.c b/src/northbridge/amd/agesa/family16kb/state_machine.c index 7794f2d..0ffa5d5 100644 --- a/src/northbridge/amd/agesa/family16kb/state_machine.c +++ b/src/northbridge/amd/agesa/family16kb/state_machine.c @@ -32,6 +32,8 @@ { AGESA_STATUS status;
+ Post->MemConfig.BottomIo = (UINT16)(CONFIG_BOTTOMIO_POSITION >> 24); + if (CONFIG(ENABLE_MRC_CACHE)) { status = OemInitResume(&Post->MemConfig.MemContext); if (status == AGESA_SUCCESS)
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 1:
Based on CB:17980. The only thing I'm a bit unsure, is where this "BottomIo line" should be situated at family16kb's state_machine.c file - before or after ENABLE_MRC_CACHE block.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 1:
What are the final MTRR settings with this? 2G is often a nice setting as it cleanly splits the 4G region in two, requiring only 1 MTRR to cover the bottom or top region.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38472/1/src/northbridge/amd/agesa/K... File src/northbridge/amd/agesa/Kconfig:
https://review.coreboot.org/c/coreboot/+/38472/1/src/northbridge/amd/agesa/K... PS1, Line 29: devices. Fits on line above?
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 1:
Patch Set 1:
What are the final MTRR settings with this? 2G is often a nice setting as it cleanly splits the 4G region in two, requiring only 1 MTRR to cover the bottom or top region.
I've done three Lenovo G505S builds: for 0x40, 0x80 and 0xc0 BottomIo values. From https://github.com/mikebdp2/bottomio_research files we could see that MTRR fixed ranges are the same (00000-9FFFF write-back, A0000-BFFFF uncachable, C0000-FFFFF write-back), while the MTRR variable ranges are:
0x40 :
reg00: base=0x0ffc00000 ( 4092MB), size= 4MB, count=1: write-protect // up to 0x1.0000.0000 reg01: base=0x000000000 ( 0MB), size= 512MB, count=1: write-back // up to 0x0.2000.0000
0x80 :
reg00: base=0x0ffc00000 ( 4092MB), size= 4MB, count=1: write-protect // up to 0x1.0000.0000 reg01: base=0x040000000 ( 1024MB), size= 512MB, count=1: write-back // up to 0x0.6000.0000 reg02: base=0x000000000 ( 0MB), size= 1024MB, count=1: write-back // up to 0.4000.0000
0xc0 :
reg00: base=0x0ffc00000 ( 4092MB), size= 4MB, count=1: write-protect // up to 0x1.0000.0000 reg01: base=0x080000000 ( 2048MB), size= 512MB, count=1: write-back // up to 0x0.a000.0000 reg02: base=0x000000000 ( 0MB), size= 2048MB, count=1: write-back // up to 0x0.8000.0000
Nico told me that a too low BottomIo setting could result in a performance hit:
You should check MTRR allocations before/after the change. If it runs out of registers, that can be bad for performance (depending on whether the CPU supports PAT).
Aside from
mtrr: your CPUs had inconsistent variable MTRR settings mtrr: probably your BIOS does not setup all CPUs. mtrr: corrected configuration.
at Linux kernel log - which is the same for 0x40/0x80/0xc0 and even for 0xd0, there weren't any other warning/error messages and the Linux kernel boot time was the same (if it could serve as a benchmark). In example, I haven't seen any "MTRR allocation failed. Graphics performance may suffer." messages yet. If I set BottomIo to 0x24 or below, a board will simply not boot, but will boot fine with 0x28 or above.
One AMD RX590 GPU needs a 0x20000000-wide MMIO resource to initialize successfully: so 0xe0-(1*0x20)=0xc0 or less BottomIo value is required for 1x RX590 ; 0xe0-(3*0x20)=0x80 or less BottomIo for 3x RX590 ; 0xe0-(5*0xa0)=0x40 or less for 5x RX590. Although none of coreboot-supported AMD boards have more than three PCIe slots, with the help of "PCIe splitters" people may be able to connect more than three GPUs to their board.
So, although I'm fine with changing a BottomIo position from 0x40000000 to 0x80000000, I'd be more happy to do this if there's a good reason: i.e. if still a real danger of performance hit.
Hello Kyösti Mälkki, Patrick Rudolph, HAOUAS Elyes, Angel Pons, build bot (Jenkins), Nico Huber, Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38472
to look at the new patch set (#2).
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
amd/agesa: Make BottomIo position configurable
Some PCI peripherals, such as discrete VGA adapters, require a great amount of memory mapped IO. This patch allows the user to select at build time the bottom IO to leave enough space for such devices.
We cannot calculate this value at runtime because it has to be set before the PCI devices are enumerated. 0x40000000 has been successfully boot-tested on A88XM-E (fam15tn), G505S (fam15tn) and AM1I-A (fam16kb).
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 --- M src/northbridge/amd/agesa/Kconfig M src/northbridge/amd/agesa/family14/state_machine.c M src/northbridge/amd/agesa/family15tn/state_machine.c M src/northbridge/amd/agesa/family16kb/state_machine.c 4 files changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/38472/2
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38472/1/src/northbridge/amd/agesa/K... File src/northbridge/amd/agesa/Kconfig:
https://review.coreboot.org/c/coreboot/+/38472/1/src/northbridge/amd/agesa/K... PS1, Line 29: devices.
Fits on line above?
Done.
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 2:
The board-specific "PCI resource allocation" changes have been abandoned in favor of this universal one, which benefits all the AGESA boards.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 2:
(3 comments)
What are the final MTRR settings with this? 2G is often a nice setting as it cleanly splits the 4G region in two, requiring only 1 MTRR to cover the bottom or top region.
I've done three Lenovo G505S builds: for 0x40, 0x80 and 0xc0 BottomIo values. From https://github.com/mikebdp2/bottomio_research files we could see that MTRR fixed ranges are the same (00000-9FFFF write-back, A0000-BFFFF uncachable, C0000-FFFFF write-back), while the MTRR variable ranges are:
Thanks for the investigation. At least everything that should be fully cached seems fine. I wonder, however, where the MTRR allo- cation is done with AGESA. It seems, it doesn't take prefetchable MMIO resources into account... let's just hope, Linux can cover the mess up.
Nico told me that a too low BottomIo setting could result in a performance hit:
You should check MTRR allocations before/after the change. If it runs out of registers, that can be bad for performance (depending on whether the CPU supports PAT).
My major concern was about prefetchable resources, but that seems to be ignored by these AGESA hook-ups anyway?
Aside from
mtrr: your CPUs had inconsistent variable MTRR settings mtrr: probably your BIOS does not setup all CPUs. mtrr: corrected configuration.
This is just more trouble in coreboot. I've been looking at the code (just a little) and couldn't find anything that would write the AP's variable MTRRs. Seems weird. But the whole mess reminds me that integrating vendorcode into coreboot, no matter if blob or open source, is just wrong.
So, although I'm fine with changing a BottomIo position from 0x40000000 to 0x80000000, I'd be more happy to do this if there's a good reason: i.e. if still a real danger of performance hit.
With the Kconfig option, we only have to discuss a default. People can choose whatever they want. I would stick to the old default though, because that seems most tested. And let's not forget about people that might just want to run a 32-bit OS (big hole would remap more RAM outside of 32-bit space).
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/K... File src/northbridge/amd/agesa/Kconfig:
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/K... PS2, Line 25: default 0x40000000 I would prefer to leave it at the current default and only override it on tested platforms.
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family14/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/f... PS2, Line 63: (UINT16) Why the explicit cast?
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/f... PS2, Line 63: Post->MemConfig.BottomIo = (UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) Should we mask it with `& 0xf8` to enforce the 128MiB alignment?
Also, how about some `MIN(0xe0, MAX(0x28, ...))` to make sure it doesn't brick because of misconfiguration?
Hello Kyösti Mälkki, Patrick Rudolph, HAOUAS Elyes, Angel Pons, build bot (Jenkins), Nico Huber, Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38472
to look at the new patch set (#3).
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
amd/agesa: Make BottomIo position configurable
Some PCI peripherals, such as discrete VGA adapters, require a great amount of memory mapped IO. This patch allows the user to select at build time the bottom IO to leave enough space for such devices.
We cannot calculate this value at runtime because it has to be set before the PCI devices are enumerated. 0x80000000 has been successfully boot-tested on A88XM-E (fam15tn), G505S (fam15tn) and AM1I-A (fam16kb).
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 --- M src/northbridge/amd/agesa/Kconfig M src/northbridge/amd/agesa/family14/state_machine.c M src/northbridge/amd/agesa/family15tn/state_machine.c M src/northbridge/amd/agesa/family16kb/state_machine.c 4 files changed, 15 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/38472/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38472/3/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family14/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/3/src/northbridge/amd/agesa/f... PS3, Line 63: Post->MemConfig.BottomIo = MIN(0xE0, MAX(0x28, ((UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) & 0xF8))); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38472/3/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family15tn/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/3/src/northbridge/amd/agesa/f... PS3, Line 33: Post->MemConfig.BottomIo = MIN(0xE0, MAX(0x28, ((UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) & 0xF8))); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38472/3/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family16kb/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/3/src/northbridge/amd/agesa/f... PS3, Line 35: Post->MemConfig.BottomIo = MIN(0xE0, MAX(0x28, ((UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) & 0xF8))); line over 96 characters
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/K... File src/northbridge/amd/agesa/Kconfig:
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/K... PS2, Line 25: default 0x40000000
I would prefer to leave it at the current default and only override it on […]
Even amd/pi has replaced 0xE0000000 value - since it gives the PCI resource allocation problems, would not be wise to intentionally provide a broken default. To ensure more stability I have changed 0x40000000 to 0x80000000 considering your 32-bit notes.
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family14/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/f... PS2, Line 63: Post->MemConfig.BottomIo = (UINT16)(CONFIG_BOTTOMIO_POSITION >> 24)
Should we mask it with `& 0xf8` to enforce the 128MiB alignment? […]
Done
https://review.coreboot.org/c/coreboot/+/38472/2/src/northbridge/amd/agesa/f... PS2, Line 63: (UINT16)
Why the explicit cast?
Has been borrowed from a merged CB:17980 for Pi.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38472/3/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family14/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/3/src/northbridge/amd/agesa/f... PS3, Line 63: Post->MemConfig.BottomIo = MIN(0xE0, MAX(0x28, ((UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) & 0xF8)));
line over 96 characters
Please make Jenkins happy and wrap it.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 3: Code-Review+1
Could we get a fam14 device tested? Paul, do you still have that E350M1?
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38472/4/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family14/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/4/src/northbridge/amd/agesa/f... PS4, Line 62: Post->MemConfig.BottomIo = MIN(0xE0, MAX(0x28, ((UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) & 0xF8))); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38472/4/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family15tn/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/4/src/northbridge/amd/agesa/f... PS4, Line 32: Post->MemConfig.BottomIo = MIN(0xE0, MAX(0x28, ((UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) & 0xF8))); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38472/4/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family16kb/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/4/src/northbridge/amd/agesa/f... PS4, Line 34: Post->MemConfig.BottomIo = MIN(0xE0, MAX(0x28, ((UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) & 0xF8))); line over 96 characters
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 4:
When could we get this merged? Without this change, people get a boot failure if a nice discrete GPU is installed, so this change is quite important
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38472/5/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family14/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/5/src/northbridge/amd/agesa/f... PS5, Line 51: Post->MemConfig.BottomIo = MIN(0xE0, MAX(0x28, ((UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) & 0xF8))); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38472/5/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family15tn/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/5/src/northbridge/amd/agesa/f... PS5, Line 21: Post->MemConfig.BottomIo = MIN(0xE0, MAX(0x28, ((UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) & 0xF8))); line over 96 characters
https://review.coreboot.org/c/coreboot/+/38472/5/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family16kb/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/5/src/northbridge/amd/agesa/f... PS5, Line 23: Post->MemConfig.BottomIo = MIN(0xE0, MAX(0x28, ((UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) & 0xF8))); line over 96 characters
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 5:
(1 comment)
Please address the 96 characters line limit warnings.
https://review.coreboot.org/c/coreboot/+/38472/5/src/northbridge/amd/agesa/K... File src/northbridge/amd/agesa/Kconfig:
https://review.coreboot.org/c/coreboot/+/38472/5/src/northbridge/amd/agesa/K... PS5, Line 31: ranges are present. …, for example, graphics cards.
Hello build bot (Jenkins), Nico Huber, Michał Żygowski, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38472
to look at the new patch set (#6).
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
amd/agesa: Make BottomIo position configurable
Some PCI peripherals, such as discrete VGA adapters, require a great amount of memory mapped IO. This patch allows the user to select at build time the bottom IO to leave enough space for such devices.
We cannot calculate this value at runtime because it has to be set before the PCI devices are enumerated. 0x80000000 has been successfully boot-tested on A88XM-E (fam15tn), G505S (fam15tn) and AM1I-A (fam16kb).
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 --- M src/northbridge/amd/agesa/Kconfig M src/northbridge/amd/agesa/family14/state_machine.c M src/northbridge/amd/agesa/family15tn/state_machine.c M src/northbridge/amd/agesa/family16kb/state_machine.c 4 files changed, 18 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/38472/6
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38472/5/src/northbridge/amd/agesa/K... File src/northbridge/amd/agesa/Kconfig:
https://review.coreboot.org/c/coreboot/+/38472/5/src/northbridge/amd/agesa/K... PS5, Line 31: ranges are present.
…, for example, graphics cards.
Everything done.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38472/7/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family16kb/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/7/src/northbridge/amd/agesa/f... PS7, Line 23: MIN(0xE0, : MAX(0x28, ((UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) & 0xF8))); It's a bit hard to read with magic numbers. I suggest (UINT16)(MIN(0xe0000000, MAX(0x28000000, CONFIG_BOTTOMIO_POSITION)) >> 24) & 0xf8;
Hello build bot (Jenkins), Nico Huber, Michał Żygowski, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38472
to look at the new patch set (#8).
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
amd/agesa: Make BottomIo position configurable
Some PCI peripherals, such as discrete VGA adapters, require a great amount of memory mapped IO. This patch allows the user to select at build time the bottom IO to leave enough space for such devices.
We cannot calculate this value at runtime because it has to be set before the PCI devices are enumerated. 0x80000000 has been successfully boot-tested on A88XM-E (fam15tn), G505S (fam15tn) and AM1I-A (fam16kb).
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 --- M src/northbridge/amd/agesa/Kconfig M src/northbridge/amd/agesa/family14/state_machine.c M src/northbridge/amd/agesa/family15tn/state_machine.c M src/northbridge/amd/agesa/family16kb/state_machine.c 4 files changed, 18 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/38472/8
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38472/7/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family16kb/state_machine.c:
https://review.coreboot.org/c/coreboot/+/38472/7/src/northbridge/amd/agesa/f... PS7, Line 23: MIN(0xE0, : MAX(0x28, ((UINT16)(CONFIG_BOTTOMIO_POSITION >> 24) & 0xF8)));
It's a bit hard to read with magic numbers. […]
Done for all
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 8: Code-Review+2
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 8:
Tested on apu1 and haven't found issues with this change
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
amd/agesa: Make BottomIo position configurable
Some PCI peripherals, such as discrete VGA adapters, require a great amount of memory mapped IO. This patch allows the user to select at build time the bottom IO to leave enough space for such devices.
We cannot calculate this value at runtime because it has to be set before the PCI devices are enumerated. 0x80000000 has been successfully boot-tested on A88XM-E (fam15tn), G505S (fam15tn) and AM1I-A (fam16kb).
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Reviewed-on: https://review.coreboot.org/c/coreboot/+/38472 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Michał Żygowski michal.zygowski@3mdeb.com --- M src/northbridge/amd/agesa/Kconfig M src/northbridge/amd/agesa/family14/state_machine.c M src/northbridge/amd/agesa/family15tn/state_machine.c M src/northbridge/amd/agesa/family16kb/state_machine.c 4 files changed, 18 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Michał Żygowski: Looks good to me, approved
diff --git a/src/northbridge/amd/agesa/Kconfig b/src/northbridge/amd/agesa/Kconfig index 19b62f2..1e0153b 100644 --- a/src/northbridge/amd/agesa/Kconfig +++ b/src/northbridge/amd/agesa/Kconfig @@ -19,6 +19,17 @@
if NORTHBRIDGE_AMD_AGESA
+config BOTTOMIO_POSITION + hex "Bottom of 32-bit IO space" + default 0x80000000 + help + If PCI peripherals with big BARs are connected to the system + the bottom of the IO must be decreased to allocate such devices. + + Declare the beginning of the 128MB-aligned MMIO region. This + option is useful when PCI peripherals requesting large address + ranges are present, for example, graphic cards. + config CONSOLE_VGA_MULTI bool default n diff --git a/src/northbridge/amd/agesa/family14/state_machine.c b/src/northbridge/amd/agesa/family14/state_machine.c index db923c5..5b00408 100644 --- a/src/northbridge/amd/agesa/family14/state_machine.c +++ b/src/northbridge/amd/agesa/family14/state_machine.c @@ -48,6 +48,8 @@
void platform_BeforeInitPost(struct sysinfo *cb, AMD_POST_PARAMS *Post) { + Post->MemConfig.BottomIo = (UINT16)(MIN(0xE0000000, + MAX(0x28000000, CONFIG_BOTTOMIO_POSITION)) >> 24) & 0xF8; }
void platform_AfterInitPost(struct sysinfo *cb, AMD_POST_PARAMS *Post) diff --git a/src/northbridge/amd/agesa/family15tn/state_machine.c b/src/northbridge/amd/agesa/family15tn/state_machine.c index 85bc258..439e15d 100644 --- a/src/northbridge/amd/agesa/family15tn/state_machine.c +++ b/src/northbridge/amd/agesa/family15tn/state_machine.c @@ -18,6 +18,8 @@
void platform_BeforeInitPost(struct sysinfo *cb, AMD_POST_PARAMS *Post) { + Post->MemConfig.BottomIo = (UINT16)(MIN(0xE0000000, + MAX(0x28000000, CONFIG_BOTTOMIO_POSITION)) >> 24) & 0xF8; }
void platform_AfterInitPost(struct sysinfo *cb, AMD_POST_PARAMS *Post) diff --git a/src/northbridge/amd/agesa/family16kb/state_machine.c b/src/northbridge/amd/agesa/family16kb/state_machine.c index a279629..be9adaf 100644 --- a/src/northbridge/amd/agesa/family16kb/state_machine.c +++ b/src/northbridge/amd/agesa/family16kb/state_machine.c @@ -20,6 +20,9 @@ { AGESA_STATUS status;
+ Post->MemConfig.BottomIo = (UINT16)(MIN(0xE0000000, + MAX(0x28000000, CONFIG_BOTTOMIO_POSITION)) >> 24) & 0xF8; + if (CONFIG(ENABLE_MRC_CACHE)) { status = OemInitResume(&Post->MemConfig.MemContext); if (status == AGESA_SUCCESS)
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38472 )
Change subject: amd/agesa: Make BottomIo position configurable ......................................................................
Patch Set 9:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2518 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2517 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2516 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2515
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Michał Żygowski (Code Review)
-
Mike Banon (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)