Evgeny Zinoviev 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 11:
(31 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: […]
Done
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
Done
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
Done
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
Done
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 94: 2 0 Enable : 2 1 Disable
Unused
Done
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
Done
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)
Done
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 35: device domain 0x0 on
Right after this line, add: […]
Done
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 38: register "docking_supported" = "0"
Can drop this
Done
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
Done
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
Done
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 60: Audio controller
You can drop this redundant part
Done
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
Done
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 81: PCI-LPC bridge
This can be dropped too
Done
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 93: Host bridge
Redundant
Done
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: […]
Done
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 102: device pci 01.1 on
What's this?
Was added by autoport. Usually discrete graphics. Removed.
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 105: device pci 1a.7 on
What's this?
No idea. I don't see any issues after removing.
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 108: device pci 1d.7 on
What's this?
Same.
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
Done
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... […]
Yes.
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 24: 0x0000000b
Replace with `12`. […]
But 0b is 11.
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 25: /* NID 0x01: Subsystem ID. */
You can drop these comments
Done
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
Done
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 29: 0x0
Same for all of these
Done
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
Done
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 60: 0x00000004
Same as above
Done
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 62: 0x3
Same as above
Done
https://review.coreboot.org/c/coreboot/+/33151/8/src/mainboard/apple/macbook... PS8, Line 65: 0x3
Same as above
Done
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. […]
Done
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
I guess it can be removed now.