Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47828 )
Change subject: soc/amd: move non-CAR linker scripts to common directory ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/47828/2/src/soc/amd/common/block/cp... File src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld:
https://review.coreboot.org/c/coreboot/+/47828/2/src/soc/amd/common/block/cp... PS2, Line 32: * | |X86_RESET_VECTOR = ROMSTAGE_ADDR + ROMSTAGE_SIZE - 0x10
Is this correct? […]
the reset vector isn't in romstage, but in bootblock, so this is wrong. not sure if that comment is still from the hybrid romstage idea that got discarded in the end to fit better into the coreboot model. will look into that and fix it
https://review.coreboot.org/c/coreboot/+/47828/2/src/soc/amd/common/block/cp... PS2, Line 85: _ = ASSERT((CONFIG_BOOTBLOCK_ADDR + CONFIG_C_ENV_BOOTBLOCK_SIZE - 0x10) == CONFIG_X86_RESET_VECTOR, "Reset vector should be -0x10 from end of bootblock");
The assertion here reveals redundancy in the use of Kconfigs. […]
i agree that that's something i should look into
https://review.coreboot.org/c/coreboot/+/47828/2/src/soc/amd/common/block/cp... PS2, Line 86: _ = ASSERT(CONFIG_BOOTBLOCK_ADDR == ((CONFIG_BOOTBLOCK_ADDR + 0xFFFF) & 0xFFFF0000), "Bootblock must be 16 bit aligned");
Is this correct? […]
not 100% certain on this and couldn't find a definite answer at the places i thought it was most likely to find some info on that. might be worth to verify if there's a requirement on the base address/granularity, but right now it's not a priority for me, since using stricter alignment/granularity requirements than necessary won't really hurt