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 13:
(16 comments)
https://review.coreboot.org/c/coreboot/+/37115/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37115/11//COMMIT_MSG@10 PS11, Line 10: at BUP Phase
… at Bring UP (BUP) phase.
Done
https://review.coreboot.org/c/coreboot/+/37115/11//COMMIT_MSG@43 PS11, Line 43: disabled: apparently
I’d use: […]
Done
https://review.coreboot.org/c/coreboot/+/37115/11//COMMIT_MSG@39 PS11, Line 39: and reboots. ME is supposed to be disabled on the next boot after DID : (DRAM Init Done). : : My numerous tests show that issuing the command and rebooting is not : enough. If we reboot too early, ME will not be disabled: apparently, : it is doing something in background after receiving the command. It : works with a delay of 500-1000 ms. : : I also tried to dump all known (documented) registers, such as GMES and : HFS, before and during the next 2 seconds after execution of the : disable command to find a possible indication that something's changed : in ME and we're ready to reboot. Found nothing unfortunately.
Indent with four spaces, so it’s clear that this belongs to item 1. […]
Done
https://review.coreboot.org/c/coreboot/+/37115/11//COMMIT_MSG@54 PS11, Line 54: PT slides don't contain any more information on it, but my tests show, : that after writing this value, GMES[31:28] is changing from 0x01 (BUP : Phase) to 0x03 (Policy Module) to 0x06 (Host Communication). Then, : after some more time, fw_init_complete bit of HFS becomes 1. : : This means that ME starts loading its kernel immediately, without : reboot. : : On the other hand, Lenovo BIOS clearly perform a reboot after enabling : it (one reboot after saving the settings, then ThinkPad logo appears, : and then one more reboot). I'm assuming we have to reset too.
Please indent too.
Done
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/me.h:
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... PS11, Line 193: #define CMOS_ME_STATE_DISABLED 1
Use tabs for alignment as above.
Done
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... PS11, Line 250: int
Make that boolean?
Done
https://review.coreboot.org/c/coreboot/+/37115/5/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/me_8.x.c:
https://review.coreboot.org/c/coreboot/+/37115/5/src/southbridge/intel/bd82x... PS5, Line 521: reset
Removed.
Done
https://review.coreboot.org/c/coreboot/+/37115/5/src/southbridge/intel/bd82x... PS5, Line 565: pci_write_config32(dev, PCI_ME_H_GS, 0x20000000);
Added a comment.
It's done I guess.
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/me_8.x.c:
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... PS11, Line 533: .client_address = MEI_ADDRESS_MKHI,
Please use tabs for alignment.
Done
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... PS11, Line 539: BIOS_DEBUG
Please make it at least a warning or even error?
Done
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... PS11, Line 573: */
Use oneline comment?
Done
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... PS11, Line 637: printk(BIOS_CRIT, "%s: mbp is not ready!\n", __func__);
Unrelated.
Done
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... PS11, Line 758: BIOS_SPEW
Level *info* or *debug* would be more useful I guess.
Done
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... PS11, Line 799: printk(BIOS_ERR, "ME: failed to enter Soft Temporary Disable mode\n");
{} around this branch.
Done
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... File src/southbridge/intel/bd82x6x/me_common.c:
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... PS11, Line 13: */
Please use SPDX license header.
Done
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... PS11, Line 28: #endif
Why is the distinction needed? Is it run in romstage and ramstage?
Yes.