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 47:
(18 comments)
https://review.coreboot.org/c/coreboot/+/30987/47/Documentation/mainboard/as... File Documentation/mainboard/asus/a88xm-e.md:
https://review.coreboot.org/c/coreboot/+/30987/47/Documentation/mainboard/as... PS47, Line 64: the vendor EFI binary Are you sure? the command looks wrong
https://review.coreboot.org/c/coreboot/+/30987/47/Documentation/mainboard/as... PS47, Line 98: Kaveri/Godavary Don't these use a different AGESA? (well, binaryPI)
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/Kconfig:
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 55: config BOARD_ASUS_A88XM_E_DDR3_VOLT_VAL I don't recall, was this ever tested? By tested, I mean all three voltages. 1.50V is the default for DDR3
https://review.coreboot.org/c/coreboot/+/30987/42/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/OemCustomize.c:
https://review.coreboot.org/c/coreboot/+/30987/42/src/mainboard/asus/a88xm-e... PS42, Line 163: /* TODO: is this OK for DDR3 socket FM2? */ : /* : MEMCLK_DIS_MAP(ANY_SOCKET, ANY_CHANNEL, 0x01, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00), : CKE_TRI_MAP(ANY_SOCKET, ANY_CHANNEL, 0x05, 0x0A), : ODT_TRI_MAP(ANY_SOCKET, ANY_CHANNEL, 0x01, 0x02, 0x00, 0x00), : CS_TRI_MAP(ANY_SOCKET, ANY_CHANNEL, 0x01, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00), : */
NEW IDEA: although coreboot's AGESA code seems to be not XMP-capable, it could be possible to use a […]
You can also rewrite AGESA and add support for XMP memory profiles. And when I say rewrite, I mean it. I am afraid to touch this code at all.
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/OemCustomize.c:
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 147: * CUSTOMER OVERIDES MEMORY TABLE
'OVERIDES' may be misspelled - perhaps 'OVERRIDES'?
Please fix.
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi/cpstate.asl:
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 16: /* This This comment should start like this:
/* * This ...
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 20: */ Needs a space
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 23: DefinitionBlock ("DSDT.AML", "DSDT", 0x01, OEM_ID, ACPI_TABLE_CREATOR, 0x00010001) Why is this commented out?
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright (C) 2012 Advanced Micro Devices, Inc. : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */ Please use SPDX for all license headers.
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?
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?
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?
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
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
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/mainboard.c:
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 47: the dedicated function Which dedicated function? Maybe look up the MSR meanings?
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/mptable.c:
PS47: Is this even correct?
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 125: Lan LAN (and add a space after it)
https://review.coreboot.org/c/coreboot/+/30987/47/src/mainboard/asus/a88xm-e... PS47, Line 137: /*Local Ints: Type Polarity Trigger Bus ID IRQ APIC ID PIN# */ This comment does not correspond to the code below...