Mike Banon 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 32:
(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
Done.
https://review.coreboot.org/c/coreboot/+/30987/31//COMMIT_MSG@28 PS31, Line 28: Seabios
SeaBIOS
Done.
https://review.coreboot.org/c/coreboot/+/30987/31//COMMIT_MSG@33 PS31, Line 33: C
Why are there two Change-Id lines?
Done.
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
Done.
https://review.coreboot.org/c/coreboot/+/30987/31/Documentation/mainboard/as... PS31, Line 69:
Trailing space on all lines, is it on purpose?
Done.
https://review.coreboot.org/c/coreboot/+/30987/31/Documentation/mainboard/as... PS31, Line 87: can broke
`can break` ?
Done.
https://review.coreboot.org/c/coreboot/+/30987/31/Documentation/mainboard/as... PS31, Line 98: Kaveri
Kaveri uses BinaryPI, AFAIK
for Balazs to address
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
Done.
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 44: DRR3
DDR3
Done.
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 48: DRR3
DDR3
Done.
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?
question for Balazs
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?
question for Balazs
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?
question for Balazs
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
Done.
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
Done.
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. […]
Mistake borrowed from F2A85-M. Need to understand a format of this table to fix, will do a bit later...
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
Mistake borrowed from F2A85-M. Need to understand a format of this table to fix, will do a bit later...
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?
Done.
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
Done.
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 31: 0x54
I would use `84` instead
Done.
https://review.coreboot.org/c/coreboot/+/30987/31/src/mainboard/asus/a88xm-e... PS31, Line 37: 0x13
Same
Done.