Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32531 )
Change subject: Add support for the 51nb X210 ......................................................................
Patch Set 21:
(11 comments)
Nice port!
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
static struct pnp_info dev_infos[] = { { NULL, 0x05 }, { NULL, 0x06 }, { NULL, 0x11 } }; static void ec_51nb_npce985la0dx_ops_enable(struct device *dev) { pnp_enabled_devices(dev, &pnp_ops, ARRAY_SIZE(dev_infos), dev_infos); }
plus in the devicetree
device pnp 4e.5 on end device pnp 4e.6 on end device pnp 4e.11 on end
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
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)
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.
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/devicetree.... File src/mainboard/51nb/x210/devicetree.cb:
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/devicetree.... PS21, Line 149: register "PcieRpEnable[2]" = "1" # Ethernet controller : register "PcieRpLtrEnable[2]" = "1" This is not RP1?
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 backlight, but from your ASL code it looks like the EC is doing that?
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/mainboard.c File src/mainboard/51nb/x210/mainboard.c:
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/mainboard.c... PS21, Line 53: } This seems like Purisms idea of how to store a serial number. You don't have to follow it. But if you want, it would be nice to put it somewhere in `src/lib/`.
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.
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); : } `pei_data` is a remnant from pre-FSP times. IIRC, the code of some Google boards use this to talk to itself, but otherwise implementing this function should be a no-op.
Probably worth to merge the two r-comp thingies into `romstage.c`.
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 default .c file for the main (ram) stage named after the entity (chip/board).
https://review.coreboot.org/#/c/32531/21/src/mainboard/51nb/x210/ramstage.c@... PS21, Line 5: * Copyright (C) 2015-2019 Google LLC I don't know how one function call can have 6 different dates of first publication, but I understand the urge to copy these lines.