Attention is currently required from: Michael Büchler. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45317 )
Change subject: mb/asrock: Add ASRock H77 Pro4-M mainboard ......................................................................
Patch Set 7: Code-Review+1
(5 comments)
File Documentation/mainboard/asrock/h77pro4-m.md:
https://review.coreboot.org/c/coreboot/+/45317/comment/0e6c1d14_0dd48e9e PS6, Line 147: work, check if your bracket expects a different assignment.
We should still try to get to these people.. […]
Perfect!
File src/mainboard/asrock/h77pro4-m/early_init.c:
https://review.coreboot.org/c/coreboot/+/45317/comment/53206601_25b27613 PS6, Line 3: #include <bootblock_common.h> : #include <device/pci_ops.h> : #include <device/pnp_ops.h> : #include <northbridge/intel/sandybridge/raminit_native.h> : #include <southbridge/intel/bd82x6x/pch.h> : #include <superio/nuvoton/nct6776/nct6776.h> : #include <superio/nuvoton/common/nuvoton.h> : #include <option.h>
The lack of order on these include lines often bothers me but I never thought about a good general r […]
I sort by full path. Anyway, this is just cosmetics
https://review.coreboot.org/c/coreboot/+/45317/comment/3f01e3ef_ce58c099 PS6, Line 59: // TODO check if it's okay to have this in devicetree.cb : /* Power RAM in S3 */ : pnp_set_logical_device(ACPI_DEV); : pnp_wr
I'm putting a comment here to make sure that I test suspend-to-RAM without this. […]
I'm pretty sure it's needed. Setting it in devicetree.cb should work, especially considering it's where LDN 0xa (ACPI) offset 0xf0 is programmed accordingly: https://imgur.com/Dn7kHij.png
https://review.coreboot.org/c/coreboot/+/45317/comment/ce319288_bade10db PS6, Line 73: cpu_fan_header = get_uint_option("cpu_fan_header", CPU_FAN_HEADER_1); : switch (cpu_fan_header) { : case CPU_FAN_HEADER_NONE: : reg = 0xff; : break; : case CPU_FAN_HEADER_1: : reg = 0x7f; : break; : case CPU_FAN_HEADER_2: : reg = 0xbf; : break; : case CPU_FAN_HEADER_BOTH: : reg = 0x3f; : break; : }
Done. […]
Good function name, I like it! I'll let you mark this one as resolved, I'm not sure if you've tested this yet.
File src/mainboard/asrock/h77pro4-m/early_init.c:
https://review.coreboot.org/c/coreboot/+/45317/comment/0128d645_8f19574c PS7, Line 44: /* The tachometer signal that goes to CPUFANIN of the Super I/O is set via : * GPIOs. : * When GP77 (register E1h[7]) is '0', CPU_FAN1 is connected. : * When GP76 (register E1h[6]) is '0', CPU_FAN2 is connected. : * When both are '0' and both fans are connected, wrong readings will : * be reported. */ Please adapt this comment's style: https://doc.coreboot.org/contributing/coding_style.html#commenting