Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/28640 )
Change subject: mb/clevo/kbl-u: Add mainboard and variants N130WU/N131WU ......................................................................
Patch Set 114:
(19 comments)
https://review.coreboot.org/c/coreboot/+/28640/63//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/28640/63//COMMIT_MSG@7 PS63, Line 7: n130bwu
the x in "n130xu" can be seen as a variable ('B', 'W' or 'Z') for all the different model numbers un […]
Done
https://review.coreboot.org/c/coreboot/+/28640/103//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/28640/103//COMMIT_MSG@31 PS103, Line 31: - Documentation
There is documentation now.
*poke*
https://review.coreboot.org/c/coreboot/+/28640/101/Documentation/mainboard/c... File Documentation/mainboard/clevo/index.md:
PS101:
I'd remove this page, and put this directly on the mainboard index
*poke*
https://review.coreboot.org/c/coreboot/+/28640/37/Documentation/mainboard/cl... File Documentation/mainboard/clevo/n130wu/index.md:
https://review.coreboot.org/c/coreboot/+/28640/37/Documentation/mainboard/cl... PS37, Line 20: | Internal flashing | Yes |
I remember that the bios region is flashable, but I will recheck.
*poke*
https://review.coreboot.org/c/coreboot/+/28640/37/Documentation/mainboard/cl... PS37, Line 26:
Also here, my editor doesn't show this line to me
I don't think it's a big deal. I'd say nearly everything has a trailing blank line.
https://review.coreboot.org/c/coreboot/+/28640/103/Documentation/mainboard/c... File Documentation/mainboard/clevo/n130wu/index.md:
https://review.coreboot.org/c/coreboot/+/28640/103/Documentation/mainboard/c... PS103, Line 16: 8587
IT8587 (maybe IT8587E)
See latest patchset
https://review.coreboot.org/c/coreboot/+/28640/114/Documentation/mainboard/c... File Documentation/mainboard/clevo/n130wu/index.md:
https://review.coreboot.org/c/coreboot/+/28640/114/Documentation/mainboard/c... PS114, Line 16: ITE 8587 ITE IT8587E
https://review.coreboot.org/c/coreboot/+/28640/101/Documentation/mainboard/c... File Documentation/mainboard/clevo/n130wu/n130wu_overview.jpg:
PS101:
needs moar jpeg […]
*poke*
https://review.coreboot.org/c/coreboot/+/28640/103/Documentation/mainboard/c... File Documentation/mainboard/clevo/n130wu/n130wu_overview.jpg:
PS103:
It's too large, and blurry as well. […]
*poke*
https://review.coreboot.org/c/coreboot/+/28640/104/src/mainboard/clevo/kbl-u... File src/mainboard/clevo/kbl-u/Kconfig:
https://review.coreboot.org/c/coreboot/+/28640/104/src/mainboard/clevo/kbl-u... PS104, Line 2: # This file is part of the coreboot project.
The second line needs to be removed.
Done
https://review.coreboot.org/c/coreboot/+/28640/101/src/mainboard/clevo/kbl-u... File src/mainboard/clevo/kbl-u/variants/n13xwu/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/28640/101/src/mainboard/clevo/kbl-u... PS101, Line 2: Enable
Really?
Done
https://review.coreboot.org/c/coreboot/+/28640/103/src/mainboard/clevo/kbl-u... File src/mainboard/clevo/kbl-u/variants/n13xwu/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/28640/103/src/mainboard/clevo/kbl-u... PS103, Line 2: Enable
lies
Done
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... File src/mainboard/clevo/kbl-u/variants/n13xwu/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 44: # VR Settings Configuration for 4 Domains table doesn't match code
https://review.coreboot.org/c/coreboot/+/28640/114/src/mainboard/clevo/kbl-u... PS114, Line 127: subsystemid 0x1558 0x1313 Inherit it?
https://review.coreboot.org/c/coreboot/+/28640/63/src/mainboard/clevo/n130bw... File src/mainboard/clevo/n130bwu/Kconfig:
https://review.coreboot.org/c/coreboot/+/28640/63/src/mainboard/clevo/n130bw... PS63, Line 67: default 4 if BOARD_CLEVO_N130BU || BOARD_CLEVO_N131BU : default 8 if BOARD_CLEVO_N130WU || BOARD_CLEVO_N131WU
Are the CPU's not user removable/replaceable? I myself was thinking of upgrading to a quad-core cpu […]
The CPUs are soldered-on.
Anyway, done.
https://review.coreboot.org/c/coreboot/+/28640/69/src/mainboard/clevo/n130wu... File src/mainboard/clevo/n130wu/Kconfig:
https://review.coreboot.org/c/coreboot/+/28640/69/src/mainboard/clevo/n130wu... PS69, Line 29: if BOARD_CLEVO_N130WU || BOARD_CLEVO_N131WU
This isn't needed here or in any of the cases below because you're already inside the top level 'if' […]
More variants will be added
https://review.coreboot.org/c/coreboot/+/28640/69/src/mainboard/clevo/n130wu... PS69, Line 36: config DEVICETREE : string : default "variants/$(CONFIG_VARIANT_DIR)/devicetree.cb"
Last time I tried the override devicetree, I felt like it was too fragile for my use. […]
Ack
https://review.coreboot.org/c/coreboot/+/28640/44/src/mainboard/clevo/n130wu... File src/mainboard/clevo/n130wu/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/28640/44/src/mainboard/clevo/n130wu... PS44, Line 81: #| Psi2Threshold | 4A | 5A | 5A | 5A |
default seems to be 2. […]
*poke*
https://review.coreboot.org/c/coreboot/+/28640/44/src/mainboard/clevo/n130wu... PS44, Line 87: #| IccMax | 6A | 64A | 31A | 31A |
SA IccMax seems to be 5A in schematics […]
*poke*