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 66:
(20 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
Inherited from F2A85-M, I guess. […]
This means that, by default, a VBIOS for the iGPU needs to be added. If it's not enabled by default, one can still boot without the iGPU VBIOS using a dedicated PCIe graphics card. Also, there are FM2 CPUs without an integrated GPU, so this seems unjustified.
Unless using an external PCIe GPU does not work at all, I don't see why this should be enabled. And if dedicated GPUs were in fact not working, this would deserve a FIXME comment stating so.
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 32: //#define IDSOPT_IDS_ENABLED TRUE
This stuff gets uncommented when there's a need for debugging. […]
In any case, looks like undefining things isn't necessary (c.f. src/vc/amd/agesa/f15tn/Include/Ids.h), so those can be removed.
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi/cpstate.asl:
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... PS66, Line 9: Starting here, you can drop a tab on the rest of the file
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi/gpe.asl:
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... PS66, Line 19: Notify (_TZ.TZ00, 0x80) This shouldn't be commented-out
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi/sata.asl:
PS62:
CC cbfs/fallback/ramstage.debug […]
Ack
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi/si.asl:
PS62:
Code for a System Status Indicator (status LEDs)
Currently, it's just an empty method declaration. Does it do anything at all? I'd try removing it and see if there are any errors.
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 77: /* DBGO("\_WAK\n") */ : /* DBGO("From S") */ : /* DBGO(Arg0) */ : /* DBGO(" to S0\n") */
Some of these "DBG" could be useful for future debugging. […]
... uncommenting each and every line is easier??? Does it even compile?
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/bootblock.c:
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... PS66, Line 14: 0xffc7ffff Masking of certain bits is usually done with an and-not operation. Using shifts makes it even clearer:
~(7 << 19)
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... PS66, Line 15: 0x00100000 Would be clearer to use:
(2 << 19)
And maybe align it with an extra space that compensates the ~ on the previous line
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... PS66, Line 19: (~0x80u) ~(1 << 7)
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 50: //#define BLDOPT_REMOVE_UDIMMS_SUPPORT TRUE
could be useful for debugging. […]
You want to debug by removing UDIMM support?
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.
Maybe the future explorers will do something good out of it
They will dig... cobwebs? They might as well dig the git history or try rewriting AGESA.
In any case, if not used on Trinity, it makes little sense to have these definitions here
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 184: // #define BLDCFG_SMBUS0_BASE_ADDRESS 0xB00
I don't want to close the road for any future improvement...
One might as well erase all of it because it does not affect the resulting binary. Sounds like several of these values are important, but they might have been defined in other headers already.
https://review.coreboot.org/c/coreboot/+/30987/62/src/mainboard/asus/a88xm-e... PS62, Line 248: #define DDR400_FREQUENCY 200 ///< DDR 400
Build failed after removal - had to restore.
Ack. Maybe factor them out into their own header
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/buildOpts.c:
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... PS66, Line 74: /* This Multi-line comments in coreboot should start like this:
/* * This
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 64: io 0x60 = 0x3f0 : irq 0x70 = 6 : drq 0x74 = 2
Done
No, don't remove the whole device, only the things inside it:
device pnp 2e.0 off end # Floppy
Same thing for the other entries
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... PS66, Line 105: /* socket 1 - Channel 0 & 1 - 8-bit SPD addresses */ : { {0x00, 0x00}, {0x00, 0x00}, }, Does it build without this?
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 44: pirq = (void *)(addr); : v = (u8 *) (addr);
u8 cast cuts the upper 24 bits, while a pirq gets a full value
No, it's casting to an u8 *pointer*, which has the same size of any other pointer. See current patchset for a better way to handle this
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/irq_tables.c:
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... PS66, Line 60: pirq_info = (void *)(&pirq->checksum + 1); that's just taking the address of "pirq->slots" in a rather fragile way
https://review.coreboot.org/c/coreboot/+/30987/66/src/mainboard/asus/a88xm-e... PS66, Line 73: v[i] Ah, I see: this is used to read the bytes for checksumming purposes. Maybe declare it in a scope, and make it const (read-only)?
{ const u8 *const v = (u8 *)(pirq); for (i = 0; i < pirq->size; i++) sum += v[i]; }