Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33151 )
Change subject: mb/apple: Add MacBook Pro 8,1 support ......................................................................
Patch Set 11:
(17 comments)
https://review.coreboot.org/c/coreboot/+/33151/11/Documentation/mainboard/ap... File Documentation/mainboard/apple/macbookpro8_1.md:
https://review.coreboot.org/c/coreboot/+/33151/11/Documentation/mainboard/ap... PS11, Line 32: 00000000:00000fff fd Does this get formatted as a block of code?
https://review.coreboot.org/c/coreboot/+/33151/11/Documentation/mainboard/ap... PS11, Line 57: - Thunderbolt : - FireWire Do the respective controllers enumerate properly on PCIe?
https://review.coreboot.org/c/coreboot/+/33151/11/Documentation/mainboard/ap... File Documentation/mainboard/apple/mbp81_chip.jpg:
PS11: You can make this image a bit smaller, there's no need to see so much fan
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro8_1/Kconfig:
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 1: BOARD_APPLE_MACBOOKPRO8_1 nit: This would read better if you separate the numbers:
BOARD_APPLE_MACBOOKPRO_8_1
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 22: macbookpro8_1 Same here
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 28: config VGA_BIOS_FILE : string : default "pci8086,0126.rom" : : config VGA_BIOS_ID : string : default "8086,0126" Did you test the VBIOS?
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro8_1/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 23: /* 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 can be dropped, as they are zero already.
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro8_1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 16: 0x0 These can be simply zero
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 31: 0x0 This one as well
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 37: subsystemid 0x106b 0x00db If it's off, does it need a subsystemid?
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 60: Audio Audio Is it stereo audio? I would shorten this as "HD Audio Controller"
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro8_1/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 12: : : #define BRIGHTNESS_UP _SB.PCI0.GFX0.INCB : #define BRIGHTNESS_DOWN _SB.PCI0.GFX0.DECB : #define ACPI_VIDEO_DEVICE _SB.PCI0.GFX0 Are you using these?
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro8_1/early_init.c:
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 18: Are you using all these headers?
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 30: Hub USB port, you mean?
In any case, it should be routed to an OC# pin.
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro8_1/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 30: Analog Really?
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro8_1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 24: RCBA32(0x38c8) = 0x00002005; : RCBA32(0x38c4) = 0x00800000; Aren't these doing the same as the VSCC settings in the devicetree?
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 32: install_intel_vga_int15_handler(GMA_INT15_ACTIVE_LFP_INT_LVDS, : GMA_INT15_PANEL_FIT_DEFAULT, : GMA_INT15_BOOT_DISPLAY_DEFAULT, 0); Does this work?