Paul Menzel 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 11:
(9 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.
https://review.coreboot.org/c/coreboot/+/37115/11//COMMIT_MSG@43 PS11, Line 43: disabled: apparently I’d use:
… disabled. Apparently, …
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.?
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.
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.
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?
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... PS11, Line 573: */ Use oneline comment?
https://review.coreboot.org/c/coreboot/+/37115/11/src/southbridge/intel/bd82... PS11, Line 574: for (i = 0; i < 400; i++) { : mdelay(50); : pci_read_dword_ptr(dev, &hfs, PCI_ME_HFS); : printk(BIOS_SPEW, "ME: hfs.fw_init_complete=%d after %dms\n", : hfs.fw_init_complete, (i+1)*50); : : if (hfs.fw_init_complete) : break; : } : : if (!hfs.fw_init_complete) : printk(BIOS_ERR, "ME: giving up on waiting for fw_init_complete\n"); Please use the stopwatch framework, and print the total time in the end.
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.