Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32531 )
Change subject: Add support for the 51nb X210 ......................................................................
Patch Set 8:
(9 comments)
https://review.coreboot.org/#/c/32531/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/32531/8//COMMIT_MSG@10 PS8, Line 10: firmware protection What does this refer to? Is the IFD fully unlocked? Are all protected ranges unset? Does it not use Boot Guard?
https://review.coreboot.org/#/c/32531/8//COMMIT_MSG@15 PS8, Line 15: EC firmware is contained within the system SPI flash, and so a blob of : EC firmware must be injected to a defined location during image build. I would suggest using a fmap to define this location. I was suggested the same for a laptop with the same problem.
https://review.coreboot.org/#/c/32531/8//COMMIT_MSG@27 PS8, Line 27: I'd suggest listing what works/does not work/is untested.
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/51nb.c File src/ec/51nb/51nb.c:
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/51nb.c@23 PS8, Line 23: /* Enable function 5 (PS/2 AUX) */
I'm not sure? There's no superio on the board, just the EC, and the EC also handles PS/2. […]
Some ECs have a SuperIO-like part. See ec/roda/it8518 for an example.
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/51nb.c@43 PS8, Line 43: CHIP_NAME("51NB EC") Not sure if all the 51nb devices use the same EC. I'd suggest specifying the EC model (NPCE9...) so that there isn't a name conflict if a 2nd 51nb EC were to be added.
Yes, this would imply moving everything in a subfolder and updating all the paths.
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/Kconfig File src/ec/51nb/Kconfig:
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/Kconfig@43 PS8, Line 43: 0x00200000
No
Is that the beginning of the BIOS region? Some other different Nuvoton EC I have has the firmware at the same location.
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/ec.h File src/ec/51nb/ec.h:
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/ec.h@20 PS8, Line 20: #define SETUP_COMMAND 0x4e
No, this is (as far as I can tell) specific to the EC part.
I recall seeing 0x4e/0x4f somewhere else... These are the IO addresses the SuperIO part of the EC uses. These are sometimes selectable (most SuperIOs use 0x2e/0x2f).
I'd say this is mainboard-specific.
https://review.coreboot.org/#/c/32531/8/src/mainboard/51nb/x210/acpi/ec.asl File src/mainboard/51nb/x210/acpi/ec.asl:
https://review.coreboot.org/#/c/32531/8/src/mainboard/51nb/x210/acpi/ec.asl@... PS8, Line 126: Device (BAT) Maybe this could go in acpi/battery.asl
https://review.coreboot.org/#/c/32531/8/src/mainboard/51nb/x210/mainboard.c File src/mainboard/51nb/x210/mainboard.c:
https://review.coreboot.org/#/c/32531/8/src/mainboard/51nb/x210/mainboard.c@... PS8, Line 57: /* Route 0x4e/4f to LPC */ Ah, this is what I recall 0x4e/0x4f from.