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 64:
(37 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 51: config ONBOARD_VGA_IS_PRIMARY
Why?
Inherited from F2A85-M, I guess. For me it's fine: quite used to DRI_PRIME=1 offloading to a discrete GPU.
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?
Done
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?
Done
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
Done.
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
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 15: *
empty line in comment
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 29: *
empty line in comment
Done
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?
This stuff gets uncommented when there's a need for debugging. Although IDS perhaps isn't fixed there - my CB:36880 (vc/amd/agesa/f16kb: fix the IDS_HDT_CONSOLE build warnings/errors) didn't get any attention, otherwise would've fixed for f15tn and f14 as well.
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
Done
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
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 17:
Two spaces surrounding several comments
Done
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?
could be useful for debugging. easier to uncomment then to search for these options
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 63: //TRUE
...
Done
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?
Done
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?
Done
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?
Maybe the future explorers will do something good out of it
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...
Done
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?
I don't want to close the road for any future improvement...
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?
Tried to remove: lets see what happens...
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 261: */
missing a space
Done
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?
Done
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?
Done
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 31: end #chip northbridge/amd/agesa/family15tn # PCI side of HT root complex
over 96 characters
Done
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???
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 68: Com1
COM1
Done
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!!
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 91: irq 0x70 = 12
It's off!!
Done (enabled)
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?
Done
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
Done
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
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 49: TODO
So? Can they be removed?
Done
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
Done
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 38: addr += 15; : addr &= ~15;
ALIGN_UP exists
Done.
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?
In any case, all this IRQ routing is planned for rewrite.
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 […]
u8 cast cuts the upper 24 bits, while a pirq gets a full value
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 68: 0);
Fits on the previous line
Done
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?
Done