Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36914 )
Change subject: binaryPI: implement C bootblock ......................................................................
Patch Set 26:
(10 comments)
https://review.coreboot.org/c/coreboot/+/36914/3/src/cpu/amd/agesa/Kconfig File src/cpu/amd/agesa/Kconfig:
https://review.coreboot.org/c/coreboot/+/36914/3/src/cpu/amd/agesa/Kconfig@3... PS3, Line 33: select C_ENVIRONMENT_BOOTBLOCK
Understood
All the binary PI boards have ROMCC_BOOTBLOCK selected or are disabled from build. So if the platform reaches C bootblock, it should have already the superio initialized properly.
https://review.coreboot.org/c/coreboot/+/36914/20/src/cpu/amd/pi/Kconfig File src/cpu/amd/pi/Kconfig:
https://review.coreboot.org/c/coreboot/+/36914/20/src/cpu/amd/pi/Kconfig@55 PS20, Line 55: config PI_AGESA_CAR_HEAP_BASE
These will need some argumentation in the commit message.
After enabling SSE2 setting writeback MTRR for AGESA heap seems not to be necessary for C bootblock. Checked on both apu1 and apu2 C bootblock
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/boot... File src/drivers/amd/agesa/bootblock.c:
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/boot... PS14, Line 42: CONFIG_PI_AGESA_CAR_HEAP_BASE, : CONFIG_PI_AGESA_HEAP_SIZE
Stoneyridge does not guard it either. […]
The config options occurred to be not unnecessary
https://review.coreboot.org/c/coreboot/+/36914/17/src/drivers/amd/agesa/boot... File src/drivers/amd/agesa/bootblock.c:
https://review.coreboot.org/c/coreboot/+/36914/17/src/drivers/amd/agesa/boot... PS17, Line 25: /* TSC cannot be relied upon. Override the TSC value passed in. */
Not sure if this comment is true. At least for fam14 TSC rescales during raminit. […]
Done
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/cach... File src/drivers/amd/agesa/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/cach... PS14, Line 40:
remove change?
Done
https://review.coreboot.org/c/coreboot/+/36914/17/src/drivers/amd/agesa/cach... File src/drivers/amd/agesa/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36914/17/src/drivers/amd/agesa/cach... PS17, Line 39: /* Turn on OSFXSR [BIT9] and OSXMMEXCPT [BIT10] onto CR4 register */
Needless whitespace change,
Done
https://review.coreboot.org/c/coreboot/+/36914/18/src/drivers/amd/agesa/cach... File src/drivers/amd/agesa/cache_as_ram.S:
https://review.coreboot.org/c/coreboot/+/36914/18/src/drivers/amd/agesa/cach... PS18, Line 40:
related?
Done
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/roms... File src/drivers/amd/agesa/romstage.c:
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/roms... PS14, Line 57: #if !CONFIG(ROMCC_BOOTBLOCK) : set_ap_entry_ptr(agesa_call); : #endif
You can probably use C code instead of CPP. […]
Done
https://review.coreboot.org/c/coreboot/+/36914/14/src/drivers/amd/agesa/roms... PS14, Line 62: : #if CONFIG(ROMCC_BOOTBLOCK) : void *asmlinkage romstage_main(unsigned long bist) : #else : asmlinkage void car_stage_entry(void) : #endif
you can avoid the ifdefs if car_stage_entry just calls romstage_main.
Done
https://review.coreboot.org/c/coreboot/+/36914/18/src/northbridge/amd/pi/boo... File src/northbridge/amd/pi/bootblock.c:
https://review.coreboot.org/c/coreboot/+/36914/18/src/northbridge/amd/pi/boo... PS18, Line 32:
Since all the boards are disabled, I may remove the amd_initmmio present in fixme.c files elsewhere. […]
Only ROM caching is set via MTRRs. Removed the amd_initmmio from bootblock to not confuse things.