Attention is currently required from: David Hendricks, Evgeny Zinoviev, Felix Singer, Martin L Roth, Paul Menzel, Renstals.
Angel Pons has posted comments on this change by Evgeny Zinoviev. ( https://review.coreboot.org/c/coreboot/+/32673?usp=email )
Change subject: mb/apple: Add MacBook Pro 10,1 (A1398) support ......................................................................
Patch Set 52:
(15 comments)
Patchset:
PS52:
Is there any reason not to merge this patch? It seems to add useful functionality and is self-contai […]
It hasn't been updated to the latest state of the codebase. I left a few comments but I am sure I missed something. Plus there's still old unresolved comments.
File src/mainboard/apple/macbookpro10_1/Kconfig:
PS52: Should have a license header
https://review.coreboot.org/c/coreboot/+/32673/comment/10c7eb0d_1541fb05?usp... : PS52, Line 22: config MAINBOARD_DIR : string : default "apple/macbookpro10_1" : : config MAINBOARD_PART_NUMBER : string : default "MacBookPro10,1" : : config VGA_BIOS_ID : string : default "8086,0166" : : config DRAM_RESET_GATE_GPIO : int : default 28 : : config USBDEBUG_HCD_INDEX : int : default 0 Types are redundant:
```suggestion config MAINBOARD_DIR default "apple/macbookpro10_1"
config MAINBOARD_PART_NUMBER default "MacBookPro10,1"
config VGA_BIOS_ID default "8086,0166"
config DRAM_RESET_GATE_GPIO default 28
config USBDEBUG_HCD_INDEX default 0 ```
https://review.coreboot.org/c/coreboot/+/32673/comment/b45223bb_8e4be2af?usp... : PS52, Line 42: config MAX_CPUS : int : default 8 Matches default value, please remove
File src/mainboard/apple/macbookpro10_1/Kconfig.name:
PS52: Should have a license header
File src/mainboard/apple/macbookpro10_1/acpi/superio.asl:
PS52: Should be licensed as CC-PDDC instead
File src/mainboard/apple/macbookpro10_1/cmos.layout:
https://review.coreboot.org/c/coreboot/+/32673/comment/e2936b51_621176d4?usp... : PS52, Line 27: 432 3 e 11 gfx_uma_size : 435 2 e 12 hybrid_graphics_mode I think `gfx_uma_size` should be 4 bits wide to support all possible values. You would also need to move the `hybrid_graphics_mode` option one bit.
```suggestion 432 4 e 11 gfx_uma_size 436 2 e 12 hybrid_graphics_mode ```
https://review.coreboot.org/c/coreboot/+/32673/comment/a27d7b87_4c7a3d6e?usp... : PS52, Line 42: #ID value text : 1 0 Disable : 1 1 Enable : 2 0 Enable : 2 1 Disable : 4 0 Fallback : 4 1 Normal : 6 0 Emergency : 6 1 Alert : 6 2 Critical : 6 3 Error : 6 4 Warning : 6 5 Notice : 6 6 Info : 6 7 Debug : 6 8 Spew : 7 0 Disable : 7 1 Enable : 7 2 Keep : 11 0 32M : 11 1 64M : 11 2 96M : 11 3 128M : 11 4 160M : 11 5 192M : 11 6 224M : 12 0 Integrated Only : 12 1 Discrete Only : 13 0 Normal : 13 1 Disabled Some of these enums are unused (e.g. ID 2). Would be good to remove unused ones.
Enum for iGPU memory seems to be missing a few values.
File src/mainboard/apple/macbookpro10_1/devicetree.cb:
PS52: Should have a license header
https://review.coreboot.org/c/coreboot/+/32673/comment/dded59f5_a9742cdb?usp... : PS52, Line 16: device cpu_cluster 0 on : chip cpu/intel/model_206ax : device lapic 0 on end : device lapic 0xacac off end : end : end : : device domain 0 on : subsystemid 0x106b 0x00f7 inherit : device pci 00.0 on end # Host bridge : device pci 01.0 on end # PCIe Bridge for discrete graphics : device pci 01.2 on end : device pci 01.1 on end : device pci 02.0 on end # Internal graphics VGA controller : chip southbridge/intel/bd82x6x # Intel Series 6 Cougar Point PCH : register "gen1_dec" = "0x000c0681" : register "gen2_dec" = "0x000c1641" : register "gen3_dec" = "0x001c0301" : register "gen4_dec" = "0x00fc0701" : register "gpi7_routing" = "2" : register "pcie_port_coalesce" = "1" : register "sata_interface_speed_support" = "0x3" : register "sata_port_map" = "0x1" : register "spi_lvscc" = "0x0" : register "spi_uvscc" = "0x2005" : register "superspeed_capable_ports" = "0x0000000f" : register "xhci_overcurrent_mapping" = "0x08040201" : register "xhci_switchable_ports" = "0x0000000f" : device pci 14.0 on end # USB 3.0 Controller : device pci 16.0 on end # Management Engine Interface 1 : device pci 16.1 off end # Management Engine Interface 2 : device pci 16.2 off end # Management Engine IDE-R : device pci 16.3 off end # Management Engine KT : device pci 19.0 off end # Intel Gigabit Ethernet : device pci 1a.0 on end # USB2 EHCI #2 : device pci 1b.0 on end # HD Audio controller : device pci 1c.0 on end # PCIe Port #1 : device pci 1c.1 on end # PCIe Port #2 : device pci 1c.2 off end # PCIe Port #3 : device pci 1c.3 off end # PCIe Port #4 : device pci 1c.4 off end # PCIe Port #5 : device pci 1c.5 off end # PCIe Port #6 : device pci 1c.6 off end # PCIe Port #7 : device pci 1c.7 off end # PCIe Port #8 : device pci 1d.0 on end # USB2 EHCI #1 : device pci 1e.0 off end # PCI bridge : device pci 1f.0 on # LPC bridge : chip drivers/apple/hybrid_graphics : device pnp ff.f on end # dummy : register "gmux_indexed" = "1" : end : end : device pci 1f.2 on end # SATA Controller 1 : device pci 1f.3 on end # SMBus : device pci 1f.5 off end # SATA Controller 2 : device pci 1f.6 off end # Thermal : end : end Should be updated to make use of chipset devicetree aliases.
File src/mainboard/apple/macbookpro10_1/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/32673/comment/e0f229da_de98c955?usp... : PS52, Line 10: // OEM revision Please remove this comment, it's autoport copypasta (which CB:82401 finally got rid of)
File src/mainboard/apple/macbookpro10_1/early_init.c:
https://review.coreboot.org/c/coreboot/+/32673/comment/717de5ae_df32ff15?usp... : PS52, Line 11: const struct southbridge_usb_port mainboard_usb_ports[] = { : { 1, 0, 0 }, /* Ext A (XHCI/EHCI) */ : { 1, 0, 1 }, /* Ext B (XHCI) */ : { 1, 0, 2 }, /* Ext C (XHCI/EHCI) */ : { 1, 0, 3 }, /* Ext D (XHCI) */ : { 0, 0, -1 }, /* Unused */ : { 1, 0, -1 }, /* SD */ : { 1, 0, -1 }, /* Wi-Fi */ : { 1, 0, -1 }, /* USB Hub (All LS/FS Devices) */ : { 1, 0, -1 }, /* Camera */ : { 1, 0, 4 }, /* Ext B (EHCI) */ : { 1, 0, 5 }, /* Ext D (EHCI) */ : { 1, 0, -1 }, /* BT */ : { 0, 0, -1 }, /* Unused */ : { 0, 0, -1 }, /* Unused */ : }; USB settings are now in devicetree
File src/mainboard/apple/macbookpro10_1/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/32673/comment/b8d5fa5f_b321ac4f?usp... : PS52, Line 8: nit: drop blank line?
File src/mainboard/apple/macbookpro10_1/spd/1g_samsung_1600.spd.hex:
PS52: nit: this is the only SPD file without a newline at the end, would be nice to add the newline for consistency
File src/mainboard/apple/macbookpro10_1/spd/2g_hynix_1600.spd.hex:
PS52: Not sure if SPD files need a license header, @service+coreboot-gerrit@felixsinger.de or @gaumless@gmail.com do you know?