[coreboot-gerrit] Change in coreboot[master]: gigabyte/ga-h61m-s2pv: Add new mainboard

[Arthur Heymans (Code Review)]

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/devicetree.cb
File src/mainboard/gigabyte/ga-h61m-s2pv/devicetree.cb:

https://review.coreboot.org/#/c/25912/1/src/mainboard/gigabyte/ga-h61m-s2pv/devicetree.cb@114
PS1, Line 114: io 0x62 = 0xa20
usually put about irq 0x70 = ...


https://review.coreboot.org/#/c/25912/1/src/mainboard/gigabyte/ga-h61m-s2pv/devicetree.cb@119
PS1, Line 119: io 0x62 = 0x64
same


https://review.coreboot.org/#/c/25912/1/src/mainboard/gigabyte/ga-h61m-s2pv/romstage.c
File src/mainboard/gigabyte/ga-h61m-s2pv/romstage.c:

https://review.coreboot.org/#/c/25912/1/src/mainboard/gigabyte/ga-h61m-s2pv/romstage.c@16
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/romstage.c@62
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/romstage.c@84
PS1, Line 84: 	pci_write_config32(PCH_LPC_DEV, LPC_GEN1_DEC, 0x3c0a01);
might not be needed to be set in romstage.



-- 
To view, visit https://review.coreboot.org/25912
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I598a0b75093a0f1aef2ac615035d66786a8c22cb
Gerrit-Change-Number: 25912
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons
Gerrit-CC: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Comment-Date: Sun, 29 Apr 2018 18:32:35 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20180429/9f9597ff/attachment.html>