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 6: Code-Review+1
(16 comments)
Patchset:
PS6:
I added documentation and a way to configure the fan header for CPUFANIN (see comment below). […]
I think I suggested this change because Windows hates NCT6776 ACPI
PS6: Just some minor things here and there, and this will be ready to go!
File Documentation/mainboard/asrock/h77pro4-m.md:
https://review.coreboot.org/c/coreboot/+/45317/comment/96452d1d_e30e733b PS6, Line 24: header I'd mention it's RS-232 (and not TTL UART)
https://review.coreboot.org/c/coreboot/+/45317/comment/584b2e3e_cb201570 PS6, Line 45: SATA at 6 Gb/s Doesn't this repeat part of what the two preceding lines say? I'd remove it
https://review.coreboot.org/c/coreboot/+/45317/comment/9d0df447_347f9d96 PS6, Line 147: work, check if your bracket expects a different assignment. I'm worried about people connecting things that doesn't use RS-232 voltages to this header, and frying them. But I wouldn't expect this kind of people to read warnings anyway.
File src/mainboard/asrock/h77pro4-m/Kconfig:
https://review.coreboot.org/c/coreboot/+/45317/comment/70244ac9_b5992129 PS6, Line 22: string Type no longer needed, see CB:56553
https://review.coreboot.org/c/coreboot/+/45317/comment/c7537d84_b0edc725 PS6, Line 26: string Type no longer needed, see CB:56554
File src/mainboard/asrock/h77pro4-m/cmos.layout:
https://review.coreboot.org/c/coreboot/+/45317/comment/470a9438_54d2a0cc PS6, Line 28: cpu_fan_header Since this option is about the tachometer signal source, how about naming it `cpu_fan_tach_src` instead?
File src/mainboard/asrock/h77pro4-m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/45317/comment/3fe68f8f_41651723 PS6, Line 75: # parallel port Comment seems redundant
File src/mainboard/asrock/h77pro4-m/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/45317/comment/0922916b_7baa141d PS6, Line 10: /* OEM revision */ This comment comes from autoport, right? I'd remove it.
File src/mainboard/asrock/h77pro4-m/early_init.c:
https://review.coreboot.org/c/coreboot/+/45317/comment/10acf7cb_3943f93c PS4, Line 54: /* Configure GPIO7 */ : pnp_set_logical_device(GPIO6789_DEV); : pnp_write_config(GPIO6789_DEV, 0xe0, 0x3f); : pnp_write_config(GPIO6789_DEV, 0xe1, 0x7f);
I copied the CMOS option tables from mb/asus/p8z77-v_lx2 and looked at mb/kontron/986lcd-m/mainboard […]
Sounds great!
File src/mainboard/asrock/h77pro4-m/early_init.c:
https://review.coreboot.org/c/coreboot/+/45317/comment/47e8df54_d4d28827 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> nit: maybe order includes alphabetically?
https://review.coreboot.org/c/coreboot/+/45317/comment/cad8d80b_265027e1 PS6, Line 18: #define CPU_FAN_HEADER_NONE 0 : #define CPU_FAN_HEADER_1 1 : #define CPU_FAN_HEADER_2 2 : #define CPU_FAN_HEADER_BOTH 3 nit: Use an enum?
https://review.coreboot.org/c/coreboot/+/45317/comment/9bb02e9b_90208f77 PS6, Line 43: u8 #include <stdint.h>
https://review.coreboot.org/c/coreboot/+/45317/comment/361c9386_e1bcaa79 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; : } Idea: place the switch/case statement in a helper function, so that you can replace the assignments and break statements with one return statement:
static u8 get_cpu_fan_header(void) { switch (get_uint_option("cpu_fan_header", CPU_FAN_HEADER_1)) { case CPU_FAN_HEADER_NONE: return 0xff; case CPU_FAN_HEADER_1: default: return 0x7f; case CPU_FAN_HEADER_2: return 0xbf; case CPU_FAN_HEADER_BOTH: return 0x3f; } }
Note that I added a `default:` case. This is to handle the unusual case where `get_uint_option()` returns something other than the defined values (which could only happen if something goes very wrong).
Feel free to pick a different function name. I'm not very inspired today 😄
File src/mainboard/asrock/h77pro4-m/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/45317/comment/2eebf7dd_593a63c2 PS6, Line 32: nit: drop blank line?