[coreboot-gerrit] [L] Change in coreboot[main]: mb/asus/h61-series: Add Asus P8H61-M LE

[Angel Pons (Code Review)]

Attention is currently required from: ItsLua.

Angel Pons has posted comments on this change by ItsLua. ( https://review.coreboot.org/c/coreboot/+/70882?usp=email )

Change subject: mb/asus/h61-series: Add Asus P8H61-M LE
......................................................................

Patch Set 10: Code-Review+1

(11 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/70882/comment/73340d07_dab57eff?usp... :
PS10, Line 26: NoVidea 
I think you mean Nvidia?

https://review.coreboot.org/c/coreboot/+/70882/comment/aa862113_b433f97d?usp... :
PS10, Line 26: PCIe 
PCIe x16 (I imagine)

https://review.coreboot.org/c/coreboot/+/70882/comment/b7856328_a8c9b090?usp... :
PS10, Line 33:  - PS/2 mouse/keyboard combo connector
It's also listed as working

https://review.coreboot.org/c/coreboot/+/70882/comment/fdf7a7b3_92cf795a?usp... :
PS10, Line 34:  - Realtek Gigabit Ethernet
If possible, make sure the MAC address doesn't change when running coreboot. If it does, then this board needs the same Realtek driver thing as the other variants.

I'm pretty sure the NIC shows up as `enpXs0` (where `X` is the PCI bus the NIC is on, which can change) on at least Linux because the devicetree doesn't have a device for the NIC. Would be nice to add the device under the corresponding root port so that coreboot generates SMBIOS information (then the NIC will always be `eno0`).

Please let me know if you have any doubts.

File src/mainboard/asus/h61-series/variants/p8h61-m_le/early_init.c:

https://review.coreboot.org/c/coreboot/+/70882/comment/77687cab_3e199cac?usp... :
PS10, Line 47: 	pnp_set_logical_device(SERIAL_DEV);
This does nothing, please remove

File src/mainboard/asus/h61-series/variants/p8h61-m_le/hda_verb.c:

https://review.coreboot.org/c/coreboot/+/70882/comment/5bb148d8_94d38d25?usp... :
PS10, Line 10: AZALIA_PIN_CFG(0, 0x11, 0x99430130),
             : 	AZALIA_PIN_CFG(0, 0x12, 0x411111f0),
             : 	AZALIA_PIN_CFG(0, 0x14, 0x01014010),
             : 	AZALIA_PIN_CFG(0, 0x15, 0x411111f0),
             : 	AZALIA_PIN_CFG(0, 0x16, 0x411111f0),
             : 	AZALIA_PIN_CFG(0, 0x17, 0x411111f0),
             : 	AZALIA_PIN_CFG(0, 0x18, 0x01a19840),
             : 	AZALIA_PIN_CFG(0, 0x19, 0x02a19c50),
             : 	AZALIA_PIN_CFG(0, 0x1a, 0x0181304f),
             : 	AZALIA_PIN_CFG(0, 0x1b, 0x02214c20),
             : 	AZALIA_PIN_CFG(0, 0x1c, 0x411111f0),
             : 	AZALIA_PIN_CFG(0, 0x1d, 0x4004c601),
             : 	AZALIA_PIN_CFG(0, 0x1e, 0x411111f0),
             : 	AZALIA_PIN_CFG(0, 0x1f, 0x411111f0),
As per CB:82394

```suggestion
    AZALIA_PIN_CFG(0, 0x11, 0x99430130),
    AZALIA_PIN_CFG(0, 0x12, AZALIA_PIN_CFG_NC(0)),
    AZALIA_PIN_CFG(0, 0x14, 0x01014010),
    AZALIA_PIN_CFG(0, 0x15, AZALIA_PIN_CFG_NC(0)),
    AZALIA_PIN_CFG(0, 0x16, AZALIA_PIN_CFG_NC(0)),
    AZALIA_PIN_CFG(0, 0x17, AZALIA_PIN_CFG_NC(0)),
    AZALIA_PIN_CFG(0, 0x18, 0x01a19840),
    AZALIA_PIN_CFG(0, 0x19, 0x02a19c50),
    AZALIA_PIN_CFG(0, 0x1a, 0x0181304f),
    AZALIA_PIN_CFG(0, 0x1b, 0x02214c20),
    AZALIA_PIN_CFG(0, 0x1c, AZALIA_PIN_CFG_NC(0)),
    AZALIA_PIN_CFG(0, 0x1d, 0x4004c601),
    AZALIA_PIN_CFG(0, 0x1e, AZALIA_PIN_CFG_NC(0)),
    AZALIA_PIN_CFG(0, 0x1f, AZALIA_PIN_CFG_NC(0)),
```

File src/mainboard/asus/h61-series/variants/p8h61-m_le/overridetree.cb:

https://review.coreboot.org/c/coreboot/+/70882/comment/881c3804_c9c70f84?usp... :
PS10, Line 8: 			register "gen2_dec" = "0x00fc0a01"
Is this actually needed? I'm not sure if anything is using this address space, and there doesn't seem to be a LPC TPM header on this board either

https://review.coreboot.org/c/coreboot/+/70882/comment/c241c2c6_9c8e453a?usp... :
PS10, Line 12: 			device pci 1b.0 on	# HD audio controller
```suggestion
    		device ref hda on
```

https://review.coreboot.org/c/coreboot/+/70882/comment/c1e39538_4d6be118?usp... :
PS10, Line 17: device pci 1c.0 on  end
             : 			device pci 1c.1 on  end
             : 			device pci 1c.2 on  end
             : 			device pci 1c.3 on  end
             : 			device pci 1c.4 on  end
             : 			device pci 1c.5 on  end
             : 			device pci 1c.6 off end
             : 			device pci 1c.7 off end
```suggestion
    		device ref pcie_rp1 on end
    		device ref pcie_rp2 on end
    		device ref pcie_rp3 on end
    		device ref pcie_rp4 on end
    		device ref pcie_rp5 on end
    		device ref pcie_rp6 on end
```
The last two root ports don't exist in the H61 PCH, so you can omit them. They're disabled by default anyway.

You can check which PCIe port is which by plugging in as many devices as possible and checking `lspci` and `lspci -nntv`

https://review.coreboot.org/c/coreboot/+/70882/comment/ab2998e3_bd62a159?usp... :
PS10, Line 26: 			device pci 1f.0 on # LPC bridge
```suggestion
    		device ref lpc on
```

https://review.coreboot.org/c/coreboot/+/70882/comment/04475183_85ec0c40?usp... :
PS10, Line 27: 				# TODO: Test Super I/O configuration
This should typically be set as per `superiotool.log` from the autoport logs