Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34603 )
Change subject: mainboard/asus: Add ASUS H110M-E/M.2 mainboard ......................................................................
Patch Set 56: Code-Review+1
(29 comments)
https://review.coreboot.org/c/coreboot/+/34603/56/src/mainboard/asus/h110m-e... File src/mainboard/asus/h110m-e_m2/Kconfig:
https://review.coreboot.org/c/coreboot/+/34603/56/src/mainboard/asus/h110m-e... PS56, Line 36: config DEVICETREE it's the default value
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... File src/mainboard/asus/h110m-e_m2/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 19: Enable
Nit: Disable
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 55:
register "PchHdaVcType" = "Vc1"
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 107: 0x0
1520
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 120: 0x0
1520
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 133: 0x0
1520
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 146: 0x0
1520
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 187: [4] = 1, \ : [5] = 1, \ : [6] = 1, \ : [7] = 1, \
These are 0 for H110
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 203: # Enable CLKREQ# : register "PcieRpClkReqSupport[7]" = "1" : # Use SRCCLKREQ1# : register "PcieRpClkReqNumber[7]" = "1"
Should not be needed and can be disabled
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 256: end
These lone `end` words can go into the previous line
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 282: device pci 1c.4 off end # PCI Express Port 5
should be on
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 298: chip superio/nuvoton/nct5539d
https://github. […]
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 301: rq 0x1c = 0x10 : irq 0x27 = 0x03 : irq 0x2a = 0xc0
Remove these writes. They are writing the default values, and coreboot hangs right when doing so.
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 343: chip drivers/pc80/tpm : device pnp 4e.0 on end # TPM module : end
You don't have a TPM
Done
https://review.coreboot.org/c/coreboot/+/34603/56/src/mainboard/asus/h110m-e... File src/mainboard/asus/h110m-e_m2/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/34603/56/src/mainboard/asus/h110m-e... PS56, Line 203: Enable They are not enabled anymore. Might as well drop the comments
https://review.coreboot.org/c/coreboot/+/34603/56/src/mainboard/asus/h110m-e... PS56, Line 298: chip superio/nuvoton/nct5539d you need to indent this, though
https://review.coreboot.org/c/coreboot/+/34603/56/src/mainboard/asus/h110m-e... PS56, Line 300: # global : # UART A these two comments aren't needed anymore
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... File src/mainboard/asus/h110m-e_m2/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 51: soc/intel/skylake
southbridge/intel/common
Done
https://review.coreboot.org/c/coreboot/+/34603/56/src/mainboard/asus/h110m-e... File src/mainboard/asus/h110m-e_m2/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/34603/56/src/mainboard/asus/h110m-e... PS56, Line 17: */ What happened to the space here?
https://review.coreboot.org/c/coreboot/+/34603/56/src/mainboard/asus/h110m-e... PS56, Line 22: / And here?
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... File src/mainboard/asus/h110m-e_m2/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 25: 0x0
these 0x0 should be 0
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 44: 0x00000004
just 4
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 45: 0x80860101
You don't need to repeat the value in the comment
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 46: 0x2
should be 2
Done
https://review.coreboot.org/c/coreboot/+/34603/56/src/mainboard/asus/h110m-e... File src/mainboard/asus/h110m-e_m2/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/34603/56/src/mainboard/asus/h110m-e... PS56, Line 26: 0x0 these are just 0 as well
https://review.coreboot.org/c/coreboot/+/34603/56/src/mainboard/asus/h110m-e... PS56, Line 49: 0x2 these are just 2 as well
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... File src/mainboard/asus/h110m-e_m2/ramstage.c:
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 27: /* Enable Virtual Channel 1 */ : params->PchHdaVcType = 0x1;
Should be set in the devicetree
Done
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... File src/mainboard/asus/h110m-e_m2/romstage.c:
https://review.coreboot.org/c/coreboot/+/34603/50/src/mainboard/asus/h110m-e... PS50, Line 45: 0x53,
If your board has only 2 DIMMs, you can probably scratch 0x51 and 0x53.
It is the case, done.
https://review.coreboot.org/c/coreboot/+/34603/53/src/mainboard/asus/h110m-e... File src/mainboard/asus/h110m-e_m2/romstage.c:
https://review.coreboot.org/c/coreboot/+/34603/53/src/mainboard/asus/h110m-e... PS53, Line 54: mem_cfg->MemorySpdPtr00 = (uintptr_t)blk.spd_array[0]; : mem_cfg->MemorySpdPtr10 = (uintptr_t)blk.spd_array[2]; : mem_cfg->MemorySpdPtr01 = (uintptr_t)blk.spd_array[1]; : mem_cfg->MemorySpdPtr11 = (uintptr_t)blk.spd_array[3];
Needs to change as the order changed above. Most likely: […]
some intel board with two DIMMs does exactly that