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 62:
(59 comments)
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/Kconfig:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 1: #***************************************************************************** What's with these line comments?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 51: config ONBOARD_VGA_IS_PRIMARY Why?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 1: #***************************************************************************** What's with these line comments?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/OemCustomize.c:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 133: /*---------------------------------------------------------------------------------------- What's with these line comments?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 150: TODO Would be good to either enable or remove the code
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/OptionsIds.h:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 9: * empty line in comment
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 15: * empty line in comment
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 29: * empty line in comment
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 32: //#define IDSOPT_IDS_ENABLED TRUE Do we really need to have this many commented-out things?
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
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?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 39: Get Taken
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 63: 0x000009C4, Why switch to four spaces here?
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
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
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?
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
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
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 106: * EHCI @ func 2 */ Same here
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?
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?
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
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 23: /* DBGO("\_PTS\n") */ Please remove commented-out things
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
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
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
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???
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/bootblock.c:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 11: most likely Would be good to confirm
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/buildOpts.c:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 12: * Training empty line in comment
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 17: Two spaces surrounding several comments
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 50: //#define BLDOPT_REMOVE_UDIMMS_SUPPORT TRUE Do we need to have those commented-out entries?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 63: //TRUE ...
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 75: //This element selects whether P-States should be forced to be independent, : // as reported by the ACPI _PSD object. For single-link processors, : // setting TRUE for OS to support this feature. But other informative comments are C-style?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 84: */ Why does this comment end on a new line?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 143: // Specify the default values for the VRM controlling the VDDNB plane. Is any of this doing any good?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 162: #if CONFIG(GFXUMA) Why check for this? The mainboard selects GFXUMA already...
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 184: // #define BLDCFG_SMBUS0_BASE_ADDRESS 0xB00 Is this good for anything?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 248: #define DDR400_FREQUENCY 200 ///< DDR 400 Isn't this already defined in another header?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 261: */ missing a space
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 262: #define QUADRANK_REGISTERED 0 ///< Quadrank registered DIMM What's with the (lack of) alignment between these options?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/cmos.layout:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 1: #***************************************************************************** What's with these line comments?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 1: #***************************************************************************** What's with these line comments?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 31: end #chip northbridge/amd/agesa/family15tn # PCI side of HT root complex over 96 characters
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 64: io 0x60 = 0x3f0 : irq 0x70 = 6 : drq 0x74 = 2 but it's off???
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 68: Com1 COM1
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 72: Com2 COM2
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 73: io 0x60 = 0x2f8 : irq 0x70 = 3 It's off!!
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 77: io 0x60 = 0x378 : irq 0x70 = 7 It's off!!
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 91: irq 0x70 = 12 It's off!!
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 96: 0x238 #Phony resource IT8603E does not have it Why not zero?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 121: { {0xA0, 0x00}, {0xA2, 0x00}, }, // socket 0 - Channel 0 & 1 - 8-bit SPD addresses Lines over 96 characters
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 14: /* #include <arch/x86/acpi/debug.asl> */ /* Include global debug methods if needed */ Dead code that won't compile
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 49: TODO So? Can they be removed?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/irq_tables.c:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 11: u8 slot, u8 rfu) Fit on the previous line
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 38: addr += 15; : addr &= ~15; ALIGN_UP exists
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 41: /* This table must be between 0xf0000 & 0x100000 */ Is this up-to-date? Why is it not checked?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 44: pirq = (void *)(addr); : v = (u8 *) (addr); Why create two aliased variables? And why does only one cast have a space after it (should not have any)?
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 68: 0); Fits on the previous line
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/mainboard.c:
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 33: /***************************************************** What's with these line comments?