Benjamin Doron has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35523 )
Change subject: mb/acer: Add Acer Aspire VN7-572G ......................................................................
Patch Set 169:
(13 comments)
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/bootblock.c:
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... PS165, Line 1: /* SPDX-License-Identifier: GPL-2.0-or-later */
Why are some GPL-2.0-or-later and others GPL-2. […]
Those I copied and adapted were preserved as GPL-2.0-only. Those I wrote were licensed permissively (or restrictively, I only know the restrictions of GPLv3 in the context of UEFI secure boot) as GPL-2.0-or-later. Regardless, I'll follow the tree in this regard.
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... PS165, Line 5: # Set backlight PWM value for eDP : register "gpu_pch_backlight_pwm_hz" = "1000" # PWM frequency : : # Enable Panel as eDP and configure power delays : register "gpu_pp_up_delay_ms" = "150" # T3 : register "gpu_pp_backlight_on_delay_ms" = "1" # T7 : register "gpu_pp_backlight_off_delay_ms" = "200" # T9 : register "gpu_pp_down_delay_ms" = "50" # T10 : register "gpu_pp_cycle_delay_ms" = "500" # T12 : : # IGD Displays : register "gfx" = "GMA_STATIC_DISPLAYS(1)"
Move down to IGD device in devicetree
Done
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... PS165, Line 35: # LPC configuration from lspci -s 1f.0 -xxx : register "lpc_iod" = "0x0010" # 80h-81h; ComB: 2F8h-2FFh (COM 2) : register "lpc_ioe" = "LPC_IOE_COMA_EN | LPC_IOE_COMB_EN | LPC_IOE_LGE_200 \ : | LPC_IOE_HGE_208 | LPC_IOE_KBC_60_64 | LPC_IOE_EC_62_66 \ : | LPC_IOE_SUPERIO_2E_2F | LPC_IOE_EC_4E_4F" # 82h-83h : register "gen1_dec" = "0x000c0081" # 84h-87h; Debug: Port 80h : register "gen3_dec" = "0x00040069" # 8Ch-8Fh; Legacy: Port 68h/6Ch : register "gen4_dec" = "0x000c1201" # 90h-93h; Index: Port 1200h
Move into LPC device
Done
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... PS165, Line 48: register "SataSalpSupport" = "1" : register "SataPortsEnable[1]" = "1" # HDD; BIT1 in SATA 92h-93h : register "SataPortsEnable[2]" = "1" # ODD; BIT2 in SATA 92h-93h
Move into SATA device
Done
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... PS165, Line 52: register "PrimaryDisplay" = "Display_Switchable"
Move into IGD device
Done
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... PS165, Line 59: # EC/KBC requires continuous mode : register "serirq_mode" = "SERIRQ_CONTINUOUS"
Move into LPC into
Done
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... PS165, Line 62: # VR Slew rate setting for improving audible noise : register "AcousticNoiseMitigation" = "1" : register "SlowSlewRateForIa" = "3" # Fast/16 : register "SlowSlewRateForGt" = "3" # Fast/16 : register "SlowSlewRateForSa" = "0" # Fast/2 : register "FastPkgCRampDisableIa" = "0" : register "FastPkgCRampDisableGt" = "0" : register "FastPkgCRampDisableSa" = "0" : : # Enable Root Port 1 (x4) for dGPU : register "PcieRpEnable[0]" = "1" : register "PcieRpAdvancedErrorReporting[0]" = "1" : register "PcieRpLtrEnable[0]" = "1" : register "PcieRpClkReqSupport[0]" = "1" : register "PcieRpClkReqNumber[0]" = "0" : register "PcieRpMaxPayload[0]" = "RpMaxPayload_256" : : # Enable Root Port 7 (x2) for NGFF : register "PcieRpEnable[6]" = "1" : register "PcieRpAdvancedErrorReporting[6]" = "1" : register "PcieRpLtrEnable[6]" = "1" : register "PcieRpClkReqSupport[6]" = "1" : register "PcieRpClkReqNumber[6]" = "3" : : # Enable Root Port 9 for LAN : register "PcieRpEnable[8]" = "1" : register "PcieRpAdvancedErrorReporting[8]" = "1" : register "PcieRpLtrEnable[8]" = "1" : register "PcieRpClkReqSupport[8]" = "1" : register "PcieRpClkReqNumber[8]" = "1" : : # Enable Root Port 10 for WLAN : register "PcieRpEnable[9]" = "1" : register "PcieRpAdvancedErrorReporting[9]" = "1" : register "PcieRpLtrEnable[9]" = "1" : register "PcieRpClkReqSupport[9]" = "1" : register "PcieRpClkReqNumber[9]" = "2" : register "PcieRpMaxPayload[9]" = "RpMaxPayload_256" : register "pcie_rp_aspm[9]" = "AspmL1" : register "pcie_rp_l1substates[9]" = "L1SS_Disabled"
Move these into their RP
Done
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... PS165, Line 103: register "usb2_ports[0]" = "{ : .enable = 1, : .ocpin = OC_SKIP, : .tx_bias = USB2_BIAS_17MV, : .tx_emp_enable = USB2_DE_EMP_ON, : .pre_emp_bias = USB2_BIAS_28MV, : .pre_emp_bit = USB2_HALF_BIT_PRE_EMP, : }" # Type-A Port (right) : register "usb2_ports[1]" = "{ : .enable = 1, : .ocpin = OC_SKIP, : .tx_bias = USB2_BIAS_17MV, : .tx_emp_enable = USB2_DE_EMP_ON, : .pre_emp_bias = USB2_BIAS_28MV, : .pre_emp_bit = USB2_HALF_BIT_PRE_EMP, : }" # Type-A Port (right) : register "usb2_ports[2]" = "{ : .enable = 1, : .ocpin = OC_SKIP, : .tx_bias = USB2_BIAS_17MV, : .tx_emp_enable = USB2_DE_EMP_ON, : .pre_emp_bias = USB2_BIAS_28MV, : .pre_emp_bit = USB2_HALF_BIT_PRE_EMP, : }" # Type-C Port : register "usb2_ports[3]" = "USB2_PORT_FLEX(OC_SKIP)" # Type-A Port (left) : register "usb2_ports[4]" = "USB2_PORT_FLEX(OC_SKIP)" # Bluetooth : register "usb2_ports[5]" = "USB2_PORT_FLEX(OC_SKIP)" # Touchscreen : register "usb2_ports[6]" = "USB2_PORT_FLEX(OC_SKIP)" # Webcam : register "usb2_ports[7]" = "USB2_PORT_FLEX(OC_SKIP)" # SD : register "usb2_ports[8]" = "USB2_PORT_FLEX(OC_SKIP)" # Finger-printer : : register "usb3_ports[0]" = "USB3_PORT_DEFAULT(OC_SKIP)" # Type-A Port (right); Capable of OTG : register "usb3_ports[1]" = "USB3_PORT_DEFAULT(OC_SKIP)" # Type-A Port (right) : register "usb3_ports[2]" = "USB3_PORT_DEFAULT(OC_SKIP)" # Type-C Port : register "usb3_ports[3]" = "USB3_PORT_DEFAULT(OC_SKIP)" # Type-C Port
Move into USB device
Done
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... PS165, Line 185: device domain 0 on
Maybe remove disabled devices?
The original idea was to allow boards to remove some SoC devices from being exposed to user configuration through something along the lines of `dev && (dev->enabled || CONFIG(dev_ENABLE))`. The chipset devicetree makes this more difficult.
In any event, this was more about which devices were removed: 01.x: PEGx 05.0: SA IMGU 13.0: ISH Various SerialIO devices and PCIe ports 16.1-16.4: AMT MEI 1e.4-1e.6: SCS 1f.6: GbE, only for AMT?
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... PS165, Line 17: Scope (_SB) : { : Device (PCI0)
Simplify to Device (_SB. […]
Done
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/gpio.h:
PS165:
Convert this to a C file (or split it up into gpio_early.c and gpio.c). See clevo/cml-u for example.
What's the objective?
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... File src/mainboard/acer/aspire_vn7_572g/mainboard.c:
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... PS165, Line 53: mainboard_enable
Rename to enable_mainboard
As mentioned on CB:43852, I'll wait for a commit that adapts the rest of the tree, to keep consistency.
https://review.coreboot.org/c/coreboot/+/35523/165/src/mainboard/acer/aspire... PS165, Line 54: mainboard_init
Rename to init_mainboard and put it before . […]
As above.