Attention is currently required from: Paul Menzel, Evgeny Zinoviev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32673 )
Change subject: mb/apple: Add MacBook Pro 10,1 (A1398) support ......................................................................
Patch Set 35:
(10 comments)
File Documentation/mainboard/apple/macbookpro10_1.md:
https://review.coreboot.org/c/coreboot/+/32673/comment/ba53b560_8a24b65a PS35, Line 16: 8MB MiB
https://review.coreboot.org/c/coreboot/+/32673/comment/9f2290a5_c4d71ee1 PS35, Line 29: 8GB GiB
File src/mainboard/apple/macbookpro10_1/Kconfig:
https://review.coreboot.org/c/coreboot/+/32673/comment/d14c2f26_5eb85cf9 PS35, Line 19: select DRIVERS_APPLE_HYBRID_GRAPHICS I'd sort the selected options alphabetically.
https://review.coreboot.org/c/coreboot/+/32673/comment/707006ba_2b77def9 PS35, Line 29: config VGA_BIOS_FILE : string : default "pci8086,0166.rom" Please drop this option. Not everyone will use a file named `pci8086,0166.rom`.
File src/mainboard/apple/macbookpro10_1/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/32673/comment/d5fd6c0b_ee4ec997 PS35, Line 8: spd.bin-file := spd.bin For memory down SPD data, we usually have a plain-text file and use the `GENERIC_SPD_BIN` Kconfig option. There's an example in hp/snb_ivb_laptops
File src/mainboard/apple/macbookpro10_1/cmos.layout:
PS35: Please get rid of the commented-out options. I'd suggest grabbing a cmos.layout from a board in the tree as a reference.
File src/mainboard/apple/macbookpro10_1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32673/comment/e379ab10_cba194c8 PS35, Line 24: 0x8086 0x7270 Replace the IDs in here with `0x106b 0x00f7` and drop all other `subsystemid 0x106b 0x00f7` lines
File src/mainboard/apple/macbookpro10_1/early_init.c:
https://review.coreboot.org/c/coreboot/+/32673/comment/da8d13e1_043cd661 PS35, Line 12: pci_write_config16(PCH_LPC_DEV, LPC_EN, 0x3f0f); : pci_write_config32(PCH_LPC_DEV, LPC_GEN1_DEC, 0x000c0681); : pci_write_config32(PCH_LPC_DEV, LPC_GEN2_DEC, 0x000c1641); : pci_write_config32(PCH_LPC_DEV, LPC_GEN3_DEC, 0x001c0301); : pci_write_config32(PCH_LPC_DEV, LPC_GEN4_DEC, 0x00fc0701); : pci_write_config16(PCH_LPC_DEV, LPC_IO_DEC, 0x0070); : } Shouldn't be needed.
https://review.coreboot.org/c/coreboot/+/32673/comment/ae6be2a1_c845d474 PS35, Line 44: /* Hide disabled devices */ Shouldn't this code be in the northbridge?
https://review.coreboot.org/c/coreboot/+/32673/comment/bdfb9b3b_34097f52 PS35, Line 45: PCI_DEV(0, 0, 0) HOST_BRIDGE