Matthew Garrett has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32531 )
Change subject: Add support for the 51nb X210 ......................................................................
Patch Set 21:
(8 comments)
https://review.coreboot.org/#/c/32531/21/src/ec/51nb/npce985la0dx/npce985la0... File src/ec/51nb/npce985la0dx/npce985la0dx.c:
PS21:
Using our pnp infrastructure this should be something like […]
Ack
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/Kconfig File src/mainboard/51nb/x210/Kconfig:
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/Kconfig@13 PS21, Line 13: select MAINBOARD_USES_FSP2_0
indent, please
Ack
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/Kconfig@18 PS21, Line 18: default 18
Not useful (unless you want to write PIRQ tables)
Ack
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/Kconfig@70 PS21, Line 70: default 0x59fe00
I'm not sure if this is meaningful in combination with a custom FMD.
Ack
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/dsdt.asl File src/mainboard/51nb/x210/dsdt.asl:
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/dsdt.asl@41 PS21, Line 41: #include <drivers/intel/gma/acpi/pch.asl>
This is supposed to be used when the Intel gfx controls the […]
Fixed - removed the opregion support and just implemented enough code for Linux to bind to the device.
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/pei_data.c File src/mainboard/51nb/x210/pei_data.c:
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/pei_data.c@... PS21, Line 24: void mainboard_fill_dq_map_data(void *dq_map_ptr) : { : /* DQ byte map */ : const u8 dq_map[2][12] = { : { 0x0F, 0xF0, 0x00, 0xF0, 0x0F, 0xF0, : 0x0F, 0x00, 0xFF, 0x00, 0xFF, 0x00 }, : { 0x33, 0xCC, 0x00, 0xCC, 0x33, 0xCC, : 0x33, 0x00, 0xFF, 0x00, 0xFF, 0x00 } }; : memcpy(dq_map_ptr, dq_map, sizeof(dq_map)); : } : : void mainboard_fill_dqs_map_data(void *dqs_map_ptr) : { : /* DQS CPU<>DRAM map */ : const u8 dqs_map[2][8] = { : { 0, 1, 3, 2, 4, 5, 6, 7 }, : { 1, 0, 4, 5, 2, 3, 6, 7 } }; : memcpy(dqs_map_ptr, dqs_map, sizeof(dqs_map)); : } :
This is for memory-down configurations only.
Ack
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/pei_data.c@... PS21, Line 59: void mainboard_fill_pei_data(struct pei_data *pei_data) : { : mainboard_fill_dq_map_data(&pei_data->dq_map); : mainboard_fill_dqs_map_data(&pei_data->dqs_map); : mainboard_fill_rcomp_res_data(&pei_data->RcompResistor); : mainboard_fill_rcomp_strength_data(&pei_data->RcompTarget); : }
Not sure what you mean. The implementations above are only used in the […]
Ack
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/ramstage.c File src/mainboard/51nb/x210/ramstage.c:
PS21:
This would usually go into `mainboard.c`. It's a convention to have one […]
Ack