Evgeny Zinoviev 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 23:
(34 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. […]
Done
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? […]
Done
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
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 29: : config VGA_BIOS_FILE : string : default "pci8086,0166.rom" : : config VGA_BIOS_ID : string : default "8086,0166"
Is the VBIOS tested?
On this one, yes.
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro10_1/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 1: romstage-y += gpio.c
Note that currently, this also needs to be built for the bootblock
Done
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?
Done
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?
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 20: /* Disable USB ports in S3 by default */ : gnvs->s3u0 = 0; : gnvs->s3u1 = 0; : : /* Disable USB ports in S5 by default */ : gnvs->s5u0 = 0; : gnvs->s5u1 = 0;
These are zero and can be dropped
Done
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
Done
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
Done
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 }
Better yet: drop it altogether.
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 18: 0x0
0
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 26: 0x0
0
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 31: 0x0
0
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 33: : register "docking_supported" = "0"
These are zero and can be dropped
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 39: : register "p_cnt_throttling_supported" = "1"
This no longer exists on master
Done
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
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 51: subsystemid 0x8086 0x7270
This can be factored out
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 63: Audio Audio
Much audio? I guess this is duplicated
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 83: PCI-LPC bridge
Can drop this part
Done
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
Done
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
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro10_1/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 14: #define BRIGHTNESS_UP _SB.PCI0.GFX0.INCB : #define BRIGHTNESS_DOWN _SB.PCI0.GFX0.DECB : #define ACPI_VIDEO_DEVICE _SB.PCI0.GFX0
Are these used?
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 28: /* Some generic macros */
Please remove this. I killed all instances of this comment in a previous change.
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 31: #include <southbridge/intel/bd82x6x/acpi/platform.asl> : /* global NVS and variables. */ : #include <southbridge/intel/bd82x6x/acpi/globalnvs.asl> : #include <southbridge/intel/bd82x6x/acpi/sleepstates.asl>
Note that some of these are now under common/
Done
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 21: 0x0000000b
This should be decimal
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 23: 0x0
This too
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 25: /* NID 0x09. */
Please get rid of these obnoxious comments
Done
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
If not using the VBIOS, you can drop the entire file.
Well, VBIOS works. Who knows, maybe someone will use it.
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro10_1/romstage.c:
PS22:
This is now called early_init. […]
Done
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
Done
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
Done
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 87: &spd_file_len);
Fits on the previous line
Done
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? […]
Done