[coreboot-gerrit] Change in coreboot[master]: mainboard: add support for lenovo x1 carbon gen 1

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/16994 )

Change subject: mainboard: add support for lenovo x1 carbon gen 1
......................................................................


Patch Set 4:

(7 comments)

A few nits and suggestions.

https://review.coreboot.org/#/c/16994/4//COMMIT_MSG
Commit Message:

PS4, Line 15: -
maybe use a different summation symbol like * to make indentation more clear.


PS4, Line 21: native graphics init works but is somehow broken before linux (because of dts value?)
maybe try libgfxinit? Should be in current master of X230, which can probably simply be copied.


PS4, Line 22: check why device consumes more power (+ 3W while idle)
Maybe c-state related? check if linux uses cpuidle (Using ACPI) or intel_idle (native) to manage c-states. If the former some more optimised c-states can be used?


https://review.coreboot.org/#/c/16994/4/src/mainboard/lenovo/x1_carbon_gen1/gpio.c
File src/mainboard/lenovo/x1_carbon_gen1/gpio.c:

PS4, Line 67: .gpio9  = GPIO_DIR_INPUT,
Not meaningful when mode is native. You could declare it static and leave those GPIO out.


PS4, Line 100: .gpio7  = GPIO_LEVEL_HIGH,
read only on input. could be left out.


PS4, Line 127: const struct pch_gpio_set1 pch_gpio_set1_invert = {
Should this not be declared as static so empty field are initialised?


https://review.coreboot.org/#/c/16994/4/src/mainboard/lenovo/x1_carbon_gen1/spd/elpida.hex
File src/mainboard/lenovo/x1_carbon_gen1/spd/elpida.hex:

Line 1: 92 11 0b 03 04 00 00 01 03 52 01 08 0c 00 20 00 
Out of pure interest: how do you come by this?


-- 
To view, visit https://review.coreboot.org/16994
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I97c8e01a3ce0577d7dc9e8df7d33db3b155fe3d6
Gerrit-PatchSet: 4
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Alexander Couzens <lynxis at fe80.eu>
Gerrit-Reviewer: Alexander Couzens <lynxis at fe80.eu>
Gerrit-Reviewer: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes