Attention is currently required from: Angel Pons, Felix Singer.
Federico Amedeo Izzo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/82010?usp=email )
Change subject: mb/aoostar: Add AOOSTAR R1 (WTR_R1) ......................................................................
Patch Set 11:
(31 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/82010/comment/d8da15a6_31b67c93 : PS8, Line 47: VDT
VBT?
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/2d100263_c87d5e64 : PS8, Line 49: vgabios blob (ID 8086,0406)
Shouldn't be needed if you use FSP GOP for video init. […]
Done
Commit Message:
https://review.coreboot.org/c/coreboot/+/82010/comment/7f4fa2eb_10eb92db : PS10, Line 57: Patchset 5: Re-enabled dptf, added default options to Kconfig. : Patchset 7: Configured USB port mapping and overcurrent, USB3.0 works : Pa
Doesn't hurt, and it's common practice in mailing lists commits.
Acknowledged
Patchset:
PS11: Thank you for your reviews 😊
File src/mainboard/aoostar/wtr_r1/Kconfig:
https://review.coreboot.org/c/coreboot/+/82010/comment/88435139_a7c555a3 : PS8, Line 28: config MAINBOARD_VENDOR : string : default "AOOSTAR"
Already set in vendor Kconfig, please drop
Done
File src/mainboard/aoostar/wtr_r1/bootblock.c:
https://review.coreboot.org/c/coreboot/+/82010/comment/461d1085_43ba42fe : PS2, Line 1:
Remove blank line
Done
File src/mainboard/aoostar/wtr_r1/bootblock.c:
https://review.coreboot.org/c/coreboot/+/82010/comment/1c7525dc_965fa615 : PS10, Line 20: ite_set_3vsbsw(GPIO_DEV, true);
Where do the Super I/O settings come from? […]
These Super I/O settings are taken from a superiotool dump while running the vendor UEFI firmware. I tried to document them with the datasheet of a similar SIO chip, as I couldn't find the exact datasheet. The Super I/O functions are working, except for the power button from off.
https://review.coreboot.org/c/coreboot/+/82010/comment/6182c868_f83a2f3e : PS10, Line 21: ite_delay_pwrgd3(GPIO_DEV);
There's very few boards that use this, is it actually needed? Hard to say without testing S3, though […]
The equivalent Super I/O register was set from the vendor UEFI firmware (according to superiotool dump), so I replaced the reg_write with set_3vsbsw.
File src/mainboard/aoostar/wtr_r1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/82010/comment/83f3e638_6f284fe7 : PS2, Line 16: register "usb2_ports[0]" = "USB2_PORT_MID(OC3)" # Type-C Port1 : register "usb2_ports[1]" = "USB2_PORT_MID(OC3)" # Type-C Port2 : register "usb2_ports[2]" = "USB2_PORT_MID(OC3)" # FPS connector : register "usb2_ports[3]" = "USB2_PORT_MID(OC_SKIP)" # M.2 WWAN : register "usb2_ports[4]" = "USB2_PORT_MID(OC3)" # USB3/2 Type A port1 : register "usb2_ports[5]" = "USB2_PORT_MID(OC1)" # USB3/2 Type A port2 : register "usb2_ports[6]" = "USB2_PORT_MID(OC1)" # USB3/2 Type A port3 : register "usb2_ports[7]" = "USB2_PORT_MID(OC_SKIP)" # Type A/ M.2 WLAN : register "usb2_ports[9]" = "USB2_PORT_MID(OC_SKIP)" # Bluetooth : : register "usb3_ports[0]" = "USB3_PORT_DEFAULT(OC3)" # USB3/2 Type A port1 : register "usb3_ports[1]" = "USB3_PORT_DEFAULT(OC1)" # USB3/2 Type A port2 : register "usb3_ports[2]" = "USB3_PORT_DEFAULT(OC1)" # USB3/2 Type A port3 : r
Please move into the devicetree below.
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/7b07ddd0_5718b16c : PS2, Line 81: device ref ipu off end
Already disabled in chipset devicetree, remove.
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/8af97df2_016de20c : PS2, Line 117: device ref heci1 on end
Enabled in chipset devicetree, remove.
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/c7a34896_84d17b8d : PS2, Line 230: device ref p2sb on end
P2SB is hidden by the FSP, which is configured accordingly in chipset devicetree. Remove.
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/95a4bb4c_11c08afa : PS2, Line 231: device ref emmc off end
Already disabled in chipset devicetree, remove.
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/754624d3_eca409ee : PS2, Line 233: device ref ufs off end
Already disabled in chipset devicetree, remove.
Done
File src/mainboard/aoostar/wtr_r1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/82010/comment/54e203a9_462c25b9 : PS8, Line 194: : [PchSerialIoIndexUART1] = PchSerialIoDisabled, : [PchSerialIoIndexUART2] = PchSerialIoDisabled,
Disabled equals zero. […]
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/24ce101e_4bc1e9df : PS8, Line 201: : [PchSerialIoIndexGSPI1] = PchSerialIoDisabled, : [PchSerialIoIndexGSPI2] = PchSerialIoDisabled, : [PchSerialIoIndexGSPI3] = PchSerialIoDisabled,
Disabled equals zero. […]
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/a426c71a_4df4b975 : PS8, Line 205: : register "serial_io_gspi_cs_mode" = "{ : [PchSerialIoIndexGSPI0] = 0, : [PchSerialIoIndexGSPI1] = 0, : [PchSerialIoIndexGSPI2] = 0, : [PchSerialIoIndexGSPI3] = 0, : }" : register "serial_io_gspi_cs_state" = "{ : [PchSerialIoIndexGSPI0] = 0, : [PchSerialIoIndexGSPI1] = 0, : [PchSerialIoIndexGSPI2] = 0, : [PchSerialIoIndexGSPI3] = 0, : }"
Devicetree "register"s are default-initialised to zero, so this can be dropped.
Done
File src/mainboard/aoostar/wtr_r1/devicetree.cb:
PS9:
Missing SPDX license identifier.
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/67d279db_028e7421 : PS9, Line 11: # FSP configuration
Remove superfluous comment
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/4934dae6_a5b55263 : PS9, Line 13: # Sagv Configuration
Same
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/5aa183e5_83fc6ce8 : PS9, Line 16: # Enable DPTF
Same
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/cdaef54f_d6d7c1bc : PS9, Line 21: # Intel Common SoC Config
Same
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/f06f2534_be683027 : PS9, Line 74: register "usb2_ports[6]" = "USB2_PORT_EMPTY"
Not needed, remove.
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/332cc47f_d91012b6 : PS9, Line 79: register "usb3_ports[2]" = "USB3_PORT_EMPTY" : register "usb3_ports[3]" = "USB3_PORT_EMPTY" : register "usb3_ports[4]" = "USB3_PORT_EMPTY"
Not needed, remove.
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/62deb40b_df08426c : PS9, Line 85: register "tcss_ports[0]" = "TCSS_PORT_DEFAULT(OC3)" # USB3/2 Type A upper
Move to tcss_xhci PCI device
Done
File src/mainboard/aoostar/wtr_r1/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/82010/comment/f68a1328_480fb5b8 : PS10, Line 67: register "usb2_ports[0]" = "USB2_PORT_TYPE_C(OC0)" # Type-C : register "usb2_ports[1]" = "USB2_PORT_MID(OC_SKIP)" # microSD card reader : register "usb2_ports[2]" = "USB2_PORT_MID(OC3)" # USB2 Type A upper : register "usb2_ports[3]" = "USB2_PORT_MID(OC3)" # USB2 Type A lower : register "usb2_ports[4]" = "USB2_PORT_MID(OC3)" # USB3/2 Type A upper : register "usb2_ports[5]" = "USB2_PORT_MID(OC3)" # USB3/2 Type A lower : register "usb2_ports[7]" = "USB2_PORT_MID(OC_SKIP)" # M.2 WLAN : : register "usb3_ports[0]" = "USB3_PORT_DEFAULT(OC0)" # Type-C : register "usb3_ports[1]" = "USB3_PORT_DEFAULT(OC_SKIP)" # microSD card reader : register "usb3_ports[5]" = "USB3_PORT_DEFAULT(OC3)" # USB3/2 Type A lower :
Rewrite as: […]
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/9b052812_7e451203 : PS10, Line 117: .clk_req = 2,
How were CLKSRC and CLKREQ values determined? These could be causing troubles with power management.
they are educated guesses, unfortunately I don't have the schematics of this board. However if I remove these, the M.2 SSD and NICs stop working.
https://review.coreboot.org/c/coreboot/+/82010/comment/fde8e9e4_44f80653 : PS10, Line 114: device ref pcie_rp3 on : register "pch_pcie_rp[PCH_RP(3)]" = "{ : .clk_src = 2, : .clk_req = 2, : .flags = PCIE_RP_CLK_REQ_DETECT, : }" : end : device ref pcie_rp7 on : register "pch_pcie_rp[PCH_RP(7)]" = "{ : .clk_src = 3, : .clk_req = 3, : .flags = PCIE_RP_CLK_REQ_DETECT, : }" : end : device ref pcie_rp9 on : register "pch_pcie_rp[PCH_RP(9)]" = "{ : .clk_src = 0, : .clk_req = 0, : .flags = PCIE_RP_CLK_REQ_DETECT, : }" : end : device ref pcie_rp10 on : register "pch_pcie_rp[PCH_RP(10)]" = "{ : .clk_src = 1, : .clk_req = 1, : .flags = PCIE_RP_CLK_REQ_DETECT, : }" : end
For accessible ports, add SMBIOS information with `smbios_slot_desc`. […]
Done
File src/mainboard/aoostar/wtr_r1/gpio.h:
https://review.coreboot.org/c/coreboot/+/82010/comment/e8b2887c_a81b27fd : PS10, Line 207: _PAD_CFG_STRUCT(GPD3, PAD_FUNC(NF1) | PAD_TRIG(OFF) | PAD_BUF(TX_RX_DISABLE) | (1 << 1), 0), /* PWRBTN# */
Try this instead: `PAD_CFG_NF(GPD3, UP_20K, DEEP, NF1)` […]
Unfortunately none of these make the power button work. I left PWROK as it's equivalent to the configuration from intelp2m. I'm guessing a Super I/O issue, as in the schematics of the similar Odroid H4, the power button is handled by the Super I/O https://wiki.odroid.com/odroid-h4/start#odroid-h4_schematic_and_full_intel_n...
File src/mainboard/aoostar/wtr_r1/gpio.h:
PS2:
Remove comments to GPIOs which are configured with PAD_NC.
Done
https://review.coreboot.org/c/coreboot/+/82010/comment/49b0ddd2_2b71c650 : PS2, Line 222: /* ------- GPIO Group PCIe vGPIO ------- */
Remove the superfluous comments below.
Done
-
Federico Amedeo Izzo (Code Review)