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 23:
(33 comments)
https://review.coreboot.org/c/coreboot/+/32604/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/32604/5//COMMIT_MSG@12 PS5, Line 12: - Linux : - GRUB, SeaBIOS
Devuan ascii, kernel 4.9.0 […]
Done
https://review.coreboot.org/c/coreboot/+/32604/1/src/mainboard/apple/macbook... File src/mainboard/apple/macbookair5_2/Kconfig:
https://review.coreboot.org/c/coreboot/+/32604/1/src/mainboard/apple/macbook... PS1, Line 45: 60
Done
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/Kconfig:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 31: default "pci8086,0166.rom" Is the VBIOS tested?
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 2: : cbfs-files-y += spd.bin : spd.bin-file := spd.bin : spd.bin-type := spd I think you can use GENERIC_SPD_BIN. see src/mb/hp/snb_ivb_laptops as an example (the revolve 810 uses that mechanism)
Note that it uses plaintext SPD files, so you may have to xxd your file
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, 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 can be dropped
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 28: // the lid is open by default. Please use C-style comments for consistency
https://review.coreboot.org/c/coreboot/+/32604/1/src/mainboard/apple/macbook... File src/mainboard/apple/macbookair5_2/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32604/1/src/mainboard/apple/macbook... PS1, Line 33: :
Add this line here: […]
Done
https://review.coreboot.org/c/coreboot/+/32604/1/src/mainboard/apple/macbook... PS1, Line 110: Host bridge
Duplicate comment
Done
https://review.coreboot.org/c/coreboot/+/32604/1/src/mainboard/apple/macbook... PS1, Line 110: device pci 00.0 on # Host bridge Host bridge : subsystemid 0x106b 0x00fe : end : device pci 01.0 off # PCIe Bridge for discrete graphics : end : device pci 02.0 on # Internal graphics VGA controller : subsystemid 0x106b 0x00fe : end
Please move above the "chip southbridge/intel/bd82x6x" line
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 2: register "gfx.did" = "{ 0x80000410, 0x80000320, 0x80000410, 0x80000410, 0x00000005 }" This can be dropped
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 19: 0x0 0
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 27: 0x0 0
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 32: 0x0 0
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 44: register "docking_supported" = "0" is zero, can be dropped
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 50: register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }" is zero, can be dropped
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 66: High Definition Audio Audio Stereo audio again :^)
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 12: : : #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/+/32604/23/src/mainboard/apple/macboo... PS23, Line 28: /* Some generic macros */ Please remove this comment. I killed it on a recent change.
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 30: : #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> Some of these are now under common
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 30: Analog This one is likely to be unused
https://review.coreboot.org/c/coreboot/+/32604/1/src/mainboard/apple/macbook... File src/mainboard/apple/macbookair5_2/gnvs.c:
PS1:
This file is a duplicate of acpi_tables.c […]
See current patchset
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/gnvs.c:
PS23: This file is a duplicate of acpi_tables.c
It doesn't even get built. Please remove.
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 21: Cirrus Cirrus CS4206
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 24: 0x0000000b Use decimal here
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 25: /* NID 0x01: Subsystem ID. */ All of the "NID" comments are of no use, they can be dropped
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 26: 0x0 Use decimal here as well
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 57: 0x80862806, /* Codec Vendor / Device ID: Intel */ Add a space between the two codecs
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 60: 0x00000004 Also decimal
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 62: 0x3 Also decimal
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 75: 0x00270500, /* Set PS to D0 on DAC1 */
please, no spaces at the start of a line
Please correct this
https://review.coreboot.org/c/coreboot/+/32604/1/src/mainboard/apple/macbook... File src/mainboard/apple/macbookair5_2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/32604/1/src/mainboard/apple/macbook... PS1, Line 62: GMA_INT15_ACTIVE_LFP_EDP
Confirmed, it's eDP. Please format this line properly, though.
Ack
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/mainboard.c:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 29: install_intel_vga_int15_handler(GMA_INT15_ACTIVE_LFP_EDP, : GMA_INT15_PANEL_FIT_DEFAULT, : GMA_INT15_BOOT_DISPLAY_DEFAULT, 0); Is the VBIOS tested?
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/romstage.c:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 73: memcpy(&spd[0], spd_file, 128); Why is only half of the spd file used? Only the 1st and 3rd quarters are used.