Pavel Sayekat has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33842 )
Change subject: src/superio/nuvoton: Add support for NCT5539D ......................................................................
Patch Set 10:
(9 comments)
Thanks for being with me so far :)
https://review.coreboot.org/#/c/33842/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33842/3//COMMIT_MSG@7 PS3, Line 7: src/superio/nuvoton/common/early_serial.c: Add symbol to select COM port for NCT5539D : : src/superio/nuvoton/Makefile.inc: Add definition for NCT5539D
Ack
Done
https://review.coreboot.org/#/c/33842/3//COMMIT_MSG@11 PS3, Line 11: src/superio/nuvoton: Add support for NCT5539D
Done
Done
https://review.coreboot.org/#/c/33842/3/src/superio/nuvoton/Makefile.inc File src/superio/nuvoton/Makefile.inc:
https://review.coreboot.org/#/c/33842/3/src/superio/nuvoton/Makefile.inc@27 PS3, Line 27: subdirs-$(CONFIG_SUPERIO_NUVOTON_NCT5539D) += nct5539d
Done
Done
https://review.coreboot.org/#/c/33842/8/src/superio/nuvoton/common/early_ser... File src/superio/nuvoton/common/early_serial.c:
https://review.coreboot.org/#/c/33842/8/src/superio/nuvoton/common/early_ser... PS8, Line 70: 0x40
0x00
The default value of 2a is c0 for nct5539, the bin equivalent is 11000000, for SINA, the 7th bit is 0 and that makes it 10000000, taking into account the first 7 bits, the equivalent hex becomes 40 and the NCT6776F-D datasheet also suggests that because of the similar values, also according to hellsenberg/ Angel Pons' suggestion.
https://review.coreboot.org/#/c/33842/8/src/superio/nuvoton/nct5539d/nct5539... File src/superio/nuvoton/nct5539d/nct5539d.h:
https://review.coreboot.org/#/c/33842/8/src/superio/nuvoton/nct5539d/nct5539... PS8, Line 29: BCLK
bclk?
Done
https://review.coreboot.org/#/c/33842/8/src/superio/nuvoton/nct5539d/nct5539... PS8, Line 50: #define NCT5539D_PCHDSW ((3 << 8) | NCT5539D_DS) : #define NCT5539D_DSWWOPT ((4 << 8) | NCT5539D_DS) : #define NCT5539D_DS3OPT ((5 << 8) | NCT5539D_DS) : #define NCT5539D_DSDSS ((6 << 8) | NCT5539D_DS) : #define NCT5539D_DSPU ((7 << 8) | NCT5539D_DS)
haven't seen those in the datasheet
Done
https://review.coreboot.org/#/c/33842/3/src/superio/nuvoton/nct5539d/nct5539... File src/superio/nuvoton/nct5539d/nct5539d.h:
https://review.coreboot.org/#/c/33842/3/src/superio/nuvoton/nct5539d/nct5539... PS3, Line 32: /*GPIO, RI PSOUT Wake-Up Status*/
Done
Done
https://review.coreboot.org/#/c/33842/8/src/superio/nuvoton/nct5539d/superio... File src/superio/nuvoton/nct5539d/superio.c:
https://review.coreboot.org/#/c/33842/8/src/superio/nuvoton/nct5539d/superio... PS8, Line 60: PNP_IO1
I don't see a corresponding resource in the datasheet
Done
https://review.coreboot.org/#/c/33842/8/src/superio/nuvoton/nct5539d/superio... PS8, Line 82: { NULL, NCT5539D_PCHDSW}, : { NULL, NCT5539D_DSWWOPT}, : { NULL, NCT5539D_DS3OPT}, : { NULL, NCT5539D_DSDSS}, : { NULL, NCT5539D_DSPU},
see the comment in the header file
Done