Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37490 )
Change subject: soc/amd/picasso: Add bootblock support ......................................................................
Patch Set 7:
(6 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 Is this architectural?
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 What's 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 14: static void amd_initmmio(void) If this is only about PCI config space, please name it accordingly.
Um, wait... isn't this the same as enable_pci_mmconf()?
https://review.coreboot.org/c/coreboot/+/37490/7/src/soc/amd/picasso/bootblo... PS7, Line 34: sb_reset_i2c_slaves(); How is this expected to make a difference for consoles? Why is this done before and in fch_pre_init()?
Also, I've peeked into it. It seems it might keep pins floating for a moment if they are actually used as GPO instead of I2C for a board. That is, if AMD allows such designs. Where is that documented?
https://review.coreboot.org/c/coreboot/+/37490/7/src/soc/amd/picasso/bootblo... PS7, Line 41: Family_Model In its encoded form, it's usually called cpuid signature. You can decode it with get_fms().
https://review.coreboot.org/c/coreboot/+/37490/7/src/soc/amd/picasso/bootblo... PS7, Line 44: i2c_soc_early_init(); I'm new to AMD. Is there any system behind these sb_, fch_, soc_ prefixes?