Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/25912 )
Change subject: gigabyte/ga-h61m-s2pv: Add new mainboard ......................................................................
Patch Set 1:
(5 comments)
Looks good. A few very minor nits though.
https://review.coreboot.org/#/c/25912/1/src/mainboard/gigabyte/ga-h61m-s2pv/... File src/mainboard/gigabyte/ga-h61m-s2pv/devicetree.cb:
https://review.coreboot.org/#/c/25912/1/src/mainboard/gigabyte/ga-h61m-s2pv/... PS1, Line 114: io 0x62 = 0xa20 usually put about irq 0x70 = ...
https://review.coreboot.org/#/c/25912/1/src/mainboard/gigabyte/ga-h61m-s2pv/... PS1, Line 119: io 0x62 = 0x64 same
https://review.coreboot.org/#/c/25912/1/src/mainboard/gigabyte/ga-h61m-s2pv/... File src/mainboard/gigabyte/ga-h61m-s2pv/romstage.c:
https://review.coreboot.org/#/c/25912/1/src/mainboard/gigabyte/ga-h61m-s2pv/... PS1, Line 16: #define SUPERIO_BASE 0x2e : #define SUPERIO_DEV PNP_DEV(SUPERIO_BASE, 0) : #define SUPERIO_GPIO PNP_DEV(SUPERIO_BASE, IT8728F_GPIO) : #define SERIAL_DEV PNP_DEV(SUPERIO_BASE, 0x01) usually put below the #include macros.
https://review.coreboot.org/#/c/25912/1/src/mainboard/gigabyte/ga-h61m-s2pv/... PS1, Line 62: ite_reg_write(IT8728F_EC, 0xf1, 0xc0); : ite_reg_write(IT8728F_EC, 0xf6, 0xf0); : ite_reg_write(IT8728F_EC, 0xf9, 0x48); : ite_reg_write(IT8728F_EC, 0x60, 0x0a); : ite_reg_write(IT8728F_EC, 0x61, 0x30); : ite_reg_write(IT8728F_EC, 0x62, 0x0a); : ite_reg_write(IT8728F_EC, 0x63, 0x20); : ite_reg_write(IT8728F_EC, 0x30, 0x01); you could probably set most of these in the devicetree/ramstage or is the fan not working in romstage if you don't do this?
https://review.coreboot.org/#/c/25912/1/src/mainboard/gigabyte/ga-h61m-s2pv/... PS1, Line 84: pci_write_config32(PCH_LPC_DEV, LPC_GEN1_DEC, 0x3c0a01); might not be needed to be set in romstage.
-
Arthur Heymans (Code Review)