Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38249 )
Change subject: src/mainboard: Port for Chuwi Minibook (m3/8GB) ......................................................................
Patch Set 3:
(13 comments)
https://review.coreboot.org/c/coreboot/+/38249/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38249/3//COMMIT_MSG@13 PS3, Line 13: Unknown soldered 8GB memory - SPD was extracted from BIOS image : (BIOS says it's Micron/2 ranks/13-15-15-34) Which type of memory is it?
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/Kconfig... File src/mainboard/chuwi/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/Kconfig... PS3, Line 2: CHUWI Innovation And Technology(ShenZhen)co.,Ltd I'd just use "CHUWI"
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... File src/mainboard/chuwi/minibook/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... PS3, Line 23: cbfs-files-y += ec.bin This is hardcoded, and shouldn't be
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... File src/mainboard/chuwi/minibook/acpi/ec.asl:
PS3: I would recommend checking the ACPI DSDT of the vendor firmware.
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... File src/mainboard/chuwi/minibook/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... PS3, Line 2: Enable I don't think so
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... PS3, Line 210: #0 Watch out for copypasta mistakes
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... File src/mainboard/chuwi/minibook/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... PS3, Line 24: "COREv4", // OEM id : "COREBOOT", // OEM table id I think there's definitions for these in arch/acpi.h
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... PS3, Line 28: //Platform This comment doesn't add any value
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... PS3, Line 34: // CPU Doesn't add anything of use either.
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... PS3, Line 37: Scope (_SB) { : Device (PCI0) Device (_SB.PCI0)
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... File src/mainboard/chuwi/minibook/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... PS3, Line 23: /* Number of entries */ Please align these comments with tabs
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... File src/mainboard/chuwi/minibook/mainboard.c:
PS3: Are you using this file?
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... File src/mainboard/chuwi/minibook/spd/spd_util.c:
https://review.coreboot.org/c/coreboot/+/38249/3/src/mainboard/chuwi/miniboo... PS3, Line 47: /* They are valid, probably */ Defaults are valid only for some types of memory.