Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41224 )
Change subject: mb/asus/.../p3b-f: Enable flashrom in ramstage final ......................................................................
Patch Set 1:
(8 comments)
https://review.coreboot.org/c/coreboot/+/41224/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41224/1//COMMIT_MSG@9 PS1, Line 9: as
As
Ack
https://review.coreboot.org/c/coreboot/+/41224/1//COMMIT_MSG@10 PS1, Line 10: trick no longer works after introducing ACPI support, add a mainboard
Why doesn’t it work anymore with ACPI support?
I still haven't figured it out. With ACPI patch compiled in, the i2cset command simply says write failed. I have to disable ACPI in kernel to succeed.
https://review.coreboot.org/c/coreboot/+/41224/1/src/mainboard/asus/p2b/vari... File src/mainboard/asus/p2b/variants/p3b-f/mainboard.c:
https://review.coreboot.org/c/coreboot/+/41224/1/src/mainboard/asus/p2b/vari... PS1, Line 3:
the "is part of" line has been dropped.
Ack
https://review.coreboot.org/c/coreboot/+/41224/1/src/mainboard/asus/p2b/vari... PS1, Line 22: if (r < 0)
Is it needed to split the strings like that? I would use full statements instead, so that the error […]
I intentionally break up the message like this to conserve flash space, rather than duplicating the bulk of it.
In the flash the message is saved as "Flashrom enable| faile|d", here with terminal zeros replaced by '|' for visualization. The middle part is shown only on failure. On success the console shows "Flashrom enabled", which doubles as a breadcrumb.
My preferred alternative would be to only print a BIOS_WARN message if flashrom enable is attempted and failed, and stay silent otherwise. At least decide the message level during runtime based on result of the attempt. With a workaround available, failing here is not a big deal.
https://review.coreboot.org/c/coreboot/+/41224/1/src/mainboard/asus/p2b/vari... PS1, Line 23: printk(BIOS_INFO," faile");
The failed message should be error or even warning level.
See above
https://review.coreboot.org/c/coreboot/+/41224/1/src/mainboard/asus/p2b/vari... PS1, Line 23: printk(BIOS_INFO," faile");
space required after that ',' (ctx:VxV)
Ack
https://review.coreboot.org/c/coreboot/+/41224/1/src/mainboard/asus/p2b/vari... PS1, Line 23: faile
failed
Ditto
https://review.coreboot.org/c/coreboot/+/41224/1/src/mainboard/asus/p2b/vari... PS1, Line 24: printk(BIOS_INFO,"d\n");
space required after that ',' (ctx:VxV)
Ack