Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37490 )
Change subject: soc/amd/picasso: Add bootblock support ......................................................................
Patch Set 12:
(15 comments)
PTAL
https://review.coreboot.org/c/coreboot/+/37490/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37490/11//COMMIT_MSG@16 PS11, Line 16: Remove postcar support : completely from Makefile.inc.
This is not really true for the change anymore.
Done
https://review.coreboot.org/c/coreboot/+/37490/11//COMMIT_MSG@21 PS11, Line 21:
Can you please add appropriate BUG= to commit message?
Done
https://review.coreboot.org/c/coreboot/+/37490/9/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/37490/9/src/soc/amd/picasso/Kconfig... PS9, Line 221: menu "PSP Configuration Options"
Felix - this is not yet addressed.
Done
https://review.coreboot.org/c/coreboot/+/37490/4/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37490/4/src/soc/amd/picasso/Makefil... PS4, Line 213: PSP_BIOSBIN_DEST=$(shell printf "%x" $(call int-subtract, $(call int-add, $(CONFIG_X86_RESET_VECTOR) 0x10) $(PSP_BIOSBIN_SIZE)))
not sure what to do about this right now. is it ok when i close this issue for now? […]
Filed b/154957411 and added a comment.
https://review.coreboot.org/c/coreboot/+/37490/7/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37490/7/src/soc/amd/picasso/Makefil... PS7, Line 44: bootblock-y += smi_util.c
Felix - did you check if all these files are required?
Yes, it's required as part of gpio_banks/gpio.c: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/th...
https://review.coreboot.org/c/coreboot/+/37490/9/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37490/9/src/soc/amd/picasso/Makefil... PS9, Line 207: 0x10
Felix - can you please address this?
Done
https://review.coreboot.org/c/coreboot/+/37490/11/src/soc/amd/picasso/Makefi... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37490/11/src/soc/amd/picasso/Makefi... PS11, Line 208: bootblock.elf
Don't you really want to just use bootblock. […]
Let's leave the elf so we can extract the size later. b/154957411
https://review.coreboot.org/c/coreboot/+/37490/11/src/soc/amd/picasso/Makefi... PS11, Line 208: $(obj)/cbfs/fallback
$(objcbfs)
Done
https://review.coreboot.org/c/coreboot/+/37490/11/src/soc/amd/picasso/Makefi... PS11, Line 209: $(CONFIG_C_ENV_BOOTBLOCK_SIZE)
I believe you can just use file-size bootblock. […]
Yeah, but the file doesn't exist at the point this runs. Let's wait for b/154957411 to refactor this code.
https://review.coreboot.org/c/coreboot/+/37490/7/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/37490/7/src/soc/amd/picasso/bootblo... PS7, Line 41: Family_Model
Let's keep this as Family Model for now for consistency. […]
Ack
https://review.coreboot.org/c/coreboot/+/37490/7/src/soc/amd/picasso/bootblo... PS7, Line 44: i2c_soc_early_init();
This seems consistent with what stoneyridge did. […]
Ack
https://review.coreboot.org/c/coreboot/+/37490/9/src/soc/amd/picasso/bootblo... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/37490/9/src/soc/amd/picasso/bootblo... PS9, Line 7: cpu/amd/msr.h
Not yet addressed?
Removed for now
https://review.coreboot.org/c/coreboot/+/37490/9/src/soc/amd/picasso/bootblo... PS9, Line 8: #include <cpu/x86/mtrr.h>
Doesn't look like this is used.
Done
https://review.coreboot.org/c/coreboot/+/37490/11/src/soc/amd/picasso/bootbl... File src/soc/amd/picasso/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/37490/11/src/soc/amd/picasso/bootbl... PS11, Line 6: #include <console/console.h>
This is not required.
We use a printk
https://review.coreboot.org/c/coreboot/+/37490/11/src/soc/amd/picasso/bootbl... PS11, Line 9: #include <timestamp.h>
Not required.
Done