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 22:
(21 comments)
https://review.coreboot.org/c/coreboot/+/32673/22//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32673/22//COMMIT_MSG@7 PS22, Line 7: A1398 I found out that these model numbers are not precise enough. A1418, for instance, came with at least four different mainboards.
I would recommend using the EMC number (e.g. EMC 2544), or the mainboard model number (820-3302)
https://review.coreboot.org/c/coreboot/+/32673/22//COMMIT_MSG@10 PS22, Line 10: : What works: : - libgfxinit : - VGA ROM loading : - Integrated GPU : - Discrete GPU : - SeaBIOS, GRUB, Tianocore : - Linux (devuan ascii, kernel 4.9) : - Wi-Fi : - Both USB ports : - Trackpad : - me_cleaner : - Integrated/Discrete graphics selection via nvramtool : - Camera : - Mic : - SD card reader : - Speaker : - Headphones : - usbdebug (the usb port on the right side) : - Backlight control via gmux (/sys/class/backlight/gmux_backlight) Maybe add some documentation and move this list there?
What about untested / non-working things?
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro10_1/Kconfig:
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 17: SERIRQ_CONTINUOUS_MODE Already selected on line 11
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro10_1/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 16: FIXME Has this been fixed?
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro10_1/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 17: /* FIXME: check this function. */ Has it been checked?
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 28: // the lid is open by default. Please be consistent with the comment style
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro10_1/cmos.layout:
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 75: gfx_uma_size This option should not be commented out
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro10_1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 2: { 0x80000410, 0x80000320, 0x80000410, 0x80000410, 0x00000005 } Please trim to size (gfx.ndid)
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 41: register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }" This can be dropped
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 63: Audio Audio Much audio? I guess this is duplicated
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 83: PCI-LPC bridge Can drop this part
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 99: Host bridge Host bridge Host bridge much? I guess it's duplicated
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 98: : device pci 00.0 on # Host bridge Host bridge : subsystemid 0x106b 0x00f7 : end : device pci 01.0 on # PCIe Bridge for discrete graphics : subsystemid 0x106b 0x00f7 : end : device pci 02.0 on # Internal graphics VGA controller : subsystemid 0x106b 0x00f7 : end Move these before the southbridge "chip" entry
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 108: device pci 01.2 on : subsystemid 0x106b 0x00f7 : end : device pci 01.1 on : subsystemid 0x106b 0x00f7 : end What is this?
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro10_1/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 30: Analog Hmm?
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro10_1/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 25: /* NID 0x09. */ Please get rid of these obnoxious comments
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro10_1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 30: LVDS But you said GFX_GMA_INTERNAL_IS_EDP in Kconfig!
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro10_1/romstage.c:
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 26: pci_write_config16(PCH_LPC_DEV, 0x82, 0x3f0f); : pci_write_config32(PCH_LPC_DEV, 0x84, 0x000c0681); : pci_write_config32(PCH_LPC_DEV, 0x88, 0x000c1641); : pci_write_config32(PCH_LPC_DEV, 0x8c, 0x001c0301); : pci_write_config32(PCH_LPC_DEV, 0x90, 0x00fc0701); : pci_write_config16(PCH_LPC_DEV, 0x80, 0x0070); There are macros for these
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 72: /* Disable IGD VGA decode, no GTT or GFX stolen */ : pci_write_config16(PCI_DEV(0, 0, 0), GGC, 2); This is confusing without braces on the if/else
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 87: &spd_file_len); Fits on the previous line
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 88: (spd_file && spd_file_len >= 128) So, if this check is false, what's the chances of the machine booting successfully?
I'm not an expert, but I'd say... ZILCH!
I would then suggest placing a die() here, with a message along the lines of: "reading spd file failed"