Attention is currently required from: Angel Pons. Michael Büchler 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:
(15 comments)
File Documentation/mainboard/asrock/h77pro4-m.md:
https://review.coreboot.org/c/coreboot/+/45317/comment/8936c0e1_065352fe PS6, Line 24: header
I'd mention it's RS-232 (and not TTL UART)
Done, good point
https://review.coreboot.org/c/coreboot/+/45317/comment/23b4096e_4f87858b PS6, Line 45: SATA at 6 Gb/s
Doesn't this repeat part of what the two preceding lines say? I'd remove it
It meant to reflect that I specifically tested it to work at 6 Gb/s speed, but I guess it's more redundant than informative..
https://review.coreboot.org/c/coreboot/+/45317/comment/4e280851_535b85dd 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 fryi […]
We should still try to get to these people.. in this case, putting myself into the mind of an adventurous person who doesn't know RS-232 but wants to find out, I think adding a bit of an explanation helps. I added a sentence that others will probably just read past.
File src/mainboard/asrock/h77pro4-m/Kconfig:
https://review.coreboot.org/c/coreboot/+/45317/comment/ea7f0e85_739d79b8 PS6, Line 22: string
Type no longer needed, see CB:56553
Done
https://review.coreboot.org/c/coreboot/+/45317/comment/c220e8b8_b0eb4269 PS6, Line 26: string
Type no longer needed, see CB:56554
Done
File src/mainboard/asrock/h77pro4-m/cmos.layout:
https://review.coreboot.org/c/coreboot/+/45317/comment/757013b1_f075fc7a PS6, Line 28: cpu_fan_header
Since this option is about the tachometer signal source, how about naming it `cpu_fan_tach_src` inst […]
Yes, it's more specific. The fan will still work even if the header is not set here, so `cpu_fan_header` might be misleading. Thanks.
File src/mainboard/asrock/h77pro4-m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/45317/comment/3fd14c87_e101c0e9 PS6, Line 75: # parallel port
Comment seems redundant
Done
File src/mainboard/asrock/h77pro4-m/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/45317/comment/2d8bc638_d507a02a PS6, Line 10: /* OEM revision */
This comment comes from autoport, right? I'd remove it.
Yes it does. Done.
File src/mainboard/asrock/h77pro4-m/early_init.c:
https://review.coreboot.org/c/coreboot/+/45317/comment/166a9f5d_682e4ad8 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?
The lack of order on these include lines often bothers me but I never thought about a good general rule. Quick research on the internet shows that some people have asked that question before and other people have opinions. There is a Google C++ Style Guide which has a short section on this topic. It also suggests an alphabetical order in this case.
But should one sort by the basename or the full path? Probably the full path..
https://review.coreboot.org/c/coreboot/+/45317/comment/2bfda1ed_ee1e7f0d 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?
Sure. I was pleased to see that using an enum instead of multiple defines doesn't change the final image with BUILD_TIMELESS=1.
https://review.coreboot.org/c/coreboot/+/45317/comment/cc1ff163_8de9b00d PS6, Line 43: u8
#include <stdint. […]
Done
https://review.coreboot.org/c/coreboot/+/45317/comment/88019f1c_76d816f5 PS6, Line 57: // FIXME 0x13,0x14 "Device IRQ Polarity Selection" are set by the vendor BIOS The vendor firmware sets these both to 0xff (default 0x00) but I realized that coreboot seems to do the same. I'll remove it.
https://review.coreboot.org/c/coreboot/+/45317/comment/6703b81f_39c95782 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.
This sets the "3VSBSW# enable bit". The vendor firmware doesn't set it. The board uses the active-high "3VSBSW" signal on pin 70 instead of "3VSBSW#" on pin 71 to select +5VSB or +5V on +5V_DUAL. Maybe that's why.
If it turns out to be necessary, should it be set in devicetree.cb instead?
https://review.coreboot.org/c/coreboot/+/45317/comment/e2820d0d_4f1899b8 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 […]
Done. I felt inspired enough to change the function name to `get_cpufanin_gpio_config` because it gets the value to be set on the GPIO register - might not be perfect.
Other than that I rarely write C and I really need this kind of suggestion, thanks :)
Oh also I can't test this change on hardware right away, maybe Sunday though..
File src/mainboard/asrock/h77pro4-m/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/45317/comment/23b82748_ad35151f PS6, Line 32:
nit: drop blank line?
Done