Attention is currently required from: Angel Pons, xin hua wang, Evgeny Zinoviev. Nicholas Chin 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 18:
(35 comments)
Patchset:
PS18: Rebased and fixed build issues and boot issues, and addressed some comments.
File Documentation/mainboard/apple/macbookpro8_1.md:
https://review.coreboot.org/c/coreboot/+/33151/comment/99f404d2_b0f73493 PS16, Line 16: 8MB
nit: 8 MiB
Done
File src/mainboard/apple/macbookpro8_1/Kconfig:
https://review.coreboot.org/c/coreboot/+/33151/comment/aeedb28c_626a9786 PS16, Line 16: select GFX_GMA_INTERNAL_IS_LVDS
Please drop
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/18826804_bec1f105 PS16, Line 21: string
Remove the type as per CB:56553
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/2e188af6_ad84564e PS16, Line 22: apple/macbookpro8_1
This now needs to be surrounded in double quotes
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/08b8395a_332e76e8 PS16, Line 25: string
Remove the type as per CB:56554
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/d05f799d_7354c630 PS16, Line 40: config MAX_CPUS : int : default 8
Remove as per CB:41839
Done
File src/mainboard/apple/macbookpro8_1/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/33151/comment/e1f71252_600f157a PS16, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright (c) 2019-2020 Evgeny Zinoviev me@ch1p.io : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */
Please use SPDX license headers
Done
File src/mainboard/apple/macbookpro8_1/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/33151/comment/2f25b6f2_f444b4ee PS16, Line 1: /* : * This file is part of the coreboot project. : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */
Please use SPDX license headers
Done
File src/mainboard/apple/macbookpro8_1/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/33151/comment/d0f058e0_4d21a5af PS16, Line 18: #include <southbridge/intel/bd82x6x/nvs.h>
Needs update
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/d4a125b3_35afdc24 PS16, Line 20: global_nvs_t
Needs update
Done
File src/mainboard/apple/macbookpro8_1/cmos.default:
https://review.coreboot.org/c/coreboot/+/33151/comment/af8ca20a_af65e819 PS16, Line 3: Enable
`Disable` would make more sense
Done
File src/mainboard/apple/macbookpro8_1/cmos.layout:
PS16:
Please indent with tabs.
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/836212ee_ffa0d443 PS16, Line 1: ## : ## This file is part of the coreboot project. : ## : ## This program is free software; you can redistribute it and/or modify : ## it under the terms of the GNU General Public License as published by : ## the Free Software Foundation; version 2 of the License. : ## : ## This program is distributed in the hope that it will be useful, : ## but WITHOUT ANY WARRANTY; without even the implied warranty of : ## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : ## GNU General Public License for more details. : ##
Please use SPDX license headers
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/9f973963_52eaafa0 PS16, Line 17: # ----------------------------------------------------------------- : # Status Register A : # ----------------------------------------------------------------- : # Status Register B : # ----------------------------------------------------------------- : # Status Register C : #96 4 r 0 status_c_rsvd : #100 1 r 0 uf_flag : #101 1 r 0 af_flag : #102 1 r 0 pf_flag : #103 1 r 0 irqf_flag : # ----------------------------------------------------------------- : # Status Register D : #104 7 r 0 status_d_rsvd : #111 1 r 0 valid_cmos_ram : # ----------------------------------------------------------------- : # Diagnostic Status Register : #112 8 r 0 diag_rsvd1
Please remove
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/f348df98_781b985e PS16, Line 38: #120 264 r 0 unused
Please remove
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/8f43881d_19372fd2 PS16, Line 44: #390 2 r 0 unused?
Please remove
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/e6c75c67_16295a28 PS16, Line 48: #392 3 r 0 unused
Please remove
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/ceef2756_398124a7 PS16, Line 50: #399 1 r 0 unused
Please remove
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/bd5ccd1f_21427fd6 PS16, Line 58: # 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
Only used with MRC. […]
Done
File src/mainboard/apple/macbookpro8_1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33151/comment/484bf661_7127886e PS16, Line 8: 0
PANEL_PORT_LVDS
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/62075d2d_2f9c3b14 PS16, Line 18: register "c1_acpower" = "1" : register "c1_battery" = "1" : register "c2_acpower" = "3" : register "c2_battery" = "3" : register "c3_acpower" = "5" : register "c3_battery" = "5"
Needs to be updated: […]
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/72e10e40_72b635e0 PS16, Line 32: subsystemid 0x8086 0x7270 inherit
I think you can move this inside the southbridge chip, put `subsystemid 0x106b 0x00db inherit` here […]
I tried that, putting `subsystemid 0x8086 0x7270 inherit` inside the `chip southbrige` section, but the build system complains about that being a syntax error. If that line was moved right before the chip section, would it override the `subsystem 0x106b 0x00db inherit` line that you suggested and work as intended?
https://review.coreboot.org/c/coreboot/+/33151/comment/95731414_cca96691 PS16, Line 46: register "c2_latency" = "0x0065"
Remove as per CB:55212
Done
File src/mainboard/apple/macbookpro8_1/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/33151/comment/e67c9113_daff46b1 PS16, Line 1: /* : * This file is part of the coreboot project. : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */
Please use SPDX license headers
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/951704ff_da7e9255 PS16, Line 18: 0x02, // DSDT revision: ACPI 2.0 and up
ACPI_DSDT_REV_2,
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/c6f21652_1f2f68a9 PS16, Line 21: // OEM revision
autoport nonsense, please remove
Done
File src/mainboard/apple/macbookpro8_1/early_init.c:
https://review.coreboot.org/c/coreboot/+/33151/comment/150abe05_ba1fbafc PS16, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright (C) 2008-2009 coresystems GmbH : * Copyright (C) 2014 Vladimir Serbinenko : * Copyright (C) 2019-2020 Evgeny Zinoviev me@ch1p.io : * : * This program is free software; you can redistribute it and/or : * modify it under the terms of the GNU General Public License as : * published by the Free Software Foundation; version 2 of : * the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */
Please use SPDX license headers
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/18eec6f0_9646d17c PS16, Line 39: void mainboard_early_init(int s3resume) : { : }
Can be removed
Done
File src/mainboard/apple/macbookpro8_1/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/33151/comment/f35e4277_1889e628 PS11, Line 30: Analog
You only need to keep Internal and the DP/HDMI groups, as per schematics.
Done
File src/mainboard/apple/macbookpro8_1/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/33151/comment/4f646aff_13ee9d22 PS16, Line 1: -- : -- This file is part of the coreboot project. : -- : -- This program is free software; you can redistribute it and/or modify : -- it under the terms of the GNU General Public License as published by : -- the Free Software Foundation; either version 2 of the License, or : -- (at your option) any later version. : -- : -- This program is distributed in the hope that it will be useful, : -- but WITHOUT ANY WARRANTY; without even the implied warranty of : -- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : -- GNU General Public License for more details. : --
Please use SPDX license headers
Done
https://review.coreboot.org/c/coreboot/+/33151/comment/fd0df1a9_705ec149 PS16, Line 31: Internal
LVDS
Done
File src/mainboard/apple/macbookpro8_1/gpio.c:
https://review.coreboot.org/c/coreboot/+/33151/comment/e6f043bf_a6a9124e PS16, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright (C) 2008-2009 coresystems GmbH : * Copyright (C) 2014 Vladimir Serbinenko : * : * This program is free software; you can redistribute it and/or : * modify it under the terms of the GNU General Public License as : * published by the Free Software Foundation; version 2 of : * the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */
Please use SPDX license headers
Done
File src/mainboard/apple/macbookpro8_1/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/33151/comment/e3e2805d_cc639973 PS8, Line 21: Cirrus
Please expand the comment, thanks
Done
File src/mainboard/apple/macbookpro8_1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33151/comment/4d046827_c34bb0af PS16, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright (C) 2019 Evgeny Zinoviev me@ch1p.io : * : * This program is free software; you can redistribute it and/or modify : * it under the terms of the GNU General Public License as published by : * the Free Software Foundation; version 2 of the License. : * : * This program is distributed in the hope that it will be useful, : * but WITHOUT ANY WARRANTY; without even the implied warranty of : * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the : * GNU General Public License for more details. : */
Please use SPDX license headers
Done