Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/11791 )
Change subject: mainboard/lenovo/t410: Add new port ......................................................................
Patch Set 21:
(19 comments)
Rebased on master, removed 90% of boilerplate code. Still boots to Linux.
https://review.coreboot.org/c/coreboot/+/11791/21/Documentation/mainboard/in... File Documentation/mainboard/index.md:
https://review.coreboot.org/c/coreboot/+/11791/21/Documentation/mainboard/in... PS21, Line 91: ## Portwell
Spurious?
no it was messing up the lenovo section
https://review.coreboot.org/c/coreboot/+/11791/21/Documentation/mainboard/le... File Documentation/mainboard/lenovo/t410_chip_location.jpg:
PS21:
I think a closer image would be more helpful. […]
no, it's assembled again
https://review.coreboot.org/c/coreboot/+/11791/17/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/Kconfig:
https://review.coreboot.org/c/coreboot/+/11791/17/src/mainboard/lenovo/t410/... PS17, Line 34: MMCONF_BASE_ADDRESS
Care to send a separate patch to move this to nb/nehalem/kconfig?
removed as it's the default value
https://review.coreboot.org/c/coreboot/+/11791/17/src/mainboard/lenovo/t410/... PS17, Line 42: config MAX_CPUS : int : default 4 : : config CPU_ADDR_BITS : int : default 36
To me it makes more sense to put these in cpu/intel/model_2056x.
removed as it's the default
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/Kconfig:
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 63: default 4
Clarckfield CPU's (quadcore) are supported by the current code. […]
done
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 21: static void acpi_update_thermal_table(global_nvs_t *gnvs) : { : gnvs->tcrt = CRITICAL_TEMPERATURE; : gnvs->tpsv = PASSIVE_TEMPERATURE; : }
Do we really need a function just for that?
Done
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 32: #
Maybe remove the empty comments
Done
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 122: end
Maybe move to the previous line, and remove the "dummy" comment?
Done
https://review.coreboot.org/c/coreboot/+/11791/18/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/mainboard.c:
https://review.coreboot.org/c/coreboot/+/11791/18/src/mainboard/lenovo/t410/... PS18, Line 20: #include <device/pci_ops.h>
remove?
Done
https://review.coreboot.org/c/coreboot/+/11791/18/src/mainboard/lenovo/t410/... PS18, Line 22: #include <northbridge/intel/nehalem/nehalem.h> : #include <southbridge/intel/bd82x6x/pch.h>
remove?
Done
https://review.coreboot.org/c/coreboot/+/11791/19/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/romstage.c:
https://review.coreboot.org/c/coreboot/+/11791/19/src/mainboard/lenovo/t410/... PS19, Line 22: #include <string.h>
remove?
Done
https://review.coreboot.org/c/coreboot/+/11791/19/src/mainboard/lenovo/t410/... PS19, Line 23: #include <arch/io.h>
remove
Done
https://review.coreboot.org/c/coreboot/+/11791/19/src/mainboard/lenovo/t410/... PS19, Line 25: #include <device/pci_def.h>
unused?
Done
https://review.coreboot.org/c/coreboot/+/11791/19/src/mainboard/lenovo/t410/... PS19, Line 26: #include <device/pnp_def.h>
unused
Done
https://review.coreboot.org/c/coreboot/+/11791/19/src/mainboard/lenovo/t410/... PS19, Line 36: #include <arch/early_variables.h>
remove
Done
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/romstage.c:
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 51: pci_write_config16(PCH_LPC_DEV, LPC_EN, : CNF2_LPC_EN | CNF1_LPC_EN | MC_LPC_EN | KBC_LPC_EN | : COMA_LPC_EN | GAMEL_LPC_EN);
Maybe reflow this so that it fits in two lines. […]
removed
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 54: : 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_GEN3_DEC, 0x1c1681); : pci_write_config32(PCH_LPC_DEV, LPC_GEN4_DEC, (0x68 & ~3) | 0x00040001);
Any idea if you need those during romstage as there is ramstage code to set those up based on dt (co […]
rebased and remove the code
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/vboot-rwa.fmd:
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 10: 0x16ffc0
I think you can drop this. It should fill the remaining space.
Done
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 27: 0xff000
same
Done