Change in coreboot[master]: amd/agesa: Make BottomIo position configurable

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Mike Banon (Code Review)

19 Jan 2020 19 Jan '20

6:48 p.m.

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)

Attachments:

attachment.htm (text/html — 5.9 KB)

Show replies by date

Mike Banon (Code Review)

19 Jan 19 Jan

7:22 p.m.

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 (Code Review)

10:23 p.m.

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 (Code Review)

20 Jan 20 Jan

12:10 p.m.

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 (Code Review)

22 Jan 22 Jan

2:28 p.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 22 Jan 2020 14:28:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Mike Banon (Code Review)

6:49 p.m.

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/38472/2

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Mike Banon (Code Review)

6:53 p.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 22 Jan 2020 18:53:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: comment

Mike Banon (Code Review)

24 Jan 24 Jan

4:16 p.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 24 Jan 2020 16:16:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Nico Huber (Code Review)

5:02 p.m.

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?

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 2 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 24 Jan 2020 17:02:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Mike Banon (Code Review)

25 Jan 25 Jan

7:59 a.m.

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

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/38472/3

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

build bot (Jenkins) (Code Review)

8 a.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sat, 25 Jan 2020 08:00:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Mike Banon (Code Review)

8:37 a.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sat, 25 Jan 2020 08:37:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Gerrit-MessageType: comment

Michał Żygowski (Code Review)

12 Feb 12 Feb

1:09 p.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Kyösti Mälkki kyosti.malkki@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 12 Feb 2020 13:09:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: build bot (Jenkins) no-reply@coreboot.org Gerrit-MessageType: comment

Nico Huber (Code Review)

14 Feb 14 Feb

12:08 p.m.

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?

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 3 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: HAOUAS Elyes ehaouas@noos.fr Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 14 Feb 2020 12:08:44 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

build bot (Jenkins) (Code Review)

27 Mar 27 Mar

7:12 a.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 4 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Fri, 27 Mar 2020 07:12:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Mike Banon (Code Review)

15 Apr 15 Apr

3:25 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 4 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 15 Apr 2020 15:25:30 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

build bot (Jenkins) (Code Review)

16 Apr 16 Apr

8:10 a.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 5 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 16 Apr 2020 08:10:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Paul Menzel (Code Review)

8:34 a.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 5 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 16 Apr 2020 08:34:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Mike Banon (Code Review)

9:17 a.m.

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/38472/6

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 6 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Mike Banon (Code Review)

9:19 a.m.

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.

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 6 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 16 Apr 2020 09:19:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: comment

Arthur Heymans (Code Review)

18 Apr 18 Apr

12:49 p.m.

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;

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 7 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sat, 18 Apr 2020 12:49:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Mike Banon (Code Review)

19 Apr 19 Apr

4:03 p.m.

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/38472/8

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 8 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Mike Banon (Code Review)

4:15 p.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 8 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sun, 19 Apr 2020 16:15:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans arthur@aheymans.xyz Gerrit-MessageType: comment

Michał Żygowski (Code Review)

21 Apr 21 Apr

10:19 a.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 8 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 21 Apr 2020 10:19:41 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Michał Żygowski (Code Review)

10:20 a.m.

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

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 8 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 21 Apr 2020 10:20:08 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Patrick Georgi (Code Review)

22 Apr 22 Apr

1:49 p.m.

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

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)

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 9 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

9elements QA (Code Review)

2:42 p.m.

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!

-- To view, visit https://review.coreboot.org/c/coreboot/+/38472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ie235631231bcb4aeebaff2e0026da2ea9d82f9d0 Gerrit-Change-Number: 38472 Gerrit-PatchSet: 9 Gerrit-Owner: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Michał Żygowski michal.zygowski@3mdeb.com Gerrit-Reviewer: Mike Banon mikebdp2@gmail.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: 9elements QA hardwaretestrobot@gmail.com Gerrit-CC: Arthur Heymans arthur@aheymans.xyz Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Wed, 22 Apr 2020 14:42:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1810

days inactive

1904

days old

coreboot-gerrit@coreboot.org

26 comments

8 participants

tags (0)

participants (8)

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)