Attention is currently required from: Evgeny Zinoviev, Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32604 )
Change subject: mb/apple: Add MacBook Air 5,2 (A1466) support ......................................................................
Patch Set 36:
(21 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/32604/comment/a8a52650_7e98082f PS36, Line 38: - Thunderbolt : The thunderbolt-to-ethernet adapter is detected by the kernel: : [ 835.495393] thunderbolt 0-1: new device found, vendor=0x1 : device=0x8003 : [ 835.495408] thunderbolt 0-1: Apple, Inc. Thunderbolt to Gigabit : Ethernet Adapter : The network interface wasn't created though, so I couldn't test the : adapter. No idea if it's linux or coreboot, but I think it's linux issue. Was the Thunderbolt-to-Ethernet adapter plugged in before booting, or was it plugged in afterwards? Would be interesting to check if coreboot detects the adapter.
In any case, Thunderbolt controllers need some initialisation. IIRC, some steps involve toggling some mainboard-specific GPIOs to reset the Thunderbolt controller.
File Documentation/mainboard/apple/macbookair5_2.md:
https://review.coreboot.org/c/coreboot/+/32604/comment/a56aab8c_b80c2594 PS36, Line 139: usb USB
File src/mainboard/apple/macbookair5_2/Kconfig:
https://review.coreboot.org/c/coreboot/+/32604/comment/309ae30c_cba52b95 PS36, Line 27: string Type not needed
File src/mainboard/apple/macbookair5_2/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/32604/comment/84a49a2d_912f2018 PS36, Line 11: gnvs->tcrt = 100; : gnvs->tpsv = 90; These GNVS values aren't used anywhere. So, these assignments are unnecessary.
File src/mainboard/apple/macbookair5_2/cmos.default:
https://review.coreboot.org/c/coreboot/+/32604/comment/96e87047_83f5902a PS36, Line 3: 32M I think macOS wants at least 64M here
https://review.coreboot.org/c/coreboot/+/32604/comment/5336f612_edec1c03 PS36, Line 4: me_state=Normal Missing defaults for `nmi`, `power_on_after_fail`
File src/mainboard/apple/macbookair5_2/cmos.layout:
https://review.coreboot.org/c/coreboot/+/32604/comment/8395e0a3_a013f9d1 PS36, Line 27: 432 3 e 11 gfx_uma_size After adding more enum values for `gfx_uma_size` (see comment below), its size needs to be increased to at least 5 bits:
432 5 e 11 gfx_uma_size
https://review.coreboot.org/c/coreboot/+/32604/comment/e75adf37_ceb1df2a PS36, Line 29: # SandyBridge MRC Scrambler Seed values : 896 32 r 0 mrc_scrambler_seed : 928 32 r 0 mrc_scrambler_seed_s3 : 960 16 r 0 mrc_scrambler_seed_chk This is only needed when using MRC.bin, which is not possible because this board selects `USE_NATIVE_RAMINIT`. So, this should be removed.
https://review.coreboot.org/c/coreboot/+/32604/comment/56304ef4_60e476bd PS36, Line 42: 2 0 Enable : 2 1 Disable This enumeration is unused, please drop
https://review.coreboot.org/c/coreboot/+/32604/comment/f18e7752_67349816 PS36, Line 58: 11 0 32M : 11 1 64M : 11 2 96M : 11 3 128M : 11 4 160M : 11 5 192M : 11 6 224M Add all possible choices for `gfx_uma_size`:
11 0 32M 11 1 64M 11 2 96M 11 3 128M 11 4 160M 11 5 192M 11 6 224M 11 7 256M 11 8 288M 11 9 320M 11 10 352M 11 11 384M 11 12 416M 11 13 448M 11 14 480M 11 15 512M 11 16 1024M
File src/mainboard/apple/macbookair5_2/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32604/comment/c8fe5a2b_2ee1c583 PS36, Line 2: register "gfx.ndid" = "2" This doesn't make sense if `gfx.did` is not specified.
https://review.coreboot.org/c/coreboot/+/32604/comment/4ea50304_da79815d PS36, Line 8: 1 PANEL_PORT_DP_A
https://review.coreboot.org/c/coreboot/+/32604/comment/bb7b5db0_d7d20fe3 PS36, Line 36: Intel Series 6 Cougar Point PCH This board uses a 7 series Panther Point PCH
https://review.coreboot.org/c/coreboot/+/32604/comment/b947d5ef_3e9e65d2 PS36, Line 42: register "pcie_port_coalesce" = "1" Coalescing isn't needed when the first PCH PCIe RP (`device pci 1c.0`) is enabled.
https://review.coreboot.org/c/coreboot/+/32604/comment/45e89ea6_7134ba85 PS36, Line 68: PCI-LPC bridge This part of the comment is redundant, I'd drop it.
File src/mainboard/apple/macbookair5_2/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/32604/comment/7c681850_c6747505 PS36, Line 10: // OEM revision I'd drop this comment
File src/mainboard/apple/macbookair5_2/early_init.c:
https://review.coreboot.org/c/coreboot/+/32604/comment/df95d34b_6f0fac2a PS32, Line 59: die("Unsupported memory, RAMCFG=%d\n", ramcfg);
I'd try to boot anyway. […]
No need to do this now.
File src/mainboard/apple/macbookair5_2/early_init.c:
https://review.coreboot.org/c/coreboot/+/32604/comment/de1bee97_98d37e9e PS36, Line 6: #include <southbridge/intel/common/gpio.h> Missing: #include <types.h>
Also, if you want, why not order the includes alphabetically?
https://review.coreboot.org/c/coreboot/+/32604/comment/38ab50cb_1c6e1ce8 PS36, Line 46: GPIO68 Looks like GPIO68 is always 0. Do you know if there's any configuration where it is 1?
https://review.coreboot.org/c/coreboot/+/32604/comment/5c03bf5a_afaa3166 PS36, Line 41: const int spd_gpio_vector[] = {71, 70, 69, 68, -1}; : int ramcfg = get_gpios(spd_gpio_vector); : int spd_index = -1; : : /* : * GPIO68 GPIO69 GPIO70 GPIO71 Memory Supported : * 0 0 0 0 4 GiB Hynix Yes : * 0 1 0 0 8 GiB Hynix No : * 0 0 1 0 4 GiB Samsung Yes : * 0 1 1 0 8 GiB Samsung No : * 0 1 1 1 8 GiB Elpida No : * 0 0 1 1 4 GiB Elpida No : */ : : switch (ramcfg) { : case 0: : spd_index = 0; : break; : case 2: : spd_index = 1; : break; : } : : if (spd_index == -1) : die("Unsupported memory, RAMCFG=%d\n", ramcfg); I'd put this into a helper function and make `spd_index` unsigned:
static unsigned int get_spd_index(void) { const int spd_gpio_vector[] = {71, 70, 69, 68, -1}; const unsigned int ramcfg = get_gpios(spd_gpio_vector);
/* * GPIO68 GPIO69 GPIO70 GPIO71 Memory Supported * 0 0 0 0 4 GiB Hynix Yes * 0 1 0 0 8 GiB Hynix No * 0 0 1 0 4 GiB Samsung Yes * 0 1 1 0 8 GiB Samsung No * 0 1 1 1 8 GiB Elpida No * 0 0 1 1 4 GiB Elpida No */ switch (ramcfg) { case 0: return 0; case 2: return 1; default: die("Unsupported memory, RAMCFG=%u\n", ramcfg); } }
File src/mainboard/apple/macbookair5_2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/32604/comment/f2bfe305_5c693bfd PS36, Line 9: GMA_INT15_PANEL_FIT_DEFAULT, : GMA_INT15_BOOT_DISPLAY_DEFAULT, 0); nit: Add another tab so that the arguments start on the same column