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.layou... 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... 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