Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34193 )
Change subject: sirqen-test ......................................................................
Patch Set 4: Code-Review-1
(31 comments)
There are many issues here.
https://review.coreboot.org/c/coreboot/+/34193/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34193/4//COMMIT_MSG@9 PS4, Line 9: Signed-off-by: Your Name dponamorev@gmail.com I don't think your name is, literally, "Your Name".
There are two Signed-off lines, and both are in the wrong position
https://review.coreboot.org/c/coreboot/+/34193/4//COMMIT_MSG@15 PS4, Line 15: Add Nuvoton superio NCT6776. Add that superio, to what?
https://review.coreboot.org/c/coreboot/+/34193/4//COMMIT_MSG@16 PS4, Line 16: Turn off SoC internal UARTs, Turn on 48Mhz for Nuvoton freq. : Add CONFIG_OPTIONROMS is not set in SeaBIOS : Transferred functions related to 48MHz generator initialization and : Nuvoton superio to main() function. : Comment Nuvoton CR27, CR26 common registers in Nuvoton init. : Hangs with post code 0x92 after any smbus read. You should split these in three separate changes.
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... File src/mainboard/intel/mohonpeak/acpi/superio.asl:
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 16: : #undef SUPERIO_DEV : #undef SUPERIO_PNP_BASE Remove the whitespace at the end of these lines
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... File src/mainboard/intel/mohonpeak/config_seabios:
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 6: # CONFIG_OPTIONROMS is not set Why?
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... File src/mainboard/intel/mohonpeak/romstage.c:
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 35: #define DEBUG_UART_EN COMA_LPC_EN Should not be needed
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 36: #define NUVOTON_HW_INIT If you are going to always define this macro, you might as well not have it. Is it for debugging?
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 106: NCT5577D Name looks wrong
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 141: { : /* Set up fan control */ : for (int i = 0; i < ARRAY_SIZE(hwm_initvals); i++) : HWM_WRITE_INITVAL(hwm_initvals[i]); : } Here, you can do something similar to what I suggested for superio_init()
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 147: : static const u16 superio_initvals[] = { Why do you need an array? Replace the macros with function calls and remove the array completely.
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 152: More whitespace
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 153: More whitespace
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 158: SUPERIO_INITVAL(0x1c, 0x80), Replace macros like this one with this function:
pnp_write_config(GLOBAL_DEV, 0x1c, 0x80);
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 188: SUPERIO_BANK(0xa), Replace macros like this one with this function:
pnp_set_logical_device(ACPI_DEV);
See how ACPI_DEV is defined at the beginning of this file, change accordingly for the rest of LDNs
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 197: Even more whitespace at the end of a line, there's more cases below but won't comment on all of them
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 233: SUPERIO_UNLOCK; nuvoton_pnp_enter_conf_state(GLOBAL_DEV);
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 234: for (int i = 0; i < ARRAY_SIZE(superio_initvals); i++) : SUPERIO_WRITE_INITVAL(superio_initvals[i]); Replace writing an array of values with proper function calls, as described above.
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 236: SUPERIO_LOCK; nuvoton_pnp_exit_conf_state(GLOBAL_DEV);
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... File src/mainboard/intel/mohonpeak/superio.h:
PS4: Everything from this file is not needed, since better alternatives already exist in the tree.
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 27: (reg << 8) You are shifting twice: once to the left here, then to the right on SUPERIO_WRITE_INITVAL.
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 30: : #define SUPERIO_UNLOCK do { \ : outb(0x87, NUVOTON_PORT); \ : outb(0x87, NUVOTON_PORT); \ : } while (0) : : #define SUPERIO_LOCK do { \ : outb(0xaa, NUVOTON_PORT); \ : } while (0) Exact same thing already exists in: - src/superio/nuvoton/common/early_serial.c - src/superio/nuvoton/common/nuvoton.h
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 40: #define SUPERIO_WRITE(reg, data) do { \ : outb((reg), NUVOTON_PORT); \ : outb((data), NUVOTON_PORT + 1); \ : } while (0) : : #define SUPERIO_WRITE_INITVAL(val) SUPERIO_WRITE((val) >> 8, (val) & 0xff) You should use: pnp_write_config(pnp_devfn_t dev, uint8_t reg, uint8_t value)
Defined in: src/include/device/pnp_ops.h
https://review.coreboot.org/c/coreboot/+/34193/4/src/mainboard/intel/mohonpe... PS4, Line 47: #define HWM_WRITE(reg, data) do { \ : outb((reg), HWM_PORT + 5); \ : outb((data), HWM_PORT + 6); \ : } while (0) : : #define HWM_WRITE_INITVAL(val) HWM_WRITE((val) >> 8, (val) & 0xff) Same story as above
https://review.coreboot.org/c/coreboot/+/34193/4/src/southbridge/intel/fsp_r... File src/southbridge/intel/fsp_rangeley/early_smbus.c:
https://review.coreboot.org/c/coreboot/+/34193/4/src/southbridge/intel/fsp_r... PS4, Line 64: unsigned unsigned int?
https://review.coreboot.org/c/coreboot/+/34193/4/src/southbridge/intel/fsp_r... PS4, Line 69: unsigned same
https://review.coreboot.org/c/coreboot/+/34193/4/src/southbridge/intel/fsp_r... File src/southbridge/intel/fsp_rangeley/lpc.c:
https://review.coreboot.org/c/coreboot/+/34193/4/src/southbridge/intel/fsp_r... PS4, Line 86: #define SIRQEN (1 << 12) You should move this to the same file ILB_OIC is defined in
https://review.coreboot.org/c/coreboot/+/34193/4/src/southbridge/intel/fsp_r... File src/southbridge/intel/fsp_rangeley/romstage.c:
https://review.coreboot.org/c/coreboot/+/34193/4/src/southbridge/intel/fsp_r... PS4, Line 156: 0x69 Magic number?
https://review.coreboot.org/c/coreboot/+/34193/4/src/southbridge/intel/fsp_r... PS4, Line 156: readed Maybe use a more meaningful variable name; what is it storing here?
https://review.coreboot.org/c/coreboot/+/34193/4/src/southbridge/intel/fsp_r... PS4, Line 164: udelay Any reason as to why?
https://review.coreboot.org/c/coreboot/+/34193/4/src/southbridge/intel/fsp_r... File src/southbridge/intel/fsp_rangeley/soc.h:
https://review.coreboot.org/c/coreboot/+/34193/4/src/southbridge/intel/fsp_r... PS4, Line 72: unsigned unsigned int?
https://review.coreboot.org/c/coreboot/+/34193/4/src/southbridge/intel/fsp_r... PS4, Line 73: unsigned same
-
Angel Pons (Code Review)