Change in coreboot[master]: asus/am1i-a: fix the VGA-related PCI resource allocation problems
Mike Banon has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38214 )
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
asus/am1i-a: fix the VGA-related PCI resource allocation problems
This workaround helps to avoid the PCI resource allocation problems which happen if a discrete VGA adapter (i.e. AMD HD6670) has been plugged in to AM1I-A board. It does not bring any downsides if there is no discrete VGA adapter.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I84f72459753ae6631ab4b318b71ce89968a336ad --- M src/mainboard/asus/am1i-a/OemCustomize.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/38214/1
diff --git a/src/mainboard/asus/am1i-a/OemCustomize.c b/src/mainboard/asus/am1i-a/OemCustomize.c index e001d43..b6ce293 100644 --- a/src/mainboard/asus/am1i-a/OemCustomize.c +++ b/src/mainboard/asus/am1i-a/OemCustomize.c @@ -148,6 +148,8 @@
void board_BeforeInitPost(struct sysinfo *cb, AMD_POST_PARAMS *InitPost) { + /* Set to 0xD0 instead of 0xE0 to avoid the PCI resource allocation problems. */ + InitPost->MemConfig.BottomIo = 0xD0; InitPost->MemConfig.PlatformMemoryConfiguration = (PSO_ENTRY *)PlatformMemoryTable; }
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38214 )
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
Patch Set 1:
This change turned out to be a standalone, and consists of just two lines of code. Please review.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38214 )
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38214/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38214/1//COMMIT_MSG@10 PS1, Line 10: in to into
https://review.coreboot.org/c/coreboot/+/38214/1//COMMIT_MSG@10 PS1, Line 10: has been is
https://review.coreboot.org/c/coreboot/+/38214/1//COMMIT_MSG@12 PS1, Line 12: Please describe your fix.
Hello Kyösti Mälkki, Angel Pons, Elisenda, build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38214
to look at the new patch set (#2).
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
asus/am1i-a: fix the VGA-related PCI resource allocation problems
This workaround helps to avoid the PCI resource allocation problems which happen if a discrete VGA adapter (i.e. AMD HD6670) is plugged into AM1I-A board. It does not bring any downsides if there is no discrete VGA adapter.
If PCI peripherals with big BARs are connected to the system the bottom of the IO must be decreased to allocate such devices.
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I84f72459753ae6631ab4b318b71ce89968a336ad --- M src/mainboard/asus/am1i-a/OemCustomize.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/38214/2
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38214 )
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
Patch Set 2:
(3 comments)
Took an excerpt of Kconfig provided by Michal at CB:38224 as description. If it's good enough, I could add it to the same changes I've done for the other AGESA boards - CB:38215, CB:38216, CB:38217, CB:38218, CB:38219.
https://review.coreboot.org/c/coreboot/+/38214/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38214/1//COMMIT_MSG@10 PS1, Line 10: in to
into
Done.
https://review.coreboot.org/c/coreboot/+/38214/1//COMMIT_MSG@10 PS1, Line 10: has been
is
Done.
https://review.coreboot.org/c/coreboot/+/38214/1//COMMIT_MSG@12 PS1, Line 12:
Please describe your fix.
Done.
Hello Kyösti Mälkki, Angel Pons, Elisenda, build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38214
to look at the new patch set (#3).
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
asus/am1i-a: fix the VGA-related PCI resource allocation problems
This workaround helps to avoid the PCI resource allocation problems which happen if a discrete VGA adapter (i.e. AMD HD6670) is plugged into AM1I-A board. It does not bring any downsides if there is no discrete VGA adapter.
If PCI peripherals with big BARs are connected to the system the bottom of the IO must be decreased to allocate such devices. Otherwise there could be i.e. a "Fatal error during GPU init".
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I84f72459753ae6631ab4b318b71ce89968a336ad --- M src/mainboard/asus/am1i-a/OemCustomize.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/38214/3
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38214 )
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG@11 PS3, Line 11: It does not bring any downsides if there is no discrete VGA adapter. Please wrap the lines after 75 characters.
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG@15 PS3, Line 15: Otherwise there could be i.e. a "Fatal error during GPU init". Maybe show the BARs reported by Linux (or coreboot) before and after.
Hello Kyösti Mälkki, Angel Pons, Elisenda, build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38214
to look at the new patch set (#4).
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
asus/am1i-a: fix the VGA-related PCI resource allocation problems
This workaround helps to avoid the PCI resource allocation problems which happen if a discrete VGA adapter (i.e. AMD HD6670) is plugged into AM1I-A board. It doesn't bring any downsides if there is no discrete VGA adapter.
If PCI peripherals with big BARs are connected to the system the bottom of the IO must be decreased to allocate such devices. Otherwise there could be i.e. a "Fatal error during GPU init".
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I84f72459753ae6631ab4b318b71ce89968a336ad --- M src/mainboard/asus/am1i-a/OemCustomize.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/38214/4
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38214 )
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG@11 PS3, Line 11: It does not bring any downsides if there is no discrete VGA adapter.
Please wrap the lines after 75 characters.
Done.
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG@15 PS3, Line 15: Otherwise there could be i.e. a "Fatal error during GPU init".
Maybe show the BARs reported by Linux (or coreboot) before and after.
A bit difficult to compare, considering I can't reach a Linux console if there's such an error. :( I tried to compare the coreboot logs using an EHCI dongle, some addresses are lower by 0x10000000 as expected: i.e. add_uma_resource_below_tolm memory_start: 0xc0000000 if no fix, 0xb0000000 with fix; DOMAIN: 0000 resource base c0000000 if no fix, 0xb0000000 with fix; etc. However, despite a console level SPEW 8 I don't see any added error messages and many PCI parts print the same info; I'm a bit lost to be honest.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38214 )
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
Patch Set 4:
(1 comment)
I wonder if we shouldn't add a global Kconfig option for that and handle it in AGESA state machine...
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG@15 PS3, Line 15: Otherwise there could be i.e. a "Fatal error during GPU init".
A bit difficult to compare, considering I can't reach a Linux console if there's such an error. […]
At least you can try without dGPU to get a comparison.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38214 )
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
Patch Set 4:
(3 comments)
Please mention in the commit message what this change does (not what symptoms it fixes). I assume it lowers the MMIO hole from 0xe0000000 to 0xd0000000. Is it just that?
Assuming it is:
* 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). * A modern platform should support memory remapping (i.e. can address the DRAM behind the hole above 4GiB). In that case, you can add a safety margin at no cost, e.g. 0xc0000000, or even 0x80000000. * Why is this handled at the mainboard level? * Please also mention in the commit message that this is about large MMIO resources. Not really VGA specific.
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG@11 PS3, Line 11: It does not bring any downsides if there is no discrete VGA adapter.
Done.
For the record, this is how breaking at 75 chars looks like in a narrow Gerrit view:
asus/am1i-a: fix the VGA-related PCI resource allocation problems
This workaround helps to avoid the PCI resource allocation problems which happen if a discrete VGA adapter (i.e. AMD HD6670) is plugged into AM1I- A board. It doesn't bring any downsides if there is no discrete VGA adapter.
The standard is 72 chars.
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG@14 PS3, Line 14: IO You are referring to MMIO, right? IO is easily confused with i/o ports. Especially when you talk about VGA resources.
https://review.coreboot.org/c/coreboot/+/38214/3/src/mainboard/asus/am1i-a/O... File src/mainboard/asus/am1i-a/OemCustomize.c:
https://review.coreboot.org/c/coreboot/+/38214/3/src/mainboard/asus/am1i-a/O... PS3, Line 152: InitPost->MemConfig.BottomIo = 0xD0; Is this about MMIO? is the value shifted by 24 bit?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38214 )
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
Patch Set 4: Code-Review+2
Hello Kyösti Mälkki, Patrick Rudolph, Angel Pons, Elisenda, build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38214
to look at the new patch set (#5).
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
asus/am1i-a: fix the VGA-related PCI resource allocation problems
This workaround helps to avoid the PCI resource allocation problems which happen if a discrete VGA adapter (i.e. AMD HD6670) is plugged into AM1I-A board. It doesn't bring any downsides if there is no discrete VGA adapter.
If PCI peripherals with big BARs are connected to the system the bottom of the IO must be decreased to allocate such devices. Otherwise there could be i.e. a "Fatal error during GPU init".
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I84f72459753ae6631ab4b318b71ce89968a336ad --- M src/mainboard/asus/am1i-a/OemCustomize.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/38214/5
Hello Kyösti Mälkki, Patrick Rudolph, Angel Pons, Elisenda, build bot (Jenkins), Michał Żygowski,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38214
to look at the new patch set (#6).
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
asus/am1i-a: fix the VGA-related PCI resource allocation problems
This workaround helps to avoid the PCI resource allocation problems which happen if a discrete VGA adapter (i.e. AMD HD6670) is plugged into AM1I-A. It doesn't cause any problem if no discrete VGA adapter.
If PCI peripherals with big BARs are connected to the system the bottom of the IO must be decreased to allocate such devices. Otherwise there could be i.e. a "Fatal error during GPU init".
Signed-off-by: Mike Banon mikebdp2@gmail.com Change-Id: I84f72459753ae6631ab4b318b71ce89968a336ad --- M src/mainboard/asus/am1i-a/OemCustomize.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/38214/6
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38214 )
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG@11 PS3, Line 11: It does not bring any downsides if there is no discrete VGA adapter.
For the record, this is how breaking at 75 chars looks like in a narrow […]
Done.
https://review.coreboot.org/c/coreboot/+/38214/3//COMMIT_MSG@14 PS3, Line 14: IO
You are referring to MMIO, right? IO is easily confused with i/o ports. […]
Yes, memory mapped IO. Maybe I should do like this commit - https://gitlab.miaounyan.eu/piernov/coreboot/commit/27c3dcb522722026479462dd... - for fam14, fam15 and fam16 boards.
https://review.coreboot.org/c/coreboot/+/38214/3/src/mainboard/asus/am1i-a/O... File src/mainboard/asus/am1i-a/OemCustomize.c:
https://review.coreboot.org/c/coreboot/+/38214/3/src/mainboard/asus/am1i-a/O... PS3, Line 152: InitPost->MemConfig.BottomIo = 0xD0;
Is this about MMIO? is the value shifted by 24 bit?
Yes, about MMIO and it is shifted by 24 bit. From ./src/vendorcode/amd/agesa/f16kb/AGESA.h :
// Advanced (Optional parameters) // Optional (all defaults values will be initialized by the // 'AmdMemInitDataStructDef' based on AMD defaults. It is up // to the IBV/OEM to change the defaults after initialization // but prior to the main entry to the memory code):
// Memory Map/Mgt.
IN UINT16 BottomIo; ///< Bottom of 32-bit IO space (8-bits). ///< NV_BOTTOM_IO[7:0]=Addr[31:24] ///<
https://review.coreboot.org/c/coreboot/+/38214/5/src/mainboard/asus/am1i-a/O... File src/mainboard/asus/am1i-a/OemCustomize.c:
https://review.coreboot.org/c/coreboot/+/38214/5/src/mainboard/asus/am1i-a/O... PS5, Line 151: /* Set to 0x40 instead of 0xE0 to avoid the PCI resource allocation problems. */ : InitPost->MemConfig.BottomIo = 0x40; See CB:30987 comment here for more info about this change from 0xD0 to 0x40 - https://review.coreboot.org/c/coreboot/+/30987/45/src/mainboard/asus/a88xm-e...
Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38214 )
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
Patch Set 6:
Patch Set 4:
- 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).
I see these messages at Linux kernel log:
mtrr: your CPUs had inconsistent variable MTRR settings mtrr: probably your BIOS does not setup all CPUs. mtrr: corrected configuration.
However, these messages were present even without this fix, as I could see from the earlier board_status reports, so it's not a fault of this fix.
- A modern platform should support memory remapping (i.e. can address the DRAM behind the hole above 4GiB). In that case, you can add a safety margin at no cost, e.g. 0xc0000000, or even 0x80000000.
You are right, at another board A88XM-E I've discovered that 0x28 and above is working perfectly, while 0x24 and below gives the not-bootable builds. Decided to pick 0x40 for safety and it works well on G505S / AM1I-A / A88XM-E. Hopefully the people with the other boards will also boot test the similar changes. Thank you very much for inspiring this research.
- Why is this handled at the mainboard level?
Thought that a tiny 2-line change for each individual board, would be easier to get merged than a single massive one which touches many boards. But, seeing the success of an older https://gitlab.miaounyan.eu/piernov/coreboot/commit/27c3dcb522722026479462dd... which landed only to AMD PI boards, maybe I should try a similar approach for AGESA fam14/15/16.
- Please also mention in the commit message that this is about large MMIO resources. Not really VGA specific.
Maybe I should start it from scratch with a single change touching many boards. Will try to take in attention your this note and the other too, thank you very much for your feedback.
Mike Banon has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38214 )
Change subject: asus/am1i-a: fix the VGA-related PCI resource allocation problems ......................................................................
Abandoned
Superseded by CB:38472 (amd/agesa: Make BottomIo position configurable)
-
Michał Żygowski (Code Review) -
Mike Banon (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)