Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37565 )
Change subject: [WIP] [Untested] Add Razer Blade Stealth IceLake Mercury White (late 2019) ......................................................................
Patch Set 3:
(22 comments)
Ice Lake /o\
https://review.coreboot.org/c/coreboot/+/37565/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37565/3//COMMIT_MSG@11 PS3, Line 11: SuperIO ITE website disagrees:
IT8320 : Highly Integrated Embedded Controller
Sure, ITE ECs also have a SuperIO-like interface, but they are ECs.
https://review.coreboot.org/c/coreboot/+/37565/3//COMMIT_MSG@13 PS3, Line 13: Two Only two LPDDR4 chips?
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... File src/mainboard/razer/blade_stealth_icl/Kconfig:
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 37: UART_FOR_CONSOLE Where is the UART routed to?
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... File src/mainboard/razer/blade_stealth_icl/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 22: CONFIG_SOC_INTEL_COMMON_BLOCK_HDA_VERB This is enforced by the mainboard selection.
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... File src/mainboard/razer/blade_stealth_icl/acpi/mainboard.asl:
PS3: Is this file needed, or is it a stub?
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... File src/mainboard/razer/blade_stealth_icl/acpi_tables.c:
PS3: There's a change dropping these empty files
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... File src/mainboard/razer/blade_stealth_icl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 39: Pch nit: PCH
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 164: Enable Really?
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 189: end Please align these `end`. That way, it looks prettier :D
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 298: end Another misalignment around here
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... File src/mainboard/razer/blade_stealth_icl/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 28: // Some generic macros I got rid of these comments recently! Please don't add them again
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 34: // CPU This comment doesn't really add any value, does it?
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 36: : Scope (_SB) { : Device (PCI0) Device (_SB.PCI0)
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 45: // Chipset specific sleep states I removed these comments as well. The sleepstates are no longer specific to each chipset.
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 49: #include "acpi/mainboard.asl" If you don't use that file, this can be dropped
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... File src/mainboard/razer/blade_stealth_icl/hda_verb.h:
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 22: /* ALC 700 */ : 0x10EC0700, 0xFFFFFFFF, 0x00000023, You can add comments to these values. Also, please use lowercase for hex constants. The third number is a length value, so it should be decimal:
0x10ec0700, /* Realtek ALC700 */ 0xffffffff, /* Subsystem ID */ 35, /* Number of four-dword sets */
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 25: AZALIA_PIN_CFG(0, 0x01, 0x00000000), Looks bogus. NID 0x01 is the subsystem ID, AFAIK, so it shouldn't have any pin_cfg macro
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 25: 0x10EC10F2 You can also put this value in place of the 0xffffffff above
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 26: AZALIA_PIN_CFG(0, 0x12, 0x40000000), AZALIA_PIN_CFG(0, 0x13, 0x40000000), : AZALIA_PIN_CFG(0, 0x14, 0x411111F0), AZALIA_PIN_CFG(0, 0x15, 0x411111F0), : AZALIA_PIN_CFG(0, 0x16, 0x411111F0), AZALIA_PIN_CFG(0, 0x17, 0x90170110), : AZALIA_PIN_CFG(0, 0x18, 0x411111F0), AZALIA_PIN_CFG(0, 0x19, 0x04A11030), : AZALIA_PIN_CFG(0, 0x1A, 0x411111F0), AZALIA_PIN_CFG(0, 0x1B, 0x411111F0), : AZALIA_PIN_CFG(0, 0x1D, 0x40622005), AZALIA_PIN_CFG(0, 0x1E, 0x411111F0), : AZALIA_PIN_CFG(0, 0x1F, 0x411111F0), AZALIA_PIN_CFG(0, 0x21, 0x04211020), : AZALIA_PIN_CFG(0, 0x29, 0x411111F0), Please put these on individual lines. Also, please use lowercase for the hex values
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 40: // Widget node 0X20 for ALC1305 20160603 update Are all of these hex spells applicable to the ALC700? If so, please group them in quadruples (groups of four). That way, one can easily check if there are actually 35 groups.
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... File src/mainboard/razer/blade_stealth_icl/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 15: #include "hda_verb.h" Why don't you just put the hda_verb.h contents here?
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... File src/mainboard/razer/blade_stealth_icl/romstage.c:
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 28: // Only one memory setup exists Please use consistent comment styles