Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37565 )
Change subject: [WIP] Add Razer Blade Stealth IceLake Mercury White (late 2019) ......................................................................
Patch Set 8:
(24 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/37565/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37565/2//COMMIT_MSG@7 PS2, Line 7: IceLake
thx
Done
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: […]
Done
https://review.coreboot.org/c/coreboot/+/37565/3//COMMIT_MSG@13 PS3, Line 13: Two
Only two LPDDR4 chips?
Done
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?
Done
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.
Done
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?
Done
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
Done
https://review.coreboot.org/c/coreboot/+/37565/2/src/mainboard/razer/blade_s... File src/mainboard/razer/blade_stealth_icl/board_info.txt:
https://review.coreboot.org/c/coreboot/+/37565/2/src/mainboard/razer/blade_s... PS2, Line 2: Board name: L
I had a contact @ razer who told me the codename. And i forgot.... […]
Done
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
Done
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 164: Enable
Really?
Done
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 189: end
Please align these `end`. […]
Done
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 298: end
Another misalignment around here
Done
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
Done
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?
Done
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 36: : Scope (_SB) { : Device (PCI0)
Device (_SB. […]
Done
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.
Done
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
Done
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. […]
Done
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
Done
https://review.coreboot.org/c/coreboot/+/37565/3/src/mainboard/razer/blade_s... PS3, Line 25: AZALIA_PIN_CFG(0, 0x01, 0x00000000),
Looks bogus. […]
Done
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. […]
Done
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 […]
Done
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. […]
Done
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
Done