Krystian Hebel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
AGESA: Fix discovery of IO APICs number
Number of IO APICs are hardcoded. This fix reads this number from the default values obtained by creating a structure for a parameter block of an AGESA.
While it is possible to modify NumberOfIoApics in callouts, every platform currently in the tree goes with the default value.
Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com Change-Id: Ib3ddfe606720143659e57fbbca7f7a3e655a7664 --- M src/drivers/amd/agesa/state_machine.c M src/northbridge/amd/agesa/agesa_helper.h M src/northbridge/amd/agesa/family15tn/northbridge.c M src/northbridge/amd/agesa/family16kb/northbridge.c 4 files changed, 21 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/34589/1
diff --git a/src/drivers/amd/agesa/state_machine.c b/src/drivers/amd/agesa/state_machine.c index 750d192..7964f4e 100644 --- a/src/drivers/amd/agesa/state_machine.c +++ b/src/drivers/amd/agesa/state_machine.c @@ -297,6 +297,23 @@ return (final < AGESA_FATAL) ? 0 : -1; }
+int amd_get_ioapic_num(void) { + AMD_INTERFACE_PARAMS aip; + AMD_EARLY_PARAMS *params; + int ret; + + agesa_locate_image(&aip.StdHeader); + + amd_create_struct(&aip, AMD_INIT_EARLY, NULL, 0); + + params = (AMD_EARLY_PARAMS *)aip.NewStructPtr; + ret = params->PlatformConfig.NumberOfIoApics; + + amd_release_struct(&aip); + + return ret; +} + #if ENV_RAMSTAGE
static void amd_bs_ramstage_init(void *arg) diff --git a/src/northbridge/amd/agesa/agesa_helper.h b/src/northbridge/amd/agesa/agesa_helper.h index 17819e9..97eba2f 100644 --- a/src/northbridge/amd/agesa/agesa_helper.h +++ b/src/northbridge/amd/agesa/agesa_helper.h @@ -39,6 +39,8 @@ void amd_initmmio(void); void amd_initenv(void);
+int amd_get_ioapic_num(void); + void *GetHeapBase(void); void EmptyHeap(void);
diff --git a/src/northbridge/amd/agesa/family15tn/northbridge.c b/src/northbridge/amd/agesa/family15tn/northbridge.c index c6457a3..e9a1e05 100644 --- a/src/northbridge/amd/agesa/family15tn/northbridge.c +++ b/src/northbridge/amd/agesa/family15tn/northbridge.c @@ -870,7 +870,7 @@ * for their APIC id and therefore must reside at 0..15 */
- u8 plat_num_io_apics = 3; /* FIXME */ + u8 plat_num_io_apics = amd_get_ioapic_num();
if ((node_nums * core_max) + plat_num_io_apics >= 0x10) { lapicid_start = (plat_num_io_apics - 1) / core_max; diff --git a/src/northbridge/amd/agesa/family16kb/northbridge.c b/src/northbridge/amd/agesa/family16kb/northbridge.c index 928d9d2..b3b68f9 100644 --- a/src/northbridge/amd/agesa/family16kb/northbridge.c +++ b/src/northbridge/amd/agesa/family16kb/northbridge.c @@ -896,7 +896,7 @@ * for their APIC id and therefore must reside at 0..15 */
- u8 plat_num_io_apics = 3; /* FIXME */ + u8 plat_num_io_apics = amd_get_ioapic_num();
if ((node_nums * core_max) + plat_num_io_apics >= 0x10) { lapicid_start = (plat_num_io_apics - 1) / core_max;
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34589/1/src/drivers/amd/agesa/state... File src/drivers/amd/agesa/state_machine.c:
https://review.coreboot.org/c/coreboot/+/34589/1/src/drivers/amd/agesa/state... PS1, Line 300: int amd_get_ioapic_num(void) { open brace '{' following function definitions go on the next line
Krystian Hebel has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
AGESA: Fix discovery of IO APICs number
Number of IO APICs are hardcoded. This fix reads this number from the default values obtained by creating a structure for a parameter block of an AGESA.
While it is possible to modify NumberOfIoApics in callouts, every platform currently in the tree goes with the default value.
Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com Change-Id: Ib3ddfe606720143659e57fbbca7f7a3e655a7664 --- M src/drivers/amd/agesa/state_machine.c M src/northbridge/amd/agesa/agesa_helper.h M src/northbridge/amd/agesa/family15tn/northbridge.c M src/northbridge/amd/agesa/family16kb/northbridge.c 4 files changed, 22 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/34589/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
Tested how? Does it increase size or boot time?
https://review.coreboot.org/c/coreboot/+/34589/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34589/2//COMMIT_MSG@9 PS2, Line 9: are is
https://review.coreboot.org/c/coreboot/+/34589/2//COMMIT_MSG@10 PS2, Line 10: parameter : block of an AGESA What is that?
Michał Żygowski has uploaded a new patch set (#3) to the change originally created by Krystian Hebel. ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
AGESA: Fix discovery of IO APICs number
Number of IO APICs is hardcoded. This fix reads this number from the default values obtained by interfacing AGESA and accessing its parameter block.
While it is possible to modify NumberOfIoApics in callouts, every platform currently in the tree goes with the default value.
Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com Change-Id: Ib3ddfe606720143659e57fbbca7f7a3e655a7664 --- M src/drivers/amd/agesa/state_machine.c M src/northbridge/amd/agesa/agesa_helper.h M src/northbridge/amd/agesa/family15tn/northbridge.c M src/northbridge/amd/agesa/family16kb/northbridge.c 4 files changed, 22 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/34589/3
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
Patch Set 3:
(2 comments)
Tested with booting PC Engines APU2 on top of the postcar stage patches in the relation chain.
It neither improves boot time nor size. Just removes the hardcoded value which is not elegant by reading the value from silicon initialization code.
https://review.coreboot.org/c/coreboot/+/34589/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34589/2//COMMIT_MSG@9 PS2, Line 9: are
is
Done
https://review.coreboot.org/c/coreboot/+/34589/2//COMMIT_MSG@10 PS2, Line 10: parameter : block of an AGESA
What is that?
Rephrased it a little bit. In short, we interface AMD proprietary initialization blob called AGESA which holds the configuration values for the processor/SoC used in initialization process inside the blob.
Michał Żygowski has uploaded a new patch set (#4) to the change originally created by Krystian Hebel. ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
AGESA: Fix discovery of IO APICs number
Number of IO APICs are hardcoded. This fix reads this number from the default values obtained by creating a structure for a parameter block of an AGESA.
While it is possible to modify NumberOfIoApics in callouts, every platform currently in the tree goes with the default value.
Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com Change-Id: Ib3ddfe606720143659e57fbbca7f7a3e655a7664 --- M src/drivers/amd/agesa/state_machine.c M src/northbridge/amd/agesa/agesa_helper.h M src/northbridge/amd/agesa/family15tn/northbridge.c M src/northbridge/amd/agesa/family16kb/northbridge.c 4 files changed, 21 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/34589/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34589/4/src/drivers/amd/agesa/state... File src/drivers/amd/agesa/state_machine.c:
https://review.coreboot.org/c/coreboot/+/34589/4/src/drivers/amd/agesa/state... PS4, Line 297: int amd_get_ioapic_num(void) { open brace '{' following function definitions go on the next line
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
Patch Set 4: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/34589/4/src/drivers/amd/agesa/state... File src/drivers/amd/agesa/state_machine.c:
https://review.coreboot.org/c/coreboot/+/34589/4/src/drivers/amd/agesa/state... PS4, Line 297: int amd_get_ioapic_num(void) {
open brace '{' following function definitions go on the next line
This should be addressed.
https://review.coreboot.org/c/coreboot/+/34589/4/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family15tn/northbridge.c:
https://review.coreboot.org/c/coreboot/+/34589/4/src/northbridge/amd/agesa/f... PS4, Line 873: u8 plat_num_io_apics = amd_get_ioapic_num(); Nit: maybe this can be made const
Hello Kyösti Mälkki, Angel Pons, Paul Menzel, build bot (Jenkins), Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34589
to look at the new patch set (#5).
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
AGESA: Fix discovery of IO APICs number
Number of IO APICs are hardcoded. This fix reads this number from the default values obtained by creating a structure for a parameter block of an AGESA.
While it is possible to modify NumberOfIoApics in callouts, every platform currently in the tree goes with the default value.
Signed-off-by: Krystian Hebel krystian.hebel@3mdeb.com Change-Id: Ib3ddfe606720143659e57fbbca7f7a3e655a7664 --- M src/drivers/amd/agesa/state_machine.c M src/northbridge/amd/agesa/agesa_helper.h M src/northbridge/amd/agesa/family15tn/northbridge.c M src/northbridge/amd/agesa/family16kb/northbridge.c 4 files changed, 22 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/89/34589/5
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
Patch Set 5: -Code-Review
(2 comments)
oops, the commit message reverted to the patchset 2 state (while it was much better in ps #3)
https://review.coreboot.org/c/coreboot/+/34589/4/src/drivers/amd/agesa/state... File src/drivers/amd/agesa/state_machine.c:
https://review.coreboot.org/c/coreboot/+/34589/4/src/drivers/amd/agesa/state... PS4, Line 297: int amd_get_ioapic_num(void) {
This should be addressed.
Done
https://review.coreboot.org/c/coreboot/+/34589/4/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family15tn/northbridge.c:
https://review.coreboot.org/c/coreboot/+/34589/4/src/northbridge/amd/agesa/f... PS4, Line 873: u8 plat_num_io_apics = amd_get_ioapic_num();
Nit: maybe this can be made const
Ack, but it was a non-const constant before, so... ;-)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
Patch Set 5: Code-Review-2
(1 comment)
Not urgent for POSTCAR_STAGE=y fix, postponed.
https://review.coreboot.org/c/coreboot/+/34589/5/src/drivers/amd/agesa/state... File src/drivers/amd/agesa/state_machine.c:
https://review.coreboot.org/c/coreboot/+/34589/5/src/drivers/amd/agesa/state... PS5, Line 305: amd_create_struct(&aip, AMD_INIT_EARLY, NULL, 0); We don't link AMD_INIT_EARLY to ramstage at all in vendorcode. vendorcode/amd/agesa/common/agesa-entry-cfg.h
It is there in the PI blob though, but we will not open up the can of worms of having multiple callsites with AMD_CREATE_STRUCT and same *params.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
Patch Set 5:
Krystian, could you abandon this change please? We decided to go for Kconfig option in CB:37169
Krystian Hebel has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34589 )
Change subject: AGESA: Fix discovery of IO APICs number ......................................................................
Abandoned
We decided to go for Kconfig option in CB:37169