Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37490 )
Change subject: soc/amd/picasso: Add bootblock support ......................................................................
Patch Set 11:
(12 comments)
https://review.coreboot.org/c/coreboot/+/37490/7/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/37490/7/src/soc/amd/picasso/Kconfig... PS7, Line 225: 0x807fff0
I'm wondering if this needs to be kept in sync with hardware or software (e.g. PSP firmware).
No, you can basically select any location in DRAM. Information about this is passed into BIOS directory table.
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"
Bootblock parameters are not PSP configuraion options.
Felix - this is not yet addressed.
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?
Felix - can you please raise a bug to capture the points raised by Kyosti and Marshall here? And update commit message to indicate that this will be followed up with updates referencing the bug?
"The benefit to using C_ENV_BOOTBLOCK_SIZE and X86_RESET_VECTOR+0x10-n is to have those base/size values later when I need to reserve that RAM."
I don't think that is correct or required. I believe this is primarily being done for S3. Let's take that up separately as part of S3 implementation.
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
There is also SCI configuration in there which makes a bit more sense, however […]
Felix - did you check if all these files are required?
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
I've raised questions some questions in patchset #4 and the solution seems to be to mention possible […]
Felix - can you please address this?
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 34: sb_reset_i2c_slaves();
Ok, these pins are indeed very I2C specific. Even in GPIO mode, they […]
Done
https://review.coreboot.org/c/coreboot/+/37490/7/src/soc/amd/picasso/bootblo... PS7, Line 41: Family_Model
This was the same format that was followed on stoney: https://source.chromium. […]
Let's keep this as Family Model for now for consistency. If there is a change required, it can be done as follow-up.
https://review.coreboot.org/c/coreboot/+/37490/7/src/soc/amd/picasso/bootblo... PS7, Line 44: i2c_soc_early_init();
Well, SoC includes the FCH... so that's technically not wrong.
This seems consistent with what stoneyridge did. If there is clean up required, let's do that as follow-up.
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
Maybe just arch/cpu. […]
Not yet addressed?
https://review.coreboot.org/c/coreboot/+/37490/9/src/soc/amd/picasso/bootblo... PS9, Line 8: #include <cpu/x86/mtrr.h>
Is this one used?
Doesn't look like this is used.
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.
https://review.coreboot.org/c/coreboot/+/37490/11/src/soc/amd/picasso/bootbl... PS11, Line 9: #include <timestamp.h> Not required.