Attention is currently required from: Angel Pons, Jonathon Hall, Felix Held.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74363 )
Change subject: mb/purism/librem_cnl: Provide CBFS setting for Mini auto power on ......................................................................
Patch Set 2:
(5 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/74363/comment/42663ddb_7b560f69 PS2, Line 9: Control Mini v1/v2 automatic power on by adding a 'board_settings' file : to CBFS. This allows us to use one build each for Mini v1/v2 that is : configured differently for different uses. By default, the EC setting is : not configured by coreboot, and the OS could configure it. : : Add ITE SuperIO configuration to device tree and configure base address : of BRAM1. (BRAM1 contains the EC firmware setting for automatic power : on.) : : Test: Build Mini v2, boot with no settings in CBFS. Add board_settings : configured for automatic power-on, flash, boot, confirm EC now powers : on automatically when power applied. Please reflow for 72 characters per line.
Patchset:
PS2: Can the hunk hooking up the Super I/O be separate? (Is there a change in behavior regarding fan control for example?)
Isn’t that auto power on setting already run-time configured using CMOS, which can be easily controlled with `nvramtool` from the OS (instead of reflashing a system) and the cmos default values, which are in CBFS?
File src/mainboard/purism/librem_cnl/variants/librem_mini/mainboard.c:
https://review.coreboot.org/c/coreboot/+/74363/comment/f9eefe3d_8aff9306 PS2, Line 7: /* Create a cbfs file called 'board_settings' to configure settings: : * Byte 0: EC automatic power on (EC_POWER_ON_*) : */ Please use the recommended multi-line comment styles from the coding style.
https://review.coreboot.org/c/coreboot/+/74363/comment/26ee5170_81c69fc5 PS2, Line 21: uint8_t ec_power_on = 0; Any reason to limit the size? Why not `unsigned int` or `bool`?
https://notabs.org/coding/smallIntsBigPenalty.htm
https://review.coreboot.org/c/coreboot/+/74363/comment/315494b2_497719a0 PS2, Line 24: /* The board settings size can be larger for forward compatibility with : * future settings, which are ignored */ Please use the recommended multi-line comment styles from the coding style.