Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30987 )
Change subject: mb/asus: Add Asus A88XM-E FM2+ with documentation ......................................................................
Patch Set 31:
(21 comments)
https://review.coreboot.org/c/coreboot/+/30987/31//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/30987/31//COMMIT_MSG@9 PS31, Line 9: S This should be at the end of the commit message
https://review.coreboot.org/c/coreboot/+/30987/31//COMMIT_MSG@28 PS31, Line 28: Seabios SeaBIOS
https://review.coreboot.org/c/coreboot/+/30987/31//COMMIT_MSG@33 PS31, Line 33: C Why are there two Change-Id lines?
https://review.coreboot.org/c/coreboot/+/30987/31/Documentation/mainboard/as... File Documentation/mainboard/asus/a88xm-e.md:
https://review.coreboot.org/c/coreboot/+/30987/31/Documentation/mainboard/as... PS31, Line 29: 8603E IT8603E
https://review.coreboot.org/c/coreboot/+/30987/31/Documentation/mainboard/as... PS31, Line 69: Trailing space on all lines, is it on purpose?
https://review.coreboot.org/c/coreboot/+/30987/31/Documentation/mainboard/as... PS31, Line 87: can broke `can break` ?
https://review.coreboot.org/c/coreboot/+/30987/31/Documentation/mainboard/as... PS31, Line 98: Kaveri Kaveri uses BinaryPI, AFAIK
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/Kconfig:
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 40: DRR3 DDR3
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 44: DRR3 DDR3
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 48: DRR3 DDR3
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 85: config ONBOARD_VGA_IS_PRIMARY : bool : default y Why?
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 93: config CONFIG_DRIVERS_PS2_KEYBOARD : bool : default y Why?
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 105: config POST_IO : bool : default n : : config CONFIG_POST_DEVICE_PCI_PCIE : bool : default y Why?
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 113: config DEVICETREE : string : default "devicetree.cb" This should be the default already
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/buildOpts.c:
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 145: FALSE Seems to be misaligned
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/cmos.layout:
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 32: 456 1 e 1 ECC_memory Isn't ECC disabled in BuildOpts.c ?
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 47: 6 5 Notice There are some missing debug levels
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/mainboard.c:
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 22: thatcher board Um, but this isn't Thatcher?
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/mptable.c:
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 29: 0x1F Put a comma after this, for consistency
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 31: 0x54 I would use `84` instead
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 37: 0x13 Same