Change in coreboot[master]: src/mainboard/asus/p8h61-m_lx2: Autoport resluts for p8h61-m_lx2
Attention is currently required from: Name of user not set #1003488, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52478 )
Change subject: src/mainboard/asus/p8h61-m_lx2: Autoport resluts for p8h61-m_lx2 ......................................................................
Patch Set 5:
(29 comments)
Patchset:
PS5: I've left a bunch of comments. When turning this port into a `mb/asus/h61-series` variant, several of my comments won't matter anymore because the code will be gone. Still, I feel these comments can be interesting and/or useful for you.
File src/mainboard/asus/p8h61-m_lx2/Kconfig:
https://review.coreboot.org/c/coreboot/+/52478/comment/7ca4dd88_fedee455 PS5, Line 16: select REALTEK_8168_RESET #LX : select RT8168_SET_LED_MODE #LX This doesn't really do anything without the corresponding entries in the devicetree.
https://review.coreboot.org/c/coreboot/+/52478/comment/5b5c4a3a_38232e3d PS5, Line 20: string Please remove the type, see CB:56553
https://review.coreboot.org/c/coreboot/+/52478/comment/0069f62e_3f7b159f PS5, Line 24: string Please remove the type, see CB:56554
https://review.coreboot.org/c/coreboot/+/52478/comment/8548c5c6_5c9a7219 PS5, Line 27: config VGA_BIOS_FILE : string : default "pci8086,0102.rom" : : config VGA_BIOS_ID : string : default "8086,0102" Please remove this. `VGA_BIOS_FILE` specifies a non-existing file, and `VGA_BIOS_ID` depends on the installed CPU, so providing these values is pointless.
https://review.coreboot.org/c/coreboot/+/52478/comment/05068763_8a8ead29 PS5, Line 35: config DRAM_RESET_GATE_GPIO # FIXME: check this : int : default 60 Just remove this. GPIO60 is not used for this (it's configured in native mode, see `gpio.c`) and boardviews show GPIO60 is unused.
For context, setting this correctly is needed on boards (most often laptops) implementing a circuit to prevent power leakage while suspended in S3 (power leakage means that battery loses charge over time, which is bad). I don't think I've ever seen a desktop board implementing this.
https://review.coreboot.org/c/coreboot/+/52478/comment/547caf9e_d1e276fd PS5, Line 39: config USBDEBUG_HCD_INDEX # FIXME: check this : int : default 2 If you can't test usbdebug, just remove this.
File src/mainboard/asus/p8h61-m_lx2/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/52478/comment/f05772bf_76619488 PS5, Line 9: // The lid is open by default. : gnvs->lids = 1; This only matters on laptops. It can safely be removed.
https://review.coreboot.org/c/coreboot/+/52478/comment/3a2de16c_1c83ec14 PS5, Line 12: // Temperature at which OS will shutdown : gnvs->tcrt = 100; : // Temperature at which OS will throttle CPU : gnvs->tpsv = 90; This only matters if other code (most often ACPI ASL code) uses these values. Otherwise, this does nothing.
You may notice that the `acpi_create_gnvs()` function is empty after applying my two suggestions. The entire file can be removed, as the `acpi_create_gnvs()` function is optional.
File src/mainboard/asus/p8h61-m_lx2/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/52478/comment/33371bd8_1997d8ee PS5, Line 2: register "gfx" = "GMA_STATIC_DISPLAYS(0)" This may be needed on laptops for LCD brightness control to work. It's unneeded on desktops.
https://review.coreboot.org/c/coreboot/+/52478/comment/c7b1bdc7_30759681 PS5, Line 3: register "gpu_cpu_backlight" = "0x00000000" : register "gpu_dp_b_hotplug" = "0" : register "gpu_dp_c_hotplug" = "0" : register "gpu_dp_d_hotplug" = "0" : register "gpu_panel_port_select" = "0" : register "gpu_panel_power_backlight_off_delay" = "0" : register "gpu_panel_power_backlight_on_delay" = "0" : register "gpu_panel_power_cycle_delay" = "0" : register "gpu_panel_power_down_delay" = "0" : register "gpu_panel_power_up_delay" = "0" : register "gpu_pch_backlight" = "0x00000000" This looks like autoport was used without collecting iGPU info. Devicetree "registers" default to zero when their value is unspecified, so this can be removed.
https://review.coreboot.org/c/coreboot/+/52478/comment/4c49627a_86877b20 PS5, Line 16: register "c1_acpower" = "1" : register "c1_battery" = "1" : register "c2_acpower" = "3" : register "c2_battery" = "3" : register "c3_acpower" = "5" : register "c3_battery" = "5" This was changed in CB:49088 and CB:49089
https://review.coreboot.org/c/coreboot/+/52478/comment/382cc327_54bdda79 PS5, Line 30: register "c2_latency" = "0x0065" This should be removed, see CB:55212
https://review.coreboot.org/c/coreboot/+/52478/comment/fb60f448_ba24ac07 PS5, Line 31: register "docking_supported" = "0" Unset devicetree settings already default to zero, so this can be removed.
https://review.coreboot.org/c/coreboot/+/52478/comment/8978f671_8c8bcbc4 PS5, Line 34: register "gen3_dec" = "0x00000000" : register "gen4_dec" = "0x00000000" : register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }" Unset devicetree settings already default to zero, so this can be removed.
https://review.coreboot.org/c/coreboot/+/52478/comment/b0600038_5334a7e3 PS5, Line 37: register "pcie_port_coalesce" = "1" util/autoport enables this by default but it's most likely not needed for your board.
https://review.coreboot.org/c/coreboot/+/52478/comment/9ff8a3fc_59449907 PS5, Line 40: register "spi_lvscc" = "0x0" : register "spi_uvscc" = "0x0" Unset devicetree settings already default to zero, so this can be removed.
https://review.coreboot.org/c/coreboot/+/52478/comment/8749495c_b7ccb30f PS5, Line 54: subsystemid 0x1043 0x844d You can inherit these subsystem IDs, see mb/asus/h61-series
https://review.coreboot.org/c/coreboot/+/52478/comment/ad54a638_2951f0c4 PS5, Line 57: subsystemid 0x1043 0x8415 Note that this subsystem ID is different from the rest. You can keep this line while still inheriting the subsystem IDs for the other devices.
https://review.coreboot.org/c/coreboot/+/52478/comment/a049e745_2094d843 PS5, Line 87: copied from LX You can dump the values programmed by vendor firmware with util/superiotool.
https://review.coreboot.org/c/coreboot/+/52478/comment/07705f82_46ae346b PS5, Line 153: Host bridge Host bridge util/autoport likes to repeat this comment, but we don't need to leave it like this.
https://review.coreboot.org/c/coreboot/+/52478/comment/7d91d3c9_0857b918 PS5, Line 153: device pci 00.0 on # Host bridge Host bridge : subsystemid 0x1043 0x844d : end : device pci 01.0 on # PEG : subsystemid 0x1043 0x844d : end : device pci 02.0 on # iGPU : subsystemid 0x1043 0x844d : end These are system agent (i.e. integrated northbridge) devices, and should logically be north of (above) the PCH (southbridge) devices. Look at mb/asus/h61-series/devicetree.cb to see what I mean.
File src/mainboard/asus/p8h61-m_lx2/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/52478/comment/a9cf595d_cef26648 PS5, Line 2: #define BRIGHTNESS_UP _SB.PCI0.GFX0.INCB : #define BRIGHTNESS_DOWN _SB.PCI0.GFX0.DECB Never used, please remove. Also remove one of the two empty lines just below.
https://review.coreboot.org/c/coreboot/+/52478/comment/e945a3d6_3346beae PS5, Line 26: #include <drivers/intel/gma/acpi/default_brightness_levels.asl> This doesn't make sense on desktop boards, there's no brightness to control. Just remove it.
File src/mainboard/asus/p8h61-m_lx2/early_init.c:
https://review.coreboot.org/c/coreboot/+/52478/comment/e457915b_ab6bf974 PS5, Line 16: { 1, 0, -1 }, : { 1, 0, -1 }, : { 1, 0, -1 }, : { 1, 0, -1 }, : { 1, 0, -1 }, : { 1, 0, -1 }, : { 1, 0, -1 }, : { 1, 0, -1 }, : { 1, 0, -1 }, : { 1, 0, -1 }, : { 1, 0, -1 }, : { 1, 0, -1 }, : { 1, 0, -1 }, : { 1, 0, -1 }, Ok, this looks awful. The values on the third column specify which overcurrent sensing pin corresponds to which USB port, and `-1` means "not mapped"...
Nothing to do here, just wanted to point this out.
https://review.coreboot.org/c/coreboot/+/52478/comment/99b9a364_92db96e9 PS5, Line 41: nuvoton_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE); You may need to program some Super I/O global registers (use util/superiotool after booting vendor firmware to dump the values)
https://review.coreboot.org/c/coreboot/+/52478/comment/55604ce1_af247558 PS5, Line 47: read_spd(&spd[0], 0x50, id_only); : read_spd(&spd[1], 0x51, id_only); : read_spd(&spd[2], 0x52, id_only); : read_spd(&spd[3], 0x53, id_only); I am certain the correct SPD map for this board is:
read_spd(&spd[0], 0x50, id_only); read_spd(&spd[2], 0x52, id_only);
File src/mainboard/asus/p8h61-m_lx2/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/52478/comment/2820e25a_7c9ea16b PS5, Line 13: Copied this from b85m-pro4 But why? Why copy this from a board made by a different vendor?
In case of doubt, it's best to specify all possible ports and find out which is which via trial and error or by reading libgfxinit logs.
File src/mainboard/asus/p8h61-m_lx2/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/52478/comment/cb7814ff_06575386 PS5, Line 21: nit(pick): Drop this empty line?
-
Angel Pons (Code Review)