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 4: Code-Review+1
(12 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/45317/comment/fe5cb925_0a7bffbf PS4, Line 30: (needs VGA BIOS) I know Windows 10 works on Sandy/Ivy without a VGA BIOS when:
- libgfxinit sets up a linear "high-resolution" framebuffer - payload is TianoCore - coreboot contains the VBT
https://review.coreboot.org/c/coreboot/+/45317/comment/1707ce1f_ff4971d1 PS4, Line 42: SATA 6Gb/s I imagine this is because none of your SATA drives are 6Gbps-capable?
https://review.coreboot.org/c/coreboot/+/45317/comment/7671353a_6e6d5128 PS4, Line 43: Rear eSATA connector It's multiplexed with the second SATA port of the ASM1061. Looks like the eSATA connector has a SATA_SEL pin, which should activate the multiplexor chip. The SATA_SEL pin is also wired to Super I/O GP23 (which is probably configured as an input and only used so that vendor firmware can read the level of SATA_SEL).
File src/mainboard/asrock/h77pro4-m/Kconfig:
https://review.coreboot.org/c/coreboot/+/45317/comment/14e9e653_705e7a06 PS4, Line 26: config INTEL_GMA_VBT_FILE : string : default "src/mainboard/$(MAINBOARDDIR)/data.vbt" : : config INTEL_GMA_ADD_VBT : bool : default y Remove these and simply select `INTEL_GMA_HAVE_VBT`
File src/mainboard/asrock/h77pro4-m/acpi_tables.c:
PS4: This file can be removed. The GNVS values aren't used.
File src/mainboard/asrock/h77pro4-m/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/45317/comment/d28d89b8_ee516815 PS4, Line 4: 0x0 nit: Use `0` instead of `0x0`
https://review.coreboot.org/c/coreboot/+/45317/comment/d3565fe1_36ac829c PS4, Line 9: 0x0 nit: same
https://review.coreboot.org/c/coreboot/+/45317/comment/ff96e38a_69759b15 PS4, Line 13: 0x0 nit: same
https://review.coreboot.org/c/coreboot/+/45317/comment/dc68d820_40f8ecee PS4, Line 22: register "c2_latency" = "0x0065" Can be removed, it's the default value when unspecified.
File src/mainboard/asrock/h77pro4-m/early_init.c:
https://review.coreboot.org/c/coreboot/+/45317/comment/dc597028_f22c2aa0 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); As per the Z77 Pro4-M boardview, these two GPIOs control which of the two CPU fan headers should the Super I/O monitor the speed of. GP76 controls CPU_FAN2 and GP77 controls CPU_FAN1. Each GPIO drives a pair of MOSFETs that connect or isolate the fan header's tachometer signal, and they're active-low (0 -> tachometer connected, 1 -> tachometer isolated/ignored). The values you program here select CPU_FAN1.
Ideally, this would be configurable. At the very least, I'd expand the comment to explain what this GPIO7 configuration is used for.
If you want to test my theory, you can use lm-sensors to check "CPU fan" speed readings for each of these cases:
pnp_write_config(GPIO6789_DEV, 0xe1, 0x3f); /* CPU fan speed would always be 0 */ pnp_write_config(GPIO6789_DEV, 0xe1, 0x7f); /* CPU fan speed reports CPU_FAN1 speed */ pnp_write_config(GPIO6789_DEV, 0xe1, 0xbf); /* CPU fan speed reports CPU_FAN2 speed */
Enabling both CPU_FAN1 and CPU_FAN2 at the same time would merely confuse the Super I/O.
File src/mainboard/asrock/h77pro4-m/gpio.c:
https://review.coreboot.org/c/coreboot/+/45317/comment/925c6e96_e4533b6d PS4, Line 71: .gpio13 = GPIO_INVERT,
Input Inversion (GP_INV[n]) — R/W. This bit only has effect if the corresponding GPIO is used as an input and used by the GPE logic, where the polarity matters. When set to ‘1’, then the GPI is inverted as it is sent to the GPE logic that is using it. This bit has no effect on the value that is reported in the GP_LVL register.
The devicetree doesn't enable GPEs for GPIO13, so the programmed value is irrelevant. Hmmm, now I wonder what this GPIO is used for.
Looking at the Z77 Pro4-M boardview, this GPIO is connected to the PME# signal (pin 65) of the Super I/O. The NCT6776 datasheet says the following about the PME# signal:
The PME# (pin 65) signal is connected to the South Bridge and is used to wake up the system from S1 sleeping states.
One control bit and four registers in the NCT6776F / NCT6776D are associated with the PME function. The control bit is at Logical Device A, CR[F2h], bit[0] and is for enabling or disabling the PME function. If this bit is set to “0”, the NCT6776F / NCT6776D won’t output any PME signal when any of the wake-up events has occurred and is enabled.
Logical Device A, CR[F2h], bit[0] defaults to 0 (Disable PME), and this patch doesn't set it. So, it doesn't matter.
TL;DR: Just leave this as-is. I was curious and dug into why this GPIO uses inversion.
File src/mainboard/asrock/h77pro4-m/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/45317/comment/1277647f_5bfd6498 PS4, Line 6: Realtek nit: Realtek ALC892