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 63:
(18 comments)
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi/cpstate.asl:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 10: /* : #include <acpi/acpi.h> : DefinitionBlock ("DSDT.AML", "DSDT", 0x01, OEM_ID, ACPI_TABLE_CREATOR, 0x00010001) : { : Scope (_PR) { : Device (CPU0) { : Name (_HID, "ACPI0007") : Name (_UID, 0) : #include "cpstate.asl" : } : Device (CPU1) { : Name (_HID, "ACPI0007") : Name (_UID, 1) : #include "cpstate.asl" : } : Device (CPU2) { : Name (_HID, "ACPI0007") : Name (_UID, 2) : #include "cpstate.asl" : } : Device (CPU3) { : Name (_HID, "ACPI0007") : Name (_UID, 3) : #include "cpstate.asl" : } : } : */
This is commented-out
Done.
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 37: /* P-state support: The maximum number of P-states supported by the */
Why not use a multi-line comment?
Done.
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 39: Get
Taken
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 63: 0x000009C4,
Why switch to four spaces here?
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi/gpe.asl:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 5:
There's two spaces around the comment delimiters
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 11: Name(PCLN, Multiply(0x100000, CONFIG_MMCONF_BUS_NUMBER)) /* Length of PCIe config space, 1MB each bus */
Line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi/routing.asl:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 35: F0:SMBus/ACPI,F1:IDE;F2:HDAudio;F3:LPC;F4:PCIBridge;F5:USB
Why is everything squished together?
To fit in a character limit, it seems... Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 42: * EHCI @ func 2 */
This fits on the previous line just fine
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 98: /* Bus 0, Dev 20 - F0:SMBus/ACPI, F1:IDE; F2:HDAudio; F3:LPC; F4:PCIBridge; F5:USB */
Line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 106: * EHCI @ func 2 */
Same here
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi/sata.asl:
PS62:
Is this needed?
CC cbfs/fallback/ramstage.debug CREATE /cb-build/coreboot-gerrit.0/default/ASUS_A88XM_E/mainboard/asus/a88xm-e/cbfs-file.aNGXui.out (from /cb-build/coreboot-gerrit.0/default/ASUS_A88XM_E/config.build) IASL /cb-build/coreboot-gerrit.0/default/ASUS_A88XM_E/dsdt.aml In file included from src/mainboard/asus/a88xm-e/dsdt.asl:46: src/southbridge/amd/agesa/hudson/acpi/fch.asl:26:11: fatal error: acpi/sata.asl: No such file or directory #include "acpi/sata.asl" ^~~~~~~~~~~~~~~ compilation terminated. make[1]: *** [src/acpi/Makefile.inc:17: /cb-build/coreboot-gerrit.0/default/ASUS_A88XM_E/dsdt.aml] Error 1 make[1]: Leaving directory '/home/coreboot/slave-root/workspace/coreboot-gerrit'
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi/si.asl:
PS62:
So, what does this do?
Code for a System Status Indicator (status LEDs)
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi/sleep.asl:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 7: * _PTS - Prepare to Sleep method
These comments are missing a space before the asterisk
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 23: /* DBGO("\_PTS\n") */
Please remove commented-out things
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 28: /* Clear sleep SMI status flag and enable sleep SMI trap. */ : /*Store(One, CSSM) : Store(One, SSEN)*/ : : /* On older chips, clear PciExpWakeDisEn */ : /*if (LLessEqual(_SB.SBRI, 0x13)) { : * Store(0,_SB.PWDE) : *} : */
Commented-out stuff
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 44: : /* : * _BFS OEM Back From Sleep method : * : * Entry: : * Arg0=The value of the sleeping state S1=1, S2=2 : * : * Exit: : * -none- : */ : Method(_BFS, 1) { : /* DBGO("\_BFS\n") */ : /* DBGO("From S") */ : /* DBGO(Arg0) */ : /* DBGO(" to S0\n") */ : }
This does nothing
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 77: /* DBGO("\_WAK\n") */ : /* DBGO("From S") */ : /* DBGO(Arg0) */ : /* DBGO(" to S0\n") */
More dead code
Some of these "DBG" could be useful for future debugging. Easier to uncomment than to write
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi/usb_oc.asl:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 3:
Three spaces???
Done