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 12:
(8 comments)
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro8_1/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 23: /* 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, as they are zero already.
Done
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro8_1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 16: 0x0
These can be simply zero
Done
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 31: 0x0
This one as well
Done
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 37: subsystemid 0x106b 0x00db
If it's off, does it need a subsystemid?
Done
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 60: Audio Audio
Is it stereo audio? I would shorten this as "HD Audio Controller"
Of course it's stereo!
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro8_1/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 12: : : #define BRIGHTNESS_UP _SB.PCI0.GFX0.INCB : #define BRIGHTNESS_DOWN _SB.PCI0.GFX0.DECB : #define ACPI_VIDEO_DEVICE _SB.PCI0.GFX0
Are you using these?
Done
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro8_1/early_init.c:
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 18:
Are you using all these headers?
Done
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... File src/mainboard/apple/macbookpro8_1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33151/11/src/mainboard/apple/macboo... PS11, Line 24: RCBA32(0x38c8) = 0x00002005; : RCBA32(0x38c4) = 0x00800000;
Aren't these doing the same as the VSCC settings in the devicetree?
You're right.