Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34475 )
Change subject: Add Razer Blade Stealth (2016) H2U ......................................................................
Patch Set 33:
(9 comments)
https://review.coreboot.org/c/coreboot/+/34475/33//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34475/33//COMMIT_MSG@13 PS33, Line 13: memory in dualchannel mode
I'd say these lines are too long for a commit message, mind splitting them?
Done
https://review.coreboot.org/c/coreboot/+/34475/33/src/mainboard/razer/Kconfi... File src/mainboard/razer/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/34475/33/src/mainboard/razer/Kconfi... PS33, Line 2: Razer
please make it uppercase "RAZER" for consistency
Done
https://review.coreboot.org/c/coreboot/+/34475/33/src/mainboard/razer/blade_... File src/mainboard/razer/blade_stealth_kbl/Kconfig:
https://review.coreboot.org/c/coreboot/+/34475/33/src/mainboard/razer/blade_... PS33, Line 33: : : config MAINBOARD_VENDOR : string : default "RAZER"
Shouldn't need to redefine this
Done
https://review.coreboot.org/c/coreboot/+/34475/33/src/mainboard/razer/blade_... PS33, Line 59: config CPU_MICROCODE_CBFS_LEN : hex : default 0x18000 : : config CPU_MICROCODE_CBFS_LOC : hex : default 0xFFE115A0
Is it needed to specify the microcode length and location?
Done
https://review.coreboot.org/c/coreboot/+/34475/33/src/mainboard/razer/blade_... PS33, Line 22: # For now no way to choose the correct the available RAM : config BOARD_RAZER_BLADE_STEALTH_KBL_16GB : bool "16GB RAM (4x MT52L1G32D4PG)" : default n : : config VGA_BIOS_ID : string : default "8086,5916" : : config IRQ_SLOT_COUNT : int : default 18 : : config MAINBOARD_VENDOR : string : default "RAZER" : : config MAINBOARD_FAMILY : string : default "BLADE_STEALTH" : : config MAINBOARD_PART_NUMBER : string : default "H2U" : : config MAINBOARD_VERSION : string : default "1.0" : : config MAINBOARD_DIR : string : default "razer/blade_stealth_kbl" : : config MAX_CPUS : int : default 4 : : config CPU_MICROCODE_CBFS_LEN : hex : default 0x18000 : : config CPU_MICROCODE_CBFS_LOC : hex : default 0xFFE115A0 : : config CBFS_SIZE : hex : default 0x5c0000
These are usually not indented
Ack
https://review.coreboot.org/c/coreboot/+/34475/33/src/mainboard/razer/blade_... File src/mainboard/razer/blade_stealth_kbl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/34475/33/src/mainboard/razer/blade_... PS33, Line 39: register "SataPortsDevSlp[0]" = "0" : register "SataPortsDevSlp[2]" = "0" : register "SataSpeedLimit" = "2"
I don't think you need to set these with SATA ports disabled
Done
https://review.coreboot.org/c/coreboot/+/34475/33/src/mainboard/razer/blade_... PS33, Line 199: [
Is this used?
Done
https://review.coreboot.org/c/coreboot/+/34475/33/src/mainboard/razer/blade_... PS33, Line 216: d
Is this used?
Without it the display is not found. Not sure if this "best practice", but i can guess that this was an "okay" way by razer here.
https://review.coreboot.org/c/coreboot/+/34475/33/src/mainboard/razer/blade_... File src/mainboard/razer/blade_stealth_kbl/mainboard.c:
https://review.coreboot.org/c/coreboot/+/34475/33/src/mainboard/razer/blade_... PS33, Line 47: MAX_SERIAL_LENGTH);
Minor: fits on the previous line
Done