Attention is currently required from: Angel Pons.
15 comments:
File Documentation/mainboard/asrock/h77pro4-m.md:
Patch Set #6, Line 24: header
I'd mention it's RS-232 (and not TTL UART)
Done, good point
Patch Set #6, 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..
Patch Set #6, 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:
Patch Set #6, Line 22: string
Type no longer needed, see CB:56553
Done
Patch Set #6, Line 26: string
Type no longer needed, see CB:56554
Done
File src/mainboard/asrock/h77pro4-m/cmos.layout:
Patch Set #6, 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:
Patch Set #6, Line 75: # parallel port
Comment seems redundant
Done
File src/mainboard/asrock/h77pro4-m/dsdt.asl:
Patch Set #6, 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:
#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..
#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.
#include <stdint. […]
Done
Patch Set #6, 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.
// 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?
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:
nit: drop blank line?
Done
To view, visit change 45317. To unsubscribe, or for help writing mail filters, visit settings.