Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34603 )
Change subject: src/mainboard/asus: Add H110M-E/M.2 mainboard support ......................................................................
Patch Set 18:
(5 comments)
I'm sorry for the waiting. I urgently needed to finish the project at work.
as he ported asrock/h110m
This work isn't completed yet. Now I'm working on gpio, and after that I need to solve problems with superio.
and probably knows Skylake stuff better than I do
I'm not sure about that :)
https://review.coreboot.org/c/coreboot/+/34603/18/src/mainboard/asus/h110m-e... File src/mainboard/asus/h110m-e_m2/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/34603/18/src/mainboard/asus/h110m-e... PS18, Line 64: need to add register "serirq_mode" = "SERIRQ_CONTINUOUS"
to set Continuous SIRQ mode
https://review.coreboot.org/c/coreboot/+/33801
https://review.coreboot.org/c/coreboot/+/34603/18/src/mainboard/asus/h110m-e... PS18, Line 94: 0x50 May be better to use the macro VR_CFG_AMP()
https://github.com/coreboot/coreboot/blob/ef7a3267870f126cc2f815812cfe545008...
https://review.coreboot.org/c/coreboot/+/34603/18/src/mainboard/asus/h110m-e... PS18, Line 145: register "i2c_voltage[4]" = "I2C_VOLTAGE_1V8" # I2C4 is 1.8V
I think you don't need this
I agree with you
https://review.coreboot.org/c/coreboot/+/34603/18/src/mainboard/asus/h110m-e... PS18, Line 278: device pci 14.2 on # Thermal Subsystem : subsystemid 0x1849 0xa131 : end
Disable this device: […]
Are you sure about that? This device is always enabled on boards with AMI UEFI.
https://review.coreboot.org/c/coreboot/+/34603/18/src/mainboard/asus/h110m-e... PS18, Line 285: device pci 16.0 on # Management Engine Interface 1 : subsystemid 0x1849 0xa131 : end
Looks disabled on your board, but it might just be hidden
Probably me_cleaner was used to disable this device. However, we need to be very careful with ME: https://recon.cx/2014/slides/Recon%202014%20Skochinsky.pdf
I'm not sure, but I think this interface is needed for initial initialization, even if we disable ME later