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 8:
(33 comments)
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... File src/mainboard/apple/macbookpro8_1/Makefile.inc:
PS8: You would want this to build with current master:
bootblock-y += early_init.c bootblock-y += gpio.c
romstage-y += early_init.c romstage-y += gpio.c
ramstage-$(CONFIG_MAINBOARD_USE_LIBGFXINIT) += gma-mainboard.ads
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... File src/mainboard/apple/macbookpro8_1/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 31: // Make this comment a C-style comment, so that it is consistent with the other comments
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... File src/mainboard/apple/macbookpro8_1/cmos.layout:
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 55: 408 1 e 1 nmi Doesn't have a default
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 56: 409 2 e 7 power_on_after_fail Doesn't have a default
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 58: # coreboot config options: EC : #411 1 e 8 first_battery : #412 1 e 1 bluetooth : #413 1 e 1 wwan : #414 1 e 1 touchpad : #415 1 e 1 wlan : #416 1 e 1 trackpoint : #417 1 e 1 fn_ctrl_swap : #418 1 e 1 sticky_fn : #419 2 e 13 usb_always_on : #421 1 e 9 sata_mode : #422 2 e 10 backlight : : # coreboot config options: cpu : #424 8 r 0 unused : : # coreboot config options: northbridge : #432 5 e 11 gfx_uma_size : #437 3 r 0 unused : #440 8 h 0 volume This is all commented-out
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 94: 2 0 Enable : 2 1 Disable Unused
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... File src/mainboard/apple/macbookpro8_1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 1: FIXME: check gfx.ndid and gfx.did You might as well drop them
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 27: end Please "pick up" these `end` (put them on the previous line)
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 35: device domain 0x0 on Right after this line, add:
subsystemid 0x8086 0x7270 inherit
Then drop the redundant subsystemid entries.
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 38: register "docking_supported" = "0" Can drop this
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 44: register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }" Can drop this
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 57: Unsupported PCI device 8086:1c2c I think you can drop this
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 60: Audio controller You can drop this redundant part
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 77: Unsupported PCI device 8086:1c27 I think you can drop this
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 81: PCI-LPC bridge This can be dropped too
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 93: Host bridge Redundant
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 93: device pci 00.0 on # Host bridge Host bridge : subsystemid 0x106b 0x00db : end : device pci 01.0 off # PCIe Bridge for discrete graphics : subsystemid 0x106b 0x00db : end : device pci 02.0 on # Internal graphics VGA controller : subsystemid 0x106b 0x00db : end This should go right after this line:
subsystemid 0x8086 0x7270 inherit
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 102: device pci 01.1 on What's this?
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 105: device pci 1a.7 on What's this?
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 108: device pci 1d.7 on What's this?
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... File src/mainboard/apple/macbookpro8_1/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 28: /* Some generic macros */ Please drop this comment
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... File src/mainboard/apple/macbookpro8_1/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 21: Cirrus Cirrus... CS4206?
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 24: 0x0000000b Replace with `12`.
Also, align the comments with tabs
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 25: /* NID 0x01: Subsystem ID. */ You can drop these comments
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 26: 0x0 Refers to codec number 0, so it should be decimal
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 29: 0x0 Same for all of these
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 57: 0x80862805, /* Codec Vendor / Device ID: Intel */ You might want to add some spacing between the two codecs
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 60: 0x00000004 Same as above
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 62: 0x3 Same as above
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 65: 0x3 Same as above
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... File src/mainboard/apple/macbookpro8_1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 24: RCBA32(0x38c8) = 0x00002005; What does this do?
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... File src/mainboard/apple/macbookpro8_1/romstage.c:
PS8: This was renamed to early_init.c
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 30: { I would use the already-existing macros to do this