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 23:
(15 comments)
Marking the old resolved comments as "Done"
https://review.coreboot.org/c/coreboot/+/30987/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/30987/2//COMMIT_MSG@7 PS2, Line 7: Adding Asus A88XM-E FM2+ motherboard with documentation, the port based on F2A85-M.
Please use imperative mode for the verbs in the commit message: "Add Asus A88XM-E FM2+... […]
Done.
https://review.coreboot.org/c/coreboot/+/30987/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/30987/3//COMMIT_MSG@7 PS3, Line 7: Add Asus A88XM-E FM2+ motherboard with documentation, the port based on F2A85-M.
Please use a prefix and a shorter commit message summary. Maybe: […]
Done.
https://review.coreboot.org/c/coreboot/+/30987/3//COMMIT_MSG@9 PS3, Line 9: Signed-off-by: Balazs Vinarz vinibali1@gmail.com
Please remove the duplicate line here, and leave the Signed-off-by line below.
Done.
https://review.coreboot.org/c/coreboot/+/30987/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/30987/4//COMMIT_MSG@9 PS4, Line 9: Signed-off-by: Balazs Vinarz vinibali1@gmail.com
Please move this to the very bottom right below the Change-Id line.
Done.
https://review.coreboot.org/c/coreboot/+/30987/4//COMMIT_MSG@10 PS4, Line 10: Change-Id: I60fa0636ba41f5f1a6a3faa2764bf2f0a968cf90
This is duplicated and can be removed.
Done.
https://review.coreboot.org/c/coreboot/+/30987/2/Documentation/mainboard/asu... File Documentation/mainboard/asus/a88xm-e.md:
https://review.coreboot.org/c/coreboot/+/30987/2/Documentation/mainboard/asu... PS2, Line 13: and their GPU is [Sea Islands] (GCN2-based).
Ack
Done.
https://review.coreboot.org/c/coreboot/+/30987/2/Documentation/mainboard/asu... PS2, Line 83: - Integrated ethernet
It's not. No interface/device found on OS level, neighter during Coreboot init. […]
Done.
https://review.coreboot.org/c/coreboot/+/30987/2/Documentation/mainboard/asu... PS2, Line 88: board with factory image makes it work again as fallback.
You will probably not get such blobs merged, even extracted microcode updates have recently been rej […]
Done.
https://review.coreboot.org/c/coreboot/+/30987/2/src/mainboard/asus/a88xm-e/... File src/mainboard/asus/a88xm-e/Kconfig:
https://review.coreboot.org/c/coreboot/+/30987/2/src/mainboard/asus/a88xm-e/... PS2, Line 29: SUPERIO_ITE_IT8728F
Yes, it is. Probably really same chip, just different package pin count. […]
Done.
https://review.coreboot.org/c/coreboot/+/30987/2/src/mainboard/asus/a88xm-e/... PS2, Line 113: config DEVICETREE : string : default "devicetree_a88xm-e.cb"
Done
Done.
https://review.coreboot.org/c/coreboot/+/30987/2/src/mainboard/asus/a88xm-e/... File src/mainboard/asus/a88xm-e/romstage.c:
https://review.coreboot.org/c/coreboot/+/30987/2/src/mainboard/asus/a88xm-e/... PS2, Line 49: pnp_devfn_t uart = PNP_DEV(0x2e, IT8728F_SP1); : pnp_devfn_t gpio = PNP_DEV(0x2e, IT8728F_GPIO);
I wouldn't bother these lines, the GPIO have a 0x07 value on superio/ite/it8728f/it8728f.h.
Done.
https://review.coreboot.org/c/coreboot/+/30987/22/src/mainboard/asus/a88xm-e... File src/mainboard/asus/a88xm-e/romstage.c:
https://review.coreboot.org/c/coreboot/+/30987/22/src/mainboard/asus/a88xm-e... PS22, Line 18: #include <console/console.h> : #include <device/pnp_type.h> : #include <device/pci_ops.h>
remove
Done.
https://review.coreboot.org/c/coreboot/+/30987/22/src/mainboard/asus/a88xm-e... PS22, Line 22: #include <southbridge/amd/common/amd_defs.h> : #include <southbridge/amd/agesa/hudson/hudson.h>
remove
Done.
https://review.coreboot.org/c/coreboot/+/30987/22/src/mainboard/asus/a88xm-e... PS22, Line 26: #include <string.h> : : #include <superio/ite/common/ite.h> : #include <superio/ite/it8728f/it8728f.h> : : #define MMIO_NON_POSTED_START 0xfed00000 : #define MMIO_NON_POSTED_END 0xfedfffff : #define SB_MMIO_MISC32(x) (*(volatile u32 *)(AMD_SB_ACPI_MMIO_ADDR + 0xE00 + (x))) : : static void sbxxx_enable_48mhzout(void) : { : /* most likely programming to 48MHz out signal */ : u32 reg32; : reg32 = SB_MMIO_MISC32(0x28); : reg32 &= 0xffc7ffff; : reg32 |= 0x00100000; : SB_MMIO_MISC32(0x28) = reg32; : : reg32 = SB_MMIO_MISC32(0x40); : reg32 &= ~0x80u; : SB_MMIO_MISC32(0x40) = reg32; : } : : static void superio_init_e(void) : { : pnp_devfn_t uart = PNP_DEV(0x2e, IT8728F_SP1); : pnp_devfn_t gpio = PNP_DEV(0x2e, IT8728F_GPIO); : : ite_kill_watchdog(gpio); : ite_enable_serial(uart, CONFIG_TTYS0_BASE); : ite_enable_3vsbsw(gpio); : }
remove
Done.
https://review.coreboot.org/c/coreboot/+/30987/22/src/mainboard/asus/a88xm-e... PS22, Line 62: pci_devfn_t dev; : : /* enable SIO LPC decode */ : dev = PCI_DEV(0, 0x14, 3); : byte = pci_read_config8(dev, 0x48); : byte |= 3; /* 2e, 2f */ : pci_write_config8(dev, 0x48, byte); : : /* enable serial decode */ : byte = pci_read_config8(dev, 0x44); : byte |= (1 << 6); /* 0x3f8 */ : pci_write_config8(dev, 0x44, byte); : : post_code(0x30); : : /* enable SB MMIO space */ : outb(0x24, 0xcd6); : outb(0x1, 0xcd7); : : /* enable SIO clock */ : sbxxx_enable_48mhzout(); : : if (CONFIG(BOARD_ASUS_A88XM_E)) : superio_init_e();
remove
Done.