Attention is currently required from: Jason Glenesk, Raul Rangel, Matt DeVillier, Fred Reitberger, Elyes Haouas, Felix Held.
Martin L Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/72658 )
Change subject: soc/amd: Create AMD common reset code ......................................................................
Patch Set 1:
(3 comments)
File src/soc/amd/common/block/include/amdblocks/reset.h:
https://review.coreboot.org/c/coreboot/+/72658/comment/b0b60466_bb9b81b6 PS1, Line 10: #include <soc/southbridge.h> : : : void do_warm_reset(void);
maybe just one empty line?
Thanks, that was a mistake.
File src/soc/amd/common/block/reset/reset.c:
PS1:
since this lives in the acpimmio pm register block, i'd consider moving it to soc/amd/common/block/p […]
There's a lot of stuff that resides in various register blocks just because that's where they put it.
By this argument, should everything that goes under the LPC registers go into the LPC block? SPI for example? I think we can all agree that doing that doesn't make sense.
I'll move it if that's really the preference, but at that point, does the reset get its own config option, or does it use the SOC_AMD_COMMON_BLOCK_PM option? If it gets it's own option, i think that's another reason to keep it in a separate directory.
https://review.coreboot.org/c/coreboot/+/72658/comment/e4909a15_e68fa6bd PS1, Line 22: outb(RST_CPU | SYS_RST, RST_CNT);
i'd either put this into the corresponding else branch or add a return after the do_cold_reset call. […]
How about just adding a comment that do_cold_reset() doesn't return? I can't say I really like either of the above options.