Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42064 )
Change subject: soc/amd/picasso: Split I2c implementation for psp_verstage ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42064/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42064/5//COMMIT_MSG@7 PS5, Line 7: soc/amd/picasso: Split I2c implementation for psp_verstage i2c or I2C. First is more readable to my eyes.
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-psp... File src/soc/amd/picasso/i2c-psp.c:
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c-psp... PS5, Line 7: #include <soc/iomap.h> I know I2C_MASTER_DEV_COUNT is there, but maybe it would be better moved to <soc/i2c.h>. This <soc/iomap.h> has lots of _BASE defines that are not valid in PSP context.
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c.c File src/soc/amd/picasso/i2c.c:
https://review.coreboot.org/c/coreboot/+/42064/5/src/soc/amd/picasso/i2c.c@1... PS5, Line 169: cfg = config_of_soc(); Unrelated. This currently brings in the entire static devicetree / static.c file into PSP verstage, possibly doubling the binary size. There are some initiatives and interest to avoid this, but nothing immediately available.
As long as you don't lock anything early, it does not matter that much if read-only (?) verstage carries different configuration vs later stages.