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

[Alexander Couzens (Code Review)]

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

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


Patch Set 8:

(14 comments)

https://review.coreboot.org/#/c/16994/8/src/mainboard/lenovo/x1_carbon_gen1/Kconfig
File src/mainboard/lenovo/x1_carbon_gen1/Kconfig:

PS8, Line 25: Workaround
> Why call it a workaround?
no idea, used the same from x230.
Will write it on my todo list


PS8, Line 42: c
> It's usually spelled upper case "Carbon".
fixed


Line 46: 	default 0xf8000000
> What does this override fix? Why is it not the chipset default?
I'll try to use the default


https://review.coreboot.org/#/c/16994/8/src/mainboard/lenovo/x1_carbon_gen1/acpi/ec.asl
File src/mainboard/lenovo/x1_carbon_gen1/acpi/ec.asl:

PS8, Line 18: 
            : Scope(\_SB.PCI0.LPCB.EC)
            : {
            : }
> Isn't this just dropped by the compiler?
fixed


https://review.coreboot.org/#/c/16994/8/src/mainboard/lenovo/x1_carbon_gen1/board_info.txt
File src/mainboard/lenovo/x1_carbon_gen1/board_info.txt:

Line 5: Flashrom support: n
> I wonder what "Flashrom support" actually means? There are many
no idea either, flashrom is working with force & brick it


https://review.coreboot.org/#/c/16994/8/src/mainboard/lenovo/x1_carbon_gen1/devicetree.cb
File src/mainboard/lenovo/x1_carbon_gen1/devicetree.cb:

PS8, Line 2: 0x80000410, 0x00000005
> No use to specify five values if `ndid == 3`.
oh. I didn't know, they are related in that way. so ndid = 5 would be the correct thing?


PS8, Line 6: 1155
> Why start at full brightness? Is the panel that dark?
what would you suggest?


PS8, Line 16: 1155
> Is this value verified?
how can I do that?


Line 18: 	# Override fuse bits that hard-code the value to 666 Mhz
> Why?
I need to verify this, if this is also needed, because of the soldered memory


PS8, Line 29: 			register "c1_acpower" = "1"	# ACPI(C1) = MWAIT(C1)
            : 			register "c2_acpower" = "3"	# ACPI(C2) = MWAIT(C3)
            : 			register "c3_acpower" = "5"	# ACPI(C3) = MWAIT(C7)
            : 
            : 			register "c1_battery" = "1"	# ACPI(C1) = MWAIT(C1)
            : 			register "c2_battery" = "3"	# ACPI(C2) = MWAIT(C3)
            : 			register "c3_battery" = "5"	# ACPI(C3) = MWAIT(C7)
> If you don't want to keep the comments in sync, just drop them.
will fix this.


Line 66: 			register "gen3_dec" = "0x000000"
> Why the gap?
What do you mean with gap?


Line 69: 			register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }"
> `0` is the default.
ok, I'll drop it.


https://review.coreboot.org/#/c/16994/8/src/mainboard/lenovo/x1_carbon_gen1/romstage.c
File src/mainboard/lenovo/x1_carbon_gen1/romstage.c:

PS8, Line 40: e_lpc(void)
            : {
            : 	/* X230 EC Decode Range Port60/64, Port62/66 */
            : 	/* Enable EC, PS/2 Keyboard/Mouse */
            : 	pci_write_config16(PCH_LPC_DEV, LPC_EN,
            : 			   CNF2_LPC_EN | CNF1_LPC_EN | MC_LPC_EN | KBC_LPC_EN |
            : 			   COMA_LPC_EN);
            : 
            : 	pci_write_config32(PCH_LPC_DEV, LPC_GEN1_DEC, 0x7c1601);
            : 	pci_write_config32(PCH_LPC_DEV, LPC_GEN2_DEC, 0xc15e1);
            : 	pci_write_config32(PCH_LPC_DEV, LPC_GEN4_DEC, 0x0c06a1);
            : 
            : 	pci_write_config16(PCH_LPC_DEV, LPC_IO_DEC, 0x10);
            : 
            : 	pci_write_config32(PCH_LPC_DEV, 0xac,
            : 			   0x80010000);
            : }
Thanks for your review, this is a todo anyway. we should reword autoport otherwise we have the same stuff again and again.


https://review.coreboot.org/#/c/16994/8/src/mainboard/lenovo/x1_carbon_gen1/thermal.h
File src/mainboard/lenovo/x1_carbon_gen1/thermal.h:

PS8, Line 17: X230
> ahem
:P


-- 
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: 8
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: Nico Huber <nico.h at gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes