Attention is currently required from: Nico Huber, Angel Pons. Evgeny Zinoviev 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 26:
(9 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/37115/comment/ecc462c6_bcaab7b3 PS25, Line 47: works with a delay of 500-1000 ms.
It's probably because we always do the "blunt" global reset via […]
IIRC I tried to send a global-reset message and it didn't work, ME just stops responding after receiving the disable message. Not 100% sure though as it was in 2019, maybe 90% sure.
Patchset:
PS25:
This looks very good and I hope you can spare just a little […]
About CMOS_ME_CHANGED. We discussed it on the channel some time ago (me and Angel, I think, and maybe siro). And decided that ME should come out of disabled state on CMOS reset, in case something goes wrong. That's the purpose of that bit, to see if we need to reset ME back to normal.
File src/mainboard/lenovo/l520/cmos.layout:
https://review.coreboot.org/c/coreboot/+/37115/comment/c04fd83c_cc8cfac8 PS25, Line 37: # coreboot config options: cpu
Sorry, this looks just wrong. Maybe update the comments to say […]
Done
https://review.coreboot.org/c/coreboot/+/37115/comment/950e722e_0676932f PS25, Line 40: #427 5 r 0 unused
Please drop comments for unused space. […]
Done
File src/southbridge/intel/bd82x6x/me.h:
https://review.coreboot.org/c/coreboot/+/37115/comment/be68e7cd_bdc70ba7 PS25, Line 178: state
Nit, macro parameters should always be guarded with additional parentheses.
Done.
File src/southbridge/intel/bd82x6x/me.c:
https://review.coreboot.org/c/coreboot/+/37115/comment/2262ea3e_b0023643 PS25, Line 282: : /* Put ME in Software Temporary Disable Mode, if needed */ : if (me_state == CMOS_ME_STATE_DISABLED : && CMOS_ME_STATE(me_state_prev) == CMOS_ME_STATE_NORMAL) { : printk(BIOS_INFO, "ME: disabling ME\n"); : if (enter_soft_temp_disable()) { : enter_soft_temp_disable_wait(); : need_reset = true; : } else { : printk(BIOS_ERR, "ME: failed to enter Soft Temporary Disable mode\n"); : } : : break; : }
Would it make sense to print the capabilities before switching to soft temporary disable mode?
It doesn't hurt, I guess.
https://review.coreboot.org/c/coreboot/+/37115/comment/d871c75b_2d84e311 PS25, Line 322: /* : * ME starts loading firmware immediately after writing to H_GS, : * but Lenovo BIOS performs a reboot after bringing ME back to : * Normal mode. Assume that global reset is needed. : */
On later ME firmware versions, there's a SET ME ENABLE RESPONSE message that should be read from the […]
I'll try tonight.
https://review.coreboot.org/c/coreboot/+/37115/comment/c4732158_7b7f9f98 PS25, Line 342: *
Nit, remove dangling asterisk to comply with coding style.
Done.
File src/southbridge/intel/bd82x6x/me_common.c:
https://review.coreboot.org/c/coreboot/+/37115/comment/4d0e4f3e_e7393ff8 PS25, Line 467: 0x20000000
0x2 << 28
Done