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
-- To view, visit https://review.coreboot.org/c/coreboot/+/34603 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Id8fa41ecccaa8dba8dc2158ce62d328c7928e05c Gerrit-Change-Number: 34603 Gerrit-PatchSet: 56 Gerrit-Owner: Pavel Sayekat Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Maxim Polyakov <m.poliakov@yahoo.com> Gerrit-Reviewer: Maxim Polyakov <max.senia.poliak@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Pavel Sayekat Gerrit-Reviewer: Sumeet R Pawnikar <sumeet.r.pawnikar@intel.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Patrick Rudolph <patrick.rudolph@9elements.com> Gerrit-CC: Patrick Rudolph <siro@das-labor.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sat, 08 Feb 2020 18:18:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Comment-In-Reply-To: Pavel Sayekat Gerrit-MessageType: comment