[coreboot-gerrit] Change in coreboot[master]: mainboard/lenovo/t430: Add Thinkpad T430 support

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/18011 )

Change subject: mainboard/lenovo/t430: Add Thinkpad T430 support
......................................................................


Patch Set 8:

(16 comments)

https://review.coreboot.org/#/c/18011/8/src/mainboard/lenovo/t430/cmos.layout
File src/mainboard/lenovo/t430/cmos.layout:

Line 51: 392          3       e       5        baud_rate
> For what?
OK


Line 59: 409          2       e       7        power_on_after_fail
> Does this option work correctly? Sometimes the EC controls it, but
Tested and working


Line 75: 424          1       e       2        hyper_threading
> Beside the hype around this option, I still doubt it did something
OK


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

PS8, Line 3: 0x80000410, 0x00000005
> Why specify more than 3 values if ndid is 3?
OK


PS8, Line 18: 0x11551155
> IIRC, "gpu_cpu_backlight" is in charge. If in doubt, keep the lower
OK


PS8, Line 41: # Intel Series 6 Cougar Point PCH
> The comment is dubious. T430 should have a Panther Point PCH.
OK


Line 46: 			register "gen3_dec" = "0x00000000"
> 0 is the default.
OK


Line 47: 			register "gen4_dec" = "0x000c06a1"
> Why use gen1, gen2, gen4 instead of gen1, gen2, gen3?
PMH7, not sure about the other one


https://review.coreboot.org/#/c/18011/8/src/mainboard/lenovo/t430/dsdt.asl
File src/mainboard/lenovo/t430/dsdt.asl:

PS8, Line 18: #define BRIGHTNESS_UP \_SB.PCI0.GFX0.INCB
            : #define BRIGHTNESS_DOWN \_SB.PCI0.GFX0.DECB
            : #define ACPI_VIDEO_DEVICE \_SB.PCI0.GFX0
            : #define EC_LENOVO_H8_ME_WORKAROUND 1
            : #define THINKPAD_EC_GPE 17
> `acpi/ec.asl` would be a better place, IMO.
OK


PS8, Line 43: 		#include <northbridge/intel/sandybridge/acpi/sandybridge.asl>
            : 		#include <drivers/intel/gma/acpi/default_brightness_levels.asl>
            : 		#include <southbridge/intel/bd82x6x/acpi/pch.asl>
            : 		#include <southbridge/intel/bd82x6x/acpi/default_irq_route.asl>
> indent one more level?
OK


https://review.coreboot.org/#/c/18011/8/src/mainboard/lenovo/t430/gpio.c
File src/mainboard/lenovo/t430/gpio.c:

Line 17: const struct pch_gpio_set1 pch_gpio_set1_mode = {
> empty line between header?
OK


https://review.coreboot.org/#/c/18011/8/src/mainboard/lenovo/t430/hda_verb.c
File src/mainboard/lenovo/t430/hda_verb.c:

PS8, Line 22: 0x0000000b
> A decimal 11 would be much more readable.
OK


https://review.coreboot.org/#/c/18011/8/src/mainboard/lenovo/t430/romstage.c
File src/mainboard/lenovo/t430/romstage.c:

PS8, Line 26: CNF2_LPC_EN | CNF1_LPC_EN
> What for?
TPM


PS8, Line 29: 	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);
> Are these all needed before the devicetree values kick in?
No


Line 35: 	pci_write_config32(PCH_LPC_DEV, 0xac, 0x80010000);
> Why? Possible side effects with early_me code?
Yes, side effects are possible


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

PS8, Line 21: 	
> tabs ?
OK


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5403cfb80a57753e873c570d95ca535cf5f45630
Gerrit-PatchSet: 8
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Reviewer: Martin Roth <martinroth at google.com>
Gerrit-Reviewer: Nico Huber <nico.h at gmx.de>
Gerrit-Reviewer: Patrick Rudolph <siro at das-labor.org>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Philipp Deppenwiese <zaolin.daisuki at gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes