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 50:
(5 comments)
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/bootblock.c:
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 34: pnp_devfn_t uart = PNP_DEV(0x2e, IT8728F_SP1);
Make these const?
Done.
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/buildOpts.c:
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 63: //#define BLDOPT_REMOVE_UDIMMS_SUPPORT TRUE
Can this be aligned with tabs please?
Done.
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 197: // #define BLDCFG_SMBUS0_BASE_ADDRESS 0xB00
Are these commented-out entries needed?
Yes - at least for the reference
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 238: { AMD_AP_MTRR_FIX16k_A0000, 0x0000000000000000 },
nit: Add a space before the hex numbers to align them
Done.
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/irq_tables.c:
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 21: static void write_pirq_info(struct irq_info *pirq_info, u8 bus, u8 devfn,
This function makes things much less clear than directly writing the fields
In any case this whole IRQ routing will be rewritten later...