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 25:
(6 comments)
https://review.coreboot.org/c/coreboot/+/32531/24/Documentation/mainboard/51... File Documentation/mainboard/51nb/x210.md:
https://review.coreboot.org/c/coreboot/+/32531/24/Documentation/mainboard/51... PS24, Line 1: # 51NB X210
file needs to be referenced by index. […]
Ack
https://review.coreboot.org/c/coreboot/+/32531/24/Documentation/mainboard/51... PS24, Line 13: #
I would suggest explaining how to flash internally using a layout/fmap, for images built without EC […]
Ack
https://review.coreboot.org/c/coreboot/+/32531/24/Documentation/mainboard/51... PS24, Line 19: is located on the upper side of the motherboard, below the keyboard
can you provide a picture? […]
Ack
https://review.coreboot.org/c/coreboot/+/32531/23/src/ec/51nb/npce985la0dx/K... File src/ec/51nb/npce985la0dx/Kconfig:
https://review.coreboot.org/c/coreboot/+/32531/23/src/ec/51nb/npce985la0dx/K... PS23, Line 27: images
Minor: there's just one image, so this should be singular
Ack
https://review.coreboot.org/c/coreboot/+/32531/23/src/ec/51nb/npce985la0dx/K... PS23, Line 29: default y
Since you're using a fmap in coreboot, you can use it with flashrom to flash only to the FMAP's BIOS […]
Ack
https://review.coreboot.org/c/coreboot/+/32531/24/src/ec/51nb/npce985la0dx/n... File src/ec/51nb/npce985la0dx/npce985la0dx.c:
https://review.coreboot.org/c/coreboot/+/32531/24/src/ec/51nb/npce985la0dx/n... PS24, Line 19: { NULL, 0x05 }, { NULL, 0x06 }, { NULL, 0x11 }
what are those LDNs used for? Do you need to set additional registers in the superio space?
Ack