Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48648 )
Change subject: mb/asus: Add ASUS H110T mainboard ......................................................................
Patch Set 1:
(15 comments)
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/Kc... File src/mainboard/asus/h110t/Kconfig:
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/Kc... PS1, Line 19: select DRIVER_INTEL_I210 This board has two NICs, right? I see two Ethernet ports. I find it weird that they are different, though.
If one of them shows up as PCI bus:dev.fun 00:1f.6 with vendor firmware, it's not i210 but the in-PCH GbE NIC, and you would want to enable the corresponding device in devicetree.cb
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/cm... File src/mainboard/asus/h110t/cmos.layout:
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/cm... PS1, Line 6: #start-bit length config config-ID name : #0 8 r 0 seconds : #8 8 r 0 alarm_seconds : #16 8 r 0 minutes : #24 8 r 0 alarm_minutes : #32 8 r 0 hours : #40 8 r 0 alarm_hours : #48 8 r 0 day_of_week : #56 8 r 0 day_of_month : #64 8 r 0 month : #72 8 r 0 year : # ----------------------------------------------------------------- : # Status Register A : #80 4 r 0 rate_select : #84 3 r 0 REF_Clock : #87 1 r 0 UIP : # ----------------------------------------------------------------- : # Status Register B : #88 1 r 0 auto_switch_DST : #89 1 r 0 24_hour_mode : #90 1 r 0 binary_values_enable : #91 1 r 0 square-wave_out_enable : #92 1 r 0 update_finished_enable : #93 1 r 0 alarm_interrupt_enable : #94 1 r 0 periodic_interrupt_enable : #95 1 r 0 disable_clock_updates : # ----------------------------------------------------------------- : # Status Register C : #96 4 r 0 status_c_rsvd : #100 1 r 0 uf_flag : #101 1 r 0 af_flag : #102 1 r 0 pf_flag : #103 1 r 0 irqf_flag : # ----------------------------------------------------------------- : # Status Register D : #104 7 r 0 status_d_rsvd : #111 1 r 0 valid_cmos_ram : # ----------------------------------------------------------------- : # Diagnostic Status Register : #112 8 r 0 diag_rsvd1 Please drop this
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/cm... PS1, Line 48: For consistency, align columns with tabs
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/cm... PS1, Line 49: #120 264 r 0 unused Drop unused and commented-out lines too
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/cm... PS1, Line 77: # SandyBridge MRC Scrambler Seed values : #896 32 r 0 mrc_scrambler_seed : #928 32 r 0 mrc_scrambler_seed_s3 Not Sandy Bridge, please drop
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/cm... PS1, Line 83: #1000 24 r 0 amd_reserved Meaningless on Intel, can drop too
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/de... File src/mainboard/asus/h110t/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/de... PS1, Line 46: # Asus specific Yeah, subsystem IDs are OEM-specific. No need to add this comment
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/de... PS1, Line 64: register "usb2_ports[13]" = "USB2_PORT_MID(OC_SKIP)" nit: add a blank line to separate USB2 from USB3
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/de... PS1, Line 82: on nit: To preserve alignment, I would add an extra space after `on`
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/de... PS1, Line 87: # SATA nit: same here, I'd align the comment with the previous lines
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/de... PS1, Line 99: register "PcieRpClkReqSupport[0]" = "1" You'd want to specify which ClkReq pin is to be used for this root port.
WARNING: Root ports are numbered starting from 1, but device functions and array indices start at zero. It's easy to get the numbers mixed up.
NOTE: if root ports aren't working as expected, check if root port functions have been swapped. With vendor firmware, if the device ID for 1c.0 is that of root port #5, functions have definitely been swapped.
FSP does its own function swapping, then coreboot updates the devicetree accounting for what FSP did. In my example, one would use `device pci 1c.4 on` and `4` for the array index.
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/de... PS1, Line 126: 2e.ff 2e.0 ?
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ds... File src/mainboard/asus/h110t/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ds... PS1, Line 20: /* Image processing unit */ : #include <soc/intel/skylake/acpi/ipu.asl> IPU isn't used on this board (it's for camera stuff)
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/gm... File src/mainboard/asus/h110t/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/gm... PS1, Line 11: ports : constant Port_List := : (HDMI1, : DP1, : LVDS, : others => Disabled); For Ada, we use three spaces for indentation
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ro... File src/mainboard/asus/h110t/romstage.c:
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ro... PS1, Line 6: include <string.h>
Is this used?
memcpy prototype is in string.h
https://man7.org/linux/man-pages/man3/memcpy.3.html