Attention is currently required from: Angel Pons, Evgeny Zinoviev. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37115 )
Change subject: sb/intel/bd82x6x: Support ME Soft Temporary Disable Mode ......................................................................
Patch Set 25: Code-Review+1
(7 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/37115/comment/000c6421_64566c09 PS25, Line 47: works with a delay of 500-1000 ms. It's probably because we always do the "blunt" global reset via ETR3 + full_reset(). This is only a last resort method and we should try a global-reset MEI message first.
Probably worth to look into after this is is merged.
Patchset:
PS25: This looks very good and I hope you can spare just a little more patience. The only thing that I see that should be addressed now is the stale comments in the `cmos.layout` files.
I had some trouble understand CMOS_ME_CHANGED. It's just a bit to tell us that we wrote `me_state_prev`... No need to change that now, but here's a half-baked idea from the top of my head: Make a single, tri-state CMOS option: (Keep, Normal, Disable). coreboot would only have to reset it to `Keep` everytime it processed one of the other states. This would also allow to keep the ME disabled if CMOS fails later.
File src/mainboard/lenovo/l520/cmos.layout:
https://review.coreboot.org/c/coreboot/+/37115/comment/82c167a4_e0c94382 PS25, Line 37: # coreboot config options: cpu Sorry, this looks just wrong. Maybe update the comments to say `me` instead?
File src/southbridge/intel/bd82x6x/me.h:
https://review.coreboot.org/c/coreboot/+/37115/comment/dd66740e_7250ce56 PS25, Line 178: state Nit, macro parameters should always be guarded with additional parentheses.
https://review.coreboot.org/c/coreboot/+/37115/comment/a877dcdb_2772a896 PS25, Line 188: u32 As I understand the manual, this could also be `u16`. Or more accurately 2x `u8`.
File src/southbridge/intel/bd82x6x/me.c:
https://review.coreboot.org/c/coreboot/+/37115/comment/4a8d154e_ca34f92e PS25, Line 342: * Nit, remove dangling asterisk to comply with coding style.
File src/southbridge/intel/bd82x6x/me_common.c:
https://review.coreboot.org/c/coreboot/+/37115/comment/64bbc35f_b57781e6 PS25, Line 499: #ifdef __SIMPLE_DEVICE__ This would be much easier in a file that simply defines __SIMPLE_DEVICE__. Mixing both in one compilation unit was never the idea.