Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40421 )
Change subject: soc/amd/picasso: Add SPI speed devicetree settings and EM100 support ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/chip.h File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/chip.h@... PS3, Line 39: uint32_t spi_normal_speed; It would be helpful to add a comment here indicating what macros can be used for speed and read mode. In fact, I think we should change those to enum in southbridge.h? Then we can simply use the enum type here which will make it pretty clear.
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/southbr... File src/soc/amd/picasso/southbridge.c:
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/southbr... PS3, Line 248: && config != NULL This check is redundant. config_of_soc() takes care of ensuring config is not NULL.
https://review.coreboot.org/c/coreboot/+/40421/3/src/soc/amd/picasso/southbr... PS3, Line 288: sb_spi_init I think this function is executed too late. Looking at the current series of patches, my understanding is that picasso will be having a bootblock. So, these settings should be done in bootblock_soc_init() or its equivalent for picasso.