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:
(20 comments)
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 29: : config VGA_BIOS_FILE : string : default "pci8086,0166.rom" : : config VGA_BIOS_ID : string : default "8086,0166" Is the VBIOS tested?
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
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 2: : cbfs-files-y += spd.bin : spd.bin-file := spd.bin : spd.bin-type := spd This could use the GENERIC_SPD mechanism. See CB:32604 for details
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 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
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. […]
Better yet: drop it altogether.
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 18: 0x0 0
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 26: 0x0 0
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 31: 0x0 0
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
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
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 51: subsystemid 0x8086 0x7270 This can be factored out
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?
This means that PEG is muxed in three: x16 ---> x8, x4, x4
It would be nice to comment what is connected to each of these.
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?
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.
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/
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
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 23: 0x0 This too
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!
If not using the VBIOS, you can drop the entire file.
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.c, and needs to be built for romstage and bootblock
https://review.coreboot.org/c/coreboot/+/32673/22/src/mainboard/apple/macboo... PS22, Line 64: DEVEN_PEG10 Note that, since the PEG port is muxed, disabling function 0 could break some stuff