Attention is currently required from: Patrick Georgi, xin hua wang, Evgeny Zinoviev. 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 16: Code-Review+1
(45 comments)
File Documentation/mainboard/apple/macbookpro8_1.md:
https://review.coreboot.org/c/coreboot/+/33151/comment/5b07654c_8f297c5f PS16, Line 16: 8MB nit: 8 MiB
https://review.coreboot.org/c/coreboot/+/33151/comment/93e1fe5c_cb9ed17e PS16, Line 55: ## Untested I imagine macOS is untested?
https://review.coreboot.org/c/coreboot/+/33151/comment/9881afd0_74553a57 PS16, Line 57: Thunderbolt Probably has issues because the thunderbolt controller needs some initialisation
File src/mainboard/apple/macbookpro8_1/Kconfig:
https://review.coreboot.org/c/coreboot/+/33151/comment/e688d9ec_ff58649f PS16, Line 16: select GFX_GMA_INTERNAL_IS_LVDS Please drop
https://review.coreboot.org/c/coreboot/+/33151/comment/e473d954_b4df4400 PS16, Line 21: string Remove the type as per CB:56553
https://review.coreboot.org/c/coreboot/+/33151/comment/c99d8b0a_2669b5aa PS16, Line 22: apple/macbookpro8_1 This now needs to be surrounded in double quotes
https://review.coreboot.org/c/coreboot/+/33151/comment/ee6fd475_8198b612 PS16, Line 25: string Remove the type as per CB:56554
https://review.coreboot.org/c/coreboot/+/33151/comment/556de4d9_46020b36 PS16, Line 38: default 28 Um, gpio.c configures this as an input.
https://review.coreboot.org/c/coreboot/+/33151/comment/76421bfd_bac7c85b PS16, Line 40: config MAX_CPUS : int : default 8 Remove as per CB:41839
File src/mainboard/apple/macbookpro8_1/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/33151/comment/e036c12c_608632ae 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
File src/mainboard/apple/macbookpro8_1/acpi/platform.asl:
https://review.coreboot.org/c/coreboot/+/33151/comment/ebccee3c_34d376b8 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
File src/mainboard/apple/macbookpro8_1/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/33151/comment/dc625150_b96a5c18 PS8, Line 31: //
Make this comment a C-style comment, so that it is consistent with the other comments
Done
File src/mainboard/apple/macbookpro8_1/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/33151/comment/75f28bcf_547ec264 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
https://review.coreboot.org/c/coreboot/+/33151/comment/aa8ef648_15047b1b PS16, Line 18: #include <southbridge/intel/bd82x6x/nvs.h> Needs update
https://review.coreboot.org/c/coreboot/+/33151/comment/7aba4edc_30ecf804 PS16, Line 20: global_nvs_t Needs update
https://review.coreboot.org/c/coreboot/+/33151/comment/3089f177_5e597011 PS16, Line 25: gnvs->tcrt = 100; : gnvs->tpsv = 90; I don't think these are needed.
File src/mainboard/apple/macbookpro8_1/cmos.default:
https://review.coreboot.org/c/coreboot/+/33151/comment/e8e11389_9c8b92cc PS16, Line 3: Enable `Disable` would make more sense
File src/mainboard/apple/macbookpro8_1/cmos.layout:
PS16: Please indent with tabs.
https://review.coreboot.org/c/coreboot/+/33151/comment/0a332e65_fbb438cc 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
https://review.coreboot.org/c/coreboot/+/33151/comment/ee37bb44_c5b09498 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
https://review.coreboot.org/c/coreboot/+/33151/comment/572d5be9_f0bf280b PS16, Line 38: #120 264 r 0 unused Please remove
https://review.coreboot.org/c/coreboot/+/33151/comment/1228646e_1ee0e7ef PS16, Line 44: #390 2 r 0 unused? Please remove
https://review.coreboot.org/c/coreboot/+/33151/comment/5899c08f_c5e00fff PS16, Line 46: # ----------------------------------------------------------------- Where are the CMOS options for `gfx_uma_size`, among other things?
https://review.coreboot.org/c/coreboot/+/33151/comment/6eadc33f_4e370443 PS16, Line 48: #392 3 r 0 unused Please remove
https://review.coreboot.org/c/coreboot/+/33151/comment/4decbf24_a3c5acf9 PS16, Line 50: #399 1 r 0 unused Please remove
https://review.coreboot.org/c/coreboot/+/33151/comment/289da6af_93777109 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.bin
File src/mainboard/apple/macbookpro8_1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33151/comment/e568732d_f01c3ba1 PS8, Line 105: device pci 1a.7 on Device gone.
this is the find_debugport for ft232h logging but the port is not the ones on the side. does anyone have the schematics to see if there is test points that one can tap into to access this? thanks
Please don't derail comment threads. You could've asked your question in a new comment thread.
File src/mainboard/apple/macbookpro8_1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33151/comment/58dea001_b9a64099 PS16, Line 8: 0 PANEL_PORT_LVDS
https://review.coreboot.org/c/coreboot/+/33151/comment/02dec7c9_9a41b200 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:
register "acpi_c1" = "1" register "acpi_c2" = "3" register "acpi_c3" = "5"
https://review.coreboot.org/c/coreboot/+/33151/comment/3defb3c2_d47deda5 PS16, Line 32: subsystemid 0x8086 0x7270 inherit I think you can move this inside the southbridge chip, put `subsystemid 0x106b 0x00db inherit` here and drop all `subsystemid 0x106b 0x00db` lines.
https://review.coreboot.org/c/coreboot/+/33151/comment/c4f0836e_c378449e PS16, Line 36: device pci 01.0 on : subsystemid 0x106b 0x00db : end Is there anything connected here? Or is this only on so that the thunderbolt root port function is discoverable? (multifunction PCI devices must implement function 0).
https://review.coreboot.org/c/coreboot/+/33151/comment/597acf1b_f3b9b591 PS16, Line 46: register "c2_latency" = "0x0065" Remove as per CB:55212
https://review.coreboot.org/c/coreboot/+/33151/comment/a150e0f0_38ac36eb PS16, Line 52: register "pcie_port_coalesce" = "1" Doesn't seem to be needed
https://review.coreboot.org/c/coreboot/+/33151/comment/146a4844_538f254f PS16, Line 74: device pci 1f.0 on end # LPC bridge No EC chip?
File src/mainboard/apple/macbookpro8_1/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/33151/comment/966417d4_0248d253 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
https://review.coreboot.org/c/coreboot/+/33151/comment/14d7b006_fe3f8060 PS16, Line 18: 0x02, // DSDT revision: ACPI 2.0 and up ACPI_DSDT_REV_2,
https://review.coreboot.org/c/coreboot/+/33151/comment/40e4ea15_3915fa16 PS16, Line 21: // OEM revision autoport nonsense, please remove
File src/mainboard/apple/macbookpro8_1/early_init.c:
https://review.coreboot.org/c/coreboot/+/33151/comment/115244dc_ce0f2a48 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
https://review.coreboot.org/c/coreboot/+/33151/comment/9568893d_56e1d5c1 PS16, Line 39: void mainboard_early_init(int s3resume) : { : } Can be removed
File src/mainboard/apple/macbookpro8_1/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/33151/comment/65769f1b_de98e115 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
https://review.coreboot.org/c/coreboot/+/33151/comment/90feb8cf_3e3cb036 PS16, Line 31: Internal LVDS
File src/mainboard/apple/macbookpro8_1/gpio.c:
https://review.coreboot.org/c/coreboot/+/33151/comment/b2a30f61_4cdd3837 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
File src/mainboard/apple/macbookpro8_1/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/33151/comment/c7327606_76cc9ba5 PS8, Line 21: Cirrus
Yes.
Please expand the comment, thanks
File src/mainboard/apple/macbookpro8_1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33151/comment/3dbf569f_73d9416f PS8, Line 24: RCBA32(0x38c8) = 0x00002005;
What does this do?
Is gone
File src/mainboard/apple/macbookpro8_1/mainboard.c:
https://review.coreboot.org/c/coreboot/+/33151/comment/26012212_f5e7ce57 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